On Tue, 21 May 2019 16:51:27 -0400
Farhan Ali <al...@linux.ibm.com> wrote:
> On 05/21/2019 12:32 PM, Cornelia Huck wrote:
> > On Mon, 20 May 2019 12:29:56 -0400
> > Eric Farman <far...@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 <coh...@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?
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;
> >>> + }
> >>> +}
> >
> >
>
