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

Dave Hansen
2022-01-28T23:18:50Z

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 */