On Fri, Feb 14, 2025 at 06:14:10AM -0800, 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.
>
> Signed-off-by: Steve Sistare <[email protected]>
> ---
> hw/vfio/common.c | 21 ++++++++++++++-------
> hw/virtio/vhost-vdpa.c | 8 ++++++--
> include/exec/memory.h | 6 +++---
> system/memory.c | 19 ++++---------------
> 4 files changed, 27 insertions(+), 27 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index c536698..3b0c520 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -246,14 +246,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,
> - Error **errp)
> +static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, MemoryRegion **mr_p,
> + hwaddr *xlat_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);
> + ret = memory_get_xlat_addr(iotlb, &mr_has_discard_manager, mr_p, xlat_p,
> + errp);
> if (ret && mr_has_discard_manager) {
> /*
> * Malicious VMs might trigger discarding of IOMMU-mapped memory. The
> @@ -281,6 +280,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;
> @@ -300,10 +301,13 @@ 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, &mr, &xlat, &local_err)) {
> 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
> @@ -1259,6 +1263,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);
>
> @@ -1269,10 +1275,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)) {
> + if (!vfio_get_xlat_addr(iotlb, &mr, &xlat, &local_err)) {
> error_report_err(local_err);
> goto out_unlock;
> }
> + translated_addr = memory_region_get_ram_addr(mr) + xlat;
>
> ret = vfio_get_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 3cdaa12..5dfe51e 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,13 @@ 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)) {
> + if (!memory_get_xlat_addr(iotlb, NULL, &mr, &xlat, &local_err)) {
> 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/exec/memory.h b/include/exec/memory.h
> index ea5d33a..8590838 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -747,13 +747,13 @@ 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
(some prior fields are prone to removal))
> + * @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 memory_get_xlat_addr(IOMMUTLBEntry *iotlb, bool *mr_has_discard_manager,
> + MemoryRegion **mr_p, hwaddr *xlat_p, Error **errp);
>
> typedef struct CoalescedMemoryRange CoalescedMemoryRange;
> typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
> diff --git a/system/memory.c b/system/memory.c
> index 4c82979..755eafe 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -2183,9 +2183,8 @@ 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 memory_get_xlat_addr(IOMMUTLBEntry *iotlb, bool *mr_has_discard_manager,
> + MemoryRegion **mr_p, hwaddr *xlat_p, Error **errp)
If we're going to return the MR anyway, probably we can drop
mr_has_discard_manager altogether..
> {
> MemoryRegion *mr;
> hwaddr xlat;
> @@ -2238,18 +2237,8 @@ bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void
> **vaddr,
> return false;
> }
>
> - 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;
> - }
> -
> + *xlat_p = xlat;
> + *mr_p = mr;
I suppose current use on the callers are still under RCU so looks ok, but
that'll need to be rich-documented.
Better way is always taking a MR reference when the MR pointer is returned,
with memory_region_ref(). Then it is even valid if by accident accessed
after rcu_read_unlock(), and caller should unref() after use.
> return true;
> }
>
> --
> 1.8.3.1
>
--
Peter Xu
