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

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

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