Re: [PATCH v2 1/2] vfio/spapr: Enhance error handling in vfio_spapr_create_window()

[Cédric Le Goater]

Hello Amit,

Please use --cover-letter for the next spin.


On 4/7/25 16:31, Amit Machhiwal wrote:

Introduce an Error ** parameter to vfio_spapr_create_window() to enable
structured error reporting. This allows the function to propagate
detailed errors back to callers.

Suggested-by: Cédric Le Goater <c...@redhat.com>
Signed-off-by: Amit Machhiwal <amach...@linux.ibm.com>
---
  hw/vfio/spapr.c | 23 ++++++++++++-----------
  1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
index 1a5d1611f2cd..4f2858b43f36 100644
--- a/hw/vfio/spapr.c
+++ b/hw/vfio/spapr.c
@@ -232,7 +232,7 @@ static int vfio_spapr_remove_window(VFIOContainer 
*container,

static int vfio_spapr_create_window(VFIOContainer *container,

This routine can return a bool since vfio_spapr_container_add_section_window()
does not check the returned errno.

                                      MemoryRegionSection *section,
-                                    hwaddr *pgsize)
+                                    hwaddr *pgsize, Error **errp)
  {
      int ret = 0;
      VFIOContainerBase *bcontainer = &container->bcontainer;
@@ -252,10 +252,10 @@ static int vfio_spapr_create_window(VFIOContainer 
*container,
      pgmask = bcontainer->pgsizes & (pagesize | (pagesize - 1));
      pagesize = pgmask ? (1ULL << (63 - clz64(pgmask))) : 0;
      if (!pagesize) {
-        error_report("Host doesn't support page size 0x%"PRIx64
-                     ", the supported mask is 0x%lx",
-                     memory_region_iommu_get_min_page_size(iommu_mr),
-                     bcontainer->pgsizes);
+        error_setg(errp, "Host doesn't support page size 0x%"PRIx64
+                   ", the supported mask is 0x%lx",
+                   memory_region_iommu_get_min_page_size(iommu_mr),
+                   bcontainer->pgsizes);

This can use error_setg_errno(errp, EINVAL, ... ) instead of
returning -EINVAL.

          return -EINVAL;
      }

@@ -302,16 +302,16 @@ static int vfio_spapr_create_window(VFIOContainer *container,

          }
      }
      if (ret) {
-        error_report("Failed to create a window, ret = %d (%m)", ret);
+        error_setg_errno(errp, -ret, "Failed to create a window, ret = %d 
(%m)", ret);
          return -errno;
      }

if (create.start_addr != section->offset_within_address_space) {

          vfio_spapr_remove_window(container, create.start_addr);

- error_report("Host doesn't support DMA window at %"HWADDR_PRIx", must be %"PRIx64,

-                     section->offset_within_address_space,
-                     (uint64_t)create.start_addr);
+        error_setg(errp, "Host doesn't support DMA window at %"HWADDR_PRIx
+                   ", must be %"PRIx64, section->offset_within_address_space,
+                   (uint64_t)create.start_addr);

This can use error_setg_errno(errp, EINVAL, ... ) instead of
returning -EINVAL.

          return -EINVAL;
      }
      trace_vfio_spapr_create_window(create.page_shift,
@@ -334,6 +334,7 @@ vfio_spapr_container_add_section_window(VFIOContainerBase 
*bcontainer,
                                                    container);
      VFIOHostDMAWindow *hostwin;
      hwaddr pgsize = 0;
+    Error *local_err = NULL;

int ret;> /*

@@ -377,9 +378,9 @@ vfio_spapr_container_add_section_window(VFIOContainerBase 
*bcontainer,
          }
      }

- ret = vfio_spapr_create_window(container, section, &pgsize);

+    ret = vfio_spapr_create_window(container, section, &pgsize, &local_err);

please pass errp instead.

      if (ret) {
-        error_setg_errno(errp, -ret, "Failed to create SPAPR window");
+        error_propagate(errp, local_err);

no need to propagate if errp is passed to vfio_spapr_create_window()


Thanks,

C.

          return false;
      }

base-commit: 53f3a13ac1069975ad47cf8bd05cc96b4ac09962