Re: [PATCH V1 16/26] vfio: return mr from vfio_get_xlat_addr

[Steven Sistare]

On 2/4/2025 10:47 AM, Cédric Le Goater wrote:

+ John (for vfio-user)

On 1/29/25 15:43, Steve Sistare wrote:

Return the memory region that the translated address is found in, for
use in a subsequent patch.  No functional change.

Keeping a reference on this memory region could be risky. What for ?

The returned mr is briefly used here in later patches:

vfio_iommu_map_notify()
  vfio_get_xlat_addr(&mr)
    vfio_container_dma_map(mr->ram_block)   ******
      if ram_block is right
        vioc->dma_map_file()
      else
        vioc->dma_map()

Signed-off-by: Steve Sistare <steven.sist...@oracle.com>
---
  hw/vfio/common.c       | 9 ++++++---
  hw/virtio/vhost-vdpa.c | 2 +-
  include/exec/memory.h  | 5 ++++-
  system/memory.c        | 8 +++++++-
  4 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index db0498e..4bbc29f 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -248,12 +248,13 @@ static bool 
vfio_listener_skipped_section(MemoryRegionSection *section)
  /* Called with rcu_read_lock held.  */
  static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
                                 ram_addr_t *ram_addr, bool *read_only,
+                               MemoryRegion **mr_p,
                                 Error **errp)
  {
      bool ret, mr_has_discard_manager;
      ret = memory_get_xlat_addr(iotlb, vaddr, ram_addr, read_only,
-                               &mr_has_discard_manager, errp);
+                               &mr_has_discard_manager, mr_p, errp);
      if (ret && mr_has_discard_manager) {
          /*
           * Malicious VMs might trigger discarding of IOMMU-mapped memory. The
@@ -300,7 +301,8 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, 
IOMMUTLBEntry *iotlb)
      if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) {
          bool read_only;
-        if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only, &local_err)) {
+        if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only, NULL,
+                                &local_err)) {
              error_report_err(local_err);
              goto out;
          }
@@ -1279,7 +1281,8 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, 
IOMMUTLBEntry *iotlb)
      }
      rcu_read_lock();
-    if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL, &local_err)) {
+    if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL, NULL,
+                            &local_err)) {
          error_report_err(local_err);
          goto out_unlock;
      }
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 3cdaa12..a1866bb 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -228,7 +228,7 @@ static void vhost_vdpa_iommu_map_notify(IOMMUNotifier *n, 
IOMMUTLBEntry *iotlb)
      if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) {
          bool read_only;
-        if (!memory_get_xlat_addr(iotlb, &vaddr, NULL, &read_only, NULL,
+        if (!memory_get_xlat_addr(iotlb, &vaddr, NULL, &read_only, NULL, NULL,
                                    &local_err)) {
              error_report_err(local_err);
              return;
diff --git a/include/exec/memory.h b/include/exec/memory.h
index ea5d33a..a2f1229 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -747,13 +747,16 @@ void 
ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
   * @read_only: indicates if writes are allowed
   * @mr_has_discard_manager: indicates memory is controlled by a
   *                          RamDiscardManager
+ * @mr_p: return the MemoryRegion containing the @iotlb translated addr
   * @errp: pointer to Error*, to store an error if it happens.
   *
   * Return: true on success, else false setting @errp with error.
   */
  bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
                            ram_addr_t *ram_addr, bool *read_only,
-                          bool *mr_has_discard_manager, Error **errp);
+                          bool *mr_has_discard_manager,
+                          MemoryRegion **mr_p,

There is a risk that the life cycle of the returned MemoryRegion
doesn't match VFIO expectations.

Also, it seems that memory_get_xlat_addr() has reached a point
where the callers need refactoring. 'mr_p' would be the 5th out
parameter and 3 of these already depend on the MemoryRegion
returned by flatview_translate().

If we return mr plus xlat, then the caller could trivially derive
vaddr, ram_addr, and read_only.

- Steve

+                          Error **errp);
  typedef struct CoalescedMemoryRange CoalescedMemoryRange;
  typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
diff --git a/system/memory.c b/system/memory.c
index 4c82979..4ec2b8f 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -2185,7 +2185,9 @@ void 
ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
  /* Called with rcu_read_lock held.  */
  bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
                            ram_addr_t *ram_addr, bool *read_only,
-                          bool *mr_has_discard_manager, Error **errp)
+                          bool *mr_has_discard_manager,
+                          MemoryRegion **mr_p,
+                          Error **errp)
  {
      MemoryRegion *mr;
      hwaddr xlat;
@@ -2250,6 +2252,10 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void 
**vaddr,
          *read_only = !writable || mr->readonly;
      }
+    if (mr_p) {
+        *mr_p = mr;
+    }
+
      return true;
  }