LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 1/1] x86/mce/amd: Add PPIN support for AMD MCE
@ 2020-03-11  4:44 Wei Huang
  2020-03-11 16:05 ` Luck, Tony
  2020-03-16 18:08 ` Borislav Petkov
  0 siblings, 2 replies; 10+ messages in thread
From: Wei Huang @ 2020-03-11  4:44 UTC (permalink / raw)
  To: linux-kernel
  Cc: tony.luck, bp, yazen.ghannam, tglx, mingo, hpa, linux-edac, x86,
	smita.koralahallichannabasappa

Newer AMD CPUs support the feature of protected processor identification
number (PPIN). This patch detects and enables PPIN support on AMD platforms
and includes PPIN info in MCE records if available. Because this feature is
almost identical on both Intel and AMD, it re-uses X86_FEATURE_INTEL_PPIN
feature bit and renames it to X86_FEATURE_PPIN.

Signed-off-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Smita Koralahalli Channabasappa <smita.koralahallichannabasappa@amd.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Yazen Ghannam <yazen.ghannam@amd.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: linux-edac <linux-edac@vger.kernel.org>
Cc: x86-ml <x86@kernel.org>
---
 arch/x86/include/asm/cpufeatures.h |  2 +-
 arch/x86/kernel/cpu/mce/amd.c      | 23 +++++++++++++++++++++++
 arch/x86/kernel/cpu/mce/core.c     |  8 ++++++--
 arch/x86/kernel/cpu/mce/intel.c    |  2 +-
 4 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index f3327cb56edf..c040ceb44b68 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -203,7 +203,7 @@
 #define X86_FEATURE_PTI			( 7*32+11) /* Kernel Page Table Isolation enabled */
 #define X86_FEATURE_RETPOLINE		( 7*32+12) /* "" Generic Retpoline mitigation for Spectre variant 2 */
 #define X86_FEATURE_RETPOLINE_AMD	( 7*32+13) /* "" AMD Retpoline mitigation for Spectre variant 2 */
-#define X86_FEATURE_INTEL_PPIN		( 7*32+14) /* Intel Processor Inventory Number */
+#define X86_FEATURE_PPIN		( 7*32+14) /* Protected Processor Inventory Number */
 #define X86_FEATURE_CDP_L2		( 7*32+15) /* Code and Data Prioritization L2 */
 #define X86_FEATURE_MSR_SPEC_CTRL	( 7*32+16) /* "" MSR SPEC_CTRL is implemented */
 #define X86_FEATURE_SSBD		( 7*32+17) /* Speculative Store Bypass Disable */
diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 52de616a8065..013c50ba4812 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -624,6 +624,27 @@ static void disable_err_thresholding(struct cpuinfo_x86 *c, unsigned int bank)
 		wrmsrl(MSR_K7_HWCR, hwcr);
 }
 
+static void mce_amd_ppin_init(struct cpuinfo_x86 *c)
+{
+	unsigned long long val;
+
+	if (c->extended_cpuid_level < 0x80000008)
+		return;
+
+	if (cpuid_ebx(0x80000008) & BIT(23)) {
+		if (rdmsrl_safe(MSR_AMD_PPIN_CTL, &val))
+			return;
+
+		if (!(val & 3UL)) {
+			wrmsrl_safe(MSR_AMD_PPIN_CTL,  val | 2UL);
+			rdmsrl_safe(MSR_AMD_PPIN_CTL, &val);
+		}
+
+		if ((val & 3UL) == 2UL)
+			set_cpu_cap(c, X86_FEATURE_PPIN);
+	}
+}
+
 /* cpu init entry point, called from mce.c with preempt off */
 void mce_amd_feature_init(struct cpuinfo_x86 *c)
 {
@@ -659,6 +680,8 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
 
 	if (mce_flags.succor)
 		deferred_error_interrupt_enable(c);
+
+	mce_amd_ppin_init(c);
 }
 
 int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 2c4f949611e4..7aab162c0a00 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -140,8 +140,12 @@ void mce_setup(struct mce *m)
 	m->apicid = cpu_data(m->extcpu).initial_apicid;
 	rdmsrl(MSR_IA32_MCG_CAP, m->mcgcap);
 
-	if (this_cpu_has(X86_FEATURE_INTEL_PPIN))
-		rdmsrl(MSR_PPIN, m->ppin);
+	if (this_cpu_has(X86_FEATURE_PPIN)) {
+		if (m->cpuvendor == X86_VENDOR_INTEL)
+			rdmsrl(MSR_PPIN, m->ppin);
+		else if (m->cpuvendor == X86_VENDOR_AMD)
+			rdmsrl(MSR_AMD_PPIN, m->ppin);
+	}
 
 	m->microcode = boot_cpu_data.microcode;
 }
diff --git a/arch/x86/kernel/cpu/mce/intel.c b/arch/x86/kernel/cpu/mce/intel.c
index 5627b1091b85..424fe1661085 100644
--- a/arch/x86/kernel/cpu/mce/intel.c
+++ b/arch/x86/kernel/cpu/mce/intel.c
@@ -504,7 +504,7 @@ static void intel_ppin_init(struct cpuinfo_x86 *c)
 		}
 
 		if ((val & 3UL) == 2UL)
-			set_cpu_cap(c, X86_FEATURE_INTEL_PPIN);
+			set_cpu_cap(c, X86_FEATURE_PPIN);
 	}
 }
 
-- 
2.24.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH 1/1] x86/mce/amd: Add PPIN support for AMD MCE
  2020-03-11  4:44 [PATCH 1/1] x86/mce/amd: Add PPIN support for AMD MCE Wei Huang
@ 2020-03-11 16:05 ` Luck, Tony
  2020-03-16 18:08 ` Borislav Petkov
  1 sibling, 0 replies; 10+ messages in thread
From: Luck, Tony @ 2020-03-11 16:05 UTC (permalink / raw)
  To: Wei Huang, linux-kernel
  Cc: bp, yazen.ghannam, tglx, mingo, hpa, linux-edac, x86,
	smita.koralahallichannabasappa

+		if ((val & 3UL) == 2UL)
+			set_cpu_cap(c, X86_FEATURE_PPIN);

You may have copied a bug of mine from upstream. We recently found
a system where the BIOS enabled PPIN and set the lock bit.

If that is possible on AMD, then you should just check for enabled at this
point. "if (val & 2UL)"

See this commit in TIP tree:

59b5809655bd ("x86/mce: Fix logic and comments around MSR_PPIN_CTL")

Otherwise looks fine:

Acked-by: Tony Luck <tony.luck@intel.com>

-Tony


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/1] x86/mce/amd: Add PPIN support for AMD MCE
  2020-03-11  4:44 [PATCH 1/1] x86/mce/amd: Add PPIN support for AMD MCE Wei Huang
  2020-03-11 16:05 ` Luck, Tony
@ 2020-03-16 18:08 ` Borislav Petkov
  2020-03-16 19:20   ` Wei Huang
  2020-03-19 20:14   ` Wei Huang
  1 sibling, 2 replies; 10+ messages in thread
From: Borislav Petkov @ 2020-03-16 18:08 UTC (permalink / raw)
  To: Wei Huang
  Cc: linux-kernel, tony.luck, yazen.ghannam, tglx, mingo, hpa,
	linux-edac, x86, smita.koralahallichannabasappa

On Tue, Mar 10, 2020 at 11:44:09PM -0500, Wei Huang wrote:
> Newer AMD CPUs support the feature of protected processor identification
> number (PPIN). This patch detects and enables PPIN support on AMD platforms

Avoid having "This patch" or "This commit" in the commit message. It is
tautologically useless.

Also, do

$ git grep 'This patch' Documentation/process

for more details.

> and includes PPIN info in MCE records if available. Because this feature is
> almost identical on both Intel and AMD, it re-uses X86_FEATURE_INTEL_PPIN
> feature bit and renames it to X86_FEATURE_PPIN.

Strictly speaking, you can't rename it anymore because it is visible in
/proc/cpuinfo and thus ABI.

Besides, we have already a cpufeatures.h leaf for exactly that word:

/* AMD-defined CPU features, CPUID level 0x80000008 (EBX), word 13 */

which means you can call the AMD define

#define X86_FEATURE_AMD_PPIN               ( 13*32+23) /* AMD Protected Processor Inventory Number */

and /proc/cpuinfo will have "amd_ppin" and the code will use the
respective vendor flag to test.

> Signed-off-by: Wei Huang <wei.huang2@amd.com>
> Signed-off-by: Smita Koralahalli Channabasappa <smita.koralahallichannabasappa@amd.com>

What does this SOB mean? Grep Documentation/ for "Co-developed-by" in
case this is what you're trying to express.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/1] x86/mce/amd: Add PPIN support for AMD MCE
  2020-03-16 18:08 ` Borislav Petkov
@ 2020-03-16 19:20   ` Wei Huang
  2020-03-19 20:14   ` Wei Huang
  1 sibling, 0 replies; 10+ messages in thread
From: Wei Huang @ 2020-03-16 19:20 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: linux-kernel, tony.luck, yazen.ghannam, tglx, mingo, hpa,
	linux-edac, x86, smita.koralahallichannabasappa



On 3/16/20 1:08 PM, Borislav Petkov wrote:
> On Tue, Mar 10, 2020 at 11:44:09PM -0500, Wei Huang wrote:
>> Newer AMD CPUs support the feature of protected processor identification
>> number (PPIN). This patch detects and enables PPIN support on AMD platforms
> 
> Avoid having "This patch" or "This commit" in the commit message. It is
> tautologically useless.
> 
> Also, do
> 
> $ git grep 'This patch' Documentation/process
> 
> for more details.
> 
>> and includes PPIN info in MCE records if available. Because this feature is
>> almost identical on both Intel and AMD, it re-uses X86_FEATURE_INTEL_PPIN
>> feature bit and renames it to X86_FEATURE_PPIN.
> 
> Strictly speaking, you can't rename it anymore because it is visible in
> /proc/cpuinfo and thus ABI.
> 
> Besides, we have already a cpufeatures.h leaf for exactly that word:
> 
> /* AMD-defined CPU features, CPUID level 0x80000008 (EBX), word 13 */
> 
> which means you can call the AMD define
> 
> #define X86_FEATURE_AMD_PPIN               ( 13*32+23) /* AMD Protected Processor Inventory Number */
> 
> and /proc/cpuinfo will have "amd_ppin" and the code will use the
> respective vendor flag to test.

Thanks. I will send V2 with fixes based on Tony's and your inputs.

> 
>> Signed-off-by: Wei Huang <wei.huang2@amd.com>
>> Signed-off-by: Smita Koralahalli Channabasappa <smita.koralahallichannabasappa@amd.com>
> 
> What does this SOB mean? Grep Documentation/ for "Co-developed-by" in
> case this is what you're trying to express.
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/1] x86/mce/amd: Add PPIN support for AMD MCE
  2020-03-16 18:08 ` Borislav Petkov
  2020-03-16 19:20   ` Wei Huang
@ 2020-03-19 20:14   ` Wei Huang
  2020-03-19 20:21     ` Borislav Petkov
  1 sibling, 1 reply; 10+ messages in thread
From: Wei Huang @ 2020-03-19 20:14 UTC (permalink / raw)
  To: Borislav Petkov, Wei Huang
  Cc: linux-kernel, tony.luck, yazen.ghannam, tglx, mingo, hpa,
	linux-edac, x86, smita.koralahallichannabasappa



On 3/16/20 1:08 PM, Borislav Petkov wrote:
> On Tue, Mar 10, 2020 at 11:44:09PM -0500, Wei Huang wrote:
>> Newer AMD CPUs support the feature of protected processor identification
>> number (PPIN). This patch detects and enables PPIN support on AMD platforms
> 
> Avoid having "This patch" or "This commit" in the commit message. It is
> tautologically useless.
> 
> Also, do
> 
> $ git grep 'This patch' Documentation/process
> 
> for more details.
> 
>> and includes PPIN info in MCE records if available. Because this feature is
>> almost identical on both Intel and AMD, it re-uses X86_FEATURE_INTEL_PPIN
>> feature bit and renames it to X86_FEATURE_PPIN.
> 
> Strictly speaking, you can't rename it anymore because it is visible in
> /proc/cpuinfo and thus ABI.
> 
> Besides, we have already a cpufeatures.h leaf for exactly that word:
> 
> /* AMD-defined CPU features, CPUID level 0x80000008 (EBX), word 13 */
> 
> which means you can call the AMD define
> 
> #define X86_FEATURE_AMD_PPIN               ( 13*32+23) /* AMD Protected Processor Inventory Number */
> 
> and /proc/cpuinfo will have "amd_ppin" and the code will use the
> respective vendor flag to test.

BTW Intel PPIN doesn't have a respective CPUID feature bit. So it relies
on intel_ppin_init() to turn it on. After that, mce_setup() can use
this_cpu_has(X86_FEATURE_INTEL_PPIN) to read MSR_PPIN. This is fine.

In contrast, AMD has 0x80000008_EBX[23] defined for PPIN feature. When
this CPUID bit is set, X86_FEATURE_AMD_PPIN is ON. But there are cases
where MSR_AMD_PPIN_CTL is locked. Under such circumstance I think
X86_FEATURE_AMD_PPIN should be cleared.

My proposal is to move mce_amd_ppin_init() function to
./arch/x86/kernel/cpu/amd.c and probe X86_FEATURE_AMD_PPIN there. This
is similar to what early_detect_mem_encrypt() does. Later, mce_setup()
can use X86_FEATURE_AMD_PPIN directly. Is this approach acceptable?

> 
>> Signed-off-by: Wei Huang <wei.huang2@amd.com>
>> Signed-off-by: Smita Koralahalli Channabasappa <smita.koralahallichannabasappa@amd.com>
> 
> What does this SOB mean? Grep Documentation/ for "Co-developed-by" in
> case this is what you're trying to express.
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/1] x86/mce/amd: Add PPIN support for AMD MCE
  2020-03-19 20:14   ` Wei Huang
@ 2020-03-19 20:21     ` Borislav Petkov
  2020-03-19 20:58       ` Huang2, Wei
  0 siblings, 1 reply; 10+ messages in thread
From: Borislav Petkov @ 2020-03-19 20:21 UTC (permalink / raw)
  To: Wei Huang
  Cc: Wei Huang, linux-kernel, tony.luck, yazen.ghannam, tglx, mingo,
	hpa, linux-edac, x86, smita.koralahallichannabasappa

On Thu, Mar 19, 2020 at 03:14:53PM -0500, Wei Huang wrote:
> My proposal is to move mce_amd_ppin_init() function to
> ./arch/x86/kernel/cpu/amd.c and probe X86_FEATURE_AMD_PPIN there. This
> is similar to what early_detect_mem_encrypt() does. Later, mce_setup()
> can use X86_FEATURE_AMD_PPIN directly. Is this approach acceptable?

You can add it to arch/x86/kernel/cpu/mce/amd.c just like
intel_ppin_init() is respectively in .../mce/intel.c, as mce/ is where
this thing is used only. We can move it to kernel/cpu/ later if it turns
out that it is needed for something else.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/1] x86/mce/amd: Add PPIN support for AMD MCE
  2020-03-19 20:21     ` Borislav Petkov
@ 2020-03-19 20:58       ` Huang2, Wei
  2020-03-19 21:09         ` Borislav Petkov
  0 siblings, 1 reply; 10+ messages in thread
From: Huang2, Wei @ 2020-03-19 20:58 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: linux-kernel, tony.luck, Ghannam, Yazen, tglx, mingo, hpa,
	linux-edac, x86, Koralahalli Channabasappa, Smita

[AMD Official Use Only - Internal Distribution Only]

From: linux-kernel-owner@vger.kernel.org <linux-kernel-owner@vger.kernel.org> on behalf of Borislav Petkov <bp@alien8.de>

Sent: Thursday, March 19, 2020 3:21 PM

> You can add it to arch/x86/kernel/cpu/mce/amd.c just like
> intel_ppin_init() is respectively in .../mce/intel.c, as mce/ is where
> this thing is used only. We can move it to kernel/cpu/ later if it turns
> out that it is needed for something else.

To use this approach, I have to clear X86_FEATURE_AMD_PPIN if MSR_AMD_PPIN_CTL is locked and disabled.

However, there is a small problem: during boot, mce_setup() is called once before mce_amd_ppin_init() is invoked. As a result, mce_setup() might read X86_FEATURE_AMD_PPIN incorrectly. This concerns me.

Thanks,
-Wei

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/1] x86/mce/amd: Add PPIN support for AMD MCE
  2020-03-19 20:58       ` Huang2, Wei
@ 2020-03-19 21:09         ` Borislav Petkov
  2020-03-19 21:44           ` Huang2, Wei
  0 siblings, 1 reply; 10+ messages in thread
From: Borislav Petkov @ 2020-03-19 21:09 UTC (permalink / raw)
  To: Huang2, Wei
  Cc: linux-kernel, tony.luck, Ghannam, Yazen, tglx, mingo, hpa,
	linux-edac, x86, Koralahalli Channabasappa, Smita

On Thu, Mar 19, 2020 at 08:58:17PM +0000, Huang2, Wei wrote:
> However, there is a small problem: during boot, mce_setup() is called
> once before mce_amd_ppin_init() is invoked.

Which call site is that exactly?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/1] x86/mce/amd: Add PPIN support for AMD MCE
  2020-03-19 21:09         ` Borislav Petkov
@ 2020-03-19 21:44           ` Huang2, Wei
  2020-03-19 21:56             ` Borislav Petkov
  0 siblings, 1 reply; 10+ messages in thread
From: Huang2, Wei @ 2020-03-19 21:44 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: linux-kernel, tony.luck, Ghannam, Yazen, tglx, mingo, hpa,
	linux-edac, x86, Koralahalli Channabasappa, Smita

[AMD Official Use Only - Internal Distribution Only]

>> However, there is a small problem: during boot, mce_setup() is called
>> once before mce_amd_ppin_init() is invoked.
>Which call site is that exactly?

It was from machine_check_poll() ==> mce_gather_info(), right around the invoke of identify_cpu() inside arch/x86/kernel/cpu/common.c file.

-Wei

> --
> Regards/Gruss,
>
> Boris.



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/1] x86/mce/amd: Add PPIN support for AMD MCE
  2020-03-19 21:44           ` Huang2, Wei
@ 2020-03-19 21:56             ` Borislav Petkov
  0 siblings, 0 replies; 10+ messages in thread
From: Borislav Petkov @ 2020-03-19 21:56 UTC (permalink / raw)
  To: Huang2, Wei
  Cc: linux-kernel, tony.luck, Ghannam, Yazen, tglx, mingo, hpa,
	linux-edac, x86, Koralahalli Channabasappa, Smita

On Thu, Mar 19, 2020 at 09:44:48PM +0000, Huang2, Wei wrote:
> It was from machine_check_poll() ==> mce_gather_info(), right around
> the invoke of identify_cpu() inside arch/x86/kernel/cpu/common.c file.

mcheck_cpu_init
|->__mcheck_cpu_init_generic
   |-> machine_check_poll
|->__mcheck_cpu_init_vendor

... and the vendor-specific init in __mcheck_cpu_init_vendor() happens
only *after* it. Oh well.

init_amd() it is then.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-03-19 21:56 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-11  4:44 [PATCH 1/1] x86/mce/amd: Add PPIN support for AMD MCE Wei Huang
2020-03-11 16:05 ` Luck, Tony
2020-03-16 18:08 ` Borislav Petkov
2020-03-16 19:20   ` Wei Huang
2020-03-19 20:14   ` Wei Huang
2020-03-19 20:21     ` Borislav Petkov
2020-03-19 20:58       ` Huang2, Wei
2020-03-19 21:09         ` Borislav Petkov
2020-03-19 21:44           ` Huang2, Wei
2020-03-19 21:56             ` Borislav Petkov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).