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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

Jan


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