Re: [Qemu-devel] [PATCH] vfio-ccw: use vfio_set_irq_signaling

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

On 6/14/19 11:06 AM, Auger Eric wrote:
> Hi Eric,
> 
> On 6/14/19 4:30 PM, Eric Farman wrote:
>>
>>
>> On 6/14/19 5:27 AM, Cornelia Huck wrote:
>>> Use the new helper.
>>>
>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>> ---
>>>  hw/vfio/ccw.c | 68 +++++++++++----------------------------------------
>>>  1 file changed, 14 insertions(+), 54 deletions(-)
>>>
>>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
>>> index 03a2becb3ec9..3dc08721a3db 100644
>>> --- a/hw/vfio/ccw.c
>>> +++ b/hw/vfio/ccw.c
>>> @@ -197,10 +197,7 @@ read_err:
>>>  static void vfio_ccw_register_io_notifier(VFIOCCWDevice *vcdev, Error **errp)
>>>  {
>>>      VFIODevice *vdev = &vcdev->vdev;
>>> -    struct vfio_irq_info *irq_info;
>>> -    struct vfio_irq_set *irq_set;
>>> -    size_t argsz;
>>> -    int32_t *pfd;
>>> +    int fd;
>>>  
>>>      if (vdev->num_irqs < VFIO_CCW_IO_IRQ_INDEX + 1) {
>>>          error_setg(errp, "vfio: unexpected number of io irqs %u",
>>> @@ -208,72 +205,35 @@ static void vfio_ccw_register_io_notifier(VFIOCCWDevice *vcdev, Error **errp)
>>>          return;
>>>      }
>>>  
>>> -    argsz = sizeof(*irq_info);
>>> -    irq_info = g_malloc0(argsz);
>>> -    irq_info->index = VFIO_CCW_IO_IRQ_INDEX;
>>> -    irq_info->argsz = argsz;
>>> -    if (ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO,
>>> -              irq_info) < 0 || irq_info->count < 1) {
>>> -        error_setg_errno(errp, errno, "vfio: Error getting irq info");
>>> -        goto out_free_info;
>>> -    }
>>> -
>>
>> Don't we still need this hunk?  (And the out_free_info label stuff that
>> cleans it up.)  I don't see vfio_set_irq_signaling() covering it.
> 
> Looks this IRQ index is always implemented and exposed by the driver,
> isn't it? In such a case it shouldn't be needed to test its presence?

Right; if we were running on an old kernel, both ioctl's would fail the
same way since they were added concurrently.  So the check today doesn't
seem very useful.

But since it's there, and we're taking it out, it got me wondering
whether there are/were intentions to expand GET_IRQ_INFO in the future.
I'm not aware of any reason vfio-ccw would need to, but want some
confirmation that I'm not overlooking anything.

Cheers,
Other Eric

> 
> Thanks
> 
> Eric
>>
>>>      if (event_notifier_init(&vcdev->io_notifier, 0)) {
>>>          error_setg_errno(errp, errno,
>>>                           "vfio: Unable to init event notifier for IO");
>>> -        goto out_free_info;
>>> +        return;
>>>      }
>>>  
>>> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
>>> -    irq_set = g_malloc0(argsz);
>>> -    irq_set->argsz = argsz;
>>> -    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
>>> -                     VFIO_IRQ_SET_ACTION_TRIGGER;
>>> -    irq_set->index = VFIO_CCW_IO_IRQ_INDEX;
>>> -    irq_set->start = 0;
>>> -    irq_set->count = 1;
>>> -    pfd = (int32_t *) &irq_set->data;
>>> -
>>> -    *pfd = event_notifier_get_fd(&vcdev->io_notifier);
>>> -    qemu_set_fd_handler(*pfd, vfio_ccw_io_notifier_handler, NULL, vcdev);
>>> -    if (ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
>>> -        error_setg(errp, "vfio: Failed to set up io notification");
>>> -        qemu_set_fd_handler(*pfd, NULL, NULL, vcdev);
>>> +    fd = event_notifier_get_fd(&vcdev->io_notifier);
>>> +    qemu_set_fd_handler(fd, vfio_ccw_io_notifier_handler, NULL, vcdev);
>>> +
>>> +    if (vfio_set_irq_signaling(vdev, VFIO_CCW_IO_IRQ_INDEX, 0,
>>> +                               VFIO_IRQ_SET_ACTION_TRIGGER, fd, errp)) {
>>> +        qemu_set_fd_handler(fd, NULL, NULL, vcdev);
>>
>> This sure looks nice though.  :)
>>
>>>          event_notifier_cleanup(&vcdev->io_notifier);
>>>      }
>>> -
>>> -    g_free(irq_set);
>>> -
>>> -out_free_info:
>>> -    g_free(irq_info);
>>>  }
>>>  
>>>  static void vfio_ccw_unregister_io_notifier(VFIOCCWDevice *vcdev)
>>>  {
>>> -    struct vfio_irq_set *irq_set;
>>> -    size_t argsz;
>>> -    int32_t *pfd;
>>> -
>>> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
>>> -    irq_set = g_malloc0(argsz);
>>> -    irq_set->argsz = argsz;
>>> -    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
>>> -                     VFIO_IRQ_SET_ACTION_TRIGGER;
>>> -    irq_set->index = VFIO_CCW_IO_IRQ_INDEX;
>>> -    irq_set->start = 0;
>>> -    irq_set->count = 1;
>>> -    pfd = (int32_t *) &irq_set->data;
>>> -    *pfd = -1;
>>> -
>>> -    if (ioctl(vcdev->vdev.fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
>>> -        error_report("vfio: Failed to de-assign device io fd: %m");
>>> +    Error *err = NULL;
>>> +
>>> +    vfio_set_irq_signaling(&vcdev->vdev, VFIO_CCW_IO_IRQ_INDEX, 0,
>>> +                           VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err);
>>> +    if (err) {
>>> +        error_reportf_err(err, VFIO_MSG_PREFIX, vcdev->vdev.name);
>>>      }
>>>  
>>>      qemu_set_fd_handler(event_notifier_get_fd(&vcdev->io_notifier),
>>>                          NULL, NULL, vcdev);
>>>      event_notifier_cleanup(&vcdev->io_notifier);
>>> -
>>> -    g_free(irq_set);
>>>  }
>>>  
>>>  static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp)
>>>
>>
>