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

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

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 <clg@redhat.com>
> Signed-off-by: Amit Machhiwal <amachhiw@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