Re: [kvm-unit-tests PATCH 2/4] arm/arm64: Read system registers to get the state of the MMU

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi Nikos,

On 3/19/21 12:24 PM, Nikos Nikoleris wrote:
> When we are in EL1 we can directly tell if the local cpu's MMU is on
> by reading a system register (SCTRL/SCTRL_EL1). In EL0, we use the
> relevant cpumask. This way we don't have to rely on the cpu id in
> thread_info when we are in setup executing in EL1. In subsequent
> patches we resolve the problem of identifying safely whether we are
> executing in EL1 or EL0.

The commit message doesn't explain why mmu_disabled_cpu_count has been removed. It
also doesn't explain why the disabled cpumask has been replaced with an enabled
cpumask.

Other than that, it is a good idea to stop mmu_enabled() from checking the cpumask
because marking the MMU as enabled/disabled requires modifying the cpumask, which
calls mmu_enabled(). That's not a problem at EL0 because EL0 cannot turn on or off
the MMU.

Thanks,

Alex

>
> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
> ---
>  lib/arm/asm/mmu-api.h     |  7 +------
>  lib/arm/asm/processor.h   |  7 +++++++
>  lib/arm64/asm/processor.h |  1 +
>  lib/arm/mmu.c             | 16 ++++++++--------
>  lib/arm/processor.c       |  5 +++++
>  lib/arm64/processor.c     |  5 +++++
>  6 files changed, 27 insertions(+), 14 deletions(-)
>
> diff --git a/lib/arm/asm/mmu-api.h b/lib/arm/asm/mmu-api.h
> index 3d04d03..05fc12b 100644
> --- a/lib/arm/asm/mmu-api.h
> +++ b/lib/arm/asm/mmu-api.h
> @@ -5,12 +5,7 @@
>  #include <stdbool.h>
>  
>  extern pgd_t *mmu_idmap;
> -extern unsigned int mmu_disabled_cpu_count;
> -extern bool __mmu_enabled(void);
> -static inline bool mmu_enabled(void)
> -{
> -	return mmu_disabled_cpu_count == 0 || __mmu_enabled();
> -}
> +extern bool mmu_enabled(void);
>  extern void mmu_mark_enabled(int cpu);
>  extern void mmu_mark_disabled(int cpu);
>  extern void mmu_enable(pgd_t *pgtable);
> diff --git a/lib/arm/asm/processor.h b/lib/arm/asm/processor.h
> index 273366d..af54c09 100644
> --- a/lib/arm/asm/processor.h
> +++ b/lib/arm/asm/processor.h
> @@ -67,11 +67,13 @@ extern int mpidr_to_cpu(uint64_t mpidr);
>  	((mpidr >> MPIDR_LEVEL_SHIFT(level)) & 0xff)
>  
>  extern void start_usr(void (*func)(void *arg), void *arg, unsigned long sp_usr);
> +extern bool __mmu_enabled(void);
>  extern bool is_user(void);
>  
>  #define CNTVCT		__ACCESS_CP15_64(1, c14)
>  #define CNTFRQ		__ACCESS_CP15(c14, 0, c0, 0)
>  #define CTR		__ACCESS_CP15(c0, 0, c0, 1)
> +#define SCTRL		__ACCESS_CP15(c1, 0, c0, 0)
>  
>  static inline u64 get_cntvct(void)
>  {
> @@ -89,6 +91,11 @@ static inline u32 get_ctr(void)
>  	return read_sysreg(CTR);
>  }
>  
> +static inline u32 get_sctrl(void)
> +{
> +	return read_sysreg(SCTRL);
> +}
> +
>  extern unsigned long dcache_line_size;
>  
>  #endif /* _ASMARM_PROCESSOR_H_ */
> diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
> index 771b2d1..8e2037e 100644
> --- a/lib/arm64/asm/processor.h
> +++ b/lib/arm64/asm/processor.h
> @@ -98,6 +98,7 @@ extern int mpidr_to_cpu(uint64_t mpidr);
>  
>  extern void start_usr(void (*func)(void *arg), void *arg, unsigned long sp_usr);
>  extern bool is_user(void);
> +extern bool __mmu_enabled(void);
>  
>  static inline u64 get_cntvct(void)
>  {
> diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
> index a1862a5..d806c63 100644
> --- a/lib/arm/mmu.c
> +++ b/lib/arm/mmu.c
> @@ -24,10 +24,9 @@ extern unsigned long etext;
>  pgd_t *mmu_idmap;
>  
>  /* CPU 0 starts with disabled MMU */
> -static cpumask_t mmu_disabled_cpumask = { {1} };
> -unsigned int mmu_disabled_cpu_count = 1;
> +static cpumask_t mmu_enabled_cpumask = { {0} };
>  
> -bool __mmu_enabled(void)
> +bool mmu_enabled(void)
>  {
>  	int cpu = current_thread_info()->cpu;
>  
> @@ -38,19 +37,20 @@ bool __mmu_enabled(void)
>  	 * spinlock, atomic bitop, etc., otherwise we'll recurse.
>  	 * [cpumask_]test_bit is safe though.
>  	 */
> -	return !cpumask_test_cpu(cpu, &mmu_disabled_cpumask);
> +	if (is_user())
> +		return cpumask_test_cpu(cpu, &mmu_enabled_cpumask);
> +	else
> +		return __mmu_enabled();
>  }
>  
>  void mmu_mark_enabled(int cpu)
>  {
> -	if (cpumask_test_and_clear_cpu(cpu, &mmu_disabled_cpumask))
> -		--mmu_disabled_cpu_count;
> +	cpumask_set_cpu(cpu, &mmu_enabled_cpumask);
>  }
>  
>  void mmu_mark_disabled(int cpu)
>  {
> -	if (!cpumask_test_and_set_cpu(cpu, &mmu_disabled_cpumask))
> -		++mmu_disabled_cpu_count;
> +	cpumask_clear_cpu(cpu, &mmu_enabled_cpumask);
>  }
>  
>  extern void asm_mmu_enable(phys_addr_t pgtable);
> diff --git a/lib/arm/processor.c b/lib/arm/processor.c
> index 773337e..a2d39a4 100644
> --- a/lib/arm/processor.c
> +++ b/lib/arm/processor.c
> @@ -145,3 +145,8 @@ bool is_user(void)
>  {
>  	return current_thread_info()->flags & TIF_USER_MODE;
>  }
> +
> +bool __mmu_enabled(void)
> +{
> +	return get_sctrl() & CR_M;
> +}
> diff --git a/lib/arm64/processor.c b/lib/arm64/processor.c
> index 2a024e3..ef55862 100644
> --- a/lib/arm64/processor.c
> +++ b/lib/arm64/processor.c
> @@ -257,3 +257,8 @@ bool is_user(void)
>  {
>  	return current_thread_info()->flags & TIF_USER_MODE;
>  }
> +
> +bool __mmu_enabled(void)
> +{
> +	return read_sysreg(sctlr_el1) & SCTLR_EL1_M;
> +}