[PATCH 0/3] Split lock enumeration cleanup and fixes

[PATCH 0/3] Split lock enumeration cleanup and fixes

[Tony Luck]

Need to revisit the sequence of operations when testing for the
split lock detect feature. It is model specific, so we must check
the model number before looking at CORE_CAPABILITIES.

I marked it all for stable ... some tweaking would be needed if
the  X86_MATCH_INTEL stuff from v5.7 doesn't get backported.

Tony Luck (3):
  x86/split_lock: Update to use X86_MATCH_INTEL_FAM6_MODEL()
  x86/split_lock: Bits in IA32_CORE_CAPABILITIES are not architectural
  x86/split_lock: Add Tremont family CPU models

 arch/x86/kernel/cpu/intel.c | 41 +++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 18 deletions(-)

-- 
2.21.1

[PATCH 1/3] x86/split_lock: Update to use X86_MATCH_INTEL_FAM6_MODEL()

[Tony Luck]

The SPLIT_LOCK_CPU() macro escaped the tree-wide sweep for old-style
initialization. Update to use X86_MATCH_INTEL_FAM6_MODEL().

Fixes: 6650cdd9a8cc ("x86/split_lock: Enable split lock detection by kernel")
Cc: <stable@vger.kernel.org>
Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/kernel/cpu/intel.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index bf08d4508ecb..6119deb32660 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -1119,8 +1119,6 @@ void switch_to_sld(unsigned long tifn)
 	sld_update_msr(!(tifn & _TIF_SLD));
 }
 
-#define SPLIT_LOCK_CPU(model) {X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY}
-
 /*
  * The following processors have the split lock detection feature. But
  * since they don't have the IA32_CORE_CAPABILITIES MSR, the feature cannot
@@ -1128,8 +1126,8 @@ void switch_to_sld(unsigned long tifn)
  * processors.
  */
 static const struct x86_cpu_id split_lock_cpu_ids[] __initconst = {
-	SPLIT_LOCK_CPU(INTEL_FAM6_ICELAKE_X),
-	SPLIT_LOCK_CPU(INTEL_FAM6_ICELAKE_L),
+	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X,           0),
+	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_L,           0),
 	{}
 };
 
-- 
2.21.1

[PATCH 2/3] x86/split_lock: Bits in IA32_CORE_CAPABILITIES are not architectural

[Tony Luck]

The Intel Software Developers' Manual erroneously listed bit 5 of the
IA32_CORE_CAPABILITIES register as an architectural feature. It is not.

Features enumerated by IA32_CORE_CAPABILITIES are model specific and
implementation details may vary in different cpu models. Thus it is only
safe to trust features after checking the CPU model.

Icelake client and server models are known to implement the split lock
detect feature even though they don't enumerate IA32_CORE_CAPABILITIES

Fixes: 6650cdd9a8cc ("x86/split_lock: Enable split lock detection by kernel")
Cc: <stable@vger.kernel.org>
Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/kernel/cpu/intel.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 6119deb32660..3b43b2c91054 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -1120,10 +1120,12 @@ void switch_to_sld(unsigned long tifn)
 }
 
 /*
- * The following processors have the split lock detection feature. But
- * since they don't have the IA32_CORE_CAPABILITIES MSR, the feature cannot
- * be enumerated. Enable it by family and model matching on these
- * processors.
+ * Bits in the IA32_CORE_CAPABILITIES are not architectural, so they
+ * should only be trusted if you know you are on a model that implements
+ * them.
+ * The driver_data field is set to zero to indicate CPU models like
+ * Icelake that are known to have the split-lock feature even though
+ * they do not enumerate IA32_CORE_CAPABILITIES.
  */
 static const struct x86_cpu_id split_lock_cpu_ids[] __initconst = {
 	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X,           0),
@@ -1133,19 +1135,21 @@ static const struct x86_cpu_id split_lock_cpu_ids[] __initconst = {
 
 void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c)
 {
-	u64 ia32_core_caps = 0;
+	const struct x86_cpu_id *m;
+	u64 ia32_core_caps;
 
-	if (c->x86_vendor != X86_VENDOR_INTEL)
+	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
 		return;
-	if (cpu_has(c, X86_FEATURE_CORE_CAPABILITIES)) {
-		/* Enumerate features reported in IA32_CORE_CAPABILITIES MSR. */
+
+	m = x86_match_cpu(split_lock_cpu_ids);
+	if (!m)
+		return;
+
+	if (m->driver_data && cpu_has(c, X86_FEATURE_CORE_CAPABILITIES)) {
 		rdmsrl(MSR_IA32_CORE_CAPS, ia32_core_caps);
-	} else if (!boot_cpu_has(X86_FEATURE_HYPERVISOR)) {
-		/* Enumerate split lock detection by family and model. */
-		if (x86_match_cpu(split_lock_cpu_ids))
-			ia32_core_caps |= MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT;
+		if (!(ia32_core_caps & MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT))
+			return;
 	}
 
-	if (ia32_core_caps & MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT)
-		split_lock_setup();
+	split_lock_setup();
 }
-- 
2.21.1

[PATCH 3/3] x86/split_lock: Add Tremont family CPU models

[Tony Luck]

Tremont CPUs support IA32_CORE_CAPABILITIES bits to indicate
whether speciifc SKUs have support for split lock detection.

Fixes: 6650cdd9a8cc ("x86/split_lock: Enable split lock detection by kernel")
Cc: <stable@vger.kernel.org>
Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/kernel/cpu/intel.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 3b43b2c91054..d559481b54a6 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -1130,6 +1130,9 @@ void switch_to_sld(unsigned long tifn)
 static const struct x86_cpu_id split_lock_cpu_ids[] __initconst = {
 	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X,           0),
 	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_L,           0),
+	X86_MATCH_INTEL_FAM6_MODEL(ATOM_TREMONT,	1),
+	X86_MATCH_INTEL_FAM6_MODEL(ATOM_TREMONT_D,	1),
+	X86_MATCH_INTEL_FAM6_MODEL(ATOM_TREMONT_L,	1),
 	{}
 };
 
-- 
2.21.1

Re: [PATCH 0/3] Split lock enumeration cleanup and fixes

[Thomas Gleixner]

Tony Luck <tony.luck@intel.com> writes:
> Need to revisit the sequence of operations when testing for the
> split lock detect feature. It is model specific, so we must check
> the model number before looking at CORE_CAPABILITIES.
>
> I marked it all for stable ... some tweaking would be needed if
> the  X86_MATCH_INTEL stuff from v5.7 doesn't get backported.

Why stable? The split lock stuff got merged post 5.6.

Thanks,

        tglx

RE: [PATCH 0/3] Split lock enumeration cleanup and fixes

[Luck, Tony]

> Why stable? The split lock stuff got merged post 5.6.

I have no sense of time ... in my memory it went in ages ago.  I guess I'm remembering
it making it into TIP.

Drop the stable stuff ... obviously no need.

-Tony

Re: [PATCH 2/3] x86/split_lock: Bits in IA32_CORE_CAPABILITIES are not architectural

[Thomas Gleixner]

Tony Luck <tony.luck@intel.com> writes:
> The Intel Software Developers' Manual erroneously listed bit 5 of the
> IA32_CORE_CAPABILITIES register as an architectural feature. It is
> not.

TBH. I'm really pissed off by that. We ask Intel for the past 20 years
that this non-enumerability and model checking has to stop.

Especially for the split lock festure we got assured that the Icelakes
are the only models which need the cpu match because it was too late to
add the capability bit.

> Features enumerated by IA32_CORE_CAPABILITIES are model specific and
> implementation details may vary in different cpu models. Thus it is only
> safe to trust features after checking the CPU model.

What's the point of the IA32_CORE_CAPABILITIES check if we need a model
match to figure out whether IA32_CORE_CAPABILITIES bit 5 is valid to
enumerate split lock detection?

IOW, are we going to see CPUs which end up in the match list and have
bit 5 cleared in IA32_CORE_CAPABILITIES?

Thanks,

        tglx

Re: [PATCH 2/3] x86/split_lock: Bits in IA32_CORE_CAPABILITIES are not architectural

[Luck, Tony]

On Fri, Apr 17, 2020 at 12:06:47AM +0200, Thomas Gleixner wrote:
> Tony Luck <tony.luck@intel.com> writes:
> > Features enumerated by IA32_CORE_CAPABILITIES are model specific and
> > implementation details may vary in different cpu models. Thus it is only
> > safe to trust features after checking the CPU model.
> 
> What's the point of the IA32_CORE_CAPABILITIES check if we need a model
> match to figure out whether IA32_CORE_CAPABILITIES bit 5 is valid to
> enumerate split lock detection?
> 
> IOW, are we going to see CPUs which end up in the match list and have
> bit 5 cleared in IA32_CORE_CAPABILITIES?

There may be low-end SKUs of a model that don't have all the features of
the high-end SKUs. So yes, you may find that a specific SKU of a model
on the list for a feature doesn't have the feature.

A model specific feature may also have implementation differences
on different models.  E.g. if Intel were to produce a model that
did split lock "right" (with thread-scoped control). That would
still use the same bit in IA32_CORE_CAPABILITIES, but the OS would
need model specific knowledge to know that this split lock detect
worked differently from another model that has split lock detect.

-Tony

Re: [PATCH 2/3] x86/split_lock: Bits in IA32_CORE_CAPABILITIES are not architectural

[Thomas Gleixner]

Tony,

"Luck, Tony" <tony.luck@intel.com> writes:
> On Fri, Apr 17, 2020 at 12:06:47AM +0200, Thomas Gleixner wrote:
>> Tony Luck <tony.luck@intel.com> writes:
>> > Features enumerated by IA32_CORE_CAPABILITIES are model specific and
>> > implementation details may vary in different cpu models. Thus it is only
>> > safe to trust features after checking the CPU model.
>> 
>> What's the point of the IA32_CORE_CAPABILITIES check if we need a model
>> match to figure out whether IA32_CORE_CAPABILITIES bit 5 is valid to
>> enumerate split lock detection?
>> 
>> IOW, are we going to see CPUs which end up in the match list and have
>> bit 5 cleared in IA32_CORE_CAPABILITIES?
>
> There may be low-end SKUs of a model that don't have all the features of
> the high-end SKUs. So yes, you may find that a specific SKU of a model
> on the list for a feature doesn't have the feature.
>
> A model specific feature may also have implementation differences
> on different models.  E.g. if Intel were to produce a model that
> did split lock "right" (with thread-scoped control). That would
> still use the same bit in IA32_CORE_CAPABILITIES, but the OS would
> need model specific knowledge to know that this split lock detect
> worked differently from another model that has split lock detect.

IA32_CORE_CAPABILITIES makes a lot of sense to enumerate per thread
functionality because common sense and consistency are overrated.

Can Intel's HW folks please stop their approach of 'let software
deal with the mess we create' once and forever?

Thanks,

        tglx

Re: [PATCH 2/3] x86/split_lock: Bits in IA32_CORE_CAPABILITIES are not architectural

[Thomas Gleixner]

Tony Luck <tony.luck@intel.com> writes:
> +	m = x86_match_cpu(split_lock_cpu_ids);
> +	if (!m)
> +		return;
> +
> +	if (m->driver_data && cpu_has(c, X86_FEATURE_CORE_CAPABILITIES))
> {

This condition results in the following:

    driver_data     MSR_CORE_CAPS	 MSR_CORE_CAPS_SLD	SLD available

1       0             Don't care          Don't care               Y
2       1                N                Don't care               Y
3       1                Y                    Y                    Y
4       1                Y                    N                    N

#2 does not make any sense to me.

Thanks,

        tglx

[tip: x86/urgent] x86/split_lock: Update to use X86_MATCH_INTEL_FAM6_MODEL()

[tip-bot2 for Tony Luck]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     3ab0762d1edfda6ccbc08f636acab42c103c299f
Gitweb:        https://git.kernel.org/tip/3ab0762d1edfda6ccbc08f636acab42c103c299f
Author:        Tony Luck <tony.luck@intel.com>
AuthorDate:    Thu, 16 Apr 2020 13:57:52 -07:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Fri, 17 Apr 2020 12:14:12 +02:00

x86/split_lock: Update to use X86_MATCH_INTEL_FAM6_MODEL()

The SPLIT_LOCK_CPU() macro escaped the tree-wide sweep for old-style
initialization. Update to use X86_MATCH_INTEL_FAM6_MODEL().

Fixes: 6650cdd9a8cc ("x86/split_lock: Enable split lock detection by kernel")
Signed-off-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lkml.kernel.org/r/20200416205754.21177-2-tony.luck@intel.com

---
 arch/x86/kernel/cpu/intel.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index bf08d45..ec0d8c7 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -1119,8 +1119,6 @@ void switch_to_sld(unsigned long tifn)
 	sld_update_msr(!(tifn & _TIF_SLD));
 }
 
-#define SPLIT_LOCK_CPU(model) {X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY}
-
 /*
  * The following processors have the split lock detection feature. But
  * since they don't have the IA32_CORE_CAPABILITIES MSR, the feature cannot
@@ -1128,8 +1126,8 @@ void switch_to_sld(unsigned long tifn)
  * processors.
  */
 static const struct x86_cpu_id split_lock_cpu_ids[] __initconst = {
-	SPLIT_LOCK_CPU(INTEL_FAM6_ICELAKE_X),
-	SPLIT_LOCK_CPU(INTEL_FAM6_ICELAKE_L),
+	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X,		0),
+	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_L,		0),
 	{}
 };
 

Re: [PATCH 2/3] x86/split_lock: Bits in IA32_CORE_CAPABILITIES are not architectural

[Luck, Tony]

On Fri, Apr 17, 2020 at 12:04:36PM +0200, Thomas Gleixner wrote:
> Tony Luck <tony.luck@intel.com> writes:
> > +	m = x86_match_cpu(split_lock_cpu_ids);
> > +	if (!m)
> > +		return;
> > +
> > +	if (m->driver_data && cpu_has(c, X86_FEATURE_CORE_CAPABILITIES))
> > {
> 
> This condition results in the following:
> 
>     driver_data     MSR_CORE_CAPS	 MSR_CORE_CAPS_SLD	SLD available
> 
> 1       0             Don't care          Don't care               Y
> 2       1                N                Don't care               Y
> 3       1                Y                    Y                    Y
> 4       1                Y                    N                    N
> 
> #2 does not make any sense to me.

Nor to me :-(

I got too clever trying to combine tests.

New version taking cases one at a time so my "stuck inside the
house for six weeks now" brain can follow the steps.

-Tony

From 8c9d779e358eaa239b9647b7a3fe8ebee9becd63 Mon Sep 17 00:00:00 2001
From: Tony Luck <tony.luck@intel.com>
Date: Thu, 16 Apr 2020 10:37:42 -0700
Subject: [PATCH v2] x86/split_lock: Bits in IA32_CORE_CAPABILITIES are not
 architectural

The Intel Software Developers' Manual erroneously listed bit 5 of the
IA32_CORE_CAPABILITIES register as an architectural feature. It is not.

Features enumerated by IA32_CORE_CAPABILITIES are model specific and
implementation details may vary in different cpu models. Thus it is only
safe to trust features after checking the CPU model.

Icelake client and server models are known to implement the split lock
detect feature even though they don't enumerate IA32_CORE_CAPABILITIES

Fixes: 6650cdd9a8cc ("x86/split_lock: Enable split lock detection by kernel")
Cc: <stable@vger.kernel.org>
Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/kernel/cpu/intel.c | 38 +++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 6119deb32660..0bf0d7e3832a 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -1120,10 +1120,12 @@ void switch_to_sld(unsigned long tifn)
 }
 
 /*
- * The following processors have the split lock detection feature. But
- * since they don't have the IA32_CORE_CAPABILITIES MSR, the feature cannot
- * be enumerated. Enable it by family and model matching on these
- * processors.
+ * Bits in the IA32_CORE_CAPABILITIES are not architectural, so they
+ * should only be trusted if you know you are on a model that implements
+ * them.
+ * The driver_data field is set to zero to indicate CPU models like
+ * Icelake that are known to have the split-lock feature even though
+ * they do not enumerate IA32_CORE_CAPABILITIES.
  */
 static const struct x86_cpu_id split_lock_cpu_ids[] __initconst = {
 	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X,           0),
@@ -1133,19 +1135,23 @@ static const struct x86_cpu_id split_lock_cpu_ids[] __initconst = {
 
 void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c)
 {
-	u64 ia32_core_caps = 0;
+	const struct x86_cpu_id *m;
+	u64 ia32_core_caps;
 
-	if (c->x86_vendor != X86_VENDOR_INTEL)
+	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
+		return;
+
+	m = x86_match_cpu(split_lock_cpu_ids);
+	if (!m)
 		return;
-	if (cpu_has(c, X86_FEATURE_CORE_CAPABILITIES)) {
-		/* Enumerate features reported in IA32_CORE_CAPABILITIES MSR. */
-		rdmsrl(MSR_IA32_CORE_CAPS, ia32_core_caps);
-	} else if (!boot_cpu_has(X86_FEATURE_HYPERVISOR)) {
-		/* Enumerate split lock detection by family and model. */
-		if (x86_match_cpu(split_lock_cpu_ids))
-			ia32_core_caps |= MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT;
-	}
 
-	if (ia32_core_caps & MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT)
-		split_lock_setup();
+	if (!m->driver_data)
+		goto setup;
+	if (!cpu_has(c, X86_FEATURE_CORE_CAPABILITIES))
+		return;
+	rdmsrl(MSR_IA32_CORE_CAPS, ia32_core_caps);
+	if (!(ia32_core_caps & MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT))
+		return;
+setup:
+	split_lock_setup();
 }
-- 
2.21.1

Re: [PATCH 2/3] x86/split_lock: Bits in IA32_CORE_CAPABILITIES are not architectural

[Thomas Gleixner]

"Luck, Tony" <tony.luck@intel.com> writes:
> On Fri, Apr 17, 2020 at 12:04:36PM +0200, Thomas Gleixner wrote:
> +	if (!m->driver_data)
> +		goto setup;
> +	if (!cpu_has(c, X86_FEATURE_CORE_CAPABILITIES))
> +		return;
> +	rdmsrl(MSR_IA32_CORE_CAPS, ia32_core_caps);
> +	if (!(ia32_core_caps & MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT))
> +		return;
> +setup:
> +	split_lock_setup();

Which looks nicer w/o the goto:

	if (m->driver_data) {
		if (!cpu_has(c, X86_FEATURE_CORE_CAPABILITIES))
			return;
		rdmsrl(MSR_IA32_CORE_CAPS, ia32_core_caps);
		if (!(ia32_core_caps & MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT))
			return;
	}

Hmm?

Re: [PATCH 2/3] x86/split_lock: Bits in IA32_CORE_CAPABILITIES are not architectural

[Luck, Tony]

On Fri, Apr 17, 2020 at 09:29:13PM +0200, Thomas Gleixner wrote:
> "Luck, Tony" <tony.luck@intel.com> writes:
> > On Fri, Apr 17, 2020 at 12:04:36PM +0200, Thomas Gleixner wrote:
> > +	if (!m->driver_data)
> > +		goto setup;
> > +	if (!cpu_has(c, X86_FEATURE_CORE_CAPABILITIES))
> > +		return;
> > +	rdmsrl(MSR_IA32_CORE_CAPS, ia32_core_caps);
> > +	if (!(ia32_core_caps & MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT))
> > +		return;
> > +setup:
> > +	split_lock_setup();
> 
> Which looks nicer w/o the goto:
> 
> 	if (m->driver_data) {
> 		if (!cpu_has(c, X86_FEATURE_CORE_CAPABILITIES))
> 			return;
> 		rdmsrl(MSR_IA32_CORE_CAPS, ia32_core_caps);
> 		if (!(ia32_core_caps & MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT))
> 			return;
> 	}
> 
> Hmm?

Swings and roundabouts ... getting rid of the goto makes for
deeper indentation. But if you really want to get rid of the
goto, then your version is fine with me.

Do you want me to spin it into v3?

-Tony

Re: [PATCH 2/3] x86/split_lock: Bits in IA32_CORE_CAPABILITIES are not architectural

[Thomas Gleixner]

"Luck, Tony" <tony.luck@intel.com> writes:

> On Fri, Apr 17, 2020 at 09:29:13PM +0200, Thomas Gleixner wrote:
>> "Luck, Tony" <tony.luck@intel.com> writes:
>> > On Fri, Apr 17, 2020 at 12:04:36PM +0200, Thomas Gleixner wrote:
>> > +	if (!m->driver_data)
>> > +		goto setup;
>> > +	if (!cpu_has(c, X86_FEATURE_CORE_CAPABILITIES))
>> > +		return;
>> > +	rdmsrl(MSR_IA32_CORE_CAPS, ia32_core_caps);
>> > +	if (!(ia32_core_caps & MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT))
>> > +		return;
>> > +setup:
>> > +	split_lock_setup();
>> 
>> Which looks nicer w/o the goto:
>> 
>> 	if (m->driver_data) {
>> 		if (!cpu_has(c, X86_FEATURE_CORE_CAPABILITIES))
>> 			return;
>> 		rdmsrl(MSR_IA32_CORE_CAPS, ia32_core_caps);
>> 		if (!(ia32_core_caps & MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT))
>> 			return;
>> 	}
>> 
>> Hmm?
>
> Swings and roundabouts ... getting rid of the goto makes for
> deeper indentation. But if you really want to get rid of the
> goto, then your version is fine with me.
>
> Do you want me to spin it into v3?

Nah. I tweak it myself.

Re: [PATCH 2/3] x86/split_lock: Bits in IA32_CORE_CAPABILITIES are not architectural

[Thomas Gleixner]

Tony,

Thomas Gleixner <tglx@linutronix.de> writes:
> "Luck, Tony" <tony.luck@intel.com> writes:
>> Swings and roundabouts ... getting rid of the goto makes for
>> deeper indentation. But if you really want to get rid of the
>> goto, then your version is fine with me.
>>
>> Do you want me to spin it into v3?
>
> Nah. I tweak it myself.

as I fear that the infinite wisdom of HW folks will add yet another
variant in the foreseeable future, I used a switch() right away and
tweaked the comments a bit.

Can you have a look, please?

Thanks,

        tglx

8<------------------
From: Tony Luck <tony.luck@intel.com>
Subject: x86/split_lock: Bits in IA32_CORE_CAPABILITIES are not architectural
Date: Thu, 16 Apr 2020 13:57:53 -0700

From: Tony Luck <tony.luck@intel.com>

The Intel Software Developers' Manual erroneously listed bit 5 of the
IA32_CORE_CAPABILITIES register as an architectural feature. It is not.

Features enumerated by IA32_CORE_CAPABILITIES are model specific and
implementation details may vary in different cpu models. Thus it is only
safe to trust features after checking the CPU model.

Icelake client and server models are known to implement the split lock
detect feature even though they don't enumerate IA32_CORE_CAPABILITIES

Fixes: 6650cdd9a8cc ("x86/split_lock: Enable split lock detection by kernel")
Signed-off-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lkml.kernel.org/r/20200416205754.21177-3-tony.luck@intel.com

---
 arch/x86/kernel/cpu/intel.c |   45 ++++++++++++++++++++++++++++++--------------
 1 file changed, 31 insertions(+), 14 deletions(-)

--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -1120,10 +1120,17 @@ void switch_to_sld(unsigned long tifn)
 }
 
 /*
- * The following processors have the split lock detection feature. But
- * since they don't have the IA32_CORE_CAPABILITIES MSR, the feature cannot
- * be enumerated. Enable it by family and model matching on these
- * processors.
+ * Bits in the IA32_CORE_CAPABILITIES are not architectural, so they should
+ * only be trusted if it is confirmed that a CPU model implements a
+ * specific feature at a particular bit position.
+ *
+ * The possible driver data field values:
+ *
+ * - 0: CPU models that are known to have the per-core split-lock detection
+ *	feature even though they do not enumerate IA32_CORE_CAPABILITIES.
+ *
+ * - 1: CPU models which may enumerate IA32_CORE_CAPABILITIES and if so use
+ *      bit 5 to enumerate the per-core split-lock detection feature.
  */
 static const struct x86_cpu_id split_lock_cpu_ids[] __initconst = {
 	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X,		0),
@@ -1133,19 +1140,29 @@ static const struct x86_cpu_id split_loc
 
 void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c)
 {
-	u64 ia32_core_caps = 0;
+	const struct x86_cpu_id *m;
+	u64 ia32_core_caps;
 
-	if (c->x86_vendor != X86_VENDOR_INTEL)
+	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
 		return;
-	if (cpu_has(c, X86_FEATURE_CORE_CAPABILITIES)) {
-		/* Enumerate features reported in IA32_CORE_CAPABILITIES MSR. */
+
+	m = x86_match_cpu(split_lock_cpu_ids);
+	if (!m)
+		return;
+
+	switch (m->driver_data) {
+	case 0:
+		break;
+	case 1:
+		if (!cpu_has(c, X86_FEATURE_CORE_CAPABILITIES))
+			return;
 		rdmsrl(MSR_IA32_CORE_CAPS, ia32_core_caps);
-	} else if (!boot_cpu_has(X86_FEATURE_HYPERVISOR)) {
-		/* Enumerate split lock detection by family and model. */
-		if (x86_match_cpu(split_lock_cpu_ids))
-			ia32_core_caps |= MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT;
+		if (!(ia32_core_caps & MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT))
+			return;
+		break;
+	default:
+		return;
 	}
 
-	if (ia32_core_caps & MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT)
-		split_lock_setup();
+	split_lock_setup();
 }

Re: [PATCH 2/3] x86/split_lock: Bits in IA32_CORE_CAPABILITIES are not architectural

[Luck, Tony]

On Fri, Apr 17, 2020 at 11:07:37PM +0200, Thomas Gleixner wrote:
> Tony,
> 
> Thomas Gleixner <tglx@linutronix.de> writes:
> > "Luck, Tony" <tony.luck@intel.com> writes:
> >> Swings and roundabouts ... getting rid of the goto makes for
> >> deeper indentation. But if you really want to get rid of the
> >> goto, then your version is fine with me.
> >>
> >> Do you want me to spin it into v3?
> >
> > Nah. I tweak it myself.
> 
> as I fear that the infinite wisdom of HW folks will add yet another
> variant in the foreseeable future, I used a switch() right away and
> tweaked the comments a bit.
> 
> Can you have a look, please?

Looks like you are all ready for "case 2:" when Intel produces
a less sucky implementation of split lock detect. Don't hold your
breath waiting for that :-)

Looks good. Should be a useful template for any future
bits that show up in CORE_CAPABILITIES.

-Tony

Re: [PATCH 2/3] x86/split_lock: Bits in IA32_CORE_CAPABILITIES are not architectural

[Thomas Gleixner]

"Luck, Tony" <tony.luck@intel.com> writes:
> On Fri, Apr 17, 2020 at 11:07:37PM +0200, Thomas Gleixner wrote:
>> Thomas Gleixner <tglx@linutronix.de> writes:
>> as I fear that the infinite wisdom of HW folks will add yet another
>> variant in the foreseeable future, I used a switch() right away and
>> tweaked the comments a bit.
>> 
>> Can you have a look, please?
>
> Looks like you are all ready for "case 2:" when Intel produces
> a less sucky implementation of split lock detect. Don't hold your
> breath waiting for that :-)

Surely not.

I'm still hoping to see quirkless TSCs and timers, sane IRET semantics
and the demise of non-maskable MSIs before my retirement. Not that I
believe it will happen, it's just because hope dies last.

> Looks good. Should be a useful template for any future
> bits that show up in CORE_CAPABILITIES.

Groan. No!

Thanks,

        tglx

Re: [PATCH 2/3] x86/split_lock: Bits in IA32_CORE_CAPABILITIES are not architectural

[Xiaoyao Li]

On 4/18/2020 5:07 AM, Thomas Gleixner wrote:
> Tony,
> 
> Thomas Gleixner <tglx@linutronix.de> writes:
>> "Luck, Tony" <tony.luck@intel.com> writes:
>>> Swings and roundabouts ... getting rid of the goto makes for
>>> deeper indentation. But if you really want to get rid of the
>>> goto, then your version is fine with me.
>>>
>>> Do you want me to spin it into v3?
>>
>> Nah. I tweak it myself.
> 
> as I fear that the infinite wisdom of HW folks will add yet another
> variant in the foreseeable future, I used a switch() right away and
> tweaked the comments a bit.
> 
> Can you have a look, please?
> 
> Thanks,
> 
>          tglx
> 
> 8<------------------
> From: Tony Luck <tony.luck@intel.com>
> Subject: x86/split_lock: Bits in IA32_CORE_CAPABILITIES are not architectural
> Date: Thu, 16 Apr 2020 13:57:53 -0700
> 
> From: Tony Luck <tony.luck@intel.com>
> 
> The Intel Software Developers' Manual erroneously listed bit 5 of the
> IA32_CORE_CAPABILITIES register as an architectural feature. It is not.
> 
> Features enumerated by IA32_CORE_CAPABILITIES are model specific and
> implementation details may vary in different cpu models. Thus it is only
> safe to trust features after checking the CPU model.
> 
> Icelake client and server models are known to implement the split lock
> detect feature even though they don't enumerate IA32_CORE_CAPABILITIES
> 
> Fixes: 6650cdd9a8cc ("x86/split_lock: Enable split lock detection by kernel")
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Link: https://lkml.kernel.org/r/20200416205754.21177-3-tony.luck@intel.com
> 
> ---
>   arch/x86/kernel/cpu/intel.c |   45 ++++++++++++++++++++++++++++++--------------
>   1 file changed, 31 insertions(+), 14 deletions(-)
> 
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -1120,10 +1120,17 @@ void switch_to_sld(unsigned long tifn)
>   }
>   
>   /*
> - * The following processors have the split lock detection feature. But
> - * since they don't have the IA32_CORE_CAPABILITIES MSR, the feature cannot
> - * be enumerated. Enable it by family and model matching on these
> - * processors.
> + * Bits in the IA32_CORE_CAPABILITIES are not architectural, so they should
> + * only be trusted if it is confirmed that a CPU model implements a
> + * specific feature at a particular bit position.
> + *
> + * The possible driver data field values:
> + *
> + * - 0: CPU models that are known to have the per-core split-lock detection
> + *	feature even though they do not enumerate IA32_CORE_CAPABILITIES.
> + *
> + * - 1: CPU models which may enumerate IA32_CORE_CAPABILITIES and if so use
> + *      bit 5 to enumerate the per-core split-lock detection feature.

So now, it's tightly associated with CPU model, which makes it harder to 
expose this feature to guest. For guest, the CPU model can be configured 
to anything.

As suggested by Sean internally, we'd better use a KVM CPUID to expose 
it to guest, which makes it independent of CPU model.

Paolo, tglx,

What do you think?

>    */
>   static const struct x86_cpu_id split_lock_cpu_ids[] __initconst = {
>   	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X,		0),
> @@ -1133,19 +1140,29 @@ static const struct x86_cpu_id split_loc
>   
>   void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c)
>   {
> -	u64 ia32_core_caps = 0;
> +	const struct x86_cpu_id *m;
> +	u64 ia32_core_caps;
>   
> -	if (c->x86_vendor != X86_VENDOR_INTEL)
> +	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
>   		return;
> -	if (cpu_has(c, X86_FEATURE_CORE_CAPABILITIES)) {
> -		/* Enumerate features reported in IA32_CORE_CAPABILITIES MSR. */
> +
> +	m = x86_match_cpu(split_lock_cpu_ids);
> +	if (!m)
> +		return;
> +
> +	switch (m->driver_data) {
> +	case 0:
> +		break;
> +	case 1:
> +		if (!cpu_has(c, X86_FEATURE_CORE_CAPABILITIES))
> +			return;
>   		rdmsrl(MSR_IA32_CORE_CAPS, ia32_core_caps);
> -	} else if (!boot_cpu_has(X86_FEATURE_HYPERVISOR)) {
> -		/* Enumerate split lock detection by family and model. */
> -		if (x86_match_cpu(split_lock_cpu_ids))
> -			ia32_core_caps |= MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT;
> +		if (!(ia32_core_caps & MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT))
> +			return;
> +		break;
> +	default:
> +		return;
>   	}
>   
> -	if (ia32_core_caps & MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT)
> -		split_lock_setup();
> +	split_lock_setup();
>   }
> 

Re: [PATCH 2/3] x86/split_lock: Bits in IA32_CORE_CAPABILITIES are not architectural

[Sean Christopherson]

On Sat, Apr 18, 2020 at 12:15:57PM +0800, Xiaoyao Li wrote:
> So now, it's tightly associated with CPU model, which makes it harder to
> expose this feature to guest. For guest, the CPU model can be configured to
> anything.
> 
> As suggested by Sean internally, we'd better use a KVM CPUID to expose it to
> guest, which makes it independent of CPU model.

Making this a paravirt feature from a KVM perspective would also let us do
the whole STICKY bit thing straight away.  I don't like paravirtualizing
something that could be emulated as-is, I but dislike it less than exposing
features based on CPU model.

Re: [PATCH 2/3] x86/split_lock: Bits in IA32_CORE_CAPABILITIES are not architectural

[Thomas Gleixner]

Xiaoyao,

can you please trim your replies?

Xiaoyao Li <xiaoyao.li@intel.com> writes:
> On 4/18/2020 5:07 AM, Thomas Gleixner wrote:
>> + * Bits in the IA32_CORE_CAPABILITIES are not architectural, so they should
>> + * only be trusted if it is confirmed that a CPU model implements a
>> + * specific feature at a particular bit position.
>> + *
>> + * The possible driver data field values:
>> + *
>> + * - 0: CPU models that are known to have the per-core split-lock detection
>> + *	feature even though they do not enumerate IA32_CORE_CAPABILITIES.
>> + *
>> + * - 1: CPU models which may enumerate IA32_CORE_CAPABILITIES and if so use
>> + *      bit 5 to enumerate the per-core split-lock detection feature.
>
> So now, it's tightly associated with CPU model, which makes it harder to 
> expose this feature to guest. For guest, the CPU model can be configured 
> to anything.
>
> As suggested by Sean internally, we'd better use a KVM CPUID to expose 
> it to guest, which makes it independent of CPU model.

Works for me.

Thanks,

        tglx

[tip: x86/urgent] x86/split_lock: Add Tremont family CPU models

[tip-bot2 for Tony Luck]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     8b9a18a9f2494144fe23fe630d0734310fa65301
Gitweb:        https://git.kernel.org/tip/8b9a18a9f2494144fe23fe630d0734310fa65301
Author:        Tony Luck <tony.luck@intel.com>
AuthorDate:    Thu, 16 Apr 2020 13:57:54 -07:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Sat, 18 Apr 2020 12:48:44 +02:00

x86/split_lock: Add Tremont family CPU models

Tremont CPUs support IA32_CORE_CAPABILITIES bits to indicate whether
specific SKUs have support for split lock detection.

Signed-off-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lkml.kernel.org/r/20200416205754.21177-4-tony.luck@intel.com

---
 arch/x86/kernel/cpu/intel.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index c23ad48..a19a680 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -1135,6 +1135,9 @@ void switch_to_sld(unsigned long tifn)
 static const struct x86_cpu_id split_lock_cpu_ids[] __initconst = {
 	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X,		0),
 	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_L,		0),
+	X86_MATCH_INTEL_FAM6_MODEL(ATOM_TREMONT,	1),
+	X86_MATCH_INTEL_FAM6_MODEL(ATOM_TREMONT_D,	1),
+	X86_MATCH_INTEL_FAM6_MODEL(ATOM_TREMONT_L,	1),
 	{}
 };
 

[tip: x86/urgent] x86/split_lock: Bits in IA32_CORE_CAPABILITIES are not architectural

[tip-bot2 for Tony Luck]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     48fd5b5ee714714f4cf9f9e1cba3b49b1fd40ed6
Gitweb:        https://git.kernel.org/tip/48fd5b5ee714714f4cf9f9e1cba3b49b1fd40ed6
Author:        Tony Luck <tony.luck@intel.com>
AuthorDate:    Thu, 16 Apr 2020 13:57:53 -07:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Sat, 18 Apr 2020 12:48:44 +02:00

x86/split_lock: Bits in IA32_CORE_CAPABILITIES are not architectural

The Intel Software Developers' Manual erroneously listed bit 5 of the
IA32_CORE_CAPABILITIES register as an architectural feature. It is not.

Features enumerated by IA32_CORE_CAPABILITIES are model specific and
implementation details may vary in different cpu models. Thus it is only
safe to trust features after checking the CPU model.

Icelake client and server models are known to implement the split lock
detect feature even though they don't enumerate IA32_CORE_CAPABILITIES

[ tglx: Use switch() for readability and massage comments ]

Fixes: 6650cdd9a8cc ("x86/split_lock: Enable split lock detection by kernel")
Signed-off-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lkml.kernel.org/r/20200416205754.21177-3-tony.luck@intel.com

---
 arch/x86/kernel/cpu/intel.c | 45 ++++++++++++++++++++++++------------
 1 file changed, 31 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index ec0d8c7..c23ad48 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -1120,10 +1120,17 @@ void switch_to_sld(unsigned long tifn)
 }
 
 /*
- * The following processors have the split lock detection feature. But
- * since they don't have the IA32_CORE_CAPABILITIES MSR, the feature cannot
- * be enumerated. Enable it by family and model matching on these
- * processors.
+ * Bits in the IA32_CORE_CAPABILITIES are not architectural, so they should
+ * only be trusted if it is confirmed that a CPU model implements a
+ * specific feature at a particular bit position.
+ *
+ * The possible driver data field values:
+ *
+ * - 0: CPU models that are known to have the per-core split-lock detection
+ *	feature even though they do not enumerate IA32_CORE_CAPABILITIES.
+ *
+ * - 1: CPU models which may enumerate IA32_CORE_CAPABILITIES and if so use
+ *      bit 5 to enumerate the per-core split-lock detection feature.
  */
 static const struct x86_cpu_id split_lock_cpu_ids[] __initconst = {
 	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X,		0),
@@ -1133,19 +1140,29 @@ static const struct x86_cpu_id split_lock_cpu_ids[] __initconst = {
 
 void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c)
 {
-	u64 ia32_core_caps = 0;
+	const struct x86_cpu_id *m;
+	u64 ia32_core_caps;
+
+	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
+		return;
 
-	if (c->x86_vendor != X86_VENDOR_INTEL)
+	m = x86_match_cpu(split_lock_cpu_ids);
+	if (!m)
 		return;
-	if (cpu_has(c, X86_FEATURE_CORE_CAPABILITIES)) {
-		/* Enumerate features reported in IA32_CORE_CAPABILITIES MSR. */
+
+	switch (m->driver_data) {
+	case 0:
+		break;
+	case 1:
+		if (!cpu_has(c, X86_FEATURE_CORE_CAPABILITIES))
+			return;
 		rdmsrl(MSR_IA32_CORE_CAPS, ia32_core_caps);
-	} else if (!boot_cpu_has(X86_FEATURE_HYPERVISOR)) {
-		/* Enumerate split lock detection by family and model. */
-		if (x86_match_cpu(split_lock_cpu_ids))
-			ia32_core_caps |= MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT;
+		if (!(ia32_core_caps & MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT))
+			return;
+		break;
+	default:
+		return;
 	}
 
-	if (ia32_core_caps & MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT)
-		split_lock_setup();
+	split_lock_setup();
 }