[PATCH v2 1/2] KVM: VMX: Don't advertise EPT switching if EPT itself is not exposed

[PATCH v2 1/2] KVM: VMX: Don't advertise EPT switching if EPT itself is not exposed

[Wanpeng Li]

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

I can use vmxcap tool to observe "EPTP Switching   yes" even if EPT is not 
exposed to L1.

EPT switching is advertised unconditionally since it is emulated, however, 
it can be treated as an extended feature for EPT and it should not be 
advertised if EPT itself is not exposed. This patch fixes it.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Jim Mattson <jmattson@google.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
v1 -> v2:
 * don't advertise "EPT VM Functions" in secondary processor-based VM-execution 
   controls if we don't actually support any VM Functions.

 arch/x86/kvm/vmx.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index c460b0b..a6861ca 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2842,8 +2842,12 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx)
 		 * Advertise EPTP switching unconditionally
 		 * since we emulate it
 		 */
-		vmx->nested.nested_vmx_vmfunc_controls =
-			VMX_VMFUNC_EPTP_SWITCHING;
+		if (enable_ept) {
+			vmx->nested.nested_vmx_vmfunc_controls =
+				VMX_VMFUNC_EPTP_SWITCHING;
+		} else
+			vmx->nested.nested_vmx_secondary_ctls_high &=
+				~SECONDARY_EXEC_ENABLE_VMFUNC;
 	}
 
 	/*
-- 
2.7.4

[PATCH v2 2/2] KVM: VMX: Fix VPID capability detection

[Wanpeng Li]

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

In my setup, EPT is not exposed to L1, the VPID capability is exposed and 
can be observed by vmxcap tool in L1:
INVVPID supported                        yes
Individual-address INVVPID               yes
Single-context INVVPID                   yes
All-context INVVPID                      yes
Single-context-retaining-globals INVVPID yes

However, the module parameter of VPID observed in L1 is always N, the
cpu_has_vmx_invvpid() check in L1 KVM fails since vmx_capability.vpid
is 0 and it is not read from MSR due to EPT is not exposed. 

The VPID can be used to tag linear mappings when EPT is not enabled. However,
current logic just detects VPID capability if EPT is enabled, this patch
fixes it.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Jim Mattson <jmattson@google.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
v1 -> v2:
 * rdmsr_safe instead of rdmsr
 * add more explanation to patch description

 arch/x86/kvm/vmx.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index a6861ca..bf804e5 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3684,15 +3684,19 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf)
 				SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE |
 				SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
 
+	rdmsr_safe(MSR_IA32_VMX_EPT_VPID_CAP,
+		&vmx_capability.ept, &vmx_capability.vpid);
+
 	if (_cpu_based_2nd_exec_control & SECONDARY_EXEC_ENABLE_EPT) {
 		/* CR3 accesses and invlpg don't need to cause VM Exits when EPT
 		   enabled */
 		_cpu_based_exec_control &= ~(CPU_BASED_CR3_LOAD_EXITING |
 					     CPU_BASED_CR3_STORE_EXITING |
 					     CPU_BASED_INVLPG_EXITING);
-		rdmsr(MSR_IA32_VMX_EPT_VPID_CAP,
-		      vmx_capability.ept, vmx_capability.vpid);
-	}
+	} else
+		vmx_capability.ept = 0;
+	if (!(_cpu_based_2nd_exec_control & SECONDARY_EXEC_ENABLE_VPID))
+		vmx_capability.vpid = 0;
 
 	min = VM_EXIT_SAVE_DEBUG_CONTROLS | VM_EXIT_ACK_INTR_ON_EXIT;
 #ifdef CONFIG_X86_64
-- 
2.7.4

Re: [PATCH v2 2/2] KVM: VMX: Fix VPID capability detection

[Jim Mattson]

On Mon, Oct 16, 2017 at 9:03 PM, Wanpeng Li <kernellwp@gmail.com> wrote:
> From: Wanpeng Li <wanpeng.li@hotmail.com>
>
> In my setup, EPT is not exposed to L1, the VPID capability is exposed and
> can be observed by vmxcap tool in L1:
> INVVPID supported                        yes
> Individual-address INVVPID               yes
> Single-context INVVPID                   yes
> All-context INVVPID                      yes
> Single-context-retaining-globals INVVPID yes
>
> However, the module parameter of VPID observed in L1 is always N, the
> cpu_has_vmx_invvpid() check in L1 KVM fails since vmx_capability.vpid
> is 0 and it is not read from MSR due to EPT is not exposed.
>
> The VPID can be used to tag linear mappings when EPT is not enabled. However,
> current logic just detects VPID capability if EPT is enabled, this patch
> fixes it.
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Cc: Jim Mattson <jmattson@google.com>
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---
> v1 -> v2:
>  * rdmsr_safe instead of rdmsr
>  * add more explanation to patch description
>
>  arch/x86/kvm/vmx.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index a6861ca..bf804e5 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -3684,15 +3684,19 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf)
>                                 SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE |
>                                 SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
>
> +       rdmsr_safe(MSR_IA32_VMX_EPT_VPID_CAP,
> +               &vmx_capability.ept, &vmx_capability.vpid);
> +
>         if (_cpu_based_2nd_exec_control & SECONDARY_EXEC_ENABLE_EPT) {
>                 /* CR3 accesses and invlpg don't need to cause VM Exits when EPT
>                    enabled */
>                 _cpu_based_exec_control &= ~(CPU_BASED_CR3_LOAD_EXITING |
>                                              CPU_BASED_CR3_STORE_EXITING |
>                                              CPU_BASED_INVLPG_EXITING);
> -               rdmsr(MSR_IA32_VMX_EPT_VPID_CAP,
> -                     vmx_capability.ept, vmx_capability.vpid);
> -       }
> +       } else
> +               vmx_capability.ept = 0;
I would expect vmx_capability.ept to already be 0 here. Otherwise, L0
is reporting inconsistent VMX capabilities.

> +       if (!(_cpu_based_2nd_exec_control & SECONDARY_EXEC_ENABLE_VPID))
> +               vmx_capability.vpid = 0;
I would expect vmx_capability.vpid to already be 0 here. Otherwise, L0
is reporting inconsistent VMX capabilities.

>
>         min = VM_EXIT_SAVE_DEBUG_CONTROLS | VM_EXIT_ACK_INTR_ON_EXIT;
>  #ifdef CONFIG_X86_64
> --
> 2.7.4
>

We already have the following code in nested_vmx_setup_ctls_msrs:

if (enable_ept) {
...
} else
        vmx->nested.nested_vmx_ept_caps = 0;

and

if (enable_vpid) {
...
} else
        vmx->nested.nested_vmx_vpid_caps = 0;

Re: [PATCH v2 2/2] KVM: VMX: Fix VPID capability detection

[Paolo Bonzini]

On 17/10/2017 19:43, Jim Mattson wrote:
>> +               &vmx_capability.ept, &vmx_capability.vpid);
>> +
>>         if (_cpu_based_2nd_exec_control & SECONDARY_EXEC_ENABLE_EPT) {
>>                 /* CR3 accesses and invlpg don't need to cause VM Exits when EPT
>>                    enabled */
>>                 _cpu_based_exec_control &= ~(CPU_BASED_CR3_LOAD_EXITING |
>>                                              CPU_BASED_CR3_STORE_EXITING |
>>                                              CPU_BASED_INVLPG_EXITING);
>> -               rdmsr(MSR_IA32_VMX_EPT_VPID_CAP,
>> -                     vmx_capability.ept, vmx_capability.vpid);
>> -       }
>> +       } else
>> +               vmx_capability.ept = 0;
> I would expect vmx_capability.ept to already be 0 here. Otherwise, L0
> is reporting inconsistent VMX capabilities.
> 
>> +       if (!(_cpu_based_2nd_exec_control & SECONDARY_EXEC_ENABLE_VPID))
>> +               vmx_capability.vpid = 0;
> I would expect vmx_capability.vpid to already be 0 here. Otherwise, L0
> is reporting inconsistent VMX capabilities.
> 

That's true, but I think it's better to be safe.  Maybe add a pr_warn if
it is not zero?

Paolo

Re: [PATCH v2 2/2] KVM: VMX: Fix VPID capability detection

[Wanpeng Li]

2017-10-18 1:48 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
> On 17/10/2017 19:43, Jim Mattson wrote:
>>> +               &vmx_capability.ept, &vmx_capability.vpid);
>>> +
>>>         if (_cpu_based_2nd_exec_control & SECONDARY_EXEC_ENABLE_EPT) {
>>>                 /* CR3 accesses and invlpg don't need to cause VM Exits when EPT
>>>                    enabled */
>>>                 _cpu_based_exec_control &= ~(CPU_BASED_CR3_LOAD_EXITING |
>>>                                              CPU_BASED_CR3_STORE_EXITING |
>>>                                              CPU_BASED_INVLPG_EXITING);
>>> -               rdmsr(MSR_IA32_VMX_EPT_VPID_CAP,
>>> -                     vmx_capability.ept, vmx_capability.vpid);
>>> -       }
>>> +       } else
>>> +               vmx_capability.ept = 0;
>> I would expect vmx_capability.ept to already be 0 here. Otherwise, L0
>> is reporting inconsistent VMX capabilities.
>>
>>> +       if (!(_cpu_based_2nd_exec_control & SECONDARY_EXEC_ENABLE_VPID))
>>> +               vmx_capability.vpid = 0;
>> I would expect vmx_capability.vpid to already be 0 here. Otherwise, L0
>> is reporting inconsistent VMX capabilities.
>>
>
> That's true, but I think it's better to be safe.  Maybe add a pr_warn if
> it is not zero?

Will do in v3.

Regards,
Wanpeng Li

Re: [PATCH v2 1/2] KVM: VMX: Don't advertise EPT switching if EPT itself is not exposed

[Jim Mattson]

Following the same line of reasoning, what if
vmx->nested.nested_vmx_secondary_ctls_high is 0 after clearing
SECONDARY_EXEC_ENABLE_VMFUNC? Does it make sense to report
CPU_BASED_ACTIVATE_SECONDARY_CONTROLS if we don't actually support any
of the secondary controls?

Reviewed-by: Jim Mattson <jmattson@google.com>

On Mon, Oct 16, 2017 at 9:03 PM, Wanpeng Li <kernellwp@gmail.com> wrote:
> From: Wanpeng Li <wanpeng.li@hotmail.com>
>
> I can use vmxcap tool to observe "EPTP Switching   yes" even if EPT is not
> exposed to L1.
>
> EPT switching is advertised unconditionally since it is emulated, however,
> it can be treated as an extended feature for EPT and it should not be
> advertised if EPT itself is not exposed. This patch fixes it.
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Cc: Jim Mattson <jmattson@google.com>
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---
> v1 -> v2:
>  * don't advertise "EPT VM Functions" in secondary processor-based VM-execution
>    controls if we don't actually support any VM Functions.
>
>  arch/x86/kvm/vmx.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index c460b0b..a6861ca 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2842,8 +2842,12 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx)
>                  * Advertise EPTP switching unconditionally
>                  * since we emulate it
>                  */
> -               vmx->nested.nested_vmx_vmfunc_controls =
> -                       VMX_VMFUNC_EPTP_SWITCHING;
> +               if (enable_ept) {
> +                       vmx->nested.nested_vmx_vmfunc_controls =
> +                               VMX_VMFUNC_EPTP_SWITCHING;
> +               } else
> +                       vmx->nested.nested_vmx_secondary_ctls_high &=
> +                               ~SECONDARY_EXEC_ENABLE_VMFUNC;
>         }
>
>         /*
> --
> 2.7.4
>

Re: [PATCH v2 1/2] KVM: VMX: Don't advertise EPT switching if EPT itself is not exposed

[Paolo Bonzini]

On 17/10/2017 19:29, Jim Mattson wrote:
> Following the same line of reasoning, what if
> vmx->nested.nested_vmx_secondary_ctls_high is 0 after clearing
> SECONDARY_EXEC_ENABLE_VMFUNC? Does it make sense to report
> CPU_BASED_ACTIVATE_SECONDARY_CONTROLS if we don't actually support any
> of the secondary controls?

All-zero is a valid value for secondary controls, so I think yes.  Besides:

1) userspace can always get into a situation where there are no valid
secondary controls but processor-based execution controls have bit 31 as
1-allowed;

2) I doubt that vmfunc can be the one bit that causes
nested_vmx_secondary_ctls_high to become zero :)

Paolo

Re: [PATCH v2 1/2] KVM: VMX: Don't advertise EPT switching if EPT itself is not exposed

[Jim Mattson]

Similarly, it is legal for the IA32_VMX_VMFUNC MSR to report all-zero.

For consistency, perhaps we should not clear the "enable VM functions"
capability in the IA32_VMX_PROCBASED_CTLS2 MSR just because we do not
support any VM functions.

On Tue, Oct 17, 2017 at 10:35 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 17/10/2017 19:29, Jim Mattson wrote:
>> Following the same line of reasoning, what if
>> vmx->nested.nested_vmx_secondary_ctls_high is 0 after clearing
>> SECONDARY_EXEC_ENABLE_VMFUNC? Does it make sense to report
>> CPU_BASED_ACTIVATE_SECONDARY_CONTROLS if we don't actually support any
>> of the secondary controls?
>
> All-zero is a valid value for secondary controls, so I think yes.  Besides:
>
> 1) userspace can always get into a situation where there are no valid
> secondary controls but processor-based execution controls have bit 31 as
> 1-allowed;
>
> 2) I doubt that vmfunc can be the one bit that causes
> nested_vmx_secondary_ctls_high to become zero :)
>
> Paolo

Re: [PATCH v2 1/2] KVM: VMX: Don't advertise EPT switching if EPT itself is not exposed

[Wanpeng Li]

2017-10-18 2:30 GMT+08:00 Jim Mattson <jmattson@google.com>:
> Similarly, it is legal for the IA32_VMX_VMFUNC MSR to report all-zero.
>
> For consistency, perhaps we should not clear the "enable VM functions"
> capability in the IA32_VMX_PROCBASED_CTLS2 MSR just because we do not
> support any VM functions.

Agreed.

Regards,
Wanpeng Li

>
> On Tue, Oct 17, 2017 at 10:35 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 17/10/2017 19:29, Jim Mattson wrote:
>>> Following the same line of reasoning, what if
>>> vmx->nested.nested_vmx_secondary_ctls_high is 0 after clearing
>>> SECONDARY_EXEC_ENABLE_VMFUNC? Does it make sense to report
>>> CPU_BASED_ACTIVATE_SECONDARY_CONTROLS if we don't actually support any
>>> of the secondary controls?
>>
>> All-zero is a valid value for secondary controls, so I think yes.  Besides:
>>
>> 1) userspace can always get into a situation where there are no valid
>> secondary controls but processor-based execution controls have bit 31 as
>> 1-allowed;
>>
>> 2) I doubt that vmfunc can be the one bit that causes
>> nested_vmx_secondary_ctls_high to become zero :)
>>
>> Paolo