diff mbox

[PATCHv5,08/19] x86/mm: Introduce variables to store number, shift and mask of KeyIDs

Message ID 20180717112029.42378-9-kirill.shutemov@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kirill A. Shutemov July 17, 2018, 11:20 a.m. UTC
mktme_nr_keyids holds number of KeyIDs available for MKTME, excluding
KeyID zero which used by TME. MKTME KeyIDs start from 1.

mktme_keyid_shift holds shift of KeyID within physical address.

mktme_keyid_mask holds mask to extract KeyID from physical address.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 arch/x86/include/asm/mktme.h | 16 ++++++++++++++++
 arch/x86/kernel/cpu/intel.c  | 12 ++++++++----
 arch/x86/mm/Makefile         |  2 ++
 arch/x86/mm/mktme.c          |  5 +++++
 4 files changed, 31 insertions(+), 4 deletions(-)
 create mode 100644 arch/x86/include/asm/mktme.h
 create mode 100644 arch/x86/mm/mktme.c

Comments

Dave Hansen July 18, 2018, 11:19 p.m. UTC | #1
On 07/17/2018 04:20 AM, Kirill A. Shutemov wrote:
> mktme_nr_keyids holds number of KeyIDs available for MKTME, excluding
> KeyID zero which used by TME. MKTME KeyIDs start from 1.
> 
> mktme_keyid_shift holds shift of KeyID within physical address.

I know what all these words mean, but the combination of them makes no
sense to me.  I still don't know what the variable does after reading this.

Is this the lowest bit in the physical address which is used for the
KeyID?  How many bits you must shift up a KeyID to get to the location
at which it can be masked into the physical address?

> mktme_keyid_mask holds mask to extract KeyID from physical address.

Good descriptions, wrong place.  Please put these in the code.

Also, the grammar constantly needs some work.  "holds mask" needs to be
"holds the mask".

> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  arch/x86/include/asm/mktme.h | 16 ++++++++++++++++
>  arch/x86/kernel/cpu/intel.c  | 12 ++++++++----
>  arch/x86/mm/Makefile         |  2 ++
>  arch/x86/mm/mktme.c          |  5 +++++
>  4 files changed, 31 insertions(+), 4 deletions(-)
>  create mode 100644 arch/x86/include/asm/mktme.h
>  create mode 100644 arch/x86/mm/mktme.c
> 
> diff --git a/arch/x86/include/asm/mktme.h b/arch/x86/include/asm/mktme.h
> new file mode 100644
> index 000000000000..df31876ec48c
> --- /dev/null
> +++ b/arch/x86/include/asm/mktme.h
> @@ -0,0 +1,16 @@
> +#ifndef	_ASM_X86_MKTME_H
> +#define	_ASM_X86_MKTME_H
> +
> +#include <linux/types.h>
> +
> +#ifdef CONFIG_X86_INTEL_MKTME
> +extern phys_addr_t mktme_keyid_mask;
> +extern int mktme_nr_keyids;
> +extern int mktme_keyid_shift;
> +#else
> +#define mktme_keyid_mask	((phys_addr_t)0)
> +#define mktme_nr_keyids		0
> +#define mktme_keyid_shift	0
> +#endif
> +
> +#endif
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index bf2caf9d52dd..efc9e9fc47d4 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -573,6 +573,9 @@ static void detect_tme(struct cpuinfo_x86 *c)
>  
>  #ifdef CONFIG_X86_INTEL_MKTME
>  	if (mktme_status == MKTME_ENABLED && nr_keyids) {
> +		mktme_nr_keyids = nr_keyids;
> +		mktme_keyid_shift = c->x86_phys_bits - keyid_bits;
> +
>  		/*
>  		 * Mask out bits claimed from KeyID from physical address mask.
>  		 *
> @@ -580,10 +583,8 @@ static void detect_tme(struct cpuinfo_x86 *c)
>  		 * and number of bits claimed for KeyID is 6, bits 51:46 of
>  		 * physical address is unusable.
>  		 */
> -		phys_addr_t keyid_mask;
> -
> -		keyid_mask = GENMASK_ULL(c->x86_phys_bits - 1, c->x86_phys_bits - keyid_bits);
> -		physical_mask &= ~keyid_mask;
> +		mktme_keyid_mask = GENMASK_ULL(c->x86_phys_bits - 1, mktme_keyid_shift);
> +		physical_mask &= ~mktme_keyid_mask;

Seems a bit silly that we introduce keyid_mask only to make it global a
few patches later.

>  	} else {
>  		/*
>  		 * Reset __PHYSICAL_MASK.
> @@ -591,6 +592,9 @@ static void detect_tme(struct cpuinfo_x86 *c)
>  		 * between CPUs.
>  		 */
>  		physical_mask = (1ULL << __PHYSICAL_MASK_SHIFT) - 1;
> +		mktme_keyid_mask = 0;
> +		mktme_keyid_shift = 0;
> +		mktme_nr_keyids = 0;
>  	}

Should be unnecessary.  These are zeroed by the compiler.

> diff --git a/arch/x86/mm/Makefile b/arch/x86/mm/Makefile
> index 4b101dd6e52f..4ebee899c363 100644
> --- a/arch/x86/mm/Makefile
> +++ b/arch/x86/mm/Makefile
> @@ -53,3 +53,5 @@ obj-$(CONFIG_PAGE_TABLE_ISOLATION)		+= pti.o
>  obj-$(CONFIG_AMD_MEM_ENCRYPT)	+= mem_encrypt.o
>  obj-$(CONFIG_AMD_MEM_ENCRYPT)	+= mem_encrypt_identity.o
>  obj-$(CONFIG_AMD_MEM_ENCRYPT)	+= mem_encrypt_boot.o
> +
> +obj-$(CONFIG_X86_INTEL_MKTME)	+= mktme.o
> diff --git a/arch/x86/mm/mktme.c b/arch/x86/mm/mktme.c
> new file mode 100644
> index 000000000000..467f1b26c737
> --- /dev/null
> +++ b/arch/x86/mm/mktme.c
> @@ -0,0 +1,5 @@
> +#include <asm/mktme.h>
> +
> +phys_addr_t mktme_keyid_mask;
> +int mktme_nr_keyids;
> +int mktme_keyid_shift;

Descriptions should be here, please, not in the changelog.
Kirill A. Shutemov July 19, 2018, 10:21 a.m. UTC | #2
On Wed, Jul 18, 2018 at 04:19:10PM -0700, Dave Hansen wrote:
> On 07/17/2018 04:20 AM, Kirill A. Shutemov wrote:
> > mktme_nr_keyids holds number of KeyIDs available for MKTME, excluding
> > KeyID zero which used by TME. MKTME KeyIDs start from 1.
> > 
> > mktme_keyid_shift holds shift of KeyID within physical address.
> 
> I know what all these words mean, but the combination of them makes no
> sense to me.  I still don't know what the variable does after reading this.
> 
> Is this the lowest bit in the physical address which is used for the
> KeyID?  How many bits you must shift up a KeyID to get to the location
> at which it can be masked into the physical address?

Right.

I'm not sure what is not clear from the description. It look fine to me.

> > mktme_keyid_mask holds mask to extract KeyID from physical address.
> 
> Good descriptions, wrong place.  Please put these in the code.

Okay.

> Also, the grammar constantly needs some work.  "holds mask" needs to be
> "holds the mask".

Right. Thanks

> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > ---
> >  arch/x86/include/asm/mktme.h | 16 ++++++++++++++++
> >  arch/x86/kernel/cpu/intel.c  | 12 ++++++++----
> >  arch/x86/mm/Makefile         |  2 ++
> >  arch/x86/mm/mktme.c          |  5 +++++
> >  4 files changed, 31 insertions(+), 4 deletions(-)
> >  create mode 100644 arch/x86/include/asm/mktme.h
> >  create mode 100644 arch/x86/mm/mktme.c
> > 
> > diff --git a/arch/x86/include/asm/mktme.h b/arch/x86/include/asm/mktme.h
> > new file mode 100644
> > index 000000000000..df31876ec48c
> > --- /dev/null
> > +++ b/arch/x86/include/asm/mktme.h
> > @@ -0,0 +1,16 @@
> > +#ifndef	_ASM_X86_MKTME_H
> > +#define	_ASM_X86_MKTME_H
> > +
> > +#include <linux/types.h>
> > +
> > +#ifdef CONFIG_X86_INTEL_MKTME
> > +extern phys_addr_t mktme_keyid_mask;
> > +extern int mktme_nr_keyids;
> > +extern int mktme_keyid_shift;
> > +#else
> > +#define mktme_keyid_mask	((phys_addr_t)0)
> > +#define mktme_nr_keyids		0
> > +#define mktme_keyid_shift	0
> > +#endif
> > +
> > +#endif
> > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> > index bf2caf9d52dd..efc9e9fc47d4 100644
> > --- a/arch/x86/kernel/cpu/intel.c
> > +++ b/arch/x86/kernel/cpu/intel.c
> > @@ -573,6 +573,9 @@ static void detect_tme(struct cpuinfo_x86 *c)
> >  
> >  #ifdef CONFIG_X86_INTEL_MKTME
> >  	if (mktme_status == MKTME_ENABLED && nr_keyids) {
> > +		mktme_nr_keyids = nr_keyids;
> > +		mktme_keyid_shift = c->x86_phys_bits - keyid_bits;
> > +
> >  		/*
> >  		 * Mask out bits claimed from KeyID from physical address mask.
> >  		 *
> > @@ -580,10 +583,8 @@ static void detect_tme(struct cpuinfo_x86 *c)
> >  		 * and number of bits claimed for KeyID is 6, bits 51:46 of
> >  		 * physical address is unusable.
> >  		 */
> > -		phys_addr_t keyid_mask;
> > -
> > -		keyid_mask = GENMASK_ULL(c->x86_phys_bits - 1, c->x86_phys_bits - keyid_bits);
> > -		physical_mask &= ~keyid_mask;
> > +		mktme_keyid_mask = GENMASK_ULL(c->x86_phys_bits - 1, mktme_keyid_shift);
> > +		physical_mask &= ~mktme_keyid_mask;
> 
> Seems a bit silly that we introduce keyid_mask only to make it global a
> few patches later.

Is it a big deal?

I found it easier to split changes into logical pieces this way.

> >  	} else {
> >  		/*
> >  		 * Reset __PHYSICAL_MASK.
> > @@ -591,6 +592,9 @@ static void detect_tme(struct cpuinfo_x86 *c)
> >  		 * between CPUs.
> >  		 */
> >  		physical_mask = (1ULL << __PHYSICAL_MASK_SHIFT) - 1;
> > +		mktme_keyid_mask = 0;
> > +		mktme_keyid_shift = 0;
> > +		mktme_nr_keyids = 0;
> >  	}
> 
> Should be unnecessary.  These are zeroed by the compiler.

No. detect_tme() called for each CPU in the system.

> > diff --git a/arch/x86/mm/Makefile b/arch/x86/mm/Makefile
> > index 4b101dd6e52f..4ebee899c363 100644
> > --- a/arch/x86/mm/Makefile
> > +++ b/arch/x86/mm/Makefile
> > @@ -53,3 +53,5 @@ obj-$(CONFIG_PAGE_TABLE_ISOLATION)		+= pti.o
> >  obj-$(CONFIG_AMD_MEM_ENCRYPT)	+= mem_encrypt.o
> >  obj-$(CONFIG_AMD_MEM_ENCRYPT)	+= mem_encrypt_identity.o
> >  obj-$(CONFIG_AMD_MEM_ENCRYPT)	+= mem_encrypt_boot.o
> > +
> > +obj-$(CONFIG_X86_INTEL_MKTME)	+= mktme.o
> > diff --git a/arch/x86/mm/mktme.c b/arch/x86/mm/mktme.c
> > new file mode 100644
> > index 000000000000..467f1b26c737
> > --- /dev/null
> > +++ b/arch/x86/mm/mktme.c
> > @@ -0,0 +1,5 @@
> > +#include <asm/mktme.h>
> > +
> > +phys_addr_t mktme_keyid_mask;
> > +int mktme_nr_keyids;
> > +int mktme_keyid_shift;
> 
> Descriptions should be here, please, not in the changelog.

Okay.
Thomas Gleixner July 19, 2018, 12:37 p.m. UTC | #3
On Thu, 19 Jul 2018, Kirill A. Shutemov wrote:
> On Wed, Jul 18, 2018 at 04:19:10PM -0700, Dave Hansen wrote:
> > >  	} else {
> > >  		/*
> > >  		 * Reset __PHYSICAL_MASK.
> > > @@ -591,6 +592,9 @@ static void detect_tme(struct cpuinfo_x86 *c)
> > >  		 * between CPUs.
> > >  		 */
> > >  		physical_mask = (1ULL << __PHYSICAL_MASK_SHIFT) - 1;
> > > +		mktme_keyid_mask = 0;
> > > +		mktme_keyid_shift = 0;
> > > +		mktme_nr_keyids = 0;
> > >  	}
> > 
> > Should be unnecessary.  These are zeroed by the compiler.
> 
> No. detect_tme() called for each CPU in the system.

And then the variables are cleared out while other CPUs can access them?
How is that supposed to work?

Thanks,

	tglx
Kirill A. Shutemov July 19, 2018, 1:12 p.m. UTC | #4
On Thu, Jul 19, 2018 at 02:37:35PM +0200, Thomas Gleixner wrote:
> On Thu, 19 Jul 2018, Kirill A. Shutemov wrote:
> > On Wed, Jul 18, 2018 at 04:19:10PM -0700, Dave Hansen wrote:
> > > >  	} else {
> > > >  		/*
> > > >  		 * Reset __PHYSICAL_MASK.
> > > > @@ -591,6 +592,9 @@ static void detect_tme(struct cpuinfo_x86 *c)
> > > >  		 * between CPUs.
> > > >  		 */
> > > >  		physical_mask = (1ULL << __PHYSICAL_MASK_SHIFT) - 1;
> > > > +		mktme_keyid_mask = 0;
> > > > +		mktme_keyid_shift = 0;
> > > > +		mktme_nr_keyids = 0;
> > > >  	}
> > > 
> > > Should be unnecessary.  These are zeroed by the compiler.
> > 
> > No. detect_tme() called for each CPU in the system.
> 
> And then the variables are cleared out while other CPUs can access them?
> How is that supposed to work?

This code path only matter in patalogical case: when MKTME configuation is
inconsitent between CPUs. Basically if BIOS screwed things up we disable
MKTME.
Thomas Gleixner July 19, 2018, 1:18 p.m. UTC | #5
On Thu, 19 Jul 2018, Kirill A. Shutemov wrote:
> On Thu, Jul 19, 2018 at 02:37:35PM +0200, Thomas Gleixner wrote:
> > On Thu, 19 Jul 2018, Kirill A. Shutemov wrote:
> > > On Wed, Jul 18, 2018 at 04:19:10PM -0700, Dave Hansen wrote:
> > > > >  	} else {
> > > > >  		/*
> > > > >  		 * Reset __PHYSICAL_MASK.
> > > > > @@ -591,6 +592,9 @@ static void detect_tme(struct cpuinfo_x86 *c)
> > > > >  		 * between CPUs.
> > > > >  		 */
> > > > >  		physical_mask = (1ULL << __PHYSICAL_MASK_SHIFT) - 1;
> > > > > +		mktme_keyid_mask = 0;
> > > > > +		mktme_keyid_shift = 0;
> > > > > +		mktme_nr_keyids = 0;
> > > > >  	}
> > > > 
> > > > Should be unnecessary.  These are zeroed by the compiler.
> > > 
> > > No. detect_tme() called for each CPU in the system.
> > 
> > And then the variables are cleared out while other CPUs can access them?
> > How is that supposed to work?
> 
> This code path only matter in patalogical case: when MKTME configuation is
> inconsitent between CPUs. Basically if BIOS screwed things up we disable
> MKTME.

I still don't see how that's supposed to work.

When the inconsistent CPU is brought up _AFTER_ MKTME is enabled, then how
does clearing the variables help? It does not magically make all the other
stuff go away.

Thanks,

	tglx
Kirill A. Shutemov July 19, 2018, 1:23 p.m. UTC | #6
On Thu, Jul 19, 2018 at 03:18:03PM +0200, Thomas Gleixner wrote:
> On Thu, 19 Jul 2018, Kirill A. Shutemov wrote:
> > On Thu, Jul 19, 2018 at 02:37:35PM +0200, Thomas Gleixner wrote:
> > > On Thu, 19 Jul 2018, Kirill A. Shutemov wrote:
> > > > On Wed, Jul 18, 2018 at 04:19:10PM -0700, Dave Hansen wrote:
> > > > > >  	} else {
> > > > > >  		/*
> > > > > >  		 * Reset __PHYSICAL_MASK.
> > > > > > @@ -591,6 +592,9 @@ static void detect_tme(struct cpuinfo_x86 *c)
> > > > > >  		 * between CPUs.
> > > > > >  		 */
> > > > > >  		physical_mask = (1ULL << __PHYSICAL_MASK_SHIFT) - 1;
> > > > > > +		mktme_keyid_mask = 0;
> > > > > > +		mktme_keyid_shift = 0;
> > > > > > +		mktme_nr_keyids = 0;
> > > > > >  	}
> > > > > 
> > > > > Should be unnecessary.  These are zeroed by the compiler.
> > > > 
> > > > No. detect_tme() called for each CPU in the system.
> > > 
> > > And then the variables are cleared out while other CPUs can access them?
> > > How is that supposed to work?
> > 
> > This code path only matter in patalogical case: when MKTME configuation is
> > inconsitent between CPUs. Basically if BIOS screwed things up we disable
> > MKTME.
> 
> I still don't see how that's supposed to work.
> 
> When the inconsistent CPU is brought up _AFTER_ MKTME is enabled, then how
> does clearing the variables help? It does not magically make all the other
> stuff go away.

We don't actually enable MKTME in kernel. BIOS does. Kernel makes choose
to use it or not. Current design targeted to be used by userspace.
So until init we don't have any other stuff to go away. We can just
pretend that MKTME was never there.
Thomas Gleixner July 19, 2018, 1:40 p.m. UTC | #7
On Thu, 19 Jul 2018, Kirill A. Shutemov wrote:
> On Thu, Jul 19, 2018 at 03:18:03PM +0200, Thomas Gleixner wrote:
> > On Thu, 19 Jul 2018, Kirill A. Shutemov wrote:
> > > On Thu, Jul 19, 2018 at 02:37:35PM +0200, Thomas Gleixner wrote:
> > > > On Thu, 19 Jul 2018, Kirill A. Shutemov wrote:
> > > > > On Wed, Jul 18, 2018 at 04:19:10PM -0700, Dave Hansen wrote:
> > > > > > >  	} else {
> > > > > > >  		/*
> > > > > > >  		 * Reset __PHYSICAL_MASK.
> > > > > > > @@ -591,6 +592,9 @@ static void detect_tme(struct cpuinfo_x86 *c)
> > > > > > >  		 * between CPUs.
> > > > > > >  		 */
> > > > > > >  		physical_mask = (1ULL << __PHYSICAL_MASK_SHIFT) - 1;
> > > > > > > +		mktme_keyid_mask = 0;
> > > > > > > +		mktme_keyid_shift = 0;
> > > > > > > +		mktme_nr_keyids = 0;
> > > > > > >  	}
> > > > > > 
> > > > > > Should be unnecessary.  These are zeroed by the compiler.
> > > > > 
> > > > > No. detect_tme() called for each CPU in the system.
> > > > 
> > > > And then the variables are cleared out while other CPUs can access them?
> > > > How is that supposed to work?
> > > 
> > > This code path only matter in patalogical case: when MKTME configuation is
> > > inconsitent between CPUs. Basically if BIOS screwed things up we disable
> > > MKTME.
> > 
> > I still don't see how that's supposed to work.
> > 
> > When the inconsistent CPU is brought up _AFTER_ MKTME is enabled, then how
> > does clearing the variables help? It does not magically make all the other
> > stuff go away.
> 
> We don't actually enable MKTME in kernel. BIOS does. Kernel makes choose
> to use it or not. Current design targeted to be used by userspace.
> So until init we don't have any other stuff to go away. We can just
> pretend that MKTME was never there.

Hotplug is not guaranteed to happen _BEFORE_ init. Think about physical
hotplug.

Thanks,

	tglx
Dave Hansen July 19, 2018, 2:23 p.m. UTC | #8
On 07/19/2018 03:21 AM, Kirill A. Shutemov wrote:
> On Wed, Jul 18, 2018 at 04:19:10PM -0700, Dave Hansen wrote:
>> On 07/17/2018 04:20 AM, Kirill A. Shutemov wrote:
>>> mktme_nr_keyids holds number of KeyIDs available for MKTME, excluding
>>> KeyID zero which used by TME. MKTME KeyIDs start from 1.
>>>
>>> mktme_keyid_shift holds shift of KeyID within physical address.
>> I know what all these words mean, but the combination of them makes no
>> sense to me.  I still don't know what the variable does after reading this.
>>
>> Is this the lowest bit in the physical address which is used for the
>> KeyID?  How many bits you must shift up a KeyID to get to the location
>> at which it can be masked into the physical address?
> Right.
> 
> I'm not sure what is not clear from the description. It look fine to me.

Well, OK, I guess I can write a better one for you.

"Position in the PTE of the lowest bit of the KeyID"

It's also a name that could use some love (now that I know what it
does).  mktme_keyid_pte_shift would be much better.  Or
mktme_keyid_low_pte_bit.
Kirill A. Shutemov July 20, 2018, 12:34 p.m. UTC | #9
On Thu, Jul 19, 2018 at 03:40:41PM +0200, Thomas Gleixner wrote:
> On Thu, 19 Jul 2018, Kirill A. Shutemov wrote:
> > On Thu, Jul 19, 2018 at 03:18:03PM +0200, Thomas Gleixner wrote:
> > > On Thu, 19 Jul 2018, Kirill A. Shutemov wrote:
> > > > On Thu, Jul 19, 2018 at 02:37:35PM +0200, Thomas Gleixner wrote:
> > > > > On Thu, 19 Jul 2018, Kirill A. Shutemov wrote:
> > > > > > On Wed, Jul 18, 2018 at 04:19:10PM -0700, Dave Hansen wrote:
> > > > > > > >  	} else {
> > > > > > > >  		/*
> > > > > > > >  		 * Reset __PHYSICAL_MASK.
> > > > > > > > @@ -591,6 +592,9 @@ static void detect_tme(struct cpuinfo_x86 *c)
> > > > > > > >  		 * between CPUs.
> > > > > > > >  		 */
> > > > > > > >  		physical_mask = (1ULL << __PHYSICAL_MASK_SHIFT) - 1;
> > > > > > > > +		mktme_keyid_mask = 0;
> > > > > > > > +		mktme_keyid_shift = 0;
> > > > > > > > +		mktme_nr_keyids = 0;
> > > > > > > >  	}
> > > > > > > 
> > > > > > > Should be unnecessary.  These are zeroed by the compiler.
> > > > > > 
> > > > > > No. detect_tme() called for each CPU in the system.
> > > > > 
> > > > > And then the variables are cleared out while other CPUs can access them?
> > > > > How is that supposed to work?
> > > > 
> > > > This code path only matter in patalogical case: when MKTME configuation is
> > > > inconsitent between CPUs. Basically if BIOS screwed things up we disable
> > > > MKTME.
> > > 
> > > I still don't see how that's supposed to work.
> > > 
> > > When the inconsistent CPU is brought up _AFTER_ MKTME is enabled, then how
> > > does clearing the variables help? It does not magically make all the other
> > > stuff go away.
> > 
> > We don't actually enable MKTME in kernel. BIOS does. Kernel makes choose
> > to use it or not. Current design targeted to be used by userspace.
> > So until init we don't have any other stuff to go away. We can just
> > pretend that MKTME was never there.
> 
> Hotplug is not guaranteed to happen _BEFORE_ init. Think about physical
> hotplug.

Ouch. I didn't think about this. :/

In this case I don't see how to handle the situation properly.
Is it okay to WARN() && pray()?
Kirill A. Shutemov July 20, 2018, 12:34 p.m. UTC | #10
On Thu, Jul 19, 2018 at 07:23:27AM -0700, Dave Hansen wrote:
> On 07/19/2018 03:21 AM, Kirill A. Shutemov wrote:
> > On Wed, Jul 18, 2018 at 04:19:10PM -0700, Dave Hansen wrote:
> >> On 07/17/2018 04:20 AM, Kirill A. Shutemov wrote:
> >>> mktme_nr_keyids holds number of KeyIDs available for MKTME, excluding
> >>> KeyID zero which used by TME. MKTME KeyIDs start from 1.
> >>>
> >>> mktme_keyid_shift holds shift of KeyID within physical address.
> >> I know what all these words mean, but the combination of them makes no
> >> sense to me.  I still don't know what the variable does after reading this.
> >>
> >> Is this the lowest bit in the physical address which is used for the
> >> KeyID?  How many bits you must shift up a KeyID to get to the location
> >> at which it can be masked into the physical address?
> > Right.
> > 
> > I'm not sure what is not clear from the description. It look fine to me.
> 
> Well, OK, I guess I can write a better one for you.
> 
> "Position in the PTE of the lowest bit of the KeyID"
> 
> It's also a name that could use some love (now that I know what it
> does).  mktme_keyid_pte_shift would be much better.  Or
> mktme_keyid_low_pte_bit.

Okay, thanks.
Thomas Gleixner July 20, 2018, 1:17 p.m. UTC | #11
On Fri, 20 Jul 2018, Kirill A. Shutemov wrote:
> On Thu, Jul 19, 2018 at 03:40:41PM +0200, Thomas Gleixner wrote:
> > > > I still don't see how that's supposed to work.
> > > > 
> > > > When the inconsistent CPU is brought up _AFTER_ MKTME is enabled, then how
> > > > does clearing the variables help? It does not magically make all the other
> > > > stuff go away.
> > > 
> > > We don't actually enable MKTME in kernel. BIOS does. Kernel makes choose
> > > to use it or not. Current design targeted to be used by userspace.
> > > So until init we don't have any other stuff to go away. We can just
> > > pretend that MKTME was never there.
> > 
> > Hotplug is not guaranteed to happen _BEFORE_ init. Think about physical
> > hotplug.
> 
> Ouch. I didn't think about this. :/
> 
> In this case I don't see how to handle the situation properly.
> Is it okay to WARN() && pray()?

Not really. First of all, you want to do the initial checking on the boot
CPU and then when secondary CPUs are brought up, verify that they have
matching parameters. If they do not, then we should just shut them down
right away before they can touch anything which is TME related and mark
them as 'don't online again'. That needs some extra logic in the hotplug
code, but I already have played with that for different reasons. Stick a
fat comment into that 'not matching' code path for now and I'll give you
the magic for preventing full bringup after polishing it a bit.

Thanks,

	tglx
Kirill A. Shutemov July 20, 2018, 1:40 p.m. UTC | #12
On Fri, Jul 20, 2018 at 03:17:54PM +0200, Thomas Gleixner wrote:
> On Fri, 20 Jul 2018, Kirill A. Shutemov wrote:
> > On Thu, Jul 19, 2018 at 03:40:41PM +0200, Thomas Gleixner wrote:
> > > > > I still don't see how that's supposed to work.
> > > > > 
> > > > > When the inconsistent CPU is brought up _AFTER_ MKTME is enabled, then how
> > > > > does clearing the variables help? It does not magically make all the other
> > > > > stuff go away.
> > > > 
> > > > We don't actually enable MKTME in kernel. BIOS does. Kernel makes choose
> > > > to use it or not. Current design targeted to be used by userspace.
> > > > So until init we don't have any other stuff to go away. We can just
> > > > pretend that MKTME was never there.
> > > 
> > > Hotplug is not guaranteed to happen _BEFORE_ init. Think about physical
> > > hotplug.
> > 
> > Ouch. I didn't think about this. :/
> > 
> > In this case I don't see how to handle the situation properly.
> > Is it okay to WARN() && pray()?
> 
> Not really. First of all, you want to do the initial checking on the boot
> CPU and then when secondary CPUs are brought up, verify that they have
> matching parameters. If they do not, then we should just shut them down
> right away before they can touch anything which is TME related and mark
> them as 'don't online again'. That needs some extra logic in the hotplug
> code, but I already have played with that for different reasons. Stick a
> fat comment into that 'not matching' code path for now and I'll give you
> the magic for preventing full bringup after polishing it a bit.

Got it. Thanks!
Kai Huang July 31, 2018, 12:08 a.m. UTC | #13
On Tue, 2018-07-17 at 14:20 +0300, Kirill A. Shutemov wrote:
> mktme_nr_keyids holds number of KeyIDs available for MKTME, excluding
> KeyID zero which used by TME. MKTME KeyIDs start from 1.
> 
> mktme_keyid_shift holds shift of KeyID within physical address.
> 
> mktme_keyid_mask holds mask to extract KeyID from physical address.

Sorry to bring this up, but AMD SME already introduced sme_me_mask, and
__sme_{set/clr} in linux/mem_encrypt.h, should we try to merge MKTME
and SME to have common variables, and reuse mem_encrypt.h? IMHO
sme_me_mask is sort of equivalent to'keyID=1'. And w/ different names
for SME and MKTME, in other components which want to use memory
encryption (ex, DMA API), we have to have explict code to distinguish
MKTME and SME IMO, which is not good.

Thanks,
-Kai
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  arch/x86/include/asm/mktme.h | 16 ++++++++++++++++
>  arch/x86/kernel/cpu/intel.c  | 12 ++++++++----
>  arch/x86/mm/Makefile         |  2 ++
>  arch/x86/mm/mktme.c          |  5 +++++
>  4 files changed, 31 insertions(+), 4 deletions(-)
>  create mode 100644 arch/x86/include/asm/mktme.h
>  create mode 100644 arch/x86/mm/mktme.c
> 
> diff --git a/arch/x86/include/asm/mktme.h
> b/arch/x86/include/asm/mktme.h
> new file mode 100644
> index 000000000000..df31876ec48c
> --- /dev/null
> +++ b/arch/x86/include/asm/mktme.h
> @@ -0,0 +1,16 @@
> +#ifndef	_ASM_X86_MKTME_H
> +#define	_ASM_X86_MKTME_H
> +
> +#include <linux/types.h>
> +
> +#ifdef CONFIG_X86_INTEL_MKTME
> +extern phys_addr_t mktme_keyid_mask;
> +extern int mktme_nr_keyids;
> +extern int mktme_keyid_shift;
> +#else
> +#define mktme_keyid_mask	((phys_addr_t)0)
> +#define mktme_nr_keyids		0
> +#define mktme_keyid_shift	0
> +#endif
> +
> +#endif
> diff --git a/arch/x86/kernel/cpu/intel.c
> b/arch/x86/kernel/cpu/intel.c
> index bf2caf9d52dd..efc9e9fc47d4 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -573,6 +573,9 @@ static void detect_tme(struct cpuinfo_x86 *c)
>  
>  #ifdef CONFIG_X86_INTEL_MKTME
>  	if (mktme_status == MKTME_ENABLED && nr_keyids) {
> +		mktme_nr_keyids = nr_keyids;
> +		mktme_keyid_shift = c->x86_phys_bits - keyid_bits;
> +
>  		/*
>  		 * Mask out bits claimed from KeyID from physical
> address mask.
>  		 *
> @@ -580,10 +583,8 @@ static void detect_tme(struct cpuinfo_x86 *c)
>  		 * and number of bits claimed for KeyID is 6, bits
> 51:46 of
>  		 * physical address is unusable.
>  		 */
> -		phys_addr_t keyid_mask;
> -
> -		keyid_mask = GENMASK_ULL(c->x86_phys_bits - 1, c-
> >x86_phys_bits - keyid_bits);
> -		physical_mask &= ~keyid_mask;
> +		mktme_keyid_mask = GENMASK_ULL(c->x86_phys_bits - 1,
> mktme_keyid_shift);
> +		physical_mask &= ~mktme_keyid_mask;
>  	} else {
>  		/*
>  		 * Reset __PHYSICAL_MASK.
> @@ -591,6 +592,9 @@ static void detect_tme(struct cpuinfo_x86 *c)
>  		 * between CPUs.
>  		 */
>  		physical_mask = (1ULL << __PHYSICAL_MASK_SHIFT) - 1;
> +		mktme_keyid_mask = 0;
> +		mktme_keyid_shift = 0;
> +		mktme_nr_keyids = 0;
>  	}
>  #endif
>  
> diff --git a/arch/x86/mm/Makefile b/arch/x86/mm/Makefile
> index 4b101dd6e52f..4ebee899c363 100644
> --- a/arch/x86/mm/Makefile
> +++ b/arch/x86/mm/Makefile
> @@ -53,3 +53,5 @@ obj-$(CONFIG_PAGE_TABLE_ISOLATION)		+
> = pti.o
>  obj-$(CONFIG_AMD_MEM_ENCRYPT)	+= mem_encrypt.o
>  obj-$(CONFIG_AMD_MEM_ENCRYPT)	+= mem_encrypt_identity.o
>  obj-$(CONFIG_AMD_MEM_ENCRYPT)	+= mem_encrypt_boot.o
> +
> +obj-$(CONFIG_X86_INTEL_MKTME)	+= mktme.o
> diff --git a/arch/x86/mm/mktme.c b/arch/x86/mm/mktme.c
> new file mode 100644
> index 000000000000..467f1b26c737
> --- /dev/null
> +++ b/arch/x86/mm/mktme.c
> @@ -0,0 +1,5 @@
> +#include <asm/mktme.h>
> +
> +phys_addr_t mktme_keyid_mask;
> +int mktme_nr_keyids;
> +int mktme_keyid_shift;
diff mbox

Patch

diff --git a/arch/x86/include/asm/mktme.h b/arch/x86/include/asm/mktme.h
new file mode 100644
index 000000000000..df31876ec48c
--- /dev/null
+++ b/arch/x86/include/asm/mktme.h
@@ -0,0 +1,16 @@ 
+#ifndef	_ASM_X86_MKTME_H
+#define	_ASM_X86_MKTME_H
+
+#include <linux/types.h>
+
+#ifdef CONFIG_X86_INTEL_MKTME
+extern phys_addr_t mktme_keyid_mask;
+extern int mktme_nr_keyids;
+extern int mktme_keyid_shift;
+#else
+#define mktme_keyid_mask	((phys_addr_t)0)
+#define mktme_nr_keyids		0
+#define mktme_keyid_shift	0
+#endif
+
+#endif
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index bf2caf9d52dd..efc9e9fc47d4 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -573,6 +573,9 @@  static void detect_tme(struct cpuinfo_x86 *c)
 
 #ifdef CONFIG_X86_INTEL_MKTME
 	if (mktme_status == MKTME_ENABLED && nr_keyids) {
+		mktme_nr_keyids = nr_keyids;
+		mktme_keyid_shift = c->x86_phys_bits - keyid_bits;
+
 		/*
 		 * Mask out bits claimed from KeyID from physical address mask.
 		 *
@@ -580,10 +583,8 @@  static void detect_tme(struct cpuinfo_x86 *c)
 		 * and number of bits claimed for KeyID is 6, bits 51:46 of
 		 * physical address is unusable.
 		 */
-		phys_addr_t keyid_mask;
-
-		keyid_mask = GENMASK_ULL(c->x86_phys_bits - 1, c->x86_phys_bits - keyid_bits);
-		physical_mask &= ~keyid_mask;
+		mktme_keyid_mask = GENMASK_ULL(c->x86_phys_bits - 1, mktme_keyid_shift);
+		physical_mask &= ~mktme_keyid_mask;
 	} else {
 		/*
 		 * Reset __PHYSICAL_MASK.
@@ -591,6 +592,9 @@  static void detect_tme(struct cpuinfo_x86 *c)
 		 * between CPUs.
 		 */
 		physical_mask = (1ULL << __PHYSICAL_MASK_SHIFT) - 1;
+		mktme_keyid_mask = 0;
+		mktme_keyid_shift = 0;
+		mktme_nr_keyids = 0;
 	}
 #endif
 
diff --git a/arch/x86/mm/Makefile b/arch/x86/mm/Makefile
index 4b101dd6e52f..4ebee899c363 100644
--- a/arch/x86/mm/Makefile
+++ b/arch/x86/mm/Makefile
@@ -53,3 +53,5 @@  obj-$(CONFIG_PAGE_TABLE_ISOLATION)		+= pti.o
 obj-$(CONFIG_AMD_MEM_ENCRYPT)	+= mem_encrypt.o
 obj-$(CONFIG_AMD_MEM_ENCRYPT)	+= mem_encrypt_identity.o
 obj-$(CONFIG_AMD_MEM_ENCRYPT)	+= mem_encrypt_boot.o
+
+obj-$(CONFIG_X86_INTEL_MKTME)	+= mktme.o
diff --git a/arch/x86/mm/mktme.c b/arch/x86/mm/mktme.c
new file mode 100644
index 000000000000..467f1b26c737
--- /dev/null
+++ b/arch/x86/mm/mktme.c
@@ -0,0 +1,5 @@ 
+#include <asm/mktme.h>
+
+phys_addr_t mktme_keyid_mask;
+int mktme_nr_keyids;
+int mktme_keyid_shift;