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

[Kai Huang <kai.huang@linux.intel.com>]

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@xen.org> 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
>