Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1355595 > unrolled thread
| Started by | Toshi Kani <toshi.kani@hpe.com> |
|---|---|
| First post | 2016-03-11 05:00 +0100 |
| Last post | 2016-03-16 00:30 +0100 |
| Articles | 16 on this page of 36 — 6 participants |
Back to article view | Back to linux.kernel
[PATCH 0/2] Refactor MTRR and PAT initializations Toshi Kani <toshi.kani@hpe.com> - 2016-03-11 05:00 +0100
[PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table Toshi Kani <toshi.kani@hpe.com> - 2016-03-11 05:00 +0100
Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table Borislav Petkov <bp@alien8.de> - 2016-03-11 10:20 +0100
Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table Toshi Kani <toshi.kani@hpe.com> - 2016-03-11 16:40 +0100
Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table Borislav Petkov <bp@alien8.de> - 2016-03-11 17:00 +0100
Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table Toshi Kani <toshi.kani@hpe.com> - 2016-03-11 19:40 +0100
Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table Borislav Petkov <bp@alien8.de> - 2016-03-12 13:00 +0100
Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table Toshi Kani <toshi.kani@hpe.com> - 2016-03-14 21:50 +0100
Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table Borislav Petkov <bp@alien8.de> - 2016-03-15 12:10 +0100
Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table Toshi Kani <toshi.kani@hpe.com> - 2016-03-15 22:10 +0100
Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-03-15 01:30 +0100
Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table Toshi Kani <toshi.kani@hpe.com> - 2016-03-15 03:20 +0100
Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table Borislav Petkov <bp@alien8.de> - 2016-03-15 12:10 +0100
Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table Toshi Kani <toshi.kani@hpe.com> - 2016-03-15 16:00 +0100
Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table Borislav Petkov <bp@alien8.de> - 2016-03-15 16:50 +0100
Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table Toshi Kani <toshi.kani@hpe.com> - 2016-03-15 17:20 +0100
Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table Borislav Petkov <bp@alien8.de> - 2016-03-15 17:40 +0100
Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-03-15 22:40 +0100
[PATCH 2/2] x86/mtrr: Refactor PAT initialization code Toshi Kani <toshi.kani@hpe.com> - 2016-03-11 05:00 +0100
Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Ingo Molnar <mingo@kernel.org> - 2016-03-11 10:10 +0100
Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Ingo Molnar <mingo@kernel.org> - 2016-03-11 10:20 +0100
Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Toshi Kani <toshi.kani@hpe.com> - 2016-03-11 18:50 +0100
Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Ingo Molnar <mingo@kernel.org> - 2016-03-12 17:20 +0100
Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Toshi Kani <toshi.kani@hpe.com> - 2016-03-14 20:00 +0100
Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code "Luis R. Rodriguez" <mcgrof@suse.com> - 2016-03-15 00:00 +0100
Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Toshi Kani <toshi.kani@hpe.com> - 2016-03-15 00:50 +0100
Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Borislav Petkov <bp@suse.de> - 2016-03-15 17:00 +0100
Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Borislav Petkov <bp@alien8.de> - 2016-03-11 10:30 +0100
Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Toshi Kani <toshi.kani@hpe.com> - 2016-03-11 19:10 +0100
Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-03-11 23:20 +0100
Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Toshi Kani <toshi.kani@hpe.com> - 2016-03-12 00:10 +0100
Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-03-12 00:40 +0100
Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Toshi Kani <toshi.kani@hpe.com> - 2016-03-12 01:30 +0100
Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-03-15 01:20 +0100
Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Toshi Kani <toshi.kani@hpe.com> - 2016-03-16 00:00 +0100
Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-03-16 00:30 +0100
Page 2 of 2 — ← Prev page 1 [2]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2016-03-11 10:20 +0100 |
| Subject | Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code |
| Message-ID | <rbroJ-3VO-13@gated-at.bofh.it> |
| In reply to | #1355740 |
* Ingo Molnar <mingo@kernel.org> wrote: > > * Toshi Kani <toshi.kani@hpe.com> wrote: > > > MTRR manages PAT initialization as it implements a rendezvous > > handler that initializes PAT as part of MTRR initialization. > > > > When CPU does not support MTRR, ex. qemu32 virtual CPU, MTRR > > simply skips PAT init, which causes PAT left enabled without > > initialization. [...] > > What practical effects does this have to the user? Does the kernel crash? Btw., I find this omission _highly_ annoying: describing what negative effects a bug _causes in practice_ is the most important part of a changelog. How on earth can an experienced contributor omit such an important component from a patch description? Most readers of changelogs couldn't care less about technical details of how the bug is fixed (of course others will read it so it's nice to have too), but what symptoms a bug causes, how serious is it, whether it should be backported are like super important compared to everything else you wrote - and both the description and the changelogs are totally silent on those topics ... I've seen this in other PAT patches - please try to improve this. Thanks, Ingo
[toc] | [prev] | [next] | [standalone]
| From | Toshi Kani <toshi.kani@hpe.com> |
|---|---|
| Date | 2016-03-11 18:50 +0100 |
| Subject | Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code |
| Message-ID | <rbzmi-1in-11@gated-at.bofh.it> |
| In reply to | #1355750 |
On Fri, 2016-03-11 at 09:13 +0000, Ingo Molnar wrote:
> * Ingo Molnar <mingo@kernel.org> wrote:
>
> >
> > * Toshi Kani <toshi.kani@hpe.com> wrote:
> >
> > > MTRR manages PAT initialization as it implements a rendezvous
> > > handler that initializes PAT as part of MTRR initialization.
> > >
> > > When CPU does not support MTRR, ex. qemu32 virtual CPU, MTRR
> > > simply skips PAT init, which causes PAT left enabled without
> > > initialization. [...]
> >
> > What practical effects does this have to the user? Does the kernel
> > crash?
>
> Btw., I find this omission _highly_ annoying: describing what negative
> effects a bug _causes in practice_ is the most important part of a
> changelog. How on earth can an experienced contributor omit such an
> important component from a patch description?
>
> Most readers of changelogs couldn't care less about technical details of
> how the bug is fixed (of course others will read it so it's nice to have
> too), but what symptoms a bug causes, how serious is it, whether it
> should be backported are like super important compared to everything else
> you wrote - and both the description and the changelogs are totally
> silent on those topics ...
>
> I've seen this in other PAT patches - please try to improve this.
My apology. I agree the importance of describing the negative effect of the
issue. This case is complicated to describe thoroughly, but here is a
summary.
The issue was reported as a regression caused by 'commit 9cd25aac1f44
("x86/mm/pat: Emulate PAT when it is disabled")'. So, the goal of this
patchset is to fix this regression.
https://lkml.org/lkml/2016/3/3/828
The negative effects of the issue were two failures in Xorg on qemu32 env,
which was triggered by the fact that its virtual CPU does not support MTRR.
https://lkml.org/lkml/2016/3/4/775
#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().
These negative effects are also caused by two different bugs, but they can
be dealt in lower priority. Fixing the pat_init() issue will avoid Xorg
hitting these cases.
When the CPU does not support MTRR, MTRR does not call pat_init(), but
leaves PAT enabled. This pat_init() issue is a long-standing issue, but
manifested as issue #1 (and then hit issue #2) with the commit because the
memtype now tracks cache attribute with 'page_cache_mode'. A WC map request
is tracked as WC in memtype, but sets a PTE as UC (pgprot) per
__cachemode2pte_tbl[]. This caused an error in reserve_pfn_range() when it
was called from track_pfn_copy(), which obtained pgprot from a PTE. It
converts pgprot to page_cache_mode, which does not necessarily result in
the original page_cache_mode since __cachemode2pte_tbl[] redirects multiple
types to UC. This is a separate issue in reserve_pfn_range().
If PAT is set to disabled properly, the code bypasses the memtype check.
So, #1 is a non-issue as a result (although it should be fixed).
This pat_init() issue existed before commit 9cd25aac1f44, but we used
pgprot in memtype. Hence, we did not have issue #1 before. But WC request
resulted in WT in effect because WC pgrot is actually WT when PAT is not
initialized. This is not how the code was designed to work. When PAT is set
to disabled properly, WC gets converted to UC. The use of WT can result in
a system crash if the target range does not support WT, but fortunately
people did not run into such issue before.
Thanks,
-Toshi
[toc] | [prev] | [next] | [standalone]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2016-03-12 17:20 +0100 |
| Subject | Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code |
| Message-ID | <rbUqJ-ks-1@gated-at.bofh.it> |
| In reply to | #1356096 |
* Toshi Kani <toshi.kani@hpe.com> wrote:
> On Fri, 2016-03-11 at 09:13 +0000, Ingo Molnar wrote:
> > * Ingo Molnar <mingo@kernel.org> wrote:
> >
> > >
> > > * Toshi Kani <toshi.kani@hpe.com> wrote:
> > >
> > > > MTRR manages PAT initialization as it implements a rendezvous
> > > > handler that initializes PAT as part of MTRR initialization.
> > > >
> > > > When CPU does not support MTRR, ex. qemu32 virtual CPU, MTRR
> > > > simply skips PAT init, which causes PAT left enabled without
> > > > initialization. [...]
> > >
> > > What practical effects does this have to the user? Does the kernel
> > > crash?
> >
> > Btw., I find this omission _highly_ annoying: describing what negative
> > effects a bug _causes in practice_ is the most important part of a
> > changelog. How on earth can an experienced contributor omit such an
> > important component from a patch description?
> >
> > Most readers of changelogs couldn't care less about technical details of
> > how the bug is fixed (of course others will read it so it's nice to have
> > too), but what symptoms a bug causes, how serious is it, whether it
> > should be backported are like super important compared to everything else
> > you wrote - and both the description and the changelogs are totally
> > silent on those topics ...
> >
> > I've seen this in other PAT patches - please try to improve this.
>
> My apology. I agree the importance of describing the negative effect of the
> issue. This case is complicated to describe thoroughly, but here is a
> summary.
The new changelog looks very good, thanks!
> The issue was reported as a regression caused by 'commit 9cd25aac1f44
> ("x86/mm/pat: Emulate PAT when it is disabled")'. So, the goal of this
> patchset is to fix this regression.
> https://lkml.org/lkml/2016/3/3/828
So one thing that matters more than anything else in the changelog, the title!
Right now the title is:
x86/mtrr: Refactor PAT initialization code
... that's a nice title for a true refactoring of the code, but this isn't really
that, the purpose of this fix is to fix a bad Xorg crash for Qemu users.
The principle you need to remember is that readers of your changelogs will be
_very happy_ about 'negative' phrases like:
bad bug
Xorg crash
boot failure
kernel crash
NULL dereference
I.e. the 'best' title for a bug fix is to characterize it in the most negative
truthful fashion in the changelog. It sounds a bit counterintuitive but it's true.
So in this case the best changelog title would be something like:
x86/pat: Fix Xorg crashes in Qemu sessions
People will absolutely _love_ such titles, because:
- users who are trying to find mysterious Xorg failures can grep for it and
might find it before it hits a stable kernel they are using
- maintainers (like me) are able to see it at a glance that this fix should go
to Linus more urgently than other fixes. (and definitely more urgently than
feature patches.)
- stable kernel maintainers and distro backporters can see it immediately at a
glance that they really want this fix.
So by being intentionally and maximally negative in the title, you are being very
helpful to your fellow developers and users!
Now consider the original title:
x86/mtrr: Refactor PAT initialization code
99% of people will glance over such a title, which is not good. Furhermore,
maintainers like me will get _annoyed_ at such titles, because this neutrally
formulated title, while very polite, actively hides the important detail that
these patches fix real negative bugs for real users.
Okay?
And please also note that in the Linux kernel no-one ever 'blames' other people
for bugs. Bugs are part of the human condition and they happen all the time as
long as they are not introduced by carelessness. So in the typical case you cannot
possibly socially embarrass any good kernel developer by reporting and fixing a
bug he introduced. The typical reaction you will get is 'oh great, one bug less to
worry about!', so socially you can be absolutely honest and 'impolite' about the
negative effects of bugs.
> The negative effects of the issue were two failures in Xorg on qemu32 env,
> which was triggered by the fact that its virtual CPU does not support MTRR.
> https://lkml.org/lkml/2016/3/4/775
> #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().
Yeah, it's nice to quote actual crash signatures as well (in a short form) -
because people hitting the crashes often do a google search and might find the fix
based on such patterns.
Thanks!
Ingo
[toc] | [prev] | [next] | [standalone]
| From | Toshi Kani <toshi.kani@hpe.com> |
|---|---|
| Date | 2016-03-14 20:00 +0100 |
| Subject | Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code |
| Message-ID | <rcFSG-7FX-23@gated-at.bofh.it> |
| In reply to | #1356467 |
On Sat, 2016-03-12 at 17:18 +0100, Ingo Molnar wrote:
> * Toshi Kani <toshi.kani@hpe.com> wrote:
>
> > On Fri, 2016-03-11 at 09:13 +0000, Ingo Molnar wrote:
> > > * Ingo Molnar <mingo@kernel.org> wrote:
> > >
> > > >
> > > > * Toshi Kani <toshi.kani@hpe.com> wrote:
> > > >
> > > > > MTRR manages PAT initialization as it implements a rendezvous
> > > > > handler that initializes PAT as part of MTRR initialization.
> > > > >
> > > > > When CPU does not support MTRR, ex. qemu32 virtual CPU, MTRR
> > > > > simply skips PAT init, which causes PAT left enabled without
> > > > > initialization. [...]
> > > >
> > > > What practical effects does this have to the user? Does the kernel
> > > > crash?
> > >
> > > Btw., I find this omission _highly_ annoying: describing what
> > > negative effects a bug _causes in practice_ is the most important
> > > part of a changelog. How on earth can an experienced contributor omit
> > > such an important component from a patch description?
> > >
> > > Most readers of changelogs couldn't care less about technical details
> > > of how the bug is fixed (of course others will read it so it's nice
> > > to have too), but what symptoms a bug causes, how serious is it,
> > > whether it should be backported are like super important compared to
> > > everything else you wrote - and both the description and the
> > > changelogs are totally silent on those topics ...
> > >
> > > I've seen this in other PAT patches - please try to improve this.
> >
> > My apology. I agree the importance of describing the negative effect of
> > the issue. This case is complicated to describe thoroughly, but here is
> > a summary.
>
> The new changelog looks very good, thanks!
>
> > The issue was reported as a regression caused by 'commit 9cd25aac1f44
> > ("x86/mm/pat: Emulate PAT when it is disabled")'. So, the goal of this
> > patchset is to fix this regression.
> > https://lkml.org/lkml/2016/3/3/828
>
> So one thing that matters more than anything else in the changelog, the
> title! Right now the title is:
>
> x86/mtrr: Refactor PAT initialization code
>
> ... that's a nice title for a true refactoring of the code, but this
> isn't really that, the purpose of this fix is to fix a bad Xorg crash for
> Qemu users.
>
> The principle you need to remember is that readers of your changelogs
> will be _very happy_ about 'negative' phrases like:
>
> bad bug
> Xorg crash
> boot failure
> kernel crash
> NULL dereference
>
> I.e. the 'best' title for a bug fix is to characterize it in the most
> negative truthful fashion in the changelog. It sounds a bit
> counterintuitive but it's true.
>
> So in this case the best changelog title would be something like:
>
> x86/pat: Fix Xorg crashes in Qemu sessions
>
> People will absolutely _love_ such titles, because:
>
> - users who are trying to find mysterious Xorg failures can grep for it
> and might find it before it hits a stable kernel they are using
>
> - maintainers (like me) are able to see it at a glance that this fix
> should go to Linus more urgently than other fixes. (and definitely more
> urgently than feature patches.)
>
> - stable kernel maintainers and distro backporters can see it
> immediately at a glance that they really want this fix.
>
> So by being intentionally and maximally negative in the title, you are
> being very helpful to your fellow developers and users!
>
> Now consider the original title:
>
> x86/mtrr: Refactor PAT initialization code
>
> 99% of people will glance over such a title, which is not good.
> Furhermore, maintainers like me will get _annoyed_ at such titles,
> because this neutrally formulated title, while very polite, actively
> hides the important detail that these patches fix real negative bugs for
> real users.
>
> Okay?
Thanks for all the explanation and guidance! That's very helpful. Yes, I
will keep this in mind.
> And please also note that in the Linux kernel no-one ever 'blames' other
> people for bugs. Bugs are part of the human condition and they happen all
> the time as long as they are not introduced by carelessness. So in the
> typical case you cannot possibly socially embarrass any good kernel
> developer by reporting and fixing a bug he introduced. The typical
> reaction you will get is 'oh great, one bug less to worry about!', so
> socially you can be absolutely honest and 'impolite' about the negative
> effects of bugs.
Understood.
> > The negative effects of the issue were two failures in Xorg on qemu32
> > env, which was triggered by the fact that its virtual CPU does not
> > support MTRR.
> > https://lkml.org/lkml/2016/3/4/775
> > #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().
>
> Yeah, it's nice to quote actual crash signatures as well (in a short
> form) - because people hitting the crashes often do a google search and
> might find the fix based on such patterns.
Will do.
Thanks!
-Toshi
[toc] | [prev] | [next] | [standalone]
| From | "Luis R. Rodriguez" <mcgrof@suse.com> |
|---|---|
| Date | 2016-03-15 00:00 +0100 |
| Subject | Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code |
| Message-ID | <rcJCW-1Hd-21@gated-at.bofh.it> |
| In reply to | #1356096 |
On Fri, Mar 11, 2016 at 11:34:26AM -0700, Toshi Kani wrote:
> On Fri, 2016-03-11 at 09:13 +0000, Ingo Molnar wrote:
> > * Ingo Molnar <mingo@kernel.org> wrote:
> >
> > >
> > > * Toshi Kani <toshi.kani@hpe.com> wrote:
> > >
> > > > MTRR manages PAT initialization as it implements a rendezvous
> > > > handler that initializes PAT as part of MTRR initialization.
> > > >
> > > > When CPU does not support MTRR, ex. qemu32 virtual CPU, MTRR
> > > > simply skips PAT init, which causes PAT left enabled without
> > > > initialization. [...]
> > >
> > > What practical effects does this have to the user? Does the kernel
> > > crash?
> >
> > Btw., I find this omission _highly_ annoying: describing what negative
> > effects a bug _causes in practice_ is the most important part of a
> > changelog. How on earth can an experienced contributor omit such an
> > important component from a patch description?
> >
> > Most readers of changelogs couldn't care less about technical details of
> > how the bug is fixed (of course others will read it so it's nice to have
> > too), but what symptoms a bug causes, how serious is it, whether it
> > should be backported are like super important compared to everything else
> > you wrote - and both the description and the changelogs are totally
> > silent on those topics ...
> >
> > I've seen this in other PAT patches - please try to improve this.
>
> My apology. I agree the importance of describing the negative effect of the
> issue. This case is complicated to describe thoroughly, but here is a
> summary.
>
> The issue was reported as a regression caused by 'commit 9cd25aac1f44
> ("x86/mm/pat: Emulate PAT when it is disabled")'. So, the goal of this
> patchset is to fix this regression.
> https://lkml.org/lkml/2016/3/3/828
Huh, interesting.
> The negative effects of the issue were two failures in Xorg on qemu32 env,
> which was triggered by the fact that its virtual CPU does not support MTRR.
> https://lkml.org/lkml/2016/3/4/775
> #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().
>
> These negative effects are also caused by two different bugs, but they can
> be dealt in lower priority. Fixing the pat_init() issue will avoid Xorg
> hitting these cases.
Was it confirmed that these patches fix this issue? Mentioning this in the
commit log would also help. Tested-by, etc.
Joe at Stratus also hit this issue but on a system where MTRR is enabled. He
sent his report only to me as he thought it was caused by the ioremap_wc()
changes and his driver was one that got it. In his case though he modified the
driver significantly, and upon inspection of that code saw how it used a
secondary backup PCI device for failover for a framebuffer device... The changes
to the driver in place are rather complex though and as such it made no sense
to further review unless he moved his changes upstream. It is still worth
noting this issue has been seeing elsehwere, but the root cause is still not
known. The error Joe got is:
x86/PAT: Xorg:37506 map pfn expected mapping type uncached-minus for [mem 0x9f000000-0x9f7fffff], got write-combining
Even though the driver is custom (and actually I even saw another unrelated
proprietary driver loaded) I figured its worth noting others have seen this
error without MTRR being disabled.
The second thread you referred to seems to say that if you built-in the code
the error does not come up. What the hell. Joe, can you try building your
driver built-in to see if you also see this go away? Even though I don't
want to support your custom hacked up driver I do want to know if your
issue goes away with built-in as well.
> When the CPU does not support MTRR, MTRR does not call pat_init(), but
> leaves PAT enabled. This pat_init() issue is a long-standing issue, but
> manifested as issue #1 (and then hit issue #2) with the commit because the
> memtype now tracks cache attribute with 'page_cache_mode'. A WC map request
> is tracked as WC in memtype, but sets a PTE as UC (pgprot) per
> __cachemode2pte_tbl[].
Can you think of anything else other than having MTRR disabled that could
cause this same ending result?
> This caused an error in reserve_pfn_range() when it
> was called from track_pfn_copy(), which obtained pgprot from a PTE. It
> converts pgprot to page_cache_mode, which does not necessarily result in
> the original page_cache_mode since __cachemode2pte_tbl[] redirects multiple
> types to UC. This is a separate issue in reserve_pfn_range().
Do you have a fix in mind for that already too?
> If PAT is set to disabled properly, the code bypasses the memtype check.
> So, #1 is a non-issue as a result (although it should be fixed).
>
> This pat_init() issue existed before commit 9cd25aac1f44, but we used
> pgprot in memtype. Hence, we did not have issue #1 before. But WC request
> resulted in WT in effect because WC pgrot is actually WT when PAT is not
> initialized. This is not how the code was designed to work. When PAT is set
> to disabled properly, WC gets converted to UC. The use of WT can result in
> a system crash if the target range does not support WT, but fortunately
> people did not run into such issue before.
And since the ioremap_wc() crusade ended via commit 2baa891e42d84, and both
were merged on v4.3 it mean that we'd likely see more of these issues now as
more driver are using PAT since v4.3.
Luis
[toc] | [prev] | [next] | [standalone]
| From | Toshi Kani <toshi.kani@hpe.com> |
|---|---|
| Date | 2016-03-15 00:50 +0100 |
| Subject | Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code |
| Message-ID | <rcKpk-2fi-13@gated-at.bofh.it> |
| In reply to | #1357675 |
On Mon, 2016-03-14 at 23:50 +0100, Luis R. Rodriguez wrote: > On Fri, Mar 11, 2016 at 11:34:26AM -0700, Toshi Kani wrote: > > On Fri, 2016-03-11 at 09:13 +0000, Ingo Molnar wrote: > > > * Ingo Molnar <mingo@kernel.org> wrote: : > > The negative effects of the issue were two failures in Xorg on qemu32 > > env, which was triggered by the fact that its virtual CPU does not > > support MTRR. > > https://lkml.org/lkml/2016/3/4/775 > > #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(). > > > > These negative effects are also caused by two different bugs, but they > > can be dealt in lower priority. Fixing the pat_init() issue will avoid > > Xorg hitting these cases. > > Was it confirmed that these patches fix this issue? Mentioning this in > the commit log would also help. Tested-by, etc. I instrumented the kernel to emulate the non-MTRR case, and reproduced the issue. I then tested to confirm that my patch fixed it. Yes, it'd be really nice if Paul can test it as well. > Joe at Stratus also hit this issue but on a system where MTRR is enabled. > He sent his report only to me as he thought it was caused by the > ioremap_wc() changes and his driver was one that got it. In his case > though he modified the driver significantly, and upon inspection of that > code saw how it used a secondary backup PCI device for failover for a > framebuffer device... The changes to the driver in place are rather > complex though and as such it made no sense to further review unless he > moved his changes upstream. It is still worth noting this issue has been > seeing elsehwere, but the root cause is still not known. The error Joe > got is: > > x86/PAT: Xorg:37506 map pfn expected mapping type uncached-minus for [mem > 0x9f000000-0x9f7fffff], got write-combining > > Even though the driver is custom (and actually I even saw another > unrelated proprietary driver loaded) I figured its worth noting others > have seen this error without MTRR being disabled. The error message looks the same. So, this could be the same issue if WC is redirected to UC without disabling PAT properly on his env. I need a whole dmesg output to confirm if this is the case. Another way to hit this error is that the driver called remap_pfn_range() with UC to a range where WC map was set by ioremap_wc() already. > The second thread you referred to seems to say that if you built-in the > code the error does not come up. What the hell. Joe, can you try building > your driver built-in to see if you also see this go away? Even though I > don't want to support your custom hacked up driver I do want to know if > your issue goes away with built-in as well. I do not have sufficient info to support this case, and do not have technical explanation for it, either. > > When the CPU does not support MTRR, MTRR does not call pat_init(), but > > leaves PAT enabled. This pat_init() issue is a long-standing issue, but > > manifested as issue #1 (and then hit issue #2) with the commit because > > the memtype now tracks cache attribute with 'page_cache_mode'. A WC map > > request is tracked as WC in memtype, but sets a PTE as UC (pgprot) per > > __cachemode2pte_tbl[]. > > Can you think of anything else other than having MTRR disabled that could > cause this same ending result? See above. > > This caused an error in reserve_pfn_range() when it > > was called from track_pfn_copy(), which obtained pgprot from a PTE. It > > converts pgprot to page_cache_mode, which does not necessarily result > > in the original page_cache_mode since __cachemode2pte_tbl[] redirects > > multiple types to UC. This is a separate issue in reserve_pfn_range(). > > Do you have a fix in mind for that already too? Yes, we can compare with pgprot values as it was the case before. > > If PAT is set to disabled properly, the code bypasses the memtype > > check. So, #1 is a non-issue as a result (although it should be fixed). > > > > This pat_init() issue existed before commit 9cd25aac1f44, but we used > > pgprot in memtype. Hence, we did not have issue #1 before. But WC > > request resulted in WT in effect because WC pgrot is actually WT when > > PAT is not initialized. This is not how the code was designed to work. > > When PAT is set to disabled properly, WC gets converted to UC. The use > > of WT can result in a system crash if the target range does not support > > WT, but fortunately people did not run into such issue before. > > And since the ioremap_wc() crusade ended via commit 2baa891e42d84, and > both were merged on v4.3 it mean that we'd likely see more of these > issues now as more driver are using PAT since v4.3. Uh-oh... Thanks, -Toshi
[toc] | [prev] | [next] | [standalone]
| From | Borislav Petkov <bp@suse.de> |
|---|---|
| Date | 2016-03-15 17:00 +0100 |
| Subject | Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code |
| Message-ID | <rcZy2-429-17@gated-at.bofh.it> |
| In reply to | #1357698 |
On Mon, Mar 14, 2016 at 06:37:20PM -0600, Toshi Kani wrote:
> Yes, it'd be really nice if Paul can test it as well.
Let's please agree on the final design of the patchset first and then
ask bug reporters to test.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
[toc] | [prev] | [next] | [standalone]
| From | Borislav Petkov <bp@alien8.de> |
|---|---|
| Date | 2016-03-11 10:30 +0100 |
| Subject | Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code |
| Message-ID | <rbryq-3Zo-19@gated-at.bofh.it> |
| In reply to | #1355599 |
On Thu, Mar 10, 2016 at 09:45:46PM -0700, Toshi Kani wrote:
> MTRR manages PAT initialization as it implements a rendezvous
> handler that initializes PAT as part of MTRR initialization.
>
> When CPU does not support MTRR, ex. qemu32 virtual CPU, MTRR
> simply skips PAT init, which causes PAT left enabled without
> initialization. Also, get_mtrr_state() calls pat_init() on
> BSP even if MTRR is disabled by its MSR. This causes pat_init()
> be called on BSP only.
So I don't understand what all this hoopla is all about: why can't you
simply call pat_disable() in mtrr_ap_init() and be done with it?
void mtrr_ap_init(void)
{
if (!mtrr_enabled()) {
pat_disable();
return;
}
?
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
[toc] | [prev] | [next] | [standalone]
| From | Toshi Kani <toshi.kani@hpe.com> |
|---|---|
| Date | 2016-03-11 19:10 +0100 |
| Subject | Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code |
| Message-ID | <rbzFF-1Ep-19@gated-at.bofh.it> |
| In reply to | #1355762 |
On Fri, 2016-03-11 at 10:24 +0100, Borislav Petkov wrote:
> On Thu, Mar 10, 2016 at 09:45:46PM -0700, Toshi Kani wrote:
> > MTRR manages PAT initialization as it implements a rendezvous
> > handler that initializes PAT as part of MTRR initialization.
> >
> > When CPU does not support MTRR, ex. qemu32 virtual CPU, MTRR
> > simply skips PAT init, which causes PAT left enabled without
> > initialization. Also, get_mtrr_state() calls pat_init() on
> > BSP even if MTRR is disabled by its MSR. This causes pat_init()
> > be called on BSP only.
>
> So I don't understand what all this hoopla is all about: why can't you
> simply call pat_disable() in mtrr_ap_init() and be done with it?
>
> void mtrr_ap_init(void)
> {
> if (!mtrr_enabled()) {
> pat_disable();
> return;
> }
>
> ?
No, it does not fix it. The problem in this particular case, i.e. MTRR
disabled by its MSR, is that mtrr_bp_init() calls pat_init() (as PAT
enabled) and initializes PAT on BSP. After APs are launched, we need the
MTRR's rendezvous handler to initialize PAT on APs to be consistent with
BSP. However, MTRR rendezvous handler is no-op since MTRR is disabled.
Hence, we cannot let mtrr_bp_init() to call pat_init() when MTRR is
disabled.
Thanks,
-Toshi
[toc] | [prev] | [next] | [standalone]
| From | "Luis R. Rodriguez" <mcgrof@kernel.org> |
|---|---|
| Date | 2016-03-11 23:20 +0100 |
| Subject | Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code |
| Message-ID | <rbDzA-4vT-15@gated-at.bofh.it> |
| In reply to | #1356110 |
On Fri, Mar 11, 2016 at 11:57:12AM -0700, Toshi Kani wrote:
> On Fri, 2016-03-11 at 10:24 +0100, Borislav Petkov wrote:
> > On Thu, Mar 10, 2016 at 09:45:46PM -0700, Toshi Kani wrote:
> > > MTRR manages PAT initialization as it implements a rendezvous
> > > handler that initializes PAT as part of MTRR initialization.
> > >
> > > When CPU does not support MTRR, ex. qemu32 virtual CPU, MTRR
> > > simply skips PAT init, which causes PAT left enabled without
> > > initialization. Also, get_mtrr_state() calls pat_init() on
> > > BSP even if MTRR is disabled by its MSR. This causes pat_init()
> > > be called on BSP only.
> >
> > So I don't understand what all this hoopla is all about: why can't you
> > simply call pat_disable() in mtrr_ap_init() and be done with it?
> >
> > void mtrr_ap_init(void)
> > {
> > if (!mtrr_enabled()) {
> > pat_disable();
> > return;
> > }
> >
> > ?
>
> No, it does not fix it. The problem in this particular case, i.e. MTRR
> disabled by its MSR, is that mtrr_bp_init() calls pat_init() (as PAT
> enabled) and initializes PAT on BSP. After APs are launched, we need the
> MTRR's rendezvous handler to initialize PAT on APs to be consistent with
> BSP. However, MTRR rendezvous handler is no-op since MTRR is disabled.
This seems like a hack on enabling PAT through MTRR code, can we have
a PAT rendezvous handler on its own, or provide a generic rendezvous
handler that lets you deal with whatever interfaces need setup. Then
conflicts can just be negotiated early.
What I'm after is seeing if we can ultimately disable MTRR on kernel
code but still have PAT enabled. I realize you've mentioned BIOS code
may use some MTRR setup code but this is only true for some systems.
I know for a fact Xen cannot use MTRR, it seems qemu32 does not enable
it either. So why not have the ability to skip through its set up ?
I'll also note Xen managed to enable PAT only without enabling MTRR,
this was done through pat_init_cache_modes() -- not sure if this can
be leveraged for qemu32...
Luis
[toc] | [prev] | [next] | [standalone]
| From | Toshi Kani <toshi.kani@hpe.com> |
|---|---|
| Date | 2016-03-12 00:10 +0100 |
| Subject | Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code |
| Message-ID | <rbElX-54U-1@gated-at.bofh.it> |
| In reply to | #1356232 |
On Fri, 2016-03-11 at 23:17 +0100, Luis R. Rodriguez wrote:
> On Fri, Mar 11, 2016 at 11:57:12AM -0700, Toshi Kani wrote:
> > On Fri, 2016-03-11 at 10:24 +0100, Borislav Petkov wrote:
> > > On Thu, Mar 10, 2016 at 09:45:46PM -0700, Toshi Kani wrote:
> > > > MTRR manages PAT initialization as it implements a rendezvous
> > > > handler that initializes PAT as part of MTRR initialization.
> > > >
> > > > When CPU does not support MTRR, ex. qemu32 virtual CPU, MTRR
> > > > simply skips PAT init, which causes PAT left enabled without
> > > > initialization. Also, get_mtrr_state() calls pat_init() on
> > > > BSP even if MTRR is disabled by its MSR. This causes pat_init()
> > > > be called on BSP only.
> > >
> > > So I don't understand what all this hoopla is all about: why can't
> > > you
> > > simply call pat_disable() in mtrr_ap_init() and be done with it?
> > >
> > > void mtrr_ap_init(void)
> > > {
> > > if (!mtrr_enabled()) {
> > > pat_disable();
> > > return;
> > > }
> > >
> > > ?
> >
> > No, it does not fix it. The problem in this particular case, i.e. MTRR
> > disabled by its MSR, is that mtrr_bp_init() calls pat_init() (as PAT
> > enabled) and initializes PAT on BSP. After APs are launched, we need
> > the MTRR's rendezvous handler to initialize PAT on APs to be consistent
> > with BSP. However, MTRR rendezvous handler is no-op since MTRR is
> > disabled.
>
> This seems like a hack on enabling PAT through MTRR code, can we have
> a PAT rendezvous handler on its own, or provide a generic rendezvous
> handler that lets you deal with whatever interfaces need setup. Then
> conflicts can just be negotiated early.
The MTRR code can be enhanced so that the rendezvous handler can handle
MTRR and PAT state independently. I noted this case as (*) in the table of
this patch description. This is a separate item, however.
MTRR calling PAT was not a hack (as I suppose we did not have VMs at that
time), although this can surely be improved. As Intel SDM state below,
both MTRR and PAT require the same procedure, and the PAT initialization
sequence is defined in the MTRR section.
===
11.12.4 Programming the PAT
:
The operating system is responsible for insuring that changes to a PAT
entry occur in a manner that maintains the consistency of the processor
caches and translation lookaside buffers (TLB). This is accomplished by
following the procedure as specified in Section 11.11.8, “MTRR
Considerations in MP Systems,” for changing the value of an MTRR in a
multiple processor system. It requires a specific sequence of operations
that includes flushing the processors caches and TLBs.
===
> What I'm after is seeing if we can ultimately disable MTRR on kernel
> code but still have PAT enabled. I realize you've mentioned BIOS code
> may use some MTRR setup code but this is only true for some systems.
> I know for a fact Xen cannot use MTRR, it seems qemu32 does not enable
> it either. So why not have the ability to skip through its set up ?
MTRR support has two meanings:
1) The kernel keeps the MTRR setup by BIOS.
2) The kernel modifies the MTRR setup.
I am in a position that we need 1) but 2). In fact, the kernel disabling
MTRRs is the same as 2).
> I'll also note Xen managed to enable PAT only without enabling MTRR,
> this was done through pat_init_cache_modes() -- not sure if this can
> be leveraged for qemu32...
I am interested to know how Xen managed this. Is this done by the Xen
hypervisor initializes guest's PAT on behalf of the guest kernel?
Thanks,
-Toshi
[toc] | [prev] | [next] | [standalone]
| From | "Luis R. Rodriguez" <mcgrof@kernel.org> |
|---|---|
| Date | 2016-03-12 00:40 +0100 |
| Subject | Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code |
| Message-ID | <rbEOZ-5kn-1@gated-at.bofh.it> |
| In reply to | #1356254 |
On Fri, Mar 11, 2016 at 3:56 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
> On Fri, 2016-03-11 at 23:17 +0100, Luis R. Rodriguez wrote:
>> On Fri, Mar 11, 2016 at 11:57:12AM -0700, Toshi Kani wrote:
>> > On Fri, 2016-03-11 at 10:24 +0100, Borislav Petkov wrote:
>> > > On Thu, Mar 10, 2016 at 09:45:46PM -0700, Toshi Kani wrote:
>> > > > MTRR manages PAT initialization as it implements a rendezvous
>> > > > handler that initializes PAT as part of MTRR initialization.
>> > > >
>> > > > When CPU does not support MTRR, ex. qemu32 virtual CPU, MTRR
>> > > > simply skips PAT init, which causes PAT left enabled without
>> > > > initialization. Also, get_mtrr_state() calls pat_init() on
>> > > > BSP even if MTRR is disabled by its MSR. This causes pat_init()
>> > > > be called on BSP only.
>> > >
>> > > So I don't understand what all this hoopla is all about: why can't
>> > > you
>> > > simply call pat_disable() in mtrr_ap_init() and be done with it?
>> > >
>> > > void mtrr_ap_init(void)
>> > > {
>> > > if (!mtrr_enabled()) {
>> > > pat_disable();
>> > > return;
>> > > }
>> > >
>> > > ?
>> >
>> > No, it does not fix it. The problem in this particular case, i.e. MTRR
>> > disabled by its MSR, is that mtrr_bp_init() calls pat_init() (as PAT
>> > enabled) and initializes PAT on BSP. After APs are launched, we need
>> > the MTRR's rendezvous handler to initialize PAT on APs to be consistent
>> > with BSP. However, MTRR rendezvous handler is no-op since MTRR is
>> > disabled.
>>
>> This seems like a hack on enabling PAT through MTRR code, can we have
>> a PAT rendezvous handler on its own, or provide a generic rendezvous
>> handler that lets you deal with whatever interfaces need setup. Then
>> conflicts can just be negotiated early.
>
> The MTRR code can be enhanced so that the rendezvous handler can handle
> MTRR and PAT state independently. I noted this case as (*) in the table of
> this patch description. This is a separate item, however.
>
> MTRR calling PAT was not a hack (as I suppose we did not have VMs at that
> time), although this can surely be improved. As Intel SDM state below,
> both MTRR and PAT require the same procedure, and the PAT initialization
> sequence is defined in the MTRR section.
>
> ===
> 11.12.4 Programming the PAT
> :
> The operating system is responsible for insuring that changes to a PAT
> entry occur in a manner that maintains the consistency of the processor
> caches and translation lookaside buffers (TLB). This is accomplished by
> following the procedure as specified in Section 11.11.8, “MTRR
> Considerations in MP Systems,” for changing the value of an MTRR in a
> multiple processor system. It requires a specific sequence of operations
> that includes flushing the processors caches and TLBs.
> ===
>
>> What I'm after is seeing if we can ultimately disable MTRR on kernel
>> code but still have PAT enabled. I realize you've mentioned BIOS code
>> may use some MTRR setup code but this is only true for some systems.
>> I know for a fact Xen cannot use MTRR, it seems qemu32 does not enable
>> it either. So why not have the ability to skip through its set up ?
>
> MTRR support has two meanings:
> 1) The kernel keeps the MTRR setup by BIOS.
> 2) The kernel modifies the MTRR setup.
>
> I am in a position that we need 1) but 2).
I take it you meant "but not 2)" ? There *are folks however who do
more as I noted earlier. Perhaps now now, but in the future I'd
encourage folks to rip MTRR out of their own BIOS, and enable a new
ACPI legacy flag to say "MTRR required". That'd eventually can help
bury MTRR for good while remaining backward compatible.
I can read the above description to also say:
"Hey you need to implement PAT with the same skeleton code as MTRR"
If we do that, we can pave the way to deprecate MTRR as legacy for
good first on Linux.
> In fact, the kernel disabling MTRRs is the same as 2).
>
>> I'll also note Xen managed to enable PAT only without enabling MTRR,
>> this was done through pat_init_cache_modes() -- not sure if this can
>> be leveraged for qemu32...
>
> I am interested to know how Xen managed this. Is this done by the Xen
> hypervisor initializes guest's PAT on behalf of the guest kernel?
Yup. And the cache read thingy was reading back its own setup, which
was different than what Linux used by default IIRC. Juergen can
elaborate more.
Luis
[toc] | [prev] | [next] | [standalone]
| From | Toshi Kani <toshi.kani@hpe.com> |
|---|---|
| Date | 2016-03-12 01:30 +0100 |
| Subject | Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code |
| Message-ID | <rbFBo-5YA-3@gated-at.bofh.it> |
| In reply to | #1356285 |
On Fri, 2016-03-11 at 15:34 -0800, Luis R. Rodriguez wrote: > On Fri, Mar 11, 2016 at 3:56 PM, Toshi Kani <toshi.kani@hpe.com> wrote: > > On Fri, 2016-03-11 at 23:17 +0100, Luis R. Rodriguez wrote: > > > On Fri, Mar 11, 2016 at 11:57:12AM -0700, Toshi Kani wrote: > > > > On Fri, 2016-03-11 at 10:24 +0100, Borislav Petkov wrote: > > > > > On Thu, Mar 10, 2016 at 09:45:46PM -0700, Toshi Kani wrote: > > > > > > MTRR manages PAT initialization as it implements a rendezvous > > > > > > handler that initializes PAT as part of MTRR initialization. : > > > > > > > > No, it does not fix it. The problem in this particular case, i.e. > > > > MTRR disabled by its MSR, is that mtrr_bp_init() calls pat_init() > > > > (as PAT enabled) and initializes PAT on BSP. After APs are > > > > launched, we need the MTRR's rendezvous handler to initialize PAT > > > > on APs to be consistent with BSP. However, MTRR rendezvous handler > > > > is no-op since MTRR is disabled. > > > > > > This seems like a hack on enabling PAT through MTRR code, can we have > > > a PAT rendezvous handler on its own, or provide a generic rendezvous > > > handler that lets you deal with whatever interfaces need setup. Then > > > conflicts can just be negotiated early. > > > > The MTRR code can be enhanced so that the rendezvous handler can handle > > MTRR and PAT state independently. I noted this case as (*) in the > > table of this patch description. This is a separate item, however. > > > > MTRR calling PAT was not a hack (as I suppose we did not have VMs at > > that time), although this can surely be improved. As Intel SDM state > > below, both MTRR and PAT require the same procedure, and the PAT > > initialization sequence is defined in the MTRR section. > > > > === > > 11.12.4 Programming the PAT > > : > > The operating system is responsible for insuring that changes to a PAT > > entry occur in a manner that maintains the consistency of the processor > > caches and translation lookaside buffers (TLB). This is accomplished by > > following the procedure as specified in Section 11.11.8, “MTRR > > Considerations in MP Systems,” for changing the value of an MTRR in a > > multiple processor system. It requires a specific sequence of > > operations that includes flushing the processors caches and TLBs. > > === > > > > > What I'm after is seeing if we can ultimately disable MTRR on kernel > > > code but still have PAT enabled. I realize you've mentioned BIOS code > > > may use some MTRR setup code but this is only true for some systems. > > > I know for a fact Xen cannot use MTRR, it seems qemu32 does not > > > enable > > > it either. So why not have the ability to skip through its set up ? > > > > MTRR support has two meanings: > > 1) The kernel keeps the MTRR setup by BIOS. > > 2) The kernel modifies the MTRR setup. > > > > I am in a position that we need 1) but 2). > > I take it you meant "but not 2)" ? Yes. :) > There *are folks however who do > more as I noted earlier. Perhaps now now, but in the future I'd > encourage folks to rip MTRR out of their own BIOS, and enable a new > ACPI legacy flag to say "MTRR required". That'd eventually can help > bury MTRR for good while remaining backward compatible. Well, BIOS using MTRR is better than BIOS setting page tables in the SMI handler. The kernel can be ignorant of the MTRR setup as long as it does not modify it. > I can read the above description to also say: > > "Hey you need to implement PAT with the same skeleton code as MTRR" No, I did not say that. MTRR's rendezvous handler can be generalized to work with both MTRR and PAT. We do not need two separate handlers. In fact, it needs to be a single handler so that both can be initialized together. > If we do that, we can pave the way to deprecate MTRR as legacy for > good first on Linux. I do not think such change will deprecate MTRR. It just means that Linux can enable PAT on virtual CPUs with PAT & !MTRR capability. > > In fact, the kernel disabling MTRRs is the same as 2). > > > > > I'll also note Xen managed to enable PAT only without enabling MTRR, > > > this was done through pat_init_cache_modes() -- not sure if this can > > > be leveraged for qemu32... > > > > I am interested to know how Xen managed this. Is this done by the Xen > > hypervisor initializes guest's PAT on behalf of the guest kernel? > > Yup. And the cache read thingy was reading back its own setup, which > was different than what Linux used by default IIRC. Juergen can > elaborate more. Yeah, I'd like to make sure that my changes won't break it. Thanks, -Toshi
[toc] | [prev] | [next] | [standalone]
| From | "Luis R. Rodriguez" <mcgrof@kernel.org> |
|---|---|
| Date | 2016-03-15 01:20 +0100 |
| Subject | Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code |
| Message-ID | <rcKSm-2ES-7@gated-at.bofh.it> |
| In reply to | #1356300 |
On Fri, Mar 11, 2016 at 06:16:36PM -0700, Toshi Kani wrote:
> On Fri, 2016-03-11 at 15:34 -0800, Luis R. Rodriguez wrote:
> > On Fri, Mar 11, 2016 at 3:56 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
> > > On Fri, 2016-03-11 at 23:17 +0100, Luis R. Rodriguez wrote:
> > > > On Fri, Mar 11, 2016 at 11:57:12AM -0700, Toshi Kani wrote:
> > > > > On Fri, 2016-03-11 at 10:24 +0100, Borislav Petkov wrote:
> > > > > > On Thu, Mar 10, 2016 at 09:45:46PM -0700, Toshi Kani wrote:
> > > > > > > MTRR manages PAT initialization as it implements a rendezvous
> > > > > > > handler that initializes PAT as part of MTRR initialization.
> :
> > > > >
> > > > > No, it does not fix it. The problem in this particular case, i.e.
> > > > > MTRR disabled by its MSR, is that mtrr_bp_init() calls pat_init()
> > > > > (as PAT enabled) and initializes PAT on BSP. After APs are
> > > > > launched, we need the MTRR's rendezvous handler to initialize PAT
> > > > > on APs to be consistent with BSP. However, MTRR rendezvous handler
> > > > > is no-op since MTRR is disabled.
> > > >
> > > > This seems like a hack on enabling PAT through MTRR code, can we have
> > > > a PAT rendezvous handler on its own, or provide a generic rendezvous
> > > > handler that lets you deal with whatever interfaces need setup. Then
> > > > conflicts can just be negotiated early.
> > >
> > > The MTRR code can be enhanced so that the rendezvous handler can handle
> > > MTRR and PAT state independently. I noted this case as (*) in the
> > > table of this patch description. This is a separate item, however.
> > >
> > > MTRR calling PAT was not a hack (as I suppose we did not have VMs at
> > > that time), although this can surely be improved. As Intel SDM state
> > > below, both MTRR and PAT require the same procedure, and the PAT
> > > initialization sequence is defined in the MTRR section.
> > >
> > > ===
> > > 11.12.4 Programming the PAT
> > > :
> > > The operating system is responsible for insuring that changes to a PAT
> > > entry occur in a manner that maintains the consistency of the processor
> > > caches and translation lookaside buffers (TLB). This is accomplished by
> > > following the procedure as specified in Section 11.11.8, “MTRR
> > > Considerations in MP Systems,” for changing the value of an MTRR in a
> > > multiple processor system. It requires a specific sequence of
> > > operations that includes flushing the processors caches and TLBs.
> > > ===
> > >
> > > > What I'm after is seeing if we can ultimately disable MTRR on kernel
> > > > code but still have PAT enabled. I realize you've mentioned BIOS code
> > > > may use some MTRR setup code but this is only true for some systems.
> > > > I know for a fact Xen cannot use MTRR, it seems qemu32 does not
> > > > enable
> > > > it either. So why not have the ability to skip through its set up ?
> > >
> > > MTRR support has two meanings:
> > > 1) The kernel keeps the MTRR setup by BIOS.
> > > 2) The kernel modifies the MTRR setup.
> > >
> > > I am in a position that we need 1) but 2).
> >
> > I take it you meant "but not 2)" ?
>
> Yes. :)
OK -- we are in agreement but we know 1) is only needed for a portion of
systems: Xen and qemu32 systems fly with no MTRR set up, and as such it
would be incorrect to run MTRR code on such systems. To these systems
MTRR functionality code should be dead, since PAT currently depends on
MTRR PAT should also be dead but as the report you're fixing shows
it wasn't. That's an issue for qemu that uses the regular x86 init path
but not for Xen. Its different for Xen as the hypervisor is the one that
set up the MSR_IA32_CR_PAT for each CPU. The *only* thing Xen does is:
void xen_start_kernel(void)
{
...
rdmsrl(MSR_IA32_CR_PAT, pat);
pat_init_cache_modes(pat);
...
}
Fortunately we only have to call pat_init_cache_modes() once, its
not per-CPU. Xen has shown then that you *can* live with PAT without
any of the complex MTRR setup / code. Please add to your table the
Xen case as well then as its important to consider. If you make it
a strong requirement to have MTRR enabled to enable PAT you'd
be disabling PAT on Xen guest boots.
As-is then your this patch which calls pat_disable() on mtrr_bp_init() for the
case where MTRR is disabled would essentially break PAT on Xen guests, so this
cannot be done. It is no longer true that if MTRR is disabled you can force
disable PAT. To do what you want you want to do we have to consider Xen.
I don't think its a good idea to keep PAT initialization meshed together
with MTRR and making it a strong requirement on enabling PAT. The MTRR
code is extremely complex. I'd like instead to encourage for us to
consider for this situation to let PAT become a first class citizen,
if MTRR is disabled but you've enabled PAT you should be able to use
it, just as Xen does. There are more reasons to enable such setup than
not to. Long term I'm advocating to see if we can get an ACPI legacy
MTRR flag that can tell us if the BIOS has MTRR ripped out, then we
can at run time also take advantage of ignoring PAT completely as well.
Note, if you insisted you didn't want to disable PAT on Xen, you could
in theory check for the subarch -- but note that the subarch is unused
yet on Xen, even though it was added to the x86 boot protocol years ago.
I have a slew of patches to make use of it to help put paravirt_enabled()
in the grave, but based on discussions with Ingo, we don't want to spread
use of the subarch in random x86 code paths, we want to compartamentalize
that. If you still want to follow your approach of just force-disabling
PAT on MTRR code if MTRR was disabled you'd have to use semantics to
figure out if the boot path came from Xen, to be more specific for Xen
PV guest types only... The current agreed approach to avoid directly
using subarch is to categorize differences between what some guests
need and bare metal under an x86 platform quirk and legacy set of components.
On the x86 init path we'd call something check for the subarch and based
on that set a series of x86 legacy features / quirks that need to be
disabled / enabled. We could add MTRR as one. I'm unifying some of this
with a bit of what goes into the ACPI IA-PC boot architecture, see
section 5.2.9.3 IA-PC Boot Architecture Flags [0]. In particular the
paravirt_enabled() series I'm working on happens to also dabble into
the no CMOS RTC case for Xen, generalizing this knocks a bit of birds
with one stone. I think we can do the same with MTRR but also be
proactive and see if we can get ACPI_FADT_NO_MTRR added as well for
a future ACPI spec to enable BIOS manufacturers to rip MTRR out.
[0] http://www.acpi.info/DOWNLOADS/ACPIspec50.pdf
/* Masks for FADT IA-PC Boot Architecture Flags (boot_flags) [Vx]=Introduced in this FADT revision */
#define ACPI_FADT_LEGACY_DEVICES (1) /* 00: [V2] System has LPC or ISA bus devices */
#define ACPI_FADT_8042 (1<<1) /* 01: [V3] System has an 8042 controller on port 60/64 */
#define ACPI_FADT_NO_VGA (1<<2) /* 02: [V4] It is not safe to probe for VGA hardware */
#define ACPI_FADT_NO_MSI (1<<3) /* 03: [V4] Message Signaled Interrupts (MSI) must not be enabled */
#define ACPI_FADT_NO_ASPM (1<<4) /* 04: [V4] PCIe ASPM control must not be enabled */
#define ACPI_FADT_NO_CMOS_RTC (1<<5) /* 05: [V5] No CMOS real-time clock present */
> > There *are folks however who do
> > more as I noted earlier. Perhaps not now, but in the future I'd
> > encourage folks to rip MTRR out of their own BIOS, and enable a new
> > ACPI legacy flag to say "MTRR required". That'd eventually can help
> > bury MTRR for good while remaining backward compatible.
>
> Well, BIOS using MTRR is better than BIOS setting page tables in the SMI
> handler.
Can some BIOSes be developed without MTRR? For instance I suspect Google might
be able to easily pull of ripping MTRR out of their BIOS if they didn't do it
already for the ChromeOS devices. If possible not only should it help with removing
complexity on the BIOS but not even having to think about that code *ever* running
on the kernel at all should be nice.
> The kernel can be ignorant of the MTRR setup as long as it does
> not modify it.
Sure, we're already there. The kernel no longer modifies the
MTRR setup unless of course you boot without PAT enabled. I think
we need to move beyond that to ACPI if we can to let regular
Linux boots never have to deal with MTRR at all. The code is
complex and nasty why not put let folks put a nail on the coffin for good?
> > I can read the above description to also say:
> >
> > "Hey you need to implement PAT with the same skeleton code as MTRR"
>
> No, I did not say that. MTRR's rendezvous handler can be generalized to
> work with both MTRR and PAT. We do not need two separate handlers. In
> fact, it needs to be a single handler so that both can be initialized
> together.
I'm not sure if that's really needed. Doesn't PAT just require setting
the wrmsrl(MSR_IA32_CR_PAT, pat) for each AP?
> > If we do that, we can pave the way to deprecate MTRR as legacy for
> > good first on Linux.
>
> I do not think such change will deprecate MTRR.
Not even for shiny new BIOSes? Post ACPI 5?
> It just means that Linux can enable PAT on virtual CPUs with PAT & !MTRR capability.
>
> > > In fact, the kernel disabling MTRRs is the same as 2).
> > >
> > > > I'll also note Xen managed to enable PAT only without enabling MTRR,
> > > > this was done through pat_init_cache_modes() -- not sure if this can
> > > > be leveraged for qemu32...
> > >
> > > I am interested to know how Xen managed this. Is this done by the Xen
> > > hypervisor initializes guest's PAT on behalf of the guest kernel?
> >
> > Yup. And the cache read thingy was reading back its own setup, which
> > was different than what Linux used by default IIRC. Juergen can
> > elaborate more.
>
> Yeah, I'd like to make sure that my changes won't break it.
I checked through code inspection and indeed, it seems it would break
Xen's PAT setup.
For the record: the issue here was code that should not run ran, that
is dead code ran. I'm working towards a generic solution for this.
Luis
[toc] | [prev] | [next] | [standalone]
| From | Toshi Kani <toshi.kani@hpe.com> |
|---|---|
| Date | 2016-03-16 00:00 +0100 |
| Subject | Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code |
| Message-ID | <rd66u-9k-5@gated-at.bofh.it> |
| In reply to | #1357711 |
On Tue, 2016-03-15 at 01:15 +0100, Luis R. Rodriguez wrote:
> On Fri, Mar 11, 2016 at 06:16:36PM -0700, Toshi Kani wrote:
> > On Fri, 2016-03-11 at 15:34 -0800, Luis R. Rodriguez wrote:
> > > On Fri, Mar 11, 2016 at 3:56 PM, Toshi Kani <toshi.kani@hpe.com>
> > > wrote:
> > > > On Fri, 2016-03-11 at 23:17 +0100, Luis R. Rodriguez wrote:
> > > > > On Fri, Mar 11, 2016 at 11:57:12AM -0700, Toshi Kani wrote:
:
> > > > > What I'm after is seeing if we can ultimately disable MTRR on
> > > > > kernel code but still have PAT enabled. I realize you've
> > > > > mentioned BIOS code may use some MTRR setup code but this is only
> > > > > true for some systems. I know for a fact Xen cannot use MTRR, it
> > > > > seems qemu32 does not enable it either. So why not have the
> > > > > ability to skip through its set up ?
> > > >
> > > > MTRR support has two meanings:
> > > > 1) The kernel keeps the MTRR setup by BIOS.
> > > > 2) The kernel modifies the MTRR setup.
> > > >
> > > > I am in a position that we need 1) but 2).
> > >
> > > I take it you meant "but not 2)" ?
> >
> > Yes. :)
>
> OK -- we are in agreement but we know 1) is only needed for a portion of
> systems: Xen and qemu32 systems fly with no MTRR set up, and as such it
> would be incorrect to run MTRR code on such systems. To these systems
> MTRR functionality code should be dead, since PAT currently depends on
> MTRR PAT should also be dead but as the report you're fixing shows
> it wasn't. That's an issue for qemu that uses the regular x86 init path
> but not for Xen. Its different for Xen as the hypervisor is the one that
> set up the MSR_IA32_CR_PAT for each CPU. The *only* thing Xen does is:
MTRR code does not have to be dead for the virtual machines with no MTRR
support. The code just needs to handle the disabled case properly. I
consider this is part of 1) that the kernel keeps the MTRR state as
disabled.
> void xen_start_kernel(void)
> {
> ...
> rdmsrl(MSR_IA32_CR_PAT, pat);
>
> pat_init_cache_modes(pat);
> ...
> }
>
> Fortunately we only have to call pat_init_cache_modes() once, its
> not per-CPU. Xen has shown then that you *can* live with PAT without
> any of the complex MTRR setup / code. Please add to your table the
> Xen case as well then as its important to consider. If you make it
> a strong requirement to have MTRR enabled to enable PAT you'd
> be disabling PAT on Xen guest boots.
Yes, I understand that the original fix will break Xen. Thanks for
pointing this out! In the next version (which is the same approach as the
two additional patches I sent you yesterday), I am going to integrate the
Xen's use-case into the framework. xen_start_kernel() will no longer need
to call pat_init_cache_modes() as a result. So, please review the next
version, and let me know if there is any issue.
> As-is then your this patch which calls pat_disable() on mtrr_bp_init()
> for the case where MTRR is disabled would essentially break PAT on Xen
> guests, so this cannot be done. It is no longer true that if MTRR is
> disabled you can force disable PAT. To do what you want you want to do we
> have to consider Xen.
>
> I don't think its a good idea to keep PAT initialization meshed together
> with MTRR and making it a strong requirement on enabling PAT. The MTRR
> code is extremely complex. I'd like instead to encourage for us to
> consider for this situation to let PAT become a first class citizen,
> if MTRR is disabled but you've enabled PAT you should be able to use
> it, just as Xen does. There are more reasons to enable such setup than
> not to. Long term I'm advocating to see if we can get an ACPI legacy
> MTRR flag that can tell us if the BIOS has MTRR ripped out, then we
> can at run time also take advantage of ignoring PAT completely as well.
>
> Note, if you insisted you didn't want to disable PAT on Xen, you could
> in theory check for the subarch -- but note that the subarch is unused
> yet on Xen, even though it was added to the x86 boot protocol years ago.
> I have a slew of patches to make use of it to help put paravirt_enabled()
> in the grave, but based on discussions with Ingo, we don't want to spread
> use of the subarch in random x86 code paths, we want to compartamentalize
> that. If you still want to follow your approach of just force-disabling
> PAT on MTRR code if MTRR was disabled you'd have to use semantics to
> figure out if the boot path came from Xen, to be more specific for Xen
> PV guest types only... The current agreed approach to avoid directly
> using subarch is to categorize differences between what some guests
> need and bare metal under an x86 platform quirk and legacy set of
> components.
>
> On the x86 init path we'd call something check for the subarch and based
> on that set a series of x86 legacy features / quirks that need to be
> disabled / enabled. We could add MTRR as one. I'm unifying some of this
> with a bit of what goes into the ACPI IA-PC boot architecture, see
> section 5.2.9.3 IA-PC Boot Architecture Flags [0]. In particular the
> paravirt_enabled() series I'm working on happens to also dabble into
> the no CMOS RTC case for Xen, generalizing this knocks a bit of birds
> with one stone. I think we can do the same with MTRR but also be
> proactive and see if we can get ACPI_FADT_NO_MTRR added as well for
> a future ACPI spec to enable BIOS manufacturers to rip MTRR out.
We do not need subarch for this case since presence of the MTRR feature can
be tested with CPUID and MSR.
> [0] http://www.acpi.info/DOWNLOADS/ACPIspec50.pdf
>
> /* Masks for FADT IA-PC Boot Architecture Flags (boot_flags)
> [Vx]=Introduced in this FADT revision */
> #define ACPI_FADT_LEGACY_DEVICES (1) /* 00: [V2] System has
> LPC or ISA bus devices */
> #define ACPI_FADT_8042 (1<<1) /* 01: [V3] System has an
> 8042 controller on port 60/64 */
> #define ACPI_FADT_NO_VGA (1<<2) /* 02: [V4] It is not
> safe to probe for VGA hardware */
> #define ACPI_FADT_NO_MSI (1<<3) /* 03: [V4] Message
> Signaled Interrupts (MSI) must not be enabled */
> #define ACPI_FADT_NO_ASPM (1<<4) /* 04: [V4] PCIe ASPM
> control must not be enabled */
> #define ACPI_FADT_NO_CMOS_RTC (1<<5) /* 05: [V5] No CMOS real-
> time clock present */
>
> > > There *are folks however who do
> > > more as I noted earlier. Perhaps not now, but in the future I'd
> > > encourage folks to rip MTRR out of their own BIOS, and enable a new
> > > ACPI legacy flag to say "MTRR required". That'd eventually can help
> > > bury MTRR for good while remaining backward compatible.
> >
> > Well, BIOS using MTRR is better than BIOS setting page tables in the
> > SMI handler.
>
> Can some BIOSes be developed without MTRR? For instance I suspect Google
> might be able to easily pull of ripping MTRR out of their BIOS if they
> didn't do it already for the ChromeOS devices. If possible not only
> should it help with removing complexity on the BIOS but not even having
> to think about that code *ever* running on the kernel at all should be
> nice.
AFAIK, MTRR is the only way to specify UC attribute in physical mode on
x86. On ia64, one can simply set the UC bit to a physical address to
specify UC attribute. I wish we had something similar.
> > The kernel can be ignorant of the MTRR setup as long as it does
> > not modify it.
>
> Sure, we're already there. The kernel no longer modifies the
> MTRR setup unless of course you boot without PAT enabled. I think
> we need to move beyond that to ACPI if we can to let regular
> Linux boots never have to deal with MTRR at all. The code is
> complex and nasty why not put let folks put a nail on the coffin for
> good?
I think we are good as long as we do not modify it. The complexity comes
with the modification.
> > > I can read the above description to also say:
> > >
> > > "Hey you need to implement PAT with the same skeleton code as MTRR"
> >
> > No, I did not say that. MTRR's rendezvous handler can be generalized
> > to work with both MTRR and PAT. We do not need two separate handlers.
> > In fact, it needs to be a single handler so that both can be
> > initialized together.
>
> I'm not sure if that's really needed. Doesn't PAT just require setting
> the wrmsrl(MSR_IA32_CR_PAT, pat) for each AP?
No, it requires the same procedure as MTRR.
> > > If we do that, we can pave the way to deprecate MTRR as legacy for
> > > good first on Linux.
> >
> > I do not think such change will deprecate MTRR.
>
> Not even for shiny new BIOSes? Post ACPI 5?
Nope.
> > It just means that Linux can enable PAT on virtual CPUs with PAT &
> > !MTRR capability.
> >
> > > > In fact, the kernel disabling MTRRs is the same as 2).
> > > >
> > > > > I'll also note Xen managed to enable PAT only without enabling
> > > > > MTRR, this was done through pat_init_cache_modes() -- not sure if
> > > > > this can be leveraged for qemu32...
> > > >
> > > > I am interested to know how Xen managed this. Is this done by the
> > > > Xen hypervisor initializes guest's PAT on behalf of the guest
> > > > kernel?
> > >
> > > Yup. And the cache read thingy was reading back its own setup, which
> > > was different than what Linux used by default IIRC. Juergen can
> > > elaborate more.
> >
> > Yeah, I'd like to make sure that my changes won't break it.
>
> I checked through code inspection and indeed, it seems it would break
> Xen's PAT setup.
>
> For the record: the issue here was code that should not run ran, that
> is dead code ran. I'm working towards a generic solution for this.
Please review the next version.
Thanks,
-Toshi
[toc] | [prev] | [next] | [standalone]
| From | "Luis R. Rodriguez" <mcgrof@kernel.org> |
|---|---|
| Date | 2016-03-16 00:30 +0100 |
| Subject | Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code |
| Message-ID | <rd6zv-B2-7@gated-at.bofh.it> |
| In reply to | #1358319 |
On Tue, Mar 15, 2016 at 05:48:44PM -0600, Toshi Kani wrote:
> On Tue, 2016-03-15 at 01:15 +0100, Luis R. Rodriguez wrote:
> > On Fri, Mar 11, 2016 at 06:16:36PM -0700, Toshi Kani wrote:
> > > On Fri, 2016-03-11 at 15:34 -0800, Luis R. Rodriguez wrote:
> > > > On Fri, Mar 11, 2016 at 3:56 PM, Toshi Kani <toshi.kani@hpe.com>
> > > > wrote:
> > > > > On Fri, 2016-03-11 at 23:17 +0100, Luis R. Rodriguez wrote:
> > > > > > On Fri, Mar 11, 2016 at 11:57:12AM -0700, Toshi Kani wrote:
> :
> > > > > > What I'm after is seeing if we can ultimately disable MTRR on
> > > > > > kernel code but still have PAT enabled. I realize you've
> > > > > > mentioned BIOS code may use some MTRR setup code but this is only
> > > > > > true for some systems. I know for a fact Xen cannot use MTRR, it
> > > > > > seems qemu32 does not enable it either. So why not have the
> > > > > > ability to skip through its set up ?
> > > > >
> > > > > MTRR support has two meanings:
> > > > > 1) The kernel keeps the MTRR setup by BIOS.
> > > > > 2) The kernel modifies the MTRR setup.
> > > > >
> > > > > I am in a position that we need 1) but 2).
> > > >
> > > > I take it you meant "but not 2)" ?
> > >
> > > Yes. :)
> >
> > OK -- we are in agreement but we know 1) is only needed for a portion of
> > systems: Xen and qemu32 systems fly with no MTRR set up, and as such it
> > would be incorrect to run MTRR code on such systems. To these systems
> > MTRR functionality code should be dead, since PAT currently depends on
> > MTRR PAT should also be dead but as the report you're fixing shows
> > it wasn't. That's an issue for qemu that uses the regular x86 init path
> > but not for Xen. Its different for Xen as the hypervisor is the one that
> > set up the MSR_IA32_CR_PAT for each CPU. The *only* thing Xen does is:
>
> MTRR code does not have to be dead for the virtual machines with no MTRR
> support. The code just needs to handle the disabled case properly. I
> consider this is part of 1) that the kernel keeps the MTRR state as
> disabled.
For Xen it was possible to use PAT without any of the MTRR code running,
I don't see why its needed then and why other virtual machines that
only need PAT need it.
> > void xen_start_kernel(void)
> > {
> > ...
> > rdmsrl(MSR_IA32_CR_PAT, pat);
> >
> > pat_init_cache_modes(pat);
> > ...
> > }
> >
> > Fortunately we only have to call pat_init_cache_modes() once, its
> > not per-CPU. Xen has shown then that you *can* live with PAT without
> > any of the complex MTRR setup / code. Please add to your table the
> > Xen case as well then as its important to consider. If you make it
> > a strong requirement to have MTRR enabled to enable PAT you'd
> > be disabling PAT on Xen guest boots.
>
> Yes, I understand that the original fix will break Xen. Thanks for
> pointing this out! In the next version (which is the same approach as the
> two additional patches I sent you yesterday), I am going to integrate the
> Xen's use-case into the framework. xen_start_kernel() will no longer need
> to call pat_init_cache_modes() as a result. So, please review the next
> version, and let me know if there is any issue.
Sure.
> > As-is then your this patch which calls pat_disable() on mtrr_bp_init()
> > for the case where MTRR is disabled would essentially break PAT on Xen
> > guests, so this cannot be done. It is no longer true that if MTRR is
> > disabled you can force disable PAT. To do what you want you want to do we
> > have to consider Xen.
> >
> > I don't think its a good idea to keep PAT initialization meshed together
> > with MTRR and making it a strong requirement on enabling PAT. The MTRR
> > code is extremely complex. I'd like instead to encourage for us to
> > consider for this situation to let PAT become a first class citizen,
> > if MTRR is disabled but you've enabled PAT you should be able to use
> > it, just as Xen does. There are more reasons to enable such setup than
> > not to. Long term I'm advocating to see if we can get an ACPI legacy
> > MTRR flag that can tell us if the BIOS has MTRR ripped out, then we
> > can at run time also take advantage of ignoring PAT completely as well.
> >
> > Note, if you insisted you didn't want to disable PAT on Xen, you could
> > in theory check for the subarch -- but note that the subarch is unused
> > yet on Xen, even though it was added to the x86 boot protocol years ago.
> > I have a slew of patches to make use of it to help put paravirt_enabled()
> > in the grave, but based on discussions with Ingo, we don't want to spread
> > use of the subarch in random x86 code paths, we want to compartamentalize
> > that. If you still want to follow your approach of just force-disabling
> > PAT on MTRR code if MTRR was disabled you'd have to use semantics to
> > figure out if the boot path came from Xen, to be more specific for Xen
> > PV guest types only... The current agreed approach to avoid directly
> > using subarch is to categorize differences between what some guests
> > need and bare metal under an x86 platform quirk and legacy set of
> > components.
> >
> > On the x86 init path we'd call something check for the subarch and based
> > on that set a series of x86 legacy features / quirks that need to be
> > disabled / enabled. We could add MTRR as one. I'm unifying some of this
> > with a bit of what goes into the ACPI IA-PC boot architecture, see
> > section 5.2.9.3 IA-PC Boot Architecture Flags [0]. In particular the
> > paravirt_enabled() series I'm working on happens to also dabble into
> > the no CMOS RTC case for Xen, generalizing this knocks a bit of birds
> > with one stone. I think we can do the same with MTRR but also be
> > proactive and see if we can get ACPI_FADT_NO_MTRR added as well for
> > a future ACPI spec to enable BIOS manufacturers to rip MTRR out.
>
> We do not need subarch for this case since presence of the MTRR feature can
> be tested with CPUID and MSR.
But in this case its about two different cases:
No MTRR - but you need to emulate PAT
No MTRR - but you can just read what the hypervisor already set up
So it a given MTRR is disabled for both, what we need then is semantics
to distinguish between qemu32 and Xen PV. Curious to see what you use,
in your current new patch it was not clear what you did.
> > [0] http://www.acpi.info/DOWNLOADS/ACPIspec50.pdf
> >
> > /* Masks for FADT IA-PC Boot Architecture Flags (boot_flags)
> > [Vx]=Introduced in this FADT revision */
> > #define ACPI_FADT_LEGACY_DEVICES (1) /* 00: [V2] System has
> > LPC or ISA bus devices */
> > #define ACPI_FADT_8042 (1<<1) /* 01: [V3] System has an
> > 8042 controller on port 60/64 */
> > #define ACPI_FADT_NO_VGA (1<<2) /* 02: [V4] It is not
> > safe to probe for VGA hardware */
> > #define ACPI_FADT_NO_MSI (1<<3) /* 03: [V4] Message
> > Signaled Interrupts (MSI) must not be enabled */
> > #define ACPI_FADT_NO_ASPM (1<<4) /* 04: [V4] PCIe ASPM
> > control must not be enabled */
> > #define ACPI_FADT_NO_CMOS_RTC (1<<5) /* 05: [V5] No CMOS real-
> > time clock present */
> >
> > > > There *are folks however who do
> > > > more as I noted earlier. Perhaps not now, but in the future I'd
> > > > encourage folks to rip MTRR out of their own BIOS, and enable a new
> > > > ACPI legacy flag to say "MTRR required". That'd eventually can help
> > > > bury MTRR for good while remaining backward compatible.
> > >
> > > Well, BIOS using MTRR is better than BIOS setting page tables in the
> > > SMI handler.
> >
> > Can some BIOSes be developed without MTRR? For instance I suspect Google
> > might be able to easily pull of ripping MTRR out of their BIOS if they
> > didn't do it already for the ChromeOS devices. If possible not only
> > should it help with removing complexity on the BIOS but not even having
> > to think about that code *ever* running on the kernel at all should be
> > nice.
>
> AFAIK, MTRR is the only way to specify UC attribute in physical mode on
> x86. On ia64, one can simply set the UC bit to a physical address to
> specify UC attribute. I wish we had something similar.
Whoa, you mean on the BIOS? This is BIG deal if true. Why haven't you just
said this a long time ago?
On x86 Linux code we now have ioremap_uc() that can't use MTRR behind the
scenes, why would something like this on the BIOS not be possible? That
ultimately uses set_pte_at(). What limitations are there on the BIOS
that prevent us from just using strong UC for PAT on the BIOS?
> > > The kernel can be ignorant of the MTRR setup as long as it does
> > > not modify it.
> >
> > Sure, we're already there. The kernel no longer modifies the
> > MTRR setup unless of course you boot without PAT enabled. I think
> > we need to move beyond that to ACPI if we can to let regular
> > Linux boots never have to deal with MTRR at all. The code is
> > complex and nasty why not put let folks put a nail on the coffin for
> > good?
>
> I think we are good as long as we do not modify it. The complexity comes
> with the modification.
Ew. I look at that code and cannot comprehend why we'd ever want to keep
it always running.
> > > > I can read the above description to also say:
> > > >
> > > > "Hey you need to implement PAT with the same skeleton code as MTRR"
> > >
> > > No, I did not say that. MTRR's rendezvous handler can be generalized
> > > to work with both MTRR and PAT. We do not need two separate handlers.
> > > In fact, it needs to be a single handler so that both can be
> > > initialized together.
> >
> > I'm not sure if that's really needed. Doesn't PAT just require setting
> > the wrmsrl(MSR_IA32_CR_PAT, pat) for each AP?
>
> No, it requires the same procedure as MTRR.
MTRR has a bunch of junk that is outside of the scope of the generic
procedure which I'd hope we can skip.
> > > > If we do that, we can pave the way to deprecate MTRR as legacy for
> > > > good first on Linux.
> > >
> > > I do not think such change will deprecate MTRR.
> >
> > Not even for shiny new BIOSes? Post ACPI 5?
>
> Nope.
Hrm...
> > > It just means that Linux can enable PAT on virtual CPUs with PAT &
> > > !MTRR capability.
> > >
> > > > > In fact, the kernel disabling MTRRs is the same as 2).
> > > > >
> > > > > > I'll also note Xen managed to enable PAT only without enabling
> > > > > > MTRR, this was done through pat_init_cache_modes() -- not sure if
> > > > > > this can be leveraged for qemu32...
> > > > >
> > > > > I am interested to know how Xen managed this. Is this done by the
> > > > > Xen hypervisor initializes guest's PAT on behalf of the guest
> > > > > kernel?
> > > >
> > > > Yup. And the cache read thingy was reading back its own setup, which
> > > > was different than what Linux used by default IIRC. Juergen can
> > > > elaborate more.
> > >
> > > Yeah, I'd like to make sure that my changes won't break it.
> >
> > I checked through code inspection and indeed, it seems it would break
> > Xen's PAT setup.
> >
> > For the record: the issue here was code that should not run ran, that
> > is dead code ran. I'm working towards a generic solution for this.
>
> Please review the next version.
Sure..
Luis
[toc] | [prev] | [standalone]
Page 2 of 2 — ← Prev page 1 [2]
Back to top | Article view | linux.kernel
csiph-web