accel/tcg/cputlb.c | 21 +++++++++++++-------- include/accel/tcg/iommu.h | 9 +++++---- include/hw/core/cpu.h | 2 ++ system/physmem.c | 24 +++++++++++++++--------- 4 files changed, 35 insertions(+), 21 deletions(-)
Just posting this here for the discussion this afternoon.
Cheers
Mark.
From afbc6b2a0f455ea4b2ee39a3cee368be5fff2c7b Mon Sep 17 00:00:00 2001
From: Mark Burton <mburton@quicinc.com>
Date: Fri, 10 Oct 2025 10:49:34 +0200
Subject: [PATCH] Record AddressSpace in full tlb so access to MMIO via IOMMU
works
Signed-off-by: Mark Burton <mburton@quicinc.com>
---
accel/tcg/cputlb.c | 21 +++++++++++++--------
include/accel/tcg/iommu.h | 9 +++++----
include/hw/core/cpu.h | 2 ++
system/physmem.c | 24 +++++++++++++++---------
4 files changed, 35 insertions(+), 21 deletions(-)
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index b09229dae8..4b1aa9df71 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1073,7 +1073,8 @@ void tlb_set_page_full(CPUState *cpu, int mmu_idx,
prot = full->prot;
asidx = cpu_asidx_from_attrs(cpu, full->attrs);
section = address_space_translate_for_iotlb(cpu, asidx, paddr_page,
- &xlat, &sz, full->attrs, &prot);
+ &xlat, &sz, &full->attrs,
+ &full->as, &prot);
assert(sz >= TARGET_PAGE_SIZE);
tlb_debug("vaddr=%016" VADDR_PRIx " paddr=0x" HWADDR_FMT_plx
@@ -1294,13 +1295,13 @@ static inline void cpu_unaligned_access(CPUState *cpu, vaddr addr,
}
static MemoryRegionSection *
-io_prepare(hwaddr *out_offset, CPUState *cpu, hwaddr xlat,
+io_prepare(hwaddr *out_offset, CPUState *cpu, AddressSpace *as, hwaddr xlat,
MemTxAttrs attrs, vaddr addr, uintptr_t retaddr)
{
MemoryRegionSection *section;
hwaddr mr_offset;
- section = iotlb_to_section(cpu, xlat, attrs);
+ section = iotlb_to_section(as, xlat, attrs);
mr_offset = (xlat & TARGET_PAGE_MASK) + addr;
cpu->mem_io_pc = retaddr;
if (!cpu->neg.can_do_io) {
@@ -1618,7 +1619,7 @@ bool tlb_plugin_lookup(CPUState *cpu, vaddr addr, int mmu_idx,
/* We must have an iotlb entry for MMIO */
if (tlb_addr & TLB_MMIO) {
MemoryRegionSection *section =
- iotlb_to_section(cpu, full->xlat_section & ~TARGET_PAGE_MASK,
+ iotlb_to_section(full->as, full->xlat_section & ~TARGET_PAGE_MASK,
full->attrs);
data->is_io = true;
data->mr = section->mr;
@@ -2028,7 +2029,8 @@ static uint64_t do_ld_mmio_beN(CPUState *cpu, CPUTLBEntryFull *full,
tcg_debug_assert(size > 0 && size <= 8);
attrs = full->attrs;
- section = io_prepare(&mr_offset, cpu, full->xlat_section, attrs, addr, ra);
+ section = io_prepare(&mr_offset, cpu, full->as,
+ full->xlat_section, attrs, addr, ra);
mr = section->mr;
BQL_LOCK_GUARD();
@@ -2049,7 +2051,8 @@ static Int128 do_ld16_mmio_beN(CPUState *cpu, CPUTLBEntryFull *full,
tcg_debug_assert(size > 8 && size <= 16);
attrs = full->attrs;
- section = io_prepare(&mr_offset, cpu, full->xlat_section, attrs, addr, ra);
+ section = io_prepare(&mr_offset, cpu, full->as,
+ full->xlat_section, attrs, addr, ra);
mr = section->mr;
BQL_LOCK_GUARD();
@@ -2593,7 +2596,8 @@ static uint64_t do_st_mmio_leN(CPUState *cpu, CPUTLBEntryFull *full,
tcg_debug_assert(size > 0 && size <= 8);
attrs = full->attrs;
- section = io_prepare(&mr_offset, cpu, full->xlat_section, attrs, addr, ra);
+ section = io_prepare(&mr_offset, cpu, full->as,
+ full->xlat_section, attrs, addr, ra);
mr = section->mr;
BQL_LOCK_GUARD();
@@ -2613,7 +2617,8 @@ static uint64_t do_st16_mmio_leN(CPUState *cpu, CPUTLBEntryFull *full,
tcg_debug_assert(size > 8 && size <= 16);
attrs = full->attrs;
- section = io_prepare(&mr_offset, cpu, full->xlat_section, attrs, addr, ra);
+ section = io_prepare(&mr_offset, cpu, full->as,
+ full->xlat_section, attrs, addr, ra);
mr = section->mr;
BQL_LOCK_GUARD();
diff --git a/include/accel/tcg/iommu.h b/include/accel/tcg/iommu.h
index 90cfd6c0ed..ac50e50601 100644
--- a/include/accel/tcg/iommu.h
+++ b/include/accel/tcg/iommu.h
@@ -16,22 +16,23 @@
/**
* iotlb_to_section:
- * @cpu: CPU performing the access
+ * @as: Address space to access
* @index: TCG CPU IOTLB entry
*
* Given a TCG CPU IOTLB entry, return the MemoryRegionSection that
* it refers to. @index will have been initially created and returned
* by memory_region_section_get_iotlb().
*/
-MemoryRegionSection *iotlb_to_section(CPUState *cpu,
- hwaddr index, MemTxAttrs attrs);
+struct MemoryRegionSection *iotlb_to_section(AddressSpace *as,
+ hwaddr index, MemTxAttrs attrs);
MemoryRegionSection *address_space_translate_for_iotlb(CPUState *cpu,
int asidx,
hwaddr addr,
hwaddr *xlat,
hwaddr *plen,
- MemTxAttrs attrs,
+ MemTxAttrs *attrs,
+ AddressSpace **as,
int *prot);
hwaddr memory_region_section_get_iotlb(CPUState *cpu,
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index c0ca4b6905..a27d8feefc 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -269,6 +269,8 @@ struct CPUTLBEntryFull {
bool guarded;
} arm;
} extra;
+
+ AddressSpace *as;
};
/*
diff --git a/system/physmem.c b/system/physmem.c
index cf7146b224..52156325d9 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -688,7 +688,8 @@ void tcg_iommu_init_notifier_list(CPUState *cpu)
MemoryRegionSection *
address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr orig_addr,
hwaddr *xlat, hwaddr *plen,
- MemTxAttrs attrs, int *prot)
+ MemTxAttrs *attrs, AddressSpace **as,
+ int *prot)
{
MemoryRegionSection *section;
IOMMUMemoryRegion *iommu_mr;
@@ -696,7 +697,8 @@ address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr orig_addr,
IOMMUTLBEntry iotlb;
int iommu_idx;
hwaddr addr = orig_addr;
- AddressSpaceDispatch *d = address_space_to_dispatch(cpu->cpu_ases[asidx].as);
+ *as = cpu->cpu_ases[asidx].as;
+ AddressSpaceDispatch *d = address_space_to_dispatch(*as);
for (;;) {
section = address_space_translate_internal(d, addr, &addr, plen, false);
@@ -708,13 +710,13 @@ address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr orig_addr,
imrc = memory_region_get_iommu_class_nocheck(iommu_mr);
- iommu_idx = imrc->attrs_to_index(iommu_mr, attrs);
+ iommu_idx = imrc->attrs_to_index(iommu_mr, *attrs);
tcg_register_iommu_notifier(cpu, iommu_mr, iommu_idx);
/* We need all the permissions, so pass IOMMU_NONE so the IOMMU
* doesn't short-cut its translation table walk.
*/
if (imrc->translate_attr) {
- iotlb = imrc->translate_attr(iommu_mr, addr, IOMMU_NONE, &attrs);
+ iotlb = imrc->translate_attr(iommu_mr, addr, IOMMU_NONE, attrs);
} else {
iotlb = imrc->translate(iommu_mr, addr, IOMMU_NONE, iommu_idx);
}
@@ -735,7 +737,8 @@ address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr orig_addr,
goto translate_fail;
}
- d = flatview_to_dispatch(address_space_to_flatview(iotlb.target_as));
+ *as = iotlb.target_as;
+ d = flatview_to_dispatch(address_space_to_flatview(*as));
}
assert(!memory_region_is_iommu(section->mr));
@@ -756,12 +759,12 @@ translate_fail:
return &d->map.sections[PHYS_SECTION_UNASSIGNED];
}
-MemoryRegionSection *iotlb_to_section(CPUState *cpu,
+
+MemoryRegionSection *iotlb_to_section(AddressSpace *as,
hwaddr index, MemTxAttrs attrs)
{
- int asidx = cpu_asidx_from_attrs(cpu, attrs);
- CPUAddressSpace *cpuas = &cpu->cpu_ases[asidx];
- AddressSpaceDispatch *d = address_space_to_dispatch(cpuas->as);
+ assert(as);
+ AddressSpaceDispatch *d = address_space_to_dispatch(as);
int section_index = index & ~TARGET_PAGE_MASK;
MemoryRegionSection *ret;
@@ -3102,6 +3105,9 @@ static void tcg_commit(MemoryListener *listener)
* That said, the listener is also called during realize, before
* all of the tcg machinery for run-on is initialized: thus halt_cond.
*/
+// Why are these removed ?
+// cpu_reloading_memory_map();
+// tlb_flush(cpuas->cpu);
if (cpu->halt_cond) {
async_run_on_cpu(cpu, tcg_commit_cpu, RUN_ON_CPU_HOST_PTR(cpuas));
} else {
--
2.51.1
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -269,6 +269,8 @@ struct CPUTLBEntryFull {
> bool guarded;
> } arm;
> } extra;
> +
> + AddressSpace *as;
> };
...
> -MemoryRegionSection *iotlb_to_section(CPUState *cpu,
> +
> +MemoryRegionSection *iotlb_to_section(AddressSpace *as,
> hwaddr index, MemTxAttrs attrs)
> {
> - int asidx = cpu_asidx_from_attrs(cpu, attrs);
> - CPUAddressSpace *cpuas = &cpu->cpu_ases[asidx];
> - AddressSpaceDispatch *d = address_space_to_dispatch(cpuas->as);
> + assert(as);
> + AddressSpaceDispatch *d = address_space_to_dispatch(as);
> int section_index = index & ~TARGET_PAGE_MASK;
Adding the as to CPUTLBEntryFull is unnecessary because
(1) Each CPUTLB, and thus each CPUTLBEntryFull, is private to the cpu.
(2) Each CPUTLBEntryFull contains the MemTxAttrs for the access.
Thus the AddressSpace is purely a function of (cpu, attrs).
We can have a conversation about where this lookup happens, and whether or not particular
functions should be passed a CPUState, but adding AddressSpace to full tlb is redundant.
r~
> On 11 Dec 2025, at 16:04, Richard Henderson <richard.henderson@linaro.org> wrote:
>
> WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.
>
>> --- a/include/hw/core/cpu.h
>> +++ b/include/hw/core/cpu.h
>> @@ -269,6 +269,8 @@ struct CPUTLBEntryFull {
>> bool guarded;
>> } arm;
>> } extra;
>> +
>> + AddressSpace *as;
>> };
> ...
>> -MemoryRegionSection *iotlb_to_section(CPUState *cpu,
>> +
>> +MemoryRegionSection *iotlb_to_section(AddressSpace *as,
>> hwaddr index, MemTxAttrs attrs)
>> {
>> - int asidx = cpu_asidx_from_attrs(cpu, attrs);
>> - CPUAddressSpace *cpuas = &cpu->cpu_ases[asidx];
>> - AddressSpaceDispatch *d = address_space_to_dispatch(cpuas->as);
>> + assert(as);
>> + AddressSpaceDispatch *d = address_space_to_dispatch(as);
>> int section_index = index & ~TARGET_PAGE_MASK;
>
> Adding the as to CPUTLBEntryFull is unnecessary because
>
> (1) Each CPUTLB, and thus each CPUTLBEntryFull, is private to the cpu.
> (2) Each CPUTLBEntryFull contains the MemTxAttrs for the access.
>
> Thus the AddressSpace is purely a function of (cpu, attrs).
The issue is, it would seem, it is also a function of the lookup provided by an IOMMU access - that kindly provides an address space independent of any CPU.
For now, the information thus provided would seem to be discarded, those subsequent accesses, using, as you suggest, an address space that is a function of the cpu and the attributes, arrives at the wrong pace.
An option, I guess, may be to provide the address space as an attribute… though maintaining the link with the page that has been thus evaluated via the IOMMU translation maybe, it would seem, challenging.
Cheers
Mark.
>
> We can have a conversation about where this lookup happens, and whether or not particular
> functions should be passed a CPUState, but adding AddressSpace to full tlb is redundant.
>
>
> r~
On 12/11/25 12:49, Mark Burton wrote: >> Adding the as to CPUTLBEntryFull is unnecessary because >> >> (1) Each CPUTLB, and thus each CPUTLBEntryFull, is private to the cpu. >> (2) Each CPUTLBEntryFull contains the MemTxAttrs for the access. >> >> Thus the AddressSpace is purely a function of (cpu, attrs). > > The issue is, it would seem, it is also a function of the lookup provided by an IOMMU access - that kindly provides an address space independent of any CPU. No, it really isn't. Because you think that, it seems like you're doing something wrong with IOMMU accesses. Since I don't know the wider context of the query means I don't know how to help you further. My shot in the dark: there is a flush that's supposed to happen for changes to a cpu's address space. There are plenty of ways in which this happens, e.g. x86 a20 translation line and pci bar changes. I forget the exact details when it comes to IOMMU, but there must be something related when translations change. The actual tcg flush will happen via the MemoryListener interface. r~
> On 11 Dec 2025, at 20:24, Richard Henderson <richard.henderson@linaro.org> wrote: > > WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros. > > On 12/11/25 12:49, Mark Burton wrote: >>> Adding the as to CPUTLBEntryFull is unnecessary because >>> >>> (1) Each CPUTLB, and thus each CPUTLBEntryFull, is private to the cpu. >>> (2) Each CPUTLBEntryFull contains the MemTxAttrs for the access. >>> >>> Thus the AddressSpace is purely a function of (cpu, attrs). >> >> The issue is, it would seem, it is also a function of the lookup provided by an IOMMU access - that kindly provides an address space independent of any CPU. > > No, it really isn't. > > Because you think that, it seems like you're doing something wrong with IOMMU accesses. > Since I don't know the wider context of the query means I don't know how to help you further. > > My shot in the dark: there is a flush that's supposed to happen for changes to a cpu's > address space. There are plenty of ways in which this happens, e.g. x86 a20 translation > line and pci bar changes. I forget the exact details when it comes to IOMMU, but there > must be something related when translations change. The actual tcg flush will happen via > the MemoryListener interface. I am absolutely prepared to believe I’m handling the IOMMU incorrectly The setup I’m using is CPU->SMMU(TBU)->AddressSpace (totally unconnected from the CPU). What I see in the code is that the IOMMU is permitted to return an address space - that address space, in the cases I have, is totally unrelated to the CPU concerned. The CPU knows (till now), nothing about that address space. The address space being returned from the IOMMU translate doesn’t seem to be used - so I’m not overly surprised that we end up in the wrong place. Perhaps what you’re saying is that somehow we should be ‘registering’ this address space with (any?) CPU that could potentially get to it... What I see is that io_prepare calls down and gets the target_as from the IOMMU translate cb, but it only returns MemroyRegionSection, not the target_as, and then e.g. int_st_mmio_leN seems to use cpu->as and index’s from that ….. I don’t see what I can be missing? Cheers Mark. > > > r~
On 12/11/25 13:49, Mark Burton wrote: > I am absolutely prepared to believe I’m handling the IOMMU incorrectly > > The setup I’m using is CPU->SMMU(TBU)->AddressSpace (totally unconnected from the CPU). > > What I see in the code is that the IOMMU is permitted to return an address space - that address space, in the cases I have, is totally unrelated to the CPU concerned. The CPU knows (till now), nothing about that address space. The address space being returned from the IOMMU translate doesn’t seem to be used - so I’m not overly surprised that we end up in the wrong place. Perhaps what you’re saying is that somehow we should be ‘registering’ this address space with (any?) CPU that could potentially get to it... > > What I see is that io_prepare calls down and gets the target_as from the IOMMU translate cb, but it only returns MemroyRegionSection, not the target_as, and then e.g. int_st_mmio_leN seems to use cpu->as and index’s from that ….. I don’t see what I can be missing? You're right that there's a disconnect. There's an initial translation in address_space_translate_for_iotlb() which records a MemoryRegionSection. Later, during execution, iotlb_to_section starts over from the cpu address space and tries to find the same MemoryRegionSection, but translation is not involved. I suspect we need to revisit CPUTLBEntryFull.xlat_section, "indexing" of MemoryRegionSection, etc. I've had in the back of my mind a reorg of the entire physical memory subsystem, with an eye toward eliminating TARGET_PAGE_SIZE entirely. The indexing nonsense would must needs change in that scenario. All very hand wavey at this point... r~
I also think I begin to see some light too - I need to dig more to understand, but you opened my eye’s to something I didn’t check for - the memory listeners. And that could explain why we’re failing - there is an important detail I didn’t mention (I didn’t think it was important ;-) ) - as a result of the IOTLB translation, in that translation callback, we are adding memory regions. I suspect the result is a somewhat ’stale’ address map - as you say “flush might help”…. I need to check on that. Anyway - THANKS ! (FWIW, if I can help at all with the re-factoring of the memory system, I’d love to. While I think the way SystemC does it is bad, I think standing back and recognising the lessons we've learned from QEMU, SystemC and indeed other systems could help guide us to a more utopian setup - perhaps - On my side, my intention is to look at the memory interface in a language and simulator agnostic fashion and try and come up with an interface that could be used across simulators and languages (e.g. Rust). This is no longer quite “pie in the sky”, there is now a proposal (which needs more work), but we’re making steps forward. But this isn’t really relevant to QEMU (yet)). Cheers Mark. > On 11 Dec 2025, at 21:14, Richard Henderson <richard.henderson@linaro.org> wrote: > > WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros. > > On 12/11/25 13:49, Mark Burton wrote: >> I am absolutely prepared to believe I’m handling the IOMMU incorrectly >> >> The setup I’m using is CPU->SMMU(TBU)->AddressSpace (totally unconnected from the CPU). >> >> What I see in the code is that the IOMMU is permitted to return an address space - that address space, in the cases I have, is totally unrelated to the CPU concerned. The CPU knows (till now), nothing about that address space. The address space being returned from the IOMMU translate doesn’t seem to be used - so I’m not overly surprised that we end up in the wrong place. Perhaps what you’re saying is that somehow we should be ‘registering’ this address space with (any?) CPU that could potentially get to it... >> >> What I see is that io_prepare calls down and gets the target_as from the IOMMU translate cb, but it only returns MemroyRegionSection, not the target_as, and then e.g. int_st_mmio_leN seems to use cpu->as and index’s from that ….. I don’t see what I can be missing? > > You're right that there's a disconnect. > > There's an initial translation in address_space_translate_for_iotlb() which records a > MemoryRegionSection. Later, during execution, iotlb_to_section starts over from the cpu > address space and tries to find the same MemoryRegionSection, but translation is not involved. > > I suspect we need to revisit CPUTLBEntryFull.xlat_section, "indexing" of > MemoryRegionSection, etc. > > I've had in the back of my mind a reorg of the entire physical memory subsystem, with an > eye toward eliminating TARGET_PAGE_SIZE entirely. The indexing nonsense would must needs > change in that scenario. All very hand wavey at this point... > > > r~
Mark Burton <mburton@qti.qualcomm.com> writes:
> Just posting this here for the discussion this afternoon.
>
> Cheers
> Mark.
>
>
> [2. 0001-Record-AddressSpace-in-full-tlb-so-access-to-MMIO-vi.patch --- text/x-diff; 0001-Record-AddressSpace-in-full-tlb-so-access-to-MMIO-vi.patch]...
We were discussing last week how we should break the dependency on
CPUState for AddressSpaces while Gustavo was looking at adding the
FEAT_MEC code.
They are really a function of the machine where most CPUs will share the
same set of views. However we still need a way to resolve the AS from
the CPUs perspective.
Copying inline for easier commenting:
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index b09229dae8..4b1aa9df71 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1073,7 +1073,8 @@ void tlb_set_page_full(CPUState *cpu, int mmu_idx,
prot = full->prot;
asidx = cpu_asidx_from_attrs(cpu, full->attrs);
section = address_space_translate_for_iotlb(cpu, asidx, paddr_page,
- &xlat, &sz, full->attrs, &prot);
+ &xlat, &sz, &full->attrs,
+ &full->as, &prot);
assert(sz >= TARGET_PAGE_SIZE);
tlb_debug("vaddr=%016" VADDR_PRIx " paddr=0x" HWADDR_FMT_plx
@@ -1294,13 +1295,13 @@ static inline void cpu_unaligned_access(CPUState *cpu, vaddr addr,
}
static MemoryRegionSection *
-io_prepare(hwaddr *out_offset, CPUState *cpu, hwaddr xlat,
+io_prepare(hwaddr *out_offset, CPUState *cpu, AddressSpace *as, hwaddr xlat,
MemTxAttrs attrs, vaddr addr, uintptr_t retaddr)
{
MemoryRegionSection *section;
hwaddr mr_offset;
- section = iotlb_to_section(cpu, xlat, attrs);
+ section = iotlb_to_section(as, xlat, attrs);
mr_offset = (xlat & TARGET_PAGE_MASK) + addr;
cpu->mem_io_pc = retaddr;
if (!cpu->neg.can_do_io) {
@@ -1618,7 +1619,7 @@ bool tlb_plugin_lookup(CPUState *cpu, vaddr addr, int mmu_idx,
/* We must have an iotlb entry for MMIO */
if (tlb_addr & TLB_MMIO) {
MemoryRegionSection *section =
- iotlb_to_section(cpu, full->xlat_section & ~TARGET_PAGE_MASK,
+ iotlb_to_section(full->as, full->xlat_section & ~TARGET_PAGE_MASK,
full->attrs);
data->is_io = true;
data->mr = section->mr;
@@ -2028,7 +2029,8 @@ static uint64_t do_ld_mmio_beN(CPUState *cpu, CPUTLBEntryFull *full,
tcg_debug_assert(size > 0 && size <= 8);
attrs = full->attrs;
- section = io_prepare(&mr_offset, cpu, full->xlat_section, attrs, addr, ra);
+ section = io_prepare(&mr_offset, cpu, full->as,
+ full->xlat_section, attrs, addr, ra);
mr = section->mr;
BQL_LOCK_GUARD();
@@ -2049,7 +2051,8 @@ static Int128 do_ld16_mmio_beN(CPUState *cpu, CPUTLBEntryFull *full,
tcg_debug_assert(size > 8 && size <= 16);
attrs = full->attrs;
- section = io_prepare(&mr_offset, cpu, full->xlat_section, attrs, addr, ra);
+ section = io_prepare(&mr_offset, cpu, full->as,
+ full->xlat_section, attrs, addr, ra);
mr = section->mr;
BQL_LOCK_GUARD();
@@ -2593,7 +2596,8 @@ static uint64_t do_st_mmio_leN(CPUState *cpu, CPUTLBEntryFull *full,
tcg_debug_assert(size > 0 && size <= 8);
attrs = full->attrs;
- section = io_prepare(&mr_offset, cpu, full->xlat_section, attrs, addr, ra);
+ section = io_prepare(&mr_offset, cpu, full->as,
+ full->xlat_section, attrs, addr, ra);
mr = section->mr;
BQL_LOCK_GUARD();
@@ -2613,7 +2617,8 @@ static uint64_t do_st16_mmio_leN(CPUState *cpu, CPUTLBEntryFull *full,
tcg_debug_assert(size > 8 && size <= 16);
attrs = full->attrs;
- section = io_prepare(&mr_offset, cpu, full->xlat_section, attrs, addr, ra);
+ section = io_prepare(&mr_offset, cpu, full->as,
+ full->xlat_section, attrs, addr, ra);
mr = section->mr;
BQL_LOCK_GUARD();
diff --git a/include/accel/tcg/iommu.h b/include/accel/tcg/iommu.h
index 90cfd6c0ed..ac50e50601 100644
--- a/include/accel/tcg/iommu.h
+++ b/include/accel/tcg/iommu.h
@@ -16,22 +16,23 @@
/**
* iotlb_to_section:
- * @cpu: CPU performing the access
+ * @as: Address space to access
* @index: TCG CPU IOTLB entry
*
* Given a TCG CPU IOTLB entry, return the MemoryRegionSection that
* it refers to. @index will have been initially created and returned
* by memory_region_section_get_iotlb().
*/
-MemoryRegionSection *iotlb_to_section(CPUState *cpu,
- hwaddr index, MemTxAttrs attrs);
+struct MemoryRegionSection *iotlb_to_section(AddressSpace *as,
+ hwaddr index, MemTxAttrs attrs);
MemoryRegionSection *address_space_translate_for_iotlb(CPUState *cpu,
int asidx,
hwaddr addr,
hwaddr *xlat,
hwaddr *plen,
- MemTxAttrs attrs,
+ MemTxAttrs *attrs,
+ AddressSpace **as,
int *prot);
hwaddr memory_region_section_get_iotlb(CPUState *cpu,
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index c0ca4b6905..a27d8feefc 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -269,6 +269,8 @@ struct CPUTLBEntryFull {
bool guarded;
} arm;
} extra;
+
+ AddressSpace *as;
};
/*
diff --git a/system/physmem.c b/system/physmem.c
index cf7146b224..52156325d9 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -688,7 +688,8 @@ void tcg_iommu_init_notifier_list(CPUState *cpu)
MemoryRegionSection *
address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr orig_addr,
hwaddr *xlat, hwaddr *plen,
- MemTxAttrs attrs, int *prot)
+ MemTxAttrs *attrs, AddressSpace **as,
+ int *prot)
{
MemoryRegionSection *section;
IOMMUMemoryRegion *iommu_mr;
@@ -696,7 +697,8 @@ address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr orig_addr,
IOMMUTLBEntry iotlb;
int iommu_idx;
hwaddr addr = orig_addr;
- AddressSpaceDispatch *d = address_space_to_dispatch(cpu->cpu_ases[asidx].as);
+ *as = cpu->cpu_ases[asidx].as;
+ AddressSpaceDispatch *d = address_space_to_dispatch(*as);
for (;;) {
section = address_space_translate_internal(d, addr, &addr, plen, false);
@@ -708,13 +710,13 @@ address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr orig_addr,
imrc = memory_region_get_iommu_class_nocheck(iommu_mr);
- iommu_idx = imrc->attrs_to_index(iommu_mr, attrs);
+ iommu_idx = imrc->attrs_to_index(iommu_mr, *attrs);
tcg_register_iommu_notifier(cpu, iommu_mr, iommu_idx);
/* We need all the permissions, so pass IOMMU_NONE so the IOMMU
* doesn't short-cut its translation table walk.
*/
if (imrc->translate_attr) {
- iotlb = imrc->translate_attr(iommu_mr, addr, IOMMU_NONE, &attrs);
+ iotlb = imrc->translate_attr(iommu_mr, addr, IOMMU_NONE, attrs);
} else {
iotlb = imrc->translate(iommu_mr, addr, IOMMU_NONE, iommu_idx);
}
@@ -735,7 +737,8 @@ address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr orig_addr,
goto translate_fail;
}
- d = flatview_to_dispatch(address_space_to_flatview(iotlb.target_as));
+ *as = iotlb.target_as;
+ d = flatview_to_dispatch(address_space_to_flatview(*as));
}
assert(!memory_region_is_iommu(section->mr));
@@ -756,12 +759,12 @@ translate_fail:
return &d->map.sections[PHYS_SECTION_UNASSIGNED];
}
-MemoryRegionSection *iotlb_to_section(CPUState *cpu,
+
+MemoryRegionSection *iotlb_to_section(AddressSpace *as,
hwaddr index, MemTxAttrs attrs)
{
- int asidx = cpu_asidx_from_attrs(cpu, attrs);
- CPUAddressSpace *cpuas = &cpu->cpu_ases[asidx];
- AddressSpaceDispatch *d = address_space_to_dispatch(cpuas->as);
+ assert(as);
+ AddressSpaceDispatch *d = address_space_to_dispatch(as);
int section_index = index & ~TARGET_PAGE_MASK;
MemoryRegionSection *ret;
@@ -3102,6 +3105,9 @@ static void tcg_commit(MemoryListener *listener)
* That said, the listener is also called during realize, before
* all of the tcg machinery for run-on is initialized: thus halt_cond.
*/
+// Why are these removed ?
+// cpu_reloading_memory_map();
+// tlb_flush(cpuas->cpu);
if (cpu->halt_cond) {
async_run_on_cpu(cpu, tcg_commit_cpu, RUN_ON_CPU_HOST_PTR(cpuas));
} else {
--
2.51.1
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
Also turns out that we have a very similar issue (though not exactly the same, and unfortunately doesn’t get fixed by this patch)
If an instruction is going to do multiple accesses to memory - (something that sets an area of memory to zero, or a DMA or….) typically we probe it, if that fails, go ahead and do the access using MMIO. If (by chance) we then get a Memory region added, lots of bad stuff happens.
One could expect many things - there used to be a cache which hid this issue - you could also expect memory regions to be added only ‘outside’ of an instruction execution block - or you could expect that adding a memory region shouldn’t break the MMIO access, or …..
On this one we’re like some guidance.
In general, I think we should build some Qemu tests that specifically test the case that memory is/is not MMIO (and, potentially, stress the case where those assumptions change)….
Cheers
Mark.
> On 9 Dec 2025, at 13:14, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.
>
> Mark Burton <mburton@qti.qualcomm.com> writes:
>
>> Just posting this here for the discussion this afternoon.
>>
>> Cheers
>> Mark.
>>
>>
>> [2. 0001-Record-AddressSpace-in-full-tlb-so-access-to-MMIO-vi.patch --- text/x-diff; 0001-Record-AddressSpace-in-full-tlb-so-access-to-MMIO-vi.patch]...
>
> We were discussing last week how we should break the dependency on
> CPUState for AddressSpaces while Gustavo was looking at adding the
> FEAT_MEC code.
>
> They are really a function of the machine where most CPUs will share the
> same set of views. However we still need a way to resolve the AS from
> the CPUs perspective.
>
> Copying inline for easier commenting:
>
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index b09229dae8..4b1aa9df71 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -1073,7 +1073,8 @@ void tlb_set_page_full(CPUState *cpu, int mmu_idx,
> prot = full->prot;
> asidx = cpu_asidx_from_attrs(cpu, full->attrs);
> section = address_space_translate_for_iotlb(cpu, asidx, paddr_page,
> - &xlat, &sz, full->attrs, &prot);
> + &xlat, &sz, &full->attrs,
> + &full->as, &prot);
> assert(sz >= TARGET_PAGE_SIZE);
>
> tlb_debug("vaddr=%016" VADDR_PRIx " paddr=0x" HWADDR_FMT_plx
> @@ -1294,13 +1295,13 @@ static inline void cpu_unaligned_access(CPUState *cpu, vaddr addr,
> }
>
> static MemoryRegionSection *
> -io_prepare(hwaddr *out_offset, CPUState *cpu, hwaddr xlat,
> +io_prepare(hwaddr *out_offset, CPUState *cpu, AddressSpace *as, hwaddr xlat,
> MemTxAttrs attrs, vaddr addr, uintptr_t retaddr)
> {
> MemoryRegionSection *section;
> hwaddr mr_offset;
>
> - section = iotlb_to_section(cpu, xlat, attrs);
> + section = iotlb_to_section(as, xlat, attrs);
> mr_offset = (xlat & TARGET_PAGE_MASK) + addr;
> cpu->mem_io_pc = retaddr;
> if (!cpu->neg.can_do_io) {
> @@ -1618,7 +1619,7 @@ bool tlb_plugin_lookup(CPUState *cpu, vaddr addr, int mmu_idx,
> /* We must have an iotlb entry for MMIO */
> if (tlb_addr & TLB_MMIO) {
> MemoryRegionSection *section =
> - iotlb_to_section(cpu, full->xlat_section & ~TARGET_PAGE_MASK,
> + iotlb_to_section(full->as, full->xlat_section & ~TARGET_PAGE_MASK,
> full->attrs);
> data->is_io = true;
> data->mr = section->mr;
> @@ -2028,7 +2029,8 @@ static uint64_t do_ld_mmio_beN(CPUState *cpu, CPUTLBEntryFull *full,
> tcg_debug_assert(size > 0 && size <= 8);
>
> attrs = full->attrs;
> - section = io_prepare(&mr_offset, cpu, full->xlat_section, attrs, addr, ra);
> + section = io_prepare(&mr_offset, cpu, full->as,
> + full->xlat_section, attrs, addr, ra);
> mr = section->mr;
>
> BQL_LOCK_GUARD();
> @@ -2049,7 +2051,8 @@ static Int128 do_ld16_mmio_beN(CPUState *cpu, CPUTLBEntryFull *full,
> tcg_debug_assert(size > 8 && size <= 16);
>
> attrs = full->attrs;
> - section = io_prepare(&mr_offset, cpu, full->xlat_section, attrs, addr, ra);
> + section = io_prepare(&mr_offset, cpu, full->as,
> + full->xlat_section, attrs, addr, ra);
> mr = section->mr;
>
> BQL_LOCK_GUARD();
> @@ -2593,7 +2596,8 @@ static uint64_t do_st_mmio_leN(CPUState *cpu, CPUTLBEntryFull *full,
> tcg_debug_assert(size > 0 && size <= 8);
>
> attrs = full->attrs;
> - section = io_prepare(&mr_offset, cpu, full->xlat_section, attrs, addr, ra);
> + section = io_prepare(&mr_offset, cpu, full->as,
> + full->xlat_section, attrs, addr, ra);
> mr = section->mr;
>
> BQL_LOCK_GUARD();
> @@ -2613,7 +2617,8 @@ static uint64_t do_st16_mmio_leN(CPUState *cpu, CPUTLBEntryFull *full,
> tcg_debug_assert(size > 8 && size <= 16);
>
> attrs = full->attrs;
> - section = io_prepare(&mr_offset, cpu, full->xlat_section, attrs, addr, ra);
> + section = io_prepare(&mr_offset, cpu, full->as,
> + full->xlat_section, attrs, addr, ra);
> mr = section->mr;
>
> BQL_LOCK_GUARD();
> diff --git a/include/accel/tcg/iommu.h b/include/accel/tcg/iommu.h
> index 90cfd6c0ed..ac50e50601 100644
> --- a/include/accel/tcg/iommu.h
> +++ b/include/accel/tcg/iommu.h
> @@ -16,22 +16,23 @@
>
> /**
> * iotlb_to_section:
> - * @cpu: CPU performing the access
> + * @as: Address space to access
> * @index: TCG CPU IOTLB entry
> *
> * Given a TCG CPU IOTLB entry, return the MemoryRegionSection that
> * it refers to. @index will have been initially created and returned
> * by memory_region_section_get_iotlb().
> */
> -MemoryRegionSection *iotlb_to_section(CPUState *cpu,
> - hwaddr index, MemTxAttrs attrs);
> +struct MemoryRegionSection *iotlb_to_section(AddressSpace *as,
> + hwaddr index, MemTxAttrs attrs);
>
> MemoryRegionSection *address_space_translate_for_iotlb(CPUState *cpu,
> int asidx,
> hwaddr addr,
> hwaddr *xlat,
> hwaddr *plen,
> - MemTxAttrs attrs,
> + MemTxAttrs *attrs,
> + AddressSpace **as,
> int *prot);
>
> hwaddr memory_region_section_get_iotlb(CPUState *cpu,
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index c0ca4b6905..a27d8feefc 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -269,6 +269,8 @@ struct CPUTLBEntryFull {
> bool guarded;
> } arm;
> } extra;
> +
> + AddressSpace *as;
> };
>
> /*
> diff --git a/system/physmem.c b/system/physmem.c
> index cf7146b224..52156325d9 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -688,7 +688,8 @@ void tcg_iommu_init_notifier_list(CPUState *cpu)
> MemoryRegionSection *
> address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr orig_addr,
> hwaddr *xlat, hwaddr *plen,
> - MemTxAttrs attrs, int *prot)
> + MemTxAttrs *attrs, AddressSpace **as,
> + int *prot)
> {
> MemoryRegionSection *section;
> IOMMUMemoryRegion *iommu_mr;
> @@ -696,7 +697,8 @@ address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr orig_addr,
> IOMMUTLBEntry iotlb;
> int iommu_idx;
> hwaddr addr = orig_addr;
> - AddressSpaceDispatch *d = address_space_to_dispatch(cpu->cpu_ases[asidx].as);
> + *as = cpu->cpu_ases[asidx].as;
> + AddressSpaceDispatch *d = address_space_to_dispatch(*as);
>
> for (;;) {
> section = address_space_translate_internal(d, addr, &addr, plen, false);
> @@ -708,13 +710,13 @@ address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr orig_addr,
>
> imrc = memory_region_get_iommu_class_nocheck(iommu_mr);
>
> - iommu_idx = imrc->attrs_to_index(iommu_mr, attrs);
> + iommu_idx = imrc->attrs_to_index(iommu_mr, *attrs);
> tcg_register_iommu_notifier(cpu, iommu_mr, iommu_idx);
> /* We need all the permissions, so pass IOMMU_NONE so the IOMMU
> * doesn't short-cut its translation table walk.
> */
> if (imrc->translate_attr) {
> - iotlb = imrc->translate_attr(iommu_mr, addr, IOMMU_NONE, &attrs);
> + iotlb = imrc->translate_attr(iommu_mr, addr, IOMMU_NONE, attrs);
> } else {
> iotlb = imrc->translate(iommu_mr, addr, IOMMU_NONE, iommu_idx);
> }
> @@ -735,7 +737,8 @@ address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr orig_addr,
> goto translate_fail;
> }
>
> - d = flatview_to_dispatch(address_space_to_flatview(iotlb.target_as));
> + *as = iotlb.target_as;
> + d = flatview_to_dispatch(address_space_to_flatview(*as));
> }
>
> assert(!memory_region_is_iommu(section->mr));
> @@ -756,12 +759,12 @@ translate_fail:
> return &d->map.sections[PHYS_SECTION_UNASSIGNED];
> }
>
> -MemoryRegionSection *iotlb_to_section(CPUState *cpu,
> +
> +MemoryRegionSection *iotlb_to_section(AddressSpace *as,
> hwaddr index, MemTxAttrs attrs)
> {
> - int asidx = cpu_asidx_from_attrs(cpu, attrs);
> - CPUAddressSpace *cpuas = &cpu->cpu_ases[asidx];
> - AddressSpaceDispatch *d = address_space_to_dispatch(cpuas->as);
> + assert(as);
> + AddressSpaceDispatch *d = address_space_to_dispatch(as);
> int section_index = index & ~TARGET_PAGE_MASK;
> MemoryRegionSection *ret;
>
> @@ -3102,6 +3105,9 @@ static void tcg_commit(MemoryListener *listener)
> * That said, the listener is also called during realize, before
> * all of the tcg machinery for run-on is initialized: thus halt_cond.
> */
> +// Why are these removed ?
> +// cpu_reloading_memory_map();
> +// tlb_flush(cpuas->cpu);
> if (cpu->halt_cond) {
> async_run_on_cpu(cpu, tcg_commit_cpu, RUN_ON_CPU_HOST_PTR(cpuas));
> } else {
> --
> 2.51.1
>
> --
> Alex Bennée
> Virtualisation Tech Lead @ Linaro
On Tue, 9 Dec 2025 12:22:09 +0000 Mark Burton <mburton@qti.qualcomm.com> wrote:
>
> Also turns out that we have a very similar issue (though not exactly the
> same, and unfortunately doesn’t get fixed by this patch)
>
> If an instruction is going to do multiple accesses to memory - (something
> that sets an area of memory to zero, or a DMA or….) typically we probe it, if
> that fails, go ahead and do the access using MMIO. If (by chance) we then get
> a Memory region added, lots of bad stuff happens.
FWIW, this exposed a bug downstream, for which the following workaround was
temporary applied:
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index b09229dae8..90011e5671 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -2529,4 +2529,5 @@ static uint64_t int_st_mmio_leN(CPUState *cpu, CPUTLBEntryFull *full,
MemoryRegion *mr, hwaddr mr_offset)
{
+ vaddr orig_vaddr = addr;
do {
MemOp this_mop;
@@ -2570,5 +2571,6 @@ static uint64_t int_st_mmio_leN(CPUState *cpu, CPUTLBEntryFull *full,
}
if (this_size == 8) {
- return 0;
+ val_le = 0;
+ break;
}
@@ -2579,4 +2581,9 @@ static uint64_t int_st_mmio_leN(CPUState *cpu, CPUTLBEntryFull *full,
} while (size);
+ if (!memory_region_get_iommu(mr)) {
+ tlb_set_page(cpu, orig_vaddr, cpu_get_phys_page_debug(cpu, orig_vaddr),
+ PAGE_READ | PAGE_WRITE, mmu_idx, TARGET_PAGE_SIZE);
+ }
+
return val_le;
}
--- 8< ---
> One could expect many things - there used to be a cache which hid this issue
> - you could also expect memory regions to be added only ‘outside’ of an
> instruction execution block - or you could expect that adding a memory region
> shouldn’t break the MMIO access, or …..
The cache Mark has mentioned here is `cpuas->memory_dispatch`, which was
removed at 2865bf1c57 (system/physmem: fix use-after-free with dispatch,
2025-07-24).
Alex Bennée <alex.bennee@linaro.org> writes:
> Mark Burton <mburton@qti.qualcomm.com> writes:
>
>> Just posting this here for the discussion this afternoon.
>>
>> Cheers
>> Mark.
>>
>>
>> [2. 0001-Record-AddressSpace-in-full-tlb-so-access-to-MMIO-vi.patch --- text/x-diff; 0001-Record-AddressSpace-in-full-tlb-so-access-to-MMIO-vi.patch]...
>
> We were discussing last week how we should break the dependency on
> CPUState for AddressSpaces while Gustavo was looking at adding the
> FEAT_MEC code.
>
> They are really a function of the machine where most CPUs will share the
> same set of views. However we still need a way to resolve the AS from
> the CPUs perspective.
>
> Copying inline for easier commenting:
>
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index b09229dae8..4b1aa9df71 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -1073,7 +1073,8 @@ void tlb_set_page_full(CPUState *cpu, int mmu_idx,
> prot = full->prot;
> asidx = cpu_asidx_from_attrs(cpu, full->attrs);
> section = address_space_translate_for_iotlb(cpu, asidx, paddr_page,
> - &xlat, &sz, full->attrs, &prot);
> + &xlat, &sz, &full->attrs,
> + &full->as, &prot);
> assert(sz >= TARGET_PAGE_SIZE);
>
> tlb_debug("vaddr=%016" VADDR_PRIx " paddr=0x" HWADDR_FMT_plx
> @@ -1294,13 +1295,13 @@ static inline void cpu_unaligned_access(CPUState *cpu, vaddr addr,
> }
>
> static MemoryRegionSection *
> -io_prepare(hwaddr *out_offset, CPUState *cpu, hwaddr xlat,
> +io_prepare(hwaddr *out_offset, CPUState *cpu, AddressSpace *as, hwaddr xlat,
> MemTxAttrs attrs, vaddr addr, uintptr_t retaddr)
> {
> MemoryRegionSection *section;
> hwaddr mr_offset;
>
> - section = iotlb_to_section(cpu, xlat, attrs);
> + section = iotlb_to_section(as, xlat, attrs);
> mr_offset = (xlat & TARGET_PAGE_MASK) + addr;
> cpu->mem_io_pc = retaddr;
> if (!cpu->neg.can_do_io) {
> @@ -1618,7 +1619,7 @@ bool tlb_plugin_lookup(CPUState *cpu, vaddr addr, int mmu_idx,
> /* We must have an iotlb entry for MMIO */
> if (tlb_addr & TLB_MMIO) {
> MemoryRegionSection *section =
> - iotlb_to_section(cpu, full->xlat_section & ~TARGET_PAGE_MASK,
> + iotlb_to_section(full->as, full->xlat_section & ~TARGET_PAGE_MASK,
> full->attrs);
> data->is_io = true;
> data->mr = section->mr;
> @@ -2028,7 +2029,8 @@ static uint64_t do_ld_mmio_beN(CPUState *cpu, CPUTLBEntryFull *full,
> tcg_debug_assert(size > 0 && size <= 8);
>
> attrs = full->attrs;
> - section = io_prepare(&mr_offset, cpu, full->xlat_section, attrs, addr, ra);
> + section = io_prepare(&mr_offset, cpu, full->as,
> + full->xlat_section, attrs, addr, ra);
> mr = section->mr;
>
> BQL_LOCK_GUARD();
> @@ -2049,7 +2051,8 @@ static Int128 do_ld16_mmio_beN(CPUState *cpu, CPUTLBEntryFull *full,
> tcg_debug_assert(size > 8 && size <= 16);
>
> attrs = full->attrs;
> - section = io_prepare(&mr_offset, cpu, full->xlat_section, attrs, addr, ra);
> + section = io_prepare(&mr_offset, cpu, full->as,
> + full->xlat_section, attrs, addr, ra);
> mr = section->mr;
>
> BQL_LOCK_GUARD();
> @@ -2593,7 +2596,8 @@ static uint64_t do_st_mmio_leN(CPUState *cpu, CPUTLBEntryFull *full,
> tcg_debug_assert(size > 0 && size <= 8);
>
> attrs = full->attrs;
> - section = io_prepare(&mr_offset, cpu, full->xlat_section, attrs, addr, ra);
> + section = io_prepare(&mr_offset, cpu, full->as,
> + full->xlat_section, attrs, addr, ra);
> mr = section->mr;
>
> BQL_LOCK_GUARD();
> @@ -2613,7 +2617,8 @@ static uint64_t do_st16_mmio_leN(CPUState *cpu, CPUTLBEntryFull *full,
> tcg_debug_assert(size > 8 && size <= 16);
>
> attrs = full->attrs;
> - section = io_prepare(&mr_offset, cpu, full->xlat_section, attrs, addr, ra);
> + section = io_prepare(&mr_offset, cpu, full->as,
> + full->xlat_section, attrs, addr, ra);
> mr = section->mr;
>
> BQL_LOCK_GUARD();
> diff --git a/include/accel/tcg/iommu.h b/include/accel/tcg/iommu.h
> index 90cfd6c0ed..ac50e50601 100644
> --- a/include/accel/tcg/iommu.h
> +++ b/include/accel/tcg/iommu.h
> @@ -16,22 +16,23 @@
>
> /**
> * iotlb_to_section:
> - * @cpu: CPU performing the access
> + * @as: Address space to access
> * @index: TCG CPU IOTLB entry
> *
> * Given a TCG CPU IOTLB entry, return the MemoryRegionSection that
> * it refers to. @index will have been initially created and returned
> * by memory_region_section_get_iotlb().
> */
> -MemoryRegionSection *iotlb_to_section(CPUState *cpu,
> - hwaddr index, MemTxAttrs attrs);
> +struct MemoryRegionSection *iotlb_to_section(AddressSpace *as,
> + hwaddr index, MemTxAttrs attrs);
>
> MemoryRegionSection *address_space_translate_for_iotlb(CPUState *cpu,
> int asidx,
> hwaddr addr,
> hwaddr *xlat,
> hwaddr *plen,
> - MemTxAttrs attrs,
> + MemTxAttrs *attrs,
> + AddressSpace **as,
> int *prot);
>
> hwaddr memory_region_section_get_iotlb(CPUState *cpu,
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index c0ca4b6905..a27d8feefc 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -269,6 +269,8 @@ struct CPUTLBEntryFull {
> bool guarded;
> } arm;
> } extra;
> +
> + AddressSpace *as;
> };
>
> /*
> diff --git a/system/physmem.c b/system/physmem.c
> index cf7146b224..52156325d9 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -688,7 +688,8 @@ void tcg_iommu_init_notifier_list(CPUState *cpu)
> MemoryRegionSection *
> address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr orig_addr,
> hwaddr *xlat, hwaddr *plen,
> - MemTxAttrs attrs, int *prot)
> + MemTxAttrs *attrs, AddressSpace **as,
> + int *prot)
> {
> MemoryRegionSection *section;
> IOMMUMemoryRegion *iommu_mr;
> @@ -696,7 +697,8 @@ address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr orig_addr,
> IOMMUTLBEntry iotlb;
> int iommu_idx;
> hwaddr addr = orig_addr;
> - AddressSpaceDispatch *d = address_space_to_dispatch(cpu->cpu_ases[asidx].as);
> + *as = cpu->cpu_ases[asidx].as;
> + AddressSpaceDispatch *d = address_space_to_dispatch(*as);
Should address_space_translate_for_iotlb be decoupled from CPUs entirely
and just be passed the computed address space from cputlb? The only
other reason we need cpu here is for the tcg_register_iommu_notifier()
call which could be passed the notifier directly.
Do we have notifiers for IOMMU's themselves to track changes or is this
something we only care about for cputlb because of the fast/slowpath handling?
>
> for (;;) {
> section = address_space_translate_internal(d, addr, &addr, plen, false);
> @@ -708,13 +710,13 @@ address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr orig_addr,
>
> imrc = memory_region_get_iommu_class_nocheck(iommu_mr);
>
> - iommu_idx = imrc->attrs_to_index(iommu_mr, attrs);
> + iommu_idx = imrc->attrs_to_index(iommu_mr, *attrs);
> tcg_register_iommu_notifier(cpu, iommu_mr, iommu_idx);
> /* We need all the permissions, so pass IOMMU_NONE so the IOMMU
> * doesn't short-cut its translation table walk.
> */
> if (imrc->translate_attr) {
> - iotlb = imrc->translate_attr(iommu_mr, addr, IOMMU_NONE, &attrs);
> + iotlb = imrc->translate_attr(iommu_mr, addr, IOMMU_NONE, attrs);
> } else {
> iotlb = imrc->translate(iommu_mr, addr, IOMMU_NONE, iommu_idx);
> }
> @@ -735,7 +737,8 @@ address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr orig_addr,
> goto translate_fail;
> }
>
> - d = flatview_to_dispatch(address_space_to_flatview(iotlb.target_as));
> + *as = iotlb.target_as;
> + d = flatview_to_dispatch(address_space_to_flatview(*as));
> }
>
> assert(!memory_region_is_iommu(section->mr));
> @@ -756,12 +759,12 @@ translate_fail:
> return &d->map.sections[PHYS_SECTION_UNASSIGNED];
> }
>
> -MemoryRegionSection *iotlb_to_section(CPUState *cpu,
> +
> +MemoryRegionSection *iotlb_to_section(AddressSpace *as,
> hwaddr index, MemTxAttrs attrs)
> {
> - int asidx = cpu_asidx_from_attrs(cpu, attrs);
> - CPUAddressSpace *cpuas = &cpu->cpu_ases[asidx];
> - AddressSpaceDispatch *d = address_space_to_dispatch(cpuas->as);
> + assert(as);
> + AddressSpaceDispatch *d = address_space_to_dispatch(as);
> int section_index = index & ~TARGET_PAGE_MASK;
> MemoryRegionSection *ret;
>
> @@ -3102,6 +3105,9 @@ static void tcg_commit(MemoryListener *listener)
> * That said, the listener is also called during realize, before
> * all of the tcg machinery for run-on is initialized: thus halt_cond.
> */
> +// Why are these removed ?
> +// cpu_reloading_memory_map();
> +// tlb_flush(cpuas->cpu);
> if (cpu->halt_cond) {
> async_run_on_cpu(cpu, tcg_commit_cpu, RUN_ON_CPU_HOST_PTR(cpuas));
> } else {
> --
> 2.51.1
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
© 2016 - 2025 Red Hat, Inc.