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 v3:
- Use error_setg_errno() in vfio_legacy_set_dirty_page_tracking()
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..c76984654a596e3016a8cf833e10143eb872e102 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
+ * pages 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 800ba0aeac84b8dcc83b042bb70c37b4bf78d3f4..5598a508399a6c0b3a20ba17311cbe83d84250c5 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1085,7 +1085,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) {
@@ -1105,7 +1105,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 096d77eac3946a9c38fc2a98116b93353f71f06e..6524575aeddcea8470b5fd10caf57475088d1813 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -210,7 +210,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);
@@ -228,8 +228,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.44.0
Hi Cédric,
On 3/6/24 14:34, Cédric Le Goater wrote:
> We will use the Error object to improve error reporting in the
> .log_global*() handlers of VFIO. Add documentation while at it.
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>
> Changes in v3:
>
> - Use error_setg_errno() in vfio_legacy_set_dirty_page_tracking()
>
> 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..c76984654a596e3016a8cf833e10143eb872e102 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
> + * pages tracking
s/pages/page?
for my education is the "#"VFIOContainerBase formalized somewhere?
> + * @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 800ba0aeac84b8dcc83b042bb70c37b4bf78d3f4..5598a508399a6c0b3a20ba17311cbe83d84250c5 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1085,7 +1085,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);
It is not obvious why we don't pass errp here. Also there is ana
error_report below. Why isn't the error propagated? (not related to your
patch though)
> }
>
> if (ret) {
> @@ -1105,7 +1105,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 096d77eac3946a9c38fc2a98116b93353f71f06e..6524575aeddcea8470b5fd10caf57475088d1813 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -210,7 +210,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);
> @@ -228,8 +228,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;
Thanks
Eric
On 3/7/24 09:09, Eric Auger wrote:
> Hi Cédric,
>
> On 3/6/24 14:34, Cédric Le Goater wrote:
>> We will use the Error object to improve error reporting in the
>> .log_global*() handlers of VFIO. Add documentation while at it.
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>
>> Changes in v3:
>>
>> - Use error_setg_errno() in vfio_legacy_set_dirty_page_tracking()
>>
>> 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..c76984654a596e3016a8cf833e10143eb872e102 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
>> + * pages tracking
> s/pages/page?
yep
> for my education is the "#"VFIOContainerBase formalized somewhere?
It's QEMU specific. See 4cf41794411f ("docs: tweak kernel-doc for QEMU
coding standards").
> + * @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 800ba0aeac84b8dcc83b042bb70c37b4bf78d3f4..5598a508399a6c0b3a20ba17311cbe83d84250c5 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -1085,7 +1085,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);
> It is not obvious why we don't pass errp here. Also there is ana
> error_report below. Why isn't the error propagated? (not related to your
> patch though)
When I started this series, I was trying to find a way to introduce
progressively the changes and this patch is preparing ground for
what is coming next. It could be merged with the following if you prefer.
Thanks,
C.
>> }
>>
>> if (ret) {
>> @@ -1105,7 +1105,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 096d77eac3946a9c38fc2a98116b93353f71f06e..6524575aeddcea8470b5fd10caf57475088d1813 100644
>> --- a/hw/vfio/container.c
>> +++ b/hw/vfio/container.c
>> @@ -210,7 +210,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);
>> @@ -228,8 +228,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;
>
> Thanks
>
> Eric
>
On 3/7/24 13:06, Cédric Le Goater wrote:
> On 3/7/24 09:09, Eric Auger wrote:
>> Hi Cédric,
>>
>> On 3/6/24 14:34, Cédric Le Goater wrote:
>>> We will use the Error object to improve error reporting in the
>>> .log_global*() handlers of VFIO. Add documentation while at it.
>>>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>> ---
>>>
>>> Changes in v3:
>>>
>>> - Use error_setg_errno() in vfio_legacy_set_dirty_page_tracking()
>>> 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..c76984654a596e3016a8cf833e10143eb872e102 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
>>> + * pages tracking
>> s/pages/page?
>
> yep
>
>> for my education is the "#"VFIOContainerBase formalized somewhere?
>
> It's QEMU specific. See 4cf41794411f ("docs: tweak kernel-doc for QEMU
> coding standards").
OK thank you for the education!
>
>> + * @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
>>> 800ba0aeac84b8dcc83b042bb70c37b4bf78d3f4..5598a508399a6c0b3a20ba17311cbe83d84250c5 100644
>>> --- a/hw/vfio/common.c
>>> +++ b/hw/vfio/common.c
>>> @@ -1085,7 +1085,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);
>> It is not obvious why we don't pass errp here. Also there is ana
>> error_report below. Why isn't the error propagated? (not related to your
>> patch though)
>
> When I started this series, I was trying to find a way to introduce
> progressively the changes and this patch is preparing ground for
> what is coming next. It could be merged with the following if you prefer.
up to you or tweek the commit msg
Eric
>
>
> Thanks,
>
> C.
>
>
>
>
>>> }
>>> if (ret) {
>>> @@ -1105,7 +1105,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
>>> 096d77eac3946a9c38fc2a98116b93353f71f06e..6524575aeddcea8470b5fd10caf57475088d1813 100644
>>> --- a/hw/vfio/container.c
>>> +++ b/hw/vfio/container.c
>>> @@ -210,7 +210,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);
>>> @@ -228,8 +228,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;
>>
>> Thanks
>>
>> Eric
>>
>
On 3/8/24 08:39, Eric Auger wrote:
>
>
> On 3/7/24 13:06, Cédric Le Goater wrote:
>> On 3/7/24 09:09, Eric Auger wrote:
>>> Hi Cédric,
>>>
>>> On 3/6/24 14:34, Cédric Le Goater wrote:
>>>> We will use the Error object to improve error reporting in the
>>>> .log_global*() handlers of VFIO. Add documentation while at it.
>>>>
>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>>> ---
>>>>
>>>> Changes in v3:
>>>>
>>>> - Use error_setg_errno() in vfio_legacy_set_dirty_page_tracking()
>>>> 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..c76984654a596e3016a8cf833e10143eb872e102 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
>>>> + * pages tracking
>>> s/pages/page?
>>
>> yep
>>
>>> for my education is the "#"VFIOContainerBase formalized somewhere?
>>
>> It's QEMU specific. See 4cf41794411f ("docs: tweak kernel-doc for QEMU
>> coding standards").
> OK thank you for the education!
Took me a while do understand where it was come from. So you educated
me also :)
>>
>>> + * @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
>>>> 800ba0aeac84b8dcc83b042bb70c37b4bf78d3f4..5598a508399a6c0b3a20ba17311cbe83d84250c5 100644
>>>> --- a/hw/vfio/common.c
>>>> +++ b/hw/vfio/common.c
>>>> @@ -1085,7 +1085,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);
>>> It is not obvious why we don't pass errp here. Also there is ana
>>> error_report below. Why isn't the error propagated? (not related to your
>>> patch though)
>>
>> When I started this series, I was trying to find a way to introduce
>> progressively the changes and this patch is preparing ground for
>> what is coming next. It could be merged with the following if you prefer.
> up to you or tweek the commit msg
ok. Let's get the initial migration part in first and then I will resend the
VFIO part.
Thanks for your time.
C.
© 2016 - 2024 Red Hat, Inc.