Re: [Xen-devel] [PATCH for-4.9 v2] x86: avoid #GP for PV guest MSR accesses

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/10/17 11:07, Jan Beulich wrote:
>>>> On 09.10.17 at 11:49, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 09/10/17 08:48, Jan Beulich wrote:
>>> Halfway recent Linux kernels probe MISC_FEATURES_ENABLES on all CPUs,
>>> leading to ugly recovered #GP fault messages with debug builds on older
>>> systems. We can do better, so introduce synthetic feature flags for
>>> both this and PLATFORM_INFO to avoid the rdmsr_safe() altogether.
>>>
>>> Note that the r/o nature of PLATFORM_INFO is now also being enforced.
>>>
>>> The rdmsr_safe() uses for MISC_ENABLE are left in place as benign - it
>>> exists for all 64-bit capable Intel CPUs (see e.g. early_init_intel()).
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>> ---
>>> v2: Force PLATFORM_INFO writes to fail.
>>>
>>> --- a/xen/arch/x86/cpu/intel.c
>>> +++ b/xen/arch/x86/cpu/intel.c
>>> @@ -21,10 +21,19 @@ static bool __init probe_intel_cpuid_fau
>>>  {
>>>     uint64_t x;
>>>  
>>> -   if (rdmsr_safe(MSR_INTEL_PLATFORM_INFO, x) ||
>>> -       !(x & MSR_PLATFORM_INFO_CPUID_FAULTING))
>>> +   if (rdmsr_safe(MSR_INTEL_PLATFORM_INFO, x))
>>>             return 0;
>>>  
>>> +   setup_force_cpu_cap(X86_FEATURE_MSR_PLATFORM_INFO);
>>> +
>>> +   if (!(x & MSR_PLATFORM_INFO_CPUID_FAULTING)) {
>>> +           if (!rdmsr_safe(MSR_INTEL_MISC_FEATURES_ENABLES, x))
>>> +                   setup_force_cpu_cap(X86_FEATURE_MSR_MISC_FEATURES);
>>> +           return 0;
>>> +   }
>>> +
>>> +   setup_force_cpu_cap(X86_FEATURE_MSR_MISC_FEATURES);
>> Why this odd arrangement with the double setup_force_cpu_cap()?  As
>> neither of these MSRs are architectural, would it not be better to probe
>> both of them, rather than assuming MISC_FEATURES_ENABLES is available if
>> faulting is reported?
> Since faulting being available means it can be enabled, and since
> enabling works by writing MISC_FEATURES_ENABLES, I don't see
> why we need to probe that second register in that case.
> set_cpuid_faulting() makes this very assumption already.

Ok.  Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

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