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

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

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