* Halil Pasic <pa...@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 <pa...@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