Re: [PATCH v2 09/16] PM / hibernate: Reserve hibernation key and erase footprints
On Wed, Sep 09, 2015 at 01:24:08PM +0100, Matt Fleming wrote:
> On Thu, 27 Aug, at 06:21:44PM, joeyli wrote:
> > On Fri, Aug 21, 2015 at 02:27:53PM +0100, Matt Fleming wrote:
> > > On Tue, 11 Aug, at 02:16:29PM, Lee, Chun-Yi wrote:
> > > > +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.
> > >
> >
> > Here to allocate an entire page because the basic unit is 'page' when
> > hibernation code checking saveable page that should included/excluded in
> > snapshot image.
> > Allocating an page to hibernation key can clearer to check the page should
> > excluded in snapshot image.
>
> Do other pieces of code do something similar, i.e. allocate an entire
> page because the hibernation code deals with pages? If so, it might be
> possible to lump all these pieces of data together into a single
> 'auxiliary' page.
>
> --
> Matt Fleming, Intel Open Source Technology Center
Thanks for your suggestions, I will lump all these data to into a single page
with the key. I want keep all the contents in the page only for hibernation's
usage but not to share with other subsystem in kernel, that's more safe to avoid
the key be accessed by with other code.
Thanks a lot!
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 09/16] PM / hibernate: Reserve hibernation key and erase footprints
On Thu, 27 Aug, at 06:21:44PM, joeyli wrote:
> On Fri, Aug 21, 2015 at 02:27:53PM +0100, Matt Fleming wrote:
> > On Tue, 11 Aug, at 02:16:29PM, Lee, Chun-Yi wrote:
> > > +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.
> >
>
> Here to allocate an entire page because the basic unit is 'page' when
> hibernation code checking saveable page that should included/excluded in
> snapshot image.
> Allocating an page to hibernation key can clearer to check the page should
> excluded in snapshot image.
Do other pieces of code do something similar, i.e. allocate an entire
page because the hibernation code deals with pages? If so, it might be
possible to lump all these pieces of data together into a single
'auxiliary' page.
--
Matt Fleming, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 09/16] PM / hibernate: Reserve hibernation key and erase footprints
On Fri, Aug 21, 2015 at 02:27:53PM +0100, Matt Fleming wrote:
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 jkos...@suse.com
Tested-by: Jiri Kosina jkos...@suse.com
Signed-off-by: Lee, Chun-Yi j...@suse.com
---
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
[...]
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().
I will use early_memunmap, thanks!
diff --git a/arch/x86/power/hibernate_keys.c
b/arch/x86/power/hibernate_keys.c
new file mode 100644
index 000..357dc0e
--- /dev/null
+++ b/arch/x86/power/hibernate_keys.c
@@ -0,0 +1,78 @@
+/* Hibernation keys handler
+ *
+ * Copyright (C) 2015 Lee, Chun-Yi j...@suse.com
+ *
+ * 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.
I want clean the setup_data fields that point to key data
before remove it from setup_data list.
+ 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().
I see, I will change the function name.
+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.
Here to allocate an entire page because the basic unit is 'page' when
hibernation code checking saveable page that should included/excluded in
snapshot image.
Allocating an page to hibernation key can clearer to check the page should
excluded in snapshot image.
--
Matt
Re: [PATCH v2 09/16] PM / hibernate: Reserve hibernation key and erase footprints
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 jkos...@suse.com
Tested-by: Jiri Kosina jkos...@suse.com
Signed-off-by: Lee, Chun-Yi j...@suse.com
---
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
[...]
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().
diff --git a/arch/x86/power/hibernate_keys.c b/arch/x86/power/hibernate_keys.c
new file mode 100644
index 000..357dc0e
--- /dev/null
+++ b/arch/x86/power/hibernate_keys.c
@@ -0,0 +1,78 @@
+/* Hibernation keys handler
+ *
+ * Copyright (C) 2015 Lee, Chun-Yi j...@suse.com
+ *
+ * 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
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 09/16] PM / hibernate: Reserve hibernation key and erase footprints
On Tue, Aug 11, 2015 at 02:16:29PM +0800, 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.
[...snip]
diff --git a/arch/x86/power/hibernate_keys.c b/arch/x86/power/hibernate_keys.c
new file mode 100644
index 000..357dc0e
--- /dev/null
+++ b/arch/x86/power/hibernate_keys.c
@@ -0,0 +1,78 @@
+/* Hibernation keys handler
+ *
[...snip]
+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;
+ }
+
+ /* Erase keys data no matter copy success or failed */
+ memset(keys, 0, sizeof(struct hibernation_keys));
+ early_memunmap(keys, sizeof(struct hibernation_keys));
+ memblock_free(keys_phys_addr, sizeof(struct hibernation_keys));
+ keys_phys_addr = 0;
+
+ return ret;
+}
+
+late_initcall(init_hibernation_keys);
Yu's reviewing triggered me rethinking...
I afraid that's too late in late_initcall stage for copying key data that
reserved by memblock. I will try to move this copy action to setup_arch().
Regards
Joey Lee
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 09/16] PM / hibernate: Reserve hibernation key and erase footprints
Hi Yu,
Thanks for your reviewing.
On Thu, Aug 13, 2015 at 02:45:32AM +, Chen, Yu C wrote:
Hi Chun-yi,
On Tue, 2015-08-11 at 14:16 +0800, Lee, Chun-Yi wrote:
+/* 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;
+
should we invoke memblock_free if early_memremap failed?
Best Regards,
Yu
Should not free memblock reservation of key data.
Using memblock_reserve is for reserve hibernation key until
init_hibernation_keys() copy key data to arbitrary allocated page.
Using early_memremap is for cleaning the setup_data header that's
used to carry key. That above 2 actions are different purposes.
So, even the action of cleaning setup_data failed, it doesn't
affect to the action for reserve hibernation key by memblock until
copy it.
Thanks a lot!
Joey Lee
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 09/16] PM / hibernate: Reserve hibernation key and erase footprints
Hi Chun-yi,
On Tue, 2015-08-11 at 14:16 +0800, Lee, Chun-Yi wrote:
+/* 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;
+
should we invoke memblock_free if early_memremap failed?
Best Regards,
Yu
