Re: [PATCH v5 01/10] vfio: Add Error** argument to .set_dirty_page_tracking() handler

[Cédric Le Goater]

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 <avih...@nvidia.com>

Thanks,

C.

Reviewed-by: Philippe Mathieu-Daudé <phi...@linaro.org>
Signed-off-by: Cédric Le Goater <c...@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