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

[Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>]

* 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? ;)

> +        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.

Anybody agrees that also adding an assert() here?

[...]

> 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.

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

[...]

-- 
Dong Jia Shi