| Message ID | 20210319122414.129364-4-nikos.nikoleris@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RFC: Minor arm/arm64 MMU fixes and checks | expand |
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)
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)
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
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)
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(-)