* [PULL 1/2] target/arm: Don't do two-stage lookup if stage 2 is disabled
2022-11-22 16:39 [PULL 0/2] target-arm queue Peter Maydell
@ 2022-11-22 16:39 ` Peter Maydell
2022-11-22 16:39 ` [PULL 2/2] target/arm: Use signed quantity to represent VMSAv8-64 translation level Peter Maydell
2022-11-22 20:36 ` [PULL 0/2] target-arm queue Stefan Hajnoczi
2 siblings, 0 replies; 4+ messages in thread
From: Peter Maydell @ 2022-11-22 16:39 UTC (permalink / raw)
To: qemu-devel
In get_phys_addr_with_struct(), we call get_phys_addr_twostage() if
the CPU supports EL2. However, we don't check here that stage 2 is
actually enabled. Instead we only check that inside
get_phys_addr_twostage() to skip stage 2 translation. This means
that even if stage 2 is disabled we still tell the stage 1 lookup to
do its page table walks via stage 2.
This works by luck for normal CPU accesses, but it breaks for debug
accesses, which are used by the disassembler and also by semihosting
file reads and writes, because the debug case takes a different code
path inside S1_ptw_translate().
This means that setups that use semihosting for file loads are broken
(a regression since 7.1, introduced in recent ptw refactoring), and
that sometimes disassembly in debug logs reports "unable to read
memory" rather than showing the guest insns.
Fix the bug by hoisting the "is stage 2 enabled?" check up to
get_phys_addr_with_struct(), so that we handle S2 disabled the same
way we do the "no EL2" case, with a simple single stage lookup.
Reported-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Message-id: 20221121212404.1450382-1-peter.maydell@linaro.org
---
target/arm/ptw.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 9a6277d862f..8ca468d65bc 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -2612,8 +2612,8 @@ static bool get_phys_addr_twostage(CPUARMState *env, S1Translate *ptw,
ret = get_phys_addr_with_struct(env, ptw, address, access_type, result, fi);
- /* If S1 fails or S2 is disabled, return early. */
- if (ret || regime_translation_disabled(env, ARMMMUIdx_Stage2, is_secure)) {
+ /* If S1 fails, return early. */
+ if (ret) {
return ret;
}
@@ -2739,7 +2739,8 @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw,
* Otherwise, a stage1+stage2 translation is just stage 1.
*/
ptw->in_mmu_idx = mmu_idx = s1_mmu_idx;
- if (arm_feature(env, ARM_FEATURE_EL2)) {
+ if (arm_feature(env, ARM_FEATURE_EL2) &&
+ !regime_translation_disabled(env, ARMMMUIdx_Stage2, is_secure)) {
return get_phys_addr_twostage(env, ptw, address, access_type,
result, fi);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PULL 2/2] target/arm: Use signed quantity to represent VMSAv8-64 translation level
2022-11-22 16:39 [PULL 0/2] target-arm queue Peter Maydell
2022-11-22 16:39 ` [PULL 1/2] target/arm: Don't do two-stage lookup if stage 2 is disabled Peter Maydell
@ 2022-11-22 16:39 ` Peter Maydell
2022-11-22 20:36 ` [PULL 0/2] target-arm queue Stefan Hajnoczi
2 siblings, 0 replies; 4+ messages in thread
From: Peter Maydell @ 2022-11-22 16:39 UTC (permalink / raw)
To: qemu-devel
From: Ard Biesheuvel <ardb@kernel.org>
The LPA2 extension implements 52-bit virtual addressing for 4k and 16k
translation granules, and for the former, this means an additional level
of translation is needed. This means we start counting at -1 instead of
0 when doing a walk, and so 'level' is now a signed quantity, and should
be typed as such. So turn it from uint32_t into int32_t.
This avoids a level of -1 getting misinterpreted as being >= 3, and
terminating a page table walk prematurely with a bogus output address.
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Cc: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
target/arm/ptw.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 8ca468d65bc..f812734bfb2 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -1172,7 +1172,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
ARMCPU *cpu = env_archcpu(env);
ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
bool is_secure = ptw->in_secure;
- uint32_t level;
+ int32_t level;
ARMVAParameters param;
uint64_t ttbr;
hwaddr descaddr, indexmask, indexmask_grainsize;
@@ -1302,7 +1302,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
*/
uint32_t sl0 = extract32(tcr, 6, 2);
uint32_t sl2 = extract64(tcr, 33, 1);
- uint32_t startlevel;
+ int32_t startlevel;
bool ok;
/* SL2 is RES0 unless DS=1 & 4kb granule. */
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread