On 25/10/22 18:39, Peter Maydell wrote:
> From: Richard Henderson <richard.henderson@linaro.org>
>
> Hoist the computation of the mmu_idx for the ptw up to
> get_phys_addr_with_struct and get_phys_addr_twostage.
> This removes the duplicate check for stage2 disabled
> from the middle of the walk, performing it only once.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> Tested-by: Alex Bennée <alex.bennee@linaro.org>
> Message-id: 20221024051851.3074715-3-richard.henderson@linaro.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> target/arm/ptw.c | 71 ++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 54 insertions(+), 17 deletions(-)
>
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index 32d64125865..3c153f68318 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -17,6 +17,7 @@
>
> typedef struct S1Translate {
> ARMMMUIdx in_mmu_idx;
> + ARMMMUIdx in_ptw_idx;
> bool in_secure;
> bool in_debug;
> bool out_secure;
> @@ -214,33 +215,24 @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw,
> {
> bool is_secure = ptw->in_secure;
> ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
> - ARMMMUIdx s2_mmu_idx = is_secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2;
> - bool s2_phys = false;
> + ARMMMUIdx s2_mmu_idx = ptw->in_ptw_idx;
> uint8_t pte_attrs;
> bool pte_secure;
>
> - if (!arm_mmu_idx_is_stage1_of_2(mmu_idx)
> - || regime_translation_disabled(env, s2_mmu_idx, is_secure)) {
> - s2_mmu_idx = is_secure ? ARMMMUIdx_Phys_S : ARMMMUIdx_Phys_NS;
> - s2_phys = true;
> - }
> -
> if (unlikely(ptw->in_debug)) {
> /*
> * From gdbstub, do not use softmmu so that we don't modify the
> * state of the cpu at all, including softmmu tlb contents.
> */
> - if (s2_phys) {
> - ptw->out_phys = addr;
> - pte_attrs = 0;
> - pte_secure = is_secure;
> - } else {
> + if (regime_is_stage2(s2_mmu_idx)) {
> S1Translate s2ptw = {
> .in_mmu_idx = s2_mmu_idx,
> + .in_ptw_idx = is_secure ? ARMMMUIdx_Phys_S : ARMMMUIdx_Phys_NS,
> .in_secure = is_secure,
> .in_debug = true,
> };
> GetPhysAddrResult s2 = { };
> +
> if (!get_phys_addr_lpae(env, &s2ptw, addr, MMU_DATA_LOAD,
> false, &s2, fi)) {
> goto fail;
> @@ -248,6 +240,11 @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw,
> ptw->out_phys = s2.f.phys_addr;
> pte_attrs = s2.cacheattrs.attrs;
> pte_secure = s2.f.attrs.secure;
> + } else {
> + /* Regime is physical. */
> + ptw->out_phys = addr;
> + pte_attrs = 0;
> + pte_secure = is_secure;
> }
> ptw->out_host = NULL;
> } else {
> @@ -268,7 +265,7 @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw,
> pte_secure = full->attrs.secure;
> }
>
> - if (!s2_phys) {
> + if (regime_is_stage2(s2_mmu_idx)) {
> uint64_t hcr = arm_hcr_el2_eff_secstate(env, is_secure);
>
> if ((hcr & HCR_PTW) && S2_attrs_are_device(hcr, pte_attrs)) {
> @@ -1263,7 +1260,18 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
> descaddr |= (address >> (stride * (4 - level))) & indexmask;
> descaddr &= ~7ULL;
> nstable = extract32(tableattrs, 4, 1);
> - ptw->in_secure = !nstable;
> + if (!nstable) {
> + /*
> + * Stage2_S -> Stage2 or Phys_S -> Phys_NS
> + * Assert that the non-secure idx are even, and relative order.
> + */
> + QEMU_BUILD_BUG_ON((ARMMMUIdx_Phys_NS & 1) != 0);
> + QEMU_BUILD_BUG_ON((ARMMMUIdx_Stage2 & 1) != 0);
> + QEMU_BUILD_BUG_ON(ARMMMUIdx_Phys_NS + 1 != ARMMMUIdx_Phys_S);
> + QEMU_BUILD_BUG_ON(ARMMMUIdx_Stage2 + 1 != ARMMMUIdx_Stage2_S);
> + ptw->in_ptw_idx &= ~1;
> + ptw->in_secure = false;
> + }
> descriptor = arm_ldq_ptw(env, ptw, descaddr, fi);
> if (fi->type != ARMFault_None) {
> goto do_fault;
> @@ -2449,6 +2457,7 @@ static bool get_phys_addr_twostage(CPUARMState *env, S1Translate *ptw,
>
> is_el0 = ptw->in_mmu_idx == ARMMMUIdx_Stage1_E0;
> ptw->in_mmu_idx = s2walk_secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2;
> + ptw->in_ptw_idx = s2walk_secure ? ARMMMUIdx_Phys_S : ARMMMUIdx_Phys_NS;
> ptw->in_secure = s2walk_secure;
>
> /*
> @@ -2508,10 +2517,32 @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw,
> ARMMMUFaultInfo *fi)
> {
> ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
> - ARMMMUIdx s1_mmu_idx = stage_1_mmu_idx(mmu_idx);
> bool is_secure = ptw->in_secure;
> + ARMMMUIdx s1_mmu_idx;
>
> - if (mmu_idx != s1_mmu_idx) {
> + switch (mmu_idx) {
> + case ARMMMUIdx_Phys_S:
> + case ARMMMUIdx_Phys_NS:
> + /* Checking Phys early avoids special casing later vs regime_el. */
> + return get_phys_addr_disabled(env, address, access_type, mmu_idx,
> + is_secure, result, fi);
> +
> + case ARMMMUIdx_Stage1_E0:
> + case ARMMMUIdx_Stage1_E1:
> + case ARMMMUIdx_Stage1_E1_PAN:
> + /* First stage lookup uses second stage for ptw. */
> + ptw->in_ptw_idx = is_secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2;
> + break;
> +
> + case ARMMMUIdx_E10_0:
> + s1_mmu_idx = ARMMMUIdx_Stage1_E0;
> + goto do_twostage;
> + case ARMMMUIdx_E10_1:
> + s1_mmu_idx = ARMMMUIdx_Stage1_E1;
> + goto do_twostage;
> + case ARMMMUIdx_E10_1_PAN:
> + s1_mmu_idx = ARMMMUIdx_Stage1_E1_PAN;
> + do_twostage:
> /*
> * Call ourselves recursively to do the stage 1 and then stage 2
> * translations if mmu_idx is a two-stage regime, and EL2 present.
> @@ -2522,6 +2553,12 @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw,
> return get_phys_addr_twostage(env, ptw, address, access_type,
> result, fi);
> }
> + /* fall through */
> +
> + default:
> + /* Single stage and second stage uses physical for ptw. */
> + ptw->in_ptw_idx = is_secure ? ARMMMUIdx_Phys_S : ARMMMUIdx_Phys_NS;
> + break;
> }
Since this commit I can not boot Trusted Firmware on the SBSA-ref machine.
--- ok 2022-10-31 23:11:48.131829719 +0000
+++ bad 2022-10-31 23:11:27.210091574 +0000
@@ -176,68 +176,68 @@
-IN:
-0x000000003fcd4b14:
-OBJD-T: 030000d4
-
-Taking exception 13 [Secure Monitor Call] on CPU 0
-...from EL1 to EL3
-...with ESR 0x17/0x5e000000
-...with ELR 0x3fcd4b18
-...to EL3 PC 0x3400 PSTATE 0x3cd
+Taking exception 3 [Prefetch Abort] on CPU 0
+...from EL3 to EL3
+...with ESR 0x21/0x86000004
+...with FAR 0x28b8
+...with ELR 0x28b8
+...to EL3 PC 0x3000 PSTATE 0x3cd