Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1432450 > unrolled thread

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

Started byBandan Das <bsd@redhat.com>
First post2016-06-28 06:40 +0200
Last post2016-06-30 09:30 +0200
Articles 9 — 4 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/5] mmu: don't set the present bit unconditionally Bandan Das <bsd@redhat.com> - 2016-06-28 06:40 +0200
    Re: [PATCH 3/5] mmu: don't set the present bit unconditionally Paolo Bonzini <pbonzini@redhat.com> - 2016-06-28 11:00 +0200
      Re: [PATCH 3/5] mmu: don't set the present bit unconditionally Bandan Das <bsd@redhat.com> - 2016-06-28 19:50 +0200
        Re: [PATCH 3/5] mmu: don't set the present bit unconditionally Paolo Bonzini <pbonzini@redhat.com> - 2016-06-28 22:30 +0200
      Re: [PATCH 3/5] mmu: don't set the present bit unconditionally Wanpeng Li <kernellwp@gmail.com> - 2016-07-05 08:00 +0200
        Re: [PATCH 3/5] mmu: don't set the present bit unconditionally Paolo Bonzini <pbonzini@redhat.com> - 2016-07-05 13:00 +0200
    Re: [PATCH 3/5] mmu: don't set the present bit unconditionally Xiao Guangrong <guangrong.xiao@linux.intel.com> - 2016-06-29 05:30 +0200
      Re: [PATCH 3/5] mmu: don't set the present bit unconditionally Paolo Bonzini <pbonzini@redhat.com> - 2016-06-29 10:20 +0200
        Re: [PATCH 3/5] mmu: don't set the present bit unconditionally Xiao Guangrong <guangrong.xiao@linux.intel.com> - 2016-06-30 09:30 +0200

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

FromBandan Das <bsd@redhat.com>
Date2016-06-28 06:40 +0200
Subject[PATCH 3/5] mmu: don't set the present bit unconditionally
Message-ID<rOSYx-1Xl-9@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

#1432660

FromPaolo Bonzini <pbonzini@redhat.com>
Date2016-06-28 11:00 +0200
Message-ID<rOX2a-4Ab-35@gated-at.bofh.it>
In reply to#1432450

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

[toc] | [prev] | [next] | [standalone]

#1433179

FromBandan Das <bsd@redhat.com>
Date2016-06-28 19:50 +0200
Message-ID<rP5j3-1EC-1@gated-at.bofh.it>
In reply to#1432660
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

[toc] | [prev] | [next] | [standalone]

#1433282

FromPaolo Bonzini <pbonzini@redhat.com>
Date2016-06-28 22:30 +0200
Message-ID<rP7NT-3gG-39@gated-at.bofh.it>
In reply to#1433179

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
>

[toc] | [prev] | [next] | [standalone]

#1436777

FromWanpeng Li <kernellwp@gmail.com>
Date2016-07-05 08:00 +0200
Message-ID<rRryN-79U-13@gated-at.bofh.it>
In reply to#1432660
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

[toc] | [prev] | [next] | [standalone]

#1436918

FromPaolo Bonzini <pbonzini@redhat.com>
Date2016-07-05 13:00 +0200
Message-ID<rRwf8-1IO-5@gated-at.bofh.it>
In reply to#1436777

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

[toc] | [prev] | [next] | [standalone]

#1433420

FromXiao Guangrong <guangrong.xiao@linux.intel.com>
Date2016-06-29 05:30 +0200
Message-ID<rPemm-7iT-5@gated-at.bofh.it>
In reply to#1432450

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.

[toc] | [prev] | [next] | [standalone]

#1433537

FromPaolo Bonzini <pbonzini@redhat.com>
Date2016-06-29 10:20 +0200
Message-ID<rPiSZ-1Lg-11@gated-at.bofh.it>
In reply to#1433420

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

[toc] | [prev] | [next] | [standalone]

#1434291

FromXiao Guangrong <guangrong.xiao@linux.intel.com>
Date2016-06-30 09:30 +0200
Message-ID<rPEA9-6Gs-9@gated-at.bofh.it>
In reply to#1433537

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web