| Message ID | 20230221034122.471707-3-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Fix gdbstub for m-profile (#1421) | expand |
On 21/2/23 04:41, Richard Henderson wrote: > M-profile is not supported by arm_is_secure, so using it as > a replacement when bypassing get_phys_addr was incorrect. > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1421 > Fixes: 4a35855682ce ("target/arm: Plumb debug into S1Translate") > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/ptw.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/target/arm/ptw.c b/target/arm/ptw.c > index cb073ac477..057cc9f641 100644 > --- a/target/arm/ptw.c > +++ b/target/arm/ptw.c > @@ -2974,9 +2974,10 @@ hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState *cs, vaddr addr, > { > ARMCPU *cpu = ARM_CPU(cs); > CPUARMState *env = &cpu->env; > + ARMMMUIdx mmu_idx = arm_mmu_idx(env); > S1Translate ptw = { > - .in_mmu_idx = arm_mmu_idx(env), > - .in_secure = arm_is_secure(env), > + .in_mmu_idx = mmu_idx, > + .in_secure = regime_is_secure(env, mmu_idx), > .in_debug = true, > }; > GetPhysAddrResult res = {}; Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On Tue, 21 Feb 2023 at 03:42, Richard Henderson <richard.henderson@linaro.org> wrote: > > M-profile is not supported by arm_is_secure, so using it as > a replacement when bypassing get_phys_addr was incorrect. That's pretty non-obvious. I think we should either make arm_is_secure() handle M-profile[*], or else have it assert if you try to call it for an M-profile CPU. [*] i.e. if (arm_feature(env, ARM_FEATURE_M)) { return env->v7m.secure; } at the top of the function. If we do the latter we wouldn't need the revert in patch 1, right? Or do you think regime_is_secure() is a better choice of function here anyway? thanks -- PMM
On 2/21/23 06:56, Peter Maydell wrote: > On Tue, 21 Feb 2023 at 03:42, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> M-profile is not supported by arm_is_secure, so using it as >> a replacement when bypassing get_phys_addr was incorrect. > > That's pretty non-obvious. I think we should either > make arm_is_secure() handle M-profile[*], or else have > it assert if you try to call it for an M-profile CPU. > > [*] i.e. > if (arm_feature(env, ARM_FEATURE_M)) { > return env->v7m.secure; > } > at the top of the function. > > If we do the latter we wouldn't need the revert in patch 1, > right? Or do you think regime_is_secure() is a better > choice of function here anyway? You're absolutely right that it's surprising, as it surprised me. I'll re-spin. r~
diff --git a/target/arm/ptw.c b/target/arm/ptw.c index cb073ac477..057cc9f641 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -2974,9 +2974,10 @@ hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState *cs, vaddr addr, { ARMCPU *cpu = ARM_CPU(cs); CPUARMState *env = &cpu->env; + ARMMMUIdx mmu_idx = arm_mmu_idx(env); S1Translate ptw = { - .in_mmu_idx = arm_mmu_idx(env), - .in_secure = arm_is_secure(env), + .in_mmu_idx = mmu_idx, + .in_secure = regime_is_secure(env, mmu_idx), .in_debug = true, }; GetPhysAddrResult res = {};
M-profile is not supported by arm_is_secure, so using it as a replacement when bypassing get_phys_addr was incorrect. Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1421 Fixes: 4a35855682ce ("target/arm: Plumb debug into S1Translate") Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/ptw.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)