Re: [PATCH V2 28/45] vfio: return mr from vfio_get_xlat_addr

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

On 2/14/2025 11:48 AM, Peter Xu wrote:
> 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 <steven.sistare@oracle.com>
>> ---
>>   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))

My bad, thanks. I'll delete vaddr, ram_addr, read_only, and add xlat.

>> + * @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..

To hoist mr_has_discard_manager to the vfio caller, I would need to return len.
Your call.

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

I can do that, or ...

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

I can do that, but it would add cycles.  Is this considered a high performance
path that may be called frequently?

- Steve

> 
>>       return true;
>>   }
>>   
>> -- 
>> 1.8.3.1
>>
>