Re: [RFC PATCH v1 8/8] vfio-ccw: Add support for the CRW irq

[Cornelia Huck <cohuck@redhat.com>]

On Fri, 15 Nov 2019 04:34:37 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> From: Farhan Ali <alifm@linux.ibm.com>
> 
> The CRW irq will be used by vfio-ccw to notify the userspace
> about any CRWs the userspace needs to handle. Let's add support
> for it.
> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
> 
> Notes:
>     v0->v1: [EF]
>      - Check vcdev->crw_region before registering the irq,
>        in case host kernel does not have matching support
>      - Split the refactoring changes to an earlier (new) patch
>        (and don't remove the "num_irqs" check in the register
>        routine, but adjust it to the check the input variable)
>      - Don't revert the cool vfio_set_irq_signaling() stuff

Only the uncool stuff? ;)

>      - Unregister CRW IRQ before IO IRQ in unrealize
>      - s/crw1/crw0/
> 
>  hw/vfio/ccw.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index b16526d5de..b3f4120118 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -48,6 +48,7 @@ struct VFIOCCWDevice {
>      uint64_t crw_region_offset;
>      struct ccw_crw_region *crw_region;
>      EventNotifier io_notifier;
> +    EventNotifier crw_notifier;
>      bool force_orb_pfch;
>      bool warned_orb_pfch;
>  };
> @@ -259,6 +260,34 @@ static void vfio_ccw_reset(DeviceState *dev)
>      ioctl(vcdev->vdev.fd, VFIO_DEVICE_RESET);
>  }
>  
> +static void vfio_ccw_crw_notifier_handler(void *opaque)
> +{
> +    VFIOCCWDevice *vcdev = opaque;
> +    struct ccw_crw_region *region = vcdev->crw_region;
> +    CRW crw;
> +    int size;
> +    uint8_t erc;
> +    uint16_t rsid;
> +
> +    if (!event_notifier_test_and_clear(&vcdev->crw_notifier)) {
> +        return;
> +    }

Referring back to the comments I made for the kernel part
(https://lore.kernel.org/kvm/20191119195236.35189d5b.cohuck@redhat.com/),
I think we may have a problem when we have multiple crws pending.

IIUC, the kernel does not provide any guarantees that we get exactly
one interrupt per crw. I'm wondering whether it would make sense to
mimic the behaviour of stcrw, i.e.

(a) get a notification
(b) read the region to obtain a crw
(c) do whatever needs to be done
(d) repeat (b) and (c) until (b) does not return a valid crw

That way, we can also accommodate arbitrary lengths of crw chains, as
we do not have to reserve space for a pre-defined number of crws in the
region.

> +
> +    memset(region, 0, sizeof(*region));
> +    size = pread(vcdev->vdev.fd, region, vcdev->crw_region_size,
> +                 vcdev->crw_region_offset);
> +
> +    if (size == -1) {
> +        error_report("vfio-ccw: Read crw region failed with errno=%d", errno);
> +        return;
> +    }
> +
> +    memcpy(&crw, &region->crw0, sizeof(CRW));
> +    erc = crw.flags & 0x003f;
> +    rsid = crw.rsid;
> +    css_queue_crw(CRW_RSC_CHP, erc, 0, 0, rsid);
> +}
> +
>  static void vfio_ccw_io_notifier_handler(void *opaque)
>  {
>      VFIOCCWDevice *vcdev = opaque;