The following series of patches implement a basic framework
for hypervisor-assisted dump. The very first patch provides
documentation explaining what this is :-) . Yes, its supposed
to be an improvement over kdump.
A list of open issues / todo list is included in the documentation.
It also appears that the not-yet-released firmware versions this was tested
on are still, ahem, incomplete; this work is also pending.
I have included most of the changes requested. Although, I did find
one or two, fixed in a later patch file rather than the first location
they appeared at.
Also it now does not block any memory on machines other than power6 boxes
which have the requisite firmware. This is from a power5 box.
from jal-lp6 a power5 machine.
.........
Phyp-dump not supported on this hardware
Using pSeries machine description
console [udbg-1] enabled
.......
I think I incorporated everyones comments so far.
-- Manish & Linas.
@@ -0,0 +1,127 @@++ Hypervisor-Assisted Dump+ ------------------------+ November 2007++The goal of hypervisor-assisted dump is to enable the dump of+a crashed system, and to do so from a fully-reset system, and+to minimize the total elapsed time until the system is back+in production use.++As compared to kdump or other strategies, hypervisor-assisted+dump offers several strong, practical advantages:++-- Unlike kdump, the system has been reset, and loaded+ with a fresh copy of the kernel. In particular,+ PCI and I/O devices have been reinitialized and are+ in a clean, consistent state.+-- As the dump is performed, the dumped memory becomes+ immediately available to the system for normal use.+-- After the dump is completed, no further reboots are+ required; the system will be fully usable, and running+ in it's normal, production mode on it normal kernel.++The above can only be accomplished by coordination with,+and assistance from the hypervisor. The procedure is+as follows:++-- When a system crashes, the hypervisor will save+ the low 256MB of RAM to a previously registered+ save region. It will also save system state, system+ registers, and hardware PTE's.++-- After the low 256MB area has been saved, the+ hypervisor will reset PCI and other hardware state.+ It will *not* clear RAM. It will then launch the+ bootloader, as normal.++-- The freshly booted kernel will notice that there+ is a new node (ibm,dump-kernel) in the device tree,+ indicating that there is crash data available from+ a previous boot. It will boot into only 256MB of RAM,+ reserving the rest of system memory.++-- Userspace tools will parse /sys/kernel/release_region+ and read /proc/vmcore to obtain the contents of memory,+ which holds the previous crashed kernel. The userspace+ tools may copy this info to disk, or network, nas, san,+ iscsi, etc. as desired.++ For Example: the values in /sys/kernel/release-region+ would look something like this (address-range pairs).+ CPU:0x177fee000-0x10000: HPTE:0x177ffe020-0x1000: /+ DUMP:0x177fff020-0x10000000, 0x10000000-0x16F1D370A++-- As the userspace tools complete saving a portion of+ dump, they echo an offset and size to+ /sys/kernel/release_region to release the reserved+ memory back to general use.++ An example of this is:+ "echo 0x40000000 0x10000000 > /sys/kernel/release_region"+ which will release 256MB at the 1GB boundary.++Please note that the hypervisor-assisted dump feature+is only available on Power6-based systems with recent+firmware versions.++Implementation details:+----------------------++During boot, a check is made to see if firmware supports+this feature on this particular machine. If it does, then+we check to see if a active dump is waiting for us. If yes+then everything but 256 MB of RAM is reserved during early+boot. This area is released once we collect a dump from user+land scripts that are run. If there is dump data, then+the /sys/kernel/release_region file is created, and+the reserved memory is held.++If there is no waiting dump data, then only the highest+256MB of the ram is reserved as a scratch area. This area+is *not* be released: this region will be kept permanently+reserved, so that it can act as a receptacle for a copy+of the low 256MB in the case a crash does occur. See,+however, "open issues" below, as to whether+such a reserved region is really needed.++Currently the dump will be copied from /proc/vmcore to a+a new file upon user intervention. The starting address+to be read and the range for each data point in provided+in /sys/kernel/release_region.++The tools to examine the dump will be same as the ones+used for kdump.++General notes:+--------------+Security: please note that there are potential security issues+with any sort of dump mechanism. In particular, plaintext+(unencrypted) data, and possibly passwords, may be present in+the dump data. Userspace tools must take adequate precautions to+preserve security.++Open issues/ToDo:+------------+ o The various code paths that tell the hypervisor that a crash+ occurred, vs. it simply being a normal reboot, should be+ reviewed, and possibly clarified/fixed.++ o Instead of using /sys/kernel, should there be a /sys/dump+ instead? There is a dump_subsys being created by the s390 code,+ perhaps the pseries code should use a similar layout as well.++ o Is reserving a 256MB region really required? The goal of+ reserving a 256MB scratch area is to make sure that no+ important crash data is clobbered when the hypervisor+ save low mem to the scratch area. But, if one could assure+ that nothing important is located in some 256MB area, then+ it would not need to be reserved. Something that can be+ improved in subsequent versions.++ o Still working the kdump team to integrate this with kdump,+ some work remains but this would not affect the current+ patches.++ o Still need to write a shell script, to copy the dump away.+ Currently I am parsing it manually.
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>
----
arch/powerpc/kernel/prom.c | 49 ++++++++++++++++++++
arch/powerpc/kernel/rtas.c | 34 +++++++++++++
arch/powerpc/platforms/pseries/Makefile | 1
arch/powerpc/platforms/pseries/phyp_dump.c | 71 +++++++++++++++++++++++++++++
include/asm-powerpc/phyp_dump.h | 38 +++++++++++++++
include/asm-powerpc/rtas.h | 3 +
6 files changed, 196 insertions(+)
Index: 2.6.25-rc1/include/asm-powerpc/phyp_dump.h
===================================================================
@@ -0,0 +1,38 @@+/*+*Hypervisor-assisteddump+*+*LinasVepstas,ManishAhuja2008+*Copyright2008IBMCorp.+*+*Thisprogramisfreesoftware;youcanredistributeitand/or+*modifyitunderthetermsoftheGNUGeneralPublicLicense+*aspublishedbytheFreeSoftwareFoundation;eitherversion+*2oftheLicense,or(atyouroption)anylaterversion.+*/++#ifndef _PPC64_PHYP_DUMP_H+#define _PPC64_PHYP_DUMP_H++#ifdef CONFIG_PHYP_DUMP++/* The RMR region will be saved for later dumping+*wheneverthekernelcrashes.Setthisto256MB.*/+#define PHYP_DUMP_RMR_START 0x0+#define PHYP_DUMP_RMR_END (1UL<<28)++structphyp_dump{+/* Memory that is reserved during very early boot. */+unsignedlonginit_reserve_start;+unsignedlonginit_reserve_size;+/* Check status during boot if dump supported, active & present*/+unsignedlongphyp_dump_configured;+unsignedlongphyp_dump_is_active;+/* store cpu & hpte size */+unsignedlongcpu_state_size;+unsignedlonghpte_region_size;+};++externstructphyp_dump*phyp_dump_info;++#endif /* CONFIG_PHYP_DUMP */+#endif /* _PPC64_PHYP_DUMP_H */
@@ -0,0 +1,71 @@+/*+*Hypervisor-assisteddump+*+*LinasVepstas,ManishAhuja2008+*Copyright2008IBMCorp.+*+*Thisprogramisfreesoftware;youcanredistributeitand/or+*modifyitunderthetermsoftheGNUGeneralPublicLicense+*aspublishedbytheFreeSoftwareFoundation;eitherversion+*2oftheLicense,or(atyouroption)anylaterversion.+*+*/++#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>++/* Global, used to communicate data between early boot and late boot */+staticstructphyp_dumpphyp_dump_global;+structphyp_dump*phyp_dump_info=&phyp_dump_global;++/**+*release_memory_range--releasememorypreviouslylmb_reserved+*@start_pfn:startingphysicalframenumber+*@nr_pages:numberofpagestofree.+*+*Thisroutinewillreleasememorythathadbeenpreviously+*lmb_reservedinearlyboot.Thereleasedmemorybecomes+*availableforgenrealuse.+*/+staticvoid+release_memory_range(unsignedlongstart_pfn,unsignedlongnr_pages)+{+structpage*rpage;+unsignedlongend_pfn;+longi;++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++;+}+}+}++staticint__initphyp_dump_setup(void)+{+unsignedlongstart_pfn,nr_pages;++/* If no memory was reserved in early boot, there is nothing to do */+if(phyp_dump_info->init_reserve_size==0)+return0;++/* 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);++return0;+}+machine_subsys_initcall(pseries,phyp_dump_setup);
@@ -1039,6 +1040,51 @@ static void __init early_reserve_mem(voi#endif}+#ifdef CONFIG_PHYP_DUMP+/**+*reserve_crashed_mem()-reserveallnot-yet-dumpedmmemory+*+*Thisroutinemayreservememoryregionsinthekernelonly+*ifthesystemissupportedandadumpwastakeninlast+*bootinstanceorifthehardwareissupportedandthe+*scratchareaneedstobesetup.Inotherinstancesitreturns+*withoutreservinganything.Thememoryincaseofdumpbeing+*activeisfreedwhenthedumpiscollected(byuserlandtools).+*/+staticvoid__initreserve_crashed_mem(void)+{+unsignedlongbase,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+*thememory_limit,itwillneedtobeadjusted.*/+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+staticinlinevoid__initreserve_crashed_mem(void){}+#endif /* CONFIG_PHYP_DUMP */++void__initearly_init_devtree(void*params){DBG(" -> early_init_devtree(%p)\n",params);
@@ -1050,6 +1096,8 @@ void __init early_init_devtree(void *par/* Some machines might need RTAS info for debugging, grab it now. */of_scan_flat_dt(early_init_dt_scan_rtas,NULL);#endif+/* scan tree to see if dump occured during last boot */+of_scan_flat_dt(early_init_dt_scan_phyp_dump,NULL);/* Retrieve various informations from the /chosen node of the*device-tree,includingtheplatformtype,initrdlocationand
Check to see if there actually is data from a previously
crashed kernel waiting. If so, Allow user-sapce tools to
grab the data (by reading /proc/kcore). When user-space
finishes dumping a section, it must release that memory
by writing to sysfs. For example,
echo "0x40000000 0x10000000" > /sys/kernel/release_region
will release 256MB starting at the 1GB. The released memory
becomes free for general use.
Signed-off-by: Linas Vepstas <linasvepstas@gmail.com>
Signed-off-by: Manish Ahuja <redacted>
------
arch/powerpc/platforms/pseries/phyp_dump.c | 81 +++++++++++++++++++++++++++--
1 file changed, 76 insertions(+), 5 deletions(-)
Index: 2.6.25-rc1/arch/powerpc/platforms/pseries/phyp_dump.c
===================================================================
@@ -12,18 +12,23 @@*/#include<linux/init.h>+#include<linux/kobject.h>#include<linux/mm.h>+#include<linux/of.h>#include<linux/pfn.h>#include<linux/swap.h>+#include<linux/sysfs.h>#include<asm/page.h>#include<asm/phyp_dump.h>#include<asm/machdep.h>+#include<asm/rtas.h>/* Global, used to communicate data between early boot and late boot */staticstructphyp_dumpphyp_dump_global;structphyp_dump*phyp_dump_info=&phyp_dump_global;+/* ------------------------------------------------- *//***release_memory_range--releasememorypreviouslylmb_reserved*@start_pfn:startingphysicalframenumber
@@ -53,18 +58,84 @@ release_memory_range(unsigned long start}}-staticint__initphyp_dump_setup(void)+/* ------------------------------------------------- */+/**+*sysfs_release_region--sysfsinterfacetoreleasememoryrange.+*+*Usage:+*"echo <start addr> <length> > /sys/kernel/release_region"+*+*Example:+*"echo 0x40000000 0x10000000 > /sys/kernel/release_region"+*+*willrelease256MBstartingat1GB.+*/+staticssize_tstore_release_region(structkobject*kobj,+structkobj_attribute*attr,+constchar*buf,size_tcount){+unsignedlongstart_addr,length,end_addr;unsignedlongstart_pfn,nr_pages;+ssize_tret;++ret=sscanf(buf,"%lx %lx",&start_addr,&length);+if(ret!=2)+return-EINVAL;++/* Range-check - don't free any reserved memory that+*wasn'treservedforphyp-dump*/+if(start_addr<phyp_dump_info->init_reserve_start)+start_addr=phyp_dump_info->init_reserve_start;++end_addr=phyp_dump_info->init_reserve_start++phyp_dump_info->init_reserve_size;+if(start_addr+length>end_addr)+length=end_addr-start_addr;++/* Release the region of memory assed in by user */+start_pfn=PFN_DOWN(start_addr);+nr_pages=PFN_DOWN(length);+release_memory_range(start_pfn,nr_pages);++returncount;+}++staticstructkobj_attributerr=__ATTR(release_region,0600,+NULL,store_release_region);++staticint__initphyp_dump_setup(void)+{+structdevice_node*rtas;+constint*dump_header=NULL;+intheader_len=0;+intrc;/* If no memory was reserved in early boot, there is nothing to do */if(phyp_dump_info->init_reserve_size==0)return0;-/* 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 if phyp dump not supported */+if(!phyp_dump_info->phyp_dump_configured)+return-ENOSYS;++/* Is there dump data waiting for us? */+rtas=of_find_node_by_path("/rtas");+if(rtas){+dump_header=of_get_property(rtas,"ibm,kernel-dump",+&header_len);+of_node_put(rtas);+}++if(dump_header==NULL)+return0;++/* Should we create a dump_subsys, analogous to s390/ipl.c ? */+rc=sysfs_create_file(kernel_kobj,&rr.attr);+if(rc){+printk(KERN_ERR"phyp-dump: unable to create sysfs file (%d)\n",+rc);+return0;+}return0;}
Set up the actual dump header, register it with the hypervisor.
Signed-off-by: Manish Ahuja <redacted>
Signed-off-by: Linas Vepstas <linasvepstas@gmail.com>
------
arch/powerpc/platforms/pseries/phyp_dump.c | 137 +++++++++++++++++++++++++++--
1 file changed, 131 insertions(+), 6 deletions(-)
Index: 2.6.25-rc1/arch/powerpc/platforms/pseries/phyp_dump.c
===================================================================
@@ -28,6 +28,117 @@staticstructphyp_dumpphyp_dump_global;structphyp_dump*phyp_dump_info=&phyp_dump_global;+staticintibm_configure_kernel_dump;+/* ------------------------------------------------- */+/* RTAS interfaces to declare the dump regions */++structdump_section{+u32dump_flags;+u16source_type;+u16error_flags;+u64source_address;+u64source_length;+u64length_copied;+u64destination_address;+};++structphyp_dump_header{+u32version;+u16num_of_sections;+u16status;++u32first_offset_section;+u32dump_disk_section;+u64block_num_dd;+u64num_of_blocks_dd;+u32offset_dd;+u32maxtime_to_auto;+/* No dump disk path string used */++structdump_sectioncpu_data;+structdump_sectionhpte_data;+structdump_sectionkernel_data;+};++/* The dump header *must be* in low memory, so .bss it */+staticstructphyp_dump_headerphdr;++#define NUM_DUMP_SECTIONS 3+#define DUMP_HEADER_VERSION 0x1+#define DUMP_REQUEST_FLAG 0x1+#define DUMP_SOURCE_CPU 0x0001+#define DUMP_SOURCE_HPTE 0x0002+#define DUMP_SOURCE_RMO 0x0011++/**+*init_dump_header()-initializetheheaderdeclaringadump+*Returns:lengthofdumpsavearea.+*+*Whenthehypervisorsavescrashedstate,itneedstoput+*itsomewhere.Thedumpheadertellsthehypervisorwhere+*thedatacanbesaved.+*/+staticunsignedlonginit_dump_header(structphyp_dump_header*ph)+{+unsignedlongaddr_offset=0;++/* Set up the dump header */+ph->version=DUMP_HEADER_VERSION;+ph->num_of_sections=NUM_DUMP_SECTIONS;+ph->status=0;++ph->first_offset_section=+(u32)offsetof(structphyp_dump_header,cpu_data);+ph->dump_disk_section=0;+ph->block_num_dd=0;+ph->num_of_blocks_dd=0;+ph->offset_dd=0;++ph->maxtime_to_auto=0;/* disabled */++/* The first two sections are mandatory */+ph->cpu_data.dump_flags=DUMP_REQUEST_FLAG;+ph->cpu_data.source_type=DUMP_SOURCE_CPU;+ph->cpu_data.source_address=0;+ph->cpu_data.source_length=phyp_dump_info->cpu_state_size;+ph->cpu_data.destination_address=addr_offset;+addr_offset+=phyp_dump_info->cpu_state_size;++ph->hpte_data.dump_flags=DUMP_REQUEST_FLAG;+ph->hpte_data.source_type=DUMP_SOURCE_HPTE;+ph->hpte_data.source_address=0;+ph->hpte_data.source_length=phyp_dump_info->hpte_region_size;+ph->hpte_data.destination_address=addr_offset;+addr_offset+=phyp_dump_info->hpte_region_size;++/* This section describes the low kernel region */+ph->kernel_data.dump_flags=DUMP_REQUEST_FLAG;+ph->kernel_data.source_type=DUMP_SOURCE_RMO;+ph->kernel_data.source_address=PHYP_DUMP_RMR_START;+ph->kernel_data.source_length=PHYP_DUMP_RMR_END;+ph->kernel_data.destination_address=addr_offset;+addr_offset+=ph->kernel_data.source_length;++returnaddr_offset;+}++staticvoidregister_dump_area(structphyp_dump_header*ph,unsignedlongaddr)+{+intrc;+ph->cpu_data.destination_address+=addr;+ph->hpte_data.destination_address+=addr;+ph->kernel_data.destination_address+=addr;++do{+rc=rtas_call(ibm_configure_kernel_dump,3,1,NULL,+1,ph,sizeof(structphyp_dump_header));+}while(rtas_busy_delay(rc));++if(rc)+printk(KERN_ERR"phyp-dump: unexpected error (%d) on "+"register\n",rc);+}+/* ------------------------------------------------- *//***release_memory_range--releasememorypreviouslylmb_reserved
@@ -118,7 +231,13 @@ static int __init phyp_dump_setup(void)if(!phyp_dump_info->phyp_dump_configured)return-ENOSYS;-/* Is there dump data waiting for us? */+/* Is there dump data waiting for us? If there isn't,+*thenregisteranewdumparea,andreleaseallof+*therestofthereservedram.+*+*The/rtas/ibm,kernel-dumprtasnodeispresentonly+*ifthereisdumpdatawaitingforus.+*/rtas=of_find_node_by_path("/rtas");if(rtas){dump_header=of_get_property(rtas,"ibm,kernel-dump",
@@ -126,17 +245,23 @@ static int __init phyp_dump_setup(void)of_node_put(rtas);}-if(dump_header==NULL)+dump_area_length=init_dump_header(&phdr);++/* align down */+dump_area_start=phyp_dump_info->init_reserve_start&PAGE_MASK;++if(dump_header==NULL){+register_dump_area(&phdr,dump_area_start);return0;+}/* Should we create a dump_subsys, analogous to s390/ipl.c ? */rc=sysfs_create_file(kernel_kobj,&rr.attr);-if(rc){+if(rc)printk(KERN_ERR"phyp-dump: unable to create sysfs file (%d)\n",rc);-return0;-}+/* ToDo: re-register the dump area, for next time. */return0;}machine_subsys_initcall(pseries,phyp_dump_setup);
@@ -122,6 +122,61 @@ static unsigned long init_dump_header(streturnaddr_offset;}+staticvoidprint_dump_header(conststructphyp_dump_header*ph)+{+#ifdef DEBUG+printk(KERN_INFO"dump header:\n");+/* setup some ph->sections required */+printk(KERN_INFO"version = %d\n",ph->version);+printk(KERN_INFO"Sections = %d\n",ph->num_of_sections);+printk(KERN_INFO"Status = 0x%x\n",ph->status);++/* No ph->disk, so all should be set to 0 */+printk(KERN_INFO"Offset to first section 0x%x\n",+ph->first_offset_section);+printk(KERN_INFO"dump disk sections should be zero\n");+printk(KERN_INFO"dump disk section = %d\n",ph->dump_disk_section);+printk(KERN_INFO"block num = %ld\n",ph->block_num_dd);+printk(KERN_INFO"number of blocks = %ld\n",ph->num_of_blocks_dd);+printk(KERN_INFO"dump disk offset = %d\n",ph->offset_dd);+printk(KERN_INFO"Max auto time= %d\n",ph->maxtime_to_auto);++/*set cpu state and hpte states as well scratch pad area */+printk(KERN_INFO" CPU AREA \n");+printk(KERN_INFO"cpu dump_flags =%d\n",ph->cpu_data.dump_flags);+printk(KERN_INFO"cpu source_type =%d\n",ph->cpu_data.source_type);+printk(KERN_INFO"cpu error_flags =%d\n",ph->cpu_data.error_flags);+printk(KERN_INFO"cpu source_address =%lx\n",+ph->cpu_data.source_address);+printk(KERN_INFO"cpu source_length =%lx\n",+ph->cpu_data.source_length);+printk(KERN_INFO"cpu length_copied =%lx\n",+ph->cpu_data.length_copied);++printk(KERN_INFO" HPTE AREA \n");+printk(KERN_INFO"HPTE dump_flags =%d\n",ph->hpte_data.dump_flags);+printk(KERN_INFO"HPTE source_type =%d\n",ph->hpte_data.source_type);+printk(KERN_INFO"HPTE error_flags =%d\n",ph->hpte_data.error_flags);+printk(KERN_INFO"HPTE source_address =%lx\n",+ph->hpte_data.source_address);+printk(KERN_INFO"HPTE source_length =%lx\n",+ph->hpte_data.source_length);+printk(KERN_INFO"HPTE length_copied =%lx\n",+ph->hpte_data.length_copied);++printk(KERN_INFO" SRSD AREA \n");+printk(KERN_INFO"SRSD dump_flags =%d\n",ph->kernel_data.dump_flags);+printk(KERN_INFO"SRSD source_type =%d\n",ph->kernel_data.source_type);+printk(KERN_INFO"SRSD error_flags =%d\n",ph->kernel_data.error_flags);+printk(KERN_INFO"SRSD source_address =%lx\n",+ph->kernel_data.source_address);+printk(KERN_INFO"SRSD source_length =%lx\n",+ph->kernel_data.source_length);+printk(KERN_INFO"SRSD length_copied =%lx\n",+ph->kernel_data.length_copied);+#endif+}+staticvoidregister_dump_area(structphyp_dump_header*ph,unsignedlongaddr){intrc;
Routines to
a. invalidate dump
b. Calculate region that is reserved and needs to be freed. This is
exported through sysfs interface.
Unregister has been removed for now as it wasn't being used.
Signed-off-by: Manish Ahuja <redacted>
-----
---
arch/powerpc/platforms/pseries/phyp_dump.c | 83 ++++++++++++++++++++++++++---
include/asm-powerpc/phyp_dump.h | 3 +
2 files changed, 80 insertions(+), 6 deletions(-)
Index: 2.6.25-rc1/arch/powerpc/platforms/pseries/phyp_dump.c
===================================================================
@@ -180,9 +184,15 @@ static void print_dump_header(const strustaticvoidregister_dump_area(structphyp_dump_header*ph,unsignedlongaddr){intrc;-ph->cpu_data.destination_address+=addr;-ph->hpte_data.destination_address+=addr;-ph->kernel_data.destination_address+=addr;++/* Add addr value if not initialized before */+if(ph->cpu_data.destination_address==0){+ph->cpu_data.destination_address+=addr;+ph->hpte_data.destination_address+=addr;+ph->kernel_data.destination_address+=addr;+}++/* ToDo Invalidate kdump and free memory range. */do{rc=rtas_call(ibm_configure_kernel_dump,3,1,NULL,
@@ -196,6 +206,30 @@ static void register_dump_area(struct ph}}+static+voidinvalidate_last_dump(structphyp_dump_header*ph,unsignedlongaddr)+{+intrc;++/* Add addr value if not initialized before */+if(ph->cpu_data.destination_address==0){+ph->cpu_data.destination_address+=addr;+ph->hpte_data.destination_address+=addr;+ph->kernel_data.destination_address+=addr;+}++do{+rc=rtas_call(ibm_configure_kernel_dump,3,1,NULL,+2,ph,sizeof(structphyp_dump_header));+}while(rtas_busy_delay(rc));++if(rc){+printk(KERN_ERR"phyp-dump: unexpected error (%d) "+"on invalidate\n",rc);+print_dump_header(ph);+}+}+/* ------------------------------------------------- *//***release_memory_range--releasememorypreviouslylmb_reserved
@@ -268,8 +302,29 @@ static ssize_t store_release_region(strureturncount;}+staticssize_tshow_release_region(structkobject*kobj,+structkobj_attribute*attr,char*buf)+{+u64second_addr_range;++/* total reserved size - start of scratch area */+second_addr_range=phyp_dump_info->init_reserve_size-+phyp_dump_info->reserved_scratch_size;+returnsprintf(buf,"CPU:0x%lx-0x%lx: HPTE:0x%lx-0x%lx:"+" DUMP:0x%lx-0x%lx, 0x%lx-0x%lx:\n",+phdr.cpu_data.destination_address,+phdr.cpu_data.length_copied,+phdr.hpte_data.destination_address,+phdr.hpte_data.length_copied,+phdr.kernel_data.destination_address,+phdr.kernel_data.length_copied,+phyp_dump_info->init_reserve_start,+second_addr_range);+}+staticstructkobj_attributerr=__ATTR(release_region,0600,-NULL,store_release_region);+show_release_region,+store_release_region);staticint__initphyp_dump_setup(void){
@@ -312,6 +367,22 @@ static int __init phyp_dump_setup(void)return0;}+/* re-register the dump area, if old dump was invalid */+if((dump_header)&&(dump_header->status&DUMP_ERROR_FLAG)){+invalidate_last_dump(&phdr,dump_area_start);+register_dump_area(&phdr,dump_area_start);+return0;+}++if(dump_header){+phyp_dump_info->reserved_scratch_addr=+dump_header->cpu_data.destination_address;+phyp_dump_info->reserved_scratch_size=+dump_header->cpu_data.source_length++dump_header->hpte_data.source_length++dump_header->kernel_data.source_length;+}+/* Should we create a dump_subsys, analogous to s390/ipl.c ? */rc=sysfs_create_file(kernel_kobj,&rr.attr);if(rc)
This patch tracks the size freed. For now it does a simple
rudimentary calculation of the ranges freed. The idea is
to keep it simple at the external shell script level and
send in large chunks for now.
Signed-off-by: Manish Ahuja <redacted>
-----
---
arch/powerpc/platforms/pseries/phyp_dump.c | 35 +++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
Index: 2.6.25-rc1/arch/powerpc/platforms/pseries/phyp_dump.c
===================================================================
From: Michael Ellerman <hidden> Date: 2008-02-22 00:53:04
On Sun, 2008-02-17 at 22:53 -0600, Manish Ahuja wrote:
The following series of patches implement a basic framework
for hypervisor-assisted dump. The very first patch provides
documentation explaining what this is :-) . Yes, its supposed
to be an improvement over kdump.
A list of open issues / todo list is included in the documentation.
It also appears that the not-yet-released firmware versions this was tested
on are still, ahem, incomplete; this work is also pending.
I have included most of the changes requested. Although, I did find
one or two, fixed in a later patch file rather than the first location
they appeared at.
This series still doesn't build on !CONFIG_RTAS configs:
http://kisskb.ellerman.id.au/kisskb/head/629/
This solution is to move early_init_dt_scan_phyp_dump() into
arch/powerpc/platforms/pseries/phyp_dump.c and provide a dummy
implementation in asm-powerpc/phyp_dump.c for the !CONFIG_PHYP_DUMP
case.
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
Changes from previous version:
The only changes are in patch 2.
moved early_init_dt_scan_phyp_dump from rtas.c to phyp_dump.c
Added dummy function in phyp_dump.h
Patch 3 required repatching due to changes to patch 2.
Resubmitting all patches to avoid confusion.
Thanks,
Manish
Michael Ellerman wrote:
On Sun, 2008-02-17 at 22:53 -0600, Manish Ahuja wrote:
quoted
The following series of patches implement a basic framework
for hypervisor-assisted dump. The very first patch provides
documentation explaining what this is :-) . Yes, its supposed
to be an improvement over kdump.
A list of open issues / todo list is included in the documentation.
It also appears that the not-yet-released firmware versions this was tested
on are still, ahem, incomplete; this work is also pending.
I have included most of the changes requested. Although, I did find
one or two, fixed in a later patch file rather than the first location
they appeared at.
This series still doesn't build on !CONFIG_RTAS configs:
http://kisskb.ellerman.id.au/kisskb/head/629/
This solution is to move early_init_dt_scan_phyp_dump() into
arch/powerpc/platforms/pseries/phyp_dump.c and provide a dummy
implementation in asm-powerpc/phyp_dump.c for the !CONFIG_PHYP_DUMP
case.
cheers
@@ -0,0 +1,127 @@++ Hypervisor-Assisted Dump+ ------------------------+ November 2007++The goal of hypervisor-assisted dump is to enable the dump of+a crashed system, and to do so from a fully-reset system, and+to minimize the total elapsed time until the system is back+in production use.++As compared to kdump or other strategies, hypervisor-assisted+dump offers several strong, practical advantages:++-- Unlike kdump, the system has been reset, and loaded+ with a fresh copy of the kernel. In particular,+ PCI and I/O devices have been reinitialized and are+ in a clean, consistent state.+-- As the dump is performed, the dumped memory becomes+ immediately available to the system for normal use.+-- After the dump is completed, no further reboots are+ required; the system will be fully usable, and running+ in it's normal, production mode on it normal kernel.++The above can only be accomplished by coordination with,+and assistance from the hypervisor. The procedure is+as follows:++-- When a system crashes, the hypervisor will save+ the low 256MB of RAM to a previously registered+ save region. It will also save system state, system+ registers, and hardware PTE's.++-- After the low 256MB area has been saved, the+ hypervisor will reset PCI and other hardware state.+ It will *not* clear RAM. It will then launch the+ bootloader, as normal.++-- The freshly booted kernel will notice that there+ is a new node (ibm,dump-kernel) in the device tree,+ indicating that there is crash data available from+ a previous boot. It will boot into only 256MB of RAM,+ reserving the rest of system memory.++-- Userspace tools will parse /sys/kernel/release_region+ and read /proc/vmcore to obtain the contents of memory,+ which holds the previous crashed kernel. The userspace+ tools may copy this info to disk, or network, nas, san,+ iscsi, etc. as desired.++ For Example: the values in /sys/kernel/release-region+ would look something like this (address-range pairs).+ CPU:0x177fee000-0x10000: HPTE:0x177ffe020-0x1000: /+ DUMP:0x177fff020-0x10000000, 0x10000000-0x16F1D370A++-- As the userspace tools complete saving a portion of+ dump, they echo an offset and size to+ /sys/kernel/release_region to release the reserved+ memory back to general use.++ An example of this is:+ "echo 0x40000000 0x10000000 > /sys/kernel/release_region"+ which will release 256MB at the 1GB boundary.++Please note that the hypervisor-assisted dump feature+is only available on Power6-based systems with recent+firmware versions.++Implementation details:+----------------------++During boot, a check is made to see if firmware supports+this feature on this particular machine. If it does, then+we check to see if a active dump is waiting for us. If yes+then everything but 256 MB of RAM is reserved during early+boot. This area is released once we collect a dump from user+land scripts that are run. If there is dump data, then+the /sys/kernel/release_region file is created, and+the reserved memory is held.++If there is no waiting dump data, then only the highest+256MB of the ram is reserved as a scratch area. This area+is *not* be released: this region will be kept permanently+reserved, so that it can act as a receptacle for a copy+of the low 256MB in the case a crash does occur. See,+however, "open issues" below, as to whether+such a reserved region is really needed.++Currently the dump will be copied from /proc/vmcore to a+a new file upon user intervention. The starting address+to be read and the range for each data point in provided+in /sys/kernel/release_region.++The tools to examine the dump will be same as the ones+used for kdump.++General notes:+--------------+Security: please note that there are potential security issues+with any sort of dump mechanism. In particular, plaintext+(unencrypted) data, and possibly passwords, may be present in+the dump data. Userspace tools must take adequate precautions to+preserve security.++Open issues/ToDo:+------------+ o The various code paths that tell the hypervisor that a crash+ occurred, vs. it simply being a normal reboot, should be+ reviewed, and possibly clarified/fixed.++ o Instead of using /sys/kernel, should there be a /sys/dump+ instead? There is a dump_subsys being created by the s390 code,+ perhaps the pseries code should use a similar layout as well.++ o Is reserving a 256MB region really required? The goal of+ reserving a 256MB scratch area is to make sure that no+ important crash data is clobbered when the hypervisor+ save low mem to the scratch area. But, if one could assure+ that nothing important is located in some 256MB area, then+ it would not need to be reserved. Something that can be+ improved in subsequent versions.++ o Still working the kdump team to integrate this with kdump,+ some work remains but this would not affect the current+ patches.++ o Still need to write a shell script, to copy the dump away.+ Currently I am parsing it manually.
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>
----
arch/powerpc/kernel/prom.c | 49 +++++++++++++
arch/powerpc/platforms/pseries/Makefile | 1
arch/powerpc/platforms/pseries/phyp_dump.c | 105 +++++++++++++++++++++++++++++
include/asm-powerpc/phyp_dump.h | 44 ++++++++++++
4 files changed, 199 insertions(+)
Index: 2.6.25-rc1/include/asm-powerpc/phyp_dump.h
===================================================================
@@ -0,0 +1,44 @@+/*+*Hypervisor-assisteddump+*+*LinasVepstas,ManishAhuja2008+*Copyright2008IBMCorp.+*+*Thisprogramisfreesoftware;youcanredistributeitand/or+*modifyitunderthetermsoftheGNUGeneralPublicLicense+*aspublishedbytheFreeSoftwareFoundation;eitherversion+*2oftheLicense,or(atyouroption)anylaterversion.+*/++#ifndef _PPC64_PHYP_DUMP_H+#define _PPC64_PHYP_DUMP_H++#ifdef CONFIG_PHYP_DUMP++/* The RMR region will be saved for later dumping+*wheneverthekernelcrashes.Setthisto256MB.*/+#define PHYP_DUMP_RMR_START 0x0+#define PHYP_DUMP_RMR_END (1UL<<28)++structphyp_dump{+/* Memory that is reserved during very early boot. */+unsignedlonginit_reserve_start;+unsignedlonginit_reserve_size;+/* Check status during boot if dump supported, active & present*/+unsignedlongphyp_dump_configured;+unsignedlongphyp_dump_is_active;+/* store cpu & hpte size */+unsignedlongcpu_state_size;+unsignedlonghpte_region_size;+};++externstructphyp_dump*phyp_dump_info;++intearly_init_dt_scan_phyp_dump(unsignedlongnode,+constchar*uname,intdepth,void*data);+#else /* CONFIG_PHYP_DUMP */+intearly_init_dt_scan_phyp_dump(unsignedlongnode,+constchar*uname,intdepth,void*data){return0;}++#endif /* CONFIG_PHYP_DUMP */+#endif /* _PPC64_PHYP_DUMP_H */
@@ -0,0 +1,105 @@+/*+*Hypervisor-assisteddump+*+*LinasVepstas,ManishAhuja2008+*Copyright2008IBMCorp.+*+*Thisprogramisfreesoftware;youcanredistributeitand/or+*modifyitunderthetermsoftheGNUGeneralPublicLicense+*aspublishedbytheFreeSoftwareFoundation;eitherversion+*2oftheLicense,or(atyouroption)anylaterversion.+*+*/++#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 */+staticstructphyp_dumpphyp_dump_global;+structphyp_dump*phyp_dump_info=&phyp_dump_global;++/**+*release_memory_range--releasememorypreviouslylmb_reserved+*@start_pfn:startingphysicalframenumber+*@nr_pages:numberofpagestofree.+*+*Thisroutinewillreleasememorythathadbeenpreviously+*lmb_reservedinearlyboot.Thereleasedmemorybecomes+*availableforgenrealuse.+*/+staticvoid+release_memory_range(unsignedlongstart_pfn,unsignedlongnr_pages)+{+structpage*rpage;+unsignedlongend_pfn;+longi;++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++;+}+}+}++staticint__initphyp_dump_setup(void)+{+unsignedlongstart_pfn,nr_pages;++/* If no memory was reserved in early boot, there is nothing to do */+if(phyp_dump_info->init_reserve_size==0)+return0;++/* 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);++return0;+}+machine_subsys_initcall(pseries,phyp_dump_setup);++int__initearly_init_dt_scan_phyp_dump(unsignedlongnode,+constchar*uname,intdepth,void*data)+{+#ifdef CONFIG_PHYP_DUMP+constunsignedint*sizes;++phyp_dump_info->phyp_dump_configured=0;+phyp_dump_info->phyp_dump_is_active=0;++if(depth!=1||strcmp(uname,"rtas")!=0)+return0;++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)+return0;++if(sizes[0]==1)+phyp_dump_info->cpu_state_size=*((unsignedlong*)&sizes[1]);++if(sizes[3]==2)+phyp_dump_info->hpte_region_size=+*((unsignedlong*)&sizes[4]);+#endif+return1;+}
@@ -1039,6 +1040,51 @@ static void __init early_reserve_mem(voi#endif}+#ifdef CONFIG_PHYP_DUMP+/**+*reserve_crashed_mem()-reserveallnot-yet-dumpedmmemory+*+*Thisroutinemayreservememoryregionsinthekernelonly+*ifthesystemissupportedandadumpwastakeninlast+*bootinstanceorifthehardwareissupportedandthe+*scratchareaneedstobesetup.Inotherinstancesitreturns+*withoutreservinganything.Thememoryincaseofdumpbeing+*activeisfreedwhenthedumpiscollected(byuserlandtools).+*/+staticvoid__initreserve_crashed_mem(void)+{+unsignedlongbase,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+*thememory_limit,itwillneedtobeadjusted.*/+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+staticinlinevoid__initreserve_crashed_mem(void){}+#endif /* CONFIG_PHYP_DUMP && CONFIG_PPC_RTAS */++void__initearly_init_devtree(void*params){DBG(" -> early_init_devtree(%p)\n",params);
@@ -1050,6 +1096,8 @@ void __init early_init_devtree(void *par/* Some machines might need RTAS info for debugging, grab it now. */of_scan_flat_dt(early_init_dt_scan_rtas,NULL);#endif+/* scan tree to see if dump occured during last boot */+of_scan_flat_dt(early_init_dt_scan_phyp_dump,NULL);/* Retrieve various informations from the /chosen node of the*device-tree,includingtheplatformtype,initrdlocationand
Check to see if there actually is data from a previously
crashed kernel waiting. If so, Allow user-sapce tools to
grab the data (by reading /proc/kcore). When user-space
finishes dumping a section, it must release that memory
by writing to sysfs. For example,
echo "0x40000000 0x10000000" > /sys/kernel/release_region
will release 256MB starting at the 1GB. The released memory
becomes free for general use.
Signed-off-by: Linas Vepstas <linasvepstas@gmail.com>
Signed-off-by: Manish Ahuja <redacted>
------
arch/powerpc/platforms/pseries/phyp_dump.c | 82 +++++++++++++++++++++++++++--
1 file changed, 77 insertions(+), 5 deletions(-)
Index: 2.6.25-rc1/arch/powerpc/platforms/pseries/phyp_dump.c
===================================================================
@@ -12,19 +12,25 @@*/#include<linux/init.h>+#include<linux/kobject.h>#include<linux/mm.h>+#include<linux/of.h>#include<linux/pfn.h>#include<linux/swap.h>+#include<linux/sysfs.h>#include<asm/page.h>#include<asm/phyp_dump.h>#include<asm/machdep.h>#include<asm/prom.h>+#include<asm/rtas.h>+/* Global, used to communicate data between early boot and late boot */staticstructphyp_dumpphyp_dump_global;structphyp_dump*phyp_dump_info=&phyp_dump_global;+/* ------------------------------------------------- *//***release_memory_range--releasememorypreviouslylmb_reserved*@start_pfn:startingphysicalframenumber
@@ -54,18 +60,84 @@ release_memory_range(unsigned long start}}-staticint__initphyp_dump_setup(void)+/* ------------------------------------------------- */+/**+*sysfs_release_region--sysfsinterfacetoreleasememoryrange.+*+*Usage:+*"echo <start addr> <length> > /sys/kernel/release_region"+*+*Example:+*"echo 0x40000000 0x10000000 > /sys/kernel/release_region"+*+*willrelease256MBstartingat1GB.+*/+staticssize_tstore_release_region(structkobject*kobj,+structkobj_attribute*attr,+constchar*buf,size_tcount){+unsignedlongstart_addr,length,end_addr;unsignedlongstart_pfn,nr_pages;+ssize_tret;++ret=sscanf(buf,"%lx %lx",&start_addr,&length);+if(ret!=2)+return-EINVAL;++/* Range-check - don't free any reserved memory that+*wasn'treservedforphyp-dump*/+if(start_addr<phyp_dump_info->init_reserve_start)+start_addr=phyp_dump_info->init_reserve_start;++end_addr=phyp_dump_info->init_reserve_start++phyp_dump_info->init_reserve_size;+if(start_addr+length>end_addr)+length=end_addr-start_addr;++/* Release the region of memory assed in by user */+start_pfn=PFN_DOWN(start_addr);+nr_pages=PFN_DOWN(length);+release_memory_range(start_pfn,nr_pages);++returncount;+}++staticstructkobj_attributerr=__ATTR(release_region,0600,+NULL,store_release_region);++staticint__initphyp_dump_setup(void)+{+structdevice_node*rtas;+constint*dump_header=NULL;+intheader_len=0;+intrc;/* If no memory was reserved in early boot, there is nothing to do */if(phyp_dump_info->init_reserve_size==0)return0;-/* 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 if phyp dump not supported */+if(!phyp_dump_info->phyp_dump_configured)+return-ENOSYS;++/* Is there dump data waiting for us? */+rtas=of_find_node_by_path("/rtas");+if(rtas){+dump_header=of_get_property(rtas,"ibm,kernel-dump",+&header_len);+of_node_put(rtas);+}++if(dump_header==NULL)+return0;++/* Should we create a dump_subsys, analogous to s390/ipl.c ? */+rc=sysfs_create_file(kernel_kobj,&rr.attr);+if(rc){+printk(KERN_ERR"phyp-dump: unable to create sysfs file (%d)\n",+rc);+return0;+}return0;}
Set up the actual dump header, register it with the hypervisor.
Signed-off-by: Manish Ahuja <redacted>
Signed-off-by: Linas Vepstas <linasvepstas@gmail.com>
------
arch/powerpc/platforms/pseries/phyp_dump.c | 137 +++++++++++++++++++++++++++--
1 file changed, 131 insertions(+), 6 deletions(-)
Index: 2.6.25-rc1/arch/powerpc/platforms/pseries/phyp_dump.c
===================================================================
@@ -30,6 +30,117 @@staticstructphyp_dumpphyp_dump_global;structphyp_dump*phyp_dump_info=&phyp_dump_global;+staticintibm_configure_kernel_dump;+/* ------------------------------------------------- */+/* RTAS interfaces to declare the dump regions */++structdump_section{+u32dump_flags;+u16source_type;+u16error_flags;+u64source_address;+u64source_length;+u64length_copied;+u64destination_address;+};++structphyp_dump_header{+u32version;+u16num_of_sections;+u16status;++u32first_offset_section;+u32dump_disk_section;+u64block_num_dd;+u64num_of_blocks_dd;+u32offset_dd;+u32maxtime_to_auto;+/* No dump disk path string used */++structdump_sectioncpu_data;+structdump_sectionhpte_data;+structdump_sectionkernel_data;+};++/* The dump header *must be* in low memory, so .bss it */+staticstructphyp_dump_headerphdr;++#define NUM_DUMP_SECTIONS 3+#define DUMP_HEADER_VERSION 0x1+#define DUMP_REQUEST_FLAG 0x1+#define DUMP_SOURCE_CPU 0x0001+#define DUMP_SOURCE_HPTE 0x0002+#define DUMP_SOURCE_RMO 0x0011++/**+*init_dump_header()-initializetheheaderdeclaringadump+*Returns:lengthofdumpsavearea.+*+*Whenthehypervisorsavescrashedstate,itneedstoput+*itsomewhere.Thedumpheadertellsthehypervisorwhere+*thedatacanbesaved.+*/+staticunsignedlonginit_dump_header(structphyp_dump_header*ph)+{+unsignedlongaddr_offset=0;++/* Set up the dump header */+ph->version=DUMP_HEADER_VERSION;+ph->num_of_sections=NUM_DUMP_SECTIONS;+ph->status=0;++ph->first_offset_section=+(u32)offsetof(structphyp_dump_header,cpu_data);+ph->dump_disk_section=0;+ph->block_num_dd=0;+ph->num_of_blocks_dd=0;+ph->offset_dd=0;++ph->maxtime_to_auto=0;/* disabled */++/* The first two sections are mandatory */+ph->cpu_data.dump_flags=DUMP_REQUEST_FLAG;+ph->cpu_data.source_type=DUMP_SOURCE_CPU;+ph->cpu_data.source_address=0;+ph->cpu_data.source_length=phyp_dump_info->cpu_state_size;+ph->cpu_data.destination_address=addr_offset;+addr_offset+=phyp_dump_info->cpu_state_size;++ph->hpte_data.dump_flags=DUMP_REQUEST_FLAG;+ph->hpte_data.source_type=DUMP_SOURCE_HPTE;+ph->hpte_data.source_address=0;+ph->hpte_data.source_length=phyp_dump_info->hpte_region_size;+ph->hpte_data.destination_address=addr_offset;+addr_offset+=phyp_dump_info->hpte_region_size;++/* This section describes the low kernel region */+ph->kernel_data.dump_flags=DUMP_REQUEST_FLAG;+ph->kernel_data.source_type=DUMP_SOURCE_RMO;+ph->kernel_data.source_address=PHYP_DUMP_RMR_START;+ph->kernel_data.source_length=PHYP_DUMP_RMR_END;+ph->kernel_data.destination_address=addr_offset;+addr_offset+=ph->kernel_data.source_length;++returnaddr_offset;+}++staticvoidregister_dump_area(structphyp_dump_header*ph,unsignedlongaddr)+{+intrc;+ph->cpu_data.destination_address+=addr;+ph->hpte_data.destination_address+=addr;+ph->kernel_data.destination_address+=addr;++do{+rc=rtas_call(ibm_configure_kernel_dump,3,1,NULL,+1,ph,sizeof(structphyp_dump_header));+}while(rtas_busy_delay(rc));++if(rc)+printk(KERN_ERR"phyp-dump: unexpected error (%d) on "+"register\n",rc);+}+/* ------------------------------------------------- *//***release_memory_range--releasememorypreviouslylmb_reserved
@@ -120,7 +233,13 @@ static int __init phyp_dump_setup(void)if(!phyp_dump_info->phyp_dump_configured)return-ENOSYS;-/* Is there dump data waiting for us? */+/* Is there dump data waiting for us? If there isn't,+*thenregisteranewdumparea,andreleaseallof+*therestofthereservedram.+*+*The/rtas/ibm,kernel-dumprtasnodeispresentonly+*ifthereisdumpdatawaitingforus.+*/rtas=of_find_node_by_path("/rtas");if(rtas){dump_header=of_get_property(rtas,"ibm,kernel-dump",
@@ -128,17 +247,23 @@ static int __init phyp_dump_setup(void)of_node_put(rtas);}-if(dump_header==NULL)+dump_area_length=init_dump_header(&phdr);++/* align down */+dump_area_start=phyp_dump_info->init_reserve_start&PAGE_MASK;++if(dump_header==NULL){+register_dump_area(&phdr,dump_area_start);return0;+}/* Should we create a dump_subsys, analogous to s390/ipl.c ? */rc=sysfs_create_file(kernel_kobj,&rr.attr);-if(rc){+if(rc)printk(KERN_ERR"phyp-dump: unable to create sysfs file (%d)\n",rc);-return0;-}+/* ToDo: re-register the dump area, for next time. */return0;}machine_subsys_initcall(pseries,phyp_dump_setup);
@@ -124,6 +124,61 @@ static unsigned long init_dump_header(streturnaddr_offset;}+staticvoidprint_dump_header(conststructphyp_dump_header*ph)+{+#ifdef DEBUG+printk(KERN_INFO"dump header:\n");+/* setup some ph->sections required */+printk(KERN_INFO"version = %d\n",ph->version);+printk(KERN_INFO"Sections = %d\n",ph->num_of_sections);+printk(KERN_INFO"Status = 0x%x\n",ph->status);++/* No ph->disk, so all should be set to 0 */+printk(KERN_INFO"Offset to first section 0x%x\n",+ph->first_offset_section);+printk(KERN_INFO"dump disk sections should be zero\n");+printk(KERN_INFO"dump disk section = %d\n",ph->dump_disk_section);+printk(KERN_INFO"block num = %ld\n",ph->block_num_dd);+printk(KERN_INFO"number of blocks = %ld\n",ph->num_of_blocks_dd);+printk(KERN_INFO"dump disk offset = %d\n",ph->offset_dd);+printk(KERN_INFO"Max auto time= %d\n",ph->maxtime_to_auto);++/*set cpu state and hpte states as well scratch pad area */+printk(KERN_INFO" CPU AREA \n");+printk(KERN_INFO"cpu dump_flags =%d\n",ph->cpu_data.dump_flags);+printk(KERN_INFO"cpu source_type =%d\n",ph->cpu_data.source_type);+printk(KERN_INFO"cpu error_flags =%d\n",ph->cpu_data.error_flags);+printk(KERN_INFO"cpu source_address =%lx\n",+ph->cpu_data.source_address);+printk(KERN_INFO"cpu source_length =%lx\n",+ph->cpu_data.source_length);+printk(KERN_INFO"cpu length_copied =%lx\n",+ph->cpu_data.length_copied);++printk(KERN_INFO" HPTE AREA \n");+printk(KERN_INFO"HPTE dump_flags =%d\n",ph->hpte_data.dump_flags);+printk(KERN_INFO"HPTE source_type =%d\n",ph->hpte_data.source_type);+printk(KERN_INFO"HPTE error_flags =%d\n",ph->hpte_data.error_flags);+printk(KERN_INFO"HPTE source_address =%lx\n",+ph->hpte_data.source_address);+printk(KERN_INFO"HPTE source_length =%lx\n",+ph->hpte_data.source_length);+printk(KERN_INFO"HPTE length_copied =%lx\n",+ph->hpte_data.length_copied);++printk(KERN_INFO" SRSD AREA \n");+printk(KERN_INFO"SRSD dump_flags =%d\n",ph->kernel_data.dump_flags);+printk(KERN_INFO"SRSD source_type =%d\n",ph->kernel_data.source_type);+printk(KERN_INFO"SRSD error_flags =%d\n",ph->kernel_data.error_flags);+printk(KERN_INFO"SRSD source_address =%lx\n",+ph->kernel_data.source_address);+printk(KERN_INFO"SRSD source_length =%lx\n",+ph->kernel_data.source_length);+printk(KERN_INFO"SRSD length_copied =%lx\n",+ph->kernel_data.length_copied);+#endif+}+staticvoidregister_dump_area(structphyp_dump_header*ph,unsignedlongaddr){intrc;
Routines to
a. invalidate dump
b. Calculate region that is reserved and needs to be freed. This is
exported through sysfs interface.
Unregister has been removed for now as it wasn't being used.
Signed-off-by: Manish Ahuja <redacted>
-----
---
arch/powerpc/platforms/pseries/phyp_dump.c | 83 ++++++++++++++++++++++++++---
include/asm-powerpc/phyp_dump.h | 3 +
2 files changed, 80 insertions(+), 6 deletions(-)
Index: 2.6.25-rc1/arch/powerpc/platforms/pseries/phyp_dump.c
===================================================================
@@ -182,9 +186,15 @@ static void print_dump_header(const strustaticvoidregister_dump_area(structphyp_dump_header*ph,unsignedlongaddr){intrc;-ph->cpu_data.destination_address+=addr;-ph->hpte_data.destination_address+=addr;-ph->kernel_data.destination_address+=addr;++/* Add addr value if not initialized before */+if(ph->cpu_data.destination_address==0){+ph->cpu_data.destination_address+=addr;+ph->hpte_data.destination_address+=addr;+ph->kernel_data.destination_address+=addr;+}++/* ToDo Invalidate kdump and free memory range. */do{rc=rtas_call(ibm_configure_kernel_dump,3,1,NULL,
@@ -198,6 +208,30 @@ static void register_dump_area(struct ph}}+static+voidinvalidate_last_dump(structphyp_dump_header*ph,unsignedlongaddr)+{+intrc;++/* Add addr value if not initialized before */+if(ph->cpu_data.destination_address==0){+ph->cpu_data.destination_address+=addr;+ph->hpte_data.destination_address+=addr;+ph->kernel_data.destination_address+=addr;+}++do{+rc=rtas_call(ibm_configure_kernel_dump,3,1,NULL,+2,ph,sizeof(structphyp_dump_header));+}while(rtas_busy_delay(rc));++if(rc){+printk(KERN_ERR"phyp-dump: unexpected error (%d) "+"on invalidate\n",rc);+print_dump_header(ph);+}+}+/* ------------------------------------------------- *//***release_memory_range--releasememorypreviouslylmb_reserved
@@ -270,8 +304,29 @@ static ssize_t store_release_region(strureturncount;}+staticssize_tshow_release_region(structkobject*kobj,+structkobj_attribute*attr,char*buf)+{+u64second_addr_range;++/* total reserved size - start of scratch area */+second_addr_range=phyp_dump_info->init_reserve_size-+phyp_dump_info->reserved_scratch_size;+returnsprintf(buf,"CPU:0x%lx-0x%lx: HPTE:0x%lx-0x%lx:"+" DUMP:0x%lx-0x%lx, 0x%lx-0x%lx:\n",+phdr.cpu_data.destination_address,+phdr.cpu_data.length_copied,+phdr.hpte_data.destination_address,+phdr.hpte_data.length_copied,+phdr.kernel_data.destination_address,+phdr.kernel_data.length_copied,+phyp_dump_info->init_reserve_start,+second_addr_range);+}+staticstructkobj_attributerr=__ATTR(release_region,0600,-NULL,store_release_region);+show_release_region,+store_release_region);staticint__initphyp_dump_setup(void){
@@ -314,6 +369,22 @@ static int __init phyp_dump_setup(void)return0;}+/* re-register the dump area, if old dump was invalid */+if((dump_header)&&(dump_header->status&DUMP_ERROR_FLAG)){+invalidate_last_dump(&phdr,dump_area_start);+register_dump_area(&phdr,dump_area_start);+return0;+}++if(dump_header){+phyp_dump_info->reserved_scratch_addr=+dump_header->cpu_data.destination_address;+phyp_dump_info->reserved_scratch_size=+dump_header->cpu_data.source_length++dump_header->hpte_data.source_length++dump_header->kernel_data.source_length;+}+/* Should we create a dump_subsys, analogous to s390/ipl.c ? */rc=sysfs_create_file(kernel_kobj,&rr.attr);if(rc)
This patch tracks the size freed. For now it does a simple
rudimentary calculation of the ranges freed. The idea is
to keep it simple at the external shell script level and
send in large chunks for now.
Signed-off-by: Manish Ahuja <redacted>
-----
---
arch/powerpc/platforms/pseries/phyp_dump.c | 35 +++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
Index: 2.6.25-rc1/arch/powerpc/platforms/pseries/phyp_dump.c
===================================================================
From: Michael Ellerman <hidden> Date: 2008-02-29 02:20:50
On Thu, 2008-02-28 at 17:57 -0600, Manish Ahuja wrote:
Changes from previous version:
The only changes are in patch 2.
moved early_init_dt_scan_phyp_dump from rtas.c to phyp_dump.c
Added dummy function in phyp_dump.h
This fixes the build failures I was seeing!
http://kisskb.ellerman.id.au/kisskb/head/664/
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
From: Joel Schopp <hidden> Date: 2008-03-03 23:36:55
This looks like it is to a stable usable point now. In my opinion it is
ready to be merged into the next tree for 2.6.26.
Reviewed-by: Joel Schopp <redacted>
Manish Ahuja wrote:
Changes from previous version:
The only changes are in patch 2.
moved early_init_dt_scan_phyp_dump from rtas.c to phyp_dump.c
Added dummy function in phyp_dump.h
Patch 3 required repatching due to changes to patch 2.
Resubmitting all patches to avoid confusion.
Thanks,
Manish
Michael Ellerman wrote:
quoted
On Sun, 2008-02-17 at 22:53 -0600, Manish Ahuja wrote:
quoted
The following series of patches implement a basic framework
for hypervisor-assisted dump. The very first patch provides
documentation explaining what this is :-) . Yes, its supposed
to be an improvement over kdump.
A list of open issues / todo list is included in the documentation.
It also appears that the not-yet-released firmware versions this was tested
on are still, ahem, incomplete; this work is also pending.
I have included most of the changes requested. Although, I did find
one or two, fixed in a later patch file rather than the first location
they appeared at.
This series still doesn't build on !CONFIG_RTAS configs:
http://kisskb.ellerman.id.au/kisskb/head/629/
This solution is to move early_init_dt_scan_phyp_dump() into
arch/powerpc/platforms/pseries/phyp_dump.c and provide a dummy
implementation in asm-powerpc/phyp_dump.c for the !CONFIG_PHYP_DUMP
case.
cheers
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>
@@ -0,0 +1,105 @@+/*+*Hypervisor-assisteddump+*+*LinasVepstas,ManishAhuja2008+*Copyright2008IBMCorp.+*+*Thisprogramisfreesoftware;youcanredistributeitand/or+*modifyitunderthetermsoftheGNUGeneralPublicLicense+*aspublishedbytheFreeSoftwareFoundation;eitherversion+*2oftheLicense,or(atyouroption)anylaterversion.+*+*/++#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 */+staticstructphyp_dumpphyp_dump_global;+structphyp_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.
This could do with a name change IMO, eg. phyp_dump_reserve_mem() or
something.
+{
+ 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
From: Paul Mackerras <hidden> Date: 2008-03-11 06:12:29
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.
From: Paul Mackerras <hidden> Date: 2008-03-11 06:16:17
Manish Ahuja writes:
Check to see if there actually is data from a previously
crashed kernel waiting. If so, Allow user-sapce tools to
grab the data (by reading /proc/kcore). When user-space
finishes dumping a section, it must release that memory
by writing to sysfs. For example,
echo "0x40000000 0x10000000" > /sys/kernel/release_region
will release 256MB starting at the 1GB. The released memory
becomes free for general use.
Signed-off-by: Linas Vepstas <linasvepstas@gmail.com>
Signed-off-by: Manish Ahuja <redacted>
------
This line needs to be exactly 3 dashes, because otherwise the tools
include the diffstat into the commit message. Putting 4 or more
dashes was an annoying habit Linas had, and it means I have to fix it
manually (usually after I have committed the patches, and then notice
that the commit message has the extra stuff in it, so I have to go
back and fix the separators, reset my tree and re-commit the patches.)
This is a somewhat weird-looking way of coping with too-long lines.
Please indent the second line either one more tab than the first line,
or else so that it starts just after the '(' in the first line (which
is what emacs will do by default). The same comment applies in
several other places.
Paul.
I think it would be clearer if you use a tab to line up the values,
like this:
#define NUM_DUMP_SECTIONS 3
#define DUMP_HEADER_VERSION 0x1
#define DUMP_REQUEST_FLAG 0x1
#define DUMP_SOURCE_CPU 0x0001
#define DUMP_SOURCE_HPTE 0x0002
#define DUMP_SOURCE_RMO 0x0011
Paul.
From: Paul Mackerras <hidden> Date: 2008-03-11 06:19:49
Manish Ahuja writes:
-static void
-release_memory_range(unsigned long start_pfn, unsigned long nr_pages)
+static
+void release_memory_range(unsigned long start_pfn, unsigned long nr_pages)
This change looks rather pointless. If you have to change it, I'd
prefer:
static void release_memory_range(unsigned long start_pfn,
unsigned long nr_pages)
Paul.
From: Michael Ellerman <hidden> Date: 2008-03-12 00:13:08
On Tue, 2008-03-11 at 17:12 +1100, Paul Mackerras wrote:
Manish Ahuja writes:
quoted
+#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
From: Michael Ellerman <hidden> Date: 2008-03-12 00:53:33
On Wed, 2008-03-12 at 11:13 +1100, Michael Ellerman wrote:
On Tue, 2008-03-11 at 17:12 +1100, Paul Mackerras wrote:
quoted
Manish Ahuja writes:
quoted
+#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
On 11/03/2008, Paul Mackerras [off-list ref] wrote:
> ------
This line needs to be exactly 3 dashes, because otherwise the tools
include the diffstat into the commit message. Putting 4 or more
dashes was an annoying habit Linas had, and it means I have to fix it
manually (usually after I have committed the patches, and then notice
that the commit message has the extra stuff in it, so I have to go
back and fix the separators, reset my tree and re-commit the patches.)
Sorry, I had no idea! If I didn't have enough dashes, then quilt would
sometimes wipe out the comment at the top, so paranoia made me
add lots of dashes.
--linas
On 10/03/2008, Michael Ellerman [off-list ref] 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
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 [off-list ref] wrote:
quoted
On Thu, 2008-02-28 at 18:24 -0600, Manish Ahuja wrote:
quoted
> +
> +/* 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
From: Michael Ellerman <hidden> Date: 2008-03-14 04:20:46
On Wed, 2008-03-12 at 23:29 -0500, Manish Ahuja wrote:
If Mike and Paul are okay, then I will leave this bit as is and fix all
other issues and comments.
Well I still don't like it - it uglifies the code _now_, for a potential
future benefit that may never come. But I don't care that much, if
Paul's happy with it let it go in.
cheers
Linas Vepstas wrote:
quoted
On 10/03/2008, Michael Ellerman [off-list ref] wrote:
quoted
On Thu, 2008-02-28 at 18:24 -0600, Manish Ahuja wrote:
quoted
> +
> +/* 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.
--
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
From: Paul Mackerras <hidden> Date: 2008-03-14 05:19:19
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.