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

[Nikos Nikoleris <nikos.nikoleris@xxxxxxx>]

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@xxxxxxx>
> > ---
> >   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)