Re: [PATCH v2 4/4] powerpc/64: system call implement the bulk of the logic in C

[Nicholas Piggin <npiggin@gmail.com>]

Christophe Leroy's on August 28, 2019 4:51 pm:
> 
> 
> Le 27/08/2019 à 15:55, Nicholas Piggin a écrit :
>> -#include <asm/reg.h>
>> +#include <asm/mmu.h>
>> +#include <asm/ptrace.h>
>> +
>> +static inline void kuap_check_amr(void)
>> +{
>> +#ifdef CONFIG_PPC_KUAP_DEBUG
>> +	if (mmu_has_feature(MMU_FTR_RADIX_KUAP))
> 
> Better:
> 
> 	if (IS_ENABLED(CONFIG_PPC_KUAP_DEBUG) && 
> mmu_has_feature(MMU_FTR_RADIX_KUAP))

That is better.

>> +		WARN_ON_ONCE(mfspr(SPRN_AMR) != AMR_KUAP_BLOCKED);
>> +#endif
>> +}
>>   
>>   /*
>>    * We support individually allowing read or write, but we don't support nesting
>> diff --git a/arch/powerpc/include/asm/cputime.h b/arch/powerpc/include/asm/cputime.h
>> index 2431b4ada2fa..f3aa9db1a3cc 100644
>> --- a/arch/powerpc/include/asm/cputime.h
>> +++ b/arch/powerpc/include/asm/cputime.h
>> @@ -60,6 +60,28 @@ static inline void arch_vtime_task_switch(struct task_struct *prev)
>>   }
>>   #endif
>>   
>> +static inline void account_cpu_user_entry(void)
>> +{
>> +	unsigned long tb = mftb();
>> +
>> +	get_accounting(current)->utime += (tb - get_accounting(current)->starttime_user);
>> +	get_accounting(current)->starttime = tb;
>> +}
> 
> Can you check the generated assembly ? I remember having bad result with 
> get_accouting() being used several times in a arch_vtime_task_switch() 
> before commit 	60f1d2893ee6 ("powerpc/time: inline 
> arch_vtime_task_switch()")

It's fine on 64s but it's accounting is a constant offset from r13 so
simple load/store can be done.

> Regardless, I think it would look better as:
> 
> static inline void account_cpu_user_entry(void)
> {
> 	unsigned long tb = mftb();
> 	struct cpu_accounting_data *acct = get_accounting(current);
> 
> 	acct->utime += (tb - acct->starttime_user);
> 	acct->starttime = tb;
> }

Yeah that's nicer.

> 
>> +static inline void account_cpu_user_exit(void)
>> +{
>> +	unsigned long tb = mftb();
>> +
>> +	get_accounting(current)->stime += (tb - get_accounting(current)->starttime);
>> +	get_accounting(current)->starttime_user = tb;
>> +}
> 
> Same here.

Will do.

Thanks,
Nick