Check enumeration before MSR save/restore

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/[email protected]/

Quoted 5 lines · 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.

Quoted 6 lines

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/[email protected]/

Added the correct email-id of Hans de Goede <[email protected]>.

Hi,

Quoted 11 lines · Pawan Gupta

Quoted 10 lines · 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.

Quoted 3 lines

Quoted 1 line

[1] https://lore.kernel.org/lkml/[email protected]/

Added the correct email-id of Hans de Goede <[email protected]>.

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 <[email protected]>

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/[email protected]/

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 ?

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.

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

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

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 <[email protected]>

Signed-off-by Pawan Gupta <[email protected]>

---
 arch/x86/include/asm/cpufeatures.h | 1 +
 arch/x86/kernel/cpu/amd.c          | 3 +++
 2 files changed, 4 insertions(+)

arch/x86/include/asm/cpufeatures.h +0 -0

index dd173733e40d..90bdb1d98531 100644

Will take a look tomorrow.

Quoted 4 lines · 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.

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

Quoted 3 lines

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?

Quoted 1 line · Borislav Petkov

Will take a look tomorrow.

Yap, needs a new revision.

Quoted 3 lines · 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 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.

Quoted 17 lines · 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 <[email protected]>

Signed-off-by Pawan Gupta <[email protected]>

---
 arch/x86/include/asm/cpufeatures.h | 1 +
 arch/x86/kernel/cpu/amd.c          | 3 +++
 2 files changed, 4 insertions(+)

arch/x86/include/asm/cpufeatures.h +0 -0

index dd173733e40d..90bdb1d98531 100644

arch/x86/include/asm/cpufeatures.h line 308

Quoted 4 lines

arch/x86/include/asm/cpufeatures.h +1 -0

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

Quoted 3 lines · Pawan Gupta

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?

Quoted 3 lines · Pawan Gupta

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?

Quoted 10 lines · Borislav Petkov

Quoted 3 lines · Pawan Gupta

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.

Quoted 1 line

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.

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.

Quoted 1 line · Pawan Gupta

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?

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.

Quoted 11 lines · Andrew Cooper

Quoted 10 lines · Borislav Petkov

Quoted 3 lines · Pawan Gupta

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.

Quoted 8 lines

Quoted 1 line

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.

Quoted 2 lines · Pawan Gupta

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.