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

[David Hildenbrand]

On 16.05.25 21:26, Steven Sistare wrote:
> On 5/16/2025 2:58 PM, David Hildenbrand wrote:
> > 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 :)
>
> Hmmmm.
> That is why we cannot return mr as the function return value.
> There are cases where the function can return error, but the mr is
> valid.  We want to take the branch in that case.  From the original
> code:
>       if (ret && mr_has_discard_manager) {

It's late in Germany, but didn't we return "ret = true" when we would 
now return mr != NULL?

I mean, there is no reason to warn about mr_has_discard_manager if ... 
there is nothing to translate to?

--
Cheers,

David / dhildenb