LKML Archive on lore.kernel.org
 help / Atom feed
* [PATCH] KVM: X86: Allow userspace to define the microcode version
@ 2018-02-26  7:23 Wanpeng Li
  2018-02-26  9:41 ` Borislav Petkov
  0 siblings, 1 reply; 43+ messages in thread
From: Wanpeng Li @ 2018-02-26  7:23 UTC (permalink / raw)
  To: linux-kernel, kvm; +Cc: Paolo Bonzini, Radim Krčmář

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-02-26  7:23 [PATCH] KVM: X86: Allow userspace to define the microcode version Wanpeng Li
@ 2018-02-26  9:41 ` Borislav Petkov
  2018-02-26 10:06   ` Wanpeng Li
  0 siblings, 1 reply; 43+ messages in thread
From: Borislav Petkov @ 2018-02-26  9:41 UTC (permalink / raw)
  To: Wanpeng Li; +Cc: linux-kernel, kvm, Paolo Bonzini, Radim Krčmář

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.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-02-26  9:41 ` Borislav Petkov
@ 2018-02-26 10:06   ` Wanpeng Li
  2018-02-26 10:49     ` Borislav Petkov
  0 siblings, 1 reply; 43+ messages in thread
From: Wanpeng Li @ 2018-02-26 10:06 UTC (permalink / raw)
  To: Borislav Petkov; +Cc: LKML, kvm, Paolo Bonzini, Radim Krčmář

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-02-26 10:06   ` Wanpeng Li
@ 2018-02-26 10:49     ` Borislav Petkov
  2018-02-26 11:02       ` Wanpeng Li
  2018-02-26 11:47       ` Paolo Bonzini
  0 siblings, 2 replies; 43+ messages in thread
From: Borislav Petkov @ 2018-02-26 10:49 UTC (permalink / raw)
  To: Wanpeng Li; +Cc: LKML, kvm, Paolo Bonzini, Radim Krčmář

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.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-02-26 10:49     ` Borislav Petkov
@ 2018-02-26 11:02       ` Wanpeng Li
  2018-02-26 11:16         ` Borislav Petkov
  2018-02-26 11:47       ` Paolo Bonzini
  1 sibling, 1 reply; 43+ messages in thread
From: Wanpeng Li @ 2018-02-26 11:02 UTC (permalink / raw)
  To: Borislav Petkov; +Cc: LKML, kvm, Paolo Bonzini, Radim Krčmář

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-02-26 11:02       ` Wanpeng Li
@ 2018-02-26 11:16         ` Borislav Petkov
  2018-02-26 11:25           ` Wanpeng Li
  0 siblings, 1 reply; 43+ messages in thread
From: Borislav Petkov @ 2018-02-26 11:16 UTC (permalink / raw)
  To: Wanpeng Li; +Cc: LKML, kvm, Paolo Bonzini, Radim Krčmář

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.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-02-26 11:16         ` Borislav Petkov
@ 2018-02-26 11:25           ` Wanpeng Li
  2018-02-26 11:30             ` Borislav Petkov
  0 siblings, 1 reply; 43+ messages in thread
From: Wanpeng Li @ 2018-02-26 11:25 UTC (permalink / raw)
  To: Borislav Petkov; +Cc: LKML, kvm, Paolo Bonzini, Radim Krčmář

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-02-26 11:25           ` Wanpeng Li
@ 2018-02-26 11:30             ` Borislav Petkov
  2018-02-26 11:37               ` Wanpeng Li
  0 siblings, 1 reply; 43+ messages in thread
From: Borislav Petkov @ 2018-02-26 11:30 UTC (permalink / raw)
  To: Wanpeng Li; +Cc: LKML, kvm, Paolo Bonzini, Radim Krčmář

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.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-02-26 11:30             ` Borislav Petkov
@ 2018-02-26 11:37               ` Wanpeng Li
  2018-02-26 11:44                 ` Borislav Petkov
  0 siblings, 1 reply; 43+ messages in thread
From: Wanpeng Li @ 2018-02-26 11:37 UTC (permalink / raw)
  To: Borislav Petkov; +Cc: LKML, kvm, Paolo Bonzini, Radim Krčmář

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-02-26 11:37               ` Wanpeng Li
@ 2018-02-26 11:44                 ` Borislav Petkov
  2018-02-26 11:52                   ` Wanpeng Li
  2018-02-26 11:54                   ` Paolo Bonzini
  0 siblings, 2 replies; 43+ messages in thread
From: Borislav Petkov @ 2018-02-26 11:44 UTC (permalink / raw)
  To: Wanpeng Li; +Cc: LKML, kvm, Paolo Bonzini, Radim Krčmář

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.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-02-26 10:49     ` Borislav Petkov
  2018-02-26 11:02       ` Wanpeng Li
@ 2018-02-26 11:47       ` Paolo Bonzini
  2018-02-26 12:20         ` Borislav Petkov
  1 sibling, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2018-02-26 11:47 UTC (permalink / raw)
  To: Borislav Petkov, Wanpeng Li; +Cc: LKML, kvm, Radim Krčmář

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-02-26 11:44                 ` Borislav Petkov
@ 2018-02-26 11:52                   ` Wanpeng Li
  2018-02-26 11:54                   ` Paolo Bonzini
  1 sibling, 0 replies; 43+ messages in thread
From: Wanpeng Li @ 2018-02-26 11:52 UTC (permalink / raw)
  To: Borislav Petkov; +Cc: LKML, kvm, Paolo Bonzini, Radim Krčmář

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-02-26 11:44                 ` Borislav Petkov
  2018-02-26 11:52                   ` Wanpeng Li
@ 2018-02-26 11:54                   ` Paolo Bonzini
  2018-02-26 12:15                     ` Borislav Petkov
  1 sibling, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2018-02-26 11:54 UTC (permalink / raw)
  To: Borislav Petkov, Wanpeng Li; +Cc: LKML, kvm, Radim Krčmář

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-02-26 11:54                   ` Paolo Bonzini
@ 2018-02-26 12:15                     ` Borislav Petkov
  2018-02-26 12:16                       ` Paolo Bonzini
  0 siblings, 1 reply; 43+ messages in thread
From: Borislav Petkov @ 2018-02-26 12:15 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Wanpeng Li, LKML, kvm, Radim Krčmář

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.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-02-26 12:15                     ` Borislav Petkov
@ 2018-02-26 12:16                       ` Paolo Bonzini
  2018-02-26 12:18                         ` Paolo Bonzini
  0 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2018-02-26 12:16 UTC (permalink / raw)
  To: Borislav Petkov; +Cc: Wanpeng Li, LKML, kvm, Radim Krčmář

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-02-26 12:16                       ` Paolo Bonzini
@ 2018-02-26 12:18                         ` Paolo Bonzini
  2018-02-26 12:22                           ` Borislav Petkov
  0 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2018-02-26 12:18 UTC (permalink / raw)
  To: Borislav Petkov; +Cc: Wanpeng Li, LKML, kvm, Radim Krčmář

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-02-26 11:47       ` Paolo Bonzini
@ 2018-02-26 12:20         ` Borislav Petkov
  0 siblings, 0 replies; 43+ messages in thread
From: Borislav Petkov @ 2018-02-26 12:20 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Wanpeng Li, LKML, kvm, Radim Krčmář

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.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-02-26 12:18                         ` Paolo Bonzini
@ 2018-02-26 12:22                           ` Borislav Petkov
  2018-02-26 12:41                             ` Paolo Bonzini
  0 siblings, 1 reply; 43+ messages in thread
From: Borislav Petkov @ 2018-02-26 12:22 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Wanpeng Li, LKML, kvm, Radim Krčmář

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.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-02-26 12:22                           ` Borislav Petkov
@ 2018-02-26 12:41                             ` Paolo Bonzini
  2018-02-26 13:05                               ` Borislav Petkov
                                                 ` (3 more replies)
  0 siblings, 4 replies; 43+ messages in thread
From: Paolo Bonzini @ 2018-02-26 12:41 UTC (permalink / raw)
  To: Borislav Petkov; +Cc: Wanpeng Li, LKML, kvm, Radim Krčmář

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-02-26 12:41                             ` Paolo Bonzini
@ 2018-02-26 13:05                               ` Borislav Petkov
  2018-02-26 14:39                               ` Konrad Rzeszutek Wilk
                                                 ` (2 subsequent siblings)
  3 siblings, 0 replies; 43+ messages in thread
From: Borislav Petkov @ 2018-02-26 13:05 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Wanpeng Li, LKML, kvm, Radim Krčmář

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.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-02-26 12:41                             ` Paolo Bonzini
  2018-02-26 13:05                               ` Borislav Petkov
@ 2018-02-26 14:39                               ` Konrad Rzeszutek Wilk
  2018-02-26 14:46                                 ` Paolo Bonzini
  2018-02-26 19:37                                 ` Borislav Petkov
  2018-04-17 10:40                               ` [PATCH] KVM: X86: Allow userspace to define the microcode version Wanpeng Li
  2018-04-24  2:56                               ` Wanpeng Li
  3 siblings, 2 replies; 43+ messages in thread
From: Konrad Rzeszutek Wilk @ 2018-02-26 14:39 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Borislav Petkov, Wanpeng Li, LKML, kvm, Radim Krčmář

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
> 

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-02-26 14:39                               ` Konrad Rzeszutek Wilk
@ 2018-02-26 14:46                                 ` Paolo Bonzini
  2018-02-26 19:37                                 ` Borislav Petkov
  1 sibling, 0 replies; 43+ messages in thread
From: Paolo Bonzini @ 2018-02-26 14:46 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk
  Cc: Borislav Petkov, Wanpeng Li, LKML, kvm, Radim Krčmář

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-02-26 14:39                               ` Konrad Rzeszutek Wilk
  2018-02-26 14:46                                 ` Paolo Bonzini
@ 2018-02-26 19:37                                 ` Borislav Petkov
  2018-02-26 20:51                                   ` Konrad Rzeszutek Wilk
  1 sibling, 1 reply; 43+ messages in thread
From: Borislav Petkov @ 2018-02-26 19:37 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk
  Cc: Paolo Bonzini, Wanpeng Li, LKML, kvm, Radim Krčmář

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.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-02-26 19:37                                 ` Borislav Petkov
@ 2018-02-26 20:51                                   ` Konrad Rzeszutek Wilk
  2018-02-26 21:30                                     ` Konrad Rzeszutek Wilk
  0 siblings, 1 reply; 43+ messages in thread
From: Konrad Rzeszutek Wilk @ 2018-02-26 20:51 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Paolo Bonzini, Wanpeng Li, LKML, kvm, Radim Krčmář

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.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-02-26 20:51                                   ` Konrad Rzeszutek Wilk
@ 2018-02-26 21:30                                     ` Konrad Rzeszutek Wilk
  2018-02-27  8:33                                       ` Paolo Bonzini
  2018-03-08  9:24                                       ` [tip:x86/pti] x86/spectre_v2: Don't check microcode versions when running under hypervisors tip-bot for Konrad Rzeszutek Wilk
  0 siblings, 2 replies; 43+ messages in thread
From: Konrad Rzeszutek Wilk @ 2018-02-26 21:30 UTC (permalink / raw)
  To: Borislav Petkov, x86, mingo, tglx
  Cc: Paolo Bonzini, Wanpeng Li, LKML, kvm, Radim Krčmář

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-02-26 21:30                                     ` Konrad Rzeszutek Wilk
@ 2018-02-27  8:33                                       ` Paolo Bonzini
  2018-03-08  9:24                                       ` [tip:x86/pti] x86/spectre_v2: Don't check microcode versions when running under hypervisors tip-bot for Konrad Rzeszutek Wilk
  1 sibling, 0 replies; 43+ messages in thread
From: Paolo Bonzini @ 2018-02-27  8:33 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk, Borislav Petkov, x86, mingo, tglx
  Cc: Wanpeng Li, LKML, kvm, Radim Krčmář

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* [tip:x86/pti] x86/spectre_v2: Don't check microcode versions when running under hypervisors
  2018-02-26 21:30                                     ` Konrad Rzeszutek Wilk
  2018-02-27  8:33                                       ` Paolo Bonzini
@ 2018-03-08  9:24                                       ` tip-bot for Konrad Rzeszutek Wilk
  1 sibling, 0 replies; 43+ messages in thread
From: tip-bot for Konrad Rzeszutek Wilk @ 2018-03-08  9:24 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: tglx, mingo, hpa, konrad.wilk, kernellwp, pbonzini, rkrcmar, kvm,
	bp, linux-kernel

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)

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-02-26 12:41                             ` Paolo Bonzini
  2018-02-26 13:05                               ` Borislav Petkov
  2018-02-26 14:39                               ` Konrad Rzeszutek Wilk
@ 2018-04-17 10:40                               ` Wanpeng Li
  2018-04-17 20:24                                 ` Eduardo Habkost
  2018-04-24  2:56                               ` Wanpeng Li
  3 siblings, 1 reply; 43+ messages in thread
From: Wanpeng Li @ 2018-04-17 10:40 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Borislav Petkov, LKML, kvm, Radim Krčmář, Eduardo Habkost

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-04-17 10:40                               ` [PATCH] KVM: X86: Allow userspace to define the microcode version Wanpeng Li
@ 2018-04-17 20:24                                 ` Eduardo Habkost
  2018-04-18  3:24                                   ` Wanpeng Li
  0 siblings, 1 reply; 43+ messages in thread
From: Eduardo Habkost @ 2018-04-17 20:24 UTC (permalink / raw)
  To: Wanpeng Li
  Cc: Paolo Bonzini, Borislav Petkov, LKML, kvm, Radim Krčmář

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-04-17 20:24                                 ` Eduardo Habkost
@ 2018-04-18  3:24                                   ` Wanpeng Li
  2018-04-18  9:03                                     ` Eduardo Habkost
  0 siblings, 1 reply; 43+ messages in thread
From: Wanpeng Li @ 2018-04-18  3:24 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: Paolo Bonzini, Borislav Petkov, LKML, kvm, Radim Krčmář

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-04-18  3:24                                   ` Wanpeng Li
@ 2018-04-18  9:03                                     ` Eduardo Habkost
  2018-04-18 10:36                                       ` Paolo Bonzini
  0 siblings, 1 reply; 43+ messages in thread
From: Eduardo Habkost @ 2018-04-18  9:03 UTC (permalink / raw)
  To: Wanpeng Li
  Cc: Paolo Bonzini, Borislav Petkov, LKML, kvm, Radim Krčmář

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-04-18  9:03                                     ` Eduardo Habkost
@ 2018-04-18 10:36                                       ` Paolo Bonzini
  2018-04-23 12:58                                         ` Borislav Petkov
  2018-04-24  2:59                                         ` Wanpeng Li
  0 siblings, 2 replies; 43+ messages in thread
From: Paolo Bonzini @ 2018-04-18 10:36 UTC (permalink / raw)
  To: Eduardo Habkost, Wanpeng Li
  Cc: Borislav Petkov, LKML, kvm, Radim Krčmář

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-04-18 10:36                                       ` Paolo Bonzini
@ 2018-04-23 12:58                                         ` Borislav Petkov
  2018-04-23 13:08                                           ` Eduardo Habkost
  2018-04-23 16:03                                           ` Paolo Bonzini
  2018-04-24  2:59                                         ` Wanpeng Li
  1 sibling, 2 replies; 43+ messages in thread
From: Borislav Petkov @ 2018-04-23 12:58 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Eduardo Habkost, Wanpeng Li, LKML, kvm, Radim Krčmář

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.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-04-23 12:58                                         ` Borislav Petkov
@ 2018-04-23 13:08                                           ` Eduardo Habkost
  2018-04-23 13:23                                             ` Borislav Petkov
  2018-04-23 16:03                                           ` Paolo Bonzini
  1 sibling, 1 reply; 43+ messages in thread
From: Eduardo Habkost @ 2018-04-23 13:08 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Paolo Bonzini, Wanpeng Li, LKML, kvm, Radim Krčmář

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-04-23 13:08                                           ` Eduardo Habkost
@ 2018-04-23 13:23                                             ` Borislav Petkov
  0 siblings, 0 replies; 43+ messages in thread
From: Borislav Petkov @ 2018-04-23 13:23 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: Paolo Bonzini, Wanpeng Li, LKML, kvm, Radim Krčmář

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.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-04-23 12:58                                         ` Borislav Petkov
  2018-04-23 13:08                                           ` Eduardo Habkost
@ 2018-04-23 16:03                                           ` Paolo Bonzini
  1 sibling, 0 replies; 43+ messages in thread
From: Paolo Bonzini @ 2018-04-23 16:03 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Eduardo Habkost, Wanpeng Li, LKML, kvm, Radim Krčmář

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-02-26 12:41                             ` Paolo Bonzini
                                                 ` (2 preceding siblings ...)
  2018-04-17 10:40                               ` [PATCH] KVM: X86: Allow userspace to define the microcode version Wanpeng Li
@ 2018-04-24  2:56                               ` Wanpeng Li
  3 siblings, 0 replies; 43+ messages in thread
From: Wanpeng Li @ 2018-04-24  2:56 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Borislav Petkov, LKML, kvm, Radim Krčmář

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-04-18 10:36                                       ` Paolo Bonzini
  2018-04-23 12:58                                         ` Borislav Petkov
@ 2018-04-24  2:59                                         ` Wanpeng Li
  2018-04-24  3:14                                           ` Konrad Rzeszutek Wilk
  1 sibling, 1 reply; 43+ messages in thread
From: Wanpeng Li @ 2018-04-24  2:59 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Eduardo Habkost, Borislav Petkov, LKML, kvm, Radim Krčmář

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-04-24  2:59                                         ` Wanpeng Li
@ 2018-04-24  3:14                                           ` Konrad Rzeszutek Wilk
  2018-04-24  5:09                                             ` Paolo Bonzini
  0 siblings, 1 reply; 43+ messages in thread
From: Konrad Rzeszutek Wilk @ 2018-04-24  3:14 UTC (permalink / raw)
  To: Wanpeng Li
  Cc: Paolo Bonzini, Eduardo Habkost, Borislav Petkov, LKML, kvm,
	Radim Krčmář

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-04-24  3:14                                           ` Konrad Rzeszutek Wilk
@ 2018-04-24  5:09                                             ` Paolo Bonzini
  2018-04-24 13:44                                               ` Konrad Rzeszutek Wilk
  0 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2018-04-24  5:09 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk, Wanpeng Li
  Cc: Eduardo Habkost, Borislav Petkov, LKML, kvm, Radim Krčmář

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-04-24  5:09                                             ` Paolo Bonzini
@ 2018-04-24 13:44                                               ` Konrad Rzeszutek Wilk
  0 siblings, 0 replies; 43+ messages in thread
From: Konrad Rzeszutek Wilk @ 2018-04-24 13:44 UTC (permalink / raw)
  To: Paolo Bonzini, Wanpeng Li
  Cc: Eduardo Habkost, Borislav Petkov, LKML, kvm, Radim Krčmář

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
  2018-02-26  9:26 Liran Alon
@ 2018-02-26 10:08 ` Wanpeng Li
  0 siblings, 0 replies; 43+ messages in thread
From: Wanpeng Li @ 2018-02-26 10:08 UTC (permalink / raw)
  To: Liran Alon; +Cc: Radim Krcmar, Paolo Bonzini, LKML, kvm

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH] KVM: X86: Allow userspace to define the microcode version
@ 2018-02-26  9:26 Liran Alon
  2018-02-26 10:08 ` Wanpeng Li
  0 siblings, 1 reply; 43+ messages in thread
From: Liran Alon @ 2018-02-26  9:26 UTC (permalink / raw)
  To: kernellwp; +Cc: rkrcmar, pbonzini, linux-kernel, kvm


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

^ permalink raw reply	[flat|nested] 43+ messages in thread

end of thread, back to index

Thread overview: 43+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-26  7:23 [PATCH] KVM: X86: Allow userspace to define the microcode version Wanpeng Li
2018-02-26  9:41 ` Borislav Petkov
2018-02-26 10:06   ` Wanpeng Li
2018-02-26 10:49     ` Borislav Petkov
2018-02-26 11:02       ` Wanpeng Li
2018-02-26 11:16         ` Borislav Petkov
2018-02-26 11:25           ` Wanpeng Li
2018-02-26 11:30             ` Borislav Petkov
2018-02-26 11:37               ` Wanpeng Li
2018-02-26 11:44                 ` Borislav Petkov
2018-02-26 11:52                   ` Wanpeng Li
2018-02-26 11:54                   ` Paolo Bonzini
2018-02-26 12:15                     ` Borislav Petkov
2018-02-26 12:16                       ` Paolo Bonzini
2018-02-26 12:18                         ` Paolo Bonzini
2018-02-26 12:22                           ` Borislav Petkov
2018-02-26 12:41                             ` Paolo Bonzini
2018-02-26 13:05                               ` Borislav Petkov
2018-02-26 14:39                               ` Konrad Rzeszutek Wilk
2018-02-26 14:46                                 ` Paolo Bonzini
2018-02-26 19:37                                 ` Borislav Petkov
2018-02-26 20:51                                   ` Konrad Rzeszutek Wilk
2018-02-26 21:30                                     ` Konrad Rzeszutek Wilk
2018-02-27  8:33                                       ` Paolo Bonzini
2018-03-08  9:24                                       ` [tip:x86/pti] x86/spectre_v2: Don't check microcode versions when running under hypervisors tip-bot for Konrad Rzeszutek Wilk
2018-04-17 10:40                               ` [PATCH] KVM: X86: Allow userspace to define the microcode version Wanpeng Li
2018-04-17 20:24                                 ` Eduardo Habkost
2018-04-18  3:24                                   ` Wanpeng Li
2018-04-18  9:03                                     ` Eduardo Habkost
2018-04-18 10:36                                       ` Paolo Bonzini
2018-04-23 12:58                                         ` Borislav Petkov
2018-04-23 13:08                                           ` Eduardo Habkost
2018-04-23 13:23                                             ` Borislav Petkov
2018-04-23 16:03                                           ` Paolo Bonzini
2018-04-24  2:59                                         ` Wanpeng Li
2018-04-24  3:14                                           ` Konrad Rzeszutek Wilk
2018-04-24  5:09                                             ` Paolo Bonzini
2018-04-24 13:44                                               ` Konrad Rzeszutek Wilk
2018-04-24  2:56                               ` Wanpeng Li
2018-02-26 11:47       ` Paolo Bonzini
2018-02-26 12:20         ` Borislav Petkov
2018-02-26  9:26 Liran Alon
2018-02-26 10:08 ` Wanpeng Li

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox