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

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

On 2/14/2025 5:42 PM, Peter Xu wrote:
> On Fri, Feb 14, 2025 at 03:40:57PM -0500, Steven Sistare wrote:
>>>> 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.
> 
> I meant only dropping mr_has_discard_manager parameter from the function
> interface, not the ram_discard_manager_is_populated() check.

Got it, will do.

>>>>    {
>>>>        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?
> 
> AFAICT, any vIOMMU mapping isn't high perf path.  In this specific path,
> the refcount op should be buried in any dma map operations..

memory_region_ref contains a comment that implies we should avoid taking a
ref if possible:
      * Memory regions without an owner are supposed to never go away;
      * we do not ref/unref them because it slows down DMA sensibly.

- Steve

> Personally I slightly prefer this one because it's always safer to take a
> refcount along with a pointer.. easier to follow.