Re: [Xen-devel] [PATCH for-4.6] p2m/ept: Set the A bit only if PML is enabled

[Kai Huang <kai.huang@xxxxxxxxxxxxxxx>]

On 09/24/2015 05:10 PM, Tim Deegan wrote:
> At 01:02 -0600 on 24 Sep (1443056566), Jan Beulich wrote:
> > > > > On 23.09.15 at 17:46, <tim@xxxxxxx> wrote:
> > > At 16:18 +0100 on 23 Sep (1443025126), Wei Liu wrote:
> > > > With the discussion still not finalised I'm a bit worried that this
> > > > issue will block the release.
> > > >
> > > > I think we have a few options here. I will list them in order of my
> > > > preference. Please correct me if I'm talking non-sense, and feel free to
> > > > add more options if I miss anything.
> > > >
> > > > 1. Disable PML on broken chips, gate access to A bit (or AD) with PML.
> > > I don't much like tying this to PML: this is not a PML-related bug and
> > > there may be CPUs that have A/D but not PML.
> > >
> > > Better to have a global read-mostly bool cpu_has_vmx_ept_broken_access_bit,
> > > or whatever name the maintainers prefer. :)
> > Actually I'd suggest a positive identification (e.g. cpu_has_ept_ad),
> > which would get forced off on known broken chips. And then, in a
> > slight variation of the previously proposed patch, at least for the
> > immediate 4.6 needs do
> >
> > --- a/xen/arch/x86/mm/p2m-ept.c
> > +++ b/xen/arch/x86/mm/p2m-ept.c
> > @@ -130,14 +130,18 @@ static void ept_p2m_type_to_flags(struct p2m_domain *p2m, 
> > ept_entry_t *entry,
> >               break;
> >           case p2m_ram_rw:
> >               entry->r = entry->w = entry->x = 1;
> > -            entry->a = entry->d = 1;
> > +            entry->a = entry->d = cpu_has_ept_ad;
> >               break;
> >           case p2m_mmio_direct:
> >               entry->r = entry->x = 1;
> >               entry->w = !rangeset_contains_singleton(mmio_ro_ranges,
> >                                                       entry->mfn);
> > -            entry->a = 1;
> > -            entry->d = entry->w;
> > +            entry->a = cpu_has_ept_ad;
> > +            entry->d = entry->w && cpu_has_ept_ad;
> >               break;
> >           case p2m_ram_logdirty:
> >               entry->r = entry->x = 1;
> Sure, that works.  I still prefer putting the workaround on the CR3
> operation, so all the cost happens on the broken chip, but I'll shut
> up about that now. :)
Sorry for late response on this issue. This is good to me too as it 
avoids the "if" gate.
> > etc along with adjusting the existing gating of PML on AD being
> > available (perhaps by simply stripping the respective bit from what
> > we read from MSR_IA32_VMX_EPT_VPID_CAP). Of course this
> > then ignores the fact that the erratum only affects the A bit, but
> > I think we can live with that.
> >
> > I also think the currently slightly strange setting of the ept_ad bit
> > should be changed: There's no point setting the bit for domains
> > not getting PML enabled (and incurring the overhead of the
> > hardware updating the bits); imo this should instead be done in
> > ept_enable_pml() / vmx_domain_enable_pml() (and undone in
> > the respective disable function).
> Yep.
Yes this is slight better. But I don't think keeping current code of 
setting ept_ad in ept_p2m_init would cause any performance regression, 
as we'll unconditionally set A/D bits to 1 in ept_p2m_type_to_flags to 
avoid having CPU to set them later. Right?
For the erratum we are talking about here, the ept_ad bit simply won't 
be set as PML is simply not supported.
Thanks,
-Kai
> > > > 2. Implement general framework to detect broken chips and apply quirks.
> > > >
> > > > I take that there is no general framework at the moment, otherwise the
> > > > patch would have used that.
> > > We already have code that detects specific chips and adjusts things,
> > > e.g. init_intel() in arch/x86/cpu/intel.c.  That seems like a good
> > > place to set the global flag above, or...
> > Together with the above I'm not sure that would be the best place
> > to deal with this (EPT specific) erratum; I think this would better be
> > contained to VMX/EPT code.
> Agreed.
>
> Tim.
>
> _______________________________________________
> 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