Re: [PATCH v2 09/16] PM / hibernate: Reserve hibernation key and erase footprints
From: Matt Fleming <hidden>
Date: 2015-08-21 13:27:58
Also in:
linux-efi, lkml
On Tue, 11 Aug, at 02:16:29PM, Lee, Chun-Yi wrote:
Add handler to parse the setup data that carrying hibernation key, it reserves hibernation key by memblock then copies key to a allocated page in later initcall stage. And for erasing footprints, the codes in this patch remove setup data that carried hibernation key, and clean the memory space that reserved by memblock. Reviewed-by: Jiri Kosina <jkosina-IBi9RG/b67k@public.gmane.org> Tested-by: Jiri Kosina <jkosina-IBi9RG/b67k@public.gmane.org> Signed-off-by: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org> --- arch/x86/include/asm/suspend.h | 4 +++ arch/x86/kernel/setup.c | 21 ++++++++++- arch/x86/power/Makefile | 1 + arch/x86/power/hibernate_keys.c | 78 +++++++++++++++++++++++++++++++++++++++++ kernel/power/power.h | 5 +++ 5 files changed, 108 insertions(+), 1 deletion(-) create mode 100644 arch/x86/power/hibernate_keys.c
[...]
quoted hunk ↗ jump to hunk
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c index 80f874b..b345359 100644 --- a/arch/x86/kernel/setup.c +++ b/arch/x86/kernel/setup.c@@ -112,6 +112,8 @@ #include <asm/alternative.h> #include <asm/prom.h> +#include <asm/suspend.h> + /* * max_low_pfn_mapped: highest direct mapped pfn under 4GB * max_pfn_mapped: highest direct mapped pfn over 4GB@@ -425,10 +427,22 @@ static void __init reserve_initrd(void) } #endif /* CONFIG_BLK_DEV_INITRD */ +static void __init remove_setup_data(u64 pa_prev, u64 pa_next) +{ + struct setup_data *data; + + if (pa_prev) { + data = early_memremap(pa_prev, sizeof(*data)); + data->next = pa_next; + early_iounmap(data, sizeof(*data));
This should be early_memunmap for consistency().
quoted hunk ↗ jump to hunk
diff --git a/arch/x86/power/hibernate_keys.c b/arch/x86/power/hibernate_keys.c new file mode 100644 index 0000000..357dc0e --- /dev/null +++ b/arch/x86/power/hibernate_keys.c@@ -0,0 +1,78 @@ +/* Hibernation keys handler + * + * Copyright (C) 2015 Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org> + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public Licence + * as published by the Free Software Foundation; either version + * 2 of the Licence, or (at your option) any later version. + */ + +#include <linux/bootmem.h> +#include <linux/memblock.h> +#include <linux/suspend.h> +#include <asm/suspend.h> + +/* physical address of hibernation keys from boot params */ +static u64 keys_phys_addr; + +/* A page used to keep hibernation keys */ +static struct hibernation_keys *hibernation_keys; + +void __init parse_hibernation_keys(u64 phys_addr, u32 data_len) +{ + struct setup_data *hibernation_setup_data; + + /* Reserve keys memory, will copy and erase in init_hibernation_keys() */ + keys_phys_addr = phys_addr + sizeof(struct setup_data); + memblock_reserve(keys_phys_addr, sizeof(struct hibernation_keys)); + + /* clear hibernation_data */ + hibernation_setup_data = early_memremap(phys_addr, data_len); + if (!hibernation_setup_data) + return; + + memset(hibernation_setup_data, 0, sizeof(struct setup_data));
Why is this necessary? You're only clearing the struct setup_data fields and you unlinked this setup_data entry in remove_setup_data() anyway.
+ early_memunmap(hibernation_setup_data, data_len);
+}
+
+int get_hibernation_key(u8 **hkey)
+{
+ if (!hibernation_keys)
+ return -ENODEV;
+
+ if (!hibernation_keys->hkey_status)
+ *hkey = hibernation_keys->hibernation_key;
+
+ return hibernation_keys->hkey_status;
+}For global functions like this it's usually much preferred to prefix the name with the subsystem, i.e. hibernation_get_key().
+static int __init init_hibernation_keys(void)
+{
+ struct hibernation_keys *keys;
+ int ret = 0;
+
+ if (!keys_phys_addr)
+ return -ENODEV;
+
+ keys = early_memremap(keys_phys_addr, sizeof(struct hibernation_keys));
+
+ /* Copy hibernation keys to a allocated page */
+ hibernation_keys = (struct hibernation_keys *)get_zeroed_page(GFP_KERNEL);
+ if (hibernation_keys) {
+ *hibernation_keys = *keys;
+ } else {
+ pr_err("PM: Allocate hibernation keys page failed\n");
+ ret = -ENOMEM;
+ }It seems overkill to allocate an entire page for 28 bytes of data. -- Matt Fleming, Intel Open Source Technology Center