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

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

[Amit Machhiwal]

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,
                                     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);
         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);
         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);
     if (ret) {
-        error_setg_errno(errp, -ret, "Failed to create SPAPR window");
+        error_propagate(errp, local_err);
         return false;
     }
 

base-commit: 53f3a13ac1069975ad47cf8bd05cc96b4ac09962
-- 
2.49.0

[PATCH v2 2/2] vfio/spapr: Fix L2 crash with PCI device passthrough with L2 guest memory > 128G

[Amit Machhiwal]

An L2 KVM guest fails to boot inside a pSeries LPAR when booted with a
memory more than 128 GB and PCI device passthrough. The L2 guest also
crashes when it is booted with a memory greater than 128 GB and a PCI
device is hotplugged later.

The issue arises from a conditional check for `levels > 1` in
`spapr_tce_create_table()` within L1 KVM. This check is meant to prevent
multi-level TCEs, which are not supported by the PowerVM hypervisor. As
a result, when QEMU makes a `VFIO_IOMMU_SPAPR_TCE_CREATE` ioctl call
with `levels > 1`, it triggers the conditional check and returns
`EINVAL`, causing the guest to crash with the following errors:

 2025-03-04T06:36:36.133117Z qemu-system-ppc64: Failed to create a window, ret = -1 (Invalid argument)
 2025-03-04T06:36:36.133176Z qemu-system-ppc64: Failed to create SPAPR window: Invalid argument
 qemu: hardware error: vfio: DMA mapping failed, unable to continue

Fix this by checking the supported DDW "levels" returned by the
VFIO_IOMMU_SPAPR_TCE_GET_INFO ioctl before attempting the TCE create
ioctl in KVM.

The patch has been tested on KVM guests with memory configurations of up
to 390GB, and 450GB on PowerVM and bare-metal environments respectively.

Link: https://lore.kernel.org/qemu-devel/20250404091721.2653539-1-amachhiw@linux.ibm.com/
Signed-off-by: Amit Machhiwal <amachhiw@linux.ibm.com>
---
 hw/vfio/spapr.c | 36 +++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
index 4f2858b43f36..bcc6fe56e76e 100644
--- a/hw/vfio/spapr.c
+++ b/hw/vfio/spapr.c
@@ -26,6 +26,7 @@ typedef struct VFIOSpaprContainer {
     VFIOContainer container;
     MemoryListener prereg_listener;
     QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
+    unsigned int levels;
 } VFIOSpaprContainer;
 
 OBJECT_DECLARE_SIMPLE_TYPE(VFIOSpaprContainer, VFIO_IOMMU_SPAPR);
@@ -236,9 +237,11 @@ static int vfio_spapr_create_window(VFIOContainer *container,
 {
     int ret = 0;
     VFIOContainerBase *bcontainer = &container->bcontainer;
+    VFIOSpaprContainer *scontainer = container_of(container, VFIOSpaprContainer,
+                                                  container);
     IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr);
     uint64_t pagesize = memory_region_iommu_get_min_page_size(iommu_mr), pgmask;
-    unsigned entries, bits_total, bits_per_level, max_levels;
+    unsigned entries, bits_total, bits_per_level, max_levels, ddw_levels;
     struct vfio_iommu_spapr_tce_create create = { .argsz = sizeof(create) };
     long rampagesize = qemu_minrampagesize();
 
@@ -291,16 +294,29 @@ static int vfio_spapr_create_window(VFIOContainer *container,
      */
     bits_per_level = ctz64(qemu_real_host_page_size()) + 8;
     create.levels = bits_total / bits_per_level;
-    if (bits_total % bits_per_level) {
-        ++create.levels;
-    }
-    max_levels = (64 - create.page_shift) / ctz64(qemu_real_host_page_size());
-    for ( ; create.levels <= max_levels; ++create.levels) {
-        ret = ioctl(container->fd, VFIO_IOMMU_SPAPR_TCE_CREATE, &create);
-        if (!ret) {
-            break;
+
+    ddw_levels = scontainer->levels;
+    if (ddw_levels > 1) {
+        if (bits_total % bits_per_level) {
+            ++create.levels;
         }
+        max_levels = (64 - create.page_shift) / ctz64(qemu_real_host_page_size());
+        for ( ; create.levels <= max_levels; ++create.levels) {
+            ret = ioctl(container->fd, VFIO_IOMMU_SPAPR_TCE_CREATE, &create);
+            if (!ret) {
+                break;
+            }
+        }
+    } else { /* ddw_levels == 1 */
+        if (create.levels > ddw_levels) {
+            error_setg(errp, "Host doesn't support multi-level TCE tables"
+                       ". Use larger IO page size. Supported mask is 0x%lx",
+                       bcontainer->pgsizes);
+            return -EINVAL;
+        }
+        ret = ioctl(container->fd, VFIO_IOMMU_SPAPR_TCE_CREATE, &create);
     }
+
     if (ret) {
         error_setg_errno(errp, -ret, "Failed to create a window, ret = %d (%m)", ret);
         return -errno;
@@ -503,6 +519,8 @@ static bool vfio_spapr_container_setup(VFIOContainerBase *bcontainer,
         goto listener_unregister_exit;
     }
 
+    scontainer->levels = info.ddw.levels;
+
     if (v2) {
         bcontainer->pgsizes = info.ddw.pgsizes;
         /*
-- 
2.49.0

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 <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

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

[Amit Machhiwal]

Hi Cédric,

Thanks for taking a look at this patch. Please find my responses below:

On 2025/04/08 08:29 AM, Cédric Le Goater wrote:
> Hello Amit,
> 
> Please use --cover-letter for the next spin.

Sure, will do.

> 
> 
> 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.

Sure, I can make this change in next version.

> 
> >                                       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.

Sure.

> 
> >           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.

Sure.

> 
> >           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()

As per my understanding, for calling error_setg() and friends, the Error **
object has be NULL. If I were to call vfio_spapr_create_window() with errp
instead of the local Error object, that'd result into the below assertion
failure with only the first patch applied and a guest booted with a memory >
128G and PCI device passthrough:

 qemu-system-ppc64: ../util/error.c:68: error_setv: Assertion `*errp == NULL' failed.

This happens because the errp would already be set in vfio_spapr_create_window()
and calling error_setg_errno(errp, ...) in vfio_spapr_container_add_section_window()
would fail as errp is no more NULL. This is the reason I chose to use a local
Error object and later propagate it with errp.

IIUC, what you mean is to pass errp in vfio_spapr_create_window() and just
return from this condition in vfio_spapr_container_add_section_window() but no
need to call error_setg_errno() or error_propagate(). I think that would work.
Please correct me.

Thanks,
Amit

> 
> 
> Thanks,
> 
> C.
> 
> 
> >           return false;
> >       }
> > 
> > base-commit: 53f3a13ac1069975ad47cf8bd05cc96b4ac09962
> 

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

[Cédric Le Goater]

On 4/8/25 11:14, Amit Machhiwal wrote:
> Hi Cédric,
> 
> Thanks for taking a look at this patch. Please find my responses below:
> 
> On 2025/04/08 08:29 AM, Cédric Le Goater wrote:
>> Hello Amit,
>>
>> Please use --cover-letter for the next spin.
> 
> Sure, will do.
> 
>>
>>
>> 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.
> 
> Sure, I can make this change in next version.
> 
>>
>>>                                        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.
> 
> Sure.
> 
>>
>>>            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.
> 
> Sure.
> 
>>
>>>            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()
> 
> As per my understanding, for calling error_setg() and friends, the Error **
> object has be NULL. If I were to call vfio_spapr_create_window() with errp
> instead of the local Error object, that'd result into the below assertion
> failure with only the first patch applied and a guest booted with a memory >
> 128G and PCI device passthrough:
> 
>   qemu-system-ppc64: ../util/error.c:68: error_setv: Assertion `*errp == NULL' failed.
> 
> This happens because the errp would already be set in vfio_spapr_create_window()
> and calling error_setg_errno(errp, ...) in vfio_spapr_container_add_section_window()
> would fail as errp is no more NULL. 

Yes but I don't understand how this can happen.

vfio_spapr_container_add_section_window() calls vfio_spapr_create_window()
and if, in each case of error, error_setg() is called and false returned,
it shouldn't reach the assert. In case of error, the caller *should not*
re-set the 'Error **' parameter, that would trigger the assert.

> This is the reason I chose to use a local
> Error object and later propagate it with errp.
> 
> IIUC, what you mean is to pass errp in vfio_spapr_create_window() and just
> return from this condition in vfio_spapr_container_add_section_window() but no
> need to call error_setg_errno() or error_propagate(). I think that would work.
> Please correct me.

yes.

Once the 'Error **' is set, one should not re-set it again. What you can
do is prepend some string to the returned error, report it or free it.

See Rules section in qapi/error.h for more info.

Thanks,

C.



> 
> Thanks,
> Amit
> 
>>
>>
>> Thanks,
>>
>> C.
>>
>>
>>>            return false;
>>>        }
>>>
>>> base-commit: 53f3a13ac1069975ad47cf8bd05cc96b4ac09962
>>
> 

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

[Amit Machhiwal]

On 2025/04/08 11:46 AM, Cédric Le Goater wrote:
> On 4/8/25 11:14, Amit Machhiwal wrote:
> > Hi Cédric,
> > 
> > Thanks for taking a look at this patch. Please find my responses below:
> > 
> > On 2025/04/08 08:29 AM, Cédric Le Goater wrote:
> > > Hello Amit,
> > > 
> > > Please use --cover-letter for the next spin.
> > 
> > Sure, will do.
> > 
> > > 
> > > 
> > > 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.
> > 
> > Sure, I can make this change in next version.
> > 
> > > 
> > > >                                        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.
> > 
> > Sure.
> > 
> > > 
> > > >            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.
> > 
> > Sure.
> > 
> > > 
> > > >            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()
> > 
> > As per my understanding, for calling error_setg() and friends, the Error **
> > object has be NULL. If I were to call vfio_spapr_create_window() with errp
> > instead of the local Error object, that'd result into the below assertion
> > failure with only the first patch applied and a guest booted with a memory >
> > 128G and PCI device passthrough:
> > 
> >   qemu-system-ppc64: ../util/error.c:68: error_setv: Assertion `*errp == NULL' failed.
> > 
> > This happens because the errp would already be set in vfio_spapr_create_window()
> > and calling error_setg_errno(errp, ...) in vfio_spapr_container_add_section_window()
> > would fail as errp is no more NULL.
> 
> Yes but I don't understand how this can happen.
> 
> vfio_spapr_container_add_section_window() calls vfio_spapr_create_window()
> and if, in each case of error, error_setg() is called and false returned,

I was hitting the assert with the belows change in
vfio_spapr_container_add_section_window() along with the above changes.

diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
index 1a5d1611f2cd..c9cc2278aaa8 100644
--- a/hw/vfio/spapr.c
+++ b/hw/vfio/spapr.c
@@ -378,7 +378,7 @@ vfio_spapr_container_add_section_window(VFIOContainerBase *bcontainer,
     }

     ret = vfio_spapr_create_window(container, section, &pgsize);
-    if (ret) {
+    if (!ret) {
         error_setg_errno(errp, -ret, "Failed to create SPAPR window");
         return false;
     }

> it shouldn't reach the assert. In case of error, the caller *should not*
> re-set the 'Error **' parameter, that would trigger the assert.
> 
> > This is the reason I chose to use a local
> > Error object and later propagate it with errp.
> > 
> > IIUC, what you mean is to pass errp in vfio_spapr_create_window() and just
> > return from this condition in vfio_spapr_container_add_section_window() but no
> > need to call error_setg_errno() or error_propagate(). I think that would work.
> > Please correct me.
> 
> yes.
> 
> Once the 'Error **' is set, one should not re-set it again. What you can
> do is prepend some string to the returned error, report it or free it.

I think following change should be enough along with your suggested changes:

diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
index 1a5d1611f2cd..27fed3cd463c 100644
--- a/hw/vfio/spapr.c
+++ b/hw/vfio/spapr.c
@@ -378,8 +378,7 @@ 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");
+    if (!ret) {
         return false;
     }

I'll be sending a v3 soon:

Thanks,
Amit

> See Rules section in qapi/error.h for more info.
> 
> Thanks,
> 
> C.
> 
> 
> 
> > 
> > Thanks,
> > Amit
> > 
> > > 
> > > 
> > > Thanks,
> > > 
> > > C.
> > > 
> > > 
> > > >            return false;
> > > >        }
> > > > 
> > > > base-commit: 53f3a13ac1069975ad47cf8bd05cc96b4ac09962
> > > 
> > 
> 
> 

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

[Cédric Le Goater]

> I think following change should be enough along with your suggested changes:
> 
> diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
> index 1a5d1611f2cd..27fed3cd463c 100644
> --- a/hw/vfio/spapr.c
> +++ b/hw/vfio/spapr.c
> @@ -378,8 +378,7 @@ 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");
> +    if (!ret) {
>           return false;
>       }

I think you mean :

     if (!vfio_spapr_create_window(container, section, &pgsize, errp)) {
         return false;
     }

if so, yes. This is the current practice in QEMU.

Thanks,

C.