Re: [RFC PATCH 0/4] Add support for EPT execute only for nested hypervisors

[Bandan Das <bsd@redhat.com>]

Xiao Guangrong <guangrong.xiao@linux.intel.com> writes:

> On 06/23/2016 12:34 AM, Bandan Das wrote:
>> Xiao Guangrong <guangrong.xiao@linux.intel.com> writes:
>>
>>> On 06/21/2016 11:12 AM, Bandan Das wrote:
>>>> 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" :)
>>>
>>> Thank you, Bandan! I really hope all mmu related patches can be CCed to me
>>> in case if i am too busy to watch patches in kvm mail list. :)
>>
>> We all try but it's easy to miss interested people in the cc list :)
>>
>>> Some points are missed in this patchset:
>>> 1) You also need to tech is_present_gpte() about this fact.
>>
>> If I understood right, this isn't needed anymore since a check
>> for rsvd bits follows and is_present_gpte already does pte & 7.
>
> is_present_gpte() and FNAME(is_present_gpte) are different paths.
>
> Currently, it is safe as is_present_gpte() is only used for 32 bit
> guests, however, making it consistent can avoid potential bugs
> in the further.

I am tempted to remove the FNAME version altogether and change is_present_gpte()
to return (pte & PT_PRESENT_MASK) || (shadow_xonly_valid && (pte & 4)). This
will take care of all cases. Hope I am not missing something :)

>>
>>> 2) update_permission_bitmask() need to be fixed as it always expects
>>>     that read-access is available (actually, read-access is equal with
>>>     user-access if ept is shadowed).
>> Do you mean this part of update_permission_bitmask() ?
>> ...
>> } else
>>    	/* Not really needed: no U/S accesses on ept  */
>> 	u = 1;
>
> Yes.