Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
Manish Ahuja writes: If Mike and Paul are okay, then I will leave this bit as is and fix all other issues and comments. Well, part of the problem is the semantic dissonance caused by having a static variable called global. Please change the name phyp_dump_global to phyp_dump_vars or something similar - that will only affect two lines of code and will reduce the ugliness a bit. Paul. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
On 10/03/2008, Michael Ellerman [EMAIL PROTECTED] wrote: On Thu, 2008-02-28 at 18:24 -0600, Manish Ahuja wrote: + +/* 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. I did this. This is a style used to minimize disruption due to future design changes. Basically, the idea is that, at some later time, for some unknown reason, we decide that this structure shouldn't be global, or maybe shouldn't be statically allocated, or maybe should be per-cpu, or who knows. By creating a pointer, and just passing that around, you isolate other code from this change. I learned this trick after spending too many months of my life hunting down globals and replacing them by dynamically allocated structs. Its a long and painful process, on many levels, often requiring major code restructuring. Code that touches globals directly is often poorly thought out, designed. But going in the opposite direction is easy: if your code always passes everything it needs as args to subroutines, then you are free clear ... if one of those args just happens to be a pointer to a global, there's no loss (not even a performance loss -- the arg passing overhead is about the same as a global TOC lookup!) So it may look weird if you're not used to seeing it; but the alternative is almost always worse. --linas ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
If Mike and Paul are okay, then I will leave this bit as is and fix all other issues and comments. Thanks, Manish Linas Vepstas wrote: On 10/03/2008, Michael Ellerman [EMAIL PROTECTED] wrote: On Thu, 2008-02-28 at 18:24 -0600, Manish Ahuja wrote: + +/* 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. I did this. This is a style used to minimize disruption due to future design changes. Basically, the idea is that, at some later time, for some unknown reason, we decide that this structure shouldn't be global, or maybe shouldn't be statically allocated, or maybe should be per-cpu, or who knows. By creating a pointer, and just passing that around, you isolate other code from this change. I learned this trick after spending too many months of my life hunting down globals and replacing them by dynamically allocated structs. Its a long and painful process, on many levels, often requiring major code restructuring. Code that touches globals directly is often poorly thought out, designed. But going in the opposite direction is easy: if your code always passes everything it needs as args to subroutines, then you are free clear ... if one of those args just happens to be a pointer to a global, there's no loss (not even a performance loss -- the arg passing overhead is about the same as a global TOC lookup!) So it may look weird if you're not used to seeing it; but the alternative is almost always worse. --linas ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
Manish Ahuja writes:
+#else /* CONFIG_PHYP_DUMP */
+int early_init_dt_scan_phyp_dump(unsigned long node,
+ const char *uname, int depth, void *data) { return 0; }
This shouldn't be in the header file. Either put it in prom.c (and
make it return 1 so the of_scan_flat_dt call doesn't have to go
through the entire device tree), or put #ifdef CONFIG_PHYP_DUMP around
the of_scan_flat_dt call itself.
+/* 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;
It's a little weird to have a static variable with global in its name.
+int __init early_init_dt_scan_phyp_dump(unsigned long node,
+ const char *uname, int depth, void *data)
+{
+#ifdef CONFIG_PHYP_DUMP
This is in phyp_dump.c, which only gets compiled if CONFIG_PHYP_DUMP
is set, so you don't need this ifdef.
Paul.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
On Tue, 2008-03-11 at 17:12 +1100, Paul Mackerras wrote:
Manish Ahuja writes:
+#else /* CONFIG_PHYP_DUMP */
+int early_init_dt_scan_phyp_dump(unsigned long node,
+ const char *uname, int depth, void *data) { return 0; }
This shouldn't be in the header file. Either put it in prom.c (and
make it return 1 so the of_scan_flat_dt call doesn't have to go
through the entire device tree), or put #ifdef CONFIG_PHYP_DUMP around
the of_scan_flat_dt call itself.
It should be in the header file, otherwise we need an #ifdef around the
call site - which is uglier.
It should definitely return 1 though.
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
signature.asc
Description: This is a digitally signed message part
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
On Wed, 2008-03-12 at 11:13 +1100, Michael Ellerman wrote:
On Tue, 2008-03-11 at 17:12 +1100, Paul Mackerras wrote:
Manish Ahuja writes:
+#else /* CONFIG_PHYP_DUMP */
+int early_init_dt_scan_phyp_dump(unsigned long node,
+ const char *uname, int depth, void *data) { return 0; }
This shouldn't be in the header file. Either put it in prom.c (and
make it return 1 so the of_scan_flat_dt call doesn't have to go
through the entire device tree), or put #ifdef CONFIG_PHYP_DUMP around
the of_scan_flat_dt call itself.
It should be in the header file, otherwise we need an #ifdef around the
call site - which is uglier.
Right I'm an idiot. It is called via a function pointer, so a static
inline (which this should be, but isn't) is no good. An #ifdef around
the call site is probably the least ugly option given that otherwise we
have to have an empty version in the binary.
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
signature.asc
Description: This is a digitally signed message part
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
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 [EMAIL PROTECTED]
Signed-off-by: Linas Vepstas [EMAIL PROTECTED]
Hi Manish,
A few comments inline ..
Index: 2.6.25-rc1/arch/powerpc/platforms/pseries/phyp_dump.c
===
--- /dev/null 1970-01-01 00:00:00.0 +
+++ 2.6.25-rc1/arch/powerpc/platforms/pseries/phyp_dump.c 2008-02-28
21:57:52.0 -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.
+
+/**
+ * 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.
Index: 2.6.25-rc1/arch/powerpc/kernel/prom.c
===
--- 2.6.25-rc1.orig/arch/powerpc/kernel/prom.c2008-02-28
21:54:57.0 -0600
+++ 2.6.25-rc1/arch/powerpc/kernel/prom.c 2008-02-28 21:55:27.0
-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
Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
On Thu, Feb 14, 2008 at 02:46:21PM +1100, Tony Breeds wrote: Hi Manish, Sorry for the minor nits but this should be: --- * Linas Vepstas, Manish Ahuja 2008 * Copyright 2008 IBM Corp. --- You can optionally use the '??' symbol after word 'Copyright' but you shouldn't use '(c)' anymore. Also in at least one place you've misspelt Copyright If we're going to nitpick, then I'd like to point out that the whole series needs to be run through checkpatch and at least the whitespace issues should be taken care of. I'm still not convinced that this is a useful feature compared to hardening kdump, especially now that ehea can handle kexec/kdump (patch posted the other day). But in the end it's up to Paul if he wants to take it or not, not me. -Olof ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
Olof, I will run it through checkpatch before resubmitting. Thanks, Manish Olof Johansson wrote: On Thu, Feb 14, 2008 at 02:46:21PM +1100, Tony Breeds wrote: Hi Manish, Sorry for the minor nits but this should be: --- * Linas Vepstas, Manish Ahuja 2008 * Copyright 2008 IBM Corp. --- You can optionally use the '??' symbol after word 'Copyright' but you shouldn't use '(c)' anymore. Also in at least one place you've misspelt Copyright If we're going to nitpick, then I'd like to point out that the whole series needs to be run through checkpatch and at least the whitespace issues should be taken care of. I'm still not convinced that this is a useful feature compared to hardening kdump, especially now that ehea can handle kexec/kdump (patch posted the other day). But in the end it's up to Paul if he wants to take it or not, not me. -Olof ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
On Tue, Feb 12, 2008 at 01:08:25AM -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 [EMAIL PROTECTED] Signed-off-by: Linas Vepstas [EMAIL PROTECTED] arch/powerpc/kernel/prom.c | 50 arch/powerpc/kernel/rtas.c | 32 + arch/powerpc/platforms/pseries/Makefile|1 arch/powerpc/platforms/pseries/phyp_dump.c | 71 + include/asm-powerpc/phyp_dump.h| 38 +++ include/asm/rtas.h |3 + 6 files changed, 195 insertions(+) Index: 2.6.24-rc5/include/asm-powerpc/phyp_dump.h === --- /dev/null 1970-01-01 00:00:00.0 + +++ 2.6.24-rc5/include/asm-powerpc/phyp_dump.h2008-02-12 06:12:37.0 -0600 @@ -0,0 +1,38 @@ +/* + * Hypervisor-assisted dump + * + * Linas Vepstas, Manish Ahuja 2007 + * Copyright (c) 2007 IBM Corp. Hi Manish, Sorry for the minor nits but this should be: --- * Linas Vepstas, Manish Ahuja 2008 * Copyright 2008 IBM Corp. --- You can optionally use the '©' symbol after word 'Copyright' but you shouldn't use '(c)' anymore. Also in at least one place you've misspelt Copyright Yours Tony linux.conf.auhttp://linux.conf.au/ || http://lca2008.linux.org.au/ Jan 28 - Feb 02 2008 The Australian Linux Technical Conference! ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
On Tue, 2008-02-12 at 01:08 -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 [EMAIL PROTECTED] Signed-off-by: Linas Vepstas [EMAIL PROTECTED] arch/powerpc/kernel/prom.c | 50 arch/powerpc/kernel/rtas.c | 32 + arch/powerpc/platforms/pseries/Makefile|1 arch/powerpc/platforms/pseries/phyp_dump.c | 71 + include/asm-powerpc/phyp_dump.h| 38 +++ include/asm/rtas.h |3 + ^^ asm/rtas.h doesn't exist. You need to clean your tree, and patch asm-powerpc/rtas.h instead. 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 signature.asc Description: This is a digitally signed message part ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
Sorry,
I think i sent the wrong patch file, it shouldn't have my printk statement in
there. Let me re-send
the correct file and let me test it once more to make sure it does the right
thing.
-Manish
Paul Mackerras wrote:
Manish Ahuja writes:
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.
[snip]
+static void __init reserve_crashed_mem(void)
+{
+unsigned long base, size;
+
+if (phyp_dump_info-phyp_dump_is_active) {
+/* Reserve *everything* above RMR. We'll free this real soon.*/
+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;
+printk(KERN_ERR Manish reserve regular kernel space is %ld %ld\n,
base, size);
+lmb_reserve(base, size);
This is still reserving memory even on systems that aren't running on
pHyp at all. Please rework this so that no memory is reserved if the
system doesn't support phyp-assisted dump.
Paul.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
Manish Ahuja writes:
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.
[snip]
+static void __init reserve_crashed_mem(void)
+{
+ unsigned long base, size;
+
+ if (phyp_dump_info-phyp_dump_is_active) {
+ /* Reserve *everything* above RMR. We'll free this real soon.*/
+ 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;
+ printk(KERN_ERR Manish reserve regular kernel space is %ld %ld\n,
base, size);
+ lmb_reserve(base, size);
This is still reserving memory even on systems that aren't running on
pHyp at all. Please rework this so that no memory is reserved if the
system doesn't support phyp-assisted dump.
Paul.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
Reposted this one. I got the email id wrong in this one. Sorry about that. Manish ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
