Re: runtime regression with "x86/mm/pat: Emulate PAT when it is disabled"

Re: runtime regression with "x86/mm/pat: Emulate PAT when it is disabled"

[Borislav Petkov]

On Fri, Mar 11, 2016 at 01:23:56PM +0000, One Thousand Gnomes wrote:
> Pentium Pro has MTRR, PAT came later.

Yep, this page says so too:

http://www.cpu-world.com/CPUs/Pentium-II/Intel-Pentium%20Pro%20200%201%20MB%20-%20GJ80521EX200%201M%20%28BP80521200%201M%29.html

> I believe the qemu32 CPU isn't a "real" CPU type therefore.

And why is that so? I suspect it has again something to do with
migration and fun. Let me add the kvm ML.

Guys, does anyone have an idea why

#define PPRO_FEATURES (CPUID_FP87 | CPUID_DE | CPUID_PSE | CPUID_TSC | \
          CPUID_MSR | CPUID_MCE | CPUID_CX8 | CPUID_PGE | CPUID_CMOV | \
          CPUID_PAT | CPUID_FXSR | CPUID_MMX | CPUID_SSE | CPUID_SSE2 | \
          CPUID_PAE | CPUID_SEP | CPUID_APIC)

has CPUID_PAT *instead* of CPUID_MTRR?

If that define is really supposed to be a bitfield of Pentium Pro
features, then it should be:

#define PPRO_FEATURES (CPUID_FP87 | CPUID_DE | CPUID_PSE | CPUID_TSC | \
          CPUID_MSR | CPUID_MCE | CPUID_CX8 | CPUID_PGE | CPUID_CMOV | \
          CPUID_MTRR | CPUID_FXSR | CPUID_MMX | CPUID_SSE | CPUID_SSE2 | \
          CPUID_PAE | CPUID_SEP | CPUID_APIC)

Hmmm?

Btw,

#define PENTIUM2_FEATURES (PENTIUM_FEATURES | CPUID_PAE | CPUID_SEP | \
          CPUID_MTRR | CPUID_PGE | CPUID_MCA | CPUID_CMOV | CPUID_PAT | \
          CPUID_PSE36 | CPUID_FXSR)

looks correct in that it has MTRR but it has PAT too.

And that looks wrong to me too since, according to Wikipedia, Pentium
III was the first to sport PAT:

https://en.wikipedia.org/wiki/Page_attribute_table

More hmmmm...

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: runtime regression with "x86/mm/pat: Emulate PAT when it is disabled"

[Paolo Bonzini]

On 11/03/2016 14:40, Borislav Petkov wrote:
> On Fri, Mar 11, 2016 at 01:23:56PM +0000, One Thousand Gnomes wrote:
>> > Pentium Pro has MTRR, PAT came later.
> Yep, this page says so too:
> 
> http://www.cpu-world.com/CPUs/Pentium-II/Intel-Pentium%20Pro%20200%201%20MB%20-%20GJ80521EX200%201M%20%28BP80521200%201M%29.html
> 
>> > I believe the qemu32 CPU isn't a "real" CPU type therefore.
> And why is that so? I suspect it has again something to do with
> migration and fun. Let me add the kvm ML.
> 
> Guys, does anyone have an idea why
> 
> #define PPRO_FEATURES (CPUID_FP87 | CPUID_DE | CPUID_PSE | CPUID_TSC | \
>           CPUID_MSR | CPUID_MCE | CPUID_CX8 | CPUID_PGE | CPUID_CMOV | \
>           CPUID_PAT | CPUID_FXSR | CPUID_MMX | CPUID_SSE | CPUID_SSE2 | \
>           CPUID_PAE | CPUID_SEP | CPUID_APIC)
> 
> has CPUID_PAT *instead* of CPUID_MTRR?

Somebody got it wrong 10-ish years ago, and nobody has ever checked since.

But, don't use qemu32 or qemu64.  Use kvm32 and kvm64, or better
something like the host you run on ("-cpu Nehalem", "-cpu SandyBridge",
"-cpu Haswell-noTSX" etc.).

I really, really should fix those defaults...

Paolo

Re: runtime regression with "x86/mm/pat: Emulate PAT when it is disabled"

[Borislav Petkov]

On Fri, Mar 11, 2016 at 08:18:23PM +0100, Paolo Bonzini wrote:
> Somebody got it wrong 10-ish years ago, and nobody has ever checked since.
> 
> But, don't use qemu32 or qemu64.  Use kvm32 and kvm64, or better
> something like the host you run on ("-cpu Nehalem", "-cpu SandyBridge",
> "-cpu Haswell-noTSX" etc.).

Paul, Richard, how about it?

> I really, really should fix those defaults...

Here's a start, while I have everything fresh in my head.

---
From: Borislav Petkov <bp@suse.de>
Date: Fri, 11 Mar 2016 23:11:05 +0100
Subject: [PATCH] target-i386/cpu: Correct MTRR and PAT feature bits

Pentium Pro had MTRRs but not PAT, PAT support appeared in Pentium III.
Fix all defines.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 target-i386/cpu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 0f38d1eae317..fa7ea4a8c229 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -308,12 +308,12 @@ static const char *cpuid_6_feature_name[] = {
 #define PENTIUM_FEATURES (I486_FEATURES | CPUID_DE | CPUID_TSC | \
           CPUID_MSR | CPUID_MCE | CPUID_CX8 | CPUID_MMX | CPUID_APIC)
 #define PENTIUM2_FEATURES (PENTIUM_FEATURES | CPUID_PAE | CPUID_SEP | \
-          CPUID_MTRR | CPUID_PGE | CPUID_MCA | CPUID_CMOV | CPUID_PAT | \
+          CPUID_MTRR | CPUID_PGE | CPUID_MCA | CPUID_CMOV | \
           CPUID_PSE36 | CPUID_FXSR)
-#define PENTIUM3_FEATURES (PENTIUM2_FEATURES | CPUID_SSE)
+#define PENTIUM3_FEATURES (PENTIUM2_FEATURES | CPUID_SSE | CPUID_PAT)
 #define PPRO_FEATURES (CPUID_FP87 | CPUID_DE | CPUID_PSE | CPUID_TSC | \
           CPUID_MSR | CPUID_MCE | CPUID_CX8 | CPUID_PGE | CPUID_CMOV | \
-          CPUID_PAT | CPUID_FXSR | CPUID_MMX | CPUID_SSE | CPUID_SSE2 | \
+          CPUID_MTRR | CPUID_FXSR | CPUID_MMX | CPUID_SSE | CPUID_SSE2 | \
           CPUID_PAE | CPUID_SEP | CPUID_APIC)
 
 #define TCG_FEATURES (CPUID_FP87 | CPUID_PSE | CPUID_TSC | CPUID_MSR | \
-- 
2.3.5

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: runtime regression with "x86/mm/pat: Emulate PAT when it is disabled"

[Bruce Ashfield]

On 2016-03-11 5:16 PM, Borislav Petkov wrote:
> On Fri, Mar 11, 2016 at 08:18:23PM +0100, Paolo Bonzini wrote:
>> Somebody got it wrong 10-ish years ago, and nobody has ever checked since.
>>
>> But, don't use qemu32 or qemu64.  Use kvm32 and kvm64, or better
>> something like the host you run on ("-cpu Nehalem", "-cpu SandyBridge",
>> "-cpu Haswell-noTSX" etc.).
>
> Paul, Richard, how about it?

I'm not Paul/Richard, but I can answer :)

We want a more generic cpu for these qemu references. Something that
doesn't vary, since it runs on any number of hosts. There are some
horrible issues we've had to solve with -cpu host in the past.

Switching to the kvm cpu type should work, plus we can add cpu
extensions on the fly as necessary.

That's definitely a valid outcome from this discussion .. a cpu
type that doesn't shoot through the middle of the expected
capabilities .. and a bonus if the kernel or qemu can be tweaked
to survive a similar mix up in the flags in the future.

Cheers,

Bruce

>
>> I really, really should fix those defaults...
>
> Here's a start, while I have everything fresh in my head.
>
> ---
> From: Borislav Petkov <bp@suse.de>
> Date: Fri, 11 Mar 2016 23:11:05 +0100
> Subject: [PATCH] target-i386/cpu: Correct MTRR and PAT feature bits
>
> Pentium Pro had MTRRs but not PAT, PAT support appeared in Pentium III.
> Fix all defines.
>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>   target-i386/cpu.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 0f38d1eae317..fa7ea4a8c229 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -308,12 +308,12 @@ static const char *cpuid_6_feature_name[] = {
>   #define PENTIUM_FEATURES (I486_FEATURES | CPUID_DE | CPUID_TSC | \
>             CPUID_MSR | CPUID_MCE | CPUID_CX8 | CPUID_MMX | CPUID_APIC)
>   #define PENTIUM2_FEATURES (PENTIUM_FEATURES | CPUID_PAE | CPUID_SEP | \
> -          CPUID_MTRR | CPUID_PGE | CPUID_MCA | CPUID_CMOV | CPUID_PAT | \
> +          CPUID_MTRR | CPUID_PGE | CPUID_MCA | CPUID_CMOV | \
>             CPUID_PSE36 | CPUID_FXSR)
> -#define PENTIUM3_FEATURES (PENTIUM2_FEATURES | CPUID_SSE)
> +#define PENTIUM3_FEATURES (PENTIUM2_FEATURES | CPUID_SSE | CPUID_PAT)
>   #define PPRO_FEATURES (CPUID_FP87 | CPUID_DE | CPUID_PSE | CPUID_TSC | \
>             CPUID_MSR | CPUID_MCE | CPUID_CX8 | CPUID_PGE | CPUID_CMOV | \
> -          CPUID_PAT | CPUID_FXSR | CPUID_MMX | CPUID_SSE | CPUID_SSE2 | \
> +          CPUID_MTRR | CPUID_FXSR | CPUID_MMX | CPUID_SSE | CPUID_SSE2 | \
>             CPUID_PAE | CPUID_SEP | CPUID_APIC)
>
>   #define TCG_FEATURES (CPUID_FP87 | CPUID_PSE | CPUID_TSC | CPUID_MSR | \
>

Re: runtime regression with "x86/mm/pat: Emulate PAT when it is disabled"

[Richard Purdie]

On Fri, 2016-03-11 at 17:28 -0500, Bruce Ashfield wrote:
> On 2016-03-11 5:16 PM, Borislav Petkov wrote:
> > On Fri, Mar 11, 2016 at 08:18:23PM +0100, Paolo Bonzini wrote:
> > > Somebody got it wrong 10-ish years ago, and nobody has ever
> > > checked since.
> > > 
> > > But, don't use qemu32 or qemu64.  Use kvm32 and kvm64, or better
> > > something like the host you run on ("-cpu Nehalem", "-cpu
> > > SandyBridge",
> > > "-cpu Haswell-noTSX" etc.).
> > 
> > Paul, Richard, how about it?
> 
> I'm not Paul/Richard, but I can answer :)
> 
> We want a more generic cpu for these qemu references. Something that
> doesn't vary, since it runs on any number of hosts. There are some
> horrible issues we've had to solve with -cpu host in the past.
> 
> Switching to the kvm cpu type should work, plus we can add cpu
> extensions on the fly as necessary.
> 
> That's definitely a valid outcome from this discussion .. a cpu
> type that doesn't shoot through the middle of the expected
> capabilities .. and a bonus if the kernel or qemu can be tweaked
> to survive a similar mix up in the flags in the future.

We'll have to go and trawl our commit logs to figure out how we ended
up with choosing qemuXX, I do remember everything else we tried
previously broke in some way. We do need something which works on quite
a wide variety of systems and if I remember rightly, the CPU features
the kvm* options provide vary quite widely and wasn't consistent. The
qemu* ones at least consistently broke everywhere! :)

Tweaking the kernel to only enable PAT with MTRR sounds like a good
fix, as does fixing the qemu cpu feature bits and I'd like to get those
patches into our builds once they're heading into the upstreams.
Meanwhile we'll see if we can find a better cpu option as well.

Cheers,

Richard

Re: runtime regression with "x86/mm/pat: Emulate PAT when it is disabled"

[Borislav Petkov]

On Fri, Mar 11, 2016 at 11:29:54PM +0000, Richard Purdie wrote:
> ... and if I remember rightly, the CPU features the kvm* options
> provide vary quite widely and wasn't consistent.

How so? Please elaborate so that we can fix those.

Btw, your reproducer works fine with -cpu kvm32 - only vncviewer's
window gets killed after a "Rect too large: 640x480 at (0, 0)" but
when I reconnect again right after it I see an X window asking me to
calibrate my touch screen and I'm at a loss as to where my touch screen
is... :-P

> Tweaking the kernel to only enable PAT with MTRR sounds like a good
> fix,

Yeah, the idea is to basically switch to uncacheable memtype as when
MTRRs are disabled.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--