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