1. Patchew
  2. QEMU
  3. View series

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

Cornelia Huck posted 1 patch 4 years, 10 months ago
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Test FreeBSD passed
Test s390x passed
Test checkpatch passed
Test asan passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190614092705.11025-1-cohuck@redhat.com
Maintainers: Eric Farman <farman@linux.ibm.com>, Farhan Ali <alifm@linux.ibm.com>, Alex Williamson <alex.williamson@redhat.com>, Cornelia Huck <cohuck@redhat.com>
There is a newer version of this series
hw/vfio/ccw.c | 68 +++++++++++----------------------------------------
1 file changed, 14 insertions(+), 54 deletions(-)
[Qemu-devel] [PATCH] vfio-ccw: use vfio_set_irq_signaling
Posted by Cornelia Huck 4 years, 10 months ago 
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;
-    }
-
     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);
         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)
-- 
2.20.1
Re: [Qemu-devel] [PATCH] vfio-ccw: use vfio_set_irq_signaling
Posted by Auger Eric 4 years, 10 months ago 
Hi Connie,

On 6/14/19 11:27 AM, Cornelia Huck wrote:
> Use the new helper.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> ---
>  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;
> -    }
> -
>      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);
>          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)
>
Re: [Qemu-devel] [PATCH] vfio-ccw: use vfio_set_irq_signaling
Posted by Eric Farman 4 years, 10 months ago 

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.

>      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)
>
Re: [Qemu-devel] [PATCH] vfio-ccw: use vfio_set_irq_signaling
Posted by Auger Eric 4 years, 10 months ago 
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?

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)
>>
>
Re: [Qemu-devel] [PATCH] vfio-ccw: use vfio_set_irq_signaling
Posted by Eric Farman 4 years, 10 months ago 

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)
>>>
>>
>
Re: [Qemu-devel] [PATCH] vfio-ccw: use vfio_set_irq_signaling
Posted by Cornelia Huck 4 years, 10 months ago 
On Fri, 14 Jun 2019 11:41:41 -0400
Eric Farman <farman@linux.ibm.com> wrote:

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

It seems I actually was a bit too delete-happy... I did not intend to
rip out that code at all, it just somehow was suddenly gone :)

Checking is probably not strictly needed, but let me send a v2 that
does not delete it (and only converts to the new interface).

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.