| Message ID | 20240514153130.394307-2-clg@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio: Improve error reporting (part 2 | expand |
Hi Cédric, On 5/14/24 17:31, Cédric Le Goater wrote: > We will use the Error object to improve error reporting in the > .log_global*() handlers of VFIO. Add documentation while at it. > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Reviewed-by: Avihai Horon <avihaih@nvidia.com> > Signed-off-by: Cédric Le Goater <clg@redhat.com> > --- > Changes in v5: > > - Fixed typo in set_dirty_page_tracking documentation > > include/hw/vfio/vfio-container-base.h | 18 ++++++++++++++++-- > hw/vfio/common.c | 4 ++-- > hw/vfio/container-base.c | 4 ++-- > hw/vfio/container.c | 6 +++--- > 4 files changed, 23 insertions(+), 9 deletions(-) > > diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h > index 3582d5f97a37877b2adfc0d0b06996c82403f8b7..326ceea52a2030eec9dad289a9845866c4a8c090 100644 > --- a/include/hw/vfio/vfio-container-base.h > +++ b/include/hw/vfio/vfio-container-base.h > @@ -82,7 +82,7 @@ int vfio_container_add_section_window(VFIOContainerBase *bcontainer, > void vfio_container_del_section_window(VFIOContainerBase *bcontainer, > MemoryRegionSection *section); > int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer, > - bool start); > + bool start, Error **errp); I am a bit confused now wrt [PATCH v2 03/11] vfio: Make VFIOIOMMUClass::attach_device() and its wrapper return bool & co Shall we return a bool or a int? Looking at ./include/qapi/error.h I have not found any stringent requirement * - Whenever practical, also return a value that indicates success / * failure. This can make the error checking more concise, and can * avoid useless error object creation and destruction. Note that * we still have many functions returning void. We recommend * • bool-valued functions return true on success / false on failure, * • pointer-valued functions return non-null / null pointer, and * • integer-valued functions return non-negative / negative. > int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer, > VFIOBitmap *vbmap, > hwaddr iova, hwaddr size); > @@ -121,9 +121,23 @@ struct VFIOIOMMUClass { > int (*attach_device)(const char *name, VFIODevice *vbasedev, > AddressSpace *as, Error **errp); > void (*detach_device)(VFIODevice *vbasedev); > + > /* migration feature */ > + > + /** > + * @set_dirty_page_tracking > + * > + * Start or stop dirty pages tracking on VFIO container > + * > + * @bcontainer: #VFIOContainerBase on which to de/activate dirty > + * page tracking > + * @start: indicates whether to start or stop dirty pages tracking > + * @errp: pointer to Error*, to store an error if it happens. > + * > + * Returns zero to indicate success and negative for error > + */ > int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer, > - bool start); > + bool start, Error **errp); > int (*query_dirty_bitmap)(const VFIOContainerBase *bcontainer, > VFIOBitmap *vbmap, > hwaddr iova, hwaddr size); > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 8f9cbdc0264044ce587877a7d19d14b28527291b..485e53916491f1164d29e739fb7106c0c77df737 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -1076,7 +1076,7 @@ static bool vfio_listener_log_global_start(MemoryListener *listener, > if (vfio_devices_all_device_dirty_tracking(bcontainer)) { > ret = vfio_devices_dma_logging_start(bcontainer); > } else { > - ret = vfio_container_set_dirty_page_tracking(bcontainer, true); > + ret = vfio_container_set_dirty_page_tracking(bcontainer, true, NULL); > } > > if (ret) { > @@ -1096,7 +1096,7 @@ static void vfio_listener_log_global_stop(MemoryListener *listener) > if (vfio_devices_all_device_dirty_tracking(bcontainer)) { > vfio_devices_dma_logging_stop(bcontainer); > } else { > - ret = vfio_container_set_dirty_page_tracking(bcontainer, false); > + ret = vfio_container_set_dirty_page_tracking(bcontainer, false, NULL); > } > > if (ret) { > diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c > index 913ae49077c4f09b7b27517c1231cfbe4befb7fb..7c0764121d24b02b6c4e66e368d7dff78a6d65aa 100644 > --- a/hw/vfio/container-base.c > +++ b/hw/vfio/container-base.c > @@ -53,14 +53,14 @@ void vfio_container_del_section_window(VFIOContainerBase *bcontainer, > } > > int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer, > - bool start) > + bool start, Error **errp) > { > if (!bcontainer->dirty_pages_supported) { > return 0; > } > > g_assert(bcontainer->ops->set_dirty_page_tracking); > - return bcontainer->ops->set_dirty_page_tracking(bcontainer, start); > + return bcontainer->ops->set_dirty_page_tracking(bcontainer, start, errp); > } > > int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer, > diff --git a/hw/vfio/container.c b/hw/vfio/container.c > index 77bdec276ec49cb9cd767c0de42ec801b4421572..c35221fbe7dc5453050f97cd186fc958e24f28f7 100644 > --- a/hw/vfio/container.c > +++ b/hw/vfio/container.c > @@ -209,7 +209,7 @@ static int vfio_legacy_dma_map(const VFIOContainerBase *bcontainer, hwaddr iova, > > static int > vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer, > - bool start) > + bool start, Error **errp) > { > const VFIOContainer *container = container_of(bcontainer, VFIOContainer, > bcontainer); > @@ -227,8 +227,8 @@ vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer, > ret = ioctl(container->fd, VFIO_IOMMU_DIRTY_PAGES, &dirty); > if (ret) { > ret = -errno; > - error_report("Failed to set dirty tracking flag 0x%x errno: %d", > - dirty.flags, errno); > + error_setg_errno(errp, errno, "Failed to set dirty tracking flag 0x%x", > + dirty.flags); > } > > return ret; Besides Reviewed-by: Eric Auger <eric.auger@redhat.com> Eric
On 5/15/24 08:40, Eric Auger wrote: > Hi Cédric, > > On 5/14/24 17:31, Cédric Le Goater wrote: >> We will use the Error object to improve error reporting in the >> .log_global*() handlers of VFIO. Add documentation while at it. >> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> Reviewed-by: Avihai Horon <avihaih@nvidia.com> >> Signed-off-by: Cédric Le Goater <clg@redhat.com> >> --- >> Changes in v5: >> >> - Fixed typo in set_dirty_page_tracking documentation >> >> include/hw/vfio/vfio-container-base.h | 18 ++++++++++++++++-- >> hw/vfio/common.c | 4 ++-- >> hw/vfio/container-base.c | 4 ++-- >> hw/vfio/container.c | 6 +++--- >> 4 files changed, 23 insertions(+), 9 deletions(-) >> >> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h >> index 3582d5f97a37877b2adfc0d0b06996c82403f8b7..326ceea52a2030eec9dad289a9845866c4a8c090 100644 >> --- a/include/hw/vfio/vfio-container-base.h >> +++ b/include/hw/vfio/vfio-container-base.h >> @@ -82,7 +82,7 @@ int vfio_container_add_section_window(VFIOContainerBase *bcontainer, >> void vfio_container_del_section_window(VFIOContainerBase *bcontainer, >> MemoryRegionSection *section); >> int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer, >> - bool start); >> + bool start, Error **errp); > I am a bit confused now wrt [PATCH v2 03/11] vfio: Make > VFIOIOMMUClass::attach_device() and its wrapper return bool & co > > Shall we return a bool or a int? It would be better to follow the best practices described in qapi/error.h. Zhenzhong excluded some files from his cleanup series to avoid conflicts with this series of mine. And indeed, I would prefer to merge this one first. It should be addressed later. > > Looking at ./include/qapi/error.h I have not found any stringent requirement > > * - Whenever practical, also return a value that indicates success / > * failure. This can make the error checking more concise, and can > * avoid useless error object creation and destruction. Note that > * we still have many functions returning void. We recommend > * • bool-valued functions return true on success / false on failure, > * • pointer-valued functions return non-null / null pointer, and > * • integer-valued functions return non-negative / negative. > > > >> int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer, >> VFIOBitmap *vbmap, >> hwaddr iova, hwaddr size); >> @@ -121,9 +121,23 @@ struct VFIOIOMMUClass { >> int (*attach_device)(const char *name, VFIODevice *vbasedev, >> AddressSpace *as, Error **errp); >> void (*detach_device)(VFIODevice *vbasedev); >> + >> /* migration feature */ >> + >> + /** >> + * @set_dirty_page_tracking >> + * >> + * Start or stop dirty pages tracking on VFIO container >> + * >> + * @bcontainer: #VFIOContainerBase on which to de/activate dirty >> + * page tracking >> + * @start: indicates whether to start or stop dirty pages tracking >> + * @errp: pointer to Error*, to store an error if it happens. >> + * >> + * Returns zero to indicate success and negative for error >> + */ >> int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer, >> - bool start); >> + bool start, Error **errp); >> int (*query_dirty_bitmap)(const VFIOContainerBase *bcontainer, >> VFIOBitmap *vbmap, >> hwaddr iova, hwaddr size); >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index 8f9cbdc0264044ce587877a7d19d14b28527291b..485e53916491f1164d29e739fb7106c0c77df737 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -1076,7 +1076,7 @@ static bool vfio_listener_log_global_start(MemoryListener *listener, >> if (vfio_devices_all_device_dirty_tracking(bcontainer)) { >> ret = vfio_devices_dma_logging_start(bcontainer); >> } else { >> - ret = vfio_container_set_dirty_page_tracking(bcontainer, true); >> + ret = vfio_container_set_dirty_page_tracking(bcontainer, true, NULL); >> } >> >> if (ret) { >> @@ -1096,7 +1096,7 @@ static void vfio_listener_log_global_stop(MemoryListener *listener) >> if (vfio_devices_all_device_dirty_tracking(bcontainer)) { >> vfio_devices_dma_logging_stop(bcontainer); >> } else { >> - ret = vfio_container_set_dirty_page_tracking(bcontainer, false); >> + ret = vfio_container_set_dirty_page_tracking(bcontainer, false, NULL); >> } >> >> if (ret) { >> diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c >> index 913ae49077c4f09b7b27517c1231cfbe4befb7fb..7c0764121d24b02b6c4e66e368d7dff78a6d65aa 100644 >> --- a/hw/vfio/container-base.c >> +++ b/hw/vfio/container-base.c >> @@ -53,14 +53,14 @@ void vfio_container_del_section_window(VFIOContainerBase *bcontainer, >> } >> >> int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer, >> - bool start) >> + bool start, Error **errp) >> { >> if (!bcontainer->dirty_pages_supported) { >> return 0; >> } >> >> g_assert(bcontainer->ops->set_dirty_page_tracking); >> - return bcontainer->ops->set_dirty_page_tracking(bcontainer, start); >> + return bcontainer->ops->set_dirty_page_tracking(bcontainer, start, errp); >> } >> >> int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer, >> diff --git a/hw/vfio/container.c b/hw/vfio/container.c >> index 77bdec276ec49cb9cd767c0de42ec801b4421572..c35221fbe7dc5453050f97cd186fc958e24f28f7 100644 >> --- a/hw/vfio/container.c >> +++ b/hw/vfio/container.c >> @@ -209,7 +209,7 @@ static int vfio_legacy_dma_map(const VFIOContainerBase *bcontainer, hwaddr iova, >> >> static int >> vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer, >> - bool start) >> + bool start, Error **errp) >> { >> const VFIOContainer *container = container_of(bcontainer, VFIOContainer, >> bcontainer); >> @@ -227,8 +227,8 @@ vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer, >> ret = ioctl(container->fd, VFIO_IOMMU_DIRTY_PAGES, &dirty); >> if (ret) { >> ret = -errno; >> - error_report("Failed to set dirty tracking flag 0x%x errno: %d", >> - dirty.flags, errno); >> + error_setg_errno(errp, errno, "Failed to set dirty tracking flag 0x%x", >> + dirty.flags); >> } >> >> return ret; > > Besides > Reviewed-by: Eric Auger <eric.auger@redhat.com> > > Eric Thanks, C.
diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h index 3582d5f97a37877b2adfc0d0b06996c82403f8b7..326ceea52a2030eec9dad289a9845866c4a8c090 100644 --- a/include/hw/vfio/vfio-container-base.h +++ b/include/hw/vfio/vfio-container-base.h @@ -82,7 +82,7 @@ int vfio_container_add_section_window(VFIOContainerBase *bcontainer, void vfio_container_del_section_window(VFIOContainerBase *bcontainer, MemoryRegionSection *section); int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer, - bool start); + bool start, Error **errp); int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer, VFIOBitmap *vbmap, hwaddr iova, hwaddr size); @@ -121,9 +121,23 @@ struct VFIOIOMMUClass { int (*attach_device)(const char *name, VFIODevice *vbasedev, AddressSpace *as, Error **errp); void (*detach_device)(VFIODevice *vbasedev); + /* migration feature */ + + /** + * @set_dirty_page_tracking + * + * Start or stop dirty pages tracking on VFIO container + * + * @bcontainer: #VFIOContainerBase on which to de/activate dirty + * page tracking + * @start: indicates whether to start or stop dirty pages tracking + * @errp: pointer to Error*, to store an error if it happens. + * + * Returns zero to indicate success and negative for error + */ int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer, - bool start); + bool start, Error **errp); int (*query_dirty_bitmap)(const VFIOContainerBase *bcontainer, VFIOBitmap *vbmap, hwaddr iova, hwaddr size); diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 8f9cbdc0264044ce587877a7d19d14b28527291b..485e53916491f1164d29e739fb7106c0c77df737 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -1076,7 +1076,7 @@ static bool vfio_listener_log_global_start(MemoryListener *listener, if (vfio_devices_all_device_dirty_tracking(bcontainer)) { ret = vfio_devices_dma_logging_start(bcontainer); } else { - ret = vfio_container_set_dirty_page_tracking(bcontainer, true); + ret = vfio_container_set_dirty_page_tracking(bcontainer, true, NULL); } if (ret) { @@ -1096,7 +1096,7 @@ static void vfio_listener_log_global_stop(MemoryListener *listener) if (vfio_devices_all_device_dirty_tracking(bcontainer)) { vfio_devices_dma_logging_stop(bcontainer); } else { - ret = vfio_container_set_dirty_page_tracking(bcontainer, false); + ret = vfio_container_set_dirty_page_tracking(bcontainer, false, NULL); } if (ret) { diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c index 913ae49077c4f09b7b27517c1231cfbe4befb7fb..7c0764121d24b02b6c4e66e368d7dff78a6d65aa 100644 --- a/hw/vfio/container-base.c +++ b/hw/vfio/container-base.c @@ -53,14 +53,14 @@ void vfio_container_del_section_window(VFIOContainerBase *bcontainer, } int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer, - bool start) + bool start, Error **errp) { if (!bcontainer->dirty_pages_supported) { return 0; } g_assert(bcontainer->ops->set_dirty_page_tracking); - return bcontainer->ops->set_dirty_page_tracking(bcontainer, start); + return bcontainer->ops->set_dirty_page_tracking(bcontainer, start, errp); } int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer, diff --git a/hw/vfio/container.c b/hw/vfio/container.c index 77bdec276ec49cb9cd767c0de42ec801b4421572..c35221fbe7dc5453050f97cd186fc958e24f28f7 100644 --- a/hw/vfio/container.c +++ b/hw/vfio/container.c @@ -209,7 +209,7 @@ static int vfio_legacy_dma_map(const VFIOContainerBase *bcontainer, hwaddr iova, static int vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer, - bool start) + bool start, Error **errp) { const VFIOContainer *container = container_of(bcontainer, VFIOContainer, bcontainer); @@ -227,8 +227,8 @@ vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer, ret = ioctl(container->fd, VFIO_IOMMU_DIRTY_PAGES, &dirty); if (ret) { ret = -errno; - error_report("Failed to set dirty tracking flag 0x%x errno: %d", - dirty.flags, errno); + error_setg_errno(errp, errno, "Failed to set dirty tracking flag 0x%x", + dirty.flags); } return ret;