[LinuxBIOS] [PATCH] v3: fix absolute calls from initram

[Carl-Daniel Hailfinger]

Absolute calls from initram were only working from the file which had
_MAINOBJECT #defined. Calls from all other files ended up in nirvana
because the compiler was not able to calculate the address of the
wrapper for the absolute call. The linker tried, but failed miserably.
Use the -combine flag and compile all of initram at once. This enables
GCC to calculate the address of the abscall wrapper, resulting in
working code.

Segher Boessenkool thinks the patched code works only by accident
because GCC has no way to specify generation of XIP code. According to
him, future GCC versions or other circumstances may break the code.

While this patch makes code work for now, it does NOT check whether the
generated code tries to write to memory outside the stack (general
writable data). That will of course fail, but I hope porters are smart
enough to avoid that.

Signed-off-by: Carl-Daniel Hailfinger

Index: LinuxBIOSv3-xiptest-working/mainboard/adl/msm800sev/Makefile
===================================================================
--- LinuxBIOSv3-xiptest-working/mainboard/adl/msm800sev/Makefile	(revision 535)
+++ LinuxBIOSv3-xiptest-working/mainboard/adl/msm800sev/Makefile	(working copy)
@@ -21,10 +21,10 @@

STAGE0_MAINBOARD_OBJ := $(obj)/mainboard/$(MAINBOARDDIR)/stage1.o

-INITRAM_OBJ =   $(obj)/mainboard/$(MAINBOARDDIR)/initram.o \
-		$(obj)/northbridge/amd/geodelx/raminit.o \
-		$(obj)/southbridge/amd/cs5536/smbus_initram.o \
-		$(obj)/arch/x86/geodelx/geodelx.o
+INITRAM_OBJ =   $(src)/mainboard/$(MAINBOARDDIR)/initram.c \
+		$(src)/northbridge/amd/geodelx/raminit.c \
+		$(src)/southbridge/amd/cs5536/smbus_initram.c \
+		$(src)/arch/x86/geodelx/geodelx.c

STAGE2_MAINBOARD_OBJ =

Index: LinuxBIOSv3-xiptest-working/mainboard/amd/norwich/Makefile
===================================================================
--- LinuxBIOSv3-xiptest-working/mainboard/amd/norwich/Makefile	(revision 535)
+++ LinuxBIOSv3-xiptest-working/mainboard/amd/norwich/Makefile	(working copy)
@@ -21,10 +21,10 @@

STAGE0_MAINBOARD_OBJ := $(obj)/mainboard/$(MAINBOARDDIR)/stage1.o

-INITRAM_OBJ =   $(obj)/mainboard/$(MAINBOARDDIR)/initram.o \
-		$(obj)/northbridge/amd/geodelx/raminit.o \
-		$(obj)/southbridge/amd/cs5536/smbus_initram.o \
-		$(obj)/arch/x86/geodelx/geodelx.o
+INITRAM_OBJ =   $(src)/mainboard/$(MAINBOARDDIR)/initram.c \
+		$(src)/northbridge/amd/geodelx/raminit.c \
+		$(src)/southbridge/amd/cs5536/smbus_initram.c \
+		$(src)/arch/x86/geodelx/geodelx.c

STAGE2_MAINBOARD_OBJ =

Index: LinuxBIOSv3-xiptest-working/mainboard/artecgroup/dbe61/Makefile
===================================================================
--- LinuxBIOSv3-xiptest-working/mainboard/artecgroup/dbe61/Makefile	(revision 535)
+++ LinuxBIOSv3-xiptest-working/mainboard/artecgroup/dbe61/Makefile	(working copy)
@@ -21,8 +21,8 @@

STAGE0_MAINBOARD_OBJ := $(obj)/mainboard/$(MAINBOARDDIR)/stage1.o

-INITRAM_OBJ =   $(obj)/mainboard/$(MAINBOARDDIR)/initram.o \
-		$(obj)/arch/x86/geodelx/geodelx.o
+INITRAM_OBJ =   $(src)/mainboard/$(MAINBOARDDIR)/initram.c \
+		$(src)/arch/x86/geodelx/geodelx.c

STAGE2_MAINBOARD_OBJ =

Index: LinuxBIOSv3-xiptest-working/mainboard/pcengines/alix1c/Makefile
===================================================================
--- LinuxBIOSv3-xiptest-working/mainboard/pcengines/alix1c/Makefile	(revision 535)
+++ LinuxBIOSv3-xiptest-working/mainboard/pcengines/alix1c/Makefile	(working copy)
@@ -21,9 +21,9 @@

STAGE0_MAINBOARD_OBJ := $(obj)/mainboard/$(MAINBOARDDIR)/stage1.o

-INITRAM_OBJ =   $(obj)/mainboard/$(MAINBOARDDIR)/initram.o \
-		$(obj)/northbridge/amd/geodelx/raminit.o \
-		$(obj)/arch/x86/geodelx/geodelx.o
+INITRAM_OBJ =   $(src)/mainboard/$(MAINBOARDDIR)/initram.c \
+		$(src)/northbridge/amd/geodelx/raminit.c \
+		$(src)/arch/x86/geodelx/geodelx.c

STAGE2_MAINBOARD_OBJ =

Index: LinuxBIOSv3-xiptest-working/mainboard/emulation/qemu-x86/Makefile
===================================================================
--- LinuxBIOSv3-xiptest-working/mainboard/emulation/qemu-x86/Makefile	(revision 535)
+++ LinuxBIOSv3-xiptest-working/mainboard/emulation/qemu-x86/Makefile	(working copy)
@@ -28,8 +28,8 @@
 # directory and is built from what was auto.c in v2.
 #

-INITRAM_OBJ = $(obj)/mainboard/$(MAINBOARDDIR)/initram.o \
-		$(obj)/mainboard/$(MAINBOARDDIR)/initram_printktest.o
+INITRAM_OBJ = $(src)/mainboard/$(MAINBOARDDIR)/initram.c \
+		$(src)/mainboard/$(MAINBOARDDIR)/initram_printktest.c

STAGE2_MAINBOARD_OBJ = vga.o

Index: LinuxBIOSv3-xiptest-working/Rules.make
===================================================================
--- LinuxBIOSv3-xiptest-working/Rules.make	(revision 535)
+++ LinuxBIOSv3-xiptest-working/Rules.make	(working copy)
@@ -78,13 +78,3 @@
    $(Q)printf "  CC      $(subst $(shell pwd)/,,$(@))\n"
    $(Q)$(CC) $(INITCFLAGS) -c $< -o $@

-#
-# RAM initialization code can not be linked at a specific address,
-# hence it has to be executed in place (XIP) position independently.
-#
-
-$(obj)/%_xip.o: $(src)/%.c
-	$(Q)mkdir -p $(dir $@)
-	$(Q)printf "  CC      $(subst $(shell pwd)/,,$(@)) (XIP)\n"
-	$(Q)$(CC) $(INITCFLAGS) -D_SHARED -fPIE -c $< -o $@
-
Index: LinuxBIOSv3-xiptest-working/arch/x86/stage1.c
===================================================================
--- LinuxBIOSv3-xiptest-working/arch/x86/stage1.c	(revision 535)
+++ LinuxBIOSv3-xiptest-working/arch/x86/stage1.c	(working copy)
@@ -137,17 +137,17 @@
    // find first initram
    if (check_normal_boot_flag()) {
    	printk(BIOS_DEBUG, "Choosing normal boot.\n");
-		ret = execute_in_place(&archive, "normal/initram.o/segment0");
+		ret = execute_in_place(&archive, "normal/initram/segment0");
    } else {
    	printk(BIOS_DEBUG, "Choosing fallback boot.\n");
-		ret = execute_in_place(&archive, "fallback/initram.o/segment0");
+		ret = execute_in_place(&archive, "fallback/initram/segment0");
    	/* Try a normal boot if fallback doesn't exist in the lar.
    	 * TODO: There are other ways to do this.
    	 * It could be ifdef or the boot flag could be forced.
    	 */
    	if (ret) {
    		printk(BIOS_DEBUG, "Fallback failed. Try normal boot\n");
-			ret = execute_in_place(&archive, "normal/initram.o/segment0");
+			ret = execute_in_place(&archive, "normal/initram/segment0");
    	}
    }

Index: LinuxBIOSv3-xiptest-working/arch/x86/Makefile
===================================================================
--- LinuxBIOSv3-xiptest-working/arch/x86/Makefile	(revision 535)
+++ LinuxBIOSv3-xiptest-working/arch/x86/Makefile	(working copy)
@@ -36,7 +36,7 @@

ROM_SIZE := $(shell expr $(CONFIG_LINUXBIOS_ROMSIZE_KB) * 1024)

-LARFILES := nocompress:normal/initram.o normal/stage2.o nocompress:normal/option_table
+LARFILES := nocompress:normal/initram normal/stage2.o nocompress:normal/option_table
 ifneq ($(CONFIG_PAYLOAD_NONE),y)
 LARFILES += normal/payload
 endif
@@ -57,11 +57,11 @@
 COMPRESSFLAG := -C nrv2b
 endif

-$(obj)/linuxbios.rom $(obj)/linuxbios.map: $(obj)/linuxbios.bootblock $(obj)/util/lar/lar lzma nrv2b $(obj)/linuxbios.initram.o $(obj)/linuxbios.stage2.o $(obj)/option_table
+$(obj)/linuxbios.rom $(obj)/linuxbios.map: $(obj)/linuxbios.bootblock $(obj)/util/lar/lar lzma nrv2b $(obj)/linuxbios.initram $(obj)/linuxbios.stage2.o $(obj)/option_table
    $(Q)rm -rf $(obj)/lar.tmp
    $(Q)mkdir $(obj)/lar.tmp
    $(Q)mkdir $(obj)/lar.tmp/normal
-	$(Q)cp $(obj)/linuxbios.initram.o $(obj)/lar.tmp/normal/initram.o
+	$(Q)cp $(obj)/linuxbios.initram $(obj)/lar.tmp/normal/initram
    $(Q)cp $(obj)/linuxbios.stage2.o $(obj)/lar.tmp/normal/stage2.o
    $(Q)cp $(obj)/option_table $(obj)/lar.tmp/normal/option_table
 ifeq ($(CONFIG_PAYLOAD_NONE),y)
@@ -232,12 +232,14 @@
    $(Q)printf "  AS      $(subst $(shell pwd)/,,$(@))\n"
    $(Q)$(AS) $(obj)/arch/x86/stage0_asm.s -o $@

-$(obj)/linuxbios.initram.o $(obj)/linuxbios.initram.map: $(obj)/stage0.init $(obj)/stage0-prefixed.o $(patsubst %.o,%_xip.o,$(INITRAM_OBJ))
+$(obj)/linuxbios.initram $(obj)/linuxbios.initram.map: $(obj)/stage0.init $(obj)/stage0-prefixed.o $(INITRAM_OBJ)
+	$(Q)printf "  CC      $(subst $(shell pwd)/,,$(@)) (XIP)\n"
+	$(Q)$(CC) $(INITCFLAGS) -D_SHARED -fPIE -c -combine $(INITRAM_OBJ) -o $(obj)/linuxbios.initram_partiallylinked.o
    $(Q)# initram links against stage0
    $(Q)printf "  LD      $(subst $(shell pwd)/,,$(@))\n"
    $(Q)$(LD) -Ttext 0 --entry main -N -R $(obj)/stage0-prefixed.o \
-		$(patsubst %.o,%_xip.o,$(INITRAM_OBJ)) -o $(obj)/linuxbios.initram.o
+		$(obj)/linuxbios.initram_partiallylinked.o -o $(obj)/linuxbios.initram
    $(Q)printf "  NM      $(subst $(shell pwd)/,,$(@))\n"
-	$(Q)$(NM) $(obj)/linuxbios.initram.o | sort -u > $(obj)/linuxbios.initram.map
+	$(Q)$(NM) $(obj)/linuxbios.initram | sort -u > $(obj)/linuxbios.initram.map

endif

[Peter Stuge]

On Tue, Dec 04, 2007 at 03:39:58AM +0100, Carl-Daniel Hailfinger wrote:
> ret = execute_in_place(&archive, "normal/initram.o/segment0");
> ret = execute_in_place(&archive, "normal/initram/segment0");

This seems to have snuck in from another patch?

//Peter

[Carl-Daniel Hailfinger]

On 04.12.2007 04:03, Peter Stuge wrote:
> On Tue, Dec 04, 2007 at 03:39:58AM +0100, Carl-Daniel Hailfinger wrote:
> > ret = execute_in_place(&archive, "normal/initram.o/segment0");
> > ret = execute_in_place(&archive, "normal/initram/segment0");
>
> This seems to have snuck in from another patch?

Yes and no. I sent another patch which changes these lines in the same
way, but this change made changing names necessary anyway, so I decided
to incorporate that change.
Btw, the "remove .o suffix during lar parsing" patch is still pending,
but I feel that it may not be entirely correct from a conceptual
standpoint (code-wise it is fine, though), and removing the .o suffix
here is just the natural way of handling the changed compilation. It's
not an object anyway.

Regards,
Carl-Daniel

[Corey Osgood]

Carl-Daniel Hailfinger wrote:
> Absolute calls from initram were only working from the file which had
> _MAINOBJECT #defined. Calls from all other files ended up in nirvana
> because the compiler was not able to calculate the address of the
> wrapper for the absolute call. The linker tried, but failed miserably.
> Use the -combine flag and compile all of initram at once. This enables
> GCC to calculate the address of the abscall wrapper, resulting in
> working code.
>
> Segher Boessenkool thinks the patched code works only by accident
> because GCC has no way to specify generation of XIP code. According to
> him, future GCC versions or other circumstances may break the code.
>
> While this patch makes code work for now, it does NOT check whether the
> generated code tries to write to memory outside the stack (general
> writable data). That will of course fail, but I hope porters are smart
> enough to avoid that.

Great work tracking this down! This is okay for now, but we need to look
for a better solution in the future. Counting on porters who may or may
not remember this discussion to avoid something isn't good future-proofing.
> Signed-off-by: Carl-Daniel Hailfinger

I've attached a QEMU bootlog, it seems to make the calls to the right
places, but still won't boot FILO. Per IRC, I won't ack at the moment
until more people can test, but it does look good to me :)

-Corey

[Corey Osgood]

Carl-Daniel Hailfinger wrote:
> Absolute calls from initram were only working from the file which had
> _MAINOBJECT #defined. Calls from all other files ended up in nirvana
> because the compiler was not able to calculate the address of the
> wrapper for the absolute call. The linker tried, but failed miserably.
> Use the -combine flag and compile all of initram at once. This enables
> GCC to calculate the address of the abscall wrapper, resulting in
> working code.
>
> Segher Boessenkool thinks the patched code works only by accident
> because GCC has no way to specify generation of XIP code. According to
> him, future GCC versions or other circumstances may break the code.
>
> While this patch makes code work for now, it does NOT check whether the
> generated code tries to write to memory outside the stack (general
> writable data). That will of course fail, but I hope porters are smart
> enough to avoid that.
>
> Signed-off-by: Carl-Daniel Hailfinger

Acked-by: Corey Osgood

[Carl-Daniel Hailfinger]

On 04.12.2007 19:40, Corey Osgood wrote:
> Carl-Daniel Hailfinger wrote:
> > Absolute calls from initram were only working from the file which had
> > _MAINOBJECT #defined. Calls from all other files ended up in nirvana
> > because the compiler was not able to calculate the address of the
> > wrapper for the absolute call. The linker tried, but failed miserably.
> > Use the -combine flag and compile all of initram at once. This enables
> > GCC to calculate the address of the abscall wrapper, resulting in
> > working code.
> >
> > Segher Boessenkool thinks the patched code works only by accident
> > because GCC has no way to specify generation of XIP code. According to
> > him, future GCC versions or other circumstances may break the code.
> >
> > While this patch makes code work for now, it does NOT check whether the
> > generated code tries to write to memory outside the stack (general
> > writable data). That will of course fail, but I hope porters are smart
> > enough to avoid that.
> >
> > Signed-off-by: Carl-Daniel Hailfinger
>
> Acked-by: Corey Osgood

Thanks, r537.

Regards,
Carl-Daniel