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