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

[Cornelia Huck]

On Tue, 17 Oct 2017 16:04:49 +0200
Halil Pasic <address@hidden> 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...");

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.