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