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

DISCUSSION Paul Gortmaker 7 participants 35 messages Mar 3, 2016 – Mar 12, 2016 1 message not in the archive

Subsystems (3)

x86 KVM x86 MM

lore

Share & tools

lore

Paul Gortmaker

author

2016-03-03 20:59 UTC #0 lore raw

So, the yocto folks moved from 4.1 to 4.4 and one of their automated qemu x86-32 boot tests started failing. None of the yocto details seem to matter since I offered to help and I've repropduced it using 100% mainline kernels and a generic distro toolchain as well.

The test case is slightly complicated, in that it relies on uvesafb being modular, and so one has to juggle modules within an ext4 image that qemu boots from. We tried making uvesafb builtin, but that made the issue magically vanish. Given PAT, this isn't too surprising.

Richard did the preliminary investigation and analysis, and from that I did a bisect, and found the commit in $SUBJECT to be the root cause, as per the discussion here:

http://lists.openembedded.org/pipermail/openembedded-core/2016-March/118397.html

I'd mentioned the above to bpetkov on IRC and after confirming it was still an issue on 4.5-rc6, he'd asked if I had a portable reproducer.

Not sure how complicated that would be, I set out to make one from my
build. With a little LD_PRELOAD type magic and ensuring all the qemu
components are in ./ I have one that runs on an otherwise qemu-free x86-64 box.

The stand alone reproducer is here; launched in 00-runme:

http://openlinux.wrs.com/pat-splat/reproducer.tar.bz2

It is nothing fancy, just a generic yocto build of "sato" (gfx enabled rootfs). When it "works" it boots to a UI touchscreen interface. When it fails, you get a black screen with a blinking cursor (as seen in "vncviewer localhost:0").

Upon failure, you can do <Ctrl>-<Alt>-<2> to get to a passwd-less root login ; there you can run dmesg and see the splat. The image is currently using 4.5-rc6 ; but any kernel can be inserted; "make modules_install INSTALL_MOD_PATH=here" and then populating those modules from "here" into /lib/modules of the loopback mounted image. And of
course updating the bzImage on the qemu cmdline. Currently it
contains a bzImage and modules for 4.5-rc6 as I last tested that.

Also note that vncviewer will disconnect when it goes from early boot 80x25 to a higer res gfx mode; just reconnect and continue observing the target.

I've ruled out yocto kernel changes, and yocto toolchain -- but maybe it is a qemu issue this commit triggers ; who knows at this point.

Since I've NFI what component(s) cause this, I wanted to have the qemu binary, all libraries etc as part of the reproducer and nothing left to chance, and I've tested the reproducer on an ancient dual core w/o vmx and w/o any qemu binaries installed. Bruce also tested it on a slightly more modern dual socket xeon with vmx and confirmed it failed there..

Inside there is a 00-runme ; mostly a copy of qemu args the yocto automated tests were using. There is also everything the qemu binaries need to run ; toplevel dir is noisy since qemu only looks in ./ it seems. There is also an ext4.img ; as mentioned earlier, this only happens when uvesafb.ko is a module, so one has to loopback mount that image and repopulate /lib/modules/ for each boot test/bisect step.

I've also included 00-bisect.txt as the output of git bisect log. And there is also 00-configs/ dir that has the ".config" kernel file for each build (dir names are "git describe" in here for easy correlation) done for the bisect (plus the latest mainline build). The failing commit in the subject is v4.1-rc5-22-g9cd25aac1f44 .

My contribution here is largely a bisect that can be relied on and providing a portable reproducer of the regression; I am by no means a PAT expert ; Richard invested more time into actually understanding the problem than I did, so I'm going to totally throw him under the bus on this when it comes to considering the ultimate root cause and possible fixes. :)

Paul.
--

Paul Gortmaker

author

2016-03-03 21:18 UTC #1 lore raw

Quoted 19 lines · [runtime regression with "x86/mm/pat: Emulate PAT when it is disabled"] On…

So, the yocto folks moved from 4.1 to 4.4 and one of their automated qemu x86-32 boot tests started failing. None of the yocto details seem to matter since I offered to help and I've repropduced it using 100% mainline kernels and a generic distro toolchain as well.
The test case is slightly complicated, in that it relies on uvesafb being modular, and so one has to juggle modules within an ext4 image that qemu boots from. We tried making uvesafb builtin, but that made the issue magically vanish. Given PAT, this isn't too surprising.
Richard did the preliminary investigation and analysis, and from that I did a bisect, and found the commit in $SUBJECT to be the root cause, as per the discussion here:
http://lists.openembedded.org/pipermail/openembedded-core/2016-March/118397.html
I'd mentioned the above to bpetkov on IRC and after confirming it was still an issue on 4.5-rc6, he'd asked if I had a portable reproducer.

Quoted 8 lines

Not sure how complicated that would be, I set out to make one from my
build. With a little LD_PRELOAD type magic and ensuring all the qemu
components are in ./ I have one that runs on an otherwise qemu-free x86-64 box.
The stand alone reproducer is here; launched in 00-runme:
http://openlinux.wrs.com/pat-splat/reproducer.tar.bz2

Apologies, I'd used an internal DNS abbreviation here that isn't global. Replace the wrs with windriver and everything should be good.

P.
--

Quoted 49 lines

It is nothing fancy, just a generic yocto build of "sato" (gfx enabled rootfs). When it "works" it boots to a UI touchscreen interface. When it fails, you get a black screen with a blinking cursor (as seen in "vncviewer localhost:0").
Upon failure, you can do <Ctrl>-<Alt>-<2> to get to a passwd-less root login ; there you can run dmesg and see the splat. The image is currently using 4.5-rc6 ; but any kernel can be inserted; "make modules_install INSTALL_MOD_PATH=here" and then populating those modules from "here" into /lib/modules of the loopback mounted image. And of
course updating the bzImage on the qemu cmdline. Currently it
contains a bzImage and modules for 4.5-rc6 as I last tested that.
Also note that vncviewer will disconnect when it goes from early boot 80x25 to a higer res gfx mode; just reconnect and continue observing the target.
I've ruled out yocto kernel changes, and yocto toolchain -- but maybe it is a qemu issue this commit triggers ; who knows at this point.
Since I've NFI what component(s) cause this, I wanted to have the qemu binary, all libraries etc as part of the reproducer and nothing left to chance, and I've tested the reproducer on an ancient dual core w/o vmx and w/o any qemu binaries installed. Bruce also tested it on a slightly more modern dual socket xeon with vmx and confirmed it failed there..
Inside there is a 00-runme ; mostly a copy of qemu args the yocto automated tests were using. There is also everything the qemu binaries need to run ; toplevel dir is noisy since qemu only looks in ./ it seems. There is also an ext4.img ; as mentioned earlier, this only happens when uvesafb.ko is a module, so one has to loopback mount that image and repopulate /lib/modules/ for each boot test/bisect step.
I've also included 00-bisect.txt as the output of git bisect log. And there is also 00-configs/ dir that has the ".config" kernel file for each build (dir names are "git describe" in here for easy correlation) done for the bisect (plus the latest mainline build). The failing commit in the subject is v4.1-rc5-22-g9cd25aac1f44 .
My contribution here is largely a bisect that can be relied on and providing a portable reproducer of the regression; I am by no means a PAT expert ; Richard invested more time into actually understanding the problem than I did, so I'm going to totally throw him under the bus on this when it comes to considering the ultimate root cause and possible fixes. :)
Paul.
--

Toshi Kani

2016-03-04 04:10 UTC #2 lore raw

In reply to Paul Gortmaker

Quoted 25 lines · Paul Gortmaker

So, the yocto folks moved from 4.1 to 4.4 and one of their automated qemu x86-32 boot tests started failing.  None of the yocto details seem to matter since I offered to help and I've repropduced it using 100% mainline kernels and a generic distro toolchain as well.
The test case is slightly complicated, in that it relies on uvesafb being modular, and so one has to juggle modules within an ext4 image that qemu boots from.  We tried making uvesafb builtin, but that made the issue magically vanish.  Given PAT, this isn't too surprising.
Richard did the preliminary investigation and analysis, and from that I did a bisect, and found the commit in $SUBJECT to be the root cause, as per the discussion here:
http://lists.openembedded.org/pipermail/openembedded-core/2016-March/1183
97.html
I'd mentioned the above to bpetkov on IRC and after confirming it was still an issue on 4.5-rc6, he'd asked if I had a portable reproducer.
Not sure how complicated that would be, I set out to make one from my build.   With a little LD_PRELOAD type magic and ensuring all the qemu components are in ./  I have one that runs on an otherwise qemu-free x86-64 box.

Quoted 8 lines

The stand alone reproducer is here; launched in 00-runme:
http://openlinux.wrs.com/pat-splat/reproducer.tar.bz2
It is nothing fancy, just a generic yocto build of "sato" (gfx enabled rootfs). When it "works" it boots to a UI touchscreen interface. When it fails, you get a black screen with a blinking cursor (as seen in "vncviewer localhost:0").

Thanks for tracking down, and packaging the reproducer. I simply untar'd and ran 00-runme, but was not able to connect with localhost:0. I am not familiar with qemu, so I have not looked into why, though...

Anyway, with regarding the error message:
"x86/PAT: Xorg:705 map pfn expected mapping type uncached-minus for [mem 0xfd000000-0xfdffffff], got write-combining"

Did it came from the following path during fork()? copy_process
copy_mm
dup_mm
dup_mmap
copy_page_range
track_pfn_copy
reserve_pfn_range

If so, track_pfn_copy() obtained pgprot from a PTE, and called reserve_pfn_range() with it. So, the error message indicates that previous ioremap_wc() (i.e. pcm WC) resulted in creating UC- map (i.e. pgprot UC-). pcm is a logical cache type and pgprot is a HW cache type. They can be different when CPU does not have support for a given logical type. This WC to UC- conversion happens when CPU does not support PAT.

Richard's change, which compares with pgprot values in reserve_pfn_range() is a good one, but I do not understand how we get into this mess. We do not have this check when PAT is disabled, and WC is supported when PAT is enabled.

Commit 9cd25aac1 changed the initial values of the pcm<->pgrot conversion tables. The tables should be initialized with the same values after pat_init() is called. Is there any possibility that ioremap_wc() was called before pat_init()..?

Also, can you send me a whole dmesg output? I'd like to check how PAT is initialized.

Thanks!
-Toshi

Paul Gortmaker

author

2016-03-04 18:37 UTC #3 lore raw

Quoted 30 lines · [Re: runtime regression with "x86/mm/pat: Emulate PAT when it is disabled"] On…

Quoted 29 lines · Paul Gortmaker
So, the yocto folks moved from 4.1 to 4.4 and one of their automated qemu x86-32 boot tests started failing.  None of the yocto details seem to matter since I offered to help and I've repropduced it using 100% mainline kernels and a generic distro toolchain as well.
The test case is slightly complicated, in that it relies on uvesafb being modular, and so one has to juggle modules within an ext4 image that qemu boots from.  We tried making uvesafb builtin, but that made the issue magically vanish.  Given PAT, this isn't too surprising.
Richard did the preliminary investigation and analysis, and from that I did a bisect, and found the commit in $SUBJECT to be the root cause, as per the discussion here:
http://lists.openembedded.org/pipermail/openembedded-core/2016-March/1183
97.html
I'd mentioned the above to bpetkov on IRC and after confirming it was still an issue on 4.5-rc6, he'd asked if I had a portable reproducer.
Not sure how complicated that would be, I set out to make one from my build.   With a little LD_PRELOAD type magic and ensuring all the qemu components are in ./  I have one that runs on an otherwise qemu-free x86-64 box.
The stand alone reproducer is here; launched in 00-runme:
http://openlinux.wrs.com/pat-splat/reproducer.tar.bz2

Quoted 8 lines

Quoted 4 lines
It is nothing fancy, just a generic yocto build of "sato" (gfx enabled rootfs). When it "works" it boots to a UI touchscreen interface. When it fails, you get a black screen with a blinking cursor (as seen in "vncviewer localhost:0").
Thanks for tracking down, and packaging the reproducer. I simply untar'd and ran 00-runme, but was not able to connect with localhost:0. I am not familiar with qemu, so I have not looked into why, though...

Maybe it was localhost:1 in your case? The qemu should have indicated what vncserver sessions it started. Can you paste in the output from
the 00-runme? I tested the reproducer on a machine that was physically
distinct from the build, and that was a generic ubuntu install, but with no qemu support installed at all and it worked there. Plus I got Bruce to test it worked on his machine, so I'm rather surprised it did not work for you.

Quoted 5 lines

Anyway, with regarding the error message:
"x86/PAT: Xorg:705 map pfn expected mapping type uncached-minus for [mem 0xfd000000-0xfdffffff], got write-combining"

Quoted 8 lines

Did it came from the following path during fork()? copy_process
copy_mm
dup_mm
dup_mmap
copy_page_range
track_pfn_copy
reserve_pfn_range

The trace is consistent, and was already captured by Richard, as per:

http://lists.openembedded.org/pipermail/openembedded-core/2016-March/118397.html

which is the link given earlier. When I say consistent, I mean that I get essentially the same thing when booting 4.5-rc6:

[   30.098100] x86/PAT: Xorg:509 map pfn expected mapping type uncached-minus for [mem 0xfd000000-0xfdffffff], got write-combining
[   30.106782] ------------[ cut here ]------------
[   30.107093] WARNING: CPU: 0 PID: 509 at /home/paul/poky/build/tmp-glibc/work-shared/qemux86/kernel-source/arch/x86/mm/pat.c:986 untrack_pfn+0x9f/0xb0()
[   30.112553] Modules linked in: 8021q parport_pc parport floppy uvesafb
[   30.113766] CPU: 0 PID: 509 Comm: Xorg Not tainted 4.5.0-rc6-yocto-standard #1
[   30.113806] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.8.2-0-g33fbe13 by qemu-project.org 04/01/2014
[   30.114078]  00000000 00003286 c0149d78 c13a6c7f 00000000 00000000 c0149dac c1052bcb
[   30.114214]  c1ac7544 00000000 000001fd c1ac1ea4 000003da c104cbdf 000003da c104cbdf
[   30.114214]  00000000 cdcf0528 00000000 c0149dbc c1052ca2 00000009 00000000 c0149de0
[   30.114214] Call Trace:
[   30.114214]  [<c13a6c7f>] dump_stack+0x58/0x79
[   30.114214]  [<c1052bcb>] warn_slowpath_common+0x8b/0xc0
[   30.114214]  [<c104cbdf>] ? untrack_pfn+0x9f/0xb0
[   30.114214]  [<c104cbdf>] ? untrack_pfn+0x9f/0xb0
[   30.114214]  [<c1052ca2>] warn_slowpath_null+0x22/0x30
[   30.114214]  [<c104cbdf>] untrack_pfn+0x9f/0xb0
[   30.114214]  [<c104ecf4>] ? __kunmap_atomic+0x54/0x110
[   30.114214]  [<c114f1cf>] unmap_single_vma+0x56f/0x580
[   30.114214]  [<c11321d0>] ? pagevec_move_tail_fn+0xa0/0xa0
[   30.114214]  [<c1150123>] unmap_vmas+0x43/0x60
[   30.114214]  [<c1154d5f>] exit_mmap+0x5f/0xf0
[   30.114214]  [<c10507bd>] mmput+0x2d/0xa0
[   30.114214]  [<c1051c19>] copy_process.part.47+0x1229/0x1430
[   30.114214]  [<c1051fb4>] _do_fork+0xb4/0x3b0
[   30.114214]  [<c105239c>] SyS_clone+0x2c/0x30
[   30.114214]  [<c1001a04>] do_syscall_32_irqs_on+0x54/0xb0
[   30.114214]  [<c18b06ca>] entry_INT80_32+0x2a/0x2a
[   30.124383] ---[ end trace f7c8a5d94542f94e ]---

Quoted 13 lines

If so, track_pfn_copy() obtained pgprot from a PTE, and called reserve_pfn_range() with it. So, the error message indicates that previous ioremap_wc() (i.e. pcm WC) resulted in creating UC- map (i.e. pgprot UC-). pcm is a logical cache type and pgprot is a HW cache type. They can be different when CPU does not have support for a given logical type. This WC to UC- conversion happens when CPU does not support PAT.
Richard's change, which compares with pgprot values in reserve_pfn_range() is a good one, but I do not understand how we get into this mess. We do not have this check when PAT is disabled, and WC is supported when PAT is enabled.

Quoted 4 lines

Commit 9cd25aac1 changed the initial values of the pcm<->pgrot conversion tables. The tables should be initialized with the same values after pat_init() is called. Is there any possibility that ioremap_wc() was called before pat_init()..?

I don't think it is an initcall ordering thing; recall that I said the problem seems to go away when built-in vs uvesafb as module. So given that, I think it is more related to where the code lands.

Quoted 3 lines

Also, can you send me a whole dmesg output? I'd like to check how PAT is initialized.

I'll send the full file off list vs. spamming everyone with it. I'm open to booting the pre-fail commit with PAT specific bootargs and the post-fail with the same and diffing the two dmesg if there are bootargs you'd like me to test. I'd also like to ensure you have a working reproducer locally so maybe we should look at how that failed 1st.

Thanks,
Paul.
--

Quoted 3 lines

Thanks!
-Toshi

Toshi Kani

2016-03-04 21:20 UTC #4 lore raw

Quoted 4 lines · Paul Gortmaker

[Re: runtime regression with "x86/mm/pat: Emulate PAT when it is
Quoted 1 line · disabled"] On 03/03/2016 (Thu 22:02) Toshi Kani wrote:
On Thu, 2016-03-03 at 15:59 -0500, Paul Gortmaker wrote:

Quoted 15 lines

Quoted 11 lines
Quoted 10 lines
The stand alone reproducer is here; launched in 00-runme:
http://openlinux.wrs.com/pat-splat/reproducer.tar.bz2
It is nothing fancy, just a generic yocto build of "sato" (gfx enabled rootfs). When it "works" it boots to a UI touchscreen interface. When
it fails, you get a black screen with a blinking cursor (as seen in "vncviewer localhost:0").
Quoted 3 lines
Thanks for tracking down, and packaging the reproducer. I simply untar'd and ran 00-runme, but was not able to connect with localhost:0. I am not familiar with qemu, so I have not looked into why, though...

Quoted 7 lines

Maybe it was localhost:1 in your case? The qemu should have indicated what vncserver sessions it started. Can you paste in the output from the 00-runme? I tested the reproducer on a machine that was physically distinct from the build, and that was a generic ubuntu install, but with no qemu support installed at all and it worked there. Plus I got Bruce to test it worked on his machine, so I'm rather surprised it did not work for you.

I am not really sure what I am doing is correct.

On one window:

# ./00-runme
Warning: vlan 0 is not connected to host network
VNC server running on '::1:5900'

And another window on the same system:

# vncviewer localhost:1

TigerVNC Viewer 64-bit v1.6.0
Built on: 2016-01-04 15:09
Copyright (C) 1999-2015 TigerVNC Team and many others (see README.txt) See http://www.tigervnc.org for information on TigerVNC. Can't open display:

Quoted 14 lines

Quoted 5 lines
Anyway, with regarding the error message:
"x86/PAT: Xorg:705 map pfn expected mapping type uncached-minus for [mem
0xfd000000-0xfdffffff], got write-combining"
Quoted 8 lines
Did it came from the following path during fork()? copy_process
copy_mm
dup_mm
dup_mmap
copy_page_range
track_pfn_copy
reserve_pfn_range

Quoted 7 lines

The trace is consistent, and was already captured by Richard, as per:
http://lists.openembedded.org/pipermail/openembedded-core/2016-March/1183
97.html
which is the link given earlier. When I say consistent, I mean that I get essentially the same thing when booting 4.5-rc6:

There are two issues here.
1. copy_process() failed in the check in reserve_pfn_range()
2. error path in copy_process() then hit WARN_ON_ONCE in untrack_pfn().

We are currently looking into #1, which Richard referred as:

| It appears that Xorg mmaps this from userspace, then later does a
| fork() to execute a utility. At this point, when creating the vmas for
| the new process, the pat code says "eeek!" as the protection mode for
| the new vmas don't match the old one, returns -EINVAL, the process dies
| and X goes with it.

Since it seemed that Richard had some analysis on #1, I wanted to confirm that the failure path of #1 was the above stack trace.

The stack trace below shows the failure path of #2.

Quoted 56 lines

[   30.098100] x86/PAT: Xorg:509 map pfn expected mapping type uncached- minus for [mem 0xfd000000-0xfdffffff], got write-combining
[   30.106782] ------------[ cut here ]------------
[   30.107093] WARNING: CPU: 0 PID: 509 at /home/paul/poky/build/tmp- glibc/work-shared/qemux86/kernel-source/arch/x86/mm/pat.c:986 untrack_pfn+0x9f/0xb0()
[   30.112553] Modules linked in: 8021q parport_pc parport floppy uvesafb [   30.113766] CPU: 0 PID: 509 Comm: Xorg Not tainted 4.5.0-rc6-yocto- standard #1
[   30.113806] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.8.2-0-g33fbe13 by qemu-project.org 04/01/2014
[   30.114078]  00000000 00003286 c0149d78 c13a6c7f 00000000 00000000 c0149dac c1052bcb
[   30.114214]  c1ac7544 00000000 000001fd c1ac1ea4 000003da c104cbdf 000003da c104cbdf
[   30.114214]  00000000 cdcf0528 00000000 c0149dbc c1052ca2 00000009 00000000 c0149de0
[   30.114214] Call Trace:
[   30.114214]  [<c13a6c7f>] dump_stack+0x58/0x79
[   30.114214]  [<c1052bcb>] warn_slowpath_common+0x8b/0xc0
[   30.114214]  [<c104cbdf>] ? untrack_pfn+0x9f/0xb0
[   30.114214]  [<c104cbdf>] ? untrack_pfn+0x9f/0xb0
[   30.114214]  [<c1052ca2>] warn_slowpath_null+0x22/0x30
[   30.114214]  [<c104cbdf>] untrack_pfn+0x9f/0xb0
[   30.114214]  [<c104ecf4>] ? __kunmap_atomic+0x54/0x110
[   30.114214]  [<c114f1cf>] unmap_single_vma+0x56f/0x580
[   30.114214]  [<c11321d0>] ? pagevec_move_tail_fn+0xa0/0xa0 [   30.114214]  [<c1150123>] unmap_vmas+0x43/0x60
[   30.114214]  [<c1154d5f>] exit_mmap+0x5f/0xf0
[   30.114214]  [<c10507bd>] mmput+0x2d/0xa0
[   30.114214]  [<c1051c19>] copy_process.part.47+0x1229/0x1430 [   30.114214]  [<c1051fb4>] _do_fork+0xb4/0x3b0
[   30.114214]  [<c105239c>] SyS_clone+0x2c/0x30
[   30.114214]  [<c1001a04>] do_syscall_32_irqs_on+0x54/0xb0 [   30.114214]  [<c18b06ca>] entry_INT80_32+0x2a/0x2a
[   30.124383] ---[ end trace f7c8a5d94542f94e ]---
Quoted 14 lines
If so, track_pfn_copy() obtained pgprot from a PTE, and called reserve_pfn_range() with it. So, the error message indicates that previous ioremap_wc() (i.e. pcm WC) resulted in creating UC- map (i.e. pgprot UC-). pcm is a logical cache type and pgprot is a HW cache type. They can be different when CPU does not have support for a given logical type. This WC to UC- conversion happens when CPU does not support PAT.
Richard's change, which compares with pgprot values in reserve_pfn_range() is a good one, but I do not understand how we get into this mess. We do not have this check when PAT is disabled, and WC is supported when PAT is enabled.
Quoted 4 lines
Commit 9cd25aac1 changed the initial values of the pcm<->pgrot conversion tables. The tables should be initialized with the same values after pat_init() is called. Is there any possibility that ioremap_wc() was called before pat_init()..?

Quoted 5 lines

I don't think it is an initcall ordering thing; recall that I said the problem seems to go away when built-in vs uvesafb as module. So given that, I think it is more related to where the code lands.
From the failure, it looks as if pat_init() was not called at all, but I

cannot explain how built-in vs module can make such difference.

Quoted 8 lines

Quoted 2 lines
Also, can you send me a whole dmesg output? I'd like to check how PAT is initialized.
I'll send the full file off list vs. spamming everyone with it. I'm open to booting the pre-fail commit with PAT specific bootargs and the post- fail with the same and diffing the two dmesg if there are bootargs you'd like me to test. I'd also like to ensure you have a working reproducer locally so maybe we should look at how that failed 1st.

Great. Yes, two dmesg will be really helpful.

Thanks,
-Toshi

Paul Gortmaker

author

2016-03-07 00:35 UTC #5 lore raw

Quoted 29 lines · [Re: runtime regression with "x86/mm/pat: Emulate PAT when it is disabled"] On…

Quoted 4 lines · Paul Gortmaker
[Re: runtime regression with "x86/mm/pat: Emulate PAT when it is
Quoted 1 line · disabled"] On 03/03/2016 (Thu 22:02) Toshi Kani wrote:
On Thu, 2016-03-03 at 15:59 -0500, Paul Gortmaker wrote:
:
Quoted 15 lines
Quoted 11 lines
Quoted 10 lines
The stand alone reproducer is here; launched in 00-runme:
http://openlinux.wrs.com/pat-splat/reproducer.tar.bz2
It is nothing fancy, just a generic yocto build of "sato" (gfx enabled rootfs). When it "works" it boots to a UI touchscreen interface. When
it fails, you get a black screen with a blinking cursor (as seen in "vncviewer localhost:0").
Quoted 3 lines
Thanks for tracking down, and packaging the reproducer. I simply untar'd and ran 00-runme, but was not able to connect with localhost:0. I am not familiar with qemu, so I have not looked into why, though...
Quoted 7 lines
Maybe it was localhost:1 in your case? The qemu should have indicated what vncserver sessions it started. Can you paste in the output from the 00-runme? I tested the reproducer on a machine that was physically distinct from the build, and that was a generic ubuntu install, but with no qemu support installed at all and it worked there. Plus I got Bruce to test it worked on his machine, so I'm rather surprised it did not work for you.

Quoted 6 lines

I am not really sure what I am doing is correct.
On one window:
# ./00-runme
Warning: vlan 0 is not connected to host network
VNC server running on '::1:5900'

Ah, it seems for some reason your system confuses qemu from using the
"normal" IPv4 default. Try editing ./00-runme and add an explicit
vnc option "-vnc 127.0.0.1:5" in front of "-show-cursor" and then:

Quoted 3 lines

And another window on the same system:
# vncviewer localhost:1

...connect to localhost:5 here. I chose 5 just to not bump into anything your system might have on :0 or :1 already.

Quoted 7 lines

TigerVNC Viewer 64-bit v1.6.0
Built on: 2016-01-04 15:09
Copyright (C) 1999-2015 TigerVNC Team and many others (see README.txt) See http://www.tigervnc.org for information on TigerVNC. Can't open display:

[snip analysis, leaving that for Richard to comment on.]

Quoted 11 lines

Quoted 9 lines
Quoted 3 lines
Also, can you send me a whole dmesg output? I'd like to check how PAT is initialized.
I'll send the full file off list vs. spamming everyone with it. I'm open to booting the pre-fail commit with PAT specific bootargs and the post- fail with the same and diffing the two dmesg if there are bootargs you'd like me to test. I'd also like to ensure you have a working reproducer locally so maybe we should look at how that failed 1st.
Great. Yes, two dmesg will be really helpful.

So I booted both with "debugpat" on the bootline; both being the last working commit [v4.1-rc5-21-g9dac62909451] and then the 1st failing commit [v4.1-rc5-22-g9cd25aac1f44]. I captured the dmesg of each, then stripped the timestamps and diffed them.

fail.txt renamed from works.txt +31 -8

@@ -454,15 +454,38 @@
  8021q: 802.1Q VLAN Support v1.8
  8021q: adding VLAN 0 to HW filter on device eth0
  x86/PAT: Overlap at 0xfd000000-0xfe000000
- x86/PAT: reserve_memtype added [mem 0xfd000000-0xfdffffff], track write-combining, req write-combining, ret write-combining
- x86/PAT: Overlap at 0xfe000000-0xfe010000
- x86/PAT: reserve_memtype added [mem 0xfe000000-0xfe00ffff], track uncached-minus, req uncached-minus, ret uncached-minus
+ x86/PAT: reserve_memtype added [mem 0xfd000000-0xfdffffff], track write-combining, req uncached-minus, ret write-combining
  x86/PAT: free_memtype request [mem 0xfd000000-0xfdffffff]
- x86/PAT: free_memtype request [mem 0xfe000000-0xfe00ffff]
+ x86/PAT: Xorg:475 map pfn expected mapping type uncached-minus for [mem 0xfd000000-0xfdffffff], got write-combining
+ ------------[ cut here ]------------
+ WARNING: CPU: 0 PID: 475 at /home/paul/poky/build/tmp-glibc/work-shared/qemux86/kernel-source/arch/x86/mm/pat.c:938 untrack_pfn+0x9f/0xb0()
+ Modules linked in: 8021q parport_pc parport floppy uvesafb
+ CPU: 0 PID: 475 Comm: Xorg Not tainted 4.1.0-rc5-yocto-standard #1
+ Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.8.2-0-g33fbe13 by qemu-project.org 04/01/2014
+  00000000 00000000 cdf75db0 c1837711 00000000 cdf75de4 c104f0cb c1a39d10
+  00000000 000001db c1a38f54 000003aa c1049ddf 000003aa c1049ddf 00000000
+  c0108840 00000000 cdf75df4 c104f1a2 00000009 00000000 cdf75e18 c1049ddf
+ Call Trace:
+  [<c1837711>] dump_stack+0x4b/0x75
+  [<c104f0cb>] warn_slowpath_common+0x8b/0xc0
+  [<c1049ddf>] ? untrack_pfn+0x9f/0xb0
+  [<c1049ddf>] ? untrack_pfn+0x9f/0xb0
+  [<c104f1a2>] warn_slowpath_null+0x22/0x30
+  [<c1049ddf>] untrack_pfn+0x9f/0xb0
+  [<c104b9d3>] ? __kunmap_atomic+0x33/0xc0
+  [<c113c07f>] unmap_single_vma+0x49f/0x4b0
+  [<c113cdc3>] unmap_vmas+0x43/0x60
+  [<c11428ff>] exit_mmap+0x5f/0xf0
+  [<c1072d39>] ? get_parent_ip+0x9/0x40
+  [<c104cccd>] mmput+0x2d/0xa0
+  [<c104dfdd>] copy_process.part.45+0x10dd/0x14a0
+  [<c104e541>] do_fork+0xc1/0x390
+  [<c104e8d5>] SyS_clone+0x25/0x30
+  [<c183ef46>] syscall_call+0x7/0x7
+ ---[ end trace dbbf5a6b2dea64ff ]---
  x86/PAT: Overlap at 0xfd000000-0xfe000000
- x86/PAT: reserve_memtype added [mem 0xfd000000-0xfdffffff], track write-combining, req write-combining, ret write-combining
- x86/PAT: Overlap at 0xfe000000-0xfe010000
- x86/PAT: reserve_memtype added [mem 0xfe000000-0xfe00ffff], track uncached-minus, req uncached-minus, ret uncached-minus
+ x86/PAT: reserve_memtype added [mem 0xfd000000-0xfdffffff], track write-combining, req uncached-minus, ret write-combining
  x86/PAT: free_memtype request [mem 0xfd000000-0xfdffffff]
+ x86/PAT: Xorg:475 map pfn expected mapping type uncached-minus for [mem 0xfd000000-0xfdffffff], got write-combining
  x86/PAT: free_memtype request [mem 0xfe000000-0xfe00ffff]
- hrtimer: interrupt took 5057254 ns
+ x86/PAT: free_memtype request [mem 0xfd000000-0xfdffffff]

One obvious difference is the number of overlaps:

~$cat fail.txt |grep Overlap | wc -l
7
~$cat works.txt |grep Overlap | wc -l
9
~$

I'm assuming everyone here is used to reading diffs, but if someone wants the full dmesg files let me know and I'll send them off-list.

Paul.

Toshi Kani

2016-03-07 15:10 UTC #6 lore raw

Quoted 7 lines · Paul Gortmaker

[Re: runtime regression with "x86/mm/pat: Emulate PAT when it is
Quoted 4 lines · disabled"] On 04/03/2016 (Fri 15:12) Toshi Kani wrote:
Quoted 3 lines · Paul Gortmaker
[Re: runtime regression with "x86/mm/pat: Emulate PAT when it is disabled"] On 03/03/2016 (Thu 22:02) Toshi Kani wrote:

Quoted 12 lines

Quoted 6 lines
I am not really sure what I am doing is correct.
On one window:
# ./00-runme
Warning: vlan 0 is not connected to host network
VNC server running on '::1:5900'
Ah, it seems for some reason your system confuses qemu from using the "normal" IPv4 default. Try editing ./00-runme and add an explicit vnc option "-vnc 127.0.0.1:5" in front of "-show-cursor" and then:
Quoted 1 line

Quoted 5 lines

Quoted 2 lines
And another window on the same system:
# vncviewer localhost:1
...connect to localhost:5 here. I chose 5 just to not bump into anything your system might have on :0 or :1 already.

Still no luck. Perhaps, vlan needs to connect?

# ./00-runme
Warning: vlan 0 is not connected to host network

# vncviewer localhost:5

Quoted 2 lines

I'm assuming everyone here is used to reading diffs, but if someone wants the full dmesg files let me know and I'll send them off-list.

Yes, please send me full dmesg files. Since I do not know your original state, the diff does not give me the whole picture.

Thanks!
-Toshi

A message that is not in the archive (sent off-list or not yet received): [email protected]

Toshi Kani

2016-03-07 22:46 UTC #8 lore raw

Quoted 47 lines · Paul Gortmaker

[dropping oe list and lkml since attaching dmesg files.]
[Re: runtime regression with "x86/mm/pat: Emulate PAT when it is
Quoted 24 lines · disabled"] On 07/03/2016 (Mon 09:03) Toshi Kani wrote:
Quoted 7 lines · Paul Gortmaker
[Re: runtime regression with "x86/mm/pat: Emulate PAT when it is
Quoted 4 lines · disabled"] On 04/03/2016 (Fri 15:12) Toshi Kani wrote:
Quoted 3 lines · Paul Gortmaker
[Re: runtime regression with "x86/mm/pat: Emulate PAT when it is disabled"] On 03/03/2016 (Thu 22:02) Toshi Kani wrote:
:
Quoted 15 lines
Quoted 6 lines
I am not really sure what I am doing is correct.
On one window:
# ./00-runme
Warning: vlan 0 is not connected to host network
VNC server running on '::1:5900'
Ah, it seems for some reason your system confuses qemu from using the "normal" IPv4 default. Try editing ./00-runme and add an explicit vnc option "-vnc 127.0.0.1:5" in front of "-show-cursor" and then:
Quoted 3 lines
And another window on the same system:
# vncviewer localhost:1
Quoted 7 lines
Quoted 2 lines
...connect to localhost:5 here. I chose 5 just to not bump into anything your system might have on :0 or :1 already.
Still no luck. Perhaps, vlan needs to connect?
# ./00-runme
Warning: vlan 0 is not connected to host network
Nope, I get that too, it isn't critical.
Quoted 9 lines
# vncviewer localhost:5
TigerVNC Viewer 64-bit v1.6.0
Built on: 2016-01-04 15:09
Copyright (C) 1999-2015 TigerVNC Team and many others (see README.txt) See http://www.tigervnc.org for information on TigerVNC. Can't open display:

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
I'm assuming everyone here is used to reading diffs, but if someone wants the full dmesg files let me know and I'll send them off-list.
Yes, please send me full dmesg files. Since I do not know your original state, the diff does not give me the whole picture.
Attached.

Thanks for the dmesg files! As I suspected, there is no message from pat_init() in both cases. That is, you are missing the following message, which shows how PAT is configured to support cache attributes.

# dmesg | grep PAT
[0.000000] x86/PAT: Configuration [0-7]: WB  WC  UC- UC  WB  WC  UC- WT

It may have seemed working before, but you did not have WC configured to PAT without calling pat_init(). There was not a proper check in place to detect this error before. Can you please check your code to see what caused this skip of pat_init()? If you have a git tree, I can take a look as well.

-Toshi

Paul Gortmaker

author

2016-03-07 23:53 UTC #9 lore raw

Quoted 3 lines · [Re: runtime regression with "x86/mm/pat: Emulate PAT when it is disabled"] On…

Quoted 2 lines · Paul Gortmaker
[dropping oe list and lkml since attaching dmesg files.]

[...]

Quoted 11 lines

Quoted 4 lines
Quoted 2 lines
Yes, please send me full dmesg files. Since I do not know your original state, the diff does not give me the whole picture.
Attached.
Thanks for the dmesg files! As I suspected, there is no message from pat_init() in both cases. That is, you are missing the following message, which shows how PAT is configured to support cache attributes.
# dmesg | grep PAT
[0.000000] x86/PAT: Configuration [0-7]: WB  WC  UC- UC  WB  WC  UC- WT  

Interesting...

Quoted 6 lines

It may have seemed working before, but you did not have WC configured to PAT without calling pat_init(). There was not a proper check in place to detect this error before. Can you please check your code to see what caused this skip of pat_init()? If you have a git tree, I can take a look as well.

You already have git copies of what I'm running, since it is vanilla mainline commits. No code changes at this end whatsoever. I did the bisect on vanilla mainline. All I took from yocto was their ".config"

To recap, v4.1-rc5-21-g9dac62909451 works, v4.1-rc5-22-g9cd25aac1f44 fails, and v4.5-rc6 also fails. If pat_init() isn't called then this is a bug in current mainline. I'll have a look later myself and see if I can trace out how we expect to get to pat_init() and how that might be skipped inadvertently unless someone beats me to it.

Paul.
--

Quoted 2 lines

-Toshi

Toshi Kani

2016-03-08 00:03 UTC #10 lore raw

Quoted 36 lines · Paul Gortmaker

[Re: runtime regression with "x86/mm/pat: Emulate PAT when it is
Quoted 3 lines · disabled"] On 07/03/2016 (Mon 16:38) Toshi Kani wrote:
Quoted 2 lines · Paul Gortmaker
[dropping oe list and lkml since attaching dmesg files.]
[...]
Quoted 5 lines
Quoted 4 lines
Quoted 2 lines
Yes, please send me full dmesg files. Since I do not know your original state, the diff does not give me the whole picture.
Attached.
Quoted 8 lines
Thanks for the dmesg files! As I suspected, there is no message from pat_init() in both cases. That is, you are missing the following message,
which shows how PAT is configured to support cache attributes.
# dmesg | grep PAT
[0.000000] x86/PAT: Configuration [0-7]: WB  WC  UC- UC  WB  WC  UC-
WT  
Interesting...
Quoted 6 lines
It may have seemed working before, but you did not have WC configured to PAT without calling pat_init(). There was not a proper check in place to detect this error before. Can you please check your code to see what caused this skip of pat_init()? If you have a git tree, I can take a look as well.
You already have git copies of what I'm running, since it is vanilla mainline commits. No code changes at this end whatsoever. I did the bisect on vanilla mainline. All I took from yocto was their ".config"

Quoted 5 lines

To recap, v4.1-rc5-21-g9dac62909451 works, v4.1-rc5-22-g9cd25aac1f44 fails, and v4.5-rc6 also fails. If pat_init() isn't called then this is a bug in current mainline. I'll have a look later myself and see if I can trace out how we expect to get to pat_init() and how that might be skipped inadvertently unless someone beats me to it.

Oh, I see. Can you send me the ".config" file?

Thanks,
-Toshi

Toshi Kani

2016-03-08 00:42 UTC #11 lore raw

Quoted 38 lines · Toshi Kani

Quoted 37 lines · Paul Gortmaker
[Re: runtime regression with "x86/mm/pat: Emulate PAT when it is
Quoted 3 lines · disabled"] On 07/03/2016 (Mon 16:38) Toshi Kani wrote:
Quoted 2 lines · Paul Gortmaker
[dropping oe list and lkml since attaching dmesg files.]
[...]
Quoted 5 lines
Quoted 4 lines
Quoted 2 lines
Yes, please send me full dmesg files. Since I do not know your original state, the diff does not give me the whole picture.
Attached.
Quoted 8 lines
Thanks for the dmesg files! As I suspected, there is no message from pat_init() in both cases. That is, you are missing the following message,
which shows how PAT is configured to support cache attributes.
# dmesg | grep PAT
[0.000000] x86/PAT: Configuration [0-7]: WB  WC  UC- UC  WB  WC  UC-
WT  
Interesting...
Quoted 7 lines
It may have seemed working before, but you did not have WC configured to PAT without calling pat_init(). There was not a proper check in place to detect this error before. Can you please check your code to see what caused this skip of pat_init()? If you have a git tree, I can
take a look as well.
You already have git copies of what I'm running, since it is vanilla mainline commits. No code changes at this end whatsoever. I did the bisect on vanilla mainline. All I took from yocto was their ".config"

Quoted 7 lines

Quoted 5 lines
To recap, v4.1-rc5-21-g9dac62909451 works, v4.1-rc5-22-g9cd25aac1f44 fails, and v4.5-rc6 also fails. If pat_init() isn't called then this is a bug in current mainline. I'll have a look later myself and see if I can trace out how we expect to get to pat_init() and how that might be skipped inadvertently unless someone beats me to it.
Oh, I see. Can you send me the ".config" file?

And also an output of /proc/cpuinfo, please?

I think I know what's going on. I noticed that you have the following message in your dmesg files.

[ 0.000000] MTRR: Disabled

MTRR is set to disabled when your CPU is Intel but does not support MTRR. Perhaps, QEMU does not emulate MTRR?

pat_init() is not called when MTRR is disabled. I think this dependency is wrong, and it needs to be fixed.

This issue has been there for a long time, and you have been running essentially as PAT disabled in the past. The commit in question simply detected this issue.

Thanks,
-Toshi

Paul Gortmaker

author

2016-03-08 03:28 UTC #12 lore raw

Quoted 45 lines · [Re: runtime regression with "x86/mm/pat: Emulate PAT when it is disabled"] On…

Quoted 44 lines · Toshi Kani
Quoted 42 lines · Paul Gortmaker
[Re: runtime regression with "x86/mm/pat: Emulate PAT when it is
Quoted 3 lines · disabled"] On 07/03/2016 (Mon 16:38) Toshi Kani wrote:
Quoted 2 lines · Paul Gortmaker
[dropping oe list and lkml since attaching dmesg files.]
[...]
Quoted 5 lines
Quoted 4 lines
Quoted 2 lines
Yes, please send me full dmesg files. Since I do not know your original state, the diff does not give me the whole picture.
Attached.
Quoted 8 lines
Thanks for the dmesg files! As I suspected, there is no message from pat_init() in both cases. That is, you are missing the following message,
which shows how PAT is configured to support cache attributes.
# dmesg | grep PAT
[0.000000] x86/PAT: Configuration [0-7]: WB  WC  UC- UC  WB  WC  UC-
WT  
Interesting...
Quoted 7 lines
It may have seemed working before, but you did not have WC configured to PAT without calling pat_init(). There was not a proper check in place to detect this error before. Can you please check your code to see what caused this skip of pat_init()? If you have a git tree, I can
take a look as well.
You already have git copies of what I'm running, since it is vanilla mainline commits. No code changes at this end whatsoever. I did the bisect on vanilla mainline. All I took from yocto was their ".config"
To recap, v4.1-rc5-21-g9dac62909451 works, v4.1-rc5-22-g9cd25aac1f44 fails, and v4.5-rc6 also fails. If pat_init() isn't called then this is a bug in current mainline. I'll have a look later myself and see if I can trace out how we expect to get to pat_init() and how that might be skipped inadvertently unless someone beats me to it.

Quoted 3 lines

Quoted 1 line
Oh, I see. Can you send me the ".config" file?
And also an output of /proc/cpuinfo, please?

Host? Guest? Both?

Quoted 7 lines

I think I know what's going on. I noticed that you have the following message in your dmesg files.
[ 0.000000] MTRR: Disabled
MTRR is set to disabled when your CPU is Intel but does not support MTRR.

I've run the test on a modern expensive xeon, a 4-5 year old xeon, and on an old pentium dual core (the cheaper dumbed down core2-duo that doesn't support virtualization) from around 2007. In all cases the result was the same. Perhaps that is because the qemu launch script appears to set the CPU type regardless? (it uses "-cpu qemu32" but I confess that I do not know exactly what silicon that tries to emulate).

Quoted 1 line

Perhaps, QEMU does not emulate MTRR?

I will be the 1st to admit that I am not a seasoned qemu user, so I've no idea if the above is true. I still prefer testing on real hardware, even if that comes across as "old school". :)

Quoted 7 lines

pat_init() is not called when MTRR is disabled. I think this dependency is wrong, and it needs to be fixed.
This issue has been there for a long time, and you have been running essentially as PAT disabled in the past. The commit in question simply detected this issue.

OK, that sounds good -- in that it seems we are finally getting to the bottom of what happened here. Any thoughts on why built-in vs. modular somehow managed to mask the issue?

Paul.
--

Quoted 3 lines

Thanks,
-Toshi

Toshi Kani

2016-03-08 15:46 UTC #13 lore raw

Quoted 50 lines · Paul Gortmaker

[Re: runtime regression with "x86/mm/pat: Emulate PAT when it is
Quoted 47 lines · disabled"] On 07/03/2016 (Mon 18:35) Toshi Kani wrote:
Quoted 46 lines · Toshi Kani
Quoted 44 lines · Paul Gortmaker
[Re: runtime regression with "x86/mm/pat: Emulate PAT when it is
Quoted 3 lines · disabled"] On 07/03/2016 (Mon 16:38) Toshi Kani wrote:
Quoted 2 lines · Paul Gortmaker
[dropping oe list and lkml since attaching dmesg files.]
[...]
Quoted 6 lines
Quoted 5 lines
Quoted 3 lines
Yes, please send me full dmesg files. Since I do not know your original state, the diff does not give me the whole picture.
Attached.
Quoted 8 lines
Thanks for the dmesg files! As I suspected, there is no message from pat_init() in both cases. That is, you are missing the following message, which shows how PAT is configured to support cache attributes.
# dmesg | grep PAT
[0.000000] x86/PAT: Configuration [0-7]:
WB  WC  UC- UC  WB  WC  UC- WT  
Interesting...
Quoted 6 lines
It may have seemed working before, but you did not have WC configured to PAT without calling pat_init(). There was not a proper check in place to detect this error before. Can you please check your code to see what caused this skip of pat_init()? If you have a git tree, I can take a look as well.
You already have git copies of what I'm running, since it is vanilla mainline commits. No code changes at this end whatsoever. I did the bisect on vanilla mainline. All I took from yocto was their ".config"
To recap, v4.1-rc5-21-g9dac62909451 works, v4.1-rc5-22- g9cd25aac1f44 fails, and v4.5-rc6 also fails. If pat_init() isn't called then this is a bug in current mainline. I'll have a look later myself and see if I can trace out how we expect to get to pat_init() and how that might be skipped inadvertently unless someone beats me to it.

Quoted 5 lines

Quoted 3 lines
Quoted 1 line
Oh, I see. Can you send me the ".config" file?
And also an output of /proc/cpuinfo, please?
Host? Guest? Both?

Guest.

Quoted 8 lines

Quoted 7 lines
I think I know what's going on. I noticed that you have the following message in your dmesg files.
[ 0.000000] MTRR: Disabled
MTRR is set to disabled when your CPU is Intel but does not support MTRR.

Quoted 6 lines

I've run the test on a modern expensive xeon, a 4-5 year old xeon, and on an old pentium dual core (the cheaper dumbed down core2-duo that doesn't support virtualization) from around 2007. In all cases the result was the same. Perhaps that is because the qemu launch script appears to set the CPU type regardless? (it uses "-cpu qemu32" but I confess that I do not know exactly what silicon that tries to emulate).

There is a matter of how qemu emulates CPU features. There is no such Intel CPU that supports PAT w/o MTRR. This is why the current code assumes this dependency.

Quoted 5 lines

Quoted 1 line
Perhaps, QEMU does not emulate MTRR?
I will be the 1st to admit that I am not a seasoned qemu user, so I've no idea if the above is true. I still prefer testing on real hardware, even if that comes across as "old school". :)

We can check it with /proc/cpuinfo on a guest.

Quoted 10 lines

Quoted 6 lines
pat_init() is not called when MTRR is disabled. I think this dependency is wrong, and it needs to be fixed.
This issue has been there for a long time, and you have been running essentially as PAT disabled in the past. The commit in question simply detected this issue.
OK, that sounds good -- in that it seems we are finally getting to the bottom of what happened here. Any thoughts on why built-in vs. modular somehow managed to mask the issue?

No idea. I do not think uvesafb being built-in vs. module has anything to do with it. But we need to verify your /proc/cpuinfo to be sure. I will work on the PAT fix once this issue is confirmed.

For the time being, please use "nopat" boot option to workaround this issue. This keeps the PAT state consistent as disabled in your env.

Thanks,
-Toshi

Paul Gortmaker

author

2016-03-10 14:43 UTC #14 lore raw

In reply to Toshi Kani

Quoted 1 line · [Re: runtime regression with "x86/mm/pat: Emulate PAT when it is disabled"] On…

On Mon, 2016-03-07 at 17:56 -0700, Toshi Kani wrote:

[...]

Quoted 1 line

And also an output of /proc/cpuinfo, please?

Here is the output of /proc/cpuinfo in the guest session, while running on pentium dual core as host (no vmx):

processor : 0
vendor_id : GenuineIntel
cpu family : 6
model : 6
model name : QEMU Virtual CPU version 2.5+
stepping : 3
cpu MHz : 2593.449
cache size : 4096 KB
physical id : 0
siblings : 1
core id : 0
cpu cores : 1
apicid : 0
initial apicid : 0
fdiv_bug : no
f00f_bug : no
coma_bug : no
fpu : yes
fpu_exception : yes
cpuid level : 4
wp : yes
flags : fpu de pse tsc msr pae mce cx8 apic sep pge cmov mmx fxsr sse sse2 pni hypervisor
bugs :
bogomips : 5186.89
clflush size : 32
cache_alignment : 32
address sizes : 36 bits physical, 32 bits virtual
power management:

Paul.
--

Quoted 18 lines

I think I know what's going on. I noticed that you have the following message in your dmesg files.
[ 0.000000] MTRR: Disabled
MTRR is set to disabled when your CPU is Intel but does not support MTRR. Perhaps, QEMU does not emulate MTRR?
pat_init() is not called when MTRR is disabled. I think this dependency is wrong, and it needs to be fixed.
This issue has been there for a long time, and you have been running essentially as PAT disabled in the past. The commit in question simply detected this issue.
Thanks,
-Toshi

Toshi Kani

2016-03-10 15:57 UTC #15 lore raw

Quoted 34 lines · Paul Gortmaker

[Re: runtime regression with "x86/mm/pat: Emulate PAT when it is
Quoted 1 line · disabled"] On 07/03/2016 (Mon 18:35) Toshi Kani wrote:
On Mon, 2016-03-07 at 17:56 -0700, Toshi Kani wrote:
[...]
Quoted 1 line
And also an output of /proc/cpuinfo, please?
Here is the output of /proc/cpuinfo in the guest session, while running on pentium dual core as host (no vmx):
processor : 0
vendor_id : GenuineIntel
cpu family : 6
model : 6
model name : QEMU Virtual CPU version 2.5+
stepping : 3
cpu MHz : 2593.449
cache size : 4096 KB
physical id : 0
siblings : 1
core id : 0
cpu cores : 1
apicid : 0
initial apicid : 0
fdiv_bug : no
f00f_bug : no
coma_bug : no
fpu : yes
fpu_exception : yes
cpuid level : 4
wp : yes

Quoted 8 lines

flags : fpu de pse tsc msr pae mce cx8 apic sep pge cmov
mmx fxsr sse sse2 pni hypervisor
bugs :
bogomips : 5186.89
clflush size : 32
cache_alignment : 32
address sizes : 36 bits physical, 32 bits virtual
power management:

This confirms the issue - QEMU's virtual Intel CPU does not support MTRR.

When MTRR is disabled, the kernel does not call pat_init(). pat_enabled() is still set to true when CONFIG_X86_PAT is set. CONFIG_X86_PAT depends on CONFIG_MTRR, and assumes that MTRR is enabled.

Thanks,
-Toshi

Quoted 24 lines

Paul.
--
Quoted 20 lines
I think I know what's going on. I noticed that you have the following message in your dmesg files.
[ 0.000000] MTRR: Disabled
MTRR is set to disabled when your CPU is Intel but does not support MTRR.
Perhaps, QEMU does not emulate MTRR?
pat_init() is not called when MTRR is disabled. I think this dependency is
wrong, and it needs to be fixed.
This issue has been there for a long time, and you have been running essentially as PAT disabled in the past. The commit in question simply detected this issue.
Thanks,
-Toshi

Borislav Petkov

2016-03-10 17:20 UTC #16 lore raw

Quoted 5 lines · Toshi Kani

This confirms the issue - QEMU's virtual Intel CPU does not support MTRR.
When MTRR is disabled, the kernel does not call pat_init(). pat_enabled() is still set to true when CONFIG_X86_PAT is set. CONFIG_X86_PAT depends on CONFIG_MTRR, and assumes that MTRR is enabled.

Aha, so "qemu32" model doesn't support MTRRs but "kvm32" does, for example. And so do the majority of the other CPU types.

Paul, can you guys run with something else besides "qemu32"? You can even take a 64-bit one and run a 32-bit guest on it.

:-)

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Paul Gortmaker

author

2016-03-10 19:04 UTC #17 lore raw

Quoted 9 lines · [Re: runtime regression with "x86/mm/pat: Emulate PAT when it is disabled"] On…

Quoted 5 lines · Toshi Kani
This confirms the issue - QEMU's virtual Intel CPU does not support MTRR.
When MTRR is disabled, the kernel does not call pat_init(). pat_enabled() is still set to true when CONFIG_X86_PAT is set. CONFIG_X86_PAT depends on CONFIG_MTRR, and assumes that MTRR is enabled.
Aha, so "qemu32" model doesn't support MTRRs but "kvm32" does, for example. And so do the majority of the other CPU types.

So, I guess that is a qemu bug? If there is no real silicon out there that has no MTRR but does claim PAT, then qemu32 is a flawed CPU type?

Quoted 3 lines

Paul, can you guys run with something else besides "qemu32"? You can even take a 64-bit one and run a 32-bit guest on it.

That is probably more of an RP question. In principle I guess other CPU types are on the table, and most likely qemu32 is just there from historical reasons. We do know that we don't want "-cpu host" though, since that will introduce variability into the automated testing.

Paul.
--

Quoted 9 lines

:-)

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Toshi Kani

2016-03-10 19:19 UTC #18 lore raw

Quoted 12 lines · Paul Gortmaker

[Re: runtime regression with "x86/mm/pat: Emulate PAT when it is
Quoted 9 lines · disabled"] On 10/03/2016 (Thu 18:20) Borislav Petkov wrote:
Quoted 7 lines · Toshi Kani
This confirms the issue - QEMU's virtual Intel CPU does not support MTRR.
When MTRR is disabled, the kernel does not call pat_init(). pat_enabled() is still set to true when CONFIG_X86_PAT is set. CONFIG_X86_PAT depends on CONFIG_MTRR, and assumes that MTRR is enabled.

Quoted 5 lines

Quoted 2 lines
Aha, so "qemu32" model doesn't support MTRRs but "kvm32" does, for example. And so do the majority of the other CPU types.
So, I guess that is a qemu bug? If there is no real silicon out there that has no MTRR but does claim PAT, then qemu32 is a flawed CPU type?

It turns out that your qemu's virtual CPU does not support PAT, either. :-) So, it is consistent on this regard. I will send patches to address this issue.

Thanks,
-Toshi

Borislav Petkov

2016-03-10 19:19 UTC #19 lore raw

In reply to Paul Gortmaker

Quoted 2 lines · Paul Gortmaker

So, I guess that is a qemu bug? If there is no real silicon out there that has no MTRR but does claim PAT, then qemu32 is a flawed CPU type?

Well, AFAICT, "qemu32" is emulating something PPRO-like:

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

and that one advertizes PAT but not MTRRs.

I need to go dig into history to find out what PPRO actually supported.

Signature · 5 lines

Regards/Gruss,
    Boris.

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

One Thousand Gnomes

2016-03-11 13:24 UTC #20 lore raw

Quoted 6 lines · On Thu, 10 Mar 2016 20:19:33 +0100 Borislav Petkov <[email protected]> wrote:

Quoted 2 lines · Paul Gortmaker
So, I guess that is a qemu bug? If there is no real silicon out there that has no MTRR but does claim PAT, then qemu32 is a flawed CPU type?
Well, AFAICT, "qemu32" is emulating something PPRO-like:

Quoted 8 lines

#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 | \
	  ^^^^^^^^^
and that one advertizes PAT but not MTRRs.
I need to go dig into history to find out what PPRO actually supported.

Pentium Pro has MTRR, PAT came later.

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

Alan

Borislav Petkov

2016-03-11 13:40 UTC #21 lore raw

Quoted 1 line · One Thousand Gnomes

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

Quoted 1 line

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,

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

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Paolo Bonzini

maintainer

2016-03-11 19:18 UTC #22 lore raw

Quoted 10 lines · Borislav Petkov

Quoted 1 line · One Thousand Gnomes
Quoted 1 line
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
Quoted 1 line
Quoted 1 line
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.

Quoted 8 lines

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

Borislav Petkov

2016-03-11 22:16 UTC #23 lore raw

Quoted 5 lines · Paolo Bonzini

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?

Quoted 1 line

I really, really should fix those defaults...

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

---
From: Borislav Petkov <[email protected]>
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 <[email protected]>

---
 target-i386/cpu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

target-i386/cpu.c +3 -3

index 0f38d1eae317..fa7ea4a8c229 100644
@@ -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 | \

Signature · 4 lines

2.3.5

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

Bruce Ashfield

2016-03-11 22:29 UTC #24 lore raw

Quoted 8 lines · Borislav Petkov

Quoted 5 lines · Paolo Bonzini
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

Quoted 39 lines

Quoted 2 lines

I really, really should fix those defaults...

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

---
From: Borislav Petkov <[email protected]>
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 <[email protected]>

---
  target-i386/cpu.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

target-i386/cpu.c +3 -3

index 0f38d1eae317..fa7ea4a8c229 100644
@@ -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 | \

Richard Purdie

2016-03-11 23:29 UTC #25 lore raw

Quoted 18 lines · Bruce Ashfield

Quoted 10 lines · Borislav Petkov
Quoted 7 lines · Paolo Bonzini
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.

Quoted 7 lines

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

Borislav Petkov

2016-03-12 12:03 UTC #26 lore raw

Quoted 2 lines · Richard Purdie

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

Quoted 2 lines

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.

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Toshi Kani

2016-03-10 19:11 UTC #27 lore raw

In reply to Borislav Petkov

Quoted 9 lines · Borislav Petkov

Quoted 7 lines · Toshi Kani
This confirms the issue - QEMU's virtual Intel CPU does not support MTRR.
When MTRR is disabled, the kernel does not call pat_init(). pat_enabled() is still set to true when CONFIG_X86_PAT is set. CONFIG_X86_PAT depends on CONFIG_MTRR, and assumes that MTRR is enabled.

Quoted 8 lines

Aha, so "qemu32" model doesn't support MTRRs but "kvm32" does, for example. And so do the majority of the other CPU types.
Paul, can you guys run with something else besides "qemu32"? You can even take a 64-bit one and run a 32-bit guest on it.
:-)

I will send a patch that sets PAT disabled when MTRR is disabled. This will solve the Paul's issue. His qemu32 model does not support PAT, either.

Ideally, PAT and MTRR features should be managed independently, but this will require much more effort and will not be easily applied to stable.

Thanks,
-Toshi

Borislav Petkov

2016-03-10 19:21 UTC #28 lore raw

Quoted 3 lines · Toshi Kani

I will send a patch that sets PAT disabled when MTRR is disabled. This will solve the Paul's issue. His qemu32 model does not support PAT, either.

It does, see my other mail. We need to figure out first why is pat_init() being called as part of MTRR setup.

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Toshi Kani

2016-03-10 19:31 UTC #29 lore raw

Quoted 7 lines · Borislav Petkov

Quoted 3 lines · Toshi Kani
I will send a patch that sets PAT disabled when MTRR is disabled. This will solve the Paul's issue. His qemu32 model does not support PAT, either.
It does, see my other mail. We need to figure out first why is pat_init() being called as part of MTRR setup.

I am not familiar with PPRO_FEATURES, but shouldn't 'flags' in /proc/cpuinfo show "pat" when X86_FEATURE_PAT is set?

pat_init() is being called as part of MTRR setup because PAT initialization requires the same CPU rendezvous operation implemented in the MTRR code.

Thanks,
-Toshi

Borislav Petkov

2016-03-10 21:08 UTC #30 lore raw

Quoted 1 line · Toshi Kani

I am not familiar with PPRO_FEATURES,

That's the feature bits of the "qemu32" model, and others, in qemu.

Quoted 1 line

but shouldn't 'flags' in /proc/cpuinfo show "pat" when X86_FEATURE_PAT is set?

static void early_init_intel(struct cpuinfo_x86 *c)
...

/*
* There is a known erratum on Pentium III and Core Solo
* and Core Duo CPUs.
* " Page with PAT set to WC while associated MTRR is UC
* may consolidate to UC "
* Because of this erratum, it is better to stick with
* setting WC in MTRR rather than using PAT on these CPUs. *
* Enable PAT WC only on P4, Core 2 or later CPUs. */
if (c->x86 == 6 && c->x86_model < 15)
clear_cpu_cap(c, X86_FEATURE_PAT);
---

which also gives a hint as to how we should fix this: pat_enabled() needs to look at that feature bit too:

---

arch/x86/mm/pat.c +1 -2

index faec01e7a17d..359c30d9a78c 100644
@@ -56,7 +56,7 @@ early_param("nopat", nopat);
 
 bool pat_enabled(void)
 {
-	return !!__pat_enabled;
+	return !!__pat_enabled && static_cpu_has(X86_FEATURE_PAT);
 }
 EXPORT_SYMBOL_GPL(pat_enabled);
---

Makes sense?

Quoted 3 lines

pat_init() is being called as part of MTRR setup because PAT initialization requires the same CPU rendezvous operation implemented in the MTRR code.

... which means, PAT depends on MTRR being present.

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Toshi Kani

2016-03-10 22:25 UTC #31 lore raw

Quoted 25 lines · Borislav Petkov

Quoted 1 line · Toshi Kani
I am not familiar with PPRO_FEATURES,
That's the feature bits of the "qemu32" model, and others, in qemu.
Quoted 2 lines
but shouldn't 'flags' in /proc/cpuinfo show "pat" when X86_FEATURE_PAT is set?
static void early_init_intel(struct cpuinfo_x86 *c)
...
        /*
         * There is a known erratum on Pentium III and Core Solo
         * and Core Duo CPUs.
         * " Page with PAT set to WC while associated MTRR is UC *   may consolidate to UC "
         * Because of this erratum, it is better to stick with
         * setting WC in MTRR rather than using PAT on these CPUs. *
         * Enable PAT WC only on P4, Core 2 or later CPUs. */
        if (c->x86 == 6 && c->x86_model < 15) clear_cpu_cap(c, X86_FEATURE_PAT);
---

Quoted 2 lines

which also gives a hint as to how we should fix this: pat_enabled() needs to look at that feature bit too:

I see. I will take a look.

Quoted 15 lines

---
arch/x86/mm/pat.c +0 -0
index faec01e7a17d..359c30d9a78c 100644
@@ -56,7 +56,7 @@ early_param("nopat", nopat);
bool pat_enabled(void)
{
arch/x86/mm/pat.c +1 -1
-	return !!__pat_enabled;
+	return !!__pat_enabled && static_cpu_has(X86_FEATURE_PAT);
}
EXPORT_SYMBOL_GPL(pat_enabled);
---

Quoted 1 line

Makes sense?

Yes, I agree that pat_enable() needs to check the PAT feature bit. In some reason, static_cpu_has(X86_FEATURE_PAT) returns 0 while cpu_has_pat returns 1 in my testing... I need to check this.

Quoted 5 lines

Quoted 3 lines
pat_init() is being called as part of MTRR setup because PAT initialization requires the same CPU rendezvous operation implemented in the MTRR code.
... which means, PAT depends on MTRR being present.

Yes, and we need more changes to handle this dependency since MTRR does not call pat_init() when it is disabled.

Attached is the changes I am working now. I will include your changes, and send them out once I finished testing. Let me know if you have any suggestion.

Thanks,
-Toshi

---
 arch/x86/include/asm/pat.h |    1 +
 arch/x86/mm/pat.c          |   84 +++++++++++++++++++++++++++-----------------
 2 files changed, 52 insertions(+), 33 deletions(-)

arch/x86/include/asm/pat.h +1 -0

index ca6c228..016142b 100644
@@ -5,6 +5,7 @@
 #include <asm/pgtable_types.h>
 
 bool pat_enabled(void);
+void pat_disable(const char *reason);
 extern void pat_init(void);
 void pat_init_cache_modes(u64);

arch/x86/mm/pat.c +51 ~~-33~~

124 lines not on this page · raw message

---
 arch/x86/kernel/cpu/mtrr/generic.c |   24 ++++++++++++++----------
 arch/x86/kernel/cpu/mtrr/main.c    |   13 ++++++++++++-
 arch/x86/kernel/cpu/mtrr/mtrr.h    |    1 +
 3 files changed, 27 insertions(+), 11 deletions(-)

arch/x86/kernel/cpu/mtrr/generic.c +14 ~~-10~~

index c870af1..136ae86 100644
@@ -444,11 +444,24 @@ static void __init print_mtrr_state(void)
 		pr_debug("TOM2: %016llx aka %lldM\n", mtrr_tom2, mtrr_tom2>>20);
 }
 
+/* PAT setup for BP. We need to go through sync steps here */
+void __init mtrr_bp_pat_init(void)
+{
+	unsigned long flags;
+
+	local_irq_save(flags);
+	prepare_set();
+
+	pat_init();
+
+	post_set();
+	local_irq_restore(flags);
+}
+
 /* Grab all of the MTRR state for this CPU into *state */
 bool __init get_mtrr_state(void)
 {
 	struct mtrr_var_range *vrs;
-	unsigned long flags;
 	unsigned lo, dummy;
 	unsigned int i;

@@ -481,15 +494,6 @@ bool __init get_mtrr_state(void)
 
 	mtrr_state_set = 1;
 
-	/* PAT setup for BP. We need to go through sync steps here */
-	local_irq_save(flags);
-	prepare_set();
-
-	pat_init();
-
-	post_set();
-	local_irq_restore(flags);
-
 	return !!(mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED);
 }

arch/x86/kernel/cpu/mtrr/main.c +12 -1

index 5c3d149..d9e91f1 100644
@@ -752,6 +752,9 @@ void __init mtrr_bp_init(void)
 			/* BIOS may override */
 			__mtrr_enabled = get_mtrr_state();
 
+			if (mtrr_enabled())
+				mtrr_bp_pat_init();
+
 			if (mtrr_cleanup(phys_addr)) {
 				changed_by_mtrr_cleanup = 1;
 				mtrr_if->set_all();

@@ -759,8 +762,16 @@ void __init mtrr_bp_init(void)
 		}
 	}
 
-	if (!mtrr_enabled())
+	if (!mtrr_enabled()) {
 		pr_info("MTRR: Disabled\n");
+
+		/*
+		 * PAT initialization relies on MTRR's rendezvous handler.
+		 * Disable PAT until the handler can initialize both features
+		 * independently.
+		 */
+		pat_disable("PAT disabled by MTRR");
+	}
 }
 
 void mtrr_ap_init(void)

arch/x86/kernel/cpu/mtrr/mtrr.h +1 -0

index 951884d..6c7ced0 100644
@@ -52,6 +52,7 @@ void set_mtrr_prepare_save(struct set_mtrr_context *ctxt);
 void fill_mtrr_var_range(unsigned int index,
 		u32 base_lo, u32 base_hi, u32 mask_lo, u32 mask_hi);
 bool get_mtrr_state(void);
+void mtrr_bp_pat_init(void);
 
 extern void set_mtrr_ops(const struct mtrr_ops *ops);

Paul Gortmaker

author

2016-03-08 03:17 UTC #32 lore raw

In reply to Toshi Kani

[Re: runtime regression with "x86/mm/pat: Emulate PAT when it is disabled"] On 07/03/2016 (Mon 17:56) Toshi Kani wrote:

[..]

Quoted 10 lines

Quoted 10 lines
Quoted 5 lines
It may have seemed working before, but you did not have WC configured to PAT without calling pat_init(). There was not a proper check in place to detect this error before. Can you please check your code to see what caused this skip of pat_init()? If you have a git tree, I can take a look as well.
You already have git copies of what I'm running, since it is vanilla mainline commits. No code changes at this end whatsoever. I did the bisect on vanilla mainline. All I took from yocto was their ".config"

Quoted 7 lines

Quoted 5 lines
To recap, v4.1-rc5-21-g9dac62909451 works, v4.1-rc5-22-g9cd25aac1f44 fails, and v4.5-rc6 also fails. If pat_init() isn't called then this is a bug in current mainline. I'll have a look later myself and see if I can trace out how we expect to get to pat_init() and how that might be skipped inadvertently unless someone beats me to it.
Oh, I see. Can you send me the ".config" file?

I packaged the .config file for every bisect point in the original reproducer tarball. Let me know if you can't find them. It should look like this:

paul@dell760-paul:~/qemu-fail$ ls -al 00-configs/
total 92
drwxrwxr-x 23 paul paul 4096 Mar  3 10:59 .
drwxrwxr-x  5 paul paul 4096 Mar  3 11:01 ..
-rw-rw-r--  1 paul paul    0 Mar  3 10:59 00-all-dot-configs-are-in-here
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.1
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.1-1281-g43224b96af31
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.1-1625-g44d21c3f3a2e
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.1-3251-g4e241557fc1c
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.1-6547-g4570a37169d4
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.1-912-ge75c73ad6447
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.1-rc4-85-gd4688bdc6335
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.1-rc5-21-g9dac62909451
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.1-rc5-22-g9cd25aac1f44
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.1-rc5-23-g7202fdb1b329
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.1-rc5-27-gd838270e2516
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.1-rc5-35-g7ea402d01cb6
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.1-rc6-256-g9dda1658a9bd
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.1-rc6-296-g7ef3d7d58d9d
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.2
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.2-rc1
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.3
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.4
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.4.1
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.4.1-348-g0194c7658611
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.5-rc6
paul@dell760-paul:~/qemu-fail$

Please check if they are present in the reproducer you downloaded. Maybe somehow I uploaded a version without the config files.

Thanks,
Paul.
--

Quoted 3 lines

Thanks,
-Toshi

Toshi Kani

2016-03-08 15:21 UTC #33 lore raw

Quoted 60 lines · Paul Gortmaker

[Re: runtime regression with "x86/mm/pat: Emulate PAT when it is disabled"] On 07/03/2016 (Mon 17:56) Toshi Kani wrote:
[..]
Quoted 14 lines
Quoted 14 lines
Quoted 8 lines
It may have seemed working before, but you did not have WC configured
to PAT without calling pat_init(). There was not a proper check in place to detect this error before. Can you please check your code to
see what caused this skip of pat_init()? If you have a git tree, I can
take a look as well.
You already have git copies of what I'm running, since it is vanilla mainline commits.  No code changes at this end whatsoever.  I did the bisect on vanilla mainline.  All I took from yocto was their ".config"
Quoted 7 lines
Quoted 5 lines
To recap, v4.1-rc5-21-g9dac62909451 works,  v4.1-rc5-22-g9cd25aac1f44 fails, and v4.5-rc6 also fails.  If pat_init() isn't called then this is a bug in current mainline.  I'll have a look later myself and see if I can trace out how we expect to get to pat_init() and how that might be skipped inadvertently unless someone beats me to it.
Oh, I see. Can you send me the ".config" file?
I packaged the .config file for every bisect point in the original reproducer tarball.  Let me know if you can't find them.  It should look like this:
paul@dell760-paul:~/qemu-fail$ ls -al 00-configs/
total 92
drwxrwxr-x 23 paul paul 4096 Mar  3 10:59 .
drwxrwxr-x  5 paul paul 4096 Mar  3 11:01 ..
-rw-rw-r--  1 paul paul    0 Mar  3 10:59 00-all-dot-configs-are-in-here
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.1
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.1-1281-g43224b96af31
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.1-1625-g44d21c3f3a2e
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.1-3251-g4e241557fc1c
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.1-6547-g4570a37169d4
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.1-912-ge75c73ad6447
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.1-rc4-85-gd4688bdc6335
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.1-rc5-21-g9dac62909451
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.1-rc5-22-g9cd25aac1f44
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.1-rc5-23-g7202fdb1b329
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.1-rc5-27-gd838270e2516
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.1-rc5-35-g7ea402d01cb6
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.1-rc6-256-g9dda1658a9bd
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.1-rc6-296-g7ef3d7d58d9d
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.2
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.2-rc1
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.3
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.4
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.4.1
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.4.1-348-g0194c7658611
drwxrwxr-x  2 paul paul 4096 Mar  3 10:59 v4.5-rc6
paul@dell760-paul:~/qemu-fail$ 

Quoted 2 lines

Please check if they are present in the reproducer you downloaded. Maybe somehow I uploaded a version without the config files.

Yes, I have these directories, but I do not see anything under them... Now that I do not think there is anything unique in your .config file, but it'd be still good to check. Please send me .config file for v4.5-rc6 (if you do not have it now, .config for other ver is OK as well).

$ ls -Rl
.:
total 84
-rw-rw-r--. 1 toshi toshi    0 Mar  3 08:59 00-all-dot-configs-are-in-here
drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.1
drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.1-1281-g43224b96af31
drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.1-1625-g44d21c3f3a2e
drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.1-3251-g4e241557fc1c
drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.1-6547-g4570a37169d4
drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.1-912-ge75c73ad6447
drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.1-rc4-85-gd4688bdc6335
drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.1-rc5-21-g9dac62909451
drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.1-rc5-22-g9cd25aac1f44
drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.1-rc5-23-g7202fdb1b329
drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.1-rc5-27-gd838270e2516
drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.1-rc5-35-g7ea402d01cb6
drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.1-rc6-256-g9dda1658a9bd
drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.1-rc6-296-g7ef3d7d58d9d
drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.2
drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.2-rc1
drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.3
drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.4
drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.4.1
drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.4.1-348-g0194c7658611
drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.5-rc6

./v4.1:
total 0

./v4.1-1281-g43224b96af31:
total 0

./v4.1-1625-g44d21c3f3a2e:
total 0

./v4.1-3251-g4e241557fc1c:
total 0
./v4.1-6547-g4570a37169d4:
total 0

./v4.1-912-ge75c73ad6447:
total 0

./v4.1-rc4-85-gd4688bdc6335:
total 0

./v4.1-rc5-21-g9dac62909451:
total 0

./v4.1-rc5-22-g9cd25aac1f44:
total 0

./v4.1-rc5-23-g7202fdb1b329:
total 0

./v4.1-rc5-27-gd838270e2516:
total 0
./v4.1-rc5-35-g7ea402d01cb6:
total 0

./v4.1-rc6-256-g9dda1658a9bd:
total 0

./v4.1-rc6-296-g7ef3d7d58d9d:
total 0

./v4.2:
total 0

./v4.2-rc1:
total 0

./v4.3:
total 0

./v4.4:
total 0
./v4.4.1:
total 0

./v4.4.1-348-g0194c7658611:
total 0

./v4.5-rc6:
total 0

Thanks,
-Toshi

Paul Gortmaker

author

2016-03-08 16:04 UTC #34 lore raw

[Re: runtime regression with "x86/mm/pat: Emulate PAT when it is disabled"] On 08/03/2016 (Tue 09:13) Toshi Kani wrote:

[...]

Quoted 7 lines

Yes, I have these directories, but I do not see anything under them... Now that I do not think there is anything unique in your .config file, but it'd be still good to check. Please send me .config file for v4.5-rc6 (if you do not have it now, .config for other ver is OK as well).
$ ls -Rl

Try "ls -aRl" -- they are _dot_ config files. I did not rename them.

Paul.
--

Quoted 88 lines

.:
total 84
-rw-rw-r--. 1 toshi toshi    0 Mar  3 08:59 00-all-dot-configs-are-in-here drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.1
drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.1-1281-g43224b96af31 drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.1-1625-g44d21c3f3a2e drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.1-3251-g4e241557fc1c drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.1-6547-g4570a37169d4 drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.1-912-ge75c73ad6447 drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.1-rc4-85-gd4688bdc6335 drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.1-rc5-21-g9dac62909451 drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.1-rc5-22-g9cd25aac1f44 drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.1-rc5-23-g7202fdb1b329 drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.1-rc5-27-gd838270e2516 drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.1-rc5-35-g7ea402d01cb6 drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.1-rc6-256-g9dda1658a9bd drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.1-rc6-296-g7ef3d7d58d9d drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.2
drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.2-rc1 drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.3
drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.4
drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.4.1
drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.4.1-348-g0194c7658611 drwxrwxr-x. 2 toshi toshi 4096 Mar  3 08:59 v4.5-rc6
./v4.1:
total 0
./v4.1-1281-g43224b96af31:
total 0
./v4.1-1625-g44d21c3f3a2e:
total 0
./v4.1-3251-g4e241557fc1c:
total 0
./v4.1-6547-g4570a37169d4:
total 0
./v4.1-912-ge75c73ad6447:
total 0
./v4.1-rc4-85-gd4688bdc6335:
total 0
./v4.1-rc5-21-g9dac62909451:
total 0
./v4.1-rc5-22-g9cd25aac1f44:
total 0
./v4.1-rc5-23-g7202fdb1b329:
total 0
./v4.1-rc5-27-gd838270e2516:
total 0
./v4.1-rc5-35-g7ea402d01cb6:
total 0
./v4.1-rc6-256-g9dda1658a9bd:
total 0
./v4.1-rc6-296-g7ef3d7d58d9d:
total 0
./v4.2:
total 0
./v4.2-rc1:
total 0
./v4.3:
total 0
./v4.4:
total 0
./v4.4.1:
total 0
./v4.4.1-348-g0194c7658611:
total 0
./v4.5-rc6:
total 0
Thanks,
-Toshi

Toshi Kani

2016-03-08 16:09 UTC #35 lore raw

Quoted 14 lines · Paul Gortmaker

[Re: runtime regression with "x86/mm/pat: Emulate PAT when it is disabled"] On 08/03/2016 (Tue 09:13) Toshi Kani wrote:
[...]
Quoted 9 lines
Yes, I have these directories, but I do not see anything under them... Now
that I do not think there is anything unique in your .config file, but it'd
be still good to check. Please send me .config file for v4.5-rc6 (if you
do not have it now, .config for other ver is OK as well).

Quoted 3 lines

Quoted 1 line
$ ls -Rl
Try "ls -aRl" -- they are _dot_ config files. I did not rename them.

Ah! Thanks, I see them now. :-)
-Toshi