Re: [Qemu-devel] [PATCH v3 3/7] s390x: improve error handling for SSCH and RSCH

[Halil Pasic]

On 10/18/2017 12:07 PM, Thomas Huth wrote:
> On 18.10.2017 11:52, Cornelia Huck wrote:
>> On Wed, 18 Oct 2017 11:30:47 +0200
>> Thomas Huth <address@hidden> wrote:
>>
>>> On 17.10.2017 16:04, Halil Pasic wrote:
>>>> Simplify the error handling of the SSCH and RSCH handler avoiding
>>>> arbitrary and cryptic error codes being used to tell how the instruction
>>>> is supposed to end.  Let the code detecting the condition tell how it's
>>>> to be handled in a less ambiguous way.  It's best to handle SSCH and RSCH
>>>> in one go as the emulation of the two shares a lot of code.
>>>>
>>>> For passthrough this change isn't pure refactoring, but changes the way
>>>> kernel reported EFAULT is handled. After clarifying the kernel interface
>>>> we decided that EFAULT shall be mapped to unit exception.  Same goes for
>>>> unexpected error codes and absence of required ORB flags.
>>>>
>>>> Signed-off-by: Halil Pasic <address@hidden>
>>>> ---
>>>>  hw/s390x/css.c              | 84 
>>>> +++++++++++++--------------------------------
>>>>  hw/s390x/s390-ccw.c         | 11 +++---
>>>>  hw/vfio/ccw.c               | 28 +++++++++++----
>>>>  include/hw/s390x/css.h      | 23 +++++++++----
>>>>  include/hw/s390x/s390-ccw.h |  2 +-
>>>>  target/s390x/ioinst.c       | 53 ++++------------------------
>>>>  6 files changed, 75 insertions(+), 126 deletions(-)
>>>>
>>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>>>> index aa233d5f8a..ff5a05c34b 100644
>>>> --- a/hw/s390x/css.c
>>>> +++ b/hw/s390x/css.c
>>>> @@ -1181,12 +1181,11 @@ static void sch_handle_start_func_virtual(SubchDev 
>>>> *sch)
>>>>  
>>>>  }
>>>>  
>>>> -static int sch_handle_start_func_passthrough(SubchDev *sch)
>>>> +static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch)
>>>>  {
>>>>  
>>>>      PMCW *p = &sch->curr_status.pmcw;
>>>>      SCSW *s = &sch->curr_status.scsw;
>>>> -    int ret;
>>>>  
>>>>      ORB *orb = &sch->orb;
>>>>      if (!(s->ctrl & SCSW_ACTL_SUSP)) {
>>>> @@ -1200,31 +1199,12 @@ static int 
>>>> sch_handle_start_func_passthrough(SubchDev *sch)
>>>>       */
>>>>      if (!(orb->ctrl0 & ORB_CTRL0_MASK_PFCH) ||
>>>>          !(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
>>>> -        return -EINVAL;
>>>> +        warn_report("vfio-ccw requires PFCH and C64 flags set...");  
>>>
>>> Not sure, but should this maybe rather be a
>>> "qemu_log_mask(LOG_GUEST_ERROR, ...)" instead?
>>
>> Is that visible by default, though? I'd rather want the admin to be
>> able to find a hint in a log somewhere why the guest I/O is rejected.
> 
> Well, the guest could also trigger this condition on purpose (e.g.
> kvm-unit-tests), so I wonder whether we want to see the warning in that
> case, too...
> IMHO this is exactly what qemu_log_mask(LOG_GUEST_ERROR, ...) has been
> implemented for: Log errors from the guest in case you suspect that the
> guest is doing something wrong. But that's just my 0.02 €, feel free to
> ignore me.
> 
>>>> @@ -1844,27 +1816,23 @@ void css_do_schm(uint8_t mbk, int update, int dct, 
>>>> uint64_t mbo)
>>>>      }
>>>>  }
>>>>  
>>>> -int css_do_rsch(SubchDev *sch)
>>>> +IOInstEnding css_do_rsch(SubchDev *sch)
>>>>  {
>>>>      SCSW *s = &sch->curr_status.scsw;
>>>>      PMCW *p = &sch->curr_status.pmcw;
>>>> -    int ret;
>>>>  
>>>>      if (~(p->flags) & (PMCW_FLAGS_MASK_DNV | PMCW_FLAGS_MASK_ENA)) {
>>>> -        ret = -ENODEV;
>>>> -        goto out;
>>>> +        return IOINST_CC_NOT_OPERATIONAL;
>>>>      }
>>>>  
>>>>      if (s->ctrl & SCSW_STCTL_STATUS_PEND) {
>>>> -        ret = -EINPROGRESS;
>>>> -        goto out;
>>>> +        return IOINST_CC_STATUS_PRESENT;
>>>>      }
>>>>  
>>>>      if (((s->ctrl & SCSW_CTRL_MASK_FCTL) != SCSW_FCTL_START_FUNC) ||
>>>>          (s->ctrl & SCSW_ACTL_RESUME_PEND) ||
>>>>          (!(s->ctrl & SCSW_ACTL_SUSP))) {
>>>> -        ret = -EINVAL;
>>>> -        goto out;
>>>> +        return IOINST_CC_BUSY;  
>>>
>>> Why is EINVAL now mapped to IOINST_CC_BUSY? Shouldn't that be
>>> IOINST_CC_STATUS_PRESENT instead?
>>
>> No, that is correct (see the PoP for when cc 2 is supposed to be set by
>> rsch).
> 
> So if this is on purpose, this change in behavior should also be
> mentioned in the patch description, I think.
> 

No. have a look at the function ioinst_handle_rsch. It used
to map -EINVAL to cc 2 (and was an oddball in this respect) so we
keep the old behavior, it's just more obvious whats happening.

Regards,
Halil

>  Thomas
>