Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1605299 > unrolled thread

[PATCH v2 2/3] KVM: VMX: Fix enable VPID even if INVVPID is not exposed in vmx capability

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 2/3] KVM: VMX: Fix enable VPID even if INVVPID is not exposed in vmx capability Wanpeng Li <kernellwp@gmail.com> - 2017-03-21 05:30 +0100
    Re: [PATCH v2 2/3] KVM: VMX: Fix enable VPID even if INVVPID is not  exposed in vmx capability David Hildenbrand <david@redhat.com> - 2017-03-21 10:00 +0100
      Re: [PATCH v2 2/3] KVM: VMX: Fix enable VPID even if INVVPID is not  exposed in vmx capability Wanpeng Li <kernellwp@gmail.com> - 2017-03-21 10:00 +0100
        Re: [PATCH v2 2/3] KVM: VMX: Fix enable VPID even if INVVPID is not  exposed in vmx capability Wanpeng Li <kernellwp@gmail.com> - 2017-03-21 10:10 +0100
        Re: [PATCH v2 2/3] KVM: VMX: Fix enable VPID even if INVVPID is not  exposed in vmx capability David Hildenbrand <david@redhat.com> - 2017-03-21 10:10 +0100
        Re: [PATCH v2 2/3] KVM: VMX: Fix enable VPID even if INVVPID is not  exposed in vmx capability David Hildenbrand <david@redhat.com> - 2017-03-21 10:20 +0100
    Re: [PATCH v2 2/3] KVM: VMX: Fix enable VPID even if INVVPID is not  exposed in vmx capability Paolo Bonzini <pbonzini@redhat.com> - 2017-03-21 17:30 +0100

#1605299 — [PATCH v2 2/3] KVM: VMX: Fix enable VPID even if INVVPID is not exposed in vmx capability

From: Wanpeng Li <wanpeng.li@hotmail.com>

This can be reproduced by running L2 on L1, and disable VPID on L0 if w/o 
commit "KVM: nVMX: Fix nested VPID vmx exec control", the L2 crash as below:

KVM: entry failed, hardware error 0x7
EAX=00000000 EBX=00000000 ECX=00000000 EDX=000306c3
ESI=00000000 EDI=00000000 EBP=00000000 ESP=00000000
EIP=0000fff0 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
ES =0000 00000000 0000ffff 00009300
CS =f000 ffff0000 0000ffff 00009b00
SS =0000 00000000 0000ffff 00009300
DS =0000 00000000 0000ffff 00009300
FS =0000 00000000 0000ffff 00009300
GS =0000 00000000 0000ffff 00009300
LDT=0000 00000000 0000ffff 00008200
TR =0000 00000000 0000ffff 00008b00
GDT=     00000000 0000ffff
IDT=     00000000 0000ffff
CR0=60000010 CR2=00000000 CR3=00000000 CR4=00000000
DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000 
DR6=00000000ffff0ff0 DR7=0000000000000400
EFER=0000000000000000

Reference SDM 30.3 INVVPID:
 
Protected Mode Exceptions
#UD 
  - If not in VMX operation.
  - If the logical processor does not support VPIDs (IA32_VMX_PROCBASED_CTLS2[37]=0).
  - If the logical processor supports VPIDs (IA32_VMX_PROCBASED_CTLS2[37]=1) but does 
    not support the INVVPID instruction (IA32_VMX_EPT_VPID_CAP[32]=0).

So we should check both VPID enable bit in vmx exec control and INVVPID support bit 
in vmx capability MSRs to enable VPID. This patch adds the guarantee to not enable VPID
if INVVPID is not exposed in vmx capability MSRs.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
 arch/x86/kvm/vmx.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 06d8080..b310214 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -1239,6 +1239,11 @@ static inline bool cpu_has_vmx_invvpid_global(void)
 	return vmx_capability.vpid & VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT;
 }
 
+static inline bool cpu_has_vmx_invvpid(void)
+{
+	return vmx_capability.vpid & VMX_VPID_INVVPID_BIT;
+}
+
 static inline bool cpu_has_vmx_ept(void)
 {
 	return vmcs_config.cpu_based_2nd_exec_ctrl &
@@ -6519,8 +6524,10 @@ static __init int hardware_setup(void)
 	if (boot_cpu_has(X86_FEATURE_NX))
 		kvm_enable_efer_bits(EFER_NX);
 
-	if (!cpu_has_vmx_vpid())
+	if (!cpu_has_vmx_vpid() ||
+		!(cpu_has_vmx_invvpid()))
 		enable_vpid = 0;
+
 	if (!cpu_has_vmx_shadow_vmcs())
 		enable_shadow_vmcs = 0;
 	if (enable_shadow_vmcs)
-- 
2.7.4

[toc] | [next] | [standalone]

#1605439 — Re: [PATCH v2 2/3] KVM: VMX: Fix enable VPID even if INVVPID is not exposed in vmx capability

On 21.03.2017 05:18, Wanpeng Li wrote:
> From: Wanpeng Li <wanpeng.li@hotmail.com>
> 
> This can be reproduced by running L2 on L1, and disable VPID on L0 if w/o 
> commit "KVM: nVMX: Fix nested VPID vmx exec control", the L2 crash as below:
> 
> KVM: entry failed, hardware error 0x7
> EAX=00000000 EBX=00000000 ECX=00000000 EDX=000306c3
> ESI=00000000 EDI=00000000 EBP=00000000 ESP=00000000
> EIP=0000fff0 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
> ES =0000 00000000 0000ffff 00009300
> CS =f000 ffff0000 0000ffff 00009b00
> SS =0000 00000000 0000ffff 00009300
> DS =0000 00000000 0000ffff 00009300
> FS =0000 00000000 0000ffff 00009300
> GS =0000 00000000 0000ffff 00009300
> LDT=0000 00000000 0000ffff 00008200
> TR =0000 00000000 0000ffff 00008b00
> GDT=     00000000 0000ffff
> IDT=     00000000 0000ffff
> CR0=60000010 CR2=00000000 CR3=00000000 CR4=00000000
> DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000 
> DR6=00000000ffff0ff0 DR7=0000000000000400
> EFER=0000000000000000
> 
> Reference SDM 30.3 INVVPID:
>  
> Protected Mode Exceptions
> #UD 
>   - If not in VMX operation.
>   - If the logical processor does not support VPIDs (IA32_VMX_PROCBASED_CTLS2[37]=0).
>   - If the logical processor supports VPIDs (IA32_VMX_PROCBASED_CTLS2[37]=1) but does 
>     not support the INVVPID instruction (IA32_VMX_EPT_VPID_CAP[32]=0).
> 
> So we should check both VPID enable bit in vmx exec control and INVVPID support bit 
> in vmx capability MSRs to enable VPID. This patch adds the guarantee to not enable VPID
> if INVVPID is not exposed in vmx capability MSRs.
> 

Makes sense to me. Wonder how many systems are out there that have VPID
but not INVVPID? Or will this never happen on real hardware?

> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---
>  arch/x86/kvm/vmx.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 06d8080..b310214 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -1239,6 +1239,11 @@ static inline bool cpu_has_vmx_invvpid_global(void)
>  	return vmx_capability.vpid & VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT;
>  }
>  
> +static inline bool cpu_has_vmx_invvpid(void)
> +{
> +	return vmx_capability.vpid & VMX_VPID_INVVPID_BIT;
> +}
> +
>  static inline bool cpu_has_vmx_ept(void)
>  {
>  	return vmcs_config.cpu_based_2nd_exec_ctrl &
> @@ -6519,8 +6524,10 @@ static __init int hardware_setup(void)
>  	if (boot_cpu_has(X86_FEATURE_NX))
>  		kvm_enable_efer_bits(EFER_NX);
>  
> -	if (!cpu_has_vmx_vpid())
> +	if (!cpu_has_vmx_vpid() ||
> +		!(cpu_has_vmx_invvpid()))

This indentation looks weird. Can't this be fit into one line?

>  		enable_vpid = 0;
> +

unrelated change

>  	if (!cpu_has_vmx_shadow_vmcs())
>  		enable_shadow_vmcs = 0;
>  	if (enable_shadow_vmcs)
> 


-- 

Thanks,

David

[toc] | [prev] | [next] | [standalone]

#1605441 — Re: [PATCH v2 2/3] KVM: VMX: Fix enable VPID even if INVVPID is not exposed in vmx capability

2017-03-21 16:50 GMT+08:00 David Hildenbrand <david@redhat.com>:
> On 21.03.2017 05:18, Wanpeng Li wrote:
>> From: Wanpeng Li <wanpeng.li@hotmail.com>
>>
>> This can be reproduced by running L2 on L1, and disable VPID on L0 if w/o
>> commit "KVM: nVMX: Fix nested VPID vmx exec control", the L2 crash as below:
>>
>> KVM: entry failed, hardware error 0x7
>> EAX=00000000 EBX=00000000 ECX=00000000 EDX=000306c3
>> ESI=00000000 EDI=00000000 EBP=00000000 ESP=00000000
>> EIP=0000fff0 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
>> ES =0000 00000000 0000ffff 00009300
>> CS =f000 ffff0000 0000ffff 00009b00
>> SS =0000 00000000 0000ffff 00009300
>> DS =0000 00000000 0000ffff 00009300
>> FS =0000 00000000 0000ffff 00009300
>> GS =0000 00000000 0000ffff 00009300
>> LDT=0000 00000000 0000ffff 00008200
>> TR =0000 00000000 0000ffff 00008b00
>> GDT=     00000000 0000ffff
>> IDT=     00000000 0000ffff
>> CR0=60000010 CR2=00000000 CR3=00000000 CR4=00000000
>> DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000
>> DR6=00000000ffff0ff0 DR7=0000000000000400
>> EFER=0000000000000000
>>
>> Reference SDM 30.3 INVVPID:
>>
>> Protected Mode Exceptions
>> #UD
>>   - If not in VMX operation.
>>   - If the logical processor does not support VPIDs (IA32_VMX_PROCBASED_CTLS2[37]=0).
>>   - If the logical processor supports VPIDs (IA32_VMX_PROCBASED_CTLS2[37]=1) but does
>>     not support the INVVPID instruction (IA32_VMX_EPT_VPID_CAP[32]=0).
>>
>> So we should check both VPID enable bit in vmx exec control and INVVPID support bit
>> in vmx capability MSRs to enable VPID. This patch adds the guarantee to not enable VPID
>> if INVVPID is not exposed in vmx capability MSRs.
>>
>
> Makes sense to me. Wonder how many systems are out there that have VPID
> but not INVVPID? Or will this never happen on real hardware?

At least this will not happen on the real hardware on my hands.

>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Radim Krčmář <rkrcmar@redhat.com>
>> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
>> ---
>>  arch/x86/kvm/vmx.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 06d8080..b310214 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -1239,6 +1239,11 @@ static inline bool cpu_has_vmx_invvpid_global(void)
>>       return vmx_capability.vpid & VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT;
>>  }
>>
>> +static inline bool cpu_has_vmx_invvpid(void)
>> +{
>> +     return vmx_capability.vpid & VMX_VPID_INVVPID_BIT;
>> +}
>> +
>>  static inline bool cpu_has_vmx_ept(void)
>>  {
>>       return vmcs_config.cpu_based_2nd_exec_ctrl &
>> @@ -6519,8 +6524,10 @@ static __init int hardware_setup(void)
>>       if (boot_cpu_has(X86_FEATURE_NX))
>>               kvm_enable_efer_bits(EFER_NX);
>>
>> -     if (!cpu_has_vmx_vpid())
>> +     if (!cpu_has_vmx_vpid() ||
>> +             !(cpu_has_vmx_invvpid()))
>
> This indentation looks weird. Can't this be fit into one line?

The same as cpu_has_vmx_ept_4levels().

>
>>               enable_vpid = 0;
>> +
>
> unrelated change

To make the vpid codes more clear. Please refer to other callees in
hardware_setup().

>
>>       if (!cpu_has_vmx_shadow_vmcs())
>>               enable_shadow_vmcs = 0;
>>       if (enable_shadow_vmcs)
>>
>
>
> --
>
> Thanks,
>
> David

[toc] | [prev] | [next] | [standalone]

#1605449 — Re: [PATCH v2 2/3] KVM: VMX: Fix enable VPID even if INVVPID is not exposed in vmx capability

2017-03-21 17:01 GMT+08:00 David Hildenbrand <david@redhat.com>:
>
>> The same as cpu_has_vmx_ept_4levels().
>>
>>>
>>>>               enable_vpid = 0;
>>>> +
>>>
>>> unrelated change
>>
>> To make the vpid codes more clear. Please refer to other callees in
>> hardware_setup().
>
> I was talking about the added empty line. This is unrelated to the patch
> you're posting.

Yeah, I was talking about the same thing. Just a *small* cleanup and
it is suitable for the patch.

Regards,
Wanpeng Li

[toc] | [prev] | [next] | [standalone]

#1605451 — Re: [PATCH v2 2/3] KVM: VMX: Fix enable VPID even if INVVPID is not exposed in vmx capability

> The same as cpu_has_vmx_ept_4levels().
> 
>>
>>>               enable_vpid = 0;
>>> +
>>
>> unrelated change
> 
> To make the vpid codes more clear. Please refer to other callees in
> hardware_setup().

I was talking about the added empty line. This is unrelated to the patch
you're posting.

-- 

Thanks,

David

[toc] | [prev] | [next] | [standalone]

#1605464 — Re: [PATCH v2 2/3] KVM: VMX: Fix enable VPID even if INVVPID is not exposed in vmx capability

>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>> Cc: Radim Krčmář <rkrcmar@redhat.com>
>>> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
>>> ---
>>>  arch/x86/kvm/vmx.c | 9 ++++++++-
>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>> index 06d8080..b310214 100644
>>> --- a/arch/x86/kvm/vmx.c
>>> +++ b/arch/x86/kvm/vmx.c
>>> @@ -1239,6 +1239,11 @@ static inline bool cpu_has_vmx_invvpid_global(void)
>>>       return vmx_capability.vpid & VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT;
>>>  }
>>>
>>> +static inline bool cpu_has_vmx_invvpid(void)
>>> +{
>>> +     return vmx_capability.vpid & VMX_VPID_INVVPID_BIT;
>>> +}
>>> +
>>>  static inline bool cpu_has_vmx_ept(void)
>>>  {
>>>       return vmcs_config.cpu_based_2nd_exec_ctrl &
>>> @@ -6519,8 +6524,10 @@ static __init int hardware_setup(void)
>>>       if (boot_cpu_has(X86_FEATURE_NX))
>>>               kvm_enable_efer_bits(EFER_NX);
>>>
>>> -     if (!cpu_has_vmx_vpid())
>>> +     if (!cpu_has_vmx_vpid() ||
>>> +             !(cpu_has_vmx_invvpid()))
>>
>> This indentation looks weird. Can't this be fit into one line?
> 
> The same as cpu_has_vmx_ept_4levels().

I only know the general rules:

1. make things fit into one line unless it really harms readability
2. when splitting conditions over multiple lines, make them start at the
same level.

And I said, this indentation looks weird, because 1 and 2 are not met.

Anyhow, the general patch is fine in my opinion.

-- 

Thanks,

David

[toc] | [prev] | [next] | [standalone]

#1605776 — Re: [PATCH v2 2/3] KVM: VMX: Fix enable VPID even if INVVPID is not exposed in vmx capability

On 21/03/2017 05:18, Wanpeng Li wrote:
> -	if (!cpu_has_vmx_vpid())
> +	if (!cpu_has_vmx_vpid() ||
> +		!(cpu_has_vmx_invvpid()))

Too many parentheses and a useless line break.

Paolo

>  		enable_vpid = 0;

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web