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

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

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

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@citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---

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) */
 #define X86_FEATURE_CONSTANT_TSC (3*32+ 8) /* TSC ticks at a constant rate */
 #define X86_FEATURE_NONSTOP_TSC	(3*32+ 9) /* TSC does not stop in C states */
 #define X86_FEATURE_ARAT	(3*32+ 10) /* Always running APIC timer */
-- 
2.3.2.209.gd67f9d5.dirty


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

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

[Roger Pau Monné]

El 27/03/15 a les 0.33, Luis R. Rodriguez ha escrit:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
> 
> 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.

AFAICT we will never support PVH on those CPU types, so I'm not sure if
it's worth disabling it. AMD K6, Centaur and VIA chips don't have the
necessary hardware extensions to allow running PVH guests.

Roger.

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

[Andrew Cooper]

On 26/03/15 23:33, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
>
> 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@citrix.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> ---
>
> 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?

~Andrew

>   #define X86_FEATURE_CONSTANT_TSC (3*32+ 8) /* TSC ticks at a constant rate */
>   #define X86_FEATURE_NONSTOP_TSC	(3*32+ 9) /* TSC does not stop in C states */
>   #define X86_FEATURE_ARAT	(3*32+ 10) /* Always running APIC timer */


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

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

[Jan Beulich]

>>> On 27.03.15 at 10:37, <andrew.cooper3@citrix.com> wrote:
> On 26/03/15 23:33, Luis R. Rodriguez wrote:
>> From: "Luis R. Rodriguez" <mcgrof@suse.com>
>>
>> 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@citrix.com>
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
>> ---
>>
>> 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@lists.xen.org
http://lists.xen.org/xen-devel

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

[Luis R. Rodriguez]

On Fri, Mar 27, 2015 at 09:32:10AM +0100, Roger Pau Monné wrote:
> El 27/03/15 a les 0.33, Luis R. Rodriguez ha escrit:
> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> > 
> > 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.
> 
> AFAICT we will never support PVH on those CPU types, so I'm not sure if
> it's worth disabling it. AMD K6, Centaur and VIA chips don't have the
> necessary hardware extensions to allow running PVH guests.

Great, should this be annotated?

  Luis