Skip to contentSkip to conversation

x86/intel_rdt: use rdmsr_safe() to workaround AWS host issue

PATCH In review Vitaly Kuznetsov 3 participants 9 messages Dec 20, 2018 – Jan 10, 2019 2 messages not in the archive

Subsystems (2)
x86 RDT - resource allocation
lore
Share & tools
lore

Vitaly Kuznetsov

author
#0 lore raw

PATCH 1/1 x86/intel_rdt: use rdmsr_safe() to workaround AWS host issue

It was found that AWS x1 instances (Xen-based) lack xen.git commit 1f1d183d4900 (x86/HVM: don't give the wrong impression of WRMSR succeeding) and because of that the wrmsr_safe() check in cache_alloc_hsw_probe() doesn't help: the consequent rdmsr() blows up with

unchecked MSR access error: RDMSR from 0xc90 at rIP:
0xffffffff88c5bba3 (native_read_msr+0x3/0x30)

The issue should definitely get fixed on AWS side. We can, however, simply workaround this in Linux and live happily after.

Signed-off-by Vitaly Kuznetsov <[email protected]>
---
 arch/x86/kernel/cpu/intel_rdt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
arch/x86/kernel/cpu/intel_rdt.c +2 -1
index 44272b7107ad..0acee6cd07a8 100644
@@ -213,7 +213,8 @@ static inline void cache_alloc_hsw_probe(void)
 
 	if (wrmsr_safe(IA32_L3_CBM_BASE, max_cbm, 0))
 		return;
-	rdmsr(IA32_L3_CBM_BASE, l, h);
+	if (rdmsr_safe(IA32_L3_CBM_BASE, &l, &h))
+		return;
 
 	/* If all the bits were set in MSR, return success */
 	if (l != max_cbm)
Signature · 1 line
2.19.2
maintainer
#1 lore raw
Quoted 22 lines · Vitaly Kuznetsov

It was found that AWS x1 instances (Xen-based) lack xen.git commit 1f1d183d4900 (x86/HVM: don't give the wrong impression of WRMSR succeeding) and because of that the wrmsr_safe() check in cache_alloc_hsw_probe() doesn't help: the consequent rdmsr() blows up with

unchecked MSR access error: RDMSR from 0xc90 at rIP:

   0xffffffff88c5bba3 (native_read_msr+0x3/0x30)

The issue should definitely get fixed on AWS side. We can, however, simply workaround this in Linux and live happily after.

Signed-off-by Vitaly Kuznetsov <[email protected]>
---
 arch/x86/kernel/cpu/intel_rdt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
arch/x86/kernel/cpu/intel_rdt.c +0 -0
index 44272b7107ad..0acee6cd07a8 100644

arch/x86/kernel/cpu/intel_rdt.c static inline void cache_alloc_hsw_probe(void) · line 220

Quoted 9 lines
arch/x86/kernel/cpu/intel_rdt.c +2 -1
@@ -213,7 +213,8 @@ static inline void cache_alloc_hsw_probe(void)
 
 	if (wrmsr_safe(IA32_L3_CBM_BASE, max_cbm, 0))
 		return;
-	rdmsr(IA32_L3_CBM_BASE, l, h);
+	if (rdmsr_safe(IA32_L3_CBM_BASE, &l, &h))
+		return;
 
 	/* If all the bits were set in MSR, return success */
 	if (l != max_cbm)

Does the below hunk work too?

Reinette, Babu: is there any reason for rdt_init_res_defs() and check_quirks() to run before get_rdt_resources() which looks at CPUID bits?

If not, we need to do something like below, for guests.

Thx.

---

arch/x86/kernel/cpu/resctrl/core.c +3 -7
index c3a9dc63edf2..64cb0fb31862 100644
@@ -969,17 +969,13 @@ static int __init resctrl_late_init(void)
 	struct rdt_resource *r;
 	int state, ret;
 
-	/*
-	 * Initialize functions(or definitions) that are different
-	 * between vendors here.
-	 */
+	if (!get_rdt_resources())
+		return -ENODEV;
+
 	rdt_init_res_defs();
 
 	check_quirks();
 
-	if (!get_rdt_resources())
-		return -ENODEV;
-
 	rdt_init_padding();
 
 	state = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
Signature · 4 lines
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
author
#2 lore raw
Quoted 29 lines · Borislav Petkov
Quoted 28 lines · Vitaly Kuznetsov

It was found that AWS x1 instances (Xen-based) lack xen.git commit 1f1d183d4900 (x86/HVM: don't give the wrong impression of WRMSR succeeding) and because of that the wrmsr_safe() check in cache_alloc_hsw_probe() doesn't help: the consequent rdmsr() blows up with

unchecked MSR access error: RDMSR from 0xc90 at rIP:

   0xffffffff88c5bba3 (native_read_msr+0x3/0x30)

The issue should definitely get fixed on AWS side. We can, however, simply workaround this in Linux and live happily after.

Signed-off-by Vitaly Kuznetsov <[email protected]>
---
 arch/x86/kernel/cpu/intel_rdt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
arch/x86/kernel/cpu/intel_rdt.c +2 -1
index 44272b7107ad..0acee6cd07a8 100644
@@ -213,7 +213,8 @@ static inline void cache_alloc_hsw_probe(void)
 
 	if (wrmsr_safe(IA32_L3_CBM_BASE, max_cbm, 0))
 		return;
-	rdmsr(IA32_L3_CBM_BASE, l, h);
+	if (rdmsr_safe(IA32_L3_CBM_BASE, &l, &h))
+		return;
Quoted 6 lines

arch/x86/kernel/cpu/intel_rdt.c static inline void cache_alloc_hsw_probe(void) · line 220

Quoted 3 lines
arch/x86/kernel/cpu/intel_rdt.c +0 -0
 	/* If all the bits were set in MSR, return success */
 	if (l != max_cbm)

Does the below hunk work too?

Yes, it does, thanks!

Signature · 1 line
Vitaly

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

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

maintainer
#5 lore raw
Quoted 7 lines · Reinette Chatre

To clarify please the last sentence applies to real Haswell server CPUs (not virtualized as prompted this work, my apologies for the confusion) that support Intel RDT but does not have CPUID enumeration for this feature. With Vitaly's patch this hardware would still be detected as supporting CAT but if CPUID enumeration is moved earlier then from what I can tell this hardware would be considered as not supporting the feature anymore.

Ok, so hw "forgot" to add CPUID again. And CPU guys should know better but everytime we hear "important reasons" why they dropped the ball there.

So, assuming RDT is not going to be supported in a guest, we need a proper fix to disable it when in a guest. So the RDT init path needs something like this then:

---

arch/x86/kernel/cpu/resctrl/core.c +3 -0
index c3a9dc63edf2..1e5a1cb49e9c 100644
@@ -969,6 +969,9 @@ static int __init resctrl_late_init(void)
 	struct rdt_resource *r;
 	int state, ret;
 
+	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
+		return -ENODEV;
+
 	/*
 	 * Initialize functions(or definitions) that are different
 	 * between vendors here.
Signature · 4 lines
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
author
#6 lore raw
Quoted 1 line · Borislav Petkov

So, assuming RDT is not going to be supported in a guest

Hm, why is that? In theory, hypervisors can pass through or emulate the required MSRs...

Signature · 1 line
Vitaly
maintainer
#7 lore raw
Quoted 2 lines · Vitaly Kuznetsov

Hm, why is that? In theory, hypervisors can pass through or emulate the required MSRs...

...and when the theory becomes reality we'll remove the check.

Signature · 4 lines
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
#8 lore raw
Quoted 6 lines · Borislav Petkov
Quoted 2 lines · Vitaly Kuznetsov

Hm, why is that? In theory, hypervisors can pass through or emulate the required MSRs...

...and when the theory becomes reality we'll remove the check.

In practice that may be a long time coming. We don't have many CLOSIDs, or bits in a cache mask, at the h/w level. If you start trying to subdivide those resources
to pass a subset to a guest, then you'll quickly find that you have no flexibility in the
guest to do anything useful. It would only work if you limited to two, or perhaps three
guests.

-Tony

author
#9 lore raw
Quoted 6 lines · Tony Luck
Quoted 5 lines · Borislav Petkov
Quoted 2 lines · Vitaly Kuznetsov

Hm, why is that? In theory, hypervisors can pass through or emulate the required MSRs...

Quoted 8 lines
Quoted 1 line

...and when the theory becomes reality we'll remove the check.

In practice that may be a long time coming. We don't have many CLOSIDs, or bits in a cache mask, at the h/w level. If you start trying to subdivide those resources to pass a subset to a guest, then you'll quickly find that you have no flexibility in the guest to do anything useful. It would only work if you limited to two, or perhaps three guests.

Running a single guest on a physical CPU is a very common scenario. In fact, sharing cores is very rare for public clouds: e.g. all worthy instance types on AWS/Azure give you dedicated cores and I don't see why hypervisor can't pass through resctl features.

The other thing is: how can we be sure that there's no hypervisor exposing these feature already? Even if open-source hypervisors like KVM/Xen don't do it it doesn't prove anything: there are numerous proprietary hypervisors and who knows what they do.

The original issue which triggered the discussion was discovered on AWS Xen where the host is buggy and I suggested a simple short-term workaround, I'm no expert in rdt/qos so I'm leaving this up to the maintainers to decide which fix deserves to go in (if any).

Signature · 1 line
Vitaly
maintainer
#10 lore raw
Quoted 4 lines · Vitaly Kuznetsov

The other thing is: how can we be sure that there's no hypervisor exposing these feature already? Even if open-source hypervisors like KVM/Xen don't do it it doesn't prove anything: there are numerous proprietary hypervisors and who knows what they do.

Well, we have this thing called CPUID which enumerates features - regardless of running on baremetal or on a hypervisor. But apparently some Intel CPUs dropped the ball there. Which caused the f*ckup we are in right now.

If you really want to not foreclose that feature for guests - and it sounds like you do - the other thing I can think of is an early quirk *setting* those feature bits for those Intel CPUs which "forgot" to set them and then changing the resctrl code to test CPUID bits first.

This way, the kernel is presented with a unified view on what is supported by the underlying machine - and that machine can be a HV or baremetal - kernel doesn't care.

Quoted 3 lines

The original issue which triggered the discussion was discovered on AWS Xen where the host is buggy and I suggested a simple short-term workaround

And I'm sick'n'tired of simple-short term workarounds and band-aids and the kernel always bending because people dropped the ball and those being perpetuated, because, sure, we'll add one more "fix" on the pile, who cares... :-\

If AWS xen is broken, then that should be fixed - not the kernel.

Signature · 4 lines
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.