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

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

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
>