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

[Eric Farman <farman@linux.ibm.com>]

On 5/21/19 12:32 PM, Cornelia Huck wrote:
> 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.

Ahhh, I didn't follow the bread crumbs back far enough to notice that.
Yes, that should help keep things in line.

> 
> 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.

I suspect you are right, because of the check on the halt side, and
considering that the clear is the biggest recovery action we have.  So
this does seem like things are okay.  I'll ponder this overnight and
finish my review tomorrow.

> 
>>
>> 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;
>>> +    }
>>> +}
>