From 0da067f2a83c61efc6f1688d4379269420838b28 Mon Sep 17 00:00:00 2001 From: Idan Horowitz Date: Fri, 1 Apr 2022 15:35:48 +0100 Subject: [PATCH 1/6] target/arm: Fix MTE access checks for disabled SEL2 While not mentioned anywhere in the actual specification text, the HCR_EL2.ATA bit is treated as '1' when EL2 is disabled at the current security state. This can be observed in the psuedo-code implementation of AArch64.AllocationTagAccessIsEnabled(). Signed-off-by: Idan Horowitz Reviewed-by: Richard Henderson Message-id: 20220328173107.311267-1-idan.horowitz@gmail.com Signed-off-by: Peter Maydell --- target/arm/helper.c | 2 +- target/arm/internals.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/target/arm/helper.c b/target/arm/helper.c index 812ca591f4..3aeaea4068 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -7176,7 +7176,7 @@ static CPAccessResult access_mte(CPUARMState *env, const ARMCPRegInfo *ri, { int el = arm_current_el(env); - if (el < 2 && arm_feature(env, ARM_FEATURE_EL2)) { + if (el < 2 && arm_is_el2_enabled(env)) { uint64_t hcr = arm_hcr_el2_eff(env); if (!(hcr & HCR_ATA) && (!(hcr & HCR_E2H) || !(hcr & HCR_TGE))) { return CP_ACCESS_TRAP_EL2; diff --git a/target/arm/internals.h b/target/arm/internals.h index a34be2e459..7f696cd36a 100644 --- a/target/arm/internals.h +++ b/target/arm/internals.h @@ -1094,7 +1094,7 @@ static inline bool allocation_tag_access_enabled(CPUARMState *env, int el, && !(env->cp15.scr_el3 & SCR_ATA)) { return false; } - if (el < 2 && arm_feature(env, ARM_FEATURE_EL2)) { + if (el < 2 && arm_is_el2_enabled(env)) { uint64_t hcr = arm_hcr_el2_eff(env); if (!(hcr & HCR_ATA) && (!(hcr & HCR_E2H) || !(hcr & HCR_TGE))) { return false; From d3b2d191119ee3e6364e470b9579e6353d202e54 Mon Sep 17 00:00:00 2001 From: Idan Horowitz Date: Fri, 1 Apr 2022 15:35:49 +0100 Subject: [PATCH 2/6] target/arm: Check VSTCR.SW when assigning the stage 2 output PA space As per the AArch64.SS2OutputPASpace() psuedo-code in the ARMv8 ARM when the PA space of the IPA is non secure, the output PA space is secure if and only if all of the bits VTCR., VSTCR. are not set. Signed-off-by: Idan Horowitz Reviewed-by: Richard Henderson Message-id: 20220327093427.1548629-2-idan.horowitz@gmail.com Signed-off-by: Peter Maydell --- target/arm/helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target/arm/helper.c b/target/arm/helper.c index 3aeaea4068..a65b39625d 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -12697,7 +12697,7 @@ bool get_phys_addr(CPUARMState *env, target_ulong address, } else { attrs->secure = !((env->cp15.vtcr_el2.raw_tcr & (VTCR_NSA | VTCR_NSW)) - || (env->cp15.vstcr_el2.raw_tcr & VSTCR_SA)); + || (env->cp15.vstcr_el2.raw_tcr & (VSTCR_SA | VSTCR_SW))); } } return 0; From bcd7a8cf38b7e9769f741419bc56675cbddb42c6 Mon Sep 17 00:00:00 2001 From: Idan Horowitz Date: Fri, 1 Apr 2022 15:35:49 +0100 Subject: [PATCH 3/6] target/arm: Take VSTCR.SW, VTCR.NSW into account in final stage 2 walk As per the AArch64.SS2InitialTTWState() psuedo-code in the ARMv8 ARM the initial PA space used for stage 2 table walks is assigned based on the SW and NSW bits of the VSTCR and VTCR registers. This was already implemented for the recursive stage 2 page table walks in S1_ptw_translate(), but was missing for the final stage 2 walk. Signed-off-by: Idan Horowitz Reviewed-by: Richard Henderson Message-id: 20220327093427.1548629-3-idan.horowitz@gmail.com Signed-off-by: Peter Maydell --- target/arm/helper.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/target/arm/helper.c b/target/arm/helper.c index a65b39625d..6fd5c27340 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -12657,6 +12657,16 @@ bool get_phys_addr(CPUARMState *env, target_ulong address, return ret; } + if (arm_is_secure_below_el3(env)) { + if (attrs->secure) { + attrs->secure = !(env->cp15.vstcr_el2.raw_tcr & VSTCR_SW); + } else { + attrs->secure = !(env->cp15.vtcr_el2.raw_tcr & VTCR_NSW); + } + } else { + assert(!attrs->secure); + } + s2_mmu_idx = attrs->secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2; is_el0 = mmu_idx == ARMMMUIdx_E10_0 || mmu_idx == ARMMMUIdx_SE10_0; From 6c05a866cf732efce5745fabea409603a9f334d0 Mon Sep 17 00:00:00 2001 From: Idan Horowitz Date: Fri, 1 Apr 2022 15:35:49 +0100 Subject: [PATCH 4/6] target/arm: Determine final stage 2 output PA space based on original IPA As per the AArch64.S2Walk() pseudo-code in the ARMv8 ARM, the final decision as to the output address's PA space based on the SA/SW/NSA/NSW bits needs to take the input IPA's PA space into account, and not the PA space of the result of the stage 2 walk itself. Signed-off-by: Idan Horowitz Reviewed-by: Richard Henderson Message-id: 20220327093427.1548629-4-idan.horowitz@gmail.com [PMM: fixed commit message typo] Signed-off-by: Peter Maydell --- target/arm/helper.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/target/arm/helper.c b/target/arm/helper.c index 6fd5c27340..7d14650615 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -12644,6 +12644,7 @@ bool get_phys_addr(CPUARMState *env, target_ulong address, hwaddr ipa; int s2_prot; int ret; + bool ipa_secure; ARMCacheAttrs cacheattrs2 = {}; ARMMMUIdx s2_mmu_idx; bool is_el0; @@ -12657,14 +12658,15 @@ bool get_phys_addr(CPUARMState *env, target_ulong address, return ret; } + ipa_secure = attrs->secure; if (arm_is_secure_below_el3(env)) { - if (attrs->secure) { + if (ipa_secure) { attrs->secure = !(env->cp15.vstcr_el2.raw_tcr & VSTCR_SW); } else { attrs->secure = !(env->cp15.vtcr_el2.raw_tcr & VTCR_NSW); } } else { - assert(!attrs->secure); + assert(!ipa_secure); } s2_mmu_idx = attrs->secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2; @@ -12701,7 +12703,7 @@ bool get_phys_addr(CPUARMState *env, target_ulong address, /* Check if IPA translates to secure or non-secure PA space. */ if (arm_is_secure_below_el3(env)) { - if (attrs->secure) { + if (ipa_secure) { attrs->secure = !(env->cp15.vstcr_el2.raw_tcr & (VSTCR_SA | VSTCR_SW)); } else { From 034e050dbd96db61ec35a34abe11c6088b5af373 Mon Sep 17 00:00:00 2001 From: Frederic Konrad Date: Fri, 1 Apr 2022 15:35:49 +0100 Subject: [PATCH 5/6] MAINTAINERS: change Fred Konrad's email address MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit frederic.konrad@adacore.com and konrad@adacore.com will stop working starting 2022-04-01. Use my personal email instead. Signed-off-by: Frederic Konrad Reviewed-by: Fabien Chouteau > Reviewed-by: Philippe Mathieu-Daudé Message-id: 1648643217-15811-1-git-send-email-frederic.konrad@adacore.com Signed-off-by: Peter Maydell --- .mailmap | 3 ++- MAINTAINERS | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.mailmap b/.mailmap index 09dcd8c216..2976a675ea 100644 --- a/.mailmap +++ b/.mailmap @@ -56,7 +56,8 @@ Alexander Graf Anthony Liguori Anthony Liguori Christian Borntraeger Filip Bozuta -Frederic Konrad +Frederic Konrad +Frederic Konrad Greg Kurz Huacai Chen Huacai Chen diff --git a/MAINTAINERS b/MAINTAINERS index cc364afef7..68142340bd 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1533,7 +1533,7 @@ F: include/hw/rtc/sun4v-rtc.h Leon3 M: Fabien Chouteau -M: KONRAD Frederic +M: Frederic Konrad S: Maintained F: hw/sparc/leon3.c F: hw/*/grlib* From a5b1e1ab662aa6dc42d5a913080fccbb8bf82e9b Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Fri, 1 Apr 2022 15:35:49 +0100 Subject: [PATCH 6/6] target/arm: Don't use DISAS_NORETURN in STXP !HAVE_CMPXCHG128 codegen In gen_store_exclusive(), if the host does not have a cmpxchg128 primitive then we generate bad code for STXP for storing two 64-bit values. We generate a call to the exit_atomic helper, which never returns, and set is_jmp to DISAS_NORETURN. However, this is forgetting that we have already emitted a brcond that jumps over this call for the case where we don't hold the exclusive. The effect is that we don't generate any code to end the TB for the exclusive-not-held execution path, which falls into the "exit with TB_EXIT_REQUESTED" code that gen_tb_end() emits. This then causes an assert at runtime when cpu_loop_exec_tb() sees an EXIT_REQUESTED TB return that wasn't for an interrupt or icount. In particular, you can hit this case when using the clang sanitizers and trying to run the xlnx-versal-virt acceptance test in 'make check-acceptance'. This bug was masked until commit 848126d11e93ff ("meson: move int128 checks from configure") because we used to set CONFIG_CMPXCHG128=1 and avoid the buggy codepath, but after that we do not. Fix the bug by not setting is_jmp. The code after the exit_atomic call up to the fail_label is dead, but TCG is smart enough to eliminate it. We do need to set 'tmp' to some valid value, though (in the same way the exit_atomic-using code in tcg/tcg-op.c does). Resolves: https://gitlab.com/qemu-project/qemu/-/issues/953 Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Message-id: 20220331150858.96348-1-peter.maydell@linaro.org --- target/arm/translate-a64.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c index d1a59fad9c..9333d7be41 100644 --- a/target/arm/translate-a64.c +++ b/target/arm/translate-a64.c @@ -2470,7 +2470,12 @@ static void gen_store_exclusive(DisasContext *s, int rd, int rt, int rt2, } else if (tb_cflags(s->base.tb) & CF_PARALLEL) { if (!HAVE_CMPXCHG128) { gen_helper_exit_atomic(cpu_env); - s->base.is_jmp = DISAS_NORETURN; + /* + * Produce a result so we have a well-formed opcode + * stream when the following (dead) code uses 'tmp'. + * TCG will remove the dead ops for us. + */ + tcg_gen_movi_i64(tmp, 0); } else if (s->be_data == MO_LE) { gen_helper_paired_cmpxchg64_le_parallel(tmp, cpu_env, cpu_exclusive_addr,