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

Wed, 15 May 2024 05:23:44 -0700 


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 <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





























					Reply via email to