| Message ID | 20240506092053.388578-2-clg@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | vfio: Improve error reporting (part 2) | expand |
Hi Cedric, On 06/05/2024 12:20, Cédric Le Goater wrote: > External email: Use caution opening links or attachments > > > We will use the Error object to improve error reporting in the > .log_global*() handlers of VFIO. Add documentation while at it. First of all, I think commit 3688fec8923 ("memory: Add Error** argument to .log_global_start() handler") forgot to set errp in vfio_listener_log_global_start() in case of error. This causes a null pointer de-reference if DPT start fails. Maybe add a fix for that in the beginning of this series, or as a stand-alone fix? Back to this patch, personally, I found the split of patch #1 and #2 a bit confusing. Maybe consider squashing patch #1 and #2 so container based and device based DPT start/stop are changed in the same patch? Like you did in patch #8? Whatever you think is better. In any case: Reviewed-by: Avihai Horon <avihaih@nvidia.com> > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > 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); > 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; > -- > 2.45.0 >
On 5/13/24 15:03, Avihai Horon wrote: > Hi Cedric, > > On 06/05/2024 12:20, Cédric Le Goater wrote: >> External email: Use caution opening links or attachments >> >> >> We will use the Error object to improve error reporting in the >> .log_global*() handlers of VFIO. Add documentation while at it. > > First of all, I think commit 3688fec8923 ("memory: Add Error** argument to .log_global_start() handler") forgot to set errp in vfio_listener_log_global_start() in case of error. yes. This is unfortunate. There has been a few respins, the series was split and I was hoping to upstream this part sooner. My bad. > This causes a null pointer de-reference if DPT start fails. > Maybe add a fix for that in the beginning of this series, or as a stand-alone fix? Since it is fixed by patch 1+2 of this series, we should be fine ? > Back to this patch, personally, I found the split of patch #1 and #2 a bit confusing. > Maybe consider squashing patch #1 and #2 so container based and device based DPT start/stop are changed in the same patch? Like you did in patch #8? > Whatever you think is better. ok. Let's see how v5 goes. I might just send a PR with it if no major changes are requested. > > In any case: > Reviewed-by: Avihai Horon <avihaih@nvidia.com> Thanks, C. > >> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> 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); >> 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; >> -- >> 2.45.0 >> >
On 13/05/2024 19:27, Cédric Le Goater wrote: > External email: Use caution opening links or attachments > > > On 5/13/24 15:03, Avihai Horon wrote: >> Hi Cedric, >> >> On 06/05/2024 12:20, Cédric Le Goater wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> We will use the Error object to improve error reporting in the >>> .log_global*() handlers of VFIO. Add documentation while at it. >> >> First of all, I think commit 3688fec8923 ("memory: Add Error** >> argument to .log_global_start() handler") forgot to set errp in >> vfio_listener_log_global_start() in case of error. > > yes. This is unfortunate. There has been a few respins, the series > was split and I was hoping to upstream this part sooner. My bad. > >> This causes a null pointer de-reference if DPT start fails. >> Maybe add a fix for that in the beginning of this series, or as a >> stand-alone fix? > > Since it is fixed by patch 1+2 of this series, we should be fine ? A fix could be useful to be backported to QEMU 9.0, no? > >> Back to this patch, personally, I found the split of patch #1 and #2 >> a bit confusing. >> Maybe consider squashing patch #1 and #2 so container based and >> device based DPT start/stop are changed in the same patch? Like you >> did in patch #8? >> Whatever you think is better. > > ok. Let's see how v5 goes. I might just send a PR with it if > no major changes are requested. > >> >> In any case: >> Reviewed-by: Avihai Horon <avihaih@nvidia.com> > > > Thanks, > > C. > > >> >>> >>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> 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); >>> 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; >>> -- >>> 2.45.0 >>> >> >
On 5/15/24 14:17, Avihai Horon wrote: > > On 13/05/2024 19:27, Cédric Le Goater wrote: >> External email: Use caution opening links or attachments >> >> >> On 5/13/24 15:03, Avihai Horon wrote: >>> Hi Cedric, >>> >>> On 06/05/2024 12:20, Cédric Le Goater wrote: >>>> External email: Use caution opening links or attachments >>>> >>>> >>>> We will use the Error object to improve error reporting in the >>>> .log_global*() handlers of VFIO. Add documentation while at it. >>> >>> First of all, I think commit 3688fec8923 ("memory: Add Error** argument to .log_global_start() handler") forgot to set errp in vfio_listener_log_global_start() in case of error. >> >> yes. This is unfortunate. There has been a few respins, the series >> was split and I was hoping to upstream this part sooner. My bad. >> >>> This causes a null pointer de-reference if DPT start fails. >>> Maybe add a fix for that in the beginning of this series, or as a stand-alone fix? >> >> Since it is fixed by patch 1+2 of this series, we should be fine ? > > A fix could be useful to be backported to QEMU 9.0, no? These changes were only merged in 9.1 with the migration PR. Am I missing something ? Thanks, C. > >> >>> Back to this patch, personally, I found the split of patch #1 and #2 a bit confusing. >>> Maybe consider squashing patch #1 and #2 so container based and device based DPT start/stop are changed in the same patch? Like you did in patch #8? >>> Whatever you think is better. >> >> ok. Let's see how v5 goes. I might just send a PR with it if >> no major changes are requested. >> >>> >>> In any case: >>> Reviewed-by: Avihai Horon <avihaih@nvidia.com> >> >> >> Thanks, >> >> C. >> >> >>> >>>> >>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>>> 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); >>>> 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; >>>> -- >>>> 2.45.0 >>>> >>> >> >
On 15/05/2024 15:25, Cédric Le Goater wrote: > External email: Use caution opening links or attachments > > > On 5/15/24 14:17, Avihai Horon wrote: >> >> On 13/05/2024 19:27, Cédric Le Goater wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> On 5/13/24 15:03, Avihai Horon wrote: >>>> Hi Cedric, >>>> >>>> On 06/05/2024 12:20, Cédric Le Goater wrote: >>>>> External email: Use caution opening links or attachments >>>>> >>>>> >>>>> We will use the Error object to improve error reporting in the >>>>> .log_global*() handlers of VFIO. Add documentation while at it. >>>> >>>> First of all, I think commit 3688fec8923 ("memory: Add Error** >>>> argument to .log_global_start() handler") forgot to set errp in >>>> vfio_listener_log_global_start() in case of error. >>> >>> yes. This is unfortunate. There has been a few respins, the series >>> was split and I was hoping to upstream this part sooner. My bad. >>> >>>> This causes a null pointer de-reference if DPT start fails. >>>> Maybe add a fix for that in the beginning of this series, or as a >>>> stand-alone fix? >>> >>> Since it is fixed by patch 1+2 of this series, we should be fine ? >> >> A fix could be useful to be backported to QEMU 9.0, no? > > These changes were only merged in 9.1 with the migration PR. > Am I missing something ? Oh, my bad. I thought they were merged in 9.0. So patches 1+2 should cover it. Thanks. > > Thanks, > > C. > > >> >>> >>>> Back to this patch, personally, I found the split of patch #1 and >>>> #2 a bit confusing. >>>> Maybe consider squashing patch #1 and #2 so container based and >>>> device based DPT start/stop are changed in the same patch? Like you >>>> did in patch #8? >>>> Whatever you think is better. >>> >>> ok. Let's see how v5 goes. I might just send a PR with it if >>> no major changes are requested. >>> >>>> >>>> In any case: >>>> Reviewed-by: Avihai Horon <avihaih@nvidia.com> >>> >>> >>> Thanks, >>> >>> C. >>> >>> >>>> >>>>> >>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>>>> 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); >>>>> 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; >>>>> -- >>>>> 2.45.0 >>>>> >>>> >>> >> >
On 5/15/24 14:29, Avihai Horon wrote: > > On 15/05/2024 15:25, Cédric Le Goater wrote: >> External email: Use caution opening links or attachments >> >> >> On 5/15/24 14:17, Avihai Horon wrote: >>> >>> On 13/05/2024 19:27, Cédric Le Goater wrote: >>>> External email: Use caution opening links or attachments >>>> >>>> >>>> On 5/13/24 15:03, Avihai Horon wrote: >>>>> Hi Cedric, >>>>> >>>>> On 06/05/2024 12:20, Cédric Le Goater wrote: >>>>>> External email: Use caution opening links or attachments >>>>>> >>>>>> >>>>>> We will use the Error object to improve error reporting in the >>>>>> .log_global*() handlers of VFIO. Add documentation while at it. >>>>> >>>>> First of all, I think commit 3688fec8923 ("memory: Add Error** argument to .log_global_start() handler") forgot to set errp in vfio_listener_log_global_start() in case of error. >>>> >>>> yes. This is unfortunate. There has been a few respins, the series >>>> was split and I was hoping to upstream this part sooner. My bad. >>>> >>>>> This causes a null pointer de-reference if DPT start fails. >>>>> Maybe add a fix for that in the beginning of this series, or as a stand-alone fix? >>>> >>>> Since it is fixed by patch 1+2 of this series, we should be fine ? >>> >>> A fix could be useful to be backported to QEMU 9.0, no? >> >> These changes were only merged in 9.1 with the migration PR. >> Am I missing something ? > > Oh, my bad. I thought they were merged in 9.0. > So patches 1+2 should cover it. yeah. I still would like to merge them asap, with your little series possibly, the one adding the event, plus the 2 cleanup series from ZhenZhong. I will update vfio-next when they are sufficiently reviewed. Thanks, C. > > Thanks. > >> >> Thanks, >> >> C. >> >> >>> >>>> >>>>> Back to this patch, personally, I found the split of patch #1 and #2 a bit confusing. >>>>> Maybe consider squashing patch #1 and #2 so container based and device based DPT start/stop are changed in the same patch? Like you did in patch #8? >>>>> Whatever you think is better. >>>> >>>> ok. Let's see how v5 goes. I might just send a PR with it if >>>> no major changes are requested. >>>> >>>>> >>>>> In any case: >>>>> Reviewed-by: Avihai Horon <avihaih@nvidia.com> >>>> >>>> >>>> Thanks, >>>> >>>> C. >>>> >>>> >>>>> >>>>>> >>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>>>>> 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); >>>>>> 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; >>>>>> -- >>>>>> 2.45.0 >>>>>> >>>>> >>>> >>> >> >
On 15/05/2024 15:36, Cédric Le Goater wrote: > External email: Use caution opening links or attachments > > > On 5/15/24 14:29, Avihai Horon wrote: >> >> On 15/05/2024 15:25, Cédric Le Goater wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> On 5/15/24 14:17, Avihai Horon wrote: >>>> >>>> On 13/05/2024 19:27, Cédric Le Goater wrote: >>>>> External email: Use caution opening links or attachments >>>>> >>>>> >>>>> On 5/13/24 15:03, Avihai Horon wrote: >>>>>> Hi Cedric, >>>>>> >>>>>> On 06/05/2024 12:20, Cédric Le Goater wrote: >>>>>>> External email: Use caution opening links or attachments >>>>>>> >>>>>>> >>>>>>> We will use the Error object to improve error reporting in the >>>>>>> .log_global*() handlers of VFIO. Add documentation while at it. >>>>>> >>>>>> First of all, I think commit 3688fec8923 ("memory: Add Error** >>>>>> argument to .log_global_start() handler") forgot to set errp in >>>>>> vfio_listener_log_global_start() in case of error. >>>>> >>>>> yes. This is unfortunate. There has been a few respins, the series >>>>> was split and I was hoping to upstream this part sooner. My bad. >>>>> >>>>>> This causes a null pointer de-reference if DPT start fails. >>>>>> Maybe add a fix for that in the beginning of this series, or as a >>>>>> stand-alone fix? >>>>> >>>>> Since it is fixed by patch 1+2 of this series, we should be fine ? >>>> >>>> A fix could be useful to be backported to QEMU 9.0, no? >>> >>> These changes were only merged in 9.1 with the migration PR. >>> Am I missing something ? >> >> Oh, my bad. I thought they were merged in 9.0. >> So patches 1+2 should cover it. > > yeah. I still would like to merge them asap, with your little series > possibly, the one adding the event, plus the 2 cleanup series from > ZhenZhong. I will update vfio-next when they are sufficiently reviewed. > Sure, I am going to post v3 of my series shortly and then review your v6.
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;