Re: [PATCH 9/9] arm: initial drop

[Christoffer Dall <christoffer.dall@linaro.org>]

On Wed, Dec 04, 2013 at 05:42:57PM +0100, Andrew Jones wrote:
> This is the initial arm test framework and a first simple test that
> checks some bootinfo. kvm isn't needed to run this test. This patch
> also adds a common build environment variable, $QEMU_BIN, which
> allows makefiles to call on qemu when needed.
> 
> Try it out with
>   yum install gcc-arm-linux-gnu dtc
>   export QEMU=[qemu with mach-virt and virtio-testdev]
>   ./configure --cross-prefix=arm-linux-gnu- --arch=arm
>   make
>   ./run_tests.sh

You refer to QEMU_BIN but your example uses QEMU= ?

It's getting to be a pretty heavy chunk this patch, it would have been
slightly easier to review if it was broken up into multiple patches, but
ok.

> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> 
> ---
> v2:
>   - add eabi utility functions needed for some toolchains, this
>     allows us to drop the divmod hacks that were in v1
>   - switch to kernel coding style
>   - some refactoring of setup code for heap init
>   - some refactoring of the simple bootinfo test for clarity
>     and reuse opportunity
>   - update base addr for the new mach-virt version
> ---
>  arm/boot.c            | 17 +++++++++++
>  arm/cstart.S          | 38 +++++++++++++++++++++++
>  arm/flat.lds          | 18 +++++++++++
>  arm/run               | 19 ++++++++++++
>  arm/unittests.cfg     | 11 +++++++
>  config/config-arm.mak | 73 +++++++++++++++++++++++++++++++++++++++++++
>  configure             | 10 ++++--
>  lib/arm/eabi_compat.c | 20 ++++++++++++
>  lib/arm/io.c          | 31 +++++++++++++++++++
>  lib/arm/io.h          | 24 +++++++++++++++
>  lib/arm/setup.c       | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/arm/sysinfo.h     | 19 ++++++++++++
>  lib/libcflat.h        |  2 ++
>  lib/libio.h           |  4 +++
>  lib/test_util.c       | 34 +++++++++++++++++++++
>  lib/test_util.h       | 13 ++++++++
>  16 files changed, 416 insertions(+), 2 deletions(-)
>  create mode 100644 arm/boot.c
>  create mode 100644 arm/cstart.S
>  create mode 100644 arm/flat.lds
>  create mode 100755 arm/run
>  create mode 100644 arm/unittests.cfg
>  create mode 100644 config/config-arm.mak
>  create mode 100644 lib/arm/eabi_compat.c
>  create mode 100644 lib/arm/io.c
>  create mode 100644 lib/arm/io.h
>  create mode 100644 lib/arm/setup.c
>  create mode 100644 lib/arm/sysinfo.h
>  create mode 100644 lib/test_util.c
>  create mode 100644 lib/test_util.h
> 
> diff --git a/arm/boot.c b/arm/boot.c
> new file mode 100644
> index 0000000000000..dc42dfc232366
> --- /dev/null
> +++ b/arm/boot.c
> @@ -0,0 +1,17 @@
> +#include "libcflat.h"
> +#include "test_util.h"
> +#include "arm/sysinfo.h"
> +
> +int main(int argc, char **argv)
> +{
> +	int ret = FAIL;
> +
> +	if (argc >= 1) {
> +		--argc;
> +		if (!strcmp(argv[0], "mem") && enough_args(argc, 1)) {
> +			if (check_u32(mem32.size/1024/1024, 10, argv[1]))
> +				ret = PASS;
> +		}
> +	}
> +	return ret;
> +}

consider renaming this file to boottest.c to avoid the confusion that
this file is needed to boot anything that runs tests...

> diff --git a/arm/cstart.S b/arm/cstart.S
> new file mode 100644
> index 0000000000000..05d4bb5becaa0
> --- /dev/null
> +++ b/arm/cstart.S
> @@ -0,0 +1,38 @@
> +
> +#define CR_B	(1 << 7)	/* Big endian */
> +
> +.arm
> +
> +.section .init
> +
> +.globl start
> +start:
> +	/* bootloader params are in r0-r2 */

which bootlaoder params are they?  What boot protocol is used, what are
you expecting to be in the various registers?

I assume this tool expects r0-r2 to follow the definitions in
Documentation/arm/Booting in the kernel?

> +	ldr	sp, =stacktop
> +
> +	mrc	p15, 0, r8, c1, c0, 0	@r8 = sctrl

that's sctlr, not sctrl.

> +	ands	r3, r8, #CR_B		@set BE, if necessary
> +	ldrne	r3, =cpu_is_be
> +	movne	r4, #1

This is deprecated for ARMv7 according to the ARM ARM.  What is the
intention here?  Does qemu support running this test tool with the
system configured for big-endian?  If so, I think this is a build option
for this binary or you need to come up with some other
architecture-compliant method of detecting the endian-state.


> +	strne	r4, [r3]
> +	bl	setup			@complete setup
> +
> +	/* start the test */
> +	ldr	r0, =__argc
> +	ldr	r0, [r0]
> +	ldr	r1, =__argv
> +	bl	main
> +	bl	exit
> +	b	halt
> +
> +.text
> +
> +.globl halt
> +halt:
> +1:	wfi
> +	b	1b
> +
> +.data
> +
> +.globl cpu_is_be
> +cpu_is_be:	.word 0
> diff --git a/arm/flat.lds b/arm/flat.lds
> new file mode 100644
> index 0000000000000..3e5d72e24989b
> --- /dev/null
> +++ b/arm/flat.lds
> @@ -0,0 +1,18 @@
> +
> +SECTIONS
> +{
> +    .text : { *(.init) *(.text) *(.text.*) }
> +    . = ALIGN(4K);
> +    .data : { *(.data) }
> +    . = ALIGN(16);
> +    .rodata : { *(.rodata) }
> +    . = ALIGN(16);
> +    .bss : { *(.bss) }
> +    . = ALIGN(4K);
> +    edata = .;
> +    . += 8K;
> +    . = ALIGN(4K);
> +    stacktop = .;
> +}
> +
> +ENTRY(start)
> diff --git a/arm/run b/arm/run
> new file mode 100755
> index 0000000000000..64446e8907564
> --- /dev/null
> +++ b/arm/run
> @@ -0,0 +1,19 @@
> +#!/bin/bash
> +
> +qemu="${QEMU:-qemu-system-arm}"
> +testdev='virtio-testdev'
> +
> +if ! $qemu -device '?' 2>&1 | grep $testdev > /dev/null; then
> +    echo \"$qemu\" has no support for the virtio test device. Exiting.
> +    exit 2
> +fi
> +
> +command="$qemu -device $testdev -display none -serial stdio "
> +command+="-M virt -cpu cortex-a15 "
> +#command+="-enable-kvm "
> +command+="-kernel"
> +echo $command "$@"
> +$command "$@"
> +ret=$?
> +echo Return value from qemu: $ret
> +exit $ret
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> new file mode 100644
> index 0000000000000..c328657b7944a
> --- /dev/null
> +++ b/arm/unittests.cfg
> @@ -0,0 +1,11 @@
> +# Define your new unittest following the convention:
> +# [unittest_name]
> +# file = foo.flat # Name of the flat file to be used
> +# smp = 2 # Number of processors the VM will use during this test
> +# extra_params = -append <params...> # Additional parameters used

used to QEMU I presume?

> +# arch = arm/arm64 # Only if the test case works only on one of them

Do we have any verified support for arm64 yet?

> +# groups = group1 group2 # Used to identify test cases with run_tests -g ...
> +
> +[boot_info]
> +file = boot.flat
> +extra_params = -m 256 -append 'mem 256'
> diff --git a/config/config-arm.mak b/config/config-arm.mak
> new file mode 100644
> index 0000000000000..d0814186b279c
> --- /dev/null
> +++ b/config/config-arm.mak
> @@ -0,0 +1,73 @@
> +mach = mach-virt
> +iodevs = pl011 virtio_mmio
> +phys_base = 0x40000000
> +
> +cstart.o = $(TEST_DIR)/cstart.o
> +bits = 32
> +ldarch = elf32-littlearm
> +kernel_offset = 0x10000
> +CFLAGS += -D__arm__
> +
> +all: test_cases
> +
> +cflatobjs += \
> +	lib/$(TEST_DIR)/iomaps.gen.o \
> +	lib/heap.o \
> +	lib/iomaps.o \
> +	lib/libio.o \
> +	lib/virtio.o \
> +	lib/virtio-testdev.o \
> +	lib/test_util.o \
> +	lib/arm/io.o \
> +	lib/arm/setup.o
> +
> +libeabi := lib/arm/libeabi.a
> +eabiobjs += \
> +	lib/arm/eabi_compat.o
> +
> +$(libcflat) $(libeabi): LDFLAGS += -nostdlib
> +$(libcflat) $(libeabi): CFLAGS += -ffreestanding -I lib
> +
> +CFLAGS += -Wextra
> +CFLAGS += -marm
> +#CFLAGS += -mcpu=$(PROCESSOR)

This looks weird, should it not be $(PROCESSOR) and default to
cortex-a15 if it's not set?

(also note that you can now use -cpu host with mach-virt which may make
your life easier).

> +CFLAGS += -mcpu=cortex-a15
> +CFLAGS += -O2

Why do we choose this particular optimzation-level in the arch-specific
config?

> +
> +libgcc := $(shell $(CC) -m$(ARCH) --print-libgcc-file-name)
> +start_addr := $(shell printf "%x\n" $$(( $(phys_base) + $(kernel_offset) )))
> +
> +FLATLIBS = $(libcflat) $(libgcc) $(libeabi)
> +%.elf: %.o $(FLATLIBS) arm/flat.lds
> +	$(CC) $(CFLAGS) -nostdlib -o $@ \
> +		-Wl,-T,arm/flat.lds,--build-id=none,-Ttext=$(start_addr) \
> +		$(filter %.o, $^) $(FLATLIBS)

I have no idea what the above rules are doing :-(

> +
> +$(libeabi): $(eabiobjs)
> +	$(AR) rcs $@ $^
> +
> +%.flat: %.elf
> +	$(OBJCOPY) -O binary $^ $@
> +
> +tests-common = $(TEST_DIR)/boot.flat
> +
> +tests_and_config = $(TEST_DIR)/*.flat $(TEST_DIR)/unittests.cfg
> +
> +test_cases: $(tests-common) $(tests)

what's the distinction between tests-common and tests?  arm/arm64 vs.
one or the other, or?

> +
> +$(TEST_DIR)/%.o: CFLAGS += -std=gnu99 -ffreestanding -I lib
> +
> +$(TEST_DIR)/boot.elf: $(cstart.o) $(TEST_DIR)/boot.o
> +
> +lib/$(TEST_DIR)/iomaps.gen.c: lib/$(TEST_DIR)/$(mach).dts
> +	scripts/gen-devtree-iomaps.pl $^ $(iodevs) > $@
> +
> +lib/$(TEST_DIR)/mach-virt.dts: dtb = $(subst .dts,.dtb,$@)
> +lib/$(TEST_DIR)/mach-virt.dts:
> +	$(QEMU_BIN) -kernel /dev/null -M virt -machine dumpdtb=$(dtb)
> +	fdtdump $(dtb) > $@
> +
> +arch_clean:
> +	$(RM) $(TEST_DIR)/*.o $(TEST_DIR)/*.flat $(TEST_DIR)/*.elf \
> +	$(libeabi) $(eabiobjs) $(TEST_DIR)/.*.d lib/arm/.*.d \
> +	lib/$(TEST_DIR)/iomaps.gen.c lib/$(TEST_DIR)/mach-virt.*
> diff --git a/configure b/configure
> index 6cfc64943f6e6..296c70182ea1d 100755
> --- a/configure
> +++ b/configure
> @@ -6,8 +6,7 @@ cc=gcc
>  ld=ld
>  objcopy=objcopy
>  ar=ar
> -arch=`uname -m | sed -e s/i.86/i386/`
> -processor="$arch"
> +arch=`uname -m | sed -e s/i.86/i386/ | sed -e 's/arm.*/arm/'`
>  cross_prefix=
>  
>  usage() {
> @@ -17,6 +16,7 @@ usage() {
>  	Options include:
>  	    --test-dir=DIR         the main directory for tests ($arch)
>  	    --arch=ARCH            architecture to compile for ($arch)
> +	    --processor=PROCESSOR  processor to compile for ($arch)
>  	    --cross-prefix=PREFIX  cross compiler prefix
>  	    --cc=CC		   c compiler to use ($cc)
>  	    --ld=LD		   ld linker to use ($ld)
> @@ -66,6 +66,9 @@ while [[ "$1" = -* ]]; do
>  	    ;;
>      esac
>  done
> +[ -z "$processor" ] && processor="$arch"
> +qemu="${QEMU:-qemu-system-$arch}"
> +
>  if [ -z "$testdir" -a \( "$arch" = "i386" -o "$arch" = "x86_64" \) ]; then
>      testdir=x86
>  elif [ -z "$testdir" ]; then
> @@ -80,6 +83,7 @@ if [ -f $testdir/run ]; then
>  fi
>  
>  # check for dependent 32 bit libraries
> +if [ "$arch" = "i386" -o "$arch" = "x86_64" ]; then
>  cat << EOF > lib_test.c
>  #include <stdc++.h>
>  #include <boost_thread-mt.h>
> @@ -94,6 +98,7 @@ if [ $exit -eq 0 ]; then
>      api=true
>  fi
>  rm -f lib_test.c
> +fi
>  
>  cat <<EOF > config.mak
>  PREFIX=$prefix
> @@ -106,4 +111,5 @@ OBJCOPY=$cross_prefix$objcopy
>  AR=$cross_prefix$ar
>  API=$api
>  TEST_DIR=$testdir
> +QEMU_BIN=$qemu
>  EOF
> diff --git a/lib/arm/eabi_compat.c b/lib/arm/eabi_compat.c
> new file mode 100644
> index 0000000000000..76e04f5543ee1
> --- /dev/null
> +++ b/lib/arm/eabi_compat.c
> @@ -0,0 +1,20 @@
> +/*
> + * Adapted from u-boot's arch/arm/lib/eabi_compat.c
> + */
> +#include "libcflat.h"
> +
> +int raise(int signum __unused)
> +{
> +	printf("Divide by zero!\n");
> +	exit(ERANGE);
> +	return 0;
> +}
> +
> +/* Dummy functions to avoid linker complaints */
> +void __aeabi_unwind_cpp_pr0(void)
> +{
> +}
> +
> +void __aeabi_unwind_cpp_pr1(void)
> +{
> +}
> diff --git a/lib/arm/io.c b/lib/arm/io.c
> new file mode 100644
> index 0000000000000..32c896c29450a
> --- /dev/null
> +++ b/lib/arm/io.c
> @@ -0,0 +1,31 @@
> +#include "libcflat.h"
> +#include "libio.h"
> +#include "iomaps.h"
> +#include "virtio-testdev.h"
> +
> +static volatile u8 *uart0_base;
> +
> +void puts(const char *s)
> +{
> +	while (*s)
> +		*uart0_base = *s++;
> +}
> +
> +void exit(int code)
> +{
> +	virtio_testdev_exit(code);
> +	halt(code);
> +}
> +
> +void io_init_early(void)
> +{
> +	const struct iomap *m = iomaps_find_compatible("arm,pl011");
> +	if (!m)
> +		halt(ENXIO);
> +	uart0_base = (u8 *)compat_ptr(m->addrs[0]);
> +}

is io_init_early going to do something else later on or is it just
early_console_init?  If the latter, then name the function as such.

> +
> +void io_init(void)
> +{
> +	virtio_testdev_init();
> +}
> diff --git a/lib/arm/io.h b/lib/arm/io.h
> new file mode 100644
> index 0000000000000..f058f7e54d4a7
> --- /dev/null
> +++ b/lib/arm/io.h
> @@ -0,0 +1,24 @@
> +#ifndef _ARM_IO_H_
> +#define _ARM_IO_H_
> +
> +#define cpu_is_be cpu_is_be

huh?

> +extern bool cpu_is_be;
> +
> +#define __bswap16 bswap16
> +static inline u16 bswap16(u16 val)
> +{
> +	u16 ret;
> +	asm volatile("rev16 %0, %1" : "=r" (ret) :  "r" (val));
> +	return ret;
> +}
> +
> +#define __bswap32 bswap32
> +static inline u32 bswap32(u32 val)
> +{
> +	u32 ret;
> +	asm volatile("rev %0, %1" : "=r" (ret) :  "r" (val));
> +	return ret;
> +}
> +
> +#include "libio.h"
> +#endif
> diff --git a/lib/arm/setup.c b/lib/arm/setup.c
> new file mode 100644
> index 0000000000000..32fa84bd0bb5b
> --- /dev/null
> +++ b/lib/arm/setup.c
> @@ -0,0 +1,85 @@
> +#include "libcflat.h"
> +#include "libio.h"
> +#include "heap.h"
> +#include "arm/sysinfo.h"
> +
> +#define FDT_SIG			0xd00dfeed
> +
> +#define KERNEL_OFFSET		0x00010000
> +#define ATAG_OFFSET		0x00000100
> +
> +#define ATAG_CORE		0x54410001
> +#define ATAG_MEM		0x54410002
> +#define ATAG_CMDLINE		0x54410009
> +
> +extern void start(void);
> +extern unsigned long stacktop;
> +extern char *__args;
> +
> +extern void io_init_early(void);
> +extern void io_init(void);
> +extern void __setup_args(void);
> +
> +u32 mach_type_id;
> +struct tag_core core;
> +struct tag_mem32 mem32;
> +
> +static void read_atags(u32 id, u32 *info)
> +{
> +	u32 *p = info;
> +
> +	if (!p) {
> +		printf("Can't find bootinfo. mach-type = %x\n", id);
> +		exit(ENOEXEC);
> +	}
> +
> +	/*
> +	 * p[0]	count of words for the tag
> +	 * p[1]	tag id
> +	 * p[2..]	tag data
> +	 */
> +	for (; p[0] != 0; p += p[0])

braces, please.

> +		switch (p[1]) {
> +		case ATAG_CORE:
> +			core.flags = p[2];
> +			core.pagesize = p[3];
> +			core.rootdev = p[4];
> +			break;
> +		case ATAG_MEM:
> +			mem32.size = p[2];
> +			mem32.start = p[3];
> +			break;
> +		case ATAG_CMDLINE:
> +			__args = (char *)&p[2];
> +			break;
> +		}
> +}
> +
> +static void read_bootinfo(u32 id, u32 *info)
> +{
> +	u32 *atags = NULL;
> +
> +	mach_type_id = id;
> +
> +	if (info[0] == be32_to_cpu(FDT_SIG)) {
> +		/*
> +		 * fdt reading is not [yet?] implemented. So calculate
> +		 * the ATAGS addr to read that instead.
> +		 */
> +		atags = (u32 *)(start - KERNEL_OFFSET + ATAG_OFFSET);

are the atags always supposed to be loaded even for device-tree booting?
I could not find this in any ARM booting documents, and QEMU does seem
to do this, but only for auto-generated device trees, which could just
be a bug in do_cpu_reset?

> +	} else if (info[1] == ATAG_CORE)
> +		atags = info;
> +
> +	read_atags(id, atags);
> +}
> +
> +void setup(u32 arg __unused, u32 id, u32 *info)
> +{
> +	io_init_early();
> +	read_bootinfo(id, info);
> +	__setup_args();
> +	heap_init(&stacktop,
> +		  mem32.size - (ptr_to_compat(&stacktop) - mem32.start),
> +		  core.pagesize);
> +	io_init();
> +}
> diff --git a/lib/arm/sysinfo.h b/lib/arm/sysinfo.h
> new file mode 100644
> index 0000000000000..f3b076e1a34c4
> --- /dev/null
> +++ b/lib/arm/sysinfo.h
> @@ -0,0 +1,19 @@
> +#ifndef _ARM_SYSINFO_H_
> +#define _ARM_SYSINFO_H_
> +#include "libcflat.h"
> +
> +struct tag_core {
> +	u32 flags;		/* bit 0 = read-only */
> +	u32 pagesize;
> +	u32 rootdev;
> +};
> +
> +struct tag_mem32 {
> +	u32   size;
> +	u32   start;	/* physical start address */
> +};
> +
> +extern u32 mach_type_id;
> +extern struct tag_core core;
> +extern struct tag_mem32 mem32;
> +#endif
> diff --git a/lib/libcflat.h b/lib/libcflat.h
> index 197b703e53b46..8c6cf1f0735ba 100644
> --- a/lib/libcflat.h
> +++ b/lib/libcflat.h
> @@ -45,6 +45,7 @@ extern char *strcat(char *dest, const char *src);
>  extern int strcmp(const char *a, const char *b);
>  
>  extern int printf(const char *fmt, ...);
> +extern int snprintf(char *buf, int size, const char *fmt, ...);
>  extern int vsnprintf(char *buf, int size, const char *fmt, va_list va);
>  
>  extern void puts(const char *s);
> @@ -60,6 +61,7 @@ extern long atol(const char *ptr);
>  		const typeof( ((type *)0)->member ) *__mptr = (ptr);	\
>  		(type *)( (char *)__mptr - offsetof(type,member) );})
>  
> +#define __unused __attribute__((__unused__))
>  #define NULL ((void *)0UL)
>  #include "errno.h"
>  #endif
> diff --git a/lib/libio.h b/lib/libio.h
> index 210b8d78af43f..5ac1056e7002f 100644
> --- a/lib/libio.h
> +++ b/lib/libio.h
> @@ -6,6 +6,10 @@
>   */
>  #include "libcflat.h"
>  
> +#ifdef __arm__
> +#include "arm/io.h"
> +#endif
> +
>  typedef u32 compat_ptr_t;
>  
>  static inline void *compat_ptr(compat_ptr_t ptr)
> diff --git a/lib/test_util.c b/lib/test_util.c
> new file mode 100644
> index 0000000000000..3de1f74f83455
> --- /dev/null
> +++ b/lib/test_util.c
> @@ -0,0 +1,34 @@
> +#include "libcflat.h"
> +#include "test_util.h"
> +
> +bool enough_args(int nargs, int needed)
> +{
> +	if (nargs >= needed)
> +		return true;
> +
> +	fail("Not enough arguments.\n");
> +	return false;
> +}
> +
> +/*
> + * Typically one would compare val == strtoul(expected, endp, base),
> + * but we don't have, nor at this point really need, strtoul, so we
> + * convert val to a string instead. base can only be 10 or 16.
> + */
> +bool check_u32(u32 val, int base, char *expected)
> +{
> +	char *fmt = base == 10 ? "%d" : "%x";
> +	char val_str[16];
> +
> +	snprintf(val_str, 16, fmt, val);
> +
> +	if (base == 16)
> +		while (*expected == '0' || *expected == 'x')
> +			++expected;
> +
> +	if (strcmp(val_str, expected) == 0)
> +		return true;
> +
> +	fail("expected %s, but have %s\n", expected, val_str);
> +	return false;
> +}
> diff --git a/lib/test_util.h b/lib/test_util.h
> new file mode 100644
> index 0000000000000..0e3e6c4a80d51
> --- /dev/null
> +++ b/lib/test_util.h
> @@ -0,0 +1,13 @@
> +#ifndef _TEST_UTIL_H_
> +#define _TEST_UTIL_H_
> +#include "libcflat.h"
> +
> +#define PASS 0
> +#define FAIL 1
> +
> +#define pass(fmt...) printf("PASS: " fmt)
> +#define fail(fmt...) printf("FAIL: " fmt)
> +
> +bool enough_args(int nargs, int needed);
> +bool check_u32(u32 val, int base, char *expected);
> +#endif
> -- 
> 1.8.1.4
> 

I was expecting to see a __raw_... IO accessor definitions for ARM here,
specifically so we avoid the register-writeback versions that are not
supported on ARM.

See arch/arm/include/asm/io.h in the kernel.

-- 
Christoffer