Re: [Qemu-devel] [PATCH for-4.1] vfio/common: Introduce vfio_set_irq_signaling helper

[Auger Eric]

Hi Alex,

On 5/16/19 12:52 AM, Alex Williamson wrote:
> On Tue,  9 Apr 2019 17:58:31 +0200
> Eric Auger <address@hidden> wrote:
> 
>> The code used to assign an interrupt index/subindex to an
>> eventfd is duplicated many times. Let's introduce an helper that
>> allows to set/unset the signaling for an ACTION_TRIGGER or
>> ACTION_UNMASK action.
>>
>> Signed-off-by: Eric Auger <address@hidden>
>>
>> ---
>>
>> This is a follow-up to
>> [PATCH v2 0/2] vfio-pci: Introduce vfio_set_event_handler().
>> It looks to me that introducing vfio_set_irq_signaling() has more
>> benefits in term of code reduction and the helper abstraction
>> looks cleaner.
>> ---
>>  hw/vfio/common.c              |  61 +++++++++
>>  hw/vfio/pci.c                 | 224 ++++++++--------------------------
>>  hw/vfio/platform.c            |  55 +++------
>>  include/hw/vfio/vfio-common.h |   2 +
>>  4 files changed, 134 insertions(+), 208 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 4374cc6176..f88fd10ca3 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -95,6 +95,67 @@ void vfio_mask_single_irqindex(VFIODevice *vbasedev, int 
>> index)
>>      ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, &irq_set);
>>  }
>>  
>> +static inline const char *action_to_str(int action)
>> +{
>> +    switch (action) {
>> +    case VFIO_IRQ_SET_ACTION_MASK:
>> +        return "MASK";
>> +    case VFIO_IRQ_SET_ACTION_UNMASK:
>> +        return "UNMASK";
>> +    case VFIO_IRQ_SET_ACTION_TRIGGER:
>> +        return "TRIGGER";
>> +    default:
>> +        return "UNKNOWN ACTION";
>> +    }
>> +}
>> +
>> +int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
>> +                           int action, int fd, Error **errp)
>> +{
>> +    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
>> +                                      .index = index };
>> +    struct vfio_irq_set *irq_set;
>> +    int argsz, ret = 0;
>> +    int32_t *pfd;
>> +
>> +    ret = ioctl(vbasedev->fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
>> +    if (ret < 0) {
>> +        error_setg_errno(errp, errno, "index %d does not exist", index);
>> +        goto error;
>> +    }
>> +    if (irq_info.count < subindex + 1) {
>> +        error_setg_errno(errp, errno, "subindex %d does not exist", 
>> subindex);
>> +        goto error;
>> +    }
>> +
>> +    argsz = sizeof(*irq_set) + sizeof(*pfd);
>> +
>> +    irq_set = g_malloc0(argsz);
>> +    irq_set->argsz = argsz;
>> +    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD | action;
>> +    irq_set->index = index;
>> +    irq_set->start = subindex;
>> +    irq_set->count = 1;
>> +    pfd = (int32_t *)&irq_set->data;
>> +    *pfd = fd;
>> +
>> +    ret = ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set);
> 
> Hi Eric,
> 
> Sorry for the long delay.  While I like the code reduction and
> simplification, is it really acceptable that every SET_IRQS ioctl is
> now a GET_IRQ_INFO + SET_IRQS?  Are we trying to protect against
> devices dynamically changing their interrupt support?  Do we not trust
> the callers?
I agree this is generally not needed. I will remove the check.
> 
>> +
>> +    g_free(irq_set);
>> +
>> +    if (ret) {
>> +        error_setg_errno(errp, -ret, "VFIO_DEVICE_SET_IRQS failure");
>> +        goto error;
>> +    }
>> +    return 0;
>> +error:
>> +    error_prepend(errp,
>> +                  "Failed to %s %s eventfd signaling for interrupt [%d,%d]: 
>> ",
>> +                  fd < 0 ? "tear down" : "set up", action_to_str(action),
>> +                  index, subindex);
> 
> 
> Maybe icing on the cake, but this leaves me wishing it printed "MSIX-3"
> rather than "[2,3]" for a PCI device ;)
OK
> 
> 
>> +    return ret;
>> +}
>> +
>>  /*
>>   * IO Port/MMIO - Beware of the endians, VFIO is always little endian
>>   */
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 504019c458..cd93ff6fa3 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
> [snip]
>> @@ -2718,77 +2630,43 @@ static void vfio_req_notifier_handler(void *opaque)
>>  
>>  static void vfio_register_req_notifier(VFIOPCIDevice *vdev)
>>  {
>> -    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
>> -                                      .index = VFIO_PCI_REQ_IRQ_INDEX };
>> -    int argsz;
>> -    struct vfio_irq_set *irq_set;
>> -    int32_t *pfd;
>> +    Error *err = NULL;
>> +    int32_t fd;
>>  
>>      if (!(vdev->features & VFIO_FEATURE_ENABLE_REQ)) {
>>          return;
>>      }
>>  
>> -    if (ioctl(vdev->vbasedev.fd,
>> -              VFIO_DEVICE_GET_IRQ_INFO, &irq_info) < 0 || irq_info.count < 
>> 1) {
>> -        return;
>> -    }
> 
> Here we used GET_IRQ_INFO to quietly only enable the request notifier
> when it's available, now it looks like this is no longer quiet if that
> support is unavailable.  Is that intentional?  Thanks,
not really I acknowledge ;-) I will restore that quiet check here.

Thanks

Eric
> 
> Alex
> 
>> -
>>      if (event_notifier_init(&vdev->req_notifier, 0)) {
>>          error_report("vfio: Unable to init event notifier for device 
>> request");
>>          return;
>>      }
>>  
>> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
>> +    fd = event_notifier_get_fd(&vdev->req_notifier);
>> +    qemu_set_fd_handler(fd, vfio_req_notifier_handler, NULL, vdev);
>>  
>> -    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_PCI_REQ_IRQ_INDEX;
>> -    irq_set->start = 0;
>> -    irq_set->count = 1;
>> -    pfd = (int32_t *)&irq_set->data;
>> -
>> -    *pfd = event_notifier_get_fd(&vdev->req_notifier);
>> -    qemu_set_fd_handler(*pfd, vfio_req_notifier_handler, NULL, vdev);
>> -
>> -    if (ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
>> -        error_report("vfio: Failed to set up device request notification");
>> -        qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
>> +    if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_REQ_IRQ_INDEX, 0,
>> +                           VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
>> +        error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>> +        qemu_set_fd_handler(fd, NULL, NULL, vdev);
>>          event_notifier_cleanup(&vdev->req_notifier);
>>      } else {
>>          vdev->req_enabled = true;
>>      }
>> -
>> -    g_free(irq_set);
>>  }
>