powerpc: Use the #address-cells information to parse memory/reg

Use the #address-cells, #size-cells information to parse the memory/reg info from device tree.

The format of memory/reg is based on the #address-cells,#size-cells. Currently, the kexec-tools doesn't use the above values in parsing the memory/reg values. Hence the kexec cannot handle cases where #address-cells, #size-cells are different, (for e.g, PPC440X ).

This patch introduces a read_memory_region_limits(), which parses the memory/reg contents based on the values of #address-cells and #size-cells.

Changed the add_usable_mem_property() to accept FILE* fp instead of int fd, as most of the other users of read_memory_region_limits() deals with FILE*.

Signed-off-by Suzuki K. Poulose <[email protected]>

---
kexec/arch/ppc/crashdump-powerpc.c | 23 ------ kexec/arch/ppc/fs2dt.c | 31 ++------ kexec/arch/ppc/kexec-ppc.c | 136 ++++++++++++++++++++++++++----------- kexec/arch/ppc/kexec-ppc.h | 6 +

4 files changed, 118 insertions(+), 78 deletions(-)

Index: kexec-tools-2.0.4/kexec/arch/ppc/kexec-ppc.c
===================================================================

kexec/arch/ppc/kexec-ppc.c +99 -37

@@ -26,6 +26,7 @@
 
 #include "config.h"
 
+unsigned long dt_address_cells = 0, dt_size_cells = 0;
 uint64_t rmo_top;
 unsigned long long crash_base = 0, crash_size = 0;
 unsigned long long initrd_base = 0, initrd_size = 0;

@@ -34,6 +35,92 @@ unsigned int rtas_base, rtas_size;
 int max_memory_ranges;
 const char *ramdisk;
 
+/*
+ * Reads the #address-cells and #size-cells on this platform.
+ * This is used to parse the memory/reg info from the device-tree
+ */
+int init_memory_region_info()
+{
+ size_t res = 0;
+ FILE *fp;
+ char *file;
+
+ file = "/proc/device-tree/#address-cells";
+ fp = fopen(file, "r");
+ if (!fp) {
+ fprintf(stderr,"Unable to open %s\n", file);
+ return -1;
+ }
+
+ res = fread(&dt_address_cells,sizeof(unsigned long),1,fp);
+ if (res != 1) {
+ fprintf(stderr,"Error reading %s\n", file);
+ return -1;
+ }
+ fclose(fp);
+ dt_address_cells *= sizeof(unsigned long);
+
+ file = "/proc/device-tree/#size-cells";
+ fp = fopen(file, "r");
+ if (!fp) {
+ fprintf(stderr,"Unable to open %s\n", file);
+ return -1;
+ }
+
+ res = fread(&dt_size_cells,sizeof(unsigned long),1,fp);
+ if (res != 1) {
+ fprintf(stderr,"Error reading %s\n", file);
+ return -1;
+ }
+ fclose(fp);
+ dt_size_cells *= sizeof(unsigned long);
+
+ return 0;
+}
+
+#define MAXBYTES 128
+/*
+ * Reads the memory region info from the device-tree node pointed
+ * by @fp and fills the *start, *end with the boundaries of the region
+ */
+int read_memory_region_limits(FILE* fp, unsigned long long *start,
+ unsigned long long *end)
+{
+ char buf[MAXBYTES];
+ unsigned long *p;
+ unsigned long nbytes = dt_address_cells + dt_size_cells;
+
+ if (fread(buf, 1, MAXBYTES, fp) != nbytes) {
+ fprintf(stderr, "Error reading the memory region info\n");
+ return -1;
+ }
+
+ p = (unsigned long*)buf;
+ if (dt_address_cells == sizeof(unsigned long)) {
+ *start = p[0];
+ p++;
+ } else if (dt_address_cells == sizeof(unsigned long long)) {
+ *start = ((unsigned long long *)p)[0];
+ p = (unsigned long long *)p + 1;
+ } else {
+ fprintf(stderr,"Unsupported value for #address-cells : %ld\n",
+ dt_address_cells);
+ return -1;
+ }
+
+ if (dt_size_cells == sizeof(unsigned long))
+ *end = *start + p[0];
+ else if (dt_size_cells == sizeof(unsigned long long))
+ *end = *start + ((unsigned long long *)p)[0];
+ else {
+ fprintf(stderr,"Unsupported value for #size-cells : %ld\n",
+ dt_size_cells);
+ return -1;
+ }
+
+ return 0;
+}
+
 void arch_reuse_initrd(void)
 {
 reuse_initrd = 1;

@@ -182,9 +269,6 @@ static int sort_base_ranges(void)
 return 0;
 }
 
-
-#define MAXBYTES 128
-
 static int realloc_memory_ranges(void)
 {
 size_t memory_range_len;

@@ -248,6 +332,8 @@ static int get_base_ranges(void)
 return -1;
 }
 while ((mentry = readdir(dmem)) != NULL) {
+ unsigned long long start, end;
+
 if (strcmp(mentry->d_name, "reg"))
 continue;
 strcat(fname, "/reg");

@@ -257,8 +343,7 @@ static int get_base_ranges(void)
 closedir(dir);
 return -1;
 }
- if ((n = fread(buf, 1, MAXBYTES, file)) < 0) {
- perror(fname);
+ if (read_memory_region_limits(file, &start, &end) != 0) {
 fclose(file);
 closedir(dmem);
 closedir(dir);

@@ -271,24 +356,8 @@ static int get_base_ranges(void)
 }
 }
 
- if (n == sizeof(uint32_t) * 2) {
- base_memory_range[local_memory_ranges].start =
- ((uint32_t *)buf)[0];
- base_memory_range[local_memory_ranges].end =
- base_memory_range[local_memory_ranges].start +
- ((uint32_t *)buf)[1];
- }
- else if (n == sizeof(uint64_t) * 2) {
- base_memory_range[local_memory_ranges].start =
- ((uint64_t *)buf)[0];
- base_memory_range[local_memory_ranges].end =
- base_memory_range[local_memory_ranges].start +
- ((uint64_t *)buf)[1];
- }
- else {
- fprintf(stderr, "Mem node has invalid size: %d\n", n);
- return -1;
- }
+ base_memory_range[local_memory_ranges].start = start;
+ base_memory_range[local_memory_ranges].end = end;
 base_memory_range[local_memory_ranges].type = RANGE_RAM;
 local_memory_ranges++;
 dbgprintf("%016llx-%016llx : %x\n",

@@ -577,20 +646,10 @@ static int get_devtree_details(unsigned
 perror(fname);
 goto error_opencdir;
 }
- if ((n = fread(buf, 1, MAXBYTES, file)) < 0) {
- perror(fname);
- goto error_openfile;
- }
- if (n == sizeof(uint64_t)) {
- rmo_base = ((uint32_t *)buf)[0];
- rmo_top = rmo_base + ((uint32_t *)buf)[1];
- } else if (n == 16) {
- rmo_base = ((uint64_t *)buf)[0];
- rmo_top = rmo_base + ((uint64_t *)buf)[1];
- } else {
- fprintf(stderr, "Mem node has invalid size: %d\n", n);
+ if (read_memory_region_limits(file, &rmo_base, &rmo_top) != 0) {
 goto error_openfile;
 }
+
 if (rmo_top > 0x30000000UL)
 rmo_top = 0x30000000UL;
 

@@ -664,7 +723,6 @@ error_opendir:
 return -1;
 }
 
-
 /* Setup a sorted list of memory ranges. */
 static int setup_memory_ranges(unsigned long kexec_flags)
 {

@@ -756,7 +814,6 @@ out:
 return -1;
 }
 
-
 /* Return a list of valid memory ranges */
 int get_memory_ranges_dt(struct memory_range **range, int *ranges,
 unsigned long kexec_flags)

@@ -778,6 +835,11 @@ int get_memory_ranges_dt(struct memory_r
 int get_memory_ranges(struct memory_range **range, int *ranges,
 unsigned long kexec_flags)
 {
+ int res = 0;
+
+ res = init_memory_region_info();
+ if (res != 0)
+ return res;
 #ifdef WITH_GAMECUBE
 return get_memory_ranges_gc(range, ranges, kexec_flags);
 #else

Index: kexec-tools-2.0.4/kexec/arch/ppc/kexec-ppc.h
===================================================================

kexec/arch/ppc/kexec-ppc.h +6 -0

@@ -69,6 +69,12 @@ extern unsigned long long initrd_base, i
 extern unsigned long long ramdisk_base, ramdisk_size;
 extern unsigned char reuse_initrd;
 extern const char *ramdisk;
+
+/* Method to parse the memory/reg nodes in device-tree */
+extern unsigned long dt_address_cells, dt_size_cells;
+extern int init_memory_region_info(void);
+extern int read_memory_region_limits(FILE *fp, unsigned long long *start,
+ unsigned long long *end);
 #define COMMAND_LINE_SIZE 512 /* from kernel */
 /*fs2dt*/
 void reserve(unsigned long long where, unsigned long long length);

Index: kexec-tools-2.0.4/kexec/arch/ppc/crashdump-powerpc.c
===================================================================

kexec/arch/ppc/crashdump-powerpc.c +2 -21

@@ -130,9 +130,8 @@ static int get_crash_memory_ranges(struc
 closedir(dir);
 goto err;
 }
- n = fread(buf, 1, MAXBYTES, file);
- if (n < 0) {
- perror(fname);
+ n = read_memory_region_limits(file, &start, &end);
+ if (n != 0) {
 fclose(file);
 closedir(dmem);
 closedir(dir);

@@ -146,24 +145,6 @@ static int get_crash_memory_ranges(struc
 goto err;
 }
 
- /*
- * FIXME: This code fails on platforms that
- * have more than one memory range specified
- * in the device-tree's /memory/reg property.
- * or where the #address-cells and #size-cells
- * are not identical.
- *
- * We should interpret the /memory/reg property
- * based on the values of the #address-cells and
- * #size-cells properites.
- */
- if (n == (sizeof(unsigned long) * 2)) {
- start = ((unsigned long *)buf)[0];
- end = start + ((unsigned long *)buf)[1];
- } else {
- start = ((unsigned long long *)buf)[0];
- end = start + ((unsigned long long *)buf)[1];
- }
 if (start == 0 && end >= (BACKUP_SRC_END + 1))
 start = BACKUP_SRC_END + 1;
 

Index: kexec-tools-2.0.4/kexec/arch/ppc/fs2dt.c
===================================================================

kexec/arch/ppc/fs2dt.c +11 -20

@@ -122,7 +122,7 @@ static unsigned propnum(const char *name
 return offset;
 }
 
-static void add_usable_mem_property(int fd, int len)
+static void add_usable_mem_property(FILE* fp, int len)
 {
 char fname[MAXPATH], *bname;
 unsigned long buf[2];

@@ -137,21 +137,11 @@ static void add_usable_mem_property(int
 if (strncmp(bname, "/memory@", 8) && strcmp(bname, "/memory"))
 return;
 
- if (len < 2 * sizeof(unsigned long))
- die("unrecoverable error: not enough data for mem property\n");
- len = 2 * sizeof(unsigned long);
-
- if (lseek(fd, 0, SEEK_SET) < 0)
+ if (fseek(fp, 0, SEEK_SET) < 0)
 die("unrecoverable error: error seeking in \"%s\": %s\n",
 pathname, strerror(errno));
- if (read(fd, buf, len) != len)
- die("unrecoverable error: error reading \"%s\": %s\n",
- pathname, strerror(errno));
-
- if (~0ULL - buf[0] < buf[1])
- die("unrecoverable error: mem property overflow\n");
- base = buf[0];
- end = base + buf[1];
+ if (read_memory_region_limits(fp, &base, &end) != 0)
+ die("unrecoverable error: error parsing memory/reg limits\n");
 
 for (range = 0; range < usablemem_rgns.size; range++) {
 loc_base = usablemem_rgns.ranges[range].start;

@@ -194,8 +184,9 @@ static void add_usable_mem_property(int
 static void putprops(char *fn, struct dirent **nlist, int numlist)
 {
 struct dirent *dp;
- int i = 0, fd, len;
+ int i = 0, len;
 struct stat statbuf;
+ FILE *fp;
 
 for (i = 0; i < numlist; i++) {
 dp = nlist[i];

@@ -243,12 +234,12 @@ static void putprops(char *fn, struct di
 *dt++ = len;
 *dt++ = propnum(fn);
 
- fd = open(pathname, O_RDONLY);
- if (fd == -1)
+ fp = fopen(pathname, "r");
+ if (fp == NULL)
 die("unrecoverable error: could not open \"%s\": %s\n",
 pathname, strerror(errno));
 
- if (read(fd, dt, len) != len)
+ if (fread(dt, 1, len, fp) != len)
 die("unrecoverable error: could not read \"%s\": %s\n",
 pathname, strerror(errno));
 

@@ -290,8 +281,8 @@ static void putprops(char *fn, struct di
 
 dt += (len + 3)/4;
 if (!strcmp(dp->d_name, "reg") && usablemem_rgns.size)
- add_usable_mem_property(fd, len);
- close(fd);
+ add_usable_mem_property(fp, len);
+ fclose(fp);
 }
 
 fn[0] = '\0';

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Could you please let me know your thoughts/comments about this patch ?

Thanks
Suzuki

Quoted 25 lines

Index: kexec-tools-2.0.4/kexec/arch/ppc/kexec-ppc.c
===================================================================

kexec/arch/ppc/kexec-ppc.c +16 -0

@@ -34,6 +35,92 @@ unsigned int rtas_base, rtas_size;
 int max_memory_ranges;
 const char *ramdisk;
 
+/*
+ * Reads the #address-cells and #size-cells on this platform.
+ * This is used to parse the memory/reg info from the device-tree
+ */
+int init_memory_region_info()
+{
+ size_t res = 0;
+ FILE *fp;
+ char *file;
+
+ file = "/proc/device-tree/#address-cells";
+ fp = fopen(file, "r");
+ if (!fp) {
+ fprintf(stderr,"Unable to open %s\n", file);
+ return -1;
+ }

kexec/arch/ppc/kexec-ppc.c unsigned int rtas_base, rtas_size; · line 61

Quoted 8 lines

kexec/arch/ppc/kexec-ppc.c +8 -0

+
+ res = fread(&dt_address_cells,sizeof(unsigned long),1,fp);
+ if (res != 1) {
+ fprintf(stderr,"Error reading %s\n", file);
+ return -1;
+ }
+ fclose(fp);
+ dt_address_cells *= sizeof(unsigned long);

Quoted 6 lines

kexec/arch/ppc/kexec-ppc.c +6 -0

+ file = "/proc/device-tree/#size-cells";
+ fp = fopen(file, "r");
+ if (!fp) {
+ fprintf(stderr,"Unable to open %s\n", file);
+ return -1;
+ }

kexec/arch/ppc/kexec-ppc.c unsigned int rtas_base, rtas_size; · line 76

Quoted 8 lines

kexec/arch/ppc/kexec-ppc.c +8 -0

+
+ res = fread(&dt_size_cells,sizeof(unsigned long),1,fp);
+ if (res != 1) {
+ fprintf(stderr,"Error reading %s\n", file);
+ return -1;
+ }
+ fclose(fp);
+ dt_size_cells *= sizeof(unsigned long);

kexec/arch/ppc/kexec-ppc.c unsigned int rtas_base, rtas_size; · line 80

Quoted 4 lines

kexec/arch/ppc/kexec-ppc.c +4 -0

+
+ return 0;
+}
+

Quoted 1 line · Suzuki Poulose

Could you please let me know your thoughts/comments about this patch ?

I'm mostly fine with it.

Maaxim copied fs2dt.c from ppc64 to ppc. So I guess ppc64 has the same problem.

ARM and MIPS is soon having DT support and kexec is probably also on their list so I would hate to see them to either copy the DT parsing file or having their own implementation.

Maybe we should try to use libfdt for dt parsing. It has /proc import support so it should be fine for our needs. It is already in tree and used by ppc32 if a basic dtb is specified. I'm not sure if the /proc interface is part of dtc or libfdt.

I'm not saying this has to be done now but maybe later before ARM and/or MIPS comes along needs something similar for their needs. If the libfdt is too complex for sucking in the dtb from /proc then maybe something else that generic and can be shared between booth ppc architectures and the other ones.

Quoted 26 lines

Quoted 26 lines

Index: kexec-tools-2.0.4/kexec/arch/ppc/kexec-ppc.c
===================================================================

kexec/arch/ppc/kexec-ppc.c +18 -0

@@ -34,6 +35,92 @@ unsigned int rtas_base, rtas_size;
 int max_memory_ranges;
 const char *ramdisk;
 
+/*
+ * Reads the #address-cells and #size-cells on this platform.
+ * This is used to parse the memory/reg info from the device-tree
+ */
+int init_memory_region_info()
+{
+ size_t res = 0;
+ FILE *fp;
+ char *file;
+
+ file = "/proc/device-tree/#address-cells";
+ fp = fopen(file, "r");
+ if (!fp) {
+ fprintf(stderr,"Unable to open %s\n", file);
+ return -1;
+ }
+
+ res = fread(&dt_address_cells,sizeof(unsigned long),1,fp);

Quoted 6 lines

kexec/arch/ppc/kexec-ppc.c unsigned int rtas_base, rtas_size; · line 61

Quoted 6 lines

kexec/arch/ppc/kexec-ppc.c +6 -0

+ if (res != 1) {
+ fprintf(stderr,"Error reading %s\n", file);
+ return -1;
+ }
+ fclose(fp);
+ dt_address_cells *= sizeof(unsigned long);

This should be sizeof(unsigned int). I know we on 32bit.

Quoted 6 lines

Quoted 6 lines

kexec/arch/ppc/kexec-ppc.c +6 -0

+ file = "/proc/device-tree/#size-cells";
+ fp = fopen(file, "r");
+ if (!fp) {
+ fprintf(stderr,"Unable to open %s\n", file);
+ return -1;
+ }

Quoted 8 lines

kexec/arch/ppc/kexec-ppc.c unsigned int rtas_base, rtas_size; · line 76

Quoted 8 lines

kexec/arch/ppc/kexec-ppc.c +8 -0

+
+ res = fread(&dt_size_cells,sizeof(unsigned long),1,fp);
+ if (res != 1) {
+ fprintf(stderr,"Error reading %s\n", file);
+ return -1;
+ }
+ fclose(fp);
+ dt_size_cells *= sizeof(unsigned long);

same here.

Quoted 1 line

Changed the add_usable_mem_property() to accept FILE* fp instead of

Quoted 1 line

as most of the other users of read_memory_region_limits() deals with

Quoted 2 lines

Signed-off-by Suzuki K. Poulose <[email protected]>

Could you please let me know your thoughts/comments about this patch ?

Quoted 1 line

Quoted 1 line

Changed the add_usable_mem_property() to accept FILE* fp instead of

int fd,

Quoted 1 line

Quoted 1 line

as most of the other users of read_memory_region_limits() deals with

FILE*.

Quoted 1 line

Quoted 1 line

Quoted 3 lines

Quoted 1 line

Signed-off-by Suzuki K. Poulose<[email protected]>

Could you please let me know your thoughts/comments about this patch ?

Is the change to use 'FILE *' actually progress?
I'd have thought that the randomly aligned read/lseek system calls that this allows to happen are not desirable for anything that isn't a true file.

I'd also suggest that the sizeof's should be applied to the actual type of the variable being read/written, not arbitrarily 'long' or 'int', and this probably ought to be some fixed size type.