| Message ID | 20230414160413.549801-3-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/arm: Fix handling of VSTCR_EL2.SW and VTCR_EL2.NSW | expand |
On 4/14/23 18:04, Peter Maydell wrote: > + /* Check if page table walk is to secure or non-secure PA space. */ > + ptw->out_secure = (is_secure > + && !(pte_secure > + ? env->cp15.vstcr_el2 & VSTCR_SW > + : env->cp15.vtcr_el2 & VTCR_NSW)); > + } else { > + /* Regime is physical */ > + ptw->out_secure = pte_secure; Is that last comment really correct? I think it could still be stage1 of 2. That said, the actual code looks correct. Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On Tue, 18 Apr 2023 at 12:01, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 4/14/23 18:04, Peter Maydell wrote: > > + /* Check if page table walk is to secure or non-secure PA space. */ > > + ptw->out_secure = (is_secure > > + && !(pte_secure > > + ? env->cp15.vstcr_el2 & VSTCR_SW > > + : env->cp15.vtcr_el2 & VTCR_NSW)); > > + } else { > > + /* Regime is physical */ > > + ptw->out_secure = pte_secure; > > Is that last comment really correct? I think it could still be stage1 of 2. I borrowed the comment from earlier in the function, in the ptw->in_debug branch of the code, which has the same if (regime_is_stage2(s2_mmu_idx)) { ...stuff... } else { /* Regime is physical */ } structure as this one does after this patch. If s2_mmu_idx isn't a stage 2 index and it's not one of the Phys indexes, what is it ? -- PMM
On 4/18/23 13:30, Peter Maydell wrote: > On Tue, 18 Apr 2023 at 12:01, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> On 4/14/23 18:04, Peter Maydell wrote: >>> + /* Check if page table walk is to secure or non-secure PA space. */ >>> + ptw->out_secure = (is_secure >>> + && !(pte_secure >>> + ? env->cp15.vstcr_el2 & VSTCR_SW >>> + : env->cp15.vtcr_el2 & VTCR_NSW)); >>> + } else { >>> + /* Regime is physical */ >>> + ptw->out_secure = pte_secure; >> >> Is that last comment really correct? I think it could still be stage1 of 2. > > I borrowed the comment from earlier in the function, in the ptw->in_debug > branch of the code, which has the same > > if (regime_is_stage2(s2_mmu_idx)) { > ...stuff... > } else { > /* Regime is physical */ > } > > structure as this one does after this patch. If s2_mmu_idx isn't > a stage 2 index and it's not one of the Phys indexes, what is it ? Oh, right. Nevermind. r~
diff --git a/target/arm/ptw.c b/target/arm/ptw.c index 06865227642..c1e124df495 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -249,7 +249,7 @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw, /* Regime is physical. */ ptw->out_phys = addr; pte_attrs = 0; - pte_secure = is_secure; + pte_secure = s2_mmu_idx == ARMMMUIdx_Phys_S; } ptw->out_host = NULL; ptw->out_rw = false; @@ -291,13 +291,15 @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw, fi->s1ns = !is_secure; return false; } + /* Check if page table walk is to secure or non-secure PA space. */ + ptw->out_secure = (is_secure + && !(pte_secure + ? env->cp15.vstcr_el2 & VSTCR_SW + : env->cp15.vtcr_el2 & VTCR_NSW)); + } else { + /* Regime is physical */ + ptw->out_secure = pte_secure; } - - /* Check if page table walk is to secure or non-secure PA space. */ - ptw->out_secure = (is_secure - && !(pte_secure - ? env->cp15.vstcr_el2 & VSTCR_SW - : env->cp15.vtcr_el2 & VTCR_NSW)); ptw->out_be = regime_translation_big_endian(env, mmu_idx); return true;
In S1_ptw_translate(), we are in one of two situations: (1) translating an S1 page table walk through S2 (2) translating an S2 page table walk through a physical regime In case (1), ptw->in_secure indicates whether S1 is a Secure or NonSecure translation regime. In case (2), ptw->in_secure indicates whether this stage 2 translation is for a Secure IPA or a NonSecure IPA. In particular, because of the VTCR_EL2.NSW and VSTCR_EL2.SW bits, we can be doing a translation of a NonSecure IPA where the page tables are in the Secure space. Correct the setting of the ptw->out_secure value for the regime-is-physical case: * it depends on whether s2_mmu_idx is Phys_S or Phys, not on the value of is_secure * we don't need to look at the VTCR_EL2.NSW and VSTCR.SW bits again here, as we already did that in get_phys_addr_twostage() to set the correct in_ptw_idx This error doesn't currently cause a problem in itself because we are incorrectly setting ptw->in_secure to s2walk_secure in get_phys_addr_twostage(), but we need to fix this before we can correct that problem. Cc: qemu-stable@nongnu.org Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- target/arm/ptw.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)