Re: [PATCH V3 26/42] vfio: return mr from vfio_get_xlat_addr

[Steven Sistare <steven.sistare@oracle.com>]

On 5/13/2025 7:12 AM, Mark Cave-Ayland wrote:
> On 12/05/2025 16:32, 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 <steven.sistare@oracle.com>
>> ---
>>   hw/vfio/listener.c      | 29 +++++++++++++++++++----------
>>   hw/virtio/vhost-vdpa.c  |  8 ++++++--
>>   include/system/memory.h | 16 +++++++---------
>>   system/memory.c         | 25 ++++---------------------
>>   4 files changed, 36 insertions(+), 42 deletions(-)
>>
>> diff --git a/hw/vfio/listener.c b/hw/vfio/listener.c
>> index e86ffcf..87b7a3c 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 bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, MemoryRegion **mr_p,
>> +                               hwaddr *xlat_p, Error **errp)
>>   {
>> -    bool ret, mr_has_discard_manager;
>> +    bool ret;
>> -    ret = memory_get_xlat_addr(iotlb, vaddr, ram_addr, read_only,
>> -                               &mr_has_discard_manager, errp);
>> -    if (ret && mr_has_discard_manager) {
>> +    ret = memory_get_xlat_addr(iotlb, mr_p, xlat_p, errp);
>> +    if (ret && memory_region_has_ram_discard_manager(*mr_p)) {
> 
> I'm trying to understand the underlying intention of this patch: is it just so that you can access the corresponding RAMBlock in vfio_container_dma_map() in patch 31 "vfio/iommufd: use IOMMU_IOAS_MAP_FILE"?

Yes.

> Given that the flatview can theoretically change at any point, it feels as if the current API whereby the vaddr is passed around is the correct approach, and that the final MemoryRegion lookup should be done at the point where it is required.

The existing code already guarantees a stable address space when vfio_container_dma_map()
is called ...

     vfio_iommu_map_notify()
         rcu_read_lock();
         vfio_get_xlat_addr()
         vfio_container_dma_map()


> If this is the case, is it not simpler to add a call to address_space_translate() in patch 31 to obtain the MemoryRegion pointer there instead?

... so it is simpler and more efficient (saving a translation) if we simply
expose mr->ram_block in that range of code.

- Steve

>>           /*
>>            * Malicious VMs might trigger discarding of IOMMU-mapped memory. The
>>            * pages will remain pinned inside vfio until unmapped, resulting in a
>> @@ -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,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
>> @@ -1047,6 +1053,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);
>> @@ -1058,9 +1066,10 @@ 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)) {
>>           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..f191360 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, &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/system/memory.h b/include/system/memory.h
>> index fbbf4cf..d743214 100644
>> --- a/include/system/memory.h
>> +++ b/include/system/memory.h
>> @@ -738,21 +738,19 @@ void ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
>>                                                RamDiscardListener *rdl);
>>   /**
>> - * memory_get_xlat_addr: Extract addresses from a TLB entry
>> + * memory_get_xlat_addr: 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
>> + * @mr_p: return the MemoryRegion containing the @iotlb translated addr.
>> + *        The MemoryRegion must not be accessed after rcu_read_unlock.
>> + * @xlat_p: return the offset of the entry from the start of @mr_p
>>    * @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, 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 63b983e..4894c0d 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)
>> +bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, MemoryRegion **mr_p,
>> +                          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
>> @@ -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.
>> @@ -2229,18 +2222,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;
>>       return true;
>>   }
> 
> 
> ATB,
> 
> Mark.
>