Re: [PATCH v2] vfio/pci: Initialize rom_read_failed in vfio_pci_load_rom()

[Philippe Mathieu-Daudé]

Hi Mario,

On 2/6/26 09:08, Mario Casquero wrote:

When vfio_device_get_region_info() fails in vfio_pci_load_rom(),
the function returns without setting vdev->rom_read_failed to true,
and without allocating vdev->rom. This leaves vdev->rom as NULL.

Signed-off-by: Mario Casquero <mcasq...@redhat.com>
Reviewed-by: Cédric Le Goater <c...@redhat.com>
---
  hw/vfio/pci.c | 11 ++++++-----
  1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 9c06b25e63..9df04fa2d7 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1027,7 +1027,7 @@ static void vfio_update_msi(VFIOPCIDevice *vdev)
      }
  }

-static void vfio_pci_load_rom(VFIOPCIDevice *vdev)

+static bool vfio_pci_load_rom(VFIOPCIDevice *vdev)

Since you are modifying this method signature, a bit nicer would be to
have it take an Error* argument, and propagate it. Anyway my 2 cents.

Reviewed-by: Philippe Mathieu-Daudé <phi...@mailo.com>

  {
      VFIODevice *vbasedev = &vdev->vbasedev;
      struct vfio_region_info *reg_info = NULL;
@@ -1041,7 +1041,7 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev)

if (ret != 0) {

          error_report("vfio: Error getting ROM info: %s", strerror(-ret));
-        return;
+        return false;
      }

trace_vfio_pci_load_rom(vbasedev->name, (unsigned long)reg_info->size,

@@ -1052,12 +1052,11 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev)
      vdev->rom_offset = reg_info->offset;

if (!vdev->rom_size) {

-        vdev->rom_read_failed = true;
          error_report("vfio-pci: Cannot read device rom at %s", 
vbasedev->name);
          error_printf("Device option ROM contents are probably invalid "
                      "(check dmesg).\nSkip option ROM probe with rombar=0, "
                      "or load from file with romfile=\n");
-        return;
+        return false;
      }

vdev->rom = g_malloc(size);

@@ -1113,6 +1112,8 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev)
              data[6] = -csum;
          }
      }
+
+    return true;
  }

/* "Raw" read of underlying config space. */

@@ -1146,7 +1147,7 @@ static uint64_t vfio_rom_read(void *opaque, hwaddr addr, 
unsigned size)

/* Load the ROM lazily when the guest tries to read it */

      if (unlikely(!vdev->rom && !vdev->rom_read_failed)) {
-        vfio_pci_load_rom(vdev);
+        vdev->rom_read_failed = !vfio_pci_load_rom(vdev);
      }

memcpy(&val, vdev->rom + addr,