All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] KVM: X86: Allow userspace to define the microcode version
@ 2018-02-27  1:18 Wanpeng Li
  2018-02-27  1:30 ` Nadav Amit
  0 siblings, 1 reply; 5+ messages in thread
From: Wanpeng Li @ 2018-02-27  1:18 UTC (permalink / raw)
  To: linux-kernel, kvm; +Cc: Paolo Bonzini, Radim Krčmář, Liran Alon

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>
Cc: Liran Alon <liran.alon@oracle.com>
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
v2 -> v3:
 * remove the shifts
 * add the MSR_IA32_UCODE_REV version to the "feature MSRs"
v1 -> v2:
 * add MSR_IA32_UCODE_REV to emulated_msrs

 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/x86.c              | 19 +++++++++++++++----
 2 files changed, 16 insertions(+), 4 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 d4985a9..00af28e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1058,6 +1058,7 @@ static unsigned num_emulated_msrs;
 static u32 msr_based_features[] = {
 	MSR_IA32_ARCH_CAPABILITIES,
 	MSR_F10H_DECFG,
+	MSR_IA32_UCODE_REV,
 };
 
 static unsigned int num_msr_based_features;
@@ -1067,8 +1068,14 @@ static int do_get_msr_feature(struct kvm_vcpu *vcpu, unsigned index, u64 *data)
 	struct kvm_msr_entry msr;
 
 	msr.index = index;
-	if (kvm_x86_ops->get_msr_feature(&msr))
-		return 1;
+	switch (msr.index) {
+	case MSR_IA32_UCODE_REV:
+		rdmsrl(msr.index, msr.data);
+		break;
+	default:
+		if (kvm_x86_ops->get_msr_feature(&msr))
+			return 1;
+	}
 
 	*data = msr.data;
 
@@ -2248,7 +2255,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:
@@ -2256,6 +2262,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;
+		break;
 	case MSR_EFER:
 		return set_efer(vcpu, data);
 	case MSR_K7_HWCR:
@@ -2551,7 +2561,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;
 		break;
 	case MSR_MTRRcap:
 	case 0x200 ... 0x2ff:
@@ -8233,6 +8243,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 related	[flat|nested] 5+ messages in thread
* Re: [PATCH v3] KVM: X86: Allow userspace to define the microcode version
  2018-02-27  1:18 [PATCH v3] KVM: X86: Allow userspace to define the microcode version Wanpeng Li
@ 2018-02-27  1:30 ` Nadav Amit
  2018-02-27  2:22   ` Wanpeng Li
  0 siblings, 1 reply; 5+ messages in thread
From: Nadav Amit @ 2018-02-27  1:30 UTC (permalink / raw)
  To: Wanpeng Li
  Cc: linux-kernel, kvm, Paolo Bonzini, Radim Krčmář,
	Liran Alon

Wanpeng Li <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>
> Cc: Liran Alon <liran.alon@oracle.com>
> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> ---
> v2 -> v3:
> * remove the shifts
> * add the MSR_IA32_UCODE_REV version to the "feature MSRs"
> v1 -> v2:
> * add MSR_IA32_UCODE_REV to emulated_msrs
> 
> arch/x86/include/asm/kvm_host.h |  1 +
> arch/x86/kvm/x86.c              | 19 +++++++++++++++----
> 2 files changed, 16 insertions(+), 4 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 d4985a9..00af28e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1058,6 +1058,7 @@ static unsigned num_emulated_msrs;
> static u32 msr_based_features[] = {
> 	MSR_IA32_ARCH_CAPABILITIES,
> 	MSR_F10H_DECFG,
> +	MSR_IA32_UCODE_REV,
> };
> 
> static unsigned int num_msr_based_features;
> @@ -1067,8 +1068,14 @@ static int do_get_msr_feature(struct kvm_vcpu *vcpu, unsigned index, u64 *data)
> 	struct kvm_msr_entry msr;
> 
> 	msr.index = index;
> -	if (kvm_x86_ops->get_msr_feature(&msr))
> -		return 1;
> +	switch (msr.index) {
> +	case MSR_IA32_UCODE_REV:
> +		rdmsrl(msr.index, msr.data);
> +		break;
> +	default:
> +		if (kvm_x86_ops->get_msr_feature(&msr))
> +			return 1;
> +	}
> 
> 	*data = msr.data;
> 
> @@ -2248,7 +2255,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:
> @@ -2256,6 +2262,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;
> +		break;
> 	case MSR_EFER:
> 		return set_efer(vcpu, data);
> 	case MSR_K7_HWCR:
> @@ -2551,7 +2561,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;

I think that the shifts are missing here (the version should be on the high
bits according to intel_get_microcode_revision() ).

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH v3] KVM: X86: Allow userspace to define the microcode version
  2018-02-27  1:30 ` Nadav Amit
@ 2018-02-27  2:22   ` Wanpeng Li
  2018-02-27  2:27     ` Nadav Amit
  2018-02-27  8:36     ` Paolo Bonzini
  0 siblings, 2 replies; 5+ messages in thread
From: Wanpeng Li @ 2018-02-27  2:22 UTC (permalink / raw)
  To: Nadav Amit
  Cc: LKML, kvm, Paolo Bonzini, Radim Krčmář, Liran Alon

2018-02-27 9:30 GMT+08:00 Nadav Amit <nadav.amit@gmail.com>:
> Wanpeng Li <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>
>> Cc: Liran Alon <liran.alon@oracle.com>
>> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
>> ---
>> v2 -> v3:
>> * remove the shifts
>> * add the MSR_IA32_UCODE_REV version to the "feature MSRs"
>> v1 -> v2:
>> * add MSR_IA32_UCODE_REV to emulated_msrs
>>
>> arch/x86/include/asm/kvm_host.h |  1 +
>> arch/x86/kvm/x86.c              | 19 +++++++++++++++----
>> 2 files changed, 16 insertions(+), 4 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 d4985a9..00af28e 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -1058,6 +1058,7 @@ static unsigned num_emulated_msrs;
>> static u32 msr_based_features[] = {
>>       MSR_IA32_ARCH_CAPABILITIES,
>>       MSR_F10H_DECFG,
>> +     MSR_IA32_UCODE_REV,
>> };
>>
>> static unsigned int num_msr_based_features;
>> @@ -1067,8 +1068,14 @@ static int do_get_msr_feature(struct kvm_vcpu *vcpu, unsigned index, u64 *data)
>>       struct kvm_msr_entry msr;
>>
>>       msr.index = index;
>> -     if (kvm_x86_ops->get_msr_feature(&msr))
>> -             return 1;
>> +     switch (msr.index) {
>> +     case MSR_IA32_UCODE_REV:
>> +             rdmsrl(msr.index, msr.data);
>> +             break;
>> +     default:
>> +             if (kvm_x86_ops->get_msr_feature(&msr))
>> +                     return 1;
>> +     }
>>
>>       *data = msr.data;
>>
>> @@ -2248,7 +2255,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:
>> @@ -2256,6 +2262,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;
>> +             break;
>>       case MSR_EFER:
>>               return set_efer(vcpu, data);
>>       case MSR_K7_HWCR:
>> @@ -2551,7 +2561,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;
>
> I think that the shifts are missing here (the version should be on the high
> bits according to intel_get_microcode_revision() ).

You are right, it seems that we all miss it before.

Regards,
Wanpeng Li

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH v3] KVM: X86: Allow userspace to define the microcode version
  2018-02-27  2:22   ` Wanpeng Li
@ 2018-02-27  2:27     ` Nadav Amit
  2018-02-27  8:36     ` Paolo Bonzini
  1 sibling, 0 replies; 5+ messages in thread
From: Nadav Amit @ 2018-02-27  2:27 UTC (permalink / raw)
  To: Wanpeng Li
  Cc: LKML, kvm, Paolo Bonzini, Radim Krčmář, Liran Alon

Wanpeng Li <kernellwp@gmail.com> wrote:

> 2018-02-27 9:30 GMT+08:00 Nadav Amit <nadav.amit@gmail.com>:
>> Wanpeng Li <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>
>>> Cc: Liran Alon <liran.alon@oracle.com>
>>> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
>>> ---
>>> v2 -> v3:
>>> * remove the shifts
>>> * add the MSR_IA32_UCODE_REV version to the "feature MSRs"
>>> v1 -> v2:
>>> * add MSR_IA32_UCODE_REV to emulated_msrs
>>> 
>>> arch/x86/include/asm/kvm_host.h |  1 +
>>> arch/x86/kvm/x86.c              | 19 +++++++++++++++----
>>> 2 files changed, 16 insertions(+), 4 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 d4985a9..00af28e 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -1058,6 +1058,7 @@ static unsigned num_emulated_msrs;
>>> static u32 msr_based_features[] = {
>>>      MSR_IA32_ARCH_CAPABILITIES,
>>>      MSR_F10H_DECFG,
>>> +     MSR_IA32_UCODE_REV,
>>> };
>>> 
>>> static unsigned int num_msr_based_features;
>>> @@ -1067,8 +1068,14 @@ static int do_get_msr_feature(struct kvm_vcpu *vcpu, unsigned index, u64 *data)
>>>      struct kvm_msr_entry msr;
>>> 
>>>      msr.index = index;
>>> -     if (kvm_x86_ops->get_msr_feature(&msr))
>>> -             return 1;
>>> +     switch (msr.index) {
>>> +     case MSR_IA32_UCODE_REV:
>>> +             rdmsrl(msr.index, msr.data);
>>> +             break;
>>> +     default:
>>> +             if (kvm_x86_ops->get_msr_feature(&msr))
>>> +                     return 1;
>>> +     }
>>> 
>>>      *data = msr.data;
>>> 
>>> @@ -2248,7 +2255,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:
>>> @@ -2256,6 +2262,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;
>>> +             break;
>>>      case MSR_EFER:
>>>              return set_efer(vcpu, data);
>>>      case MSR_K7_HWCR:
>>> @@ -2551,7 +2561,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;
>> 
>> I think that the shifts are missing here (the version should be on the high
>> bits according to intel_get_microcode_revision() ).
> 
> You are right, it seems that we all miss it before.

Nah. I’m not that smart. It was ok on v2 ;-)

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH v3] KVM: X86: Allow userspace to define the microcode version
  2018-02-27  2:22   ` Wanpeng Li
  2018-02-27  2:27     ` Nadav Amit
@ 2018-02-27  8:36     ` Paolo Bonzini
  1 sibling, 0 replies; 5+ messages in thread
From: Paolo Bonzini @ 2018-02-27  8:36 UTC (permalink / raw)
  To: Wanpeng Li, Nadav Amit; +Cc: LKML, kvm, Radim Krčmář, Liran Alon

On 27/02/2018 03:22, Wanpeng Li wrote:
>>> @@ -2551,7 +2561,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;
>> I think that the shifts are missing here (the version should be on the high
>> bits according to intel_get_microcode_revision() ).
> You are right, it seems that we all miss it before.

It's not that the shift are missing.  It's that microcode_version should
be u64 and initialized to 0x100000000ULL.  Sorry I was too concise in my
review of v2, and made that implicit.

(Boris noticed offlist that AMD places the revision is in bit 0-31).

Thanks,

Paolo

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-02-27  8:36 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-27  1:18 [PATCH v3] KVM: X86: Allow userspace to define the microcode version Wanpeng Li
2018-02-27  1:30 ` Nadav Amit
2018-02-27  2:22   ` Wanpeng Li
2018-02-27  2:27     ` Nadav Amit
2018-02-27  8:36     ` Paolo Bonzini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.