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

[Eric Farman]

On 11/20/19 7:39 AM, Cornelia Huck wrote:
> 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?

Yeah, seems proper.

> 
>> +                                           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?

Um...  Dealers choice?  :)  I'll fix this up.

> 
>> +        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.
>