[PATCH] KVM: X86: Allow userspace to define the microcode version

[PATCH] KVM: X86: Allow userspace to define the microcode version

[Wanpeng Li]

From: Wanpeng Li <wanpengli@tencent.com>

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
don't check that they are running virtualized (i.e., they should trust the
hypervisor) from disabling features that are effectively not buggy.

Suggested-by: Filippo Sironi <sironi@amazon.de>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
 arch/x86/include/asm/kvm_host.h | 1 +
 arch/x86/kvm/x86.c              | 8 ++++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 938d453..6e13f2f 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -507,6 +507,7 @@ struct kvm_vcpu_arch {
 	u64 smi_count;
 	bool tpr_access_reporting;
 	u64 ia32_xss;
+	u32 microcode_version;
 
 	/*
 	 * Paging state of the vcpu
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1a3ed81..cc51c61 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2247,7 +2247,6 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 
 	switch (msr) {
 	case MSR_AMD64_NB_CFG:
-	case MSR_IA32_UCODE_REV:
 	case MSR_IA32_UCODE_WRITE:
 	case MSR_VM_HSAVE_PA:
 	case MSR_AMD64_PATCH_LOADER:
@@ -2255,6 +2254,10 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_AMD64_DC_CFG:
 		break;
 
+	case MSR_IA32_UCODE_REV:
+		if (msr_info->host_initiated)
+			vcpu->arch.microcode_version = data >> 32;
+		break;
 	case MSR_EFER:
 		return set_efer(vcpu, data);
 	case MSR_K7_HWCR:
@@ -2550,7 +2553,7 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		msr_info->data = 0;
 		break;
 	case MSR_IA32_UCODE_REV:
-		msr_info->data = 0x100000000ULL;
+		msr_info->data = (u64)vcpu->arch.microcode_version << 32;
 		break;
 	case MSR_MTRRcap:
 	case 0x200 ... 0x2ff:
@@ -8232,6 +8235,7 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 	vcpu->arch.regs_dirty = ~0;
 
 	vcpu->arch.ia32_xss = 0;
+	vcpu->arch.microcode_version = 0x1;
 
 	kvm_x86_ops->vcpu_reset(vcpu, init_event);
 }
-- 
2.7.4

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Borislav Petkov]

On Mon, Feb 26, 2018 at 03:23:58PM +0800, Wanpeng Li wrote:
> From: Wanpeng Li <wanpengli@tencent.com>
> 
> 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?

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Wanpeng Li]

2018-02-26 17:41 GMT+08:00 Borislav Petkov <bp@alien8.de>:
> On Mon, Feb 26, 2018 at 03:23:58PM +0800, Wanpeng Li wrote:
>> From: Wanpeng Li <wanpengli@tencent.com>
>>
>> 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.

Regards,
Wanpeng Li

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Borislav Petkov]

On Mon, Feb 26, 2018 at 06:06:42PM +0800, Wanpeng Li wrote:
> 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.

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

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

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Wanpeng Li]

2018-02-26 18:49 GMT+08:00 Borislav Petkov <bp@alien8.de>:
> On Mon, Feb 26, 2018 at 06:06:42PM +0800, Wanpeng Li wrote:
>> 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.

This is the original kvm implementation before the patch.

>
>>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?!

Just one revision.

>
> This whole thing sounds like the wrong approach to me.
>
>> 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.

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

Regards,
Wanpeng Li

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Borislav Petkov]

On Mon, Feb 26, 2018 at 07:02:29PM +0800, Wanpeng Li wrote:
> > 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?

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

My argument stands: exposing microcode revisions to guests is the wrong
approach. Instead, the kernel should not look at microcode revisions if
it runs virtualized.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Wanpeng Li]

2018-02-26 19:16 GMT+08:00 Borislav Petkov <bp@alien8.de>:
> On Mon, Feb 26, 2018 at 07:02:29PM +0800, Wanpeng Li wrote:
>> > 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?

Both are the same values set by kvm userspace.

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

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

Regards,
Wanpeng Li

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Borislav Petkov]

On Mon, Feb 26, 2018 at 07:25:28PM +0800, Wanpeng Li wrote:
> 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?

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

Why is it still needed?

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Wanpeng Li]

2018-02-26 19:30 GMT+08:00 Borislav Petkov <bp@alien8.de>:
> On Mon, Feb 26, 2018 at 07:25:28PM +0800, Wanpeng Li wrote:
>> 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?

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

>
>> This is correct. The link explains why the userspace sets microcode
>> revision is still needed.
>
> Why is it still needed?

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

Regards,
Wanpeng Li

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Borislav Petkov]

On Mon, Feb 26, 2018 at 07:37:32PM +0800, Wanpeng Li wrote:
> 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.

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

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Wanpeng Li]

2018-02-26 19:44 GMT+08:00 Borislav Petkov <bp@alien8.de>:
> On Mon, Feb 26, 2018 at 07:37:32PM +0800, Wanpeng Li wrote:
>> 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.
>
>> 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.

Both I and the link agree with your opinion. However, it is hard to
fix all the guest images which have already been used by customers in
cloud environment, anyway, this patch supplies an alternative way to
work around by host admin.

Regards,
Wanpeng Li

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Paolo Bonzini]

On 26/02/2018 12:44, Borislav Petkov wrote:
>> 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.

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)?

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

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

Paolo

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Borislav Petkov]

On Mon, Feb 26, 2018 at 12:54:52PM +0100, Paolo Bonzini wrote:
> 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?

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

Yap.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Paolo Bonzini]

On 26/02/2018 13:15, Borislav Petkov wrote:
> On Mon, Feb 26, 2018 at 12:54:52PM +0100, Paolo Bonzini wrote:
>> 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?

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.

Thanks,

Paolo

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

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Paolo Bonzini]

On 26/02/2018 13:16, Paolo Bonzini wrote:
> On 26/02/2018 13:15, Borislav Petkov wrote:
>> On Mon, Feb 26, 2018 at 12:54:52PM +0100, Paolo Bonzini wrote:
>>> 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?
> 
> 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.

Paolo

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Borislav Petkov]

On Mon, Feb 26, 2018 at 01:18:07PM +0100, Paolo Bonzini wrote:
> > 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...

?

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Paolo Bonzini]

On 26/02/2018 13:22, Borislav Petkov wrote:
> On Mon, Feb 26, 2018 at 01:18:07PM +0100, Paolo Bonzini wrote:
>>> 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. :)

Paolo

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Borislav Petkov]

On Mon, Feb 26, 2018 at 01:41:38PM +0100, Paolo Bonzini wrote:
> 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. :)

I fear that that might get misused and we probably should consider some
trivial range checking and each qemu cpu model would have a valid range
or so.

Or we should better do that in kvm_set_msr_common directly... although
if we do it here, it would require kvm knowing about all those different
microcode revisions and qemu cpu models sounds better...

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Konrad Rzeszutek Wilk]

On Mon, Feb 26, 2018 at 01:41:38PM +0100, Paolo Bonzini wrote:
> On 26/02/2018 13:22, Borislav Petkov wrote:
> > On Mon, Feb 26, 2018 at 01:18:07PM +0100, Paolo Bonzini wrote:
> >>> 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.

0x1 you mean.
> 
> 2) Only Amazon uses this feature and we ignore it. :)

And every other vendor.

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 <konrad.wilk@oracle.com>
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 <konrad.wilk@oracle.com>
---
 arch/x86/kernel/cpu/intel.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index d19e903214b4..87d044ce837f 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -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)
-- 
2.13.4

> 
> Paolo
> 

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Paolo Bonzini]

On 26/02/2018 15:39, Konrad Rzeszutek Wilk wrote:
> 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.

Yes, the patch below is definitely a good idea.  Can you send it to
Thomas and Ingo?

Thanks,

Paolo

> From 36ea81363c38942057b006ccaf2ef26708a894bb Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 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.

More "trust" (as the comment says in the code below) than believe.

And perhaps the code would be more accurate if it said "hope" instead of
"trust". :)

Paolo

> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  arch/x86/kernel/cpu/intel.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index d19e903214b4..87d044ce837f 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -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)
> 

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Borislav Petkov]

On Mon, Feb 26, 2018 at 09:39:12AM -0500, Konrad Rzeszutek Wilk wrote:
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index d19e903214b4..87d044ce837f 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -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.

It boils down to the same thing in the end but this is bit nicer and
besides the rest of the code uses cpu_has() too.

Otherwise, yap, this looks like a good idea.

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Konrad Rzeszutek Wilk]

On Mon, Feb 26, 2018 at 08:37:11PM +0100, Borislav Petkov wrote:
> On Mon, Feb 26, 2018 at 09:39:12AM -0500, Konrad Rzeszutek Wilk wrote:
> > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> > index d19e903214b4..87d044ce837f 100644
> > --- a/arch/x86/kernel/cpu/intel.c
> > +++ b/arch/x86/kernel/cpu/intel.c
> > @@ -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.

Ah yes. Let me fix it up and repost.

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Konrad Rzeszutek Wilk]

On Mon, Feb 26, 2018 at 03:51:30PM -0500, Konrad Rzeszutek Wilk wrote:
> On Mon, Feb 26, 2018 at 08:37:11PM +0100, Borislav Petkov wrote:
> > On Mon, Feb 26, 2018 at 09:39:12AM -0500, Konrad Rzeszutek Wilk wrote:
> > > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> > > index d19e903214b4..87d044ce837f 100644
> > > --- a/arch/x86/kernel/cpu/intel.c
> > > +++ b/arch/x86/kernel/cpu/intel.c
> > > @@ -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.
> 
> 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 <konrad.wilk@oracle.com>
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: stable@vger.kernel.org
CC: Ingo Molnar <mingo@redhat.com>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: x86@kernel.org
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Arjan van de Ven <arjan@linux.intel.com>
Cc: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

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

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index d19e903214b4..4aa9fd379390 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -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)
-- 
2.13.4

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Paolo Bonzini]

On 26/02/2018 22:30, Konrad Rzeszutek Wilk wrote:
> On Mon, Feb 26, 2018 at 03:51:30PM -0500, Konrad Rzeszutek Wilk wrote:
>> On Mon, Feb 26, 2018 at 08:37:11PM +0100, Borislav Petkov wrote:
>>> On Mon, Feb 26, 2018 at 09:39:12AM -0500, Konrad Rzeszutek Wilk wrote:
>>>> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
>>>> index d19e903214b4..87d044ce837f 100644
>>>> --- a/arch/x86/kernel/cpu/intel.c
>>>> +++ b/arch/x86/kernel/cpu/intel.c
>>>> @@ -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.
>>
>> 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 <konrad.wilk@oracle.com>
> 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: stable@vger.kernel.org
> CC: Ingo Molnar <mingo@redhat.com>
> CC: "H. Peter Anvin" <hpa@zytor.com>
> CC: x86@kernel.org
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Arjan van de Ven <arjan@linux.intel.com>
> Cc: David Woodhouse <dwmw@amazon.co.uk>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> ---
> 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.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

> ---
>  arch/x86/kernel/cpu/intel.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index d19e903214b4..4aa9fd379390 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -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)
> 

[tip:x86/pti] x86/spectre_v2: Don't check microcode versions when running under hypervisors

[tip-bot for Konrad Rzeszutek Wilk]

Commit-ID:  36268223c1e9981d6cfc33aff8520b3bde4b8114
Gitweb:     https://git.kernel.org/tip/36268223c1e9981d6cfc33aff8520b3bde4b8114
Author:     Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
AuthorDate: Mon, 26 Feb 2018 09:35:01 -0500
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 8 Mar 2018 10:13:02 +0100

x86/spectre_v2: Don't check microcode versions when running under hypervisors

As:

 1) It's known that hypervisors lie about the environment 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.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Cc: Wanpeng Li <kernellwp@gmail.com>
Cc: kvm <kvm@vger.kernel.org>
Cc: Krčmář <rkrcmar@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: stable@vger.kernel.org
Link: https://lkml.kernel.org/r/20180226213019.GE9497@char.us.oracle.com

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

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index d19e903214b4..4aa9fd379390 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -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)

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Wanpeng Li]

Cc Eduardo,
2018-02-26 20:41 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
> On 26/02/2018 13:22, Borislav Petkov wrote:
>> On Mon, Feb 26, 2018 at 01:18:07PM +0100, Paolo Bonzini wrote:
>>>> 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.

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?

Regards,
Wanpeng Li

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Eduardo Habkost]

On Tue, Apr 17, 2018 at 06:40:58PM +0800, Wanpeng Li wrote:
> Cc Eduardo,
> 2018-02-26 20:41 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
> > On 26/02/2018 13:22, Borislav Petkov wrote:
> >> On Mon, Feb 26, 2018 at 01:18:07PM +0100, Paolo Bonzini wrote:
> >>>> 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.
> 
> 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?

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.

-- 
Eduardo

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Wanpeng Li]

2018-04-18 4:24 GMT+08:00 Eduardo Habkost <ehabkost@redhat.com>:
> On Tue, Apr 17, 2018 at 06:40:58PM +0800, Wanpeng Li wrote:
>> Cc Eduardo,
>> 2018-02-26 20:41 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>> > On 26/02/2018 13:22, Borislav Petkov wrote:
>> >> On Mon, Feb 26, 2018 at 01:18:07PM +0100, Paolo Bonzini wrote:
>> >>>> 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.
>>
>> 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?
>
> 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?

Regards,
Wanpeng Li

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Eduardo Habkost]

On Wed, Apr 18, 2018 at 11:24:22AM +0800, Wanpeng Li wrote:
> 2018-04-18 4:24 GMT+08:00 Eduardo Habkost <ehabkost@redhat.com>:
> > On Tue, Apr 17, 2018 at 06:40:58PM +0800, Wanpeng Li wrote:
> >> Cc Eduardo,
> >> 2018-02-26 20:41 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
> >> > On 26/02/2018 13:22, Borislav Petkov wrote:
> >> >> On Mon, Feb 26, 2018 at 01:18:07PM +0100, Paolo Bonzini wrote:
> >> >>>> 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.
> >>
> >> 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?
> >
> > 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.

-- 
Eduardo

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Paolo Bonzini]

On 18/04/2018 11:03, Eduardo Habkost wrote:
>>> 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).

Paolo

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Borislav Petkov]

On Wed, Apr 18, 2018 at 12:36:37PM +0200, Paolo Bonzini wrote:
> On 18/04/2018 11:03, Eduardo Habkost wrote:
> >>> 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).

Can't qemu do:

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

?

:)

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Eduardo Habkost]

On Mon, Apr 23, 2018 at 02:58:49PM +0200, Borislav Petkov wrote:
> On Wed, Apr 18, 2018 at 12:36:37PM +0200, Paolo Bonzini wrote:
> > On 18/04/2018 11:03, Eduardo Habkost wrote:
> > >>> 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).
> 
> 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?

-- 
Eduardo

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Borislav Petkov]

On Mon, Apr 23, 2018 at 10:08:23AM -0300, Eduardo Habkost wrote:
> On Mon, Apr 23, 2018 at 02:58:49PM +0200, Borislav Petkov wrote:
> > On Wed, Apr 18, 2018 at 12:36:37PM +0200, Paolo Bonzini wrote:
> > > On 18/04/2018 11:03, Eduardo Habkost wrote:
> > > >>> 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).
> > 
> > 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?

To save an MSR read per logical CPU but from the looks of it, that
KVM_GET_MSRS is widely used in qemu with kvm_get_msrs() so I guess one
more MSR doesn't matter.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Paolo Bonzini]

On 23/04/2018 14:58, Borislav Petkov wrote:
>>>> 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).
> Can't qemu do:
> 
> grep microcode /proc/cpuinfo | awk '{ print $3 }' | head -n 1
> 
> ?

Yes, but you took that a statement bit too narrowly.

We didn't include KVM_GET_MSRS because microcode version wasn't
otherwise available; rather, there's a general need for KVM userspace to
know the values of host MSRs, and microcode is an example.

Paolo

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Wanpeng Li]

2018-04-18 18:36 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
> On 18/04/2018 11:03, Eduardo Habkost wrote:
>>>> 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).

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

Regards,
Wanpeng Li

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Konrad Rzeszutek Wilk]

On Tue, Apr 24, 2018 at 10:59:04AM +0800, Wanpeng Li wrote:
> 2018-04-18 18:36 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
> > On 18/04/2018 11:03, Eduardo Habkost wrote:
> >>>> 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).
> 
> How about the live migration? What will happen if the source and
> destination machines have different microcode version?

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?

For example, see:
https://software.intel.com/sites/default/files/managed/1d/46/Retpoline-A-Branch-Target-Injection-Mitigation.pdf

5.3:
"To remedy this situation, an operating system running as a VM can query bit 2 of the
IA32_ARCH_CAPABILITIES MSR, known as “RSB Alternate” (RSBA). When RSBA is set, it
indicates that the VM may run on a processor vulnerable to exploits of Empty RSB
conditions regardless of the processor’s DisplayFamily/DisplayModel signature, and
that the operating system should deploy appropriate mitigations. Virtual machine
managers (VMM) may set RSBA via MSR interception to indicate that a virtual machine
might run at some time in the future on a vulnerable processor."

Perhaps the guest should do a bit of sampling of various CPUIDs as the migration
has been done? Is there a nice KVM hook inside of the guest to do this?

> 
> Regards,
> Wanpeng Li

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Paolo Bonzini]

On 24/04/2018 05:14, Konrad Rzeszutek Wilk wrote:
> 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?

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

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Konrad Rzeszutek Wilk]

On April 24, 2018 1:09:00 AM EDT, Paolo Bonzini <pbonzini@redhat.com> wrote:
>On 24/04/2018 05:14, Konrad Rzeszutek Wilk wrote:
>> 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?
>
>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.

I understand how it ought to be but sadly the cloud vendors have a mix of hardware.


With the retpoline/IBRS support (like what RH kernel has) you could migrate from Skylake to Broadwell and switching over from IBRS to retpoline would be good.

Hence asking about this.

>
>Paolo

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Wanpeng Li]

2018-02-26 20:41 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
> On 26/02/2018 13:22, Borislav Petkov wrote:
>> On Mon, Feb 26, 2018 at 01:18:07PM +0100, Paolo Bonzini wrote:
>>>> 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

I found vmware on my macbook will also do this. :)

Regards,
Wanpeng Li

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Paolo Bonzini]

On 26/02/2018 11:49, Borislav Petkov wrote:
>> 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.

It's actually 0x100000000.

>> 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?!

I don't understand this either.

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.

Paolo

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Borislav Petkov]

On Mon, Feb 26, 2018 at 12:47:27PM +0100, Paolo Bonzini wrote:
> It's actually 0x100000000.

Even more wrong. Revision ID is bits [31:0].

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

So if we decide to do microcode revisions in the guest, we should
consider the fact that microcode revisions need to be handled the same
way as CPUID bits: a revision number means something on baremetal and
implies a certain functionality, just like a set CPUID bit does.

So we either have that same functionality in the guest or we don't talk
about revisions at all. Because if we end up lying by coming up with
revision numbers, all hell will break loose in the guest.

IMO, of course.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Liran Alon]

----- kernellwp@gmail.com wrote:

> From: Wanpeng Li <wanpengli@tencent.com>
> 
> 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
> don't check that they are running virtualized (i.e., they should trust
> the
> hypervisor) from disabling features that are effectively not buggy.
> 
> Suggested-by: Filippo Sironi <sironi@amazon.de>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> ---
>  arch/x86/include/asm/kvm_host.h | 1 +
>  arch/x86/kvm/x86.c              | 8 ++++++--
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h
> b/arch/x86/include/asm/kvm_host.h
> index 938d453..6e13f2f 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -507,6 +507,7 @@ struct kvm_vcpu_arch {
>  	u64 smi_count;
>  	bool tpr_access_reporting;
>  	u64 ia32_xss;
> +	u32 microcode_version;
>  
>  	/*
>  	 * Paging state of the vcpu
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 1a3ed81..cc51c61 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2247,7 +2247,6 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu,
> struct msr_data *msr_info)
>  
>  	switch (msr) {
>  	case MSR_AMD64_NB_CFG:
> -	case MSR_IA32_UCODE_REV:
>  	case MSR_IA32_UCODE_WRITE:
>  	case MSR_VM_HSAVE_PA:
>  	case MSR_AMD64_PATCH_LOADER:
> @@ -2255,6 +2254,10 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu,
> struct msr_data *msr_info)
>  	case MSR_AMD64_DC_CFG:
>  		break;
>  
> +	case MSR_IA32_UCODE_REV:
> +		if (msr_info->host_initiated)
> +			vcpu->arch.microcode_version = data >> 32;
> +		break;
>  	case MSR_EFER:
>  		return set_efer(vcpu, data);
>  	case MSR_K7_HWCR:
> @@ -2550,7 +2553,7 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu,
> struct msr_data *msr_info)
>  		msr_info->data = 0;
>  		break;
>  	case MSR_IA32_UCODE_REV:
> -		msr_info->data = 0x100000000ULL;
> +		msr_info->data = (u64)vcpu->arch.microcode_version << 32;
>  		break;
>  	case MSR_MTRRcap:
>  	case 0x200 ... 0x2ff:
> @@ -8232,6 +8235,7 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool
> init_event)
>  	vcpu->arch.regs_dirty = ~0;
>  
>  	vcpu->arch.ia32_xss = 0;
> +	vcpu->arch.microcode_version = 0x1;
>  
>  	kvm_x86_ops->vcpu_reset(vcpu, init_event);
>  }
> -- 
> 2.7.4

I think you need to add MSR_IA32_UCODE_REV to emulated_msrs[]
to allow for proper live-migration of this MSR value.

The rest seems fine to me. :)

Regards,
-Liran

Re: [PATCH] KVM: X86: Allow userspace to define the microcode version

[Wanpeng Li]

2018-02-26 17:26 GMT+08:00 Liran Alon <liran.alon@oracle.com>:
>
> ----- kernellwp@gmail.com wrote:
>
>> From: Wanpeng Li <wanpengli@tencent.com>
>>
>> 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
>> don't check that they are running virtualized (i.e., they should trust
>> the
>> hypervisor) from disabling features that are effectively not buggy.
>>
>> Suggested-by: Filippo Sironi <sironi@amazon.de>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Radim Krčmář <rkrcmar@redhat.com>
>> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
>> ---
>>  arch/x86/include/asm/kvm_host.h | 1 +
>>  arch/x86/kvm/x86.c              | 8 ++++++--
>>  2 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h
>> b/arch/x86/include/asm/kvm_host.h
>> index 938d453..6e13f2f 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -507,6 +507,7 @@ struct kvm_vcpu_arch {
>>       u64 smi_count;
>>       bool tpr_access_reporting;
>>       u64 ia32_xss;
>> +     u32 microcode_version;
>>
>>       /*
>>        * Paging state of the vcpu
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 1a3ed81..cc51c61 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -2247,7 +2247,6 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu,
>> struct msr_data *msr_info)
>>
>>       switch (msr) {
>>       case MSR_AMD64_NB_CFG:
>> -     case MSR_IA32_UCODE_REV:
>>       case MSR_IA32_UCODE_WRITE:
>>       case MSR_VM_HSAVE_PA:
>>       case MSR_AMD64_PATCH_LOADER:
>> @@ -2255,6 +2254,10 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu,
>> struct msr_data *msr_info)
>>       case MSR_AMD64_DC_CFG:
>>               break;
>>
>> +     case MSR_IA32_UCODE_REV:
>> +             if (msr_info->host_initiated)
>> +                     vcpu->arch.microcode_version = data >> 32;
>> +             break;
>>       case MSR_EFER:
>>               return set_efer(vcpu, data);
>>       case MSR_K7_HWCR:
>> @@ -2550,7 +2553,7 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu,
>> struct msr_data *msr_info)
>>               msr_info->data = 0;
>>               break;
>>       case MSR_IA32_UCODE_REV:
>> -             msr_info->data = 0x100000000ULL;
>> +             msr_info->data = (u64)vcpu->arch.microcode_version << 32;
>>               break;
>>       case MSR_MTRRcap:
>>       case 0x200 ... 0x2ff:
>> @@ -8232,6 +8235,7 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool
>> init_event)
>>       vcpu->arch.regs_dirty = ~0;
>>
>>       vcpu->arch.ia32_xss = 0;
>> +     vcpu->arch.microcode_version = 0x1;
>>
>>       kvm_x86_ops->vcpu_reset(vcpu, init_event);
>>  }
>> --
>> 2.7.4
>
> I think you need to add MSR_IA32_UCODE_REV to emulated_msrs[]
> to allow for proper live-migration of this MSR value.

Good catch, will do in v2.

Regards,
Wanpeng Li