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

[Thomas Huth]

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?
Anyway, as Cornelia already mentioned it: Please drop the trailing dots.

> +        sch_gen_unit_exception(sch);
> +        css_inject_io_interrupt(sch);
> +        return IOINST_CC_EXPECTED;
>      }
[...]
> @@ -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?

>      }
[...]
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 76323c6bde..1cc2e5d873 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -47,9 +47,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 +60,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 +71,24 @@ 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) {
> +    case 0:
> +        return IOINST_CC_EXPECTED;
> +    case EBUSY:
> +        return IOINST_CC_BUSY;
> +    case ENODEV:
> +    case EACCES:
> +        return IOINST_CC_NOT_OPERATIONAL;
> +    case EFAULT:
> +    default:
> +        sch_gen_unit_exception(sch);
> +        css_inject_io_interrupt(sch);
> +        return IOINST_CC_EXPECTED;

Do we feel really confident that it is OK to do the setcc() in case of
an exception here later? ... otherwise it might be necessery to
introduce something like IOINST_EXCEPTION to the enum to signal the
ioinst_handle_xxx() callers that they should not do the setcc() anymore...

 Thomas