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

[Halil Pasic <pasic@linux.vnet.ibm.com>]

On 10/10/2017 10:13 AM, Dong Jia Shi wrote:
> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-10-04 17:41:39 +0200]:
> 
> [...]
> 
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index 4f47dbc8b0..b2978c3bae 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -1003,12 +1003,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)) {
>> @@ -1022,31 +1021,11 @@ static int sch_handle_start_func_passthrough(SubchDev *sch)
>>       */
>>      if (!(orb->ctrl0 & ORB_CTRL0_MASK_PFCH) ||
>>          !(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
>> -        return -EINVAL;
>> +        sch_gen_unit_exception(sch);
>> +        css_inject_io_interrupt(sch);
> Last cycle, we agreed to add some log here. Sth. like:
> warn_report("vfio-ccw requires PFCH and C64 flags set...");
> 
> I promised to do a fix for this piece of code. But since this patch
> already fixed it, I guess what I have to do is to add the log only? Or
> you would like to add it by yourself? ;)
> 

I think I forgot this one. Should there be a v3 I could add this too.
Otherwise I would not mind if you do it on top.

>> +        return (IOInstEnding){.cc = 0};
>>      }
>> -
>> -    ret = s390_ccw_cmd_request(orb, s, sch->driver_data);
>> -    switch (ret) {
>> -    /* Currently we don't update control block and just return the cc code. */
>> -    case 0:
>> -        break;
>> -    case -EBUSY:
>> -        break;
>> -    case -ENODEV:
>> -        break;
>> -    case -EACCES:
>> -        /* Let's reflect an inaccessible host device by cc 3. */
>> -        ret = -ENODEV;
>> -        break;
>> -    default:
>> -       /*
>> -        * All other return codes will trigger a program check,
>> -        * or set cc to 1.
>> -        */
>> -       break;
>> -    };
>> -
>> -    return ret;
>> +    return s390_ccw_cmd_request(sch);
>>  }
>>
>>  /*
> [...]
> 
>> @@ -1084,16 +1063,15 @@ int do_subchannel_work_passthrough(SubchDev *sch)
>>          /* TODO: Halt handling */
>>          sch_handle_halt_func(sch);
>>      } else if (s->ctrl & SCSW_FCTL_START_FUNC) {
>> -        ret = sch_handle_start_func_passthrough(sch);
>> +        return sch_handle_start_func_passthrough(sch);
>>      }
>> -
>> -    return ret;
>> +    return (IOInstEnding){.cc = 0};
>>  }
>>
>> -static int do_subchannel_work(SubchDev *sch)
>> +static IOInstEnding do_subchannel_work(SubchDev *sch)
>>  {
>>      if (!sch->do_subchannel_work) {
>> -        return -EINVAL;
>> +        return (IOInstEnding){.cc = 1};
> This keeps the logic here as-is, so it is right.
> 

Yep.

> Anybody agrees that also adding an assert() here?

With automated regression testing in place I'm for it, without
I feel uncomfortable doing it myself. You could do this
on top if you like.

> 
> [...]
> 
>> diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
>> index 8614dda6f8..5d2c305b71 100644
>> --- a/hw/s390x/s390-ccw.c
>> +++ b/hw/s390x/s390-ccw.c
>> @@ -18,15 +18,14 @@
>>  #include "hw/s390x/css-bridge.h"
>>  #include "hw/s390x/s390-ccw.h"
>>
>> -int s390_ccw_cmd_request(ORB *orb, SCSW *scsw, void *data)
>> +IOInstEnding s390_ccw_cmd_request(SubchDev *sch)
>>  {
>> -    S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(data);
>> +    S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(sch->driver_data);
>>
>> -    if (cdc->handle_request) {
>> -        return cdc->handle_request(orb, scsw, data);
>> -    } else {
>> -        return -ENOSYS;
>> +    if (!cdc->handle_request) {
>> +        return (IOInstEnding){.cc = 1};
> Same consideration as the last comment.

Same here.

> 
>>      }
>> +    return cdc->handle_request(sch);
>>  }
>>
>>  static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
> 
> [...]
> 

Except for the missing warning are you OK with the rest
of the patch? I would like to re-claim your r-b (dropped
because changes weren't just minor).

Halil