[9/9] arm: initial drop

Message ID	1386175377-23086-10-git-send-email-drjones@redhat.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <kvm-owner@kernel.org> From: Andrew Jones <drjones@redhat.com> To: kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org Subject: [PATCH 9/9] arm: initial drop Date: Wed, 4 Dec 2013 17:42:57 +0100 Message-Id: <1386175377-23086-10-git-send-email-drjones@redhat.com> In-Reply-To: <1386175377-23086-1-git-send-email-drjones@redhat.com> References: <1386175377-23086-1-git-send-email-drjones@redhat.com> Sender: kvm-owner@vger.kernel.org Precedence: bulk

Andrew Jones Dec. 4, 2013, 4:42 p.m. UTC

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

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

Christoffer Dall Dec. 29, 2013, 6:31 a.m. UTC | #1

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.

Peter Maydell Dec. 29, 2013, 9:18 a.m. UTC | #2

On 29 December 2013 06:31, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> 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?

Heh, yes, that's just a bug in QEMU -- I didn't notice when I added the
support for autogenerated device trees that we were using "is there a
dtb filename?" to trigger whether to write ATAGS info in reset. I'll fix
that at some point soon...

thanks
-- PMM
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andrew Jones Jan. 2, 2014, 4:54 p.m. UTC | #3

On Sat, Dec 28, 2013 at 10:31:35PM -0800, Christoffer Dall wrote:
> 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= ?

Two different vars. QEMU_BIN is an internal makefile var set in
config.mak. QEMU is an ENV. Hmm, I guess I could reuse the QEMU
name in the makefiles too...

<snip>
> > +	return ret;
> > +}
> 
> consider renaming this file to boottest.c to avoid the confusion that
> this file is needed to boot anything that runs tests...

I think I'll rename it to selftest.c, as it's really just a test
framework selftest.

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

Correct. I guess I can add a 'see Documentation/arm/Booting comment'

> 
> > +	ldr	sp, =stacktop
> > +
> > +	mrc	p15, 0, r8, c1, c0, 0	@r8 = sctrl
> 
> that's sctlr, not sctrl.

fixed

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

Yes, qemu allows big-endian. I haven't tested it though, but suspect
someday we will want big-endian guests tested as well. I'll fix the
detection.

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

correct

> 
> > +# arch = arm/arm64 # Only if the test case works only on one of them
> 
> Do we have any verified support for arm64 yet?

not yet, high on my TODO list though.

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

Agreed.

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

I'll think about how we might/must use '-cpu host'.

> 
> > +CFLAGS += -mcpu=cortex-a15
> > +CFLAGS += -O2
> 
> Why do we choose this particular optimzation-level in the arch-specific
> config?

My cross-compiler was generating broken code with anything less. I
haven't checked later compilers yet to see if it's fixed or not.

> 
> > +
> > +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 :-(

Calls the linker on the *.o files with the supplied linker script, and
a .text addr of $(start_addr). Also gets rid of the build-id section, which
messes up the start address. And supplies libs to link with, which need to
be in a particular order (defined in FLATLIBS).

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

Correct. Eventually we'll have tests-common, which will be the majority
and apply to both, and also tests which are specific to one.

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

I'll rename for now, as I don't know if it will do something else
yet. I guess we can rename it again if it ever does.

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

See lib/libio.h, it has

#ifndef cpu_is_be
#define cpu_is_be 0
#endif

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

added.

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

Looks like Peter confirms I can't rely on this anymore. I guess it's
time to just be one with libfdt.

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

k, I'll grab them, but they'll go in lib/arm/io.h. I think I'll drop
these lib/test_util.* in v3, so far they're fairly useless cruft. We
can bring them back if they have enough purpose later.

drew
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Peter Maydell Jan. 2, 2014, 5:40 p.m. UTC | #4

On 2 January 2014 16:54, Andrew Jones <drjones@redhat.com> wrote:
> On Sat, Dec 28, 2013 at 10:31:35PM -0800, Christoffer Dall wrote:
>> On Wed, Dec 04, 2013 at 05:42:57PM +0100, Andrew Jones wrote:
>> > +   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.
>
> Yes, qemu allows big-endian. I haven't tested it though, but suspect
> someday we will want big-endian guests tested as well. I'll fix the
> detection.

QEMU does not support system mode big-endian. (Some of the
pieces are there for it but we have no board that needs it.)

Also, you should read up in the ARM ARM about the differences
between old style BE32 and new BE8. (The quick summary is here:
http://translatedcode.wordpress.com/2012/04/)
What you're trying to do here is enable old-style BE32. This is
not supported on any ARMv7 or later CPU, which means it does
not overlap at all with virtualization support.

We probably do care about BE8 big-endian guest support testing.
(kvmtool can run guests in this mode; QEMU doesn't currently).

thanks
-- PMM
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Christoffer Dall Jan. 2, 2014, 5:44 p.m. UTC | #5

On Thu, Jan 02, 2014 at 05:54:25PM +0100, Andrew Jones wrote:
> On Sat, Dec 28, 2013 at 10:31:35PM -0800, Christoffer Dall wrote:
> > 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= ?
> 
> Two different vars. QEMU_BIN is an internal makefile var set in
> config.mak. QEMU is an ENV. Hmm, I guess I could reuse the QEMU
> name in the makefiles too...
> 
> <snip>
> > > +	return ret;
> > > +}
> > 
> > consider renaming this file to boottest.c to avoid the confusion that
> > this file is needed to boot anything that runs tests...
> 
> I think I'll rename it to selftest.c, as it's really just a test
> framework selftest.
> 
> > 
> > > 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?
> 
> Correct. I guess I can add a 'see Documentation/arm/Booting comment'
> 
> > 
> > > +	ldr	sp, =stacktop
> > > +
> > > +	mrc	p15, 0, r8, c1, c0, 0	@r8 = sctrl
> > 
> > that's sctlr, not sctrl.
> 
> fixed
> 
> > 
> > > +	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.
> 
> Yes, qemu allows big-endian. I haven't tested it though, but suspect
> someday we will want big-endian guests tested as well. I'll fix the
> detection.
> 
> > > +++ 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?
> 
> correct
> 
> > 
> > > +# arch = arm/arm64 # Only if the test case works only on one of them
> > 
> > Do we have any verified support for arm64 yet?
> 
> not yet, high on my TODO list though.
> 
> > > +
> > > +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?
> 
> Agreed.
> 
> > 
> > (also note that you can now use -cpu host with mach-virt which may make
> > your life easier).
> 
> I'll think about how we might/must use '-cpu host'.
> 
> > 
> > > +CFLAGS += -mcpu=cortex-a15
> > > +CFLAGS += -O2
> > 
> > Why do we choose this particular optimzation-level in the arch-specific
> > config?
> 
> My cross-compiler was generating broken code with anything less. I
> haven't checked later compilers yet to see if it's fixed or not.
> 

which GCC version?

> > 
> > > +
> > > +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 :-(
> 
> Calls the linker on the *.o files with the supplied linker script, and
> a .text addr of $(start_addr). Also gets rid of the build-id section, which
> messes up the start address. And supplies libs to link with, which need to
> be in a particular order (defined in FLATLIBS).
> 

thanks.

> > 
> > > +
> > > +$(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?
> 
> Correct. Eventually we'll have tests-common, which will be the majority
> and apply to both, and also tests which are specific to one.
> 
> > > +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.
> 
> I'll rename for now, as I don't know if it will do something else
> yet. I guess we can rename it again if it ever does.
> 
> > 
> > > +
> > > +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;
> 
> See lib/libio.h, it has
> 
> #ifndef cpu_is_be
> #define cpu_is_be 0
> #endif
> 

ah, yuck.

> > > +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.
> 
> added.
> 
> > 
> > > +		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?
> 
> Looks like Peter confirms I can't rely on this anymore. I guess it's
> time to just be one with libfdt.
> 
> > > 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.
> 
> k, I'll grab them, but they'll go in lib/arm/io.h. I think I'll drop
> these lib/test_util.* in v3, so far they're fairly useless cruft. We
> can bring them back if they have enough purpose later.
> 

I already did that for my WIP, see commit
680054064db4dd710991f064a88a12012944d376 in:

https://github.com/columbia/kvm-unit-tests.git

-Christoffer
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Christoffer Dall Jan. 2, 2014, 6:09 p.m. UTC | #6

On Thu, Jan 02, 2014 at 05:40:24PM +0000, Peter Maydell wrote:
> On 2 January 2014 16:54, Andrew Jones <drjones@redhat.com> wrote:
> > On Sat, Dec 28, 2013 at 10:31:35PM -0800, Christoffer Dall wrote:
> >> On Wed, Dec 04, 2013 at 05:42:57PM +0100, Andrew Jones wrote:
> >> > +   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.
> >
> > Yes, qemu allows big-endian. I haven't tested it though, but suspect
> > someday we will want big-endian guests tested as well. I'll fix the
> > detection.
> 
> QEMU does not support system mode big-endian. (Some of the
> pieces are there for it but we have no board that needs it.)
> 
> Also, you should read up in the ARM ARM about the differences
> between old style BE32 and new BE8. (The quick summary is here:
> http://translatedcode.wordpress.com/2012/04/)
> What you're trying to do here is enable old-style BE32. This is
> not supported on any ARMv7 or later CPU, which means it does
> not overlap at all with virtualization support.
> 
> We probably do care about BE8 big-endian guest support testing.
> (kvmtool can run guests in this mode; QEMU doesn't currently).
> 
Yeah, so I think this logic should go away from the test suite for now,
focs on getting a decent set of actual functional tests up and running,
and then probably add aarch64 support before looking at big-endian
support.

Thanks,
-Christoffer
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andrew Jones Jan. 2, 2014, 6:44 p.m. UTC | #7

On Thu, Jan 02, 2014 at 10:09:50AM -0800, Christoffer Dall wrote:
> On Thu, Jan 02, 2014 at 05:40:24PM +0000, Peter Maydell wrote:
> > On 2 January 2014 16:54, Andrew Jones <drjones@redhat.com> wrote:
> > > On Sat, Dec 28, 2013 at 10:31:35PM -0800, Christoffer Dall wrote:
> > >> On Wed, Dec 04, 2013 at 05:42:57PM +0100, Andrew Jones wrote:
> > >> > +   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.
> > >
> > > Yes, qemu allows big-endian. I haven't tested it though, but suspect
> > > someday we will want big-endian guests tested as well. I'll fix the
> > > detection.
> > 
> > QEMU does not support system mode big-endian. (Some of the
> > pieces are there for it but we have no board that needs it.)
> > 
> > Also, you should read up in the ARM ARM about the differences
> > between old style BE32 and new BE8. (The quick summary is here:
> > http://translatedcode.wordpress.com/2012/04/)
> > What you're trying to do here is enable old-style BE32. This is
> > not supported on any ARMv7 or later CPU, which means it does
> > not overlap at all with virtualization support.
> > 
> > We probably do care about BE8 big-endian guest support testing.
> > (kvmtool can run guests in this mode; QEMU doesn't currently).
> > 
> Yeah, so I think this logic should go away from the test suite for now,
> focs on getting a decent set of actual functional tests up and running,
> and then probably add aarch64 support before looking at big-endian
> support.
>

OK, sounds good. The cpu_is_be stuff scattered about should still be fine
to leave in, and we can go about actually detecting BE and setting
cpu_is_be whenever.

thanks,
drew
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andrew Jones Jan. 2, 2014, 6:50 p.m. UTC | #8

On Thu, Jan 02, 2014 at 09:44:39AM -0800, Christoffer Dall wrote:
> > My cross-compiler was generating broken code with anything less. I
> > haven't checked later compilers yet to see if it's fixed or not.
> > 
> 
> which GCC version?

$ arm-linux-gnu-gcc -v
Using built-in specs.
COLLECT_GCC=arm-linux-gnu-gcc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/arm-linux-gnueabi/4.8.1/lto-wrapper
Target: arm-linux-gnueabi
Configured with: ../gcc-4.8.1-20130717/configure --bindir=/usr/bin
--build=x86_64-redhat-linux-gnu --datadir=/usr/share
--disable-decimal-float --disable-dependency-tracking --disable-gold
--disable-libgomp --disable-libmudflap --disable-libquadmath
--disable-libssp --disable-nls --disable-plugin --disable-shared
--disable-silent-rules --disable-sjlj-exceptions --disable-threads
--enable-checking= --enable-gnu-unique-object --enable-initfini-array
--enable-languages=c --enable-linker-build-id --enable-nls
--enable-obsolete --enable-targets=all --exec-prefix=/usr
--host=x86_64-redhat-linux-gnu --includedir=/usr/include
--infodir=/usr/share/info --libexecdir=/usr/libexec --localstatedir=/var
--mandir=/usr/share/man --prefix=/usr --program-prefix=arm-linux-gnu-
--sbindir=/usr/sbin --sharedstatedir=/var/lib --sysconfdir=/etc
--target=arm-linux-gnueabi
--with-bugurl=http://bugzilla.redhat.com/bugzilla/
--with-linker-hash-style=gnu --with-newlib --with-system-libunwind
--with-system-zlib --without-headers
--with-isl=/builddir/build/BUILD/gcc-4.8.1-20130717/isl-install
--with-cloog=/builddir/build/BUILD/gcc-4.8.1-20130717/cloog-install
Thread model: single
gcc version 4.8.1 20130717 (Red Hat 4.8.1-5) (GCC)

> > > 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.
> > 
> > k, I'll grab them, but they'll go in lib/arm/io.h. I think I'll drop
> > these lib/test_util.* in v3, so far they're fairly useless cruft. We
> > can bring them back if they have enough purpose later.
> > 
> 
> I already did that for my WIP, see commit
> 680054064db4dd710991f064a88a12012944d376 in:
> 
> https://github.com/columbia/kvm-unit-tests.git

I'll fetch your tree and start going through it tomorrow. Thanks for
jumping in!!

drew
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Christoffer Dall Jan. 2, 2014, 7:17 p.m. UTC | #9

On Thu, Jan 02, 2014 at 07:50:27PM +0100, Andrew Jones wrote:
> On Thu, Jan 02, 2014 at 09:44:39AM -0800, Christoffer Dall wrote:
> > > My cross-compiler was generating broken code with anything less. I
> > > haven't checked later compilers yet to see if it's fixed or not.
> > > 
> > 
> > which GCC version?
> 
> $ arm-linux-gnu-gcc -v
> Using built-in specs.
> COLLECT_GCC=arm-linux-gnu-gcc
> COLLECT_LTO_WRAPPER=/usr/libexec/gcc/arm-linux-gnueabi/4.8.1/lto-wrapper
> Target: arm-linux-gnueabi
> Configured with: ../gcc-4.8.1-20130717/configure --bindir=/usr/bin
> --build=x86_64-redhat-linux-gnu --datadir=/usr/share
> --disable-decimal-float --disable-dependency-tracking --disable-gold
> --disable-libgomp --disable-libmudflap --disable-libquadmath
> --disable-libssp --disable-nls --disable-plugin --disable-shared
> --disable-silent-rules --disable-sjlj-exceptions --disable-threads
> --enable-checking= --enable-gnu-unique-object --enable-initfini-array
> --enable-languages=c --enable-linker-build-id --enable-nls
> --enable-obsolete --enable-targets=all --exec-prefix=/usr
> --host=x86_64-redhat-linux-gnu --includedir=/usr/include
> --infodir=/usr/share/info --libexecdir=/usr/libexec --localstatedir=/var
> --mandir=/usr/share/man --prefix=/usr --program-prefix=arm-linux-gnu-
> --sbindir=/usr/sbin --sharedstatedir=/var/lib --sysconfdir=/etc
> --target=arm-linux-gnueabi
> --with-bugurl=http://bugzilla.redhat.com/bugzilla/
> --with-linker-hash-style=gnu --with-newlib --with-system-libunwind
> --with-system-zlib --without-headers
> --with-isl=/builddir/build/BUILD/gcc-4.8.1-20130717/isl-install
> --with-cloog=/builddir/build/BUILD/gcc-4.8.1-20130717/cloog-install
> Thread model: single
> gcc version 4.8.1 20130717 (Red Hat 4.8.1-5) (GCC)
> 
> > > > 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.
> > > 
> > > k, I'll grab them, but they'll go in lib/arm/io.h. I think I'll drop
> > > these lib/test_util.* in v3, so far they're fairly useless cruft. We
> > > can bring them back if they have enough purpose later.
> > > 
> > 
> > I already did that for my WIP, see commit
> > 680054064db4dd710991f064a88a12012944d376 in:
> > 
> > https://github.com/columbia/kvm-unit-tests.git
> 
> I'll fetch your tree and start going through it tomorrow. Thanks for
> jumping in!!
> 
Sure, if you fix anything in your existing series, I'll be happy to
rebase my patches and take a pass at squashing some of them and such so
we can get them out as an RFC.

-Christoffer
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andrew Jones Jan. 3, 2014, 5:52 p.m. UTC | #10

On Thu, Jan 02, 2014 at 11:17:51AM -0800, Christoffer Dall wrote:
> On Thu, Jan 02, 2014 at 07:50:27PM +0100, Andrew Jones wrote:
> > On Thu, Jan 02, 2014 at 09:44:39AM -0800, Christoffer Dall wrote:
> > > > My cross-compiler was generating broken code with anything less. I
> > > > haven't checked later compilers yet to see if it's fixed or not.
> > > > 
> > > 
> > > which GCC version?
> > 
> > $ arm-linux-gnu-gcc -v
> > Using built-in specs.
> > COLLECT_GCC=arm-linux-gnu-gcc
> > COLLECT_LTO_WRAPPER=/usr/libexec/gcc/arm-linux-gnueabi/4.8.1/lto-wrapper
> > Target: arm-linux-gnueabi
> > Configured with: ../gcc-4.8.1-20130717/configure --bindir=/usr/bin
> > --build=x86_64-redhat-linux-gnu --datadir=/usr/share
> > --disable-decimal-float --disable-dependency-tracking --disable-gold
> > --disable-libgomp --disable-libmudflap --disable-libquadmath
> > --disable-libssp --disable-nls --disable-plugin --disable-shared
> > --disable-silent-rules --disable-sjlj-exceptions --disable-threads
> > --enable-checking= --enable-gnu-unique-object --enable-initfini-array
> > --enable-languages=c --enable-linker-build-id --enable-nls
> > --enable-obsolete --enable-targets=all --exec-prefix=/usr
> > --host=x86_64-redhat-linux-gnu --includedir=/usr/include
> > --infodir=/usr/share/info --libexecdir=/usr/libexec --localstatedir=/var
> > --mandir=/usr/share/man --prefix=/usr --program-prefix=arm-linux-gnu-
> > --sbindir=/usr/sbin --sharedstatedir=/var/lib --sysconfdir=/etc
> > --target=arm-linux-gnueabi
> > --with-bugurl=http://bugzilla.redhat.com/bugzilla/
> > --with-linker-hash-style=gnu --with-newlib --with-system-libunwind
> > --with-system-zlib --without-headers
> > --with-isl=/builddir/build/BUILD/gcc-4.8.1-20130717/isl-install
> > --with-cloog=/builddir/build/BUILD/gcc-4.8.1-20130717/cloog-install
> > Thread model: single
> > gcc version 4.8.1 20130717 (Red Hat 4.8.1-5) (GCC)
> > 
> > > > > 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.
> > > > 
> > > > k, I'll grab them, but they'll go in lib/arm/io.h. I think I'll drop
> > > > these lib/test_util.* in v3, so far they're fairly useless cruft. We
> > > > can bring them back if they have enough purpose later.
> > > > 
> > > 
> > > I already did that for my WIP, see commit
> > > 680054064db4dd710991f064a88a12012944d376 in:
> > > 
> > > https://github.com/columbia/kvm-unit-tests.git
> > 
> > I'll fetch your tree and start going through it tomorrow. Thanks for
> > jumping in!!
> > 
> Sure, if you fix anything in your existing series, I'll be happy to
> rebase my patches and take a pass at squashing some of them and such so
> we can get them out as an RFC.

Lot's of good stuff in there. I also had a start on the psci/smp stuff
(in the form of notes, not code) before the holiday break, so I might have
some more suggestions there once we get the patches rebased and cleaned up.
I'd like to get a v3 posted [and committed] of this series, a version that
uses libfdt, before we get too far along, allowing us to build on a clean
base.

Thanks,
drew
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Christoffer Dall Jan. 3, 2014, 5:55 p.m. UTC | #11

On 3 January 2014 09:52, Andrew Jones <drjones@redhat.com> wrote:
> On Thu, Jan 02, 2014 at 11:17:51AM -0800, Christoffer Dall wrote:
>> On Thu, Jan 02, 2014 at 07:50:27PM +0100, Andrew Jones wrote:
>> > On Thu, Jan 02, 2014 at 09:44:39AM -0800, Christoffer Dall wrote:
>> > > > My cross-compiler was generating broken code with anything less. I
>> > > > haven't checked later compilers yet to see if it's fixed or not.
>> > > >
>> > >
>> > > which GCC version?
>> >
>> > $ arm-linux-gnu-gcc -v
>> > Using built-in specs.
>> > COLLECT_GCC=arm-linux-gnu-gcc
>> > COLLECT_LTO_WRAPPER=/usr/libexec/gcc/arm-linux-gnueabi/4.8.1/lto-wrapper
>> > Target: arm-linux-gnueabi
>> > Configured with: ../gcc-4.8.1-20130717/configure --bindir=/usr/bin
>> > --build=x86_64-redhat-linux-gnu --datadir=/usr/share
>> > --disable-decimal-float --disable-dependency-tracking --disable-gold
>> > --disable-libgomp --disable-libmudflap --disable-libquadmath
>> > --disable-libssp --disable-nls --disable-plugin --disable-shared
>> > --disable-silent-rules --disable-sjlj-exceptions --disable-threads
>> > --enable-checking= --enable-gnu-unique-object --enable-initfini-array
>> > --enable-languages=c --enable-linker-build-id --enable-nls
>> > --enable-obsolete --enable-targets=all --exec-prefix=/usr
>> > --host=x86_64-redhat-linux-gnu --includedir=/usr/include
>> > --infodir=/usr/share/info --libexecdir=/usr/libexec --localstatedir=/var
>> > --mandir=/usr/share/man --prefix=/usr --program-prefix=arm-linux-gnu-
>> > --sbindir=/usr/sbin --sharedstatedir=/var/lib --sysconfdir=/etc
>> > --target=arm-linux-gnueabi
>> > --with-bugurl=http://bugzilla.redhat.com/bugzilla/
>> > --with-linker-hash-style=gnu --with-newlib --with-system-libunwind
>> > --with-system-zlib --without-headers
>> > --with-isl=/builddir/build/BUILD/gcc-4.8.1-20130717/isl-install
>> > --with-cloog=/builddir/build/BUILD/gcc-4.8.1-20130717/cloog-install
>> > Thread model: single
>> > gcc version 4.8.1 20130717 (Red Hat 4.8.1-5) (GCC)
>> >
>> > > > > 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.
>> > > >
>> > > > k, I'll grab them, but they'll go in lib/arm/io.h. I think I'll drop
>> > > > these lib/test_util.* in v3, so far they're fairly useless cruft. We
>> > > > can bring them back if they have enough purpose later.
>> > > >
>> > >
>> > > I already did that for my WIP, see commit
>> > > 680054064db4dd710991f064a88a12012944d376 in:
>> > >
>> > > https://github.com/columbia/kvm-unit-tests.git
>> >
>> > I'll fetch your tree and start going through it tomorrow. Thanks for
>> > jumping in!!
>> >
>> Sure, if you fix anything in your existing series, I'll be happy to
>> rebase my patches and take a pass at squashing some of them and such so
>> we can get them out as an RFC.
>
> Lot's of good stuff in there. I also had a start on the psci/smp stuff
> (in the form of notes, not code) before the holiday break, so I might have
> some more suggestions there once we get the patches rebased and cleaned up.
> I'd like to get a v3 posted [and committed] of this series, a version that
> uses libfdt, before we get too far along, allowing us to build on a clean
> base.
>
Sounds fine, I'll keep building the stuff I need for other
measurements, but fear not, I have magic powers in rebasing complex
systems on top of evolving code bases :)

-Christoffer
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[9/9] arm: initial drop

Commit Message

Comments

Patch