[PATCH] check that acpi_wakeup_address is below 1MB v2

[PATCH] check that acpi_wakeup_address is below 1MB v2

[matthieu castet]

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

Hi,

this patch add a check that the memory allocated for s3 wakeup is in the 
first 1MB as required by acpi spec.


Signed-off-by: "Matthieu CASTET <castet.matthieu@free.fr>"


[-- Attachment #2: acpi_alloc_s3_mem.diff --]
[-- Type: text/x-diff, Size: 683 bytes --]

diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 6bc815c..6636859 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -65,6 +65,13 @@ void __init acpi_reserve_bootmem(void)
 	acpi_wakeup_address = (unsigned long)alloc_bootmem_low(PAGE_SIZE*2);
 	if (!acpi_wakeup_address)
 		printk(KERN_ERR "ACPI: Cannot allocate lowmem, S3 disabled.\n");
+	/* check if we are in first 1MB of memory */
+	if (__pa(acpi_wakeup_address) >= 1024*1024-PAGE_SIZE*2) {
+		printk(KERN_ERR "ACPI: Cannot allocate lowmem in first 1MB,"
+				"S3 disabled.\n");
+		free_bootmem(acpi_wakeup_address, PAGE_SIZE*2);
+		acpi_wakeup_address = 0;
+	}
 }
 
 

Re: [PATCH] check that acpi_wakeup_address is below 1MB v2

[Zhao Yakui]

On Thu, 2008-04-10 at 23:33 +0200, matthieu castet wrote:
> Hi,
> 
> this patch add a check that the memory allocated for s3 wakeup is in the 
> first 1MB as required by acpi spec.
> 
Have you found whether some systems are afflicted by this ?

In fact that OS allacates the acpi_wake_address follows the below two
functions:
reserve_bootmem(0, PAGE_SIZE, BOOTMEM_DEFAULT);
reserve_bootmem(PAGE_SIZE, PAGE_SIZE, BOOTMEM_DEFAULT);

The above mechanism can insure that the acpi_wake_address is in memroy
below 1M. (It is unncessary to apply the patch. IMO)

Of course the addressing check will be more strict after this patch is
applied.

Best regards
   Yakui
> 
> Signed-off-by: "Matthieu CASTET <castet.matthieu@free.fr>"
> 

Re: [PATCH] check that acpi_wakeup_address is below 1MB v2

[Rafael J. Wysocki]

On Friday, 11 of April 2008, Zhao Yakui wrote:
> On Thu, 2008-04-10 at 23:33 +0200, matthieu castet wrote:
> > Hi,
> > 
> > this patch add a check that the memory allocated for s3 wakeup is in the 
> > first 1MB as required by acpi spec.
> > 
> Have you found whether some systems are afflicted by this ?
> 
> In fact that OS allacates the acpi_wake_address follows the below two
> functions:
> reserve_bootmem(0, PAGE_SIZE, BOOTMEM_DEFAULT);
> reserve_bootmem(PAGE_SIZE, PAGE_SIZE, BOOTMEM_DEFAULT);
> 
> The above mechanism can insure that the acpi_wake_address is in memroy
> below 1M. (It is unncessary to apply the patch. IMO)

Agreed.
 
> Of course the addressing check will be more strict after this patch is
> applied.

Matthieu, can you please explain why you consider the patch as necessary?

Thanks,
Rafael

Re: [PATCH] check that acpi_wakeup_address is below 1MB v2

[matthieu castet]

Hi,

Rafael J. Wysocki wrote:
> On Friday, 11 of April 2008, Zhao Yakui wrote:
>> On Thu, 2008-04-10 at 23:33 +0200, matthieu castet wrote:
>>> Hi,
>>>
>>> this patch add a check that the memory allocated for s3 wakeup is in the 
>>> first 1MB as required by acpi spec.
>>>
>> Have you found whether some systems are afflicted by this ?
>>
>> In fact that OS allacates the acpi_wake_address follows the below two
>> functions:
>> reserve_bootmem(0, PAGE_SIZE, BOOTMEM_DEFAULT);
>> reserve_bootmem(PAGE_SIZE, PAGE_SIZE, BOOTMEM_DEFAULT);
>>
That's on 32 bits kernel. Aren't the 1MB limitation valid on 64 bits 
kernel ?

>> The above mechanism can insure that the acpi_wake_address is in memroy
>> below 1M. (It is unncessary to apply the patch. IMO)
> 
> Agreed.
>  
>> Of course the addressing check will be more strict after this patch is
>> applied.
> 
> Matthieu, can you please explain why you consider the patch as necessary?
This can help to debug strange S3 wakeup problem. It make sure the 
allocation is correct.

Also I believe relying on the bootmem allocator internal to be sure it 
is in the first 1MB is a bad things (why can't it start allocating 
memory to the top of 16MB memory).
If you are sure of the allocated memory before the call, why don't do a
reserve_bootmem(PAGE_SIZE*2, PAGE_SIZE*2, BOOTMEM_DEFAULT); ?


Matthieu