| Message ID | 20171017140453.51099-4-pasic@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | improve error handling for IO instr | expand |
On Tue, 17 Oct 2017 16:04:49 +0200 Halil Pasic <pasic@linux.vnet.ibm.com> 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 <pasic@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..."); Drop the trailing dots? > + sch_gen_unit_exception(sch); > + css_inject_io_interrupt(sch); > + return IOINST_CC_EXPECTED; > } > - > - 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); > } > > /* > @@ -1233,7 +1213,7 @@ static int sch_handle_start_func_passthrough(SubchDev *sch) > * read/writes) asynchronous later on if we start supporting more than > * our current very simple devices. > */ > -int do_subchannel_work_virtual(SubchDev *sch) > +IOInstEnding do_subchannel_work_virtual(SubchDev *sch) > { > > SCSW *s = &sch->curr_status.scsw; > @@ -1247,12 +1227,12 @@ int do_subchannel_work_virtual(SubchDev *sch) > sch_handle_start_func_virtual(sch); > } > css_inject_io_interrupt(sch); > - return 0; > + /* inst must succeed if this func is called */ > + return IOINST_CC_EXPECTED; > } > > -int do_subchannel_work_passthrough(SubchDev *sch) > +IOInstEnding do_subchannel_work_passthrough(SubchDev *sch) > { > - int ret = 0; > SCSW *s = &sch->curr_status.scsw; > > if (s->ctrl & SCSW_FCTL_CLEAR_FUNC) { > @@ -1262,16 +1242,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 IOINST_CC_EXPECTED; > } > > -static int do_subchannel_work(SubchDev *sch) > +static IOInstEnding do_subchannel_work(SubchDev *sch) > { > if (!sch->do_subchannel_work) { > - return -EINVAL; > + return IOINST_CC_STATUS_PRESENT; > } > g_assert(sch->curr_status.scsw.ctrl & SCSW_CTRL_MASK_FCTL); > return sch->do_subchannel_work(sch); > @@ -1561,27 +1540,23 @@ static void css_update_chnmon(SubchDev *sch) > } > } > > -int css_do_ssch(SubchDev *sch, ORB *orb) > +IOInstEnding css_do_ssch(SubchDev *sch, ORB *orb) > { > 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_FCTL_START_FUNC | > SCSW_FCTL_HALT_FUNC | > SCSW_FCTL_CLEAR_FUNC)) { > - ret = -EBUSY; > - goto out; > + return IOINST_CC_BUSY; > } > > /* If monitoring is active, update counter. */ > @@ -1594,10 +1569,7 @@ int css_do_ssch(SubchDev *sch, ORB *orb) > s->ctrl |= (SCSW_FCTL_START_FUNC | SCSW_ACTL_START_PEND); > s->flags &= ~SCSW_FLAGS_MASK_PNO; > > - ret = do_subchannel_work(sch); > - > -out: > - return ret; > + return do_subchannel_work(sch); > } > > static void copy_irb_to_guest(IRB *dest, const IRB *src, PMCW *pmcw, > @@ -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; > } > > /* If monitoring is active, update counter. */ > @@ -1873,11 +1841,7 @@ int css_do_rsch(SubchDev *sch) > } > > s->ctrl |= SCSW_ACTL_RESUME_PEND; > - do_subchannel_work(sch); > - ret = 0; > - > -out: > - return ret; > + return do_subchannel_work(sch); > } > > int css_do_rchp(uint8_t cssid, uint8_t chpid) > diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c > index 8614dda6f8..0ef232ec27 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 IOINST_CC_STATUS_PRESENT; > } > + return cdc->handle_request(sch); > } > > static void s390_ccw_get_dev_info(S390CCWDevice *cdev, > 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; > } Checking ret for the negative error values is more idiomatic. > - > - 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 7e0dbd162f..10bde18452 100644 > --- a/include/hw/s390x/css.h > +++ b/include/hw/s390x/css.h > @@ -136,11 +136,22 @@ struct SubchDev { > /* transport-provided data: */ > int (*ccw_cb) (SubchDev *, CCW1); > void (*disable_cb)(SubchDev *); > - int (*do_subchannel_work) (SubchDev *); > + IOInstEnding (*do_subchannel_work) (SubchDev *); > SenseId id; > void *driver_data; > }; > > +static inline void sch_gen_unit_exception(SubchDev *sch) > +{ > + sch->curr_status.scsw.ctrl &= ~SCSW_ACTL_START_PEND; > + sch->curr_status.scsw.ctrl |= SCSW_STCTL_PRIMARY | > + SCSW_STCTL_SECONDARY | > + SCSW_STCTL_ALERT | > + SCSW_STCTL_STATUS_PEND; > + sch->curr_status.scsw.cpa = sch->channel_prog + 8; > + sch->curr_status.scsw.dstat = SCSW_DSTAT_UNIT_EXCEP; > +} > + > extern const VMStateDescription vmstate_subch_dev; > > /* > @@ -199,9 +210,9 @@ void css_generate_sch_crws(uint8_t cssid, uint8_t ssid, uint16_t schid, > void css_generate_chp_crws(uint8_t cssid, uint8_t chpid); > void css_generate_css_crws(uint8_t cssid); > void css_clear_sei_pending(void); > -int s390_ccw_cmd_request(ORB *orb, SCSW *scsw, void *data); > -int do_subchannel_work_virtual(SubchDev *sub); > -int do_subchannel_work_passthrough(SubchDev *sub); > +IOInstEnding s390_ccw_cmd_request(SubchDev *sch); > +IOInstEnding do_subchannel_work_virtual(SubchDev *sub); > +IOInstEnding do_subchannel_work_passthrough(SubchDev *sub); > > typedef enum { > CSS_IO_ADAPTER_VIRTIO = 0, > @@ -232,7 +243,7 @@ int css_do_msch(SubchDev *sch, const SCHIB *schib); > int css_do_xsch(SubchDev *sch); > int css_do_csch(SubchDev *sch); > int css_do_hsch(SubchDev *sch); > -int css_do_ssch(SubchDev *sch, ORB *orb); > +IOInstEnding css_do_ssch(SubchDev *sch, ORB *orb); > int css_do_tsch_get_irb(SubchDev *sch, IRB *irb, int *irb_len); > void css_do_tsch_update_subch(SubchDev *sch); > int css_do_stcrw(CRW *crw); > @@ -243,7 +254,7 @@ int css_collect_chp_desc(int m, uint8_t cssid, uint8_t f_chpid, uint8_t l_chpid, > void css_do_schm(uint8_t mbk, int update, int dct, uint64_t mbo); > int css_enable_mcsse(void); > int css_enable_mss(void); > -int css_do_rsch(SubchDev *sch); > +IOInstEnding css_do_rsch(SubchDev *sch); > int css_do_rchp(uint8_t cssid, uint8_t chpid); > bool css_present(uint8_t cssid); > #endif > diff --git a/include/hw/s390x/s390-ccw.h b/include/hw/s390x/s390-ccw.h > index 9f45cf1347..7d15a1a5d4 100644 > --- a/include/hw/s390x/s390-ccw.h > +++ b/include/hw/s390x/s390-ccw.h > @@ -33,7 +33,7 @@ typedef struct S390CCWDeviceClass { > CCWDeviceClass parent_class; > void (*realize)(S390CCWDevice *dev, char *sysfsdev, Error **errp); > void (*unrealize)(S390CCWDevice *dev, Error **errp); > - int (*handle_request) (ORB *, SCSW *, void *); > + IOInstEnding (*handle_request) (SubchDev *sch); > } S390CCWDeviceClass; > > #endif > diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c > index 47490f838a..16b5cf2fed 100644 > --- a/target/s390x/ioinst.c > +++ b/target/s390x/ioinst.c > @@ -218,8 +218,6 @@ void ioinst_handle_ssch(S390CPU *cpu, uint64_t reg1, uint32_t ipb) > SubchDev *sch; > ORB orig_orb, orb; > uint64_t addr; > - int ret = -ENODEV; > - int cc; > CPUS390XState *env = &cpu->env; > uint8_t ar; > > @@ -239,33 +237,11 @@ void ioinst_handle_ssch(S390CPU *cpu, uint64_t reg1, uint32_t ipb) > } > trace_ioinst_sch_id("ssch", cssid, ssid, schid); > sch = css_find_subch(m, cssid, ssid, schid); > - if (sch && css_subch_visible(sch)) { > - ret = css_do_ssch(sch, &orb); > - } > - switch (ret) { > - case -ENODEV: > - cc = 3; > - break; > - case -EBUSY: > - cc = 2; > - break; > - case -EFAULT: > - /* > - * TODO: > - * I'm wondering whether there is something better > - * to do for us here (like setting some device or > - * subchannel status). > - */ > - program_interrupt(env, PGM_ADDRESSING, 4); > + if (!sch || !css_subch_visible(sch)) { > + setcc(cpu, 3); > return; > - case 0: > - cc = 0; > - break; > - default: > - cc = 1; > - break; > } > - setcc(cpu, cc); > + setcc(cpu, css_do_ssch(sch, &orb)); > } > > void ioinst_handle_stcrw(S390CPU *cpu, uint32_t ipb) > @@ -784,8 +760,6 @@ void ioinst_handle_rsch(S390CPU *cpu, uint64_t reg1) > { > int cssid, ssid, schid, m; > SubchDev *sch; > - int ret = -ENODEV; > - int cc; > > if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid)) { > program_interrupt(&cpu->env, PGM_OPERAND, 4); > @@ -793,24 +767,11 @@ void ioinst_handle_rsch(S390CPU *cpu, uint64_t reg1) > } > trace_ioinst_sch_id("rsch", cssid, ssid, schid); > sch = css_find_subch(m, cssid, ssid, schid); > - if (sch && css_subch_visible(sch)) { > - ret = css_do_rsch(sch); > - } > - switch (ret) { > - case -ENODEV: > - cc = 3; > - break; > - case -EINVAL: > - cc = 2; > - break; > - case 0: > - cc = 0; > - break; > - default: > - cc = 1; > - break; > + if (!sch || !css_subch_visible(sch)) { > + setcc(cpu, 3); > + return; > } > - setcc(cpu, cc); > + setcc(cpu, css_do_rsch(sch)); > } > > #define RCHP_REG1_RES(_reg) (_reg & 0x00000000ff00ff00) Looks sane.
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 <pasic@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? 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
On 18.10.2017 11:30, Thomas Huth 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 <pasic@linux.vnet.ibm.com> [...] >> @@ -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... ... or maybe rather at least return IOINST_CC_STATUS_PRESENT instead? IOINST_CC_EXPECTED sounds somewhat wrong to me here. Thomas
On Wed, 18 Oct 2017 11:30:47 +0200 Thomas Huth <thuth@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 <pasic@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.
On Wed, 18 Oct 2017 11:52:03 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 18.10.2017 11:30, Thomas Huth 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 <pasic@linux.vnet.ibm.com> > [...] > >> @@ -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... > > ... or maybe rather at least return IOINST_CC_STATUS_PRESENT instead? > IOINST_CC_EXPECTED sounds somewhat wrong to me here. But the ssch did conclude as expected :) Keep in mind that QEMU performs the start function synchronously (i.e., before the condition code is set). On real hardware, you get a cc 0 for the ssch if the subchannel is basically in a status that's ok for triggering the start function. A unit exception is a possible result of the start function (and therefore generating an I/O interrupt, which you only get if ssch set cc 0.)
On 18.10.2017 11:58, Cornelia Huck wrote: > On Wed, 18 Oct 2017 11:52:03 +0200 > Thomas Huth <thuth@redhat.com> wrote: > >> On 18.10.2017 11:30, Thomas Huth 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 <pasic@linux.vnet.ibm.com> >> [...] >>>> @@ -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... >> >> ... or maybe rather at least return IOINST_CC_STATUS_PRESENT instead? >> IOINST_CC_EXPECTED sounds somewhat wrong to me here. > > But the ssch did conclude as expected :) > > Keep in mind that QEMU performs the start function synchronously (i.e., > before the condition code is set). On real hardware, you get a cc 0 for > the ssch if the subchannel is basically in a status that's ok for > triggering the start function. A unit exception is a possible result of > the start function (and therefore generating an I/O interrupt, which > you only get if ssch set cc 0.) OK, I'm not that familiar with the I/O sub-system. If you say that it is ok, then simply ignore my comment, please :-) Thomas
On 18.10.2017 11:52, Cornelia Huck wrote: > On Wed, 18 Oct 2017 11:30:47 +0200 > Thomas Huth <thuth@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 <pasic@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. Well, the guest could also trigger this condition on purpose (e.g. kvm-unit-tests), so I wonder whether we want to see the warning in that case, too... IMHO this is exactly what qemu_log_mask(LOG_GUEST_ERROR, ...) has been implemented for: Log errors from the guest in case you suspect that the guest is doing something wrong. But that's just my 0.02 €, feel free to ignore me. >>> @@ -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). So if this is on purpose, this change in behavior should also be mentioned in the patch description, I think. Thomas
On 10/18/2017 12:07 PM, Thomas Huth wrote: > On 18.10.2017 11:52, Cornelia Huck wrote: >> On Wed, 18 Oct 2017 11:30:47 +0200 >> Thomas Huth <thuth@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 <pasic@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. > > Well, the guest could also trigger this condition on purpose (e.g. > kvm-unit-tests), so I wonder whether we want to see the warning in that > case, too... > IMHO this is exactly what qemu_log_mask(LOG_GUEST_ERROR, ...) has been > implemented for: Log errors from the guest in case you suspect that the > guest is doing something wrong. But that's just my 0.02 €, feel free to > ignore me. > >>>> @@ -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). > > So if this is on purpose, this change in behavior should also be > mentioned in the patch description, I think. > No. have a look at the function ioinst_handle_rsch. It used to map -EINVAL to cc 2 (and was an oddball in this respect) so we keep the old behavior, it's just more obvious whats happening. Regards, Halil > Thomas >
On 18.10.2017 13:07, Halil Pasic wrote: > > > On 10/18/2017 12:07 PM, Thomas Huth wrote: >> On 18.10.2017 11:52, Cornelia Huck wrote: >>> On Wed, 18 Oct 2017 11:30:47 +0200 >>> Thomas Huth <thuth@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 <pasic@linux.vnet.ibm.com> [...] >>>>> @@ -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). >> >> So if this is on purpose, this change in behavior should also be >> mentioned in the patch description, I think. > > No. have a look at the function ioinst_handle_rsch. It used > to map -EINVAL to cc 2 (and was an oddball in this respect) so we > keep the old behavior, it's just more obvious whats happening. Oh, you're right, I missed that different mapping of the error codes! ... so I see, the previous behavior was really confusing and error prone, it's really good that you're cleaning this up now! Thomas
On 10/18/2017 01:12 PM, Thomas Huth wrote: > On 18.10.2017 13:07, Halil Pasic wrote: >> >> >> On 10/18/2017 12:07 PM, Thomas Huth wrote: >>> On 18.10.2017 11:52, Cornelia Huck wrote: >>>> On Wed, 18 Oct 2017 11:30:47 +0200 >>>> Thomas Huth <thuth@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 <pasic@linux.vnet.ibm.com> > [...] >>>>>> @@ -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). >>> >>> So if this is on purpose, this change in behavior should also be >>> mentioned in the patch description, I think. >> >> No. have a look at the function ioinst_handle_rsch. It used >> to map -EINVAL to cc 2 (and was an oddball in this respect) so we >> keep the old behavior, it's just more obvious whats happening. > > Oh, you're right, I missed that different mapping of the error codes! > ... so I see, the previous behavior was really confusing and error > prone, it's really good that you're cleaning this up now! > > Thomas > Many thanks for the watchful eyes! In the end I did manage to sneak in a bug (for MSCH). I prefer a lots of false positives that than a couple of false negatives (for review). Regards, Halil
* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-10-17 16:04:49 +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 and absence of required ORB flags. > > Signed-off-by: Halil Pasic <pasic@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(-) > Agree for what already planned to fix when applying. LGTM: Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
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..."); + sch_gen_unit_exception(sch); + css_inject_io_interrupt(sch); + return IOINST_CC_EXPECTED; } - - 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); } /* @@ -1233,7 +1213,7 @@ static int sch_handle_start_func_passthrough(SubchDev *sch) * read/writes) asynchronous later on if we start supporting more than * our current very simple devices. */ -int do_subchannel_work_virtual(SubchDev *sch) +IOInstEnding do_subchannel_work_virtual(SubchDev *sch) { SCSW *s = &sch->curr_status.scsw; @@ -1247,12 +1227,12 @@ int do_subchannel_work_virtual(SubchDev *sch) sch_handle_start_func_virtual(sch); } css_inject_io_interrupt(sch); - return 0; + /* inst must succeed if this func is called */ + return IOINST_CC_EXPECTED; } -int do_subchannel_work_passthrough(SubchDev *sch) +IOInstEnding do_subchannel_work_passthrough(SubchDev *sch) { - int ret = 0; SCSW *s = &sch->curr_status.scsw; if (s->ctrl & SCSW_FCTL_CLEAR_FUNC) { @@ -1262,16 +1242,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 IOINST_CC_EXPECTED; } -static int do_subchannel_work(SubchDev *sch) +static IOInstEnding do_subchannel_work(SubchDev *sch) { if (!sch->do_subchannel_work) { - return -EINVAL; + return IOINST_CC_STATUS_PRESENT; } g_assert(sch->curr_status.scsw.ctrl & SCSW_CTRL_MASK_FCTL); return sch->do_subchannel_work(sch); @@ -1561,27 +1540,23 @@ static void css_update_chnmon(SubchDev *sch) } } -int css_do_ssch(SubchDev *sch, ORB *orb) +IOInstEnding css_do_ssch(SubchDev *sch, ORB *orb) { 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_FCTL_START_FUNC | SCSW_FCTL_HALT_FUNC | SCSW_FCTL_CLEAR_FUNC)) { - ret = -EBUSY; - goto out; + return IOINST_CC_BUSY; } /* If monitoring is active, update counter. */ @@ -1594,10 +1569,7 @@ int css_do_ssch(SubchDev *sch, ORB *orb) s->ctrl |= (SCSW_FCTL_START_FUNC | SCSW_ACTL_START_PEND); s->flags &= ~SCSW_FLAGS_MASK_PNO; - ret = do_subchannel_work(sch); - -out: - return ret; + return do_subchannel_work(sch); } static void copy_irb_to_guest(IRB *dest, const IRB *src, PMCW *pmcw, @@ -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; } /* If monitoring is active, update counter. */ @@ -1873,11 +1841,7 @@ int css_do_rsch(SubchDev *sch) } s->ctrl |= SCSW_ACTL_RESUME_PEND; - do_subchannel_work(sch); - ret = 0; - -out: - return ret; + return do_subchannel_work(sch); } int css_do_rchp(uint8_t cssid, uint8_t chpid) diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c index 8614dda6f8..0ef232ec27 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 IOINST_CC_STATUS_PRESENT; } + return cdc->handle_request(sch); } static void s390_ccw_get_dev_info(S390CCWDevice *cdev, 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; } - - 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 7e0dbd162f..10bde18452 100644 --- a/include/hw/s390x/css.h +++ b/include/hw/s390x/css.h @@ -136,11 +136,22 @@ struct SubchDev { /* transport-provided data: */ int (*ccw_cb) (SubchDev *, CCW1); void (*disable_cb)(SubchDev *); - int (*do_subchannel_work) (SubchDev *); + IOInstEnding (*do_subchannel_work) (SubchDev *); SenseId id; void *driver_data; }; +static inline void sch_gen_unit_exception(SubchDev *sch) +{ + sch->curr_status.scsw.ctrl &= ~SCSW_ACTL_START_PEND; + sch->curr_status.scsw.ctrl |= SCSW_STCTL_PRIMARY | + SCSW_STCTL_SECONDARY | + SCSW_STCTL_ALERT | + SCSW_STCTL_STATUS_PEND; + sch->curr_status.scsw.cpa = sch->channel_prog + 8; + sch->curr_status.scsw.dstat = SCSW_DSTAT_UNIT_EXCEP; +} + extern const VMStateDescription vmstate_subch_dev; /* @@ -199,9 +210,9 @@ void css_generate_sch_crws(uint8_t cssid, uint8_t ssid, uint16_t schid, void css_generate_chp_crws(uint8_t cssid, uint8_t chpid); void css_generate_css_crws(uint8_t cssid); void css_clear_sei_pending(void); -int s390_ccw_cmd_request(ORB *orb, SCSW *scsw, void *data); -int do_subchannel_work_virtual(SubchDev *sub); -int do_subchannel_work_passthrough(SubchDev *sub); +IOInstEnding s390_ccw_cmd_request(SubchDev *sch); +IOInstEnding do_subchannel_work_virtual(SubchDev *sub); +IOInstEnding do_subchannel_work_passthrough(SubchDev *sub); typedef enum { CSS_IO_ADAPTER_VIRTIO = 0, @@ -232,7 +243,7 @@ int css_do_msch(SubchDev *sch, const SCHIB *schib); int css_do_xsch(SubchDev *sch); int css_do_csch(SubchDev *sch); int css_do_hsch(SubchDev *sch); -int css_do_ssch(SubchDev *sch, ORB *orb); +IOInstEnding css_do_ssch(SubchDev *sch, ORB *orb); int css_do_tsch_get_irb(SubchDev *sch, IRB *irb, int *irb_len); void css_do_tsch_update_subch(SubchDev *sch); int css_do_stcrw(CRW *crw); @@ -243,7 +254,7 @@ int css_collect_chp_desc(int m, uint8_t cssid, uint8_t f_chpid, uint8_t l_chpid, void css_do_schm(uint8_t mbk, int update, int dct, uint64_t mbo); int css_enable_mcsse(void); int css_enable_mss(void); -int css_do_rsch(SubchDev *sch); +IOInstEnding css_do_rsch(SubchDev *sch); int css_do_rchp(uint8_t cssid, uint8_t chpid); bool css_present(uint8_t cssid); #endif diff --git a/include/hw/s390x/s390-ccw.h b/include/hw/s390x/s390-ccw.h index 9f45cf1347..7d15a1a5d4 100644 --- a/include/hw/s390x/s390-ccw.h +++ b/include/hw/s390x/s390-ccw.h @@ -33,7 +33,7 @@ typedef struct S390CCWDeviceClass { CCWDeviceClass parent_class; void (*realize)(S390CCWDevice *dev, char *sysfsdev, Error **errp); void (*unrealize)(S390CCWDevice *dev, Error **errp); - int (*handle_request) (ORB *, SCSW *, void *); + IOInstEnding (*handle_request) (SubchDev *sch); } S390CCWDeviceClass; #endif diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c index 47490f838a..16b5cf2fed 100644 --- a/target/s390x/ioinst.c +++ b/target/s390x/ioinst.c @@ -218,8 +218,6 @@ void ioinst_handle_ssch(S390CPU *cpu, uint64_t reg1, uint32_t ipb) SubchDev *sch; ORB orig_orb, orb; uint64_t addr; - int ret = -ENODEV; - int cc; CPUS390XState *env = &cpu->env; uint8_t ar; @@ -239,33 +237,11 @@ void ioinst_handle_ssch(S390CPU *cpu, uint64_t reg1, uint32_t ipb) } trace_ioinst_sch_id("ssch", cssid, ssid, schid); sch = css_find_subch(m, cssid, ssid, schid); - if (sch && css_subch_visible(sch)) { - ret = css_do_ssch(sch, &orb); - } - switch (ret) { - case -ENODEV: - cc = 3; - break; - case -EBUSY: - cc = 2; - break; - case -EFAULT: - /* - * TODO: - * I'm wondering whether there is something better - * to do for us here (like setting some device or - * subchannel status). - */ - program_interrupt(env, PGM_ADDRESSING, 4); + if (!sch || !css_subch_visible(sch)) { + setcc(cpu, 3); return; - case 0: - cc = 0; - break; - default: - cc = 1; - break; } - setcc(cpu, cc); + setcc(cpu, css_do_ssch(sch, &orb)); } void ioinst_handle_stcrw(S390CPU *cpu, uint32_t ipb) @@ -784,8 +760,6 @@ void ioinst_handle_rsch(S390CPU *cpu, uint64_t reg1) { int cssid, ssid, schid, m; SubchDev *sch; - int ret = -ENODEV; - int cc; if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid)) { program_interrupt(&cpu->env, PGM_OPERAND, 4); @@ -793,24 +767,11 @@ void ioinst_handle_rsch(S390CPU *cpu, uint64_t reg1) } trace_ioinst_sch_id("rsch", cssid, ssid, schid); sch = css_find_subch(m, cssid, ssid, schid); - if (sch && css_subch_visible(sch)) { - ret = css_do_rsch(sch); - } - switch (ret) { - case -ENODEV: - cc = 3; - break; - case -EINVAL: - cc = 2; - break; - case 0: - cc = 0; - break; - default: - cc = 1; - break; + if (!sch || !css_subch_visible(sch)) { + setcc(cpu, 3); + return; } - setcc(cpu, cc); + setcc(cpu, css_do_rsch(sch)); } #define RCHP_REG1_RES(_reg) (_reg & 0x00000000ff00ff00)
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 <pasic@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(-)