Re: [PATCH V8 09/44] x86/pkeys: Enable PKS on cpus which support it

Ira Weiny
2022-01-28T23:41:07Z

On Fri, Jan 28, 2022 at 03:18:29PM -0800, Dave Hansen wrote:
> On 1/27/22 09:54, ira.weiny@intel.com wrote:
> > Protection Keys for Supervisor pages (PKS) enables fast, hardware thread
> > specific, manipulation of permission restrictions on supervisor page
> > mappings. It uses the same mechanism of Protection Keys as those on
> > User mappings but applies that mechanism to supervisor mappings using a
> > supervisor specific MSR.
> >
> > Bit 24 of CR4 is used to enable the feature by software. Define
> > pks_setup() to be called when PKS is configured.
>
> Again, no need to specify the bit numbers. We have it in the code. :)
> At most, just say something like:
>
> PKS is enabled by a new bit in a control register.
> or
> PKS is enabled by a new bit in CR4.
>
> > Initially, pks_setup() initializes the per-cpu MSR with 0 to enable all
> > access on all pkeys.
>
> Why not just make it restrictive to start out? That's what we do for PKU.

This maintains compatibility with the code prior to this patch. Ie no
restrictions on kernel mappings.

I'll place the default value patch before this one and use it in this patch.

>
> > asm/pks.h is added as a new file to store new
> > internal functions and structures such as pks_setup().
>
> One writing nit: try to speak in active voice.
>
> Passive: "Foo is added"
> Active: "Add foo"
>
> It actually makes thing shorter and easier to read:
>
> Add asm/pks.h to store new internal functions and structures
> such as pks_setup().

Ok. I'll update the commit message.

>
> > diff --git a/arch/x86/include/uapi/asm/processor-flags.h b/arch/x86/include/uapi/asm/processor-flags.h
> > index bcba3c643e63..191c574b2390 100644
> > --- a/arch/x86/include/uapi/asm/processor-flags.h
> > +++ b/arch/x86/include/uapi/asm/processor-flags.h
> > @@ -130,6 +130,8 @@
> > #define X86_CR4_SMAP _BITUL(X86_CR4_SMAP_BIT)
> > #define X86_CR4_PKE_BIT 22 /* enable Protection Keys support */
> > #define X86_CR4_PKE _BITUL(X86_CR4_PKE_BIT)
> > +#define X86_CR4_PKS_BIT 24 /* enable Protection Keys for Supervisor */
> > +#define X86_CR4_PKS _BITUL(X86_CR4_PKS_BIT)
> >
> > /*
> > * x86-64 Task Priority Register, CR8
> > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> > index 7b8382c11788..83c1abce7d93 100644
> > --- a/arch/x86/kernel/cpu/common.c
> > +++ b/arch/x86/kernel/cpu/common.c
> > @@ -59,6 +59,7 @@
> > #include <asm/cpu_device_id.h>
> > #include <asm/uv/uv.h>
> > #include <asm/sigframe.h>
> > +#include <asm/pks.h>
> >
> > #include "cpu.h"
> >
> > @@ -1632,6 +1633,7 @@ static void identify_cpu(struct cpuinfo_x86 *c)
> >
> > x86_init_rdrand(c);
> > setup_pku(c);
> > + pks_setup();
> >
> > /*
> > * Clear/Set all flags overridden by options, need do it
> > diff --git a/arch/x86/mm/pkeys.c b/arch/x86/mm/pkeys.c
> > index cf12d8bf122b..02629219e683 100644
> > --- a/arch/x86/mm/pkeys.c
> > +++ b/arch/x86/mm/pkeys.c
> > @@ -206,3 +206,19 @@ u32 pkey_update_pkval(u32 pkval, int pkey, u32 accessbits)
> > pkval &= ~(PKEY_ACCESS_MASK << shift);
> > return pkval | accessbits << shift;
> > }
> > +
> > +#ifdef CONFIG_ARCH_ENABLE_SUPERVISOR_PKEYS
> > +
> > +/*
> > + * PKS is independent of PKU and either or both may be supported on a CPU.
> > + */
> > +void pks_setup(void)
> > +{
> > + if (!cpu_feature_enabled(X86_FEATURE_PKS))
> > + return;
> > +
> > + wrmsrl(MSR_IA32_PKRS, 0);
>
> This probably needs a one-line comment about what it's doing. As a
> general rule, I'd much rather have a one-sentence note in a code comment
> than in the changelog.

Fair enough,
Ira

>
> > + cr4_set_bits(X86_CR4_PKS);
> > +}
> > +
> > +#endif /* CONFIG_ARCH_ENABLE_SUPERVISOR_PKEYS */
>