[PATCH 1/2] Add comp_copy function for use with CONFIG_IMAGE_COMPRESSION_NONE

[PATCH 1/2] Add comp_copy function for use with CONFIG_IMAGE_COMPRESSION_NONE

[Sascha Hauer]

The Makefile compression commands all append the size of the
uncompressed image. With CONFIG_IMAGE_COMPRESSION_NONE simply
'shipped' is used which does not append the size. Add and use
a special comp_copy function which adds the size. This helps
us to get the uncompressed image size in the startup code later.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/pbl/Makefile | 2 +-
 images/Makefile       | 2 +-
 scripts/Makefile.lib  | 8 ++++++++
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/arm/pbl/Makefile b/arch/arm/pbl/Makefile
index 1ff39db..c455112 100644
--- a/arch/arm/pbl/Makefile
+++ b/arch/arm/pbl/Makefile
@@ -3,7 +3,7 @@ suffix_$(CONFIG_IMAGE_COMPRESSION_GZIP) = gzip
 suffix_$(CONFIG_IMAGE_COMPRESSION_LZO)	= lzo
 suffix_$(CONFIG_IMAGE_COMPRESSION_LZ4)	= lz4
 suffix_$(CONFIG_IMAGE_COMPRESSION_XZKERN)	= xzkern
-suffix_$(CONFIG_IMAGE_COMPRESSION_NONE)	= shipped
+suffix_$(CONFIG_IMAGE_COMPRESSION_NONE)	= comp_copy
 
 OBJCOPYFLAGS_zbarebox.bin = -O binary
 piggy_o := piggy.$(suffix_y).o
diff --git a/images/Makefile b/images/Makefile
index da9cc8d..0537af1 100644
--- a/images/Makefile
+++ b/images/Makefile
@@ -85,7 +85,7 @@ suffix_$(CONFIG_IMAGE_COMPRESSION_GZIP) = gzip
 suffix_$(CONFIG_IMAGE_COMPRESSION_LZO)  = lzo
 suffix_$(CONFIG_IMAGE_COMPRESSION_LZ4)	= lz4
 suffix_$(CONFIG_IMAGE_COMPRESSION_XZKERN) = xzkern
-suffix_$(CONFIG_IMAGE_COMPRESSION_NONE) = shipped
+suffix_$(CONFIG_IMAGE_COMPRESSION_NONE) = comp_copy
 
 # barebox.z - compressed barebox binary
 # ----------------------------------------------------------------
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index e55bc27..e79998c 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -380,6 +380,14 @@ cmd_lz4 = (cat $(filter-out FORCE,$^) | \
 %.lz4: %
 	$(call if_changed,lz4)
 
+# comp_copy
+# ---------------------------------------------------------------------------
+# Wrapper which only copies a file, but compatible to the compression
+# functions above. Appends the size to the result file
+quiet_cmd_comp_copy ?= SHIPPED_S $@
+cmd_comp_copy ?= cat $(filter-out FORCE,$^) > $@; \
+                 $(call size_append, $(filter-out FORCE,$^)) >> $@
+
 quiet_cmd_disasm = DISASM  $@
 cmd_disasm = $(OBJDUMP) -d $< > $@
 
-- 
2.8.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/2] ARM: Fix calling of arm_mem_barebox_image()

[Sascha Hauer]

arm_mem_barebox_image() is used to pick a suitable place where to
put the final image to. This is called from both the PBL uncompression
code and also from the final image. To make it work properly it is
crucial that it's called with the same arguments both times. Currently
it is called with the wrong image size from the PBL uncompression code.
The size passed to arm_mem_barebox_image() has to be the size of the
whole uncompressed image including the BSS segment size. The PBL code
calls it with the compressed image size instead and without the BSS
segment. This patch fixes this by reading the uncompressed image size
from the compressed binary (the uncompressed size is appended to the
end of the compressed binary by our compression wrappers). The size
of the BSS segment is unknown though by the PBL uncompression code,
so we introduce a maximum BSS size which is used instead.

The code before this patch worked by accident because the base address
of the final image was aligned down to a 1MiB boundary. The alignment
was sufficient already to make enough space. This breaks though when
the uncompressed image including BSS becomes bigger than 1MiB while
the compressed image is smaller.

Fixes: 65071bd0: arm: Clarify memory layout calculation

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/cpu/start-pbl.c           |  6 ++++--
 arch/arm/cpu/start.c               |  3 +--
 arch/arm/cpu/uncompress.c          | 10 ++++++----
 arch/arm/include/asm/barebox-arm.h |  9 ++++++++-
 4 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/arch/arm/cpu/start-pbl.c b/arch/arm/cpu/start-pbl.c
index f723edc..5f1469b 100644
--- a/arch/arm/cpu/start-pbl.c
+++ b/arch/arm/cpu/start-pbl.c
@@ -28,6 +28,7 @@
 #include <asm/sections.h>
 #include <asm/pgtable.h>
 #include <asm/cache.h>
+#include <asm/unaligned.h>
 
 #include "mmu-early.h"
 
@@ -49,7 +50,7 @@ __noreturn void barebox_single_pbl_start(unsigned long membase,
 		unsigned long memsize, void *boarddata)
 {
 	uint32_t offset;
-	uint32_t pg_start, pg_end, pg_len;
+	uint32_t pg_start, pg_end, pg_len, uncompressed_len;
 	void __noreturn (*barebox)(unsigned long, unsigned long, void *);
 	uint32_t endmem = membase + memsize;
 	unsigned long barebox_base;
@@ -63,9 +64,10 @@ __noreturn void barebox_single_pbl_start(unsigned long membase,
 	pg_start = (uint32_t)&input_data - offset;
 	pg_end = (uint32_t)&input_data_end - offset;
 	pg_len = pg_end - pg_start;
+	uncompressed_len = get_unaligned((const u32 *)(pg_start + pg_len - 4));
 
 	if (IS_ENABLED(CONFIG_RELOCATABLE))
-		barebox_base = arm_mem_barebox_image(membase, endmem, pg_len);
+		barebox_base = arm_mem_barebox_image(membase, endmem, uncompressed_len + MAX_BSS_SIZE);
 	else
 		barebox_base = TEXT_BASE;
 
diff --git a/arch/arm/cpu/start.c b/arch/arm/cpu/start.c
index f25e592..0120117 100644
--- a/arch/arm/cpu/start.c
+++ b/arch/arm/cpu/start.c
@@ -143,8 +143,7 @@ __noreturn void barebox_non_pbl_start(unsigned long membase,
 {
 	unsigned long endmem = membase + memsize;
 	unsigned long malloc_start, malloc_end;
-	unsigned long barebox_size = barebox_image_size +
-		((unsigned long)&__bss_stop - (unsigned long)&__bss_start);
+	unsigned long barebox_size = barebox_image_size + MAX_BSS_SIZE;
 
 	if (IS_ENABLED(CONFIG_RELOCATABLE)) {
 		unsigned long barebox_base = arm_mem_barebox_image(membase,
diff --git a/arch/arm/cpu/uncompress.c b/arch/arm/cpu/uncompress.c
index b8e2e9f..eeb5a65 100644
--- a/arch/arm/cpu/uncompress.c
+++ b/arch/arm/cpu/uncompress.c
@@ -29,6 +29,7 @@
 #include <asm/sections.h>
 #include <asm/pgtable.h>
 #include <asm/cache.h>
+#include <asm/unaligned.h>
 
 #include <debug_ll.h>
 
@@ -44,7 +45,7 @@ static int __attribute__((__used__))
 void __noreturn barebox_multi_pbl_start(unsigned long membase,
 		unsigned long memsize, void *boarddata)
 {
-	uint32_t pg_len;
+	uint32_t pg_len, uncompressed_len;
 	void __noreturn (*barebox)(unsigned long, unsigned long, void *);
 	uint32_t endmem = membase + memsize;
 	unsigned long barebox_base;
@@ -72,10 +73,11 @@ void __noreturn barebox_multi_pbl_start(unsigned long membase,
 	 */
 	pg_start = image_end + 1;
 	pg_len = *(image_end);
+	uncompressed_len = get_unaligned((const u32 *)(pg_start + pg_len - 4));
 
 	if (IS_ENABLED(CONFIG_RELOCATABLE))
 		barebox_base = arm_mem_barebox_image(membase, endmem,
-						     pg_len);
+						     uncompressed_len + MAX_BSS_SIZE);
 	else
 		barebox_base = TEXT_BASE;
 
@@ -92,8 +94,8 @@ void __noreturn barebox_multi_pbl_start(unsigned long membase,
 	free_mem_ptr = arm_mem_early_malloc(membase, endmem);
 	free_mem_end_ptr = arm_mem_early_malloc_end(membase, endmem);
 
-	pr_debug("uncompressing barebox binary at 0x%p (size 0x%08x) to 0x%08lx\n",
-			pg_start, pg_len, barebox_base);
+	pr_debug("uncompressing barebox binary at 0x%p (size 0x%08x) to 0x%08lx (uncompressed size: 0x%08x)\n",
+			pg_start, pg_len, barebox_base, uncompressed_len);
 
 	pbl_barebox_uncompress((void*)barebox_base, pg_start, pg_len);
 
diff --git a/arch/arm/include/asm/barebox-arm.h b/arch/arm/include/asm/barebox-arm.h
index 0acdfa3..061296a 100644
--- a/arch/arm/include/asm/barebox-arm.h
+++ b/arch/arm/include/asm/barebox-arm.h
@@ -164,6 +164,13 @@ static inline unsigned long arm_mem_barebox_image(unsigned long membase,
 		static void __naked noinline __##name			\
 			(uint32_t arg0, uint32_t arg1, uint32_t arg2)
 
-
+/*
+ * When using compressed images in conjunction with relocatable images
+ * the PBL code must pick a suitable place where to uncompress the barebox
+ * image. For doing this the PBL code must know the size of the final
+ * image including the BSS segment. The BSS size is unknown to the PBL
+ * code, so define a maximum BSS size here.
+ */
+#define MAX_BSS_SIZE SZ_1M
 
 #endif	/* _BAREBOX_ARM_H_ */
-- 
2.8.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] ARM: Fix calling of arm_mem_barebox_image()

[Trent Piepho]

On Wed, 2016-09-14 at 10:21 +0200, Sascha Hauer wrote:
> arm_mem_barebox_image() is used to pick a suitable place where to
> put the final image to. This is called from both the PBL uncompression
> code and also from the final image. To make it work properly it is
> crucial that it's called with the same arguments both times. Currently

This code has changed since I was working with it, but wouldn't
arm_mem_barebox_image() returning a different value from when the PBL
code calls it versus barebox_non_pbl_start() just result in an
unnecessary relocation of the uncompressed barebox from the PBL's choice
to the main barebox choice?


> it is called with the wrong image size from the PBL uncompression code.
> The size passed to arm_mem_barebox_image() has to be the size of the
> whole uncompressed image including the BSS segment size. The PBL code
> calls it with the compressed image size instead and without the BSS
> segment. This patch fixes this by reading the uncompressed image size
> from the compressed binary (the uncompressed size is appended to the
> end of the compressed binary by our compression wrappers). The size
> of the BSS segment is unknown though by the PBL uncompression code,
> so we introduce a maximum BSS size which is used instead.

Could the size including BSS be appended?  Seems like putting:
size -A barebox | awk '$1==".bss"{print $2}'
in the size_append function would do that pretty simply.


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] ARM: Fix calling of arm_mem_barebox_image()

[Sascha Hauer]

On Wed, Sep 14, 2016 at 06:27:04PM +0000, Trent Piepho wrote:
> On Wed, 2016-09-14 at 10:21 +0200, Sascha Hauer wrote:
> > arm_mem_barebox_image() is used to pick a suitable place where to
> > put the final image to. This is called from both the PBL uncompression
> > code and also from the final image. To make it work properly it is
> > crucial that it's called with the same arguments both times. Currently
> 
> This code has changed since I was working with it, but wouldn't
> arm_mem_barebox_image() returning a different value from when the PBL
> code calls it versus barebox_non_pbl_start() just result in an
> unnecessary relocation of the uncompressed barebox from the PBL's choice
> to the main barebox choice?

That may work when both regions do not overlap.

> 
> 
> > it is called with the wrong image size from the PBL uncompression code.
> > The size passed to arm_mem_barebox_image() has to be the size of the
> > whole uncompressed image including the BSS segment size. The PBL code
> > calls it with the compressed image size instead and without the BSS
> > segment. This patch fixes this by reading the uncompressed image size
> > from the compressed binary (the uncompressed size is appended to the
> > end of the compressed binary by our compression wrappers). The size
> > of the BSS segment is unknown though by the PBL uncompression code,
> > so we introduce a maximum BSS size which is used instead.
> 
> Could the size including BSS be appended?  Seems like putting:
> size -A barebox | awk '$1==".bss"{print $2}'
> in the size_append function would do that pretty simply.

I already tried. Somehow I didn't like the result that much, see the
patch below. The patch also still misses the single pbl handling.

Sascha

--------------------8<--------------------

From ea6cf2609ee1285cf415f28ded2317f31ddec7b2 Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Tue, 12 Jul 2016 12:45:42 +0200
Subject: [PATCH] wip

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/cpu/start.c         | 19 +++++++++----------
 arch/arm/cpu/uncompress.c    | 11 +++++++++--
 arch/arm/lib32/barebox.lds.S |  1 +
 images/Makefile              | 13 +++++++++++++
 4 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/arch/arm/cpu/start.c b/arch/arm/cpu/start.c
index f25e592..1ee669b 100644
--- a/arch/arm/cpu/start.c
+++ b/arch/arm/cpu/start.c
@@ -34,6 +34,7 @@
 #include "mmu-early.h"
 
 unsigned long arm_stack_top;
+static unsigned long arm_barebox_base;
 static unsigned long arm_barebox_size;
 static void *barebox_boarddata;
 static unsigned long barebox_boarddata_size;
@@ -116,7 +117,7 @@ static inline unsigned long arm_mem_boarddata(unsigned long membase,
 {
 	unsigned long mem;
 
-	mem = arm_mem_barebox_image(membase, endmem, barebox_image_size);
+	mem = arm_barebox_base;
 	mem -= ALIGN(size, 64);
 
 	return mem;
@@ -143,15 +144,13 @@ __noreturn void barebox_non_pbl_start(unsigned long membase,
 {
 	unsigned long endmem = membase + memsize;
 	unsigned long malloc_start, malloc_end;
-	unsigned long barebox_size = barebox_image_size +
-		((unsigned long)&__bss_stop - (unsigned long)&__bss_start);
+	unsigned long barebox_size = ld_var(__bss_stop) - ld_var(_text);
+	unsigned long barebox_base;
 
-	if (IS_ENABLED(CONFIG_RELOCATABLE)) {
-		unsigned long barebox_base = arm_mem_barebox_image(membase,
-								   endmem,
-								   barebox_size);
+	barebox_base = arm_mem_barebox_image(membase, endmem, barebox_size);
+
+	if (IS_ENABLED(CONFIG_RELOCATABLE))
 		relocate_to_adr(barebox_base);
-	}
 
 	setup_c();
 
@@ -161,8 +160,8 @@ __noreturn void barebox_non_pbl_start(unsigned long membase,
 
 	arm_stack_top = endmem;
 	arm_barebox_size = barebox_size;
-	malloc_end = arm_mem_barebox_image(membase, endmem,
-						arm_barebox_size);
+	arm_barebox_base = barebox_base;
+	malloc_end = barebox_base;
 
 	if (IS_ENABLED(CONFIG_MMU_EARLY)) {
 		unsigned long ttb = arm_mem_ttb(membase, endmem);
diff --git a/arch/arm/cpu/uncompress.c b/arch/arm/cpu/uncompress.c
index b8e2e9f..0c54bc5 100644
--- a/arch/arm/cpu/uncompress.c
+++ b/arch/arm/cpu/uncompress.c
@@ -29,6 +29,7 @@
 #include <asm/sections.h>
 #include <asm/pgtable.h>
 #include <asm/cache.h>
+#include <asm/unaligned.h>
 
 #include <debug_ll.h>
 
@@ -49,6 +50,7 @@ void __noreturn barebox_multi_pbl_start(unsigned long membase,
 	uint32_t endmem = membase + memsize;
 	unsigned long barebox_base;
 	uint32_t *image_end;
+	uint32_t full_size;
 	void *pg_start;
 	unsigned long pc = get_pc();
 
@@ -69,19 +71,24 @@ void __noreturn barebox_multi_pbl_start(unsigned long membase,
 	/*
 	 * image_end is the first location after the executable. It contains
 	 * the size of the appended compressed binary followed by the binary.
+	 * The last 32bit word of the compressed binary contains the full size
+	 * of the uncompressed barebox image including bss.
 	 */
 	pg_start = image_end + 1;
-	pg_len = *(image_end);
+	pg_len = *(image_end) - sizeof(uint32_t);
+	full_size = get_unaligned_le32((void *)(pg_start + pg_len));
 
 	if (IS_ENABLED(CONFIG_RELOCATABLE))
 		barebox_base = arm_mem_barebox_image(membase, endmem,
-						     pg_len);
+						     full_size);
 	else
 		barebox_base = TEXT_BASE;
 
 	setup_c();
 
 	pr_debug("memory at 0x%08lx, size 0x%08lx\n", membase, memsize);
+	pr_debug("barebox image has full size 0x%08x, putting image to 0x%08lx\n",
+		 full_size, barebox_base);
 
 	if (IS_ENABLED(CONFIG_MMU_EARLY)) {
 		unsigned long ttb = arm_mem_ttb(membase, endmem);
diff --git a/arch/arm/lib32/barebox.lds.S b/arch/arm/lib32/barebox.lds.S
index 6dc8bd2..170b3b1 100644
--- a/arch/arm/lib32/barebox.lds.S
+++ b/arch/arm/lib32/barebox.lds.S
@@ -123,4 +123,5 @@ SECTIONS
 	__bss_stop = .;
 	_end = .;
 	_barebox_image_size = __bss_start - TEXT_BASE;
+	_barebox_full_size = __bss_stop - TEXT_BASE;
 }
diff --git a/images/Makefile b/images/Makefile
index da9cc8d..98812ed 100644
--- a/images/Makefile
+++ b/images/Makefile
@@ -67,6 +67,18 @@ $(obj)/%.pbl: $(pbl-lds) $(barebox-pbl-common) FORCE
 $(obj)/%.pblb: $(obj)/%.pbl FORCE
 	$(call if_changed,objcopy_bin,$(*F))
 
+barebox_full_size = printf $(shell							\
+		    hex_size=$$($(NM) $1 | awk '$$3 == "_barebox_full_size" { print $$1 }');	\
+											\
+echo $$hex_size |									\
+	sed 's/\(..\)/\1 /g' | {							\
+		read ch0 ch1 ch2 ch3;							\
+		for ch in $$ch3 $$ch2 $$ch1 $$ch0; do					\
+			printf '%s%03o' '\\' $$((0x$$ch)); 				\
+		done;									\
+	}										\
+)
+
 quiet_cmd_pblx ?= PBLX    $@
       cmd_pblx ?= cat $(obj)/$(patsubst %.pblx,%.pblb,$(2)) > $@; \
 		  $(call size_append, $(obj)/barebox.z) >> $@; \
@@ -91,6 +103,7 @@ suffix_$(CONFIG_IMAGE_COMPRESSION_NONE) = shipped
 # ----------------------------------------------------------------
 $(obj)/barebox.z: $(obj)/../barebox.bin FORCE
 	$(call if_changed,$(suffix_y))
+	$(call barebox_full_size, $(obj)/../barebox) >> $@
 
 # %.img - create a copy from another file
 # ----------------------------------------------------------------
-- 
2.8.1

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] ARM: Fix calling of arm_mem_barebox_image()

[Trent Piepho]

On Thu, 2016-09-15 at 09:10 +0200, Sascha Hauer wrote:
> On Wed, Sep 14, 2016 at 06:27:04PM +0000, Trent Piepho wrote:
> > On Wed, 2016-09-14 at 10:21 +0200, Sascha Hauer wrote:
> > > arm_mem_barebox_image() is used to pick a suitable place where to
> > > put the final image to. This is called from both the PBL uncompression
> > > code and also from the final image. To make it work properly it is
> > > crucial that it's called with the same arguments both times. Currently
> > 
> > This code has changed since I was working with it, but wouldn't
> > arm_mem_barebox_image() returning a different value from when the PBL
> > code calls it versus barebox_non_pbl_start() just result in an
> > unnecessary relocation of the uncompressed barebox from the PBL's choice
> > to the main barebox choice?
> 
> That may work when both regions do not overlap.

Ah, good point.  I suppose barebox could check that it doesn't try to
relocate on top of itself while running.


> I already tried. Somehow I didn't like the result that much, see the
> patch below. The patch also still misses the single pbl handling.

Here's what I was thinking.  multi and single pbl should work, getting
the size (data+bss) from the compressed data's size word.  Non-pbl and
PBL main barebox know the data via the linker symbols.

From: Trent Piepho <tpiepho@kymetacorp.com>
Date: Sat, 17 Sep 2016 16:24:59 -0700
Subject: [PATCH] Include BSS in the size appended to compressed barebox

In a PBL config the compressed main barebox image includes the
uncompressed size as a LE 32-bit word at the end of the compressed
image.  This allows the pbl to know how much space the main barebox
will need.  But this value alone is not correct, as the image also
needs BSS space which, being all zero, is not actually stored in the
compressed image.

While the BSS space could be estimated, it is important that both the
PBL and the main barebox use the same value.

To fix this, when making the barebox.z compressed data for the PBL,
include BSS in the size word.  Now the PBL knows the size it needs.

The main boxbox image (both in PBL and non-PBL modes) already knows
the correct values as they are embedded in the binary directly by the
linker, and are used elsewhere like setup_c() and
mem_malloc_resource().
---
 arch/arm/cpu/start-pbl.c  |  7 +++++--
 arch/arm/cpu/uncompress.c |  8 ++++++--
 images/Makefile           |  4 ++++
 scripts/Makefile.lib      | 23 ++++++++++-------------
 4 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/arch/arm/cpu/start-pbl.c b/arch/arm/cpu/start-pbl.c
index f723edc..1869a96 100644
--- a/arch/arm/cpu/start-pbl.c
+++ b/arch/arm/cpu/start-pbl.c
@@ -28,6 +28,7 @@
 #include <asm/sections.h>
 #include <asm/pgtable.h>
 #include <asm/cache.h>
+#include <asm/unaligned.h>
 
 #include "mmu-early.h"
 
@@ -49,7 +50,7 @@ __noreturn void barebox_single_pbl_start(unsigned long membase,
 		unsigned long memsize, void *boarddata)
 {
 	uint32_t offset;
-	uint32_t pg_start, pg_end, pg_len;
+	uint32_t pg_start, pg_end, pg_len, barebox_mem_len;
 	void __noreturn (*barebox)(unsigned long, unsigned long, void *);
 	uint32_t endmem = membase + memsize;
 	unsigned long barebox_base;
@@ -63,9 +64,11 @@ __noreturn void barebox_single_pbl_start(unsigned long membase,
 	pg_start = (uint32_t)&input_data - offset;
 	pg_end = (uint32_t)&input_data_end - offset;
 	pg_len = pg_end - pg_start;
+	/* Size word at end of image includes space for BSS */
+	barebox_mem_len = get_unaligned((const u32 *)(pg_start + pg_len - 4));
 
 	if (IS_ENABLED(CONFIG_RELOCATABLE))
-		barebox_base = arm_mem_barebox_image(membase, endmem, pg_len);
+		barebox_base = arm_mem_barebox_image(membase, endmem, barebox_mem_len);
 	else
 		barebox_base = TEXT_BASE;
 
diff --git a/arch/arm/cpu/uncompress.c b/arch/arm/cpu/uncompress.c
index b8e2e9f..9bec77f 100644
--- a/arch/arm/cpu/uncompress.c
+++ b/arch/arm/cpu/uncompress.c
@@ -29,6 +29,7 @@
 #include <asm/sections.h>
 #include <asm/pgtable.h>
 #include <asm/cache.h>
+#include <asm/unaligned.h>
 
 #include <debug_ll.h>
 
@@ -44,7 +45,7 @@ static int __attribute__((__used__))
 void __noreturn barebox_multi_pbl_start(unsigned long membase,
 		unsigned long memsize, void *boarddata)
 {
-	uint32_t pg_len;
+	uint32_t pg_len, barebox_mem_len;
 	void __noreturn (*barebox)(unsigned long, unsigned long, void *);
 	uint32_t endmem = membase + memsize;
 	unsigned long barebox_base;
@@ -69,13 +70,16 @@ void __noreturn barebox_multi_pbl_start(unsigned long membase,
 	/*
 	 * image_end is the first location after the executable. It contains
 	 * the size of the appended compressed binary followed by the binary.
+	 * The final word of the compressed binary is the space (uncompressed
+	 * image plus bss room) needed by barebox.
 	 */
 	pg_start = image_end + 1;
 	pg_len = *(image_end);
+	barebox_mem_len = get_unaligned((const u32 *)(pg_start + pg_len - 4));
 
 	if (IS_ENABLED(CONFIG_RELOCATABLE))
 		barebox_base = arm_mem_barebox_image(membase, endmem,
-						     pg_len);
+						     barebox_mem_len);
 	else
 		barebox_base = TEXT_BASE;
 
diff --git a/images/Makefile b/images/Makefile
index da9cc8d..cc76958 100644
--- a/images/Makefile
+++ b/images/Makefile
@@ -89,8 +89,12 @@ suffix_$(CONFIG_IMAGE_COMPRESSION_NONE) = shipped
 
 # barebox.z - compressed barebox binary
 # ----------------------------------------------------------------
+quiet_cmd_sizebss = SIZEBSS $@
+cmd_sizebss = $(call size_append, $<, $(obj)/../barebox) >> $@
+
 $(obj)/barebox.z: $(obj)/../barebox.bin FORCE
 	$(call if_changed,$(suffix_y))
+	$(call if_changed,sizebss)
 
 # %.img - create a copy from another file
 # ----------------------------------------------------------------
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index e55bc27..5b9d172 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -299,20 +299,17 @@ cmd_env=$(srctree)/scripts/genenv $(srctree) $(objtree) $@ $<
 
 # Bzip2 and LZMA do not include size in file... so we have to fake that;
 # append the size as a 32-bit littleendian number as gzip does.
-size_append = printf $(shell						\
-dec_size=0;								\
-for F in $1; do								\
-	fsize=$$(stat -c "%s" $$F);					\
-	dec_size=$$(expr $$dec_size + $$fsize);				\
+# If a second argument is supplied, include the BSS space it uses, as
+# this gives the memory needs to load a compressed binary.
+size_append = size=0;							\
+[ -n "$2" ] && size=`size -A $2 | awk '$$1==".bss"{print $$2}'`;	\
+for F in $1; do 							\
+	fsize=`stat -c %s $$F`;						\
+	size=$$((size + fsize));					\
 done;									\
-printf "%08x\n" $$dec_size |						\
-	sed 's/\(..\)/\1 /g' | {					\
-		read ch0 ch1 ch2 ch3;					\
-		for ch in $$ch3 $$ch2 $$ch1 $$ch0; do			\
-			printf '%s%03o' '\\' $$((0x$$ch)); 		\
-		done;							\
-	}								\
-)
+printf `printf '\\\\%03o\\\\%03o\\\\%03o\\\\%03o' 			\
+	$$((size&0xff)) $$((size>>8&0xff)) 				\
+	$$((size>>16&0xff)) $$((size>>24&0xff))`
 
 quiet_cmd_bzip2 = BZIP2   $@
 cmd_bzip2 = (cat $(filter-out FORCE,$^) | \
-- 
2.7.0.25.gfc10eb5.dirty


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] ARM: Fix calling of arm_mem_barebox_image()

[Sascha Hauer]

On Thu, Sep 22, 2016 at 12:35:14AM +0000, Trent Piepho wrote:
> On Thu, 2016-09-15 at 09:10 +0200, Sascha Hauer wrote:
> > On Wed, Sep 14, 2016 at 06:27:04PM +0000, Trent Piepho wrote:
> > > On Wed, 2016-09-14 at 10:21 +0200, Sascha Hauer wrote:
> > > > arm_mem_barebox_image() is used to pick a suitable place where to
> > > > put the final image to. This is called from both the PBL uncompression
> > > > code and also from the final image. To make it work properly it is
> > > > crucial that it's called with the same arguments both times. Currently
> > > 
> > > This code has changed since I was working with it, but wouldn't
> > > arm_mem_barebox_image() returning a different value from when the PBL
> > > code calls it versus barebox_non_pbl_start() just result in an
> > > unnecessary relocation of the uncompressed barebox from the PBL's choice
> > > to the main barebox choice?
> > 
> > That may work when both regions do not overlap.
> 
> Ah, good point.  I suppose barebox could check that it doesn't try to
> relocate on top of itself while running.
> 
> 
> > I already tried. Somehow I didn't like the result that much, see the
> > patch below. The patch also still misses the single pbl handling.
> 
> Here's what I was thinking.  multi and single pbl should work, getting
> the size (data+bss) from the compressed data's size word.  Non-pbl and
> PBL main barebox know the data via the linker symbols.

The single pbl does not work with this patch. It only adds the size to
barebox.z, but that files is only generated and used for the multi pbl
case.

Generally we can go in this direction, but the single pbl case must
work, or alternatively, the multi pbl and single pbl case should be
unified.

>  void __noreturn barebox_multi_pbl_start(unsigned long membase,
>  		unsigned long memsize, void *boarddata)
>  {
> -	uint32_t pg_len;
> +	uint32_t pg_len, barebox_mem_len;
>  	void __noreturn (*barebox)(unsigned long, unsigned long, void *);
>  	uint32_t endmem = membase + memsize;
>  	unsigned long barebox_base;
> @@ -69,13 +70,16 @@ void __noreturn barebox_multi_pbl_start(unsigned long membase,
>  	/*
>  	 * image_end is the first location after the executable. It contains
>  	 * the size of the appended compressed binary followed by the binary.
> +	 * The final word of the compressed binary is the space (uncompressed
> +	 * image plus bss room) needed by barebox.
>  	 */
>  	pg_start = image_end + 1;
>  	pg_len = *(image_end);

I believe we must substract 4 from pg_len here as pg_len now includes
the additional size including bss value. pbl_barebox_uncompress() needs
the real size of the compressed binary. I don't know how tolerant the
different decompression algorithms are against additional garbage at the
end. I think this should be:

-	pg_len = *(image_end);
+	pg_len = *(image_end) - sizeof(uint32_t);
+	barebox_mem_len = get_unaligned_le32((void *)(pg_start + pg_len));

> +	barebox_mem_len = get_unaligned((const u32 *)(pg_start + pg_len - 4));
>  
>  	if (IS_ENABLED(CONFIG_RELOCATABLE))
>  		barebox_base = arm_mem_barebox_image(membase, endmem,
> -						     pg_len);
> +						     barebox_mem_len);
>  	else
>  		barebox_base = TEXT_BASE;
>  
> diff --git a/images/Makefile b/images/Makefile
> index da9cc8d..cc76958 100644
> --- a/images/Makefile
> +++ b/images/Makefile
> @@ -89,8 +89,12 @@ suffix_$(CONFIG_IMAGE_COMPRESSION_NONE) = shipped
>  
>  # barebox.z - compressed barebox binary
>  # ----------------------------------------------------------------
> +quiet_cmd_sizebss = SIZEBSS $@
> +cmd_sizebss = $(call size_append, $<, $(obj)/../barebox) >> $@
> +
>  $(obj)/barebox.z: $(obj)/../barebox.bin FORCE
>  	$(call if_changed,$(suffix_y))
> +	$(call if_changed,sizebss)
>  
>  # %.img - create a copy from another file
>  # ----------------------------------------------------------------
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index e55bc27..5b9d172 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -299,20 +299,17 @@ cmd_env=$(srctree)/scripts/genenv $(srctree) $(objtree) $@ $<
>  
>  # Bzip2 and LZMA do not include size in file... so we have to fake that;
>  # append the size as a 32-bit littleendian number as gzip does.
> -size_append = printf $(shell						\
> -dec_size=0;								\
> -for F in $1; do								\
> -	fsize=$$(stat -c "%s" $$F);					\
> -	dec_size=$$(expr $$dec_size + $$fsize);				\
> +# If a second argument is supplied, include the BSS space it uses, as
> +# this gives the memory needs to load a compressed binary.
> +size_append = size=0;							\
> +[ -n "$2" ] && size=`size -A $2 | awk '$$1==".bss"{print $$2}'`;	\
> +for F in $1; do 							\
> +	fsize=`stat -c %s $$F`;						\
> +	size=$$((size + fsize));					\
>  done;									\
> -printf "%08x\n" $$dec_size |						\
> -	sed 's/\(..\)/\1 /g' | {					\
> -		read ch0 ch1 ch2 ch3;					\
> -		for ch in $$ch3 $$ch2 $$ch1 $$ch0; do			\
> -			printf '%s%03o' '\\' $$((0x$$ch)); 		\
> -		done;							\
> -	}								\
> -)
> +printf `printf '\\\\%03o\\\\%03o\\\\%03o\\\\%03o' 			\
> +	$$((size&0xff)) $$((size>>8&0xff)) 				\
> +	$$((size>>16&0xff)) $$((size>>24&0xff))`

This last change that changes the way the size is converted to binary is
not necessary, right?

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox