[patch] hibernation: utilize ACPI hardware signature

[patch] hibernation: utilize ACPI hardware signature

[Shaohua Li]

ACPI defines a hardware signature. BIOS calculates the signature
according to hardware configure, if hardware changes, the signature will
change, in this case, S4 resume should fail.

Signed-off-by: Shaohua Li <shaohua.li@intel.com>
---
 drivers/acpi/sleep/main.c |   13 +++++++++++++
 include/linux/suspend.h   |    5 +++++
 kernel/power/disk.c       |   21 ++++++++++++++++++---
 kernel/power/power.h      |    6 +++---
 kernel/power/swap.c       |   24 +++++++++++++++---------
 5 files changed, 54 insertions(+), 15 deletions(-)

Index: linux/drivers/acpi/sleep/main.c
===================================================================
--- linux.orig/drivers/acpi/sleep/main.c	2008-01-02 14:04:41.000000000 +0800
+++ linux/drivers/acpi/sleep/main.c	2008-01-02 14:44:06.000000000 +0800
@@ -295,6 +295,18 @@ static void acpi_hibernation_restore_cle
 	acpi_hw_enable_all_runtime_gpes();
 }
 
+static unsigned long acpi_hibernation_hardware_signature(void)
+{
+	acpi_status status;
+	struct acpi_table_facs *facs;
+
+	status = acpi_get_table_by_index(ACPI_TABLE_INDEX_FACS,
+		(struct acpi_table_header **)&facs);
+	if (ACPI_FAILURE(status))
+		return 0;
+	return facs->hardware_signature;
+}
+
 static struct platform_hibernation_ops acpi_hibernation_ops = {
 	.start = acpi_hibernation_start,
 	.pre_snapshot = acpi_hibernation_prepare,
@@ -304,6 +316,7 @@ static struct platform_hibernation_ops a
 	.leave = acpi_hibernation_leave,
 	.pre_restore = acpi_hibernation_pre_restore,
 	.restore_cleanup = acpi_hibernation_restore_cleanup,
+	.hardware_signature = acpi_hibernation_hardware_signature,
 };
 #endif				/* CONFIG_HIBERNATION */
 
Index: linux/include/linux/suspend.h
===================================================================
--- linux.orig/include/linux/suspend.h	2008-01-02 14:04:41.000000000 +0800
+++ linux/include/linux/suspend.h	2008-01-02 14:33:34.000000000 +0800
@@ -169,6 +169,10 @@ extern void mark_free_pages(struct zone 
  * @restore_cleanup: Clean up after a failing image restoration.
  *	Called right after the nonboot CPUs have been enabled and before
  *	thawing devices (runs with IRQs on).
+ *
+ * @hardware_signature: Hardware signature in the platform.
+ *	If platform hardware changes, the signature will change. Hibernation
+ *	will check if the signature before/after a suspend matches.
  */
 struct platform_hibernation_ops {
 	int (*start)(void);
@@ -179,6 +183,7 @@ struct platform_hibernation_ops {
 	void (*leave)(void);
 	int (*pre_restore)(void);
 	void (*restore_cleanup)(void);
+	unsigned long (*hardware_signature)(void);
 };
 
 #ifdef CONFIG_HIBERNATION
Index: linux/kernel/power/disk.c
===================================================================
--- linux.orig/kernel/power/disk.c	2008-01-02 14:04:41.000000000 +0800
+++ linux/kernel/power/disk.c	2008-01-02 14:43:10.000000000 +0800
@@ -139,6 +139,14 @@ static void platform_restore_cleanup(int
 		hibernation_ops->restore_cleanup();
 }
 
+static unsigned long platform_hardware_signature(int platform_mode)
+{
+	if (platform_mode && hibernation_ops &&
+	    hibernation_ops->hardware_signature)
+		return hibernation_ops->hardware_signature();
+	return 0;
+}
+
 /**
  *	create_image - freeze devices that need to be frozen with interrupts
  *	off, create the hibernation image and thaw those devices.  Control
@@ -381,6 +389,7 @@ static int prepare_processes(void)
 int hibernate(void)
 {
 	int error;
+	unsigned long hd_sig;
 
 	mutex_lock(&pm_mutex);
 	/* The snapshot device should not be opened while we're running */
@@ -389,6 +398,7 @@ int hibernate(void)
 		goto Unlock;
 	}
 
+	hd_sig = platform_hardware_signature(hibernation_mode == HIBERNATION_PLATFORM);
 	error = pm_notifier_call_chain(PM_HIBERNATION_PREPARE);
 	if (error)
 		goto Exit;
@@ -418,7 +428,7 @@ int hibernate(void)
 		if (hibernation_mode == HIBERNATION_PLATFORM)
 			flags |= SF_PLATFORM_MODE;
 		pr_debug("PM: writing image.\n");
-		error = swsusp_write(flags);
+		error = swsusp_write(flags, hd_sig);
 		swsusp_free();
 		if (!error)
 			power_down();
@@ -455,6 +465,7 @@ static int software_resume(void)
 {
 	int error;
 	unsigned int flags;
+	unsigned long hd_sig;
 
 	/*
 	 * name_to_dev_t() below takes a sysfs buffer mutex when sysfs
@@ -489,9 +500,13 @@ static int software_resume(void)
 	}
 
 	pr_debug("PM: Checking swsusp image.\n");
-	error = swsusp_check();
+	error = swsusp_check(&flags, &hd_sig);
 	if (error)
 		goto Unlock;
+	if (hd_sig != platform_hardware_signature(flags & SF_PLATFORM_MODE)) {
+		printk(KERN_ERR"PM: Hardware changed, can't resume.\n");
+		goto Unlock;
+	}
 
 	/* The snapshot device should not be opened while we're running */
 	if (!atomic_add_unless(&snapshot_device_available, -1, 0)) {
@@ -512,7 +527,7 @@ static int software_resume(void)
 
 	pr_debug("PM: Reading swsusp image.\n");
 
-	error = swsusp_read(&flags);
+	error = swsusp_read();
 	if (!error)
 		hibernation_restore(flags & SF_PLATFORM_MODE);
 
Index: linux/kernel/power/power.h
===================================================================
--- linux.orig/kernel/power/power.h	2008-01-02 14:04:41.000000000 +0800
+++ linux/kernel/power/power.h	2008-01-02 14:06:05.000000000 +0800
@@ -180,12 +180,12 @@ extern int swsusp_swap_in_use(void);
 #define SF_PLATFORM_MODE	1
 
 /* kernel/power/disk.c */
-extern int swsusp_check(void);
+extern int swsusp_check(unsigned int *flags_p, unsigned long *hd_sig);
 extern int swsusp_shrink_memory(void);
 extern void swsusp_free(void);
 extern int swsusp_resume(void);
-extern int swsusp_read(unsigned int *flags_p);
-extern int swsusp_write(unsigned int flags);
+extern int swsusp_read(void);
+extern int swsusp_write(unsigned int flags, unsigned long hd_sig);
 extern void swsusp_close(void);
 
 struct timeval;
Index: linux/kernel/power/swap.c
===================================================================
--- linux.orig/kernel/power/swap.c	2008-01-02 14:04:41.000000000 +0800
+++ linux/kernel/power/swap.c	2008-01-02 14:34:49.000000000 +0800
@@ -33,9 +33,10 @@ extern char resume_file[];
 #define SWSUSP_SIG	"S1SUSPEND"
 
 struct swsusp_header {
-	char reserved[PAGE_SIZE - 20 - sizeof(sector_t) - sizeof(int)];
+	char reserved[PAGE_SIZE - 20 - sizeof(sector_t) - sizeof(int) - sizeof(unsigned long)];
 	sector_t image;
 	unsigned int flags;	/* Flags to pass to the "boot" kernel */
+	unsigned long hardware_signature;
 	char	orig_sig[10];
 	char	sig[10];
 } __attribute__((packed));
@@ -139,7 +140,7 @@ static int wait_on_bio_chain(struct bio 
  * Saving part
  */
 
-static int mark_swapfiles(sector_t start, unsigned int flags)
+static int mark_swapfiles(sector_t start, unsigned int flags, unsigned long hd_sig)
 {
 	int error;
 
@@ -150,6 +151,7 @@ static int mark_swapfiles(sector_t start
 		memcpy(swsusp_header->sig,SWSUSP_SIG, 10);
 		swsusp_header->image = start;
 		swsusp_header->flags = flags;
+		swsusp_header->hardware_signature = hd_sig;
 		error = bio_write_page(swsusp_resume_block,
 					swsusp_header, NULL);
 	} else {
@@ -379,7 +381,7 @@ static int enough_swap(unsigned int nr_p
  *	correctly, we'll mark system clean, anyway.)
  */
 
-int swsusp_write(unsigned int flags)
+int swsusp_write(unsigned int flags, unsigned long hd_sig)
 {
 	struct swap_map_handle handle;
 	struct snapshot_handle snapshot;
@@ -418,7 +420,7 @@ int swsusp_write(unsigned int flags)
 		if (!error) {
 			flush_swap_writer(&handle);
 			printk("S");
-			error = mark_swapfiles(start, flags);
+			error = mark_swapfiles(start, flags, hd_sig);
 			printk("|\n");
 		}
 	}
@@ -545,18 +547,15 @@ static int load_image(struct swap_map_ha
 
 /**
  *	swsusp_read - read the hibernation image.
- *	@flags_p: flags passed by the "frozen" kernel in the image header should
- *		  be written into this memeory location
  */
 
-int swsusp_read(unsigned int *flags_p)
+int swsusp_read()
 {
 	int error;
 	struct swap_map_handle handle;
 	struct snapshot_handle snapshot;
 	struct swsusp_info *header;
 
-	*flags_p = swsusp_header->flags;
 	if (IS_ERR(resume_bdev)) {
 		pr_debug("swsusp: block device not initialised\n");
 		return PTR_ERR(resume_bdev);
@@ -585,9 +584,13 @@ int swsusp_read(unsigned int *flags_p)
 
 /**
  *      swsusp_check - Check for swsusp signature in the resume device
+ *	@flags_p: flags passed by the "frozen" kernel in the image header should
+ *		  be written into this memeory location
+ *	@hd_sig: hardware signature passed by the "frozen" kernel in the image
+ * 		 header should be written into this memeory location
  */
 
-int swsusp_check(void)
+int swsusp_check(unsigned int *flags_p, unsigned long *hd_sig)
 {
 	int error;
 
@@ -605,6 +608,9 @@ int swsusp_check(void)
 			/* Reset swap signature now */
 			error = bio_write_page(swsusp_resume_block,
 						swsusp_header, NULL);
+
+			*flags_p = swsusp_header->flags;
+			*hd_sig = swsusp_header->hardware_signature;
 		} else {
 			return -EINVAL;
 		}

Re: [linux-pm] [patch] hibernation: utilize ACPI hardware signature

[Maxim Levitsky]

On Wednesday, 2 January 2008 08:59:22 Shaohua Li wrote:
> ACPI defines a hardware signature. BIOS calculates the signature
> according to hardware configure, if hardware changes, the signature will
> change, in this case, S4 resume should fail.
> 
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> ---
>  drivers/acpi/sleep/main.c |   13 +++++++++++++
>  include/linux/suspend.h   |    5 +++++
>  kernel/power/disk.c       |   21 ++++++++++++++++++---
>  kernel/power/power.h      |    6 +++---
>  kernel/power/swap.c       |   24 +++++++++++++++---------
>  5 files changed, 54 insertions(+), 15 deletions(-)

Once reading the ACPI spec I noticed that ACPI supports this feature
so it is qute nice to implement it, since this will prevent resume from disk what hardware was changed.

But.. how many bioses are broken regarding this feature....

Regards,
	Maxim Levitsky

Re: [linux-pm] [patch] hibernation: utilize ACPI hardware signature

[Johannes Berg]

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


> Once reading the ACPI spec I noticed that ACPI supports this feature
> so it is qute nice to implement it, since this will prevent resume from disk what hardware was changed.
> 
> But.. how many bioses are broken regarding this feature....

If they are broken though, how likely is it they'll have a changing
signature? I'd expect the most likely broken case is they just don't
calculate it at all and leave it constant.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [patch] hibernation: utilize ACPI hardware signature

[Erik Andrén]

[-- Attachment #1.1: Type: text/plain, Size: 1290 bytes --]

Hi,

2008/1/2, Shaohua Li <shaohua.li@intel.com>:
>
> ACPI defines a hardware signature. BIOS calculates the signature
> according to hardware configure, if hardware changes, the signature will
> change, in this case, S4 resume should fail.
>
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> ---


Would it be possible to extend this mechanism to prevent the following
scenario:

1. Linux image A is suspended to disk
2. Linux image B is booted and various changes to the system are done.
3. Linux image B is shut down
4. Linux image A is booted, restoring the suspend to disk image.
5. Chaos is ensured as the file system state is changed in regard to how
linux image A expects it.

Correct behaviour would naturally be that image A detects that changes have
been made under its feet and proceed to perform a normal boot instead of
resuming the stored suspend-to-disk image.

Is there another mechanism preventing this?
I remember destroying a system by doing this sequence of operations a couple
of years ago.

With kind regards,
Erik Andrén


-
> 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
>

[-- Attachment #1.2: Type: text/html, Size: 1893 bytes --]

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

Re: [patch] hibernation: utilize ACPI hardware signature

[Rafael J. Wysocki]

On Wednesday, 2 of January 2008, Shaohua Li wrote:
> ACPI defines a hardware signature. BIOS calculates the signature
> according to hardware configure, if hardware changes, the signature will
> change, in this case, S4 resume should fail.

The idea is fine, but I'd prefer to do that in a more straightforward way.
Namely, we can just:
* write the signature into a variable in, for example,
  acpi_hibernation_prepare() (then, the "old" signature value will be
  automatically saved in the image)
* compare it with a the "new" value read from the BIOS in
  acpi_hibernation_leave() and panic if there's a mismatch
* add a configuration option to disable this behavior (just in case)
This way we can avoid modifying the entire generic interface to add the feature
specific to ACPI.

Still, if you want the boot kernel to check the signature, which will be more
elegant (but please note that on x86-64 the boot kernel need not support ACPI
at all), you can use the (recently introduced) architecture part of the image
header for this purpose, without modifying the generic interface.

Greetings,
Rafael

Re: [patch] hibernation: utilize ACPI hardware signature

[Rafael J. Wysocki]

On Wednesday, 2 of January 2008, Erik Andrén wrote:
> Hi,
> 
> 2008/1/2, Shaohua Li <shaohua.li@intel.com>:
> >
> > ACPI defines a hardware signature. BIOS calculates the signature
> > according to hardware configure, if hardware changes, the signature will
> > change, in this case, S4 resume should fail.
> >
> > Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> > ---
> 
> 
> Would it be possible to extend this mechanism to prevent the following
> scenario:
> 
> 1. Linux image A is suspended to disk
> 2. Linux image B is booted and various changes to the system are done.
> 3. Linux image B is shut down
> 4. Linux image A is booted, restoring the suspend to disk image.
> 5. Chaos is ensured as the file system state is changed in regard to how
> linux image A expects it.
> 
> Correct behaviour would naturally be that image A detects that changes have
> been made under its feet and proceed to perform a normal boot instead of
> resuming the stored suspend-to-disk image.

It should be possible in theory.

> Is there another mechanism preventing this?

Not at the kernel level, but you can prevent this from happening by running
mkswap on all swap spaces that refuse to come up after a fresh boot.

Greetings,
Rafael
-
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: [linux-pm] Re: [patch] hibernation: utilize ACPI hardware signature

[Nigel Cunningham]

Hi.

Rafael J. Wysocki wrote:
> On Wednesday, 2 of January 2008, Erik Andrén wrote:
>> Hi,
>>
>> 2008/1/2, Shaohua Li <shaohua.li@intel.com>:
>>> ACPI defines a hardware signature. BIOS calculates the signature
>>> according to hardware configure, if hardware changes, the signature will
>>> change, in this case, S4 resume should fail.
>>>
>>> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
>>> ---
>>
>> Would it be possible to extend this mechanism to prevent the following
>> scenario:
>>
>> 1. Linux image A is suspended to disk
>> 2. Linux image B is booted and various changes to the system are done.
>> 3. Linux image B is shut down
>> 4. Linux image A is booted, restoring the suspend to disk image.
>> 5. Chaos is ensured as the file system state is changed in regard to how
>> linux image A expects it.
>>
>> Correct behaviour would naturally be that image A detects that changes have
>> been made under its feet and proceed to perform a normal boot instead of
>> resuming the stored suspend-to-disk image.
> 
> It should be possible in theory.
> 
>> Is there another mechanism preventing this?
> 
> Not at the kernel level, but you can prevent this from happening by running
> mkswap on all swap spaces that refuse to come up after a fresh boot.

We really should do something about this. It should be possible to
handle this properly if something along the following lines was implemented:

1) Each filesystem implements a function taking a pointer to a struct
block_device and returns a mount count for that filesystem without
making any modifications to the filesystem.
2) Hibernation implementations store the major & minor numbers and mount
counts for each mounted filesystem in the image header when hibernating,
and recheck those values at resume time. If the mount count on any
filesystem has changes, we warn the user, invalidate the image and boot
normally.

Regards,

Nigel
-
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: [linux-pm] Re: [patch] hibernation: utilize ACPI hardware signature

[Nigel Cunningham]

Hi.

Rafael J. Wysocki wrote:
> On Wednesday, 2 of January 2008, Shaohua Li wrote:
>> ACPI defines a hardware signature. BIOS calculates the signature
>> according to hardware configure, if hardware changes, the signature will
>> change, in this case, S4 resume should fail.
> 
> The idea is fine, but I'd prefer to do that in a more straightforward way.
> Namely, we can just:
> * write the signature into a variable in, for example,
>   acpi_hibernation_prepare() (then, the "old" signature value will be
>   automatically saved in the image)
> * compare it with a the "new" value read from the BIOS in
>   acpi_hibernation_leave() and panic if there's a mismatch
> * add a configuration option to disable this behavior (just in case)
> This way we can avoid modifying the entire generic interface to add the feature
> specific to ACPI.
> 
> Still, if you want the boot kernel to check the signature, which will be more
> elegant (but please note that on x86-64 the boot kernel need not support ACPI
> at all), you can use the (recently introduced) architecture part of the image
> header for this purpose, without modifying the generic interface.

I suppose we can always disable this when we start to support hardware
changing over hibernate (I have ideas in this direction - memory cold
plugging, for a start).

Regards,

Nigel

Re: [linux-pm] Re: [patch] hibernation: utilize ACPI hardware signature

[Rafael J. Wysocki]

On Wednesday, 2 of January 2008, Nigel Cunningham wrote:
> Hi.
> 
> Rafael J. Wysocki wrote:
> > On Wednesday, 2 of January 2008, Shaohua Li wrote:
> >> ACPI defines a hardware signature. BIOS calculates the signature
> >> according to hardware configure, if hardware changes, the signature will
> >> change, in this case, S4 resume should fail.
> > 
> > The idea is fine, but I'd prefer to do that in a more straightforward way.
> > Namely, we can just:
> > * write the signature into a variable in, for example,
> >   acpi_hibernation_prepare() (then, the "old" signature value will be
> >   automatically saved in the image)
> > * compare it with a the "new" value read from the BIOS in
> >   acpi_hibernation_leave() and panic if there's a mismatch
> > * add a configuration option to disable this behavior (just in case)
> > This way we can avoid modifying the entire generic interface to add the feature
> > specific to ACPI.
> > 
> > Still, if you want the boot kernel to check the signature, which will be more
> > elegant (but please note that on x86-64 the boot kernel need not support ACPI
> > at all), you can use the (recently introduced) architecture part of the image
> > header for this purpose, without modifying the generic interface.
> 
> I suppose we can always disable this when we start to support hardware
> changing over hibernate (I have ideas in this direction - memory cold
> plugging, for a start).

Well, if we support such features, we won't be following ACPI any more.

Greetings,
Rafael

Re: [linux-pm] Re: [patch] hibernation: utilize ACPI hardware signature

[Rafael J. Wysocki]

On Wednesday, 2 of January 2008, Nigel Cunningham wrote:
> Hi.
> 
> Rafael J. Wysocki wrote:
> > On Wednesday, 2 of January 2008, Erik Andrén wrote:
> >> Hi,
> >>
> >> 2008/1/2, Shaohua Li <shaohua.li@intel.com>:
> >>> ACPI defines a hardware signature. BIOS calculates the signature
> >>> according to hardware configure, if hardware changes, the signature will
> >>> change, in this case, S4 resume should fail.
> >>>
> >>> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> >>> ---
> >>
> >> Would it be possible to extend this mechanism to prevent the following
> >> scenario:
> >>
> >> 1. Linux image A is suspended to disk
> >> 2. Linux image B is booted and various changes to the system are done.
> >> 3. Linux image B is shut down
> >> 4. Linux image A is booted, restoring the suspend to disk image.
> >> 5. Chaos is ensured as the file system state is changed in regard to how
> >> linux image A expects it.
> >>
> >> Correct behaviour would naturally be that image A detects that changes have
> >> been made under its feet and proceed to perform a normal boot instead of
> >> resuming the stored suspend-to-disk image.
> > 
> > It should be possible in theory.
> > 
> >> Is there another mechanism preventing this?
> > 
> > Not at the kernel level, but you can prevent this from happening by running
> > mkswap on all swap spaces that refuse to come up after a fresh boot.
> 
> We really should do something about this. It should be possible to
> handle this properly if something along the following lines was implemented:
> 
> 1) Each filesystem implements a function taking a pointer to a struct
> block_device and returns a mount count for that filesystem without
> making any modifications to the filesystem.
> 2) Hibernation implementations store the major & minor numbers and mount
> counts for each mounted filesystem in the image header when hibernating,
> and recheck those values at resume time. If the mount count on any
> filesystem has changes, we warn the user, invalidate the image and boot
> normally.

That may quickly become complicated.

For example, boot kernel need not contain all drivers used by the hibernated
ones, so some filesystems may be physically inaccessible to them.

Greetings,
Rafael
-
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: [linux-pm] [patch] hibernation: utilize ACPI hardware signature

[Shaohua Li]

On Wed, 2008-01-02 at 17:29 +0800, Johannes Berg wrote:
> > Once reading the ACPI spec I noticed that ACPI supports this feature
> > so it is qute nice to implement it, since this will prevent resume from disk what hardware was changed.
> > 
> > But.. how many bioses are broken regarding this feature....
> 
> If they are broken though, how likely is it they'll have a changing
> signature? I'd expect the most likely broken case is they just don't
> calculate it at all and leave it constant.
In my limited check, Thinkpad implements the signature fine, others
sound not. They are not broken, but just not implement the signature,
eg, it's always 0.

Thanks,
Shaohua

Re: [patch] hibernation: utilize ACPI hardware signature

[Shaohua Li]

On Wed, 2008-01-02 at 22:05 +0800, Rafael J. Wysocki wrote:
> On Wednesday, 2 of January 2008, Shaohua Li wrote:
> > ACPI defines a hardware signature. BIOS calculates the signature
> > according to hardware configure, if hardware changes, the signature
> will
> > change, in this case, S4 resume should fail.
> 
> The idea is fine, but I'd prefer to do that in a more straightforward
> way.
> Namely, we can just:
> * write the signature into a variable in, for example,
>   acpi_hibernation_prepare() (then, the "old" signature value will be
>   automatically saved in the image)
> * compare it with a the "new" value read from the BIOS in
>   acpi_hibernation_leave() and panic if there's a mismatch
> * add a configuration option to disable this behavior (just in case)
> This way we can avoid modifying the entire generic interface to add
> the feature
> specific to ACPI.
it would be better we do the check in boot kernel. Why is so bad to do
it in generic code? Other platforms can implement it too, like
calculating the signature in OS.

> Still, if you want the boot kernel to check the signature, which will
> be more
> elegant (but please note that on x86-64 the boot kernel need not
> support ACPI
> at all), you can use the (recently introduced) architecture part of
> the image
> header for this purpose, without modifying the generic interface.
Where can I get the code with architecture image header support? appears
can't find it.

Thanks,
Shaohua

Re: [linux-pm] Re: [patch] hibernation: utilize ACPI hardware signature

[Nigel Cunningham]

Hi.

Rafael J. Wysocki wrote:
>>>> Is there another mechanism preventing this?
>>> Not at the kernel level, but you can prevent this from happening by running
>>> mkswap on all swap spaces that refuse to come up after a fresh boot.
>> We really should do something about this. It should be possible to
>> handle this properly if something along the following lines was implemented:
>>
>> 1) Each filesystem implements a function taking a pointer to a struct
>> block_device and returns a mount count for that filesystem without
>> making any modifications to the filesystem.
>> 2) Hibernation implementations store the major & minor numbers and mount
>> counts for each mounted filesystem in the image header when hibernating,
>> and recheck those values at resume time. If the mount count on any
>> filesystem has changes, we warn the user, invalidate the image and boot
>> normally.
> 
> That may quickly become complicated.
> 
> For example, boot kernel need not contain all drivers used by the hibernated
> ones, so some filesystems may be physically inaccessible to them.

Mmmm. That's true, and I'd never heard that point raised before or
thought of it myself. Thanks!

Nigel

Re: [linux-pm] Re: [patch] hibernation: utilize ACPI hardware signature

[Nigel Cunningham]

Hi.

Rafael J. Wysocki wrote:
>> I suppose we can always disable this when we start to support hardware
>> changing over hibernate (I have ideas in this direction - memory cold
>> plugging, for a start).
> 
> Well, if we support such features, we won't be following ACPI any more.

Mmm. Apparently I'm not as focused on specs as you :).

I was starting to think through whether it could make for a new and much
faster was to boot live cds - something along the lines of complete
redetection of hardware and memory cold plugging. Of course it's only in
the 'I wonder if this would be possible' stage at the mo, but seemed at
least feasible - keep the e820 tables from boot, make _init routines not
get thrown away and do something like the kexec device shutdown etc
around an atomic restore.

I know there'd be limitations, but perhaps worth thinking about...

Nigel

Re: [linux-pm] Re: [patch] hibernation: utilize ACPI hardware signature

[Rafael J. Wysocki]

On Thursday, 3 of January 2008, Nigel Cunningham wrote:
> Hi.
> 
> Rafael J. Wysocki wrote:
> >> I suppose we can always disable this when we start to support hardware
> >> changing over hibernate (I have ideas in this direction - memory cold
> >> plugging, for a start).
> > 
> > Well, if we support such features, we won't be following ACPI any more.
> 
> Mmm. Apparently I'm not as focused on specs as you :).
> 
> I was starting to think through whether it could make for a new and much
> faster was to boot live cds - something along the lines of complete
> redetection of hardware and memory cold plugging. Of course it's only in
> the 'I wonder if this would be possible' stage at the mo, but seemed at
> least feasible - keep the e820 tables from boot, make _init routines not
> get thrown away and do something like the kexec device shutdown etc
> around an atomic restore.
> 
> I know there'd be limitations, but perhaps worth thinking about...

Well, what if the new e820 map is incompatible with the old one?

Rafael

Re: [patch] hibernation: utilize ACPI hardware signature

[Rafael J. Wysocki]

On Thursday, 3 of January 2008, Shaohua Li wrote:
> 
> On Wed, 2008-01-02 at 22:05 +0800, Rafael J. Wysocki wrote:
> > On Wednesday, 2 of January 2008, Shaohua Li wrote:
> > > ACPI defines a hardware signature. BIOS calculates the signature
> > > according to hardware configure, if hardware changes, the signature
> > will
> > > change, in this case, S4 resume should fail.
> > 
> > The idea is fine, but I'd prefer to do that in a more straightforward
> > way.
> > Namely, we can just:
> > * write the signature into a variable in, for example,
> >   acpi_hibernation_prepare() (then, the "old" signature value will be
> >   automatically saved in the image)
> > * compare it with a the "new" value read from the BIOS in
> >   acpi_hibernation_leave() and panic if there's a mismatch
> > * add a configuration option to disable this behavior (just in case)
> > This way we can avoid modifying the entire generic interface to add
> > the feature
> > specific to ACPI.
> it would be better we do the check in boot kernel.

Franky, I think we should also check in the image kernel, in case the boot
one doesn't support ACPI as I said.

> Why is so bad to do it in generic code?

I just don't like adding more callbacks for this purpose.

> Other platforms can implement it too, like calculating the signature in OS.
> 
> > Still, if you want the boot kernel to check the signature, which will
> > be more
> > elegant (but please note that on x86-64 the boot kernel need not
> > support ACPI
> > at all), you can use the (recently introduced) architecture part of
> > the image
> > header for this purpose, without modifying the generic interface.
> Where can I get the code with architecture image header support? appears
> can't find it.

Please have a look at arch_hibernation_header_save() and
arch_hibernation_header_restore() in arch/x86/kernel/suspend_64.c.

They are used by init_header_complete() and check_image_kernel() defined
in kernel/power/power.h .

Currently only x86_64 defines these functions, but I'm going to implement
them on i386 too.

Thanks,
Rafael

Re: [linux-pm] Re: [patch] hibernation: utilize ACPI hardware signature

[Nigel Cunningham]

Hi.

Rafael J. Wysocki wrote:
> On Thursday, 3 of January 2008, Nigel Cunningham wrote:
>> Hi.
>>
>> Rafael J. Wysocki wrote:
>>>> I suppose we can always disable this when we start to support hardware
>>>> changing over hibernate (I have ideas in this direction - memory cold
>>>> plugging, for a start).
>>> Well, if we support such features, we won't be following ACPI any more.
>> Mmm. Apparently I'm not as focused on specs as you :).
>>
>> I was starting to think through whether it could make for a new and much
>> faster was to boot live cds - something along the lines of complete
>> redetection of hardware and memory cold plugging. Of course it's only in
>> the 'I wonder if this would be possible' stage at the mo, but seemed at
>> least feasible - keep the e820 tables from boot, make _init routines not
>> get thrown away and do something like the kexec device shutdown etc
>> around an atomic restore.
>>
>> I know there'd be limitations, but perhaps worth thinking about...
> 
> Well, what if the new e820 map is incompatible with the old one?

In the worst case, you tell the user about it and carry on with the old
one. Perhaps this would be the killer for the live cd idea - perhaps
e820 maps vary too much, there'd be no possible workaround and this
would never be practical. It's worth at least thinking about, though,
isn't it?

Nigel

Re: [patch] hibernation: utilize ACPI hardware signature

[Shaohua Li]

On Fri, 2008-01-04 at 01:04 +0800, Rafael J. Wysocki wrote:
> On Thursday, 3 of January 2008, Shaohua Li wrote:
> >
> > On Wed, 2008-01-02 at 22:05 +0800, Rafael J. Wysocki wrote:
> > > On Wednesday, 2 of January 2008, Shaohua Li wrote:
> > > > ACPI defines a hardware signature. BIOS calculates the signature
> > > > according to hardware configure, if hardware changes, the
> signature
> > > will
> > > > change, in this case, S4 resume should fail.
> > >
> > > The idea is fine, but I'd prefer to do that in a more
> straightforward
> > > way.
> > > Namely, we can just:
> > > * write the signature into a variable in, for example,
> > >   acpi_hibernation_prepare() (then, the "old" signature value will
> be
> > >   automatically saved in the image)
> > > * compare it with a the "new" value read from the BIOS in
> > >   acpi_hibernation_leave() and panic if there's a mismatch
> > > * add a configuration option to disable this behavior (just in
> case)
> > > This way we can avoid modifying the entire generic interface to
> add
> > > the feature
> > > specific to ACPI.
> > it would be better we do the check in boot kernel.
> Franky, I think we should also check in the image kernel, in case the
> boot
> one doesn't support ACPI as I said.
Ok, makes sense. I changed to check the signature in .higberation_leave

Signed-off-by: Shaohua Li <shaohua.li@intel.com>
---
 drivers/acpi/sleep/main.c |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Index: linux/drivers/acpi/sleep/main.c
===================================================================
--- linux.orig/drivers/acpi/sleep/main.c	2008-01-03 13:37:08.000000000 +0800
+++ linux/drivers/acpi/sleep/main.c	2008-01-04 13:36:10.000000000 +0800
@@ -256,6 +256,17 @@ static int acpi_hibernation_enter(void)
 	return ACPI_SUCCESS(status) ? 0 : -EFAULT;
 }
 
+static unsigned long s4_hardware_signature;
+static struct acpi_table_facs *facs;
+static int nosigcheck;
+
+static int __init acpi_s4_nosigcheck(char *str)
+{
+	nosigcheck = 1;
+	return 1;
+}
+__setup("acpi_s4_nosigcheck", acpi_s4_nosigcheck);
+
 static void acpi_hibernation_leave(void)
 {
 	/*
@@ -263,6 +274,11 @@ static void acpi_hibernation_leave(void)
 	 * enable it here.
 	 */
 	acpi_enable();
+	if (facs && s4_hardware_signature != facs->hardware_signature) {
+		printk(KERN_EMERG"PM: Hardware changed in the S4 circle, can't resume\n");
+		if (!nosigcheck)
+			panic("S4 resume error");
+	}
 }
 
 static void acpi_hibernation_finish(void)
@@ -449,6 +465,10 @@ int __init acpi_sleep_init(void)
 		sleep_states[ACPI_STATE_S4] = 1;
 		printk(" S4");
 	}
+	acpi_get_table_by_index(ACPI_TABLE_INDEX_FACS,
+		(struct acpi_table_header **)&facs);
+	if (facs)
+		s4_hardware_signature = facs->hardware_signature;
 #endif
 	status = acpi_get_sleep_type_data(ACPI_STATE_S5, &type_a, &type_b);
 	if (ACPI_SUCCESS(status)) {

Re: [patch] hibernation: utilize ACPI hardware signature

[Rafael J. Wysocki]

On Friday, 4 of January 2008, Shaohua Li wrote:
> 
> On Fri, 2008-01-04 at 01:04 +0800, Rafael J. Wysocki wrote:
> > On Thursday, 3 of January 2008, Shaohua Li wrote:
> > >
> > > On Wed, 2008-01-02 at 22:05 +0800, Rafael J. Wysocki wrote:
> > > > On Wednesday, 2 of January 2008, Shaohua Li wrote:
> > > > > ACPI defines a hardware signature. BIOS calculates the signature
> > > > > according to hardware configure, if hardware changes, the
> > signature
> > > > will
> > > > > change, in this case, S4 resume should fail.
> > > >
> > > > The idea is fine, but I'd prefer to do that in a more
> > straightforward
> > > > way.
> > > > Namely, we can just:
> > > > * write the signature into a variable in, for example,
> > > >   acpi_hibernation_prepare() (then, the "old" signature value will
> > be
> > > >   automatically saved in the image)
> > > > * compare it with a the "new" value read from the BIOS in
> > > >   acpi_hibernation_leave() and panic if there's a mismatch
> > > > * add a configuration option to disable this behavior (just in
> > case)
> > > > This way we can avoid modifying the entire generic interface to
> > add
> > > > the feature
> > > > specific to ACPI.
> > > it would be better we do the check in boot kernel.
> > Franky, I think we should also check in the image kernel, in case the
> > boot
> > one doesn't support ACPI as I said.
> Ok, makes sense. I changed to check the signature in .higberation_leave

Thanks, comments below.

> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> ---
>  drivers/acpi/sleep/main.c |   20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> Index: linux/drivers/acpi/sleep/main.c
> ===================================================================
> --- linux.orig/drivers/acpi/sleep/main.c	2008-01-03 13:37:08.000000000 +0800
> +++ linux/drivers/acpi/sleep/main.c	2008-01-04 13:36:10.000000000 +0800
> @@ -256,6 +256,17 @@ static int acpi_hibernation_enter(void)
>  	return ACPI_SUCCESS(status) ? 0 : -EFAULT;
>  }
>  
> +static unsigned long s4_hardware_signature;
> +static struct acpi_table_facs *facs;
> +static int nosigcheck;

Use bool perhaps?

> +
> +static int __init acpi_s4_nosigcheck(char *str)
> +{
> +	nosigcheck = 1;

And "true" here?

> +	return 1;
> +}
> +__setup("acpi_s4_nosigcheck", acpi_s4_nosigcheck);
> +

Please put this function at the end of the file.  Also, I'd call it
"acpi_s4_nosig", but whatever.

>  static void acpi_hibernation_leave(void)
>  {
>  	/*
> @@ -263,6 +274,11 @@ static void acpi_hibernation_leave(void)
>  	 * enable it here.
>  	 */
>  	acpi_enable();
> +	if (facs && s4_hardware_signature != facs->hardware_signature) {

I think we don't need to evaluate the condition if nosigcheck is true, but ...

> +		printk(KERN_EMERG"PM: Hardware changed in the S4 circle, can't resume\n");
> +		if (!nosigcheck)
> +			panic("S4 resume error");
> +	}
>  }
>  
>  static void acpi_hibernation_finish(void)
> @@ -449,6 +465,10 @@ int __init acpi_sleep_init(void)
>  		sleep_states[ACPI_STATE_S4] = 1;
>  		printk(" S4");
>  	}

... I'd put "if (!nosigcheck)" around the following 4 lines and then we
wouldn't need to check nosigcheck in acpi_hibernation_leave(), because facs
would be NULL.

Also, please put that into the block that contains the 'printk(" S4")' above.

> +	acpi_get_table_by_index(ACPI_TABLE_INDEX_FACS,
> +		(struct acpi_table_header **)&facs);
> +	if (facs)
> +		s4_hardware_signature = facs->hardware_signature;
>  #endif
>  	status = acpi_get_sleep_type_data(ACPI_STATE_S5, &type_a, &type_b);
>  	if (ACPI_SUCCESS(status)) {

Greetings,
Rafael

Re: [patch] hibernation: utilize ACPI hardware signature

[Shaohua Li]

On Sat, 2008-01-05 at 04:44 +0800, Rafael J. Wysocki wrote:
> On Friday, 4 of January 2008, Shaohua Li wrote:
> >
> > On Fri, 2008-01-04 at 01:04 +0800, Rafael J. Wysocki wrote:
> > > On Thursday, 3 of January 2008, Shaohua Li wrote:
> > > >
> > > > On Wed, 2008-01-02 at 22:05 +0800, Rafael J. Wysocki wrote:
> > > > > On Wednesday, 2 of January 2008, Shaohua Li wrote:
> > > > > > ACPI defines a hardware signature. BIOS calculates the
> signature
> > > > > > according to hardware configure, if hardware changes, the
> > > signature
> > > > > will
> > > > > > change, in this case, S4 resume should fail.
> > > > >
> > > > > The idea is fine, but I'd prefer to do that in a more
> > > straightforward
> > > > > way.
> > > > > Namely, we can just:
> > > > > * write the signature into a variable in, for example,
> > > > >   acpi_hibernation_prepare() (then, the "old" signature value
> will
> > > be
> > > > >   automatically saved in the image)
> > > > > * compare it with a the "new" value read from the BIOS in
> > > > >   acpi_hibernation_leave() and panic if there's a mismatch
> > > > > * add a configuration option to disable this behavior (just in
> > > case)
> > > > > This way we can avoid modifying the entire generic interface
> to
> > > add
> > > > > the feature
> > > > > specific to ACPI.
> > > > it would be better we do the check in boot kernel.
> > > Franky, I think we should also check in the image kernel, in case
> the
> > > boot
> > > one doesn't support ACPI as I said.
> > Ok, makes sense. I changed to check the signature
> in .higberation_leave
> 
> Thanks, comments below.
> 
> > Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> > ---
> >  drivers/acpi/sleep/main.c |   20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > Index: linux/drivers/acpi/sleep/main.c
> > ===================================================================
> > --- linux.orig/drivers/acpi/sleep/main.c      2008-01-03
> 13:37:08.000000000 +0800
> > +++ linux/drivers/acpi/sleep/main.c   2008-01-04 13:36:10.000000000
> +0800
> > @@ -256,6 +256,17 @@ static int acpi_hibernation_enter(void)
> >       return ACPI_SUCCESS(status) ? 0 : -EFAULT;
> >  }
> > 
> > +static unsigned long s4_hardware_signature;
> > +static struct acpi_table_facs *facs;
> > +static int nosigcheck;
> 
> Use bool perhaps?
> 
> > +
> > +static int __init acpi_s4_nosigcheck(char *str)
> > +{
> > +     nosigcheck = 1;
> 
> And "true" here?
> 
> > +     return 1;
> > +}
> > +__setup("acpi_s4_nosigcheck", acpi_s4_nosigcheck);
> > +
> 
> Please put this function at the end of the file.  Also, I'd call it
> "acpi_s4_nosig", but whatever.
Fixed all except this one, the routine is defined with HIBERATION
configed.

Signed-off-by: Shaohua Li <shaohua.li@intel.com>
---
 drivers/acpi/sleep/main.c |   22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Index: linux/drivers/acpi/sleep/main.c
===================================================================
--- linux.orig/drivers/acpi/sleep/main.c	2008-01-04 13:44:40.000000000 +0800
+++ linux/drivers/acpi/sleep/main.c	2008-01-07 09:31:42.000000000 +0800
@@ -256,6 +256,17 @@ static int acpi_hibernation_enter(void)
 	return ACPI_SUCCESS(status) ? 0 : -EFAULT;
 }
 
+static unsigned long s4_hardware_signature;
+static struct acpi_table_facs *facs;
+static bool nosigcheck;
+
+static int __init acpi_s4_nosigcheck(char *str)
+{
+	nosigcheck = true;
+	return 1;
+}
+__setup("acpi_s4_nosigcheck", acpi_s4_nosigcheck);
+
 static void acpi_hibernation_leave(void)
 {
 	/*
@@ -263,6 +274,10 @@ static void acpi_hibernation_leave(void)
 	 * enable it here.
 	 */
 	acpi_enable();
+	if (facs && s4_hardware_signature != facs->hardware_signature) {
+		printk(KERN_EMERG"PM: Hardware changed in the S4 circle, can't resume\n");
+		panic("S4 resume error");
+	}
 }
 
 static void acpi_hibernation_finish(void)
@@ -448,6 +463,13 @@ int __init acpi_sleep_init(void)
 		hibernation_set_ops(&acpi_hibernation_ops);
 		sleep_states[ACPI_STATE_S4] = 1;
 		printk(" S4");
+		if (!nosigcheck) {
+			acpi_get_table_by_index(ACPI_TABLE_INDEX_FACS,
+				(struct acpi_table_header **)&facs);
+			if (facs)
+				s4_hardware_signature =
+					facs->hardware_signature;
+		}
 	}
 #endif
 	status = acpi_get_sleep_type_data(ACPI_STATE_S5, &type_a, &type_b);

Re: [patch] hibernation: utilize ACPI hardware signature

[Rafael J. Wysocki]

On Monday, 7 of January 2008, Shaohua Li wrote:
> 
> On Sat, 2008-01-05 at 04:44 +0800, Rafael J. Wysocki wrote:
> > On Friday, 4 of January 2008, Shaohua Li wrote:
> > >
> > > On Fri, 2008-01-04 at 01:04 +0800, Rafael J. Wysocki wrote:
> > > > On Thursday, 3 of January 2008, Shaohua Li wrote:
> > > > >
> > > > > On Wed, 2008-01-02 at 22:05 +0800, Rafael J. Wysocki wrote:
> > > > > > On Wednesday, 2 of January 2008, Shaohua Li wrote:
> > > > > > > ACPI defines a hardware signature. BIOS calculates the
> > signature
> > > > > > > according to hardware configure, if hardware changes, the
> > > > signature
> > > > > > will
> > > > > > > change, in this case, S4 resume should fail.
> > > > > >
> > > > > > The idea is fine, but I'd prefer to do that in a more
> > > > straightforward
> > > > > > way.
> > > > > > Namely, we can just:
> > > > > > * write the signature into a variable in, for example,
> > > > > >   acpi_hibernation_prepare() (then, the "old" signature value
> > will
> > > > be
> > > > > >   automatically saved in the image)
> > > > > > * compare it with a the "new" value read from the BIOS in
> > > > > >   acpi_hibernation_leave() and panic if there's a mismatch
> > > > > > * add a configuration option to disable this behavior (just in
> > > > case)
> > > > > > This way we can avoid modifying the entire generic interface
> > to
> > > > add
> > > > > > the feature
> > > > > > specific to ACPI.
> > > > > it would be better we do the check in boot kernel.
> > > > Franky, I think we should also check in the image kernel, in case
> > the
> > > > boot
> > > > one doesn't support ACPI as I said.
> > > Ok, makes sense. I changed to check the signature
> > in .higberation_leave
> > 
> > Thanks, comments below.
> > 
> > > Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> > > ---
> > >  drivers/acpi/sleep/main.c |   20 ++++++++++++++++++++
> > >  1 file changed, 20 insertions(+)
> > >
> > > Index: linux/drivers/acpi/sleep/main.c
> > > ===================================================================
> > > --- linux.orig/drivers/acpi/sleep/main.c      2008-01-03
> > 13:37:08.000000000 +0800
> > > +++ linux/drivers/acpi/sleep/main.c   2008-01-04 13:36:10.000000000
> > +0800
> > > @@ -256,6 +256,17 @@ static int acpi_hibernation_enter(void)
> > >       return ACPI_SUCCESS(status) ? 0 : -EFAULT;
> > >  }
> > > 
> > > +static unsigned long s4_hardware_signature;
> > > +static struct acpi_table_facs *facs;
> > > +static int nosigcheck;
> > 
> > Use bool perhaps?
> > 
> > > +
> > > +static int __init acpi_s4_nosigcheck(char *str)
> > > +{
> > > +     nosigcheck = 1;
> > 
> > And "true" here?
> > 
> > > +     return 1;
> > > +}
> > > +__setup("acpi_s4_nosigcheck", acpi_s4_nosigcheck);
> > > +
> > 
> > Please put this function at the end of the file.  Also, I'd call it
> > "acpi_s4_nosig", but whatever.
> Fixed all except this one, the routine is defined with HIBERATION
> configed.

I've just realized that we're not doing the right thing here ...
 
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> ---
>  drivers/acpi/sleep/main.c |   22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> Index: linux/drivers/acpi/sleep/main.c
> ===================================================================
> --- linux.orig/drivers/acpi/sleep/main.c	2008-01-04 13:44:40.000000000 +0800
> +++ linux/drivers/acpi/sleep/main.c	2008-01-07 09:31:42.000000000 +0800
> @@ -256,6 +256,17 @@ static int acpi_hibernation_enter(void)
>  	return ACPI_SUCCESS(status) ? 0 : -EFAULT;
>  }
>  
> +static unsigned long s4_hardware_signature;
> +static struct acpi_table_facs *facs;
> +static bool nosigcheck;
> +
> +static int __init acpi_s4_nosigcheck(char *str)
> +{
> +	nosigcheck = true;
> +	return 1;
> +}
> +__setup("acpi_s4_nosigcheck", acpi_s4_nosigcheck);
> +
>  static void acpi_hibernation_leave(void)
>  {
>  	/*
> @@ -263,6 +274,10 @@ static void acpi_hibernation_leave(void)
>  	 * enable it here.
>  	 */
>  	acpi_enable();
> +	if (facs && s4_hardware_signature != facs->hardware_signature) {

... because we should read the signature from the hardware here, while we're
comparing two values read from memory.  They will always be equal. :-)

> +		printk(KERN_EMERG"PM: Hardware changed in the S4 circle, can't resume\n");
> +		panic("S4 resume error");
> +	}
>  }
>  
>  static void acpi_hibernation_finish(void)
> @@ -448,6 +463,13 @@ int __init acpi_sleep_init(void)
>  		hibernation_set_ops(&acpi_hibernation_ops);
>  		sleep_states[ACPI_STATE_S4] = 1;
>  		printk(" S4");
> +		if (!nosigcheck) {
> +			acpi_get_table_by_index(ACPI_TABLE_INDEX_FACS,
> +				(struct acpi_table_header **)&facs);
> +			if (facs)
> +				s4_hardware_signature =
> +					facs->hardware_signature;
> +		}
>  	}
>  #endif
>  	status = acpi_get_sleep_type_data(ACPI_STATE_S5, &type_a, &type_b);
> 
> 
> -

Greetings,
Rafael

Re: [patch] hibernation: utilize ACPI hardware signature

[Shaohua Li]

On Fri, 2008-01-11 at 07:39 +0800, Rafael J. Wysocki wrote:
> On Monday, 7 of January 2008, Shaohua Li wrote:
> >
> > On Sat, 2008-01-05 at 04:44 +0800, Rafael J. Wysocki wrote:
> > > On Friday, 4 of January 2008, Shaohua Li wrote:
> > > >
> > > > On Fri, 2008-01-04 at 01:04 +0800, Rafael J. Wysocki wrote:
> > > > > On Thursday, 3 of January 2008, Shaohua Li wrote:
> > > > > >
> > > > > > On Wed, 2008-01-02 at 22:05 +0800, Rafael J. Wysocki wrote:
> > > > > > > On Wednesday, 2 of January 2008, Shaohua Li wrote:
> > > > > > > > ACPI defines a hardware signature. BIOS calculates the
> > > signature
> > > > > > > > according to hardware configure, if hardware changes,
> the
> > > > > signature
> > > > > > > will
> > > > > > > > change, in this case, S4 resume should fail.
> > > > > > >
> > > > > > > The idea is fine, but I'd prefer to do that in a more
> > > > > straightforward
> > > > > > > way.
> > > > > > > Namely, we can just:
> > > > > > > * write the signature into a variable in, for example,
> > > > > > >   acpi_hibernation_prepare() (then, the "old" signature
> value
> > > will
> > > > > be
> > > > > > >   automatically saved in the image)
> > > > > > > * compare it with a the "new" value read from the BIOS in
> > > > > > >   acpi_hibernation_leave() and panic if there's a mismatch
> > > > > > > * add a configuration option to disable this behavior
> (just in
> > > > > case)
> > > > > > > This way we can avoid modifying the entire generic
> interface
> > > to
> > > > > add
> > > > > > > the feature
> > > > > > > specific to ACPI.
> > > > > > it would be better we do the check in boot kernel.
> > > > > Franky, I think we should also check in the image kernel, in
> case
> > > the
> > > > > boot
> > > > > one doesn't support ACPI as I said.
> > > > Ok, makes sense. I changed to check the signature
> > > in .higberation_leave
> > >
> > > Thanks, comments below.
> > >
> > > > Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> > > > ---
> > > >  drivers/acpi/sleep/main.c |   20 ++++++++++++++++++++
> > > >  1 file changed, 20 insertions(+)
> > > >
> > > > Index: linux/drivers/acpi/sleep/main.c
> > > >
> ===================================================================
> > > > --- linux.orig/drivers/acpi/sleep/main.c      2008-01-03
> > > 13:37:08.000000000 +0800
> > > > +++ linux/drivers/acpi/sleep/main.c   2008-01-04
> 13:36:10.000000000
> > > +0800
> > > > @@ -256,6 +256,17 @@ static int acpi_hibernation_enter(void)
> > > >       return ACPI_SUCCESS(status) ? 0 : -EFAULT;
> > > >  }
> > > >
> > > > +static unsigned long s4_hardware_signature;
> > > > +static struct acpi_table_facs *facs;
> > > > +static int nosigcheck;
> > >
> > > Use bool perhaps?
> > >
> > > > +
> > > > +static int __init acpi_s4_nosigcheck(char *str)
> > > > +{
> > > > +     nosigcheck = 1;
> > >
> > > And "true" here?
> > >
> > > > +     return 1;
> > > > +}
> > > > +__setup("acpi_s4_nosigcheck", acpi_s4_nosigcheck);
> > > > +
> > >
> > > Please put this function at the end of the file.  Also, I'd call
> it
> > > "acpi_s4_nosig", but whatever.
> > Fixed all except this one, the routine is defined with HIBERATION
> > configed.
> 
> I've just realized that we're not doing the right thing here ...
> 
> > Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> > ---
> >  drivers/acpi/sleep/main.c |   22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >
> > Index: linux/drivers/acpi/sleep/main.c
> > ===================================================================
> > --- linux.orig/drivers/acpi/sleep/main.c      2008-01-04
> 13:44:40.000000000 +0800
> > +++ linux/drivers/acpi/sleep/main.c   2008-01-07 09:31:42.000000000
> +0800
> > @@ -256,6 +256,17 @@ static int acpi_hibernation_enter(void)
> >       return ACPI_SUCCESS(status) ? 0 : -EFAULT;
> >  }
> > 
> > +static unsigned long s4_hardware_signature;
> > +static struct acpi_table_facs *facs;
> > +static bool nosigcheck;
> > +
> > +static int __init acpi_s4_nosigcheck(char *str)
> > +{
> > +     nosigcheck = true;
> > +     return 1;
> > +}
> > +__setup("acpi_s4_nosigcheck", acpi_s4_nosigcheck);
> > +
> >  static void acpi_hibernation_leave(void)
> >  {
> >       /*
> > @@ -263,6 +274,10 @@ static void acpi_hibernation_leave(void)
> >        * enable it here.
> >        */
> >       acpi_enable();
> > +     if (facs && s4_hardware_signature != facs->hardware_signature)
> {
> 
> ... because we should read the signature from the hardware here, while
> we're
> comparing two values read from memory.  They will always be equal. :-)
No, facs isn't a copy of ACPI FACS table (imagine how we sets
facs->waking_vector). It's mapped to BIOS table.

Thanks,
Shaohua

Re: [patch] hibernation: utilize ACPI hardware signature

[Rafael J. Wysocki]

On Friday, 11 of January 2008, Shaohua Li wrote:
> 
> On Fri, 2008-01-11 at 07:39 +0800, Rafael J. Wysocki wrote:
> > On Monday, 7 of January 2008, Shaohua Li wrote:
> > >
> > > On Sat, 2008-01-05 at 04:44 +0800, Rafael J. Wysocki wrote:
> > > > On Friday, 4 of January 2008, Shaohua Li wrote:
> > > > >
> > > > > On Fri, 2008-01-04 at 01:04 +0800, Rafael J. Wysocki wrote:
> > > > > > On Thursday, 3 of January 2008, Shaohua Li wrote:
> > > > > > >
> > > > > > > On Wed, 2008-01-02 at 22:05 +0800, Rafael J. Wysocki wrote:
> > > > > > > > On Wednesday, 2 of January 2008, Shaohua Li wrote:
> > > > > > > > > ACPI defines a hardware signature. BIOS calculates the
> > > > signature
> > > > > > > > > according to hardware configure, if hardware changes,
> > the
> > > > > > signature
> > > > > > > > will
> > > > > > > > > change, in this case, S4 resume should fail.
> > > > > > > >
> > > > > > > > The idea is fine, but I'd prefer to do that in a more
> > > > > > straightforward
> > > > > > > > way.
> > > > > > > > Namely, we can just:
> > > > > > > > * write the signature into a variable in, for example,
> > > > > > > >   acpi_hibernation_prepare() (then, the "old" signature
> > value
> > > > will
> > > > > > be
> > > > > > > >   automatically saved in the image)
> > > > > > > > * compare it with a the "new" value read from the BIOS in
> > > > > > > >   acpi_hibernation_leave() and panic if there's a mismatch
> > > > > > > > * add a configuration option to disable this behavior
> > (just in
> > > > > > case)
> > > > > > > > This way we can avoid modifying the entire generic
> > interface
> > > > to
> > > > > > add
> > > > > > > > the feature
> > > > > > > > specific to ACPI.
> > > > > > > it would be better we do the check in boot kernel.
> > > > > > Franky, I think we should also check in the image kernel, in
> > case
> > > > the
> > > > > > boot
> > > > > > one doesn't support ACPI as I said.
> > > > > Ok, makes sense. I changed to check the signature
> > > > in .higberation_leave
> > > >
> > > > Thanks, comments below.
> > > >
> > > > > Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> > > > > ---
> > > > >  drivers/acpi/sleep/main.c |   20 ++++++++++++++++++++
> > > > >  1 file changed, 20 insertions(+)
> > > > >
> > > > > Index: linux/drivers/acpi/sleep/main.c
> > > > >
> > ===================================================================
> > > > > --- linux.orig/drivers/acpi/sleep/main.c      2008-01-03
> > > > 13:37:08.000000000 +0800
> > > > > +++ linux/drivers/acpi/sleep/main.c   2008-01-04
> > 13:36:10.000000000
> > > > +0800
> > > > > @@ -256,6 +256,17 @@ static int acpi_hibernation_enter(void)
> > > > >       return ACPI_SUCCESS(status) ? 0 : -EFAULT;
> > > > >  }
> > > > >
> > > > > +static unsigned long s4_hardware_signature;
> > > > > +static struct acpi_table_facs *facs;
> > > > > +static int nosigcheck;
> > > >
> > > > Use bool perhaps?
> > > >
> > > > > +
> > > > > +static int __init acpi_s4_nosigcheck(char *str)
> > > > > +{
> > > > > +     nosigcheck = 1;
> > > >
> > > > And "true" here?
> > > >
> > > > > +     return 1;
> > > > > +}
> > > > > +__setup("acpi_s4_nosigcheck", acpi_s4_nosigcheck);
> > > > > +
> > > >
> > > > Please put this function at the end of the file.  Also, I'd call
> > it
> > > > "acpi_s4_nosig", but whatever.
> > > Fixed all except this one, the routine is defined with HIBERATION
> > > configed.
> > 
> > I've just realized that we're not doing the right thing here ...
> > 
> > > Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> > > ---
> > >  drivers/acpi/sleep/main.c |   22 ++++++++++++++++++++++
> > >  1 file changed, 22 insertions(+)
> > >
> > > Index: linux/drivers/acpi/sleep/main.c
> > > ===================================================================
> > > --- linux.orig/drivers/acpi/sleep/main.c      2008-01-04
> > 13:44:40.000000000 +0800
> > > +++ linux/drivers/acpi/sleep/main.c   2008-01-07 09:31:42.000000000
> > +0800
> > > @@ -256,6 +256,17 @@ static int acpi_hibernation_enter(void)
> > >       return ACPI_SUCCESS(status) ? 0 : -EFAULT;
> > >  }
> > > 
> > > +static unsigned long s4_hardware_signature;
> > > +static struct acpi_table_facs *facs;
> > > +static bool nosigcheck;
> > > +
> > > +static int __init acpi_s4_nosigcheck(char *str)
> > > +{
> > > +     nosigcheck = true;
> > > +     return 1;
> > > +}
> > > +__setup("acpi_s4_nosigcheck", acpi_s4_nosigcheck);
> > > +
> > >  static void acpi_hibernation_leave(void)
> > >  {
> > >       /*
> > > @@ -263,6 +274,10 @@ static void acpi_hibernation_leave(void)
> > >        * enable it here.
> > >        */
> > >       acpi_enable();
> > > +     if (facs && s4_hardware_signature != facs->hardware_signature)
> > {
> > 
> > ... because we should read the signature from the hardware here, while
> > we're
> > comparing two values read from memory.  They will always be equal. :-)
> No, facs isn't a copy of ACPI FACS table (imagine how we sets
> facs->waking_vector). It's mapped to BIOS table.

Ah, ok.  Thanks for the clarification.

I'll rebase your patch on top of the suspend branch.

Greetings,
Rafael