On Wed, 19 Oct 2022 at 13:15, Timofey Kutergin <tkuter...@gmail.com> wrote:
>
> - synchronize PSTATE.PAN with changes in CPSR.PAN in aarch32 mode
> - set PAN bit automatically on exception entry if SCTLR_SPAN bit
> is set
> - throw permission fault during address translation when PAN is
> enabled and kernel tries to access user acessible page
> - ignore SCTLR_XP bit for armv7 and armv8 (conflicts with SCTLR_SPAN).
>
> Signed-off-by: Timofey Kutergin <tkuter...@gmail.com>
> ---
> target/arm/helper.c | 6 ++++++
> target/arm/ptw.c | 11 ++++++++++-
> 2 files changed, 16 insertions(+), 1 deletion(-)
Thanks for this patch. I think you've caught all the places
we aren't correctly implementing AArch32 PAN handling.
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index dde64a487a..5299f67e3f 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -9052,6 +9052,11 @@ void cpsr_write(CPUARMState *env, uint32_t val,
> uint32_t mask,
> }
> mask &= ~CACHED_CPSR_BITS;
> env->uncached_cpsr = (env->uncached_cpsr & ~mask) | (val & mask);
> + if (env->uncached_cpsr & CPSR_PAN) {
> + env->pstate |= PSTATE_PAN;
> + } else {
> + env->pstate &= ~PSTATE_PAN;
> + }
This approach means we're storing the PAN bit in two places,
both in env->uncached_cpsr and in env->pstate. We don't do
this for any other bits as far as I can see. I think we should
either:
(1) have the code that changes behaviour based on PAN look
at either env->pstate or env->uncached_cpsr depending
on whether we're AArch64 or AArch32
(2) always store the state in env->pstate only, and handle
this in read/write of the CPSR the same way we do with
other "cached" bits
I think the intention of the current code is (1), and the
only place we get this wrong is in arm_mmu_idx_el(),
which is checking env->pstate only. (The other places that
directly check env->pstate are all in AArch64-only code,
and various AArch32-only bits of code already check
env->uncached_cpsr.) A function like
bool arm_pan_enabled(CPUARMState *env)
{
if (is_a64(env)) {
return env->pstate & PSTATE_PAN;
} else {
return env->uncached_cpsr & CPSR_PAN;
}
}
and then using that in arm_mmu_idx_el() should I think
mean you don't need to change either cpsr_write() or
take_aarch32_exception().
> if (rebuild_hflags) {
> arm_rebuild_hflags(env);
> }
> @@ -9592,6 +9597,7 @@ static void take_aarch32_exception(CPUARMState *env,
> int new_mode,
> /* ... the target is EL1 and SCTLR.SPAN is 0. */
> if (!(env->cp15.sctlr_el[new_el] & SCTLR_SPAN)) {
> env->uncached_cpsr |= CPSR_PAN;
> + env->pstate |= PSTATE_PAN;
> }
> break;
> }
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index 23f16f4ff7..204a73350f 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -659,6 +659,13 @@ static bool get_phys_addr_v6(CPUARMState *env, uint32_t
> address,
> goto do_fault;
> }
>
> + if (regime_is_pan(env, mmu_idx) && !regime_is_user(env, mmu_idx) &&
> + simple_ap_to_rw_prot_is_user(ap >> 1, 1) &&
> + access_type != MMU_INST_FETCH) {
> + fi->type = ARMFault_Permission;
> + goto do_fault;
> + }
This assumes we're using the SCTLR.AFE==1 simplified
permissions model, but PAN should apply even if we're using the
old model. So we need a ap_to_rw_prot_is_user() to check the
permissions in that model.
The check is also being done before the Access fault check, but
the architecture says that Access faults take priority over
Permission faults.
> +
> if (arm_feature(env, ARM_FEATURE_V6K) &&
> (regime_sctlr(env, mmu_idx) & SCTLR_AFE)) {
> /* The simplified model uses AP[0] as an access control bit. */
> @@ -2506,7 +2513,9 @@ bool get_phys_addr_with_secure(CPUARMState *env,
> target_ulong address,
> if (regime_using_lpae_format(env, mmu_idx)) {
> return get_phys_addr_lpae(env, address, access_type, mmu_idx,
> is_secure, false, result, fi);
> - } else if (regime_sctlr(env, mmu_idx) & SCTLR_XP) {
> + } else if (arm_feature(env, ARM_FEATURE_V7) ||
> + arm_feature(env, ARM_FEATURE_V8) || (
V8 always implies V7, so we only need to check V7 here.
> + regime_sctlr(env, mmu_idx) & SCTLR_XP)) {
> return get_phys_addr_v6(env, address, access_type, mmu_idx,
> is_secure, result, fi);
> } else {
thanks
-- PMM
