Re: [tip:x86/mm] x86/tme: Detect if TME and MKTME is activated by BIOS

[Kai Huang]

On Tue, 2018-03-13 at 15:49 +0300, Kirill A. Shutemov wrote:
> On Tue, Mar 13, 2018 at 03:12:02PM +1300, Kai Huang wrote:
> > It seems setup_pku() will call get_cpu_cap to restore c-
> > >x86_phys_bits
> > later? In which case I think you need to change setup_pku as well.
> 
> Thanks for catching this.
> 
> I think setup_pku() shouldn't call get_cpu_cap().
> 
> Any objections against this:
> 
> diff --git a/arch/x86/kernel/cpu/common.c
> b/arch/x86/kernel/cpu/common.c
> index 348cf4821240..ce10d8ae4cd6 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -362,6 +362,8 @@ static bool pku_disabled;
> 
>  static __always_inline void setup_pku(struct cpuinfo_x86 *c)
>  {
> +   u32 eax, ebx, ecx, edx;
> +
> /* check the boot processor, plus compile options for PKU: */
> if (!cpu_feature_enabled(X86_FEATURE_PKU))
> return;
> @@ -377,7 +379,8 @@ static __always_inline void setup_pku(struct
> cpuinfo_x86 *c)
>  * cpuid bit to be set.  We need to ensure that we
>  * update that bit in this CPU's "cpu_info".
>  */
> -   get_cpu_cap(c);
> +   cpuid_count(0x0007, 0, , , , );
> +   c->x86_capability[CPUID_7_ECX] = ecx;
>  }
> 
>  #ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS
> 
> > And for the comments here, I think it can be refined. It is true
> > that
> > VM guest needs to know bits of physical address, but this info is
> > not
> > used only by VM. I think the reason we need to update is this is
> > simply
> > the fact.
> 
> Fair enough. Like this?

Yes good to me. Thanks.

Thanks,
-Kai
> 
> diff --git a/arch/x86/kernel/cpu/intel.c
> b/arch/x86/kernel/cpu/intel.c
> index e8ddc6dcfd53..ac45ba7398d9 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -612,11 +612,8 @@ static void detect_tme(struct cpuinfo_x86 *c)
>  #endif
> 
> /*
> -* Exclude KeyID bits from physical address bits.
> -*
> -* We have to do this even if we are not going to use KeyID
> bits
> -* ourself. VM guests still have to know that these bits are
> not usable
> -* for physical address.
> +* KeyID bits effectively lower number of physical address
> bits.
> +* Let's update cpuinfo_x86::x86_phys_bits to reflect the
> fact.
>  */
> c->x86_phys_bits -= keyid_bits;
>  }

Re: [tip:x86/mm] x86/tme: Detect if TME and MKTME is activated by BIOS

[Kai Huang]

On Tue, 2018-03-13 at 15:49 +0300, Kirill A. Shutemov wrote:
> On Tue, Mar 13, 2018 at 03:12:02PM +1300, Kai Huang wrote:
> > It seems setup_pku() will call get_cpu_cap to restore c-
> > >x86_phys_bits
> > later? In which case I think you need to change setup_pku as well.
> 
> Thanks for catching this.
> 
> I think setup_pku() shouldn't call get_cpu_cap().
> 
> Any objections against this:
> 
> diff --git a/arch/x86/kernel/cpu/common.c
> b/arch/x86/kernel/cpu/common.c
> index 348cf4821240..ce10d8ae4cd6 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -362,6 +362,8 @@ static bool pku_disabled;
> 
>  static __always_inline void setup_pku(struct cpuinfo_x86 *c)
>  {
> +   u32 eax, ebx, ecx, edx;
> +
> /* check the boot processor, plus compile options for PKU: */
> if (!cpu_feature_enabled(X86_FEATURE_PKU))
> return;
> @@ -377,7 +379,8 @@ static __always_inline void setup_pku(struct
> cpuinfo_x86 *c)
>  * cpuid bit to be set.  We need to ensure that we
>  * update that bit in this CPU's "cpu_info".
>  */
> -   get_cpu_cap(c);
> +   cpuid_count(0x0007, 0, , , , );
> +   c->x86_capability[CPUID_7_ECX] = ecx;
>  }
> 
>  #ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS
> 
> > And for the comments here, I think it can be refined. It is true
> > that
> > VM guest needs to know bits of physical address, but this info is
> > not
> > used only by VM. I think the reason we need to update is this is
> > simply
> > the fact.
> 
> Fair enough. Like this?

Yes good to me. Thanks.

Thanks,
-Kai
> 
> diff --git a/arch/x86/kernel/cpu/intel.c
> b/arch/x86/kernel/cpu/intel.c
> index e8ddc6dcfd53..ac45ba7398d9 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -612,11 +612,8 @@ static void detect_tme(struct cpuinfo_x86 *c)
>  #endif
> 
> /*
> -* Exclude KeyID bits from physical address bits.
> -*
> -* We have to do this even if we are not going to use KeyID
> bits
> -* ourself. VM guests still have to know that these bits are
> not usable
> -* for physical address.
> +* KeyID bits effectively lower number of physical address
> bits.
> +* Let's update cpuinfo_x86::x86_phys_bits to reflect the
> fact.
>  */
> c->x86_phys_bits -= keyid_bits;
>  }

Re: [tip:x86/mm] x86/tme: Detect if TME and MKTME is activated by BIOS

[Dave Hansen]

On 03/13/2018 05:49 AM, Kirill A. Shutemov wrote:
> On Tue, Mar 13, 2018 at 03:12:02PM +1300, Kai Huang wrote:
>> It seems setup_pku() will call get_cpu_cap to restore c->x86_phys_bits
>> later? In which case I think you need to change setup_pku as well.
> Thanks for catching this.
> 
> I think setup_pku() shouldn't call get_cpu_cap().

I think if you want to make it illegal to call get_cpu_cap() twice, you
should enforce that.

Re: [tip:x86/mm] x86/tme: Detect if TME and MKTME is activated by BIOS

[Dave Hansen]

On 03/13/2018 05:49 AM, Kirill A. Shutemov wrote:
> On Tue, Mar 13, 2018 at 03:12:02PM +1300, Kai Huang wrote:
>> It seems setup_pku() will call get_cpu_cap to restore c->x86_phys_bits
>> later? In which case I think you need to change setup_pku as well.
> Thanks for catching this.
> 
> I think setup_pku() shouldn't call get_cpu_cap().

I think if you want to make it illegal to call get_cpu_cap() twice, you
should enforce that.

Re: [tip:x86/mm] x86/tme: Detect if TME and MKTME is activated by BIOS

[Kirill A. Shutemov]

On Tue, Mar 13, 2018 at 03:12:02PM +1300, Kai Huang wrote:
> It seems setup_pku() will call get_cpu_cap to restore c->x86_phys_bits
> later? In which case I think you need to change setup_pku as well.

Thanks for catching this.

I think setup_pku() shouldn't call get_cpu_cap().

Any objections against this:

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 348cf4821240..ce10d8ae4cd6 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -362,6 +362,8 @@ static bool pku_disabled;

 static __always_inline void setup_pku(struct cpuinfo_x86 *c)
 {
+   u32 eax, ebx, ecx, edx;
+
/* check the boot processor, plus compile options for PKU: */
if (!cpu_feature_enabled(X86_FEATURE_PKU))
return;
@@ -377,7 +379,8 @@ static __always_inline void setup_pku(struct cpuinfo_x86 *c)
 * cpuid bit to be set.  We need to ensure that we
 * update that bit in this CPU's "cpu_info".
 */
-   get_cpu_cap(c);
+   cpuid_count(0x0007, 0, , , , );
+   c->x86_capability[CPUID_7_ECX] = ecx;
 }

 #ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS

> And for the comments here, I think it can be refined. It is true that
> VM guest needs to know bits of physical address, but this info is not
> used only by VM. I think the reason we need to update is this is simply
> the fact.

Fair enough. Like this?

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index e8ddc6dcfd53..ac45ba7398d9 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -612,11 +612,8 @@ static void detect_tme(struct cpuinfo_x86 *c)
 #endif

/*
-* Exclude KeyID bits from physical address bits.
-*
-* We have to do this even if we are not going to use KeyID bits
-* ourself. VM guests still have to know that these bits are not usable
-* for physical address.
+* KeyID bits effectively lower number of physical address bits.
+* Let's update cpuinfo_x86::x86_phys_bits to reflect the fact.
 */
c->x86_phys_bits -= keyid_bits;
 }
-- 
 Kirill A. Shutemov

Re: [tip:x86/mm] x86/tme: Detect if TME and MKTME is activated by BIOS

[Kirill A. Shutemov]

On Tue, Mar 13, 2018 at 03:12:02PM +1300, Kai Huang wrote:
> It seems setup_pku() will call get_cpu_cap to restore c->x86_phys_bits
> later? In which case I think you need to change setup_pku as well.

Thanks for catching this.

I think setup_pku() shouldn't call get_cpu_cap().

Any objections against this:

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 348cf4821240..ce10d8ae4cd6 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -362,6 +362,8 @@ static bool pku_disabled;

 static __always_inline void setup_pku(struct cpuinfo_x86 *c)
 {
+   u32 eax, ebx, ecx, edx;
+
/* check the boot processor, plus compile options for PKU: */
if (!cpu_feature_enabled(X86_FEATURE_PKU))
return;
@@ -377,7 +379,8 @@ static __always_inline void setup_pku(struct cpuinfo_x86 *c)
 * cpuid bit to be set.  We need to ensure that we
 * update that bit in this CPU's "cpu_info".
 */
-   get_cpu_cap(c);
+   cpuid_count(0x0007, 0, , , , );
+   c->x86_capability[CPUID_7_ECX] = ecx;
 }

 #ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS

> And for the comments here, I think it can be refined. It is true that
> VM guest needs to know bits of physical address, but this info is not
> used only by VM. I think the reason we need to update is this is simply
> the fact.

Fair enough. Like this?

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index e8ddc6dcfd53..ac45ba7398d9 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -612,11 +612,8 @@ static void detect_tme(struct cpuinfo_x86 *c)
 #endif

/*
-* Exclude KeyID bits from physical address bits.
-*
-* We have to do this even if we are not going to use KeyID bits
-* ourself. VM guests still have to know that these bits are not usable
-* for physical address.
+* KeyID bits effectively lower number of physical address bits.
+* Let's update cpuinfo_x86::x86_phys_bits to reflect the fact.
 */
c->x86_phys_bits -= keyid_bits;
 }
-- 
 Kirill A. Shutemov

Re: [tip:x86/mm] x86/tme: Detect if TME and MKTME is activated by BIOS

[Kai Huang]

On Mon, 2018-03-12 at 05:21 -0700, tip-bot for Kirill A. Shutemov
wrote:
> Commit-ID:  cb06d8e3d020c30fe10ae711c925a5319ab82c88
> Gitweb: https://git.kernel.org/tip/cb06d8e3d020c30fe10ae711c925a5
> 319ab82c88
> Author: Kirill A. Shutemov 
> AuthorDate: Mon, 5 Mar 2018 19:25:50 +0300
> Committer:  Ingo Molnar 
> CommitDate: Mon, 12 Mar 2018 12:10:54 +0100
> 
> x86/tme: Detect if TME and MKTME is activated by BIOS
> 
> IA32_TME_ACTIVATE MSR (0x982) can be used to check if BIOS has
> enabled
> TME and MKTME. It includes which encryption policy/algorithm is
> selected
> for TME or available for MKTME. For MKTME, the MSR also enumerates
> how
> many KeyIDs are available.
> 
> We would need to exclude KeyID bits from physical address bits.
> detect_tme() would adjust cpuinfo_x86::x86_phys_bits accordingly.
> 
> We have to do this even if we are not going to use KeyID bits
> ourself. VM guests still have to know that these bits are not usable
> for physical address.
> 
> Signed-off-by: Kirill A. Shutemov 
> Cc: Dave Hansen 
> Cc: Kai Huang 
> Cc: Linus Torvalds 
> Cc: Peter Zijlstra 
> Cc: Thomas Gleixner 
> Cc: Tom Lendacky 
> Cc: linux...@kvack.org
> Link: http://lkml.kernel.org/r/20180305162610.37510-3-kirill.shutemov
> @linux.intel.com
> Signed-off-by: Ingo Molnar 
> ---
>  arch/x86/kernel/cpu/intel.c | 90
> +
>  1 file changed, 90 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/intel.c
> b/arch/x86/kernel/cpu/intel.c
> index 4aa9fd379390..b862067bb33c 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -510,6 +510,93 @@ static void detect_vmx_virtcap(struct
> cpuinfo_x86 *c)
>   }
>  }
>  
> +#define MSR_IA32_TME_ACTIVATE0x982
> +
> +/* Helpers to access TME_ACTIVATE MSR */
> +#define TME_ACTIVATE_LOCKED(x)   (x & 0x1)
> +#define TME_ACTIVATE_ENABLED(x)  (x & 0x2)
> +
> +#define TME_ACTIVATE_POLICY(x)   ((x >> 4) & 0xf)
> /* Bits 7:4 */
> +#define TME_ACTIVATE_POLICY_AES_XTS_128  0
> +
> +#define TME_ACTIVATE_KEYID_BITS(x)   ((x >> 32) & 0xf)   /
> * Bits 35:32 */
> +
> +#define TME_ACTIVATE_CRYPTO_ALGS(x)  ((x >> 48) & 0x)
> /* Bits 63:48 */
> +#define TME_ACTIVATE_CRYPTO_AES_XTS_128  1
> +
> +/* Values for mktme_status (SW only construct) */
> +#define MKTME_ENABLED0
> +#define MKTME_DISABLED   1
> +#define MKTME_UNINITIALIZED  2
> +static int mktme_status = MKTME_UNINITIALIZED;
> +
> +static void detect_tme(struct cpuinfo_x86 *c)
> +{
> + u64 tme_activate, tme_policy, tme_crypto_algs;
> + int keyid_bits = 0, nr_keyids = 0;
> + static u64 tme_activate_cpu0 = 0;
> +
> + rdmsrl(MSR_IA32_TME_ACTIVATE, tme_activate);
> +
> + if (mktme_status != MKTME_UNINITIALIZED) {
> + if (tme_activate != tme_activate_cpu0) {
> + /* Broken BIOS? */
> + pr_err_once("x86/tme: configuation is
> inconsistent between CPUs\n");
> + pr_err_once("x86/tme: MKTME is not
> usable\n");
> + mktme_status = MKTME_DISABLED;
> +
> + /* Proceed. We may need to exclude bits from
> x86_phys_bits. */
> + }
> + } else {
> + tme_activate_cpu0 = tme_activate;
> + }
> +
> + if (!TME_ACTIVATE_LOCKED(tme_activate) ||
> !TME_ACTIVATE_ENABLED(tme_activate)) {
> + pr_info_once("x86/tme: not enabled by BIOS\n");
> + mktme_status = MKTME_DISABLED;
> + return;
> + }
> +
> + if (mktme_status != MKTME_UNINITIALIZED)
> + goto detect_keyid_bits;
> +
> + pr_info("x86/tme: enabled by BIOS\n");
> +
> + tme_policy = TME_ACTIVATE_POLICY(tme_activate);
> + if (tme_policy != TME_ACTIVATE_POLICY_AES_XTS_128)
> + pr_warn("x86/tme: Unknown policy is active:
> %#llx\n", tme_policy);
> +
> + tme_crypto_algs = TME_ACTIVATE_CRYPTO_ALGS(tme_activate);
> + if (!(tme_crypto_algs & TME_ACTIVATE_CRYPTO_AES_XTS_128)) {
> + pr_err("x86/mktme: No known encryption algorithm is
> supported: %#llx\n",
> + tme_crypto_algs);
> + mktme_status = MKTME_DISABLED;
> + }
> +detect_keyid_bits:
> + keyid_bits = TME_ACTIVATE_KEYID_BITS(tme_activate);
> + nr_keyids = (1UL << keyid_bits) - 1;
> + if (nr_keyids) {
> + pr_info_once("x86/mktme: enabled by BIOS\n");
> + pr_info_once("x86/mktme: %d KeyIDs available\n",
> nr_keyids);
> + } else {
> + pr_info_once("x86/mktme: disabled by BIOS\n");
> + }
> +
> + if (mktme_status == MKTME_UNINITIALIZED) {
> + /* 

Re: [tip:x86/mm] x86/tme: Detect if TME and MKTME is activated by BIOS

[Kai Huang]

On Mon, 2018-03-12 at 05:21 -0700, tip-bot for Kirill A. Shutemov
wrote:
> Commit-ID:  cb06d8e3d020c30fe10ae711c925a5319ab82c88
> Gitweb: https://git.kernel.org/tip/cb06d8e3d020c30fe10ae711c925a5
> 319ab82c88
> Author: Kirill A. Shutemov 
> AuthorDate: Mon, 5 Mar 2018 19:25:50 +0300
> Committer:  Ingo Molnar 
> CommitDate: Mon, 12 Mar 2018 12:10:54 +0100
> 
> x86/tme: Detect if TME and MKTME is activated by BIOS
> 
> IA32_TME_ACTIVATE MSR (0x982) can be used to check if BIOS has
> enabled
> TME and MKTME. It includes which encryption policy/algorithm is
> selected
> for TME or available for MKTME. For MKTME, the MSR also enumerates
> how
> many KeyIDs are available.
> 
> We would need to exclude KeyID bits from physical address bits.
> detect_tme() would adjust cpuinfo_x86::x86_phys_bits accordingly.
> 
> We have to do this even if we are not going to use KeyID bits
> ourself. VM guests still have to know that these bits are not usable
> for physical address.
> 
> Signed-off-by: Kirill A. Shutemov 
> Cc: Dave Hansen 
> Cc: Kai Huang 
> Cc: Linus Torvalds 
> Cc: Peter Zijlstra 
> Cc: Thomas Gleixner 
> Cc: Tom Lendacky 
> Cc: linux...@kvack.org
> Link: http://lkml.kernel.org/r/20180305162610.37510-3-kirill.shutemov
> @linux.intel.com
> Signed-off-by: Ingo Molnar 
> ---
>  arch/x86/kernel/cpu/intel.c | 90
> +
>  1 file changed, 90 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/intel.c
> b/arch/x86/kernel/cpu/intel.c
> index 4aa9fd379390..b862067bb33c 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -510,6 +510,93 @@ static void detect_vmx_virtcap(struct
> cpuinfo_x86 *c)
>   }
>  }
>  
> +#define MSR_IA32_TME_ACTIVATE0x982
> +
> +/* Helpers to access TME_ACTIVATE MSR */
> +#define TME_ACTIVATE_LOCKED(x)   (x & 0x1)
> +#define TME_ACTIVATE_ENABLED(x)  (x & 0x2)
> +
> +#define TME_ACTIVATE_POLICY(x)   ((x >> 4) & 0xf)
> /* Bits 7:4 */
> +#define TME_ACTIVATE_POLICY_AES_XTS_128  0
> +
> +#define TME_ACTIVATE_KEYID_BITS(x)   ((x >> 32) & 0xf)   /
> * Bits 35:32 */
> +
> +#define TME_ACTIVATE_CRYPTO_ALGS(x)  ((x >> 48) & 0x)
> /* Bits 63:48 */
> +#define TME_ACTIVATE_CRYPTO_AES_XTS_128  1
> +
> +/* Values for mktme_status (SW only construct) */
> +#define MKTME_ENABLED0
> +#define MKTME_DISABLED   1
> +#define MKTME_UNINITIALIZED  2
> +static int mktme_status = MKTME_UNINITIALIZED;
> +
> +static void detect_tme(struct cpuinfo_x86 *c)
> +{
> + u64 tme_activate, tme_policy, tme_crypto_algs;
> + int keyid_bits = 0, nr_keyids = 0;
> + static u64 tme_activate_cpu0 = 0;
> +
> + rdmsrl(MSR_IA32_TME_ACTIVATE, tme_activate);
> +
> + if (mktme_status != MKTME_UNINITIALIZED) {
> + if (tme_activate != tme_activate_cpu0) {
> + /* Broken BIOS? */
> + pr_err_once("x86/tme: configuation is
> inconsistent between CPUs\n");
> + pr_err_once("x86/tme: MKTME is not
> usable\n");
> + mktme_status = MKTME_DISABLED;
> +
> + /* Proceed. We may need to exclude bits from
> x86_phys_bits. */
> + }
> + } else {
> + tme_activate_cpu0 = tme_activate;
> + }
> +
> + if (!TME_ACTIVATE_LOCKED(tme_activate) ||
> !TME_ACTIVATE_ENABLED(tme_activate)) {
> + pr_info_once("x86/tme: not enabled by BIOS\n");
> + mktme_status = MKTME_DISABLED;
> + return;
> + }
> +
> + if (mktme_status != MKTME_UNINITIALIZED)
> + goto detect_keyid_bits;
> +
> + pr_info("x86/tme: enabled by BIOS\n");
> +
> + tme_policy = TME_ACTIVATE_POLICY(tme_activate);
> + if (tme_policy != TME_ACTIVATE_POLICY_AES_XTS_128)
> + pr_warn("x86/tme: Unknown policy is active:
> %#llx\n", tme_policy);
> +
> + tme_crypto_algs = TME_ACTIVATE_CRYPTO_ALGS(tme_activate);
> + if (!(tme_crypto_algs & TME_ACTIVATE_CRYPTO_AES_XTS_128)) {
> + pr_err("x86/mktme: No known encryption algorithm is
> supported: %#llx\n",
> + tme_crypto_algs);
> + mktme_status = MKTME_DISABLED;
> + }
> +detect_keyid_bits:
> + keyid_bits = TME_ACTIVATE_KEYID_BITS(tme_activate);
> + nr_keyids = (1UL << keyid_bits) - 1;
> + if (nr_keyids) {
> + pr_info_once("x86/mktme: enabled by BIOS\n");
> + pr_info_once("x86/mktme: %d KeyIDs available\n",
> nr_keyids);
> + } else {
> + pr_info_once("x86/mktme: disabled by BIOS\n");
> + }
> +
> + if (mktme_status == MKTME_UNINITIALIZED) {
> + /* MKTME is usable */
> + mktme_status = MKTME_ENABLED;
> + }
> +
> + /*
> +  * Exclude KeyID bits from physical address bits.
> +  *
> +  * We have to do this even if we are not going to use KeyID
> bits
> +  *