Re: [PATCH 09/14] vfio: add vfio_device_get_irq_info() helper

Cédric Le Goater Wed, 23 Apr 2025 10:17:36 -0700

On 4/9/25 15:48, John Levon wrote:

Add a helper similar to vfio_device_get_region_info() and use it
everywhere.


Replace a couple of needless allocations with stack variables.

As a side-effect, this fixes a minor error reporting issue in the call
from vfio_msix_early_setup().

Signed-off-by: John Levon <john.le...@nutanix.com>




Reviewed-by: Cédric Le Goater <c...@redhat.com>

Some comments below,

--->   hw/vfio/ap.c                  | 19 ++++++++++---------
  hw/vfio/ccw.c                 | 20 +++++++++++---------
  hw/vfio/device.c              | 15 +++++++++++++++
  hw/vfio/pci.c                 | 23 +++++++++++------------
  hw/vfio/platform.c            |  6 +++---
  include/hw/vfio/vfio-device.h |  3 +++
  6 files changed, 53 insertions(+), 33 deletions(-)

diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
index 4af7379d4f..f311bca5b6 100644
--- a/hw/vfio/ap.c
+++ b/hw/vfio/ap.c
@@ -74,10 +74,10 @@ static bool vfio_ap_register_irq_notifier(VFIOAPDevice 
*vapdev,
                                            unsigned int irq, Error **errp)
  {
      int fd;
-    size_t argsz;
+    int ret;
      IOHandler *fd_read;
      EventNotifier *notifier;
-    g_autofree struct vfio_irq_info *irq_info = NULL;
+    struct vfio_irq_info irq_info;
      VFIODevice *vdev = &vapdev->vdev;

switch (irq) {

@@ -96,14 +96,15 @@ static bool vfio_ap_register_irq_notifier(VFIOAPDevice 
*vapdev,
          return false;
      }

- argsz = sizeof(*irq_info);

-    irq_info = g_malloc0(argsz);
-    irq_info->index = irq;
-    irq_info->argsz = argsz;
+    ret = vfio_device_get_irq_info(vdev, irq, &irq_info);
+
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "vfio: Error getting irq info");
+        return false;
+    }

- 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");
+    if (irq_info.count < 1) {
+        error_setg_errno(errp, EINVAL, "vfio: Error getting irq info, 
count=0");


I am not sure using error_setg_errno() is interesting in that case. May be 
simply
use error_setg(). Same below.


Thanks,

C.

          return false;
      }

diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c

index 98aa0000da..dac8769925 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -376,8 +376,8 @@ static bool vfio_ccw_register_irq_notifier(VFIOCCWDevice 
*vcdev,
                                             Error **errp)
  {
      VFIODevice *vdev = &vcdev->vdev;
-    g_autofree struct vfio_irq_info *irq_info = NULL;
-    size_t argsz;
+    struct vfio_irq_info irq_info;
+    int ret;
      int fd;
      EventNotifier *notifier;
      IOHandler *fd_read;
@@ -406,13 +406,15 @@ static bool vfio_ccw_register_irq_notifier(VFIOCCWDevice 
*vcdev,
          return false;
      }

- argsz = sizeof(*irq_info);

-    irq_info = g_malloc0(argsz);
-    irq_info->index = irq;
-    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");
+    ret = vfio_device_get_irq_info(vdev, irq, &irq_info);
+
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "vfio: Error getting irq info");
+        return false;
+    }
+
+    if (irq_info.count < 1) {
+        error_setg_errno(errp, EINVAL, "vfio: Error getting irq info, 
count=0");
          return false;
      }

diff --git a/hw/vfio/device.c b/hw/vfio/device.c

index b9473878fc..2966171118 100644
--- a/hw/vfio/device.c
+++ b/hw/vfio/device.c
@@ -185,6 +185,21 @@ bool vfio_device_irq_set_signaling(VFIODevice *vbasedev, 
int index, int subindex
      return false;
  }

+int vfio_device_get_irq_info(VFIODevice *vbasedev, int index,

+                             struct vfio_irq_info *info)
+{
+    int ret;
+
+    memset(info, 0, sizeof(*info));
+
+    info->argsz = sizeof(*info);
+    info->index = index;
+
+    ret = ioctl(vbasedev->fd, VFIO_DEVICE_GET_IRQ_INFO, info);
+
+    return ret < 0 ? -errno : ret;
+}
+
  int vfio_device_get_region_info(VFIODevice *vbasedev, int index,
                                  struct vfio_region_info **info)
  {
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 090b2f2ef0..ac53c43f2b 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1555,8 +1555,7 @@ static bool vfio_msix_early_setup(VFIOPCIDevice *vdev, 
Error **errp)
      uint16_t ctrl;
      uint32_t table, pba;
      int ret, fd = vdev->vbasedev.fd;
-    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
-                                      .index = VFIO_PCI_MSIX_IRQ_INDEX };
+    struct vfio_irq_info irq_info;
      VFIOMSIXInfo *msix;

pos = pci_find_capability(&vdev->pdev, PCI_CAP_ID_MSIX);

@@ -1593,7 +1592,8 @@ static bool vfio_msix_early_setup(VFIOPCIDevice *vdev, 
Error **errp)
      msix->pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
      msix->entries = (ctrl & PCI_MSIX_FLAGS_QSIZE) + 1;

- ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);

+    ret = vfio_device_get_irq_info(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX,
+                                   &irq_info);
      if (ret < 0) {
          error_setg_errno(errp, -ret, "failed to get MSI-X irq info");
          g_free(msix);
@@ -2737,7 +2737,7 @@ static bool vfio_populate_device(VFIOPCIDevice *vdev, 
Error **errp)
  {
      VFIODevice *vbasedev = &vdev->vbasedev;
      g_autofree struct vfio_region_info *reg_info = NULL;
-    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) };
+    struct vfio_irq_info irq_info;
      int i, ret = -1;

/* Sanity check device */

@@ -2798,12 +2798,10 @@ static bool vfio_populate_device(VFIOPCIDevice *vdev, 
Error **errp)
          }
      }

- irq_info.index = VFIO_PCI_ERR_IRQ_INDEX;

-
-    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
+    ret = vfio_device_get_irq_info(vbasedev, VFIO_PCI_ERR_IRQ_INDEX, 
&irq_info);
      if (ret) {
          /* This can fail for an old kernel or legacy PCI dev */
-        trace_vfio_populate_device_get_irq_info_failure(strerror(errno));
+        trace_vfio_populate_device_get_irq_info_failure(strerror(-ret));
      } else if (irq_info.count == 1) {
          vdev->pci_aer = true;
      } else {
@@ -2912,17 +2910,18 @@ 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 };
+    struct vfio_irq_info irq_info;
      Error *err = NULL;
      int32_t fd;
+    int ret;

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) {
+    ret = vfio_device_get_irq_info(&vdev->vbasedev, VFIO_PCI_REQ_IRQ_INDEX,
+                                   &irq_info);
+    if (ret < 0 || irq_info.count < 1) {
          return;
      }

diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c

index 877d69b7aa..fd176c18a4 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -475,10 +475,10 @@ static bool vfio_populate_device(VFIODevice *vbasedev, 
Error **errp)
      QSIMPLEQ_INIT(&vdev->pending_intp_queue);

for (i = 0; i < vbasedev->num_irqs; i++) {

-        struct vfio_irq_info irq = { .argsz = sizeof(irq) };
+        struct vfio_irq_info irq;
+
+        ret = vfio_device_get_irq_info(vbasedev, i, &irq);

- irq.index = i;

-        ret = ioctl(vbasedev->fd, VFIO_DEVICE_GET_IRQ_INFO, &irq);
          if (ret) {
              error_setg_errno(errp, -ret, "failed to get device irq info");
              goto irq_err;
diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
index 3563a82ede..9522a09c48 100644
--- a/include/hw/vfio/vfio-device.h
+++ b/include/hw/vfio/vfio-device.h
@@ -144,6 +144,9 @@ int vfio_device_get_region_info(VFIODevice *vbasedev, int 
index,
  int vfio_device_get_region_info_type(VFIODevice *vbasedev, uint32_t type,
                                       uint32_t subtype, struct 
vfio_region_info **info);
  bool vfio_device_has_region_cap(VFIODevice *vbasedev, int region, uint16_t 
cap_type);
+
+int vfio_device_get_irq_info(VFIODevice *vbasedev, int index,
+                                struct vfio_irq_info *info);
  #endif

/* Returns 0 on success, or a negative errno. */

Re: [PATCH 09/14] vfio: add vfio_device_get_irq_info() helper

Reply via email to