Re: [Qemu-devel] [PATCH v6 10/13] s390x/css: introduce and realize ccw-request callback

[Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>]

* Auger Eric <eric.auger@redhat.com> [2017-05-01 19:28:23 +0200]:

Hi Eric,

> > diff --git a/hw/s390x/s390-ccw.h b/hw/s390x/s390-ccw.h
> > index b58d8e9..4e2fa65 100644
> > --- a/hw/s390x/s390-ccw.h
> > +++ b/hw/s390x/s390-ccw.h
> > @@ -27,6 +27,7 @@ typedef struct S390CCWDevice {
> >      CcwDevice parent_obj;
> >      CssDevId hostid;
> >      char *mdevid;
> > +    int (*handle_request) (ORB *, SCSW *, void *);
> shouldn't this callback be part of the class instead?
Ok. This sounds more proper.

> 
> >  } S390CCWDevice;
> >  
> >  typedef struct S390CCWDeviceClass {
> > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> > index cda3057..3ff78c5 100644
> > --- a/hw/vfio/ccw.c
> > +++ b/hw/vfio/ccw.c
> > @@ -47,6 +47,36 @@ 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)
> > +{
> > +    S390CCWDevice *cdev = data;
> > +    VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev);
> > +    struct ccw_io_region *region = vcdev->io_region;
> > +    int ret;
> > +
> > +    QEMU_BUILD_BUG_ON(sizeof(region->orb_area) != sizeof(ORB));
> > +    QEMU_BUILD_BUG_ON(sizeof(region->scsw_area) != sizeof(SCSW));
> > +    QEMU_BUILD_BUG_ON(sizeof(region->irb_area) != sizeof(IRB));
> > +
> > +    memset(region, 0, sizeof(*region));
> > +
> > +    memcpy(region->orb_area, orb, sizeof(ORB));
> > +    memcpy(region->scsw_area, scsw, sizeof(SCSW));
> > +
> > +again:
> > +    ret = pwrite(vcdev->vdev.fd, region,
> > +                 vcdev->io_region_size, vcdev->io_region_offset);
> > +    if (ret != vcdev->io_region_size) {
> > +        if (errno == EAGAIN) {
> > +            goto again;
> > +        }
> > +        error_report("vfio-ccw: wirte I/O region failed with errno=%d", errno);
> > +        return -errno;
> > +    }
> > +
> > +    return region->ret_code;
> > +}
> > +
> >  static void vfio_ccw_reset(DeviceState *dev)
> >  {
> >      CcwDevice *ccw_dev = DO_UPCAST(CcwDevice, parent_obj, dev);
> > @@ -59,10 +89,52 @@ static void vfio_ccw_reset(DeviceState *dev)
> >  static void vfio_ccw_io_notifier_handler(void *opaque)
> >  {
> >      VFIOCCWDevice *vcdev = opaque;
> > +    struct ccw_io_region *region = vcdev->io_region;
> > +    S390CCWDevice *cdev = S390_CCW_DEVICE(vcdev);
> > +    CcwDevice *ccw_dev = CCW_DEVICE(cdev);
> > +    SubchDev *sch = ccw_dev->sch;
> > +    SCSW *s = &sch->curr_status.scsw;
> > +    IRB irb;
> >  
> >      if (!event_notifier_test_and_clear(&vcdev->io_notifier)) {
> >          return;
> >      }
> > +
> > +    if (pread(vcdev->vdev.fd, region,
> > +              vcdev->io_region_size, vcdev->io_region_offset) == -1) {
> is it possible to read less bytes than requested?
Currently, impossible. The kernel side returns either the full region,
or an error.
But we surely could make it more friendly for a partial return. I will
add a check for that case, and generate a channel program check for it.

> 
> Thanks
> 
> Eric
> > +        switch (errno) {
> > +        case ENODEV:
> > +            /* Generate a deferred cc 3 condition. */
> > +            s->flags |= SCSW_FLAGS_MASK_CC;
> > +            s->ctrl &= ~SCSW_CTRL_MASK_STCTL;
> > +            s->ctrl |= (SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND);
> > +            goto read_err;
> > +        case EFAULT:
> > +            /* memory problem, generate channel data check */
> > +            s->ctrl &= ~SCSW_ACTL_START_PEND;
> > +            s->cstat = SCSW_CSTAT_DATA_CHECK;
> > +            s->ctrl &= ~SCSW_CTRL_MASK_STCTL;
> > +            s->ctrl |= SCSW_STCTL_PRIMARY | SCSW_STCTL_SECONDARY |
> > +                    SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
> > +            goto read_err;
> > +        default:
> > +            /* error, generate channel program check */
> > +            s->ctrl &= ~SCSW_ACTL_START_PEND;
> > +            s->cstat = SCSW_CSTAT_PROG_CHECK;
> > +            s->ctrl &= ~SCSW_CTRL_MASK_STCTL;
> > +            s->ctrl |= SCSW_STCTL_PRIMARY | SCSW_STCTL_SECONDARY |
> > +                    SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
> > +            goto read_err;
> > +        }
> > +    }
> > +
> > +    memcpy(&irb, region->irb_area, sizeof(IRB));
> > +
> > +    /* Update control block via irb. */
> > +    copy_scsw_to_guest(s, &irb.scsw);
> > +
> > +read_err:
> > +    css_inject_io_interrupt(sch);
> >  }
> >  
> >  static void vfio_ccw_register_io_notifier(VFIOCCWDevice *vcdev, Error **errp)
[...]

-- 
Dong Jia Shi