[PATCH 0/2] x86/mm: Fix couple MKTME-related issues

[PATCH 0/2] x86/mm: Fix couple MKTME-related issues

[Kirill A. Shutemov]

Kai has pointed to few issues around x86_phys_bits in MKTME changes.

Here is fixes. Please review and consider applying.

Kirill A. Shutemov (2):
  x86/mm: Fix comment in detect_tme() regarding x86_phys_bits
  x86/mm: Do not lose cpuinfo_x86:x86_phys_bits adjustment

 arch/x86/include/asm/processor.h |  1 +
 arch/x86/kernel/cpu/amd.c        |  3 ++-
 arch/x86/kernel/cpu/common.c     | 14 ++++++++++++++
 arch/x86/kernel/cpu/intel.c      |  8 +++-----
 4 files changed, 20 insertions(+), 6 deletions(-)

-- 
2.16.1

[PATCH 1/2] x86/mm: Fix comment in detect_tme() regarding x86_phys_bits

[Kirill A. Shutemov]

As Kai pointed, we adjust x86_phys_bits not only to communicate
available physical address space to virtual machines, but mainly to
reflect the fact that the address space is reduced.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Suggested-by: Kai Huang <kai.huang@linux.intel.com>
---
 arch/x86/kernel/cpu/intel.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index f0481b85c39d..fd379358c58d 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -619,11 +619,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;
 }
-- 
2.16.1

[PATCH 2/2] x86/mm: Do not lose cpuinfo_x86:x86_phys_bits adjustment

[Kirill A. Shutemov]

Some features (Intel MKTME, AMD SME) may reduce number of effectively
available physical address bits. We adjust x86_phys_bits accordingly.

But if get_cpu_cap() got called more than one time we may lose this
information.

That's exactly what happens in setup_pku(): it gets called after
detect_tme() and x86_phys_bits gets overwritten.

Add x86_phys_bits_adj which stores by how many bits we should reduce
x86_phys_bits comparing to what CPUID returns.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Reported-by: Kai Huang <kai.huang@linux.intel.com>
Fixes: cb06d8e3d020 ("x86/tme: Detect if TME and MKTME is activated by BIOS")
---
 arch/x86/include/asm/processor.h |  1 +
 arch/x86/kernel/cpu/amd.c        |  3 ++-
 arch/x86/kernel/cpu/common.c     | 14 ++++++++++++++
 arch/x86/kernel/cpu/intel.c      |  1 +
 4 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index b0ccd4847a58..1250547c8eb7 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -98,6 +98,7 @@ struct cpuinfo_x86 {
 #endif
 	__u8			x86_virt_bits;
 	__u8			x86_phys_bits;
+	__u8			x86_phys_bits_adj;
 	/* CPUID returned core id bits: */
 	__u8			x86_coreid_bits;
 	__u8			cu_id;
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 12bc0a1139da..4d0ec075f99e 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -583,7 +583,8 @@ static void early_detect_mem_encrypt(struct cpuinfo_x86 *c)
 		 * will be a value above 32-bits this is still done for
 		 * CONFIG_X86_32 so that accurate values are reported.
 		 */
-		c->x86_phys_bits -= (cpuid_ebx(0x8000001f) >> 6) & 0x3f;
+		c->x86_phys_bits_adj = (cpuid_ebx(0x8000001f) >> 6) & 0x3f;
+		c->x86_phys_bits -= c->x86_phys_bits_adj;
 
 		if (IS_ENABLED(CONFIG_X86_32))
 			goto clear_all;
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 348cf4821240..d2e3dd827691 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -854,6 +854,20 @@ void get_cpu_cap(struct cpuinfo_x86 *c)
 		c->x86_virt_bits = (eax >> 8) & 0xff;
 		c->x86_phys_bits = eax & 0xff;
 		c->x86_capability[CPUID_8000_0008_EBX] = ebx;
+
+		/*
+		 * Some features (Intel MKTME, AMD SME) may reduce number
+		 * of effectively available physical address bits.
+		 *
+		 * We adjust x86_phys_bits accordingly.
+		 *
+		 * But if get_cpu_cap() got called more than one time we
+		 * may lose this information.
+		 *
+		 * x86_phys_bits_adj stores by how many bits we should
+		 * reduce x86_phys_bits comparing to what CPUID returns.
+		 */
+		c->x86_phys_bits -= c->x86_phys_bits_adj;
 	}
 #ifdef CONFIG_X86_32
 	else if (cpu_has(c, X86_FEATURE_PAE) || cpu_has(c, X86_FEATURE_PSE36))
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index fd379358c58d..801c2e42b87d 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -623,6 +623,7 @@ static void detect_tme(struct cpuinfo_x86 *c)
 	 * Let's update cpuinfo_x86::x86_phys_bits to reflect the fact.
 	 */
 	c->x86_phys_bits -= keyid_bits;
+	c->x86_phys_bits_adj = keyid_bits;
 }
 
 static void init_intel_energy_perf(struct cpuinfo_x86 *c)
-- 
2.16.1

Re: [PATCH 2/2] x86/mm: Do not lose cpuinfo_x86:x86_phys_bits adjustment

[Thomas Gleixner]

On Thu, 15 Mar 2018, Kirill A. Shutemov wrote:

> Some features (Intel MKTME, AMD SME) may reduce number of effectively

May? They fricking reduce the number of bits.

> available physical address bits. We adjust x86_phys_bits accordingly.
> 
> But if get_cpu_cap() got called more than one time we may lose this
> information.

We may? Dammit, I asked you more than once to stop writing fairy
tales. Changelogs are about facts and not about may/could or whatever. And
not WE lose the information, the information gets overwritten by the
subsequent invocation of get_cpu_cap().

> That's exactly what happens in setup_pku(): it gets called after
> detect_tme() and x86_phys_bits gets overwritten.
> 
> Add x86_phys_bits_adj which stores by how many bits we should reduce
> x86_phys_bits comparing to what CPUID returns.

That's just sloppy, really.

The real question is: Why on earth is get_cpu_cap() updating the 0x80000008
leaf information again after the first initialization?

If there is no reason to do so, then this needs to be taken out of
get_cpu_caps().

If there is a reason, then this wants to be explained proper.

This 'add some duct tape' mode has to stop. The cpu feature detection is
messy enough already, there is no need to add more to it unless there is a
real compelling reason.

Thanks,

	tglx

[tip:x86/mm] x86/mm: Update comment in detect_tme() regarding x86_phys_bits

[tip-bot for Kirill A. Shutemov]

Commit-ID:  547edaca247abf910e32f0cd883ba83b8fc6d0ed
Gitweb:     https://git.kernel.org/tip/547edaca247abf910e32f0cd883ba83b8fc6d0ed
Author:     Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
AuthorDate: Thu, 15 Mar 2018 16:49:06 +0300
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 27 Mar 2018 11:49:58 +0200

x86/mm: Update comment in detect_tme() regarding x86_phys_bits

As Kai pointed out, the primary reason for adjusting x86_phys_bits is to
reflect that the the address space is reduced and not the ability to
communicate the available physical address space to virtual machines.

Suggested-by: Kai Huang <kai.huang@linux.intel.com>
Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: linux-mm@kvack.org
Link: https://lkml.kernel.org/r/20180315134907.9311-2-kirill.shutemov@linux.intel.com

---
 arch/x86/kernel/cpu/intel.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 3f8d7a3b6447..6106d11ceb6b 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -587,11 +587,8 @@ detect_keyid_bits:
 	}
 
 	/*
-	 * 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 the number of physical address
+	 * bits.  Update cpuinfo_x86::x86_phys_bits accordingly.
 	 */
 	c->x86_phys_bits -= keyid_bits;
 }

[PATCHv2] x86/mm: Do not lose cpuinfo_x86::x86_phys_bits adjustment

[Kirill A. Shutemov]

Some features (Intel MKTME, AMD SME) reduce number of effectively
available physical address bits. We adjust x86_phys_bits accordingly.

If get_cpu_cap() got called more than one time we lose this adjustement.

That's exactly what happens in setup_pku(): it gets called after
detect_tme() and cpuinfo_x86::x86_phys_bits gets overwritten.

Extract address sizes enumeration into a separate routine and get it
called only from early_identify_cpu() and from generic_identify().

It makes get_cpu_cap() safe to be called later during boot proccess
without risk to overwrite cpuinfo_x86::x86_phys_bits.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Reported-by: Kai Huang <kai.huang@linux.intel.com>
Fixes: cb06d8e3d020 ("x86/tme: Detect if TME and MKTME is activated by BIOS")
---
 arch/x86/kernel/cpu/common.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 348cf4821240..2981bf287ef5 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -848,18 +848,6 @@ void get_cpu_cap(struct cpuinfo_x86 *c)
 		c->x86_power = edx;
 	}
 
-	if (c->extended_cpuid_level >= 0x80000008) {
-		cpuid(0x80000008, &eax, &ebx, &ecx, &edx);
-
-		c->x86_virt_bits = (eax >> 8) & 0xff;
-		c->x86_phys_bits = eax & 0xff;
-		c->x86_capability[CPUID_8000_0008_EBX] = ebx;
-	}
-#ifdef CONFIG_X86_32
-	else if (cpu_has(c, X86_FEATURE_PAE) || cpu_has(c, X86_FEATURE_PSE36))
-		c->x86_phys_bits = 36;
-#endif
-
 	if (c->extended_cpuid_level >= 0x8000000a)
 		c->x86_capability[CPUID_8000_000A_EDX] = cpuid_edx(0x8000000a);
 
@@ -874,6 +862,23 @@ void get_cpu_cap(struct cpuinfo_x86 *c)
 	apply_forced_caps(c);
 }
 
+static void get_cpu_address_sizes(struct cpuinfo_x86 *c)
+{
+	u32 eax, ebx, ecx, edx;
+
+	if (c->extended_cpuid_level >= 0x80000008) {
+		cpuid(0x80000008, &eax, &ebx, &ecx, &edx);
+
+		c->x86_virt_bits = (eax >> 8) & 0xff;
+		c->x86_phys_bits = eax & 0xff;
+		c->x86_capability[CPUID_8000_0008_EBX] = ebx;
+	}
+#ifdef CONFIG_X86_32
+	else if (cpu_has(c, X86_FEATURE_PAE) || cpu_has(c, X86_FEATURE_PSE36))
+		c->x86_phys_bits = 36;
+#endif
+}
+
 static void identify_cpu_without_cpuid(struct cpuinfo_x86 *c)
 {
 #ifdef CONFIG_X86_32
@@ -965,6 +970,7 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 		cpu_detect(c);
 		get_cpu_vendor(c);
 		get_cpu_cap(c);
+		get_cpu_address_sizes(c);
 		setup_force_cpu_cap(X86_FEATURE_CPUID);
 
 		if (this_cpu->c_early_init)
@@ -1097,6 +1103,8 @@ static void generic_identify(struct cpuinfo_x86 *c)
 
 	get_cpu_cap(c);
 
+	get_cpu_address_sizes(c);
+
 	if (c->cpuid_level >= 0x00000001) {
 		c->initial_apicid = (cpuid_ebx(1) >> 24) & 0xFF;
 #ifdef CONFIG_X86_32
-- 
2.16.2