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

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

On 10/16/2015 04:17 PM, Jan Beulich wrote:
>>>> On 16.10.15 at 04:21, <kai.huang@linux.intel.com> wrote:
>> Existing PML implementation turns on EPT A/D bit unconditionally if PML is
>> supported by hardware. This works but enabling of EPT A/D bit can be
>> deferred
>> until PML get enabled. There's no point in enabling the extra feature for
>> every
>> domain when we're not meaning to use it (yet).
>>
>> Sanity live migration and GUI display were tested on Broadwell Machine.
>>
>> Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> There's so little in this patch that came from me that I don't think this is
> warranted; but if you want to keep it, the order needs to be switched.
> Instead I'd suggest Suggested-by:.
I'll change it to Suggested-by.

>
>> +void vmx_domain_update_eptp(struct domain *d)
>> +{
>> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>> +    struct vcpu *v;
>> +
>> +    ASSERT(atomic_read(&d->pause_count));
> This should imo check controller_pause_count.
This function is called between domain_pause and domain_unpause, and 
domain_pause increases d->pause_count, not d->controller_pause_count, so 
we should check d->pause_count, right?

>
>> +    for_each_vcpu( d, v )
> Coding style: You need to settle on whether you want to treat
> for_each_vcpu like a keyword (then there's a blank missing before
> the opening paren) or like a normal identifier (then the blanks
> immediately inside the parens need to go away).
Oh. I will add a blank before the opening paren.

>
>>   static void ept_flush_pml_buffers(struct p2m_domain *p2m)
>>   {
>> +    /* Domain must have been paused */
>> +    ASSERT(atomic_read(&p2m->domain->pause_count));
> This seems unrelated - did you really mean it to go into this patch?
This function is also supposed to be called when domain is paused, so 
making it consistent with ept_enable{disable}_pml, I also added the 
ASSERT here. Is this reasonable?

Thanks,
-Kai
>
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>