Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 28 Jan 2022 15:41:03 -0800
From:   Ira Weiny <ira.weiny@intel.com>
To:     Dave Hansen <dave.hansen@intel.com>
Cc:     Dave Hansen <dave.hansen@linux.intel.com>,
        "H. Peter Anvin" <hpa@zytor.com>,
        Dan Williams <dan.j.williams@intel.com>,
        Fenghua Yu <fenghua.yu@intel.com>,
        Rick Edgecombe <rick.p.edgecombe@intel.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH V8 09/44] x86/pkeys: Enable PKS on cpus which support it
Message-ID: <20220128234103.GL785175@iweiny-DESK2.sc.intel.com>
References: <20220127175505.851391-1-ira.weiny@intel.com>
 <20220127175505.851391-10-ira.weiny@intel.com>
 <f9db9626-de92-65a4-57f1-cf94511dd137@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f9db9626-de92-65a4-57f1-cf94511dd137@intel.com>
User-Agent: Mutt/1.11.1 (2018-12-01)
Precedence: bulk

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