From: Wanpeng Li <[email protected]>
Linux (among the others) has checks to make sure that certain features
aren't enabled on a certain family/model/stepping if the microcode version
isn't greater than or equal to a known good version.
By exposing the real microcode version, we're preventing buggy guests that
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 <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: Radim Krčmář <[email protected]>
Signed-off-by: Wanpeng Li <[email protected]>
---
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
----- [email protected] wrote:
> From: Wanpeng Li <[email protected]>
>
> Linux (among the others) has checks to make sure that certain features
>
> aren't enabled on a certain family/model/stepping if the microcode
> version
> isn't greater than or equal to a known good version.
>
> By exposing the real microcode version, we're preventing buggy guests
> that
> 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 <[email protected]>
> Cc: Paolo Bonzini <[email protected]>
> Cc: Radim Krčmář <[email protected]>
> Signed-off-by: Wanpeng Li <[email protected]>
> ---
> 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
On Mon, Feb 26, 2018 at 03:23:58PM +0800, Wanpeng Li wrote:
> From: Wanpeng Li <[email protected]>
>
> Linux (among the others) has checks to make sure that certain features
> aren't enabled on a certain family/model/stepping if the microcode version
> isn't greater than or equal to a known good version.
>
> By exposing the real microcode version, we're preventing buggy guests that
Where do we prevent userspace from coming up with some non-sensical
microcode revision?
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
2018-02-26 17:41 GMT+08:00 Borislav Petkov <[email protected]>:
> On Mon, Feb 26, 2018 at 03:23:58PM +0800, Wanpeng Li wrote:
>> From: Wanpeng Li <[email protected]>
>>
>> Linux (among the others) has checks to make sure that certain features
>> aren't enabled on a certain family/model/stepping if the microcode version
>> isn't greater than or equal to a known good version.
>>
>> By exposing the real microcode version, we're preventing buggy guests that
>
> Where do we prevent userspace from coming up with some non-sensical
> microcode revision?
I think it is the host admin(e.g. cloud provider)'s responsibility to
set an expected microcode revision. In addition, the non-sensical
value which is written by the guest will not reflect to guest-visible
microcode revision and just be ignored in this implementation.
Regards,
Wanpeng Li
2018-02-26 17:26 GMT+08:00 Liran Alon <[email protected]>:
>
> ----- [email protected] wrote:
>
>> From: Wanpeng Li <[email protected]>
>>
>> Linux (among the others) has checks to make sure that certain features
>>
>> aren't enabled on a certain family/model/stepping if the microcode
>> version
>> isn't greater than or equal to a known good version.
>>
>> By exposing the real microcode version, we're preventing buggy guests
>> that
>> 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 <[email protected]>
>> Cc: Paolo Bonzini <[email protected]>
>> Cc: Radim Krčmář <[email protected]>
>> Signed-off-by: Wanpeng Li <[email protected]>
>> ---
>> 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
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.
2018-02-26 18:49 GMT+08:00 Borislav Petkov <[email protected]>:
> 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
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.
2018-02-26 19:16 GMT+08:00 Borislav Petkov <[email protected]>:
> 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
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.
2018-02-26 19:30 GMT+08:00 Borislav Petkov <[email protected]>:
> 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
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.
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
2018-02-26 19:44 GMT+08:00 Borislav Petkov <[email protected]>:
> 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
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
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.
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.
>
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
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.
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.
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
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.
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 <[email protected]>
Date: Mon, 26 Feb 2018 09:35:01 -0500
Subject: [PATCH] x86/spectre_v2: Don't check bad microcode versions when
running under hypervisors.
As:
1) We know they lie about the env anyhow (host mismatch)
2) Even if the hypervisor (Xen, KVM, VMWare, etc) provided
a valid "correct" value, it all gets to be very murky
when migration happens (do you provide the "new"
microcode of the machine?).
And in reality the cloud vendors are the ones that should make
sure that the microcode that is running is correct and we should
just sing lalalala and believe them.
Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
---
arch/x86/kernel/cpu/intel.c | 7 +++++++
1 file changed, 7 insertions(+)
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
>
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 <[email protected]>
> Date: Mon, 26 Feb 2018 09:35:01 -0500
> Subject: [PATCH] x86/spectre_v2: Don't check bad microcode versions when
> running under hypervisors.
>
> As:
> 1) We know they lie about the env anyhow (host mismatch)
> 2) Even if the hypervisor (Xen, KVM, VMWare, etc) provided
> a valid "correct" value, it all gets to be very murky
> when migration happens (do you provide the "new"
> microcode of the machine?).
>
> And in reality the cloud vendors are the ones that should make
> sure that the microcode that is running is correct and we should
> just sing lalalala and believe them.
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 <[email protected]>
> ---
> 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)
>
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.
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.
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 <[email protected]>
Date: Mon, 26 Feb 2018 09:35:01 -0500
Subject: [PATCH v2] x86/spectre_v2: Don't check bad microcode versions when
running under hypervisors.
As:
1) We know they lie about the env anyhow (host mismatch)
2) Even if the hypervisor (Xen, KVM, VMWare, etc) provided
a valid "correct" value, it all gets to be very murky
when migration happens (do you provide the "new"
microcode of the machine?).
And in reality the cloud vendors are the ones that should make
sure that the microcode that is running is correct and we should
just sing lalalala and trust them.
CC: [email protected]
CC: Ingo Molnar <[email protected]>
CC: "H. Peter Anvin" <[email protected]>
CC: [email protected]
Cc: Tom Lendacky <[email protected]>
Cc: Andi Kleen <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Masami Hiramatsu <[email protected]>
Cc: Arjan van de Ven <[email protected]>
Cc: David Woodhouse <[email protected]>
Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
---
v2: Change comments to be more in line with the state of the world.
v3: Use cpu_has instead of boot_cpu_has per Borislav's review.
---
arch/x86/kernel/cpu/intel.c | 7 +++++++
1 file changed, 7 insertions(+)
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
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 <[email protected]>
> Date: Mon, 26 Feb 2018 09:35:01 -0500
> Subject: [PATCH v2] x86/spectre_v2: Don't check bad microcode versions when
> running under hypervisors.
>
> As:
> 1) We know they lie about the env anyhow (host mismatch)
> 2) Even if the hypervisor (Xen, KVM, VMWare, etc) provided
> a valid "correct" value, it all gets to be very murky
> when migration happens (do you provide the "new"
> microcode of the machine?).
>
> And in reality the cloud vendors are the ones that should make
> sure that the microcode that is running is correct and we should
> just sing lalalala and trust them.
>
> CC: [email protected]
> CC: Ingo Molnar <[email protected]>
> CC: "H. Peter Anvin" <[email protected]>
> CC: [email protected]
> Cc: Tom Lendacky <[email protected]>
> Cc: Andi Kleen <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Masami Hiramatsu <[email protected]>
> Cc: Arjan van de Ven <[email protected]>
> Cc: David Woodhouse <[email protected]>
> Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
>
> ---
> v2: Change comments to be more in line with the state of the world.
> v3: Use cpu_has instead of boot_cpu_has per Borislav's review.
Reviewed-by: Paolo Bonzini <[email protected]>
> ---
> 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)
>
Commit-ID: 36268223c1e9981d6cfc33aff8520b3bde4b8114
Gitweb: https://git.kernel.org/tip/36268223c1e9981d6cfc33aff8520b3bde4b8114
Author: Konrad Rzeszutek Wilk <[email protected]>
AuthorDate: Mon, 26 Feb 2018 09:35:01 -0500
Committer: Thomas Gleixner <[email protected]>
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 <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Reviewed-by: Paolo Bonzini <[email protected]>
Cc: Wanpeng Li <[email protected]>
Cc: kvm <[email protected]>
Cc: Krčmář <[email protected]>
Cc: Borislav Petkov <[email protected]>
CC: "H. Peter Anvin" <[email protected]>
CC: [email protected]
Link: https://lkml.kernel.org/r/[email protected]
---
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)
Cc Eduardo,
2018-02-26 20:41 GMT+08:00 Paolo Bonzini <[email protected]>:
> 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
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 <[email protected]>:
> > 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
2018-04-18 4:24 GMT+08:00 Eduardo Habkost <[email protected]>:
> 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 <[email protected]>:
>> > 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
On Wed, Apr 18, 2018 at 11:24:22AM +0800, Wanpeng Li wrote:
> 2018-04-18 4:24 GMT+08:00 Eduardo Habkost <[email protected]>:
> > 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 <[email protected]>:
> >> > 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
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
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.
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
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.
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
2018-02-26 20:41 GMT+08:00 Paolo Bonzini <[email protected]>:
> 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
2018-04-18 18:36 GMT+08:00 Paolo Bonzini <[email protected]>:
> 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
On Tue, Apr 24, 2018 at 10:59:04AM +0800, Wanpeng Li wrote:
> 2018-04-18 18:36 GMT+08:00 Paolo Bonzini <[email protected]>:
> > 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
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
On April 24, 2018 1:09:00 AM EDT, Paolo Bonzini <[email protected]> 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