Re: [PATCH v5] KVM: VMX: Fix enable VPID conditions

[Paolo Bonzini]

On 23/03/2017 13:30, Wanpeng Li wrote:
> From: Wanpeng Li 
> 
> 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= EBX= ECX= EDX=000306c3
> ESI= EDI= EBP= ESP=
> EIP=fff0 EFL=0002 [---] CPL=0 II=0 A20=1 SMM=0 HLT=0
> ES =   9300
> CS =f000   9b00
> SS =   9300
> DS =   9300
> FS =   9300
> GS =   9300
> LDT=   8200
> TR =   8b00
> GDT=  
> IDT=  
> CR0=6010 CR2= CR3= CR4=
> DR0= DR1= DR2= 
> DR3= 
> DR6=0ff0 DR7=0400
> EFER=
> 
> 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 either INVVPID or single-context/all-context invalidation is not 
> exposed in 
> vmx capability MSRs.
> 
> Reviewed-by: David Hildenbrand 
> Cc: David Hildenbrand 
> Cc: Jim Mattson 
> Cc: Paolo Bonzini 
> Cc: Radim Krčmář 
> Signed-off-by: Wanpeng Li 
> ---
>  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 8795a70..8925c76 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 &
> @@ -6518,8 +6523,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() ||
> + !(cpu_has_vmx_invvpid_single() || cpu_has_vmx_invvpid_global()))
>   enable_vpid = 0;
> +
>   if (!cpu_has_vmx_shadow_vmcs())
>   enable_shadow_vmcs = 0;
>   if (enable_shadow_vmcs)
> 

Queued, thanks.

Paolo

Re: [PATCH v5] KVM: VMX: Fix enable VPID conditions

[Paolo Bonzini]

On 23/03/2017 13:30, Wanpeng Li wrote:
> From: Wanpeng Li 
> 
> 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= EBX= ECX= EDX=000306c3
> ESI= EDI= EBP= ESP=
> EIP=fff0 EFL=0002 [---] CPL=0 II=0 A20=1 SMM=0 HLT=0
> ES =   9300
> CS =f000   9b00
> SS =   9300
> DS =   9300
> FS =   9300
> GS =   9300
> LDT=   8200
> TR =   8b00
> GDT=  
> IDT=  
> CR0=6010 CR2= CR3= CR4=
> DR0= DR1= DR2= 
> DR3= 
> DR6=0ff0 DR7=0400
> EFER=
> 
> 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 either INVVPID or single-context/all-context invalidation is not 
> exposed in 
> vmx capability MSRs.
> 
> Reviewed-by: David Hildenbrand 
> Cc: David Hildenbrand 
> Cc: Jim Mattson 
> Cc: Paolo Bonzini 
> Cc: Radim Krčmář 
> Signed-off-by: Wanpeng Li 
> ---
>  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 8795a70..8925c76 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 &
> @@ -6518,8 +6523,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() ||
> + !(cpu_has_vmx_invvpid_single() || cpu_has_vmx_invvpid_global()))
>   enable_vpid = 0;
> +
>   if (!cpu_has_vmx_shadow_vmcs())
>   enable_shadow_vmcs = 0;
>   if (enable_shadow_vmcs)
> 

Queued, thanks.

Paolo

Re: [PATCH v5] KVM: VMX: Fix enable VPID conditions

[Jim Mattson]

On Thu, Mar 23, 2017 at 5:30 AM, Wanpeng Li  wrote:
> From: Wanpeng Li 
>
> 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= EBX= ECX= EDX=000306c3
> ESI= EDI= EBP= ESP=
> EIP=fff0 EFL=0002 [---] CPL=0 II=0 A20=1 SMM=0 HLT=0
> ES =   9300
> CS =f000   9b00
> SS =   9300
> DS =   9300
> FS =   9300
> GS =   9300
> LDT=   8200
> TR =   8b00
> GDT=  
> IDT=  
> CR0=6010 CR2= CR3= CR4=
> DR0= DR1= DR2= 
> DR3=
> DR6=0ff0 DR7=0400
> EFER=
>
> 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 either INVVPID or single-context/all-context invalidation is not 
> exposed in
> vmx capability MSRs.
>
> Reviewed-by: David Hildenbrand 
Reviewed-by: Jim Mattson 
> Cc: David Hildenbrand 
> Cc: Jim Mattson 
> Cc: Paolo Bonzini 
> Cc: Radim Krčmář 
> Signed-off-by: Wanpeng Li 
> ---
>  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 8795a70..8925c76 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 &
> @@ -6518,8 +6523,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() ||
> +   !(cpu_has_vmx_invvpid_single() || 
> cpu_has_vmx_invvpid_global()))
> enable_vpid = 0;
> +
> if (!cpu_has_vmx_shadow_vmcs())
> enable_shadow_vmcs = 0;
> if (enable_shadow_vmcs)
> --
> 2.7.4
>

Re: [PATCH v5] KVM: VMX: Fix enable VPID conditions

[Jim Mattson]

On Thu, Mar 23, 2017 at 5:30 AM, Wanpeng Li  wrote:
> From: Wanpeng Li 
>
> 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= EBX= ECX= EDX=000306c3
> ESI= EDI= EBP= ESP=
> EIP=fff0 EFL=0002 [---] CPL=0 II=0 A20=1 SMM=0 HLT=0
> ES =   9300
> CS =f000   9b00
> SS =   9300
> DS =   9300
> FS =   9300
> GS =   9300
> LDT=   8200
> TR =   8b00
> GDT=  
> IDT=  
> CR0=6010 CR2= CR3= CR4=
> DR0= DR1= DR2= 
> DR3=
> DR6=0ff0 DR7=0400
> EFER=
>
> 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 either INVVPID or single-context/all-context invalidation is not 
> exposed in
> vmx capability MSRs.
>
> Reviewed-by: David Hildenbrand 
Reviewed-by: Jim Mattson 
> Cc: David Hildenbrand 
> Cc: Jim Mattson 
> Cc: Paolo Bonzini 
> Cc: Radim Krčmář 
> Signed-off-by: Wanpeng Li 
> ---
>  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 8795a70..8925c76 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 &
> @@ -6518,8 +6523,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() ||
> +   !(cpu_has_vmx_invvpid_single() || 
> cpu_has_vmx_invvpid_global()))
> enable_vpid = 0;
> +
> if (!cpu_has_vmx_shadow_vmcs())
> enable_shadow_vmcs = 0;
> if (enable_shadow_vmcs)
> --
> 2.7.4
>

Re: [PATCH v5] KVM: VMX: Fix enable VPID conditions

[David Hildenbrand]

> - if (!cpu_has_vmx_vpid())
> + if (!cpu_has_vmx_vpid() || !cpu_has_vmx_invvpid() ||
> + !(cpu_has_vmx_invvpid_single() || cpu_has_vmx_invvpid_global()))

I still don't like this way of indentation, but looks like I am the only
one complaining :)

So I think this patch is just fine now.

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


-- 

Thanks,

David

Re: [PATCH v5] KVM: VMX: Fix enable VPID conditions

[David Hildenbrand]

> - if (!cpu_has_vmx_vpid())
> + if (!cpu_has_vmx_vpid() || !cpu_has_vmx_invvpid() ||
> + !(cpu_has_vmx_invvpid_single() || cpu_has_vmx_invvpid_global()))

I still don't like this way of indentation, but looks like I am the only
one complaining :)

So I think this patch is just fine now.

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


-- 

Thanks,

David