From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kai Huang Subject: Re: [PATCH for-4.6] p2m/ept: Set the A bit only if PML is enabled Date: Mon, 28 Sep 2015 16:42:05 +0800 Message-ID: <5608FD5D.6040102@linux.intel.com> References: <1442393271-12388-1-git-send-email-ross.lagerwall@citrix.com> <20150923151846.GA9208@zion.uk.xensource.com> <20150923154629.GA61801@deinos.phlegethon.org> <5603BC3602000078000A5173@prv-mh.provo.novell.com> <20150924091003.GA63393@deinos.phlegethon.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150924091003.GA63393@deinos.phlegethon.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tim Deegan , Jan Beulich Cc: Kevin Tian , Wei Liu , George Dunlap , Andrew Cooper , xen-devel@lists.xen.org, Ross Lagerwall , Jun Nakajima , Keir Fraser List-Id: xen-devel@lists.xenproject.org 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, 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@lists.xen.org > http://lists.xen.org/xen-devel >