diff mbox

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

Message ID 20160311221651.GE4347@pd.tnic (mailing list archive)
State New, archived
Headers show

Commit Message

Borislav Petkov March 11, 2016, 10:16 p.m. UTC 
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(-)

Comments

Bruce Ashfield March 11, 2016, 10:28 p.m. UTC | #1 
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 | \
>

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Richard Purdie March 11, 2016, 11:29 p.m. UTC | #2 
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

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Borislav Petkov March 12, 2016, 12:03 p.m. UTC | #3 
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.
diff mbox

Patch

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 | \