| Message ID | 20230124000027.3565716-13-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Implement FEAT_RME | expand |
On Tue, 24 Jan 2023 at 00:01, Richard Henderson <richard.henderson@linaro.org> wrote: > > Test in_space instead of in_secure so that we don't switch > out of Root space. Handle the output space change immediately, > rather than try and combine the NSTable and NS bits later. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/ptw.c | 31 ++++++++++++++----------------- > 1 file changed, 14 insertions(+), 17 deletions(-) > > diff --git a/target/arm/ptw.c b/target/arm/ptw.c > index c1b0b8e610..ddafb1f329 100644 > --- a/target/arm/ptw.c > +++ b/target/arm/ptw.c > @@ -1240,7 +1240,6 @@ 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; > int32_t level; > ARMVAParameters param; > uint64_t ttbr; > @@ -1256,7 +1255,6 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw, > uint64_t descaddrmask; > bool aarch64 = arm_el_is_aa64(env, el); > uint64_t descriptor, new_descriptor; > - bool nstable; > > /* TODO: This code does not support shareability levels. */ > if (aarch64) { > @@ -1417,29 +1415,29 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw, > descaddrmask = MAKE_64BIT_MASK(0, 40); > } > descaddrmask &= ~indexmask_grainsize; > - > - /* > - * Secure accesses start with the page table in secure memory and > - * can be downgraded to non-secure at any step. Non-secure accesses > - * remain non-secure. We implement this by just ORing in the NSTable/NS > - * bits at each step. > - */ > - tableattrs = is_secure ? 0 : (1 << 4); > + tableattrs = 0; > > next_level: > descaddr |= (address >> (stride * (4 - level))) & indexmask; > descaddr &= ~7ULL; > - nstable = extract32(tableattrs, 4, 1); > - if (nstable && ptw->in_secure) { > - /* > - * Stage2_S -> Stage2 or Phys_S -> Phys_NS > - * Assert that the non-secure idx are even, and relative order. > - */ > + > + /* > + * Process the NSTable bit from the previous level. This changes > + * the table address space and the output space from Secure to > + * NonSecure. With RME, the EL3 translation regime does not change > + * from Root to NonSecure. > + */ To check my understanding, this means that the bit that the spec describes as FEAT_RME changing the behaviour of NSTable in the EL3 stage 1 translation regime is implemented by us by having the in_space for EL3 be different for FEAT_RME and not-FEAT_RME ? > + if (extract32(tableattrs, 4, 1) && ptw->in_space == ARMSS_Secure) { > + /* Stage2_S -> Stage2 or Phys_S -> Phys_NS */ > QEMU_BUILD_BUG_ON(ARMMMUIdx_Phys_S + 1 != ARMMMUIdx_Phys_NS); > QEMU_BUILD_BUG_ON(ARMMMUIdx_Stage2_S + 1 != ARMMMUIdx_Stage2); > ptw->in_ptw_idx += 1; > ptw->in_secure = false; > + ptw->in_space = ARMSS_NonSecure; > + result->f.attrs.secure = false; > + result->f.attrs.space = ARMSS_NonSecure; > } > + > if (!S1_ptw_translate(env, ptw, descaddr, fi)) { > goto do_fault; > } > @@ -1542,7 +1540,6 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw, > */ > attrs = new_descriptor & (MAKE_64BIT_MASK(2, 10) | MAKE_64BIT_MASK(50, 14)); > if (!regime_is_stage2(mmu_idx)) { > - attrs |= nstable << 5; /* NS */ This removes the code where we copy the NSTable bit across to attrs, but there's still code below here that assumes it can get the combined NS bit from bit 5 of attrs, isn't there? (It passes it to get_S1prot().) thanks -- PMM
On 2/10/23 01:36, Peter Maydell wrote: > On Tue, 24 Jan 2023 at 00:01, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> Test in_space instead of in_secure so that we don't switch >> out of Root space. Handle the output space change immediately, >> rather than try and combine the NSTable and NS bits later. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> target/arm/ptw.c | 31 ++++++++++++++----------------- >> 1 file changed, 14 insertions(+), 17 deletions(-) >> >> diff --git a/target/arm/ptw.c b/target/arm/ptw.c >> index c1b0b8e610..ddafb1f329 100644 >> --- a/target/arm/ptw.c >> +++ b/target/arm/ptw.c >> @@ -1240,7 +1240,6 @@ 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; >> int32_t level; >> ARMVAParameters param; >> uint64_t ttbr; >> @@ -1256,7 +1255,6 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw, >> uint64_t descaddrmask; >> bool aarch64 = arm_el_is_aa64(env, el); >> uint64_t descriptor, new_descriptor; >> - bool nstable; >> >> /* TODO: This code does not support shareability levels. */ >> if (aarch64) { >> @@ -1417,29 +1415,29 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw, >> descaddrmask = MAKE_64BIT_MASK(0, 40); >> } >> descaddrmask &= ~indexmask_grainsize; >> - >> - /* >> - * Secure accesses start with the page table in secure memory and >> - * can be downgraded to non-secure at any step. Non-secure accesses >> - * remain non-secure. We implement this by just ORing in the NSTable/NS >> - * bits at each step. >> - */ >> - tableattrs = is_secure ? 0 : (1 << 4); >> + tableattrs = 0; >> >> next_level: >> descaddr |= (address >> (stride * (4 - level))) & indexmask; >> descaddr &= ~7ULL; >> - nstable = extract32(tableattrs, 4, 1); >> - if (nstable && ptw->in_secure) { >> - /* >> - * Stage2_S -> Stage2 or Phys_S -> Phys_NS >> - * Assert that the non-secure idx are even, and relative order. >> - */ >> + >> + /* >> + * Process the NSTable bit from the previous level. This changes >> + * the table address space and the output space from Secure to >> + * NonSecure. With RME, the EL3 translation regime does not change >> + * from Root to NonSecure. >> + */ > > To check my understanding, this means that the bit that the spec > describes as FEAT_RME changing the behaviour of NSTable in the EL3 > stage 1 translation regime is implemented by us by having the > in_space for EL3 be different for FEAT_RME and not-FEAT_RME ? Correct -- space is Secure for non-RME EL3, and Root for RME EL3. >> attrs = new_descriptor & (MAKE_64BIT_MASK(2, 10) | MAKE_64BIT_MASK(50, 14)); >> if (!regime_is_stage2(mmu_idx)) { >> - attrs |= nstable << 5; /* NS */ > > This removes the code where we copy the NSTable bit across to attrs, > but there's still code below here that assumes it can get the combined > NS bit from bit 5 of attrs, isn't there? (It passes it to get_S1prot().) Oops. This gets fixed in patch 14. Some reordering needed... r~
diff --git a/target/arm/ptw.c b/target/arm/ptw.c index c1b0b8e610..ddafb1f329 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -1240,7 +1240,6 @@ 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; int32_t level; ARMVAParameters param; uint64_t ttbr; @@ -1256,7 +1255,6 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw, uint64_t descaddrmask; bool aarch64 = arm_el_is_aa64(env, el); uint64_t descriptor, new_descriptor; - bool nstable; /* TODO: This code does not support shareability levels. */ if (aarch64) { @@ -1417,29 +1415,29 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw, descaddrmask = MAKE_64BIT_MASK(0, 40); } descaddrmask &= ~indexmask_grainsize; - - /* - * Secure accesses start with the page table in secure memory and - * can be downgraded to non-secure at any step. Non-secure accesses - * remain non-secure. We implement this by just ORing in the NSTable/NS - * bits at each step. - */ - tableattrs = is_secure ? 0 : (1 << 4); + tableattrs = 0; next_level: descaddr |= (address >> (stride * (4 - level))) & indexmask; descaddr &= ~7ULL; - nstable = extract32(tableattrs, 4, 1); - if (nstable && ptw->in_secure) { - /* - * Stage2_S -> Stage2 or Phys_S -> Phys_NS - * Assert that the non-secure idx are even, and relative order. - */ + + /* + * Process the NSTable bit from the previous level. This changes + * the table address space and the output space from Secure to + * NonSecure. With RME, the EL3 translation regime does not change + * from Root to NonSecure. + */ + if (extract32(tableattrs, 4, 1) && ptw->in_space == ARMSS_Secure) { + /* Stage2_S -> Stage2 or Phys_S -> Phys_NS */ QEMU_BUILD_BUG_ON(ARMMMUIdx_Phys_S + 1 != ARMMMUIdx_Phys_NS); QEMU_BUILD_BUG_ON(ARMMMUIdx_Stage2_S + 1 != ARMMMUIdx_Stage2); ptw->in_ptw_idx += 1; ptw->in_secure = false; + ptw->in_space = ARMSS_NonSecure; + result->f.attrs.secure = false; + result->f.attrs.space = ARMSS_NonSecure; } + if (!S1_ptw_translate(env, ptw, descaddr, fi)) { goto do_fault; } @@ -1542,7 +1540,6 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw, */ attrs = new_descriptor & (MAKE_64BIT_MASK(2, 10) | MAKE_64BIT_MASK(50, 14)); if (!regime_is_stage2(mmu_idx)) { - attrs |= nstable << 5; /* NS */ if (!param.hpd) { attrs |= extract64(tableattrs, 0, 2) << 53; /* XN, PXN */ /*
Test in_space instead of in_secure so that we don't switch out of Root space. Handle the output space change immediately, rather than try and combine the NSTable and NS bits later. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/ptw.c | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-)