Re: [PATCH v2 10/29] vfio/container: pass MemoryRegion to DMA operations

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

On 5/21/2025 3:43 AM, Cédric Le Goater wrote:
> +Steven,
> 
> On 5/20/25 17:03, John Levon wrote:
>> Pass through the MemoryRegion to DMA operation handlers of vfio
>> containers. The vfio-user container will need this later, to translate
>> the vaddr into an offset for the dma map vfio-user message.
>>
>> Originally-by: John Johnson <john.g.johnson@oracle.com>
>> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
>> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>> Signed-off-by: John Levon <john.levon@nutanix.com>
> 
> 
> This change conflicts with :
> 
>    https://lore.kernel.org/qemu-devel/1747063973-124548-28-git-send-email-steven.sistare@oracle.com
> 
> Steven,
> 
> Could we use this version instead for the "live update" series ?

Sure, I will drop my patch and use this.

A few nits, iff there is another version of the series.

I suggest "mrp" be renamed "mr", to be consistent with all other
MemoryRegion* variables. A MemoryRegion is never passed by value,
so the "p" is understood.

This comment for @dma_map needs polishing for clarity:

+     * Map an address range into the container. Note that @mrp will within an
+     * RCU read lock region across this call.

- Steve

>> ---
>>   include/hw/vfio/vfio-container-base.h | 9 +++++----
>>   hw/vfio/container-base.c              | 4 ++--
>>   hw/vfio/container.c                   | 3 ++-
>>   hw/vfio/iommufd.c                     | 3 ++-
>>   hw/vfio/listener.c                    | 6 +++---
>>   5 files changed, 14 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
>> index 41c49b2aa5..c18986a621 100644
>> --- a/include/hw/vfio/vfio-container-base.h
>> +++ b/include/hw/vfio/vfio-container-base.h
>> @@ -78,7 +78,7 @@ void vfio_address_space_insert(VFIOAddressSpace *space,
>>   int vfio_container_dma_map(VFIOContainerBase *bcontainer,
>>                              hwaddr iova, ram_addr_t size,
>> -                           void *vaddr, bool readonly);
>> +                           void *vaddr, bool readonly, MemoryRegion *mrp);
>>   int vfio_container_dma_unmap(VFIOContainerBase *bcontainer,
>>                                hwaddr iova, ram_addr_t size,
>>                                IOMMUTLBEntry *iotlb, bool unmap_all);
>> @@ -149,18 +149,19 @@ struct VFIOIOMMUClass {
>>       /**
>>        * @dma_map
>>        *
>> -     * Map an address range into the container.
>> +     * Map an address range into the container. Note that @mrp will within an
>> +     * RCU read lock region across this call.
>>        *
>>        * @bcontainer: #VFIOContainerBase to use
>>        * @iova: start address to map
>>        * @size: size of the range to map
>>        * @vaddr: process virtual address of mapping
>>        * @readonly: true if mapping should be readonly
>> +     * @mrp: the memory region for this mapping
>>        */
>>       int (*dma_map)(const VFIOContainerBase *bcontainer,
>>                      hwaddr iova, ram_addr_t size,
>> -                   void *vaddr, bool readonly);
>> -
>> +                   void *vaddr, bool readonly, MemoryRegion *mrp);
>>       /**
>>        * @dma_unmap
>>        *
>> diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
>> index 1c6ca94b60..a677bb6694 100644
>> --- a/hw/vfio/container-base.c
>> +++ b/hw/vfio/container-base.c
>> @@ -75,12 +75,12 @@ void vfio_address_space_insert(VFIOAddressSpace *space,
>>   int vfio_container_dma_map(VFIOContainerBase *bcontainer,
>>                              hwaddr iova, ram_addr_t size,
>> -                           void *vaddr, bool readonly)
>> +                           void *vaddr, bool readonly, MemoryRegion *mrp)
>>   {
>>       VFIOIOMMUClass *vioc = VFIO_IOMMU_GET_CLASS(bcontainer);
>>       g_assert(vioc->dma_map);
>> -    return vioc->dma_map(bcontainer, iova, size, vaddr, readonly);
>> +    return vioc->dma_map(bcontainer, iova, size, vaddr, readonly, mrp);
>>   }
>>   int vfio_container_dma_unmap(VFIOContainerBase *bcontainer,
>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>> index a9f0dbaec4..98d6b9f90c 100644
>> --- a/hw/vfio/container.c
>> +++ b/hw/vfio/container.c
>> @@ -207,7 +207,8 @@ static int vfio_legacy_dma_unmap(const VFIOContainerBase *bcontainer,
>>   }
>>   static int vfio_legacy_dma_map(const VFIOContainerBase *bcontainer, hwaddr iova,
>> -                               ram_addr_t size, void *vaddr, bool readonly)
>> +                               ram_addr_t size, void *vaddr, bool readonly,
>> +                               MemoryRegion *mrp)
>>   {
>>       const VFIOContainer *container = container_of(bcontainer, VFIOContainer,
>>                                                     bcontainer);
>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>> index af1c7ab10a..a2518c4a5d 100644
>> --- a/hw/vfio/iommufd.c
>> +++ b/hw/vfio/iommufd.c
>> @@ -34,7 +34,8 @@
>>               TYPE_HOST_IOMMU_DEVICE_IOMMUFD "-vfio"
>>   static int iommufd_cdev_map(const VFIOContainerBase *bcontainer, hwaddr iova,
>> -                            ram_addr_t size, void *vaddr, bool readonly)
>> +                            ram_addr_t size, void *vaddr, bool readonly,
>> +                            MemoryRegion *mrp)
>>   {
>>       const VFIOIOMMUFDContainer *container =
>>           container_of(bcontainer, VFIOIOMMUFDContainer, bcontainer);
>> diff --git a/hw/vfio/listener.c b/hw/vfio/listener.c
>> index 0afafe3464..a1d2d2561d 100644
>> --- a/hw/vfio/listener.c
>> +++ b/hw/vfio/listener.c
>> @@ -170,7 +170,7 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>>            */
>>           ret = vfio_container_dma_map(bcontainer, iova,
>>                                        iotlb->addr_mask + 1, vaddr,
>> -                                     read_only);
>> +                                     read_only, mr);
>>           if (ret) {
>>               error_report("vfio_container_dma_map(%p, 0x%"HWADDR_PRIx", "
>>                            "0x%"HWADDR_PRIx", %p) = %d (%s)",
>> @@ -240,7 +240,7 @@ static int vfio_ram_discard_notify_populate(RamDiscardListener *rdl,
>>           vaddr = memory_region_get_ram_ptr(section->mr) + start;
>>           ret = vfio_container_dma_map(bcontainer, iova, next - start,
>> -                                     vaddr, section->readonly);
>> +                                     vaddr, section->readonly, section->mr);
>>           if (ret) {
>>               /* Rollback */
>>               vfio_ram_discard_notify_discard(rdl, section);
>> @@ -564,7 +564,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
>>       }
>>       ret = vfio_container_dma_map(bcontainer, iova, int128_get64(llsize),
>> -                                 vaddr, section->readonly);
>> +                                 vaddr, section->readonly, section->mr);
>>       if (ret) {
>>           error_setg(&err, "vfio_container_dma_map(%p, 0x%"HWADDR_PRIx", "
>>                      "0x%"HWADDR_PRIx", %p) = %d (%s)",
>