Re: [RFC PATCH] x86: make sure wakeup code is below 1M

Re: [RFC PATCH] x86: make sure wakeup code is below 1M

[Rafael J. Wysocki]

On Monday 09 November 2009, Yinghai Lu wrote:
> 
> try to find_e820_area/reserve_early, and call acpi_reserve_memory early
> 
> to get area is below 1M
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>

I generally like it, two comments below.

> ---
>  arch/x86/include/asm/acpi.h  |    2 +-
>  arch/x86/kernel/acpi/sleep.c |   15 +++++++++------
>  arch/x86/kernel/setup.c      |   13 +++++++------
>  3 files changed, 17 insertions(+), 13 deletions(-)
> 
> Index: linux-2.6/arch/x86/include/asm/acpi.h
> ===================================================================
> --- linux-2.6.orig/arch/x86/include/asm/acpi.h
> +++ linux-2.6/arch/x86/include/asm/acpi.h
> @@ -118,7 +118,7 @@ extern void acpi_restore_state_mem(void)
>  extern unsigned long acpi_wakeup_address;
>  
>  /* early initialization routine */
> -extern void acpi_reserve_bootmem(void);
> +extern void acpi_reserve_memory(void);

If you change the name of the function, IMO it would be better to call it
something like acpi_reserve_wakeup_memory().  This way it would be clear what
the memory is reserved for.
  
>  /*
>   * Check if the CPU can handle C2 and deeper
> Index: linux-2.6/arch/x86/kernel/acpi/sleep.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/kernel/acpi/sleep.c
> +++ linux-2.6/arch/x86/kernel/acpi/sleep.c
> @@ -119,29 +119,32 @@ void acpi_restore_state_mem(void)
>  
>  
>  /**
> - * acpi_reserve_bootmem - do _very_ early ACPI initialisation
> + * acpi_reserve_memory - do _very_ early ACPI initialisation
>   *
>   * We allocate a page from the first 1MB of memory for the wakeup
>   * routine for when we come back from a sleep state. The
>   * runtime allocator allows specification of <16MB pages, but not
>   * <1MB pages.
>   */
> -void __init acpi_reserve_bootmem(void)
> +void __init acpi_reserve_memory(void)
>  {
> +	unsigned long mem;
> +
>  	if ((&wakeup_code_end - &wakeup_code_start) > WAKEUP_SIZE) {
>  		printk(KERN_ERR
>  		       "ACPI: Wakeup code way too big, S3 disabled.\n");
>  		return;
>  	}
>  
> -	acpi_realmode = (unsigned long)alloc_bootmem_low(WAKEUP_SIZE);
> +	mem = find_e820_area(0, 1<<20, WAKEUP_SIZE, PAGE_SIZE);
>  
> -	if (!acpi_realmode) {
> +	if (mem == -1L) {

Could we change the condition to avoid the "-1L" here?  It doesn't look very
nice.

>  		printk(KERN_ERR "ACPI: Cannot allocate lowmem, S3 disabled.\n");
>  		return;
>  	}
> -
> -	acpi_wakeup_address = virt_to_phys((void *)acpi_realmode);
> +	acpi_realmode = (unsigned long) phys_to_virt(mem);
> +	acpi_wakeup_address = mem;
> +	reserve_early(mem, mem + WAKEUP_SIZE, "WAKEUP");
>  }

Thanks,
Rafael

[PATCH] x86: make sure wakeup code is below 1M -v2

[Yinghai Lu]

try to find_e820_area/reserve_early, and call acpi_reserve_memory early

to get area is below 1M

-v2: change function name to acpi_reserve_wakeup_memory according to Rafael

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 arch/x86/include/asm/acpi.h  |    2 +-
 arch/x86/kernel/acpi/sleep.c |   15 +++++++++------
 arch/x86/kernel/setup.c      |   13 +++++++------
 3 files changed, 17 insertions(+), 13 deletions(-)

Index: linux-2.6/arch/x86/include/asm/acpi.h
===================================================================
--- linux-2.6.orig/arch/x86/include/asm/acpi.h
+++ linux-2.6/arch/x86/include/asm/acpi.h
@@ -118,7 +118,7 @@ extern void acpi_restore_state_mem(void)
 extern unsigned long acpi_wakeup_address;
 
 /* early initialization routine */
-extern void acpi_reserve_bootmem(void);
+extern void acpi_reserve_wakeup_memory(void);
 
 /*
  * Check if the CPU can handle C2 and deeper
Index: linux-2.6/arch/x86/kernel/acpi/sleep.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/acpi/sleep.c
+++ linux-2.6/arch/x86/kernel/acpi/sleep.c
@@ -119,29 +119,32 @@ void acpi_restore_state_mem(void)
 
 
 /**
- * acpi_reserve_bootmem - do _very_ early ACPI initialisation
+ * acpi_reserve_wakeup_memory - do _very_ early ACPI initialisation
  *
  * We allocate a page from the first 1MB of memory for the wakeup
  * routine for when we come back from a sleep state. The
  * runtime allocator allows specification of <16MB pages, but not
  * <1MB pages.
  */
-void __init acpi_reserve_bootmem(void)
+void __init acpi_reserve_wakeup_memory(void)
 {
+	unsigned long mem;
+
 	if ((&wakeup_code_end - &wakeup_code_start) > WAKEUP_SIZE) {
 		printk(KERN_ERR
 		       "ACPI: Wakeup code way too big, S3 disabled.\n");
 		return;
 	}
 
-	acpi_realmode = (unsigned long)alloc_bootmem_low(WAKEUP_SIZE);
+	mem = find_e820_area(0, 1<<20, WAKEUP_SIZE, PAGE_SIZE);
 
-	if (!acpi_realmode) {
+	if (mem == -1L) {
 		printk(KERN_ERR "ACPI: Cannot allocate lowmem, S3 disabled.\n");
 		return;
 	}
-
-	acpi_wakeup_address = virt_to_phys((void *)acpi_realmode);
+	acpi_realmode = (unsigned long) phys_to_virt(mem);
+	acpi_wakeup_address = mem;
+	reserve_early(mem, mem + WAKEUP_SIZE, "ACPI WAKEUP");
 }
 
 
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
@@ -905,6 +905,13 @@ void __init setup_arch(char **cmdline_p)
 
 	reserve_brk();
 
+#ifdef CONFIG_ACPI_SLEEP
+	/*
+	 * Reserve low memory region for sleep support.
+	 * even before init_memory_mapping
+	 */
+	acpi_reserve_wakeup_memory();
+#endif
 	init_gbpages();
 
 	/* max_pfn_mapped is updated here */
@@ -956,12 +963,6 @@ void __init setup_arch(char **cmdline_p)
 
 	initmem_init(0, max_pfn, acpi, k8);
 
-#ifdef CONFIG_ACPI_SLEEP
-	/*
-	 * Reserve low memory region for sleep support.
-	 */
-	acpi_reserve_bootmem();
-#endif
 	/*
 	 * Find and reserve possible boot-time SMP configuration:
 	 */

Re: [PATCH] x86: make sure wakeup code is below 1M -v2

[Ingo Molnar]

* Yinghai Lu <yinghai@kernel.org> wrote:

> 
> try to find_e820_area/reserve_early, and call acpi_reserve_memory early
> 
> to get area is below 1M
> 
> -v2: change function name to acpi_reserve_wakeup_memory according to Rafael
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> ---
>  arch/x86/include/asm/acpi.h  |    2 +-
>  arch/x86/kernel/acpi/sleep.c |   15 +++++++++------
>  arch/x86/kernel/setup.c      |   13 +++++++------
>  3 files changed, 17 insertions(+), 13 deletions(-)

Looks good. Rafael, Peter, does it look good to you too?

	Ingo

Re: [PATCH] x86: make sure wakeup code is below 1M -v2

[H. Peter Anvin]

On 11/10/2009 11:48 PM, Ingo Molnar wrote:
> 
> * Yinghai Lu <yinghai@kernel.org> wrote:
> 
>>
>> try to find_e820_area/reserve_early, and call acpi_reserve_memory early
>>
>> to get area is below 1M
>>
>> -v2: change function name to acpi_reserve_wakeup_memory according to Rafael
>>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>
>> ---
>>  arch/x86/include/asm/acpi.h  |    2 +-
>>  arch/x86/kernel/acpi/sleep.c |   15 +++++++++------
>>  arch/x86/kernel/setup.c      |   13 +++++++------
>>  3 files changed, 17 insertions(+), 13 deletions(-)
> 
> Looks good. Rafael, Peter, does it look good to you too?
> 

Looks sane to me.

Acked-by: H. Peter Anvin <hpa@zytor.com>

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

Re: [PATCH] x86: make sure wakeup code is below 1M -v2

[Rafael J. Wysocki]

On Wednesday 11 November 2009, Ingo Molnar wrote:
> 
> * Yinghai Lu <yinghai@kernel.org> wrote:
> 
> > 
> > try to find_e820_area/reserve_early, and call acpi_reserve_memory early
> > 
> > to get area is below 1M
> > 
> > -v2: change function name to acpi_reserve_wakeup_memory according to Rafael
> > 
> > Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> > 
> > ---
> >  arch/x86/include/asm/acpi.h  |    2 +-
> >  arch/x86/kernel/acpi/sleep.c |   15 +++++++++------
> >  arch/x86/kernel/setup.c      |   13 +++++++------
> >  3 files changed, 17 insertions(+), 13 deletions(-)
> 
> Looks good. Rafael, Peter, does it look good to you too?

Yes, it does.

Acked-by: Rafael J. Wysocki <rjw@sisk.pl>

Re: [PATCH] x86: make sure wakeup code is below 1M -v2

[ykzhao]

On Wed, 2009-11-11 at 10:27 +0800, Yinghai Lu wrote:
> try to find_e820_area/reserve_early, and call acpi_reserve_memory early
> 
> to get area is below 1M
> 
> -v2: change function name to acpi_reserve_wakeup_memory according to Rafael
It seems that the function of find_e820_area is called in several
places.
   >Initmem_init: bootmap = find_e820_area(0, end_pfn<<PAGE_SHIFT,
bootmap_size, PAGE_SIZE);
   
If we also call it in the acpi_reserve_wakeup_memory, do we get the same
base address as that obtained in initmem_init?

Thanks.

> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> ---
>  arch/x86/include/asm/acpi.h  |    2 +-
>  arch/x86/kernel/acpi/sleep.c |   15 +++++++++------
>  arch/x86/kernel/setup.c      |   13 +++++++------
>  3 files changed, 17 insertions(+), 13 deletions(-)
> 
> Index: linux-2.6/arch/x86/include/asm/acpi.h
> ===================================================================
> --- linux-2.6.orig/arch/x86/include/asm/acpi.h
> +++ linux-2.6/arch/x86/include/asm/acpi.h
> @@ -118,7 +118,7 @@ extern void acpi_restore_state_mem(void)
>  extern unsigned long acpi_wakeup_address;
>  
>  /* early initialization routine */
> -extern void acpi_reserve_bootmem(void);
> +extern void acpi_reserve_wakeup_memory(void);
>  
>  /*
>   * Check if the CPU can handle C2 and deeper
> Index: linux-2.6/arch/x86/kernel/acpi/sleep.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/kernel/acpi/sleep.c
> +++ linux-2.6/arch/x86/kernel/acpi/sleep.c
> @@ -119,29 +119,32 @@ void acpi_restore_state_mem(void)
>  
> 
>  /**
> - * acpi_reserve_bootmem - do _very_ early ACPI initialisation
> + * acpi_reserve_wakeup_memory - do _very_ early ACPI initialisation
>   *
>   * We allocate a page from the first 1MB of memory for the wakeup
>   * routine for when we come back from a sleep state. The
>   * runtime allocator allows specification of <16MB pages, but not
>   * <1MB pages.
>   */
> -void __init acpi_reserve_bootmem(void)
> +void __init acpi_reserve_wakeup_memory(void)
>  {
> +	unsigned long mem;
> +
>  	if ((&wakeup_code_end - &wakeup_code_start) > WAKEUP_SIZE) {
>  		printk(KERN_ERR
>  		       "ACPI: Wakeup code way too big, S3 disabled.\n");
>  		return;
>  	}
>  
> -	acpi_realmode = (unsigned long)alloc_bootmem_low(WAKEUP_SIZE);
> +	mem = find_e820_area(0, 1<<20, WAKEUP_SIZE, PAGE_SIZE);
>  
> -	if (!acpi_realmode) {
> +	if (mem == -1L) {
>  		printk(KERN_ERR "ACPI: Cannot allocate lowmem, S3 disabled.\n");
>  		return;
>  	}
> -
> -	acpi_wakeup_address = virt_to_phys((void *)acpi_realmode);
> +	acpi_realmode = (unsigned long) phys_to_virt(mem);
> +	acpi_wakeup_address = mem;
> +	reserve_early(mem, mem + WAKEUP_SIZE, "ACPI WAKEUP");
>  }
>  
> 
> 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
> @@ -905,6 +905,13 @@ void __init setup_arch(char **cmdline_p)
>  
>  	reserve_brk();
>  
> +#ifdef CONFIG_ACPI_SLEEP
> +	/*
> +	 * Reserve low memory region for sleep support.
> +	 * even before init_memory_mapping
> +	 */
> +	acpi_reserve_wakeup_memory();
> +#endif
>  	init_gbpages();
>  
>  	/* max_pfn_mapped is updated here */
> @@ -956,12 +963,6 @@ void __init setup_arch(char **cmdline_p)
>  
>  	initmem_init(0, max_pfn, acpi, k8);
>  
> -#ifdef CONFIG_ACPI_SLEEP
> -	/*
> -	 * Reserve low memory region for sleep support.
> -	 */
> -	acpi_reserve_bootmem();
> -#endif
>  	/*
>  	 * Find and reserve possible boot-time SMP configuration:
>  	 */
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] x86: make sure wakeup code is below 1M -v2

[Yinghai Lu]

ykzhao wrote:
> On Wed, 2009-11-11 at 10:27 +0800, Yinghai Lu wrote:
>> try to find_e820_area/reserve_early, and call acpi_reserve_memory early
>>
>> to get area is below 1M
>>
>> -v2: change function name to acpi_reserve_wakeup_memory according to Rafael
> It seems that the function of find_e820_area is called in several
> places.
>    >Initmem_init: bootmap = find_e820_area(0, end_pfn<<PAGE_SHIFT,
> bootmap_size, PAGE_SIZE);
>    
> If we also call it in the acpi_reserve_wakeup_memory, do we get the same
> base address as that obtained in initmem_init?

no. find_e820_area will check the reserve res array that could be updated by reserve_early.

YH

Re: [PATCH] x86: make sure wakeup code is below 1M -v2

[ykzhao]

On Thu, 2009-11-12 at 03:05 +0800, Yinghai Lu wrote:
> ykzhao wrote:
> > On Wed, 2009-11-11 at 10:27 +0800, Yinghai Lu wrote:
> >> try to find_e820_area/reserve_early, and call acpi_reserve_memory early
> >>
> >> to get area is below 1M
> >>
> >> -v2: change function name to acpi_reserve_wakeup_memory according to Rafael
> > It seems that the function of find_e820_area is called in several
> > places.
> >    >Initmem_init: bootmap = find_e820_area(0, end_pfn<<PAGE_SHIFT,
> > bootmap_size, PAGE_SIZE);
> >    
> > If we also call it in the acpi_reserve_wakeup_memory, do we get the same
> > base address as that obtained in initmem_init?
> 
> no. find_e820_area will check the reserve res array that could be updated by reserve_early.
It will check the reserved region array when calling the function of
find_e820_area. 
But it seems that the array is not updated when the find_e820_area is
called in the function of initmem_init.

At the same time the  acpi_reserve_bootmem is called after initializing
the boot mem allocator. Can we continue to reserve some region by using
find_e820_area?

thanks.
> 
> YH

Re: [PATCH] x86: make sure wakeup code is below 1M -v2

[Yinghai Lu]

ykzhao wrote:
> On Thu, 2009-11-12 at 03:05 +0800, Yinghai Lu wrote:
>> ykzhao wrote:
>>> On Wed, 2009-11-11 at 10:27 +0800, Yinghai Lu wrote:
>>>> try to find_e820_area/reserve_early, and call acpi_reserve_memory early
>>>>
>>>> to get area is below 1M
>>>>
>>>> -v2: change function name to acpi_reserve_wakeup_memory according to Rafael
>>> It seems that the function of find_e820_area is called in several
>>> places.
>>>    >Initmem_init: bootmap = find_e820_area(0, end_pfn<<PAGE_SHIFT,
>>> bootmap_size, PAGE_SIZE);
>>>    
>>> If we also call it in the acpi_reserve_wakeup_memory, do we get the same
>>> base address as that obtained in initmem_init?
>> no. find_e820_area will check the reserve res array that could be updated by reserve_early.
> It will check the reserved region array when calling the function of
> find_e820_area. 
> But it seems that the array is not updated when the find_e820_area is
> called in the function of initmem_init.

right after that will use reserve_bootmem for those range in initmem_init.

also we could reserve_early there and let bootmem conversion to do that for us.

but for numa there is some chance to use other node bootmem to for that bootdata and bitmap.
so make it simple just use reserve_bootmem there.

YH

Re: [PATCH] x86: make sure wakeup code is below 1M -v2

[ykzhao]

On Thu, 2009-11-12 at 11:12 +0800, Yinghai Lu wrote:
> ykzhao wrote:
> > On Thu, 2009-11-12 at 03:05 +0800, Yinghai Lu wrote:
> >> ykzhao wrote:
> >>> On Wed, 2009-11-11 at 10:27 +0800, Yinghai Lu wrote:
> >>>> try to find_e820_area/reserve_early, and call acpi_reserve_memory early
> >>>>
> >>>> to get area is below 1M
> >>>>
> >>>> -v2: change function name to acpi_reserve_wakeup_memory according to Rafael
> >>> It seems that the function of find_e820_area is called in several
> >>> places.
> >>>    >Initmem_init: bootmap = find_e820_area(0, end_pfn<<PAGE_SHIFT,
> >>> bootmap_size, PAGE_SIZE);
> >>>    
> >>> If we also call it in the acpi_reserve_wakeup_memory, do we get the same
> >>> base address as that obtained in initmem_init?
> >> no. find_e820_area will check the reserve res array that could be updated by reserve_early.
> > It will check the reserved region array when calling the function of
> > find_e820_area. 
> > But it seems that the array is not updated when the find_e820_area is
> > called in the function of initmem_init.
> 
> right after that will use reserve_bootmem for those range in initmem_init.
Yes. The reserve_bootmem is called for the range in initmem_init.
But the reserved_early array is not updated.
> 
> also we could reserve_early there and let bootmem conversion to do that for us.
> 
> but for numa there is some chance to use other node bootmem to for that bootdata and bitmap.
> so make it simple just use reserve_bootmem there.
> 
> YH

Re: [PATCH] x86: make sure wakeup code is below 1M -v2

[Yinghai Lu]

ykzhao wrote:
> On Thu, 2009-11-12 at 11:12 +0800, Yinghai Lu wrote:
>> ykzhao wrote:
>>> On Thu, 2009-11-12 at 03:05 +0800, Yinghai Lu wrote:
>>>> ykzhao wrote:
>>>>> On Wed, 2009-11-11 at 10:27 +0800, Yinghai Lu wrote:
>>>>>> try to find_e820_area/reserve_early, and call acpi_reserve_memory early
>>>>>>
>>>>>> to get area is below 1M
>>>>>>
>>>>>> -v2: change function name to acpi_reserve_wakeup_memory according to Rafael
>>>>> It seems that the function of find_e820_area is called in several
>>>>> places.
>>>>>    >Initmem_init: bootmap = find_e820_area(0, end_pfn<<PAGE_SHIFT,
>>>>> bootmap_size, PAGE_SIZE);
>>>>>    
>>>>> If we also call it in the acpi_reserve_wakeup_memory, do we get the same
>>>>> base address as that obtained in initmem_init?
>>>> no. find_e820_area will check the reserve res array that could be updated by reserve_early.
>>> It will check the reserved region array when calling the function of
>>> find_e820_area. 
>>> But it seems that the array is not updated when the find_e820_area is
>>> called in the function of initmem_init.
>> right after that will use reserve_bootmem for those range in initmem_init.
> Yes. The reserve_bootmem is called for the range in initmem_init.
> But the reserved_early array is not updated.

it is not needed anymore because bootmem for that node is ready at that point, could use 
reserve_bootmem_node directly. and before that early_res_to_bootmem will convert that early resource that 
fall into that node range to bootmem reserved too.

please check code setup_node_bootmem/early_node_mem/early_res_to_bootmem ...and reserve_bootmem_node...

YH

Re: [PATCH] x86: make sure wakeup code is below 1M -v2

[ykzhao]

On Thu, 2009-11-12 at 13:21 +0800, Yinghai Lu wrote:
> ykzhao wrote:
> > On Thu, 2009-11-12 at 11:12 +0800, Yinghai Lu wrote:
> >> ykzhao wrote:
> >>> On Thu, 2009-11-12 at 03:05 +0800, Yinghai Lu wrote:
> >>>> ykzhao wrote:
> >>>>> On Wed, 2009-11-11 at 10:27 +0800, Yinghai Lu wrote:
> >>>>>> try to find_e820_area/reserve_early, and call acpi_reserve_memory early
> >>>>>>
> >>>>>> to get area is below 1M
> >>>>>>
> >>>>>> -v2: change function name to acpi_reserve_wakeup_memory according to Rafael
> >>>>> It seems that the function of find_e820_area is called in several
> >>>>> places.
> >>>>>    >Initmem_init: bootmap = find_e820_area(0, end_pfn<<PAGE_SHIFT,
> >>>>> bootmap_size, PAGE_SIZE);
> >>>>>    
> >>>>> If we also call it in the acpi_reserve_wakeup_memory, do we get the same
> >>>>> base address as that obtained in initmem_init?
> >>>> no. find_e820_area will check the reserve res array that could be updated by reserve_early.
> >>> It will check the reserved region array when calling the function of
> >>> find_e820_area. 
> >>> But it seems that the array is not updated when the find_e820_area is
> >>> called in the function of initmem_init.
> >> right after that will use reserve_bootmem for those range in initmem_init.
> > Yes. The reserve_bootmem is called for the range in initmem_init.
> > But the reserved_early array is not updated.
> 
> it is not needed anymore because bootmem for that node is ready at that point, could use 
> reserve_bootmem_node directly. and before that early_res_to_bootmem will convert that early resource that 
> fall into that node range to bootmem reserved too.
After the bootmem allocator is initialized, the early_res_to_bootmem
will convert the resource in reserved_early array and reserve them in
the bootmem. 

In this patch the find_e820_area is used in the
acpi_reserve_wakeup_memory, which is called after initializing the
bootmem allocator.
Can we still use the find_e820_area after the bootmem allocator is
initialized? 
It seems that the bootmem bitmap is also found by using the
find_e820_area. But we don't update the reserved_early array any more.
Maybe we will get the overlap address with the bootmem bitmap for the
wakeup code.

thanks.
> 
> please check code setup_node_bootmem/early_node_mem/early_res_to_bootmem ...and reserve_bootmem_node...

> 
> YH

Re: [PATCH] x86: make sure wakeup code is below 1M -v2

[H. Peter Anvin]

On 11/11/2009 11:14 PM, ykzhao wrote:
> 
> In this patch the find_e820_area is used in the
> acpi_reserve_wakeup_memory, which is called after initializing the
> bootmem allocator.
> Can we still use the find_e820_area after the bootmem allocator is
> initialized? 

Obviously not: the bootmem allocator now thinks it owns memory, and it
wouldn't know not to allocate something that is later claimed by
find_e820_area.  Problem.

> It seems that the bootmem bitmap is also found by using the
> find_e820_area. But we don't update the reserved_early array any more.
> Maybe we will get the overlap address with the bootmem bitmap for the
> wakeup code.

Not just the bitmap, but any bootmem allocation could conflict...

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

Re: [PATCH] x86: make sure wakeup code is below 1M -v2

[ykzhao]

On Thu, 2009-11-12 at 13:21 +0800, Yinghai Lu wrote:
> ykzhao wrote:
> > On Thu, 2009-11-12 at 11:12 +0800, Yinghai Lu wrote:
> >> ykzhao wrote:
> >>> On Thu, 2009-11-12 at 03:05 +0800, Yinghai Lu wrote:
> >>>> ykzhao wrote:
> >>>>> On Wed, 2009-11-11 at 10:27 +0800, Yinghai Lu wrote:
> >>>>>> try to find_e820_area/reserve_early, and call acpi_reserve_memory early
> >>>>>>
> >>>>>> to get area is below 1M
> >>>>>>
> >>>>>> -v2: change function name to acpi_reserve_wakeup_memory according to Rafael
> >>>>> It seems that the function of find_e820_area is called in several
> >>>>> places.
> >>>>>    >Initmem_init: bootmap = find_e820_area(0, end_pfn<<PAGE_SHIFT,
> >>>>> bootmap_size, PAGE_SIZE);
> >>>>>    
> >>>>> If we also call it in the acpi_reserve_wakeup_memory, do we get the same
> >>>>> base address as that obtained in initmem_init?
> >>>> no. find_e820_area will check the reserve res array that could be updated by reserve_early.
> >>> It will check the reserved region array when calling the function of
> >>> find_e820_area. 
> >>> But it seems that the array is not updated when the find_e820_area is
> >>> called in the function of initmem_init.
> >> right after that will use reserve_bootmem for those range in initmem_init.
> > Yes. The reserve_bootmem is called for the range in initmem_init.
> > But the reserved_early array is not updated.
> 
> it is not needed anymore because bootmem for that node is ready at that point, could use 
> reserve_bootmem_node directly. and before that early_res_to_bootmem will convert that early resource that 
> fall into that node range to bootmem reserved too.
> 
> please check code setup_node_bootmem/early_node_mem/early_res_to_bootmem ...and reserve_bootmem_node...
Sorry that I don't notice that the function
of acpi_reserve_wakeup_memory is also moved early before initializing
the bootmem allocator.
If so, there is no problem.

thanks.
> 
> YH

Re: [PATCH] x86: make sure wakeup code is below 1M -v2

[Pavel Machek]

On Tue 2009-11-10 18:27:23, Yinghai Lu wrote:
> 
> try to find_e820_area/reserve_early, and call acpi_reserve_memory early
> 
> to get area is below 1M

Does it fix anything? I can't tell from the changelog...

> Signed-off-by: Yinghai Lu <yinghai@kernel.org>

Please CC me on suspend patches.
>  /*
>   * Check if the CPU can handle C2 and deeper
> Index: linux-2.6/arch/x86/kernel/acpi/sleep.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/kernel/acpi/sleep.c
> +++ linux-2.6/arch/x86/kernel/acpi/sleep.c
> @@ -119,29 +119,32 @@ void acpi_restore_state_mem(void)
>  
>  
>  /**
> - * acpi_reserve_bootmem - do _very_ early ACPI initialisation
> + * acpi_reserve_wakeup_memory - do _very_ early ACPI initialisation
>   *
>   * We allocate a page from the first 1MB of memory for the wakeup
>   * routine for when we come back from a sleep state. The
>   * runtime allocator allows specification of <16MB pages, but not
>   * <1MB pages.
>   */
> -void __init acpi_reserve_bootmem(void)
> +void __init acpi_reserve_wakeup_memory(void)
>  {
> +	unsigned long mem;
> +
>  	if ((&wakeup_code_end - &wakeup_code_start) > WAKEUP_SIZE) {
>  		printk(KERN_ERR
>  		       "ACPI: Wakeup code way too big, S3 disabled.\n");
>  		return;
>  	}
>  
> -	acpi_realmode = (unsigned long)alloc_bootmem_low(WAKEUP_SIZE);
> +	mem = find_e820_area(0, 1<<20, WAKEUP_SIZE, PAGE_SIZE);
>  
> -	if (!acpi_realmode) {
> +	if (mem == -1L) {
>  		printk(KERN_ERR "ACPI: Cannot allocate lowmem, S3 disabled.\n");
>  		return;
>  	}

How is it better then old code?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] x86: make sure wakeup code is below 1M -v2

[Yinghai Lu]

Pavel Machek wrote:
> On Tue 2009-11-10 18:27:23, Yinghai Lu wrote:
>> try to find_e820_area/reserve_early, and call acpi_reserve_memory early
>>
>> to get area is below 1M
> 
>> -	acpi_realmode = (unsigned long)alloc_bootmem_low(WAKEUP_SIZE);
>> +	mem = find_e820_area(0, 1<<20, WAKEUP_SIZE, PAGE_SIZE);
>>  
>> -	if (!acpi_realmode) {
>> +	if (mem == -1L) {
>>  		printk(KERN_ERR "ACPI: Cannot allocate lowmem, S3 disabled.\n");
>>  		return;
>>  	}
> 
> How is it better then old code?

could get range below 1M.

alloc_bootmem_low can not make sure get that below 1M.

init_memory_mapping could use below 1M as page table...

so need to get some buffer for acpi wakeup realmode code.

YH

Re: [PATCH] x86: make sure wakeup code is below 1M -v2

[Rafael J. Wysocki]

On Thursday 12 November 2009, Pavel Machek wrote:
> On Tue 2009-11-10 18:27:23, Yinghai Lu wrote:
> > 
> > try to find_e820_area/reserve_early, and call acpi_reserve_memory early
> > 
> > to get area is below 1M
> 
> Does it fix anything? I can't tell from the changelog...

Yes, it does.  If the area is above 1M, we can't run real mode code from it.

This matters for NUMA machines supporting S3 (yes, there are a few models
out there).

Thanks,
Rafael

Re: [PATCH] x86: make sure wakeup code is below 1M -v2

[Ingo Molnar]

* Rafael J. Wysocki <rjw@sisk.pl> wrote:

> On Thursday 12 November 2009, Pavel Machek wrote:
> > On Tue 2009-11-10 18:27:23, Yinghai Lu wrote:
> > > 
> > > try to find_e820_area/reserve_early, and call acpi_reserve_memory early
> > > 
> > > to get area is below 1M
> > 
> > Does it fix anything? I can't tell from the changelog...
> 
> Yes, it does.  If the area is above 1M, we can't run real mode code 
> from it.
> 
> This matters for NUMA machines supporting S3 (yes, there are a few 
> models out there).

It might also matter in more exotic kernels that might do some large 
allocations. (say 'nopentium' can cause +1MB/+2MB of kernel page table 
allocations per one GB of RAM)

Physical constraints to allocations (which this is) should be expressed 
early and with high priority, to make sure they can be met.

	Ingo

Re: [PATCH] x86: make sure wakeup code is below 1M -v2

[H. Peter Anvin]

On 11/12/2009 11:36 PM, Ingo Molnar wrote:
> 
> Physical constraints to allocations (which this is) should be expressed 
> early and with high priority, to make sure they can be met.
> 

Indeed, and in this case this means before bootmem allocator initialization.

	-hpa

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

Re: [PATCH] x86: make sure wakeup code is below 1M -v2

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> wrote:

> On 11/12/2009 11:36 PM, Ingo Molnar wrote:
> > 
> > Physical constraints to allocations (which this is) should be expressed 
> > early and with high priority, to make sure they can be met.
> > 
> 
> Indeed, and in this case this means before bootmem allocator initialization.

The other advantage of Yinghai's patch is that it removes a bootmem 
usage from x86, which moves us a bit forward on the road to remove the 
bootmem allocator altogether and go from early allocators to slab 
straight away.

	Ingo