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]:

> 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.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
> 
> AFAIR we decided in the previous round to rather do transformation
> and fixing in one patch than touch stuff twice. Hence this patch
> ain't pure transformation any more.
> ---
>  hw/s390x/css.c              | 83 +++++++++++++--------------------------------
>  hw/s390x/s390-ccw.c         | 11 +++---
>  hw/vfio/ccw.c               | 30 ++++++++++++----
>  include/hw/s390x/css.h      | 24 +++++++++----
>  include/hw/s390x/s390-ccw.h |  2 +-
>  target/s390x/ioinst.c       | 53 ++++-------------------------
>  6 files changed, 77 insertions(+), 126 deletions(-)
> 
> 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);
> +        return (IOInstEnding){.cc = 0};
This behavior change is not mentioned in the commit message. Right?

[...]

> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index a8baadf57a..dbb5b201de 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -23,6 +23,7 @@
>  #include "hw/s390x/s390-ccw.h"
>  #include "hw/s390x/ccw-device.h"
>  #include "qemu/error-report.h"
> +#include "cpu.h"
We need this because?

> 
>  #define TYPE_VFIO_CCW "vfio-ccw"
>  typedef struct VFIOCCWDevice {
> @@ -47,9 +48,9 @@ struct VFIODeviceOps vfio_ccw_ops = {
>      .vfio_compute_needs_reset = vfio_ccw_compute_needs_reset,
>  };
> 
> -static int vfio_ccw_handle_request(ORB *orb, SCSW *scsw, void *data)
> +static IOInstEnding vfio_ccw_handle_request(SubchDev *sch)
>  {
> -    S390CCWDevice *cdev = data;
> +    S390CCWDevice *cdev = sch->driver_data;
>      VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev);
>      struct ccw_io_region *region = vcdev->io_region;
>      int ret;
> @@ -60,8 +61,8 @@ static int vfio_ccw_handle_request(ORB *orb, SCSW *scsw, void *data)
> 
>      memset(region, 0, sizeof(*region));
> 
> -    memcpy(region->orb_area, orb, sizeof(ORB));
> -    memcpy(region->scsw_area, scsw, sizeof(SCSW));
> +    memcpy(region->orb_area, &sch->orb, sizeof(ORB));
> +    memcpy(region->scsw_area, &sch->curr_status.scsw, sizeof(SCSW));
> 
>  again:
>      ret = pwrite(vcdev->vdev.fd, region,
> @@ -71,10 +72,25 @@ again:
>              goto again;
>          }
>          error_report("vfio-ccw: wirte I/O region failed with errno=%d", errno);
> -        return -errno;
> +        ret = -errno;
> +    } else {
> +        ret = region->ret_code;
> +    }
> +    switch (-ret) {
> +    /* Currently we don't update control block and just return the cc code. */
This is not true anymore? At least for the EFAULT case.

> +    case 0:
> +        return (IOInstEnding){.cc = 0};
> +    case EBUSY:
> +        return (IOInstEnding){.cc = 2};
> +    case ENODEV:
> +    case EACCES:
> +        return (IOInstEnding){.cc = 3};
> +    case EFAULT:
> +    default:
> +        sch_gen_unit_exception(sch);
> +        css_inject_io_interrupt(sch);
> +        return (IOInstEnding){.cc = 0};
>      }
> -
> -    return region->ret_code;
>  }
> 
>  static void vfio_ccw_reset(DeviceState *dev)
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 66916b6546..2116c6b3c7 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
[...]

> @@ -197,13 +208,14 @@ SubchDev *css_find_subch(uint8_t m, uint8_t cssid, uint8_t ssid,
>                           uint16_t schid);
>  bool css_subch_visible(SubchDev *sch);
>  void css_conditional_io_interrupt(SubchDev *sch);
> +
Extra change.

>  int css_do_stsch(SubchDev *sch, SCHIB *schib);
>  bool css_schid_final(int m, uint8_t cssid, uint8_t ssid, uint16_t schid);
>  int css_do_msch(SubchDev *sch, const SCHIB *schib);
[...]

-- 
Dong Jia Shi