bootwrapper: kexec and external payloads

Some platforms have a boot agent that can create or modify properties in the device-tree and load images into memory. Provide a helper to set loader_info used by prep_initrd().

Signed-off-by Milton Miller <[email protected]>

Acked-by David Gibson <[email protected]>

---
re 12168
rediffed types.h, offset in ops.h

Index: kernel/arch/powerpc/boot/ops.h
===================================================================

arch/powerpc/boot/ops.h +1 -0

@@ -163,6 +163,7 @@ void dt_fixup_clock(const char *path, u3
 void __dt_fixup_mac_addresses(u32 startindex, ...);
 #define dt_fixup_mac_addresses(...) \
 	__dt_fixup_mac_addresses(0, __VA_ARGS__, NULL)
+void dt_find_initrd(void);
 
 
 static inline void *find_node_by_linuxphandle(const u32 linuxphandle)

Index: kernel/arch/powerpc/boot/types.h
===================================================================

arch/powerpc/boot/types.h +1 -0

+
 #define min(x,y) ({ \
 	typeof(x) _x = (x);	\
 	typeof(y) _y = (y);	\

Index: kernel/arch/powerpc/boot/devtree.c
===================================================================

arch/powerpc/boot/devtree.c +1 -0

@@ -1,6 +1,7 @@
 /*
  * devtree.c - convenience functions for device tree manipulation
  * Copyright 2007 David Gibson, IBM Corporation.
+ * Copyright 2007 Milton Miller, IBM Corporation.
  * Copyright (c) 2007 Freescale Semiconductor, Inc.
  *
  * Authors: David Gibson <[email protected]>

@@ -333,3 +334,68 @@ int dt_is_compatible(void *node, const c
 
 	return 0;
 }

arch/powerpc/boot/devtree.c +10 -0

+
+	rc = getprop(devp, start_prop, &initrd_start, sizeof(initrd_start));
+	if (rc < 0)
+		return;				/* not found */
+	/* The properties had to be 8 bytes until 2.6.22 */
+	if (rc == sizeof(unsigned long)) {
+		unsigned long tmp;
+		memcpy(&tmp, &initrd_start, rc);
+		initrd_start = tmp;
+	} else if (rc != sizeof(initrd_start)) {	/* now they

can be 4 */

arch/powerpc/boot/devtree.c +30 -0

+	}
+
+	rc = getprop(devp, end_prop, &initrd_end, sizeof(initrd_end));
+	if (rc < 0) {
+		printf("chosen has %s but no %s!\n\r", start_prop, end_prop);
+		return;
+	}
+	if (rc == sizeof(unsigned long)) {
+		unsigned long tmp;
+		memcpy(&tmp, &initrd_end, rc);
+		initrd_end = tmp;
+	} else if (rc != sizeof(initrd_end)) {
+		printf("unexpected length of %s in /chosen!\n\r", end_prop);
+		return;
+	}
+
+	/* Check for presence, ignore if (partially) loaded above 32 bits */
+	if (initrd_start == initrd_end) {
+		printf("ignoring empty device-tree supplied initrd\n");
+	} else if (initrd_start > initrd_end) {
+		printf("ignoring device-tree supplied initrd: start 0x%llx"
+				" > end 0x%llx \n", initrd_start, initrd_end);
+	} else if (initrd_end > UINT_MAX) {
+		printf("ignoring device-tree supplied initrd:"
+				" end 0x%llx > 32 bits\n", initrd_end);
+	} else {
+		loader_info.initrd_addr = initrd_start;
+		loader_info.initrd_size  = initrd_end - initrd_start;
+	}
+}

_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Quoted 6 lines · Milton Miller

Some platforms have a boot agent that can create or modify properties in the device-tree and load images into memory. Provide a helper to set loader_info used by prep_initrd().

Signed-off-by Milton Miller <[email protected]>

Acked-by David Gibson <[email protected]>

Hrm, despite my earlier ack, I'm going to whinge about a few nits here.

Quoted 24 lines

---
re 12168
rediffed types.h, offset in ops.h

Index: kernel/arch/powerpc/boot/ops.h
===================================================================
--- kernel.orig/arch/powerpc/boot/ops.h 2007-09-17 22:12:47.000000000
-0500 +++ kernel/arch/powerpc/boot/ops.h 2007-09-17 22:12:51.000000000
-0500 @@ -163,6 +163,7 @@ void dt_fixup_clock(const char *path, u3
void __dt_fixup_mac_addresses(u32 startindex, ...); #define dt_fixup_mac_addresses(...) \

arch/powerpc/boot/ops.h +1 -0

 	__dt_fixup_mac_addresses(0, __VA_ARGS__, NULL)
+void dt_find_initrd(void);
 
 
 static inline void *find_node_by_linuxphandle(const u32 linuxphandle)

Index: kernel/arch/powerpc/boot/types.h
===================================================================
--- kernel.orig/arch/powerpc/boot/types.h 2007-09-17 22:12:47.000000000
-0500 +++ kernel/arch/powerpc/boot/types.h 2007-09-17 22:12:51.000000000
-0500 @@ -12,6 +12,8 @@ typedef short s16;
typedef int s32;
typedef long long s64;

Quoted 1 line

+#define UINT_MAX 0xFFFFFFFF

I actually don't like this constant - at the point you compare you care, explicitly, about the value not being over 32-bits, rather than whether it fits a uint, so the named constant is more misleading than helpful.

Quoted 9 lines

+/**
+ * dt_find_initrd - set loader initrd location based on existing properties + *
+ * finds the linux,initrd-start and linux,initrd-end properties in
+ * the /chosen node and sets the loader initrd fields accordingly.
+ *
+ * Use this if your loader sets the properties to allow other code to
+ * relocate the tree and/or cause r3 and r4 to be set on true OF
+ * platforms.

I am unable to make sense of the paragraph above.

Quoted 6 lines · Milton Miller

Some platforms have a boot agent that can create or modify properties in the device-tree and load images into memory. Provide a helper to set loader_info used by prep_initrd().

Signed-off-by Milton Miller <[email protected]>

Acked-by David Gibson <[email protected]>

Hrm, despite my earlier ack, I'm going to whinge about a few nits here.

Quoted 27 lines

---
re 12168
rediffed types.h, offset in ops.h

Index: kernel/arch/powerpc/boot/ops.h
===================================================================
--- kernel.orig/arch/powerpc/boot/ops.h 2007-09-17 22:12:47.000000000
-0500
+++ kernel/arch/powerpc/boot/ops.h 2007-09-17 22:12:51.000000000 -0500

arch/powerpc/boot/ops.h +1 -0

@@ -163,6 +163,7 @@ void dt_fixup_clock(const char *path, u3
 void __dt_fixup_mac_addresses(u32 startindex, ...);
 #define dt_fixup_mac_addresses(...) \
 	__dt_fixup_mac_addresses(0, __VA_ARGS__, NULL)
+void dt_find_initrd(void);
 
 
 static inline void *find_node_by_linuxphandle(const u32 linuxphandle)

Index: kernel/arch/powerpc/boot/types.h
===================================================================
--- kernel.orig/arch/powerpc/boot/types.h 2007-09-17
22:12:47.000000000 -0500
+++ kernel/arch/powerpc/boot/types.h 2007-09-17 22:12:51.000000000
-0500

arch/powerpc/boot/types.h +0 -0

@@ -12,6 +12,8 @@ typedef short			s16;
 typedef int			s32;
 typedef long long		s64;
 

arch/powerpc/boot/types.h typedef short s16; · line 15

Quoted 1 line

arch/powerpc/boot/types.h +1 -0

+#define UINT_MAX	0xFFFFFFFF

I actually don't like this constant - at the point you compare you care, explicitly, about the value not being over 32-bits, rather than whether it fits a uint, so the named constant is more misleading than helpful.

Quoted 20 lines

arch/powerpc/boot/types.h +1 -0

+
 #define min(x,y) ({ \
 	typeof(x) _x = (x);	\
 	typeof(y) _y = (y);	\

Index: kernel/arch/powerpc/boot/devtree.c
===================================================================
--- kernel.orig/arch/powerpc/boot/devtree.c 2007-09-17
22:12:47.000000000 -0500
+++ kernel/arch/powerpc/boot/devtree.c 2007-09-17 22:12:51.000000000
-0500

arch/powerpc/boot/devtree.c +1 -0

@@ -1,6 +1,7 @@
 /*
  * devtree.c - convenience functions for device tree manipulation
  * Copyright 2007 David Gibson, IBM Corporation.
+ * Copyright 2007 Milton Miller, IBM Corporation.
  * Copyright (c) 2007 Freescale Semiconductor, Inc.
  *
  * Authors: David Gibson <[email protected]>

@@ -333,3 +334,68 @@ int dt_is_compatible(void *node, const c
 

Quoted 5 lines

arch/powerpc/boot/devtree.c +3 -0

 	return 0;
 }
+
+/**
+ * dt_find_initrd - set loader initrd location based on existing 

Quoted 8 lines

properties
+ *
+ * finds the linux,initrd-start and linux,initrd-end properties in
+ * the /chosen node and sets the loader initrd fields accordingly.
+ *
+ * Use this if your loader sets the properties to allow other code to
+ * relocate the tree and/or cause r3 and r4 to be set on true OF
+ * platforms.

I am unable to make sense of the paragraph above.

arch/powerpc/boot/devtree.c int dt_is_compatible(void *node, const c · line 354

Quoted 9 lines

+ */

arch/powerpc/boot/devtree.c +7 -0

+void dt_find_initrd(void)
+{
+	int rc;
+	unsigned long long initrd_start, initrd_end;
+	void *devp;
+	static const char start_prop[] = "linux,initrd-start";
+	static const char end_prop[] = "linux,initrd-end";

I think these constants are more obscuring than useful.

arch/powerpc/boot/devtree.c int dt_is_compatible(void *node, const c · line 359

Quoted 6 lines

arch/powerpc/boot/devtree.c +5 -0

+
+	devp = finddevice("/chosen");
+	if (! devp) {
+		return;
+	}

CodingStyle would not put { } here.

Quoted 11 lines

arch/powerpc/boot/devtree.c +10 -0

+
+	rc = getprop(devp, start_prop, &initrd_start, sizeof(initrd_start));
+	if (rc < 0)
+		return;				/* not found */
+	/* The properties had to be 8 bytes until 2.6.22 */
+	if (rc == sizeof(unsigned long)) {
+		unsigned long tmp;
+		memcpy(&tmp, &initrd_start, rc);
+		initrd_start = tmp;
+	} else if (rc != sizeof(initrd_start)) {	/* now they

Quoted 1 line

can be 4 */

Right. 8 bytes and 4 bytes, so you should be using explicit length types instead of long and long long.

Review

Quoted 3 lines

(quoted diff) +2 -0

+		printf("unexpected length of %s in /chosen!\n\r", start_prop);
+		return;

All these printf() / return stanzas add a lot of verbosity to this function. Any way they can be consolidated a bit, maybe a single error path that just prints the property values, so the user can figure out what was wrong with them.

Quoted 36 lines

arch/powerpc/boot/devtree.c +30 -0

+	}
+
+	rc = getprop(devp, end_prop, &initrd_end, sizeof(initrd_end));
+	if (rc < 0) {
+		printf("chosen has %s but no %s!\n\r", start_prop, end_prop);
+		return;
+	}
+	if (rc == sizeof(unsigned long)) {
+		unsigned long tmp;
+		memcpy(&tmp, &initrd_end, rc);
+		initrd_end = tmp;
+	} else if (rc != sizeof(initrd_end)) {
+		printf("unexpected length of %s in /chosen!\n\r", end_prop);
+		return;
+	}
+
+	/* Check for presence, ignore if (partially) loaded above 32 bits */
+	if (initrd_start == initrd_end) {
+		printf("ignoring empty device-tree supplied initrd\n");
+	} else if (initrd_start > initrd_end) {
+		printf("ignoring device-tree supplied initrd: start 0x%llx"
+				" > end 0x%llx \n", initrd_start, initrd_end);
+	} else if (initrd_end > UINT_MAX) {
+		printf("ignoring device-tree supplied initrd:"
+				" end 0x%llx > 32 bits\n", initrd_end);
+	} else {
+		loader_info.initrd_addr = initrd_start;
+		loader_info.initrd_size  = initrd_end - initrd_start;
+	}
+}

_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Signature · 4 lines

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

Quoted 39 lines · David Gibson

Quoted 7 lines · Milton Miller

Some platforms have a boot agent that can create or modify properties in
the device-tree and load images into memory. Provide a helper to set loader_info used by prep_initrd().

Signed-off-by Milton Miller <[email protected]>

Acked-by David Gibson <[email protected]>

Hrm, despite my earlier ack, I'm going to whinge about a few nits here.

Quoted 27 lines

---
re 12168
rediffed types.h, offset in ops.h

Index: kernel/arch/powerpc/boot/ops.h
===================================================================
--- kernel.orig/arch/powerpc/boot/ops.h 2007-09-17 22:12:47.000000000
-0500
+++ kernel/arch/powerpc/boot/ops.h 2007-09-17 22:12:51.000000000 -0500

arch/powerpc/boot/ops.h +1 -0

@@ -163,6 +163,7 @@ void dt_fixup_clock(const char *path, u3
 void __dt_fixup_mac_addresses(u32 startindex, ...);
 #define dt_fixup_mac_addresses(...) \
 	__dt_fixup_mac_addresses(0, __VA_ARGS__, NULL)
+void dt_find_initrd(void);
 
 
 static inline void *find_node_by_linuxphandle(const u32 linuxphandle)

Index: kernel/arch/powerpc/boot/types.h
===================================================================
--- kernel.orig/arch/powerpc/boot/types.h 2007-09-17
22:12:47.000000000 -0500
+++ kernel/arch/powerpc/boot/types.h 2007-09-17 22:12:51.000000000
-0500

arch/powerpc/boot/types.h +0 -0

@@ -12,6 +12,8 @@ typedef short			s16;
 typedef int			s32;
 typedef long long		s64;
 

Quoted 6 lines

arch/powerpc/boot/types.h typedef short s16; · line 15

Quoted 1 line

arch/powerpc/boot/types.h +1 -0

+#define UINT_MAX	0xFFFFFFFF

I actually don't like this constant - at the point you compare you care, explicitly, about the value not being over 32-bits, rather than whether it fits a uint, so the named constant is more misleading than helpful.

Arguable, I don't like counting F's or zeros in C code.

It is used as the max address that the wrapper deals with, both here and memranges, which happens to be 32 bits.

Actually it cares about overflowing the unsigned long in loader_info, not that the address fits in 32 bits.

So it should be ULONG_MAX now (malloc and all the address code was changed to use unsigned long instead of unsigned int since the patch was written).

And dt_xlate needs the same information. Its is using a hardcoded 64 bit constant to provide the a simiar check.

Quoted 20 lines

Quoted 20 lines

arch/powerpc/boot/types.h +1 -0

+
 #define min(x,y) ({ \
 	typeof(x) _x = (x);	\
 	typeof(y) _y = (y);	\

Index: kernel/arch/powerpc/boot/devtree.c
===================================================================
--- kernel.orig/arch/powerpc/boot/devtree.c 2007-09-17
22:12:47.000000000 -0500
+++ kernel/arch/powerpc/boot/devtree.c 2007-09-17 22:12:51.000000000
-0500

arch/powerpc/boot/devtree.c +1 -0

@@ -1,6 +1,7 @@
 /*
  * devtree.c - convenience functions for device tree manipulation
  * Copyright 2007 David Gibson, IBM Corporation.
+ * Copyright 2007 Milton Miller, IBM Corporation.
  * Copyright (c) 2007 Freescale Semiconductor, Inc.
  *
  * Authors: David Gibson <[email protected]>

@@ -333,3 +334,68 @@ int dt_is_compatible(void *node, const c
 

Quoted 15 lines

Quoted 5 lines

arch/powerpc/boot/devtree.c +3 -0

 	return 0;
 }
+
+/**
+ * dt_find_initrd - set loader initrd location based on existing 

Quoted 8 lines

properties
+ *
+ * finds the linux,initrd-start and linux,initrd-end properties in
+ * the /chosen node and sets the loader initrd fields accordingly.
+ *
+ * Use this if your loader sets the properties to allow other code to
+ * relocate the tree and/or cause r3 and r4 to be set on true OF
+ * platforms.

I am unable to make sense of the paragraph above.

The phrase "relocate the tree" should be "relocate the initrd", which the wrapper will do if it located below vmlinux.size. Also, r3 and r4 need to be set when booting the kernel from a client interface with an initrd so it can take it into consideration when choosing the location to build the flat tree.

How about:

Filling in the loader info allows main.c to be aware of the initrd, meaning prep_initrd will move the initrd if it will be overwritten when the kernel is copied to its runtime location. In addition, if you are booting the kernel from a client interface instead of a flat device tree, this also causes r3 and r4 to be set so the kernel can avoid overwriting the initrd when creating the flat tree.

Quoted 12 lines

arch/powerpc/boot/devtree.c int dt_is_compatible(void *node, const c · line 354

Quoted 10 lines

+ */

arch/powerpc/boot/devtree.c +7 -0

+void dt_find_initrd(void)
+{
+	int rc;
+	unsigned long long initrd_start, initrd_end;
+	void *devp;
+	static const char start_prop[] = "linux,initrd-start";
+	static const char end_prop[] = "linux,initrd-end";

I think these constants are more obscuring than useful.

They are useful to the extent they reduce the number of source characters causing about 8 less line wraps. Since they are used multiple places, the compiler only needs to emit one copy of this byte sequence. Admitedly you made this point in a prior review.

Quoted 7 lines

arch/powerpc/boot/devtree.c int dt_is_compatible(void *node, const c · line 359

Quoted 5 lines

arch/powerpc/boot/devtree.c +5 -0

+
+	devp = finddevice("/chosen");
+	if (! devp) {
+		return;
+	}

CodingStyle would not put { } here.

Yea, nit. not sure why I have the braces there, I usually follow that one. And what's that space doing after !?

Quoted 15 lines

Quoted 11 lines

arch/powerpc/boot/devtree.c +10 -0

+
+	rc = getprop(devp, start_prop, &initrd_start, sizeof(initrd_start));
+	if (rc < 0)
+		return;				/* not found */
+	/* The properties had to be 8 bytes until 2.6.22 */
+	if (rc == sizeof(unsigned long)) {
+		unsigned long tmp;
+		memcpy(&tmp, &initrd_start, rc);
+		initrd_start = tmp;
+	} else if (rc != sizeof(initrd_start)) {	/* now they

Quoted 1 line

can be 4 */

Right. 8 bytes and 4 bytes, so you should be using explicit length types instead of long and long long.

Ok, I guess we ahve u32 and u64 in types.h now. But there is other code in the wrapper that assumes its compiled 32 bit ILP.

Quoted 7 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+		printf("unexpected length of %s in /chosen!\n\r", start_prop);
+		return;

All these printf() / return stanzas add a lot of verbosity to this function. Any way they can be consolidated a bit, maybe a single error path that just prints the property values, so the user can figure out what was wrong with them.

On a prior review I got asked to split the reasons for ingoring the values below (the > 32 bit address from end < start cases). Admitedly without all the detailed errors the justification for the string varables is reduced.

Quoted 45 lines · Milton Miller

Quoted 42 lines · David Gibson

Quoted 7 lines · Milton Miller

Some platforms have a boot agent that can create or modify properties in
the device-tree and load images into memory. Provide a helper to set loader_info used by prep_initrd().

Signed-off-by Milton Miller <[email protected]>

Acked-by David Gibson <[email protected]>

Hrm, despite my earlier ack, I'm going to whinge about a few nits here.

Quoted 30 lines

---
re 12168
rediffed types.h, offset in ops.h

Index: kernel/arch/powerpc/boot/ops.h
===================================================================
--- kernel.orig/arch/powerpc/boot/ops.h 2007-09-17
22:12:47.000000000
-0500
+++ kernel/arch/powerpc/boot/ops.h 2007-09-17 22:12:51.000000000
-0500

arch/powerpc/boot/ops.h +1 -0

@@ -163,6 +163,7 @@ void dt_fixup_clock(const char *path, u3
 void __dt_fixup_mac_addresses(u32 startindex, ...);
 #define dt_fixup_mac_addresses(...) \
 	__dt_fixup_mac_addresses(0, __VA_ARGS__, NULL)
+void dt_find_initrd(void);
 
 
 static inline void *find_node_by_linuxphandle(const u32 

linuxphandle)
Index: kernel/arch/powerpc/boot/types.h
===================================================================
--- kernel.orig/arch/powerpc/boot/types.h 2007-09-17
22:12:47.000000000 -0500
+++ kernel/arch/powerpc/boot/types.h 2007-09-17 22:12:51.000000000
-0500

arch/powerpc/boot/types.h +0 -0

@@ -12,6 +12,8 @@ typedef short			s16;
 typedef int			s32;
 typedef long long		s64;
 

Quoted 8 lines

Quoted 6 lines

arch/powerpc/boot/types.h typedef short s16; · line 15

Quoted 1 line

arch/powerpc/boot/types.h +1 -0

+#define UINT_MAX	0xFFFFFFFF

I actually don't like this constant - at the point you compare you care, explicitly, about the value not being over 32-bits, rather than whether it fits a uint, so the named constant is more misleading than helpful.

Arguable, I don't like counting F's or zeros in C code.

So check if (addr >> 32) is non-zero.

Quoted 2 lines

It is used as the max address that the wrapper deals with, both here and memranges, which happens to be 32 bits.

Right and the reasons for that being the value it is are not because it also hapeens to be the max uint *or* ulong.

Quoted 55 lines

Actually it cares about overflowing the unsigned long in loader_info, not that the address fits in 32 bits.

So it should be ULONG_MAX now (malloc and all the address code was changed to use unsigned long instead of unsigned int since the patch was written).

And dt_xlate needs the same information. Its is using a hardcoded 64 bit constant to provide the a simiar check.

Quoted 20 lines

Quoted 20 lines

arch/powerpc/boot/types.h +1 -0

+
 #define min(x,y) ({ \
 	typeof(x) _x = (x);	\
 	typeof(y) _y = (y);	\

Index: kernel/arch/powerpc/boot/devtree.c
===================================================================
--- kernel.orig/arch/powerpc/boot/devtree.c 2007-09-17
22:12:47.000000000 -0500
+++ kernel/arch/powerpc/boot/devtree.c 2007-09-17 22:12:51.000000000
-0500

arch/powerpc/boot/devtree.c +1 -0

@@ -1,6 +1,7 @@
 /*
  * devtree.c - convenience functions for device tree manipulation
  * Copyright 2007 David Gibson, IBM Corporation.
+ * Copyright 2007 Milton Miller, IBM Corporation.
  * Copyright (c) 2007 Freescale Semiconductor, Inc.
  *
  * Authors: David Gibson <[email protected]>

@@ -333,3 +334,68 @@ int dt_is_compatible(void *node, const c
 

Quoted 16 lines

Quoted 6 lines

arch/powerpc/boot/devtree.c +3 -0

 	return 0;
 }
+
+/**
+ * dt_find_initrd - set loader initrd location based on existing

properties

Quoted 8 lines

+ *
+ * finds the linux,initrd-start and linux,initrd-end properties in
+ * the /chosen node and sets the loader initrd fields accordingly.
+ *
+ * Use this if your loader sets the properties to allow other code to
+ * relocate the tree and/or cause r3 and r4 to be set on true OF
+ * platforms.

I am unable to make sense of the paragraph above.

The phrase "relocate the tree" should be "relocate the initrd", which the wrapper will do if it located below vmlinux.size. Also, r3 and r4 need to be set when booting the kernel from a client interface with an initrd so it can take it into consideration when choosing the location to build the flat tree.

Quoted 8 lines

How about:

Filling in the loader info allows main.c to be aware of the initrd, meaning prep_initrd will move the initrd if it will be overwritten when the kernel is copied to its runtime location. In addition, if you are booting the kernel from a client interface instead of a flat device tree, this also causes r3 and r4 to be set so the kernel can avoid overwriting the initrd when creating the flat tree.

Clearer, but I still don't see that it says anything useful - "finds the initrd from the devtree and does all the things that are supposed to be done with an initrd" - which is implied in the previous paragraph.

Quoted 11 lines

Quoted 10 lines

arch/powerpc/boot/devtree.c int dt_is_compatible(void *node, const c · line 354

Quoted 8 lines

+ */

arch/powerpc/boot/devtree.c +7 -0

+void dt_find_initrd(void)
+{
+	int rc;
+	unsigned long long initrd_start, initrd_end;
+	void *devp;
+	static const char start_prop[] = "linux,initrd-start";
+	static const char end_prop[] = "linux,initrd-end";

I think these constants are more obscuring than useful.

Quoted 4 lines

They are useful to the extent they reduce the number of source characters causing about 8 less line wraps. Since they are used multiple places, the compiler only needs to emit one copy of this byte sequence. Admitedly you made this point in a prior review.

The compiler is perfectly capable of folding identical string constants.

Quoted 28 lines

Quoted 7 lines

arch/powerpc/boot/devtree.c int dt_is_compatible(void *node, const c · line 359

Quoted 5 lines

arch/powerpc/boot/devtree.c +5 -0

+
+	devp = finddevice("/chosen");
+	if (! devp) {
+		return;
+	}

CodingStyle would not put { } here.

Yea, nit. not sure why I have the braces there, I usually follow that one. And what's that space doing after !?

Quoted 16 lines

Quoted 12 lines

(quoted diff) +2 -0

+
+	rc = getprop(devp, start_prop, &initrd_start, 

sizeof(initrd_start));

arch/powerpc/boot/devtree.c +8 -0

+	if (rc < 0)
+		return;				/* not found */
+	/* The properties had to be 8 bytes until 2.6.22 */
+	if (rc == sizeof(unsigned long)) {
+		unsigned long tmp;
+		memcpy(&tmp, &initrd_start, rc);
+		initrd_start = tmp;
+	} else if (rc != sizeof(initrd_start)) {	/* now they

Quoted 1 line

can be 4 */

Right. 8 bytes and 4 bytes, so you should be using explicit length types instead of long and long long.

Quoted 2 lines

Ok, I guess we ahve u32 and u64 in types.h now. But there is other code in the wrapper that assumes its compiled 32 bit ILP.

Yes, but that's no reason to propagate the sin.

Quoted 9 lines

Quoted 8 lines

Review

Quoted 3 lines

(quoted diff) +2 -0

+		printf("unexpected length of %s in /chosen!\n\r", start_prop);
+		return;

All these printf() / return stanzas add a lot of verbosity to this function. Any way they can be consolidated a bit, maybe a single error path that just prints the property values, so the user can figure out what was wrong with them.

Quoted 4 lines

On a prior review I got asked to split the reasons for ingoring the values below (the > 32 bit address from end < start cases). Admitedly without all the detailed errors the justification for the string varables is reduced.

Hrm. Well, I can't say I'm entirely convinced either way on this one. I guess I'll think about it again once the rest is cleaned up.

Record the number of header bytes skipped in the total bytes read field.

This is needed for the initramfs parsing code to find the end of the zip file.

Signed-off-by Milton Miller <[email protected]>

Add a set of library routines to manage gross memory allocations.

This code uses an array in bss to store upto 32 entrys with merging representing a range of memory below rma_end (aka end of real mode memory at 0).

To use this code, a platform would set rma_end (find_rma_end), mark memory ranges occupied (add_known_ranges et al), initialize malloc in the spaces between (ranges_init_malloc), and optionally use the supplied vmlinux_alloc may be used.

Signed-off-by Milton Miller <[email protected]>

---
vs 12172
rename rmo_end to rma_end (real mode area, as used in papr) removed section labels (now in ops.h)
rediff ops.h, Makefile
moved find_rma_end here (from kexec.c in a later patch) find_rma_end searches by node type for "memory", checks that
the parent is the root node, then looks for a reg property with the first address/size pair starting at 0.

Quoted 6 lines · Milton Miller

Add a set of library routines to manage gross memory allocations.

This code uses an array in bss to store upto 32 entrys with merging representing a range of memory below rma_end (aka end of real mode memory at 0).

Quoted 4 lines

To use this code, a platform would set rma_end (find_rma_end), mark memory ranges occupied (add_known_ranges et al), initialize malloc in the spaces between (ranges_init_malloc), and optionally use the supplied vmlinux_alloc may be used.

Urg. It's an awful lot of code for the bootwrapper. Am I right in understanding that the only reason to use the ranges code is for the ranges based malloc() and vmlinux_alloc() you get out of it?

Add code to check if the processor is in 64 or 32 bit mode using only instructions from the 32 bit subset. If the processor is in 64 bit mode, switch to 32 bit mode by clearing MSR[SF].

Also add a 64 bit procedure descriptor to use as a elf64 entry point.

Signed-off-by Milton Miller <[email protected]>

---
vs 12173
correct comment (equal vs not-equal)

Index: kernel/arch/powerpc/boot/crt0.S
===================================================================

arch/powerpc/boot/crt0.S +32 -0

+	.long	0, _zimage_start	/* big endian, supported reloc ppc32 */
+	.long	0, 0, 0, 0, 0, 0
+
+
 	.weak	_zimage_start
 	.globl	_zimage_start
 _zimage_start:
 	.globl	_zimage_start_lib
 _zimage_start_lib:
+	/* Check if the processor is running in 32 bit mode, using
+	 * only 32 bit instructions which should be safe on 32 and
+	 * 64 bit processors.
+	 *
+	 * Subtract bottom 32 bits of MSR from full value recording
+	 * the result.  Since MSR[SF] is in the high word, we will
+	 * be equal iff in 32 bit mode (either the processor is
+	 * a 32 bit processor or MSR[SF] = 0).
+	 */
+	mfmsr	r0		/* grab whole msr		*/
+	rlwinm	r8,r0,0,0,31	/* extract bottom word		*/
+	subf.	r8,r8,r0	/* subtract, same?		*/
+	beq	0f		/* yes: we are 32 bit mode	*/
+
+	/* We are in 64-bit mode.  This program must run in 32 bit
+	 * mode.  Assume we are actually running somewhere in the
+	 * low 32 bits of the address space, so we can just turn
+	 * off MSR[SF] which is bit 0.
+	 */
+	.machine push
+	.machine "ppc64"
+	rldicl	r0,r0,0,1
+	sync
+	mtmsrd	r0
+	isync
+	.machine pop
+0:	/* We are now in 32-bit mode */
+
 	/* Work out the offset between the address we were linked at
 	   and the address where we're running. */
 	bl	1f

_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev

This code creates a 32 bit zImage wrapper for a 32 or 64 bit PowerPC Linux kernel. This allows you to kexec a zImage with its compressed vmlinux instead of the uncompressed vmlinux elf. The code is also packaged as a 64 bit elf for use by kexec-tools on 64 bit kernels.

Limitations:
Note: the device-tree generated by kexec-tools is currently version 2, not the version 16 supported by the boot code base.

Signed-off-by Milton Miller <[email protected]>

---
find_rmo_end moved to memranges as find_rma_end. early_scan_flat_tree replaced with calls to its pieces in kexec_platform_init
rediff wrapper, Makefile, ops.h

Index: kernel/arch/powerpc/boot/kexec.c
===================================================================

arch/powerpc/boot/kexec.c new file +18 -0

@@ -0,0 +1,122 @@
+/*
+ * 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 protected]>

arch/powerpc/boot/kexec.c new file +43 -0

+}
+
+/**
+ * setup_initial_heap - setup a small heap in the bss
+ * Using a preallocated heap, setup for scanning the device tree.
+ * Intended for the initial read while the tree will remain read-only so
+ * a minimal malloc and search limit can be used.  This way we don't have
+ * lots of data or bss to clear.
+ */
+static void setup_initial_heap(void)
+{
+	static char initial_heap[8*1024];
+	void *heap_end;
+
+	heap_end = simple_alloc_init(initial_heap,
+			sizeof(initial_heap) * 7 / 8,
+			sizeof(long), 64);
+
+	if (heap_end - sizeof(initial_heap) > (void *)&initial_heap[0])
+		fatal("Initial heap too big\n\r");
+}
+
+static void init_flat_tree(struct boot_param_header *dt_blob)
+{
+	int rc;
+
+	rc = ft_init(dt_blob, dt_blob->totalsize, /* max_finddevice */ 1024);
+	if (rc)
+		fatal("Unable to initialize device_tree library!\n\r");
+}
+
+static void *saved_vmlinux_addr;
+
+static void *kexec_vmlinux_alloc(unsigned long size)
+{
+	void *addr;
+
+	addr = ranges_vmlinux_alloc(size);
+
+	saved_vmlinux_addr = addr;
+	return addr;
+}
+

arch/powerpc/boot/kexec.c new file +31 -0

+
+	return finalize_chain();
+}
+
+void kexec_platform_init(struct boot_param_header *dt_blob)
+{
+	slaves_are_low();
+	move_slaves_up();
+
+	setup_initial_heap();
+	init_flat_tree(dt_blob);
+	/*
+	 * drivers can malloc and read the tree, but not realloc later
+	 * or modify the tree now.
+	 */
+	if (!console_ops.write)
+		find_console_from_tree();
+
+	find_rma_end();
+	dt_find_initrd();
+	add_known_ranges(dt_blob);
+	ranges_init_malloc();
+
+	/* now that we have a malloc region, start over from the flat tree */
+	init_flat_tree(dt_blob);
+
+	platform_ops.vmlinux_alloc = kexec_vmlinux_alloc;
+	platform_ops.fixups = kexec_fixups;
+	finalize_chain = dt_ops.finalize;
+	dt_ops.finalize = kexec_finalize;
+}

Index: kernel/arch/powerpc/boot/crt0_kexec.S
===================================================================

arch/powerpc/boot/crt0_kexec.S new file +45 -0

@@ -0,0 +1,45 @@
+/*
+ * 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 2007 IBM Corporation.
+ *
+ * Authors: Milton Miller <[email protected]>
+ *
+ */
+	/*
+	 * The kernel calls out to the first image with
+	 * r3 = boot cpu, r4 = entrypoint, r5 = 0
+	 *
+	 * kexec-tools purgatory calls this as it would a linux kernel:
+	 * r3 = boot block, r4 = entrypoint, r5 = 0
+	 *
+	 * The boot block boot_cpu field has been filled in.
+	 *
+	 * kexec-tools and its purgatory are suppposed to copy SMP_SLAVE_SIZE
+	 * bytes from the from entry point, but aparently instead it copies
+	 * from the image start.
+	 */
+	.globl _zimage_start
+_zimage_start:
+
+#include "marshal_low.S"
+
+	.globl	_zimage_start_plat
+_zimage_start_plat:
+	b	_zimage_start_lib
+
+	.globl	platform_init
+platform_init:
+	b	kexec_platform_init

Index: kernel/arch/powerpc/boot/wrapper
===================================================================

arch/powerpc/boot/wrapper +4 -0

+    ;;
 miboot|uboot)
     # miboot and U-boot want just the bare bits, not an ELF binary
     ext=bin

@@ -190,6 +196,9 @@ uboot)
     fi
     exit 0
     ;;
+kexec64)
+    ${CROSS}objcopy -O elf64-powerpc $ofile
+    ;;
 esac
 
 addsec() {

Index: kernel/arch/powerpc/boot/Makefile
===================================================================

arch/powerpc/boot/Makefile +8 -4

@@ -42,12 +42,12 @@ $(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 \
+		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 \
 		cpm-serial.c stdlib.c
-src-plat := of.c cuboot-83xx.c cuboot-85xx.c holly.c \
+src-plat := of.c cuboot-83xx.c cuboot-85xx.c holly.c crt0_kexec.S \
 		cuboot-ebony.c treeboot-ebony.c prpmc2800.c \
 		ps3-head.S ps3-hvcall.S ps3.c treeboot-bamboo.c cuboot-8xx.c \
 		cuboot-pq2.c cuboot-sequoia.c treeboot-walnut.c cuboot-bamboo.c

@@ -128,6 +128,9 @@ quiet_cmd_wrap	= WRAP    $@
       cmd_wrap	=$(CONFIG_SHELL) $(wrapper) -c -o $@ -p $2 $(CROSSWRAP) \
 		$(if $3, -s $3)$(if $4, -d $4)$(if $5, -i $5) vmlinux
 
+kexec-$(CONFIG_PPC32)			+= zImage.kexec
+kexec-$(CONFIG_PPC64)			+= zImage.kexec64
+
 image-$(CONFIG_PPC_PSERIES)		+= zImage.pseries
 image-$(CONFIG_PPC_MAPLE)		+= zImage.pseries
 image-$(CONFIG_PPC_IBM_CELL_BLADE)	+= zImage.pseries

@@ -140,6 +143,7 @@ image-$(CONFIG_PPC_HOLLY)		+= zImage.hol
 image-$(CONFIG_PPC_PRPMC2800)		+= zImage.prpmc2800
 image-$(CONFIG_PPC_ISERIES)		+= zImage.iseries
 image-$(CONFIG_DEFAULT_UIMAGE)		+= uImage
+image-$(CONFIG_KEXEC)			+= $(kexec-y)
 
 ifneq ($(CONFIG_DEVICE_TREE),"")
 image-$(CONFIG_PPC_8xx)			+= cuImage.8xx

@@ -158,7 +162,7 @@ ifeq ($(CONFIG_PPC32),y)
 image-$(CONFIG_PPC_PMAC)	+= zImage.coff zImage.miboot
 endif
 
-initrd-  := $(patsubst zImage%, zImage.initrd%, $(image-n) $(image-))
+initrd-  := $(patsubst zImage%, zImage.initrd%, $(image-n) $(image-) $(kexec-))
 initrd-y := $(patsubst zImage%, zImage.initrd%, \
 		$(patsubst treeImage%, treeImage.initrd%, $(image-y)))
 initrd-y := $(filter-out $(image-y), $(initrd-y))

@@ -227,7 +231,7 @@ install: $(CONFIGURE) $(addprefix $(obj)
 
 # anything not in $(targets)
 clean-files += $(image-) $(initrd-) zImage zImage.initrd cuImage.* treeImage.* \
-	otheros.bld
+	otheros.bld $(kexec-)
 
 # clean up files cached by wrapper
 clean-kernel := vmlinux.strip vmlinux.bin

Index: kernel/arch/powerpc/boot/ops.h
===================================================================

arch/powerpc/boot/ops.h +0 -0

 int dt_xlate_reg(void *node, int res, unsigned long *addr, unsigned long *size);
 int dt_xlate_addr(void *node, u32 *buf, int buflen, unsigned long *xlated_addr);

_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev

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 protected]>

---
vs 12175
Rediffed Makefile, ops, kexec.c

Index: kernel/arch/powerpc/boot/flatdevtree_conv.c
===================================================================

arch/powerpc/boot/flatdevtree_conv.c new file +15 -0

@@ -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.
+ *

arch/powerpc/boot/flatdevtree_conv.c new file +5 -0

+#define OUT_COMPAT 16
+
+#ifdef NO_CHECK
+static int check_v123_tree(u32 *start, u32 *limit)
+{

arch/powerpc/boot/flatdevtree_conv.c new file +92 -0

+{
+	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;
+		}

arch/powerpc/boot/flatdevtree_conv.c new file +36 -0

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

arch/powerpc/boot/flatdevtree_conv.c new file +29 -0

+
+	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"

arch/powerpc/boot/flatdevtree_conv.c new file +50 -0

+	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
===================================================================

arch/powerpc/boot/Makefile +1 -1

@@ -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
===================================================================

arch/powerpc/boot/ops.h +3 -0

@@ -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
===================================================================

arch/powerpc/boot/kexec.c +1 -0

@@ -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 protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Quoted 42 lines · Milton Miller

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 protected]>

---
vs 12175
Rediffed Makefile, ops, kexec.c

Index: kernel/arch/powerpc/boot/flatdevtree_conv.c
===================================================================

arch/powerpc/boot/flatdevtree_conv.c new file +0 -0

17:49:04.000000000 -0500

arch/powerpc/boot/flatdevtree_conv.c new file +2 -0

@@ -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 protected]>
+ */

arch/powerpc/boot/flatdevtree_conv.c new file +2 -0

+#include "flatdevtree.h"
+#include "stdio.h"

arch/powerpc/boot/flatdevtree_conv.c line 25

Quoted 4 lines

arch/powerpc/boot/flatdevtree_conv.c new file +4 -0

+#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.

Quoted 11 lines

arch/powerpc/boot/flatdevtree_conv.c new file +10 -0

+#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

Quoted 5 lines

+ * @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?

Quoted 100 lines

arch/powerpc/boot/flatdevtree_conv.c new file +42 -0

+{
+	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.
+ */

arch/powerpc/boot/flatdevtree_conv.c new file +55 -0

+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

Quoted 4 lines

+ * @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.

Quoted 41 lines

arch/powerpc/boot/flatdevtree_conv.c new file +40 -0

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

arch/powerpc/boot/flatdevtree_conv.c line 188

Quoted 6 lines

arch/powerpc/boot/flatdevtree_conv.c new file +6 -0

+	u32 *dtp;
+	u32 need = 0, nops;
+	int slen;
+
+	if (tree->magic != OF_DT_HEADER)
+		fatal("%s: no magic", __FUNCTION__);

More pointless tests...

arch/powerpc/boot/flatdevtree_conv.c line 191

Quoted 3 lines

arch/powerpc/boot/flatdevtree_conv.c new file +2 -0

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

Rather, don't need to do anything.

Quoted 36 lines

arch/powerpc/boot/flatdevtree_conv.c new file +35 -0

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

arch/powerpc/boot/flatdevtree_conv.c line 230

Quoted 4 lines

arch/powerpc/boot/flatdevtree_conv.c new file +4 -0

+	/*
+	 * 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.

Quoted 105 lines

arch/powerpc/boot/flatdevtree_conv.c new file +50 -0

+	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

arch/powerpc/boot/flatdevtree_conv.c new file +1 -1

@@ -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 \

arch/powerpc/boot/flatdevtree_conv.c new file +0 -0

 		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

arch/powerpc/boot/flatdevtree_conv.c new file +3 -0

@@ -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

arch/powerpc/boot/flatdevtree_conv.c new file +1 -0

@@ -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 protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Signature · 4 lines

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

Quoted 1 line · David Gibson

On Fri, Sep 21, 2007 at 06:05:06PM -0500, Milton Miller wrote:

Quoted 6 lines

arch/powerpc/boot/flatdevtree_conv.c line 25

Quoted 2 lines

arch/powerpc/boot/flatdevtree_conv.c new file +2 -0

+#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.

Quoted 10 lines

Quoted 10 lines

arch/powerpc/boot/flatdevtree_conv.c new file +10 -0

+#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

Quoted 8 lines

Quoted 5 lines

+ * @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.

Quoted 11 lines

arch/powerpc/boot/flatdevtree_conv.c line 142

Quoted 7 lines

arch/powerpc/boot/flatdevtree_conv.c new file +7 -0

+#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).

Quoted 4 lines

arch/powerpc/boot/flatdevtree_conv.c line 191

Quoted 2 lines

arch/powerpc/boot/flatdevtree_conv.c new file +2 -0

+	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 :-).

Quoted 8 lines

arch/powerpc/boot/flatdevtree_conv.c line 230

Quoted 4 lines

arch/powerpc/boot/flatdevtree_conv.c new file +4 -0

+	/*
+	 * 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.

Quoted 2 lines · Milton Miller

Quoted 1 line · David Gibson

On Fri, Sep 21, 2007 at 06:05:06PM -0500, Milton Miller wrote:

[snip]

Quoted 7 lines

Quoted 6 lines

arch/powerpc/boot/flatdevtree_conv.c line 25

Quoted 2 lines

arch/powerpc/boot/flatdevtree_conv.c new file +2 -0

+#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.

Quoted 2 lines

Outputing v17 instead of 16 requires more words to be added to the header, and the library does fine with v16.

For now. libfdt will want v17. Although it will (eventually) have it's own v16->v17 conversion code.

Quoted 4 lines

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.

!? what's your source for this. v2 and v3 were absolutely supposed to be backwards compatible with v1 which would not be the case with silent semantic changes such as this.

Quoted 3 lines

Actually,
when converting v3 to v16 some of the code issn't needed, the ifs allow the code size to be reduced.

Yes, but it never will be, because the only reason we'd include this file is for converting old kexec-tools device trees which are v2 not v3.

Quoted 19 lines

Quoted 18 lines

Quoted 5 lines

arch/powerpc/boot/flatdevtree_conv.c new file +5 -0

+#define OUT_COMPAT 16
+
+#ifdef NO_CHECK
+static int check_v123_tree(u32 *start, u32 *limit)
+{

arch/powerpc/boot/flatdevtree_conv.c line 40

Quoted 10 lines

arch/powerpc/boot/flatdevtree_conv.c new file +10 -0

+	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?

Quoted 6 lines

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.

Right. Debugging code shouldn't pollute final patches.

[snip]

Quoted 12 lines

Quoted 11 lines

arch/powerpc/boot/flatdevtree_conv.c line 142

Quoted 7 lines

arch/powerpc/boot/flatdevtree_conv.c new file +7 -0

+#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.

Quoted 8 lines

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

This is a hell of a lot of bother to go to for a few bytes. Allocating a big-enough buffer for any reasonable devtree in BSS and memmove()ing into there would be far simpler.

[snip]

Quoted 5 lines

Quoted 4 lines

arch/powerpc/boot/flatdevtree_conv.c line 191

Quoted 2 lines

arch/powerpc/boot/flatdevtree_conv.c new file +2 -0

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

Rather, don't need to do anything.

Quoted 6 lines

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 :-).

There will never be a v4. Or anything between v3 and v16.

[snip]

Quoted 14 lines

Quoted 8 lines

arch/powerpc/boot/flatdevtree_conv.c line 230

Quoted 4 lines

arch/powerpc/boot/flatdevtree_conv.c new file +4 -0

+	/*
+	 * 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.

Quoted 8 lines

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.

Hrm. Yes, well, the iSeries tree is weird in a bunch of ways.

Quoted 4 lines

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.

Again. We don't need to deal with the general case here - just the real case of trees produced by old kexec-tools.

Quoted 14 lines · David Gibson

Quoted 2 lines · Milton Miller

Quoted 1 line · David Gibson

On Fri, Sep 21, 2007 at 06:05:06PM -0500, Milton Miller wrote:

[snip]

Quoted 9 lines

Quoted 6 lines

arch/powerpc/boot/flatdevtree_conv.c line 25

Quoted 2 lines

arch/powerpc/boot/flatdevtree_conv.c new file +2 -0

+#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.

Quoted 2 lines

For now. libfdt will want v17. Although it will (eventually) have it's own v16->v17 conversion code.

If libfdt gets merged without supporting v16 input, then that will be a
regression. This code isn't pretending to address that. The current
flat tree code works with v16.

Quoted 6 lines

Quoted 5 lines

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.

Quoted 3 lines

!? what's your source for this. v2 and v3 were absolutely supposed to be backwards compatible with v1 which would not be the case with silent semantic changes such as this.

What's your souce for saying the were supposed to be backwards compatable? That dtc fills out the struct header so?

My source is my involvment when v2 was defined (they were discovered while writing my device tree generation code):

The actual binary structure is compatable, just not the contents of the properties nor how any slave cpus wait (for some trees it doesn't matter).

http://git.kernel.org/?p=linux/kernel/git/horms/kexec-tools-
testing.git;a=blob;f=kexec/arch/ppc64/fs2dt.c;
hb=b84b87747a16f0afbef6f6802bb794a94f4961d9

And some more changes just before that:

http://git.kernel.org/?p=linux/kernel/git/torvalds/old-2.6-bkcvs.git;
a=history;f=arch/ppc64/kernel/prom_init.c;
h=e570799a84cc5328e9f0fd44592cb0b828d8c13a;
hb=4ae24c4e8a8f68950a7774ca1cdfe69bfe4e2ffc

So its mostly when the kernel generated and required v1 trees, it was ppc64 only and had these other content and handoff semantics. If it were to get a v1 tree, it only copes for the boot cpu determination I'm not aware of any code other than the kernel that would actually generate a v1 tree (other than dtc, which always supporteed v2, and doesn't care about these differences).

Quoted 14 lines · Milton Miller

Quoted 13 lines · David Gibson

Quoted 2 lines · Milton Miller

Quoted 1 line · David Gibson

On Fri, Sep 21, 2007 at 06:05:06PM -0500, Milton Miller wrote:

[snip]

Quoted 8 lines

Quoted 2 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+#define MIN_VERSION 2
+#define OUT_VERSION 16z

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.

Quoted 6 lines

Quoted 3 lines

!? what's your source for this. v2 and v3 were absolutely supposed to be backwards compatible with v1 which would not be the case with silent semantic changes such as this.

What's your souce for saying the were supposed to be backwards compatable? That dtc fills out the struct header so?

Sitting next to BenH and knowing he always intended them to be so.

Quoted 7 lines

My source is my involvment when v2 was defined (they were discovered while writing my device tree generation code):

The actual binary structure is compatable, just not the contents of the properties nor how any slave cpus wait (for some trees it doesn't matter).

Quoted 3 lines

http://git.kernel.org/?p=linux/kernel/git/horms/kexec-tools-
testing.git;a=blob;f=kexec/arch/ppc64/fs2dt.c;
hb=b84b87747a16f0afbef6f6802bb794a94f4961d9

An old version of fs2dt is hardly definitive. It could just be Plain Wrong, nothing to do with the dt version.

Quoted 7 lines

And some more changes just before that:

http://git.kernel.org/?p=linux/kernel/git/torvalds/old-2.6-bkcvs.git;
a=history;f=arch/ppc64/kernel/prom_init.c;
h=e570799a84cc5328e9f0fd44592cb0b828d8c13a;
hb=4ae24c4e8a8f68950a7774ca1cdfe69bfe4e2ffc

I don't know what bit you're referring to in that batch of commits.