| Message ID | 20221102054706.1015830-1-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/arm: Two fixes for secure ptw | expand |
On 2/11/22 06:47, Richard Henderson wrote: > Reversed the sense of non-secure in get_phys_addr_lpae, > and failed to initialize attrs.secure for ARMMMUIdx_Phys_S. > > Fixes: 48da29e4 ("target/arm: Add ptw_idx to S1Translate") > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1293 Thanks! Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/ptw.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-)
On Wed, 2 Nov 2022 at 05:47, Richard Henderson <richard.henderson@linaro.org> wrote: > > Reversed the sense of non-secure in get_phys_addr_lpae, > and failed to initialize attrs.secure for ARMMMUIdx_Phys_S. > > Fixes: 48da29e4 ("target/arm: Add ptw_idx to S1Translate") > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1293 > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/ptw.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/target/arm/ptw.c b/target/arm/ptw.c > index 58a7bbda50..df3573f150 100644 > --- a/target/arm/ptw.c > +++ b/target/arm/ptw.c > @@ -1357,7 +1357,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw, > descaddr |= (address >> (stride * (4 - level))) & indexmask; > descaddr &= ~7ULL; > nstable = extract32(tableattrs, 4, 1); > - if (!nstable) { > + if (nstable) { > /* > * Stage2_S -> Stage2 or Phys_S -> Phys_NS > * Assert that the non-secure idx are even, and relative order. > @@ -2671,6 +2671,13 @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw, > bool is_secure = ptw->in_secure; > ARMMMUIdx s1_mmu_idx; > > + /* > + * The page table entries may downgrade secure to non-secure, but > + * cannot upgrade an non-secure translation regime's attributes > + * to secure. > + */ > + result->f.attrs.secure = is_secure; > + > switch (mmu_idx) { > case ARMMMUIdx_Phys_S: > case ARMMMUIdx_Phys_NS: > @@ -2712,12 +2719,6 @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw, > break; > } > > - /* > - * The page table entries may downgrade secure to non-secure, but > - * cannot upgrade an non-secure translation regime's attributes > - * to secure. > - */ > - result->f.attrs.secure = is_secure; > result->f.attrs.user = regime_is_user(env, mmu_idx); Do we also need to move this setting of attrs.user ? get_phys_addr_disabled() doesn't set that either. thanks -- PMM
On Thu, 3 Nov 2022 at 13:19, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Wed, 2 Nov 2022 at 05:47, Richard Henderson > <richard.henderson@linaro.org> wrote: > > > > Reversed the sense of non-secure in get_phys_addr_lpae, > > and failed to initialize attrs.secure for ARMMMUIdx_Phys_S. > > > > Fixes: 48da29e4 ("target/arm: Add ptw_idx to S1Translate") > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1293 > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > > --- > > target/arm/ptw.c | 15 ++++++++------- > > 1 file changed, 8 insertions(+), 7 deletions(-) > > > > diff --git a/target/arm/ptw.c b/target/arm/ptw.c > > index 58a7bbda50..df3573f150 100644 > > --- a/target/arm/ptw.c > > +++ b/target/arm/ptw.c > > @@ -1357,7 +1357,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw, > > descaddr |= (address >> (stride * (4 - level))) & indexmask; > > descaddr &= ~7ULL; > > nstable = extract32(tableattrs, 4, 1); > > - if (!nstable) { > > + if (nstable) { > > /* > > * Stage2_S -> Stage2 or Phys_S -> Phys_NS > > * Assert that the non-secure idx are even, and relative order. > > @@ -2671,6 +2671,13 @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw, > > bool is_secure = ptw->in_secure; > > ARMMMUIdx s1_mmu_idx; > > > > + /* > > + * The page table entries may downgrade secure to non-secure, but > > + * cannot upgrade an non-secure translation regime's attributes > > + * to secure. > > + */ > > + result->f.attrs.secure = is_secure; > > + > > switch (mmu_idx) { > > case ARMMMUIdx_Phys_S: > > case ARMMMUIdx_Phys_NS: > > @@ -2712,12 +2719,6 @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw, > > break; > > } > > > > - /* > > - * The page table entries may downgrade secure to non-secure, but > > - * cannot upgrade an non-secure translation regime's attributes > > - * to secure. > > - */ > > - result->f.attrs.secure = is_secure; > > result->f.attrs.user = regime_is_user(env, mmu_idx); > > Do we also need to move this setting of attrs.user ? > get_phys_addr_disabled() doesn't set that either. I've applied this to target-arm.next for the moment anyway, since it is definitely fixing an attrs.secure related bug. I can replace that with a v2 or we can do a follow-on patch, depending whether you get to this before or after I send out a pullreq. thanks -- PMM
On 11/4/22 00:19, Peter Maydell wrote: >> @@ -2671,6 +2671,13 @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw, >> bool is_secure = ptw->in_secure; >> ARMMMUIdx s1_mmu_idx; >> >> + /* >> + * The page table entries may downgrade secure to non-secure, but >> + * cannot upgrade an non-secure translation regime's attributes >> + * to secure. >> + */ >> + result->f.attrs.secure = is_secure; >> + >> switch (mmu_idx) { >> case ARMMMUIdx_Phys_S: >> case ARMMMUIdx_Phys_NS: >> @@ -2712,12 +2719,6 @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw, >> break; >> } >> >> - /* >> - * The page table entries may downgrade secure to non-secure, but >> - * cannot upgrade an non-secure translation regime's attributes >> - * to secure. >> - */ >> - result->f.attrs.secure = is_secure; >> result->f.attrs.user = regime_is_user(env, mmu_idx); > > Do we also need to move this setting of attrs.user ? > get_phys_addr_disabled() doesn't set that either. I don't think so. The only cases which don't pass through the setting of attrs.user are the two Phys mmu_idx. Which was by design per the comment up there about artificially deciding which EL regime they belong to. r~
On Thu, 3 Nov 2022 at 20:30, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 11/4/22 00:19, Peter Maydell wrote: > >> @@ -2671,6 +2671,13 @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw, > >> bool is_secure = ptw->in_secure; > >> ARMMMUIdx s1_mmu_idx; > >> > >> + /* > >> + * The page table entries may downgrade secure to non-secure, but > >> + * cannot upgrade an non-secure translation regime's attributes > >> + * to secure. > >> + */ > >> + result->f.attrs.secure = is_secure; > >> + > >> switch (mmu_idx) { > >> case ARMMMUIdx_Phys_S: > >> case ARMMMUIdx_Phys_NS: > >> @@ -2712,12 +2719,6 @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw, > >> break; > >> } > >> > >> - /* > >> - * The page table entries may downgrade secure to non-secure, but > >> - * cannot upgrade an non-secure translation regime's attributes > >> - * to secure. > >> - */ > >> - result->f.attrs.secure = is_secure; > >> result->f.attrs.user = regime_is_user(env, mmu_idx); > > > > Do we also need to move this setting of attrs.user ? > > get_phys_addr_disabled() doesn't set that either. > > I don't think so. The only cases which don't pass through the setting of attrs.user are > the two Phys mmu_idx. Which was by design per the comment up there about artificially > deciding which EL regime they belong to. OK. Do we ever do anything with the attrs for a phys tlb lookup except use them internally for details of the stage 2 tlb walk ? I guess they get used for the memory transaction to do the walk. That matches the old code that just had a local MemTxAttrs in arm_ldq_ptw() to do the walk which therefore implicitly got user == false. -- PMM
On 11/4/22 21:58, Peter Maydell wrote: > OK. Do we ever do anything with the attrs for a phys tlb lookup > except use them internally for details of the stage 2 tlb walk ? > I guess they get used for the memory transaction to do the walk. > That matches the old code that just had a local MemTxAttrs in > arm_ldq_ptw() to do the walk which therefore implicitly got > user == false. Exactly. I can't think of any reason .user would be apply outside of the stage1 lookup, which is what records the setting for any future mmio. r~
diff --git a/target/arm/ptw.c b/target/arm/ptw.c index 58a7bbda50..df3573f150 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -1357,7 +1357,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw, descaddr |= (address >> (stride * (4 - level))) & indexmask; descaddr &= ~7ULL; nstable = extract32(tableattrs, 4, 1); - if (!nstable) { + if (nstable) { /* * Stage2_S -> Stage2 or Phys_S -> Phys_NS * Assert that the non-secure idx are even, and relative order. @@ -2671,6 +2671,13 @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw, bool is_secure = ptw->in_secure; ARMMMUIdx s1_mmu_idx; + /* + * The page table entries may downgrade secure to non-secure, but + * cannot upgrade an non-secure translation regime's attributes + * to secure. + */ + result->f.attrs.secure = is_secure; + switch (mmu_idx) { case ARMMMUIdx_Phys_S: case ARMMMUIdx_Phys_NS: @@ -2712,12 +2719,6 @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw, break; } - /* - * The page table entries may downgrade secure to non-secure, but - * cannot upgrade an non-secure translation regime's attributes - * to secure. - */ - result->f.attrs.secure = is_secure; result->f.attrs.user = regime_is_user(env, mmu_idx); /*
Reversed the sense of non-secure in get_phys_addr_lpae, and failed to initialize attrs.secure for ARMMMUIdx_Phys_S. Fixes: 48da29e4 ("target/arm: Add ptw_idx to S1Translate") Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1293 Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/ptw.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)