From: Andrew Jones <drjones@redhat.com>
To: Nikos Nikoleris <nikos.nikoleris@arm.com>
Cc: kvm@vger.kernel.org, alexandru.elisei@arm.com
Subject: Re: [kvm-unit-tests PATCH 0/4] RFC: Minor arm/arm64 MMU fixes and checks
Date: Tue, 23 Mar 2021 12:24:11 +0100 [thread overview]
Message-ID: <20210323112411.6uwqex6x2py4va6d@kamzik.brq.redhat.com> (raw)
In-Reply-To: <20210319122414.129364-1-nikos.nikoleris@arm.com>
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;
+}
next prev parent reply other threads:[~2021-03-23 11:25 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-19 12:24 [kvm-unit-tests PATCH 0/4] RFC: Minor arm/arm64 MMU fixes and checks Nikos Nikoleris
2021-03-19 12:24 ` [kvm-unit-tests PATCH 1/4] arm/arm64: Avoid calling cpumask_test_cpu for CPUs above nr_cpu Nikos Nikoleris
2021-03-22 9:31 ` Andrew Jones
2021-03-22 9:45 ` Nikos Nikoleris
2021-03-22 10:12 ` Andrew Jones
2021-03-22 10:40 ` Nikos Nikoleris
2021-03-22 10:53 ` Andrew Jones
2021-03-19 12:24 ` [kvm-unit-tests PATCH 2/4] arm/arm64: Read system registers to get the state of the MMU Nikos Nikoleris
2021-03-22 10:30 ` Alexandru Elisei
2021-03-22 11:14 ` Nikos Nikoleris
2021-03-22 15:25 ` Alexandru Elisei
2021-03-19 12:24 ` [kvm-unit-tests PATCH 3/4] arm/arm64: Track whether thread_info has been initialized Nikos Nikoleris
2021-03-22 10:34 ` Alexandru Elisei
2021-03-22 10:59 ` Nikos Nikoleris
2021-03-22 12:11 ` Andrew Jones
2021-03-19 12:24 ` [kvm-unit-tests PATCH 4/4] arm/arm64: Add sanity checks to the cpumask API Nikos Nikoleris
2021-03-23 11:24 ` Andrew Jones [this message]
2021-03-23 11:40 ` [kvm-unit-tests PATCH 0/4] RFC: Minor arm/arm64 MMU fixes and checks Alexandru Elisei
2021-03-23 11:51 ` Andrew Jones
2021-03-23 12:15 ` Nikos Nikoleris
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210323112411.6uwqex6x2py4va6d@kamzik.brq.redhat.com \
--to=drjones@redhat.com \
--cc=alexandru.elisei@arm.com \
--cc=kvm@vger.kernel.org \
--cc=nikos.nikoleris@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).