Re: [PATCH 0/4] Better memcpy_mcsafe()

Re: [PATCH 0/4] Better memcpy_mcsafe()

[Borislav Petkov]

On Fri, Aug 26, 2016 at 02:08:03PM -0700, Tony Luck wrote:
> 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! :-)))

-- 
Regards/Gruss,
    Boris.

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

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

[Borislav Petkov]

On Fri, Aug 26, 2016 at 02:08:03PM -0700, Tony Luck wrote:
> 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 <tony.luck@intel.com>
> ---
>  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(+)
> 
> diff --git a/arch/x86/include/asm/string_64.h b/arch/x86/include/asm/string_64.h
> index 90dbbd9666d4..877a1dfbf770 100644
> --- a/arch/x86/include/asm/string_64.h
> +++ b/arch/x86/include/asm/string_64.h
> @@ -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
>   *
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index 79d8ec849468..c192fabc3d76 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -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();
>  
>  	/*
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 37ff0158e45f..c88191074879 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c

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

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

> @@ -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
> -- 
> 2.5.0
> 

-- 
Regards/Gruss,
    Boris.

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

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

[Linus Torvalds]

On Fri, Aug 26, 2016 at 10:26 PM, Borislav Petkov <bp@alien8.de> wrote:
>
> 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

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

[Luck, 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 <tony.luck@intel.com>
---

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(+)

diff --git a/arch/x86/include/asm/string_64.h b/arch/x86/include/asm/string_64.h
index 90dbbd9666d4..877a1dfbf770 100644
--- a/arch/x86/include/asm/string_64.h
+++ b/arch/x86/include/asm/string_64.h
@@ -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
  *
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 79d8ec849468..c192fabc3d76 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -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();
 
 	/*
diff --git a/arch/x86/kernel/quirks.c b/arch/x86/kernel/quirks.c
index cc457ff818ad..51402a7e4ca6 100644
--- a/arch/x86/kernel/quirks.c
+++ b/arch/x86/kernel/quirks.c
@@ -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
-- 
2.5.0

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

[Borislav Petkov]

On Tue, Aug 30, 2016 at 11:53:33AM -0700, Luck, Tony wrote:
> 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 <tony.luck@intel.com>
> ---

...

> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index 79d8ec849468..c192fabc3d76 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -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)

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

Btw, it needs documenting over mcheck_enable().

-- 
Regards/Gruss,
    Boris.

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

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

[Luck, Tony]

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

> Btw, it needs documenting over mcheck_enable().

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

-Tony

Re: [PATCH 0/4] Better memcpy_mcsafe()

[Ingo Molnar]

* Tony Luck <tony.luck@intel.com> wrote:

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