Re: [PATCH v5 01/10] vfio: Add Error** argument to .set_dirty_page_tracking() handler
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.
Re: [PATCH v5 01/10] vfio: Add Error** argument to .set_dirty_page_tracking() handler
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
Thanks,
C.
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Cédric Le Goater
---
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
Re: [PATCH v5 01/10] vfio: Add Error** argument to .set_dirty_page_tracking() handler
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
Thanks,
C.
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Cédric Le Goater
---
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 =
Re: [PATCH v5 01/10] vfio: Add Error** argument to .set_dirty_page_tracking() handler
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
Thanks,
C.
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Cédric Le Goater
---
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
Re: [PATCH v5 01/10] vfio: Add Error** argument to .set_dirty_page_tracking() handler
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
Thanks,
C.
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Cédric Le Goater
---
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
Re: [PATCH v5 01/10] vfio: Add Error** argument to .set_dirty_page_tracking() handler
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
Thanks,
C.
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Cédric Le Goater
---
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
Re: [PATCH v5 01/10] vfio: Add Error** argument to .set_dirty_page_tracking() handler
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
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Cédric Le Goater
---
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
[PATCH v5 01/10] vfio: Add Error** argument to .set_dirty_page_tracking() handler
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é
Signed-off-by: Cédric Le Goater
---
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,
