LinuxLists.cc - 2.6.38.2 breaks suspend to disk

[permalink] [raw]

Subject: Re: 2.6.38.2 breaks suspend to disk

On 03/30/2011 11:32 AM, Michael Leun wrote:
> Hi,
>
> suspend to disk works with 2.6.38.1 but does not with 2.6.38.2 on my
> acer 1825ptz. Machine freezes after resume.
>
> git bisect yields:
>
> ml@jill:/usr/src/kernel/b/linux-2.6.38.y> git bisect bad
> ff518ea26654e05d325d996f6e3a7f5f569cc2d5 is the first bad commit
> commit ff518ea26654e05d325d996f6e3a7f5f569cc2d5
> Author: Yinghai Lu<[email protected]>
> Date: Fri Feb 18 11:30:30 2011 +0000
>
> x86: Cleanup highmap after brk is concluded
>
> commit e5f15b45ddf3afa2bbbb10c7ea34fb32b6de0a0e upstream.
>
> Now cleanup_highmap actually is in two steps: one is early in head64.c
> and only clears above _end; a second one is in init_memory_mapping() and
> tries to clean from _brk_end to _end.
> It should check if those boundaries are PMD_SIZE aligned but currently
> does not.
> Also init_memory_mapping() is called several times for numa or memory
> hotplug, so we really should not handle initial kernel mappings there.
>
> This patch moves cleanup_highmap() down after _brk_end is settled so
> we can do everything in one step.
> Also we honor max_pfn_mapped in the implementation of cleanup_highmap.
>
> Signed-off-by: Yinghai Lu<[email protected]>
> Signed-off-by: Stefano Stabellini<[email protected]>
> LKML-Reference:<alpine.DEB.2.00.1103171739050.3382@kaball-desktop>
> Signed-off-by: H. Peter Anvin<[email protected]>
> Signed-off-by: Greg Kroah-Hartman<[email protected]>
>
> :040000 040000 b5ed0c2971ba1162c7cd289dd351d1700eb52fbc 8f830fdb43fa30ddebb485e6f6455d669300874b M arch
>

can you please try following partial reverting patch?

diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index 7942335..07688d1 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -302,11 +302,11 @@ void __init init_extra_mapping_uc(unsigned long phys, unsigned long size)
void __init cleanup_highmap(void)
{
unsigned long vaddr = __START_KERNEL_map;
- unsigned long vaddr_end = __START_KERNEL_map + (max_pfn_mapped << PAGE_SHIFT);
unsigned long end = roundup((unsigned long)_brk_end, PMD_SIZE) - 1;
pmd_t *pmd = level2_kernel_pgt;
+ pmd_t *last_pmd = pmd + PTRS_PER_PMD;

- for (; vaddr + PMD_SIZE - 1 < vaddr_end; pmd++, vaddr += PMD_SIZE) {
+ for (; pmd < last_pmd; pmd++, vaddr += PMD_SIZE) {
if (pmd_none(*pmd))
continue;
if (vaddr < (unsigned long) _text || vaddr > end)

2011-03-31 07:10:26

by Michael Leun

[permalink] [raw]

Subject: Re: 2.6.38.2 breaks suspend to disk

On Wed, 30 Mar 2011 22:14:56 -0700
Yinghai Lu <[email protected]> wrote:

> On 03/30/2011 11:32 AM, Michael Leun wrote:
> > Hi,
> >
> > suspend to disk works with 2.6.38.1 but does not with 2.6.38.2 on my
> > acer 1825ptz. Machine freezes after resume.
> >
> > git bisect yields:
> >
> > ml@jill:/usr/src/kernel/b/linux-2.6.38.y> git bisect bad
> > ff518ea26654e05d325d996f6e3a7f5f569cc2d5 is the first bad commit
> > commit ff518ea26654e05d325d996f6e3a7f5f569cc2d5
> > Author: Yinghai Lu<[email protected]>
> > Date: Fri Feb 18 11:30:30 2011 +0000
> >
> > x86: Cleanup highmap after brk is concluded
> >
> > commit e5f15b45ddf3afa2bbbb10c7ea34fb32b6de0a0e upstream.
> >
> > Now cleanup_highmap actually is in two steps: one is early in
> > head64.c and only clears above _end; a second one is in
> > init_memory_mapping() and tries to clean from _brk_end to _end.
> > It should check if those boundaries are PMD_SIZE aligned but
> > currently does not.
> > Also init_memory_mapping() is called several times for numa or
> > memory hotplug, so we really should not handle initial kernel
> > mappings there.
> >
> > This patch moves cleanup_highmap() down after _brk_end is
> > settled so we can do everything in one step.
> > Also we honor max_pfn_mapped in the implementation of
> > cleanup_highmap.
> >
> > Signed-off-by: Yinghai Lu<[email protected]>
> > Signed-off-by: Stefano
> > Stabellini<[email protected]>
> > LKML-Reference:<alpine.DEB.2.00.1103171739050.3382@kaball-desktop>
> > Signed-off-by: H. Peter Anvin<[email protected]> Signed-off-by: Greg
> > Kroah-Hartman<[email protected]>
> >
> > :040000 040000 b5ed0c2971ba1162c7cd289dd351d1700eb52fbc
> > 8f830fdb43fa30ddebb485e6f6455d669300874b M arch
> >
>
> can you please try following partial reverting patch?
>
> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
> index 7942335..07688d1 100644
> --- a/arch/x86/mm/init_64.c
> +++ b/arch/x86/mm/init_64.c
> @@ -302,11 +302,11 @@ void __init init_extra_mapping_uc(unsigned long
> phys, unsigned long size) void __init cleanup_highmap(void)
> {
> unsigned long vaddr = __START_KERNEL_map;
> - unsigned long vaddr_end = __START_KERNEL_map +
> (max_pfn_mapped << PAGE_SHIFT); unsigned long end = roundup((unsigned
> long)_brk_end, PMD_SIZE) - 1; pmd_t *pmd = level2_kernel_pgt;
> + pmd_t *last_pmd = pmd + PTRS_PER_PMD;
>
> - for (; vaddr + PMD_SIZE - 1 < vaddr_end; pmd++, vaddr +=
> PMD_SIZE) {
> + for (; pmd < last_pmd; pmd++, vaddr += PMD_SIZE) {
> if (pmd_none(*pmd))
> continue;
> if (vaddr < (unsigned long) _text || vaddr > end)
>

Nope, unfortunately this does not change anything (neither when
applying ontop of the tree resulting from bisecting, nor ontop
2.6.38.2).

Just to make shure there was no mistake when bisecting I did

patch -R -p1 <~/linux-2.6.38.y.git-ff518ea26654e05d325d996f6e3a7f5f569cc2d5.txt

and then indeed it worked again.

--
MfG,

Michael Leun

2011-03-31 14:48:23

[permalink] [raw]

Subject: Re: 2.6.38.2 breaks suspend to disk

On Thu, 31 Mar 2011, Michael Leun wrote:
> Nope, unfortunately this does not change anything (neither when
> applying ontop of the tree resulting from bisecting, nor ontop
> 2.6.38.2).
>
> Just to make shure there was no mistake when bisecting I did
>
> patch -R -p1 <~/linux-2.6.38.y.git-ff518ea26654e05d325d996f6e3a7f5f569cc2d5.txt
>
> and then indeed it worked again.

This patch fixes it for me.

---

save cr4 to mmu_cr4_features at boot time

Signed-off-by: Stefano Stabellini <[email protected]>

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

/*

2011-03-31 16:04:50

by Michael Leun

[permalink] [raw]

Subject: Re: 2.6.38.2 breaks suspend to disk

On Thu, 31 Mar 2011 15:48:44 +0100
Stefano Stabellini <[email protected]> wrote:

> On Thu, 31 Mar 2011, Michael Leun wrote:
> > Nope, unfortunately this does not change anything (neither when
> > applying ontop of the tree resulting from bisecting, nor ontop
> > 2.6.38.2).
> >
> > Just to make shure there was no mistake when bisecting I did
> >
> > patch -R -p1
> > <~/linux-2.6.38.y.git-ff518ea26654e05d325d996f6e3a7f5f569cc2d5.txt
> >
> > and then indeed it worked again.
>
> This patch fixes it for me.
>
>
> ---
>
> save cr4 to mmu_cr4_features at boot time
>
> Signed-off-by: Stefano Stabellini <[email protected]>
>
> 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
>
> /*
>

Confirmed - fixes it for me too. Thanks.

--
MfG,

Michael Leun

2011-03-31 21:48:53

by Rafael J. Wysocki

[permalink] [raw]

Subject: Re: 2.6.38.2 breaks suspend to disk

On Thursday, March 31, 2011, Stefano Stabellini wrote:
> On Thu, 31 Mar 2011, Michael Leun wrote:
> > Nope, unfortunately this does not change anything (neither when
> > applying ontop of the tree resulting from bisecting, nor ontop
> > 2.6.38.2).
> >
> > Just to make shure there was no mistake when bisecting I did
> >
> > patch -R -p1 <~/linux-2.6.38.y.git-ff518ea26654e05d325d996f6e3a7f5f569cc2d5.txt
> >
> > and then indeed it worked again.
>
> This patch fixes it for me.
>
>
> ---
>
> save cr4 to mmu_cr4_features at boot time
>
> Signed-off-by: Stefano Stabellini <[email protected]>

Acked-by: Rafael J. Wysocki <[email protected]>

Peter, Ingo, please regard this as urgent, it breaks -stable.

Thanks,
Rafael

> 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
>
> /*
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>

2011-03-31 22:21:21

[permalink] [raw]

Subject: Re: 2.6.38.2 breaks suspend to disk

On 03/31/2011 02:48 PM, Rafael J. Wysocki wrote:
> On Thursday, March 31, 2011, Stefano Stabellini wrote:
>> On Thu, 31 Mar 2011, Michael Leun wrote:
>>> Nope, unfortunately this does not change anything (neither when
>>> applying ontop of the tree resulting from bisecting, nor ontop
>>> 2.6.38.2).
>>>
>>> Just to make shure there was no mistake when bisecting I did
>>>
>>> patch -R -p1<~/linux-2.6.38.y.git-ff518ea26654e05d325d996f6e3a7f5f569cc2d5.txt
>>>
>>> and then indeed it worked again.
>>
>> This patch fixes it for me.
>>
>>
>> ---
>>
>> save cr4 to mmu_cr4_features at boot time
>>
>> Signed-off-by: Stefano Stabellini<[email protected]>
>
> Acked-by: Rafael J. Wysocki<[email protected]>
>
> Peter, Ingo, please regard this as urgent, it breaks -stable.
>
> Thanks,
> Rafael
>
>
>> 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
>>
>> /*

Stefano, can you explain why it fixes the problem?

or original patch uncover the bug?

Thanks

Yinghai

2011-04-01 06:15:47

by Ingo Molnar

[permalink] [raw]

Subject: Re: 2.6.38.2 breaks suspend to disk

* Rafael J. Wysocki <[email protected]> wrote:

> On Thursday, March 31, 2011, Stefano Stabellini wrote:
> > On Thu, 31 Mar 2011, Michael Leun wrote:
> > > Nope, unfortunately this does not change anything (neither when
> > > applying ontop of the tree resulting from bisecting, nor ontop
> > > 2.6.38.2).
> > >
> > > Just to make shure there was no mistake when bisecting I did
> > >
> > > patch -R -p1 <~/linux-2.6.38.y.git-ff518ea26654e05d325d996f6e3a7f5f569cc2d5.txt
> > >
> > > and then indeed it worked again.
> >
> > This patch fixes it for me.
> >
> >
> > ---
> >
> > save cr4 to mmu_cr4_features at boot time
> >
> > Signed-off-by: Stefano Stabellini <[email protected]>
>
> Acked-by: Rafael J. Wysocki <[email protected]>
>
> Peter, Ingo, please regard this as urgent, it breaks -stable.

Stable team, please revert it from -stable for now.

Who pushed it to -stable so soon, just a few days after the commit has hit .39
upstream? It was not marked -stable in the commit either.

Thanks,

Ingo

2011-04-01 11:32:17

[permalink] [raw]

Subject: Re: 2.6.38.2 breaks suspend to disk

On Thu, 31 Mar 2011, Yinghai Lu wrote:
> Stefano, can you explain why it fixes the problem?
>
> or original patch uncover the bug?

as I wrote in the other email, e5f15b45ddf3afa2bbbb10c7ea34fb32b6de0a0e
removes cleanup_highmap_brk_end but this function was not only dealing
with mappings but it was also setting mmu_cr4_features, therefore the
setting of mmu_cr4_features was lost.

I think the real bug is that mmu_cr4_features ended up in
cleanup_highmap_brk_end by mistake.

2011-04-01 16:07:22

[permalink] [raw]

Subject: Re: 2.6.38.2 breaks suspend to disk

On 04/01/2011 04:32 AM, Stefano Stabellini wrote:
> On Thu, 31 Mar 2011, Yinghai Lu wrote:
>> Stefano, can you explain why it fixes the problem?
>>
>> or original patch uncover the bug?
>
> as I wrote in the other email, e5f15b45ddf3afa2bbbb10c7ea34fb32b6de0a0e
> removes cleanup_highmap_brk_end but this function was not only dealing
> with mappings but it was also setting mmu_cr4_features, therefore the
> setting of mmu_cr4_features was lost.
>
> I think the real bug is that mmu_cr4_features ended up in
> cleanup_highmap_brk_end by mistake.

originally mmu_cr4 is saved in init_memory_mapping.

and init_memory_mapping will update cr4.

so please use this version instead.

From: Stefano Stabellini <[email protected]>

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

Save cr4 to mmu_cr4_features at boot time

Michael reported 2.6.38.2 hibernation is broken by one backported patch.
it cause a freeze when resuming from hibernation

| "x86: Cleanup highmap after brk is concluded"
| commit id e5f15b45ddf3afa2bbbb10c7ea34fb32b6de0a0e.

it turns out the mmu_cr4 save it lost somehow.

-v2: need to save mmu_cr4 after init_memory_mapping(), because it updates mmu_cr4
and originally that storing action is after updating. -- by Yinghai

Bisected-and-tested-by: Michael Leun <[email protected]>
Signed-off-by: Stefano Stabellini <[email protected]>
Signed-off-by: Yinghai Lu <[email protected]>

---
arch/x86/kernel/setup.c | 9 +++++++++
1 file changed, 9 insertions(+)

Index: linux-2.6/arch/x86/kernel/setup.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/setup.c
+++ linux-2.6/arch/x86/kernel/setup.c
@@ -294,10 +294,17 @@ static void __init init_gbpages(void)
else
direct_gbpages = 0;
}
+static void __init store_mmu_cr4(void)
+{
+ mmu_cr4_features = read_cr4();
+}
#else
static inline void init_gbpages(void)
{
}
+static void __init store_mmu_cr4(void)
+{
+}
static void __init cleanup_highmap(void)
{
}
@@ -929,6 +936,8 @@ void __init setup_arch(char **cmdline_p)
/* max_pfn_mapped is updated here */
max_low_pfn_mapped = init_memory_mapping(0, max_low_pfn<<PAGE_SHIFT);
max_pfn_mapped = max_low_pfn_mapped;
+ /* after init_memory_mapping updating cr4*/
+ store_mmu_cr4();

#ifdef CONFIG_X86_64
if (max_pfn > max_low_pfn) {

2011-04-01 16:23:58

[permalink] [raw]

Subject: Re: 2.6.38.2 breaks suspend to disk

On 04/01/2011 09:06 AM, Yinghai Lu wrote:
> Index: linux-2.6/arch/x86/kernel/setup.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/kernel/setup.c
> +++ linux-2.6/arch/x86/kernel/setup.c
> @@ -294,10 +294,17 @@ static void __init init_gbpages(void)
> else
> direct_gbpages = 0;
> }
> +static void __init store_mmu_cr4(void)
> +{
> + mmu_cr4_features = read_cr4();
> +}
> #else
> static inline void init_gbpages(void)
> {
> }
> +static void __init store_mmu_cr4(void)
> +{
> +}
> static void __init cleanup_highmap(void)
> {
> }
> @@ -929,6 +936,8 @@ void __init setup_arch(char **cmdline_p)
> /* max_pfn_mapped is updated here */
> max_low_pfn_mapped = init_memory_mapping(0, max_low_pfn<<PAGE_SHIFT);
> max_pfn_mapped = max_low_pfn_mapped;
> + /* after init_memory_mapping updating cr4*/
> + store_mmu_cr4();
>
> #ifdef CONFIG_X86_64
> if (max_pfn > max_low_pfn) {

This looks really, really, really wrong.

Why the heck should we save and restore CR4 only for x86-64?

-hpa

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

2011-04-01 17:14:55

[permalink] [raw]

Subject: Re: 2.6.38.2 breaks suspend to disk

On 04/01/2011 09:22 AM, H. Peter Anvin wrote:
> On 04/01/2011 09:06 AM, Yinghai Lu wrote:
>> Index: linux-2.6/arch/x86/kernel/setup.c
>> ===================================================================
>> --- linux-2.6.orig/arch/x86/kernel/setup.c
>> +++ linux-2.6/arch/x86/kernel/setup.c
>> @@ -294,10 +294,17 @@ static void __init init_gbpages(void)
>> else
>> direct_gbpages = 0;
>> }
>> +static void __init store_mmu_cr4(void)
>> +{
>> + mmu_cr4_features = read_cr4();
>> +}
>> #else
>> static inline void init_gbpages(void)
>> {
>> }
>> +static void __init store_mmu_cr4(void)
>> +{
>> +}
>> static void __init cleanup_highmap(void)
>> {
>> }
>> @@ -929,6 +936,8 @@ void __init setup_arch(char **cmdline_p)
>> /* max_pfn_mapped is updated here */
>> max_low_pfn_mapped = init_memory_mapping(0, max_low_pfn<<PAGE_SHIFT);
>> max_pfn_mapped = max_low_pfn_mapped;
>> + /* after init_memory_mapping updating cr4*/
>> + store_mmu_cr4();
>>
>> #ifdef CONFIG_X86_64
>> if (max_pfn> max_low_pfn) {
>
> This looks really, really, really wrong.
>
> Why the heck should we save and restore CR4 only for x86-64?

Peter, this patch just restoring old sequence. If you think 32 bit should do same thing, we can do that in another patch.

Also after closing looking, Stefano's v1 patch should be ok too.
because init_memory_mapping is using set_in_cr4(...) and it will update mmu_cr4_features at the same time.

Thanks

Yinghai

2011-04-01 18:14:18

[permalink] [raw]

Subject: Re: 2.6.38.2 breaks suspend to disk

On Fri, 1 Apr 2011, H. Peter Anvin wrote:
> On 04/01/2011 09:06 AM, Yinghai Lu wrote:
> > Index: linux-2.6/arch/x86/kernel/setup.c
> > ===================================================================
> > --- linux-2.6.orig/arch/x86/kernel/setup.c
> > +++ linux-2.6/arch/x86/kernel/setup.c
> > @@ -294,10 +294,17 @@ static void __init init_gbpages(void)
> > else
> > direct_gbpages = 0;
> > }
> > +static void __init store_mmu_cr4(void)
> > +{
> > + mmu_cr4_features = read_cr4();
> > +}
> > #else
> > static inline void init_gbpages(void)
> > {
> > }
> > +static void __init store_mmu_cr4(void)
> > +{
> > +}
> > static void __init cleanup_highmap(void)
> > {
> > }
> > @@ -929,6 +936,8 @@ void __init setup_arch(char **cmdline_p)
> > /* max_pfn_mapped is updated here */
> > max_low_pfn_mapped = init_memory_mapping(0, max_low_pfn<<PAGE_SHIFT);
> > max_pfn_mapped = max_low_pfn_mapped;
> > + /* after init_memory_mapping updating cr4*/
> > + store_mmu_cr4();
> >
> > #ifdef CONFIG_X86_64
> > if (max_pfn > max_low_pfn) {
>
> This looks really, really, really wrong.
>
> Why the heck should we save and restore CR4 only for x86-64?

AFAICT it has always been the case since the beginning of time (Initial
git repository build).

2011-04-01 18:15:15

[permalink] [raw]

Subject: Re: 2.6.38.2 breaks suspend to disk

On Fri, 1 Apr 2011, Yinghai Lu wrote:
> Peter, this patch just restoring old sequence. If you think 32 bit should do same thing, we can do that in another patch.

indeed

> Also after closing looking, Stefano's v1 patch should be ok too.
> because init_memory_mapping is using set_in_cr4(...) and it will update mmu_cr4_features at the same time.

both patches are fine by me

2011-04-01 18:56:17

[permalink] [raw]

Subject: Re: 2.6.38.2 breaks suspend to disk

On 04/01/2011 11:14 AM, Stefano Stabellini wrote:
>> Why the heck should we save and restore CR4 only for x86-64?
>
> AFAICT it has always been the case since the beginning of time (Initial
> git repository build).

Doesn't change the fact that at least conceptually it's wrong; consider
bits like NX. Rafael, do you have any thoughts on this?

-hpa

2011-04-01 19:32:45

[permalink] [raw]

Subject: Re: 2.6.38.2 breaks suspend to disk

On 04/01/2011 11:55 AM, H. Peter Anvin wrote:
> On 04/01/2011 11:14 AM, Stefano Stabellini wrote:
>>> Why the heck should we save and restore CR4 only for x86-64?
>>
>> AFAICT it has always been the case since the beginning of time (Initial
>> git repository build).
>
> Doesn't change the fact that at least conceptually it's wrong; consider
> bits like NX. Rafael, do you have any thoughts on this?
>
more info:

in arch/x86/kernel/setup.c we have
#if !defined(CONFIG_X86_PAE) || defined(CONFIG_X86_64)
unsigned long mmu_cr4_features;
#else
unsigned long mmu_cr4_features = X86_CR4_PAE;
#endif

in arch/x86/kernel/head_32.S

/*
* New page tables may be in 4Mbyte page mode and may
* be using the global pages.
*
* NOTE! If we are on a 486 we may have no cr4 at all!
* So we do not try to touch it unless we really have
* some bits in it to set. This won't work if the BSP
* implements cr4 but this AP does not -- very unlikely
* but be warned! The same applies to the pse feature
* if not equally supported. --macro
*
* NOTE! We have to correct for the fact that we're
* not yet offset PAGE_OFFSET..
*/
#define cr4_bits pa(mmu_cr4_features)
movl cr4_bits,%edx
andl %edx,%edx
jz 6f
movl %cr4,%eax # Turn on paging options (PSE,PAE,..)
orl %edx,%eax
movl %eax,%cr4
...
6:

So for 32 bit, PAE support is not compiled in for old 486 cpus.
mmu_cr4_feautres will be used to make sure head_32.S will not access cr4.

that could be the reason why 32 bit does not do read back at beginning.

Thanks

Yinghai

2011-04-01 19:36:59

[permalink] [raw]

Subject: Re: 2.6.38.2 breaks suspend to disk

On 04/01/2011 12:32 PM, Yinghai Lu wrote:
>
> So for 32 bit, PAE support is not compiled in for old 486 cpus.
> mmu_cr4_feautres will be used to make sure head_32.S will not access cr4.
>
> that could be the reason why 32 bit does not do read back at beginning.
>

Yes, but that doesn't explain why we shouldn't set new bits like NX and
PSE in this register.

-hpa

2011-04-01 19:55:23

[permalink] [raw]

Subject: Re: 2.6.38.2 breaks suspend to disk

On 04/01/2011 12:36 PM, H. Peter Anvin wrote:
> On 04/01/2011 12:32 PM, Yinghai Lu wrote:
>>
>> So for 32 bit, PAE support is not compiled in for old 486 cpus.
>> mmu_cr4_feautres will be used to make sure head_32.S will not access cr4.
>>
>> that could be the reason why 32 bit does not do read back at beginning.
>>
>
> Yes, but that doesn't explain why we shouldn't set new bits like NX and
> PSE in this register.

ok, please check if you are happy with this one.

From: Stefano Stabellini <[email protected]>

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

Save cr4 to mmu_cr4_features at boot time

Michael reported 2.6.38.2 hibernation is broken by one backported patch.
it cause a freeze when resuming from hibernation

| "x86: Cleanup highmap after brk is concluded"
| commit id e5f15b45ddf3afa2bbbb10c7ea34fb32b6de0a0e.

it turns out the mmu_cr4 save it lost somehow.

-v3: read back cr4 for 32bit too according to HPA

Bisected-and-tested-by: Michael Leun <[email protected]>
Signed-off-by: Stefano Stabellini <[email protected]>
Signed-off-by: Yinghai Lu <[email protected]>

---
arch/x86/kernel/setup.c | 12 ++++++++++++
1 file changed, 12 insertions(+)

Index: linux-2.6/arch/x86/kernel/setup.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/setup.c
+++ linux-2.6/arch/x86/kernel/setup.c
@@ -219,6 +219,17 @@ unsigned long mmu_cr4_features;
unsigned long mmu_cr4_features = X86_CR4_PAE;
#endif

+#if defined(CONFIG_X86_PAE) || defined(CONFIG_X86_64)
+static void __init read_back_mmu_cr4(void)
+{
+ mmu_cr4_features = read_cr4();
+}
+#else
+static void __init read_back_mmu_cr4(void)
+{
+}
+#endif
+
/* Boot loader ID and version as integers, for the benefit of proc_dointvec */
int bootloader_type, bootloader_version;

@@ -892,6 +903,7 @@ void __init setup_arch(char **cmdline_p)

high_memory = (void *)__va(max_pfn * PAGE_SIZE - 1) + 1;
#endif
+ read_back_mmu_cr4();

/*
* Find and reserve possible boot-time SMP configuration:

2011-04-01 20:22:33

[permalink] [raw]

Subject: Re: 2.6.38.2 breaks suspend to disk

On 04/01/2011 12:54 PM, Yinghai Lu wrote:
>
> ok, please check if you are happy with this one.
>

The best would simply be:

mmu_cr4_features = read_cr4_safe();

If this has to run before we can handle exceptions, one can verify the
existence by testing for the CPUID instruction (a CPU has CR4 if and
only if it has CPUID):

if (boot_cpu_data.cpuid_level >= 0)
mmu_cr4_features = read_cr4_safe();

... since we set cpuid_level to -1 if there is no CPUID instruction.

-hpa

2011-04-01 21:24:55

[permalink] [raw]

Subject: Re: 2.6.38.2 breaks suspend to disk

On Fri, Apr 1, 2011 at 1:21 PM, H. Peter Anvin <[email protected]> wrote:
> On 04/01/2011 12:54 PM, Yinghai Lu wrote:
>>
>> ok, please check if you are happy with this one.
>>
>
> The best would simply be:
>
> ? ? ? ?mmu_cr4_features = read_cr4_safe();
>
> If this has to run before we can handle exceptions, one can verify the
> existence by testing for the CPUID instruction (a CPU has CR4 if and
> only if it has CPUID):
>
> ? ? ? ?if (boot_cpu_data.cpuid_level >= 0)
> ? ? ? ? ? ? ? ?mmu_cr4_features = read_cr4_safe();
>
> ... since we set cpuid_level to -1 if there is no CPUID instruction.

in that case could use read_cr4 directly.

please check attached -v4

Thanks

Attachments:

fix_store.patch (2.44 kB)

2011-04-01 21:31:04

[permalink] [raw]

Subject: Re: 2.6.38.2 breaks suspend to disk

On 04/01/2011 02:24 PM, Yinghai Lu wrote:
> On Fri, Apr 1, 2011 at 1:21 PM, H. Peter Anvin <[email protected]> wrote:
>> On 04/01/2011 12:54 PM, Yinghai Lu wrote:
>>>
>>> ok, please check if you are happy with this one.
>>>
>>
>> The best would simply be:
>>
>> mmu_cr4_features = read_cr4_safe();
>>
>> If this has to run before we can handle exceptions, one can verify the
>> existence by testing for the CPUID instruction (a CPU has CR4 if and
>> only if it has CPUID):
>>
>> if (boot_cpu_data.cpuid_level >= 0)
>> mmu_cr4_features = read_cr4_safe();
>>
>> ... since we set cpuid_level to -1 if there is no CPUID instruction.
>
> in that case could use read_cr4 directly.
>
> please check attached -v4
>

Err, yes, that's what I meant.

Now, why the heck did you introduce a bunch of CONFIG_HIBERNATION #ifdefs?

-hpa

2011-04-01 21:37:21

[permalink] [raw]

Subject: Re: 2.6.38.2 breaks suspend to disk

On Fri, Apr 1, 2011 at 2:30 PM, H. Peter Anvin <[email protected]> wrote:
> On 04/01/2011 02:24 PM, Yinghai Lu wrote:
>> On Fri, Apr 1, 2011 at 1:21 PM, H. Peter Anvin <[email protected]> wrote:
>>> On 04/01/2011 12:54 PM, Yinghai Lu wrote:
>>>>
>>>> ok, please check if you are happy with this one.
>>>>
>>>
>>> The best would simply be:
>>>
>>> ? ? ? ?mmu_cr4_features = read_cr4_safe();
>>>
>>> If this has to run before we can handle exceptions, one can verify the
>>> existence by testing for the CPUID instruction (a CPU has CR4 if and
>>> only if it has CPUID):
>>>
>>> ? ? ? ?if (boot_cpu_data.cpuid_level >= 0)
>>> ? ? ? ? ? ? ? ?mmu_cr4_features = read_cr4_safe();
>>>
>>> ... since we set cpuid_level to -1 if there is no CPUID instruction.
>>
>> in that case could use read_cr4 directly.
>>
>> please check attached -v4
>>
>
> Err, yes, that's what I meant.
>
> Now, why the heck did you introduce a bunch of CONFIG_HIBERNATION #ifdefs?

mmu_cr4_features: have two usages:
1. for 32bit to control cr4 access in head_32.S
2. for hibernation resume.

include/asm/processor.h:extern unsigned long mmu_cr4_features;
include/asm/processor.h: mmu_cr4_features |= mask;
include/asm/processor.h: mmu_cr4_features &= ~mask;
kernel/head_32.S:#define cr4_bits pa(mmu_cr4_features)
kernel/setup.c: * mmu_cr4_features two purpose:
kernel/setup.c:unsigned long mmu_cr4_features;
kernel/setup.c:unsigned long mmu_cr4_features = X86_CR4_PAE;
kernel/setup.c: mmu_cr4_features = read_cr4();
power/hibernate_asm_32.S: movl mmu_cr4_features, %ecx
power/hibernate_asm_32.S: movl mmu_cr4_features, %ecx
power/hibernate_asm_64.S: movq mmu_cr4_features(%rip), %rax
power/hibernate_asm_64.S: movq mmu_cr4_features(%rip), %rax

So we don't need to read back cr4 and save it if CONFIG_HIBERNATION is
not defined.

Thanks

Yinghai

2011-04-01 21:43:00

[permalink] [raw]

Subject: Re: 2.6.38.2 breaks suspend to disk

On 04/01/2011 02:37 PM, Yinghai Lu wrote:
>>
>> Err, yes, that's what I meant.
>>
>> Now, why the heck did you introduce a bunch of CONFIG_HIBERNATION #ifdefs?
>
> mmu_cr4_features: have two usages:
> 1. for 32bit to control cr4 access in head_32.S
> 2. for hibernation resume.
>
> include/asm/processor.h:extern unsigned long mmu_cr4_features;
> include/asm/processor.h: mmu_cr4_features |= mask;
> include/asm/processor.h: mmu_cr4_features &= ~mask;
> kernel/head_32.S:#define cr4_bits pa(mmu_cr4_features)
> kernel/setup.c: * mmu_cr4_features two purpose:
> kernel/setup.c:unsigned long mmu_cr4_features;
> kernel/setup.c:unsigned long mmu_cr4_features = X86_CR4_PAE;
> kernel/setup.c: mmu_cr4_features = read_cr4();
> power/hibernate_asm_32.S: movl mmu_cr4_features, %ecx
> power/hibernate_asm_32.S: movl mmu_cr4_features, %ecx
> power/hibernate_asm_64.S: movq mmu_cr4_features(%rip), %rax
> power/hibernate_asm_64.S: movq mmu_cr4_features(%rip), %rax
>
> So we don't need to read back cr4 and save it if CONFIG_HIBERNATION is
> not defined.
>

And why on Earth is it worth saving a couple of instructions (and
introducing code ugliness and a more complex testing matrix) in the case
when it is not?

-hpa

2011-04-01 23:05:43

[permalink] [raw]

Subject: Re: 2.6.38.2 breaks suspend to disk

On 04/01/2011 02:42 PM, H. Peter Anvin wrote:
>
> And why on Earth is it worth saving a couple of instructions (and
> introducing code ugliness and a more complex testing matrix) in the case
> when it is not?

Please check this one, it moves storing mmu_cr4 to arch_prepare_suspend.

Thanks

Yinghai

From: Stefano Stabellini <[email protected]>

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

Save cr4 to mmu_cr4_features at boot time

Michael reported 2.6.38.2 hibernation is broken by one backported patch.
it cause a freeze when resuming from hibernation

| "x86: Cleanup highmap after brk is concluded"
| commit id e5f15b45ddf3afa2bbbb10c7ea34fb32b6de0a0e.

it turns out the mmu_cr4 save it lost somehow.

-v3: read back cr4 for 32bit too according to HPA
-v4: use cpuid_level to check if we can read cr4 according to HPA
don't touch mmu_cr4_features if CONFIG_HIBERNATION is defined from Yinghai
-v5: reduce the using of CONFIG_HIBERNATION

Bisected-and-tested-by: Michael Leun <[email protected]>
Signed-off-by: Stefano Stabellini <[email protected]>
Signed-off-by: Yinghai Lu <[email protected]>

---
arch/x86/include/asm/processor.h | 2 --
arch/x86/include/asm/suspend_32.h | 2 +-
arch/x86/include/asm/suspend_64.h | 5 +----
arch/x86/kernel/setup.c | 16 +++++++++++++++-
4 files changed, 17 insertions(+), 8 deletions(-)

Index: linux-2.6/arch/x86/kernel/setup.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/setup.c
+++ linux-2.6/arch/x86/kernel/setup.c
@@ -212,12 +212,26 @@ struct cpuinfo_x86 boot_cpu_data __read_
EXPORT_SYMBOL(boot_cpu_data);
#endif

-
+/*
+ * mmu_cr4_features has two purposes:
+ * a. head_32.S will access cr4 according if X86_CR4_PAE is set in it.
+ * b. store read back cr4 for hibernation
+ */
#if !defined(CONFIG_X86_PAE) || defined(CONFIG_X86_64)
unsigned long mmu_cr4_features;
#else
unsigned long mmu_cr4_features = X86_CR4_PAE;
#endif
+#ifdef CONFIG_HIBERNATION
+int arch_prepare_suspend(void)
+{
+ /* a CPU has CR4 iff it has CPUID --- hpa */
+ if (boot_cpu_data.cpuid_level >= 0)
+ mmu_cr4_features = read_cr4();
+
+ return 0;
+}
+#endif

/* Boot loader ID and version as integers, for the benefit of proc_dointvec */
int bootloader_type, bootloader_version;
Index: linux-2.6/arch/x86/include/asm/processor.h
===================================================================
--- linux-2.6.orig/arch/x86/include/asm/processor.h
+++ linux-2.6/arch/x86/include/asm/processor.h
@@ -601,7 +601,6 @@ static inline void set_in_cr4(unsigned l
{
unsigned long cr4;

- mmu_cr4_features |= mask;
cr4 = read_cr4();
cr4 |= mask;
write_cr4(cr4);
@@ -611,7 +610,6 @@ static inline void clear_in_cr4(unsigned
{
unsigned long cr4;

- mmu_cr4_features &= ~mask;
cr4 = read_cr4();
cr4 &= ~mask;
write_cr4(cr4);
Index: linux-2.6/arch/x86/include/asm/suspend_32.h
===================================================================
--- linux-2.6.orig/arch/x86/include/asm/suspend_32.h
+++ linux-2.6/arch/x86/include/asm/suspend_32.h
@@ -9,7 +9,7 @@
#include <asm/desc.h>
#include <asm/i387.h>

-static inline int arch_prepare_suspend(void) { return 0; }
+int arch_prepare_suspend(void);

/* image of the saved processor state */
struct saved_context {
Index: linux-2.6/arch/x86/include/asm/suspend_64.h
===================================================================
--- linux-2.6.orig/arch/x86/include/asm/suspend_64.h
+++ linux-2.6/arch/x86/include/asm/suspend_64.h
@@ -9,10 +9,7 @@
#include <asm/desc.h>
#include <asm/i387.h>

-static inline int arch_prepare_suspend(void)
-{
- return 0;
-}
+int arch_prepare_suspend(void);

/*
* Image of the saved processor state, used by the low level ACPI suspend to

2011-04-01 23:13:17

[permalink] [raw]

Subject: Re: 2.6.38.2 breaks suspend to disk

On 04/01/2011 04:04 PM, Yinghai Lu wrote:
> On 04/01/2011 02:42 PM, H. Peter Anvin wrote:
>>
>> And why on Earth is it worth saving a couple of instructions (and
>> introducing code ugliness and a more complex testing matrix) in the case
>> when it is not?
>
> Please check this one, it moves storing mmu_cr4 to arch_prepare_suspend.
>

You keep moving things around instead of answering the question. It
might be the right thing to do, but I would like an answer why, in your
opinion, the easy way isn't feasible.

For suspend/resume, the right thing really is just to save CR4 like any
other processor register.

-hpa

2011-04-01 23:54:33

by Rafael J. Wysocki

[permalink] [raw]

Subject: Re: 2.6.38.2 breaks suspend to disk

On Friday, April 01, 2011, H. Peter Anvin wrote:
> On 04/01/2011 11:14 AM, Stefano Stabellini wrote:
> >> Why the heck should we save and restore CR4 only for x86-64?
> >
> > AFAICT it has always been the case since the beginning of time (Initial
> > git repository build).
>
> Doesn't change the fact that at least conceptually it's wrong; consider
> bits like NX. Rafael, do you have any thoughts on this?

I don't seem to remember that being intentional. I think we should do the
same on both x86s.

Thanks,
Rafael

2011-04-02 00:10:40

[permalink] [raw]

Subject: Re: 2.6.38.2 breaks suspend to disk

On 04/01/2011 04:12 PM, H. Peter Anvin wrote:
> On 04/01/2011 04:04 PM, Yinghai Lu wrote:
>> On 04/01/2011 02:42 PM, H. Peter Anvin wrote:
>>>
>>> And why on Earth is it worth saving a couple of instructions (and
>>> introducing code ugliness and a more complex testing matrix) in the case
>>> when it is not?
>>
>> Please check this one, it moves storing mmu_cr4 to arch_prepare_suspend.
>>
>
> You keep moving things around instead of answering the question. It
> might be the right thing to do, but I would like an answer why, in your
> opinion, the easy way isn't feasible.

want to find right place to read back cr4 for hibernation/resume.

it is one time using, so we could just read back one time at last and avoid touching it
inset/clear_in_cr4.

>
> For suspend/resume, the right thing really is just to save CR4 like any
> other processor register.

not sure why mmu_cr4_features get overloaded. maybe power guys want to save one variable instead of using restore_rc4.

Thanks

Yinghai

2011-04-06 20:28:35