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

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

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.sistare@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;
>>   }
>