Messages in this thread | | | Date | Fri, 28 Jan 2022 15:18:29 -0800 | From | Dave Hansen <> | Subject | Re: [PATCH V8 09/44] x86/pkeys: Enable PKS on cpus which support it |
| |
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.
> 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().
> 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.
> + cr4_set_bits(X86_CR4_PKS); > +} > + > +#endif /* CONFIG_ARCH_ENABLE_SUPERVISOR_PKEYS */
| |