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

[Nikos Nikoleris <nikos.nikoleris@arm.com>]

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)