| Message ID | 20180717112029.42378-9-kirill.shutemov@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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.
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
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.
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
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.
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
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.
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()?
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.
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
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!
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 --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;
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