[PATCH 1/1] x86/mce/amd: Add PPIN support for AMD MCE

[PATCH 1/1] x86/mce/amd: Add PPIN support for AMD MCE

[Wei Huang]

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

RE: [PATCH 1/1] x86/mce/amd: Add PPIN support for AMD MCE

[Luck, Tony]

+		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

Re: [PATCH 1/1] x86/mce/amd: Add PPIN support for AMD MCE

[Borislav Petkov]

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

Re: [PATCH 1/1] x86/mce/amd: Add PPIN support for AMD MCE

[Wei Huang]

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.
> 

Re: [PATCH 1/1] x86/mce/amd: Add PPIN support for AMD MCE

[Wei Huang]

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.
> 

Re: [PATCH 1/1] x86/mce/amd: Add PPIN support for AMD MCE

[Borislav Petkov]

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

Re: [PATCH 1/1] x86/mce/amd: Add PPIN support for AMD MCE

[Huang2, Wei]

[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

Re: [PATCH 1/1] x86/mce/amd: Add PPIN support for AMD MCE

[Borislav Petkov]

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

Re: [PATCH 1/1] x86/mce/amd: Add PPIN support for AMD MCE

[Huang2, Wei]

[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.

Re: [PATCH 1/1] x86/mce/amd: Add PPIN support for AMD MCE

[Borislav Petkov]

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