[PATCH 0/5] Add support for EPT execute only for nested hypervisors

[PATCH 0/5] Add support for EPT execute only for nested hypervisors

[Bandan Das]

These patches are based on reviews to my RFC
http://www.spinics.net/lists/kvm/msg134440.html

Changes since RFC:
 - Remove shadow_xonly_valid, it's not needed
 - Remove checks from is_shadow_present_pte()
 - In reset_tdp_shadow_zero_bits_mask, pass correct execonly to __reset_rsvds_bits_mask_ept
 - Reuse shadow_user_mask in set_spte()
 - Remove is_present_gpte() and inline the operation at the two call sites

I spoke to Paolo about this a while back and thought to post this as
RFC while I am thinking of adding some unit tests.

Background: ESX refuses to run as L1 if support for EPT execute only isn't
found. I am not really sure if it uses it for anything since just advertising
the bits seems to work but adding the necessary plumbing seemed like a good idea.

Xiao, I took the liberty of adding you based on "git blame" :)

Thanks in advance.

Bandan Das (5):
  mmu: mark spte present if the x bit is set
  mmu: pass execonly value when initializing rsvd bits
  mmu: don't set the present bit unconditionally
  mmu: remove is_present_gpte()
  nvmx: advertise support for ept execute only

 arch/x86/kvm/mmu.c         | 26 ++++++++++++++++++--------
 arch/x86/kvm/mmu.h         |  5 -----
 arch/x86/kvm/paging_tmpl.h |  4 ++--
 arch/x86/kvm/vmx.c         |  5 ++++-
 arch/x86/kvm/x86.c         |  2 +-
 5 files changed, 25 insertions(+), 17 deletions(-)

-- 
2.5.5

[PATCH 1/5] mmu: mark spte present if the x bit is set

[Bandan Das]

This is safe because is_shadow_present_pte() is called
on host controlled page table and we know the spte is
valid

Signed-off-by: Bandan Das <bsd@redhat.com>
---
 arch/x86/kvm/mmu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index def97b3..a50af79 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -304,7 +304,8 @@ static int is_nx(struct kvm_vcpu *vcpu)
 
 static int is_shadow_present_pte(u64 pte)
 {
-	return pte & PT_PRESENT_MASK && !is_mmio_spte(pte);
+	return pte & (PT_PRESENT_MASK | shadow_x_mask) &&
+		!is_mmio_spte(pte);
 }
 
 static int is_large_pte(u64 pte)
-- 
2.5.5

[PATCH 2/5] mmu: pass execonly value when initializing rsvd bits

[Bandan Das]

In reset_tdp_shadow_zero_bits_mask, we always pass false
when initializing the reserved bits. By initializing with the
correct value of ept exec only, the host can correctly
identify if the guest pte is valid. Note that
kvm_init_shadow_ept_mmu() already knows about execonly.

Signed-off-by: Bandan Das <bsd@redhat.com>
---
 arch/x86/kvm/mmu.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index a50af79..875d4f7 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -3831,23 +3831,27 @@ static inline bool boot_cpu_is_amd(void)
 
 /*
  * the direct page table on host, use as much mmu features as
- * possible, however, kvm currently does not do execution-protection.
+ * possible
  */
 static void
 reset_tdp_shadow_zero_bits_mask(struct kvm_vcpu *vcpu,
 				struct kvm_mmu *context)
 {
+	bool execonly;
+
 	if (boot_cpu_is_amd())
 		__reset_rsvds_bits_mask(vcpu, &context->shadow_zero_check,
 					boot_cpu_data.x86_phys_bits,
 					context->shadow_root_level, false,
 					boot_cpu_has(X86_FEATURE_GBPAGES),
 					true, true);
-	else
+	else {
+		execonly = !(context->guest_rsvd_check.bad_mt_xwr &
+			     (1ull << VMX_EPT_EXECUTABLE_MASK));
 		__reset_rsvds_bits_mask_ept(&context->shadow_zero_check,
 					    boot_cpu_data.x86_phys_bits,
-					    false);
-
+					    execonly);
+	}
 }
 
 /*
-- 
2.5.5

[PATCH 3/5] mmu: don't set the present bit unconditionally

[Bandan Das]

To support execute only mappings on behalf of L1
hypervisors, we teach set_spte() to honor L1's valid XWR
bits. This is only if host supports EPT execute only. Reuse
ACC_USER_MASK to signify if the L1 hypervisor has the R bit
set

Signed-off-by: Bandan Das <bsd@redhat.com>
---
 arch/x86/kvm/mmu.c         | 9 +++++++--
 arch/x86/kvm/paging_tmpl.h | 2 +-
 arch/x86/kvm/vmx.c         | 2 +-
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 875d4f7..ee2fb16 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -2516,13 +2516,17 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 		    gfn_t gfn, kvm_pfn_t pfn, bool speculative,
 		    bool can_unsync, bool host_writable)
 {
-	u64 spte;
+	u64 spte = 0;
 	int ret = 0;
+	struct kvm_mmu *context = &vcpu->arch.mmu;
+	bool execonly = !(context->guest_rsvd_check.bad_mt_xwr &
+			  (1ull << VMX_EPT_EXECUTABLE_MASK));
 
 	if (set_mmio_spte(vcpu, sptep, gfn, pfn, pte_access))
 		return 0;
 
-	spte = PT_PRESENT_MASK;
+	if (!execonly)
+		spte |= PT_PRESENT_MASK;
 	if (!speculative)
 		spte |= shadow_accessed_mask;
 
@@ -2531,6 +2535,7 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 	else
 		spte |= shadow_nx_mask;
 
+	/* In the EPT case, shadow_user_mask is PT_PRESENT_MASK */
 	if (pte_access & ACC_USER_MASK)
 		spte |= shadow_user_mask;
 
diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index bc019f7..896118e 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -187,7 +187,7 @@ static inline unsigned FNAME(gpte_access)(struct kvm_vcpu *vcpu, u64 gpte)
 #if PTTYPE == PTTYPE_EPT
 	access = ((gpte & VMX_EPT_WRITABLE_MASK) ? ACC_WRITE_MASK : 0) |
 		((gpte & VMX_EPT_EXECUTABLE_MASK) ? ACC_EXEC_MASK : 0) |
-		ACC_USER_MASK;
+		((gpte & VMX_EPT_READABLE_MASK) ? ACC_USER_MASK : 0);
 #else
 	BUILD_BUG_ON(ACC_EXEC_MASK != PT_PRESENT_MASK);
 	BUILD_BUG_ON(ACC_EXEC_MASK != 1);
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 003618e..417debc 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -6366,7 +6366,7 @@ static __init int hardware_setup(void)
 	vmx_disable_intercept_msr_write_x2apic(0x83f);
 
 	if (enable_ept) {
-		kvm_mmu_set_mask_ptes(0ull,
+		kvm_mmu_set_mask_ptes(PT_PRESENT_MASK,
 			(enable_ept_ad_bits) ? VMX_EPT_ACCESS_BIT : 0ull,
 			(enable_ept_ad_bits) ? VMX_EPT_DIRTY_BIT : 0ull,
 			0ull, VMX_EPT_EXECUTABLE_MASK);
-- 
2.5.5

[PATCH 4/5] mmu: remove is_present_gpte()

[Bandan Das]

We have two versions of the above function.
To prevent confusion and bugs in the future, remove
the non-FNAME version entirely and replace all calls
with the actual check.

Signed-off-by: Bandan Das <bsd@redhat.com>
---
 arch/x86/kvm/mmu.c         | 2 +-
 arch/x86/kvm/mmu.h         | 5 -----
 arch/x86/kvm/paging_tmpl.h | 2 +-
 arch/x86/kvm/x86.c         | 2 +-
 4 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index ee2fb16..7197e36 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -3195,7 +3195,7 @@ static int mmu_alloc_shadow_roots(struct kvm_vcpu *vcpu)
 		MMU_WARN_ON(VALID_PAGE(root));
 		if (vcpu->arch.mmu.root_level == PT32E_ROOT_LEVEL) {
 			pdptr = vcpu->arch.mmu.get_pdptr(vcpu, i);
-			if (!is_present_gpte(pdptr)) {
+			if (!(pdptr & PT_PRESENT_MASK)) {
 				vcpu->arch.mmu.pae_root[i] = 0;
 				continue;
 			}
diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
index 66b33b9..ddc56e9 100644
--- a/arch/x86/kvm/mmu.h
+++ b/arch/x86/kvm/mmu.h
@@ -93,11 +93,6 @@ static inline int kvm_mmu_reload(struct kvm_vcpu *vcpu)
 	return kvm_mmu_load(vcpu);
 }
 
-static inline int is_present_gpte(unsigned long pte)
-{
-	return pte & PT_PRESENT_MASK;
-}
-
 /*
  * Currently, we have two sorts of write-protection, a) the first one
  * write-protects guest page to sync the guest modification, b) another one is
diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 896118e..1384cb7 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -131,7 +131,7 @@ static inline void FNAME(protect_clean_gpte)(unsigned *access, unsigned gpte)
 static inline int FNAME(is_present_gpte)(unsigned long pte)
 {
 #if PTTYPE != PTTYPE_EPT
-	return is_present_gpte(pte);
+	return pte & PT_PRESENT_MASK;
 #else
 	return pte & 7;
 #endif
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 902d9da..dc4fed7 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -537,7 +537,7 @@ int load_pdptrs(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, unsigned long cr3)
 		goto out;
 	}
 	for (i = 0; i < ARRAY_SIZE(pdpte); ++i) {
-		if (is_present_gpte(pdpte[i]) &&
+		if ((pdpte[i] & PT_PRESENT_MASK) &&
 		    (pdpte[i] &
 		     vcpu->arch.mmu.guest_rsvd_check.rsvd_bits_mask[0][2])) {
 			ret = 0;
-- 
2.5.5

[PATCH 5/5] nvmx: advertise support for ept execute only

[Bandan Das]

KVM MMU now knows about execute only mappings, so
advertise the feature to L1 hypervisors

Signed-off-by: Bandan Das <bsd@redhat.com>
---
 arch/x86/kvm/vmx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 417debc..0fe61a3 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2717,6 +2717,9 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx)
 		vmx->nested.nested_vmx_ept_caps = VMX_EPT_PAGE_WALK_4_BIT |
 			 VMX_EPTP_WB_BIT | VMX_EPT_2MB_PAGE_BIT |
 			 VMX_EPT_INVEPT_BIT;
+		if (cpu_has_vmx_ept_execute_only())
+			vmx->nested.nested_vmx_ept_caps |=
+				VMX_EPT_EXECUTE_ONLY_BIT;
 		vmx->nested.nested_vmx_ept_caps &= vmx_capability.ept;
 		/*
 		 * For nested guests, we don't do anything specific
-- 
2.5.5

Re: [PATCH 1/5] mmu: mark spte present if the x bit is set

[Paolo Bonzini]

On 28/06/2016 06:32, Bandan Das wrote:
> This is safe because is_shadow_present_pte() is called
> on host controlled page table and we know the spte is
> valid
> 
> Signed-off-by: Bandan Das <bsd@redhat.com>
> ---
>  arch/x86/kvm/mmu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index def97b3..a50af79 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -304,7 +304,8 @@ static int is_nx(struct kvm_vcpu *vcpu)
>  
>  static int is_shadow_present_pte(u64 pte)
>  {
> -	return pte & PT_PRESENT_MASK && !is_mmio_spte(pte);
> +	return pte & (PT_PRESENT_MASK | shadow_x_mask) &&
> +		!is_mmio_spte(pte);

This should really be pte & 7 when using EPT.  But this is okay as an
alternative to a new shadow_present_mask.

Paolo

>  }
>  
>  static int is_large_pte(u64 pte)
> 

Re: [PATCH 3/5] mmu: don't set the present bit unconditionally

[Paolo Bonzini]

On 28/06/2016 06:32, Bandan Das wrote:
> +	bool execonly = !(context->guest_rsvd_check.bad_mt_xwr &
> +			  (1ull << VMX_EPT_EXECUTABLE_MASK));
>  
>  	if (set_mmio_spte(vcpu, sptep, gfn, pfn, pte_access))
>  		return 0;
>  
> -	spte = PT_PRESENT_MASK;
> +	if (!execonly)
> +		spte |= PT_PRESENT_MASK;

This needs a comment:

	/*
	 * There are two cases in which execonly is false: 1) for
	 * non-EPT page tables, in which case we need to set the
	 * P bit; 2) for EPT page tables where an X-- page table
	 * entry is invalid, in which case we need to force the R
	 * bit of the page table entry to 1.
	 */
	BUILD_BUG_ON(PT_PRESENT_MASK != VMX_EPT_READABLE_MASK);
	if (!execonly)
		spte |= PT_PRESENT_MASK;
	

>  	if (!speculative)
>  		spte |= shadow_accessed_mask;
> 
>  	if (enable_ept) {
> -		kvm_mmu_set_mask_ptes(0ull,
> +		kvm_mmu_set_mask_ptes(PT_PRESENT_MASK,

This should be VMX_EPT_READABLE_MASK.

> @@ -2531,6 +2535,7 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
>  	else
>  		spte |= shadow_nx_mask;
>  
> +	/* In the EPT case, shadow_user_mask is PT_PRESENT_MASK */

I don't think this comment is necessary, but it's better to add one in
FNAME(gpte_access).

	/*
	 * In the EPT case, a page table can be executable but not
	 * readable (on some processors).  Therefore, set_spte does not
	 * automatically set bit 0 if execute-only is supported.
	 * Instead, since EPT page tables do not have a U bit, we
	 * repurpose ACC_USER_MASK to signify readability.  Likewise,
	 * when EPT is in use shadow_user_mask is set to
	 * VMX_EPT_READABLE_MASK.
	 */
	

Thanks,

Paolo

>  	if (pte_access & ACC_USER_MASK)
>  		spte |= shadow_user_mask;


Paolo

>  			(enable_ept_ad_bits) ? VMX_EPT_ACCESS_BIT : 0ull,
>  			(enable_ept_ad_bits) ? VMX_EPT_DIRTY_BIT : 0ull,
>  			0ull, VMX_EPT_EXECUTABLE_MASK);

Re: [PATCH 3/5] mmu: don't set the present bit unconditionally

[Bandan Das]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 28/06/2016 06:32, Bandan Das wrote:
>> +	bool execonly = !(context->guest_rsvd_check.bad_mt_xwr &
>> +			  (1ull << VMX_EPT_EXECUTABLE_MASK));
>>  
>>  	if (set_mmio_spte(vcpu, sptep, gfn, pfn, pte_access))
>>  		return 0;
>>  
>> -	spte = PT_PRESENT_MASK;
>> +	if (!execonly)
>> +		spte |= PT_PRESENT_MASK;
>
> This needs a comment:
>
> 	/*
> 	 * There are two cases in which execonly is false: 1) for
> 	 * non-EPT page tables, in which case we need to set the
> 	 * P bit; 2) for EPT page tables where an X-- page table
> 	 * entry is invalid, in which case we need to force the R
> 	 * bit of the page table entry to 1.
> 	 */

I think this should be: 2) for EPT page tables where an X-- page
table entry is invalid and a EPT misconfig is injected to the guest
before we reach here.

> 	BUILD_BUG_ON(PT_PRESENT_MASK != VMX_EPT_READABLE_MASK);
> 	if (!execonly)
> 		spte |= PT_PRESENT_MASK;
> 	
>
>>  	if (!speculative)
>>  		spte |= shadow_accessed_mask;
>> 
>>  	if (enable_ept) {
>> -		kvm_mmu_set_mask_ptes(0ull,
>> +		kvm_mmu_set_mask_ptes(PT_PRESENT_MASK,
>
> This should be VMX_EPT_READABLE_MASK.
>
>> @@ -2531,6 +2535,7 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
>>  	else
>>  		spte |= shadow_nx_mask;
>>  
>> +	/* In the EPT case, shadow_user_mask is PT_PRESENT_MASK */
>
> I don't think this comment is necessary, but it's better to add one in
> FNAME(gpte_access).
>
> 	/*
> 	 * In the EPT case, a page table can be executable but not
> 	 * readable (on some processors).  Therefore, set_spte does not
> 	 * automatically set bit 0 if execute-only is supported.
> 	 * Instead, since EPT page tables do not have a U bit, we
> 	 * repurpose ACC_USER_MASK to signify readability.  Likewise,
> 	 * when EPT is in use shadow_user_mask is set to
> 	 * VMX_EPT_READABLE_MASK.
> 	 */
> 	
>
> Thanks,
>
> Paolo
>
>>  	if (pte_access & ACC_USER_MASK)
>>  		spte |= shadow_user_mask;
>
>
> Paolo
>
>>  			(enable_ept_ad_bits) ? VMX_EPT_ACCESS_BIT : 0ull,
>>  			(enable_ept_ad_bits) ? VMX_EPT_DIRTY_BIT : 0ull,
>>  			0ull, VMX_EPT_EXECUTABLE_MASK);
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] mmu: mark spte present if the x bit is set

[Bandan Das]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 28/06/2016 06:32, Bandan Das wrote:
>> This is safe because is_shadow_present_pte() is called
>> on host controlled page table and we know the spte is
>> valid
>> 
>> Signed-off-by: Bandan Das <bsd@redhat.com>
>> ---
>>  arch/x86/kvm/mmu.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>> index def97b3..a50af79 100644
>> --- a/arch/x86/kvm/mmu.c
>> +++ b/arch/x86/kvm/mmu.c
>> @@ -304,7 +304,8 @@ static int is_nx(struct kvm_vcpu *vcpu)
>>  
>>  static int is_shadow_present_pte(u64 pte)
>>  {
>> -	return pte & PT_PRESENT_MASK && !is_mmio_spte(pte);
>> +	return pte & (PT_PRESENT_MASK | shadow_x_mask) &&
>> +		!is_mmio_spte(pte);
>
> This should really be pte & 7 when using EPT.  But this is okay as an
> alternative to a new shadow_present_mask.

I could revive shadow_xonly_valid probably... Anyway, for now I will
add a TODO comment here.

> Paolo
>
>>  }
>>  
>>  static int is_large_pte(u64 pte)
>> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] mmu: mark spte present if the x bit is set

[Paolo Bonzini]

On 28/06/2016 19:33, Bandan Das wrote:
>>> >>  static int is_shadow_present_pte(u64 pte)
>>> >>  {
>>> >> -	return pte & PT_PRESENT_MASK && !is_mmio_spte(pte);
>>> >> +	return pte & (PT_PRESENT_MASK | shadow_x_mask) &&
>>> >> +		!is_mmio_spte(pte);
>> >
>> > This should really be pte & 7 when using EPT.  But this is okay as an
>> > alternative to a new shadow_present_mask.
> I could revive shadow_xonly_valid probably... Anyway, for now I will
> add a TODO comment here.

It's okay to it like this, because the only invalid PTEs reaching this
point are those that is_mmio_spte filters away.  Hence you'll never get
-W- PTEs here, and pte & 7 is really the same as how you wrote it.  It's
pretty clever, and doesn't need a TODO at all. :)

Paolo

Re: [PATCH 3/5] mmu: don't set the present bit unconditionally

[Paolo Bonzini]

On 28/06/2016 19:30, Bandan Das wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>> On 28/06/2016 06:32, Bandan Das wrote:
>>> +	bool execonly = !(context->guest_rsvd_check.bad_mt_xwr &
>>> +			  (1ull << VMX_EPT_EXECUTABLE_MASK));
>>>  
>>>  	if (set_mmio_spte(vcpu, sptep, gfn, pfn, pte_access))
>>>  		return 0;
>>>  
>>> -	spte = PT_PRESENT_MASK;
>>> +	if (!execonly)
>>> +		spte |= PT_PRESENT_MASK;
>>
>> This needs a comment:
>>
>> 	/*
>> 	 * There are two cases in which execonly is false: 1) for
>> 	 * non-EPT page tables, in which case we need to set the
>> 	 * P bit; 2) for EPT page tables where an X-- page table
>> 	 * entry is invalid, in which case we need to force the R
>> 	 * bit of the page table entry to 1.
>> 	 */
> 
> I think this should be: 2) for EPT page tables where an X-- page
> table entry is invalid and a EPT misconfig is injected to the guest
> before we reach here.

Right, I messed this one up.  shadow_user_mask and ACC_USER_MASK work
the same way on processors that do not support execonly.

Paolo

>> 	BUILD_BUG_ON(PT_PRESENT_MASK != VMX_EPT_READABLE_MASK);
>> 	if (!execonly)
>> 		spte |= PT_PRESENT_MASK;
>> 	
>>
>>>  	if (!speculative)
>>>  		spte |= shadow_accessed_mask;
>>>
>>>  	if (enable_ept) {
>>> -		kvm_mmu_set_mask_ptes(0ull,
>>> +		kvm_mmu_set_mask_ptes(PT_PRESENT_MASK,
>>
>> This should be VMX_EPT_READABLE_MASK.
>>
>>> @@ -2531,6 +2535,7 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
>>>  	else
>>>  		spte |= shadow_nx_mask;
>>>  
>>> +	/* In the EPT case, shadow_user_mask is PT_PRESENT_MASK */
>>
>> I don't think this comment is necessary, but it's better to add one in
>> FNAME(gpte_access).
>>
>> 	/*
>> 	 * In the EPT case, a page table can be executable but not
>> 	 * readable (on some processors).  Therefore, set_spte does not
>> 	 * automatically set bit 0 if execute-only is supported.
>> 	 * Instead, since EPT page tables do not have a U bit, we
>> 	 * repurpose ACC_USER_MASK to signify readability.  Likewise,
>> 	 * when EPT is in use shadow_user_mask is set to
>> 	 * VMX_EPT_READABLE_MASK.
>> 	 */
>> 	
>>
>> Thanks,
>>
>> Paolo
>>
>>>  	if (pte_access & ACC_USER_MASK)
>>>  		spte |= shadow_user_mask;
>>
>>
>> Paolo
>>
>>>  			(enable_ept_ad_bits) ? VMX_EPT_ACCESS_BIT : 0ull,
>>>  			(enable_ept_ad_bits) ? VMX_EPT_DIRTY_BIT : 0ull,
>>>  			0ull, VMX_EPT_EXECUTABLE_MASK);
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/5] mmu: mark spte present if the x bit is set

[Bandan Das]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 28/06/2016 19:33, Bandan Das wrote:
>>>> >>  static int is_shadow_present_pte(u64 pte)
>>>> >>  {
>>>> >> -	return pte & PT_PRESENT_MASK && !is_mmio_spte(pte);
>>>> >> +	return pte & (PT_PRESENT_MASK | shadow_x_mask) &&
>>>> >> +		!is_mmio_spte(pte);
>>> >
>>> > This should really be pte & 7 when using EPT.  But this is okay as an
>>> > alternative to a new shadow_present_mask.
>> I could revive shadow_xonly_valid probably... Anyway, for now I will
>> add a TODO comment here.
>
> It's okay to it like this, because the only invalid PTEs reaching this
> point are those that is_mmio_spte filters away.  Hence you'll never get
> -W- PTEs here, and pte & 7 is really the same as how you wrote it.  It's
> pretty clever, and doesn't need a TODO at all. :)

Thanks, understood. So, the way it is written now covers all cases for
pte & 7. Let's still add a comment - clever things are usually
confusing to many!

> Paolo

Re: [PATCH 1/5] mmu: mark spte present if the x bit is set

[Paolo Bonzini]

On 28/06/2016 22:37, Bandan Das wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> On 28/06/2016 19:33, Bandan Das wrote:
>>>>>>>  static int is_shadow_present_pte(u64 pte)
>>>>>>>  {
>>>>>>> -	return pte & PT_PRESENT_MASK && !is_mmio_spte(pte);
>>>>>>> +	return pte & (PT_PRESENT_MASK | shadow_x_mask) &&
>>>>>>> +		!is_mmio_spte(pte);
>>>>>
>>>>> This should really be pte & 7 when using EPT.  But this is okay as an
>>>>> alternative to a new shadow_present_mask.
>>> I could revive shadow_xonly_valid probably... Anyway, for now I will
>>> add a TODO comment here.
>>
>> It's okay to it like this, because the only invalid PTEs reaching this
>> point are those that is_mmio_spte filters away.  Hence you'll never get
>> -W- PTEs here, and pte & 7 is really the same as how you wrote it.  It's
>> pretty clever, and doesn't need a TODO at all. :)
> 
> Thanks, understood. So, the way it is written now covers all cases for
> pte & 7. Let's still add a comment - clever things are usually
> confusing to many!

I think another way to write it is "(pte & 0xFFFFFFFFull) &&
!is_mmio_spte(pte)", since non-present/non-MMIO SPTEs never use bits
1..31 (they can have non-zero bits 32..63 on 32-bit CPUs where we don't
update the PTEs atomically).  Guangrong, what do you prefer?

Paolo

Re: [PATCH 1/5] mmu: mark spte present if the x bit is set

[Bandan Das]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 28/06/2016 22:37, Bandan Das wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> On 28/06/2016 19:33, Bandan Das wrote:
>>>>>>>>  static int is_shadow_present_pte(u64 pte)
>>>>>>>>  {
>>>>>>>> -	return pte & PT_PRESENT_MASK && !is_mmio_spte(pte);
>>>>>>>> +	return pte & (PT_PRESENT_MASK | shadow_x_mask) &&
>>>>>>>> +		!is_mmio_spte(pte);
>>>>>>
>>>>>> This should really be pte & 7 when using EPT.  But this is okay as an
>>>>>> alternative to a new shadow_present_mask.
>>>> I could revive shadow_xonly_valid probably... Anyway, for now I will
>>>> add a TODO comment here.
>>>
>>> It's okay to it like this, because the only invalid PTEs reaching this
>>> point are those that is_mmio_spte filters away.  Hence you'll never get
>>> -W- PTEs here, and pte & 7 is really the same as how you wrote it.  It's
>>> pretty clever, and doesn't need a TODO at all. :)
>> 
>> Thanks, understood. So, the way it is written now covers all cases for
>> pte & 7. Let's still add a comment - clever things are usually
>> confusing to many!
>
> I think another way to write it is "(pte & 0xFFFFFFFFull) &&
> !is_mmio_spte(pte)", since non-present/non-MMIO SPTEs never use bits
> 1..31 (they can have non-zero bits 32..63 on 32-bit CPUs where we don't
> update the PTEs atomically).  Guangrong, what do you prefer?

Actually, I like this one better although until now, I was not sure if it's
a safe assumption for non-ept cases. From your description, it looks like
it is.

> Paolo

Re: [PATCH 1/5] mmu: mark spte present if the x bit is set

[Xiao Guangrong]

On 06/29/2016 04:49 AM, Paolo Bonzini wrote:
>
>
> On 28/06/2016 22:37, Bandan Das wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> On 28/06/2016 19:33, Bandan Das wrote:
>>>>>>>>   static int is_shadow_present_pte(u64 pte)
>>>>>>>>   {
>>>>>>>> -	return pte & PT_PRESENT_MASK && !is_mmio_spte(pte);
>>>>>>>> +	return pte & (PT_PRESENT_MASK | shadow_x_mask) &&
>>>>>>>> +		!is_mmio_spte(pte);
>>>>>>
>>>>>> This should really be pte & 7 when using EPT.  But this is okay as an
>>>>>> alternative to a new shadow_present_mask.
>>>> I could revive shadow_xonly_valid probably... Anyway, for now I will
>>>> add a TODO comment here.
>>>
>>> It's okay to it like this, because the only invalid PTEs reaching this
>>> point are those that is_mmio_spte filters away.  Hence you'll never get
>>> -W- PTEs here, and pte & 7 is really the same as how you wrote it.  It's
>>> pretty clever, and doesn't need a TODO at all. :)
>>
>> Thanks, understood. So, the way it is written now covers all cases for
>> pte & 7. Let's still add a comment - clever things are usually
>> confusing to many!
>
> I think another way to write it is "(pte & 0xFFFFFFFFull) &&
> !is_mmio_spte(pte)", since non-present/non-MMIO SPTEs never use bits
> 1..31 (they can have non-zero bits 32..63 on 32-bit CPUs where we don't
> update the PTEs atomically).  Guangrong, what do you prefer?

I think the way you innovated is better. :)

Re: [PATCH 2/5] mmu: pass execonly value when initializing rsvd bits

[Xiao Guangrong]

On 06/28/2016 12:32 PM, Bandan Das wrote:
> In reset_tdp_shadow_zero_bits_mask, we always pass false
> when initializing the reserved bits. By initializing with the
> correct value of ept exec only, the host can correctly
> identify if the guest pte is valid. Note that
> kvm_init_shadow_ept_mmu() already knows about execonly.
>
> Signed-off-by: Bandan Das <bsd@redhat.com>
> ---
>   arch/x86/kvm/mmu.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index a50af79..875d4f7 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -3831,23 +3831,27 @@ static inline bool boot_cpu_is_amd(void)
>
>   /*
>    * the direct page table on host, use as much mmu features as
> - * possible, however, kvm currently does not do execution-protection.
> + * possible
>    */
>   static void
>   reset_tdp_shadow_zero_bits_mask(struct kvm_vcpu *vcpu,
>   				struct kvm_mmu *context)
>   {

It is not necessary. reset_tdp_shadow_zero_bits_mask() is used for
the guest without nested-ept, host never sets shadow execute-only
actively.

For the nested ept guest, kvm_init_shadow_ept_mmu() has already
handled xonly case perfectly.

Re: [PATCH 3/5] mmu: don't set the present bit unconditionally

[Xiao Guangrong]

On 06/28/2016 12:32 PM, Bandan Das wrote:
> To support execute only mappings on behalf of L1
> hypervisors, we teach set_spte() to honor L1's valid XWR
> bits. This is only if host supports EPT execute only. Reuse
> ACC_USER_MASK to signify if the L1 hypervisor has the R bit
> set
>
> Signed-off-by: Bandan Das <bsd@redhat.com>
> ---
>   arch/x86/kvm/mmu.c         | 9 +++++++--
>   arch/x86/kvm/paging_tmpl.h | 2 +-
>   arch/x86/kvm/vmx.c         | 2 +-
>   3 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 875d4f7..ee2fb16 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -2516,13 +2516,17 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
>   		    gfn_t gfn, kvm_pfn_t pfn, bool speculative,
>   		    bool can_unsync, bool host_writable)
>   {
> -	u64 spte;
> +	u64 spte = 0;
>   	int ret = 0;
> +	struct kvm_mmu *context = &vcpu->arch.mmu;
> +	bool execonly = !(context->guest_rsvd_check.bad_mt_xwr &
> +			  (1ull << VMX_EPT_EXECUTABLE_MASK));

Could we introduce a new field, say execonly, to "struct kvm_mmu"?
That would make the code be more clearer.

Re: [PATCH 3/5] mmu: don't set the present bit unconditionally

[Paolo Bonzini]

On 29/06/2016 05:17, Xiao Guangrong wrote:
>>
>> +++ b/arch/x86/kvm/mmu.c
>> @@ -2516,13 +2516,17 @@ static int set_spte(struct kvm_vcpu *vcpu, u64
>> *sptep,
>>               gfn_t gfn, kvm_pfn_t pfn, bool speculative,
>>               bool can_unsync, bool host_writable)
>>   {
>> -    u64 spte;
>> +    u64 spte = 0;
>>       int ret = 0;
>> +    struct kvm_mmu *context = &vcpu->arch.mmu;
>> +    bool execonly = !(context->guest_rsvd_check.bad_mt_xwr &
>> +              (1ull << VMX_EPT_EXECUTABLE_MASK));
> 
> Could we introduce a new field, say execonly, to "struct kvm_mmu"?
> That would make the code be more clearer.

Given how execonly is used, let's add shadow_present_mask instead.

Paolo

Re: [PATCH 3/5] mmu: don't set the present bit unconditionally

[Xiao Guangrong]

On 06/29/2016 04:18 PM, Paolo Bonzini wrote:
>
>
> On 29/06/2016 05:17, Xiao Guangrong wrote:
>>>
>>> +++ b/arch/x86/kvm/mmu.c
>>> @@ -2516,13 +2516,17 @@ static int set_spte(struct kvm_vcpu *vcpu, u64
>>> *sptep,
>>>                gfn_t gfn, kvm_pfn_t pfn, bool speculative,
>>>                bool can_unsync, bool host_writable)
>>>    {
>>> -    u64 spte;
>>> +    u64 spte = 0;
>>>        int ret = 0;
>>> +    struct kvm_mmu *context = &vcpu->arch.mmu;
>>> +    bool execonly = !(context->guest_rsvd_check.bad_mt_xwr &
>>> +              (1ull << VMX_EPT_EXECUTABLE_MASK));
>>
>> Could we introduce a new field, say execonly, to "struct kvm_mmu"?
>> That would make the code be more clearer.
>
> Given how execonly is used, let's add shadow_present_mask instead.

Yup, it is better.

Re: [PATCH 1/5] mmu: mark spte present if the x bit is set

[Wanpeng Li]

2016-06-29 4:49 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
[...]
>
> I think another way to write it is "(pte & 0xFFFFFFFFull) &&
> !is_mmio_spte(pte)", since non-present/non-MMIO SPTEs never use bits

I misunderstand it here, this will also treat -W- EPT SPTEs as present, right?

Regards,
Wanpeng Li

Re: [PATCH 3/5] mmu: don't set the present bit unconditionally

[Wanpeng Li]

2016-06-28 16:57 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>
>
> On 28/06/2016 06:32, Bandan Das wrote:
>> +     bool execonly = !(context->guest_rsvd_check.bad_mt_xwr &
>> +                       (1ull << VMX_EPT_EXECUTABLE_MASK));
>>
>>       if (set_mmio_spte(vcpu, sptep, gfn, pfn, pte_access))
>>               return 0;
>>
>> -     spte = PT_PRESENT_MASK;
>> +     if (!execonly)
>> +             spte |= PT_PRESENT_MASK;
>
> This needs a comment:
>
>         /*
>          * There are two cases in which execonly is false: 1) for
>          * non-EPT page tables, in which case we need to set the
>          * P bit; 2) for EPT page tables where an X-- page table

In the scenario of non-EPT shadow page table and non-nested, the
present bit can't be set any more since
context->guest_rsvd_check.bad_mt_xwr is always 0.

Regards,
Wanpeng Li

Re: [PATCH 1/5] mmu: mark spte present if the x bit is set

[Paolo Bonzini]

On 05/07/2016 05:06, Wanpeng Li wrote:
> 2016-06-29 4:49 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
> [...]
>>
>> I think another way to write it is "(pte & 0xFFFFFFFFull) &&
>> !is_mmio_spte(pte)", since non-present/non-MMIO SPTEs never use bits
> 
> I misunderstand it here, this will also treat -W- EPT SPTEs as present, right?

-W- EPT SPTEs are present but invalid.  They should never happen unless
they are MMIO SPTEs (in which case !is_mmio_spte(pte) will return true
and the function will return false).

Paolo

> Regards,
> Wanpeng Li
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 3/5] mmu: don't set the present bit unconditionally

[Paolo Bonzini]

On 05/07/2016 07:50, Wanpeng Li wrote:
>> > This needs a comment:
>> >
>> >         /*
>> >          * There are two cases in which execonly is false: 1) for
>> >          * non-EPT page tables, in which case we need to set the
>> >          * P bit; 2) for EPT page tables where an X-- page table
> In the scenario of non-EPT shadow page table and non-nested, the
> present bit can't be set any more since
> context->guest_rsvd_check.bad_mt_xwr is always 0.

This will be fixed with a new shadow_present_mask variable.

Paolo

Re: [PATCH 1/5] mmu: mark spte present if the x bit is set

[Wanpeng Li]

2016-07-05 18:50 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>
>
> On 05/07/2016 05:06, Wanpeng Li wrote:
>> 2016-06-29 4:49 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>> [...]
>>>
>>> I think another way to write it is "(pte & 0xFFFFFFFFull) &&
>>> !is_mmio_spte(pte)", since non-present/non-MMIO SPTEs never use bits
>>
>> I misunderstand it here, this will also treat -W- EPT SPTEs as present, right?
>
> -W- EPT SPTEs are present but invalid.  They should never happen unless
> they are MMIO SPTEs (in which case !is_mmio_spte(pte) will return true
> and the function will return false).

Thanks for the explanation. :)

Regards,
Wanpeng Li