[kvm-unit-tests PATCH 0/4] RFC: Minor arm/arm64 MMU fixes and checks

[kvm-unit-tests PATCH 0/4] RFC: Minor arm/arm64 MMU fixes and checks

[Nikos Nikoleris]

Prior to this set of fixes, the code in setup() which we call to
initialize the system has a circular dependency. cpu_init()
(eventually) calls spin_lock() and __mmu_enabled(). However, at this
point, __mmu_enabled() may have undefined behavior as we haven't
initialized the current thread_info (cpu field). Moving
thread_info_init() above cpu_init() is not possible as it relies on
mpidr_to_cpu() which won't return the right results before cpu_init().
In addition, mem_init() also calls __mmu_enabled() and therefore
suffers from the same problem. Right now, everything works as
thread_info maps to memory which is implicitly initialized to 0 (cpu =
0) and makes the assumption that the cpu that runs setup() will be the
first cpu in the DT.

This set of patches changes the code slightly and avoids this
assumptions. In addition, it adds assertions to catch problems like
the above. The current solution relies on the thread_info() of the cpu
that setup() run to be initialized to zero (should we make it
explicit?).

There are a couple of options I considered in addressing this issue
which didn't seem satisfactory:

- Change the mmu_disabled_count global variable to mmu_enabled_count
  to count the number of active mmu's and bypass __mmu_enabled() when
  it's 0. This is a global variable and at the momement all write to
  it are protected by a lock but it's rather fragile and could easily
  cause issues in the future.
- Explicitly initialize current_thread_info()->cpu = 0 in setup()
  before anything else or make the first call of thread_info_init() a
  special case and avoid looking up mpidr_to_cpu(). This way we can
  move thread_info_init() to the top of setup(). If the CPU setup is
  running on is not the first that appears in the DT then this
  solution won't work.

Thanks,

Nikos

Nikos Nikoleris (4):
  arm/arm64: Avoid calling cpumask_test_cpu for CPUs above nr_cpu
  arm/arm64: Read system registers to get the state of the MMU
  arm/arm64: Track whether thread_info has been initialized
  arm/arm64: Add sanity checks to the cpumask API

 lib/arm/asm/cpumask.h     |  7 ++++++-
 lib/arm/asm/mmu-api.h     |  7 +------
 lib/arm/asm/processor.h   |  7 +++++++
 lib/arm/asm/thread_info.h |  1 +
 lib/arm64/asm/processor.h |  1 +
 lib/arm/mmu.c             | 16 ++++++++--------
 lib/arm/processor.c       | 10 ++++++++--
 lib/arm64/processor.c     | 10 ++++++++--
 8 files changed, 40 insertions(+), 19 deletions(-)

-- 
2.25.1

[kvm-unit-tests PATCH 1/4] arm/arm64: Avoid calling cpumask_test_cpu for CPUs above nr_cpu

[Nikos Nikoleris]

Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
---
 lib/arm/asm/cpumask.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/arm/asm/cpumask.h b/lib/arm/asm/cpumask.h
index 6683bb6..02124de 100644
--- a/lib/arm/asm/cpumask.h
+++ b/lib/arm/asm/cpumask.h
@@ -105,7 +105,7 @@ static inline void cpumask_copy(cpumask_t *dst, const cpumask_t *src)
 
 static inline int cpumask_next(int cpu, const cpumask_t *mask)
 {
-	while (cpu < nr_cpus && !cpumask_test_cpu(++cpu, mask))
+	while (++cpu < nr_cpus && !cpumask_test_cpu(cpu, mask))
 		;
 	return cpu;
 }
-- 
2.25.1

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

[Nikos Nikoleris]

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.

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;
+}
-- 
2.25.1

[kvm-unit-tests PATCH 3/4] arm/arm64: Track whether thread_info has been initialized

[Nikos Nikoleris]

Introduce a new flag in the thread_info to track whether a thread_info
struct is initialized yet or not.

Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
---
 lib/arm/asm/thread_info.h | 1 +
 lib/arm/processor.c       | 5 +++--
 lib/arm64/processor.c     | 5 +++--
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/lib/arm/asm/thread_info.h b/lib/arm/asm/thread_info.h
index eaa7258..926c2a3 100644
--- a/lib/arm/asm/thread_info.h
+++ b/lib/arm/asm/thread_info.h
@@ -45,6 +45,7 @@ static inline void *thread_stack_alloc(void)
 }
 
 #define TIF_USER_MODE		(1U << 0)
+#define TIF_VALID		(1U << 1)
 
 struct thread_info {
 	int cpu;
diff --git a/lib/arm/processor.c b/lib/arm/processor.c
index a2d39a4..702fbc3 100644
--- a/lib/arm/processor.c
+++ b/lib/arm/processor.c
@@ -119,7 +119,7 @@ void thread_info_init(struct thread_info *ti, unsigned int flags)
 {
 	memset(ti, 0, sizeof(struct thread_info));
 	ti->cpu = mpidr_to_cpu(get_mpidr());
-	ti->flags = flags;
+	ti->flags = flags | TIF_VALID;
 }
 
 void start_usr(void (*func)(void *arg), void *arg, unsigned long sp_usr)
@@ -143,7 +143,8 @@ void start_usr(void (*func)(void *arg), void *arg, unsigned long sp_usr)
 
 bool is_user(void)
 {
-	return current_thread_info()->flags & TIF_USER_MODE;
+	struct thread_info *ti = current_thread_info();
+	return (ti->flags & TIF_VALID) && (ti->flags & TIF_USER_MODE);
 }
 
 bool __mmu_enabled(void)
diff --git a/lib/arm64/processor.c b/lib/arm64/processor.c
index ef55862..231d71e 100644
--- a/lib/arm64/processor.c
+++ b/lib/arm64/processor.c
@@ -227,7 +227,7 @@ static void __thread_info_init(struct thread_info *ti, unsigned int flags)
 {
 	memset(ti, 0, sizeof(struct thread_info));
 	ti->cpu = mpidr_to_cpu(get_mpidr());
-	ti->flags = flags;
+	ti->flags = flags | TIF_VALID;
 }
 
 void thread_info_init(struct thread_info *ti, unsigned int flags)
@@ -255,7 +255,8 @@ void start_usr(void (*func)(void *arg), void *arg, unsigned long sp_usr)
 
 bool is_user(void)
 {
-	return current_thread_info()->flags & TIF_USER_MODE;
+	struct thread_info *ti = current_thread_info();
+	return (ti->flags & TIF_VALID) && (ti->flags & TIF_USER_MODE);
 }
 
 bool __mmu_enabled(void)
-- 
2.25.1

[kvm-unit-tests PATCH 4/4] arm/arm64: Add sanity checks to the cpumask API

[Nikos Nikoleris]

Make sure that any calls to the cpumask API target a valid CPU. The
CPU is identified by an int in the range [0, nr_cpus), where nr_cpus
is set based on information in the DT.

Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
---
 lib/arm/asm/cpumask.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/arm/asm/cpumask.h b/lib/arm/asm/cpumask.h
index 02124de..3fa57bf 100644
--- a/lib/arm/asm/cpumask.h
+++ b/lib/arm/asm/cpumask.h
@@ -20,26 +20,31 @@ typedef struct cpumask {
 
 static inline void cpumask_set_cpu(int cpu, cpumask_t *mask)
 {
+	assert(cpu >= 0 && cpu < nr_cpus);
 	set_bit(cpu, cpumask_bits(mask));
 }
 
 static inline void cpumask_clear_cpu(int cpu, cpumask_t *mask)
 {
+	assert(cpu >= 0 && cpu < nr_cpus);
 	clear_bit(cpu, cpumask_bits(mask));
 }
 
 static inline int cpumask_test_cpu(int cpu, const cpumask_t *mask)
 {
+	assert(cpu >= 0 && cpu < nr_cpus);
 	return test_bit(cpu, cpumask_bits(mask));
 }
 
 static inline int cpumask_test_and_set_cpu(int cpu, cpumask_t *mask)
 {
+	assert(cpu >= 0 && cpu < nr_cpus);
 	return test_and_set_bit(cpu, cpumask_bits(mask));
 }
 
 static inline int cpumask_test_and_clear_cpu(int cpu, cpumask_t *mask)
 {
+	assert(cpu >= 0 && cpu < nr_cpus);
 	return test_and_clear_bit(cpu, cpumask_bits(mask));
 }
 
-- 
2.25.1

Re: [kvm-unit-tests PATCH 1/4] arm/arm64: Avoid calling cpumask_test_cpu for CPUs above nr_cpu

[Andrew Jones]

On Fri, Mar 19, 2021 at 12:24:11PM +0000, Nikos Nikoleris wrote:
> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
> ---
>  lib/arm/asm/cpumask.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/arm/asm/cpumask.h b/lib/arm/asm/cpumask.h
> index 6683bb6..02124de 100644
> --- a/lib/arm/asm/cpumask.h
> +++ b/lib/arm/asm/cpumask.h
> @@ -105,7 +105,7 @@ static inline void cpumask_copy(cpumask_t *dst, const cpumask_t *src)
>  
>  static inline int cpumask_next(int cpu, const cpumask_t *mask)
>  {
> -	while (cpu < nr_cpus && !cpumask_test_cpu(++cpu, mask))
> +	while (++cpu < nr_cpus && !cpumask_test_cpu(cpu, mask))
>  		;
>  	return cpu;

This looks like the right thing to do, but I'm surprised that
I've never seen an assert in cpumask_test_cpu, even though
it looks like we call cpumask_next with cpu == nr_cpus - 1
in several places.

Can you please add a commit message explaining how you found
this bug?

Thanks,
drew

Re: [kvm-unit-tests PATCH 1/4] arm/arm64: Avoid calling cpumask_test_cpu for CPUs above nr_cpu

[Nikos Nikoleris]

Hi Drew,

On 22/03/2021 09:31, Andrew Jones wrote:
> On Fri, Mar 19, 2021 at 12:24:11PM +0000, Nikos Nikoleris wrote:
>> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
>> ---
>>   lib/arm/asm/cpumask.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/arm/asm/cpumask.h b/lib/arm/asm/cpumask.h
>> index 6683bb6..02124de 100644
>> --- a/lib/arm/asm/cpumask.h
>> +++ b/lib/arm/asm/cpumask.h
>> @@ -105,7 +105,7 @@ static inline void cpumask_copy(cpumask_t *dst, const cpumask_t *src)
>>   
>>   static inline int cpumask_next(int cpu, const cpumask_t *mask)
>>   {
>> -	while (cpu < nr_cpus && !cpumask_test_cpu(++cpu, mask))
>> +	while (++cpu < nr_cpus && !cpumask_test_cpu(cpu, mask))
>>   		;
>>   	return cpu;
> 

Thanks for reviewing this!


> This looks like the right thing to do, but I'm surprised that
> I've never seen an assert in cpumask_test_cpu, even though
> it looks like we call cpumask_next with cpu == nr_cpus - 1
> in several places.
> 

cpumask_next() would trigger one of the assertions in the 4th patch in 
this series without this fix. The 4th patch is a way to demonstrate (if 
we apply it without the rest) the problem of using cpu0's 
thread_info->cpu uninitialized.

> Can you please add a commit message explaining how you found
> this bug?
> 

Yes I'll do that.

Thanks,

Nikos

> Thanks,
> drew
> 

Re: [kvm-unit-tests PATCH 1/4] arm/arm64: Avoid calling cpumask_test_cpu for CPUs above nr_cpu

[Andrew Jones]

On Mon, Mar 22, 2021 at 09:45:09AM +0000, Nikos Nikoleris wrote:
> Hi Drew,
> 
> On 22/03/2021 09:31, Andrew Jones wrote:
> > On Fri, Mar 19, 2021 at 12:24:11PM +0000, Nikos Nikoleris wrote:
> > > Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
> > > ---
> > >   lib/arm/asm/cpumask.h | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/lib/arm/asm/cpumask.h b/lib/arm/asm/cpumask.h
> > > index 6683bb6..02124de 100644
> > > --- a/lib/arm/asm/cpumask.h
> > > +++ b/lib/arm/asm/cpumask.h
> > > @@ -105,7 +105,7 @@ static inline void cpumask_copy(cpumask_t *dst, const cpumask_t *src)
> > >   static inline int cpumask_next(int cpu, const cpumask_t *mask)
> > >   {
> > > -	while (cpu < nr_cpus && !cpumask_test_cpu(++cpu, mask))
> > > +	while (++cpu < nr_cpus && !cpumask_test_cpu(cpu, mask))
> > >   		;
> > >   	return cpu;
> > 
> 
> Thanks for reviewing this!
> 
> 
> > This looks like the right thing to do, but I'm surprised that
> > I've never seen an assert in cpumask_test_cpu, even though
> > it looks like we call cpumask_next with cpu == nr_cpus - 1
> > in several places.
> > 
> 
> cpumask_next() would trigger one of the assertions in the 4th patch in this
> series without this fix. The 4th patch is a way to demonstrate (if we apply
> it without the rest) the problem of using cpu0's thread_info->cpu
> uninitialized.

Ah, I see my error. I had already applied your 4th patch but hadn't
reviewed it yet, so I didn't realize it was new code. Now it makes
sense that we didn't hit that assert before (it didn't exist
before :-)

> 
> > Can you please add a commit message explaining how you found
> > this bug?
> > 
> 
> Yes I'll do that.

If you just write one here then I'll add it while applying. The rest of
the patches look good to me. So no need to respin.

Thanks,
drew

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

[Alexandru Elisei]

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;
> +}

Re: [kvm-unit-tests PATCH 3/4] arm/arm64: Track whether thread_info has been initialized

[Alexandru Elisei]

Hi Nikos,

On 3/19/21 12:24 PM, Nikos Nikoleris wrote:
> Introduce a new flag in the thread_info to track whether a thread_info
> struct is initialized yet or not.

There's no explanation why this is needed. The flag checked only by is_user(), and
before thread_info is initialized, flags is zero, so is_user() would return false,
right? Or am I missing something?

Thanks,

Alex

>
> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
> ---
>  lib/arm/asm/thread_info.h | 1 +
>  lib/arm/processor.c       | 5 +++--
>  lib/arm64/processor.c     | 5 +++--
>  3 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/lib/arm/asm/thread_info.h b/lib/arm/asm/thread_info.h
> index eaa7258..926c2a3 100644
> --- a/lib/arm/asm/thread_info.h
> +++ b/lib/arm/asm/thread_info.h
> @@ -45,6 +45,7 @@ static inline void *thread_stack_alloc(void)
>  }
>  
>  #define TIF_USER_MODE		(1U << 0)
> +#define TIF_VALID		(1U << 1)
>  
>  struct thread_info {
>  	int cpu;
> diff --git a/lib/arm/processor.c b/lib/arm/processor.c
> index a2d39a4..702fbc3 100644
> --- a/lib/arm/processor.c
> +++ b/lib/arm/processor.c
> @@ -119,7 +119,7 @@ void thread_info_init(struct thread_info *ti, unsigned int flags)
>  {
>  	memset(ti, 0, sizeof(struct thread_info));
>  	ti->cpu = mpidr_to_cpu(get_mpidr());
> -	ti->flags = flags;
> +	ti->flags = flags | TIF_VALID;
>  }
>  
>  void start_usr(void (*func)(void *arg), void *arg, unsigned long sp_usr)
> @@ -143,7 +143,8 @@ void start_usr(void (*func)(void *arg), void *arg, unsigned long sp_usr)
>  
>  bool is_user(void)
>  {
> -	return current_thread_info()->flags & TIF_USER_MODE;
> +	struct thread_info *ti = current_thread_info();
> +	return (ti->flags & TIF_VALID) && (ti->flags & TIF_USER_MODE);
>  }
>  
>  bool __mmu_enabled(void)
> diff --git a/lib/arm64/processor.c b/lib/arm64/processor.c
> index ef55862..231d71e 100644
> --- a/lib/arm64/processor.c
> +++ b/lib/arm64/processor.c
> @@ -227,7 +227,7 @@ static void __thread_info_init(struct thread_info *ti, unsigned int flags)
>  {
>  	memset(ti, 0, sizeof(struct thread_info));
>  	ti->cpu = mpidr_to_cpu(get_mpidr());
> -	ti->flags = flags;
> +	ti->flags = flags | TIF_VALID;
>  }
>  
>  void thread_info_init(struct thread_info *ti, unsigned int flags)
> @@ -255,7 +255,8 @@ void start_usr(void (*func)(void *arg), void *arg, unsigned long sp_usr)
>  
>  bool is_user(void)
>  {
> -	return current_thread_info()->flags & TIF_USER_MODE;
> +	struct thread_info *ti = current_thread_info();
> +	return (ti->flags & TIF_VALID) && (ti->flags & TIF_USER_MODE);
>  }
>  
>  bool __mmu_enabled(void)

Re: [kvm-unit-tests PATCH 1/4] arm/arm64: Avoid calling cpumask_test_cpu for CPUs above nr_cpu

[Nikos Nikoleris]

On 22/03/2021 10:12, Andrew Jones wrote:
> On Mon, Mar 22, 2021 at 09:45:09AM +0000, Nikos Nikoleris wrote:
>> Hi Drew,
>>
>> On 22/03/2021 09:31, Andrew Jones wrote:
>>> On Fri, Mar 19, 2021 at 12:24:11PM +0000, Nikos Nikoleris wrote:
>>>> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
>>>> ---
>>>>    lib/arm/asm/cpumask.h | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/lib/arm/asm/cpumask.h b/lib/arm/asm/cpumask.h
>>>> index 6683bb6..02124de 100644
>>>> --- a/lib/arm/asm/cpumask.h
>>>> +++ b/lib/arm/asm/cpumask.h
>>>> @@ -105,7 +105,7 @@ static inline void cpumask_copy(cpumask_t *dst, const cpumask_t *src)
>>>>    static inline int cpumask_next(int cpu, const cpumask_t *mask)
>>>>    {
>>>> -  while (cpu < nr_cpus && !cpumask_test_cpu(++cpu, mask))
>>>> +  while (++cpu < nr_cpus && !cpumask_test_cpu(cpu, mask))
>>>>                    ;
>>>>            return cpu;
>>>
>>
>> Thanks for reviewing this!
>>
>>
>>> This looks like the right thing to do, but I'm surprised that
>>> I've never seen an assert in cpumask_test_cpu, even though
>>> it looks like we call cpumask_next with cpu == nr_cpus - 1
>>> in several places.
>>>
>>
>> cpumask_next() would trigger one of the assertions in the 4th patch in this
>> series without this fix. The 4th patch is a way to demonstrate (if we apply
>> it without the rest) the problem of using cpu0's thread_info->cpu
>> uninitialized.
>
> Ah, I see my error. I had already applied your 4th patch but hadn't
> reviewed it yet, so I didn't realize it was new code. Now it makes
> sense that we didn't hit that assert before (it didn't exist
> before :-)
>
>>
>>> Can you please add a commit message explaining how you found
>>> this bug?
>>>
>>
>> Yes I'll do that.
>
> If you just write one here then I'll add it while applying. The rest of
> the patches look good to me. So no need to respin.
>

Sounds good! Maybe we can add something along the lines of:

Prior to this change, a call of cpumask_next(cpu, mask) where cpu=nr_cpu
- 1 (assuming all cpus are enumerated in the range 0..nr_cpus - 1) would
make an out-of-bounds access to the mask. In many cases, this is still a
valid memory location due the implementation of cpumask_t, however, in
certain configurations (for example, nr_cpus == sizeof(long)) this would
cause an access outside the bounds of the mask too.

This patch changes the way we guard calls to cpumask_test_cpu() in
cpumask_next() to avoid the above condition. A following change adds
assertions to catch out-of-bounds accesses to cpumask_t.

Thanks,

Nikos

> Thanks,
> drew
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [kvm-unit-tests PATCH 1/4] arm/arm64: Avoid calling cpumask_test_cpu for CPUs above nr_cpu

[Andrew Jones]

On Mon, Mar 22, 2021 at 10:40:26AM +0000, Nikos Nikoleris wrote:
> 
> Prior to this change, a call of cpumask_next(cpu, mask) where cpu=nr_cpu
> - 1 (assuming all cpus are enumerated in the range 0..nr_cpus - 1) would
> make an out-of-bounds access to the mask. In many cases, this is still a
> valid memory location due the implementation of cpumask_t, however, in
> certain configurations (for example, nr_cpus == sizeof(long)) this would
> cause an access outside the bounds of the mask too.
> 
> This patch changes the way we guard calls to cpumask_test_cpu() in
> cpumask_next() to avoid the above condition. A following change adds
> assertions to catch out-of-bounds accesses to cpumask_t.
> 

Thanks, I've added it. It looks like Alexandru would also like commit
message improvements. I can add those too.

Thanks,
drew

Re: [kvm-unit-tests PATCH 3/4] arm/arm64: Track whether thread_info has been initialized

[Nikos Nikoleris]

Hi Alex,

On 22/03/2021 10:34, Alexandru Elisei wrote:
> Hi Nikos,
> 
> On 3/19/21 12:24 PM, Nikos Nikoleris wrote:
>> Introduce a new flag in the thread_info to track whether a thread_info
>> struct is initialized yet or not.
> 
> There's no explanation why this is needed. The flag checked only by is_user(), and
> before thread_info is initialized, flags is zero, so is_user() would return false,
> right? Or am I missing something?
> 

I am still not sure what's the right approach here. I didn't like and I 
still don't like the fact that we rely on implicit 0 initialization to 
get the right behavior. This will break once we add support for EFI. I 
think we should explicitly initialize thread_info to 0. I was thinking 
of adding a thread_info_alloc() function to do this.

By having this flag I think we can guard accesses to the thread_info in 
general. I didn't want to turn the define smp_processor_id to a function 
here but I think we should and assert that the thread_info is valid and 
avoid reading current_thread_info()->cpu.

Having said that, this would still work without the patch and I am happy 
to drop it if the above doesn't makes sense.

Thanks,

Nikos

> Thanks,
> 
> Alex
> 
>>
>> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
>> ---
>>   lib/arm/asm/thread_info.h | 1 +
>>   lib/arm/processor.c       | 5 +++--
>>   lib/arm64/processor.c     | 5 +++--
>>   3 files changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/arm/asm/thread_info.h b/lib/arm/asm/thread_info.h
>> index eaa7258..926c2a3 100644
>> --- a/lib/arm/asm/thread_info.h
>> +++ b/lib/arm/asm/thread_info.h
>> @@ -45,6 +45,7 @@ static inline void *thread_stack_alloc(void)
>>   }
>>   
>>   #define TIF_USER_MODE		(1U << 0)
>> +#define TIF_VALID		(1U << 1)
>>   
>>   struct thread_info {
>>   	int cpu;
>> diff --git a/lib/arm/processor.c b/lib/arm/processor.c
>> index a2d39a4..702fbc3 100644
>> --- a/lib/arm/processor.c
>> +++ b/lib/arm/processor.c
>> @@ -119,7 +119,7 @@ void thread_info_init(struct thread_info *ti, unsigned int flags)
>>   {
>>   	memset(ti, 0, sizeof(struct thread_info));
>>   	ti->cpu = mpidr_to_cpu(get_mpidr());
>> -	ti->flags = flags;
>> +	ti->flags = flags | TIF_VALID;
>>   }
>>   
>>   void start_usr(void (*func)(void *arg), void *arg, unsigned long sp_usr)
>> @@ -143,7 +143,8 @@ void start_usr(void (*func)(void *arg), void *arg, unsigned long sp_usr)
>>   
>>   bool is_user(void)
>>   {
>> -	return current_thread_info()->flags & TIF_USER_MODE;
>> +	struct thread_info *ti = current_thread_info();
>> +	return (ti->flags & TIF_VALID) && (ti->flags & TIF_USER_MODE);
>>   }
>>   
>>   bool __mmu_enabled(void)
>> diff --git a/lib/arm64/processor.c b/lib/arm64/processor.c
>> index ef55862..231d71e 100644
>> --- a/lib/arm64/processor.c
>> +++ b/lib/arm64/processor.c
>> @@ -227,7 +227,7 @@ static void __thread_info_init(struct thread_info *ti, unsigned int flags)
>>   {
>>   	memset(ti, 0, sizeof(struct thread_info));
>>   	ti->cpu = mpidr_to_cpu(get_mpidr());
>> -	ti->flags = flags;
>> +	ti->flags = flags | TIF_VALID;
>>   }
>>   
>>   void thread_info_init(struct thread_info *ti, unsigned int flags)
>> @@ -255,7 +255,8 @@ void start_usr(void (*func)(void *arg), void *arg, unsigned long sp_usr)
>>   
>>   bool is_user(void)
>>   {
>> -	return current_thread_info()->flags & TIF_USER_MODE;
>> +	struct thread_info *ti = current_thread_info();
>> +	return (ti->flags & TIF_VALID) && (ti->flags & TIF_USER_MODE);
>>   }
>>   
>>   bool __mmu_enabled(void)

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

[Nikos Nikoleris]

Hi Alex,

On 22/03/2021 10:30, Alexandru Elisei wrote:
> 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.
> 

Thanks for the review!

> 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.
> 

Would this make more sense then?

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.

In addition, this change:
* Removes mmu_disabled_cpu_count as it is no
longer necessary and assumed that calls to 
mmu_mark_enabled()/mmu_mark_disabled() were serialized. This is 
currently true but a future change could easily break that assumption.
* Changes mmu_disabled_mask to mmu_enabled_mask and inverts the logic to 
track in a more intuitive way that all CPUs start with the MMU OFF and 
at some point, we turn them ON.

> 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.

Indeed, I didn't catch that initially but I think you're right.

Thanks,

Nikos

> 
> 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;
>> +}

Re: [kvm-unit-tests PATCH 3/4] arm/arm64: Track whether thread_info has been initialized

[Andrew Jones]

On Mon, Mar 22, 2021 at 10:59:31AM +0000, Nikos Nikoleris wrote:
> Hi Alex,
> 
> On 22/03/2021 10:34, Alexandru Elisei wrote:
> > Hi Nikos,
> > 
> > On 3/19/21 12:24 PM, Nikos Nikoleris wrote:
> > > Introduce a new flag in the thread_info to track whether a thread_info
> > > struct is initialized yet or not.
> > 
> > There's no explanation why this is needed. The flag checked only by is_user(), and
> > before thread_info is initialized, flags is zero, so is_user() would return false,
> > right? Or am I missing something?
> > 
> 
> I am still not sure what's the right approach here. I didn't like and I
> still don't like the fact that we rely on implicit 0 initialization to get
> the right behavior. This will break once we add support for EFI. I think we
> should explicitly initialize thread_info to 0.

I just sent a patch doing this. Let me know what you think.


> I was thinking of adding a
> thread_info_alloc() function to do this.

I'm not sure how this would look. We want the thread-info to live on the
bottom of the stack and the bootcpu's stack is allocated in the linker
script.

> 
> By having this flag I think we can guard accesses to the thread_info in
> general. I didn't want to turn the define smp_processor_id to a function
> here but I think we should and assert that the thread_info is valid and
> avoid reading current_thread_info()->cpu.

Hmm, yeah, hopefully we can avoid this flag and adding an assert to
smp_processor_id(). Let's take another look at this after we ensure
that the thread-info is explicitly zeroed at startup.

Thanks,
drew

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

[Alexandru Elisei]

Hi Nikos,

On 3/22/21 11:14 AM, Nikos Nikoleris wrote:
> Hi Alex,
>
> On 22/03/2021 10:30, Alexandru Elisei wrote:
>> 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.
>>
>
> Thanks for the review!
>
>> 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.
>>
>
> Would this make more sense then?
>
> 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.

I think the last sentence should be removed. If this patch relies on is_user()
working correctly and is_user() is not, then is_user() should be fixed before this
patch. If is_user() is working correctly and another patch modifies it for reasons
other than correctness, then it shouldn't be mentioned here.

>
> In addition, this change:
> * Removes mmu_disabled_cpu_count as it is no
> longer necessary and assumed that calls to
> mmu_mark_enabled()/mmu_mark_disabled() were serialized. This is currently true
> but a future change could easily break that assumption.
> * Changes mmu_disabled_mask to mmu_enabled_mask and inverts the logic to track
> in a more intuitive way that all CPUs start with the MMU OFF and at some point,
> we turn them ON.

Much better, thanks.

Nitpicks below.

>
>> 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.
>
> Indeed, I didn't catch that initially but I think you're right.
>
> Thanks,
>
> Nikos
>
>>
>> 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);
>>> +}

I don't think this is needed. The get_<sysreg>() functions above are here because
they are used from arch independent code, and a function with an identical
declaration is also present in lib/arm64/asm/processor.h.

I think the function can be folded into lib/arm/processor.c::__mmu_enabled(),
similar to __mmu_enabled() for arm64.

>>> +
>>>   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();
>>>   }

This is how mmu_enabled() looks like with this patch:

bool mmu_enabled(void)
{
    int cpu = current_thread_info()->cpu;

    /*
     * mmu_enabled is called from places that are guarding the
     * use of exclusive ops (which require the mmu to be enabled).
     * That means we CANNOT call anything from here that may use a
     * spinlock, atomic bitop, etc., otherwise we'll recurse.
     * [cpumask_]test_bit is safe though.
     */
    if (is_user())
        return cpumask_test_cpu(cpu, &mmu_enabled_cpumask);
    else
        return __mmu_enabled();
}

The commit message says that "[..] we don't have to rely on the cpu id in
thread_info when we are in setup executing in EL1", but still we read it here
unconditionally. Maybe the assignment could be moved to the is_user() branch, to
make it absolutely clear current_thread_info() is not always correct when calling
mmu_enabled()?

Thanks,

Alex

>>>     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;
>>> +}

Re: [kvm-unit-tests PATCH 0/4] RFC: Minor arm/arm64 MMU fixes and checks

[Andrew Jones]

On Fri, Mar 19, 2021 at 12:24:10PM +0000, Nikos Nikoleris wrote:
> Prior to this set of fixes, the code in setup() which we call to
> initialize the system has a circular dependency. cpu_init()
> (eventually) calls spin_lock() and __mmu_enabled(). However, at this
> point, __mmu_enabled() may have undefined behavior as we haven't
> initialized the current thread_info (cpu field). Moving
> thread_info_init() above cpu_init() is not possible as it relies on
> mpidr_to_cpu() which won't return the right results before cpu_init().
> In addition, mem_init() also calls __mmu_enabled() and therefore
> suffers from the same problem. Right now, everything works as
> thread_info maps to memory which is implicitly initialized to 0 (cpu =
> 0) and makes the assumption that the cpu that runs setup() will be the
> first cpu in the DT.
> 
> This set of patches changes the code slightly and avoids this
> assumptions. In addition, it adds assertions to catch problems like
> the above. The current solution relies on the thread_info() of the cpu
> that setup() run to be initialized to zero (should we make it
> explicit?).
> 
> There are a couple of options I considered in addressing this issue
> which didn't seem satisfactory:
> 
> - Change the mmu_disabled_count global variable to mmu_enabled_count
>   to count the number of active mmu's and bypass __mmu_enabled() when
>   it's 0. This is a global variable and at the momement all write to
>   it are protected by a lock but it's rather fragile and could easily
>   cause issues in the future.
> - Explicitly initialize current_thread_info()->cpu = 0 in setup()
>   before anything else or make the first call of thread_info_init() a
>   special case and avoid looking up mpidr_to_cpu(). This way we can
>   move thread_info_init() to the top of setup(). If the CPU setup is
>   running on is not the first that appears in the DT then this
>   solution won't work.
> 
> Thanks,
> 
> Nikos
> 
> Nikos Nikoleris (4):
>   arm/arm64: Avoid calling cpumask_test_cpu for CPUs above nr_cpu
>   arm/arm64: Read system registers to get the state of the MMU
>   arm/arm64: Track whether thread_info has been initialized
>   arm/arm64: Add sanity checks to the cpumask API
> 
>  lib/arm/asm/cpumask.h     |  7 ++++++-
>  lib/arm/asm/mmu-api.h     |  7 +------
>  lib/arm/asm/processor.h   |  7 +++++++
>  lib/arm/asm/thread_info.h |  1 +
>  lib/arm64/asm/processor.h |  1 +
>  lib/arm/mmu.c             | 16 ++++++++--------
>  lib/arm/processor.c       | 10 ++++++++--
>  lib/arm64/processor.c     | 10 ++++++++--
>  8 files changed, 40 insertions(+), 19 deletions(-)
> 
> -- 
> 2.25.1
>

Hi Nikos,

So, I like patches 1, 2, and 4. I think 3 can be dropped with the
addition of "arm/arm64: Zero BSS and stack at startup". Would you
agree? I've applied all the updated commit messages and all of
Alexandru's suggestions to 2. Patch 2 now looks like the diff below.

Alexandru, are you satisfied with 1, 2, and 4? If so, please let me
know and I'll add r-b's for you before queuing.

Thanks,
drew


diff --git a/lib/arm/asm/mmu-api.h b/lib/arm/asm/mmu-api.h
index 3d04d0312622..05fc12b5afb8 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 273366d1fe1c..6b762ad060eb 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)
 {
diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
index a1862a55db8b..15eef007f256 100644
--- a/lib/arm/mmu.c
+++ b/lib/arm/mmu.c
@@ -24,13 +24,10 @@ 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;
 
-bool __mmu_enabled(void)
+bool mmu_enabled(void)
 {
-	int cpu = current_thread_info()->cpu;
-
 	/*
 	 * mmu_enabled is called from places that are guarding the
 	 * use of exclusive ops (which require the mmu to be enabled).
@@ -38,19 +35,22 @@ 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()) {
+		int cpu = current_thread_info()->cpu;
+		return cpumask_test_cpu(cpu, &mmu_enabled_cpumask);
+	}
+
+	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 773337e6d3b7..9d5759686b73 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 read_sysreg(SCTRL) & CR_M;
+}
diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
index 771b2d1e0c94..8e2037e1a43e 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/arm64/processor.c b/lib/arm64/processor.c
index 2a024e3f4e9d..ef558625e284 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;
+}

Re: [kvm-unit-tests PATCH 0/4] RFC: Minor arm/arm64 MMU fixes and checks

[Alexandru Elisei]

Hi Drew,

On 3/23/21 11:24 AM, Andrew Jones wrote:
> On Fri, Mar 19, 2021 at 12:24:10PM +0000, Nikos Nikoleris wrote:
>> Prior to this set of fixes, the code in setup() which we call to
>> initialize the system has a circular dependency. cpu_init()
>> (eventually) calls spin_lock() and __mmu_enabled(). However, at this
>> point, __mmu_enabled() may have undefined behavior as we haven't
>> initialized the current thread_info (cpu field). Moving
>> thread_info_init() above cpu_init() is not possible as it relies on
>> mpidr_to_cpu() which won't return the right results before cpu_init().
>> In addition, mem_init() also calls __mmu_enabled() and therefore
>> suffers from the same problem. Right now, everything works as
>> thread_info maps to memory which is implicitly initialized to 0 (cpu =
>> 0) and makes the assumption that the cpu that runs setup() will be the
>> first cpu in the DT.
>>
>> This set of patches changes the code slightly and avoids this
>> assumptions. In addition, it adds assertions to catch problems like
>> the above. The current solution relies on the thread_info() of the cpu
>> that setup() run to be initialized to zero (should we make it
>> explicit?).
>>
>> There are a couple of options I considered in addressing this issue
>> which didn't seem satisfactory:
>>
>> - Change the mmu_disabled_count global variable to mmu_enabled_count
>>   to count the number of active mmu's and bypass __mmu_enabled() when
>>   it's 0. This is a global variable and at the momement all write to
>>   it are protected by a lock but it's rather fragile and could easily
>>   cause issues in the future.
>> - Explicitly initialize current_thread_info()->cpu = 0 in setup()
>>   before anything else or make the first call of thread_info_init() a
>>   special case and avoid looking up mpidr_to_cpu(). This way we can
>>   move thread_info_init() to the top of setup(). If the CPU setup is
>>   running on is not the first that appears in the DT then this
>>   solution won't work.
>>
>> Thanks,
>>
>> Nikos
>>
>> Nikos Nikoleris (4):
>>   arm/arm64: Avoid calling cpumask_test_cpu for CPUs above nr_cpu
>>   arm/arm64: Read system registers to get the state of the MMU
>>   arm/arm64: Track whether thread_info has been initialized
>>   arm/arm64: Add sanity checks to the cpumask API
>>
>>  lib/arm/asm/cpumask.h     |  7 ++++++-
>>  lib/arm/asm/mmu-api.h     |  7 +------
>>  lib/arm/asm/processor.h   |  7 +++++++
>>  lib/arm/asm/thread_info.h |  1 +
>>  lib/arm64/asm/processor.h |  1 +
>>  lib/arm/mmu.c             | 16 ++++++++--------
>>  lib/arm/processor.c       | 10 ++++++++--
>>  lib/arm64/processor.c     | 10 ++++++++--
>>  8 files changed, 40 insertions(+), 19 deletions(-)
>>
>> -- 
>> 2.25.1
>>
> Hi Nikos,
>
> So, I like patches 1, 2, and 4. I think 3 can be dropped with the
> addition of "arm/arm64: Zero BSS and stack at startup". Would you
> agree? I've applied all the updated commit messages and all of
> Alexandru's suggestions to 2. Patch 2 now looks like the diff below.
>
> Alexandru, are you satisfied with 1, 2, and 4? If so, please let me
> know and I'll add r-b's for you before queuing.

The diff looks good, and I agree that we can come back to #3 once we have a better
understanding of what needs to be done. Patch #1 is a nice fix for what I imagine
was a hard to find bug. Patch #4 also looks correct to me. Yes, please add my
Reviewed-by for the series.

Thanks,

Alex

>
> Thanks,
> drew
>
>
> diff --git a/lib/arm/asm/mmu-api.h b/lib/arm/asm/mmu-api.h
> index 3d04d0312622..05fc12b5afb8 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 273366d1fe1c..6b762ad060eb 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)
>  {
> diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
> index a1862a55db8b..15eef007f256 100644
> --- a/lib/arm/mmu.c
> +++ b/lib/arm/mmu.c
> @@ -24,13 +24,10 @@ 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;
>  
> -bool __mmu_enabled(void)
> +bool mmu_enabled(void)
>  {
> -	int cpu = current_thread_info()->cpu;
> -
>  	/*
>  	 * mmu_enabled is called from places that are guarding the
>  	 * use of exclusive ops (which require the mmu to be enabled).
> @@ -38,19 +35,22 @@ 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()) {
> +		int cpu = current_thread_info()->cpu;
> +		return cpumask_test_cpu(cpu, &mmu_enabled_cpumask);
> +	}
> +
> +	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 773337e6d3b7..9d5759686b73 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 read_sysreg(SCTRL) & CR_M;
> +}
> diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
> index 771b2d1e0c94..8e2037e1a43e 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/arm64/processor.c b/lib/arm64/processor.c
> index 2a024e3f4e9d..ef558625e284 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;
> +}
>

Re: [kvm-unit-tests PATCH 0/4] RFC: Minor arm/arm64 MMU fixes and checks

[Andrew Jones]

On Tue, Mar 23, 2021 at 11:40:37AM +0000, Alexandru Elisei wrote:
> Hi Drew,
> 
> On 3/23/21 11:24 AM, Andrew Jones wrote:
> > On Fri, Mar 19, 2021 at 12:24:10PM +0000, Nikos Nikoleris wrote:
> >> Prior to this set of fixes, the code in setup() which we call to
> >> initialize the system has a circular dependency. cpu_init()
> >> (eventually) calls spin_lock() and __mmu_enabled(). However, at this
> >> point, __mmu_enabled() may have undefined behavior as we haven't
> >> initialized the current thread_info (cpu field). Moving
> >> thread_info_init() above cpu_init() is not possible as it relies on
> >> mpidr_to_cpu() which won't return the right results before cpu_init().
> >> In addition, mem_init() also calls __mmu_enabled() and therefore
> >> suffers from the same problem. Right now, everything works as
> >> thread_info maps to memory which is implicitly initialized to 0 (cpu =
> >> 0) and makes the assumption that the cpu that runs setup() will be the
> >> first cpu in the DT.
> >>
> >> This set of patches changes the code slightly and avoids this
> >> assumptions. In addition, it adds assertions to catch problems like
> >> the above. The current solution relies on the thread_info() of the cpu
> >> that setup() run to be initialized to zero (should we make it
> >> explicit?).
> >>
> >> There are a couple of options I considered in addressing this issue
> >> which didn't seem satisfactory:
> >>
> >> - Change the mmu_disabled_count global variable to mmu_enabled_count
> >>   to count the number of active mmu's and bypass __mmu_enabled() when
> >>   it's 0. This is a global variable and at the momement all write to
> >>   it are protected by a lock but it's rather fragile and could easily
> >>   cause issues in the future.
> >> - Explicitly initialize current_thread_info()->cpu = 0 in setup()
> >>   before anything else or make the first call of thread_info_init() a
> >>   special case and avoid looking up mpidr_to_cpu(). This way we can
> >>   move thread_info_init() to the top of setup(). If the CPU setup is
> >>   running on is not the first that appears in the DT then this
> >>   solution won't work.
> >>
> >> Thanks,
> >>
> >> Nikos
> >>
> >> Nikos Nikoleris (4):
> >>   arm/arm64: Avoid calling cpumask_test_cpu for CPUs above nr_cpu
> >>   arm/arm64: Read system registers to get the state of the MMU
> >>   arm/arm64: Track whether thread_info has been initialized
> >>   arm/arm64: Add sanity checks to the cpumask API
> >>
> >>  lib/arm/asm/cpumask.h     |  7 ++++++-
> >>  lib/arm/asm/mmu-api.h     |  7 +------
> >>  lib/arm/asm/processor.h   |  7 +++++++
> >>  lib/arm/asm/thread_info.h |  1 +
> >>  lib/arm64/asm/processor.h |  1 +
> >>  lib/arm/mmu.c             | 16 ++++++++--------
> >>  lib/arm/processor.c       | 10 ++++++++--
> >>  lib/arm64/processor.c     | 10 ++++++++--
> >>  8 files changed, 40 insertions(+), 19 deletions(-)
> >>
> >> -- 
> >> 2.25.1
> >>
> > Hi Nikos,
> >
> > So, I like patches 1, 2, and 4. I think 3 can be dropped with the
> > addition of "arm/arm64: Zero BSS and stack at startup". Would you
> > agree? I've applied all the updated commit messages and all of
> > Alexandru's suggestions to 2. Patch 2 now looks like the diff below.
> >
> > Alexandru, are you satisfied with 1, 2, and 4? If so, please let me
> > know and I'll add r-b's for you before queuing.
> 
> The diff looks good, and I agree that we can come back to #3 once we have a better
> understanding of what needs to be done. Patch #1 is a nice fix for what I imagine
> was a hard to find bug. Patch #4 also looks correct to me. Yes, please add my
> Reviewed-by for the series.
> 
> Thanks,
> 
> Alex

Thanks, applied 1, 2, and 4 to arm/queue

https://gitlab.com/rhdrjones/kvm-unit-tests/-/commits/arm/queue

Thanks,
drew


> 
> >
> > Thanks,
> > drew
> >
> >
> > diff --git a/lib/arm/asm/mmu-api.h b/lib/arm/asm/mmu-api.h
> > index 3d04d0312622..05fc12b5afb8 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 273366d1fe1c..6b762ad060eb 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)
> >  {
> > diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
> > index a1862a55db8b..15eef007f256 100644
> > --- a/lib/arm/mmu.c
> > +++ b/lib/arm/mmu.c
> > @@ -24,13 +24,10 @@ 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;
> >  
> > -bool __mmu_enabled(void)
> > +bool mmu_enabled(void)
> >  {
> > -	int cpu = current_thread_info()->cpu;
> > -
> >  	/*
> >  	 * mmu_enabled is called from places that are guarding the
> >  	 * use of exclusive ops (which require the mmu to be enabled).
> > @@ -38,19 +35,22 @@ 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()) {
> > +		int cpu = current_thread_info()->cpu;
> > +		return cpumask_test_cpu(cpu, &mmu_enabled_cpumask);
> > +	}
> > +
> > +	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 773337e6d3b7..9d5759686b73 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 read_sysreg(SCTRL) & CR_M;
> > +}
> > diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
> > index 771b2d1e0c94..8e2037e1a43e 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/arm64/processor.c b/lib/arm64/processor.c
> > index 2a024e3f4e9d..ef558625e284 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;
> > +}
> >
> 

Re: [kvm-unit-tests PATCH 0/4] RFC: Minor arm/arm64 MMU fixes and checks

[Nikos Nikoleris]

On 23/03/2021 11:51, Andrew Jones wrote:
> On Tue, Mar 23, 2021 at 11:40:37AM +0000, Alexandru Elisei wrote:
>> Hi Drew,
>>
>> On 3/23/21 11:24 AM, Andrew Jones wrote:
>>> On Fri, Mar 19, 2021 at 12:24:10PM +0000, Nikos Nikoleris wrote:
>>>> Prior to this set of fixes, the code in setup() which we call to
>>>> initialize the system has a circular dependency. cpu_init()
>>>> (eventually) calls spin_lock() and __mmu_enabled(). However, at this
>>>> point, __mmu_enabled() may have undefined behavior as we haven't
>>>> initialized the current thread_info (cpu field). Moving
>>>> thread_info_init() above cpu_init() is not possible as it relies on
>>>> mpidr_to_cpu() which won't return the right results before cpu_init().
>>>> In addition, mem_init() also calls __mmu_enabled() and therefore
>>>> suffers from the same problem. Right now, everything works as
>>>> thread_info maps to memory which is implicitly initialized to 0 (cpu =
>>>> 0) and makes the assumption that the cpu that runs setup() will be the
>>>> first cpu in the DT.
>>>>
>>>> This set of patches changes the code slightly and avoids this
>>>> assumptions. In addition, it adds assertions to catch problems like
>>>> the above. The current solution relies on the thread_info() of the cpu
>>>> that setup() run to be initialized to zero (should we make it
>>>> explicit?).
>>>>
>>>> There are a couple of options I considered in addressing this issue
>>>> which didn't seem satisfactory:
>>>>
>>>> - Change the mmu_disabled_count global variable to mmu_enabled_count
>>>>    to count the number of active mmu's and bypass __mmu_enabled() when
>>>>    it's 0. This is a global variable and at the momement all write to
>>>>    it are protected by a lock but it's rather fragile and could easily
>>>>    cause issues in the future.
>>>> - Explicitly initialize current_thread_info()->cpu = 0 in setup()
>>>>    before anything else or make the first call of thread_info_init() a
>>>>    special case and avoid looking up mpidr_to_cpu(). This way we can
>>>>    move thread_info_init() to the top of setup(). If the CPU setup is
>>>>    running on is not the first that appears in the DT then this
>>>>    solution won't work.
>>>>
>>>> Thanks,
>>>>
>>>> Nikos
>>>>
>>>> Nikos Nikoleris (4):
>>>>    arm/arm64: Avoid calling cpumask_test_cpu for CPUs above nr_cpu
>>>>    arm/arm64: Read system registers to get the state of the MMU
>>>>    arm/arm64: Track whether thread_info has been initialized
>>>>    arm/arm64: Add sanity checks to the cpumask API
>>>>
>>>>   lib/arm/asm/cpumask.h     |  7 ++++++-
>>>>   lib/arm/asm/mmu-api.h     |  7 +------
>>>>   lib/arm/asm/processor.h   |  7 +++++++
>>>>   lib/arm/asm/thread_info.h |  1 +
>>>>   lib/arm64/asm/processor.h |  1 +
>>>>   lib/arm/mmu.c             | 16 ++++++++--------
>>>>   lib/arm/processor.c       | 10 ++++++++--
>>>>   lib/arm64/processor.c     | 10 ++++++++--
>>>>   8 files changed, 40 insertions(+), 19 deletions(-)
>>>>
>>>> -- 
>>>> 2.25.1
>>>>
>>> Hi Nikos,
>>>
>>> So, I like patches 1, 2, and 4. I think 3 can be dropped with the
>>> addition of "arm/arm64: Zero BSS and stack at startup". Would you
>>> agree? I've applied all the updated commit messages and all of
>>> Alexandru's suggestions to 2. Patch 2 now looks like the diff below.
>>>
>>> Alexandru, are you satisfied with 1, 2, and 4? If so, please let me
>>> know and I'll add r-b's for you before queuing.
>>
>> The diff looks good, and I agree that we can come back to #3 once we have a better
>> understanding of what needs to be done. Patch #1 is a nice fix for what I imagine
>> was a hard to find bug. Patch #4 also looks correct to me. Yes, please add my
>> Reviewed-by for the series.
>>
>> Thanks,
>>
>> Alex
> 
> Thanks, applied 1, 2, and 4 to arm/queue
> 
> https://gitlab.com/rhdrjones/kvm-unit-tests/-/commits/arm/queue
> 
> Thanks,
> drew
> 

Looks good to me. Thank you both for the reviews and the suggestions!

Thanks,

Nikos

> 
>>
>>>
>>> Thanks,
>>> drew
>>>
>>>
>>> diff --git a/lib/arm/asm/mmu-api.h b/lib/arm/asm/mmu-api.h
>>> index 3d04d0312622..05fc12b5afb8 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 273366d1fe1c..6b762ad060eb 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)
>>>   {
>>> diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
>>> index a1862a55db8b..15eef007f256 100644
>>> --- a/lib/arm/mmu.c
>>> +++ b/lib/arm/mmu.c
>>> @@ -24,13 +24,10 @@ 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;
>>>   
>>> -bool __mmu_enabled(void)
>>> +bool mmu_enabled(void)
>>>   {
>>> -	int cpu = current_thread_info()->cpu;
>>> -
>>>   	/*
>>>   	 * mmu_enabled is called from places that are guarding the
>>>   	 * use of exclusive ops (which require the mmu to be enabled).
>>> @@ -38,19 +35,22 @@ 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()) {
>>> +		int cpu = current_thread_info()->cpu;
>>> +		return cpumask_test_cpu(cpu, &mmu_enabled_cpumask);
>>> +	}
>>> +
>>> +	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 773337e6d3b7..9d5759686b73 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 read_sysreg(SCTRL) & CR_M;
>>> +}
>>> diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
>>> index 771b2d1e0c94..8e2037e1a43e 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/arm64/processor.c b/lib/arm64/processor.c
>>> index 2a024e3f4e9d..ef558625e284 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;
>>> +}
>>>
>>
>