| Message ID | 20190507154733.28604-3-cohuck@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | vfio-ccw: support hsch/csch (QEMU part) | expand |
On Tue, 7 May 2019 17:47:33 +0200 Cornelia Huck <cohuck@redhat.com> 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(-) Ping. I'd like to include this in my next QEMU pull request; any comments? (I already have a headers update queued.)
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(-) > > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > index 8fc9e35ba5d3..5b21a74b5c29 100644 > --- a/hw/s390x/css.c > +++ b/hw/s390x/css.c > @@ -22,6 +22,7 @@ > #include "trace.h" > #include "hw/s390x/s390_flic.h" > #include "hw/s390x/s390-virtio-ccw.h" > +#include "hw/s390x/s390-ccw.h" > > typedef struct CrwContainer { > CRW crw; > @@ -1191,6 +1192,26 @@ static void sch_handle_start_func_virtual(SubchDev *sch) > > } > > +static void sch_handle_halt_func_passthrough(SubchDev *sch) > +{ > + int ret; > + > + ret = vfio_ccw_handle_halt(sch); > + if (ret == -ENOSYS) { > + sch_handle_halt_func(sch); > + } > +} > + > +static void sch_handle_clear_func_passthrough(SubchDev *sch) > +{ > + int ret; > + > + ret = vfio_ccw_handle_clear(sch); > + if (ret == -ENOSYS) { > + sch_handle_clear_func(sch); > + } > +} > + > static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch) > { > SCHIB *schib = &sch->curr_status; > @@ -1230,11 +1251,9 @@ IOInstEnding do_subchannel_work_passthrough(SubchDev *sch) > SCHIB *schib = &sch->curr_status; > > if (schib->scsw.ctrl & SCSW_FCTL_CLEAR_FUNC) { > - /* TODO: Clear handling */ > - sch_handle_clear_func(sch); > + sch_handle_clear_func_passthrough(sch); > } else if (schib->scsw.ctrl & SCSW_FCTL_HALT_FUNC) { > - /* TODO: Halt handling */ > - sch_handle_halt_func(sch); > + sch_handle_halt_func_passthrough(sch); > } else if (schib->scsw.ctrl & SCSW_FCTL_START_FUNC) { > return sch_handle_start_func_passthrough(sch); > } > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c > index 31dd3a2a87b6..175a17b1772a 100644 > --- a/hw/vfio/ccw.c > +++ b/hw/vfio/ccw.c > @@ -2,9 +2,12 @@ > * vfio based subchannel assignment support > * > * Copyright 2017 IBM Corp. > + * Copyright 2019 Red Hat, Inc. > + * > * Author(s): Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> > * Xiao Feng Ren <renxiaof@linux.vnet.ibm.com> > * Pierre Morel <pmorel@linux.vnet.ibm.com> > + * Cornelia Huck <cohuck@redhat.com> > * > * This work is licensed under the terms of the GNU GPL, version 2 or (at > * your option) any later version. See the COPYING file in the top-level > @@ -32,6 +35,9 @@ struct VFIOCCWDevice { > uint64_t io_region_size; > uint64_t io_region_offset; > struct ccw_io_region *io_region; > + uint64_t async_cmd_region_size; > + uint64_t async_cmd_region_offset; > + struct ccw_cmd_region *async_cmd_region; > EventNotifier io_notifier; > bool force_orb_pfch; > bool warned_orb_pfch; > @@ -114,6 +120,87 @@ again: > } > } > > +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" ? 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; > + } > +} > + > +int vfio_ccw_handle_halt(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_HSCH; > + > +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 -EBUSY: > + case -ENODEV: > + case -EACCES: > + return 0; > + case -EFAULT: > + default: > + sch_gen_unit_exception(sch); > + css_inject_io_interrupt(sch); > + return 0; > + } > +} > + > static void vfio_ccw_reset(DeviceState *dev) > { > CcwDevice *ccw_dev = DO_UPCAST(CcwDevice, parent_obj, dev); > @@ -287,9 +374,13 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp) > return; > } > > + /* > + * We always expect at least the I/O region to be present. We also > + * may have a variable number of regions governed by capabilities. > + */ > if (vdev->num_regions < VFIO_CCW_CONFIG_REGION_INDEX + 1) { > - error_setg(errp, "vfio: Unexpected number of the I/O region %u", > - vdev->num_regions); > + error_setg(errp, "vfio: too few regions (%u), expected at least %u", > + vdev->num_regions, VFIO_CCW_CONFIG_REGION_INDEX + 1); > return; > } > > @@ -309,11 +400,26 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp) > vcdev->io_region_offset = info->offset; > vcdev->io_region = g_malloc0(info->size); > > + /* check for the optional async command region */ > + ret = vfio_get_dev_region_info(vdev, VFIO_REGION_TYPE_CCW, > + VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD, &info); > + if (!ret) { > + vcdev->async_cmd_region_size = info->size; > + if (sizeof(*vcdev->async_cmd_region) != vcdev->async_cmd_region_size) { > + error_setg(errp, "vfio: Unexpected size of the async cmd region"); > + g_free(info); > + return; > + } > + vcdev->async_cmd_region_offset = info->offset; > + vcdev->async_cmd_region = g_malloc0(info->size); > + } > + > g_free(info); > } > > static void vfio_ccw_put_region(VFIOCCWDevice *vcdev) > { > + g_free(vcdev->async_cmd_region); > g_free(vcdev->io_region); > } > > diff --git a/include/hw/s390x/s390-ccw.h b/include/hw/s390x/s390-ccw.h > index 901d805d79a3..e9c7e1db5761 100644 > --- a/include/hw/s390x/s390-ccw.h > +++ b/include/hw/s390x/s390-ccw.h > @@ -37,4 +37,7 @@ typedef struct S390CCWDeviceClass { > IOInstEnding (*handle_request) (SubchDev *sch); > } S390CCWDeviceClass; > > +int vfio_ccw_handle_clear(SubchDev *sch); > +int vfio_ccw_handle_halt(SubchDev *sch); > + > #endif >
On 5/20/19 4:42 AM, Cornelia Huck wrote: > On Tue, 7 May 2019 17:47:33 +0200 > Cornelia Huck <cohuck@redhat.com> 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(-) > > Ping. I'd like to include this in my next QEMU pull request; any > comments? Apologies, I was looking at this late last week and thought it'd all become clear over the weekend. That didn't happen, so I've asked the only question I came up with. :) - Eric > > (I already have a headers update queued.) >
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" ? Hm... good question. But unfortunately not one I can answer quickly, so I'll put off queuing this patch and just send a pull request without it. It's not like we're in a hurry :) > > That possibly worrisome question aside, this seems generally fine. Thanks for looking! > > + > > +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; > > + }
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.) 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; > > + } > > +}
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; >>> + } >>> +} >
On 05/07/2019 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(-) > > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > index 8fc9e35ba5d3..5b21a74b5c29 100644 > --- a/hw/s390x/css.c > +++ b/hw/s390x/css.c > @@ -22,6 +22,7 @@ > #include "trace.h" > #include "hw/s390x/s390_flic.h" > #include "hw/s390x/s390-virtio-ccw.h" > +#include "hw/s390x/s390-ccw.h" > > typedef struct CrwContainer { > CRW crw; > @@ -1191,6 +1192,26 @@ static void sch_handle_start_func_virtual(SubchDev *sch) > > } > > +static void sch_handle_halt_func_passthrough(SubchDev *sch) > +{ > + int ret; > + > + ret = vfio_ccw_handle_halt(sch); > + if (ret == -ENOSYS) { > + sch_handle_halt_func(sch); > + } > +} > + > +static void sch_handle_clear_func_passthrough(SubchDev *sch) > +{ > + int ret; > + > + ret = vfio_ccw_handle_clear(sch); > + if (ret == -ENOSYS) { > + sch_handle_clear_func(sch); > + } > +} > + > static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch) > { > SCHIB *schib = &sch->curr_status; > @@ -1230,11 +1251,9 @@ IOInstEnding do_subchannel_work_passthrough(SubchDev *sch) > SCHIB *schib = &sch->curr_status; > > if (schib->scsw.ctrl & SCSW_FCTL_CLEAR_FUNC) { > - /* TODO: Clear handling */ > - sch_handle_clear_func(sch); > + sch_handle_clear_func_passthrough(sch); > } else if (schib->scsw.ctrl & SCSW_FCTL_HALT_FUNC) { > - /* TODO: Halt handling */ > - sch_handle_halt_func(sch); > + sch_handle_halt_func_passthrough(sch); > } else if (schib->scsw.ctrl & SCSW_FCTL_START_FUNC) { > return sch_handle_start_func_passthrough(sch); > } > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c > index 31dd3a2a87b6..175a17b1772a 100644 > --- a/hw/vfio/ccw.c > +++ b/hw/vfio/ccw.c > @@ -2,9 +2,12 @@ > * vfio based subchannel assignment support > * > * Copyright 2017 IBM Corp. > + * Copyright 2019 Red Hat, Inc. > + * > * Author(s): Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> > * Xiao Feng Ren <renxiaof@linux.vnet.ibm.com> > * Pierre Morel <pmorel@linux.vnet.ibm.com> > + * Cornelia Huck <cohuck@redhat.com> > * > * This work is licensed under the terms of the GNU GPL, version 2 or (at > * your option) any later version. See the COPYING file in the top-level > @@ -32,6 +35,9 @@ struct VFIOCCWDevice { > uint64_t io_region_size; > uint64_t io_region_offset; > struct ccw_io_region *io_region; > + uint64_t async_cmd_region_size; > + uint64_t async_cmd_region_offset; > + struct ccw_cmd_region *async_cmd_region; > EventNotifier io_notifier; > bool force_orb_pfch; > bool warned_orb_pfch; > @@ -114,6 +120,87 @@ again: > } > } > > +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; > + > +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; > + } > +} > + > +int vfio_ccw_handle_halt(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_HSCH; > + > +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 -EBUSY: > + case -ENODEV: > + case -EACCES: > + return 0; > + case -EFAULT: > + default: > + sch_gen_unit_exception(sch); > + css_inject_io_interrupt(sch); > + return 0; > + } > +} > + > static void vfio_ccw_reset(DeviceState *dev) > { > CcwDevice *ccw_dev = DO_UPCAST(CcwDevice, parent_obj, dev); > @@ -287,9 +374,13 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp) > return; > } > > + /* > + * We always expect at least the I/O region to be present. We also > + * may have a variable number of regions governed by capabilities. > + */ > if (vdev->num_regions < VFIO_CCW_CONFIG_REGION_INDEX + 1) { > - error_setg(errp, "vfio: Unexpected number of the I/O region %u", > - vdev->num_regions); > + error_setg(errp, "vfio: too few regions (%u), expected at least %u", > + vdev->num_regions, VFIO_CCW_CONFIG_REGION_INDEX + 1); > return; > } > > @@ -309,11 +400,26 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp) > vcdev->io_region_offset = info->offset; > vcdev->io_region = g_malloc0(info->size); > > + /* check for the optional async command region */ > + ret = vfio_get_dev_region_info(vdev, VFIO_REGION_TYPE_CCW, > + VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD, &info); > + if (!ret) { > + vcdev->async_cmd_region_size = info->size; > + if (sizeof(*vcdev->async_cmd_region) != vcdev->async_cmd_region_size) { > + error_setg(errp, "vfio: Unexpected size of the async cmd region"); > + g_free(info); > + return; > + } > + vcdev->async_cmd_region_offset = info->offset; > + vcdev->async_cmd_region = g_malloc0(info->size); > + } > + > g_free(info); > } > > static void vfio_ccw_put_region(VFIOCCWDevice *vcdev) > { > + g_free(vcdev->async_cmd_region); > g_free(vcdev->io_region); > } > > diff --git a/include/hw/s390x/s390-ccw.h b/include/hw/s390x/s390-ccw.h > index 901d805d79a3..e9c7e1db5761 100644 > --- a/include/hw/s390x/s390-ccw.h > +++ b/include/hw/s390x/s390-ccw.h > @@ -37,4 +37,7 @@ typedef struct S390CCWDeviceClass { > IOInstEnding (*handle_request) (SubchDev *sch); > } S390CCWDeviceClass; > > +int vfio_ccw_handle_clear(SubchDev *sch); > +int vfio_ccw_handle_halt(SubchDev *sch); > + We are not making clear and halt functions part of the S390CCWDeviceClass, is there are reason for doing this? Currently we handle ssch through the handle_request function, it just looks a little inconsistent. Thanks Farhan > #endif >
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; >>> + } >>> +} > >
On Tue, 21 May 2019 16:51:27 -0400 Farhan Ali <alifm@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 <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? 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; > >>> + } > >>> +} > > > > >
On Tue, 21 May 2019 16:50:47 -0400 Farhan Ali <alifm@linux.ibm.com> wrote: > On 05/07/2019 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(-) > > diff --git a/include/hw/s390x/s390-ccw.h b/include/hw/s390x/s390-ccw.h > > index 901d805d79a3..e9c7e1db5761 100644 > > --- a/include/hw/s390x/s390-ccw.h > > +++ b/include/hw/s390x/s390-ccw.h > > @@ -37,4 +37,7 @@ typedef struct S390CCWDeviceClass { > > IOInstEnding (*handle_request) (SubchDev *sch); > > } S390CCWDeviceClass; > > > > +int vfio_ccw_handle_clear(SubchDev *sch); > > +int vfio_ccw_handle_halt(SubchDev *sch); > > + > > We are not making clear and halt functions part of the > S390CCWDeviceClass, is there are reason for doing this? > Currently we handle ssch through the handle_request function, it just > looks a little inconsistent. I don't quite remember why I did it this way; not sure if there is a good reason for that (that patch has been around for too long...) We can change such internal details later on, though. (And I think your comment has merit.)
On 05/22/2019 06:17 AM, Cornelia Huck wrote: > On Tue, 21 May 2019 16:50:47 -0400 > Farhan Ali <alifm@linux.ibm.com> wrote: > >> On 05/07/2019 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(-) > >>> diff --git a/include/hw/s390x/s390-ccw.h b/include/hw/s390x/s390-ccw.h >>> index 901d805d79a3..e9c7e1db5761 100644 >>> --- a/include/hw/s390x/s390-ccw.h >>> +++ b/include/hw/s390x/s390-ccw.h >>> @@ -37,4 +37,7 @@ typedef struct S390CCWDeviceClass { >>> IOInstEnding (*handle_request) (SubchDev *sch); >>> } S390CCWDeviceClass; >>> >>> +int vfio_ccw_handle_clear(SubchDev *sch); >>> +int vfio_ccw_handle_halt(SubchDev *sch); >>> + >> >> We are not making clear and halt functions part of the >> S390CCWDeviceClass, is there are reason for doing this? >> Currently we handle ssch through the handle_request function, it just >> looks a little inconsistent. > > I don't quite remember why I did it this way; not sure if there is a > good reason for that (that patch has been around for too long...) > > We can change such internal details later on, though. (And I think your > comment has merit.) > > Yes, sure we could change it later on. I do prefer your way though, it avoids one more layer of indirection. Thanks Farhan
On Tue, 21 May 2019 16:47:45 -0400 Eric Farman <farman@linux.ibm.com> wrote: > On 5/21/19 12:32 PM, Cornelia Huck wrote: > > 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. Ok, what's the verdict here? :) (I'm trying to clean up my pending stuff :)
On 5/29/19 5:48 AM, Cornelia Huck wrote: > On Tue, 21 May 2019 16:47:45 -0400 > Eric Farman <farman@linux.ibm.com> wrote: > >> On 5/21/19 12:32 PM, Cornelia Huck wrote: > >>> 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. > > Ok, what's the verdict here? :) Gah, I left this sit in my draft folder before holiday. Sorry about that! > > (I'm trying to clean up my pending stuff :) >
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> This looks fine to me; thanks for the explanations! Reviewed-by: Eric Farman <farman@linux.ibm.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(-) > > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > index 8fc9e35ba5d3..5b21a74b5c29 100644 > --- a/hw/s390x/css.c > +++ b/hw/s390x/css.c > @@ -22,6 +22,7 @@ > #include "trace.h" > #include "hw/s390x/s390_flic.h" > #include "hw/s390x/s390-virtio-ccw.h" > +#include "hw/s390x/s390-ccw.h" > > typedef struct CrwContainer { > CRW crw; > @@ -1191,6 +1192,26 @@ static void sch_handle_start_func_virtual(SubchDev *sch) > > } > > +static void sch_handle_halt_func_passthrough(SubchDev *sch) > +{ > + int ret; > + > + ret = vfio_ccw_handle_halt(sch); > + if (ret == -ENOSYS) { > + sch_handle_halt_func(sch); > + } > +} > + > +static void sch_handle_clear_func_passthrough(SubchDev *sch) > +{ > + int ret; > + > + ret = vfio_ccw_handle_clear(sch); > + if (ret == -ENOSYS) { > + sch_handle_clear_func(sch); > + } > +} > + > static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch) > { > SCHIB *schib = &sch->curr_status; > @@ -1230,11 +1251,9 @@ IOInstEnding do_subchannel_work_passthrough(SubchDev *sch) > SCHIB *schib = &sch->curr_status; > > if (schib->scsw.ctrl & SCSW_FCTL_CLEAR_FUNC) { > - /* TODO: Clear handling */ > - sch_handle_clear_func(sch); > + sch_handle_clear_func_passthrough(sch); > } else if (schib->scsw.ctrl & SCSW_FCTL_HALT_FUNC) { > - /* TODO: Halt handling */ > - sch_handle_halt_func(sch); > + sch_handle_halt_func_passthrough(sch); > } else if (schib->scsw.ctrl & SCSW_FCTL_START_FUNC) { > return sch_handle_start_func_passthrough(sch); > } > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c > index 31dd3a2a87b6..175a17b1772a 100644 > --- a/hw/vfio/ccw.c > +++ b/hw/vfio/ccw.c > @@ -2,9 +2,12 @@ > * vfio based subchannel assignment support > * > * Copyright 2017 IBM Corp. > + * Copyright 2019 Red Hat, Inc. > + * > * Author(s): Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> > * Xiao Feng Ren <renxiaof@linux.vnet.ibm.com> > * Pierre Morel <pmorel@linux.vnet.ibm.com> > + * Cornelia Huck <cohuck@redhat.com> > * > * This work is licensed under the terms of the GNU GPL, version 2 or (at > * your option) any later version. See the COPYING file in the top-level > @@ -32,6 +35,9 @@ struct VFIOCCWDevice { > uint64_t io_region_size; > uint64_t io_region_offset; > struct ccw_io_region *io_region; > + uint64_t async_cmd_region_size; > + uint64_t async_cmd_region_offset; > + struct ccw_cmd_region *async_cmd_region; > EventNotifier io_notifier; > bool force_orb_pfch; > bool warned_orb_pfch; > @@ -114,6 +120,87 @@ again: > } > } > > +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; > + > +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; > + } > +} > + > +int vfio_ccw_handle_halt(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_HSCH; > + > +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 -EBUSY: > + case -ENODEV: > + case -EACCES: > + return 0; > + case -EFAULT: > + default: > + sch_gen_unit_exception(sch); > + css_inject_io_interrupt(sch); > + return 0; > + } > +} > + > static void vfio_ccw_reset(DeviceState *dev) > { > CcwDevice *ccw_dev = DO_UPCAST(CcwDevice, parent_obj, dev); > @@ -287,9 +374,13 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp) > return; > } > > + /* > + * We always expect at least the I/O region to be present. We also > + * may have a variable number of regions governed by capabilities. > + */ > if (vdev->num_regions < VFIO_CCW_CONFIG_REGION_INDEX + 1) { > - error_setg(errp, "vfio: Unexpected number of the I/O region %u", > - vdev->num_regions); > + error_setg(errp, "vfio: too few regions (%u), expected at least %u", > + vdev->num_regions, VFIO_CCW_CONFIG_REGION_INDEX + 1); > return; > } > > @@ -309,11 +400,26 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp) > vcdev->io_region_offset = info->offset; > vcdev->io_region = g_malloc0(info->size); > > + /* check for the optional async command region */ > + ret = vfio_get_dev_region_info(vdev, VFIO_REGION_TYPE_CCW, > + VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD, &info); > + if (!ret) { > + vcdev->async_cmd_region_size = info->size; > + if (sizeof(*vcdev->async_cmd_region) != vcdev->async_cmd_region_size) { > + error_setg(errp, "vfio: Unexpected size of the async cmd region"); > + g_free(info); > + return; > + } > + vcdev->async_cmd_region_offset = info->offset; > + vcdev->async_cmd_region = g_malloc0(info->size); > + } > + > g_free(info); > } > > static void vfio_ccw_put_region(VFIOCCWDevice *vcdev) > { > + g_free(vcdev->async_cmd_region); > g_free(vcdev->io_region); > } > > diff --git a/include/hw/s390x/s390-ccw.h b/include/hw/s390x/s390-ccw.h > index 901d805d79a3..e9c7e1db5761 100644 > --- a/include/hw/s390x/s390-ccw.h > +++ b/include/hw/s390x/s390-ccw.h > @@ -37,4 +37,7 @@ typedef struct S390CCWDeviceClass { > IOInstEnding (*handle_request) (SubchDev *sch); > } S390CCWDeviceClass; > > +int vfio_ccw_handle_clear(SubchDev *sch); > +int vfio_ccw_handle_halt(SubchDev *sch); > + > #endif >
On Wed, 29 May 2019 09:47:57 -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> > > This looks fine to me; thanks for the explanations! > > Reviewed-by: Eric Farman <farman@linux.ibm.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(-) Thanks for your review! Queued to s390-next.
diff --git a/hw/s390x/css.c b/hw/s390x/css.c index 8fc9e35ba5d3..5b21a74b5c29 100644 --- a/hw/s390x/css.c +++ b/hw/s390x/css.c @@ -22,6 +22,7 @@ #include "trace.h" #include "hw/s390x/s390_flic.h" #include "hw/s390x/s390-virtio-ccw.h" +#include "hw/s390x/s390-ccw.h" typedef struct CrwContainer { CRW crw; @@ -1191,6 +1192,26 @@ static void sch_handle_start_func_virtual(SubchDev *sch) } +static void sch_handle_halt_func_passthrough(SubchDev *sch) +{ + int ret; + + ret = vfio_ccw_handle_halt(sch); + if (ret == -ENOSYS) { + sch_handle_halt_func(sch); + } +} + +static void sch_handle_clear_func_passthrough(SubchDev *sch) +{ + int ret; + + ret = vfio_ccw_handle_clear(sch); + if (ret == -ENOSYS) { + sch_handle_clear_func(sch); + } +} + static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch) { SCHIB *schib = &sch->curr_status; @@ -1230,11 +1251,9 @@ IOInstEnding do_subchannel_work_passthrough(SubchDev *sch) SCHIB *schib = &sch->curr_status; if (schib->scsw.ctrl & SCSW_FCTL_CLEAR_FUNC) { - /* TODO: Clear handling */ - sch_handle_clear_func(sch); + sch_handle_clear_func_passthrough(sch); } else if (schib->scsw.ctrl & SCSW_FCTL_HALT_FUNC) { - /* TODO: Halt handling */ - sch_handle_halt_func(sch); + sch_handle_halt_func_passthrough(sch); } else if (schib->scsw.ctrl & SCSW_FCTL_START_FUNC) { return sch_handle_start_func_passthrough(sch); } diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c index 31dd3a2a87b6..175a17b1772a 100644 --- a/hw/vfio/ccw.c +++ b/hw/vfio/ccw.c @@ -2,9 +2,12 @@ * vfio based subchannel assignment support * * Copyright 2017 IBM Corp. + * Copyright 2019 Red Hat, Inc. + * * Author(s): Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> * Xiao Feng Ren <renxiaof@linux.vnet.ibm.com> * Pierre Morel <pmorel@linux.vnet.ibm.com> + * Cornelia Huck <cohuck@redhat.com> * * This work is licensed under the terms of the GNU GPL, version 2 or (at * your option) any later version. See the COPYING file in the top-level @@ -32,6 +35,9 @@ struct VFIOCCWDevice { uint64_t io_region_size; uint64_t io_region_offset; struct ccw_io_region *io_region; + uint64_t async_cmd_region_size; + uint64_t async_cmd_region_offset; + struct ccw_cmd_region *async_cmd_region; EventNotifier io_notifier; bool force_orb_pfch; bool warned_orb_pfch; @@ -114,6 +120,87 @@ again: } } +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; + +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; + } +} + +int vfio_ccw_handle_halt(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_HSCH; + +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 -EBUSY: + case -ENODEV: + case -EACCES: + return 0; + case -EFAULT: + default: + sch_gen_unit_exception(sch); + css_inject_io_interrupt(sch); + return 0; + } +} + static void vfio_ccw_reset(DeviceState *dev) { CcwDevice *ccw_dev = DO_UPCAST(CcwDevice, parent_obj, dev); @@ -287,9 +374,13 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp) return; } + /* + * We always expect at least the I/O region to be present. We also + * may have a variable number of regions governed by capabilities. + */ if (vdev->num_regions < VFIO_CCW_CONFIG_REGION_INDEX + 1) { - error_setg(errp, "vfio: Unexpected number of the I/O region %u", - vdev->num_regions); + error_setg(errp, "vfio: too few regions (%u), expected at least %u", + vdev->num_regions, VFIO_CCW_CONFIG_REGION_INDEX + 1); return; } @@ -309,11 +400,26 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp) vcdev->io_region_offset = info->offset; vcdev->io_region = g_malloc0(info->size); + /* check for the optional async command region */ + ret = vfio_get_dev_region_info(vdev, VFIO_REGION_TYPE_CCW, + VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD, &info); + if (!ret) { + vcdev->async_cmd_region_size = info->size; + if (sizeof(*vcdev->async_cmd_region) != vcdev->async_cmd_region_size) { + error_setg(errp, "vfio: Unexpected size of the async cmd region"); + g_free(info); + return; + } + vcdev->async_cmd_region_offset = info->offset; + vcdev->async_cmd_region = g_malloc0(info->size); + } + g_free(info); } static void vfio_ccw_put_region(VFIOCCWDevice *vcdev) { + g_free(vcdev->async_cmd_region); g_free(vcdev->io_region); } diff --git a/include/hw/s390x/s390-ccw.h b/include/hw/s390x/s390-ccw.h index 901d805d79a3..e9c7e1db5761 100644 --- a/include/hw/s390x/s390-ccw.h +++ b/include/hw/s390x/s390-ccw.h @@ -37,4 +37,7 @@ typedef struct S390CCWDeviceClass { IOInstEnding (*handle_request) (SubchDev *sch); } S390CCWDeviceClass; +int vfio_ccw_handle_clear(SubchDev *sch); +int vfio_ccw_handle_halt(SubchDev *sch); + #endif
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(-)