[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.