On 5/19/2025 4:28 AM, David Hildenbrand wrote:
On 17.05.25 15:55, Steven Sistare wrote:
On 5/16/2025 4:50 PM, David Hildenbrand wrote:
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.sist...@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?
The issue is that mr_has_discard_manager has been replaced by
memory_region_has_ram_discard_manager(mr), hence mr must be returned to
evaluate memory_region_has_ram_discard_manager(), hence we cannot return
mr == NULL as the error indication.
Let's try to understand what the code wants to achieve.
If *our translation was successful* and we have a RAMDiscardManager, we want to
print a warning, stating that this *successful* translation is problematic.
memory_get_xlat_addr() currently returns true *if the translation was successful*. Now we
return "mr" when the translation is successful.
if (mr && memory_region_has_ram_discard_manager(mr))
Checks the exact same thing? I really don't see the problem you see.
Ah yes, "ret != 0" now means success. I've been slow to adjust to the change.
I'll submit a V5 with your ack and the corrected line:
if (mr && memory_region_has_ram_discard_manager(mr)) {
- Steve
