KVM: X86: Allow userspace to define the microcode version

From: Wanpeng Li <[email protected]>

Linux (among the others) has checks to make sure that certain features aren't enabled on a certain family/model/stepping if the microcode version isn't greater than or equal to a known good version.

By exposing the real microcode version, we're preventing buggy guests that

Quoted 7 lines

From: Wanpeng Li <[email protected]>

Linux (among the others) has checks to make sure that certain features aren't enabled on a certain family/model/stepping if the microcode version isn't greater than or equal to a known good version.

By exposing the real microcode version, we're preventing buggy guests that

Where do we prevent userspace from coming up with some non-sensical microcode revision?

I think it is the host admin(e.g. cloud provider)'s responsibility to set an expected microcode revision.

In addition, the non-sensical value which is written by the guest will not reflect to guest-visible microcode revision and just be ignored in this implementation.

Linux (among the others) has checks to make sure that certain features aren't enabled on a certain family/model/stepping if the microcode version isn't greater than or equal to a known good version.

Quoted 2 lines

I think it is the host admin(e.g. cloud provider)'s responsibility to set an expected microcode revision.

+ vcpu->arch.microcode_version = 0x1;

That already looks pretty arbitrary and non-sensical to me.

Quoted 4 lines

In addition, the non-sensical value which is written by the guest will not reflect to guest-visible microcode revision and just be ignored in this implementation.

Huh? How so?

So a guest will have *two* microcode revisions - both of which are most likely wrong?!

This whole thing sounds like the wrong approach to me.

Quoted 3 lines

Linux (among the others) has checks to make sure that certain features aren't enabled on a certain family/model/stepping if the microcode version isn't greater than or equal to a known good version.

It sounds to me like the proper fix is to make the kernel *not* look at microcode revisions when running virtualized. The same way we're not loading microcode in a guest:

        if (native_cpuid_ecx(1) & BIT(31))

Letting userspace control the microcode revision number is revision number management SNAFU waiting to happen IMO.

Quoted 2 lines

So a guest will have *two* microcode revisions - both of which are most likely wrong?!

Just one revision.

https://lkml.org/lkml/2017/12/9/29 The original discussion explain in more details.

Quoted 4 lines

Quoted 2 lines

So a guest will have *two* microcode revisions - both of which are most likely wrong?!

Just one revision.

So what does "the non-sensical value which is written by the guest will not reflect to guest-visible microcode revision" even mean then?

cat /proc/cpuinfo

in the guest shows what exactly?

And what would RDMSR 0x8b show then?

Quoted 3 lines

https://lkml.org/lkml/2017/12/9/29 The original discussion explain in more details.

approach. Instead, the kernel should not look at microcode revisions if it runs virtualized.

Both are the same values set by kvm userspace.

This is correct. The link explains why the userspace sets microcode revision is still needed.

Quoted 1 line

Both are the same values set by kvm userspace.

This still doesn't answer my question what "the non-sensical value which is written by the guest will not reflect to guest-visible microcode revision" means?

Quoted 3 lines

This is correct. The link explains why the userspace sets microcode revision is still needed.

Why is it still needed?

The guest write is ignored as the original kvm implementation before the patch.

Hmm, the apic_check_deadline_errata() example can be referred to.

Quoted 1 line

The guest write is ignored as the original kvm implementation before the patch.

That will never work because there's no virtualized microcode loader. Which will be a dumb idea anyway.

Goes to show that dealing with microcode revisions for a guest is the wrong approach.

Quoted 1 line

Hmm, the apic_check_deadline_errata() example can be referred to.

So that's basically what I'm saying - fix apic_check_deadline_errata() to check whether the kernel runs as a guest.

Quoted 1 line

The guest write is ignored as the original kvm implementation before the patch.

That will never work because there's no virtualized microcode loader. Which will be a dumb idea anyway.

Goes to show that dealing with microcode revisions for a guest is the wrong approach.

So that's basically what I'm saying - fix apic_check_deadline_errata() to check whether the kernel runs as a guest.

I don't understand how one thing follows from the other. How are writes to 0x8B related to having a virtualized microcode loaded (which is a concept that actually makes no sense at all)?

It has already been fixed for a few months, and fixing it is indeed the right thing to do independent of this patch.

Quoted 3 lines · Paolo Bonzini

I don't understand how one thing follows from the other. How are writes to 0x8B related to having a virtualized microcode loaded (which is a concept that actually makes no sense at all)?

I'm questioning the whole idea. 0x8b is the MSR which gives you the microcode revision. Most CPUs don't even allow writing to it, AFAICT. (SDM says "may prevent writing" on VM transitions.)

So how is that host-initiated write to 0x8b is even going to work, in reality? kvm module writes the microcode version in there? How does the admin work around that?

Quoted 2 lines

It has already been fixed for a few months, and fixing it is indeed the right thing to do independent of this patch.

Yap.

Quoted 9 lines · Borislav Petkov

Quoted 3 lines · Paolo Bonzini

I don't understand how one thing follows from the other. How are writes to 0x8B related to having a virtualized microcode loaded (which is a concept that actually makes no sense at all)?

I'm questioning the whole idea. 0x8b is the MSR which gives you the microcode revision. Most CPUs don't even allow writing to it, AFAICT. (SDM says "may prevent writing" on VM transitions.)

Quoted 3 lines

So how is that host-initiated write to 0x8b is even going to work, in reality? kvm module writes the microcode version in there? How does the admin work around that?

In this context, "host-initiated" write means written by KVM userspace with ioctl(KVM_SET_MSR). It generally happens only on VM startup, reset or live migration.

Quoted 3 lines

In this context, "host-initiated" write means written by KVM userspace with ioctl(KVM_SET_MSR). It generally happens only on VM startup, reset or live migration.

To be clear, the target of the write is still the vCPU's emulated MSR.

Quoted 5 lines · Paolo Bonzini

Quoted 3 lines

In this context, "host-initiated" write means written by KVM userspace with ioctl(KVM_SET_MSR). It generally happens only on VM startup, reset or live migration.

To be clear, the target of the write is still the vCPU's emulated MSR.

So how am I to imagine this as a user:

qemu-system-x86_64 --microcode-revision=0xdeadbeef...

More like "-cpu foo,ucode_rev=0xdeadbeef". But in practice what would happen is one of the following:

1) "-cpu host" sets ucode_rev to the same value of the host, everyone else leaves it to zero as is now.

2) Only Amazon uses this feature and we ignore it. :)

Quoted 9 lines · Borislav Petkov

Quoted 5 lines · Paolo Bonzini

Quoted 3 lines

In this context, "host-initiated" write means written by KVM userspace with ioctl(KVM_SET_MSR). It generally happens only on VM startup, reset or live migration.

To be clear, the target of the write is still the vCPU's emulated MSR.

So how am I to imagine this as a user:

Quoted 1 line

qemu-system-x86_64 --microcode-revision=0xdeadbeef...

More like "-cpu foo,ucode_rev=0xdeadbeef". But in practice what would happen is one of the following:

1) "-cpu host" sets ucode_rev to the same value of the host, everyone else leaves it to zero as is now.

2) Only Amazon uses this feature and we ignore it. :)

From 36ea81363c38942057b006ccaf2ef26708a894bb Mon Sep 17 00:00:00 2001

Paolo

Perhaps both ideas should be done? The one Boris suggested (See below, not compile tested yet), and also this one? Keep in mind that other hypervisor offerings (say Xen, VMWare, etc) may have provided the correct microcode or not and it would be good for the OS to be comfortable running under them.

From 36ea81363c38942057b006ccaf2ef26708a894bb Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <[email protected]>
Date: Mon, 26 Feb 2018 09:35:01 -0500
Subject: [PATCH] x86/spectre_v2: Don't check bad microcode versions when
running under hypervisors.

As:
1) We know they lie about the env anyhow (host mismatch)
2) Even if the hypervisor (Xen, KVM, VMWare, etc) provided a valid "correct" value, it all gets to be very murky when migration happens (do you provide the "new" microcode of the machine?).

And in reality the cloud vendors are the ones that should make sure that the microcode that is running is correct and we should just sing lalalala and believe them.

Signed-off-by Konrad Rzeszutek Wilk <[email protected]>

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

arch/x86/kernel/cpu/intel.c +7 -0

index d19e903214b4..87d044ce837f 100644
@@ -144,6 +144,13 @@ static bool bad_spectre_microcode(struct cpuinfo_x86 *c)
 {
 	int i;
 
+	/*
+	 * We know that the hypervisor lie to us on the microcode version so
+	 * we may as well trust that it is running the correct version.
+	 */
+	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
+		return false;
+
 	for (i = 0; i < ARRAY_SIZE(spectre_bad_microcodes); i++) {
 		if (c->x86_model == spectre_bad_microcodes[i].model &&
 		    c->x86_stepping == spectre_bad_microcodes[i].stepping)

arch/x86/kernel/cpu/intel.c static bool bad_spectre_microcode(struct cpuinfo_x86 *c) · line 151

Quoted 13 lines · Konrad Rzeszutek Wilk

arch/x86/kernel/cpu/intel.c +5 -0

index d19e903214b4..87d044ce837f 100644
@@ -144,6 +144,13 @@ static bool bad_spectre_microcode(struct cpuinfo_x86 *c)
 {
 	int i;
 
+	/*
+	 * We know that the hypervisor lie to us on the microcode version so
+	 * we may as well trust that it is running the correct version.
+	 */
+	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))

I guess

cpu_has(c, X86_FEATURE_HYPERVISOR)

since we're passing a ptr to the current CPU.

Quoted 17 lines · Borislav Petkov

arch/x86/kernel/cpu/intel.c static bool bad_spectre_microcode(struct cpuinfo_x86 *c) · line 151

Quoted 13 lines · Konrad Rzeszutek Wilk

arch/x86/kernel/cpu/intel.c +5 -0

index d19e903214b4..87d044ce837f 100644
@@ -144,6 +144,13 @@ static bool bad_spectre_microcode(struct cpuinfo_x86 *c)
 {
 	int i;
 
+	/*
+	 * We know that the hypervisor lie to us on the microcode version so
+	 * we may as well trust that it is running the correct version.
+	 */
+	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))

I guess

Quoted 3 lines

cpu_has(c, X86_FEATURE_HYPERVISOR)

since we're passing a ptr to the current CPU.

Ah yes. Let me fix it up and repost.

From 6abac2ccf105d57d60c094950e32139e435cbefe Mon Sep 17 00:00:00 2001

Quoted 18 lines · Konrad Rzeszutek Wilk

Quoted 17 lines · Borislav Petkov

arch/x86/kernel/cpu/intel.c static bool bad_spectre_microcode(struct cpuinfo_x86 *c) · line 151

Quoted 13 lines · Konrad Rzeszutek Wilk

arch/x86/kernel/cpu/intel.c +5 -0

index d19e903214b4..87d044ce837f 100644
@@ -144,6 +144,13 @@ static bool bad_spectre_microcode(struct cpuinfo_x86 *c)
 {
 	int i;
 
+	/*
+	 * We know that the hypervisor lie to us on the microcode version so
+	 * we may as well trust that it is running the correct version.
+	 */
+	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))

I guess

Quoted 5 lines

Quoted 3 lines

cpu_has(c, X86_FEATURE_HYPERVISOR)

since we're passing a ptr to the current CPU.

Ah yes. Let me fix it up and repost.

I've posted it (but I can't seem to find it on LKML). Here it is in this thread. Also adding ingo + tglrx

From 6abac2ccf105d57d60c094950e32139e435cbefe Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <[email protected]>
Date: Mon, 26 Feb 2018 09:35:01 -0500
Subject: [PATCH v2] x86/spectre_v2: Don't check bad microcode versions when
running under hypervisors.

As:
1) We know they lie about the env anyhow (host mismatch)
2) Even if the hypervisor (Xen, KVM, VMWare, etc) provided a valid "correct" value, it all gets to be very murky when migration happens (do you provide the "new" microcode of the machine?).

And in reality the cloud vendors are the ones that should make sure that the microcode that is running is correct and we should just sing lalalala and trust them.

CC [email protected]

CC Ingo Molnar <[email protected]>

CC "H. Peter Anvin" <[email protected]>

CC [email protected]

Cc Tom Lendacky <[email protected]>

Cc Andi Kleen <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Masami Hiramatsu <[email protected]>

Cc Arjan van de Ven <[email protected]>

Cc David Woodhouse <[email protected]>

Signed-off-by Konrad Rzeszutek Wilk <[email protected]>

---
v2: Change comments to be more in line with the state of the world.
v3: Use cpu_has instead of boot_cpu_has per Borislav's review.

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

arch/x86/kernel/cpu/intel.c +7 -0

index d19e903214b4..4aa9fd379390 100644
@@ -144,6 +144,13 @@ static bool bad_spectre_microcode(struct cpuinfo_x86 *c)
 {
 	int i;
 
+	/*
+	 * We know that the hypervisor lie to us on the microcode version so
+	 * we may as well hope that it is running the correct version.
+	 */
+	if (cpu_has(c, X86_FEATURE_HYPERVISOR))
+		return false;
+
 	for (i = 0; i < ARRAY_SIZE(spectre_bad_microcodes); i++) {
 		if (c->x86_model == spectre_bad_microcodes[i].model &&
 		    c->x86_stepping == spectre_bad_microcodes[i].stepping)

Quoted 9 lines · Paolo Bonzini

Quoted 5 lines

Quoted 3 lines

In this context, "host-initiated" write means written by KVM userspace with ioctl(KVM_SET_MSR). It generally happens only on VM startup, reset or live migration.

To be clear, the target of the write is still the vCPU's emulated MSR.

So how am I to imagine this as a user:

Quoted 1 line

qemu-system-x86_64 --microcode-revision=0xdeadbeef...

More like "-cpu foo,ucode_rev=0xdeadbeef". But in practice what would happen is one of the following:

1) "-cpu host" sets ucode_rev to the same value of the host, everyone else leaves it to zero as is now.

Cc Eduardo,
2018-02-26 20:41 GMT+08:00 Paolo Bonzini <[email protected]>:

Quoted 15 lines · Borislav Petkov

Quoted 10 lines · Paolo Bonzini

Quoted 5 lines

Quoted 3 lines

In this context, "host-initiated" write means written by KVM userspace with ioctl(KVM_SET_MSR). It generally happens only on VM startup, reset or live migration.

To be clear, the target of the write is still the vCPU's emulated MSR.

So how am I to imagine this as a user:

qemu-system-x86_64 --microcode-revision=0xdeadbeef...

More like "-cpu foo,ucode_rev=0xdeadbeef". But in practice what would happen is one of the following:

Quoted 2 lines

1) "-cpu host" sets ucode_rev to the same value of the host, everyone else leaves it to zero as is now.

Hi Paolo,

Do you mean the host admin to get the ucode_rev from the host and set to -cpu host, ucode_rev=xxxxxx or qemu get the ucode_rev directly by rdmsr?

Quoted 22 lines

Cc Eduardo,
2018-02-26 20:41 GMT+08:00 Paolo Bonzini <[email protected]>:

Quoted 10 lines · Borislav Petkov

Quoted 9 lines · Paolo Bonzini

Quoted 5 lines

Quoted 3 lines

In this context, "host-initiated" write means written by KVM userspace with ioctl(KVM_SET_MSR). It generally happens only on VM startup, reset or live migration.

To be clear, the target of the write is still the vCPU's emulated MSR.

So how am I to imagine this as a user:

Quoted 7 lines

Quoted 1 line

qemu-system-x86_64 --microcode-revision=0xdeadbeef...

More like "-cpu foo,ucode_rev=0xdeadbeef". But in practice what would happen is one of the following:

1) "-cpu host" sets ucode_rev to the same value of the host, everyone else leaves it to zero as is now.

Hi Paolo,

Quoted 3 lines

Do you mean the host admin to get the ucode_rev from the host and set to -cpu host, ucode_rev=xxxxxx or qemu get the ucode_rev directly by rdmsr?

QEMU setting ucode_rev automatically using the host value when using "-cpu host" (with no need for explicit ucode_rev option) makes sense to me.

2018-04-18 4:24 GMT+08:00 Eduardo Habkost <[email protected]>:

Quoted 27 lines · Wanpeng Li

Quoted 25 lines

Cc Eduardo,
2018-02-26 20:41 GMT+08:00 Paolo Bonzini <[email protected]>:

Quoted 10 lines · Borislav Petkov

Quoted 9 lines · Paolo Bonzini

Quoted 5 lines

Quoted 3 lines

In this context, "host-initiated" write means written by KVM userspace with ioctl(KVM_SET_MSR). It generally happens only on VM startup, reset or live migration.

To be clear, the target of the write is still the vCPU's emulated MSR.

So how am I to imagine this as a user:

Quoted 7 lines

Quoted 1 line

qemu-system-x86_64 --microcode-revision=0xdeadbeef...

More like "-cpu foo,ucode_rev=0xdeadbeef". But in practice what would happen is one of the following:

1) "-cpu host" sets ucode_rev to the same value of the host, everyone else leaves it to zero as is now.

Hi Paolo,

Do you mean the host admin to get the ucode_rev from the host and set to -cpu host, ucode_rev=xxxxxx or qemu get the ucode_rev directly by rdmsr?

Quoted 3 lines

QEMU setting ucode_rev automatically using the host value when using "-cpu host" (with no need for explicit ucode_rev option) makes sense to me.

QEMU can't get the host value by rdmsr MSR_IA32_UCODE_REV directly since rdmsr will #GP when ring !=0, any idea?

Quoted 5 lines

Quoted 3 lines

QEMU setting ucode_rev automatically using the host value when using "-cpu host" (with no need for explicit ucode_rev option) makes sense to me.

QEMU can't get the host value by rdmsr MSR_IA32_UCODE_REV directly since rdmsr will #GP when ring !=0, any idea?

By looking at kvm_get_msr_feature(), it looks like ioctl(system_fd, KVM_GET_MSRS) would return the host MSR value for us.

Quoted 3 lines · Eduardo Habkost

Quoted 3 lines

Quoted 3 lines

QEMU setting ucode_rev automatically using the host value when using "-cpu host" (with no need for explicit ucode_rev option) makes sense to me.

Quoted 5 lines

Quoted 2 lines

QEMU can't get the host value by rdmsr MSR_IA32_UCODE_REV directly since rdmsr will #GP when ring !=0, any idea?

By looking at kvm_get_msr_feature(), it looks like ioctl(system_fd, KVM_GET_MSRS) would return the host MSR value for us.

Yes, that's exactly what it was introduced for (together with other MSRs including VMX capabilities).

Quoted 10 lines · Paolo Bonzini

Quoted 8 lines · Eduardo Habkost

Quoted 5 lines

Quoted 3 lines

QEMU setting ucode_rev automatically using the host value when using "-cpu host" (with no need for explicit ucode_rev option) makes sense to me.

QEMU can't get the host value by rdmsr MSR_IA32_UCODE_REV directly since rdmsr will #GP when ring !=0, any idea?

By looking at kvm_get_msr_feature(), it looks like ioctl(system_fd, KVM_GET_MSRS) would return the host MSR value for us.

Quoted 2 lines

Yes, that's exactly what it was introduced for (together with other MSRs including VMX capabilities).

Can't qemu do:

grep microcode /proc/cpuinfo | awk '{ print $3 }' | head -n 1

Quoted 14 lines · Borislav Petkov

Quoted 12 lines · Paolo Bonzini

Quoted 8 lines · Eduardo Habkost

Quoted 5 lines

Quoted 3 lines

QEMU setting ucode_rev automatically using the host value when using "-cpu host" (with no need for explicit ucode_rev option) makes sense to me.

QEMU can't get the host value by rdmsr MSR_IA32_UCODE_REV directly since rdmsr will #GP when ring !=0, any idea?

By looking at kvm_get_msr_feature(), it looks like ioctl(system_fd, KVM_GET_MSRS) would return the host MSR value for us.

Yes, that's exactly what it was introduced for (together with other MSRs including VMX capabilities).

Quoted 3 lines

Can't qemu do:

grep microcode /proc/cpuinfo | awk '{ print $3 }' | head -n 1

It could, but why would QEMU do it if a real API already exists for that?

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

QEMU can't get the host value by rdmsr MSR_IA32_UCODE_REV directly since rdmsr will #GP when ring !=0, any idea?

By looking at kvm_get_msr_feature(), it looks like ioctl(system_fd, KVM_GET_MSRS) would return the host MSR value for us.

Quoted 2 lines

Yes, that's exactly what it was introduced for (together with other MSRs including VMX capabilities).

Can't qemu do:

grep microcode /proc/cpuinfo | awk '{ print $3 }' | head -n 1

?

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

QEMU setting ucode_rev automatically using the host value when using "-cpu host" (with no need for explicit ucode_rev option) makes sense to me.

QEMU can't get the host value by rdmsr MSR_IA32_UCODE_REV directly since rdmsr will #GP when ring !=0, any idea?

By looking at kvm_get_msr_feature(), it looks like ioctl(system_fd, KVM_GET_MSRS) would return the host MSR value for us.

Yes, that's exactly what it was introduced for (together with other MSRs including VMX capabilities).

2018-04-18 18:36 GMT+08:00 Paolo Bonzini <[email protected]>:

Quoted 10 lines · Eduardo Habkost

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

QEMU setting ucode_rev automatically using the host value when using "-cpu host" (with no need for explicit ucode_rev option) makes sense to me.

QEMU can't get the host value by rdmsr MSR_IA32_UCODE_REV directly since rdmsr will #GP when ring !=0, any idea?

By looking at kvm_get_msr_feature(), it looks like ioctl(system_fd, KVM_GET_MSRS) would return the host MSR value for us.

Quoted 2 lines

Yes, that's exactly what it was introduced for (together with other MSRs including VMX capabilities).

How about the live migration? What will happen if the source and destination machines have different microcode version?

Regards,
Wanpeng Li

You would need to include the microcode version in the migration stream.

But this brings another point - what if we want to manifest certain new CPUID bits?

Quoted 1 line · Konrad Rzeszutek Wilk

You would need to include the microcode version in the migration

stream.

Quoted 1 line

Quoted 2 lines

But this brings another point - what if we want to manifest certain new CPUID bits?

You don't do that across migration. Generally if you want to do live migration and you set up the guest to know everything about the host (down to the microcode level), you should make sure your host are pretty
much identical.

Paolo

Quoted 7 lines · Paolo Bonzini

Quoted 5 lines

Quoted 3 lines

In this context, "host-initiated" write means written by KVM userspace with ioctl(KVM_SET_MSR). It generally happens only on VM startup, reset or live migration.

To be clear, the target of the write is still the vCPU's emulated MSR.

Quoted 3 lines

So how am I to imagine this as a user:

qemu-system-x86_64 --microcode-revision=0xdeadbeef...

More like "-cpu foo,ucode_rev=0xdeadbeef". But in practice what would happen is one of the following:

1) "-cpu host" sets ucode_rev to the same value of the host, everyone

Quoted 2 lines

I think it is the host admin(e.g. cloud provider)'s responsibility to set an expected microcode revision.

+ vcpu->arch.microcode_version = 0x1;

That already looks pretty arbitrary and non-sensical to me.

Quoted 3 lines

In addition, the non-sensical value which is written by the guest will not reflect to guest-visible microcode revision and just be ignored in this implementation.

Huh? How so?

So a guest will have *two* microcode revisions - both of which are most likely wrong?!

It's actually 0x100000000.

Actually I think this patch makes sense, since some errata actually can be worked around in the guest in the same way as the host. However, it should also be tied to the recently introduced mechanism to read MSR contents from the host.