Re: [PATCH] x86/EPT: defer enabling of A/D maintenance until PML get enabled

[Kai Huang <kai.huang@linux.intel.com>]

On 10/15/2015 03:11 PM, Jan Beulich wrote:
>>>> On 15.10.15 at 08:42, <kai.huang@linux.intel.com> wrote:
>> Thanks for your comments Jan. Actually I am not happy with combining
>> with EPT A/D bit update with PML enabling to single function. After
>> thinking again, how about adding a separate vmx function (ex,
>> vmx_domain_update_eptp) to update EPTP of VMCS of all vcpus of domain
>> after p2m->ept.ept_ad is updated. Another good is this function can also
>> be used in the future for other runtime updates to p2m->ept.
>>
>> What's your idea?
> I don't mind, but that's really more of a question to the VMX maintainers.
Then I would prefer this way.

Kevin,

Do you have any comments on this thread?
>
>> --- a/xen/arch/x86/mm/p2m-ept.c
>> +++ b/xen/arch/x86/mm/p2m-ept.c
>> @@ -1129,17 +1129,26 @@ void ept_sync_domain(struct p2m_domain *p2m)
>>
>>    static void ept_enable_pml(struct p2m_domain *p2m)
>>    {
>>        /*
>> -     * No need to check if vmx_domain_enable_pml has succeeded or not, as
>> +     * No need to return if vmx_domain_enable_pml has succeeded or not, as
> It seems to me that you'd better use "whether" instead of "if" now
> (and then perhaps also drop the "or not").
OK. Thanks.
>
>>         * ept_p2m_type_to_flags will do the check, and write protection will be
>>         * used if PML is not enabled.
>>         */
>> -    vmx_domain_enable_pml(p2m->domain);
>> +    if ( vmx_domain_enable_pml(p2m->domain) )
>> +        return;
>> +
>> +    p2m->ept.ept_ad = 1;
>> +    vmx_domain_update_eptp(p2m->domain);
> Shouldn't you enable A/D _before_ enabling PML, at least without
> having a domain-is-paused check here?
Looks we don't have such function. How about just add 
ASSERT(atomic_read(&d->pause_count)), just the same as in 
vmx_domain_enable_pml ?

Thanks,
-Kai
>
> Jan
>
>