On Wed, 18 Oct 2017 11:30:47 +0200 Thomas Huth <th...@redhat.com> wrote:
> 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 <pa...@linux.vnet.ibm.com> > > --- > > 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? Is that visible by default, though? I'd rather want the admin to be able to find a hint in a log somewhere why the guest I/O is rejected. > 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? No, that is correct (see the PoP for when cc 2 is supposed to be set by rsch). > > > } > [...] > > 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... I think Halil's comments in patch 2 already hint at possibly needing to add IOINST_EXCEPTION later.