Skip to contentSkip to conversation

Better memcpy_mcsafe()

PATCH In review Borislav Petkov 4 participants 7 messages Aug 27, 2016 – Sep 1, 2016 2 messages not in the archive

Subsystems (2)
x86 PCI
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Borislav Petkov

author maintainer
#1 lore raw
Quoted 6 lines · Tony Luck

The original version of this used a check of the x86_model_id string for the magic "Intel(R) Xeon(R) CPU E7-" to determine whether we are running on a cpu that supports machine check recovery.

Boris tried to talk me out of that, but at the time I didn't think there was a viable alternate option, and somehow he fell for that line.

Never doing that again! :-)))

Signature · 5 lines
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

A message that is not in the archive (sent off-list or not yet received): dc51c61a114c713cb3eb645481f4bfd07a51408e.1472244975.git.tony.luck@intel.com

author maintainer
#3 lore raw

Re: [PATCH 2/4] x86/mce, PCI: Provide quirks to identify Xeon models with machine check recovery

drivers/pci/quirks.c

Quoted 65 lines · Tony Luck

Each Xeon includes a number of capability registers in PCI space that describe some features not enumerated by CPUID.

Use these to determine that we are running on a model that can recover from machine checks. Hooks for Ivybridge ... Skylake provided.

Signed-off-by Tony Luck <[email protected]>
---
 arch/x86/include/asm/string_64.h |  3 +++
 arch/x86/kernel/cpu/mcheck/mce.c |  7 +++++++
 drivers/pci/quirks.c             | 31 +++++++++++++++++++++++++++++++
 3 files changed, 41 insertions(+)
arch/x86/include/asm/string_64.h +3 -0
index 90dbbd9666d4..877a1dfbf770 100644
@@ -2,6 +2,7 @@
 #define _ASM_X86_STRING_64_H
 
 #ifdef __KERNEL__
+#include <linux/jump_label.h>
 
 /* Written 2002 by Andi Kleen */
 

@@ -78,6 +79,8 @@ int strcmp(const char *cs, const char *ct);
 #define memset(s, c, n) __memset(s, c, n)
 #endif
 
+DECLARE_STATIC_KEY_FALSE(mcsafe_key);
+
 /**
  * memcpy_mcsafe - copy memory with indication if a machine check happened
  *
arch/x86/kernel/cpu/mcheck/mce.c +7 -0
index 79d8ec849468..c192fabc3d76 100644
@@ -41,6 +41,7 @@
 #include <linux/debugfs.h>
 #include <linux/irq_work.h>
 #include <linux/export.h>
+#include <linux/jump_label.h>
 
 #include <asm/processor.h>
 #include <asm/traps.h>

@@ -2676,8 +2677,14 @@ static int __init mcheck_debugfs_init(void)
 static int __init mcheck_debugfs_init(void) { return -EINVAL; }
 #endif
 
+DEFINE_STATIC_KEY_FALSE(mcsafe_key);
+EXPORT_SYMBOL_GPL(mcsafe_key);
+
 static int __init mcheck_late_init(void)
 {
+	if (mca_cfg.recovery)
+		static_branch_inc(&mcsafe_key);
+
 	mcheck_debugfs_init();
 
 	/*
drivers/pci/quirks.c +0 -0
index 37ff0158e45f..c88191074879 100644

Shouldn't all that stuff below be in arch/x86/?

arch/x86/pci/fixup.c maybe, for example?

Quoted 38 lines
drivers/pci/quirks.c +31 -0
@@ -4428,3 +4428,34 @@ static void quirk_intel_qat_vf_cap(struct pci_dev *pdev)
 	}
 }
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x443, quirk_intel_qat_vf_cap);
+
+#if defined(CONFIG_X86_64) && defined(CONFIG_X86_MCE)
+#include <linux/jump_label.h>
+#include <asm/string_64.h>
+
+/* Ivy Bridge, Haswell, Broadwell */
+static void quirk_intel_brickland_xeon_ras_cap(struct pci_dev *pdev)
+{
+	u32 capid0;
+
+	pci_read_config_dword(pdev, 0x84, &capid0);
+
+	if (capid0 & 0x10)
+		static_branch_inc(&mcsafe_key);
+}
+
+/* Skylake */
+static void quirk_intel_purley_xeon_ras_cap(struct pci_dev *pdev)
+{
+	u32 capid0;
+
+	pci_read_config_dword(pdev, 0x84, &capid0);
+
+	if ((capid0 & 0xc0) == 0xc0)
+		static_branch_inc(&mcsafe_key);
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x0ec3, quirk_intel_brickland_xeon_ras_cap);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2fc0, quirk_intel_brickland_xeon_ras_cap);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fc0, quirk_intel_brickland_xeon_ras_cap);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2083, quirk_intel_purley_xeon_ras_cap);
+#endif
Signature · 1 line
2.5.0
Signature · 5 lines
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
#4 lore raw

Re: [PATCH 2/4] x86/mce, PCI: Provide quirks to identify Xeon models with machine check recovery

Quoted 4 lines · Borislav Petkov

Shouldn't all that stuff below be in arch/x86/?

arch/x86/pci/fixup.c maybe, for example?

Good catch.

As it's under a CONFIG_X86_64 test anyway, it looks to make a ton more sense in the x86 pci fixup code.

Linus

#5 lore raw

PATCH v2 2/4 x86/mce, PCI: Provide quirks to identify Xeon models with machine check recovery

Each Xeon includes a number of capability registers in PCI space that describe some features not enumerated by CPUID.

Use these to determine that we are running on a model that can recover from machine checks. Hooks for Ivybridge ... Skylake provided.

Signed-off-by Tony Luck <[email protected]>

---

V2:
Boris & Linus: move quirks from generic code to arch/x86
Ingo: Use arch/x86/kernel/early-quirks.c? Can't do that. It only looks at
bridge devices on top level bus. The devices I need are deeper in the hierarchy on per-socket buses.
No changes to parts 1, 3, 4.

 arch/x86/include/asm/string_64.h |  3 +++
 arch/x86/kernel/cpu/mcheck/mce.c |  7 +++++++
 arch/x86/kernel/quirks.c         | 31 +++++++++++++++++++++++++++++++
 3 files changed, 41 insertions(+)
arch/x86/include/asm/string_64.h +3 -0
index 90dbbd9666d4..877a1dfbf770 100644
@@ -2,6 +2,7 @@
 #define _ASM_X86_STRING_64_H
 
 #ifdef __KERNEL__
+#include <linux/jump_label.h>
 
 /* Written 2002 by Andi Kleen */
 

@@ -78,6 +79,8 @@ int strcmp(const char *cs, const char *ct);
 #define memset(s, c, n) __memset(s, c, n)
 #endif
 
+DECLARE_STATIC_KEY_FALSE(mcsafe_key);
+
 /**
  * memcpy_mcsafe - copy memory with indication if a machine check happened
  *
arch/x86/kernel/cpu/mcheck/mce.c +7 -0
index 79d8ec849468..c192fabc3d76 100644
@@ -41,6 +41,7 @@
 #include <linux/debugfs.h>
 #include <linux/irq_work.h>
 #include <linux/export.h>
+#include <linux/jump_label.h>
 
 #include <asm/processor.h>
 #include <asm/traps.h>

@@ -2676,8 +2677,14 @@ static int __init mcheck_debugfs_init(void)
 static int __init mcheck_debugfs_init(void) { return -EINVAL; }
 #endif
 
+DEFINE_STATIC_KEY_FALSE(mcsafe_key);
+EXPORT_SYMBOL_GPL(mcsafe_key);
+
 static int __init mcheck_late_init(void)
 {
+	if (mca_cfg.recovery)
+		static_branch_inc(&mcsafe_key);
+
 	mcheck_debugfs_init();
 
 	/*
arch/x86/kernel/quirks.c +31 -0
index cc457ff818ad..51402a7e4ca6 100644
@@ -626,3 +626,34 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_16H_NB_F3,
 			amd_disable_seq_and_redirect_scrub);
 
 #endif
+
+#if defined(CONFIG_X86_64) && defined(CONFIG_X86_MCE)
+#include <linux/jump_label.h>
+#include <asm/string_64.h>
+
+/* Ivy Bridge, Haswell, Broadwell */
+static void quirk_intel_brickland_xeon_ras_cap(struct pci_dev *pdev)
+{
+	u32 capid0;
+
+	pci_read_config_dword(pdev, 0x84, &capid0);
+
+	if (capid0 & 0x10)
+		static_branch_inc(&mcsafe_key);
+}
+
+/* Skylake */
+static void quirk_intel_purley_xeon_ras_cap(struct pci_dev *pdev)
+{
+	u32 capid0;
+
+	pci_read_config_dword(pdev, 0x84, &capid0);
+
+	if ((capid0 & 0xc0) == 0xc0)
+		static_branch_inc(&mcsafe_key);
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x0ec3, quirk_intel_brickland_xeon_ras_cap);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2fc0, quirk_intel_brickland_xeon_ras_cap);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fc0, quirk_intel_brickland_xeon_ras_cap);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2083, quirk_intel_purley_xeon_ras_cap);
+#endif
Signature · 1 line
2.5.0
author maintainer
#6 lore raw

Re: [PATCH V2 2/4] x86/mce, PCI: Provide quirks to identify Xeon models with machine check recovery

Quoted 8 lines · Tony

Each Xeon includes a number of capability registers in PCI space that describe some features not enumerated by CPUID.

Use these to determine that we are running on a model that can recover from machine checks. Hooks for Ivybridge ... Skylake provided.

Signed-off-by Tony Luck <[email protected]>

---

...

Quoted 13 lines
arch/x86/kernel/cpu/mcheck/mce.c +1 -0
index 79d8ec849468..c192fabc3d76 100644
@@ -41,6 +41,7 @@
 #include <linux/debugfs.h>
 #include <linux/irq_work.h>
 #include <linux/export.h>
+#include <linux/jump_label.h>
 
 #include <asm/processor.h>
 #include <asm/traps.h>

arch/x86/kernel/cpu/mcheck/mce.c static int __init mcheck_debugfs_init(void) · line 2685

Quoted 9 lines
arch/x86/kernel/cpu/mcheck/mce.c +4 -0
@@ -2676,8 +2677,14 @@ static int __init mcheck_debugfs_init(void)
 static int __init mcheck_debugfs_init(void) { return -EINVAL; }
 #endif
 
+DEFINE_STATIC_KEY_FALSE(mcsafe_key);
+EXPORT_SYMBOL_GPL(mcsafe_key);
+
 static int __init mcheck_late_init(void)
 {
+	if (mca_cfg.recovery)

What are we doing with the recovery bool? You want to keep the cmdline
switch: mce=recovery?

Btw, it needs documenting over mcheck_enable().

Signature · 5 lines
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
#7 lore raw

RE: [PATCH V2 2/4] x86/mce, PCI: Provide quirks to identify Xeon models with machine check recovery

Quoted 2 lines

What are we doing with the recovery bool? You want to keep the cmdline
switch: mce=recovery?

I want to keep if for new systems ... there will be a lag between me getting them, and adding new quirks.

Quoted 1 line

Btw, it needs documenting over mcheck_enable().

Yes - should have been done before. Will spin new version.

-Tony

#8 lore raw
In reply to a message not in the archive
Quoted 7 lines · Tony Luck

The original version of this used a check of the x86_model_id string for the magic "Intel(R) Xeon(R) CPU E7-" to determine whether we are running on a cpu that supports machine check recovery.

Boris tried to talk me out of that, but at the time I didn't think there was a viable alternate option, and somehow he fell for that line.

Quoted 7 lines

It turns out there is a better way, that isn't as painful as I thought it might be. It does help guarantee future employment, as I'll have to add a new quirk for each CPU generation. But the check for "E7" would have eventually failed and required a patch too.

The downside of a quirk is that it runs after the X86_FEATURE patching code. So instead of "static_cpu_has()" we use "static_branch_unlikely(&mcsafe_key)"

So why not move it to the early PCI quirk code in arch/x86/ and get rid of this quirk within a quirk?

Thanks,

Ingo