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@xxxxxxxxxxxxxxx>
> > ---
> > 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.
--
Kirill A. Shutemov
