[PATCH 0/3] Check enumeration before MSR save/restore

[PATCH 0/3] Check enumeration before MSR save/restore

[Pawan Gupta]

Hi,

This patchset is to fix the "unchecked MSR access error" [1] during S3
resume. Patch 1/3 adds a feature bit for MSR_IA32_TSX_CTRL.

Patch 2/3 adds a feature bit for MSR_AMD64_LS_CFG.

Patch 3/3 adds check for feature bit before adding any speculation
control MSR to the list of MSRs to save/restore.

[1] https://lore.kernel.org/lkml/20220906201743.436091-1-hdegoede@redhat.com/

Pawan Gupta (3):
  x86/tsx: Add feature bit for TSX control MSR support
  x86/cpu/amd: Add feature bit for MSR_AMD64_LS_CFG enumeration
  x86/pm: Add enumeration check before spec MSRs save/restore setup

 arch/x86/include/asm/cpufeatures.h |  2 ++
 arch/x86/kernel/cpu/amd.c          |  3 +++
 arch/x86/kernel/cpu/tsx.c          | 30 +++++++++++++++---------------
 arch/x86/power/cpu.c               | 23 ++++++++++++++++-------
 4 files changed, 36 insertions(+), 22 deletions(-)


base-commit: 80e78fcce86de0288793a0ef0f6acf37656ee4cf
-- 
2.37.2

[PATCH 1/3] x86/tsx: Add feature bit for TSX control MSR support

[Pawan Gupta]

Support for TSX control MSR is enumerated in MSR_IA32_ARCH_CAPABILITIES.
This is different from how other CPU features are enumerated i.e. via
CPUID. Enumerating support for TSX control currently has an overhead of
reading the MSR every time which can be avoided.

Set a new feature bit X86_FEATURE_TSX_CTRL when MSR_IA32_TSX_CTRL is
present. Also make tsx_ctrl_is_supported() use feature bit instead of
reading the MSR.

This will also be useful for any code that wants to use the feature bit
instead of a calling tsx_ctrl_is_supported().

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
---
 arch/x86/include/asm/cpufeatures.h |  1 +
 arch/x86/kernel/cpu/tsx.c          | 30 +++++++++++++++---------------
 2 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index ef4775c6db01..dd173733e40d 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -304,6 +304,7 @@
 #define X86_FEATURE_UNRET		(11*32+15) /* "" AMD BTB untrain return */
 #define X86_FEATURE_USE_IBPB_FW		(11*32+16) /* "" Use IBPB during runtime firmware calls */
 #define X86_FEATURE_RSB_VMEXIT_LITE	(11*32+17) /* "" Fill RSB on VM exit when EIBRS is enabled */
+#define X86_FEATURE_MSR_TSX_CTRL	(11*32+18) /* "" MSR IA32_TSX_CTRL */
 
 /* Intel-defined CPU features, CPUID level 0x00000007:1 (EAX), word 12 */
 #define X86_FEATURE_AVX_VNNI		(12*32+ 4) /* AVX VNNI instructions */
diff --git a/arch/x86/kernel/cpu/tsx.c b/arch/x86/kernel/cpu/tsx.c
index ec7bbac3a9f2..9fe488dbed15 100644
--- a/arch/x86/kernel/cpu/tsx.c
+++ b/arch/x86/kernel/cpu/tsx.c
@@ -60,20 +60,7 @@ static void tsx_enable(void)
 
 static bool tsx_ctrl_is_supported(void)
 {
-	u64 ia32_cap = x86_read_arch_cap_msr();
-
-	/*
-	 * TSX is controlled via MSR_IA32_TSX_CTRL.  However, support for this
-	 * MSR is enumerated by ARCH_CAP_TSX_MSR bit in MSR_IA32_ARCH_CAPABILITIES.
-	 *
-	 * TSX control (aka MSR_IA32_TSX_CTRL) is only available after a
-	 * microcode update on CPUs that have their MSR_IA32_ARCH_CAPABILITIES
-	 * bit MDS_NO=1. CPUs with MDS_NO=0 are not planned to get
-	 * MSR_IA32_TSX_CTRL support even after a microcode update. Thus,
-	 * tsx= cmdline requests will do nothing on CPUs without
-	 * MSR_IA32_TSX_CTRL support.
-	 */
-	return !!(ia32_cap & ARCH_CAP_TSX_CTRL_MSR);
+	return cpu_feature_enabled(X86_FEATURE_MSR_TSX_CTRL);
 }
 
 static enum tsx_ctrl_states x86_get_tsx_auto_mode(void)
@@ -191,7 +178,20 @@ void __init tsx_init(void)
 		return;
 	}
 
-	if (!tsx_ctrl_is_supported()) {
+	/*
+	 * TSX is controlled via MSR_IA32_TSX_CTRL.  However, support for this
+	 * MSR is enumerated by ARCH_CAP_TSX_MSR bit in MSR_IA32_ARCH_CAPABILITIES.
+	 *
+	 * TSX control (aka MSR_IA32_TSX_CTRL) is only available after a
+	 * microcode update on CPUs that have their MSR_IA32_ARCH_CAPABILITIES
+	 * bit MDS_NO=1. CPUs with MDS_NO=0 are not planned to get
+	 * MSR_IA32_TSX_CTRL support even after a microcode update. Thus,
+	 * tsx= cmdline requests will do nothing on CPUs without
+	 * MSR_IA32_TSX_CTRL support.
+	 */
+	if (x86_read_arch_cap_msr() & ARCH_CAP_TSX_CTRL_MSR) {
+		setup_force_cpu_cap(X86_FEATURE_MSR_TSX_CTRL);
+	} else {
 		tsx_ctrl_state = TSX_CTRL_NOT_SUPPORTED;
 		return;
 	}
-- 
2.37.2

Re: [PATCH 1/3] x86/tsx: Add feature bit for TSX control MSR support

[Dave Hansen]

On 9/12/22 16:39, Pawan Gupta wrote:
> Support for TSX control MSR is enumerated in MSR_IA32_ARCH_CAPABILITIES.
> This is different from how other CPU features are enumerated i.e. via
> CPUID. Enumerating support for TSX control currently has an overhead of
> reading the MSR every time which can be avoided.

I only see tsx_ctrl_is_supported() getting called in three places:

> 1 tsx.c tsx_clear_cpuid       138 } else if (tsx_ctrl_is_supported()) {
> 2 tsx.c tsx_dev_mode_disable  161 if (!boot_cpu_has_bug(X86_BUG_TAA) || !tsx_ctrl_is_supported() ||
> 3 tsx.c tsx_init              194 if (!tsx_ctrl_is_supported()) {

Those all look like boot-time things to me.  Why does the overhead matter?

Re: [PATCH 1/3] x86/tsx: Add feature bit for TSX control MSR support

[Pawan Gupta]

On Tue, Nov 08, 2022 at 10:27:56AM -0800, Dave Hansen wrote:
>On 9/12/22 16:39, Pawan Gupta wrote:
>> Support for TSX control MSR is enumerated in MSR_IA32_ARCH_CAPABILITIES.
>> This is different from how other CPU features are enumerated i.e. via
>> CPUID. Enumerating support for TSX control currently has an overhead of
>> reading the MSR every time which can be avoided.
>
>I only see tsx_ctrl_is_supported() getting called in three places:
>
>> 1 tsx.c tsx_clear_cpuid       138 } else if (tsx_ctrl_is_supported()) {
>> 2 tsx.c tsx_dev_mode_disable  161 if (!boot_cpu_has_bug(X86_BUG_TAA) || !tsx_ctrl_is_supported() ||
>> 3 tsx.c tsx_init              194 if (!tsx_ctrl_is_supported()) {
>
>Those all look like boot-time things to me.

Except tsx_clear_cpuid() could be called during S3 resume as part of
secondary CPU's init, but still its not too often.

>Why does the overhead matter?

This patch is mainly needed for patch 3/3 that relies on feature bits to
decide which MSRs to save/restore during suspend/resume.

I just gave a hint about it in the commit message:

     This will also be useful for any code that wants to use the feature bit
     instead of a calling tsx_ctrl_is_supported().

I will fix the commit message with this as the main reason for adding
the feature bit.

[PATCH 2/3] x86/cpu/amd: Add feature bit for MSR_AMD64_LS_CFG enumeration

[Pawan Gupta]

Currently there is no easy way to enumerate MSR_AMD64_LS_CFG. As this
MSR is supported on AMD CPU families 10h to 18h, set a new feature bit
on these CPU families. The new bit can be used to detect the MSR
support.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
---
 arch/x86/include/asm/cpufeatures.h | 1 +
 arch/x86/kernel/cpu/amd.c          | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index dd173733e40d..90bdb1d98531 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -305,6 +305,7 @@
 #define X86_FEATURE_USE_IBPB_FW		(11*32+16) /* "" Use IBPB during runtime firmware calls */
 #define X86_FEATURE_RSB_VMEXIT_LITE	(11*32+17) /* "" Fill RSB on VM exit when EIBRS is enabled */
 #define X86_FEATURE_MSR_TSX_CTRL	(11*32+18) /* "" MSR IA32_TSX_CTRL */
+#define X86_FEATURE_MSR_LS_CFG		(11*32+19) /* "" MSR AMD64_LS_CFG */
 
 /* Intel-defined CPU features, CPUID level 0x00000007:1 (EAX), word 12 */
 #define X86_FEATURE_AVX_VNNI		(12*32+ 4) /* AVX VNNI instructions */
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 48276c0e479d..88f59d630826 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -521,6 +521,9 @@ static void bsp_init_amd(struct cpuinfo_x86 *c)
 		__max_die_per_package = nodes_per_socket = ((value >> 3) & 7) + 1;
 	}
 
+	if (c->x86 >= 0x10 && c->x86 <= 0x18)
+		setup_force_cpu_cap(X86_FEATURE_MSR_LS_CFG);
+
 	if (!boot_cpu_has(X86_FEATURE_AMD_SSBD) &&
 	    !boot_cpu_has(X86_FEATURE_VIRT_SSBD) &&
 	    c->x86 >= 0x15 && c->x86 <= 0x17) {
-- 
2.37.2

Re: [PATCH 2/3] x86/cpu/amd: Add feature bit for MSR_AMD64_LS_CFG enumeration

[Borislav Petkov]

On Mon, Sep 12, 2022 at 04:40:47PM -0700, Pawan Gupta wrote:
> Currently there is no easy way to enumerate MSR_AMD64_LS_CFG. As this
> MSR is supported on AMD CPU families 10h to 18h, set a new feature bit
> on these CPU families. The new bit can be used to detect the MSR
> support.
> 
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> ---
>  arch/x86/include/asm/cpufeatures.h | 1 +
>  arch/x86/kernel/cpu/amd.c          | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index dd173733e40d..90bdb1d98531 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -305,6 +305,7 @@
>  #define X86_FEATURE_USE_IBPB_FW		(11*32+16) /* "" Use IBPB during runtime firmware calls */
>  #define X86_FEATURE_RSB_VMEXIT_LITE	(11*32+17) /* "" Fill RSB on VM exit when EIBRS is enabled */
>  #define X86_FEATURE_MSR_TSX_CTRL	(11*32+18) /* "" MSR IA32_TSX_CTRL */
> +#define X86_FEATURE_MSR_LS_CFG		(11*32+19) /* "" MSR AMD64_LS_CFG */

We already have that one:

#define X86_FEATURE_LS_CFG_SSBD         ( 7*32+24)  /* "" AMD SSBD implementation via LS_CFG MSR */

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 2/3] x86/cpu/amd: Add feature bit for MSR_AMD64_LS_CFG enumeration

[Pawan Gupta]

On Tue, Nov 08, 2022 at 07:54:01PM +0100, Borislav Petkov wrote:
>On Mon, Sep 12, 2022 at 04:40:47PM -0700, Pawan Gupta wrote:
>> Currently there is no easy way to enumerate MSR_AMD64_LS_CFG. As this
>> MSR is supported on AMD CPU families 10h to 18h, set a new feature bit
>> on these CPU families. The new bit can be used to detect the MSR
>> support.
>>
>> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
>> ---
>>  arch/x86/include/asm/cpufeatures.h | 1 +
>>  arch/x86/kernel/cpu/amd.c          | 3 +++
>>  2 files changed, 4 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
>> index dd173733e40d..90bdb1d98531 100644
>> --- a/arch/x86/include/asm/cpufeatures.h
>> +++ b/arch/x86/include/asm/cpufeatures.h
>> @@ -305,6 +305,7 @@
>>  #define X86_FEATURE_USE_IBPB_FW		(11*32+16) /* "" Use IBPB during runtime firmware calls */
>>  #define X86_FEATURE_RSB_VMEXIT_LITE	(11*32+17) /* "" Fill RSB on VM exit when EIBRS is enabled */
>>  #define X86_FEATURE_MSR_TSX_CTRL	(11*32+18) /* "" MSR IA32_TSX_CTRL */
>> +#define X86_FEATURE_MSR_LS_CFG		(11*32+19) /* "" MSR AMD64_LS_CFG */
>
>We already have that one:
>
>#define X86_FEATURE_LS_CFG_SSBD         ( 7*32+24)  /* "" AMD SSBD implementation via LS_CFG MSR */

Looking at bsp_init_amd() this feature bit will only be set on AMD
families 0x15-0x17. Andrew mentioned that the MSR LS_CFG is present on
AMD family >= 0x10 && family <= 0x18. That is why I added a new bit for
the MSR.

Re: [PATCH 2/3] x86/cpu/amd: Add feature bit for MSR_AMD64_LS_CFG enumeration

[Borislav Petkov]

On Tue, Nov 08, 2022 at 02:51:41PM -0800, Pawan Gupta wrote:
> Looking at bsp_init_amd() this feature bit will only be set on AMD
> families 0x15-0x17. Andrew mentioned that the MSR LS_CFG is present on
> AMD family >= 0x10 && family <= 0x18.

Do you need to save that MSR on those families?

Or do 0x15-0x18 suffice?

Yes, 0x18 because that's Hygon and that does its own detection.

So, do you need to save it on families 0x10-0x14?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 2/3] x86/cpu/amd: Add feature bit for MSR_AMD64_LS_CFG enumeration

[Andrew Cooper]

On 08/11/2022 23:10, Borislav Petkov wrote:
> On Tue, Nov 08, 2022 at 02:51:41PM -0800, Pawan Gupta wrote:
>> Looking at bsp_init_amd() this feature bit will only be set on AMD
>> families 0x15-0x17. Andrew mentioned that the MSR LS_CFG is present on
>> AMD family >= 0x10 && family <= 0x18.
> Do you need to save that MSR on those families?
>
> Or do 0x15-0x18 suffice?
>
> Yes, 0x18 because that's Hygon and that does its own detection.
>
> So, do you need to save it on families 0x10-0x14?

https://www.amd.com/system/files/documents/software-techniques-for-managing-speculation.pdf 
Mitigation G-2.

The MSR exists on Fam 10/12/14/15/16/17, and in all cases the
LFENCE_DISPATCH bit wants setting if not already set.

The MSR is missing on Fam 0f/11 but these parts already have the wanted
behaviour.

~Andrew

Re: [PATCH 2/3] x86/cpu/amd: Add feature bit for MSR_AMD64_LS_CFG enumeration

[Pawan Gupta]

On Wed, Nov 09, 2022 at 12:45:58AM +0000, Andrew Cooper wrote:
>On 08/11/2022 23:10, Borislav Petkov wrote:
>> On Tue, Nov 08, 2022 at 02:51:41PM -0800, Pawan Gupta wrote:
>>> Looking at bsp_init_amd() this feature bit will only be set on AMD
>>> families 0x15-0x17. Andrew mentioned that the MSR LS_CFG is present on
>>> AMD family >= 0x10 && family <= 0x18.
>> Do you need to save that MSR on those families?
>>
>> Or do 0x15-0x18 suffice?
>>
>> Yes, 0x18 because that's Hygon and that does its own detection.
>>
>> So, do you need to save it on families 0x10-0x14?
>
>https://www.amd.com/system/files/documents/software-techniques-for-managing-speculation.pdf 
>Mitigation G-2.
>
>The MSR exists on Fam 10/12/14/15/16/17, and in all cases the
>LFENCE_DISPATCH bit wants setting if not already set.

Isn't that a different MSR:

   #define MSR_AMD64_LS_CFG                0xc0011020

   #define MSR_F10H_DECFG                  0xc0011029
   #define MSR_F10H_DECFG_LFENCE_SERIALIZE_BIT     1

Looks like we need to restore this MSR too, and we can use existing
X86_FEATURE_XMM2 to enumerate it.

If SSBD is the only reason to restore MSR_AMD64_LS_CFG then we should be
able to use X86_FEATURE_LS_CFG_SSBD for enumeration.

Re: [PATCH 2/3] x86/cpu/amd: Add feature bit for MSR_AMD64_LS_CFG enumeration

[Borislav Petkov]

On Wed, Nov 09, 2022 at 09:37:20AM -0800, Pawan Gupta wrote:
> Looks like we need to restore this MSR too,

Why do we? Is that MSR read-only too or what is the reason for that one?

> and we can use existing X86_FEATURE_XMM2 to enumerate it.

Or 86_FEATURE_LFENCE_RDTSC.

> If SSBD is the only reason to restore MSR_AMD64_LS_CFG then we should
> be able to use X86_FEATURE_LS_CFG_SSBD for enumeration.

Yes, MSR_AMD64_LS_CFG is used in SSBD mitigations. For everything <= 0x12:

        /* AMD Family 0xf - 0x12 */
        VULNWL_AMD(0x0f,        NO_MELTDOWN | NO_SSB | NO_L1TF | NO_MDS | NO_SWAPGS | NO_ITLB_MULTIHIT | NO_MMIO),
        VULNWL_AMD(0x10,        NO_MELTDOWN | NO_SSB | NO_L1TF | NO_MDS | NO_SWAPGS | NO_ITLB_MULTIHIT | NO_MMIO),
        VULNWL_AMD(0x11,        NO_MELTDOWN | NO_SSB | NO_L1TF | NO_MDS | NO_SWAPGS | NO_ITLB_MULTIHIT | NO_MMIO),
        VULNWL_AMD(0x12,        NO_MELTDOWN | NO_SSB | NO_L1TF | NO_MDS | NO_SWAPGS | NO_ITLB_MULTIHIT | NO_MMIO),

On F14h it says here:

[    0.278930] Speculative Store Bypass: Vulnerable

simply because it is not present there.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 2/3] x86/cpu/amd: Add feature bit for MSR_AMD64_LS_CFG enumeration

[Pawan Gupta]

On Wed, Nov 09, 2022 at 07:34:55PM +0100, Borislav Petkov wrote:
>On Wed, Nov 09, 2022 at 09:37:20AM -0800, Pawan Gupta wrote:
>> Looks like we need to restore this MSR too,
>
>Why do we? Is that MSR read-only too or what is the reason for that one?

Please ignore, it doesn't concern this series.

>Yes, MSR_AMD64_LS_CFG is used in SSBD mitigations. For everything <= 0x12:
>
>        /* AMD Family 0xf - 0x12 */
>        VULNWL_AMD(0x0f,        NO_MELTDOWN | NO_SSB | NO_L1TF | NO_MDS | NO_SWAPGS | NO_ITLB_MULTIHIT | NO_MMIO),
>        VULNWL_AMD(0x10,        NO_MELTDOWN | NO_SSB | NO_L1TF | NO_MDS | NO_SWAPGS | NO_ITLB_MULTIHIT | NO_MMIO),
>        VULNWL_AMD(0x11,        NO_MELTDOWN | NO_SSB | NO_L1TF | NO_MDS | NO_SWAPGS | NO_ITLB_MULTIHIT | NO_MMIO),
>        VULNWL_AMD(0x12,        NO_MELTDOWN | NO_SSB | NO_L1TF | NO_MDS | NO_SWAPGS | NO_ITLB_MULTIHIT | NO_MMIO),
>
>On F14h it says here:
>
>[    0.278930] Speculative Store Bypass: Vulnerable
>
>simply because it is not present there.

So looks like MSR_AMD64_LS_CFG should only be restored when
X86_FEATURE_LS_CFG_SSBD is present. I will make this change in v2.

Re: [PATCH 2/3] x86/cpu/amd: Add feature bit for MSR_AMD64_LS_CFG enumeration

[Andrew Cooper]

On 09/11/2022 17:37, Pawan Gupta wrote:
> On Wed, Nov 09, 2022 at 12:45:58AM +0000, Andrew Cooper wrote:
>> On 08/11/2022 23:10, Borislav Petkov wrote:
>>> On Tue, Nov 08, 2022 at 02:51:41PM -0800, Pawan Gupta wrote:
>>>> Looking at bsp_init_amd() this feature bit will only be set on AMD
>>>> families 0x15-0x17. Andrew mentioned that the MSR LS_CFG is present on
>>>> AMD family >= 0x10 && family <= 0x18.
>>> Do you need to save that MSR on those families?
>>>
>>> Or do 0x15-0x18 suffice?
>>>
>>> Yes, 0x18 because that's Hygon and that does its own detection.
>>>
>>> So, do you need to save it on families 0x10-0x14?
>>
>> https://www.amd.com/system/files/documents/software-techniques-for-managing-speculation.pdf 
>>
>> Mitigation G-2.
>>
>> The MSR exists on Fam 10/12/14/15/16/17, and in all cases the
>> LFENCE_DISPATCH bit wants setting if not already set.
>
> Isn't that a different MSR:
>
>   #define MSR_AMD64_LS_CFG                0xc0011020
>
>   #define MSR_F10H_DECFG                  0xc0011029
>   #define MSR_F10H_DECFG_LFENCE_SERIALIZE_BIT     1

Oh yes.  You're absolutely correct.  I did get the two mixed up.

Funnily enough, the lfence dispatch properties are in the Decode/Execute
configuration MSR, while the knob for Speculative Store Bypass is in the
Load/Store configuration MSR.

> Looks like we need to restore this MSR too, and we can use existing
> X86_FEATURE_XMM2 to enumerate it.

In this case, I wouldn't say so.

For lfence dispatch, there are no user options.  The bit needs setting,
and you don't care about preserving the old value.

In fact, AMD retroactively declared bit is architectural, and it's
fixed-1 in Fam19h and later so you can't even accidentally turn off
speculation protections.  (Actually, so a malicious hypervisor can't
turn off speculation protections behind the back of an encrypted VM.)

>
> If SSBD is the only reason to restore MSR_AMD64_LS_CFG then we should be
> able to use X86_FEATURE_LS_CFG_SSBD for enumeration.

Yeah, although it occurs to me that you probably don't want to
save/restore the whole MSR.  It's full of other things which want to
stay in their current configuration.

~Andrew

Re: [PATCH 2/3] x86/cpu/amd: Add feature bit for MSR_AMD64_LS_CFG enumeration

[Pawan Gupta]

On Thu, Nov 10, 2022 at 12:00:11AM +0000, Andrew Cooper wrote:
>On 09/11/2022 17:37, Pawan Gupta wrote:
>> If SSBD is the only reason to restore MSR_AMD64_LS_CFG then we should be
>> able to use X86_FEATURE_LS_CFG_SSBD for enumeration.
>
>Yeah, although it occurs to me that you probably don't want to
>save/restore the whole MSR.  It's full of other things which want to
>stay in their current configuration.

Upstream currently saves and restores the whole MSR_AMD64_LS_CFG at
suspend/resume.

Is it that the configuration saved at suspend(other than SSBD) will not
be relevant at resume for this MSR? Is there any harm if keep the
current behavior?

Re: [PATCH 2/3] x86/cpu/amd: Add feature bit for MSR_AMD64_LS_CFG enumeration

[Pawan Gupta]

On Wed, Nov 09, 2022 at 12:10:32AM +0100, Borislav Petkov wrote:
>On Tue, Nov 08, 2022 at 02:51:41PM -0800, Pawan Gupta wrote:
>> Looking at bsp_init_amd() this feature bit will only be set on AMD
>> families 0x15-0x17. Andrew mentioned that the MSR LS_CFG is present on
>> AMD family >= 0x10 && family <= 0x18.
>
>Do you need to save that MSR on those families?
>
>Or do 0x15-0x18 suffice?
>
>Yes, 0x18 because that's Hygon and that does its own detection.
>
>So, do you need to save it on families 0x10-0x14?

As per Andrew's comment [1] the MSR needs to be restored for dispatch
serialising bit(except for family 0x11) :

   AMD LS_CFG is more complicated, because the dispatch serialising bit
   needs setting unilaterally (families 0x10, 0x12 thru 0x18), but the SSBD
   control ought to resolve on the next context switch.

[1] https://lore.kernel.org/lkml/6049e5bc-122f-5b4c-c1dc-0591eccf3525@citrix.com/

[PATCH 3/3] x86/pm: Add enumeration check before spec MSRs save/restore setup

[Pawan Gupta]

On an Intel Atom N2600 (and presumable other Cedar Trail models)
MSR_IA32_TSX_CTRL can be read, causing saved_msr.valid to be set for it
by msr_build_context().

This causes restore_processor_state() to try and restore it, but writing
this MSR is not allowed on the Intel Atom N2600 leading to:

[   99.955141] unchecked MSR access error: WRMSR to 0x122 (tried to write 0x0000000000000002) at rIP: 0xffffffff8b07a574 (native_write_msr+0x4/0x20)
[   99.955176] Call Trace:
[   99.955186]  <TASK>
[   99.955195]  restore_processor_state+0x275/0x2c0
[   99.955246]  x86_acpi_suspend_lowlevel+0x10e/0x140
[   99.955273]  acpi_suspend_enter+0xd3/0x100
[   99.955297]  suspend_devices_and_enter+0x7e2/0x830
[   99.955341]  pm_suspend.cold+0x2d2/0x35e
[   99.955368]  state_store+0x68/0xd0
[   99.955402]  kernfs_fop_write_iter+0x15e/0x210
[   99.955442]  vfs_write+0x225/0x4b0
[   99.955523]  ksys_write+0x59/0xd0
[   99.955557]  do_syscall_64+0x58/0x80
[   99.955579]  ? do_syscall_64+0x67/0x80
[   99.955600]  ? up_read+0x17/0x20
[   99.955631]  ? lock_is_held_type+0xe3/0x140
[   99.955670]  ? asm_exc_page_fault+0x22/0x30
[   99.955688]  ? lockdep_hardirqs_on+0x7d/0x100
[   99.955710]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
[   99.955723] RIP: 0033:0x7f7d0fb018f7
[   99.955741] Code: 0f 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 48 89 54 24 18 48 89 74 24
[   99.955753] RSP: 002b:00007ffd03292ee8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
[   99.955771] RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007f7d0fb018f7
[   99.955781] RDX: 0000000000000004 RSI: 00007ffd03292fd0 RDI: 0000000000000004
[   99.955790] RBP: 00007ffd03292fd0 R08: 000000000000c0fe R09: 0000000000000000
[   99.955799] R10: 00007f7d0fb85fb0 R11: 0000000000000246 R12: 0000000000000004
[   99.955808] R13: 000055df564173e0 R14: 0000000000000004 R15: 00007f7d0fbf49e0
[   99.955910]  </TASK>

Add speculation control MSRs to MSR save/restore list only if their
corresponding feature bit is set. This prevents MSR save/restore to
attempt invalid MSR.

Reported-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
---
 arch/x86/power/cpu.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c
index bb176c72891c..b3492dd55e61 100644
--- a/arch/x86/power/cpu.c
+++ b/arch/x86/power/cpu.c
@@ -511,17 +511,26 @@ static int pm_cpu_check(const struct x86_cpu_id *c)
 	return ret;
 }
 
+struct msr_enumeration {
+	u32 msr_no;
+	u32 feature;
+};
+
 static void pm_save_spec_msr(void)
 {
-	u32 spec_msr_id[] = {
-		MSR_IA32_SPEC_CTRL,
-		MSR_IA32_TSX_CTRL,
-		MSR_TSX_FORCE_ABORT,
-		MSR_IA32_MCU_OPT_CTRL,
-		MSR_AMD64_LS_CFG,
+	struct msr_enumeration msr_enum[] = {
+		{MSR_IA32_SPEC_CTRL,	X86_FEATURE_MSR_SPEC_CTRL},
+		{MSR_IA32_TSX_CTRL,	X86_FEATURE_MSR_TSX_CTRL},
+		{MSR_TSX_FORCE_ABORT,	X86_FEATURE_TSX_FORCE_ABORT},
+		{MSR_IA32_MCU_OPT_CTRL,	X86_FEATURE_SRBDS_CTRL},
+		{MSR_AMD64_LS_CFG,	X86_FEATURE_MSR_LS_CFG},
 	};
+	int i;
 
-	msr_build_context(spec_msr_id, ARRAY_SIZE(spec_msr_id));
+	for (i = 0; i < ARRAY_SIZE(msr_enum); i++) {
+		if (boot_cpu_has(msr_enum[i].feature))
+			msr_build_context(&msr_enum[i].msr_no, 1);
+	}
 }
 
 static int pm_check_save_msr(void)
-- 
2.37.2

Re: [PATCH 3/3] x86/pm: Add enumeration check before spec MSRs save/restore setup

[Dave Hansen]

On 9/12/22 16:41, Pawan Gupta wrote:
> On an Intel Atom N2600 (and presumable other Cedar Trail models)
> MSR_IA32_TSX_CTRL can be read, causing saved_msr.valid to be set for it
> by msr_build_context().

This changelog needs some help.  Shouldn't it be something like this?

pm_save_spec_msr() keeps a list of all the MSRs which _might_ need to be
saved and restored at hibernate?? and resume??.  However, it has zero
awareness of CPU support for these MSRs.  It mostly works by
unconditionally attempting to manipulate these MSRs and relying on
rdmsrl_safe() being able to handle a #GP on CPUs where the support is
unavailable.

However, it's possible for reads (RDMSR) to be supported for a given MSR
while writes (WRMSR) are not.  In this case, msr_build_context() sees a
successful read (RDMSR) and marks the MSR as 'valid'.  Then, later, a
write (WRMSR) fails, producing a nasty (but harmless) error message.

To fix this, add the corresponding X86_FEATURE bit for each MSR.  Avoid
trying to manipulate the MSR when the feature bit is clear.  This
required adding a X86_FEATURE bit for MSRs that do not have one already,
but it's a small price to pay.

Re: [PATCH 3/3] x86/pm: Add enumeration check before spec MSRs save/restore setup

[Pawan Gupta]

On Tue, Nov 08, 2022 at 10:40:06AM -0800, Dave Hansen wrote:
>On 9/12/22 16:41, Pawan Gupta wrote:
>> On an Intel Atom N2600 (and presumable other Cedar Trail models)
>> MSR_IA32_TSX_CTRL can be read, causing saved_msr.valid to be set for it
>> by msr_build_context().
>
>This changelog needs some help.  Shouldn't it be something like this?
>
>pm_save_spec_msr() keeps a list of all the MSRs which _might_ need to be
>saved and restored at hibernate?? and resume??.  However, it has zero
>awareness of CPU support for these MSRs.  It mostly works by
>unconditionally attempting to manipulate these MSRs and relying on
>rdmsrl_safe() being able to handle a #GP on CPUs where the support is
>unavailable.
>
>However, it's possible for reads (RDMSR) to be supported for a given MSR
>while writes (WRMSR) are not.  In this case, msr_build_context() sees a
>successful read (RDMSR) and marks the MSR as 'valid'.  Then, later, a
>write (WRMSR) fails, producing a nasty (but harmless) error message.
>
>To fix this, add the corresponding X86_FEATURE bit for each MSR.  Avoid
>trying to manipulate the MSR when the feature bit is clear.  This
>required adding a X86_FEATURE bit for MSRs that do not have one already,
>but it's a small price to pay.

Yes, that's a lot better. Thanks.

Re: [PATCH 0/3] Check enumeration before MSR save/restore

[Pawan Gupta]

On Mon, Sep 12, 2022 at 04:38:44PM -0700, Pawan Gupta wrote:
> Hi,
> 
> This patchset is to fix the "unchecked MSR access error" [1] during S3
> resume. Patch 1/3 adds a feature bit for MSR_IA32_TSX_CTRL.
> 
> Patch 2/3 adds a feature bit for MSR_AMD64_LS_CFG.
> 
> Patch 3/3 adds check for feature bit before adding any speculation
> control MSR to the list of MSRs to save/restore.
> 
> [1] https://lore.kernel.org/lkml/20220906201743.436091-1-hdegoede@redhat.com/

Added the correct email-id of Hans de Goede <hdegoede@redhat.com>.

Re: [PATCH 0/3] Check enumeration before MSR save/restore

[Hans de Goede]

Hi,

On 9/13/22 02:50, Pawan Gupta wrote:
> On Mon, Sep 12, 2022 at 04:38:44PM -0700, Pawan Gupta wrote:
>> Hi,
>>
>> This patchset is to fix the "unchecked MSR access error" [1] during S3
>> resume. Patch 1/3 adds a feature bit for MSR_IA32_TSX_CTRL.
>>
>> Patch 2/3 adds a feature bit for MSR_AMD64_LS_CFG.
>>
>> Patch 3/3 adds check for feature bit before adding any speculation
>> control MSR to the list of MSRs to save/restore.
>>
>> [1] https://lore.kernel.org/lkml/20220906201743.436091-1-hdegoede@redhat.com/
> 
> Added the correct email-id of Hans de Goede <hdegoede@redhat.com>.

I have tested this series and I can confirm that it fixes the exception
which I was seeing on a Packard Bell Dot SC with an Atom N2600 CPU:

Tested-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

Re: [PATCH 0/3] Check enumeration before MSR save/restore

[Pawan Gupta]

On Sat, Sep 17, 2022 at 01:42:13PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 9/13/22 02:50, Pawan Gupta wrote:
> > On Mon, Sep 12, 2022 at 04:38:44PM -0700, Pawan Gupta wrote:
> >> Hi,
> >>
> >> This patchset is to fix the "unchecked MSR access error" [1] during S3
> >> resume. Patch 1/3 adds a feature bit for MSR_IA32_TSX_CTRL.
> >>
> >> Patch 2/3 adds a feature bit for MSR_AMD64_LS_CFG.
> >>
> >> Patch 3/3 adds check for feature bit before adding any speculation
> >> control MSR to the list of MSRs to save/restore.
> >>
> >> [1] https://lore.kernel.org/lkml/20220906201743.436091-1-hdegoede@redhat.com/
> > 
> > Added the correct email-id of Hans de Goede <hdegoede@redhat.com>.
> 
> I have tested this series and I can confirm that it fixes the exception
> which I was seeing on a Packard Bell Dot SC with an Atom N2600 CPU:
> 
> Tested-by: Hans de Goede <hdegoede@redhat.com>

Thanks.

Re: [PATCH 0/3] Check enumeration before MSR save/restore

[Hans de Goede]

Hi All,

On 9/13/22 01:38, Pawan Gupta wrote:
> Hi,
> 
> This patchset is to fix the "unchecked MSR access error" [1] during S3
> resume. Patch 1/3 adds a feature bit for MSR_IA32_TSX_CTRL.
> 
> Patch 2/3 adds a feature bit for MSR_AMD64_LS_CFG.
> 
> Patch 3/3 adds check for feature bit before adding any speculation
> control MSR to the list of MSRs to save/restore.
> 
> [1] https://lore.kernel.org/lkml/20220906201743.436091-1-hdegoede@redhat.com/
> 
> Pawan Gupta (3):
>   x86/tsx: Add feature bit for TSX control MSR support
>   x86/cpu/amd: Add feature bit for MSR_AMD64_LS_CFG enumeration
>   x86/pm: Add enumeration check before spec MSRs save/restore setup

What is the status of this series ?

To me this seems like a sensible way to solve the problem which
I reported and other similar problems...

Regards,

Hans

Re: [PATCH 0/3] Check enumeration before MSR save/restore

[Borislav Petkov]

On Tue, Nov 08, 2022 at 07:16:00PM +0100, Hans de Goede wrote:
> What is the status of this series ?

Lost in the avalanche of patches. ;-\

Will take a look tomorrow.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 0/3] Check enumeration before MSR save/restore

[Borislav Petkov]

On Tue, Nov 08, 2022 at 07:26:43PM +0100, Borislav Petkov wrote:
> Will take a look tomorrow.

Yap, needs a new revision.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 0/3] Check enumeration before MSR save/restore

[Pawan Gupta]

On Tue, Nov 08, 2022 at 07:55:04PM +0100, Borislav Petkov wrote:
>On Tue, Nov 08, 2022 at 07:26:43PM +0100, Borislav Petkov wrote:
>> Will take a look tomorrow.
>
>Yap, needs a new revision.

Sure, I will revise this.