[RFC] 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

On 05/30/11 12:00, Suzuki Poulose wrote:
> 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 <suzuki@in.ibm.com>

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

Thanks
Suzuki

>
> ---
> 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-tools-2.0.4.orig/kexec/arch/ppc/kexec-ppc.c
> +++ kexec-tools-2.0.4/kexec/arch/ppc/kexec-ppc.c
> @@ -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-tools-2.0.4.orig/kexec/arch/ppc/kexec-ppc.h
> +++ kexec-tools-2.0.4/kexec/arch/ppc/kexec-ppc.h
> @@ -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-tools-2.0.4.orig/kexec/arch/ppc/crashdump-powerpc.c
> +++ kexec-tools-2.0.4/kexec/arch/ppc/crashdump-powerpc.c
> @@ -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-tools-2.0.4.orig/kexec/arch/ppc/fs2dt.c
> +++ kexec-tools-2.0.4/kexec/arch/ppc/fs2dt.c
> @@ -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
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

Suzuki Poulose wrote:
> 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.

> Thanks
> Suzuki
> 
>>
>> Index: kexec-tools-2.0.4/kexec/arch/ppc/kexec-ppc.c
>> ===================================================================
>> --- kexec-tools-2.0.4.orig/kexec/arch/ppc/kexec-ppc.c
>> +++ kexec-tools-2.0.4/kexec/arch/ppc/kexec-ppc.c
>> @@ -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);

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

>> + 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);

same here.

>> +
>> + return 0;
>> +}
>> +

Sebastian

> > 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 <suzuki@in.ibm.com>
> 
> 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.

	David

On 06/06/11 14:21, Sebastian Andrzej Siewior wrote:
> Suzuki Poulose wrote:
>> 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.

Yes, you are right. Porting this patch over to ppc64 is in my TODO list.

> 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.
OK

>>> Index: kexec-tools-2.0.4/kexec/arch/ppc/kexec-ppc.c
>>> ===================================================================
>>> --- kexec-tools-2.0.4.orig/kexec/arch/ppc/kexec-ppc.c
>>> +++ kexec-tools-2.0.4/kexec/arch/ppc/kexec-ppc.c
>>> @@ -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);
>
> This should be sizeof(unsigned int). I know we on 32bit.
>
OK. I was using (unsigned long) to get the word size on the machine. Given
this code is duplicated in ppc64, thought of having a generic code which works
fine for all ppcXX. As you mentioned, if we go about moving to a single copy of
fdt code, using long would help us.

>>> + 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);
>
> same here.

Thanks
Suzuki

On 06/06/11 14:30, David Laight wrote:
>>> 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<suzuki@in.ibm.com>
>>
>> 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 will revert the other users back to 'fd'
>
> 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.
I have used 'unsigned long'(for word sized values) or 'unsigned long long'
(for double words) just to make sure we get the right values. Is this OK ?

Thanks
Suzuki