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

[Bandan Das <bsd@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 21/06/2016 05:12, 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. Use ACC_USER_MASK to signify
>> if the L1 hypervisor has the present bit set.
>
> has the "R" bit set.

Yep, noted!

>> Signed-off-by: Bandan Das <bsd@redhat.com>
>> ---
>>  arch/x86/kvm/mmu.c         | 11 ++++++++---
>>  arch/x86/kvm/paging_tmpl.h |  2 +-
>>  2 files changed, 9 insertions(+), 4 deletions(-)
...
>> --- a/arch/x86/kvm/mmu.c
>> +++ b/arch/x86/kvm/mmu.c
>> @@ -2528,7 +2528,8 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
>>  	if (set_mmio_spte(vcpu, sptep, gfn, pfn, pte_access))
>>  		return 0;
>>  
>> -	spte = PT_PRESENT_MASK;
>> +	if (!shadow_xonly_valid)
>> +		spte = PT_PRESENT_MASK;
>>  	if (!speculative)
>>  		spte |= shadow_accessed_mask;
>>  
>> @@ -2537,8 +2538,12 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
>>  	else
>>  		spte |= shadow_nx_mask;
>>  
>> -	if (pte_access & ACC_USER_MASK)
>> -		spte |= shadow_user_mask;
>> +	if (pte_access & ACC_USER_MASK) {
>> +		if (shadow_xonly_valid)
>> +			spte |= PT_PRESENT_MASK;
>> +		else
>> +			spte |= shadow_user_mask;
>> +	}
>
> Can you instead pass VMX_READABLE_MASK to kvm_mmu_set_mask_ptes in vmx.c?

So, leave spte = PT_PRESENT_MASK as is and make VMX_READABLE_MASK 1ULL if
execute only is supported ?
And then :
if !(pte_access & ACC_USER_MASK) {
       spte &= ~VMX_READABLE_MASK 

       
>>  	if (level > PT_PAGE_TABLE_LEVEL)
>>  		spte |= PT_PAGE_SIZE_MASK;
>> diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
>> index 9f5bd06..5366a55 100644
>> --- a/arch/x86/kvm/paging_tmpl.h
>> +++ b/arch/x86/kvm/paging_tmpl.h
>> @@ -192,7 +192,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);
>
> This part is fine.
>
> Paolo
>
>>  #else
>>  	BUILD_BUG_ON(ACC_EXEC_MASK != PT_PRESENT_MASK);
>>  	BUILD_BUG_ON(ACC_EXEC_MASK != 1);
>>