| Message ID | 20170810133512.13442-3-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/08/2017 15:35, David Hildenbrand wrote: > We already always set that type but don't check if it is supported. Also > for nVMX, we only support WB for now. Let's just require it. > > Signed-off-by: David Hildenbrand <david@redhat.com> It may only happen on a broken nested hypervisor, so the patch is okay. Paolo > --- > arch/x86/kvm/vmx.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index f6638ed..023c6dc 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -1200,6 +1200,11 @@ static inline bool cpu_has_vmx_ept_4levels(void) > return vmx_capability.ept & VMX_EPT_PAGE_WALK_4_BIT; > } > > +static inline bool cpu_has_vmx_ept_mt_wb(void) > +{ > + return vmx_capability.ept & VMX_EPTP_WB_BIT; > +} > + > static inline bool cpu_has_vmx_ept_ad_bits(void) > { > return vmx_capability.ept & VMX_EPT_AD_BIT; > @@ -4302,7 +4307,6 @@ static u64 construct_eptp(struct kvm_vcpu *vcpu, unsigned long root_hpa) > { > u64 eptp = VMX_EPTP_MT_WB | VMX_EPTP_PWL_4; > > - /* TODO write the value reading from MSR */ > if (enable_ept_ad_bits && > (!is_guest_mode(vcpu) || nested_ept_ad_enabled(vcpu))) > eptp |= VMX_EPTP_AD_ENABLE_BIT; > @@ -6638,7 +6642,8 @@ static __init int hardware_setup(void) > init_vmcs_shadow_fields(); > > if (!cpu_has_vmx_ept() || > - !cpu_has_vmx_ept_4levels()) { > + !cpu_has_vmx_ept_4levels() || > + !cpu_has_vmx_ept_mt_wb()) { > enable_ept = 0; > enable_unrestricted_guest = 0; > enable_ept_ad_bits = 0; >
On Thu, Aug 10, 2017 at 04:42:02PM +0200, Paolo Bonzini wrote: > On 10/08/2017 15:35, David Hildenbrand wrote: > > We already always set that type but don't check if it is supported. Also > > for nVMX, we only support WB for now. Let's just require it. > > > > Signed-off-by: David Hildenbrand <david@redhat.com> > > It may only happen on a broken nested hypervisor, so the patch is okay. Is there one in the wild? > > Paolo > > > --- > > arch/x86/kvm/vmx.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > > index f6638ed..023c6dc 100644 > > --- a/arch/x86/kvm/vmx.c > > +++ b/arch/x86/kvm/vmx.c > > @@ -1200,6 +1200,11 @@ static inline bool cpu_has_vmx_ept_4levels(void) > > return vmx_capability.ept & VMX_EPT_PAGE_WALK_4_BIT; > > } > > > > +static inline bool cpu_has_vmx_ept_mt_wb(void) > > +{ > > + return vmx_capability.ept & VMX_EPTP_WB_BIT; > > +} > > + > > static inline bool cpu_has_vmx_ept_ad_bits(void) > > { > > return vmx_capability.ept & VMX_EPT_AD_BIT; > > @@ -4302,7 +4307,6 @@ static u64 construct_eptp(struct kvm_vcpu *vcpu, unsigned long root_hpa) > > { > > u64 eptp = VMX_EPTP_MT_WB | VMX_EPTP_PWL_4; > > > > - /* TODO write the value reading from MSR */ > > if (enable_ept_ad_bits && > > (!is_guest_mode(vcpu) || nested_ept_ad_enabled(vcpu))) > > eptp |= VMX_EPTP_AD_ENABLE_BIT; > > @@ -6638,7 +6642,8 @@ static __init int hardware_setup(void) > > init_vmcs_shadow_fields(); > > > > if (!cpu_has_vmx_ept() || > > - !cpu_has_vmx_ept_4levels()) { > > + !cpu_has_vmx_ept_4levels() || > > + !cpu_has_vmx_ept_mt_wb()) { > > enable_ept = 0; > > enable_unrestricted_guest = 0; > > enable_ept_ad_bits = 0; > > >
David Hildenbrand <david@redhat.com> writes: > We already always set that type but don't check if it is supported. Also > for nVMX, we only support WB for now. Let's just require it. Out of curiosity, is there old hardware that supports ept but not WB ? Bandan > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > arch/x86/kvm/vmx.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index f6638ed..023c6dc 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -1200,6 +1200,11 @@ static inline bool cpu_has_vmx_ept_4levels(void) > return vmx_capability.ept & VMX_EPT_PAGE_WALK_4_BIT; > } > > +static inline bool cpu_has_vmx_ept_mt_wb(void) > +{ > + return vmx_capability.ept & VMX_EPTP_WB_BIT; > +} > + > static inline bool cpu_has_vmx_ept_ad_bits(void) > { > return vmx_capability.ept & VMX_EPT_AD_BIT; > @@ -4302,7 +4307,6 @@ static u64 construct_eptp(struct kvm_vcpu *vcpu, unsigned long root_hpa) > { > u64 eptp = VMX_EPTP_MT_WB | VMX_EPTP_PWL_4; > > - /* TODO write the value reading from MSR */ > if (enable_ept_ad_bits && > (!is_guest_mode(vcpu) || nested_ept_ad_enabled(vcpu))) > eptp |= VMX_EPTP_AD_ENABLE_BIT; > @@ -6638,7 +6642,8 @@ static __init int hardware_setup(void) > init_vmcs_shadow_fields(); > > if (!cpu_has_vmx_ept() || > - !cpu_has_vmx_ept_4levels()) { > + !cpu_has_vmx_ept_4levels() || > + !cpu_has_vmx_ept_mt_wb()) { > enable_ept = 0; > enable_unrestricted_guest = 0; > enable_ept_ad_bits = 0;
On 10.08.2017 21:40, Bandan Das wrote: > David Hildenbrand <david@redhat.com> writes: > >> We already always set that type but don't check if it is supported. Also >> for nVMX, we only support WB for now. Let's just require it. > > Out of curiosity, is there old hardware that supports ept but not WB ? > I guess no. KVM always sets up WB for all eptps. KVM would in the current state simply not work on these machines (vm entry failure). I guess the same is also true for 4level page-walk length. This should also be part of any EPT implementation but still we have to check for it.
On 10.08.2017 20:30, Konrad Rzeszutek Wilk wrote: > On Thu, Aug 10, 2017 at 04:42:02PM +0200, Paolo Bonzini wrote: >> On 10/08/2017 15:35, David Hildenbrand wrote: >>> We already always set that type but don't check if it is supported. Also >>> for nVMX, we only support WB for now. Let's just require it. >>> >>> Signed-off-by: David Hildenbrand <david@redhat.com> >> >> It may only happen on a broken nested hypervisor, so the patch is okay. > > Is there one in the wild? I think only the following scenario would see a change: Hypervisor supports but doesn't announce the WB memory type for EPT. KVM runs as nested hypervisor. It will now refuse to use EPT but should continue to work without EPT. In this case, the hypervisor would have a BUG and we as a nested hypervisor would do the right thing. >> >> Paolo >>
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index f6638ed..023c6dc 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -1200,6 +1200,11 @@ static inline bool cpu_has_vmx_ept_4levels(void) return vmx_capability.ept & VMX_EPT_PAGE_WALK_4_BIT; } +static inline bool cpu_has_vmx_ept_mt_wb(void) +{ + return vmx_capability.ept & VMX_EPTP_WB_BIT; +} + static inline bool cpu_has_vmx_ept_ad_bits(void) { return vmx_capability.ept & VMX_EPT_AD_BIT; @@ -4302,7 +4307,6 @@ static u64 construct_eptp(struct kvm_vcpu *vcpu, unsigned long root_hpa) { u64 eptp = VMX_EPTP_MT_WB | VMX_EPTP_PWL_4; - /* TODO write the value reading from MSR */ if (enable_ept_ad_bits && (!is_guest_mode(vcpu) || nested_ept_ad_enabled(vcpu))) eptp |= VMX_EPTP_AD_ENABLE_BIT; @@ -6638,7 +6642,8 @@ static __init int hardware_setup(void) init_vmcs_shadow_fields(); if (!cpu_has_vmx_ept() || - !cpu_has_vmx_ept_4levels()) { + !cpu_has_vmx_ept_4levels() || + !cpu_has_vmx_ept_mt_wb()) { enable_ept = 0; enable_unrestricted_guest = 0; enable_ept_ad_bits = 0;
We already always set that type but don't check if it is supported. Also for nVMX, we only support WB for now. Let's just require it. Signed-off-by: David Hildenbrand <david@redhat.com> --- arch/x86/kvm/vmx.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)