| Message ID | 1508213038-5593-2-git-send-email-wanpeng.li@hotmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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;
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
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
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