On Tue, 25 Aug 2026 at 15:53, Alex Bennée <[email protected]> wrote:
>
> In the Armv7 ARM (DDI0406C) any walk of the short descriptor
> table (TranslationTableWalkSD()) which has a second stage has the
> opportunity to fault with a translation fault in
> SecondStageTranslate() before a potential domain checking fault.
>
> Moving the check down and lightly re-factoring setting level = 2 we
> can more closely match the architectural behaviour.
>
> Reported-by: Karl Mehltretter (@kmehltretter)
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4233
> Signed-off-by: Alex Bennée <[email protected]>
>
> ---
> I've only checked the get_phys_addr_v6 change against the Arm ARM but
> I've fairly confident the v5 one is the same. I've bundled everything
> together for review but this patch can't be merged as is because it
> does too much in one commit. Some open questions:
>
> - should we properly factor out a boot_v6.S and share with other tests
I don't care, but would prefer the test case in its own commit
rather than in the same commit as the bugfix.
> - could the ptw code better match the psuedocode (e.g. case l1desc<1:0>)
> - which Arm ARM should be checked for get_phys_addr_v5
DDI0100E, which corresponds to the 2nd edition printed book version
(which covers v5TE). I happen to own a copy of that, which is what
I've used for the review of the v5 parts below.
> ---
> target/arm/ptw.c | 31 ++--
> tests/tcg/arm/system/test-domainfault.c | 148 ++++++++++++++++++
> tests/tcg/arm/Makefile.softmmu-target | 23 ++-
> tests/tcg/arm/system/test-domainfault-start.S | 62 ++++++++
> tests/tcg/arm/system/test-domainfault.ld | 17 ++
> 5 files changed, 264 insertions(+), 17 deletions(-)
> create mode 100644 tests/tcg/arm/system/test-domainfault.c
> create mode 100644 tests/tcg/arm/system/test-domainfault-start.S
> create mode 100644 tests/tcg/arm/system/test-domainfault.ld
>
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index a29de0385f4..ddfbb79beb6 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -1172,19 +1172,13 @@ static bool get_phys_addr_v5(CPUARMState *env,
> S1Translate *ptw,
> fi->type = ARMFault_Translation;
> goto do_fault;
> }
> - if (type != 2) {
> - level = 2;
> - }
> - if (domain_prot == 0 || domain_prot == 2) {
> - fi->type = ARMFault_Domain;
> - goto do_fault;
> - }
> if (type == 2) {
> /* 1Mb section. */
> phys_addr = (desc & 0xfff00000) | (address & 0x000fffff);
> ap = (desc >> 10) & 3;
> result->f.lg_page_size = 20; /* 1MB */
> } else {
> + level = 2;
> /* Lookup l2 entry. */
> if (type == 1) {
> /* Coarse pagetable. */
> @@ -1239,6 +1233,10 @@ static bool get_phys_addr_v5(CPUARMState *env,
> S1Translate *ptw,
> g_assert_not_reached();
> }
> }
> + if (domain_prot == 0 || domain_prot == 2) {
> + fi->type = ARMFault_Domain;
> + goto do_fault;
> + }
> result->f.prot = ap_to_rw_prot(env, ptw->in_mmu_idx, ap, domain_prot);
> result->f.prot |= result->f.prot ? PAGE_EXEC : 0;
> if (ptw->in_prot_check & ~result->f.prot) {
This matches Table 3-5 in the v5 Arm ARM (ARM DDI 0100E), which
lists the priority encoding of different MMU faults. Domain faults
are lower priority than Translation faults but higher priority
than Permission faults. It also matches the flowchart in Figure
3-9, which does the domain check only after loading the second
level descriptor.
> @@ -1254,6 +1252,7 @@ do_fault:
> return false;
> }
>
> +/* See TranslationTableWalkSD() in Armv7 ARM (DDI0406C) */
...but note that the domain and permission checks we do in this function
are handled in the pseudocode in its caller TranslateAddressV().
> static bool get_phys_addr_v6(CPUARMState *env, S1Translate *ptw,
> uint32_t address, MMUAccessType access_type,
> GetPhysAddrResult *result, ARMMMUFaultInfo *fi)
> @@ -1288,6 +1287,7 @@ static bool get_phys_addr_v6(CPUARMState *env,
> S1Translate *ptw,
> if (fi->type != ARMFault_None) {
> goto do_fault;
> }
> + /* l1desc<1:0> */
I think personally I wouldn't bother with this comment.
> type = (desc & 3);
> if (type == 0 || (type == 3 && !cpu_isar_feature(aa32_pxn, cpu))) {
> /* Section translation fault, or attempt to use the encoding
> @@ -1305,15 +1305,6 @@ static bool get_phys_addr_v6(CPUARMState *env,
> S1Translate *ptw,
> } else {
> dacr = env->cp15.dacr_s;
> }
> - if (type == 1) {
> - level = 2;
> - }
> - domain_prot = (dacr >> (domain * 2)) & 3;
> - if (domain_prot == 0 || domain_prot == 2) {
> - /* Section or Page domain fault */
> - fi->type = ARMFault_Domain;
> - goto do_fault;
> - }
> if (type != 1) {
> if (desc & (1 << 18)) {
> /* Supersection. */
> @@ -1331,6 +1322,7 @@ static bool get_phys_addr_v6(CPUARMState *env,
> S1Translate *ptw,
> pxn = desc & 1;
> ns = extract32(desc, 19, 1);
> } else {
> + level = 2;
> if (cpu_isar_feature(aa32_pxn, cpu)) {
> pxn = (desc >> 2) & 1;
> }
> @@ -1373,6 +1365,13 @@ static bool get_phys_addr_v6(CPUARMState *env,
> S1Translate *ptw,
> */
> out_space = ARMSS_NonSecure;
> }
> + /* Extract from DACR indexed by domain */
> + domain_prot = (dacr >> (domain * 2)) & 3;
> + if (domain_prot == 0 || domain_prot == 2) {
> + /* Section or Page domain fault */
> + fi->type = ARMFault_Domain;
> + goto do_fault;
> + }
> if (domain_prot == 3) {
> result->f.prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> } else {
This one is trickier because v6 introduced the AccessFlag bit and
associated fault. If we're taking an AccessFlag fault that has to
be taken before the domain fault (the pseudocode does it inside
TranslationTableWalkSD()). So for us the place where we check
domain_prot needs to go somewhere after the AccessFlag fault check
but before the Permission fault check. At the moment you have it
before the AccessFlag check.
thanks
-- PMM
