[PATCH urgent] x86: Save cr4 to mmu_cr4_features at boot time

[PATCH urgent] x86: Save cr4 to mmu_cr4_features at boot time

[Stefano Stabellini]

Save cr4 to mmu_cr4_features at boot time

This patch fixes a freeze when resuming from hibernation, introduced by
"x86: Cleanup highmap after brk is concluded", commit id
e5f15b45ddf3afa2bbbb10c7ea34fb32b6de0a0e.
This patch should be backported to all the stable tree where the
offending commit is present.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Tested-by: Michael Leun <lkml20101129@newton.leun.net>
CC: Yinghai Lu <yinghai@kernel.org>
CC: stable <stable@kernel.org>

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 5a0484a..0943eb2 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -891,6 +891,7 @@ void __init setup_arch(char **cmdline_p)
 		max_low_pfn = max_pfn;
 
 	high_memory = (void *)__va(max_pfn * PAGE_SIZE - 1) + 1;
+	mmu_cr4_features = read_cr4();
 #endif
 
 	/*

Re: [PATCH urgent] x86: Save cr4 to mmu_cr4_features at boot time

[Ingo Molnar]

* Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:

> Save cr4 to mmu_cr4_features at boot time
> 
> This patch fixes a freeze when resuming from hibernation, introduced by
> "x86: Cleanup highmap after brk is concluded", commit id
> e5f15b45ddf3afa2bbbb10c7ea34fb32b6de0a0e.
> This patch should be backported to all the stable tree where the
> offending commit is present.

No, the offending commit should be reverted from -stable. It's not at all clear 
yet why this fix patch helps.

Thanks,

	Ingo

Re: [PATCH urgent] x86: Save cr4 to mmu_cr4_features at boot time

[Stefano Stabellini]

On Fri, 1 Apr 2011, Ingo Molnar wrote:
> 
> * Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> 
> > Save cr4 to mmu_cr4_features at boot time
> > 
> > This patch fixes a freeze when resuming from hibernation, introduced by
> > "x86: Cleanup highmap after brk is concluded", commit id
> > e5f15b45ddf3afa2bbbb10c7ea34fb32b6de0a0e.
> > This patch should be backported to all the stable tree where the
> > offending commit is present.
> 
> No, the offending commit should be reverted from -stable. It's not at all clear 
> yet why this fix patch helps.

e5f15b45ddf3afa2bbbb10c7ea34fb32b6de0a0e removes cleanup_highmap_brk_end
because the initial mapping of the region between _brk_end and _end is
going to be cleared by cleanup_highmap instead.

However cleanup_highmap_brk_end was not only clearing mappings, it was
also setting mmu_cr4_features, and this bit was lost with the removal of
cleanup_highmap_brk_end.

So this fix patch restores things back to how they were before
e5f15b45ddf3afa2bbbb10c7ea34fb32b6de0a0e (apart from the fact that
mappings between _brk_end and _end are cleared in cleanup_highmap now).

Obviously settings mmu_cr4_features shouldn't have been done in
cleanup_highmap_brk_end, since it doesn't have anything to do with
mappings.



P.S.
I asked for a backport of e5f15b45ddf3afa2bbbb10c7ea34fb32b6de0a0e to
the stable trees because I forgot to add "CC stable@kernel.org" to the
commit message and this commit is needed to fix a critical crash on boot
on xen.

Re: [PATCH urgent] x86: Save cr4 to mmu_cr4_features at boot time

[H. Peter Anvin]

On 04/01/2011 04:30 AM, Stefano Stabellini wrote:
> 
> P.S.
> I asked for a backport of e5f15b45ddf3afa2bbbb10c7ea34fb32b6de0a0e to
> the stable trees because I forgot to add "CC stable@kernel.org" to the
> commit message and this commit is needed to fix a critical crash on boot
> on xen.

Yes, and you broke ALL machines in the process.  Great job.

	-hpa

Re: [PATCH urgent] x86: Save cr4 to mmu_cr4_features at boot time

[Ingo Molnar]

* Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:

> P.S.
>
> I asked for a backport of e5f15b45ddf3afa2bbbb10c7ea34fb32b6de0a0e to the 
> stable trees because I forgot to add "CC stable@kernel.org" to the commit 
> message and this commit is needed to fix a critical crash on boot on xen.

You are displaying a dangerously amateurish attitude towards quality control 
and you clealy have no idea what it takes to push fixes into -stable.

You forwarded a brand-new, visibly dangerous native kernel commit to -stable 
without Cc:-ing the maintainers and you have caused a native kernel regression 
that way. You are not maintaining the code in question and you have not 
committed the patch in question.

-stable team, please revert this commit. The whole issue obviously needs more 
testing and we'll forward a proper fix once it's available and tested.

	Ingo

Re: [stable] [PATCH urgent] x86: Save cr4 to mmu_cr4_features at boot time

[Greg KH]

On Fri, Apr 01, 2011 at 09:08:12PM +0200, Ingo Molnar wrote:
> 
> * Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> 
> > P.S.
> >
> > I asked for a backport of e5f15b45ddf3afa2bbbb10c7ea34fb32b6de0a0e to the 
> > stable trees because I forgot to add "CC stable@kernel.org" to the commit 
> > message and this commit is needed to fix a critical crash on boot on xen.
> 
> You are displaying a dangerously amateurish attitude towards quality control 
> and you clealy have no idea what it takes to push fixes into -stable.
> 
> You forwarded a brand-new, visibly dangerous native kernel commit to -stable 
> without Cc:-ing the maintainers and you have caused a native kernel regression 
> that way. You are not maintaining the code in question and you have not 
> committed the patch in question.
> 
> -stable team, please revert this commit. The whole issue obviously needs more 
> testing and we'll forward a proper fix once it's available and tested.

Ok, when I return from vacation on Tuesday I will revert it and do new
releases.

thanks,

greg k-h

Re: [PATCH urgent] x86: Save cr4 to mmu_cr4_features at boot time

[Stefano Stabellini]

On Fri, 1 Apr 2011, Ingo Molnar wrote:
> * Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> > I asked for a backport of e5f15b45ddf3afa2bbbb10c7ea34fb32b6de0a0e to the 
> > stable trees because I forgot to add "CC stable@kernel.org" to the commit 
> > message and this commit is needed to fix a critical crash on boot on xen.
> 
> You are displaying a dangerously amateurish attitude towards quality control 
> and you clealy have no idea what it takes to push fixes into -stable.
> 
> You forwarded a brand-new, visibly dangerous native kernel commit to -stable 
> without Cc:-ing the maintainers and you have caused a native kernel regression 
> that way. You are not maintaining the code in question and you have not 
> committed the patch in question.


Just to clarify the situation the patch "x86-64: finish
cleanup_highmaps()'s job wrt. _brk_end" was backported to the stable
trees (including Jeremy's 2.6.32 xen tree because he pulled from
2.6.32.y) breaking boot on xen.
Yinghai's patch plus another patch of mine fix that breakage and that is
why I ask for it to be backported.
I didn't mean for the backport to be done right now, I just wanted to
notify the stable maintainers that the particular commit should be
backported at some point.

I didn't mean to overstep your authority in any way.
I know that ignorance is never an excuse but I am still new to this and
my knowledge of how the process works is limited to a blog entry by Greg
KH from an year ago.
I realize that stable trees are really important so I am very sorry to
have introduced such an important breakage.

Re: [PATCH urgent] x86: Save cr4 to mmu_cr4_features at boot time

[Ingo Molnar]

* Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:

> Just to clarify the situation the patch "x86-64: finish cleanup_highmaps()'s 
> job wrt. _brk_end" was backported to the stable trees [...]

There's no commit with such a title upstream - there's not even one that is 
close. Could you cite the sha1 you refer to?

> [...] (including Jeremy's 2.6.32 xen tree because he pulled from 2.6.32.y) 
> breaking boot on xen.

Basing upstream-relevant trees on stable backported sha1's is a very, very bad 
idea.

> Yinghai's patch plus another patch of mine fix that breakage and that is why 
> I ask for it to be backported.

So it fixes a commit that is nowhere to be found upstream?

> I didn't mean for the backport to be done right now, I just wanted to
> notify the stable maintainers that the particular commit should be
> backported at some point.
> 
> I didn't mean to overstep your authority in any way.

It's not about overstepping authority - it's about not breaking the native 
kernel on millions of boxes.

Thanks,

	Ingo

Re: [PATCH urgent] x86: Save cr4 to mmu_cr4_features at boot time

[Stefano Stabellini]

On Mon, 4 Apr 2011, Ingo Molnar wrote:
> 
> * Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> 
> > Just to clarify the situation the patch "x86-64: finish cleanup_highmaps()'s 
> > job wrt. _brk_end" was backported to the stable trees [...]
> 
> There's no commit with such a title upstream - there's not even one that is 
> close. Could you cite the sha1 you refer to?

The commit id upstream is 498343967613183611ac37dccb2846496d954c06


> > [...] (including Jeremy's 2.6.32 xen tree because he pulled from 2.6.32.y) 
> > breaking boot on xen.
> 
> Basing upstream-relevant trees on stable backported sha1's is a very, very bad 
> idea.

We are not using Jeremy's 2.6.32 tree for development anymore, we
develop on upstream directly.


> > Yinghai's patch plus another patch of mine fix that breakage and that is why 
> > I ask for it to be backported.
> 
> So it fixes a commit that is nowhere to be found upstream?

It fixes 498343967613183611ac37dccb2846496d954c06, if you are interested
in all the details of the conversation with Yinghai and Peter, here are
a couple of links to the lkml archives:

https://lkml.org/lkml/2011/1/31/232
https://lkml.org/lkml/2011/2/28/410

Re: [PATCH urgent] x86: Save cr4 to mmu_cr4_features at boot time

[Ingo Molnar]

* Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:

> On Mon, 4 Apr 2011, Ingo Molnar wrote:
> > 
> > * Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> > 
> > > Just to clarify the situation the patch "x86-64: finish cleanup_highmaps()'s 
> > > job wrt. _brk_end" was backported to the stable trees [...]
> > 
> > There's no commit with such a title upstream - there's not even one that is 
> > close. Could you cite the sha1 you refer to?
> 
> The commit id upstream is 498343967613183611ac37dccb2846496d954c06

That commit is from the v2.6.30 era:

 |
 | commit 498343967613183611ac37dccb2846496d954c06
 | Author:     Jan Beulich <jbeulich@novell.com>
 | AuthorDate: Wed May 6 13:06:47 2009 +0100
 | Commit:     H. Peter Anvin <hpa@zytor.com>
 | CommitDate: Thu May 7 21:51:34 2009 -0700
 |
 |    x86-64: finish cleanup_highmaps()'s job wrt. _brk_end
 |

... i.e. the bug is almost 2 years and 8 -stable cycles old!

Peter very consciously did not mark the fix for this commit as -stable 
material. It was ineligible for -stable for multiple reasons: it by no means 
fixed a 2.6.39 regression and the fix was literally just a few days old.

Thanks,

	Ingo

Re: [PATCH urgent] x86: Save cr4 to mmu_cr4_features at boot time

[Rafael J. Wysocki]

On Monday, April 04, 2011, Ingo Molnar wrote:
> 
> * Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> 
> > On Mon, 4 Apr 2011, Ingo Molnar wrote:
> > > 
> > > * Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> > > 
> > > > Just to clarify the situation the patch "x86-64: finish cleanup_highmaps()'s 
> > > > job wrt. _brk_end" was backported to the stable trees [...]
> > > 
> > > There's no commit with such a title upstream - there's not even one that is 
> > > close. Could you cite the sha1 you refer to?
> > 
> > The commit id upstream is 498343967613183611ac37dccb2846496d954c06
> 
> That commit is from the v2.6.30 era:
> 
>  |
>  | commit 498343967613183611ac37dccb2846496d954c06
>  | Author:     Jan Beulich <jbeulich@novell.com>
>  | AuthorDate: Wed May 6 13:06:47 2009 +0100
>  | Commit:     H. Peter Anvin <hpa@zytor.com>
>  | CommitDate: Thu May 7 21:51:34 2009 -0700
>  |
>  |    x86-64: finish cleanup_highmaps()'s job wrt. _brk_end
>  |
> 
> ... i.e. the bug is almost 2 years and 8 -stable cycles old!
> 
> Peter very consciously did not mark the fix for this commit as -stable 
> material. It was ineligible for -stable for multiple reasons: it by no means 
> fixed a 2.6.39 regression and the fix was literally just a few days old.

Has this issue been resolved in the mainline, BTW?

Rafael

Re: [PATCH urgent] x86: Save cr4 to mmu_cr4_features at boot time

[H. Peter Anvin]

On 04/04/2011 11:29 PM, Rafael J. Wysocki wrote:
>>
>> Peter very consciously did not mark the fix for this commit as -stable 
>> material. It was ineligible for -stable for multiple reasons: it by no means 
>> fixed a 2.6.39 regression and the fix was literally just a few days old.
> 
> Has this issue been resolved in the mainline, BTW?
> 

Just to refresh my memory... is this an issue in mainline, or is it only
a problem in the backport (I'm wondering if the trampoline unification
patches might have accidentally solved the issue)?

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH urgent] x86: Save cr4 to mmu_cr4_features at boot time

[Rafael J. Wysocki]

On Tuesday, April 05, 2011, H. Peter Anvin wrote:
> On 04/04/2011 11:29 PM, Rafael J. Wysocki wrote:
> >>
> >> Peter very consciously did not mark the fix for this commit as -stable 
> >> material. It was ineligible for -stable for multiple reasons: it by no means 
> >> fixed a 2.6.39 regression and the fix was literally just a few days old.
> > 
> > Has this issue been resolved in the mainline, BTW?
> > 
> 
> Just to refresh my memory... is this an issue in mainline, or is it only
> a problem in the backport (I'm wondering if the trampoline unification
> patches might have accidentally solved the issue)?


The problem is in mainline too, please fix ASAP.

Thanks,
Rafael

Re: [PATCH urgent] x86: Save cr4 to mmu_cr4_features at boot time

[H. Peter Anvin]

On 04/04/2011 11:43 PM, Rafael J. Wysocki wrote:
>>
>> Just to refresh my memory... is this an issue in mainline, or is it only
>> a problem in the backport (I'm wondering if the trampoline unification
>> patches might have accidentally solved the issue)?
> 
> The problem is in mainline too, please fix ASAP.

Thanks for confirming, will look at it in the morning.  Just looking for
the sanest place to put the save/restore (it really should be done at
the same places where we save/restore MISC_ENABLE.)

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH urgent] x86: Save cr4 to mmu_cr4_features at boot time

[H. Peter Anvin]

[-- Attachment #1: Type: text/plain, Size: 1055 bytes --]

On 04/04/2011 11:43 PM, Rafael J. Wysocki wrote:
> On Tuesday, April 05, 2011, H. Peter Anvin wrote:
>> On 04/04/2011 11:29 PM, Rafael J. Wysocki wrote:
>>>>
>>>> Peter very consciously did not mark the fix for this commit as -stable 
>>>> material. It was ineligible for -stable for multiple reasons: it by no means 
>>>> fixed a 2.6.39 regression and the fix was literally just a few days old.
>>>
>>> Has this issue been resolved in the mainline, BTW?
>>>
>>
>> Just to refresh my memory... is this an issue in mainline, or is it only
>> a problem in the backport (I'm wondering if the trampoline unification
>> patches might have accidentally solved the issue)?
> 
> 
> The problem is in mainline too, please fix ASAP.
> 

For the suspend/resume case this seems like the sanest way to fix it in
my opinion.  However, I am a bit concerned since I'm still not sure
we're programming registers in the correct order, that is:

MISC_ENABLE -> EFER -> cr4 -> cr3 -> cr0

I will look at this issue later this evening, but I wanted your opinion
on it.

	-hpa

[-- Attachment #2: diff --]
[-- Type: text/plain, Size: 599 bytes --]

diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c
index 87bb35e..69dbf42 100644
--- a/arch/x86/power/cpu.c
+++ b/arch/x86/power/cpu.c
@@ -98,13 +98,12 @@ static void __save_processor_state(struct saved_context *ctxt)
 	ctxt->cr0 = read_cr0();
 	ctxt->cr2 = read_cr2();
 	ctxt->cr3 = read_cr3();
-#ifdef CONFIG_X86_32
 	ctxt->cr4 = read_cr4_safe();
-#else
-/* CONFIG_X86_64 */
-	ctxt->cr4 = read_cr4();
+	mmu_cr4_features = ctxt->cr4;
+#ifdef CONFIG_X86_64
 	ctxt->cr8 = read_cr8();
 #endif
+
 	ctxt->misc_enable_saved = !rdmsrl_safe(MSR_IA32_MISC_ENABLE,
 					       &ctxt->misc_enable);
 }

Re: [PATCH urgent] x86: Save cr4 to mmu_cr4_features at boot time

[Rafael J. Wysocki]

On Wednesday, April 06, 2011, H. Peter Anvin wrote:
> On 04/04/2011 11:43 PM, Rafael J. Wysocki wrote:
> > On Tuesday, April 05, 2011, H. Peter Anvin wrote:
> >> On 04/04/2011 11:29 PM, Rafael J. Wysocki wrote:
> >>>>
> >>>> Peter very consciously did not mark the fix for this commit as -stable 
> >>>> material. It was ineligible for -stable for multiple reasons: it by no means 
> >>>> fixed a 2.6.39 regression and the fix was literally just a few days old.
> >>>
> >>> Has this issue been resolved in the mainline, BTW?
> >>>
> >>
> >> Just to refresh my memory... is this an issue in mainline, or is it only
> >> a problem in the backport (I'm wondering if the trampoline unification
> >> patches might have accidentally solved the issue)?
> > 
> > 
> > The problem is in mainline too, please fix ASAP.
> > 
> 
> For the suspend/resume case this seems like the sanest way to fix it in
> my opinion.  However, I am a bit concerned since I'm still not sure
> we're programming registers in the correct order, that is:
> 
> MISC_ENABLE -> EFER -> cr4 -> cr3 -> cr0
> 
> I will look at this issue later this evening, but I wanted your opinion
> on it.

Do you mean during resume?

I think we can try to make the ordering more appropriate, but I'm not really
sure it would be a good idea to do that in the same patch.  Probably not.

Also, our current ordering has never been reported to cause problems to anyone.

Thanks,
Rafael

Re: [PATCH urgent] x86: Save cr4 to mmu_cr4_features at boot time

[H. Peter Anvin]

On 04/05/2011 09:34 PM, Rafael J. Wysocki wrote:
> 
> Do you mean during resume?
> 
> I think we can try to make the ordering more appropriate, but I'm not really
> sure it would be a good idea to do that in the same patch.  Probably not.
> 
> Also, our current ordering has never been reported to cause problems to anyone.
> 

Yes, I mean on resume.

I'm not sure if we'd know since it would manifest as a very early failure.

Anyway, agreed it's not the same patch.

Two things:

1. Do you agree that this is the right place to put this?  There seems
to be some other things in __save_processor_state() which puts things in
places outside struct saved_context, but I would like your opinion.

2. While we're at it, why is mtrr_save_fixed_ranges() only called on x86-32?

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH urgent] x86: Save cr4 to mmu_cr4_features at boot time

[Greg KH]

On Fri, Apr 01, 2011 at 08:23:38AM +0200, Ingo Molnar wrote:
> 
> * Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> 
> > Save cr4 to mmu_cr4_features at boot time
> > 
> > This patch fixes a freeze when resuming from hibernation, introduced by
> > "x86: Cleanup highmap after brk is concluded", commit id
> > e5f15b45ddf3afa2bbbb10c7ea34fb32b6de0a0e.
> > This patch should be backported to all the stable tree where the
> > offending commit is present.
> 
> No, the offending commit should be reverted from -stable. It's not at all clear 
> yet why this fix patch helps.

It's now reverted.

thanks,

greg k-h