Re: [bootwrapper PATCH v2 11/13] Announce locations of memory objects

Robin Murphy
2022-01-14T16:05:41Z

On 2022-01-14 15:30, Andre Przywara wrote:
> On Fri, 14 Jan 2022 10:56:51 +0000
> Mark Rutland <mark.rutland@arm.com> wrote:
>
> Hi Mark,
>
>> To make it easier to debug boot failures, log the location of memory
>> objects at boot time.
>>
>> This is logged to the serial console as:
>>
>> | Boot-wrapper v0.2
>> | Entered at EL3
>> | Memory layout:
>> | [0000000080000000..0000000080001f90] => boot-wrapper
>> | [000000008000fff8..0000000080010000] => mbox
>> | [0000000080200000..00000000822af200] => kernel
>> | [0000000088000000..0000000088002857] => dtb
>>
>> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
>> ---
>> common/init.c | 27 +++++++++++++++++++++++++++
>> common/platform.c | 13 +++++++++++++
>> include/platform.h | 2 ++
>> model.lds.S | 20 ++++++++++++++------
>> 4 files changed, 56 insertions(+), 6 deletions(-)
>>
>> diff --git a/common/init.c b/common/init.c
>> index 2600f73..fc74b9e 100644
>> --- a/common/init.c
>> +++ b/common/init.c
>> @@ -14,6 +14,32 @@ static void announce_bootwrapper(void)
>> print_string("Boot-wrapper v0.2\r\n");
>> }
>>
>> +#define announce_object(object, desc) \
>> +do { \
>> + extern char object##__start[]; \
>> + extern char object##__end[]; \
>> + print_string("["); \
>> + print_ulong_hex((unsigned long)object##__start); \
>> + print_string(".."); \
>> + print_ulong_hex((unsigned long)object##__end); \
>> + print_string("] => " desc "\r\n"); \
>> +} while (0)
>> +
>> +static void announce_objects(void)
>> +{
>> + print_string("Memory layout:\r\n");
>> + announce_object(text, "boot-wrapper");
>> + announce_object(mbox, "mbox");
>> + announce_object(kernel, "kernel");
>> +#ifdef XEN
>> + announce_object(xen, "xen");
>> +#endif
>> + announce_object(dtb, "dtb");
>> +#ifdef USE_INITRD
>> + announce_object(filesystem, "initrd");
>> +#endif
>> +}
>> +
>> void announce_arch(void);
>>
>> void cpu_init_bootwrapper(void)
>> @@ -22,6 +48,7 @@ void cpu_init_bootwrapper(void)
>> init_uart();
>> announce_bootwrapper();
>> announce_arch();
>> + announce_objects();
>> print_string("\r\n");
>> init_platform();
>> }
>> diff --git a/common/platform.c b/common/platform.c
>> index 80d0562..26e5944 100644
>> --- a/common/platform.c
>> +++ b/common/platform.c
>> @@ -52,6 +52,19 @@ void print_string(const char *str)
>> print_char(*str++);
>> }
>>
>> +#define HEX_CHARS_PER_LONG (2 * sizeof(long))
>> +
>> +void print_ulong_hex(unsigned long val)
>> +{
>> + const char hex_chars[16] = "0123456789abcdef";
>
> This breaks the build for me (I guess because of the const?):
> -------------------
> ld --gc-sections arch/aarch64/boot.o arch/aarch64/stack.o arch/aarch64/utils.o arch/aarch64/init.o arch/aarch64/psci.o common/boot.o common/bakery_lock.o common/platform.o common/lib.o common/init.o common/psci.o common/gic-v3.o -o linux-system.axf --script=model.lds
> ld: common/platform.o: in function `print_ulong_hex':
> /src/boot-wrapper-aarch64.git/common/platform.c:58: undefined reference to `__stack_chk_guard'
> ld: /src/boot-wrapper-aarch64.git/common/platform.c:58: undefined reference to `__stack_chk_guard'
> ld: /src/boot-wrapper-aarch64.git/common/platform.c:66: undefined reference to `__stack_chk_fail'
> /src/boot-wrapper-aarch64.git/common/platform.c:66:(.text.print_ulong_hex+0xa0): relocation truncated to fit: R_AARCH64_CALL26 against undefined symbol `__stack_chk_fail'
> make: *** [Makefile:684: linux-system.axf] Error 1
> ------------------

I got curious and tried a quick test compiling this function in
isolation, and indeed for some reason GCC (9.3.0 at -O2) seems to end up
allocating 80 bytes of stack, which is apparently large enough to
trigger insertion of a stack check, and laboriously copying the string
from static data into it. FWIW either making hex_chars static const, or
simply '#define hex_chars "01234567abcdef"', not only avoids a stack
check but also leads to notably nicer code.

Of course, something like "print_char(v + (v > 9 ? 'a' : '0'));" is best
of all in terms of code/data, but risks venturing into "too clever"
territory.

Robin.

> Adding -fno-stack-protector fixes it again.
>
> I am still checking on each one of those compiler options, shall I send a
> smaller version of that patch 3/9 of mine, meanwhile, just carrying the
> uncontested -ffreestanding and -nostdlib, plus -fno-stack-protector?
>
> Cheers,
> Andre
>
>> + int i;
>> +
>> + for (i = HEX_CHARS_PER_LONG - 1; i >= 0; i--) {
>> + int v = (val >> (4 * i)) & 0xf;
>> + print_char(hex_chars[v]);
>> + }
>> +}
>> +
>> void init_uart(void)
>> {
>> /*
>> diff --git a/include/platform.h b/include/platform.h
>> index 237b481..c88e124 100644
>> --- a/include/platform.h
>> +++ b/include/platform.h
>> @@ -11,6 +11,8 @@
>>
>> void print_char(char c);
>> void print_string(const char *str);
>> +void print_ulong_hex(unsigned long val);
>> +
>> void init_uart(void);
>>
>> void init_platform(void);
>> diff --git a/model.lds.S b/model.lds.S
>> index d4e7e13..dacaa25 100644
>> --- a/model.lds.S
>> +++ b/model.lds.S
>> @@ -35,46 +35,54 @@ SECTIONS
>> * the boot section's *(.data)
>> */
>> .kernel (PHYS_OFFSET + KERNEL_OFFSET): {
>> - kernel = .;
>> + kernel__start = .;
>> KERNEL
>> + kernel__end = .;
>> }
>>
>> #ifdef XEN
>> .xen (PHYS_OFFSET + XEN_OFFSET): {
>> - xen = .;
>> + xen__start = .;
>> XEN
>> + xen__end = .;
>> }
>>
>> - entrypoint = xen;
>> + entrypoint = xen__start;
>> #else
>> - entrypoint = kernel;
>> + entrypoint = kernel__start;
>> #endif
>>
>> .dtb (PHYS_OFFSET + FDT_OFFSET): {
>> + dtb__start = .;
>> dtb = .;
>> ./fdt.dtb
>> + dtb__end = .;
>> }
>>
>> #ifdef USE_INITRD
>> .filesystem (PHYS_OFFSET + FS_OFFSET): {
>> - filesystem = .;
>> + filesystem__start = .;
>> FILESYSTEM
>> - fs_size = . - filesystem;
>> + filesystem__end = .;
>> }
>> #endif
>>
>> .boot PHYS_OFFSET: {
>> + text__start = .;
>> *(.init)
>> *(.text*)
>> *(.data* .rodata* .bss* COMMON)
>> *(.vectors)
>> *(.stack)
>> PROVIDE(etext = .);
>> + text__end = .;
>> }
>>
>> .mbox (PHYS_OFFSET + MBOX_OFFSET): {
>> + mbox__start = .;
>> mbox = .;
>> QUAD(0x0)
>> + mbox__end = .;
>> }
>>
>> ASSERT(etext <= (PHYS_OFFSET + TEXT_LIMIT), ".text overflow!")