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

[Tim Deegan <tim@xxxxxxx>]

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. :)

> 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.

> >> 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