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

[Kai Huang <kai.huang@xxxxxxxxxxxxxxx>]

On 10/16/2015 04:17 PM, Jan Beulich wrote:
> > > > On 16.10.15 at 04:21, <kai.huang@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 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@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel