* [PATCH V5] vfio: return mr from vfio_get_xlat_addr
@ 2025-05-19 13:26 Steve Sistare
2025-05-19 13:46 ` John Levon
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Steve Sistare @ 2025-05-19 13:26 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-Andre Lureau, Thanos Makatos, Daniel P. Berrange,
Paolo Bonzini, Peter Xu, David Hildenbrand, Cedric Le Goater,
Stefano Garzarella, Michael S. Tsirkin, Alex Williamson,
Philippe Mathieu-Daude, John Levon, Steve Sistare
Modify memory_get_xlat_addr and vfio_get_xlat_addr to return the memory
region that the translated address is found in. This will be needed by
CPR in a subsequent patch to map blocks using IOMMU_IOAS_MAP_FILE.
Also return the xlat offset, so we can simplify the interface by removing
the out parameters that can be trivially derived from mr and xlat.
Lastly, rename the functions to to memory_translate_iotlb() and
vfio_translate_iotlb().
Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
Acked-by: David Hildenbrand <david@redhat.com>
---
hw/vfio/listener.c | 33 ++++++++++++++++++++++-----------
hw/virtio/vhost-vdpa.c | 9 +++++++--
include/system/memory.h | 19 +++++++++----------
system/memory.c | 32 +++++++-------------------------
4 files changed, 45 insertions(+), 48 deletions(-)
diff --git a/hw/vfio/listener.c b/hw/vfio/listener.c
index bfacb3d..0afafe3 100644
--- a/hw/vfio/listener.c
+++ b/hw/vfio/listener.c
@@ -90,16 +90,17 @@ static bool vfio_listener_skipped_section(MemoryRegionSection *section)
section->offset_within_address_space & (1ULL << 63);
}
-/* Called with rcu_read_lock held. */
-static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
- ram_addr_t *ram_addr, bool *read_only,
- Error **errp)
+/*
+ * Called with rcu_read_lock held.
+ * The returned MemoryRegion must not be accessed after calling rcu_read_unlock.
+ */
+static MemoryRegion *vfio_translate_iotlb(IOMMUTLBEntry *iotlb, hwaddr *xlat_p,
+ Error **errp)
{
- bool ret, mr_has_discard_manager;
+ MemoryRegion *mr;
- ret = memory_get_xlat_addr(iotlb, vaddr, ram_addr, read_only,
- &mr_has_discard_manager, errp);
- if (ret && mr_has_discard_manager) {
+ mr = memory_translate_iotlb(iotlb, xlat_p, errp);
+ if (mr && memory_region_has_ram_discard_manager(mr)) {
/*
* Malicious VMs might trigger discarding of IOMMU-mapped memory. The
* pages will remain pinned inside vfio until unmapped, resulting in a
@@ -118,7 +119,7 @@ static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
" intended via an IOMMU. It's possible to mitigate "
" by setting/adjusting RLIMIT_MEMLOCK.");
}
- return ret;
+ return mr;
}
static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
@@ -126,6 +127,8 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
VFIOGuestIOMMU *giommu = container_of(n, VFIOGuestIOMMU, n);
VFIOContainerBase *bcontainer = giommu->bcontainer;
hwaddr iova = iotlb->iova + giommu->iommu_offset;
+ MemoryRegion *mr;
+ hwaddr xlat;
void *vaddr;
int ret;
Error *local_err = NULL;
@@ -150,10 +153,14 @@ 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)) {
+ mr = vfio_translate_iotlb(iotlb, &xlat, &local_err);
+ if (!mr) {
error_report_err(local_err);
goto out;
}
+ vaddr = memory_region_get_ram_ptr(mr) + xlat;
+ read_only = !(iotlb->perm & IOMMU_WO) || mr->readonly;
+
/*
* vaddr is only valid until rcu_read_unlock(). But after
* vfio_dma_map has set up the mapping the pages will be
@@ -1010,6 +1017,8 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
ram_addr_t translated_addr;
Error *local_err = NULL;
int ret = -EINVAL;
+ MemoryRegion *mr;
+ ram_addr_t xlat;
trace_vfio_iommu_map_dirty_notify(iova, iova + iotlb->addr_mask);
@@ -1021,9 +1030,11 @@ 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)) {
+ mr = vfio_translate_iotlb(iotlb, &xlat, &local_err);
+ if (!mr) {
goto out_unlock;
}
+ translated_addr = memory_region_get_ram_addr(mr) + xlat;
ret = vfio_container_query_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1,
translated_addr, &local_err);
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 1ab2c11..a1dd9e1 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -209,6 +209,8 @@ static void vhost_vdpa_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
int ret;
Int128 llend;
Error *local_err = NULL;
+ MemoryRegion *mr;
+ hwaddr xlat;
if (iotlb->target_as != &address_space_memory) {
error_report("Wrong target AS \"%s\", only system memory is allowed",
@@ -228,11 +230,14 @@ 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,
- &local_err)) {
+ mr = memory_translate_iotlb(iotlb, &xlat, &local_err);
+ if (!mr) {
error_report_err(local_err);
return;
}
+ vaddr = memory_region_get_ram_ptr(mr) + xlat;
+ read_only = !(iotlb->perm & IOMMU_WO) || mr->readonly;
+
ret = vhost_vdpa_dma_map(s, VHOST_VDPA_GUEST_PA_ASID, iova,
iotlb->addr_mask + 1, vaddr, read_only);
if (ret) {
diff --git a/include/system/memory.h b/include/system/memory.h
index fbbf4cf..13416d7 100644
--- a/include/system/memory.h
+++ b/include/system/memory.h
@@ -738,21 +738,20 @@ void ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
RamDiscardListener *rdl);
/**
- * memory_get_xlat_addr: Extract addresses from a TLB entry
+ * memory_translate_iotlb: Extract addresses from a TLB entry.
+ * Called with rcu_read_lock held.
*
* @iotlb: pointer to an #IOMMUTLBEntry
- * @vaddr: virtual address
- * @ram_addr: RAM address
- * @read_only: indicates if writes are allowed
- * @mr_has_discard_manager: indicates memory is controlled by a
- * RamDiscardManager
+ * @xlat_p: return the offset of the entry from the start of the returned
+ * MemoryRegion.
* @errp: pointer to Error*, to store an error if it happens.
*
- * Return: true on success, else false setting @errp with error.
+ * Return: On success, return the MemoryRegion containing the @iotlb translated
+ * addr. The MemoryRegion must not be accessed after rcu_read_unlock.
+ * On failure, return NULL, 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);
+MemoryRegion *memory_translate_iotlb(IOMMUTLBEntry *iotlb, hwaddr *xlat_p,
+ Error **errp);
typedef struct CoalescedMemoryRange CoalescedMemoryRange;
typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
diff --git a/system/memory.c b/system/memory.c
index 63b983e..306e9ff 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -2174,18 +2174,14 @@ 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)
+MemoryRegion *memory_translate_iotlb(IOMMUTLBEntry *iotlb, hwaddr *xlat_p,
+ Error **errp)
{
MemoryRegion *mr;
hwaddr xlat;
hwaddr len = iotlb->addr_mask + 1;
bool writable = iotlb->perm & IOMMU_WO;
- if (mr_has_discard_manager) {
- *mr_has_discard_manager = false;
- }
/*
* The IOMMU TLB entry we have just covers translation through
* this IOMMU to its immediate target. We need to translate
@@ -2195,7 +2191,7 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
&xlat, &len, writable, MEMTXATTRS_UNSPECIFIED);
if (!memory_region_is_ram(mr)) {
error_setg(errp, "iommu map to non memory area %" HWADDR_PRIx "", xlat);
- return false;
+ return NULL;
} else if (memory_region_has_ram_discard_manager(mr)) {
RamDiscardManager *rdm = memory_region_get_ram_discard_manager(mr);
MemoryRegionSection tmp = {
@@ -2203,9 +2199,6 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
.offset_within_region = xlat,
.size = int128_make64(len),
};
- if (mr_has_discard_manager) {
- *mr_has_discard_manager = true;
- }
/*
* Malicious VMs can map memory into the IOMMU, which is expected
* to remain discarded. vfio will pin all pages, populating memory.
@@ -2216,7 +2209,7 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
error_setg(errp, "iommu map to discarded memory (e.g., unplugged"
" via virtio-mem): %" HWADDR_PRIx "",
iotlb->translated_addr);
- return false;
+ return NULL;
}
}
@@ -2226,22 +2219,11 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
*/
if (len & iotlb->addr_mask) {
error_setg(errp, "iommu has granularity incompatible with target AS");
- return false;
+ return NULL;
}
- if (vaddr) {
- *vaddr = memory_region_get_ram_ptr(mr) + xlat;
- }
-
- if (ram_addr) {
- *ram_addr = memory_region_get_ram_addr(mr) + xlat;
- }
-
- if (read_only) {
- *read_only = !writable || mr->readonly;
- }
-
- return true;
+ *xlat_p = xlat;
+ return mr;
}
void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH V5] vfio: return mr from vfio_get_xlat_addr
2025-05-19 13:26 [PATCH V5] vfio: return mr from vfio_get_xlat_addr Steve Sistare
@ 2025-05-19 13:46 ` John Levon
2025-05-20 6:46 ` Cédric Le Goater
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: John Levon @ 2025-05-19 13:46 UTC (permalink / raw)
To: Steve Sistare
Cc: qemu-devel, Marc-Andre Lureau, Thanos Makatos, Daniel P. Berrange,
Paolo Bonzini, Peter Xu, David Hildenbrand, Cedric Le Goater,
Stefano Garzarella, Michael S. Tsirkin, Alex Williamson,
Philippe Mathieu-Daude
On Mon, May 19, 2025 at 06:26:43AM -0700, Steve Sistare wrote:
> !-------------------------------------------------------------------|
> CAUTION: External Email
>
> |-------------------------------------------------------------------!
>
> Modify memory_get_xlat_addr and vfio_get_xlat_addr to return the memory
> region that the translated address is found in. This will be needed by
> CPR in a subsequent patch to map blocks using IOMMU_IOAS_MAP_FILE.
>
> Also return the xlat offset, so we can simplify the interface by removing
> the out parameters that can be trivially derived from mr and xlat.
>
> Lastly, rename the functions to to memory_translate_iotlb() and
> vfio_translate_iotlb().
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> Acked-by: David Hildenbrand <david@redhat.com>
Reviewed-by: John Levon <john.levon@nutanix.com>
regards
john
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V5] vfio: return mr from vfio_get_xlat_addr
2025-05-19 13:26 [PATCH V5] vfio: return mr from vfio_get_xlat_addr Steve Sistare
2025-05-19 13:46 ` John Levon
@ 2025-05-20 6:46 ` Cédric Le Goater
2025-05-20 7:32 ` Cédric Le Goater
2025-05-26 16:57 ` Cédric Le Goater
3 siblings, 0 replies; 9+ messages in thread
From: Cédric Le Goater @ 2025-05-20 6:46 UTC (permalink / raw)
To: Steve Sistare, qemu-devel
Cc: Marc-Andre Lureau, Thanos Makatos, Daniel P. Berrange,
Paolo Bonzini, Peter Xu, David Hildenbrand, Stefano Garzarella,
Michael S. Tsirkin, Alex Williamson, Philippe Mathieu-Daude,
John Levon
On 5/19/25 15:26, Steve Sistare wrote:
> Modify memory_get_xlat_addr and vfio_get_xlat_addr to return the memory
> region that the translated address is found in. This will be needed by
> CPR in a subsequent patch to map blocks using IOMMU_IOAS_MAP_FILE.
>
> Also return the xlat offset, so we can simplify the interface by removing
> the out parameters that can be trivially derived from mr and xlat.
>
> Lastly, rename the functions to to memory_translate_iotlb() and
> vfio_translate_iotlb().
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> Acked-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
Thanks,
C.
> ---
> hw/vfio/listener.c | 33 ++++++++++++++++++++++-----------
> hw/virtio/vhost-vdpa.c | 9 +++++++--
> include/system/memory.h | 19 +++++++++----------
> system/memory.c | 32 +++++++-------------------------
> 4 files changed, 45 insertions(+), 48 deletions(-)
>
> diff --git a/hw/vfio/listener.c b/hw/vfio/listener.c
> index bfacb3d..0afafe3 100644
> --- a/hw/vfio/listener.c
> +++ b/hw/vfio/listener.c
> @@ -90,16 +90,17 @@ static bool vfio_listener_skipped_section(MemoryRegionSection *section)
> section->offset_within_address_space & (1ULL << 63);
> }
>
> -/* Called with rcu_read_lock held. */
> -static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> - ram_addr_t *ram_addr, bool *read_only,
> - Error **errp)
> +/*
> + * Called with rcu_read_lock held.
> + * The returned MemoryRegion must not be accessed after calling rcu_read_unlock.
> + */
> +static MemoryRegion *vfio_translate_iotlb(IOMMUTLBEntry *iotlb, hwaddr *xlat_p,
> + Error **errp)
> {
> - bool ret, mr_has_discard_manager;
> + MemoryRegion *mr;
>
> - ret = memory_get_xlat_addr(iotlb, vaddr, ram_addr, read_only,
> - &mr_has_discard_manager, errp);
> - if (ret && mr_has_discard_manager) {
> + mr = memory_translate_iotlb(iotlb, xlat_p, errp);
> + if (mr && memory_region_has_ram_discard_manager(mr)) {
> /*
> * Malicious VMs might trigger discarding of IOMMU-mapped memory. The
> * pages will remain pinned inside vfio until unmapped, resulting in a
> @@ -118,7 +119,7 @@ static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> " intended via an IOMMU. It's possible to mitigate "
> " by setting/adjusting RLIMIT_MEMLOCK.");
> }
> - return ret;
> + return mr;
> }
>
> static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> @@ -126,6 +127,8 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> VFIOGuestIOMMU *giommu = container_of(n, VFIOGuestIOMMU, n);
> VFIOContainerBase *bcontainer = giommu->bcontainer;
> hwaddr iova = iotlb->iova + giommu->iommu_offset;
> + MemoryRegion *mr;
> + hwaddr xlat;
> void *vaddr;
> int ret;
> Error *local_err = NULL;
> @@ -150,10 +153,14 @@ 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)) {
> + mr = vfio_translate_iotlb(iotlb, &xlat, &local_err);
> + if (!mr) {
> error_report_err(local_err);
> goto out;
> }
> + vaddr = memory_region_get_ram_ptr(mr) + xlat;
> + read_only = !(iotlb->perm & IOMMU_WO) || mr->readonly;
> +
> /*
> * vaddr is only valid until rcu_read_unlock(). But after
> * vfio_dma_map has set up the mapping the pages will be
> @@ -1010,6 +1017,8 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> ram_addr_t translated_addr;
> Error *local_err = NULL;
> int ret = -EINVAL;
> + MemoryRegion *mr;
> + ram_addr_t xlat;
>
> trace_vfio_iommu_map_dirty_notify(iova, iova + iotlb->addr_mask);
>
> @@ -1021,9 +1030,11 @@ 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)) {
> + mr = vfio_translate_iotlb(iotlb, &xlat, &local_err);
> + if (!mr) {
> goto out_unlock;
> }
> + translated_addr = memory_region_get_ram_addr(mr) + xlat;
>
> ret = vfio_container_query_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1,
> translated_addr, &local_err);
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 1ab2c11..a1dd9e1 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -209,6 +209,8 @@ static void vhost_vdpa_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> int ret;
> Int128 llend;
> Error *local_err = NULL;
> + MemoryRegion *mr;
> + hwaddr xlat;
>
> if (iotlb->target_as != &address_space_memory) {
> error_report("Wrong target AS \"%s\", only system memory is allowed",
> @@ -228,11 +230,14 @@ 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,
> - &local_err)) {
> + mr = memory_translate_iotlb(iotlb, &xlat, &local_err);
> + if (!mr) {
> error_report_err(local_err);
> return;
> }
> + vaddr = memory_region_get_ram_ptr(mr) + xlat;
> + read_only = !(iotlb->perm & IOMMU_WO) || mr->readonly;
> +
> ret = vhost_vdpa_dma_map(s, VHOST_VDPA_GUEST_PA_ASID, iova,
> iotlb->addr_mask + 1, vaddr, read_only);
> if (ret) {
> diff --git a/include/system/memory.h b/include/system/memory.h
> index fbbf4cf..13416d7 100644
> --- a/include/system/memory.h
> +++ b/include/system/memory.h
> @@ -738,21 +738,20 @@ void ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
> RamDiscardListener *rdl);
>
> /**
> - * memory_get_xlat_addr: Extract addresses from a TLB entry
> + * memory_translate_iotlb: Extract addresses from a TLB entry.
> + * Called with rcu_read_lock held.
> *
> * @iotlb: pointer to an #IOMMUTLBEntry
> - * @vaddr: virtual address
> - * @ram_addr: RAM address
> - * @read_only: indicates if writes are allowed
> - * @mr_has_discard_manager: indicates memory is controlled by a
> - * RamDiscardManager
> + * @xlat_p: return the offset of the entry from the start of the returned
> + * MemoryRegion.
> * @errp: pointer to Error*, to store an error if it happens.
> *
> - * Return: true on success, else false setting @errp with error.
> + * Return: On success, return the MemoryRegion containing the @iotlb translated
> + * addr. The MemoryRegion must not be accessed after rcu_read_unlock.
> + * On failure, return NULL, 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);
> +MemoryRegion *memory_translate_iotlb(IOMMUTLBEntry *iotlb, hwaddr *xlat_p,
> + Error **errp);
>
> typedef struct CoalescedMemoryRange CoalescedMemoryRange;
> typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
> diff --git a/system/memory.c b/system/memory.c
> index 63b983e..306e9ff 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -2174,18 +2174,14 @@ 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)
> +MemoryRegion *memory_translate_iotlb(IOMMUTLBEntry *iotlb, hwaddr *xlat_p,
> + Error **errp)
> {
> MemoryRegion *mr;
> hwaddr xlat;
> hwaddr len = iotlb->addr_mask + 1;
> bool writable = iotlb->perm & IOMMU_WO;
>
> - if (mr_has_discard_manager) {
> - *mr_has_discard_manager = false;
> - }
> /*
> * The IOMMU TLB entry we have just covers translation through
> * this IOMMU to its immediate target. We need to translate
> @@ -2195,7 +2191,7 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> &xlat, &len, writable, MEMTXATTRS_UNSPECIFIED);
> if (!memory_region_is_ram(mr)) {
> error_setg(errp, "iommu map to non memory area %" HWADDR_PRIx "", xlat);
> - return false;
> + return NULL;
> } else if (memory_region_has_ram_discard_manager(mr)) {
> RamDiscardManager *rdm = memory_region_get_ram_discard_manager(mr);
> MemoryRegionSection tmp = {
> @@ -2203,9 +2199,6 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> .offset_within_region = xlat,
> .size = int128_make64(len),
> };
> - if (mr_has_discard_manager) {
> - *mr_has_discard_manager = true;
> - }
> /*
> * Malicious VMs can map memory into the IOMMU, which is expected
> * to remain discarded. vfio will pin all pages, populating memory.
> @@ -2216,7 +2209,7 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> error_setg(errp, "iommu map to discarded memory (e.g., unplugged"
> " via virtio-mem): %" HWADDR_PRIx "",
> iotlb->translated_addr);
> - return false;
> + return NULL;
> }
> }
>
> @@ -2226,22 +2219,11 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> */
> if (len & iotlb->addr_mask) {
> error_setg(errp, "iommu has granularity incompatible with target AS");
> - return false;
> + return NULL;
> }
>
> - if (vaddr) {
> - *vaddr = memory_region_get_ram_ptr(mr) + xlat;
> - }
> -
> - if (ram_addr) {
> - *ram_addr = memory_region_get_ram_addr(mr) + xlat;
> - }
> -
> - if (read_only) {
> - *read_only = !writable || mr->readonly;
> - }
> -
> - return true;
> + *xlat_p = xlat;
> + return mr;
> }
>
> void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V5] vfio: return mr from vfio_get_xlat_addr
2025-05-19 13:26 [PATCH V5] vfio: return mr from vfio_get_xlat_addr Steve Sistare
2025-05-19 13:46 ` John Levon
2025-05-20 6:46 ` Cédric Le Goater
@ 2025-05-20 7:32 ` Cédric Le Goater
2025-05-20 13:42 ` Steven Sistare
2025-05-26 16:57 ` Cédric Le Goater
3 siblings, 1 reply; 9+ messages in thread
From: Cédric Le Goater @ 2025-05-20 7:32 UTC (permalink / raw)
To: Steve Sistare, qemu-devel
Cc: Marc-Andre Lureau, Thanos Makatos, Daniel P. Berrange,
Paolo Bonzini, Peter Xu, David Hildenbrand, Stefano Garzarella,
Michael S. Tsirkin, Alex Williamson, Philippe Mathieu-Daude,
John Levon
On 5/19/25 15:26, Steve Sistare wrote:
> Modify memory_get_xlat_addr and vfio_get_xlat_addr to return the memory
> region that the translated address is found in. This will be needed by
> CPR in a subsequent patch to map blocks using IOMMU_IOAS_MAP_FILE.
>
> Also return the xlat offset, so we can simplify the interface by removing
> the out parameters that can be trivially derived from mr and xlat.
>
> Lastly, rename the functions to to memory_translate_iotlb() and
> vfio_translate_iotlb().
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> Acked-by: David Hildenbrand <david@redhat.com>
> ---
> hw/vfio/listener.c | 33 ++++++++++++++++++++++-----------
> hw/virtio/vhost-vdpa.c | 9 +++++++--
> include/system/memory.h | 19 +++++++++----------
> system/memory.c | 32 +++++++-------------------------
> 4 files changed, 45 insertions(+), 48 deletions(-)
>
> diff --git a/hw/vfio/listener.c b/hw/vfio/listener.c
> index bfacb3d..0afafe3 100644
> --- a/hw/vfio/listener.c
> +++ b/hw/vfio/listener.c
> @@ -90,16 +90,17 @@ static bool vfio_listener_skipped_section(MemoryRegionSection *section)
> section->offset_within_address_space & (1ULL << 63);
> }
>
> -/* Called with rcu_read_lock held. */
> -static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> - ram_addr_t *ram_addr, bool *read_only,
> - Error **errp)
> +/*
> + * Called with rcu_read_lock held.
> + * The returned MemoryRegion must not be accessed after calling rcu_read_unlock.
> + */
> +static MemoryRegion *vfio_translate_iotlb(IOMMUTLBEntry *iotlb, hwaddr *xlat_p,
> + Error **errp)
> {
> - bool ret, mr_has_discard_manager;
> + MemoryRegion *mr;
>
> - ret = memory_get_xlat_addr(iotlb, vaddr, ram_addr, read_only,
> - &mr_has_discard_manager, errp);
> - if (ret && mr_has_discard_manager) {
> + mr = memory_translate_iotlb(iotlb, xlat_p, errp);
> + if (mr && memory_region_has_ram_discard_manager(mr)) {
> /*
> * Malicious VMs might trigger discarding of IOMMU-mapped memory. The
> * pages will remain pinned inside vfio until unmapped, resulting in a
> @@ -118,7 +119,7 @@ static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> " intended via an IOMMU. It's possible to mitigate "
> " by setting/adjusting RLIMIT_MEMLOCK.");
> }
> - return ret;
> + return mr;
> }
>
> static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> @@ -126,6 +127,8 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> VFIOGuestIOMMU *giommu = container_of(n, VFIOGuestIOMMU, n);
> VFIOContainerBase *bcontainer = giommu->bcontainer;
> hwaddr iova = iotlb->iova + giommu->iommu_offset;
> + MemoryRegion *mr;
> + hwaddr xlat;
> void *vaddr;
> int ret;
> Error *local_err = NULL;
> @@ -150,10 +153,14 @@ 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)) {
> + mr = vfio_translate_iotlb(iotlb, &xlat, &local_err);
> + if (!mr) {
> error_report_err(local_err);
> goto out;
> }
> + vaddr = memory_region_get_ram_ptr(mr) + xlat;
> + read_only = !(iotlb->perm & IOMMU_WO) || mr->readonly;
> +
> /*
> * vaddr is only valid until rcu_read_unlock(). But after
> * vfio_dma_map has set up the mapping the pages will be
> @@ -1010,6 +1017,8 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> ram_addr_t translated_addr;
> Error *local_err = NULL;
> int ret = -EINVAL;
> + MemoryRegion *mr;
> + ram_addr_t xlat;
xlat should be :
hwaddr xlat;
Thanks,
C.
>
> trace_vfio_iommu_map_dirty_notify(iova, iova + iotlb->addr_mask);
>
> @@ -1021,9 +1030,11 @@ 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)) {
> + mr = vfio_translate_iotlb(iotlb, &xlat, &local_err);
> + if (!mr) {
> goto out_unlock;
> }
> + translated_addr = memory_region_get_ram_addr(mr) + xlat;
>
> ret = vfio_container_query_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1,
> translated_addr, &local_err);
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 1ab2c11..a1dd9e1 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -209,6 +209,8 @@ static void vhost_vdpa_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> int ret;
> Int128 llend;
> Error *local_err = NULL;
> + MemoryRegion *mr;
> + hwaddr xlat;
>
> if (iotlb->target_as != &address_space_memory) {
> error_report("Wrong target AS \"%s\", only system memory is allowed",
> @@ -228,11 +230,14 @@ 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,
> - &local_err)) {
> + mr = memory_translate_iotlb(iotlb, &xlat, &local_err);
> + if (!mr) {
> error_report_err(local_err);
> return;
> }
> + vaddr = memory_region_get_ram_ptr(mr) + xlat;
> + read_only = !(iotlb->perm & IOMMU_WO) || mr->readonly;
> +
> ret = vhost_vdpa_dma_map(s, VHOST_VDPA_GUEST_PA_ASID, iova,
> iotlb->addr_mask + 1, vaddr, read_only);
> if (ret) {
> diff --git a/include/system/memory.h b/include/system/memory.h
> index fbbf4cf..13416d7 100644
> --- a/include/system/memory.h
> +++ b/include/system/memory.h
> @@ -738,21 +738,20 @@ void ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
> RamDiscardListener *rdl);
>
> /**
> - * memory_get_xlat_addr: Extract addresses from a TLB entry
> + * memory_translate_iotlb: Extract addresses from a TLB entry.
> + * Called with rcu_read_lock held.
> *
> * @iotlb: pointer to an #IOMMUTLBEntry
> - * @vaddr: virtual address
> - * @ram_addr: RAM address
> - * @read_only: indicates if writes are allowed
> - * @mr_has_discard_manager: indicates memory is controlled by a
> - * RamDiscardManager
> + * @xlat_p: return the offset of the entry from the start of the returned
> + * MemoryRegion.
> * @errp: pointer to Error*, to store an error if it happens.
> *
> - * Return: true on success, else false setting @errp with error.
> + * Return: On success, return the MemoryRegion containing the @iotlb translated
> + * addr. The MemoryRegion must not be accessed after rcu_read_unlock.
> + * On failure, return NULL, 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);
> +MemoryRegion *memory_translate_iotlb(IOMMUTLBEntry *iotlb, hwaddr *xlat_p,
> + Error **errp);
>
> typedef struct CoalescedMemoryRange CoalescedMemoryRange;
> typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
> diff --git a/system/memory.c b/system/memory.c
> index 63b983e..306e9ff 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -2174,18 +2174,14 @@ 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)
> +MemoryRegion *memory_translate_iotlb(IOMMUTLBEntry *iotlb, hwaddr *xlat_p,
> + Error **errp)
> {
> MemoryRegion *mr;
> hwaddr xlat;
> hwaddr len = iotlb->addr_mask + 1;
> bool writable = iotlb->perm & IOMMU_WO;
>
> - if (mr_has_discard_manager) {
> - *mr_has_discard_manager = false;
> - }
> /*
> * The IOMMU TLB entry we have just covers translation through
> * this IOMMU to its immediate target. We need to translate
> @@ -2195,7 +2191,7 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> &xlat, &len, writable, MEMTXATTRS_UNSPECIFIED);
> if (!memory_region_is_ram(mr)) {
> error_setg(errp, "iommu map to non memory area %" HWADDR_PRIx "", xlat);
> - return false;
> + return NULL;
> } else if (memory_region_has_ram_discard_manager(mr)) {
> RamDiscardManager *rdm = memory_region_get_ram_discard_manager(mr);
> MemoryRegionSection tmp = {
> @@ -2203,9 +2199,6 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> .offset_within_region = xlat,
> .size = int128_make64(len),
> };
> - if (mr_has_discard_manager) {
> - *mr_has_discard_manager = true;
> - }
> /*
> * Malicious VMs can map memory into the IOMMU, which is expected
> * to remain discarded. vfio will pin all pages, populating memory.
> @@ -2216,7 +2209,7 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> error_setg(errp, "iommu map to discarded memory (e.g., unplugged"
> " via virtio-mem): %" HWADDR_PRIx "",
> iotlb->translated_addr);
> - return false;
> + return NULL;
> }
> }
>
> @@ -2226,22 +2219,11 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> */
> if (len & iotlb->addr_mask) {
> error_setg(errp, "iommu has granularity incompatible with target AS");
> - return false;
> + return NULL;
> }
>
> - if (vaddr) {
> - *vaddr = memory_region_get_ram_ptr(mr) + xlat;
> - }
> -
> - if (ram_addr) {
> - *ram_addr = memory_region_get_ram_addr(mr) + xlat;
> - }
> -
> - if (read_only) {
> - *read_only = !writable || mr->readonly;
> - }
> -
> - return true;
> + *xlat_p = xlat;
> + return mr;
> }
>
> void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V5] vfio: return mr from vfio_get_xlat_addr
2025-05-20 7:32 ` Cédric Le Goater
@ 2025-05-20 13:42 ` Steven Sistare
2025-05-20 13:46 ` Cédric Le Goater
0 siblings, 1 reply; 9+ messages in thread
From: Steven Sistare @ 2025-05-20 13:42 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: Marc-Andre Lureau, Thanos Makatos, Daniel P. Berrange,
Paolo Bonzini, Peter Xu, David Hildenbrand, Stefano Garzarella,
Michael S. Tsirkin, Alex Williamson, Philippe Mathieu-Daude,
John Levon
On 5/20/2025 3:32 AM, Cédric Le Goater wrote:
> On 5/19/25 15:26, Steve Sistare wrote:
>> Modify memory_get_xlat_addr and vfio_get_xlat_addr to return the memory
>> region that the translated address is found in. This will be needed by
>> CPR in a subsequent patch to map blocks using IOMMU_IOAS_MAP_FILE.
>>
>> Also return the xlat offset, so we can simplify the interface by removing
>> the out parameters that can be trivially derived from mr and xlat.
>>
>> Lastly, rename the functions to to memory_translate_iotlb() and
>> vfio_translate_iotlb().
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> Acked-by: David Hildenbrand <david@redhat.com>
>> ---
>> hw/vfio/listener.c | 33 ++++++++++++++++++++++-----------
>> hw/virtio/vhost-vdpa.c | 9 +++++++--
>> include/system/memory.h | 19 +++++++++----------
>> system/memory.c | 32 +++++++-------------------------
>> 4 files changed, 45 insertions(+), 48 deletions(-)
>>
>> diff --git a/hw/vfio/listener.c b/hw/vfio/listener.c
>> index bfacb3d..0afafe3 100644
>> --- a/hw/vfio/listener.c
>> +++ b/hw/vfio/listener.c
>> @@ -90,16 +90,17 @@ static bool vfio_listener_skipped_section(MemoryRegionSection *section)
>> section->offset_within_address_space & (1ULL << 63);
>> }
>> -/* Called with rcu_read_lock held. */
>> -static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
>> - ram_addr_t *ram_addr, bool *read_only,
>> - Error **errp)
>> +/*
>> + * Called with rcu_read_lock held.
>> + * The returned MemoryRegion must not be accessed after calling rcu_read_unlock.
>> + */
>> +static MemoryRegion *vfio_translate_iotlb(IOMMUTLBEntry *iotlb, hwaddr *xlat_p,
>> + Error **errp)
>> {
>> - bool ret, mr_has_discard_manager;
>> + MemoryRegion *mr;
>> - ret = memory_get_xlat_addr(iotlb, vaddr, ram_addr, read_only,
>> - &mr_has_discard_manager, errp);
>> - if (ret && mr_has_discard_manager) {
>> + mr = memory_translate_iotlb(iotlb, xlat_p, errp);
>> + if (mr && memory_region_has_ram_discard_manager(mr)) {
>> /*
>> * Malicious VMs might trigger discarding of IOMMU-mapped memory. The
>> * pages will remain pinned inside vfio until unmapped, resulting in a
>> @@ -118,7 +119,7 @@ static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
>> " intended via an IOMMU. It's possible to mitigate "
>> " by setting/adjusting RLIMIT_MEMLOCK.");
>> }
>> - return ret;
>> + return mr;
>> }
>> static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>> @@ -126,6 +127,8 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>> VFIOGuestIOMMU *giommu = container_of(n, VFIOGuestIOMMU, n);
>> VFIOContainerBase *bcontainer = giommu->bcontainer;
>> hwaddr iova = iotlb->iova + giommu->iommu_offset;
>> + MemoryRegion *mr;
>> + hwaddr xlat;
>> void *vaddr;
>> int ret;
>> Error *local_err = NULL;
>> @@ -150,10 +153,14 @@ 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)) {
>> + mr = vfio_translate_iotlb(iotlb, &xlat, &local_err);
>> + if (!mr) {
>> error_report_err(local_err);
>> goto out;
>> }
>> + vaddr = memory_region_get_ram_ptr(mr) + xlat;
>> + read_only = !(iotlb->perm & IOMMU_WO) || mr->readonly;
>> +
>> /*
>> * vaddr is only valid until rcu_read_unlock(). But after
>> * vfio_dma_map has set up the mapping the pages will be
>> @@ -1010,6 +1017,8 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>> ram_addr_t translated_addr;
>> Error *local_err = NULL;
>> int ret = -EINVAL;
>> + MemoryRegion *mr;
>> + ram_addr_t xlat;
>
> xlat should be :
>
> hwaddr xlat;
Will you make that change when you pull, or do you want me to submit a V6
with this and the RB's?
- Steve
trace_vfio_iommu_map_dirty_notify(iova, iova + iotlb->addr_mask);
>> @@ -1021,9 +1030,11 @@ 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)) {
>> + mr = vfio_translate_iotlb(iotlb, &xlat, &local_err);
>> + if (!mr) {
>> goto out_unlock;
>> }
>> + translated_addr = memory_region_get_ram_addr(mr) + xlat;
>> ret = vfio_container_query_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1,
>> translated_addr, &local_err);
>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>> index 1ab2c11..a1dd9e1 100644
>> --- a/hw/virtio/vhost-vdpa.c
>> +++ b/hw/virtio/vhost-vdpa.c
>> @@ -209,6 +209,8 @@ static void vhost_vdpa_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>> int ret;
>> Int128 llend;
>> Error *local_err = NULL;
>> + MemoryRegion *mr;
>> + hwaddr xlat;
>> if (iotlb->target_as != &address_space_memory) {
>> error_report("Wrong target AS \"%s\", only system memory is allowed",
>> @@ -228,11 +230,14 @@ 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,
>> - &local_err)) {
>> + mr = memory_translate_iotlb(iotlb, &xlat, &local_err);
>> + if (!mr) {
>> error_report_err(local_err);
>> return;
>> }
>> + vaddr = memory_region_get_ram_ptr(mr) + xlat;
>> + read_only = !(iotlb->perm & IOMMU_WO) || mr->readonly;
>> +
>> ret = vhost_vdpa_dma_map(s, VHOST_VDPA_GUEST_PA_ASID, iova,
>> iotlb->addr_mask + 1, vaddr, read_only);
>> if (ret) {
>> diff --git a/include/system/memory.h b/include/system/memory.h
>> index fbbf4cf..13416d7 100644
>> --- a/include/system/memory.h
>> +++ b/include/system/memory.h
>> @@ -738,21 +738,20 @@ void ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
>> RamDiscardListener *rdl);
>> /**
>> - * memory_get_xlat_addr: Extract addresses from a TLB entry
>> + * memory_translate_iotlb: Extract addresses from a TLB entry.
>> + * Called with rcu_read_lock held.
>> *
>> * @iotlb: pointer to an #IOMMUTLBEntry
>> - * @vaddr: virtual address
>> - * @ram_addr: RAM address
>> - * @read_only: indicates if writes are allowed
>> - * @mr_has_discard_manager: indicates memory is controlled by a
>> - * RamDiscardManager
>> + * @xlat_p: return the offset of the entry from the start of the returned
>> + * MemoryRegion.
>> * @errp: pointer to Error*, to store an error if it happens.
>> *
>> - * Return: true on success, else false setting @errp with error.
>> + * Return: On success, return the MemoryRegion containing the @iotlb translated
>> + * addr. The MemoryRegion must not be accessed after rcu_read_unlock.
>> + * On failure, return NULL, 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);
>> +MemoryRegion *memory_translate_iotlb(IOMMUTLBEntry *iotlb, hwaddr *xlat_p,
>> + Error **errp);
>> typedef struct CoalescedMemoryRange CoalescedMemoryRange;
>> typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
>> diff --git a/system/memory.c b/system/memory.c
>> index 63b983e..306e9ff 100644
>> --- a/system/memory.c
>> +++ b/system/memory.c
>> @@ -2174,18 +2174,14 @@ 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)
>> +MemoryRegion *memory_translate_iotlb(IOMMUTLBEntry *iotlb, hwaddr *xlat_p,
>> + Error **errp)
>> {
>> MemoryRegion *mr;
>> hwaddr xlat;
>> hwaddr len = iotlb->addr_mask + 1;
>> bool writable = iotlb->perm & IOMMU_WO;
>> - if (mr_has_discard_manager) {
>> - *mr_has_discard_manager = false;
>> - }
>> /*
>> * The IOMMU TLB entry we have just covers translation through
>> * this IOMMU to its immediate target. We need to translate
>> @@ -2195,7 +2191,7 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
>> &xlat, &len, writable, MEMTXATTRS_UNSPECIFIED);
>> if (!memory_region_is_ram(mr)) {
>> error_setg(errp, "iommu map to non memory area %" HWADDR_PRIx "", xlat);
>> - return false;
>> + return NULL;
>> } else if (memory_region_has_ram_discard_manager(mr)) {
>> RamDiscardManager *rdm = memory_region_get_ram_discard_manager(mr);
>> MemoryRegionSection tmp = {
>> @@ -2203,9 +2199,6 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
>> .offset_within_region = xlat,
>> .size = int128_make64(len),
>> };
>> - if (mr_has_discard_manager) {
>> - *mr_has_discard_manager = true;
>> - }
>> /*
>> * Malicious VMs can map memory into the IOMMU, which is expected
>> * to remain discarded. vfio will pin all pages, populating memory.
>> @@ -2216,7 +2209,7 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
>> error_setg(errp, "iommu map to discarded memory (e.g., unplugged"
>> " via virtio-mem): %" HWADDR_PRIx "",
>> iotlb->translated_addr);
>> - return false;
>> + return NULL;
>> }
>> }
>> @@ -2226,22 +2219,11 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
>> */
>> if (len & iotlb->addr_mask) {
>> error_setg(errp, "iommu has granularity incompatible with target AS");
>> - return false;
>> + return NULL;
>> }
>> - if (vaddr) {
>> - *vaddr = memory_region_get_ram_ptr(mr) + xlat;
>> - }
>> -
>> - if (ram_addr) {
>> - *ram_addr = memory_region_get_ram_addr(mr) + xlat;
>> - }
>> -
>> - if (read_only) {
>> - *read_only = !writable || mr->readonly;
>> - }
>> -
>> - return true;
>> + *xlat_p = xlat;
>> + return mr;
>> }
>> void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V5] vfio: return mr from vfio_get_xlat_addr
2025-05-20 13:42 ` Steven Sistare
@ 2025-05-20 13:46 ` Cédric Le Goater
2025-05-25 21:23 ` Cédric Le Goater
0 siblings, 1 reply; 9+ messages in thread
From: Cédric Le Goater @ 2025-05-20 13:46 UTC (permalink / raw)
To: Steven Sistare, qemu-devel
Cc: Marc-Andre Lureau, Thanos Makatos, Daniel P. Berrange,
Paolo Bonzini, Peter Xu, David Hildenbrand, Stefano Garzarella,
Michael S. Tsirkin, Alex Williamson, Philippe Mathieu-Daude,
John Levon
>>> @@ -1010,6 +1017,8 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>>> ram_addr_t translated_addr;
>>> Error *local_err = NULL;
>>> int ret = -EINVAL;
>>> + MemoryRegion *mr;
>>> + ram_addr_t xlat;
>>
>> xlat should be :
>>
>> hwaddr xlat;
>
> Will you make that change when you pull, or do you want me to submit a V6
> with this and the RB's?
I can handle it.
An ack from Michael (vhost) is needed for the PR.
Thanks,
C.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V5] vfio: return mr from vfio_get_xlat_addr
2025-05-20 13:46 ` Cédric Le Goater
@ 2025-05-25 21:23 ` Cédric Le Goater
2025-05-26 14:37 ` Michael S. Tsirkin
0 siblings, 1 reply; 9+ messages in thread
From: Cédric Le Goater @ 2025-05-25 21:23 UTC (permalink / raw)
To: Steven Sistare, qemu-devel
Cc: Marc-Andre Lureau, Thanos Makatos, Daniel P. Berrange,
Paolo Bonzini, Peter Xu, David Hildenbrand, Stefano Garzarella,
Michael S. Tsirkin, Alex Williamson, Philippe Mathieu-Daude,
John Levon
Michael,
On 5/20/25 15:46, Cédric Le Goater wrote:
>
>>>> @@ -1010,6 +1017,8 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>>>> ram_addr_t translated_addr;
>>>> Error *local_err = NULL;
>>>> int ret = -EINVAL;
>>>> + MemoryRegion *mr;
>>>> + ram_addr_t xlat;
>>>
>>> xlat should be :
>>>
>>> hwaddr xlat;
>>
>> Will you make that change when you pull, or do you want me to submit a V6
>> with this and the RB's?
>
> I can handle it.
>
> An ack from Michael (vhost) is needed for the PR.
Are you okay with us merging this patch via the vfio queue ?
This would facilitate the progress of 2 larges series : live update
and vfio-user
Thanks,
C.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V5] vfio: return mr from vfio_get_xlat_addr
2025-05-25 21:23 ` Cédric Le Goater
@ 2025-05-26 14:37 ` Michael S. Tsirkin
0 siblings, 0 replies; 9+ messages in thread
From: Michael S. Tsirkin @ 2025-05-26 14:37 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Steven Sistare, qemu-devel, Marc-Andre Lureau, Thanos Makatos,
Daniel P. Berrange, Paolo Bonzini, Peter Xu, David Hildenbrand,
Stefano Garzarella, Alex Williamson, Philippe Mathieu-Daude,
John Levon
On Sun, May 25, 2025 at 11:23:57PM +0200, Cédric Le Goater wrote:
> Michael,
>
> On 5/20/25 15:46, Cédric Le Goater wrote:
> >
> > > > > @@ -1010,6 +1017,8 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> > > > > ram_addr_t translated_addr;
> > > > > Error *local_err = NULL;
> > > > > int ret = -EINVAL;
> > > > > + MemoryRegion *mr;
> > > > > + ram_addr_t xlat;
> > > >
> > > > xlat should be :
> > > >
> > > > hwaddr xlat;
> > >
> > > Will you make that change when you pull, or do you want me to submit a V6
> > > with this and the RB's?
> >
> > I can handle it.
> >
> > An ack from Michael (vhost) is needed for the PR.
>
> Are you okay with us merging this patch via the vfio queue ?
>
> This would facilitate the progress of 2 larges series : live update
> and vfio-user
>
> Thanks,
>
> C.
sure, thanks!
Acked-by: Michael S. Tsirkin <mst@redhat.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V5] vfio: return mr from vfio_get_xlat_addr
2025-05-19 13:26 [PATCH V5] vfio: return mr from vfio_get_xlat_addr Steve Sistare
` (2 preceding siblings ...)
2025-05-20 7:32 ` Cédric Le Goater
@ 2025-05-26 16:57 ` Cédric Le Goater
3 siblings, 0 replies; 9+ messages in thread
From: Cédric Le Goater @ 2025-05-26 16:57 UTC (permalink / raw)
To: Steve Sistare, qemu-devel
Cc: Marc-Andre Lureau, Thanos Makatos, Daniel P. Berrange,
Paolo Bonzini, Peter Xu, David Hildenbrand, Stefano Garzarella,
Michael S. Tsirkin, Alex Williamson, Philippe Mathieu-Daude,
John Levon
On 5/19/25 15:26, Steve Sistare wrote:
> Modify memory_get_xlat_addr and vfio_get_xlat_addr to return the memory
> region that the translated address is found in. This will be needed by
> CPR in a subsequent patch to map blocks using IOMMU_IOAS_MAP_FILE.
>
> Also return the xlat offset, so we can simplify the interface by removing
> the out parameters that can be trivially derived from mr and xlat.
>
> Lastly, rename the functions to to memory_translate_iotlb() and
> vfio_translate_iotlb().
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> Acked-by: David Hildenbrand <david@redhat.com>
Applied to vfio-next.
Thanks,
C.
> ---
> hw/vfio/listener.c | 33 ++++++++++++++++++++++-----------
> hw/virtio/vhost-vdpa.c | 9 +++++++--
> include/system/memory.h | 19 +++++++++----------
> system/memory.c | 32 +++++++-------------------------
> 4 files changed, 45 insertions(+), 48 deletions(-)
>
> diff --git a/hw/vfio/listener.c b/hw/vfio/listener.c
> index bfacb3d..0afafe3 100644
> --- a/hw/vfio/listener.c
> +++ b/hw/vfio/listener.c
> @@ -90,16 +90,17 @@ static bool vfio_listener_skipped_section(MemoryRegionSection *section)
> section->offset_within_address_space & (1ULL << 63);
> }
>
> -/* Called with rcu_read_lock held. */
> -static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> - ram_addr_t *ram_addr, bool *read_only,
> - Error **errp)
> +/*
> + * Called with rcu_read_lock held.
> + * The returned MemoryRegion must not be accessed after calling rcu_read_unlock.
> + */
> +static MemoryRegion *vfio_translate_iotlb(IOMMUTLBEntry *iotlb, hwaddr *xlat_p,
> + Error **errp)
> {
> - bool ret, mr_has_discard_manager;
> + MemoryRegion *mr;
>
> - ret = memory_get_xlat_addr(iotlb, vaddr, ram_addr, read_only,
> - &mr_has_discard_manager, errp);
> - if (ret && mr_has_discard_manager) {
> + mr = memory_translate_iotlb(iotlb, xlat_p, errp);
> + if (mr && memory_region_has_ram_discard_manager(mr)) {
> /*
> * Malicious VMs might trigger discarding of IOMMU-mapped memory. The
> * pages will remain pinned inside vfio until unmapped, resulting in a
> @@ -118,7 +119,7 @@ static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> " intended via an IOMMU. It's possible to mitigate "
> " by setting/adjusting RLIMIT_MEMLOCK.");
> }
> - return ret;
> + return mr;
> }
>
> static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> @@ -126,6 +127,8 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> VFIOGuestIOMMU *giommu = container_of(n, VFIOGuestIOMMU, n);
> VFIOContainerBase *bcontainer = giommu->bcontainer;
> hwaddr iova = iotlb->iova + giommu->iommu_offset;
> + MemoryRegion *mr;
> + hwaddr xlat;
> void *vaddr;
> int ret;
> Error *local_err = NULL;
> @@ -150,10 +153,14 @@ 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)) {
> + mr = vfio_translate_iotlb(iotlb, &xlat, &local_err);
> + if (!mr) {
> error_report_err(local_err);
> goto out;
> }
> + vaddr = memory_region_get_ram_ptr(mr) + xlat;
> + read_only = !(iotlb->perm & IOMMU_WO) || mr->readonly;
> +
> /*
> * vaddr is only valid until rcu_read_unlock(). But after
> * vfio_dma_map has set up the mapping the pages will be
> @@ -1010,6 +1017,8 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> ram_addr_t translated_addr;
> Error *local_err = NULL;
> int ret = -EINVAL;
> + MemoryRegion *mr;
> + ram_addr_t xlat;
>
> trace_vfio_iommu_map_dirty_notify(iova, iova + iotlb->addr_mask);
>
> @@ -1021,9 +1030,11 @@ 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)) {
> + mr = vfio_translate_iotlb(iotlb, &xlat, &local_err);
> + if (!mr) {
> goto out_unlock;
> }
> + translated_addr = memory_region_get_ram_addr(mr) + xlat;
>
> ret = vfio_container_query_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1,
> translated_addr, &local_err);
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 1ab2c11..a1dd9e1 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -209,6 +209,8 @@ static void vhost_vdpa_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> int ret;
> Int128 llend;
> Error *local_err = NULL;
> + MemoryRegion *mr;
> + hwaddr xlat;
>
> if (iotlb->target_as != &address_space_memory) {
> error_report("Wrong target AS \"%s\", only system memory is allowed",
> @@ -228,11 +230,14 @@ 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,
> - &local_err)) {
> + mr = memory_translate_iotlb(iotlb, &xlat, &local_err);
> + if (!mr) {
> error_report_err(local_err);
> return;
> }
> + vaddr = memory_region_get_ram_ptr(mr) + xlat;
> + read_only = !(iotlb->perm & IOMMU_WO) || mr->readonly;
> +
> ret = vhost_vdpa_dma_map(s, VHOST_VDPA_GUEST_PA_ASID, iova,
> iotlb->addr_mask + 1, vaddr, read_only);
> if (ret) {
> diff --git a/include/system/memory.h b/include/system/memory.h
> index fbbf4cf..13416d7 100644
> --- a/include/system/memory.h
> +++ b/include/system/memory.h
> @@ -738,21 +738,20 @@ void ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
> RamDiscardListener *rdl);
>
> /**
> - * memory_get_xlat_addr: Extract addresses from a TLB entry
> + * memory_translate_iotlb: Extract addresses from a TLB entry.
> + * Called with rcu_read_lock held.
> *
> * @iotlb: pointer to an #IOMMUTLBEntry
> - * @vaddr: virtual address
> - * @ram_addr: RAM address
> - * @read_only: indicates if writes are allowed
> - * @mr_has_discard_manager: indicates memory is controlled by a
> - * RamDiscardManager
> + * @xlat_p: return the offset of the entry from the start of the returned
> + * MemoryRegion.
> * @errp: pointer to Error*, to store an error if it happens.
> *
> - * Return: true on success, else false setting @errp with error.
> + * Return: On success, return the MemoryRegion containing the @iotlb translated
> + * addr. The MemoryRegion must not be accessed after rcu_read_unlock.
> + * On failure, return NULL, 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);
> +MemoryRegion *memory_translate_iotlb(IOMMUTLBEntry *iotlb, hwaddr *xlat_p,
> + Error **errp);
>
> typedef struct CoalescedMemoryRange CoalescedMemoryRange;
> typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
> diff --git a/system/memory.c b/system/memory.c
> index 63b983e..306e9ff 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -2174,18 +2174,14 @@ 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)
> +MemoryRegion *memory_translate_iotlb(IOMMUTLBEntry *iotlb, hwaddr *xlat_p,
> + Error **errp)
> {
> MemoryRegion *mr;
> hwaddr xlat;
> hwaddr len = iotlb->addr_mask + 1;
> bool writable = iotlb->perm & IOMMU_WO;
>
> - if (mr_has_discard_manager) {
> - *mr_has_discard_manager = false;
> - }
> /*
> * The IOMMU TLB entry we have just covers translation through
> * this IOMMU to its immediate target. We need to translate
> @@ -2195,7 +2191,7 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> &xlat, &len, writable, MEMTXATTRS_UNSPECIFIED);
> if (!memory_region_is_ram(mr)) {
> error_setg(errp, "iommu map to non memory area %" HWADDR_PRIx "", xlat);
> - return false;
> + return NULL;
> } else if (memory_region_has_ram_discard_manager(mr)) {
> RamDiscardManager *rdm = memory_region_get_ram_discard_manager(mr);
> MemoryRegionSection tmp = {
> @@ -2203,9 +2199,6 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> .offset_within_region = xlat,
> .size = int128_make64(len),
> };
> - if (mr_has_discard_manager) {
> - *mr_has_discard_manager = true;
> - }
> /*
> * Malicious VMs can map memory into the IOMMU, which is expected
> * to remain discarded. vfio will pin all pages, populating memory.
> @@ -2216,7 +2209,7 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> error_setg(errp, "iommu map to discarded memory (e.g., unplugged"
> " via virtio-mem): %" HWADDR_PRIx "",
> iotlb->translated_addr);
> - return false;
> + return NULL;
> }
> }
>
> @@ -2226,22 +2219,11 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> */
> if (len & iotlb->addr_mask) {
> error_setg(errp, "iommu has granularity incompatible with target AS");
> - return false;
> + return NULL;
> }
>
> - if (vaddr) {
> - *vaddr = memory_region_get_ram_ptr(mr) + xlat;
> - }
> -
> - if (ram_addr) {
> - *ram_addr = memory_region_get_ram_addr(mr) + xlat;
> - }
> -
> - if (read_only) {
> - *read_only = !writable || mr->readonly;
> - }
> -
> - return true;
> + *xlat_p = xlat;
> + return mr;
> }
>
> void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-05-26 16:58 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-19 13:26 [PATCH V5] vfio: return mr from vfio_get_xlat_addr Steve Sistare
2025-05-19 13:46 ` John Levon
2025-05-20 6:46 ` Cédric Le Goater
2025-05-20 7:32 ` Cédric Le Goater
2025-05-20 13:42 ` Steven Sistare
2025-05-20 13:46 ` Cédric Le Goater
2025-05-25 21:23 ` Cédric Le Goater
2025-05-26 14:37 ` Michael S. Tsirkin
2025-05-26 16:57 ` Cédric Le Goater
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).