Re: [RFC PATCH v1 7/8] vfio-ccw: Refactor ccw irq handler

[Cornelia Huck]

On Fri, 15 Nov 2019 04:34:36 +0100
Eric Farman <address@hidden> wrote:

> Make it easier to add new ones in the future.
> 
> Signed-off-by: Eric Farman <address@hidden>
> ---
>  hw/vfio/ccw.c | 55 ++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 39 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 2b1a83b94c..b16526d5de 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -334,22 +334,35 @@ read_err:
>      css_inject_io_interrupt(sch);
>  }
>  
> -static void vfio_ccw_register_io_notifier(VFIOCCWDevice *vcdev, Error **errp)
> +static void vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev, int irq,

Maybe make this unsigned?

> +                                           Error **errp)
>  {
>      VFIODevice *vdev = &vcdev->vdev;
>      struct vfio_irq_info *irq_info;
>      size_t argsz;
>      int fd;
> +    EventNotifier *notifier;
> +    IOHandler *fd_read;
> +
> +    switch (irq) {
> +    case VFIO_CCW_IO_IRQ_INDEX:
> +        notifier = &vcdev->io_notifier;
> +        fd_read = vfio_ccw_io_notifier_handler;
> +        break;
> +    default:
> +        error_setg(errp, "vfio: Unsupported device irq(%d) fd: %m", irq);

Hm, which errno is this supposed to print?

> +        return;
> +    }
>  
> -    if (vdev->num_irqs < VFIO_CCW_IO_IRQ_INDEX + 1) {
> -        error_setg(errp, "vfio: unexpected number of io irqs %u",
> +    if (vdev->num_irqs < irq + 1) {
> +        error_setg(errp, "vfio: unexpected number of irqs %u",
>                     vdev->num_irqs);
>          return;
>      }
>  
>      argsz = sizeof(*irq_info);
>      irq_info = g_malloc0(argsz);
> -    irq_info->index = VFIO_CCW_IO_IRQ_INDEX;
> +    irq_info->index = irq;
>      irq_info->argsz = argsz;
>      if (ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO,
>                irq_info) < 0 || irq_info->count < 1) {
> @@ -357,37 +370,47 @@ static void vfio_ccw_register_io_notifier(VFIOCCWDevice 
> *vcdev, Error **errp)
>          goto out_free_info;
>      }
>  
> -    if (event_notifier_init(&vcdev->io_notifier, 0)) {
> +    if (event_notifier_init(notifier, 0)) {
>          error_setg_errno(errp, errno,
> -                         "vfio: Unable to init event notifier for IO");
> +                         "vfio: Unable to init event notifier for irq (%d)", 
> irq);
>          goto out_free_info;
>      }
>  
> -    fd = event_notifier_get_fd(&vcdev->io_notifier);
> -    qemu_set_fd_handler(fd, vfio_ccw_io_notifier_handler, NULL, vcdev);
> +    fd = event_notifier_get_fd(notifier);
> +    qemu_set_fd_handler(fd, fd_read, NULL, vcdev);
>  
> -    if (vfio_set_irq_signaling(vdev, VFIO_CCW_IO_IRQ_INDEX, 0,
> +    if (vfio_set_irq_signaling(vdev, irq, 0,
>                                 VFIO_IRQ_SET_ACTION_TRIGGER, fd, errp)) {
>          qemu_set_fd_handler(fd, NULL, NULL, vcdev);
> -        event_notifier_cleanup(&vcdev->io_notifier);
> +        event_notifier_cleanup(notifier);
>      }
>  
>  out_free_info:
>      g_free(irq_info);
>  }
>  
> -static void vfio_ccw_unregister_io_notifier(VFIOCCWDevice *vcdev)
> +static void vfio_ccw_unregister_irq_notifier(VFIOCCWDevice *vcdev, int irq)

Also unsigned here?

>  {
>      Error *err = NULL;
> +    EventNotifier *notifier;
> +
> +    switch (irq) {
> +    case VFIO_CCW_IO_IRQ_INDEX:
> +        notifier = &vcdev->io_notifier;
> +        break;
> +    default:
> +        error_report("vfio: Unsupported device irq(%d) fd: %m", irq);

Same comment for the %m.

> +        return;
> +    }
>  
> -    if (vfio_set_irq_signaling(&vcdev->vdev, VFIO_CCW_IO_IRQ_INDEX, 0,
> +    if (vfio_set_irq_signaling(&vcdev->vdev, irq, 0,
>                                 VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) {
>          error_reportf_err(err, VFIO_MSG_PREFIX, vcdev->vdev.name);
>      }
>  
> -    qemu_set_fd_handler(event_notifier_get_fd(&vcdev->io_notifier),
> +    qemu_set_fd_handler(event_notifier_get_fd(notifier),
>                          NULL, NULL, vcdev);
> -    event_notifier_cleanup(&vcdev->io_notifier);
> +    event_notifier_cleanup(notifier);
>  }
>  
>  static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp)
> @@ -590,7 +613,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error 
> **errp)
>          goto out_region_err;
>      }
>  
> -    vfio_ccw_register_io_notifier(vcdev, &err);
> +    vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX, &err);
>      if (err) {
>          goto out_notifier_err;
>      }
> @@ -619,7 +642,7 @@ static void vfio_ccw_unrealize(DeviceState *dev, Error 
> **errp)
>      S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev);
>      VFIOGroup *group = vcdev->vdev.group;
>  
> -    vfio_ccw_unregister_io_notifier(vcdev);
> +    vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX);
>      vfio_ccw_put_region(vcdev);
>      vfio_ccw_put_device(vcdev);
>      vfio_put_group(group);

Otherwise, looks good.