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.
