Re: [PATCH V7 05/18] x86/pks: Add PKS setup code

[Thomas Gleixner]

On Thu, Nov 25 2021 at 16:15, Thomas Gleixner wrote:
> On Tue, Aug 03 2021 at 21:32, ira weiny wrote:
> Aside of that, the function which set's up the init value is really
> bogus. As you explained in the cover letter a kernel user has to:
>
>1) Claim an index in the enum
>2) Add a default value to the array in that function
>
> Seriously? How is that any better than doing:
>
> #define PKS_KEY0_DEFAULT  PKR_RW_ENABLE
> #define PKS_KEY1_DEFAULT  PKR_ACCESS_DISABLE
> ...
> #define PKS_KEY15_DEFAULT PKR_ACCESS_DISABLE
>
> and let the compiler construct pkrs_init_value?
>
> TBH, it's not and this function has to be ripped out in case that you
> need a dynamic allocation of keys some day. So what is this buying us
> aside of horrible to read and utterly pointless code?

And as Taoyi confirmed its broken.

It surely takes a reviewer to spot that and an external engineer to run
rdmsr -a to validate that this is not working as expected, right?

Sigh...

Re: [PATCH V7 05/18] x86/pks: Add PKS setup code

[Thomas Gleixner]

On Fri, Nov 26 2021 at 11:11, taoyi ty wrote:
> On 11/25/21 11:15 PM, Thomas Gleixner wrote:
>>> +void setup_pks(void)
>>> +{
>>> +   if (!cpu_feature_enabled(X86_FEATURE_PKS))
>>> +   return;
>>> +
>>> +   write_pkrs(pkrs_init_value);
>> 
>> Is the init value set up _before_ this function is invoked for the first
>> time?
>> 
> Setting up for cpu0 is before create_initial_pkrs_value. therefore pkrs 
> value of cpu0 won't be set correctly.
>
> [root@AliYun ~]# rdmsr -a 0x06E1
> 0
> 5554
> 5554
> 5554
> 5554
> 5554
> 5554
> 5554
> 5554
> 5554
>
> Here are my test results after applying the patches

Thanks for confirming what I assumed from looking at the patches!

   tglx