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

[Farhan Ali <alifm@linux.ibm.com>]

On 05/21/2019 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.
> 
> 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?

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? :)


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