[PATCH v1 0/2] KVM: VMX: require EPTP WB (write-back) support

[PATCH v1 0/2] KVM: VMX: require EPTP WB (write-back) support

[David Hildenbrand]

I also have a patch that allows VMX and nVMX to use the UC (unchached)
memory type.
.
However I am not sure if we have to take care of anything special in that
case? Any insights?

David Hildenbrand (2):
  KVM: VMX: cleanup EPTP definitions
  KVM: VMX: always require WB memory type for EPT

 arch/x86/include/asm/vmx.h | 11 ++++++-----
 arch/x86/kvm/vmx.c         | 34 ++++++++++++++++++----------------
 2 files changed, 24 insertions(+), 21 deletions(-)

-- 
2.9.4

[PATCH v1 1/2] KVM: VMX: cleanup EPTP definitions

[David Hildenbrand]

Don't use shifts, tag them correctly as EPTP and use better matching
names (PWL vs. GAW).

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/x86/include/asm/vmx.h | 11 ++++++-----
 arch/x86/kvm/vmx.c         | 25 +++++++++++--------------
 2 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
index 5f63a2e..929b3fc 100644
--- a/arch/x86/include/asm/vmx.h
+++ b/arch/x86/include/asm/vmx.h
@@ -468,12 +468,13 @@ enum vmcs_field {
 #define VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT      (1ull << 10) /* (42 - 32) */
 #define VMX_VPID_EXTENT_SINGLE_NON_GLOBAL_BIT   (1ull << 11) /* (43 - 32) */
 
-#define VMX_EPT_DEFAULT_GAW			3
-#define VMX_EPT_MAX_GAW				0x4
 #define VMX_EPT_MT_EPTE_SHIFT			3
-#define VMX_EPT_GAW_EPTP_SHIFT			3
-#define VMX_EPT_AD_ENABLE_BIT			(1ull << 6)
-#define VMX_EPT_DEFAULT_MT			0x6ull
+#define VMX_EPTP_PWL_MASK			0x38ull
+#define VMX_EPTP_PWL_4				0x38ull
+#define VMX_EPTP_AD_ENABLE_BIT			(1ull << 6)
+#define VMX_EPTP_MT_MASK			0x7ull
+#define VMX_EPTP_MT_WB				0x6ull
+#define VMX_EPTP_MT_UC				0x0ull
 #define VMX_EPT_READABLE_MASK			0x1ull
 #define VMX_EPT_WRITABLE_MASK			0x2ull
 #define VMX_EPT_EXECUTABLE_MASK			0x4ull
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index c483f99..f6638ed 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -4300,14 +4300,12 @@ static void vmx_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
 
 static u64 construct_eptp(struct kvm_vcpu *vcpu, unsigned long root_hpa)
 {
-	u64 eptp;
+	u64 eptp = VMX_EPTP_MT_WB | VMX_EPTP_PWL_4;
 
 	/* TODO write the value reading from MSR */
-	eptp = VMX_EPT_DEFAULT_MT |
-		VMX_EPT_DEFAULT_GAW << VMX_EPT_GAW_EPTP_SHIFT;
 	if (enable_ept_ad_bits &&
 	    (!is_guest_mode(vcpu) || nested_ept_ad_enabled(vcpu)))
-		eptp |= VMX_EPT_AD_ENABLE_BIT;
+		eptp |= VMX_EPTP_AD_ENABLE_BIT;
 	eptp |= (root_hpa & PAGE_MASK);
 
 	return eptp;
@@ -7879,16 +7877,15 @@ static int handle_preemption_timer(struct kvm_vcpu *vcpu)
 static bool valid_ept_address(struct kvm_vcpu *vcpu, u64 address)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
-	u64 mask = address & 0x7;
 	int maxphyaddr = cpuid_maxphyaddr(vcpu);
 
 	/* Check for memory type validity */
-	switch (mask) {
-	case 0:
+	switch (address & VMX_EPTP_MT_MASK) {
+	case VMX_EPTP_MT_UC:
 		if (!(vmx->nested.nested_vmx_ept_caps & VMX_EPTP_UC_BIT))
 			return false;
 		break;
-	case 6:
+	case VMX_EPTP_MT_WB:
 		if (!(vmx->nested.nested_vmx_ept_caps & VMX_EPTP_WB_BIT))
 			return false;
 		break;
@@ -7896,8 +7893,8 @@ static bool valid_ept_address(struct kvm_vcpu *vcpu, u64 address)
 		return false;
 	}
 
-	/* Bits 5:3 must be 3 */
-	if (((address >> VMX_EPT_GAW_EPTP_SHIFT) & 0x7) != VMX_EPT_DEFAULT_GAW)
+	/* only 4 levels page-walk length are valid */
+	if ((address & VMX_EPTP_PWL_MASK) != VMX_EPTP_PWL_4)
 		return false;
 
 	/* Reserved bits should not be set */
@@ -7905,7 +7902,7 @@ static bool valid_ept_address(struct kvm_vcpu *vcpu, u64 address)
 		return false;
 
 	/* AD, if set, should be supported */
-	if ((address & VMX_EPT_AD_ENABLE_BIT)) {
+	if (address & VMX_EPTP_AD_ENABLE_BIT) {
 		if (!(vmx->nested.nested_vmx_ept_caps & VMX_EPT_AD_BIT))
 			return false;
 	}
@@ -7933,7 +7930,7 @@ static int nested_vmx_eptp_switching(struct kvm_vcpu *vcpu,
 				     &address, index * 8, 8))
 		return 1;
 
-	accessed_dirty = !!(address & VMX_EPT_AD_ENABLE_BIT);
+	accessed_dirty = !!(address & VMX_EPTP_AD_ENABLE_BIT);
 
 	/*
 	 * If the (L2) guest does a vmfunc to the currently
@@ -9499,7 +9496,7 @@ static void __init vmx_check_processor_compat(void *rtn)
 
 static int get_ept_level(void)
 {
-	return VMX_EPT_DEFAULT_GAW + 1;
+	return 4;
 }
 
 static u64 vmx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
@@ -9702,7 +9699,7 @@ static void nested_ept_inject_page_fault(struct kvm_vcpu *vcpu,
 
 static bool nested_ept_ad_enabled(struct kvm_vcpu *vcpu)
 {
-	return nested_ept_get_cr3(vcpu) & VMX_EPT_AD_ENABLE_BIT;
+	return nested_ept_get_cr3(vcpu) & VMX_EPTP_AD_ENABLE_BIT;
 }
 
 /* Callbacks for nested_ept_init_mmu_context: */
-- 
2.9.4

Re: [PATCH v1 1/2] KVM: VMX: cleanup EPTP definitions

[Bandan Das]

David Hildenbrand <david@redhat.com> writes:

> Don't use shifts, tag them correctly as EPTP and use better matching
> names (PWL vs. GAW).

Fair enough. This reminds me, I also don't particularly like the vmx->nested.nested...
accesses that are spread all throughout :)

> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  arch/x86/include/asm/vmx.h | 11 ++++++-----
>  arch/x86/kvm/vmx.c         | 25 +++++++++++--------------
>  2 files changed, 17 insertions(+), 19 deletions(-)
>
> diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
> index 5f63a2e..929b3fc 100644
> --- a/arch/x86/include/asm/vmx.h
> +++ b/arch/x86/include/asm/vmx.h
> @@ -468,12 +468,13 @@ enum vmcs_field {
>  #define VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT      (1ull << 10) /* (42 - 32) */
>  #define VMX_VPID_EXTENT_SINGLE_NON_GLOBAL_BIT   (1ull << 11) /* (43 - 32) */
>  
> -#define VMX_EPT_DEFAULT_GAW			3
> -#define VMX_EPT_MAX_GAW				0x4
>  #define VMX_EPT_MT_EPTE_SHIFT			3
> -#define VMX_EPT_GAW_EPTP_SHIFT			3
> -#define VMX_EPT_AD_ENABLE_BIT			(1ull << 6)
> -#define VMX_EPT_DEFAULT_MT			0x6ull
> +#define VMX_EPTP_PWL_MASK			0x38ull
> +#define VMX_EPTP_PWL_4				0x38ull
> +#define VMX_EPTP_AD_ENABLE_BIT			(1ull << 6)
> +#define VMX_EPTP_MT_MASK			0x7ull
> +#define VMX_EPTP_MT_WB				0x6ull
> +#define VMX_EPTP_MT_UC				0x0ull
>  #define VMX_EPT_READABLE_MASK			0x1ull
>  #define VMX_EPT_WRITABLE_MASK			0x2ull
>  #define VMX_EPT_EXECUTABLE_MASK			0x4ull
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index c483f99..f6638ed 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -4300,14 +4300,12 @@ static void vmx_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0)
>  
>  static u64 construct_eptp(struct kvm_vcpu *vcpu, unsigned long root_hpa)
>  {
> -	u64 eptp;
> +	u64 eptp = VMX_EPTP_MT_WB | VMX_EPTP_PWL_4;
>  
>  	/* TODO write the value reading from MSR */
> -	eptp = VMX_EPT_DEFAULT_MT |
> -		VMX_EPT_DEFAULT_GAW << VMX_EPT_GAW_EPTP_SHIFT;
>  	if (enable_ept_ad_bits &&
>  	    (!is_guest_mode(vcpu) || nested_ept_ad_enabled(vcpu)))
> -		eptp |= VMX_EPT_AD_ENABLE_BIT;
> +		eptp |= VMX_EPTP_AD_ENABLE_BIT;
>  	eptp |= (root_hpa & PAGE_MASK);
>  
>  	return eptp;
> @@ -7879,16 +7877,15 @@ static int handle_preemption_timer(struct kvm_vcpu *vcpu)
>  static bool valid_ept_address(struct kvm_vcpu *vcpu, u64 address)
>  {
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
> -	u64 mask = address & 0x7;
>  	int maxphyaddr = cpuid_maxphyaddr(vcpu);
>  
>  	/* Check for memory type validity */
> -	switch (mask) {
> -	case 0:
> +	switch (address & VMX_EPTP_MT_MASK) {
> +	case VMX_EPTP_MT_UC:
>  		if (!(vmx->nested.nested_vmx_ept_caps & VMX_EPTP_UC_BIT))
>  			return false;
>  		break;
> -	case 6:
> +	case VMX_EPTP_MT_WB:
>  		if (!(vmx->nested.nested_vmx_ept_caps & VMX_EPTP_WB_BIT))
>  			return false;
>  		break;
> @@ -7896,8 +7893,8 @@ static bool valid_ept_address(struct kvm_vcpu *vcpu, u64 address)
>  		return false;
>  	}
>  
> -	/* Bits 5:3 must be 3 */
> -	if (((address >> VMX_EPT_GAW_EPTP_SHIFT) & 0x7) != VMX_EPT_DEFAULT_GAW)
> +	/* only 4 levels page-walk length are valid */
> +	if ((address & VMX_EPTP_PWL_MASK) != VMX_EPTP_PWL_4)
>  		return false;
>  
>  	/* Reserved bits should not be set */
> @@ -7905,7 +7902,7 @@ static bool valid_ept_address(struct kvm_vcpu *vcpu, u64 address)
>  		return false;
>  
>  	/* AD, if set, should be supported */
> -	if ((address & VMX_EPT_AD_ENABLE_BIT)) {
> +	if (address & VMX_EPTP_AD_ENABLE_BIT) {
>  		if (!(vmx->nested.nested_vmx_ept_caps & VMX_EPT_AD_BIT))
>  			return false;
>  	}
> @@ -7933,7 +7930,7 @@ static int nested_vmx_eptp_switching(struct kvm_vcpu *vcpu,
>  				     &address, index * 8, 8))
>  		return 1;
>  
> -	accessed_dirty = !!(address & VMX_EPT_AD_ENABLE_BIT);
> +	accessed_dirty = !!(address & VMX_EPTP_AD_ENABLE_BIT);
>  
>  	/*
>  	 * If the (L2) guest does a vmfunc to the currently
> @@ -9499,7 +9496,7 @@ static void __init vmx_check_processor_compat(void *rtn)
>  
>  static int get_ept_level(void)
>  {
> -	return VMX_EPT_DEFAULT_GAW + 1;
> +	return 4;
>  }
>  
>  static u64 vmx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
> @@ -9702,7 +9699,7 @@ static void nested_ept_inject_page_fault(struct kvm_vcpu *vcpu,
>  
>  static bool nested_ept_ad_enabled(struct kvm_vcpu *vcpu)
>  {
> -	return nested_ept_get_cr3(vcpu) & VMX_EPT_AD_ENABLE_BIT;
> +	return nested_ept_get_cr3(vcpu) & VMX_EPTP_AD_ENABLE_BIT;
>  }
>  
>  /* Callbacks for nested_ept_init_mmu_context: */

Re: [PATCH v1 1/2] KVM: VMX: cleanup EPTP definitions

[David Hildenbrand]

On 10.08.2017 21:38, Bandan Das wrote:
> David Hildenbrand <david@redhat.com> writes:
> 
>> Don't use shifts, tag them correctly as EPTP and use better matching
>> names (PWL vs. GAW).
> 
> Fair enough. This reminds me, I also don't particularly like the vmx->nested.nested...
> accesses that are spread all throughout :)

Me too, I got a bunch of patches lying around that abstract VMX features
into something like X86 features.

#define VMX_FEATURE_BASIC_INOUT                (0*32 + 18)

#define VMX_FEATURE_PREEMPTION_TIMER           (1*32 + 6)
#define VMX_FEATURE_POSTED_INTR                (1*32 + 7)

#define VMX_FEATURE_TPR_SHADOW                 (2*32 + 21)
#define VMX_FEATURE_MSR_BITMAP                 (2*32 + 28)
#define VMX_FEATURE_SECONDARY_EXEC_CTRLS       (2*32 + 31)

#define VMX_FEATURE_VIRTUALIZE_APIC_ACCESSES   (3*32 + 0)
...
#define VMX_FEATURE_EXIT_LOAD_IA32_PAT         (4*32 + 19)
...
#define VMX_FEATURE_ENTRY_LOAD_IA32_PAT        (5*32 + 14)
...
#define VMX_FEATURE_EPT_EXECUTE_ONLY           (6*32 + 0)
...
#define VMX_FEATURE_INVVPID                    (7*32 + 0)
...

/* whether our CPU supports a certain VMX feature */
bool vmx_has(unsigned int feature);
  -> replaces most cpu_has_vmx_*

/* whether our nested CPU is allowed to use a certain VMX feature */
bool nested_vmx_has(struct kvm_vcpu *vcpu, unsigned int feature);
  -> replaces most checks against
     vmx->nested.nested_vmx_secondary_ctls_high and friends

/* whether L1 enabled a certain VMX feature for L2 */
bool nested_cpu_has(struct vmcs12 *vmcs12, unsigned int feature);
  -> merges/replaces most checks like nested_cpu_has/nested_cpu_has2 ...


I am not yet happy with the end result. Anybody interested or has any
suggestions?

-- 

Thanks,

David

Re: [PATCH v1 1/2] KVM: VMX: cleanup EPTP definitions

[David Hildenbrand]

On 10.08.2017 15:35, David Hildenbrand wrote:
> Don't use shifts, tag them correctly as EPTP and use better matching
> names (PWL vs. GAW).
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  arch/x86/include/asm/vmx.h | 11 ++++++-----
>  arch/x86/kvm/vmx.c         | 25 +++++++++++--------------
>  2 files changed, 17 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
> index 5f63a2e..929b3fc 100644
> --- a/arch/x86/include/asm/vmx.h
> +++ b/arch/x86/include/asm/vmx.h
> @@ -468,12 +468,13 @@ enum vmcs_field {
>  #define VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT      (1ull << 10) /* (42 - 32) */
>  #define VMX_VPID_EXTENT_SINGLE_NON_GLOBAL_BIT   (1ull << 11) /* (43 - 32) */
>  
> -#define VMX_EPT_DEFAULT_GAW			3
> -#define VMX_EPT_MAX_GAW				0x4
>  #define VMX_EPT_MT_EPTE_SHIFT			3
> -#define VMX_EPT_GAW_EPTP_SHIFT			3
> -#define VMX_EPT_AD_ENABLE_BIT			(1ull << 6)
> -#define VMX_EPT_DEFAULT_MT			0x6ull
> +#define VMX_EPTP_PWL_MASK			0x38ull
> +#define VMX_EPTP_PWL_4				0x38ull

stupid typo, this should be 0x18ull (otherwise you'll get a kernel panic
...)


-- 

Thanks,

David

[PATCH v1 2/2] KVM: VMX: always require WB memory type for EPT

[David Hildenbrand]

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(-)

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;
-- 
2.9.4

Re: [PATCH v1 2/2] KVM: VMX: always require WB memory type for EPT

[Paolo Bonzini]

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;
> 

Re: [PATCH v1 2/2] KVM: VMX: always require WB memory type for EPT

[Konrad Rzeszutek Wilk]

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;
> > 
> 

Re: [PATCH v1 2/2] KVM: VMX: always require WB memory type for EPT

[David Hildenbrand]

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
>>


-- 

Thanks,

David

Re: [PATCH v1 2/2] KVM: VMX: always require WB memory type for EPT

[Bandan Das]

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;

Re: [PATCH v1 2/2] KVM: VMX: always require WB memory type for EPT

[David Hildenbrand]

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.

-- 

Thanks,

David

Re: [PATCH v1 0/2] KVM: VMX: require EPTP WB (write-back) support

[Radim Krčmář]

2017-08-10 15:35+0200, David Hildenbrand:
> I also have a patch that allows VMX and nVMX to use the UC (unchached)
> memory type.
> .
> However I am not sure if we have to take care of anything special in that
> case? Any insights?

We're modifying the EPT structure through cache in root mode and I think
the cache would have to be manually flushed before entering non-root
mode.

And UC accesses are going to be slower, so I don't see the benefits for
VMX.  We can allow UC in nVMX as it will make no difference.

Re: [PATCH v1 0/2] KVM: VMX: require EPTP WB (write-back) support

[David Hildenbrand]

On 10.08.2017 16:23, Radim Krčmář wrote:
> 2017-08-10 15:35+0200, David Hildenbrand:
>> I also have a patch that allows VMX and nVMX to use the UC (unchached)
>> memory type.
>> .
>> However I am not sure if we have to take care of anything special in that
>> case? Any insights?
> 
> We're modifying the EPT structure through cache in root mode and I think
> the cache would have to be manually flushed before entering non-root
> mode.
> 
> And UC accesses are going to be slower, so I don't see the benefits for
> VMX.  We can allow UC in nVMX as it will make no difference.
> 

That would mean, even fake it's existence but always create EPTP with WB
for nVMX?

-- 

Thanks,

David

Re: [PATCH v1 0/2] KVM: VMX: require EPTP WB (write-back) support

[Radim Krčmář]

2017-08-10 16:25+0200, David Hildenbrand:
> On 10.08.2017 16:23, Radim Krčmář wrote:
> > 2017-08-10 15:35+0200, David Hildenbrand:
> >> I also have a patch that allows VMX and nVMX to use the UC (unchached)
> >> memory type.
> >> .
> >> However I am not sure if we have to take care of anything special in that
> >> case? Any insights?
> > 
> > We're modifying the EPT structure through cache in root mode and I think
> > the cache would have to be manually flushed before entering non-root
> > mode.
> > 
> > And UC accesses are going to be slower, so I don't see the benefits for
> > VMX.  We can allow UC in nVMX as it will make no difference.
> > 
> 
> That would mean, even fake it's existence but always create EPTP with WB
> for nVMX?

Yes, although I just realized a case where I can't find any code nor
hardware guarantees that handle it:

KVM allows UC access to a page that the guest wanted as UC if
kvm_arch_has_noncoherent_dma().  The guest can bypass the cache when
accessing EPT pages, set UC in EPTP and expect it to work, but KVM would
access the EPT pages through cache and might therefore shadow stale
data.