* [patch 0/1] [patch 0/1] include storage keys in hibernation image @ 2011-08-16 14:14 Martin Schwidefsky 2011-08-16 14:14 ` [patch 1/1] [PATCH] " Martin Schwidefsky 2011-08-16 14:14 ` Martin Schwidefsky 0 siblings, 2 replies; 35+ messages in thread From: Martin Schwidefsky @ 2011-08-16 14:14 UTC (permalink / raw) To: linux-pm, linux-kernel, linux-s390, Rafael J. Wysocki, Pavel Machek, Jiri Slaby Hi Rafael, Version 2 of the storage key patch for hibernation on s390. I've added the following changes as you requested: 1) moved the s390 specific code to arch/s390/kernel/suspend.c 2) introduced CONFIG_ARCH_SAVE_PAGE_KEYS and added a select to the main s390 Kconfig file 3) added the empty default functions to include/linux/suspend.h -- blue skies, Martin. "Reality continues to ruin my life." - Calvin. ^ permalink raw reply [flat|nested] 35+ messages in thread
* [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-08-16 14:14 [patch 0/1] [patch 0/1] include storage keys in hibernation image Martin Schwidefsky @ 2011-08-16 14:14 ` Martin Schwidefsky 2011-08-16 14:14 ` Martin Schwidefsky 1 sibling, 0 replies; 35+ messages in thread From: Martin Schwidefsky @ 2011-08-16 14:14 UTC (permalink / raw) To: linux-pm, linux-kernel, linux-s390, Rafael J. Wysocki, Pavel Machek, Jiri Cc: Martin Schwidefsky [-- Attachment #1: 201-hibernate-storage-keys.diff --] [-- Type: text/plain, Size: 9951 bytes --] From: Martin Schwidefsky <schwidefsky@de.ibm.com> For s390 there is one additional byte associated with each page, the storage key. This byte contains the referenced and changed bits and needs to be included into the hibernation image. If the storage keys are not restored to their previous state all original pages would appear to be dirty. This can cause inconsistencies e.g. with read-only filesystems. Cc: Pavel Machek <pavel@ucw.cz> Cc: Rafael J. Wysocki <rjw@sisk.pl> Cc: Jiri Slaby <jslaby@suse.cz> Cc: linux-pm@lists.linux-foundation.org Cc: linux-kernel@vger.kernel.org Cc: linux-s390@vger.kernel.org Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com> --- arch/s390/Kconfig | 1 arch/s390/kernel/suspend.c | 118 ++++++++++++++++++++++++++++++++++++++++ arch/s390/kernel/swsusp_asm64.S | 3 + include/linux/suspend.h | 34 +++++++++++ kernel/power/Kconfig | 3 + kernel/power/snapshot.c | 18 ++++++ 6 files changed, 177 insertions(+) Index: hibernate-2.6/arch/s390/Kconfig =================================================================== --- hibernate-2.6.orig/arch/s390/Kconfig 2011-08-09 17:40:09.220010373 +0200 +++ hibernate-2.6/arch/s390/Kconfig 2011-08-16 16:06:09.845540931 +0200 @@ -91,6 +91,7 @@ select HAVE_ARCH_MUTEX_CPU_RELAX select HAVE_ARCH_JUMP_LABEL if !MARCH_G5 select HAVE_RCU_TABLE_FREE if SMP + select ARCH_SAVE_PAGE_KEYS if HIBERNATION select ARCH_INLINE_SPIN_TRYLOCK select ARCH_INLINE_SPIN_TRYLOCK_BH select ARCH_INLINE_SPIN_LOCK Index: hibernate-2.6/arch/s390/kernel/suspend.c =================================================================== --- hibernate-2.6.orig/arch/s390/kernel/suspend.c 2009-09-24 09:06:44.000000000 +0200 +++ hibernate-2.6/arch/s390/kernel/suspend.c 2011-08-16 16:06:09.845540931 +0200 @@ -7,6 +7,7 @@ */ #include <linux/pfn.h> +#include <linux/mm.h> #include <asm/system.h> /* @@ -14,6 +15,123 @@ */ extern const void __nosave_begin, __nosave_end; +/* + * The restore of the saved pages in an hibernation image will set + * the change and referenced bits in the storage key for each page. + * Overindication of the referenced bits after an hibernation cycle + * does not cause any harm but the overindication of the change bits + * would cause trouble. + * Use the ARCH_SAVE_PAGE_KEYS hooks to save the storage key of each + * page to the most significant byte of the associated page frame + * number in the hibernation image. + */ + +/* + * Key storage is allocated as a linked list of pages. + * The size of the keys array is (PAGE_SIZE - sizeof(long)) + */ +struct page_key_data { + struct page_key_data *next; + unsigned char data[]; +}; + +#define PAGE_KEY_DATA_SIZE (PAGE_SIZE - sizeof(struct page_key_data *)) + +static struct page_key_data *page_key_data; +static struct page_key_data *page_key_rp, *page_key_wp; +static unsigned long page_key_rx, page_key_wx; + +/* + * For each page in the hibernation image one additional byte is + * stored in the most significant byte of the page frame number. + * On suspend no additional memory is required but on resume the + * keys need to be memorized until the page data has been restored. + * Only then can the storage keys be set to their old state. + */ +unsigned long page_key_additional_pages(unsigned long pages) +{ + return DIV_ROUND_UP(pages, PAGE_KEY_DATA_SIZE); +} + +/* + * Free page_key_data list of arrays. + */ +void page_key_free(void) +{ + struct page_key_data *pkd; + + while (page_key_data) { + pkd = page_key_data; + page_key_data = pkd->next; + free_page((unsigned long) pkd); + } +} + +/* + * Allocate page_key_data list of arrays with enough room to store + * one byte for each page in the hibernation image. + */ +int page_key_alloc(unsigned long pages) +{ + struct page_key_data *pk; + unsigned long size; + + size = DIV_ROUND_UP(pages, PAGE_KEY_DATA_SIZE); + while (size--) { + pk = (struct page_key_data *) get_zeroed_page(GFP_KERNEL); + if (!pk) { + page_key_free(); + return -ENOMEM; + } + pk->next = page_key_data; + page_key_data = pk; + } + page_key_rp = page_key_wp = page_key_data; + page_key_rx = page_key_wx = 0; + return 0; +} + +/* + * Save the storage key into the upper 8 bits of the page frame number. + */ +void page_key_read(unsigned long *pfn) +{ + unsigned long addr; + + addr = (unsigned long) page_address(pfn_to_page(*pfn)); + *(unsigned char *) pfn = (unsigned char) page_get_storage_key(addr); +} + +/* + * Extract the storage key from the upper 8 bits of the page frame number + * and store it in the page_key_data list of arrays. + */ +void page_key_memorize(unsigned long *pfn) +{ + page_key_wp->data[page_key_wx] = *(unsigned char *) pfn; + *(unsigned char *) pfn = 0; + if (++page_key_wx < PAGE_KEY_DATA_SIZE) + return; + page_key_wp = page_key_wp->next; + page_key_wx = 0; +} + +/* + * Get the next key from the page_key_data list of arrays and set the + * storage key of the page referred by @address. If @address refers to + * a "safe" page the swsusp_arch_resume code will transfer the storage + * key from the buffer page to the original page. + */ +void page_key_write(void *address) +{ + page_set_storage_key((unsigned long) address, + page_key_rp->data[page_key_rx], 0); + if (++page_key_rx >= PAGE_KEY_DATA_SIZE) + return; + page_key_rp = page_key_rp->next; + page_key_rx = 0; +} + int pfn_is_nosave(unsigned long pfn) { unsigned long nosave_begin_pfn = PFN_DOWN(__pa(&__nosave_begin)); Index: hibernate-2.6/arch/s390/kernel/swsusp_asm64.S =================================================================== --- hibernate-2.6.orig/arch/s390/kernel/swsusp_asm64.S 2011-08-09 17:40:09.228010544 +0200 +++ hibernate-2.6/arch/s390/kernel/swsusp_asm64.S 2011-08-16 16:06:09.845540931 +0200 @@ -136,11 +136,14 @@ 0: lg %r2,8(%r1) lg %r4,0(%r1) + iske %r0,%r4 lghi %r3,PAGE_SIZE lghi %r5,PAGE_SIZE 1: mvcle %r2,%r4,0 jo 1b + lg %r2,8(%r1) + sske %r0,%r2 lg %r1,16(%r1) ltgr %r1,%r1 jnz 0b Index: hibernate-2.6/include/linux/suspend.h =================================================================== --- hibernate-2.6.orig/include/linux/suspend.h 2011-08-09 17:40:10.164030551 +0200 +++ hibernate-2.6/include/linux/suspend.h 2011-08-16 16:06:09.845540931 +0200 @@ -334,4 +334,38 @@ } #endif +#ifdef CONFIG_ARCH_SAVE_PAGE_KEYS +/* + * The ARCH_SAVE_PAGE_KEYS functions can be used by an architecture + * to save/restore additional information to/from the array of page + * frame numbers in the hibernation image. For s390 this is used to + * save and restore the storage key for each page that is included + * in the hibernation image. + */ +unsigned long page_key_additional_pages(unsigned long pages); +int page_key_alloc(unsigned long pages); +void page_key_free(void); +void page_key_read(unsigned long *pfn); +void page_key_memorize(unsigned long *pfn); +void page_key_write(void *address); + +#else /* !CONFIG_ARCH_SAVE_PAGE_KEYS */ + +static inline unsigned long page_key_additional_pages(unsigned long pages) +{ + return 0; +} + +static inline int page_key_alloc(unsigned long pages) +{ + return 0; +} + +static inline void page_key_free(void) {} +static inline void page_key_read(unsigned long *pfn) {} +static inline void page_key_memorize(unsigned long *pfn) {} +static inline void page_key_write(void *address) {} + +#endif /* !CONFIG_ARCH_SAVE_PAGE_KEYS */ + #endif /* _LINUX_SUSPEND_H */ Index: hibernate-2.6/kernel/power/Kconfig =================================================================== --- hibernate-2.6.orig/kernel/power/Kconfig 2011-08-09 17:40:10.192031153 +0200 +++ hibernate-2.6/kernel/power/Kconfig 2011-08-16 16:06:09.845540931 +0200 @@ -65,6 +65,9 @@ For more information take a look at <file:Documentation/power/swsusp.txt>. +config ARCH_SAVE_PAGE_KEYS + bool + config PM_STD_PARTITION string "Default resume partition" depends on HIBERNATION Index: hibernate-2.6/kernel/power/snapshot.c =================================================================== --- hibernate-2.6.orig/kernel/power/snapshot.c 2011-07-08 11:25:06.293203483 +0200 +++ hibernate-2.6/kernel/power/snapshot.c 2011-08-16 16:06:09.845540931 +0200 @@ -1339,6 +1339,9 @@ count += highmem; count -= totalreserve_pages; + /* Add number of pages required for page keys (s390 only). */ + size += page_key_additional_pages(saveable); + /* Compute the maximum number of saveable pages to leave in memory. */ max_size = (count - (size + PAGES_FOR_IO)) / 2 - 2 * DIV_ROUND_UP(reserved_size, PAGE_SIZE); @@ -1662,6 +1665,8 @@ buf[j] = memory_bm_next_pfn(bm); if (unlikely(buf[j] == BM_END_OF_MAP)) break; + /* Save page key for data page (s390 only). */ + page_key_read(buf + j); } } @@ -1821,6 +1826,9 @@ if (unlikely(buf[j] == BM_END_OF_MAP)) break; + /* Extract and buffer page key for data page (s390 only). */ + page_key_memorize(buf + j); + if (memory_bm_pfn_present(bm, buf[j])) memory_bm_set_bit(bm, buf[j]); else @@ -2223,6 +2231,11 @@ if (error) return error; + /* Allocate buffer for page keys. */ + error = page_key_alloc(nr_copy_pages); + if (error) + return error; + } else if (handle->cur <= nr_meta_pages + 1) { error = unpack_orig_pfns(buffer, ©_bm); if (error) @@ -2243,6 +2256,8 @@ } } else { copy_last_highmem_page(); + /* Restore page key for data page (s390 only). */ + page_key_write(handle->buffer); handle->buffer = get_buffer(&orig_bm, &ca); if (IS_ERR(handle->buffer)) return PTR_ERR(handle->buffer); @@ -2264,6 +2279,9 @@ void snapshot_write_finalize(struct snapshot_handle *handle) { copy_last_highmem_page(); + /* Restore page key for data page (s390 only). */ + page_key_write(handle->buffer); + page_key_free(); /* Free only if we have loaded the image entirely */ if (handle->cur > 1 && handle->cur > nr_meta_pages + nr_copy_pages) { memory_bm_free(&orig_bm, PG_UNSAFE_CLEAR); ^ permalink raw reply [flat|nested] 35+ messages in thread
* [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-08-16 14:14 [patch 0/1] [patch 0/1] include storage keys in hibernation image Martin Schwidefsky 2011-08-16 14:14 ` [patch 1/1] [PATCH] " Martin Schwidefsky @ 2011-08-16 14:14 ` Martin Schwidefsky 2011-08-16 17:56 ` Rafael J. Wysocki 2011-08-16 17:56 ` Rafael J. Wysocki 1 sibling, 2 replies; 35+ messages in thread From: Martin Schwidefsky @ 2011-08-16 14:14 UTC (permalink / raw) To: linux-pm, linux-kernel, linux-s390, Rafael J. Wysocki, Pavel Machek, Jiri Slaby Cc: Martin Schwidefsky [-- Attachment #1: 201-hibernate-storage-keys.diff --] [-- Type: text/plain, Size: 9952 bytes --] From: Martin Schwidefsky <schwidefsky@de.ibm.com> For s390 there is one additional byte associated with each page, the storage key. This byte contains the referenced and changed bits and needs to be included into the hibernation image. If the storage keys are not restored to their previous state all original pages would appear to be dirty. This can cause inconsistencies e.g. with read-only filesystems. Cc: Pavel Machek <pavel@ucw.cz> Cc: Rafael J. Wysocki <rjw@sisk.pl> Cc: Jiri Slaby <jslaby@suse.cz> Cc: linux-pm@lists.linux-foundation.org Cc: linux-kernel@vger.kernel.org Cc: linux-s390@vger.kernel.org Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com> --- arch/s390/Kconfig | 1 arch/s390/kernel/suspend.c | 118 ++++++++++++++++++++++++++++++++++++++++ arch/s390/kernel/swsusp_asm64.S | 3 + include/linux/suspend.h | 34 +++++++++++ kernel/power/Kconfig | 3 + kernel/power/snapshot.c | 18 ++++++ 6 files changed, 177 insertions(+) Index: hibernate-2.6/arch/s390/Kconfig =================================================================== --- hibernate-2.6.orig/arch/s390/Kconfig 2011-08-09 17:40:09.220010373 +0200 +++ hibernate-2.6/arch/s390/Kconfig 2011-08-16 16:06:09.845540931 +0200 @@ -91,6 +91,7 @@ select HAVE_ARCH_MUTEX_CPU_RELAX select HAVE_ARCH_JUMP_LABEL if !MARCH_G5 select HAVE_RCU_TABLE_FREE if SMP + select ARCH_SAVE_PAGE_KEYS if HIBERNATION select ARCH_INLINE_SPIN_TRYLOCK select ARCH_INLINE_SPIN_TRYLOCK_BH select ARCH_INLINE_SPIN_LOCK Index: hibernate-2.6/arch/s390/kernel/suspend.c =================================================================== --- hibernate-2.6.orig/arch/s390/kernel/suspend.c 2009-09-24 09:06:44.000000000 +0200 +++ hibernate-2.6/arch/s390/kernel/suspend.c 2011-08-16 16:06:09.845540931 +0200 @@ -7,6 +7,7 @@ */ #include <linux/pfn.h> +#include <linux/mm.h> #include <asm/system.h> /* @@ -14,6 +15,123 @@ */ extern const void __nosave_begin, __nosave_end; +/* + * The restore of the saved pages in an hibernation image will set + * the change and referenced bits in the storage key for each page. + * Overindication of the referenced bits after an hibernation cycle + * does not cause any harm but the overindication of the change bits + * would cause trouble. + * Use the ARCH_SAVE_PAGE_KEYS hooks to save the storage key of each + * page to the most significant byte of the associated page frame + * number in the hibernation image. + */ + +/* + * Key storage is allocated as a linked list of pages. + * The size of the keys array is (PAGE_SIZE - sizeof(long)) + */ +struct page_key_data { + struct page_key_data *next; + unsigned char data[]; +}; + +#define PAGE_KEY_DATA_SIZE (PAGE_SIZE - sizeof(struct page_key_data *)) + +static struct page_key_data *page_key_data; +static struct page_key_data *page_key_rp, *page_key_wp; +static unsigned long page_key_rx, page_key_wx; + +/* + * For each page in the hibernation image one additional byte is + * stored in the most significant byte of the page frame number. + * On suspend no additional memory is required but on resume the + * keys need to be memorized until the page data has been restored. + * Only then can the storage keys be set to their old state. + */ +unsigned long page_key_additional_pages(unsigned long pages) +{ + return DIV_ROUND_UP(pages, PAGE_KEY_DATA_SIZE); +} + +/* + * Free page_key_data list of arrays. + */ +void page_key_free(void) +{ + struct page_key_data *pkd; + + while (page_key_data) { + pkd = page_key_data; + page_key_data = pkd->next; + free_page((unsigned long) pkd); + } +} + +/* + * Allocate page_key_data list of arrays with enough room to store + * one byte for each page in the hibernation image. + */ +int page_key_alloc(unsigned long pages) +{ + struct page_key_data *pk; + unsigned long size; + + size = DIV_ROUND_UP(pages, PAGE_KEY_DATA_SIZE); + while (size--) { + pk = (struct page_key_data *) get_zeroed_page(GFP_KERNEL); + if (!pk) { + page_key_free(); + return -ENOMEM; + } + pk->next = page_key_data; + page_key_data = pk; + } + page_key_rp = page_key_wp = page_key_data; + page_key_rx = page_key_wx = 0; + return 0; +} + +/* + * Save the storage key into the upper 8 bits of the page frame number. + */ +void page_key_read(unsigned long *pfn) +{ + unsigned long addr; + + addr = (unsigned long) page_address(pfn_to_page(*pfn)); + *(unsigned char *) pfn = (unsigned char) page_get_storage_key(addr); +} + +/* + * Extract the storage key from the upper 8 bits of the page frame number + * and store it in the page_key_data list of arrays. + */ +void page_key_memorize(unsigned long *pfn) +{ + page_key_wp->data[page_key_wx] = *(unsigned char *) pfn; + *(unsigned char *) pfn = 0; + if (++page_key_wx < PAGE_KEY_DATA_SIZE) + return; + page_key_wp = page_key_wp->next; + page_key_wx = 0; +} + +/* + * Get the next key from the page_key_data list of arrays and set the + * storage key of the page referred by @address. If @address refers to + * a "safe" page the swsusp_arch_resume code will transfer the storage + * key from the buffer page to the original page. + */ +void page_key_write(void *address) +{ + page_set_storage_key((unsigned long) address, + page_key_rp->data[page_key_rx], 0); + if (++page_key_rx >= PAGE_KEY_DATA_SIZE) + return; + page_key_rp = page_key_rp->next; + page_key_rx = 0; +} + int pfn_is_nosave(unsigned long pfn) { unsigned long nosave_begin_pfn = PFN_DOWN(__pa(&__nosave_begin)); Index: hibernate-2.6/arch/s390/kernel/swsusp_asm64.S =================================================================== --- hibernate-2.6.orig/arch/s390/kernel/swsusp_asm64.S 2011-08-09 17:40:09.228010544 +0200 +++ hibernate-2.6/arch/s390/kernel/swsusp_asm64.S 2011-08-16 16:06:09.845540931 +0200 @@ -136,11 +136,14 @@ 0: lg %r2,8(%r1) lg %r4,0(%r1) + iske %r0,%r4 lghi %r3,PAGE_SIZE lghi %r5,PAGE_SIZE 1: mvcle %r2,%r4,0 jo 1b + lg %r2,8(%r1) + sske %r0,%r2 lg %r1,16(%r1) ltgr %r1,%r1 jnz 0b Index: hibernate-2.6/include/linux/suspend.h =================================================================== --- hibernate-2.6.orig/include/linux/suspend.h 2011-08-09 17:40:10.164030551 +0200 +++ hibernate-2.6/include/linux/suspend.h 2011-08-16 16:06:09.845540931 +0200 @@ -334,4 +334,38 @@ } #endif +#ifdef CONFIG_ARCH_SAVE_PAGE_KEYS +/* + * The ARCH_SAVE_PAGE_KEYS functions can be used by an architecture + * to save/restore additional information to/from the array of page + * frame numbers in the hibernation image. For s390 this is used to + * save and restore the storage key for each page that is included + * in the hibernation image. + */ +unsigned long page_key_additional_pages(unsigned long pages); +int page_key_alloc(unsigned long pages); +void page_key_free(void); +void page_key_read(unsigned long *pfn); +void page_key_memorize(unsigned long *pfn); +void page_key_write(void *address); + +#else /* !CONFIG_ARCH_SAVE_PAGE_KEYS */ + +static inline unsigned long page_key_additional_pages(unsigned long pages) +{ + return 0; +} + +static inline int page_key_alloc(unsigned long pages) +{ + return 0; +} + +static inline void page_key_free(void) {} +static inline void page_key_read(unsigned long *pfn) {} +static inline void page_key_memorize(unsigned long *pfn) {} +static inline void page_key_write(void *address) {} + +#endif /* !CONFIG_ARCH_SAVE_PAGE_KEYS */ + #endif /* _LINUX_SUSPEND_H */ Index: hibernate-2.6/kernel/power/Kconfig =================================================================== --- hibernate-2.6.orig/kernel/power/Kconfig 2011-08-09 17:40:10.192031153 +0200 +++ hibernate-2.6/kernel/power/Kconfig 2011-08-16 16:06:09.845540931 +0200 @@ -65,6 +65,9 @@ For more information take a look at <file:Documentation/power/swsusp.txt>. +config ARCH_SAVE_PAGE_KEYS + bool + config PM_STD_PARTITION string "Default resume partition" depends on HIBERNATION Index: hibernate-2.6/kernel/power/snapshot.c =================================================================== --- hibernate-2.6.orig/kernel/power/snapshot.c 2011-07-08 11:25:06.293203483 +0200 +++ hibernate-2.6/kernel/power/snapshot.c 2011-08-16 16:06:09.845540931 +0200 @@ -1339,6 +1339,9 @@ count += highmem; count -= totalreserve_pages; + /* Add number of pages required for page keys (s390 only). */ + size += page_key_additional_pages(saveable); + /* Compute the maximum number of saveable pages to leave in memory. */ max_size = (count - (size + PAGES_FOR_IO)) / 2 - 2 * DIV_ROUND_UP(reserved_size, PAGE_SIZE); @@ -1662,6 +1665,8 @@ buf[j] = memory_bm_next_pfn(bm); if (unlikely(buf[j] == BM_END_OF_MAP)) break; + /* Save page key for data page (s390 only). */ + page_key_read(buf + j); } } @@ -1821,6 +1826,9 @@ if (unlikely(buf[j] == BM_END_OF_MAP)) break; + /* Extract and buffer page key for data page (s390 only). */ + page_key_memorize(buf + j); + if (memory_bm_pfn_present(bm, buf[j])) memory_bm_set_bit(bm, buf[j]); else @@ -2223,6 +2231,11 @@ if (error) return error; + /* Allocate buffer for page keys. */ + error = page_key_alloc(nr_copy_pages); + if (error) + return error; + } else if (handle->cur <= nr_meta_pages + 1) { error = unpack_orig_pfns(buffer, ©_bm); if (error) @@ -2243,6 +2256,8 @@ } } else { copy_last_highmem_page(); + /* Restore page key for data page (s390 only). */ + page_key_write(handle->buffer); handle->buffer = get_buffer(&orig_bm, &ca); if (IS_ERR(handle->buffer)) return PTR_ERR(handle->buffer); @@ -2264,6 +2279,9 @@ void snapshot_write_finalize(struct snapshot_handle *handle) { copy_last_highmem_page(); + /* Restore page key for data page (s390 only). */ + page_key_write(handle->buffer); + page_key_free(); /* Free only if we have loaded the image entirely */ if (handle->cur > 1 && handle->cur > nr_meta_pages + nr_copy_pages) { memory_bm_free(&orig_bm, PG_UNSAFE_CLEAR); ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-08-16 14:14 ` Martin Schwidefsky @ 2011-08-16 17:56 ` Rafael J. Wysocki 2011-08-16 17:56 ` Rafael J. Wysocki 1 sibling, 0 replies; 35+ messages in thread From: Rafael J. Wysocki @ 2011-08-16 17:56 UTC (permalink / raw) To: Martin Schwidefsky; +Cc: linux-s390, linux-pm, Jiri Slaby, linux-kernel On Tuesday, August 16, 2011, Martin Schwidefsky wrote: > From: Martin Schwidefsky <schwidefsky@de.ibm.com> > > For s390 there is one additional byte associated with each page, > the storage key. This byte contains the referenced and changed > bits and needs to be included into the hibernation image. > If the storage keys are not restored to their previous state all > original pages would appear to be dirty. This can cause > inconsistencies e.g. with read-only filesystems. > > Cc: Pavel Machek <pavel@ucw.cz> > Cc: Rafael J. Wysocki <rjw@sisk.pl> > Cc: Jiri Slaby <jslaby@suse.cz> > Cc: linux-pm@lists.linux-foundation.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-s390@vger.kernel.org > Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com> OK, I don't have any complaints. Do you want me to take this patch or do you want to push it through the s390 tree? Rafael > --- > > arch/s390/Kconfig | 1 > arch/s390/kernel/suspend.c | 118 ++++++++++++++++++++++++++++++++++++++++ > arch/s390/kernel/swsusp_asm64.S | 3 + > include/linux/suspend.h | 34 +++++++++++ > kernel/power/Kconfig | 3 + > kernel/power/snapshot.c | 18 ++++++ > 6 files changed, 177 insertions(+) > > Index: hibernate-2.6/arch/s390/Kconfig > =================================================================== > --- hibernate-2.6.orig/arch/s390/Kconfig 2011-08-09 17:40:09.220010373 +0200 > +++ hibernate-2.6/arch/s390/Kconfig 2011-08-16 16:06:09.845540931 +0200 > @@ -91,6 +91,7 @@ > select HAVE_ARCH_MUTEX_CPU_RELAX > select HAVE_ARCH_JUMP_LABEL if !MARCH_G5 > select HAVE_RCU_TABLE_FREE if SMP > + select ARCH_SAVE_PAGE_KEYS if HIBERNATION > select ARCH_INLINE_SPIN_TRYLOCK > select ARCH_INLINE_SPIN_TRYLOCK_BH > select ARCH_INLINE_SPIN_LOCK > Index: hibernate-2.6/arch/s390/kernel/suspend.c > =================================================================== > --- hibernate-2.6.orig/arch/s390/kernel/suspend.c 2009-09-24 09:06:44.000000000 +0200 > +++ hibernate-2.6/arch/s390/kernel/suspend.c 2011-08-16 16:06:09.845540931 +0200 > @@ -7,6 +7,7 @@ > */ > > #include <linux/pfn.h> > +#include <linux/mm.h> > #include <asm/system.h> > > /* > @@ -14,6 +15,123 @@ > */ > extern const void __nosave_begin, __nosave_end; > > +/* > + * The restore of the saved pages in an hibernation image will set > + * the change and referenced bits in the storage key for each page. > + * Overindication of the referenced bits after an hibernation cycle > + * does not cause any harm but the overindication of the change bits > + * would cause trouble. > + * Use the ARCH_SAVE_PAGE_KEYS hooks to save the storage key of each > + * page to the most significant byte of the associated page frame > + * number in the hibernation image. > + */ > + > +/* > + * Key storage is allocated as a linked list of pages. > + * The size of the keys array is (PAGE_SIZE - sizeof(long)) > + */ > +struct page_key_data { > + struct page_key_data *next; > + unsigned char data[]; > +}; > + > +#define PAGE_KEY_DATA_SIZE (PAGE_SIZE - sizeof(struct page_key_data *)) > + > +static struct page_key_data *page_key_data; > +static struct page_key_data *page_key_rp, *page_key_wp; > +static unsigned long page_key_rx, page_key_wx; > + > +/* > + * For each page in the hibernation image one additional byte is > + * stored in the most significant byte of the page frame number. > + * On suspend no additional memory is required but on resume the > + * keys need to be memorized until the page data has been restored. > + * Only then can the storage keys be set to their old state. > + */ > +unsigned long page_key_additional_pages(unsigned long pages) > +{ > + return DIV_ROUND_UP(pages, PAGE_KEY_DATA_SIZE); > +} > + > +/* > + * Free page_key_data list of arrays. > + */ > +void page_key_free(void) > +{ > + struct page_key_data *pkd; > + > + while (page_key_data) { > + pkd = page_key_data; > + page_key_data = pkd->next; > + free_page((unsigned long) pkd); > + } > +} > + > +/* > + * Allocate page_key_data list of arrays with enough room to store > + * one byte for each page in the hibernation image. > + */ > +int page_key_alloc(unsigned long pages) > +{ > + struct page_key_data *pk; > + unsigned long size; > + > + size = DIV_ROUND_UP(pages, PAGE_KEY_DATA_SIZE); > + while (size--) { > + pk = (struct page_key_data *) get_zeroed_page(GFP_KERNEL); > + if (!pk) { > + page_key_free(); > + return -ENOMEM; > + } > + pk->next = page_key_data; > + page_key_data = pk; > + } > + page_key_rp = page_key_wp = page_key_data; > + page_key_rx = page_key_wx = 0; > + return 0; > +} > + > +/* > + * Save the storage key into the upper 8 bits of the page frame number. > + */ > +void page_key_read(unsigned long *pfn) > +{ > + unsigned long addr; > + > + addr = (unsigned long) page_address(pfn_to_page(*pfn)); > + *(unsigned char *) pfn = (unsigned char) page_get_storage_key(addr); > +} > + > +/* > + * Extract the storage key from the upper 8 bits of the page frame number > + * and store it in the page_key_data list of arrays. > + */ > +void page_key_memorize(unsigned long *pfn) > +{ > + page_key_wp->data[page_key_wx] = *(unsigned char *) pfn; > + *(unsigned char *) pfn = 0; > + if (++page_key_wx < PAGE_KEY_DATA_SIZE) > + return; > + page_key_wp = page_key_wp->next; > + page_key_wx = 0; > +} > + > +/* > + * Get the next key from the page_key_data list of arrays and set the > + * storage key of the page referred by @address. If @address refers to > + * a "safe" page the swsusp_arch_resume code will transfer the storage > + * key from the buffer page to the original page. > + */ > +void page_key_write(void *address) > +{ > + page_set_storage_key((unsigned long) address, > + page_key_rp->data[page_key_rx], 0); > + if (++page_key_rx >= PAGE_KEY_DATA_SIZE) > + return; > + page_key_rp = page_key_rp->next; > + page_key_rx = 0; > +} > + > int pfn_is_nosave(unsigned long pfn) > { > unsigned long nosave_begin_pfn = PFN_DOWN(__pa(&__nosave_begin)); > Index: hibernate-2.6/arch/s390/kernel/swsusp_asm64.S > =================================================================== > --- hibernate-2.6.orig/arch/s390/kernel/swsusp_asm64.S 2011-08-09 17:40:09.228010544 +0200 > +++ hibernate-2.6/arch/s390/kernel/swsusp_asm64.S 2011-08-16 16:06:09.845540931 +0200 > @@ -136,11 +136,14 @@ > 0: > lg %r2,8(%r1) > lg %r4,0(%r1) > + iske %r0,%r4 > lghi %r3,PAGE_SIZE > lghi %r5,PAGE_SIZE > 1: > mvcle %r2,%r4,0 > jo 1b > + lg %r2,8(%r1) > + sske %r0,%r2 > lg %r1,16(%r1) > ltgr %r1,%r1 > jnz 0b > Index: hibernate-2.6/include/linux/suspend.h > =================================================================== > --- hibernate-2.6.orig/include/linux/suspend.h 2011-08-09 17:40:10.164030551 +0200 > +++ hibernate-2.6/include/linux/suspend.h 2011-08-16 16:06:09.845540931 +0200 > @@ -334,4 +334,38 @@ > } > #endif > > +#ifdef CONFIG_ARCH_SAVE_PAGE_KEYS > +/* > + * The ARCH_SAVE_PAGE_KEYS functions can be used by an architecture > + * to save/restore additional information to/from the array of page > + * frame numbers in the hibernation image. For s390 this is used to > + * save and restore the storage key for each page that is included > + * in the hibernation image. > + */ > +unsigned long page_key_additional_pages(unsigned long pages); > +int page_key_alloc(unsigned long pages); > +void page_key_free(void); > +void page_key_read(unsigned long *pfn); > +void page_key_memorize(unsigned long *pfn); > +void page_key_write(void *address); > + > +#else /* !CONFIG_ARCH_SAVE_PAGE_KEYS */ > + > +static inline unsigned long page_key_additional_pages(unsigned long pages) > +{ > + return 0; > +} > + > +static inline int page_key_alloc(unsigned long pages) > +{ > + return 0; > +} > + > +static inline void page_key_free(void) {} > +static inline void page_key_read(unsigned long *pfn) {} > +static inline void page_key_memorize(unsigned long *pfn) {} > +static inline void page_key_write(void *address) {} > + > +#endif /* !CONFIG_ARCH_SAVE_PAGE_KEYS */ > + > #endif /* _LINUX_SUSPEND_H */ > Index: hibernate-2.6/kernel/power/Kconfig > =================================================================== > --- hibernate-2.6.orig/kernel/power/Kconfig 2011-08-09 17:40:10.192031153 +0200 > +++ hibernate-2.6/kernel/power/Kconfig 2011-08-16 16:06:09.845540931 +0200 > @@ -65,6 +65,9 @@ > > For more information take a look at <file:Documentation/power/swsusp.txt>. > > +config ARCH_SAVE_PAGE_KEYS > + bool > + > config PM_STD_PARTITION > string "Default resume partition" > depends on HIBERNATION > Index: hibernate-2.6/kernel/power/snapshot.c > =================================================================== > --- hibernate-2.6.orig/kernel/power/snapshot.c 2011-07-08 11:25:06.293203483 +0200 > +++ hibernate-2.6/kernel/power/snapshot.c 2011-08-16 16:06:09.845540931 +0200 > @@ -1339,6 +1339,9 @@ > count += highmem; > count -= totalreserve_pages; > > + /* Add number of pages required for page keys (s390 only). */ > + size += page_key_additional_pages(saveable); > + > /* Compute the maximum number of saveable pages to leave in memory. */ > max_size = (count - (size + PAGES_FOR_IO)) / 2 > - 2 * DIV_ROUND_UP(reserved_size, PAGE_SIZE); > @@ -1662,6 +1665,8 @@ > buf[j] = memory_bm_next_pfn(bm); > if (unlikely(buf[j] == BM_END_OF_MAP)) > break; > + /* Save page key for data page (s390 only). */ > + page_key_read(buf + j); > } > } > > @@ -1821,6 +1826,9 @@ > if (unlikely(buf[j] == BM_END_OF_MAP)) > break; > > + /* Extract and buffer page key for data page (s390 only). */ > + page_key_memorize(buf + j); > + > if (memory_bm_pfn_present(bm, buf[j])) > memory_bm_set_bit(bm, buf[j]); > else > @@ -2223,6 +2231,11 @@ > if (error) > return error; > > + /* Allocate buffer for page keys. */ > + error = page_key_alloc(nr_copy_pages); > + if (error) > + return error; > + > } else if (handle->cur <= nr_meta_pages + 1) { > error = unpack_orig_pfns(buffer, ©_bm); > if (error) > @@ -2243,6 +2256,8 @@ > } > } else { > copy_last_highmem_page(); > + /* Restore page key for data page (s390 only). */ > + page_key_write(handle->buffer); > handle->buffer = get_buffer(&orig_bm, &ca); > if (IS_ERR(handle->buffer)) > return PTR_ERR(handle->buffer); > @@ -2264,6 +2279,9 @@ > void snapshot_write_finalize(struct snapshot_handle *handle) > { > copy_last_highmem_page(); > + /* Restore page key for data page (s390 only). */ > + page_key_write(handle->buffer); > + page_key_free(); > /* Free only if we have loaded the image entirely */ > if (handle->cur > 1 && handle->cur > nr_meta_pages + nr_copy_pages) { > memory_bm_free(&orig_bm, PG_UNSAFE_CLEAR); > > > ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-08-16 14:14 ` Martin Schwidefsky 2011-08-16 17:56 ` Rafael J. Wysocki @ 2011-08-16 17:56 ` Rafael J. Wysocki 2011-08-17 7:43 ` Martin Schwidefsky 2011-08-17 7:43 ` Martin Schwidefsky 1 sibling, 2 replies; 35+ messages in thread From: Rafael J. Wysocki @ 2011-08-16 17:56 UTC (permalink / raw) To: Martin Schwidefsky Cc: linux-pm, linux-kernel, linux-s390, Pavel Machek, Jiri Slaby On Tuesday, August 16, 2011, Martin Schwidefsky wrote: > From: Martin Schwidefsky <schwidefsky@de.ibm.com> > > For s390 there is one additional byte associated with each page, > the storage key. This byte contains the referenced and changed > bits and needs to be included into the hibernation image. > If the storage keys are not restored to their previous state all > original pages would appear to be dirty. This can cause > inconsistencies e.g. with read-only filesystems. > > Cc: Pavel Machek <pavel@ucw.cz> > Cc: Rafael J. Wysocki <rjw@sisk.pl> > Cc: Jiri Slaby <jslaby@suse.cz> > Cc: linux-pm@lists.linux-foundation.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-s390@vger.kernel.org > Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com> OK, I don't have any complaints. Do you want me to take this patch or do you want to push it through the s390 tree? Rafael > --- > > arch/s390/Kconfig | 1 > arch/s390/kernel/suspend.c | 118 ++++++++++++++++++++++++++++++++++++++++ > arch/s390/kernel/swsusp_asm64.S | 3 + > include/linux/suspend.h | 34 +++++++++++ > kernel/power/Kconfig | 3 + > kernel/power/snapshot.c | 18 ++++++ > 6 files changed, 177 insertions(+) > > Index: hibernate-2.6/arch/s390/Kconfig > =================================================================== > --- hibernate-2.6.orig/arch/s390/Kconfig 2011-08-09 17:40:09.220010373 +0200 > +++ hibernate-2.6/arch/s390/Kconfig 2011-08-16 16:06:09.845540931 +0200 > @@ -91,6 +91,7 @@ > select HAVE_ARCH_MUTEX_CPU_RELAX > select HAVE_ARCH_JUMP_LABEL if !MARCH_G5 > select HAVE_RCU_TABLE_FREE if SMP > + select ARCH_SAVE_PAGE_KEYS if HIBERNATION > select ARCH_INLINE_SPIN_TRYLOCK > select ARCH_INLINE_SPIN_TRYLOCK_BH > select ARCH_INLINE_SPIN_LOCK > Index: hibernate-2.6/arch/s390/kernel/suspend.c > =================================================================== > --- hibernate-2.6.orig/arch/s390/kernel/suspend.c 2009-09-24 09:06:44.000000000 +0200 > +++ hibernate-2.6/arch/s390/kernel/suspend.c 2011-08-16 16:06:09.845540931 +0200 > @@ -7,6 +7,7 @@ > */ > > #include <linux/pfn.h> > +#include <linux/mm.h> > #include <asm/system.h> > > /* > @@ -14,6 +15,123 @@ > */ > extern const void __nosave_begin, __nosave_end; > > +/* > + * The restore of the saved pages in an hibernation image will set > + * the change and referenced bits in the storage key for each page. > + * Overindication of the referenced bits after an hibernation cycle > + * does not cause any harm but the overindication of the change bits > + * would cause trouble. > + * Use the ARCH_SAVE_PAGE_KEYS hooks to save the storage key of each > + * page to the most significant byte of the associated page frame > + * number in the hibernation image. > + */ > + > +/* > + * Key storage is allocated as a linked list of pages. > + * The size of the keys array is (PAGE_SIZE - sizeof(long)) > + */ > +struct page_key_data { > + struct page_key_data *next; > + unsigned char data[]; > +}; > + > +#define PAGE_KEY_DATA_SIZE (PAGE_SIZE - sizeof(struct page_key_data *)) > + > +static struct page_key_data *page_key_data; > +static struct page_key_data *page_key_rp, *page_key_wp; > +static unsigned long page_key_rx, page_key_wx; > + > +/* > + * For each page in the hibernation image one additional byte is > + * stored in the most significant byte of the page frame number. > + * On suspend no additional memory is required but on resume the > + * keys need to be memorized until the page data has been restored. > + * Only then can the storage keys be set to their old state. > + */ > +unsigned long page_key_additional_pages(unsigned long pages) > +{ > + return DIV_ROUND_UP(pages, PAGE_KEY_DATA_SIZE); > +} > + > +/* > + * Free page_key_data list of arrays. > + */ > +void page_key_free(void) > +{ > + struct page_key_data *pkd; > + > + while (page_key_data) { > + pkd = page_key_data; > + page_key_data = pkd->next; > + free_page((unsigned long) pkd); > + } > +} > + > +/* > + * Allocate page_key_data list of arrays with enough room to store > + * one byte for each page in the hibernation image. > + */ > +int page_key_alloc(unsigned long pages) > +{ > + struct page_key_data *pk; > + unsigned long size; > + > + size = DIV_ROUND_UP(pages, PAGE_KEY_DATA_SIZE); > + while (size--) { > + pk = (struct page_key_data *) get_zeroed_page(GFP_KERNEL); > + if (!pk) { > + page_key_free(); > + return -ENOMEM; > + } > + pk->next = page_key_data; > + page_key_data = pk; > + } > + page_key_rp = page_key_wp = page_key_data; > + page_key_rx = page_key_wx = 0; > + return 0; > +} > + > +/* > + * Save the storage key into the upper 8 bits of the page frame number. > + */ > +void page_key_read(unsigned long *pfn) > +{ > + unsigned long addr; > + > + addr = (unsigned long) page_address(pfn_to_page(*pfn)); > + *(unsigned char *) pfn = (unsigned char) page_get_storage_key(addr); > +} > + > +/* > + * Extract the storage key from the upper 8 bits of the page frame number > + * and store it in the page_key_data list of arrays. > + */ > +void page_key_memorize(unsigned long *pfn) > +{ > + page_key_wp->data[page_key_wx] = *(unsigned char *) pfn; > + *(unsigned char *) pfn = 0; > + if (++page_key_wx < PAGE_KEY_DATA_SIZE) > + return; > + page_key_wp = page_key_wp->next; > + page_key_wx = 0; > +} > + > +/* > + * Get the next key from the page_key_data list of arrays and set the > + * storage key of the page referred by @address. If @address refers to > + * a "safe" page the swsusp_arch_resume code will transfer the storage > + * key from the buffer page to the original page. > + */ > +void page_key_write(void *address) > +{ > + page_set_storage_key((unsigned long) address, > + page_key_rp->data[page_key_rx], 0); > + if (++page_key_rx >= PAGE_KEY_DATA_SIZE) > + return; > + page_key_rp = page_key_rp->next; > + page_key_rx = 0; > +} > + > int pfn_is_nosave(unsigned long pfn) > { > unsigned long nosave_begin_pfn = PFN_DOWN(__pa(&__nosave_begin)); > Index: hibernate-2.6/arch/s390/kernel/swsusp_asm64.S > =================================================================== > --- hibernate-2.6.orig/arch/s390/kernel/swsusp_asm64.S 2011-08-09 17:40:09.228010544 +0200 > +++ hibernate-2.6/arch/s390/kernel/swsusp_asm64.S 2011-08-16 16:06:09.845540931 +0200 > @@ -136,11 +136,14 @@ > 0: > lg %r2,8(%r1) > lg %r4,0(%r1) > + iske %r0,%r4 > lghi %r3,PAGE_SIZE > lghi %r5,PAGE_SIZE > 1: > mvcle %r2,%r4,0 > jo 1b > + lg %r2,8(%r1) > + sske %r0,%r2 > lg %r1,16(%r1) > ltgr %r1,%r1 > jnz 0b > Index: hibernate-2.6/include/linux/suspend.h > =================================================================== > --- hibernate-2.6.orig/include/linux/suspend.h 2011-08-09 17:40:10.164030551 +0200 > +++ hibernate-2.6/include/linux/suspend.h 2011-08-16 16:06:09.845540931 +0200 > @@ -334,4 +334,38 @@ > } > #endif > > +#ifdef CONFIG_ARCH_SAVE_PAGE_KEYS > +/* > + * The ARCH_SAVE_PAGE_KEYS functions can be used by an architecture > + * to save/restore additional information to/from the array of page > + * frame numbers in the hibernation image. For s390 this is used to > + * save and restore the storage key for each page that is included > + * in the hibernation image. > + */ > +unsigned long page_key_additional_pages(unsigned long pages); > +int page_key_alloc(unsigned long pages); > +void page_key_free(void); > +void page_key_read(unsigned long *pfn); > +void page_key_memorize(unsigned long *pfn); > +void page_key_write(void *address); > + > +#else /* !CONFIG_ARCH_SAVE_PAGE_KEYS */ > + > +static inline unsigned long page_key_additional_pages(unsigned long pages) > +{ > + return 0; > +} > + > +static inline int page_key_alloc(unsigned long pages) > +{ > + return 0; > +} > + > +static inline void page_key_free(void) {} > +static inline void page_key_read(unsigned long *pfn) {} > +static inline void page_key_memorize(unsigned long *pfn) {} > +static inline void page_key_write(void *address) {} > + > +#endif /* !CONFIG_ARCH_SAVE_PAGE_KEYS */ > + > #endif /* _LINUX_SUSPEND_H */ > Index: hibernate-2.6/kernel/power/Kconfig > =================================================================== > --- hibernate-2.6.orig/kernel/power/Kconfig 2011-08-09 17:40:10.192031153 +0200 > +++ hibernate-2.6/kernel/power/Kconfig 2011-08-16 16:06:09.845540931 +0200 > @@ -65,6 +65,9 @@ > > For more information take a look at <file:Documentation/power/swsusp.txt>. > > +config ARCH_SAVE_PAGE_KEYS > + bool > + > config PM_STD_PARTITION > string "Default resume partition" > depends on HIBERNATION > Index: hibernate-2.6/kernel/power/snapshot.c > =================================================================== > --- hibernate-2.6.orig/kernel/power/snapshot.c 2011-07-08 11:25:06.293203483 +0200 > +++ hibernate-2.6/kernel/power/snapshot.c 2011-08-16 16:06:09.845540931 +0200 > @@ -1339,6 +1339,9 @@ > count += highmem; > count -= totalreserve_pages; > > + /* Add number of pages required for page keys (s390 only). */ > + size += page_key_additional_pages(saveable); > + > /* Compute the maximum number of saveable pages to leave in memory. */ > max_size = (count - (size + PAGES_FOR_IO)) / 2 > - 2 * DIV_ROUND_UP(reserved_size, PAGE_SIZE); > @@ -1662,6 +1665,8 @@ > buf[j] = memory_bm_next_pfn(bm); > if (unlikely(buf[j] == BM_END_OF_MAP)) > break; > + /* Save page key for data page (s390 only). */ > + page_key_read(buf + j); > } > } > > @@ -1821,6 +1826,9 @@ > if (unlikely(buf[j] == BM_END_OF_MAP)) > break; > > + /* Extract and buffer page key for data page (s390 only). */ > + page_key_memorize(buf + j); > + > if (memory_bm_pfn_present(bm, buf[j])) > memory_bm_set_bit(bm, buf[j]); > else > @@ -2223,6 +2231,11 @@ > if (error) > return error; > > + /* Allocate buffer for page keys. */ > + error = page_key_alloc(nr_copy_pages); > + if (error) > + return error; > + > } else if (handle->cur <= nr_meta_pages + 1) { > error = unpack_orig_pfns(buffer, ©_bm); > if (error) > @@ -2243,6 +2256,8 @@ > } > } else { > copy_last_highmem_page(); > + /* Restore page key for data page (s390 only). */ > + page_key_write(handle->buffer); > handle->buffer = get_buffer(&orig_bm, &ca); > if (IS_ERR(handle->buffer)) > return PTR_ERR(handle->buffer); > @@ -2264,6 +2279,9 @@ > void snapshot_write_finalize(struct snapshot_handle *handle) > { > copy_last_highmem_page(); > + /* Restore page key for data page (s390 only). */ > + page_key_write(handle->buffer); > + page_key_free(); > /* Free only if we have loaded the image entirely */ > if (handle->cur > 1 && handle->cur > nr_meta_pages + nr_copy_pages) { > memory_bm_free(&orig_bm, PG_UNSAFE_CLEAR); > > > ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-08-16 17:56 ` Rafael J. Wysocki @ 2011-08-17 7:43 ` Martin Schwidefsky 2011-08-17 7:43 ` Martin Schwidefsky 1 sibling, 0 replies; 35+ messages in thread From: Martin Schwidefsky @ 2011-08-17 7:43 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: linux-s390, linux-pm, Jiri Slaby, linux-kernel On Tue, 16 Aug 2011 19:56:47 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > On Tuesday, August 16, 2011, Martin Schwidefsky wrote: > > From: Martin Schwidefsky <schwidefsky@de.ibm.com> > > > > For s390 there is one additional byte associated with each page, > > the storage key. This byte contains the referenced and changed > > bits and needs to be included into the hibernation image. > > If the storage keys are not restored to their previous state all > > original pages would appear to be dirty. This can cause > > inconsistencies e.g. with read-only filesystems. > > > > Cc: Pavel Machek <pavel@ucw.cz> > > Cc: Rafael J. Wysocki <rjw@sisk.pl> > > Cc: Jiri Slaby <jslaby@suse.cz> > > Cc: linux-pm@lists.linux-foundation.org > > Cc: linux-kernel@vger.kernel.org > > Cc: linux-s390@vger.kernel.org > > Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com> > > OK, I don't have any complaints. Do you want me to take this > patch or do you want to push it through the s390 tree? The patch affects the common power management code. At minimum I would like an acked-by to take it into the s390 tree. I would feel more comfortable if you'd take it via the power management tree though. -- blue skies, Martin. "Reality continues to ruin my life." - Calvin. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-08-16 17:56 ` Rafael J. Wysocki 2011-08-17 7:43 ` Martin Schwidefsky @ 2011-08-17 7:43 ` Martin Schwidefsky 2011-08-17 9:15 ` Rafael J. Wysocki 2011-08-17 9:15 ` Rafael J. Wysocki 1 sibling, 2 replies; 35+ messages in thread From: Martin Schwidefsky @ 2011-08-17 7:43 UTC (permalink / raw) To: Rafael J. Wysocki Cc: linux-pm, linux-kernel, linux-s390, Pavel Machek, Jiri Slaby On Tue, 16 Aug 2011 19:56:47 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > On Tuesday, August 16, 2011, Martin Schwidefsky wrote: > > From: Martin Schwidefsky <schwidefsky@de.ibm.com> > > > > For s390 there is one additional byte associated with each page, > > the storage key. This byte contains the referenced and changed > > bits and needs to be included into the hibernation image. > > If the storage keys are not restored to their previous state all > > original pages would appear to be dirty. This can cause > > inconsistencies e.g. with read-only filesystems. > > > > Cc: Pavel Machek <pavel@ucw.cz> > > Cc: Rafael J. Wysocki <rjw@sisk.pl> > > Cc: Jiri Slaby <jslaby@suse.cz> > > Cc: linux-pm@lists.linux-foundation.org > > Cc: linux-kernel@vger.kernel.org > > Cc: linux-s390@vger.kernel.org > > Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com> > > OK, I don't have any complaints. Do you want me to take this > patch or do you want to push it through the s390 tree? The patch affects the common power management code. At minimum I would like an acked-by to take it into the s390 tree. I would feel more comfortable if you'd take it via the power management tree though. -- blue skies, Martin. "Reality continues to ruin my life." - Calvin. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-08-17 7:43 ` Martin Schwidefsky @ 2011-08-17 9:15 ` Rafael J. Wysocki 2011-08-17 9:15 ` Rafael J. Wysocki 1 sibling, 0 replies; 35+ messages in thread From: Rafael J. Wysocki @ 2011-08-17 9:15 UTC (permalink / raw) To: Martin Schwidefsky Cc: linux-pm, linux-kernel, linux-s390, Pavel Machek, Jiri Slaby On Wednesday, August 17, 2011, Martin Schwidefsky wrote: > On Tue, 16 Aug 2011 19:56:47 +0200 > "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > > > On Tuesday, August 16, 2011, Martin Schwidefsky wrote: > > > From: Martin Schwidefsky <schwidefsky@de.ibm.com> > > > > > > For s390 there is one additional byte associated with each page, > > > the storage key. This byte contains the referenced and changed > > > bits and needs to be included into the hibernation image. > > > If the storage keys are not restored to their previous state all > > > original pages would appear to be dirty. This can cause > > > inconsistencies e.g. with read-only filesystems. > > > > > > Cc: Pavel Machek <pavel@ucw.cz> > > > Cc: Rafael J. Wysocki <rjw@sisk.pl> > > > Cc: Jiri Slaby <jslaby@suse.cz> > > > Cc: linux-pm@lists.linux-foundation.org > > > Cc: linux-kernel@vger.kernel.org > > > Cc: linux-s390@vger.kernel.org > > > Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com> > > > > OK, I don't have any complaints. Do you want me to take this > > patch or do you want to push it through the s390 tree? > > The patch affects the common power management code. At minimum I > would like an acked-by to take it into the s390 tree. I would feel > more comfortable if you'd take it via the power management tree > though. OK, I will. Expect it to appear in 3.2. :-) Thanks, Rafael ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-08-17 7:43 ` Martin Schwidefsky 2011-08-17 9:15 ` Rafael J. Wysocki @ 2011-08-17 9:15 ` Rafael J. Wysocki 1 sibling, 0 replies; 35+ messages in thread From: Rafael J. Wysocki @ 2011-08-17 9:15 UTC (permalink / raw) To: Martin Schwidefsky; +Cc: linux-s390, linux-pm, Jiri Slaby, linux-kernel On Wednesday, August 17, 2011, Martin Schwidefsky wrote: > On Tue, 16 Aug 2011 19:56:47 +0200 > "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > > > On Tuesday, August 16, 2011, Martin Schwidefsky wrote: > > > From: Martin Schwidefsky <schwidefsky@de.ibm.com> > > > > > > For s390 there is one additional byte associated with each page, > > > the storage key. This byte contains the referenced and changed > > > bits and needs to be included into the hibernation image. > > > If the storage keys are not restored to their previous state all > > > original pages would appear to be dirty. This can cause > > > inconsistencies e.g. with read-only filesystems. > > > > > > Cc: Pavel Machek <pavel@ucw.cz> > > > Cc: Rafael J. Wysocki <rjw@sisk.pl> > > > Cc: Jiri Slaby <jslaby@suse.cz> > > > Cc: linux-pm@lists.linux-foundation.org > > > Cc: linux-kernel@vger.kernel.org > > > Cc: linux-s390@vger.kernel.org > > > Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com> > > > > OK, I don't have any complaints. Do you want me to take this > > patch or do you want to push it through the s390 tree? > > The patch affects the common power management code. At minimum I > would like an acked-by to take it into the s390 tree. I would feel > more comfortable if you'd take it via the power management tree > though. OK, I will. Expect it to appear in 3.2. :-) Thanks, Rafael ^ permalink raw reply [flat|nested] 35+ messages in thread
* [patch 0/1] [RFC] include storage keys in hibernation image @ 2011-06-08 7:45 Martin Schwidefsky 2011-06-08 7:45 ` [patch 1/1] [PATCH] " Martin Schwidefsky 2011-06-08 7:45 ` Martin Schwidefsky 0 siblings, 2 replies; 35+ messages in thread From: Martin Schwidefsky @ 2011-06-08 7:45 UTC (permalink / raw) To: linux-pm, linux-kernel, linux-s390, Pavel Machek, Rafael J. Wysocki, Jiri Slaby Greetings, we have discovered a shortcoming of the s390 support for supend to disk. The problem is that we currently do not save and restore the storage keys - the one additional byte associated with each 4K page, a unique property of s390. The resume from disk will read the hibernation image from disk and restore the original pages. The I/O or the memory move for safe pages will set the referenced and the dirty bit in the storage key for every restored page. Without a reset to the state before the hibernation cycle the pages will appear to be dirty which causes problems for e.g. read-only filesystems. The solution implemented with this patch saves the storage key in the upper 8 bits of the page-frame-numbers. The code adds 6 new function in kernel/power/snapshot.c which are conditionally defined with CONFIG_S390. The call sites of these functions are scattered in the snapshot code which makes it hard to find a better abstraction for the interface. The comments should make clear what the functions do. -- blue skies, Martin. "Reality continues to ruin my life." - Calvin. ^ permalink raw reply [flat|nested] 35+ messages in thread
* [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-06-08 7:45 [patch 0/1] [RFC] " Martin Schwidefsky @ 2011-06-08 7:45 ` Martin Schwidefsky 2011-06-12 12:41 ` Rafael J. Wysocki 2011-07-28 22:01 ` Rafael J. Wysocki 2011-06-08 7:45 ` Martin Schwidefsky 1 sibling, 2 replies; 35+ messages in thread From: Martin Schwidefsky @ 2011-06-08 7:45 UTC (permalink / raw) To: linux-pm, linux-kernel, linux-s390, Pavel Machek, Rafael J. Wysocki, Jiri Slaby Cc: Martin Schwidefsky [-- Attachment #1: 201-hibernate-storage-keys.diff --] [-- Type: text/plain, Size: 7937 bytes --] From: Martin Schwidefsky <schwidefsky@de.ibm.com> For s390 there is one additional byte associated with each page, the storage key. This byte contains the referenced and changed bits and needs to be included into the hibernation image. If the storage keys are not restored to their previous state all original pages would appear to be dirty. This can cause inconsistencies e.g. with read-only filesystems. Cc: Pavel Machek <pavel@ucw.cz> Cc: Rafael J. Wysocki <rjw@sisk.pl> Cc: Jiri Slaby <jslaby@suse.cz> Cc: linux-pm@lists.linux-foundation.org Cc: linux-kernel@vger.kernel.org Cc: linux-s390@vger.kernel.org Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com> --- arch/s390/kernel/swsusp_asm64.S | 3 kernel/power/snapshot.c | 148 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 151 insertions(+) diff -urpN linux-2.6/arch/s390/kernel/swsusp_asm64.S linux-2.6-patched/arch/s390/kernel/swsusp_asm64.S --- linux-2.6/arch/s390/kernel/swsusp_asm64.S 2011-05-19 06:06:34.000000000 +0200 +++ linux-2.6-patched/arch/s390/kernel/swsusp_asm64.S 2011-06-06 11:14:43.000000000 +0200 @@ -138,11 +138,14 @@ swsusp_arch_resume: 0: lg %r2,8(%r1) lg %r4,0(%r1) + iske %r0,%r4 lghi %r3,PAGE_SIZE lghi %r5,PAGE_SIZE 1: mvcle %r2,%r4,0 jo 1b + lg %r2,8(%r1) + sske %r0,%r2 lg %r1,16(%r1) ltgr %r1,%r1 jnz 0b diff -urpN linux-2.6/kernel/power/snapshot.c linux-2.6-patched/kernel/power/snapshot.c --- linux-2.6/kernel/power/snapshot.c 2011-06-06 11:14:39.000000000 +0200 +++ linux-2.6-patched/kernel/power/snapshot.c 2011-06-06 11:14:43.000000000 +0200 @@ -1022,6 +1022,137 @@ static inline void copy_data_page(unsign } #endif /* CONFIG_HIGHMEM */ +#ifdef CONFIG_S390 +/* + * For s390 there is one additional byte associated with each page, + * the storage key. The key consists of the access-control bits + * (alias the key), the fetch-protection bit, the referenced bit + * and the change bit (alias dirty bit). Linux uses only the + * referenced bit and the change bit for pages in the page cache. + * Any modification of a page will set the change bit, any access + * sets the referenced bit. Overindication of the referenced bits + * after a hibernation cycle does not cause any harm but the + * overindication of the change bits would cause trouble. + * Therefore it is necessary to include the storage keys in the + * hibernation image. The storage keys are saved to the most + * significant byte of each page frame number in the list of pfns + * in the hibernation image. + */ + +/* + * Key storage is allocated as a linked list of pages. + * The size of the keys array is (PAGE_SIZE - sizeof(long)) + */ +struct page_key_data { + struct page_key_data *next; + unsigned char data[]; +}; + +#define PAGE_KEY_DATA_SIZE (PAGE_SIZE - sizeof(struct page_key_data *)) + +static struct page_key_data *page_key_data; +static struct page_key_data *page_key_rp, *page_key_wp; +static unsigned long page_key_rx, page_key_wx; + +/* + * For each page in the hibernation image one additional byte is + * stored in the most significant byte of the page frame number. + * On suspend no additional memory is required but on resume the + * keys need to be memorized until the page data has been restored. + * Only then can the storage keys be set to their old state. + */ +static inline unsigned long page_key_additional_pages(unsigned long pages) +{ + return DIV_ROUND_UP(pages, PAGE_KEY_DATA_SIZE); +} + +/* + * Free page_key_data list of arrays. + */ +static void page_key_free(void) +{ + struct page_key_data *pkd; + + while (page_key_data) { + pkd = page_key_data; + page_key_data = pkd->next; + free_page((unsigned long) pkd); + } +} + +/* + * Allocate page_key_data list of arrays with enough room to store + * one byte for each page in the hibernation image. + */ +static int page_key_alloc(unsigned long pages) +{ + struct page_key_data *pk; + unsigned long size; + + size = DIV_ROUND_UP(pages, PAGE_KEY_DATA_SIZE); + while (size--) { + pk = (struct page_key_data *) get_zeroed_page(GFP_KERNEL); + if (!pk) { + page_key_free(); + return -ENOMEM; + } + pk->next = page_key_data; + page_key_data = pk; + } + page_key_rp = page_key_wp = page_key_data; + page_key_rx = page_key_wx = 0; + return 0; +} + +/* + * Save the storage key into the upper 8 bits of the page frame number. + */ +static inline void page_key_read(unsigned long *pfn) +{ + unsigned long addr; + + addr = (unsigned long) page_address(pfn_to_page(*pfn)); + *(unsigned char *) pfn = (unsigned char) page_get_storage_key(addr); +} + +/* + * Extract the storage key from the upper 8 bits of the page frame number + * and store it in the page_key_data list of arrays. + */ +static inline void page_key_memorize(unsigned long *pfn) +{ + page_key_wp->data[page_key_wx] = *(unsigned char *) pfn; + *(unsigned char *) pfn = 0; + if (++page_key_wx < PAGE_KEY_DATA_SIZE) + return; + page_key_wp = page_key_wp->next; + page_key_wx = 0; +} + +/* + * Get the next key from the page_key_data list of arrays and set the + * storage key of the page referred by @address. If @address refers to + * a "safe" page the swsusp_arch_resume code will transfer the storage + * key from the buffer page to the original page. + */ +static void page_key_write(void *address) +{ + page_set_storage_key((unsigned long) address, + page_key_rp->data[page_key_rx], 0); + if (++page_key_rx >= PAGE_KEY_DATA_SIZE) + return; + page_key_rp = page_key_rp->next; + page_key_rx = 0; +} +#else +static unsigned long page_key_additional_pages(unsigned long pages) { return 0; } +static inline int page_key_alloc(unsigned long pages) { return 0; } +static inline void page_key_free(void) { } +static inline void page_key_read(unsigned long *pfn) { } +static inline void page_key_memorize(unsigned long *pfn) { } +static inline void page_key_write(void *address) { } +#endif + static void copy_data_pages(struct memory_bitmap *copy_bm, struct memory_bitmap *orig_bm) { @@ -1335,6 +1466,9 @@ int hibernate_preallocate_memory(void) count += highmem; count -= totalreserve_pages; + /* Add number of pages required for storage keys (s390 only). */ + size += page_key_additional_pages(saveable); + /* Compute the maximum number of saveable pages to leave in memory. */ max_size = (count - (size + PAGES_FOR_IO)) / 2 - 2 * DIV_ROUND_UP(reserved_size, PAGE_SIZE); @@ -1658,6 +1792,8 @@ pack_pfns(unsigned long *buf, struct mem buf[j] = memory_bm_next_pfn(bm); if (unlikely(buf[j] == BM_END_OF_MAP)) break; + /* Save storage key for data page (s390 only). */ + page_key_read(buf + j); } } @@ -1817,6 +1953,9 @@ static int unpack_orig_pfns(unsigned lon if (unlikely(buf[j] == BM_END_OF_MAP)) break; + /* Extract and buffer storage key for data page (s390 only). */ + page_key_memorize(buf + j); + if (memory_bm_pfn_present(bm, buf[j])) memory_bm_set_bit(bm, buf[j]); else @@ -2219,6 +2358,10 @@ int snapshot_write_next(struct snapshot_ if (error) return error; + error = page_key_alloc(nr_copy_pages); + if (error) + return error; + } else if (handle->cur <= nr_meta_pages + 1) { error = unpack_orig_pfns(buffer, ©_bm); if (error) @@ -2239,6 +2382,8 @@ int snapshot_write_next(struct snapshot_ } } else { copy_last_highmem_page(); + /* Restore storage key for data page (s390 only). */ + page_key_write(handle->buffer); handle->buffer = get_buffer(&orig_bm, &ca); if (IS_ERR(handle->buffer)) return PTR_ERR(handle->buffer); @@ -2260,6 +2405,9 @@ int snapshot_write_next(struct snapshot_ void snapshot_write_finalize(struct snapshot_handle *handle) { copy_last_highmem_page(); + /* Restore storage key for data page (s390 only). */ + page_key_write(handle->buffer); + page_key_free(); /* Free only if we have loaded the image entirely */ if (handle->cur > 1 && handle->cur > nr_meta_pages + nr_copy_pages) { memory_bm_free(&orig_bm, PG_UNSAFE_CLEAR); ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-06-08 7:45 ` [patch 1/1] [PATCH] " Martin Schwidefsky @ 2011-06-12 12:41 ` Rafael J. Wysocki 2011-07-28 22:01 ` Rafael J. Wysocki 1 sibling, 0 replies; 35+ messages in thread From: Rafael J. Wysocki @ 2011-06-12 12:41 UTC (permalink / raw) To: Martin Schwidefsky Cc: linux-pm, linux-kernel, linux-s390, Pavel Machek, Jiri Slaby, Len Brown On Wednesday, June 08, 2011, Martin Schwidefsky wrote: > From: Martin Schwidefsky <schwidefsky@de.ibm.com> > > For s390 there is one additional byte associated with each page, > the storage key. This byte contains the referenced and changed > bits and needs to be included into the hibernation image. > If the storage keys are not restored to their previous state all > original pages would appear to be dirty. This can cause > inconsistencies e.g. with read-only filesystems. > > Cc: Pavel Machek <pavel@ucw.cz> > Cc: Rafael J. Wysocki <rjw@sisk.pl> > Cc: Jiri Slaby <jslaby@suse.cz> > Cc: linux-pm@lists.linux-foundation.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-s390@vger.kernel.org > Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com> > --- > > arch/s390/kernel/swsusp_asm64.S | 3 > kernel/power/snapshot.c | 148 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 151 insertions(+) > > diff -urpN linux-2.6/arch/s390/kernel/swsusp_asm64.S linux-2.6-patched/arch/s390/kernel/swsusp_asm64.S > --- linux-2.6/arch/s390/kernel/swsusp_asm64.S 2011-05-19 06:06:34.000000000 +0200 > +++ linux-2.6-patched/arch/s390/kernel/swsusp_asm64.S 2011-06-06 11:14:43.000000000 +0200 > @@ -138,11 +138,14 @@ swsusp_arch_resume: > 0: > lg %r2,8(%r1) > lg %r4,0(%r1) > + iske %r0,%r4 > lghi %r3,PAGE_SIZE > lghi %r5,PAGE_SIZE > 1: > mvcle %r2,%r4,0 > jo 1b > + lg %r2,8(%r1) > + sske %r0,%r2 > lg %r1,16(%r1) > ltgr %r1,%r1 > jnz 0b I cannot comment on the assembly. > diff -urpN linux-2.6/kernel/power/snapshot.c linux-2.6-patched/kernel/power/snapshot.c > --- linux-2.6/kernel/power/snapshot.c 2011-06-06 11:14:39.000000000 +0200 > +++ linux-2.6-patched/kernel/power/snapshot.c 2011-06-06 11:14:43.000000000 +0200 > @@ -1022,6 +1022,137 @@ static inline void copy_data_page(unsign > } > #endif /* CONFIG_HIGHMEM */ > > +#ifdef CONFIG_S390 > +/* > + * For s390 there is one additional byte associated with each page, > + * the storage key. The key consists of the access-control bits > + * (alias the key), the fetch-protection bit, the referenced bit > + * and the change bit (alias dirty bit). Linux uses only the > + * referenced bit and the change bit for pages in the page cache. > + * Any modification of a page will set the change bit, any access > + * sets the referenced bit. Overindication of the referenced bits > + * after a hibernation cycle does not cause any harm but the > + * overindication of the change bits would cause trouble. > + * Therefore it is necessary to include the storage keys in the > + * hibernation image. The storage keys are saved to the most > + * significant byte of each page frame number in the list of pfns > + * in the hibernation image. > + */ Let me say that is not exactly straightforward. :-) One thing I don't really understand is where those storage keys are normally stored so that they aren't present in the image without this additional mechanism. Could you explain that a bit more, please? > + > +/* > + * Key storage is allocated as a linked list of pages. > + * The size of the keys array is (PAGE_SIZE - sizeof(long)) > + */ > +struct page_key_data { > + struct page_key_data *next; > + unsigned char data[]; > +}; This seems to be similar to the data structure for the saving of ACPI NVS memory, so perhaps it's possible to combine the two. > +#define PAGE_KEY_DATA_SIZE (PAGE_SIZE - sizeof(struct page_key_data *)) > + > +static struct page_key_data *page_key_data; > +static struct page_key_data *page_key_rp, *page_key_wp; > +static unsigned long page_key_rx, page_key_wx; > + > +/* > + * For each page in the hibernation image one additional byte is > + * stored in the most significant byte of the page frame number. I'm not sure it's really worth the complication. If you simply store those keys in separete pages, you'd need one additional page per PAGE_SIZE page frames, so given PAGE_SIZE = 4096 that's one page per 16 MB of memory, which is not a whole lot (64 extra pages per GB if I'm not mistaken). It seems that doing it will simplify things quite a git. Apart from this, I'm not sure that kernel/power/snapshot.c is the right place for all this S390-specific code. I'd rather see it in arch/s390. Thanks, Rafael ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. @ 2011-06-12 12:41 ` Rafael J. Wysocki 0 siblings, 0 replies; 35+ messages in thread From: Rafael J. Wysocki @ 2011-06-12 12:41 UTC (permalink / raw) To: Martin Schwidefsky; +Cc: linux-s390, linux-kernel, linux-pm, Jiri Slaby On Wednesday, June 08, 2011, Martin Schwidefsky wrote: > From: Martin Schwidefsky <schwidefsky@de.ibm.com> > > For s390 there is one additional byte associated with each page, > the storage key. This byte contains the referenced and changed > bits and needs to be included into the hibernation image. > If the storage keys are not restored to their previous state all > original pages would appear to be dirty. This can cause > inconsistencies e.g. with read-only filesystems. > > Cc: Pavel Machek <pavel@ucw.cz> > Cc: Rafael J. Wysocki <rjw@sisk.pl> > Cc: Jiri Slaby <jslaby@suse.cz> > Cc: linux-pm@lists.linux-foundation.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-s390@vger.kernel.org > Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com> > --- > > arch/s390/kernel/swsusp_asm64.S | 3 > kernel/power/snapshot.c | 148 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 151 insertions(+) > > diff -urpN linux-2.6/arch/s390/kernel/swsusp_asm64.S linux-2.6-patched/arch/s390/kernel/swsusp_asm64.S > --- linux-2.6/arch/s390/kernel/swsusp_asm64.S 2011-05-19 06:06:34.000000000 +0200 > +++ linux-2.6-patched/arch/s390/kernel/swsusp_asm64.S 2011-06-06 11:14:43.000000000 +0200 > @@ -138,11 +138,14 @@ swsusp_arch_resume: > 0: > lg %r2,8(%r1) > lg %r4,0(%r1) > + iske %r0,%r4 > lghi %r3,PAGE_SIZE > lghi %r5,PAGE_SIZE > 1: > mvcle %r2,%r4,0 > jo 1b > + lg %r2,8(%r1) > + sske %r0,%r2 > lg %r1,16(%r1) > ltgr %r1,%r1 > jnz 0b I cannot comment on the assembly. > diff -urpN linux-2.6/kernel/power/snapshot.c linux-2.6-patched/kernel/power/snapshot.c > --- linux-2.6/kernel/power/snapshot.c 2011-06-06 11:14:39.000000000 +0200 > +++ linux-2.6-patched/kernel/power/snapshot.c 2011-06-06 11:14:43.000000000 +0200 > @@ -1022,6 +1022,137 @@ static inline void copy_data_page(unsign > } > #endif /* CONFIG_HIGHMEM */ > > +#ifdef CONFIG_S390 > +/* > + * For s390 there is one additional byte associated with each page, > + * the storage key. The key consists of the access-control bits > + * (alias the key), the fetch-protection bit, the referenced bit > + * and the change bit (alias dirty bit). Linux uses only the > + * referenced bit and the change bit for pages in the page cache. > + * Any modification of a page will set the change bit, any access > + * sets the referenced bit. Overindication of the referenced bits > + * after a hibernation cycle does not cause any harm but the > + * overindication of the change bits would cause trouble. > + * Therefore it is necessary to include the storage keys in the > + * hibernation image. The storage keys are saved to the most > + * significant byte of each page frame number in the list of pfns > + * in the hibernation image. > + */ Let me say that is not exactly straightforward. :-) One thing I don't really understand is where those storage keys are normally stored so that they aren't present in the image without this additional mechanism. Could you explain that a bit more, please? > + > +/* > + * Key storage is allocated as a linked list of pages. > + * The size of the keys array is (PAGE_SIZE - sizeof(long)) > + */ > +struct page_key_data { > + struct page_key_data *next; > + unsigned char data[]; > +}; This seems to be similar to the data structure for the saving of ACPI NVS memory, so perhaps it's possible to combine the two. > +#define PAGE_KEY_DATA_SIZE (PAGE_SIZE - sizeof(struct page_key_data *)) > + > +static struct page_key_data *page_key_data; > +static struct page_key_data *page_key_rp, *page_key_wp; > +static unsigned long page_key_rx, page_key_wx; > + > +/* > + * For each page in the hibernation image one additional byte is > + * stored in the most significant byte of the page frame number. I'm not sure it's really worth the complication. If you simply store those keys in separete pages, you'd need one additional page per PAGE_SIZE page frames, so given PAGE_SIZE = 4096 that's one page per 16 MB of memory, which is not a whole lot (64 extra pages per GB if I'm not mistaken). It seems that doing it will simplify things quite a git. Apart from this, I'm not sure that kernel/power/snapshot.c is the right place for all this S390-specific code. I'd rather see it in arch/s390. Thanks, Rafael ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-06-12 12:41 ` Rafael J. Wysocki (?) @ 2011-06-14 8:50 ` Martin Schwidefsky 2011-06-14 20:50 ` Rafael J. Wysocki 2011-06-14 20:50 ` Rafael J. Wysocki -1 siblings, 2 replies; 35+ messages in thread From: Martin Schwidefsky @ 2011-06-14 8:50 UTC (permalink / raw) To: Rafael J. Wysocki Cc: linux-pm, linux-kernel, linux-s390, Pavel Machek, Jiri Slaby, Len Brown Hi Rafael, first of all: thanks for the review. On Sun, 12 Jun 2011 14:41:34 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > > diff -urpN linux-2.6/arch/s390/kernel/swsusp_asm64.S linux-2.6-patched/arch/s390/kernel/swsusp_asm64.S > > --- linux-2.6/arch/s390/kernel/swsusp_asm64.S 2011-05-19 06:06:34.000000000 +0200 > > +++ linux-2.6-patched/arch/s390/kernel/swsusp_asm64.S 2011-06-06 11:14:43.000000000 +0200 > > @@ -138,11 +138,14 @@ swsusp_arch_resume: > > 0: > > lg %r2,8(%r1) > > lg %r4,0(%r1) > > + iske %r0,%r4 > > lghi %r3,PAGE_SIZE > > lghi %r5,PAGE_SIZE > > 1: > > mvcle %r2,%r4,0 > > jo 1b > > + lg %r2,8(%r1) > > + sske %r0,%r2 > > lg %r1,16(%r1) > > ltgr %r1,%r1 > > jnz 0b > > I cannot comment on the assembly. The swsusp_arch_resume code copies the safe pages to their final target page. Writing to a page sets the dirty bit in the storage key. Which means that the storage key needs to be restored >after< the copy has been done. To avoid having to deal with additional pages where the storage key are kept while the copy is still pending I simply set the storage key of the safe page and transfer it from the safe page to the final target page after the copy has been done. That is what the iske and the sske instruction are doing. > > diff -urpN linux-2.6/kernel/power/snapshot.c linux-2.6-patched/kernel/power/snapshot.c > > --- linux-2.6/kernel/power/snapshot.c 2011-06-06 11:14:39.000000000 +0200 > > +++ linux-2.6-patched/kernel/power/snapshot.c 2011-06-06 11:14:43.000000000 +0200 > > @@ -1022,6 +1022,137 @@ static inline void copy_data_page(unsign > > } > > #endif /* CONFIG_HIGHMEM */ > > > > +#ifdef CONFIG_S390 > > +/* > > + * For s390 there is one additional byte associated with each page, > > + * the storage key. The key consists of the access-control bits > > + * (alias the key), the fetch-protection bit, the referenced bit > > + * and the change bit (alias dirty bit). Linux uses only the > > + * referenced bit and the change bit for pages in the page cache. > > + * Any modification of a page will set the change bit, any access > > + * sets the referenced bit. Overindication of the referenced bits > > + * after a hibernation cycle does not cause any harm but the > > + * overindication of the change bits would cause trouble. > > + * Therefore it is necessary to include the storage keys in the > > + * hibernation image. The storage keys are saved to the most > > + * significant byte of each page frame number in the list of pfns > > + * in the hibernation image. > > + */ > > Let me say that is not exactly straightforward. :-) > > One thing I don't really understand is where those storage keys are normally > stored so that they aren't present in the image without this additional > mechanism. Could you explain that a bit more, please? The storage keys are outside of the directly addressable memory, the only way to get to them is with special instructions: iske - insert storage key extended, ivsk - insert virtual storage key, sske - set storage key extended, and rrbe - reset reference bit extended. The storage key of a page has 7 bits, 4 bit for access-control, the fetch-protection-bit, the reference-bit and the change-bit. In Linux only the reference and change bits are used. These bits are per physical page, one of the major differences of the s390 machines compared to other architectures. We had a number of interesting problems because of pte dirty & referenced vs page dirty & referenced. For e.g. x86 a page is implicitly clean if it has no mapper. On s390 is depends on the contents of the storage key. Which is why SetPageUptodate clears the storage key. In regard to hibernation the important aspect is that each and every write to a page sets the dirty bit even if it is done by I/O. The restore of the hibernation image therefore makes all pages dirty. To get the correct original combination of page content and storage key content the storage key needs to be set after the last access to the page content. > > + > > +/* > > + * Key storage is allocated as a linked list of pages. > > + * The size of the keys array is (PAGE_SIZE - sizeof(long)) > > + */ > > +struct page_key_data { > > + struct page_key_data *next; > > + unsigned char data[]; > > +}; > > This seems to be similar to the data structure for the saving of ACPI NVS > memory, so perhaps it's possible to combine the two. Almost the same, I thought about ways to merge them. I dropped the idea in order to keep the patch as small as possible. > > +#define PAGE_KEY_DATA_SIZE (PAGE_SIZE - sizeof(struct page_key_data *)) > > + > > +static struct page_key_data *page_key_data; > > +static struct page_key_data *page_key_rp, *page_key_wp; > > +static unsigned long page_key_rx, page_key_wx; > > + > > +/* > > + * For each page in the hibernation image one additional byte is > > + * stored in the most significant byte of the page frame number. > > I'm not sure it's really worth the complication. If you simply store those > keys in separete pages, you'd need one additional page per PAGE_SIZE > page frames, so given PAGE_SIZE = 4096 that's one page per 16 MB of memory, > which is not a whole lot (64 extra pages per GB if I'm not mistaken). > > It seems that doing it will simplify things quite a git. One of the versions of the patch actually did that and it turned out to be much larger then the proposed solution. That older version had another step in snapshot_read_next / snapshot_write_next to copy the storage keys to pages allocated for that purpose. I had some trouble with the way how the code deals with the orig_bm and the copy_bm though. Not trivial code.. In the end I decided to use some of the unused 12 bits in the pfn list entries, it avoids any structural changes to the snapshot code and it resulted in the smallest patch (at least so far). One thing that seemed important to me was to make the patch obviously correct for the other architectures, for !s390 the new functions are nops and there is no structural change. > Apart from this, I'm not sure that kernel/power/snapshot.c is the right > place for all this S390-specific code. I'd rather see it in arch/s390. The alternative would be to move the s390 specific page_key_xxx functions to some architecture file. The hooks in snapshot.c would have to stay though. Unfortunately they are in odd places and you can only understand them in the context of the s390 storage keys. -- blue skies, Martin. "Reality continues to ruin my life." - Calvin. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-06-14 8:50 ` Martin Schwidefsky @ 2011-06-14 20:50 ` Rafael J. Wysocki 2011-06-15 7:36 ` Martin Schwidefsky 2011-06-15 7:36 ` Martin Schwidefsky 2011-06-14 20:50 ` Rafael J. Wysocki 1 sibling, 2 replies; 35+ messages in thread From: Rafael J. Wysocki @ 2011-06-14 20:50 UTC (permalink / raw) To: Martin Schwidefsky Cc: linux-pm, linux-kernel, linux-s390, Pavel Machek, Jiri Slaby, Len Brown On Tuesday, June 14, 2011, Martin Schwidefsky wrote: > Hi Rafael, Hi, > first of all: thanks for the review. > > On Sun, 12 Jun 2011 14:41:34 +0200 > "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > > > > diff -urpN linux-2.6/arch/s390/kernel/swsusp_asm64.S linux-2.6-patched/arch/s390/kernel/swsusp_asm64.S > > > --- linux-2.6/arch/s390/kernel/swsusp_asm64.S 2011-05-19 06:06:34.000000000 +0200 > > > +++ linux-2.6-patched/arch/s390/kernel/swsusp_asm64.S 2011-06-06 11:14:43.000000000 +0200 > > > @@ -138,11 +138,14 @@ swsusp_arch_resume: > > > 0: > > > lg %r2,8(%r1) > > > lg %r4,0(%r1) > > > + iske %r0,%r4 > > > lghi %r3,PAGE_SIZE > > > lghi %r5,PAGE_SIZE > > > 1: > > > mvcle %r2,%r4,0 > > > jo 1b > > > + lg %r2,8(%r1) > > > + sske %r0,%r2 > > > lg %r1,16(%r1) > > > ltgr %r1,%r1 > > > jnz 0b > > > > I cannot comment on the assembly. > > The swsusp_arch_resume code copies the safe pages to their final target > page. Writing to a page sets the dirty bit in the storage key. Which > means that the storage key needs to be restored >after< the copy has > been done. To avoid having to deal with additional pages where the > storage key are kept while the copy is still pending I simply set the > storage key of the safe page and transfer it from the safe page to the > final target page after the copy has been done. That is what the iske > and the sske instruction are doing. I see. > > > diff -urpN linux-2.6/kernel/power/snapshot.c linux-2.6-patched/kernel/power/snapshot.c > > > --- linux-2.6/kernel/power/snapshot.c 2011-06-06 11:14:39.000000000 +0200 > > > +++ linux-2.6-patched/kernel/power/snapshot.c 2011-06-06 11:14:43.000000000 +0200 > > > @@ -1022,6 +1022,137 @@ static inline void copy_data_page(unsign > > > } > > > #endif /* CONFIG_HIGHMEM */ > > > > > > +#ifdef CONFIG_S390 > > > +/* > > > + * For s390 there is one additional byte associated with each page, > > > + * the storage key. The key consists of the access-control bits > > > + * (alias the key), the fetch-protection bit, the referenced bit > > > + * and the change bit (alias dirty bit). Linux uses only the > > > + * referenced bit and the change bit for pages in the page cache. > > > + * Any modification of a page will set the change bit, any access > > > + * sets the referenced bit. Overindication of the referenced bits > > > + * after a hibernation cycle does not cause any harm but the > > > + * overindication of the change bits would cause trouble. > > > + * Therefore it is necessary to include the storage keys in the > > > + * hibernation image. The storage keys are saved to the most > > > + * significant byte of each page frame number in the list of pfns > > > + * in the hibernation image. > > > + */ > > > > Let me say that is not exactly straightforward. :-) > > > > One thing I don't really understand is where those storage keys are normally > > stored so that they aren't present in the image without this additional > > mechanism. Could you explain that a bit more, please? > > The storage keys are outside of the directly addressable memory, the only > way to get to them is with special instructions: > iske - insert storage key extended, ivsk - insert virtual storage key, > sske - set storage key extended, and rrbe - reset reference bit extended. > The storage key of a page has 7 bits, 4 bit for access-control, the > fetch-protection-bit, the reference-bit and the change-bit. In Linux only > the reference and change bits are used. OK, so it looks like we would only need to store 2 bits per page in additional allocations. > These bits are per physical page, one of the major differences of the s390 > machines compared to other architectures. We had a number of interesting > problems because of pte dirty & referenced vs page dirty & referenced. > For e.g. x86 a page is implicitly clean if it has no mapper. On s390 is > depends on the contents of the storage key. Which is why SetPageUptodate > clears the storage key. > > In regard to hibernation the important aspect is that each and every write > to a page sets the dirty bit even if it is done by I/O. The restore of the > hibernation image therefore makes all pages dirty. To get the correct > original combination of page content and storage key content the storage > key needs to be set after the last access to the page content. What, in your opinion, is the right time for restoring the original storage keys after the image has been loaded? It seems that should happen after passing control to the hibernated kernel. > > > + > > > +/* > > > + * Key storage is allocated as a linked list of pages. > > > + * The size of the keys array is (PAGE_SIZE - sizeof(long)) > > > + */ > > > +struct page_key_data { > > > + struct page_key_data *next; > > > + unsigned char data[]; > > > +}; > > > > This seems to be similar to the data structure for the saving of ACPI NVS > > memory, so perhaps it's possible to combine the two. > > Almost the same, I thought about ways to merge them. I dropped the idea in > order to keep the patch as small as possible. I think, however, that we really should try to merge them. The only difference seems to be how the additionally allocated pages will be populated and what's going to happen to their contents during restore. ACPI will simply copy the NVS memory to those pages, while S390 will save the relevant storage key bits in there. > > > +#define PAGE_KEY_DATA_SIZE (PAGE_SIZE - sizeof(struct page_key_data *)) > > > + > > > +static struct page_key_data *page_key_data; > > > +static struct page_key_data *page_key_rp, *page_key_wp; > > > +static unsigned long page_key_rx, page_key_wx; > > > + > > > +/* > > > + * For each page in the hibernation image one additional byte is > > > + * stored in the most significant byte of the page frame number. > > > > I'm not sure it's really worth the complication. If you simply store those > > keys in separete pages, you'd need one additional page per PAGE_SIZE > > page frames, so given PAGE_SIZE = 4096 that's one page per 16 MB of memory, > > which is not a whole lot (64 extra pages per GB if I'm not mistaken). > > > > It seems that doing it will simplify things quite a git. > > One of the versions of the patch actually did that and it turned out to > be much larger then the proposed solution. That older version had another > step in snapshot_read_next / snapshot_write_next to copy the storage keys > to pages allocated for that purpose. I had some trouble with the way how > the code deals with the orig_bm and the copy_bm though. Not trivial code.. Well, I'm sure we can handle that. :-) I thought of the following approach: * Allocate memory for saving the relevant bits of the storage keys before the memory preallocation kicks in. * Save the storage key bits to that memory before creating the image (be careful about the storage keys of the pages they are being stored into). * After getting control from the boot kernel, restore the storage key bits saved before creating the image. * Free additional memory used for storing them. This way we wouldn't have to worry about any interactions with snapshot_read_next / snapshot_write_next etc. Thanks, Rafael ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-06-14 20:50 ` Rafael J. Wysocki @ 2011-06-15 7:36 ` Martin Schwidefsky 2011-06-15 7:36 ` Martin Schwidefsky 1 sibling, 0 replies; 35+ messages in thread From: Martin Schwidefsky @ 2011-06-15 7:36 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: linux-s390, linux-kernel, linux-pm, Jiri Slaby On Tue, 14 Jun 2011 22:50:14 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > On Tuesday, June 14, 2011, Martin Schwidefsky wrote: > > Hi Rafael, > > > On Sun, 12 Jun 2011 14:41:34 +0200 > > "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > > > > > > diff -urpN linux-2.6/kernel/power/snapshot.c linux-2.6-patched/kernel/power/snapshot.c > > > > --- linux-2.6/kernel/power/snapshot.c 2011-06-06 11:14:39.000000000 +0200 > > > > +++ linux-2.6-patched/kernel/power/snapshot.c 2011-06-06 11:14:43.000000000 +0200 > > > > @@ -1022,6 +1022,137 @@ static inline void copy_data_page(unsign > > > > } > > > > #endif /* CONFIG_HIGHMEM */ > > > > > > > > +#ifdef CONFIG_S390 > > > > +/* > > > > + * For s390 there is one additional byte associated with each page, > > > > + * the storage key. The key consists of the access-control bits > > > > + * (alias the key), the fetch-protection bit, the referenced bit > > > > + * and the change bit (alias dirty bit). Linux uses only the > > > > + * referenced bit and the change bit for pages in the page cache. > > > > + * Any modification of a page will set the change bit, any access > > > > + * sets the referenced bit. Overindication of the referenced bits > > > > + * after a hibernation cycle does not cause any harm but the > > > > + * overindication of the change bits would cause trouble. > > > > + * Therefore it is necessary to include the storage keys in the > > > > + * hibernation image. The storage keys are saved to the most > > > > + * significant byte of each page frame number in the list of pfns > > > > + * in the hibernation image. > > > > + */ > > > > > > Let me say that is not exactly straightforward. :-) > > > > > > One thing I don't really understand is where those storage keys are normally > > > stored so that they aren't present in the image without this additional > > > mechanism. Could you explain that a bit more, please? > > > > The storage keys are outside of the directly addressable memory, the only > > way to get to them is with special instructions: > > iske - insert storage key extended, ivsk - insert virtual storage key, > > sske - set storage key extended, and rrbe - reset reference bit extended. > > The storage key of a page has 7 bits, 4 bit for access-control, the > > fetch-protection-bit, the reference-bit and the change-bit. In Linux only > > the reference and change bits are used. > > OK, so it looks like we would only need to store 2 bits per page in additional > allocations. For now we'd only need two bits, but with KVM guests we will have to store the complete storage key. The guest OS could make use of all the bits in the storage key, the KVM host needs to preserve them. > > These bits are per physical page, one of the major differences of the s390 > > machines compared to other architectures. We had a number of interesting > > problems because of pte dirty & referenced vs page dirty & referenced. > > For e.g. x86 a page is implicitly clean if it has no mapper. On s390 is > > depends on the contents of the storage key. Which is why SetPageUptodate > > clears the storage key. > > > > In regard to hibernation the important aspect is that each and every write > > to a page sets the dirty bit even if it is done by I/O. The restore of the > > hibernation image therefore makes all pages dirty. To get the correct > > original combination of page content and storage key content the storage > > key needs to be set after the last access to the page content. > > What, in your opinion, is the right time for restoring the original storage > keys after the image has been loaded? It seems that should happen after > passing control to the hibernated kernel. Anytime after the final write of the page content completed and before the hibernated kernel gets control (or shortly after in the resume code of the restored kernel). > > > > + > > > > +/* > > > > + * Key storage is allocated as a linked list of pages. > > > > + * The size of the keys array is (PAGE_SIZE - sizeof(long)) > > > > + */ > > > > +struct page_key_data { > > > > + struct page_key_data *next; > > > > + unsigned char data[]; > > > > +}; > > > > > > This seems to be similar to the data structure for the saving of ACPI NVS > > > memory, so perhaps it's possible to combine the two. > > > > Almost the same, I thought about ways to merge them. I dropped the idea in > > order to keep the patch as small as possible. > > I think, however, that we really should try to merge them. The only > difference seems to be how the additionally allocated pages will be populated > and what's going to happen to their contents during restore. > > ACPI will simply copy the NVS memory to those pages, while S390 will save > the relevant storage key bits in there. One complication to keep in mind is that we need to know which storage key goes to which page frame. We need something like the orig_bm/copy_bm or we'd have to store the pfn with the key. Simply storing the key for every page will make the array unnecessarily big. > > > > +#define PAGE_KEY_DATA_SIZE (PAGE_SIZE - sizeof(struct page_key_data *)) > > > > + > > > > +static struct page_key_data *page_key_data; > > > > +static struct page_key_data *page_key_rp, *page_key_wp; > > > > +static unsigned long page_key_rx, page_key_wx; > > > > + > > > > +/* > > > > + * For each page in the hibernation image one additional byte is > > > > + * stored in the most significant byte of the page frame number. > > > > > > I'm not sure it's really worth the complication. If you simply store those > > > keys in separete pages, you'd need one additional page per PAGE_SIZE > > > page frames, so given PAGE_SIZE = 4096 that's one page per 16 MB of memory, > > > which is not a whole lot (64 extra pages per GB if I'm not mistaken). > > > > > > It seems that doing it will simplify things quite a git. > > > > One of the versions of the patch actually did that and it turned out to > > be much larger then the proposed solution. That older version had another > > step in snapshot_read_next / snapshot_write_next to copy the storage keys > > to pages allocated for that purpose. I had some trouble with the way how > > the code deals with the orig_bm and the copy_bm though. Not trivial code.. > > Well, I'm sure we can handle that. :-) > > I thought of the following approach: > > * Allocate memory for saving the relevant bits of the storage keys before > the memory preallocation kicks in. > > * Save the storage key bits to that memory before creating the image (be > careful about the storage keys of the pages they are being stored into). > > * After getting control from the boot kernel, restore the storage key bits > saved before creating the image. > > * Free additional memory used for storing them. > > This way we wouldn't have to worry about any interactions with > snapshot_read_next / snapshot_write_next etc. Well before the preallocation kicked in we don't know which are the relevant storage keys. The only other option would be to store all of them which I consider to be a inferior solution. -- blue skies, Martin. "Reality continues to ruin my life." - Calvin. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-06-14 20:50 ` Rafael J. Wysocki 2011-06-15 7:36 ` Martin Schwidefsky @ 2011-06-15 7:36 ` Martin Schwidefsky 2011-06-15 23:21 ` Rafael J. Wysocki ` (5 more replies) 1 sibling, 6 replies; 35+ messages in thread From: Martin Schwidefsky @ 2011-06-15 7:36 UTC (permalink / raw) To: Rafael J. Wysocki Cc: linux-pm, linux-kernel, linux-s390, Pavel Machek, Jiri Slaby, Len Brown On Tue, 14 Jun 2011 22:50:14 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > On Tuesday, June 14, 2011, Martin Schwidefsky wrote: > > Hi Rafael, > > > On Sun, 12 Jun 2011 14:41:34 +0200 > > "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > > > > > > diff -urpN linux-2.6/kernel/power/snapshot.c linux-2.6-patched/kernel/power/snapshot.c > > > > --- linux-2.6/kernel/power/snapshot.c 2011-06-06 11:14:39.000000000 +0200 > > > > +++ linux-2.6-patched/kernel/power/snapshot.c 2011-06-06 11:14:43.000000000 +0200 > > > > @@ -1022,6 +1022,137 @@ static inline void copy_data_page(unsign > > > > } > > > > #endif /* CONFIG_HIGHMEM */ > > > > > > > > +#ifdef CONFIG_S390 > > > > +/* > > > > + * For s390 there is one additional byte associated with each page, > > > > + * the storage key. The key consists of the access-control bits > > > > + * (alias the key), the fetch-protection bit, the referenced bit > > > > + * and the change bit (alias dirty bit). Linux uses only the > > > > + * referenced bit and the change bit for pages in the page cache. > > > > + * Any modification of a page will set the change bit, any access > > > > + * sets the referenced bit. Overindication of the referenced bits > > > > + * after a hibernation cycle does not cause any harm but the > > > > + * overindication of the change bits would cause trouble. > > > > + * Therefore it is necessary to include the storage keys in the > > > > + * hibernation image. The storage keys are saved to the most > > > > + * significant byte of each page frame number in the list of pfns > > > > + * in the hibernation image. > > > > + */ > > > > > > Let me say that is not exactly straightforward. :-) > > > > > > One thing I don't really understand is where those storage keys are normally > > > stored so that they aren't present in the image without this additional > > > mechanism. Could you explain that a bit more, please? > > > > The storage keys are outside of the directly addressable memory, the only > > way to get to them is with special instructions: > > iske - insert storage key extended, ivsk - insert virtual storage key, > > sske - set storage key extended, and rrbe - reset reference bit extended. > > The storage key of a page has 7 bits, 4 bit for access-control, the > > fetch-protection-bit, the reference-bit and the change-bit. In Linux only > > the reference and change bits are used. > > OK, so it looks like we would only need to store 2 bits per page in additional > allocations. For now we'd only need two bits, but with KVM guests we will have to store the complete storage key. The guest OS could make use of all the bits in the storage key, the KVM host needs to preserve them. > > These bits are per physical page, one of the major differences of the s390 > > machines compared to other architectures. We had a number of interesting > > problems because of pte dirty & referenced vs page dirty & referenced. > > For e.g. x86 a page is implicitly clean if it has no mapper. On s390 is > > depends on the contents of the storage key. Which is why SetPageUptodate > > clears the storage key. > > > > In regard to hibernation the important aspect is that each and every write > > to a page sets the dirty bit even if it is done by I/O. The restore of the > > hibernation image therefore makes all pages dirty. To get the correct > > original combination of page content and storage key content the storage > > key needs to be set after the last access to the page content. > > What, in your opinion, is the right time for restoring the original storage > keys after the image has been loaded? It seems that should happen after > passing control to the hibernated kernel. Anytime after the final write of the page content completed and before the hibernated kernel gets control (or shortly after in the resume code of the restored kernel). > > > > + > > > > +/* > > > > + * Key storage is allocated as a linked list of pages. > > > > + * The size of the keys array is (PAGE_SIZE - sizeof(long)) > > > > + */ > > > > +struct page_key_data { > > > > + struct page_key_data *next; > > > > + unsigned char data[]; > > > > +}; > > > > > > This seems to be similar to the data structure for the saving of ACPI NVS > > > memory, so perhaps it's possible to combine the two. > > > > Almost the same, I thought about ways to merge them. I dropped the idea in > > order to keep the patch as small as possible. > > I think, however, that we really should try to merge them. The only > difference seems to be how the additionally allocated pages will be populated > and what's going to happen to their contents during restore. > > ACPI will simply copy the NVS memory to those pages, while S390 will save > the relevant storage key bits in there. One complication to keep in mind is that we need to know which storage key goes to which page frame. We need something like the orig_bm/copy_bm or we'd have to store the pfn with the key. Simply storing the key for every page will make the array unnecessarily big. > > > > +#define PAGE_KEY_DATA_SIZE (PAGE_SIZE - sizeof(struct page_key_data *)) > > > > + > > > > +static struct page_key_data *page_key_data; > > > > +static struct page_key_data *page_key_rp, *page_key_wp; > > > > +static unsigned long page_key_rx, page_key_wx; > > > > + > > > > +/* > > > > + * For each page in the hibernation image one additional byte is > > > > + * stored in the most significant byte of the page frame number. > > > > > > I'm not sure it's really worth the complication. If you simply store those > > > keys in separete pages, you'd need one additional page per PAGE_SIZE > > > page frames, so given PAGE_SIZE = 4096 that's one page per 16 MB of memory, > > > which is not a whole lot (64 extra pages per GB if I'm not mistaken). > > > > > > It seems that doing it will simplify things quite a git. > > > > One of the versions of the patch actually did that and it turned out to > > be much larger then the proposed solution. That older version had another > > step in snapshot_read_next / snapshot_write_next to copy the storage keys > > to pages allocated for that purpose. I had some trouble with the way how > > the code deals with the orig_bm and the copy_bm though. Not trivial code.. > > Well, I'm sure we can handle that. :-) > > I thought of the following approach: > > * Allocate memory for saving the relevant bits of the storage keys before > the memory preallocation kicks in. > > * Save the storage key bits to that memory before creating the image (be > careful about the storage keys of the pages they are being stored into). > > * After getting control from the boot kernel, restore the storage key bits > saved before creating the image. > > * Free additional memory used for storing them. > > This way we wouldn't have to worry about any interactions with > snapshot_read_next / snapshot_write_next etc. Well before the preallocation kicked in we don't know which are the relevant storage keys. The only other option would be to store all of them which I consider to be a inferior solution. -- blue skies, Martin. "Reality continues to ruin my life." - Calvin. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-06-15 7:36 ` Martin Schwidefsky @ 2011-06-15 23:21 ` Rafael J. Wysocki 2011-06-15 23:21 ` Rafael J. Wysocki ` (4 subsequent siblings) 5 siblings, 0 replies; 35+ messages in thread From: Rafael J. Wysocki @ 2011-06-15 23:21 UTC (permalink / raw) To: Martin Schwidefsky Cc: linux-pm, linux-kernel, linux-s390, Pavel Machek, Jiri Slaby, Len Brown On Wednesday, June 15, 2011, Martin Schwidefsky wrote: > On Tue, 14 Jun 2011 22:50:14 +0200 ... > > Well, I'm sure we can handle that. :-) > > > > I thought of the following approach: > > > > * Allocate memory for saving the relevant bits of the storage keys before > > the memory preallocation kicks in. > > > > * Save the storage key bits to that memory before creating the image (be > > careful about the storage keys of the pages they are being stored into). > > > > * After getting control from the boot kernel, restore the storage key bits > > saved before creating the image. > > > > * Free additional memory used for storing them. > > > > This way we wouldn't have to worry about any interactions with > > snapshot_read_next / snapshot_write_next etc. > > Well before the preallocation kicked in we don't know which are the relevant > storage keys. The only other option would be to store all of them which I > consider to be a inferior solution. OK, I see your point. You think it's necessary to save the entire storage key for each page and you'd like to save storage keys only for those pages, whose contents is going to be saved in the image. Now, I need to look at your original patch again. :-) ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-06-15 7:36 ` Martin Schwidefsky 2011-06-15 23:21 ` Rafael J. Wysocki @ 2011-06-15 23:21 ` Rafael J. Wysocki 2011-07-03 17:46 ` Pavel Machek ` (3 subsequent siblings) 5 siblings, 0 replies; 35+ messages in thread From: Rafael J. Wysocki @ 2011-06-15 23:21 UTC (permalink / raw) To: Martin Schwidefsky; +Cc: linux-s390, linux-kernel, linux-pm, Jiri Slaby On Wednesday, June 15, 2011, Martin Schwidefsky wrote: > On Tue, 14 Jun 2011 22:50:14 +0200 ... > > Well, I'm sure we can handle that. :-) > > > > I thought of the following approach: > > > > * Allocate memory for saving the relevant bits of the storage keys before > > the memory preallocation kicks in. > > > > * Save the storage key bits to that memory before creating the image (be > > careful about the storage keys of the pages they are being stored into). > > > > * After getting control from the boot kernel, restore the storage key bits > > saved before creating the image. > > > > * Free additional memory used for storing them. > > > > This way we wouldn't have to worry about any interactions with > > snapshot_read_next / snapshot_write_next etc. > > Well before the preallocation kicked in we don't know which are the relevant > storage keys. The only other option would be to store all of them which I > consider to be a inferior solution. OK, I see your point. You think it's necessary to save the entire storage key for each page and you'd like to save storage keys only for those pages, whose contents is going to be saved in the image. Now, I need to look at your original patch again. :-) ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-06-15 7:36 ` Martin Schwidefsky 2011-06-15 23:21 ` Rafael J. Wysocki 2011-06-15 23:21 ` Rafael J. Wysocki @ 2011-07-03 17:46 ` Pavel Machek 2011-07-03 17:46 ` Pavel Machek ` (2 subsequent siblings) 5 siblings, 0 replies; 35+ messages in thread From: Pavel Machek @ 2011-07-03 17:46 UTC (permalink / raw) To: Martin Schwidefsky; +Cc: linux-s390, linux-kernel, linux-pm, Jiri Slaby Hi! > > I think, however, that we really should try to merge them. The only > > difference seems to be how the additionally allocated pages will be populated > > and what's going to happen to their contents during restore. > > > > ACPI will simply copy the NVS memory to those pages, while S390 will save > > the relevant storage key bits in there. > > One complication to keep in mind is that we need to know which storage key > goes to which page frame. We need something like the orig_bm/copy_bm or > we'd have to store the pfn with the key. Simply storing the key for every > page will make the array unnecessarily big. How big is the overhead? In percent / in megabytes? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-06-15 7:36 ` Martin Schwidefsky ` (2 preceding siblings ...) 2011-07-03 17:46 ` Pavel Machek @ 2011-07-03 17:46 ` Pavel Machek 2011-07-04 8:09 ` Martin Schwidefsky 2011-07-04 8:09 ` Martin Schwidefsky 2011-07-07 21:36 ` Rafael J. Wysocki 2011-07-07 21:36 ` Rafael J. Wysocki 5 siblings, 2 replies; 35+ messages in thread From: Pavel Machek @ 2011-07-03 17:46 UTC (permalink / raw) To: Martin Schwidefsky Cc: Rafael J. Wysocki, linux-pm, linux-kernel, linux-s390, Jiri Slaby, Len Brown Hi! > > I think, however, that we really should try to merge them. The only > > difference seems to be how the additionally allocated pages will be populated > > and what's going to happen to their contents during restore. > > > > ACPI will simply copy the NVS memory to those pages, while S390 will save > > the relevant storage key bits in there. > > One complication to keep in mind is that we need to know which storage key > goes to which page frame. We need something like the orig_bm/copy_bm or > we'd have to store the pfn with the key. Simply storing the key for every > page will make the array unnecessarily big. How big is the overhead? In percent / in megabytes? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-07-03 17:46 ` Pavel Machek @ 2011-07-04 8:09 ` Martin Schwidefsky 2011-07-04 8:09 ` Martin Schwidefsky 1 sibling, 0 replies; 35+ messages in thread From: Martin Schwidefsky @ 2011-07-04 8:09 UTC (permalink / raw) To: Pavel Machek; +Cc: linux-s390, linux-kernel, linux-pm, Jiri Slaby On Sun, 3 Jul 2011 19:46:16 +0200 Pavel Machek <pavel@ucw.cz> wrote: > Hi! > > > > I think, however, that we really should try to merge them. The only > > > difference seems to be how the additionally allocated pages will be populated > > > and what's going to happen to their contents during restore. > > > > > > ACPI will simply copy the NVS memory to those pages, while S390 will save > > > the relevant storage key bits in there. > > > > One complication to keep in mind is that we need to know which storage key > > goes to which page frame. We need something like the orig_bm/copy_bm or > > we'd have to store the pfn with the key. Simply storing the key for every > > page will make the array unnecessarily big. > > How big is the overhead? In percent / in megabytes? Well, that depends on the ratio of the size of the hibernation image and the total size of the ram. Consider a 1TB machine with a hibernation image size of lets say 128 GB. The hibernation image would require 32 MB worth of storage keys (0.024%), for 1TB the array size would be 256 MB (0.19%). -- blue skies, Martin. "Reality continues to ruin my life." - Calvin. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-07-03 17:46 ` Pavel Machek 2011-07-04 8:09 ` Martin Schwidefsky @ 2011-07-04 8:09 ` Martin Schwidefsky 1 sibling, 0 replies; 35+ messages in thread From: Martin Schwidefsky @ 2011-07-04 8:09 UTC (permalink / raw) To: Pavel Machek Cc: Rafael J. Wysocki, linux-pm, linux-kernel, linux-s390, Jiri Slaby, Len Brown On Sun, 3 Jul 2011 19:46:16 +0200 Pavel Machek <pavel@ucw.cz> wrote: > Hi! > > > > I think, however, that we really should try to merge them. The only > > > difference seems to be how the additionally allocated pages will be populated > > > and what's going to happen to their contents during restore. > > > > > > ACPI will simply copy the NVS memory to those pages, while S390 will save > > > the relevant storage key bits in there. > > > > One complication to keep in mind is that we need to know which storage key > > goes to which page frame. We need something like the orig_bm/copy_bm or > > we'd have to store the pfn with the key. Simply storing the key for every > > page will make the array unnecessarily big. > > How big is the overhead? In percent / in megabytes? Well, that depends on the ratio of the size of the hibernation image and the total size of the ram. Consider a 1TB machine with a hibernation image size of lets say 128 GB. The hibernation image would require 32 MB worth of storage keys (0.024%), for 1TB the array size would be 256 MB (0.19%). -- blue skies, Martin. "Reality continues to ruin my life." - Calvin. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-06-15 7:36 ` Martin Schwidefsky ` (3 preceding siblings ...) 2011-07-03 17:46 ` Pavel Machek @ 2011-07-07 21:36 ` Rafael J. Wysocki 2011-07-07 21:36 ` Rafael J. Wysocki 5 siblings, 0 replies; 35+ messages in thread From: Rafael J. Wysocki @ 2011-07-07 21:36 UTC (permalink / raw) To: Martin Schwidefsky; +Cc: linux-s390, linux-kernel, linux-pm, Jiri Slaby On Wednesday, June 15, 2011, Martin Schwidefsky wrote: > On Tue, 14 Jun 2011 22:50:14 +0200 > "Rafael J. Wysocki" <rjw@sisk.pl> wrote: ... > > Well before the preallocation kicked in we don't know which are the relevant > storage keys. The only other option would be to store all of them which I > consider to be a inferior solution. I've been thinking about that recently a bit. Don't we need to restore the keys of the page frames that aren't copied into the image too? Rafael ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-06-15 7:36 ` Martin Schwidefsky ` (4 preceding siblings ...) 2011-07-07 21:36 ` Rafael J. Wysocki @ 2011-07-07 21:36 ` Rafael J. Wysocki 2011-07-08 8:29 ` Martin Schwidefsky 2011-07-08 8:29 ` Martin Schwidefsky 5 siblings, 2 replies; 35+ messages in thread From: Rafael J. Wysocki @ 2011-07-07 21:36 UTC (permalink / raw) To: Martin Schwidefsky Cc: linux-pm, linux-kernel, linux-s390, Pavel Machek, Jiri Slaby, Len Brown On Wednesday, June 15, 2011, Martin Schwidefsky wrote: > On Tue, 14 Jun 2011 22:50:14 +0200 > "Rafael J. Wysocki" <rjw@sisk.pl> wrote: ... > > Well before the preallocation kicked in we don't know which are the relevant > storage keys. The only other option would be to store all of them which I > consider to be a inferior solution. I've been thinking about that recently a bit. Don't we need to restore the keys of the page frames that aren't copied into the image too? Rafael ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-07-07 21:36 ` Rafael J. Wysocki @ 2011-07-08 8:29 ` Martin Schwidefsky 2011-07-08 8:29 ` Martin Schwidefsky 1 sibling, 0 replies; 35+ messages in thread From: Martin Schwidefsky @ 2011-07-08 8:29 UTC (permalink / raw) To: Rafael J. Wysocki Cc: linux-pm, linux-kernel, linux-s390, Pavel Machek, Jiri Slaby, Len Brown On Thu, 7 Jul 2011 23:36:25 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > On Wednesday, June 15, 2011, Martin Schwidefsky wrote: > > On Tue, 14 Jun 2011 22:50:14 +0200 > > "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > ... > > > > Well before the preallocation kicked in we don't know which are the relevant > > storage keys. The only other option would be to store all of them which I > > consider to be a inferior solution. > > I've been thinking about that recently a bit. > > Don't we need to restore the keys of the page frames that aren't copied > into the image too? Pages that are free (= not part of the image) will get a zero storage key once they are allocated again. So no, we do not need to include these keys into the hibernation image. -- blue skies, Martin. "Reality continues to ruin my life." - Calvin. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-07-07 21:36 ` Rafael J. Wysocki 2011-07-08 8:29 ` Martin Schwidefsky @ 2011-07-08 8:29 ` Martin Schwidefsky 1 sibling, 0 replies; 35+ messages in thread From: Martin Schwidefsky @ 2011-07-08 8:29 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: linux-s390, linux-kernel, linux-pm, Jiri Slaby On Thu, 7 Jul 2011 23:36:25 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > On Wednesday, June 15, 2011, Martin Schwidefsky wrote: > > On Tue, 14 Jun 2011 22:50:14 +0200 > > "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > ... > > > > Well before the preallocation kicked in we don't know which are the relevant > > storage keys. The only other option would be to store all of them which I > > consider to be a inferior solution. > > I've been thinking about that recently a bit. > > Don't we need to restore the keys of the page frames that aren't copied > into the image too? Pages that are free (= not part of the image) will get a zero storage key once they are allocated again. So no, we do not need to include these keys into the hibernation image. -- blue skies, Martin. "Reality continues to ruin my life." - Calvin. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-06-14 8:50 ` Martin Schwidefsky 2011-06-14 20:50 ` Rafael J. Wysocki @ 2011-06-14 20:50 ` Rafael J. Wysocki 1 sibling, 0 replies; 35+ messages in thread From: Rafael J. Wysocki @ 2011-06-14 20:50 UTC (permalink / raw) To: Martin Schwidefsky; +Cc: linux-s390, linux-kernel, linux-pm, Jiri Slaby On Tuesday, June 14, 2011, Martin Schwidefsky wrote: > Hi Rafael, Hi, > first of all: thanks for the review. > > On Sun, 12 Jun 2011 14:41:34 +0200 > "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > > > > diff -urpN linux-2.6/arch/s390/kernel/swsusp_asm64.S linux-2.6-patched/arch/s390/kernel/swsusp_asm64.S > > > --- linux-2.6/arch/s390/kernel/swsusp_asm64.S 2011-05-19 06:06:34.000000000 +0200 > > > +++ linux-2.6-patched/arch/s390/kernel/swsusp_asm64.S 2011-06-06 11:14:43.000000000 +0200 > > > @@ -138,11 +138,14 @@ swsusp_arch_resume: > > > 0: > > > lg %r2,8(%r1) > > > lg %r4,0(%r1) > > > + iske %r0,%r4 > > > lghi %r3,PAGE_SIZE > > > lghi %r5,PAGE_SIZE > > > 1: > > > mvcle %r2,%r4,0 > > > jo 1b > > > + lg %r2,8(%r1) > > > + sske %r0,%r2 > > > lg %r1,16(%r1) > > > ltgr %r1,%r1 > > > jnz 0b > > > > I cannot comment on the assembly. > > The swsusp_arch_resume code copies the safe pages to their final target > page. Writing to a page sets the dirty bit in the storage key. Which > means that the storage key needs to be restored >after< the copy has > been done. To avoid having to deal with additional pages where the > storage key are kept while the copy is still pending I simply set the > storage key of the safe page and transfer it from the safe page to the > final target page after the copy has been done. That is what the iske > and the sske instruction are doing. I see. > > > diff -urpN linux-2.6/kernel/power/snapshot.c linux-2.6-patched/kernel/power/snapshot.c > > > --- linux-2.6/kernel/power/snapshot.c 2011-06-06 11:14:39.000000000 +0200 > > > +++ linux-2.6-patched/kernel/power/snapshot.c 2011-06-06 11:14:43.000000000 +0200 > > > @@ -1022,6 +1022,137 @@ static inline void copy_data_page(unsign > > > } > > > #endif /* CONFIG_HIGHMEM */ > > > > > > +#ifdef CONFIG_S390 > > > +/* > > > + * For s390 there is one additional byte associated with each page, > > > + * the storage key. The key consists of the access-control bits > > > + * (alias the key), the fetch-protection bit, the referenced bit > > > + * and the change bit (alias dirty bit). Linux uses only the > > > + * referenced bit and the change bit for pages in the page cache. > > > + * Any modification of a page will set the change bit, any access > > > + * sets the referenced bit. Overindication of the referenced bits > > > + * after a hibernation cycle does not cause any harm but the > > > + * overindication of the change bits would cause trouble. > > > + * Therefore it is necessary to include the storage keys in the > > > + * hibernation image. The storage keys are saved to the most > > > + * significant byte of each page frame number in the list of pfns > > > + * in the hibernation image. > > > + */ > > > > Let me say that is not exactly straightforward. :-) > > > > One thing I don't really understand is where those storage keys are normally > > stored so that they aren't present in the image without this additional > > mechanism. Could you explain that a bit more, please? > > The storage keys are outside of the directly addressable memory, the only > way to get to them is with special instructions: > iske - insert storage key extended, ivsk - insert virtual storage key, > sske - set storage key extended, and rrbe - reset reference bit extended. > The storage key of a page has 7 bits, 4 bit for access-control, the > fetch-protection-bit, the reference-bit and the change-bit. In Linux only > the reference and change bits are used. OK, so it looks like we would only need to store 2 bits per page in additional allocations. > These bits are per physical page, one of the major differences of the s390 > machines compared to other architectures. We had a number of interesting > problems because of pte dirty & referenced vs page dirty & referenced. > For e.g. x86 a page is implicitly clean if it has no mapper. On s390 is > depends on the contents of the storage key. Which is why SetPageUptodate > clears the storage key. > > In regard to hibernation the important aspect is that each and every write > to a page sets the dirty bit even if it is done by I/O. The restore of the > hibernation image therefore makes all pages dirty. To get the correct > original combination of page content and storage key content the storage > key needs to be set after the last access to the page content. What, in your opinion, is the right time for restoring the original storage keys after the image has been loaded? It seems that should happen after passing control to the hibernated kernel. > > > + > > > +/* > > > + * Key storage is allocated as a linked list of pages. > > > + * The size of the keys array is (PAGE_SIZE - sizeof(long)) > > > + */ > > > +struct page_key_data { > > > + struct page_key_data *next; > > > + unsigned char data[]; > > > +}; > > > > This seems to be similar to the data structure for the saving of ACPI NVS > > memory, so perhaps it's possible to combine the two. > > Almost the same, I thought about ways to merge them. I dropped the idea in > order to keep the patch as small as possible. I think, however, that we really should try to merge them. The only difference seems to be how the additionally allocated pages will be populated and what's going to happen to their contents during restore. ACPI will simply copy the NVS memory to those pages, while S390 will save the relevant storage key bits in there. > > > +#define PAGE_KEY_DATA_SIZE (PAGE_SIZE - sizeof(struct page_key_data *)) > > > + > > > +static struct page_key_data *page_key_data; > > > +static struct page_key_data *page_key_rp, *page_key_wp; > > > +static unsigned long page_key_rx, page_key_wx; > > > + > > > +/* > > > + * For each page in the hibernation image one additional byte is > > > + * stored in the most significant byte of the page frame number. > > > > I'm not sure it's really worth the complication. If you simply store those > > keys in separete pages, you'd need one additional page per PAGE_SIZE > > page frames, so given PAGE_SIZE = 4096 that's one page per 16 MB of memory, > > which is not a whole lot (64 extra pages per GB if I'm not mistaken). > > > > It seems that doing it will simplify things quite a git. > > One of the versions of the patch actually did that and it turned out to > be much larger then the proposed solution. That older version had another > step in snapshot_read_next / snapshot_write_next to copy the storage keys > to pages allocated for that purpose. I had some trouble with the way how > the code deals with the orig_bm and the copy_bm though. Not trivial code.. Well, I'm sure we can handle that. :-) I thought of the following approach: * Allocate memory for saving the relevant bits of the storage keys before the memory preallocation kicks in. * Save the storage key bits to that memory before creating the image (be careful about the storage keys of the pages they are being stored into). * After getting control from the boot kernel, restore the storage key bits saved before creating the image. * Free additional memory used for storing them. This way we wouldn't have to worry about any interactions with snapshot_read_next / snapshot_write_next etc. Thanks, Rafael ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-06-12 12:41 ` Rafael J. Wysocki (?) (?) @ 2011-06-14 8:50 ` Martin Schwidefsky -1 siblings, 0 replies; 35+ messages in thread From: Martin Schwidefsky @ 2011-06-14 8:50 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: linux-s390, linux-kernel, linux-pm, Jiri Slaby Hi Rafael, first of all: thanks for the review. On Sun, 12 Jun 2011 14:41:34 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > > diff -urpN linux-2.6/arch/s390/kernel/swsusp_asm64.S linux-2.6-patched/arch/s390/kernel/swsusp_asm64.S > > --- linux-2.6/arch/s390/kernel/swsusp_asm64.S 2011-05-19 06:06:34.000000000 +0200 > > +++ linux-2.6-patched/arch/s390/kernel/swsusp_asm64.S 2011-06-06 11:14:43.000000000 +0200 > > @@ -138,11 +138,14 @@ swsusp_arch_resume: > > 0: > > lg %r2,8(%r1) > > lg %r4,0(%r1) > > + iske %r0,%r4 > > lghi %r3,PAGE_SIZE > > lghi %r5,PAGE_SIZE > > 1: > > mvcle %r2,%r4,0 > > jo 1b > > + lg %r2,8(%r1) > > + sske %r0,%r2 > > lg %r1,16(%r1) > > ltgr %r1,%r1 > > jnz 0b > > I cannot comment on the assembly. The swsusp_arch_resume code copies the safe pages to their final target page. Writing to a page sets the dirty bit in the storage key. Which means that the storage key needs to be restored >after< the copy has been done. To avoid having to deal with additional pages where the storage key are kept while the copy is still pending I simply set the storage key of the safe page and transfer it from the safe page to the final target page after the copy has been done. That is what the iske and the sske instruction are doing. > > diff -urpN linux-2.6/kernel/power/snapshot.c linux-2.6-patched/kernel/power/snapshot.c > > --- linux-2.6/kernel/power/snapshot.c 2011-06-06 11:14:39.000000000 +0200 > > +++ linux-2.6-patched/kernel/power/snapshot.c 2011-06-06 11:14:43.000000000 +0200 > > @@ -1022,6 +1022,137 @@ static inline void copy_data_page(unsign > > } > > #endif /* CONFIG_HIGHMEM */ > > > > +#ifdef CONFIG_S390 > > +/* > > + * For s390 there is one additional byte associated with each page, > > + * the storage key. The key consists of the access-control bits > > + * (alias the key), the fetch-protection bit, the referenced bit > > + * and the change bit (alias dirty bit). Linux uses only the > > + * referenced bit and the change bit for pages in the page cache. > > + * Any modification of a page will set the change bit, any access > > + * sets the referenced bit. Overindication of the referenced bits > > + * after a hibernation cycle does not cause any harm but the > > + * overindication of the change bits would cause trouble. > > + * Therefore it is necessary to include the storage keys in the > > + * hibernation image. The storage keys are saved to the most > > + * significant byte of each page frame number in the list of pfns > > + * in the hibernation image. > > + */ > > Let me say that is not exactly straightforward. :-) > > One thing I don't really understand is where those storage keys are normally > stored so that they aren't present in the image without this additional > mechanism. Could you explain that a bit more, please? The storage keys are outside of the directly addressable memory, the only way to get to them is with special instructions: iske - insert storage key extended, ivsk - insert virtual storage key, sske - set storage key extended, and rrbe - reset reference bit extended. The storage key of a page has 7 bits, 4 bit for access-control, the fetch-protection-bit, the reference-bit and the change-bit. In Linux only the reference and change bits are used. These bits are per physical page, one of the major differences of the s390 machines compared to other architectures. We had a number of interesting problems because of pte dirty & referenced vs page dirty & referenced. For e.g. x86 a page is implicitly clean if it has no mapper. On s390 is depends on the contents of the storage key. Which is why SetPageUptodate clears the storage key. In regard to hibernation the important aspect is that each and every write to a page sets the dirty bit even if it is done by I/O. The restore of the hibernation image therefore makes all pages dirty. To get the correct original combination of page content and storage key content the storage key needs to be set after the last access to the page content. > > + > > +/* > > + * Key storage is allocated as a linked list of pages. > > + * The size of the keys array is (PAGE_SIZE - sizeof(long)) > > + */ > > +struct page_key_data { > > + struct page_key_data *next; > > + unsigned char data[]; > > +}; > > This seems to be similar to the data structure for the saving of ACPI NVS > memory, so perhaps it's possible to combine the two. Almost the same, I thought about ways to merge them. I dropped the idea in order to keep the patch as small as possible. > > +#define PAGE_KEY_DATA_SIZE (PAGE_SIZE - sizeof(struct page_key_data *)) > > + > > +static struct page_key_data *page_key_data; > > +static struct page_key_data *page_key_rp, *page_key_wp; > > +static unsigned long page_key_rx, page_key_wx; > > + > > +/* > > + * For each page in the hibernation image one additional byte is > > + * stored in the most significant byte of the page frame number. > > I'm not sure it's really worth the complication. If you simply store those > keys in separete pages, you'd need one additional page per PAGE_SIZE > page frames, so given PAGE_SIZE = 4096 that's one page per 16 MB of memory, > which is not a whole lot (64 extra pages per GB if I'm not mistaken). > > It seems that doing it will simplify things quite a git. One of the versions of the patch actually did that and it turned out to be much larger then the proposed solution. That older version had another step in snapshot_read_next / snapshot_write_next to copy the storage keys to pages allocated for that purpose. I had some trouble with the way how the code deals with the orig_bm and the copy_bm though. Not trivial code.. In the end I decided to use some of the unused 12 bits in the pfn list entries, it avoids any structural changes to the snapshot code and it resulted in the smallest patch (at least so far). One thing that seemed important to me was to make the patch obviously correct for the other architectures, for !s390 the new functions are nops and there is no structural change. > Apart from this, I'm not sure that kernel/power/snapshot.c is the right > place for all this S390-specific code. I'd rather see it in arch/s390. The alternative would be to move the s390 specific page_key_xxx functions to some architecture file. The hooks in snapshot.c would have to stay though. Unfortunately they are in odd places and you can only understand them in the context of the s390 storage keys. -- blue skies, Martin. "Reality continues to ruin my life." - Calvin. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-06-08 7:45 ` [patch 1/1] [PATCH] " Martin Schwidefsky @ 2011-07-28 22:01 ` Rafael J. Wysocki 2011-07-28 22:01 ` Rafael J. Wysocki 1 sibling, 0 replies; 35+ messages in thread From: Rafael J. Wysocki @ 2011-07-28 22:01 UTC (permalink / raw) To: Martin Schwidefsky Cc: linux-pm, linux-kernel, linux-s390, Pavel Machek, Jiri Slaby Hi, Sorry for the extreme delay. On Wednesday, June 08, 2011, Martin Schwidefsky wrote: > From: Martin Schwidefsky <schwidefsky@de.ibm.com> > > For s390 there is one additional byte associated with each page, > the storage key. This byte contains the referenced and changed > bits and needs to be included into the hibernation image. > If the storage keys are not restored to their previous state all > original pages would appear to be dirty. This can cause > inconsistencies e.g. with read-only filesystems. > > Cc: Pavel Machek <pavel@ucw.cz> > Cc: Rafael J. Wysocki <rjw@sisk.pl> > Cc: Jiri Slaby <jslaby@suse.cz> > Cc: linux-pm@lists.linux-foundation.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-s390@vger.kernel.org > Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com> > --- > > arch/s390/kernel/swsusp_asm64.S | 3 > kernel/power/snapshot.c | 148 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 151 insertions(+) > > diff -urpN linux-2.6/arch/s390/kernel/swsusp_asm64.S linux-2.6-patched/arch/s390/kernel/swsusp_asm64.S > --- linux-2.6/arch/s390/kernel/swsusp_asm64.S 2011-05-19 06:06:34.000000000 +0200 > +++ linux-2.6-patched/arch/s390/kernel/swsusp_asm64.S 2011-06-06 11:14:43.000000000 +0200 > @@ -138,11 +138,14 @@ swsusp_arch_resume: > 0: > lg %r2,8(%r1) > lg %r4,0(%r1) > + iske %r0,%r4 > lghi %r3,PAGE_SIZE > lghi %r5,PAGE_SIZE > 1: > mvcle %r2,%r4,0 > jo 1b > + lg %r2,8(%r1) > + sske %r0,%r2 > lg %r1,16(%r1) > ltgr %r1,%r1 > jnz 0b > diff -urpN linux-2.6/kernel/power/snapshot.c linux-2.6-patched/kernel/power/snapshot.c > --- linux-2.6/kernel/power/snapshot.c 2011-06-06 11:14:39.000000000 +0200 > +++ linux-2.6-patched/kernel/power/snapshot.c 2011-06-06 11:14:43.000000000 +0200 > @@ -1022,6 +1022,137 @@ static inline void copy_data_page(unsign > } > #endif /* CONFIG_HIGHMEM */ > > +#ifdef CONFIG_S390 > +/* > + * For s390 there is one additional byte associated with each page, > + * the storage key. The key consists of the access-control bits > + * (alias the key), the fetch-protection bit, the referenced bit > + * and the change bit (alias dirty bit). Linux uses only the > + * referenced bit and the change bit for pages in the page cache. > + * Any modification of a page will set the change bit, any access > + * sets the referenced bit. Overindication of the referenced bits > + * after a hibernation cycle does not cause any harm but the > + * overindication of the change bits would cause trouble. > + * Therefore it is necessary to include the storage keys in the > + * hibernation image. The storage keys are saved to the most > + * significant byte of each page frame number in the list of pfns > + * in the hibernation image. > + */ > + > +/* > + * Key storage is allocated as a linked list of pages. > + * The size of the keys array is (PAGE_SIZE - sizeof(long)) > + */ > +struct page_key_data { > + struct page_key_data *next; > + unsigned char data[]; > +}; > + > +#define PAGE_KEY_DATA_SIZE (PAGE_SIZE - sizeof(struct page_key_data *)) > + > +static struct page_key_data *page_key_data; > +static struct page_key_data *page_key_rp, *page_key_wp; > +static unsigned long page_key_rx, page_key_wx; > + > +/* > + * For each page in the hibernation image one additional byte is > + * stored in the most significant byte of the page frame number. > + * On suspend no additional memory is required but on resume the > + * keys need to be memorized until the page data has been restored. > + * Only then can the storage keys be set to their old state. > + */ > +static inline unsigned long page_key_additional_pages(unsigned long pages) > +{ > + return DIV_ROUND_UP(pages, PAGE_KEY_DATA_SIZE); > +} > + > +/* > + * Free page_key_data list of arrays. > + */ > +static void page_key_free(void) > +{ > + struct page_key_data *pkd; > + > + while (page_key_data) { > + pkd = page_key_data; > + page_key_data = pkd->next; > + free_page((unsigned long) pkd); > + } > +} > + > +/* > + * Allocate page_key_data list of arrays with enough room to store > + * one byte for each page in the hibernation image. > + */ > +static int page_key_alloc(unsigned long pages) > +{ > + struct page_key_data *pk; > + unsigned long size; > + > + size = DIV_ROUND_UP(pages, PAGE_KEY_DATA_SIZE); > + while (size--) { > + pk = (struct page_key_data *) get_zeroed_page(GFP_KERNEL); > + if (!pk) { > + page_key_free(); > + return -ENOMEM; > + } > + pk->next = page_key_data; > + page_key_data = pk; > + } > + page_key_rp = page_key_wp = page_key_data; > + page_key_rx = page_key_wx = 0; > + return 0; > +} > + > +/* > + * Save the storage key into the upper 8 bits of the page frame number. > + */ > +static inline void page_key_read(unsigned long *pfn) > +{ > + unsigned long addr; > + > + addr = (unsigned long) page_address(pfn_to_page(*pfn)); > + *(unsigned char *) pfn = (unsigned char) page_get_storage_key(addr); > +} > + > +/* > + * Extract the storage key from the upper 8 bits of the page frame number > + * and store it in the page_key_data list of arrays. > + */ > +static inline void page_key_memorize(unsigned long *pfn) > +{ > + page_key_wp->data[page_key_wx] = *(unsigned char *) pfn; > + *(unsigned char *) pfn = 0; > + if (++page_key_wx < PAGE_KEY_DATA_SIZE) > + return; > + page_key_wp = page_key_wp->next; > + page_key_wx = 0; > +} > + > +/* > + * Get the next key from the page_key_data list of arrays and set the > + * storage key of the page referred by @address. If @address refers to > + * a "safe" page the swsusp_arch_resume code will transfer the storage > + * key from the buffer page to the original page. > + */ > +static void page_key_write(void *address) > +{ > + page_set_storage_key((unsigned long) address, > + page_key_rp->data[page_key_rx], 0); > + if (++page_key_rx >= PAGE_KEY_DATA_SIZE) > + return; > + page_key_rp = page_key_rp->next; > + page_key_rx = 0; > +} > +#else > +static unsigned long page_key_additional_pages(unsigned long pages) { return 0; } > +static inline int page_key_alloc(unsigned long pages) { return 0; } > +static inline void page_key_free(void) { } > +static inline void page_key_read(unsigned long *pfn) { } > +static inline void page_key_memorize(unsigned long *pfn) { } > +static inline void page_key_write(void *address) { } > +#endif Having reconsidered things I think the code under the #ifdef above should really go to arch/s390. Now, for the purpose of exporting the headers I'd introduce CONFIG_ARCH_SAVE_PAGE_KEYS and make S390 do select ARCH_SAVE_PAGE_KEYS if HIBERNATION and I'd put a #ifdef depending on that into include/linux/suspend.h. Apart from this, I have only one complaint, which is that the kerneldoc comments should follow the standard (the other comments in snapshot.c don't, but that's a matter for a separate patch). Thanks, Rafael ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. @ 2011-07-28 22:01 ` Rafael J. Wysocki 0 siblings, 0 replies; 35+ messages in thread From: Rafael J. Wysocki @ 2011-07-28 22:01 UTC (permalink / raw) To: Martin Schwidefsky; +Cc: linux-s390, linux-pm, Jiri Slaby, linux-kernel Hi, Sorry for the extreme delay. On Wednesday, June 08, 2011, Martin Schwidefsky wrote: > From: Martin Schwidefsky <schwidefsky@de.ibm.com> > > For s390 there is one additional byte associated with each page, > the storage key. This byte contains the referenced and changed > bits and needs to be included into the hibernation image. > If the storage keys are not restored to their previous state all > original pages would appear to be dirty. This can cause > inconsistencies e.g. with read-only filesystems. > > Cc: Pavel Machek <pavel@ucw.cz> > Cc: Rafael J. Wysocki <rjw@sisk.pl> > Cc: Jiri Slaby <jslaby@suse.cz> > Cc: linux-pm@lists.linux-foundation.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-s390@vger.kernel.org > Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com> > --- > > arch/s390/kernel/swsusp_asm64.S | 3 > kernel/power/snapshot.c | 148 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 151 insertions(+) > > diff -urpN linux-2.6/arch/s390/kernel/swsusp_asm64.S linux-2.6-patched/arch/s390/kernel/swsusp_asm64.S > --- linux-2.6/arch/s390/kernel/swsusp_asm64.S 2011-05-19 06:06:34.000000000 +0200 > +++ linux-2.6-patched/arch/s390/kernel/swsusp_asm64.S 2011-06-06 11:14:43.000000000 +0200 > @@ -138,11 +138,14 @@ swsusp_arch_resume: > 0: > lg %r2,8(%r1) > lg %r4,0(%r1) > + iske %r0,%r4 > lghi %r3,PAGE_SIZE > lghi %r5,PAGE_SIZE > 1: > mvcle %r2,%r4,0 > jo 1b > + lg %r2,8(%r1) > + sske %r0,%r2 > lg %r1,16(%r1) > ltgr %r1,%r1 > jnz 0b > diff -urpN linux-2.6/kernel/power/snapshot.c linux-2.6-patched/kernel/power/snapshot.c > --- linux-2.6/kernel/power/snapshot.c 2011-06-06 11:14:39.000000000 +0200 > +++ linux-2.6-patched/kernel/power/snapshot.c 2011-06-06 11:14:43.000000000 +0200 > @@ -1022,6 +1022,137 @@ static inline void copy_data_page(unsign > } > #endif /* CONFIG_HIGHMEM */ > > +#ifdef CONFIG_S390 > +/* > + * For s390 there is one additional byte associated with each page, > + * the storage key. The key consists of the access-control bits > + * (alias the key), the fetch-protection bit, the referenced bit > + * and the change bit (alias dirty bit). Linux uses only the > + * referenced bit and the change bit for pages in the page cache. > + * Any modification of a page will set the change bit, any access > + * sets the referenced bit. Overindication of the referenced bits > + * after a hibernation cycle does not cause any harm but the > + * overindication of the change bits would cause trouble. > + * Therefore it is necessary to include the storage keys in the > + * hibernation image. The storage keys are saved to the most > + * significant byte of each page frame number in the list of pfns > + * in the hibernation image. > + */ > + > +/* > + * Key storage is allocated as a linked list of pages. > + * The size of the keys array is (PAGE_SIZE - sizeof(long)) > + */ > +struct page_key_data { > + struct page_key_data *next; > + unsigned char data[]; > +}; > + > +#define PAGE_KEY_DATA_SIZE (PAGE_SIZE - sizeof(struct page_key_data *)) > + > +static struct page_key_data *page_key_data; > +static struct page_key_data *page_key_rp, *page_key_wp; > +static unsigned long page_key_rx, page_key_wx; > + > +/* > + * For each page in the hibernation image one additional byte is > + * stored in the most significant byte of the page frame number. > + * On suspend no additional memory is required but on resume the > + * keys need to be memorized until the page data has been restored. > + * Only then can the storage keys be set to their old state. > + */ > +static inline unsigned long page_key_additional_pages(unsigned long pages) > +{ > + return DIV_ROUND_UP(pages, PAGE_KEY_DATA_SIZE); > +} > + > +/* > + * Free page_key_data list of arrays. > + */ > +static void page_key_free(void) > +{ > + struct page_key_data *pkd; > + > + while (page_key_data) { > + pkd = page_key_data; > + page_key_data = pkd->next; > + free_page((unsigned long) pkd); > + } > +} > + > +/* > + * Allocate page_key_data list of arrays with enough room to store > + * one byte for each page in the hibernation image. > + */ > +static int page_key_alloc(unsigned long pages) > +{ > + struct page_key_data *pk; > + unsigned long size; > + > + size = DIV_ROUND_UP(pages, PAGE_KEY_DATA_SIZE); > + while (size--) { > + pk = (struct page_key_data *) get_zeroed_page(GFP_KERNEL); > + if (!pk) { > + page_key_free(); > + return -ENOMEM; > + } > + pk->next = page_key_data; > + page_key_data = pk; > + } > + page_key_rp = page_key_wp = page_key_data; > + page_key_rx = page_key_wx = 0; > + return 0; > +} > + > +/* > + * Save the storage key into the upper 8 bits of the page frame number. > + */ > +static inline void page_key_read(unsigned long *pfn) > +{ > + unsigned long addr; > + > + addr = (unsigned long) page_address(pfn_to_page(*pfn)); > + *(unsigned char *) pfn = (unsigned char) page_get_storage_key(addr); > +} > + > +/* > + * Extract the storage key from the upper 8 bits of the page frame number > + * and store it in the page_key_data list of arrays. > + */ > +static inline void page_key_memorize(unsigned long *pfn) > +{ > + page_key_wp->data[page_key_wx] = *(unsigned char *) pfn; > + *(unsigned char *) pfn = 0; > + if (++page_key_wx < PAGE_KEY_DATA_SIZE) > + return; > + page_key_wp = page_key_wp->next; > + page_key_wx = 0; > +} > + > +/* > + * Get the next key from the page_key_data list of arrays and set the > + * storage key of the page referred by @address. If @address refers to > + * a "safe" page the swsusp_arch_resume code will transfer the storage > + * key from the buffer page to the original page. > + */ > +static void page_key_write(void *address) > +{ > + page_set_storage_key((unsigned long) address, > + page_key_rp->data[page_key_rx], 0); > + if (++page_key_rx >= PAGE_KEY_DATA_SIZE) > + return; > + page_key_rp = page_key_rp->next; > + page_key_rx = 0; > +} > +#else > +static unsigned long page_key_additional_pages(unsigned long pages) { return 0; } > +static inline int page_key_alloc(unsigned long pages) { return 0; } > +static inline void page_key_free(void) { } > +static inline void page_key_read(unsigned long *pfn) { } > +static inline void page_key_memorize(unsigned long *pfn) { } > +static inline void page_key_write(void *address) { } > +#endif Having reconsidered things I think the code under the #ifdef above should really go to arch/s390. Now, for the purpose of exporting the headers I'd introduce CONFIG_ARCH_SAVE_PAGE_KEYS and make S390 do select ARCH_SAVE_PAGE_KEYS if HIBERNATION and I'd put a #ifdef depending on that into include/linux/suspend.h. Apart from this, I have only one complaint, which is that the kerneldoc comments should follow the standard (the other comments in snapshot.c don't, but that's a matter for a separate patch). Thanks, Rafael ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-07-28 22:01 ` Rafael J. Wysocki (?) @ 2011-08-09 15:45 ` Martin Schwidefsky 2011-08-09 19:56 ` Rafael J. Wysocki 2011-08-09 19:56 ` Rafael J. Wysocki -1 siblings, 2 replies; 35+ messages in thread From: Martin Schwidefsky @ 2011-08-09 15:45 UTC (permalink / raw) To: Rafael J. Wysocki Cc: linux-pm, linux-kernel, linux-s390, Pavel Machek, Jiri Slaby On Fri, 29 Jul 2011 00:01:14 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > Sorry for the extreme delay. No big deal, I have been busy with other things anyway. > Having reconsidered things I think the code under the #ifdef above > should really go to arch/s390. Ok, that is reasonable. > Now, for the purpose of exporting the headers I'd introduce > CONFIG_ARCH_SAVE_PAGE_KEYS and make S390 do > > select ARCH_SAVE_PAGE_KEYS if HIBERNATION > > and I'd put a #ifdef depending on that into include/linux/suspend.h. > > Apart from this, I have only one complaint, which is that the kerneldoc > comments should follow the standard (the other comments in snapshot.c don't, > but that's a matter for a separate patch). Sounds good. I will come up with new patches for this and resend them for review. Might be one or two weeks though, currently conferencing in Orlando.. -- blue skies, Martin. "Reality continues to ruin my life." - Calvin. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-08-09 15:45 ` Martin Schwidefsky @ 2011-08-09 19:56 ` Rafael J. Wysocki 2011-08-09 19:56 ` Rafael J. Wysocki 1 sibling, 0 replies; 35+ messages in thread From: Rafael J. Wysocki @ 2011-08-09 19:56 UTC (permalink / raw) To: Martin Schwidefsky; +Cc: linux-s390, linux-pm, Jiri Slaby, linux-kernel On Tuesday, August 09, 2011, Martin Schwidefsky wrote: > On Fri, 29 Jul 2011 00:01:14 +0200 > "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > > > Sorry for the extreme delay. > > No big deal, I have been busy with other things anyway. > > > Having reconsidered things I think the code under the #ifdef above > > should really go to arch/s390. > > Ok, that is reasonable. > > > Now, for the purpose of exporting the headers I'd introduce > > CONFIG_ARCH_SAVE_PAGE_KEYS and make S390 do > > > > select ARCH_SAVE_PAGE_KEYS if HIBERNATION > > > > and I'd put a #ifdef depending on that into include/linux/suspend.h. > > > > Apart from this, I have only one complaint, which is that the kerneldoc > > comments should follow the standard (the other comments in snapshot.c don't, > > but that's a matter for a separate patch). > > Sounds good. I will come up with new patches for this and resend them > for review. Might be one or two weeks though, currently conferencing in > Orlando.. OK, thanks! ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-08-09 15:45 ` Martin Schwidefsky 2011-08-09 19:56 ` Rafael J. Wysocki @ 2011-08-09 19:56 ` Rafael J. Wysocki 1 sibling, 0 replies; 35+ messages in thread From: Rafael J. Wysocki @ 2011-08-09 19:56 UTC (permalink / raw) To: Martin Schwidefsky Cc: linux-pm, linux-kernel, linux-s390, Pavel Machek, Jiri Slaby On Tuesday, August 09, 2011, Martin Schwidefsky wrote: > On Fri, 29 Jul 2011 00:01:14 +0200 > "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > > > Sorry for the extreme delay. > > No big deal, I have been busy with other things anyway. > > > Having reconsidered things I think the code under the #ifdef above > > should really go to arch/s390. > > Ok, that is reasonable. > > > Now, for the purpose of exporting the headers I'd introduce > > CONFIG_ARCH_SAVE_PAGE_KEYS and make S390 do > > > > select ARCH_SAVE_PAGE_KEYS if HIBERNATION > > > > and I'd put a #ifdef depending on that into include/linux/suspend.h. > > > > Apart from this, I have only one complaint, which is that the kerneldoc > > comments should follow the standard (the other comments in snapshot.c don't, > > but that's a matter for a separate patch). > > Sounds good. I will come up with new patches for this and resend them > for review. Might be one or two weeks though, currently conferencing in > Orlando.. OK, thanks! ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-07-28 22:01 ` Rafael J. Wysocki (?) (?) @ 2011-08-09 15:45 ` Martin Schwidefsky -1 siblings, 0 replies; 35+ messages in thread From: Martin Schwidefsky @ 2011-08-09 15:45 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: linux-s390, linux-pm, Jiri Slaby, linux-kernel On Fri, 29 Jul 2011 00:01:14 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > Sorry for the extreme delay. No big deal, I have been busy with other things anyway. > Having reconsidered things I think the code under the #ifdef above > should really go to arch/s390. Ok, that is reasonable. > Now, for the purpose of exporting the headers I'd introduce > CONFIG_ARCH_SAVE_PAGE_KEYS and make S390 do > > select ARCH_SAVE_PAGE_KEYS if HIBERNATION > > and I'd put a #ifdef depending on that into include/linux/suspend.h. > > Apart from this, I have only one complaint, which is that the kerneldoc > comments should follow the standard (the other comments in snapshot.c don't, > but that's a matter for a separate patch). Sounds good. I will come up with new patches for this and resend them for review. Might be one or two weeks though, currently conferencing in Orlando.. -- blue skies, Martin. "Reality continues to ruin my life." - Calvin. ^ permalink raw reply [flat|nested] 35+ messages in thread
* [patch 1/1] [PATCH] include storage keys in hibernation image. 2011-06-08 7:45 [patch 0/1] [RFC] " Martin Schwidefsky 2011-06-08 7:45 ` [patch 1/1] [PATCH] " Martin Schwidefsky @ 2011-06-08 7:45 ` Martin Schwidefsky 1 sibling, 0 replies; 35+ messages in thread From: Martin Schwidefsky @ 2011-06-08 7:45 UTC (permalink / raw) To: linux-pm, linux-kernel, linux-s390, Pavel Machek, Rafael J. Wysocki, Jiri Cc: Martin Schwidefsky [-- Attachment #1: 201-hibernate-storage-keys.diff --] [-- Type: text/plain, Size: 7936 bytes --] From: Martin Schwidefsky <schwidefsky@de.ibm.com> For s390 there is one additional byte associated with each page, the storage key. This byte contains the referenced and changed bits and needs to be included into the hibernation image. If the storage keys are not restored to their previous state all original pages would appear to be dirty. This can cause inconsistencies e.g. with read-only filesystems. Cc: Pavel Machek <pavel@ucw.cz> Cc: Rafael J. Wysocki <rjw@sisk.pl> Cc: Jiri Slaby <jslaby@suse.cz> Cc: linux-pm@lists.linux-foundation.org Cc: linux-kernel@vger.kernel.org Cc: linux-s390@vger.kernel.org Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com> --- arch/s390/kernel/swsusp_asm64.S | 3 kernel/power/snapshot.c | 148 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 151 insertions(+) diff -urpN linux-2.6/arch/s390/kernel/swsusp_asm64.S linux-2.6-patched/arch/s390/kernel/swsusp_asm64.S --- linux-2.6/arch/s390/kernel/swsusp_asm64.S 2011-05-19 06:06:34.000000000 +0200 +++ linux-2.6-patched/arch/s390/kernel/swsusp_asm64.S 2011-06-06 11:14:43.000000000 +0200 @@ -138,11 +138,14 @@ swsusp_arch_resume: 0: lg %r2,8(%r1) lg %r4,0(%r1) + iske %r0,%r4 lghi %r3,PAGE_SIZE lghi %r5,PAGE_SIZE 1: mvcle %r2,%r4,0 jo 1b + lg %r2,8(%r1) + sske %r0,%r2 lg %r1,16(%r1) ltgr %r1,%r1 jnz 0b diff -urpN linux-2.6/kernel/power/snapshot.c linux-2.6-patched/kernel/power/snapshot.c --- linux-2.6/kernel/power/snapshot.c 2011-06-06 11:14:39.000000000 +0200 +++ linux-2.6-patched/kernel/power/snapshot.c 2011-06-06 11:14:43.000000000 +0200 @@ -1022,6 +1022,137 @@ static inline void copy_data_page(unsign } #endif /* CONFIG_HIGHMEM */ +#ifdef CONFIG_S390 +/* + * For s390 there is one additional byte associated with each page, + * the storage key. The key consists of the access-control bits + * (alias the key), the fetch-protection bit, the referenced bit + * and the change bit (alias dirty bit). Linux uses only the + * referenced bit and the change bit for pages in the page cache. + * Any modification of a page will set the change bit, any access + * sets the referenced bit. Overindication of the referenced bits + * after a hibernation cycle does not cause any harm but the + * overindication of the change bits would cause trouble. + * Therefore it is necessary to include the storage keys in the + * hibernation image. The storage keys are saved to the most + * significant byte of each page frame number in the list of pfns + * in the hibernation image. + */ + +/* + * Key storage is allocated as a linked list of pages. + * The size of the keys array is (PAGE_SIZE - sizeof(long)) + */ +struct page_key_data { + struct page_key_data *next; + unsigned char data[]; +}; + +#define PAGE_KEY_DATA_SIZE (PAGE_SIZE - sizeof(struct page_key_data *)) + +static struct page_key_data *page_key_data; +static struct page_key_data *page_key_rp, *page_key_wp; +static unsigned long page_key_rx, page_key_wx; + +/* + * For each page in the hibernation image one additional byte is + * stored in the most significant byte of the page frame number. + * On suspend no additional memory is required but on resume the + * keys need to be memorized until the page data has been restored. + * Only then can the storage keys be set to their old state. + */ +static inline unsigned long page_key_additional_pages(unsigned long pages) +{ + return DIV_ROUND_UP(pages, PAGE_KEY_DATA_SIZE); +} + +/* + * Free page_key_data list of arrays. + */ +static void page_key_free(void) +{ + struct page_key_data *pkd; + + while (page_key_data) { + pkd = page_key_data; + page_key_data = pkd->next; + free_page((unsigned long) pkd); + } +} + +/* + * Allocate page_key_data list of arrays with enough room to store + * one byte for each page in the hibernation image. + */ +static int page_key_alloc(unsigned long pages) +{ + struct page_key_data *pk; + unsigned long size; + + size = DIV_ROUND_UP(pages, PAGE_KEY_DATA_SIZE); + while (size--) { + pk = (struct page_key_data *) get_zeroed_page(GFP_KERNEL); + if (!pk) { + page_key_free(); + return -ENOMEM; + } + pk->next = page_key_data; + page_key_data = pk; + } + page_key_rp = page_key_wp = page_key_data; + page_key_rx = page_key_wx = 0; + return 0; +} + +/* + * Save the storage key into the upper 8 bits of the page frame number. + */ +static inline void page_key_read(unsigned long *pfn) +{ + unsigned long addr; + + addr = (unsigned long) page_address(pfn_to_page(*pfn)); + *(unsigned char *) pfn = (unsigned char) page_get_storage_key(addr); +} + +/* + * Extract the storage key from the upper 8 bits of the page frame number + * and store it in the page_key_data list of arrays. + */ +static inline void page_key_memorize(unsigned long *pfn) +{ + page_key_wp->data[page_key_wx] = *(unsigned char *) pfn; + *(unsigned char *) pfn = 0; + if (++page_key_wx < PAGE_KEY_DATA_SIZE) + return; + page_key_wp = page_key_wp->next; + page_key_wx = 0; +} + +/* + * Get the next key from the page_key_data list of arrays and set the + * storage key of the page referred by @address. If @address refers to + * a "safe" page the swsusp_arch_resume code will transfer the storage + * key from the buffer page to the original page. + */ +static void page_key_write(void *address) +{ + page_set_storage_key((unsigned long) address, + page_key_rp->data[page_key_rx], 0); + if (++page_key_rx >= PAGE_KEY_DATA_SIZE) + return; + page_key_rp = page_key_rp->next; + page_key_rx = 0; +} +#else +static unsigned long page_key_additional_pages(unsigned long pages) { return 0; } +static inline int page_key_alloc(unsigned long pages) { return 0; } +static inline void page_key_free(void) { } +static inline void page_key_read(unsigned long *pfn) { } +static inline void page_key_memorize(unsigned long *pfn) { } +static inline void page_key_write(void *address) { } +#endif + static void copy_data_pages(struct memory_bitmap *copy_bm, struct memory_bitmap *orig_bm) { @@ -1335,6 +1466,9 @@ int hibernate_preallocate_memory(void) count += highmem; count -= totalreserve_pages; + /* Add number of pages required for storage keys (s390 only). */ + size += page_key_additional_pages(saveable); + /* Compute the maximum number of saveable pages to leave in memory. */ max_size = (count - (size + PAGES_FOR_IO)) / 2 - 2 * DIV_ROUND_UP(reserved_size, PAGE_SIZE); @@ -1658,6 +1792,8 @@ pack_pfns(unsigned long *buf, struct mem buf[j] = memory_bm_next_pfn(bm); if (unlikely(buf[j] == BM_END_OF_MAP)) break; + /* Save storage key for data page (s390 only). */ + page_key_read(buf + j); } } @@ -1817,6 +1953,9 @@ static int unpack_orig_pfns(unsigned lon if (unlikely(buf[j] == BM_END_OF_MAP)) break; + /* Extract and buffer storage key for data page (s390 only). */ + page_key_memorize(buf + j); + if (memory_bm_pfn_present(bm, buf[j])) memory_bm_set_bit(bm, buf[j]); else @@ -2219,6 +2358,10 @@ int snapshot_write_next(struct snapshot_ if (error) return error; + error = page_key_alloc(nr_copy_pages); + if (error) + return error; + } else if (handle->cur <= nr_meta_pages + 1) { error = unpack_orig_pfns(buffer, ©_bm); if (error) @@ -2239,6 +2382,8 @@ int snapshot_write_next(struct snapshot_ } } else { copy_last_highmem_page(); + /* Restore storage key for data page (s390 only). */ + page_key_write(handle->buffer); handle->buffer = get_buffer(&orig_bm, &ca); if (IS_ERR(handle->buffer)) return PTR_ERR(handle->buffer); @@ -2260,6 +2405,9 @@ int snapshot_write_next(struct snapshot_ void snapshot_write_finalize(struct snapshot_handle *handle) { copy_last_highmem_page(); + /* Restore storage key for data page (s390 only). */ + page_key_write(handle->buffer); + page_key_free(); /* Free only if we have loaded the image entirely */ if (handle->cur > 1 && handle->cur > nr_meta_pages + nr_copy_pages) { memory_bm_free(&orig_bm, PG_UNSAFE_CLEAR); ^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2011-08-17 9:15 UTC | newest] Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-08-16 14:14 [patch 0/1] [patch 0/1] include storage keys in hibernation image Martin Schwidefsky 2011-08-16 14:14 ` [patch 1/1] [PATCH] " Martin Schwidefsky 2011-08-16 14:14 ` Martin Schwidefsky 2011-08-16 17:56 ` Rafael J. Wysocki 2011-08-16 17:56 ` Rafael J. Wysocki 2011-08-17 7:43 ` Martin Schwidefsky 2011-08-17 7:43 ` Martin Schwidefsky 2011-08-17 9:15 ` Rafael J. Wysocki 2011-08-17 9:15 ` Rafael J. Wysocki -- strict thread matches above, loose matches on Subject: below -- 2011-06-08 7:45 [patch 0/1] [RFC] " Martin Schwidefsky 2011-06-08 7:45 ` [patch 1/1] [PATCH] " Martin Schwidefsky 2011-06-12 12:41 ` Rafael J. Wysocki 2011-06-12 12:41 ` Rafael J. Wysocki 2011-06-14 8:50 ` Martin Schwidefsky 2011-06-14 20:50 ` Rafael J. Wysocki 2011-06-15 7:36 ` Martin Schwidefsky 2011-06-15 7:36 ` Martin Schwidefsky 2011-06-15 23:21 ` Rafael J. Wysocki 2011-06-15 23:21 ` Rafael J. Wysocki 2011-07-03 17:46 ` Pavel Machek 2011-07-03 17:46 ` Pavel Machek 2011-07-04 8:09 ` Martin Schwidefsky 2011-07-04 8:09 ` Martin Schwidefsky 2011-07-07 21:36 ` Rafael J. Wysocki 2011-07-07 21:36 ` Rafael J. Wysocki 2011-07-08 8:29 ` Martin Schwidefsky 2011-07-08 8:29 ` Martin Schwidefsky 2011-06-14 20:50 ` Rafael J. Wysocki 2011-06-14 8:50 ` Martin Schwidefsky 2011-07-28 22:01 ` Rafael J. Wysocki 2011-07-28 22:01 ` Rafael J. Wysocki 2011-08-09 15:45 ` Martin Schwidefsky 2011-08-09 19:56 ` Rafael J. Wysocki 2011-08-09 19:56 ` Rafael J. Wysocki 2011-08-09 15:45 ` Martin Schwidefsky 2011-06-08 7:45 ` Martin Schwidefsky
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.