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>
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
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.
© 2016 - 2026 Red Hat, Inc.