Re: [Xen-devel] [RFT] x86/pvh: disable all MTRR CPU features on cpuid for dom0

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.03.15 at 10:37, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 26/03/15 23:33, Luis R. Rodriguez wrote:
>> From: "Luis R. Rodriguez" <mcgrof@xxxxxxxx>
>>
>> Commit 586ab6a by Roger disabled the main MTRR CPU feature
>> on the cpuid when on pvh in order to avoid having MTRR code
>> run on the OS on dom0. This missed the fact that not all CPUs
>> use this cpuid to identify MTRR support, this complets that.
>>
>> This could in theory fix some potential issues where MTRR
>> code was previously enabled, for instance running cat /proc/mtrr
>> on a Linux system where MTRR was actually disabled.
>>
>> Cc: Roger Pau Monnà <roger.pau@xxxxxxxxxx>
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
>> Signed-off-by: Luis R. Rodriguez <mcgrof@xxxxxxxx>
>> ---
>>
>> Konrad asked me to submit this. I don't have time to test this
>> though so it goes as RFT. If it clears through the flames of
>> xen testing thunder please consider merging.
>>
>>   xen/arch/x86/traps.c             | 6 ++++++
>>   xen/include/asm-x86/cpufeature.h | 3 +++
>>   2 files changed, 9 insertions(+)
>>
>> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
>> index ec324b0..23c3bbc 100644
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -888,7 +888,13 @@ void pv_cpuid(struct cpu_user_regs *regs)
>>           __clear_bit(X86_FEATURE_ACC, &d);
>>           __clear_bit(X86_FEATURE_PBE, &d);
>>           if ( is_pvh_vcpu(curr) )
>> +        {
>>               __clear_bit(X86_FEATURE_MTRR, &d);
>> +            __clear_bit(X86_FEATURE_K6_MTRR, &d);
>> +            __clear_bit(X86_FEATURE_CENTAUR_MCR, &d);
>> +            __clear_bit(X86_FEATURE_CYRIX_ARR, &d);
>> +        }
>> +
>>   
>>           __clear_bit(X86_FEATURE_DTES64 % 32, &c);
>>           __clear_bit(X86_FEATURE_MWAIT % 32, &c);
>> diff --git a/xen/include/asm-x86/cpufeature.h 
> b/xen/include/asm-x86/cpufeature.h
>> index 7963a3a..2ce5c7c 100644
>> --- a/xen/include/asm-x86/cpufeature.h
>> +++ b/xen/include/asm-x86/cpufeature.h
>> @@ -61,6 +61,9 @@
>>   
>>   /* Other features, Linux-defined mapping, word 3 */
>>   /* This range is used for feature bits which conflict or are synthesized */
>> +#define X86_FEATURE_K6_MTRR     (3*32+ 1) /* AMD K6 nonstandard MTRRs */
>> +#define X86_FEATURE_CYRIX_ARR   (3*32+ 2) /* Cyrix ARRs (= MTRRs) */
>> +#define X86_FEATURE_CENTAUR_MCR (3*32+ 3) /* Centaur MCRs (= MTRRs) */
> 
> You are introducing 3 new defines, so you can clear them out of a word.
> 
> Under what circumstances would these bits will ever be set in the first 
> place?

And then, if any of them can validly be set on a 64-bit capable CPU,
the mechanism to clear them is completely wrong: Did you spot the
"synthesized" in the comment above, and that this is index 3? I ask
because the change to traps.c clears these flags from word 0,
where those bits have completely different meaning.

Jan

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