| Message ID | 20240516124658.850504-2-clg@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | vfio: Improve error reporting (part 2) | expand |
I had a look at this before I realized it's already in. I'm sending this out not to demand any change, but only to point out an issue to be avoided in future work. Cédric Le Goater <clg@redhat.com> writes: > 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> > Reviewed-by: Eric Auger <eric.auger@redhat.com> > Signed-off-by: Cédric Le Goater <clg@redhat.com> > --- > 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); > 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); Note for later: this is always called via vfio_container_set_dirty_page_tracking(). > 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) { Note for later: all callers pass NULL to ignore the new Error. > 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); Silent improvement: errno is now reported symbolically (like "Operation not permitted") instead of numerically (like "1"). Worth mentioning in the commit message. Since the callers pass NULL to ignore the Error, this error condition is now silent, I believe. I figure you correct this in later patches. If we accept temporary loss of error reporting, the commit message should mention it. Loss of error reporting is easy enough to avoid, though: have the callers pass a pointer to a local @err, and on failure report it with error_report_err(). > } > > return ret;
On 5/29/24 08:26, Markus Armbruster wrote: > I had a look at this before I realized it's already in. I'm sending > this out not to demand any change, but only to point out an issue to be > avoided in future work. > > Cédric Le Goater <clg@redhat.com> writes: > >> 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> >> Reviewed-by: Eric Auger <eric.auger@redhat.com> >> Signed-off-by: Cédric Le Goater <clg@redhat.com> >> --- >> 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); >> 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); > > Note for later: this is always called via > vfio_container_set_dirty_page_tracking(). > >> 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) { > > Note for later: all callers pass NULL to ignore the new Error. > >> 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); > > Silent improvement: errno is now reported symbolically (like "Operation > not permitted") instead of numerically (like "1"). Worth mentioning in > the commit message. ok. > Since the callers pass NULL to ignore the Error, this error condition is > now silent, I believe. > > I figure you correct this in later patches. If we accept temporary loss > of error reporting, the commit message should mention it. Loss of error > reporting is easy enough to avoid, though: have the callers pass a > pointer to a local @err, and on failure report it with > error_report_err(). Yes. The following patch is changing the NULL to an Error pointer. Indeed, I could have merged both patches and resend. Seemed overkill at the time and I preferred the changes to get more exposure too. I did it the way you propose in some of the other patches. I think the code of this series was reshuffled a few times and it fell through the cracks. Let's add some laziness on top of that to be honest. 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;