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.
>
> > > {
> > > 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..
Personally I slightly prefer this one because it's always safer to take a
refcount along with a pointer.. easier to follow.
--
Peter Xu
