Subject: Re: [PATCH 8/15] bootwrapper: convert flatdevtree to version 16

From: Milton Miller <[email-masked]>
Date: Mon, 24 Sep 2007 01:54:32 -0500

On Sep 23, 2007, at 10:36 PM, David Gibson wrote:

> On Fri, Sep 21, 2007 at 06:05:06PM -0500, Milton Miller wrote:
>> kexec-tools still produces a version 2 device tree, while the
>> libraries in the wrapper only support version 16 and later.
>>
>> Add a routine to convert a v2 flat device tree to a v16 one inplace
>> by inserting OF_DT_NOP and chomping full path.  Make space for new
>> headers by moving and then chomping the OF_DT_NOPs.
>>
>> Signed-off-by: Milton Miller <[email-masked]>
>> ---
>> vs 12175
>> Rediffed Makefile, ops, kexec.c
>>
>> Index: kernel/arch/powerpc/boot/flatdevtree_conv.c
>> ===================================================================
>> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
>> +++ kernel/arch/powerpc/boot/flatdevtree_conv.c	2007-09-20 
>> 17:49:04.000000000 -0500
>> @@ -0,0 +1,280 @@
>> +/*
>> + * This program is free software; you can redistribute it and/or 
>> modify
>> + * it under the terms of the GNU General Public License as published 
>> by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA  
>> 02110-1301, USA.
>> + *
>> + * Copyright IBM Corporation 2007
>> + *
>> + * Authors: Milton Miller <[email-masked]>
>> + */
>> +#include "flatdevtree.h"
>> +#include "stdio.h"
>> +#include "ops.h"
>> +
>> +#define MIN_VERSION 2
>> +#define OUT_VERSION 16
>
> Should output version 17.  In any case, don't try to be so general -
> just convert v123 (all basically the same) to latest (i.e. v17)
> without all the #if nonsense.

Outputing v17 instead of 16 requires more words to be added to the 
header, and the library does fine with v16.  Actually the v1 trees has 
some other differences such as initrd addresses were kernel linear not 
real, cpus were assigned logical numbers  ... so while the structure 
didn't change except for the header field, the contents did.  Actually, 
when converting v3 to v16 some of the code issn't needed, the ifs allow 
the code size to be reduced.

>
>> +#define OUT_COMPAT 16
>> +
>> +#ifdef NO_CHECK
>> +static int check_v123_tree(u32 *start, u32 *limit)
>> +{
>> +	return 0;
>> +}
>> +#else
>> +/**
>> + * check_v123_tree - check integrety of a version 1, 2, or 3 tree
>> + * @start: the start of the device tree struct
>> + * @limit: the end of the region for the struct
>> + * structural checks on device_tree
>> + */
>> +static int check_v123_tree(u32 *start, u32 *limit)
>
> What is the point of this check?  If the device tree is corrupt, we're
> stuffed anyway, so why bother?

Hence the ifdef NO_CHECK.   When developing, sometimes its nice to know 
if its your input or your program.  These functions are destructive to 
an improperlly formed tree, and in non-obvious ways.  When debugging, 
it's not hard to hardcode console write or read the printf string 
buffer with a hardware debugger to see error messages.  That said, it 
could be removed.

>
>> +{
>> +	u32 len;
>> +	int depth = 0;
>> +	u32 *dtp = start;
>> +
>> +	while (dtp < limit)
>> +		switch (*dtp) {
>> +		case OF_DT_END:
>> +			if (depth)
>> +				return -1;
>> +			return ++dtp - start;
>> +		case OF_DT_NOP:
>> +			dtp++;
>> +			break;
>> +		case OF_DT_END_NODE:
>> +			dtp++;
>> +			depth--;
>> +			break;
>> +		case OF_DT_BEGIN_NODE:
>> +			len = strlen((char *)(++dtp));
>> +			/* check path is suffix to previous? */
>> +			dtp += 1 + (len / 4);
>> +			depth++;
>> +			break;
>> +		case OF_DT_PROP:
>> +			len = dtp[1];
>> +			dtp += 3;
>> +			if ((len >= 8) && ((long)dtp & 4))
>> +				dtp++;
>> +			dtp += (len + 3) / 4;
>> +			break;
>> +		default:
>> +			return -1;
>> +		}
>> +	return -1;	/* no OF_DT_END */
>> +}
>> +#endif
>> +
>> +/**
>> + * nop_to_v16 - add %OF_DT_NOP to hide alignment differences
>> + * @dtp: pointer to the beginning of the struct area to modify
>> + * insert %OF_DT_NOP into the dt_struct @dtp to make it v16 from v1, 
>> 2, or 3.
>> + */
>> +static int nop_to_v16(u32 *dtp)
>> +{
>> +	int nops = 0;
>> +	char *p, *s;
>> +	int len;
>> +	u32 *next;
>> +
>> +	while (*dtp != OF_DT_END)
>> +		switch (*dtp) {
>> +		case OF_DT_BEGIN_NODE:
>> +			/* v2 & v3 names are full path, v16+ is relative */
>> +			p = (char *)(++dtp);
>> +			len = strlen(p);
>> +			next = dtp + 1 + len / 4;
>> +
>> +			for (s = p + len; *s != '/'; s--)
>> +				if (s == p)
>> +					fatal("name %s has no '/'", p);
>> +
>> +			len -= s++ - p;		/* not the slash but the nul */
>> +			memmove(p, s, len);
>> +			while (len % 4)
>> +				p[len++] = '\0';
>> +			dtp += len / 4;
>> +			while (dtp != next) {
>> +				*dtp++ = OF_DT_NOP;
>> +				nops++;
>> +			}
>> +			break;
>> +		case OF_DT_PROP:
>> +			/* convert from align_8 to align_4 via prefixing nop */
>> +			len = dtp[1];
>> +			if ((len >= 8) && !((long)dtp & 4)) {
>> +				memmove(dtp+1, dtp, 12);
>> +				*dtp++ = OF_DT_NOP;
>> +				nops++;
>> +			}
>> +			dtp += 3 + (len + 3)/4;
>> +			break;
>> +		default:
>> +			fatal("%s: unrecognised tag %d at %p\n", __FUNCTION__,
>> +				*dtp, dtp);
>> +		case OF_DT_NOP:
>> +			nops ++;
>> +			/* fall through */
>> +		case OF_DT_END_NODE:
>> +			dtp ++;
>> +			break;
>> +		}
>> +	return nops;
>> +}
>> +
>> +#if MIN_VERSION < 3 || OUT_VERSION > 16
>> +/**
>> + * move_nops_fwd - move nops in a v16 dt_struct to the beginning
>> + * @start - device tree starting address
>> + * @count - number of %OF_DT_NOP cells to move
>> + */
>> +static void move_nops_fwd(u32 *start, int count)
>
> What on earth is the point of this.  The NOPs are perfectly valid
> scattered within the tree, why go to all this trouble to shuffle them
> about.

And if you notice, there is a "how many to move" argument.  The point 
of moving them to the front of the tree is the v17 device tree header 
takes more space than the v3 one, and the v2 header is smaller than 
both v17 and v16 header.  Since I am converting the tree in place, the 
space has to come from somewhere.  Since we are pretty much guaranteed 
to get several nops, this function moves them forward so they can be 
overwritten.  In practice we move 1-3 NOPS from the early properties > 
8 bytes and early grandchild nodes (eg /cpus/PowerPC,xxx).

>
>> +{
>> +	u32 *dtp = start;
>> +	int len;
>> +	while (count)
>> +		switch (*dtp) {
>> +		case OF_DT_NOP:
>> +			memmove(start+1,start,(dtp-start) * 4);
>> +			*start++ = OF_DT_NOP;
>> +			dtp++;
>> +			count--;
>> +			break;
>> +		case OF_DT_END_NODE:
>> +			dtp++;
>> +			break;
>> +		case OF_DT_BEGIN_NODE:
>> +			len = strlen((char *)(++dtp));
>> +			dtp += 1 + len / 4;
>> +			break;
>> +		case OF_DT_PROP:
>> +			len = dtp[1];
>> +			dtp += 3 + (len + 3) / 4;
>> +			break;
>> +		case OF_DT_END:
>> +			fatal("Not enough nops -- need %d more\n", count);
>> +			return;
>> +		default:
>> +			fatal("%s: unknown tag %d at %p", __FUNCTION__, *dtp, dtp)
>> +		}
>> +}
>> +#endif
>> +
>> +/**
>> + * conv_flattree_inplace upgrade the version of a boot_param_header
>> + * @tree: pointer to the device tree header to convert
>> + *
>> + * Converts a v1, 2, 3 device tree (of at least MIN_VERSION)
>> + * in place to OUT_VERSION (16) format, usable by flatdevtree.c
>> + */
>> +void conv_flattree_inplace(struct boot_param_header *tree)
>> +{
>> +	u32 *dtp;
>> +	u32 need = 0, nops;
>> +	int slen;
>> +
>> +	if (tree->magic != OF_DT_HEADER)
>> +		fatal("%s: no magic", __FUNCTION__);
>
> More pointless tests...

It says you didn't pass a tree ... yea, its dumb, but I prefer an 
explicit error to figuring out where a machine check came from.

>
>> +	if (tree->last_comp_version > 3)
>> +		return;			/* don't know what to do */
>
> Rather, don't need to do anything.

If the tree is >= 16 then we don't presently need to do anything.   If 
there is a theoritical v4 tree we don't know what to do.  And if output 
version is 17 but input is 16 we don't know what to do either, because 
there likely aren't nops in the tree to consume.  I suppose we could 
preceed it with a check for version == OUTPUT_VERSION, but then I'm 
sure I'd get pointless differentation :-).

>
>> +
>> +	if (tree->version < MIN_VERSION) {
>> +		printf("%s: Warning: can't handle version %d tree\n",
>> +				__FUNCTION__, tree->version);
>> +		return;
>> +	}
>> +
>> +	if (tree->version < 2)
>> +		need++;		/* boot_cpu_id */
>> +
>> +	if (tree->version < 3)
>> +		need++;		/* dt_string_size */
>> +
>> +	if (OUT_VERSION > 16)
>> +		need++;		/* dt_struct_size */
>> +
>> +	dtp = (void *)tree + tree->off_dt_struct;
>> +
>> +	slen = check_v123_tree(dtp, (void *)tree + tree->totalsize);
>> +	if (slen < 0)
>> +		fatal("device tree check failed\n");
>> +
>> +	nops = nop_to_v16(dtp);
>> +
>> +	if (need & 1)		/* keep 8 byte alignment of mem reserve */
>> +		need++;
>> +
>> +	if (need > nops)
>> +		fatal("Didn't find enough space to add new header fields\n\r"
>> +			"(needed %d found %d tree %p)", need, nops, tree);
>> +
>> +	/* ok now compress the dtb struct */
>> +	move_nops_fwd(dtp, need);
>> +	dtp += need;
>> +
>> +	/*
>> +	 * move mem_rsvmap and dt_strings if they are before dt_struct
>> +	 * onto our nops .  Adjust start addresses for the 3 sections.
>> +	 */
>
> Hrm.  Do we really need to worry about this case.  You may be
> producing v2 trees in kexec-tools, but do they actually have the
> blocks out of order?  dtc certainly never produced them that way.

Out of order?  There has never been a spec as to the order of the 
blocks, only the implicit assumption that they follow the device tree 
header in a reasonably packed sequence.  booting-without-of says it 
must be in ram,; the offsets are unsigned 32 bit quantities.

As to the order, used, the first implemntation was the kernel which 
writes memreserve, strings, then struct (both the openfirmware client 
in prom_init and the iSeries procedural library in dt.c).  The second 
implentation written is a procedural based library (similar to iseries, 
never published, but still used internally) that starts with a 
pre-built header and string table, builds the dt_struct as the 
functions are called, and when finished copies the memreserve table and 
fills in the dt_size, total size, and memreserve offset.

fs2dt writes memreserve, struct, then strings.  Aparently the same as 
dtc.  But yes, the strings can be before the struct, and the mem 
reserve may or may not be when the strings are before the struct.

>
>> +	if ((tree->off_mem_rsvmap < tree->off_dt_struct) ||
>> +		(tree->off_dt_strings < tree->off_dt_struct)) {
>> +		int start, end;
>> +		void *ptr;
>> +
>> +		if (tree->off_mem_rsvmap < tree->off_dt_strings)
>> +			start = tree->off_mem_rsvmap;
>> +		else
>> +			start = tree->off_dt_strings;
>> +
>> +		end = tree->off_dt_struct;
>> +		ptr = (void *)tree + start;
>> +
>> +		memmove(ptr + 4 * need, ptr, end - start);
>> +
>> +		if (tree->off_mem_rsvmap < tree->off_dt_struct)
>> +			tree->off_mem_rsvmap += 4 * need;
>> +		if (tree->off_dt_strings < tree->off_dt_struct)
>> +			tree->off_dt_strings += 4 * need;
>> +	}
>> +	tree->off_dt_struct += 4 * need;
>> +
>> +	/* ok now we have space to extend the header. */
>> +	if (tree->version < 2 && MIN_VERSION < 2) {
>> +		tree->boot_cpuid_phys = 0;	/* default, caller can fix */
>> +	}
>> +
>> +	/* calculate size of dt_strings_size */
>> +	if (tree->version < 3 && MIN_VERSION < 3) {
>> +		int end = tree->totalsize;
>> +
>> +		if (tree->off_dt_strings < tree->off_mem_rsvmap)
>> +			end = tree->off_mem_rsvmap;
>> +
>> +		if ((tree->off_dt_strings < tree->off_dt_struct) &&
>> +				(end > tree->off_dt_struct))
>> +			end = tree->off_dt_struct;
>> +
>> +		tree->dt_strings_size = end - tree->off_dt_strings;
>> +	}
>> +
>> +#if OUT_VERSION > 16
>> +	tree->dt_struct_size = 4 * slen;
>> +#endif
>> +
>> +	tree->version = OUT_VERSION;
>> +	tree->last_comp_version = OUT_COMPAT;
>> +
>> +	return;
>> +}
>> Index: kernel/arch/powerpc/boot/Makefile
>> ===================================================================
>> --- kernel.orig/arch/powerpc/boot/Makefile	2007-09-20 
>> 17:42:24.000000000 -0500
>> +++ kernel/arch/powerpc/boot/Makefile	2007-09-20 17:49:04.000000000 
>> -0500
>> @@ -42,7 +42,7 @@ $(addprefix $(obj)/,$(zlib) gunzip_util.
>>  	$(addprefix $(obj)/,$(zliblinuxheader)) $(addprefix 
>> $(obj)/,$(zlibheader))
>>
>>  src-wlib := string.S crt0.S stdio.c main.c flatdevtree.c 
>> flatdevtree_misc.c \
>> -		marshal.c memranges.c kexec.c \
>> +		flatdevtree_conv.c marshal.c memranges.c kexec.c \
>>  		ns16550.c serial.c simple_alloc.c div64.S util.S \
>>  		gunzip_util.c elf_util.c $(zlib) devtree.c oflib.c ofconsole.c \
>>  		4xx.c ebony.c mv64x60.c mpsc.c mv64x60_i2c.c cuboot.c bamboo.c \
>> Index: kernel/arch/powerpc/boot/ops.h
>> ===================================================================
>> --- kernel.orig/arch/powerpc/boot/ops.h	2007-09-20 17:42:24.000000000 
>> -0500
>> +++ kernel/arch/powerpc/boot/ops.h	2007-09-20 17:49:04.000000000 -0500
>> @@ -81,7 +81,10 @@ struct loader_info {
>>  };
>>  extern struct loader_info loader_info;
>>
>> +struct boot_param_header;
>> +
>>  void start(void);
>> +void conv_flattree_inplace(struct boot_param_header *tree);
>>  int ft_init(void *dt_blob, unsigned int max_size, unsigned int 
>> max_find_device);
>>  int serial_console_init(void);
>>  int ns16550_console_init(void *devp, struct serial_console_data 
>> *scdp);
>> Index: kernel/arch/powerpc/boot/kexec.c
>> ===================================================================
>> --- kernel.orig/arch/powerpc/boot/kexec.c	2007-09-20 
>> 17:42:24.000000000 -0500
>> +++ kernel/arch/powerpc/boot/kexec.c	2007-09-20 17:49:04.000000000 
>> -0500
>> @@ -99,6 +99,7 @@ void kexec_platform_init(struct boot_par
>>  	move_slaves_up();
>>
>>  	setup_initial_heap();
>> +	conv_flattree_inplace(dt_blob);
>>  	init_flat_tree(dt_blob);
>>  	/*
>>  	 * drivers can malloc and read the tree, but not realloc later
>> _______________________________________________
>> Linuxppc-dev mailing list
>> [email-masked]
>> https://ozlabs.org/mailman/listinfo/linuxppc-dev
>>
>
> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ 
> _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson
>