Re: [qemu-s390x] [PATCH v4 2/2] vfio-ccw: support async command subregion

[Cornelia Huck]

On Tue, 21 May 2019 16:51:27 -0400
Farhan Ali <address@hidden> wrote:

> On 05/21/2019 12:32 PM, Cornelia Huck wrote:
> > On Mon, 20 May 2019 12:29:56 -0400
> > Eric Farman <address@hidden> 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 <address@hidden>
> >>> ---
> >>>   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.)  
> 
> 
> But don't virtual and passthrough device end up calling the same 
> ioinst_handle_* functions to interpret the I/O instructions?

Yes, they do.

> 
> As you mentioned all the ioinst_handle_* functions are called with the 
> qemu_mutex held. So I am confused as why virtual devices should be fine 
> and not passthrough? :)

I'm mostly worried about the "wipe the area, then call pwrite()"
sequence. We might wipe too often; but if clear is about hammering
through in any case, this is hopefully 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;
> >>> +    }
> >>> +}  
> > 
> >   
>