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

[Bandan Das <bsd@redhat.com>]

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