Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
flat view
From: Michael Ellerman <hidden>
Date: 2008-03-11 01:02:21
On Thu, 2008-02-28 at 18:24 -0600, Manish Ahuja wrote:
Initial patch for reserving memory in early boot, and freeing it later. If the previous boot had ended with a crash, the reserved memory would contain a copy of the crashed kernel data. Signed-off-by: Manish Ahuja <redacted> Signed-off-by: Linas Vepstas <linasvepstas@gmail.com>
Hi Manish, A few comments inline ..
quoted hunk ↗ jump to hunk
Index: 2.6.25-rc1/arch/powerpc/platforms/pseries/phyp_dump.c ===================================================================--- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ 2.6.25-rc1/arch/powerpc/platforms/pseries/phyp_dump.c 2008-02-28 21:57:52.000000000 -0600@@ -0,0 +1,105 @@ +/* + * Hypervisor-assisted dump + * + * Linas Vepstas, Manish Ahuja 2008 + * Copyright 2008 IBM Corp. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version + * 2 of the License, or (at your option) any later version. + * + */ + +#include <linux/init.h> +#include <linux/mm.h> +#include <linux/pfn.h> +#include <linux/swap.h> + +#include <asm/page.h> +#include <asm/phyp_dump.h> +#include <asm/machdep.h> +#include <asm/prom.h> + +/* Global, used to communicate data between early boot and late boot */ +static struct phyp_dump phyp_dump_global; +struct phyp_dump *phyp_dump_info = &phyp_dump_global;
I don't see the point of this. You have a static (ie. non-global) struct called phyp_dump_global, then you create a pointer to it and pass that around. It could just be: phyp_dump.h: extern struct phyp_dump phyp_dump_info; phyp_dump.c: struct phyp_dump phyp_dump_info; phyp_dump_info.foo = bar; I also think the struct should be called phyp_dump_info, not phyp_dump - it contains info about phyp_dump, not the dump itself.
quoted hunk ↗ jump to hunk
+ +/** + * release_memory_range -- release memory previously lmb_reserved + * @start_pfn: starting physical frame number + * @nr_pages: number of pages to free. + * + * This routine will release memory that had been previously + * lmb_reserved in early boot. The released memory becomes + * available for genreal use. + */ +static void +release_memory_range(unsigned long start_pfn, unsigned long nr_pages) +{ + struct page *rpage; + unsigned long end_pfn; + long i; + + end_pfn = start_pfn + nr_pages; + + for (i = start_pfn; i <= end_pfn; i++) { + rpage = pfn_to_page(i); + if (PageReserved(rpage)) { + ClearPageReserved(rpage); + init_page_count(rpage); + __free_page(rpage); + totalram_pages++; + } + } +} + +static int __init phyp_dump_setup(void) +{ + unsigned long start_pfn, nr_pages; + + /* If no memory was reserved in early boot, there is nothing to do */ + if (phyp_dump_info->init_reserve_size == 0) + return 0; + + /* Release memory that was reserved in early boot */ + start_pfn = PFN_DOWN(phyp_dump_info->init_reserve_start); + nr_pages = PFN_DOWN(phyp_dump_info->init_reserve_size); + release_memory_range(start_pfn, nr_pages); + + return 0; +} +machine_subsys_initcall(pseries, phyp_dump_setup); + +int __init early_init_dt_scan_phyp_dump(unsigned long node, + const char *uname, int depth, void *data) +{ +#ifdef CONFIG_PHYP_DUMP + const unsigned int *sizes; + + phyp_dump_info->phyp_dump_configured = 0; + phyp_dump_info->phyp_dump_is_active = 0; + + if (depth != 1 || strcmp(uname, "rtas") != 0) + return 0; + + if (of_get_flat_dt_prop(node, "ibm,configure-kernel-dump", NULL)) + phyp_dump_info->phyp_dump_configured++; + + if (of_get_flat_dt_prop(node, "ibm,dump-kernel", NULL)) + phyp_dump_info->phyp_dump_is_active++; + + sizes = of_get_flat_dt_prop(node, "ibm,configure-kernel-dump-sizes", + NULL); + if (!sizes) + return 0; + + if (sizes[0] == 1) + phyp_dump_info->cpu_state_size = *((unsigned long *)&sizes[1]); + + if (sizes[3] == 2) + phyp_dump_info->hpte_region_size = + *((unsigned long *)&sizes[4]); +#endif
This doesn't need to be inside #ifdef, you have a dummy version already defined in the header file.
quoted hunk ↗ jump to hunk
Index: 2.6.25-rc1/arch/powerpc/kernel/prom.c ===================================================================--- 2.6.25-rc1.orig/arch/powerpc/kernel/prom.c 2008-02-28 21:54:57.000000000 -0600 +++ 2.6.25-rc1/arch/powerpc/kernel/prom.c 2008-02-28 21:55:27.000000000 -0600@@ -1039,6 +1040,51 @@ static void __init early_reserve_mem(voi #endif } +#ifdef CONFIG_PHYP_DUMP +/** + * reserve_crashed_mem() - reserve all not-yet-dumped mmemory + * + * This routine may reserve memory regions in the kernel only + * if the system is supported and a dump was taken in last + * boot instance or if the hardware is supported and the + * scratch area needs to be setup. In other instances it returns + * without reserving anything. The memory in case of dump being + * active is freed when the dump is collected (by userland tools). + */ +static void __init reserve_crashed_mem(void)
This could do with a name change IMO, eg. phyp_dump_reserve_mem() or something.
quoted hunk ↗ jump to hunk
+{ + unsigned long base, size; + if (!phyp_dump_info->phyp_dump_configured) { + printk(KERN_ERR "Phyp-dump not supported on this hardware\n"); + return; + } + + if (phyp_dump_info->phyp_dump_is_active) { + /* Reserve *everything* above RMR.Area freed by userland tools*/ + base = PHYP_DUMP_RMR_END; + size = lmb_end_of_DRAM() - base; + + /* XXX crashed_ram_end is wrong, since it may be beyond + * the memory_limit, it will need to be adjusted. */ + lmb_reserve(base, size); + + phyp_dump_info->init_reserve_start = base; + phyp_dump_info->init_reserve_size = size; + } else { + size = phyp_dump_info->cpu_state_size + + phyp_dump_info->hpte_region_size + + PHYP_DUMP_RMR_END; + base = lmb_end_of_DRAM() - size; + lmb_reserve(base, size); + phyp_dump_info->init_reserve_start = base; + phyp_dump_info->init_reserve_size = size; + } +} +#else +static inline void __init reserve_crashed_mem(void) {} +#endif /* CONFIG_PHYP_DUMP && CONFIG_PPC_RTAS */
cheers -- Michael Ellerman OzLabs, IBM Australia Development Lab wwweb: http://michael.ellerman.id.au phone: +61 2 6212 1183 (tie line 70 21183) We do not inherit the earth from our ancestors, we borrow it from our children. - S.M.A.R.T Person
Attachments
- signature.asc [application/pgp-signature] 189 bytes