Re: [Qemu-devel] [PATCH v4 2/2] vfio-ccw: support async command subregion

[Cornelia Huck <cohuck@redhat.com>]

On Mon, 20 May 2019 12:29:56 -0400
Eric Farman <farman@linux.ibm.com> wrote:

> On 5/7/19 11:47 AM, Cornelia Huck wrote:
> > A vfio-ccw device may provide an async command subregion for
> > issuing halt/clear subchannel requests. If it is present, use
> > it for sending halt/clear request to the device; if not, fall
> > back to emulation (as done today).
> > 
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >  hw/s390x/css.c              |  27 +++++++--
> >  hw/vfio/ccw.c               | 110 +++++++++++++++++++++++++++++++++++-
> >  include/hw/s390x/s390-ccw.h |   3 +
> >  3 files changed, 134 insertions(+), 6 deletions(-)
> > 

> > +int vfio_ccw_handle_clear(SubchDev *sch)
> > +{
> > +    S390CCWDevice *cdev = sch->driver_data;
> > +    VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev);
> > +    struct ccw_cmd_region *region = vcdev->async_cmd_region;
> > +    int ret;
> > +
> > +    if (!vcdev->async_cmd_region) {
> > +        /* Async command region not available, fall back to emulation */
> > +        return -ENOSYS;
> > +    }
> > +
> > +    memset(region, 0, sizeof(*region));
> > +    region->command = VFIO_CCW_ASYNC_CMD_CSCH;  
> 
> Considering the serialization you added on the kernel side, what happens
> if another vcpu runs this code (or _halt) and clears the async region
> before the kernel code gains control from the pwrite() call below?
> Asked another way, there's nothing preventing us from issuing more than
> one asynchronous command concurrently, so how do we make sure the
> command gets to the kernel rather than "current command wins"  ?

This send me digging through the code, because if two threads can call
this at the same time for passthrough, we'd also have the same problem
for virtual.

If I followed the code correctly, all I/O instruction interpretation is
currently serialized via qemu_mutex_{lock,unlock}_iothread() (see
target/s390x/kvm.c respectively target/s390x/misc_helper.c). This
should mostly be enough to avoid stepping on each other's toes.

Why mostly? I'm not sure yet whether we handling multiple requests for
passthrough devices correctly yet (virtual should be fine.)

Start vs. (start|halt|clear) is fine, as the code checks whether
something is already pending before poking the kernel interface.
Likewise, halt vs. (start|halt|clear) is fine, as the code checks for
halt or clear and start and halt use different regions. The problematic
one is clear, as that's something that's always supposed to go through.
Probably fine if clear should always "win", but I need to think some
more about that.

> 
> That possibly worrisome question aside, this seems generally fine.
> 
> 
> > +
> > +again:
> > +    ret = pwrite(vcdev->vdev.fd, region,
> > +                 vcdev->async_cmd_region_size, vcdev->async_cmd_region_offset);
> > +    if (ret != vcdev->async_cmd_region_size) {
> > +        if (errno == EAGAIN) {
> > +            goto again;
> > +        }
> > +        error_report("vfio-ccw: write cmd region failed with errno=%d", errno);
> > +        ret = -errno;
> > +    } else {
> > +        ret = region->ret_code;
> > +    }
> > +    switch (ret) {
> > +    case 0:
> > +    case -ENODEV:
> > +    case -EACCES:
> > +        return 0;
> > +    case -EFAULT:
> > +    default:
> > +        sch_gen_unit_exception(sch);
> > +        css_inject_io_interrupt(sch);
> > +        return 0;
> > +    }
> > +}