[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Farhan Ali
Subject: Re: [qemu-s390x] [PATCH v4 2/2] vfio-ccw: support async command subregion
Date: Tue, 21 May 2019 16:51:27 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0

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?

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.

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]