diff mbox series

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

Message ID 20200311044409.2717587-1-wei.huang2@amd.com (mailing list archive)
State New, archived
Headers show
Series [1/1] x86/mce/amd: Add PPIN support for AMD MCE | expand

Commit Message

Wei Huang March 11, 2020, 4:44 a.m. UTC 
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(-)

Comments

Tony Luck March 11, 2020, 4:05 p.m. UTC | #1 
+		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
Borislav Petkov March 16, 2020, 6:08 p.m. UTC | #2 
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.
Wei Huang March 16, 2020, 7:20 p.m. UTC | #3 
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.
>
Wei Huang March 19, 2020, 8:14 p.m. UTC | #4 
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.
>
Borislav Petkov March 19, 2020, 8:21 p.m. UTC | #5 
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.
Wei Huang March 19, 2020, 8:58 p.m. UTC | #6 
[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
Borislav Petkov March 19, 2020, 9:09 p.m. UTC | #7 
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?
Wei Huang March 19, 2020, 9:44 p.m. UTC | #8 
[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.
Borislav Petkov March 19, 2020, 9:56 p.m. UTC | #9 
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.
diff mbox series

Patch

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);
 	}
 }