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

["Cédric Le Goater" <clg@redhat.com>]

On 4/8/25 14:40, 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 <clg@redhat.com>
> Signed-off-by: Amit Machhiwal <amachhiw@linux.ibm.com>
> --->   hw/vfio/spapr.c | 33 ++++++++++++++++-----------------
>   1 file changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
> index 1a5d1611f2cd..dd9207679dbe 100644
> --- a/hw/vfio/spapr.c
> +++ b/hw/vfio/spapr.c
> @@ -230,9 +230,9 @@ static int vfio_spapr_remove_window(VFIOContainer *container,
>       return 0;
>   }
>   
> -static int vfio_spapr_create_window(VFIOContainer *container,
> +static bool vfio_spapr_create_window(VFIOContainer *container,
>                                       MemoryRegionSection *section,
> -                                    hwaddr *pgsize)
> +                                    hwaddr *pgsize, Error **errp)
>   {
>       int ret = 0;
>       VFIOContainerBase *bcontainer = &container->bcontainer;
> @@ -252,11 +252,11 @@ 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);
> -        return -EINVAL;
> +        error_setg_errno(errp, EINVAL, "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);
> +        return false;
>       }
>   
>       /*
> @@ -302,17 +302,17 @@ static int vfio_spapr_create_window(VFIOContainer *container,
>           }
>       }
>       if (ret) {
> -        error_report("Failed to create a window, ret = %d (%m)", ret);
> -        return -errno;
> +        error_setg_errno(errp, errno, "Failed to create a window, ret = %d", ret);
> +        return false;
>       }
>   
>       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);
> -        return -EINVAL;
> +        error_setg_errno(errp, EINVAL, "Host doesn't support DMA window at %"HWADDR_PRIx
> +                         ", must be %"PRIx64, section->offset_within_address_space,
> +                         (uint64_t)create.start_addr);
> +        return false;
>       }
>       trace_vfio_spapr_create_window(create.page_shift,
>                                      create.levels,
> @@ -320,7 +320,7 @@ static int vfio_spapr_create_window(VFIOContainer *container,
>                                      create.start_addr);
>       *pgsize = pagesize;
>   
> -    return 0;
> +    return true;
>   }
>   
>   static bool
> @@ -377,9 +377,8 @@ vfio_spapr_container_add_section_window(VFIOContainerBase *bcontainer,
>           }
>       }
>   
> -    ret = vfio_spapr_create_window(container, section, &pgsize);
> -    if (ret) {
> -        error_setg_errno(errp, -ret, "Failed to create SPAPR window");
> +    ret = vfio_spapr_create_window(container, section, &pgsize, errp);
> +    if (!ret) {
>           return false;
>       }
>   

ret is not needed. Minor.


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

Thanks,

C.