Re: [PATCH V4] vfio: return mr from vfio_get_xlat_addr

[David Hildenbrand <david@redhat.com>]

On 16.05.25 19:13, 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.
> 
> Lastly, rename the functions to  to memory_translate_iotlb() and
> vfio_translate_iotlb().
> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>   hw/vfio/listener.c      | 33 ++++++++++++++++++++++-----------
>   hw/virtio/vhost-vdpa.c  |  9 +++++++--
>   include/system/memory.h | 19 +++++++++----------
>   system/memory.c         | 32 +++++++-------------------------
>   4 files changed, 45 insertions(+), 48 deletions(-)
> 
> diff --git a/hw/vfio/listener.c b/hw/vfio/listener.c
> index bfacb3d..a4931f1 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 MemoryRegion *vfio_translate_iotlb(IOMMUTLBEntry *iotlb, hwaddr *xlat_p,
> +                                          Error **errp)
>   {
> -    bool ret, mr_has_discard_manager;
> +    MemoryRegion *mr;
>   
> -    ret = memory_get_xlat_addr(iotlb, vaddr, ram_addr, read_only,
> -                               &mr_has_discard_manager, errp);
> -    if (ret && mr_has_discard_manager) {
> +    mr = memory_translate_iotlb(iotlb, xlat_p, errp);
> +    if (!mr && memory_region_has_ram_discard_manager(mr)) {

Pretty sue this should be if (mr && ...)

otherwise we'd be dereferencing NULL :)


Apart from that, LGTM.

Thanks!

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb