[kvm-unit-tests PATCH 0/7] arm/arm64: Add support for running under kvmtool

[kvm-unit-tests PATCH 0/7] arm/arm64: Add support for running under kvmtool

[Alexandru Elisei]

kvm-unit-tests is designed to be run with QEMU as the virtual machine
monitor. It relies on devices emulated by QEMU (like isa-debug-exit or
testdev) and it makes certain assumptions based on the implicit QEMU
virtual environment configuration (like the serial base address).

kvmtool [1] is a lightweight virtual machine monitor for running KVM
guests. kvmtool has reduced complexity compared to QEMU and is easily
hackable.

This patch series aims to make it possible to run kvm-unit-tests using
kvmtool on the arm and arm64 architectures, with one caveat: when
terminating a test, the userspace process won't exit with an exit code that
signals the success or failure of the test. Output from the test can still
be parsed to determine the outcome of the test.

* Patches 1 and 2 add support for discovering the ns16550a UART emulated by
  kvmtool on arm and arm64.
* Patches 3,4 and 5 provide an alternative mechanism for terminating the
  virtual machine by using PSCI.
* Patches 6 and 7 remove an error when parsing the test parameters
  generated by kvmtool.


[1] https://git.kernel.org/pub/scm/linux/kernel/git/will/kvmtool.git/

Alexandru Elisei (7):
  lib: arm: Discover ns16550a UART
  lib: arm: Remove warning about uart0_base mismatch
  lib: chr-testdev: Make chr_testdev_init() return status
  lib: arm: Implement PSCI SYSTEM_OFF in psci_system_off()
  lib: arm: Fallback to psci_system_off() in exit()
  lib: argv: Implement argv_find() for test parameters
  arm/arm64: Use argv_find() for test names

 lib/arm/asm/psci.h |  1 +
 lib/argv.h         |  1 +
 lib/chr-testdev.h  |  2 +-
 lib/argv.c         | 11 +++++++++++
 lib/arm/io.c       | 38 +++++++++++++++++++++++---------------
 lib/arm/psci.c     |  6 ++++++
 lib/chr-testdev.c  |  8 +++++---
 arm/gic.c          |  7 ++++---
 arm/selftest.c     | 19 ++++++++++++-------
 9 files changed, 64 insertions(+), 29 deletions(-)

-- 
2.17.0

[kvm-unit-tests PATCH 1/7] lib: arm: Discover ns16550a UART

[Alexandru Elisei]

Add support for discovering the ns16550a UART from the device tree. This
particular UART model is emulated by kvmtool.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 lib/arm/io.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/lib/arm/io.c b/lib/arm/io.c
index d2c1a07c19ee..35fc05aeb4db 100644
--- a/lib/arm/io.c
+++ b/lib/arm/io.c
@@ -32,22 +32,26 @@ static volatile u8 *uart0_base = (u8 *)UART_EARLY_BASE;
 
 static void uart0_init(void)
 {
-	const char *compatible = "arm,pl011";
+	const char *compatible[] = {"arm,pl011", "ns16550a"};
 	struct dt_pbus_reg base;
-	int ret;
+	int i, ret;
 
 	ret = dt_get_default_console_node();
 	assert(ret >= 0 || ret == -FDT_ERR_NOTFOUND);
 
 	if (ret == -FDT_ERR_NOTFOUND) {
 
-		ret = dt_pbus_get_base_compatible(compatible, &base);
-		assert(ret == 0 || ret == -FDT_ERR_NOTFOUND);
+		for (i = 0; i < ARRAY_SIZE(compatible); i++) {
+			ret = dt_pbus_get_base_compatible(compatible[i], &base);
+			assert(ret == 0 || ret == -FDT_ERR_NOTFOUND);
+
+			if (ret == 0)
+				break;
+		}
 
 		if (ret) {
-			printf("%s: %s not found in the device tree, "
-				"aborting...\n",
-				__func__, compatible);
+			printf("%s: Compatible UART not found in the device tree, "
+				"aborting...\n", __func__);
 			abort();
 		}
 
-- 
2.17.0

[kvm-unit-tests PATCH 2/7] lib: arm: Remove warning about uart0_base mismatch

[Alexandru Elisei]

A warning is displayed if uart0_base is different from what the code
expects qemu to generate for the pl011 UART in the device tree. However,
now we support the ns16550a UART emulated by kvmtool, which has a
different address. This leads to the  warning being displayed even though
the UART is configured and working as expected.

Now that we support multiple UARTs, the warning serves no purpose, so
remove it.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 lib/arm/io.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/lib/arm/io.c b/lib/arm/io.c
index 35fc05aeb4db..87435150f73e 100644
--- a/lib/arm/io.c
+++ b/lib/arm/io.c
@@ -61,12 +61,6 @@ static void uart0_init(void)
 	}
 
 	uart0_base = ioremap(base.addr, base.size);
-
-	if (uart0_base != (u8 *)UART_EARLY_BASE) {
-		printf("WARNING: early print support may not work. "
-		       "Found uart at %p, but early base is %p.\n",
-			uart0_base, (u8 *)UART_EARLY_BASE);
-	}
 }
 
 void io_init(void)
-- 
2.17.0

[kvm-unit-tests PATCH 3/7] lib: chr-testdev: Make chr_testdev_init() return status

[Alexandru Elisei]

Make chr_testdev_init() return 0 (success) if the virtio console was
initialized properly, otherwise return -1 (failure).

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 lib/chr-testdev.h | 2 +-
 lib/chr-testdev.c | 8 +++++---
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/lib/chr-testdev.h b/lib/chr-testdev.h
index ffd9a851aa9b..fdd0582e2da1 100644
--- a/lib/chr-testdev.h
+++ b/lib/chr-testdev.h
@@ -9,6 +9,6 @@
  *
  * This work is licensed under the terms of the GNU LGPL, version 2.
  */
-extern void chr_testdev_init(void);
+extern int chr_testdev_init(void);
 extern void chr_testdev_exit(int code);
 #endif
diff --git a/lib/chr-testdev.c b/lib/chr-testdev.c
index 6890f63c8b29..26e14301e3db 100644
--- a/lib/chr-testdev.c
+++ b/lib/chr-testdev.c
@@ -47,7 +47,7 @@ out:
 	spin_unlock(&lock);
 }
 
-void chr_testdev_init(void)
+int chr_testdev_init(void)
 {
 	const char *io_names[] = { "input", "output" };
 	struct virtqueue *vqs[2];
@@ -57,7 +57,7 @@ void chr_testdev_init(void)
 	if (vcon == NULL) {
 		printf("%s: %s: can't find a virtio-console\n",
 				__func__, TESTDEV_NAME);
-		return;
+		return -1;
 	}
 
 	ret = vcon->config->find_vqs(vcon, 2, vqs, NULL, io_names);
@@ -65,9 +65,11 @@ void chr_testdev_init(void)
 		printf("%s: %s: can't init virtqueues\n",
 				__func__, TESTDEV_NAME);
 		vcon = NULL;
-		return;
+		return -1;
 	}
 
 	in_vq = vqs[0];
 	out_vq = vqs[1];
+
+	return 0;
 }
-- 
2.17.0

[kvm-unit-tests PATCH 4/7] lib: arm: Implement PSCI SYSTEM_OFF in psci_system_off()

[Alexandru Elisei]

A new function psci_system_off() is added which implements the PSCI
SYSTEM_OFF function. A call causes the hypervisor to terminate the virtual
machine.

Consumers for the function will be added in a later patch.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 lib/arm/asm/psci.h | 1 +
 lib/arm/psci.c     | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/lib/arm/asm/psci.h b/lib/arm/asm/psci.h
index ed51708fd265..93409996349a 100644
--- a/lib/arm/asm/psci.h
+++ b/lib/arm/asm/psci.h
@@ -9,5 +9,6 @@ extern int psci_cpu_on(unsigned long cpuid, unsigned long entry_point);
 extern void psci_sys_reset(void);
 extern int cpu_psci_cpu_boot(unsigned int cpu);
 extern void cpu_psci_cpu_die(void);
+extern void psci_system_off(void);
 
 #endif /* _ASMARM_PSCI_H_ */
diff --git a/lib/arm/psci.c b/lib/arm/psci.c
index 119f74e57e91..d8dc428e8e8b 100644
--- a/lib/arm/psci.c
+++ b/lib/arm/psci.c
@@ -48,6 +48,12 @@ void cpu_psci_cpu_die(void)
 	printf("CPU%d unable to power off (error = %d)\n", smp_processor_id(), err);
 }
 
+void psci_system_off(void)
+{
+	int err = psci_invoke(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
+	printf("CPU%d unable to do system off (error = %d)\n", smp_processor_id(), err);
+}
+
 void psci_sys_reset(void)
 {
 	psci_invoke(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
-- 
2.17.0

[kvm-unit-tests PATCH 5/7] lib: arm: Fallback to psci_system_off() in exit()

[Alexandru Elisei]

On arm and arm64, kvm-unit-tests uses the QEMU chr-testdev device to shut
down the virtual machine at the end of a test. The function
psci_system_off() provides another mechanism for terminating the virtual
machine. If the chr-testdev device hasn't been initialized successfully,
then use psci_system_off() to terminate the test instead of
chr_testdev_exit().

chr-testdev is implemented on top of virtio console. This patch makes it
possible for a virtual machine manager which doesn't have support for
chr-testdev, but has been configured not to emulate a virtio console, to
gracefully terminate a virtual machine after a test has been completed.

There is one limitation to using psci_system_off() to terminate a test:
chr-testdev allows kvm-unit-tests to specify an exit code;
psci_system_off() has no such mechanism.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 lib/arm/io.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/lib/arm/io.c b/lib/arm/io.c
index 87435150f73e..9fe9bd0bf659 100644
--- a/lib/arm/io.c
+++ b/lib/arm/io.c
@@ -11,6 +11,7 @@
 #include <libcflat.h>
 #include <devicetree.h>
 #include <chr-testdev.h>
+#include "arm/asm/psci.h"
 #include <asm/spinlock.h>
 #include <asm/io.h>
 
@@ -18,6 +19,8 @@
 
 extern void halt(int code);
 
+static bool testdev_enabled;
+
 /*
  * Use this guess for the pl011 base in order to make an attempt at
  * having earlier printf support. We'll overwrite it with the real
@@ -65,8 +68,12 @@ static void uart0_init(void)
 
 void io_init(void)
 {
+	int err;
+
 	uart0_init();
-	chr_testdev_init();
+	err = chr_testdev_init();
+	if (!err)
+		testdev_enabled = true;
 }
 
 void puts(const char *s)
@@ -79,7 +86,10 @@ void puts(const char *s)
 
 void exit(int code)
 {
-	chr_testdev_exit(code);
+	if (testdev_enabled)
+		chr_testdev_exit(code);
+	else
+		psci_system_off();
 	halt(code);
 	__builtin_unreachable();
 }
-- 
2.17.0

[kvm-unit-tests PATCH 6/7] lib: argv: Implement argv_find() for test parameters

[Alexandru Elisei]

The current behavior when searching for a specific parameter is to exit
with an error when the first unexpected parameter is found.

The function argv_find() will search for a specific parameter, ignoring all
other parameters.

Consumers for the function will be added in a later patch.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 lib/argv.h |  1 +
 lib/argv.c | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/lib/argv.h b/lib/argv.h
index 2104dd42748d..efc390ae4fae 100644
--- a/lib/argv.h
+++ b/lib/argv.h
@@ -8,3 +8,4 @@
 extern void __setup_args(void);
 extern void setup_args_progname(const char *args);
 extern void setup_env(char *env, int size);
+extern int argv_find(const char *needle, int argc, char **argv);
diff --git a/lib/argv.c b/lib/argv.c
index f0e183a8f02f..6e4593997e0b 100644
--- a/lib/argv.c
+++ b/lib/argv.c
@@ -144,3 +144,14 @@ void setup_env(char *env, int size)
 		*env++ = '\0';
 	}
 }
+
+int argv_find(const char *needle, int argc, char **argv)
+{
+	int i;
+
+	for (i = 0; i < argc; i++)
+		if (strcmp(argv[i], needle) == 0)
+			return i;
+
+	return -1;
+}
-- 
2.17.0

[kvm-unit-tests PATCH 7/7] arm/arm64: Use argv_find() for test names

[Alexandru Elisei]

Instead of aborting the test when an unexpected parameter is found, use
argv_find() to search for the desired parameter. On arm and arm64, this
allows kvm-unit-tests to be used with virtual machine monitors like kvmtool
which automatically adds parameters to the kernel command line.

For example, to run the gicv3-ipi test the following command was used:

lkvm run -k gic.flat -c 8 -m 410 -p "ipi" --irqchip=gicv3

The resulting kernel command line that was passed to kvm-unit-tests was:

"console=ttyS0 rw rootflags=trans=virtio,version=9p2000.L,cache=loose rootfstype=9p init=/virt/init  ip=dhcp ipi"

Without the patch, the following error occurs:
"ABORT: gicv3: Unknown subtest 'console=ttyS0'"

With the patch, the test is able to run.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/gic.c      |  7 ++++---
 arm/selftest.c | 19 ++++++++++++-------
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index ed5642e74f70..f3746de4bfd8 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -12,6 +12,7 @@
  * This work is licensed under the terms of the GNU LGPL, version 2.
  */
 #include <libcflat.h>
+#include <argv.h>
 #include <asm/setup.h>
 #include <asm/processor.h>
 #include <asm/delay.h>
@@ -533,13 +534,13 @@ int main(int argc, char **argv)
 	if (argc < 2)
 		report_abort("no test specified");
 
-	if (strcmp(argv[1], "ipi") == 0) {
+	if (argv_find("ipi", argc, argv) != -1) {
 		report_prefix_push(argv[1]);
 		nr_cpu_check(2);
 		on_cpus(ipi_test, NULL);
-	} else if (strcmp(argv[1], "active") == 0) {
+	} else if (argv_find("active", argc, argv) != -1) {
 		run_active_clear_test();
-	} else if (strcmp(argv[1], "mmio") == 0) {
+	} else if (argv_find("mmio", argc, argv) != -1) {
 		report_prefix_push(argv[1]);
 		gic_test_mmio();
 		report_prefix_pop();
diff --git a/arm/selftest.c b/arm/selftest.c
index ea5101ef7217..44b2f23ca115 100644
--- a/arm/selftest.c
+++ b/arm/selftest.c
@@ -8,6 +8,7 @@
 #include <libcflat.h>
 #include <util.h>
 #include <devicetree.h>
+#include <argv.h>
 #include <asm/setup.h>
 #include <asm/ptrace.h>
 #include <asm/asm-offsets.h>
@@ -319,28 +320,32 @@ static void cpu_report(void *data __unused)
 
 int main(int argc, char **argv)
 {
+	int pos;
+
 	report_prefix_push("selftest");
 
 	if (argc < 2)
 		report_abort("no test specified");
 
-	report_prefix_push(argv[1]);
-
-	if (strcmp(argv[1], "setup") == 0) {
+	if ((pos = argv_find("setup", argc, argv)) != -1) {
 
-		check_setup(argc-2, &argv[2]);
+		report_prefix_push("setup");
+		check_setup(argc-(pos+1), &argv[pos+1]);
 
-	} else if (strcmp(argv[1], "vectors-kernel") == 0) {
+	} else if (argv_find("vectors-kernel", argc, argv) != -1) {
 
+		report_prefix_push("vectors-kernel");
 		check_vectors(NULL);
 
-	} else if (strcmp(argv[1], "vectors-user") == 0) {
+	} else if (argv_find("vectors-user", argc, argv) != -1) {
 
+		report_prefix_push("vectors-user");
 		start_usr(check_vectors, NULL,
 				(unsigned long)thread_stack_alloc());
 
-	} else if (strcmp(argv[1], "smp") == 0) {
+	} else if (argv_find("smp", argc, argv) != -1) {
 
+		report_prefix_push("smp");
 		report("PSCI version", psci_check());
 		on_cpus(cpu_report, NULL);
 
-- 
2.17.0

Re: [kvm-unit-tests PATCH 1/7] lib: arm: Discover ns16550a UART

[Andre Przywara]

On Thu, 24 Jan 2019 11:16:28 +0000
Alexandru Elisei <alexandru.elisei@arm.com> wrote:

> Add support for discovering the ns16550a UART from the device tree.
> This particular UART model is emulated by kvmtool.

Maybe we should mention that this happens to work because both UARTs
have their TX data register at offset 0, because they are by no means
"compatible" otherwise.
But that just a documentation issue, so:

> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

> ---
>  lib/arm/io.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/arm/io.c b/lib/arm/io.c
> index d2c1a07c19ee..35fc05aeb4db 100644
> --- a/lib/arm/io.c
> +++ b/lib/arm/io.c
> @@ -32,22 +32,26 @@ static volatile u8 *uart0_base = (u8
> *)UART_EARLY_BASE; 
>  static void uart0_init(void)
>  {
> -	const char *compatible = "arm,pl011";
> +	const char *compatible[] = {"arm,pl011", "ns16550a"};
>  	struct dt_pbus_reg base;
> -	int ret;
> +	int i, ret;
>  
>  	ret = dt_get_default_console_node();
>  	assert(ret >= 0 || ret == -FDT_ERR_NOTFOUND);
>  
>  	if (ret == -FDT_ERR_NOTFOUND) {
>  
> -		ret = dt_pbus_get_base_compatible(compatible, &base);
> -		assert(ret == 0 || ret == -FDT_ERR_NOTFOUND);
> +		for (i = 0; i < ARRAY_SIZE(compatible); i++) {
> +			ret =
> dt_pbus_get_base_compatible(compatible[i], &base);
> +			assert(ret == 0 || ret == -FDT_ERR_NOTFOUND);
> +
> +			if (ret == 0)
> +				break;
> +		}
>  
>  		if (ret) {
> -			printf("%s: %s not found in the device tree,
> "
> -				"aborting...\n",
> -				__func__, compatible);
> +			printf("%s: Compatible UART not found in the
> device tree, "
> +				"aborting...\n", __func__);
>  			abort();
>  		}
>  

Re: [kvm-unit-tests PATCH 2/7] lib: arm: Remove warning about uart0_base mismatch

[Andre Przywara]

On Thu, 24 Jan 2019 11:16:29 +0000
Alexandru Elisei <alexandru.elisei@arm.com> wrote:

> A warning is displayed if uart0_base is different from what the code
> expects qemu to generate for the pl011 UART in the device tree.
> However, now we support the ns16550a UART emulated by kvmtool, which
> has a different address. This leads to the  warning being displayed
> even though the UART is configured and working as expected.
> 
> Now that we support multiple UARTs, the warning serves no purpose, so
> remove it.

Mmh, but we use that address before, right? So for anything not
emulating an UART at this QEMU specific address we write to some random
(device) memory?

Drew, how important is this early print feature for kvm-unit-tests?

Cheers,
Andre.

> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  lib/arm/io.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/lib/arm/io.c b/lib/arm/io.c
> index 35fc05aeb4db..87435150f73e 100644
> --- a/lib/arm/io.c
> +++ b/lib/arm/io.c
> @@ -61,12 +61,6 @@ static void uart0_init(void)
>  	}
>  
>  	uart0_base = ioremap(base.addr, base.size);
> -
> -	if (uart0_base != (u8 *)UART_EARLY_BASE) {
> -		printf("WARNING: early print support may not work. "
> -		       "Found uart at %p, but early base is %p.\n",
> -			uart0_base, (u8 *)UART_EARLY_BASE);
> -	}
>  }
>  
>  void io_init(void)

Re: [kvm-unit-tests PATCH 2/7] lib: arm: Remove warning about uart0_base mismatch

[Andrew Jones]

On Thu, Jan 24, 2019 at 11:59:43AM +0000, Andre Przywara wrote:
> On Thu, 24 Jan 2019 11:16:29 +0000
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> > A warning is displayed if uart0_base is different from what the code
> > expects qemu to generate for the pl011 UART in the device tree.
> > However, now we support the ns16550a UART emulated by kvmtool, which
> > has a different address. This leads to the  warning being displayed
> > even though the UART is configured and working as expected.
> > 
> > Now that we support multiple UARTs, the warning serves no purpose, so
> > remove it.
> 
> Mmh, but we use that address before, right? So for anything not
> emulating an UART at this QEMU specific address we write to some random
> (device) memory?
> 
> Drew, how important is this early print feature for kvm-unit-tests?

The setup code passes through quite a few asserts before getting through
io_init() (including in uart0_init), so I think there's still value in
having a guessed UART address. Maybe we can provide guesses for both
QEMU and kvmtool, and some selection method, that would be used until
we've properly assigned uart0_base from DT?

> 
> Cheers,
> Andre.
> 
> > 
> > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > ---
> >  lib/arm/io.c | 6 ------
> >  1 file changed, 6 deletions(-)
> > 
> > diff --git a/lib/arm/io.c b/lib/arm/io.c
> > index 35fc05aeb4db..87435150f73e 100644
> > --- a/lib/arm/io.c
> > +++ b/lib/arm/io.c
> > @@ -61,12 +61,6 @@ static void uart0_init(void)
> >  	}
> >  
> >  	uart0_base = ioremap(base.addr, base.size);
> > -
> > -	if (uart0_base != (u8 *)UART_EARLY_BASE) {
> > -		printf("WARNING: early print support may not work. "
> > -		       "Found uart at %p, but early base is %p.\n",
> > -			uart0_base, (u8 *)UART_EARLY_BASE);
> > -	}
> >  }

This warning is doing what it should, which is pointing out that the
UART_EARLY_BASE guess appears to be wrong. If we can provide a way
to support more than one guess, then we should keep this warning but
adjust it to match one of any of the guesses.

Thanks,
drew

Re: [kvm-unit-tests PATCH 3/7] lib: chr-testdev: Make chr_testdev_init() return status

[Andrew Jones]

On Thu, Jan 24, 2019 at 11:16:30AM +0000, Alexandru Elisei wrote:
> Make chr_testdev_init() return 0 (success) if the virtio console was
> initialized properly, otherwise return -1 (failure).
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  lib/chr-testdev.h | 2 +-
>  lib/chr-testdev.c | 8 +++++---
>  2 files changed, 6 insertions(+), 4 deletions(-)

This patch isn't necessary. I'll explain why in patch 5/7.

> 
> diff --git a/lib/chr-testdev.h b/lib/chr-testdev.h
> index ffd9a851aa9b..fdd0582e2da1 100644
> --- a/lib/chr-testdev.h
> +++ b/lib/chr-testdev.h
> @@ -9,6 +9,6 @@
>   *
>   * This work is licensed under the terms of the GNU LGPL, version 2.
>   */
> -extern void chr_testdev_init(void);
> +extern int chr_testdev_init(void);
>  extern void chr_testdev_exit(int code);
>  #endif
> diff --git a/lib/chr-testdev.c b/lib/chr-testdev.c
> index 6890f63c8b29..26e14301e3db 100644
> --- a/lib/chr-testdev.c
> +++ b/lib/chr-testdev.c
> @@ -47,7 +47,7 @@ out:
>  	spin_unlock(&lock);
>  }
>  
> -void chr_testdev_init(void)
> +int chr_testdev_init(void)
>  {
>  	const char *io_names[] = { "input", "output" };
>  	struct virtqueue *vqs[2];
> @@ -57,7 +57,7 @@ void chr_testdev_init(void)
>  	if (vcon == NULL) {
>  		printf("%s: %s: can't find a virtio-console\n",
>  				__func__, TESTDEV_NAME);
> -		return;
> +		return -1;
>  	}
>  
>  	ret = vcon->config->find_vqs(vcon, 2, vqs, NULL, io_names);
> @@ -65,9 +65,11 @@ void chr_testdev_init(void)
>  		printf("%s: %s: can't init virtqueues\n",
>  				__func__, TESTDEV_NAME);
>  		vcon = NULL;
> -		return;
> +		return -1;
>  	}
>  
>  	in_vq = vqs[0];
>  	out_vq = vqs[1];
> +
> +	return 0;
>  }
> -- 
> 2.17.0
> 

Re: [kvm-unit-tests PATCH 5/7] lib: arm: Fallback to psci_system_off() in exit()

[Andrew Jones]

On Thu, Jan 24, 2019 at 11:16:32AM +0000, Alexandru Elisei wrote:
> On arm and arm64, kvm-unit-tests uses the QEMU chr-testdev device to shut
> down the virtual machine at the end of a test. The function
> psci_system_off() provides another mechanism for terminating the virtual
> machine. If the chr-testdev device hasn't been initialized successfully,
> then use psci_system_off() to terminate the test instead of
> chr_testdev_exit().
> 
> chr-testdev is implemented on top of virtio console. This patch makes it
> possible for a virtual machine manager which doesn't have support for
> chr-testdev, but has been configured not to emulate a virtio console, to
> gracefully terminate a virtual machine after a test has been completed.
> 
> There is one limitation to using psci_system_off() to terminate a test:
> chr-testdev allows kvm-unit-tests to specify an exit code;
> psci_system_off() has no such mechanism.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  lib/arm/io.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/arm/io.c b/lib/arm/io.c
> index 87435150f73e..9fe9bd0bf659 100644
> --- a/lib/arm/io.c
> +++ b/lib/arm/io.c
> @@ -11,6 +11,7 @@
>  #include <libcflat.h>
>  #include <devicetree.h>
>  #include <chr-testdev.h>
> +#include "arm/asm/psci.h"
>  #include <asm/spinlock.h>
>  #include <asm/io.h>
>  
> @@ -18,6 +19,8 @@
>  
>  extern void halt(int code);
>  
> +static bool testdev_enabled;
> +
>  /*
>   * Use this guess for the pl011 base in order to make an attempt at
>   * having earlier printf support. We'll overwrite it with the real
> @@ -65,8 +68,12 @@ static void uart0_init(void)
>  
>  void io_init(void)
>  {
> +	int err;
> +
>  	uart0_init();
> -	chr_testdev_init();
> +	err = chr_testdev_init();
> +	if (!err)
> +		testdev_enabled = true;
>  }
>  
>  void puts(const char *s)
> @@ -79,7 +86,10 @@ void puts(const char *s)
>  
>  void exit(int code)
>  {
> -	chr_testdev_exit(code);
> +	if (testdev_enabled)
> +		chr_testdev_exit(code);
> +	else
> +		psci_system_off();
>  	halt(code);
>  	__builtin_unreachable();
>  }
> -- 
> 2.17.0
>

chr_testdev_init() ensures vcon is NULL if it fails to initialize.
chr_testdev_exit() immediately returns if vcon is NULL. This was
done by design to allow fallback exits to be placed below the
chr_testdev_exit call, e.g. halt().

We should be able to drop patch 3/7 and change exit() to this

  void exit(int code)
  {
      chr_testdev_exit(code);
      psci_system_off();
      halt(code);
      __builtin_unreachable();
  }

Thanks,
drew

Re: [kvm-unit-tests PATCH 4/7] lib: arm: Implement PSCI SYSTEM_OFF in psci_system_off()

[Andrew Jones]

On Thu, Jan 24, 2019 at 11:16:31AM +0000, Alexandru Elisei wrote:
> A new function psci_system_off() is added which implements the PSCI
> SYSTEM_OFF function. A call causes the hypervisor to terminate the virtual
> machine.
> 
> Consumers for the function will be added in a later patch.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  lib/arm/asm/psci.h | 1 +
>  lib/arm/psci.c     | 6 ++++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/lib/arm/asm/psci.h b/lib/arm/asm/psci.h
> index ed51708fd265..93409996349a 100644
> --- a/lib/arm/asm/psci.h
> +++ b/lib/arm/asm/psci.h
> @@ -9,5 +9,6 @@ extern int psci_cpu_on(unsigned long cpuid, unsigned long entry_point);
>  extern void psci_sys_reset(void);
>  extern int cpu_psci_cpu_boot(unsigned int cpu);
>  extern void cpu_psci_cpu_die(void);
> +extern void psci_system_off(void);
>  
>  #endif /* _ASMARM_PSCI_H_ */
> diff --git a/lib/arm/psci.c b/lib/arm/psci.c
> index 119f74e57e91..d8dc428e8e8b 100644
> --- a/lib/arm/psci.c
> +++ b/lib/arm/psci.c
> @@ -48,6 +48,12 @@ void cpu_psci_cpu_die(void)
>  	printf("CPU%d unable to power off (error = %d)\n", smp_processor_id(), err);
>  }
>  
> +void psci_system_off(void)
> +{
> +	int err = psci_invoke(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
> +	printf("CPU%d unable to do system off (error = %d)\n", smp_processor_id(), err);
> +}
> +
>  void psci_sys_reset(void)
>  {
>  	psci_invoke(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
> -- 
> 2.17.0
>

We should either rename psci_sys_reset to psci_system_reset or name this
new function with the psci_sys_* pattern.

Thanks,
drew

Re: [kvm-unit-tests PATCH 7/7] arm/arm64: Use argv_find() for test names

[Andrew Jones]

On Thu, Jan 24, 2019 at 11:16:34AM +0000, Alexandru Elisei wrote:
> Instead of aborting the test when an unexpected parameter is found, use
> argv_find() to search for the desired parameter. On arm and arm64, this
> allows kvm-unit-tests to be used with virtual machine monitors like kvmtool
> which automatically adds parameters to the kernel command line.
> 
> For example, to run the gicv3-ipi test the following command was used:
> 
> lkvm run -k gic.flat -c 8 -m 410 -p "ipi" --irqchip=gicv3
> 
> The resulting kernel command line that was passed to kvm-unit-tests was:
> 
> "console=ttyS0 rw rootflags=trans=virtio,version=9p2000.L,cache=loose rootfstype=9p init=/virt/init  ip=dhcp ipi"

Why not write a patch for kvmtool that provides a new command line
parameter which says to not add any kernel parameters other than what
is provided with '-p'? Giving the user full control over the kernel
command line seems like a feature kvmtool would want anyway.

Thanks,
drew

> 
> Without the patch, the following error occurs:
> "ABORT: gicv3: Unknown subtest 'console=ttyS0'"
> 
> With the patch, the test is able to run.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  arm/gic.c      |  7 ++++---
>  arm/selftest.c | 19 ++++++++++++-------
>  2 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index ed5642e74f70..f3746de4bfd8 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -12,6 +12,7 @@
>   * This work is licensed under the terms of the GNU LGPL, version 2.
>   */
>  #include <libcflat.h>
> +#include <argv.h>
>  #include <asm/setup.h>
>  #include <asm/processor.h>
>  #include <asm/delay.h>
> @@ -533,13 +534,13 @@ int main(int argc, char **argv)
>  	if (argc < 2)
>  		report_abort("no test specified");
>  
> -	if (strcmp(argv[1], "ipi") == 0) {
> +	if (argv_find("ipi", argc, argv) != -1) {
>  		report_prefix_push(argv[1]);
>  		nr_cpu_check(2);
>  		on_cpus(ipi_test, NULL);
> -	} else if (strcmp(argv[1], "active") == 0) {
> +	} else if (argv_find("active", argc, argv) != -1) {
>  		run_active_clear_test();
> -	} else if (strcmp(argv[1], "mmio") == 0) {
> +	} else if (argv_find("mmio", argc, argv) != -1) {
>  		report_prefix_push(argv[1]);
>  		gic_test_mmio();
>  		report_prefix_pop();
> diff --git a/arm/selftest.c b/arm/selftest.c
> index ea5101ef7217..44b2f23ca115 100644
> --- a/arm/selftest.c
> +++ b/arm/selftest.c
> @@ -8,6 +8,7 @@
>  #include <libcflat.h>
>  #include <util.h>
>  #include <devicetree.h>
> +#include <argv.h>
>  #include <asm/setup.h>
>  #include <asm/ptrace.h>
>  #include <asm/asm-offsets.h>
> @@ -319,28 +320,32 @@ static void cpu_report(void *data __unused)
>  
>  int main(int argc, char **argv)
>  {
> +	int pos;
> +
>  	report_prefix_push("selftest");
>  
>  	if (argc < 2)
>  		report_abort("no test specified");
>  
> -	report_prefix_push(argv[1]);
> -
> -	if (strcmp(argv[1], "setup") == 0) {
> +	if ((pos = argv_find("setup", argc, argv)) != -1) {
>  
> -		check_setup(argc-2, &argv[2]);
> +		report_prefix_push("setup");
> +		check_setup(argc-(pos+1), &argv[pos+1]);
>  
> -	} else if (strcmp(argv[1], "vectors-kernel") == 0) {
> +	} else if (argv_find("vectors-kernel", argc, argv) != -1) {
>  
> +		report_prefix_push("vectors-kernel");
>  		check_vectors(NULL);
>  
> -	} else if (strcmp(argv[1], "vectors-user") == 0) {
> +	} else if (argv_find("vectors-user", argc, argv) != -1) {
>  
> +		report_prefix_push("vectors-user");
>  		start_usr(check_vectors, NULL,
>  				(unsigned long)thread_stack_alloc());
>  
> -	} else if (strcmp(argv[1], "smp") == 0) {
> +	} else if (argv_find("smp", argc, argv) != -1) {
>  
> +		report_prefix_push("smp");
>  		report("PSCI version", psci_check());
>  		on_cpus(cpu_report, NULL);
>  
> -- 
> 2.17.0
> 

Re: [kvm-unit-tests PATCH 1/7] lib: arm: Discover ns16550a UART

[Andrew Jones]

On Thu, Jan 24, 2019 at 11:16:28AM +0000, Alexandru Elisei wrote:
> Add support for discovering the ns16550a UART from the device tree. This
> particular UART model is emulated by kvmtool.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  lib/arm/io.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/arm/io.c b/lib/arm/io.c
> index d2c1a07c19ee..35fc05aeb4db 100644
> --- a/lib/arm/io.c
> +++ b/lib/arm/io.c
> @@ -32,22 +32,26 @@ static volatile u8 *uart0_base = (u8 *)UART_EARLY_BASE;
>  
>  static void uart0_init(void)
>  {
> -	const char *compatible = "arm,pl011";
> +	const char *compatible[] = {"arm,pl011", "ns16550a"};
>  	struct dt_pbus_reg base;
> -	int ret;
> +	int i, ret;
>  
>  	ret = dt_get_default_console_node();
>  	assert(ret >= 0 || ret == -FDT_ERR_NOTFOUND);
>  
>  	if (ret == -FDT_ERR_NOTFOUND) {
>  
> -		ret = dt_pbus_get_base_compatible(compatible, &base);
> -		assert(ret == 0 || ret == -FDT_ERR_NOTFOUND);
> +		for (i = 0; i < ARRAY_SIZE(compatible); i++) {
> +			ret = dt_pbus_get_base_compatible(compatible[i], &base);
> +			assert(ret == 0 || ret == -FDT_ERR_NOTFOUND);
> +
> +			if (ret == 0)
> +				break;
> +		}
>  
>  		if (ret) {
> -			printf("%s: %s not found in the device tree, "
> -				"aborting...\n",
> -				__func__, compatible);
> +			printf("%s: Compatible UART not found in the device tree, "
> +				"aborting...\n", __func__);
>  			abort();
>  		}
>  
> -- 
> 2.17.0
>

I agree with Andre that we should document this UART "driver" only ever
writes to the TX register. And that it assumes that TX register is at
the 0th offset.

Thanks,
drew

Re: [kvm-unit-tests PATCH 5/7] lib: arm: Fallback to psci_system_off() in exit()

[Andrew Jones]

On Thu, Jan 24, 2019 at 02:00:20PM +0100, Andrew Jones wrote:
> On Thu, Jan 24, 2019 at 11:16:32AM +0000, Alexandru Elisei wrote:
> > On arm and arm64, kvm-unit-tests uses the QEMU chr-testdev device to shut
> > down the virtual machine at the end of a test. The function
> > psci_system_off() provides another mechanism for terminating the virtual
> > machine. If the chr-testdev device hasn't been initialized successfully,
> > then use psci_system_off() to terminate the test instead of
> > chr_testdev_exit().
> > 
> > chr-testdev is implemented on top of virtio console. This patch makes it
> > possible for a virtual machine manager which doesn't have support for
> > chr-testdev, but has been configured not to emulate a virtio console, to
> > gracefully terminate a virtual machine after a test has been completed.
> > 
> > There is one limitation to using psci_system_off() to terminate a test:
> > chr-testdev allows kvm-unit-tests to specify an exit code;
> > psci_system_off() has no such mechanism.
> > 
> > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > ---
> >  lib/arm/io.c | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/arm/io.c b/lib/arm/io.c
> > index 87435150f73e..9fe9bd0bf659 100644
> > --- a/lib/arm/io.c
> > +++ b/lib/arm/io.c
> > @@ -11,6 +11,7 @@
> >  #include <libcflat.h>
> >  #include <devicetree.h>
> >  #include <chr-testdev.h>
> > +#include "arm/asm/psci.h"
> >  #include <asm/spinlock.h>
> >  #include <asm/io.h>
> >  
> > @@ -18,6 +19,8 @@
> >  
> >  extern void halt(int code);
> >  
> > +static bool testdev_enabled;
> > +
> >  /*
> >   * Use this guess for the pl011 base in order to make an attempt at
> >   * having earlier printf support. We'll overwrite it with the real
> > @@ -65,8 +68,12 @@ static void uart0_init(void)
> >  
> >  void io_init(void)
> >  {
> > +	int err;
> > +
> >  	uart0_init();
> > -	chr_testdev_init();
> > +	err = chr_testdev_init();
> > +	if (!err)
> > +		testdev_enabled = true;
> >  }
> >  
> >  void puts(const char *s)
> > @@ -79,7 +86,10 @@ void puts(const char *s)
> >  
> >  void exit(int code)
> >  {
> > -	chr_testdev_exit(code);
> > +	if (testdev_enabled)
> > +		chr_testdev_exit(code);
> > +	else
> > +		psci_system_off();
> >  	halt(code);
> >  	__builtin_unreachable();
> >  }
> > -- 
> > 2.17.0
> >
> 
> chr_testdev_init() ensures vcon is NULL if it fails to initialize.
> chr_testdev_exit() immediately returns if vcon is NULL. This was
> done by design to allow fallback exits to be placed below the
> chr_testdev_exit call, e.g. halt().
> 
> We should be able to drop patch 3/7 and change exit() to this
> 
>   void exit(int code)
>   {
>       chr_testdev_exit(code);
>       psci_system_off();
>       halt(code);
>       __builtin_unreachable();
>   }
>

There's also a framework for exits that can't return status codes. powerpc
uses it. Before exiting with psci_system_off we need to make this print
statement

 printf("\nEXIT: STATUS=%d\n", ((code) << 1) | 1); 

And run_qemu in arm/run needs to be changed to run_qemu_status. It's
hacky, but maybe we can live with it until kvmtool offers some sort of
debug exit.

Thanks,
drew

Re: [kvm-unit-tests PATCH 7/7] arm/arm64: Use argv_find() for test names

[Andre Przywara]

On Thu, 24 Jan 2019 14:07:32 +0100
Andrew Jones <drjones@redhat.com> wrote:

> On Thu, Jan 24, 2019 at 11:16:34AM +0000, Alexandru Elisei wrote:
> > Instead of aborting the test when an unexpected parameter is found,
> > use argv_find() to search for the desired parameter. On arm and
> > arm64, this allows kvm-unit-tests to be used with virtual machine
> > monitors like kvmtool which automatically adds parameters to the
> > kernel command line.
> > 
> > For example, to run the gicv3-ipi test the following command was
> > used:
> > 
> > lkvm run -k gic.flat -c 8 -m 410 -p "ipi" --irqchip=gicv3
> > 
> > The resulting kernel command line that was passed to kvm-unit-tests
> > was:
> > 
> > "console=ttyS0 rw
> > rootflags=trans=virtio,version=9p2000.L,cache=loose rootfstype=9p
> > init=/virt/init  ip=dhcp ipi"  
> 
> Why not write a patch for kvmtool that provides a new command line
> parameter which says to not add any kernel parameters other than what
> is provided with '-p'? Giving the user full control over the kernel
> command line seems like a feature kvmtool would want anyway.

Thanks, that was the reply I was hoping for ;-)
Gives me some leverage to justify the corresponding kvmtool patch.

Actually we are about to gain the --firmware switch for ARM in kvmtool,
and I am looking at piggy-backing on that.

Cheers,
Andre.

> > 
> > Without the patch, the following error occurs:
> > "ABORT: gicv3: Unknown subtest 'console=ttyS0'"
> > 
> > With the patch, the test is able to run.
> > 
> > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > ---
> >  arm/gic.c      |  7 ++++---
> >  arm/selftest.c | 19 ++++++++++++-------
> >  2 files changed, 16 insertions(+), 10 deletions(-)
> > 
> > diff --git a/arm/gic.c b/arm/gic.c
> > index ed5642e74f70..f3746de4bfd8 100644
> > --- a/arm/gic.c
> > +++ b/arm/gic.c
> > @@ -12,6 +12,7 @@
> >   * This work is licensed under the terms of the GNU LGPL, version
> > 2. */
> >  #include <libcflat.h>
> > +#include <argv.h>
> >  #include <asm/setup.h>
> >  #include <asm/processor.h>
> >  #include <asm/delay.h>
> > @@ -533,13 +534,13 @@ int main(int argc, char **argv)
> >  	if (argc < 2)
> >  		report_abort("no test specified");
> >  
> > -	if (strcmp(argv[1], "ipi") == 0) {
> > +	if (argv_find("ipi", argc, argv) != -1) {
> >  		report_prefix_push(argv[1]);
> >  		nr_cpu_check(2);
> >  		on_cpus(ipi_test, NULL);
> > -	} else if (strcmp(argv[1], "active") == 0) {
> > +	} else if (argv_find("active", argc, argv) != -1) {
> >  		run_active_clear_test();
> > -	} else if (strcmp(argv[1], "mmio") == 0) {
> > +	} else if (argv_find("mmio", argc, argv) != -1) {
> >  		report_prefix_push(argv[1]);
> >  		gic_test_mmio();
> >  		report_prefix_pop();
> > diff --git a/arm/selftest.c b/arm/selftest.c
> > index ea5101ef7217..44b2f23ca115 100644
> > --- a/arm/selftest.c
> > +++ b/arm/selftest.c
> > @@ -8,6 +8,7 @@
> >  #include <libcflat.h>
> >  #include <util.h>
> >  #include <devicetree.h>
> > +#include <argv.h>
> >  #include <asm/setup.h>
> >  #include <asm/ptrace.h>
> >  #include <asm/asm-offsets.h>
> > @@ -319,28 +320,32 @@ static void cpu_report(void *data __unused)
> >  
> >  int main(int argc, char **argv)
> >  {
> > +	int pos;
> > +
> >  	report_prefix_push("selftest");
> >  
> >  	if (argc < 2)
> >  		report_abort("no test specified");
> >  
> > -	report_prefix_push(argv[1]);
> > -
> > -	if (strcmp(argv[1], "setup") == 0) {
> > +	if ((pos = argv_find("setup", argc, argv)) != -1) {
> >  
> > -		check_setup(argc-2, &argv[2]);
> > +		report_prefix_push("setup");
> > +		check_setup(argc-(pos+1), &argv[pos+1]);
> >  
> > -	} else if (strcmp(argv[1], "vectors-kernel") == 0) {
> > +	} else if (argv_find("vectors-kernel", argc, argv) != -1) {
> >  
> > +		report_prefix_push("vectors-kernel");
> >  		check_vectors(NULL);
> >  
> > -	} else if (strcmp(argv[1], "vectors-user") == 0) {
> > +	} else if (argv_find("vectors-user", argc, argv) != -1) {
> >  
> > +		report_prefix_push("vectors-user");
> >  		start_usr(check_vectors, NULL,
> >  				(unsigned
> > long)thread_stack_alloc()); 
> > -	} else if (strcmp(argv[1], "smp") == 0) {
> > +	} else if (argv_find("smp", argc, argv) != -1) {
> >  
> > +		report_prefix_push("smp");
> >  		report("PSCI version", psci_check());
> >  		on_cpus(cpu_report, NULL);
> >  
> > -- 
> > 2.17.0
> >   

Re: [kvm-unit-tests PATCH 1/7] lib: arm: Discover ns16550a UART

[Alexandru Elisei]

On 1/24/19 1:11 PM, Andrew Jones wrote:
> On Thu, Jan 24, 2019 at 11:16:28AM +0000, Alexandru Elisei wrote:
>> Add support for discovering the ns16550a UART from the device tree. This
>> particular UART model is emulated by kvmtool.
>>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>>   lib/arm/io.c | 18 +++++++++++-------
>>   1 file changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/arm/io.c b/lib/arm/io.c
>> index d2c1a07c19ee..35fc05aeb4db 100644
>> --- a/lib/arm/io.c
>> +++ b/lib/arm/io.c
>> @@ -32,22 +32,26 @@ static volatile u8 *uart0_base = (u8 *)UART_EARLY_BASE;
>>   
>>   static void uart0_init(void)
>>   {
>> -	const char *compatible = "arm,pl011";
>> +	const char *compatible[] = {"arm,pl011", "ns16550a"};
>>   	struct dt_pbus_reg base;
>> -	int ret;
>> +	int i, ret;
>>   
>>   	ret = dt_get_default_console_node();
>>   	assert(ret >= 0 || ret == -FDT_ERR_NOTFOUND);
>>   
>>   	if (ret == -FDT_ERR_NOTFOUND) {
>>   
>> -		ret = dt_pbus_get_base_compatible(compatible, &base);
>> -		assert(ret == 0 || ret == -FDT_ERR_NOTFOUND);
>> +		for (i = 0; i < ARRAY_SIZE(compatible); i++) {
>> +			ret = dt_pbus_get_base_compatible(compatible[i], &base);
>> +			assert(ret == 0 || ret == -FDT_ERR_NOTFOUND);
>> +
>> +			if (ret == 0)
>> +				break;
>> +		}
>>   
>>   		if (ret) {
>> -			printf("%s: %s not found in the device tree, "
>> -				"aborting...\n",
>> -				__func__, compatible);
>> +			printf("%s: Compatible UART not found in the device tree, "
>> +				"aborting...\n", __func__);
>>   			abort();
>>   		}
>>   
>> -- 
>> 2.17.0
>>
> I agree with Andre that we should document this UART "driver" only ever
> writes to the TX register. And that it assumes that TX register is at
> the 0th offset.
>
> Thanks,
> drew
Sure, I'll write a comment documenting this behavior in the next version of the 
patches.

Re: [kvm-unit-tests PATCH 4/7] lib: arm: Implement PSCI SYSTEM_OFF in psci_system_off()

[Alexandru Elisei]

On 1/24/19 1:01 PM, Andrew Jones wrote:
> On Thu, Jan 24, 2019 at 11:16:31AM +0000, Alexandru Elisei wrote:
>> A new function psci_system_off() is added which implements the PSCI
>> SYSTEM_OFF function. A call causes the hypervisor to terminate the virtual
>> machine.
>>
>> Consumers for the function will be added in a later patch.
>>
>> Signed-off-by: Alexandru Elisei<alexandru.elisei@arm.com>
>> ---
>>   lib/arm/asm/psci.h | 1 +
>>   lib/arm/psci.c     | 6 ++++++
>>   2 files changed, 7 insertions(+)
>>
>> diff --git a/lib/arm/asm/psci.h b/lib/arm/asm/psci.h
>> index ed51708fd265..93409996349a 100644
>> --- a/lib/arm/asm/psci.h
>> +++ b/lib/arm/asm/psci.h
>> @@ -9,5 +9,6 @@ extern int psci_cpu_on(unsigned long cpuid, unsigned long entry_point);
>>   extern void psci_sys_reset(void);
>>   extern int cpu_psci_cpu_boot(unsigned int cpu);
>>   extern void cpu_psci_cpu_die(void);
>> +extern void psci_system_off(void);
>>   
>>   #endif /* _ASMARM_PSCI_H_ */
>> diff --git a/lib/arm/psci.c b/lib/arm/psci.c
>> index 119f74e57e91..d8dc428e8e8b 100644
>> --- a/lib/arm/psci.c
>> +++ b/lib/arm/psci.c
>> @@ -48,6 +48,12 @@ void cpu_psci_cpu_die(void)
>>   	printf("CPU%d unable to power off (error = %d)\n", smp_processor_id(), err);
>>   }
>>   
>> +void psci_system_off(void)
>> +{
>> +	int err = psci_invoke(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
>> +	printf("CPU%d unable to do system off (error = %d)\n", smp_processor_id(), err);
>> +}
>> +
>>   void psci_sys_reset(void)
>>   {
>>   	psci_invoke(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
>> -- 
>> 2.17.0
>>
> We should either rename psci_sys_reset to psci_system_reset or name this
> new function with the psci_sys_* pattern.
>
> Thanks,
> drew
I am thinking we should rename psci_sys_reset() to match the PSCI specification 
function name, which is SYSTEM_OFF. If you're ok with it, I'll do the rename in 
the next patch set version.

Re: [kvm-unit-tests PATCH 5/7] lib: arm: Fallback to psci_system_off() in exit()

[Alexandru Elisei]

On 1/24/19 1:00 PM, Andrew Jones wrote:
> On Thu, Jan 24, 2019 at 11:16:32AM +0000, Alexandru Elisei wrote:
>> On arm and arm64, kvm-unit-tests uses the QEMU chr-testdev device to shut
>> down the virtual machine at the end of a test. The function
>> psci_system_off() provides another mechanism for terminating the virtual
>> machine. If the chr-testdev device hasn't been initialized successfully,
>> then use psci_system_off() to terminate the test instead of
>> chr_testdev_exit().
>>
>> chr-testdev is implemented on top of virtio console. This patch makes it
>> possible for a virtual machine manager which doesn't have support for
>> chr-testdev, but has been configured not to emulate a virtio console, to
>> gracefully terminate a virtual machine after a test has been completed.
>>
>> There is one limitation to using psci_system_off() to terminate a test:
>> chr-testdev allows kvm-unit-tests to specify an exit code;
>> psci_system_off() has no such mechanism.
>>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>>  lib/arm/io.c | 14 ++++++++++++--
>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/arm/io.c b/lib/arm/io.c
>> index 87435150f73e..9fe9bd0bf659 100644
>> --- a/lib/arm/io.c
>> +++ b/lib/arm/io.c
>> @@ -11,6 +11,7 @@
>>  #include <libcflat.h>
>>  #include <devicetree.h>
>>  #include <chr-testdev.h>
>> +#include "arm/asm/psci.h"
>>  #include <asm/spinlock.h>
>>  #include <asm/io.h>
>>  
>> @@ -18,6 +19,8 @@
>>  
>>  extern void halt(int code);
>>  
>> +static bool testdev_enabled;
>> +
>>  /*
>>   * Use this guess for the pl011 base in order to make an attempt at
>>   * having earlier printf support. We'll overwrite it with the real
>> @@ -65,8 +68,12 @@ static void uart0_init(void)
>>  
>>  void io_init(void)
>>  {
>> +	int err;
>> +
>>  	uart0_init();
>> -	chr_testdev_init();
>> +	err = chr_testdev_init();
>> +	if (!err)
>> +		testdev_enabled = true;
>>  }
>>  
>>  void puts(const char *s)
>> @@ -79,7 +86,10 @@ void puts(const char *s)
>>  
>>  void exit(int code)
>>  {
>> -	chr_testdev_exit(code);
>> +	if (testdev_enabled)
>> +		chr_testdev_exit(code);
>> +	else
>> +		psci_system_off();
>>  	halt(code);
>>  	__builtin_unreachable();
>>  }
>> -- 
>> 2.17.0
>>
> chr_testdev_init() ensures vcon is NULL if it fails to initialize.
> chr_testdev_exit() immediately returns if vcon is NULL. This was
> done by design to allow fallback exits to be placed below the
> chr_testdev_exit call, e.g. halt().
>
> We should be able to drop patch 3/7 and change exit() to this
>
>   void exit(int code)
>   {
>       chr_testdev_exit(code);
>       psci_system_off();
>       halt(code);
>       __builtin_unreachable();
>   }
>
> Thanks,
> drew
I wasn't aware of this design decision, thank you for explaining it. I'll
implement the changes you suggested, including dropping patch 3/7: "lib:
chr-testdev: Make chr_testdev_init() return status".

Re: [kvm-unit-tests PATCH 4/7] lib: arm: Implement PSCI SYSTEM_OFF in psci_system_off()

[Andrew Jones]

On Fri, Jan 25, 2019 at 02:08:45PM +0000, Alexandru Elisei wrote:
> 
> On 1/24/19 1:01 PM, Andrew Jones wrote:
> > On Thu, Jan 24, 2019 at 11:16:31AM +0000, Alexandru Elisei wrote:
> > > A new function psci_system_off() is added which implements the PSCI
> > > SYSTEM_OFF function. A call causes the hypervisor to terminate the virtual
> > > machine.
> > > 
> > > Consumers for the function will be added in a later patch.
> > > 
> > > Signed-off-by: Alexandru Elisei<alexandru.elisei@arm.com>
> > > ---
> > >   lib/arm/asm/psci.h | 1 +
> > >   lib/arm/psci.c     | 6 ++++++
> > >   2 files changed, 7 insertions(+)
> > > 
> > > diff --git a/lib/arm/asm/psci.h b/lib/arm/asm/psci.h
> > > index ed51708fd265..93409996349a 100644
> > > --- a/lib/arm/asm/psci.h
> > > +++ b/lib/arm/asm/psci.h
> > > @@ -9,5 +9,6 @@ extern int psci_cpu_on(unsigned long cpuid, unsigned long entry_point);
> > >   extern void psci_sys_reset(void);
> > >   extern int cpu_psci_cpu_boot(unsigned int cpu);
> > >   extern void cpu_psci_cpu_die(void);
> > > +extern void psci_system_off(void);
> > >   #endif /* _ASMARM_PSCI_H_ */
> > > diff --git a/lib/arm/psci.c b/lib/arm/psci.c
> > > index 119f74e57e91..d8dc428e8e8b 100644
> > > --- a/lib/arm/psci.c
> > > +++ b/lib/arm/psci.c
> > > @@ -48,6 +48,12 @@ void cpu_psci_cpu_die(void)
> > >   	printf("CPU%d unable to power off (error = %d)\n", smp_processor_id(), err);
> > >   }
> > > +void psci_system_off(void)
> > > +{
> > > +	int err = psci_invoke(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
> > > +	printf("CPU%d unable to do system off (error = %d)\n", smp_processor_id(), err);
> > > +}
> > > +
> > >   void psci_sys_reset(void)
> > >   {
> > >   	psci_invoke(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
> > > -- 
> > > 2.17.0
> > > 
> > We should either rename psci_sys_reset to psci_system_reset or name this
> > new function with the psci_sys_* pattern.
> > 
> > Thanks,
> > drew
> I am thinking we should rename psci_sys_reset() to match the PSCI
> specification function name, which is SYSTEM_OFF. If you're ok with it, I'll
> do the rename in the next patch set version.

Yup, I'm fine with the renaming.

Thanks,
drew

Re: [kvm-unit-tests PATCH 5/7] lib: arm: Fallback to psci_system_off() in exit()

[Alexandru Elisei]

On 1/24/19 1:35 PM, Andrew Jones wrote:
> On Thu, Jan 24, 2019 at 02:00:20PM +0100, Andrew Jones wrote:
>> [..]
>> chr_testdev_init() ensures vcon is NULL if it fails to initialize.
>> chr_testdev_exit() immediately returns if vcon is NULL. This was
>> done by design to allow fallback exits to be placed below the
>> chr_testdev_exit call, e.g. halt().
>>
>> We should be able to drop patch 3/7 and change exit() to this
>>
>>   void exit(int code)
>>   {
>>       chr_testdev_exit(code);
>>       psci_system_off();
>>       halt(code);
>>       __builtin_unreachable();
>>   }
>>
> There's also a framework for exits that can't return status codes. powerpc
> uses it. Before exiting with psci_system_off we need to make this print
> statement
>
>  printf("\nEXIT: STATUS=%d\n", ((code) << 1) | 1); 
>
> And run_qemu in arm/run needs to be changed to run_qemu_status. It's
> hacky, but maybe we can live with it until kvmtool offers some sort of
> debug exit.
>
> Thanks,
> drew

I can make the change, but if I understand the scripts/arch-run.bash code
correctly, run_qemu() will check if QEMU was terminated because of a signal and
adjust the return code to take that into account. Using run_qemu_status() means
that check won't be made when the tests are run under QEMU, is that acceptable?

Re: [kvm-unit-tests PATCH 5/7] lib: arm: Fallback to psci_system_off() in exit()

[Andrew Jones]

On Fri, Jan 25, 2019 at 02:56:30PM +0000, Alexandru Elisei wrote:
> 
> On 1/24/19 1:35 PM, Andrew Jones wrote:
> > On Thu, Jan 24, 2019 at 02:00:20PM +0100, Andrew Jones wrote:
> >> [..]
> >> chr_testdev_init() ensures vcon is NULL if it fails to initialize.
> >> chr_testdev_exit() immediately returns if vcon is NULL. This was
> >> done by design to allow fallback exits to be placed below the
> >> chr_testdev_exit call, e.g. halt().
> >>
> >> We should be able to drop patch 3/7 and change exit() to this
> >>
> >>   void exit(int code)
> >>   {
> >>       chr_testdev_exit(code);
> >>       psci_system_off();
> >>       halt(code);
> >>       __builtin_unreachable();
> >>   }
> >>
> > There's also a framework for exits that can't return status codes. powerpc
> > uses it. Before exiting with psci_system_off we need to make this print
> > statement
> >
> >  printf("\nEXIT: STATUS=%d\n", ((code) << 1) | 1); 
> >
> > And run_qemu in arm/run needs to be changed to run_qemu_status. It's
> > hacky, but maybe we can live with it until kvmtool offers some sort of
> > debug exit.
> >
> > Thanks,
> > drew
> 
> I can make the change, but if I understand the scripts/arch-run.bash code
> correctly, run_qemu() will check if QEMU was terminated because of a signal and
> adjust the return code to take that into account. Using run_qemu_status() means
> that check won't be made when the tests are run under QEMU, is that acceptable?
>

run_qemu_status() first calls run_qemu(). If the return value from
run_qemu() is 1, then it'll change the value to whatever the EXIT:
status line says it should be. If run_qemu() detected that a signal
caused the exit, then the return value won't be 1. In that case
run_qemu_status() will simply propagate the value to the caller.

It might be worth doing a few tests to ensure that all works out as
designed, but I'm pretty sure it should.

Thanks,
drew

Re: [kvm-unit-tests PATCH 5/7] lib: arm: Fallback to psci_system_off() in exit()

[Alexandru Elisei]

On 1/25/19 3:31 PM, Andrew Jones wrote:
> On Fri, Jan 25, 2019 at 02:56:30PM +0000, Alexandru Elisei wrote:
>> On 1/24/19 1:35 PM, Andrew Jones wrote:
>>> On Thu, Jan 24, 2019 at 02:00:20PM +0100, Andrew Jones wrote:
>>>> [..]
>>>> chr_testdev_init() ensures vcon is NULL if it fails to initialize.
>>>> chr_testdev_exit() immediately returns if vcon is NULL. This was
>>>> done by design to allow fallback exits to be placed below the
>>>> chr_testdev_exit call, e.g. halt().
>>>>
>>>> We should be able to drop patch 3/7 and change exit() to this
>>>>
>>>>   void exit(int code)
>>>>   {
>>>>       chr_testdev_exit(code);
>>>>       psci_system_off();
>>>>       halt(code);
>>>>       __builtin_unreachable();
>>>>   }
>>>>
>>> There's also a framework for exits that can't return status codes. powerpc
>>> uses it. Before exiting with psci_system_off we need to make this print
>>> statement
>>>
>>>  printf("\nEXIT: STATUS=%d\n", ((code) << 1) | 1); 
>>>
>>> And run_qemu in arm/run needs to be changed to run_qemu_status. It's
>>> hacky, but maybe we can live with it until kvmtool offers some sort of
>>> debug exit.
>>>
>>> Thanks,
>>> drew
>> I can make the change, but if I understand the scripts/arch-run.bash code
>> correctly, run_qemu() will check if QEMU was terminated because of a signal and
>> adjust the return code to take that into account. Using run_qemu_status() means
>> that check won't be made when the tests are run under QEMU, is that acceptable?
>>
> run_qemu_status() first calls run_qemu(). If the return value from
> run_qemu() is 1, then it'll change the value to whatever the EXIT:
> status line says it should be. If run_qemu() detected that a signal
> caused the exit, then the return value won't be 1. In that case
> run_qemu_status() will simply propagate the value to the caller.
>
> It might be worth doing a few tests to ensure that all works out as
> designed, but I'm pretty sure it should.
>
> Thanks,
> drew

Now it makes more sense, thank you. I'll make the change and run some QEMU and
kvmtool tests.

Re: [kvm-unit-tests PATCH 5/7] lib: arm: Fallback to psci_system_off() in exit()

[Andrew Jones]

On Fri, Jan 25, 2019 at 04:31:37PM +0100, Andrew Jones wrote:
> On Fri, Jan 25, 2019 at 02:56:30PM +0000, Alexandru Elisei wrote:
> > 
> > On 1/24/19 1:35 PM, Andrew Jones wrote:
> > > On Thu, Jan 24, 2019 at 02:00:20PM +0100, Andrew Jones wrote:
> > >> [..]
> > >> chr_testdev_init() ensures vcon is NULL if it fails to initialize.
> > >> chr_testdev_exit() immediately returns if vcon is NULL. This was
> > >> done by design to allow fallback exits to be placed below the
> > >> chr_testdev_exit call, e.g. halt().
> > >>
> > >> We should be able to drop patch 3/7 and change exit() to this
> > >>
> > >>   void exit(int code)
> > >>   {
> > >>       chr_testdev_exit(code);
> > >>       psci_system_off();
> > >>       halt(code);
> > >>       __builtin_unreachable();
> > >>   }
> > >>
> > > There's also a framework for exits that can't return status codes. powerpc
> > > uses it. Before exiting with psci_system_off we need to make this print
> > > statement
> > >
> > >  printf("\nEXIT: STATUS=%d\n", ((code) << 1) | 1); 
> > >
> > > And run_qemu in arm/run needs to be changed to run_qemu_status. It's
> > > hacky, but maybe we can live with it until kvmtool offers some sort of
> > > debug exit.
> > >
> > > Thanks,
> > > drew
> > 
> > I can make the change, but if I understand the scripts/arch-run.bash code
> > correctly, run_qemu() will check if QEMU was terminated because of a signal and
> > adjust the return code to take that into account. Using run_qemu_status() means
> > that check won't be made when the tests are run under QEMU, is that acceptable?
> >
> 
> run_qemu_status() first calls run_qemu(). If the return value from
> run_qemu() is 1, then it'll change the value to whatever the EXIT:
> status line says it should be. If run_qemu() detected that a signal
> caused the exit, then the return value won't be 1. In that case
> run_qemu_status() will simply propagate the value to the caller.
> 
> It might be worth doing a few tests to ensure that all works out as
> designed, but I'm pretty sure it should.
>

It just occurred to me that you must not be using the run scripts anyway,
since they would require further patches to work. In that case, there's no
need to change arm/run unless you also provide those additional patches.

BTW, I wouldn't be opposed to a second run script, rather than trying to
make one script work for both qemu and kvmtool. Ideally both scripts would
be driven by the same higher level scripts using the same unittests.cfg
file though. The unittests.cfg extra_params field will make that a bit
challenging, but otherwise I think adding a few new helper functions to
scripts/arch-run.bash may be all that's necessary.

drew

Re: [kvm-unit-tests PATCH 5/7] lib: arm: Fallback to psci_system_off() in exit()

[Andre Przywara]

On Fri, 25 Jan 2019 17:05:45 +0100
Andrew Jones <drjones@redhat.com> wrote:

Hi,

> On Fri, Jan 25, 2019 at 04:31:37PM +0100, Andrew Jones wrote:
> > On Fri, Jan 25, 2019 at 02:56:30PM +0000, Alexandru Elisei wrote:  
> > > 
> > > On 1/24/19 1:35 PM, Andrew Jones wrote:  
> > > > On Thu, Jan 24, 2019 at 02:00:20PM +0100, Andrew Jones wrote:  
> > > >> [..]
> > > >> chr_testdev_init() ensures vcon is NULL if it fails to
> > > >> initialize. chr_testdev_exit() immediately returns if vcon is
> > > >> NULL. This was done by design to allow fallback exits to be
> > > >> placed below the chr_testdev_exit call, e.g. halt().
> > > >>
> > > >> We should be able to drop patch 3/7 and change exit() to this
> > > >>
> > > >>   void exit(int code)
> > > >>   {
> > > >>       chr_testdev_exit(code);
> > > >>       psci_system_off();
> > > >>       halt(code);
> > > >>       __builtin_unreachable();
> > > >>   }
> > > >>  
> > > > There's also a framework for exits that can't return status
> > > > codes. powerpc uses it. Before exiting with psci_system_off we
> > > > need to make this print statement
> > > >
> > > >  printf("\nEXIT: STATUS=%d\n", ((code) << 1) | 1); 
> > > >
> > > > And run_qemu in arm/run needs to be changed to run_qemu_status.
> > > > It's hacky, but maybe we can live with it until kvmtool offers
> > > > some sort of debug exit.
> > > >
> > > > Thanks,
> > > > drew  
> > > 
> > > I can make the change, but if I understand the
> > > scripts/arch-run.bash code correctly, run_qemu() will check if
> > > QEMU was terminated because of a signal and adjust the return
> > > code to take that into account. Using run_qemu_status() means
> > > that check won't be made when the tests are run under QEMU, is
> > > that acceptable? 
> > 
> > run_qemu_status() first calls run_qemu(). If the return value from
> > run_qemu() is 1, then it'll change the value to whatever the EXIT:
> > status line says it should be. If run_qemu() detected that a signal
> > caused the exit, then the return value won't be 1. In that case
> > run_qemu_status() will simply propagate the value to the caller.
> > 
> > It might be worth doing a few tests to ensure that all works out as
> > designed, but I'm pretty sure it should.
> >  
> 
> It just occurred to me that you must not be using the run scripts
> anyway, since they would require further patches to work. In that
> case, there's no need to change arm/run unless you also provide those
> additional patches.
> 
> BTW, I wouldn't be opposed to a second run script, rather than trying
> to make one script work for both qemu and kvmtool. Ideally both
> scripts would be driven by the same higher level scripts using the
> same unittests.cfg file though. The unittests.cfg extra_params field
> will make that a bit challenging, but otherwise I think adding a few
> new helper functions to scripts/arch-run.bash may be all that's
> necessary.

Yeah, I had some patches along those lines: split test parameters
from QEMU parameters, abstract common stuff like number of cores and
amount of memory, mark tests as QEMU only and so on. And I had a
separate run script for kvmtool, IIRC.

If there is interest I can try to post them, but I would consider
this an additional effort on top of this series.

Cheers,
Andre.

Re: [kvm-unit-tests PATCH 2/7] lib: arm: Remove warning about uart0_base mismatch

[Alexandru Elisei]

On 1/24/19 12:37 PM, Andrew Jones wrote:
> On Thu, Jan 24, 2019 at 11:59:43AM +0000, Andre Przywara wrote:
>> On Thu, 24 Jan 2019 11:16:29 +0000
>> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>>
>>> A warning is displayed if uart0_base is different from what the code
>>> expects qemu to generate for the pl011 UART in the device tree.
>>> However, now we support the ns16550a UART emulated by kvmtool, which
>>> has a different address. This leads to the  warning being displayed
>>> even though the UART is configured and working as expected.
>>>
>>> Now that we support multiple UARTs, the warning serves no purpose, so
>>> remove it.
>> Mmh, but we use that address before, right? So for anything not
>> emulating an UART at this QEMU specific address we write to some random
>> (device) memory?
>>
>> Drew, how important is this early print feature for kvm-unit-tests?
> The setup code passes through quite a few asserts before getting through
> io_init() (including in uart0_init), so I think there's still value in
> having a guessed UART address. Maybe we can provide guesses for both
> QEMU and kvmtool, and some selection method, that would be used until
> we've properly assigned uart0_base from DT?
>
>> Cheers,
>> Andre.
>>
>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>>> ---
>>>  lib/arm/io.c | 6 ------
>>>  1 file changed, 6 deletions(-)
>>>
>>> diff --git a/lib/arm/io.c b/lib/arm/io.c
>>> index 35fc05aeb4db..87435150f73e 100644
>>> --- a/lib/arm/io.c
>>> +++ b/lib/arm/io.c
>>> @@ -61,12 +61,6 @@ static void uart0_init(void)
>>>  	}
>>>  
>>>  	uart0_base = ioremap(base.addr, base.size);
>>> -
>>> -	if (uart0_base != (u8 *)UART_EARLY_BASE) {
>>> -		printf("WARNING: early print support may not work. "
>>> -		       "Found uart at %p, but early base is %p.\n",
>>> -			uart0_base, (u8 *)UART_EARLY_BASE);
>>> -	}
>>>  }
> This warning is doing what it should, which is pointing out that the
> UART_EARLY_BASE guess appears to be wrong. If we can provide a way
> to support more than one guess, then we should keep this warning but
> adjust it to match one of any of the guesses.
>
> Thanks,
> drew

I'm not really sure how to implement a selection method. I've looked at
splitting io_init() into uart0_init() and chr_testdev_init() and calling
uart0_init() very early in the setup process, but uart0_init() itself uses
printf() and assert().

I've also thought about adding another function, something like
uart0_early_init(), that is called very early in setup() and gets the base
address from the dtb bootargs. But that means calling dt_init() and
dt_get_bootargs(), which can fail.

One other option that could work is to make it a compile-time configuration.

What do you think?

Re: [kvm-unit-tests PATCH 5/7] lib: arm: Fallback to psci_system_off() in exit()

[Alexandru Elisei]

On 1/25/19 4:14 PM, Andre Przywara wrote:
> On Fri, 25 Jan 2019 17:05:45 +0100
> Andrew Jones <drjones@redhat.com> wrote:
>
> Hi,
>
>> On Fri, Jan 25, 2019 at 04:31:37PM +0100, Andrew Jones wrote:
>>> On Fri, Jan 25, 2019 at 02:56:30PM +0000, Alexandru Elisei wrote:  
>>>> On 1/24/19 1:35 PM, Andrew Jones wrote:  
>>>>> On Thu, Jan 24, 2019 at 02:00:20PM +0100, Andrew Jones wrote:  
>>>>>> [..]
>>>>>> chr_testdev_init() ensures vcon is NULL if it fails to
>>>>>> initialize. chr_testdev_exit() immediately returns if vcon is
>>>>>> NULL. This was done by design to allow fallback exits to be
>>>>>> placed below the chr_testdev_exit call, e.g. halt().
>>>>>>
>>>>>> We should be able to drop patch 3/7 and change exit() to this
>>>>>>
>>>>>>   void exit(int code)
>>>>>>   {
>>>>>>       chr_testdev_exit(code);
>>>>>>       psci_system_off();
>>>>>>       halt(code);
>>>>>>       __builtin_unreachable();
>>>>>>   }
>>>>>>  
>>>>> There's also a framework for exits that can't return status
>>>>> codes. powerpc uses it. Before exiting with psci_system_off we
>>>>> need to make this print statement
>>>>>
>>>>>  printf("\nEXIT: STATUS=%d\n", ((code) << 1) | 1); 
>>>>>
>>>>> And run_qemu in arm/run needs to be changed to run_qemu_status.
>>>>> It's hacky, but maybe we can live with it until kvmtool offers
>>>>> some sort of debug exit.
>>>>>
>>>>> Thanks,
>>>>> drew  
>>>> I can make the change, but if I understand the
>>>> scripts/arch-run.bash code correctly, run_qemu() will check if
>>>> QEMU was terminated because of a signal and adjust the return
>>>> code to take that into account. Using run_qemu_status() means
>>>> that check won't be made when the tests are run under QEMU, is
>>>> that acceptable? 
>>> run_qemu_status() first calls run_qemu(). If the return value from
>>> run_qemu() is 1, then it'll change the value to whatever the EXIT:
>>> status line says it should be. If run_qemu() detected that a signal
>>> caused the exit, then the return value won't be 1. In that case
>>> run_qemu_status() will simply propagate the value to the caller.
>>>
>>> It might be worth doing a few tests to ensure that all works out as
>>> designed, but I'm pretty sure it should.
>>>  
>> It just occurred to me that you must not be using the run scripts
>> anyway, since they would require further patches to work. In that
>> case, there's no need to change arm/run unless you also provide those
>> additional patches.
>>
>> BTW, I wouldn't be opposed to a second run script, rather than trying
>> to make one script work for both qemu and kvmtool. Ideally both
>> scripts would be driven by the same higher level scripts using the
>> same unittests.cfg file though. The unittests.cfg extra_params field
>> will make that a bit challenging, but otherwise I think adding a few
>> new helper functions to scripts/arch-run.bash may be all that's
>> necessary.
> Yeah, I had some patches along those lines: split test parameters
> from QEMU parameters, abstract common stuff like number of cores and
> amount of memory, mark tests as QEMU only and so on. And I had a
> separate run script for kvmtool, IIRC.
>
> If there is interest I can try to post them, but I would consider
> this an additional effort on top of this series.
>
> Cheers,
> Andre.

I don't use the kvm-unit-tests run script, that is correct. I would prefer that
I don't change arm/run and keep the function exit() like you originally suggested:

  void exit(int code)
  {
      chr_testdev_exit(code);
      psci_system_off();
      halt(code);
      __builtin_unreachable();
  }

If anyone is interested, I or Andre can post patches for the run scripts as a separate set. Is that alright with you, Andrew?

Re: [kvm-unit-tests PATCH 2/7] lib: arm: Remove warning about uart0_base mismatch

[Andrew Jones]

On Fri, Jan 25, 2019 at 04:36:13PM +0000, Alexandru Elisei wrote:
> On 1/24/19 12:37 PM, Andrew Jones wrote:
> > On Thu, Jan 24, 2019 at 11:59:43AM +0000, Andre Przywara wrote:
> >> On Thu, 24 Jan 2019 11:16:29 +0000
> >> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> >>
> >>> A warning is displayed if uart0_base is different from what the code
> >>> expects qemu to generate for the pl011 UART in the device tree.
> >>> However, now we support the ns16550a UART emulated by kvmtool, which
> >>> has a different address. This leads to the  warning being displayed
> >>> even though the UART is configured and working as expected.
> >>>
> >>> Now that we support multiple UARTs, the warning serves no purpose, so
> >>> remove it.
> >> Mmh, but we use that address before, right? So for anything not
> >> emulating an UART at this QEMU specific address we write to some random
> >> (device) memory?
> >>
> >> Drew, how important is this early print feature for kvm-unit-tests?
> > The setup code passes through quite a few asserts before getting through
> > io_init() (including in uart0_init), so I think there's still value in
> > having a guessed UART address. Maybe we can provide guesses for both
> > QEMU and kvmtool, and some selection method, that would be used until
> > we've properly assigned uart0_base from DT?
> >
> >> Cheers,
> >> Andre.
> >>
> >>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> >>> ---
> >>>  lib/arm/io.c | 6 ------
> >>>  1 file changed, 6 deletions(-)
> >>>
> >>> diff --git a/lib/arm/io.c b/lib/arm/io.c
> >>> index 35fc05aeb4db..87435150f73e 100644
> >>> --- a/lib/arm/io.c
> >>> +++ b/lib/arm/io.c
> >>> @@ -61,12 +61,6 @@ static void uart0_init(void)
> >>>  	}
> >>>  
> >>>  	uart0_base = ioremap(base.addr, base.size);
> >>> -
> >>> -	if (uart0_base != (u8 *)UART_EARLY_BASE) {
> >>> -		printf("WARNING: early print support may not work. "
> >>> -		       "Found uart at %p, but early base is %p.\n",
> >>> -			uart0_base, (u8 *)UART_EARLY_BASE);
> >>> -	}
> >>>  }
> > This warning is doing what it should, which is pointing out that the
> > UART_EARLY_BASE guess appears to be wrong. If we can provide a way
> > to support more than one guess, then we should keep this warning but
> > adjust it to match one of any of the guesses.
> >
> > Thanks,
> > drew
> 
> I'm not really sure how to implement a selection method. I've looked at
> splitting io_init() into uart0_init() and chr_testdev_init() and calling
> uart0_init() very early in the setup process, but uart0_init() itself uses
> printf() and assert().
> 
> I've also thought about adding another function, something like
> uart0_early_init(), that is called very early in setup() and gets the base
> address from the dtb bootargs. But that means calling dt_init() and
> dt_get_bootargs(), which can fail.
> 
> One other option that could work is to make it a compile-time configuration.
> 
> What do you think?
>

Compile-time is fine, which I guess will result in a new configure script
option as well. I wonder if we shouldn't consider generating a config.h
file with stuff like this rather than adding another -D to the compile
line.

drew 

Re: [kvm-unit-tests PATCH 5/7] lib: arm: Fallback to psci_system_off() in exit()

[Andrew Jones]

On Fri, Jan 25, 2019 at 04:44:39PM +0000, Alexandru Elisei wrote:
> On 1/25/19 4:14 PM, Andre Przywara wrote:
> > On Fri, 25 Jan 2019 17:05:45 +0100
> > Andrew Jones <drjones@redhat.com> wrote:
> >
> > Hi,
> >
> >> On Fri, Jan 25, 2019 at 04:31:37PM +0100, Andrew Jones wrote:
> >>> On Fri, Jan 25, 2019 at 02:56:30PM +0000, Alexandru Elisei wrote:  
> >>>> On 1/24/19 1:35 PM, Andrew Jones wrote:  
> >>>>> On Thu, Jan 24, 2019 at 02:00:20PM +0100, Andrew Jones wrote:  
> >>>>>> [..]
> >>>>>> chr_testdev_init() ensures vcon is NULL if it fails to
> >>>>>> initialize. chr_testdev_exit() immediately returns if vcon is
> >>>>>> NULL. This was done by design to allow fallback exits to be
> >>>>>> placed below the chr_testdev_exit call, e.g. halt().
> >>>>>>
> >>>>>> We should be able to drop patch 3/7 and change exit() to this
> >>>>>>
> >>>>>>   void exit(int code)
> >>>>>>   {
> >>>>>>       chr_testdev_exit(code);
> >>>>>>       psci_system_off();
> >>>>>>       halt(code);
> >>>>>>       __builtin_unreachable();
> >>>>>>   }
> >>>>>>  
> >>>>> There's also a framework for exits that can't return status
> >>>>> codes. powerpc uses it. Before exiting with psci_system_off we
> >>>>> need to make this print statement
> >>>>>
> >>>>>  printf("\nEXIT: STATUS=%d\n", ((code) << 1) | 1); 
> >>>>>
> >>>>> And run_qemu in arm/run needs to be changed to run_qemu_status.
> >>>>> It's hacky, but maybe we can live with it until kvmtool offers
> >>>>> some sort of debug exit.
> >>>>>
> >>>>> Thanks,
> >>>>> drew  
> >>>> I can make the change, but if I understand the
> >>>> scripts/arch-run.bash code correctly, run_qemu() will check if
> >>>> QEMU was terminated because of a signal and adjust the return
> >>>> code to take that into account. Using run_qemu_status() means
> >>>> that check won't be made when the tests are run under QEMU, is
> >>>> that acceptable? 
> >>> run_qemu_status() first calls run_qemu(). If the return value from
> >>> run_qemu() is 1, then it'll change the value to whatever the EXIT:
> >>> status line says it should be. If run_qemu() detected that a signal
> >>> caused the exit, then the return value won't be 1. In that case
> >>> run_qemu_status() will simply propagate the value to the caller.
> >>>
> >>> It might be worth doing a few tests to ensure that all works out as
> >>> designed, but I'm pretty sure it should.
> >>>  
> >> It just occurred to me that you must not be using the run scripts
> >> anyway, since they would require further patches to work. In that
> >> case, there's no need to change arm/run unless you also provide those
> >> additional patches.
> >>
> >> BTW, I wouldn't be opposed to a second run script, rather than trying
> >> to make one script work for both qemu and kvmtool. Ideally both
> >> scripts would be driven by the same higher level scripts using the
> >> same unittests.cfg file though. The unittests.cfg extra_params field
> >> will make that a bit challenging, but otherwise I think adding a few
> >> new helper functions to scripts/arch-run.bash may be all that's
> >> necessary.
> > Yeah, I had some patches along those lines: split test parameters
> > from QEMU parameters, abstract common stuff like number of cores and
> > amount of memory, mark tests as QEMU only and so on. And I had a
> > separate run script for kvmtool, IIRC.
> >
> > If there is interest I can try to post them, but I would consider
> > this an additional effort on top of this series.
> >
> > Cheers,
> > Andre.
> 
> I don't use the kvm-unit-tests run script, that is correct. I would prefer that
> I don't change arm/run and keep the function exit() like you originally suggested:
> 
>   void exit(int code)
>   {
>       chr_testdev_exit(code);
>       psci_system_off();
>       halt(code);
>       __builtin_unreachable();
>   }
> 
> If anyone is interested, I or Andre can post patches for the run scripts as a separate set. Is that alright with you, Andrew?
>

Yup, that's fine.

Re: [kvm-unit-tests PATCH 7/7] arm/arm64: Use argv_find() for test names

[Alexandru Elisei]

On 1/24/19 1:07 PM, Andrew Jones wrote:
> On Thu, Jan 24, 2019 at 11:16:34AM +0000, Alexandru Elisei wrote:
>> Instead of aborting the test when an unexpected parameter is found, use
>> argv_find() to search for the desired parameter. On arm and arm64, this
>> allows kvm-unit-tests to be used with virtual machine monitors like kvmtool
>> which automatically adds parameters to the kernel command line.
>>
>> For example, to run the gicv3-ipi test the following command was used:
>>
>> lkvm run -k gic.flat -c 8 -m 410 -p "ipi" --irqchip=gicv3
>>
>> The resulting kernel command line that was passed to kvm-unit-tests was:
>>
>> "console=ttyS0 rw rootflags=trans=virtio,version=9p2000.L,cache=loose rootfstype=9p init=/virt/init  ip=dhcp ipi"
> Why not write a patch for kvmtool that provides a new command line
> parameter which says to not add any kernel parameters other than what
> is provided with '-p'? Giving the user full control over the kernel
> command line seems like a feature kvmtool would want anyway.
>
> Thanks,
> drew

A patch to remove the kvmtool kernel command line has been posted [1]. I will
drop patches 6 and 7, thank you for the feedback.

[1] https://www.spinics.net/lists/kvm/msg181009.html

Re: [kvm-unit-tests PATCH 2/7] lib: arm: Remove warning about uart0_base mismatch

[Alexandru Elisei]

On 1/25/19 4:47 PM, Andrew Jones wrote:
> On Fri, Jan 25, 2019 at 04:36:13PM +0000, Alexandru Elisei wrote:
>> On 1/24/19 12:37 PM, Andrew Jones wrote:
>>> On Thu, Jan 24, 2019 at 11:59:43AM +0000, Andre Przywara wrote:
>>>> On Thu, 24 Jan 2019 11:16:29 +0000
>>>> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>>>>
>>>>> A warning is displayed if uart0_base is different from what the code
>>>>> expects qemu to generate for the pl011 UART in the device tree.
>>>>> However, now we support the ns16550a UART emulated by kvmtool, which
>>>>> has a different address. This leads to the  warning being displayed
>>>>> even though the UART is configured and working as expected.
>>>>>
>>>>> Now that we support multiple UARTs, the warning serves no purpose, so
>>>>> remove it.
>>>> Mmh, but we use that address before, right? So for anything not
>>>> emulating an UART at this QEMU specific address we write to some random
>>>> (device) memory?
>>>>
>>>> Drew, how important is this early print feature for kvm-unit-tests?
>>> The setup code passes through quite a few asserts before getting through
>>> io_init() (including in uart0_init), so I think there's still value in
>>> having a guessed UART address. Maybe we can provide guesses for both
>>> QEMU and kvmtool, and some selection method, that would be used until
>>> we've properly assigned uart0_base from DT?
>>>
>>>> Cheers,
>>>> Andre.
>>>>
>>>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>>>>> ---
>>>>>  lib/arm/io.c | 6 ------
>>>>>  1 file changed, 6 deletions(-)
>>>>>
>>>>> diff --git a/lib/arm/io.c b/lib/arm/io.c
>>>>> index 35fc05aeb4db..87435150f73e 100644
>>>>> --- a/lib/arm/io.c
>>>>> +++ b/lib/arm/io.c
>>>>> @@ -61,12 +61,6 @@ static void uart0_init(void)
>>>>>  	}
>>>>>  
>>>>>  	uart0_base = ioremap(base.addr, base.size);
>>>>> -
>>>>> -	if (uart0_base != (u8 *)UART_EARLY_BASE) {
>>>>> -		printf("WARNING: early print support may not work. "
>>>>> -		       "Found uart at %p, but early base is %p.\n",
>>>>> -			uart0_base, (u8 *)UART_EARLY_BASE);
>>>>> -	}
>>>>>  }
>>> This warning is doing what it should, which is pointing out that the
>>> UART_EARLY_BASE guess appears to be wrong. If we can provide a way
>>> to support more than one guess, then we should keep this warning but
>>> adjust it to match one of any of the guesses.
>>>
>>> Thanks,
>>> drew
>> I'm not really sure how to implement a selection method. I've looked at
>> splitting io_init() into uart0_init() and chr_testdev_init() and calling
>> uart0_init() very early in the setup process, but uart0_init() itself uses
>> printf() and assert().
>>
>> I've also thought about adding another function, something like
>> uart0_early_init(), that is called very early in setup() and gets the base
>> address from the dtb bootargs. But that means calling dt_init() and
>> dt_get_bootargs(), which can fail.
>>
>> One other option that could work is to make it a compile-time configuration.
>>
>> What do you think?
>>
> Compile-time is fine, which I guess will result in a new configure script
> option as well. I wonder if we shouldn't consider generating a config.h
> file with stuff like this rather than adding another -D to the compile
> line.
>
> drew 

I propose a new configuration option called --vmm, with possible values qemu and
kvmtool, which defaults to qemu if not set.

Another possibility would be to have an --uart-base option, but that means we
are expecting the user to be aware of the uart base address for the virtual
machine manager, which might be unreasonable.

This is a quick prototype of how using -D for conditional compilation would look
like (the configure changes are included too):

diff --git a/configure b/configure
index df8581e3a906..7a56ba47707f 100755
--- a/configure
+++ b/configure
@@ -70,6 +70,9 @@ while [[ "$1" = -* ]]; do
            ;;
        --ld)
            ld="$arg"
+        ;;
+    --vmm)
+        vmm="$arg"
            ;;
        --enable-pretty-print-stacks)
            pretty_print_stacks=yes
@@ -108,6 +111,14 @@ if [ "$arch" = "i386" ] || [ "$arch" = "x86_64" ]; then
     testdir=x86
 elif [ "$arch" = "arm" ] || [ "$arch" = "arm64" ]; then
     testdir=arm
+    if [ -z "$vmm" ] || [ "$vmm" = "qemu" ]; then
+        uart_early_base=0x09000000UL
+    elif [ "$vmm" = "kvmtool" ]; then
+        uart_early_base=0x3f8
+    else
+        echo '--vmm must be one of "qemu" or "kvmtool"'
+        usage
+    fi
 elif [ "$arch" = "ppc64" ]; then
     testdir=powerpc
     firmware="$testdir/boot_rom.bin"
@@ -197,4 +208,5 @@ PRETTY_PRINT_STACKS=$pretty_print_stacks
 ENVIRON_DEFAULT=$environ_default
 ERRATATXT=errata.txt
 U32_LONG_FMT=$u32_long
+UART_EARLY_BASE=$uart_early_base
 EOF
diff --git a/Makefile b/Makefile
index e9f02272e156..225c2a525cdf 100644
--- a/Makefile
+++ b/Makefile
@@ -72,6 +72,10 @@ COMMON_CFLAGS += $(fno_pic) $(no_pie)
 CFLAGS += $(COMMON_CFLAGS)
 CFLAGS += -Wmissing-parameter-type -Wold-style-declaration -Woverride-init
 
+ifneq ($(UART_EARLY_BASE),)
+CFLAGS += -DUART_EARLY_BASE=$(UART_EARLY_BASE)
+endif
+
 CXXFLAGS += $(COMMON_CFLAGS)
 
 autodepend-flags = -MMD -MF $(dir $*).$(notdir $*).d

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH 2/7] lib: arm: Remove warning about uart0_base mismatch

[Andrew Jones]

On Mon, Jan 28, 2019 at 02:24:29PM +0000, Alexandru Elisei wrote:
> On 1/25/19 4:47 PM, Andrew Jones wrote:
> > On Fri, Jan 25, 2019 at 04:36:13PM +0000, Alexandru Elisei wrote:
> >> On 1/24/19 12:37 PM, Andrew Jones wrote:
> >>> On Thu, Jan 24, 2019 at 11:59:43AM +0000, Andre Przywara wrote:
> >>>> On Thu, 24 Jan 2019 11:16:29 +0000
> >>>> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> >>>>
> >>>>> A warning is displayed if uart0_base is different from what the code
> >>>>> expects qemu to generate for the pl011 UART in the device tree.
> >>>>> However, now we support the ns16550a UART emulated by kvmtool, which
> >>>>> has a different address. This leads to the  warning being displayed
> >>>>> even though the UART is configured and working as expected.
> >>>>>
> >>>>> Now that we support multiple UARTs, the warning serves no purpose, so
> >>>>> remove it.
> >>>> Mmh, but we use that address before, right? So for anything not
> >>>> emulating an UART at this QEMU specific address we write to some random
> >>>> (device) memory?
> >>>>
> >>>> Drew, how important is this early print feature for kvm-unit-tests?
> >>> The setup code passes through quite a few asserts before getting through
> >>> io_init() (including in uart0_init), so I think there's still value in
> >>> having a guessed UART address. Maybe we can provide guesses for both
> >>> QEMU and kvmtool, and some selection method, that would be used until
> >>> we've properly assigned uart0_base from DT?
> >>>
> >>>> Cheers,
> >>>> Andre.
> >>>>
> >>>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> >>>>> ---
> >>>>>  lib/arm/io.c | 6 ------
> >>>>>  1 file changed, 6 deletions(-)
> >>>>>
> >>>>> diff --git a/lib/arm/io.c b/lib/arm/io.c
> >>>>> index 35fc05aeb4db..87435150f73e 100644
> >>>>> --- a/lib/arm/io.c
> >>>>> +++ b/lib/arm/io.c
> >>>>> @@ -61,12 +61,6 @@ static void uart0_init(void)
> >>>>>  	}
> >>>>>  
> >>>>>  	uart0_base = ioremap(base.addr, base.size);
> >>>>> -
> >>>>> -	if (uart0_base != (u8 *)UART_EARLY_BASE) {
> >>>>> -		printf("WARNING: early print support may not work. "
> >>>>> -		       "Found uart at %p, but early base is %p.\n",
> >>>>> -			uart0_base, (u8 *)UART_EARLY_BASE);
> >>>>> -	}
> >>>>>  }
> >>> This warning is doing what it should, which is pointing out that the
> >>> UART_EARLY_BASE guess appears to be wrong. If we can provide a way
> >>> to support more than one guess, then we should keep this warning but
> >>> adjust it to match one of any of the guesses.
> >>>
> >>> Thanks,
> >>> drew
> >> I'm not really sure how to implement a selection method. I've looked at
> >> splitting io_init() into uart0_init() and chr_testdev_init() and calling
> >> uart0_init() very early in the setup process, but uart0_init() itself uses
> >> printf() and assert().
> >>
> >> I've also thought about adding another function, something like
> >> uart0_early_init(), that is called very early in setup() and gets the base
> >> address from the dtb bootargs. But that means calling dt_init() and
> >> dt_get_bootargs(), which can fail.
> >>
> >> One other option that could work is to make it a compile-time configuration.
> >>
> >> What do you think?
> >>
> > Compile-time is fine, which I guess will result in a new configure script
> > option as well. I wonder if we shouldn't consider generating a config.h
> > file with stuff like this rather than adding another -D to the compile
> > line.
> >
> > drew 
> 
> I propose a new configuration option called --vmm, with possible values qemu and
> kvmtool, which defaults to qemu if not set.
> 
> Another possibility would be to have an --uart-base option, but that means we
> are expecting the user to be aware of the uart base address for the virtual
> machine manager, which might be unreasonable.
> 
> This is a quick prototype of how using -D for conditional compilation would look
> like (the configure changes are included too):
> 
> diff --git a/configure b/configure
> index df8581e3a906..7a56ba47707f 100755
> --- a/configure
> +++ b/configure
> @@ -70,6 +70,9 @@ while [[ "$1" = -* ]]; do
>             ;;
>         --ld)
>             ld="$arg"
> +        ;;
> +    --vmm)
> +        vmm="$arg"
>             ;;
>         --enable-pretty-print-stacks)
>             pretty_print_stacks=yes
> @@ -108,6 +111,14 @@ if [ "$arch" = "i386" ] || [ "$arch" = "x86_64" ]; then
>      testdir=x86
>  elif [ "$arch" = "arm" ] || [ "$arch" = "arm64" ]; then
>      testdir=arm
> +    if [ -z "$vmm" ] || [ "$vmm" = "qemu" ]; then
> +        uart_early_base=0x09000000UL

You can drop the 'UL'.

> +    elif [ "$vmm" = "kvmtool" ]; then
> +        uart_early_base=0x3f8
> +    else
> +        echo '--vmm must be one of "qemu" or "kvmtool"'
> +        usage

You're outputting usage here, but you didn't add vmm to the help text.

> +    fi
>  elif [ "$arch" = "ppc64" ]; then
>      testdir=powerpc
>      firmware="$testdir/boot_rom.bin"
> @@ -197,4 +208,5 @@ PRETTY_PRINT_STACKS=$pretty_print_stacks
>  ENVIRON_DEFAULT=$environ_default
>  ERRATATXT=errata.txt
>  U32_LONG_FMT=$u32_long
> +UART_EARLY_BASE=$uart_early_base
>  EOF
> diff --git a/Makefile b/Makefile
> index e9f02272e156..225c2a525cdf 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -72,6 +72,10 @@ COMMON_CFLAGS += $(fno_pic) $(no_pie)
>  CFLAGS += $(COMMON_CFLAGS)
>  CFLAGS += -Wmissing-parameter-type -Wold-style-declaration -Woverride-init
>  
> +ifneq ($(UART_EARLY_BASE),)
> +CFLAGS += -DUART_EARLY_BASE=$(UART_EARLY_BASE)
> +endif

This type of thing is what I would like to avoid by introducing a
config.h file. In the least we shouldn't add this -D to CFLAGS for
all architectures. It can be added to the %.elf rule in
arm/Makefile.common

> +
>  CXXFLAGS += $(COMMON_CFLAGS)
>  
>  autodepend-flags = -MMD -MF $(dir $*).$(notdir $*).d
>

You'll also want to patch lib/arm/io.c with
 
-/*
- * Use this guess for the pl011 base in order to make an attempt at
- * having earlier printf support. We'll overwrite it with the real
- * base address that we read from the device tree later. This is
- * the address we expect QEMU's mach-virt machine type to put in
- * its generated device tree.
- */
-#define UART_EARLY_BASE 0x09000000UL
-
 static struct spinlock uart_lock;
-static volatile u8 *uart0_base = (u8 *)UART_EARLY_BASE;
+static volatile u8 *uart0_base = (u8 *)(unsigned long)UART_EARLY_BASE;



This is all a bit on the ugly side, but I can't think of anything
better. 

Thanks,
drew

Re: [kvm-unit-tests PATCH 2/7] lib: arm: Remove warning about uart0_base mismatch

[Andre Przywara]

On Mon, 28 Jan 2019 17:31:01 +0100
Andrew Jones <drjones@redhat.com> wrote:

> On Mon, Jan 28, 2019 at 02:24:29PM +0000, Alexandru Elisei wrote:
> > On 1/25/19 4:47 PM, Andrew Jones wrote:  
> > > On Fri, Jan 25, 2019 at 04:36:13PM +0000, Alexandru Elisei
> > > wrote:  
> > >> On 1/24/19 12:37 PM, Andrew Jones wrote:  
> > >>> On Thu, Jan 24, 2019 at 11:59:43AM +0000, Andre Przywara
> > >>> wrote:  
> > >>>> On Thu, 24 Jan 2019 11:16:29 +0000
> > >>>> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> > >>>>  
> > >>>>> A warning is displayed if uart0_base is different from what
> > >>>>> the code expects qemu to generate for the pl011 UART in the
> > >>>>> device tree. However, now we support the ns16550a UART
> > >>>>> emulated by kvmtool, which has a different address. This
> > >>>>> leads to the  warning being displayed even though the UART is
> > >>>>> configured and working as expected.
> > >>>>>
> > >>>>> Now that we support multiple UARTs, the warning serves no
> > >>>>> purpose, so remove it.  
> > >>>> Mmh, but we use that address before, right? So for anything not
> > >>>> emulating an UART at this QEMU specific address we write to
> > >>>> some random (device) memory?
> > >>>>
> > >>>> Drew, how important is this early print feature for
> > >>>> kvm-unit-tests?  
> > >>> The setup code passes through quite a few asserts before
> > >>> getting through io_init() (including in uart0_init), so I think
> > >>> there's still value in having a guessed UART address. Maybe we
> > >>> can provide guesses for both QEMU and kvmtool, and some
> > >>> selection method, that would be used until we've properly
> > >>> assigned uart0_base from DT? 
> > >>>> Cheers,
> > >>>> Andre.
> > >>>>  
> > >>>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > >>>>> ---
> > >>>>>  lib/arm/io.c | 6 ------
> > >>>>>  1 file changed, 6 deletions(-)
> > >>>>>
> > >>>>> diff --git a/lib/arm/io.c b/lib/arm/io.c
> > >>>>> index 35fc05aeb4db..87435150f73e 100644
> > >>>>> --- a/lib/arm/io.c
> > >>>>> +++ b/lib/arm/io.c
> > >>>>> @@ -61,12 +61,6 @@ static void uart0_init(void)
> > >>>>>  	}
> > >>>>>  
> > >>>>>  	uart0_base = ioremap(base.addr, base.size);
> > >>>>> -
> > >>>>> -	if (uart0_base != (u8 *)UART_EARLY_BASE) {
> > >>>>> -		printf("WARNING: early print support may not
> > >>>>> work. "
> > >>>>> -		       "Found uart at %p, but early base is
> > >>>>> %p.\n",
> > >>>>> -			uart0_base, (u8 *)UART_EARLY_BASE);
> > >>>>> -	}
> > >>>>>  }  
> > >>> This warning is doing what it should, which is pointing out
> > >>> that the UART_EARLY_BASE guess appears to be wrong. If we can
> > >>> provide a way to support more than one guess, then we should
> > >>> keep this warning but adjust it to match one of any of the
> > >>> guesses.
> > >>>
> > >>> Thanks,
> > >>> drew  
> > >> I'm not really sure how to implement a selection method. I've
> > >> looked at splitting io_init() into uart0_init() and
> > >> chr_testdev_init() and calling uart0_init() very early in the
> > >> setup process, but uart0_init() itself uses printf() and
> > >> assert().
> > >>
> > >> I've also thought about adding another function, something like
> > >> uart0_early_init(), that is called very early in setup() and
> > >> gets the base address from the dtb bootargs. But that means
> > >> calling dt_init() and dt_get_bootargs(), which can fail.
> > >>
> > >> One other option that could work is to make it a compile-time
> > >> configuration.
> > >>
> > >> What do you think?
> > >>  
> > > Compile-time is fine, which I guess will result in a new
> > > configure script option as well. I wonder if we shouldn't
> > > consider generating a config.h file with stuff like this rather
> > > than adding another -D to the compile line.
> > >
> > > drew   
> > 
> > I propose a new configuration option called --vmm, with possible
> > values qemu and kvmtool, which defaults to qemu if not set.
> > 
> > Another possibility would be to have an --uart-base option, but
> > that means we are expecting the user to be aware of the uart base
> > address for the virtual machine manager, which might be
> > unreasonable.
> > 
> > This is a quick prototype of how using -D for conditional
> > compilation would look like (the configure changes are included
> > too):
> > 
> > diff --git a/configure b/configure
> > index df8581e3a906..7a56ba47707f 100755
> > --- a/configure
> > +++ b/configure
> > @@ -70,6 +70,9 @@ while [[ "$1" = -* ]]; do
> >             ;;
> >         --ld)
> >             ld="$arg"
> > +        ;;
> > +    --vmm)
> > +        vmm="$arg"
> >             ;;
> >         --enable-pretty-print-stacks)
> >             pretty_print_stacks=yes
> > @@ -108,6 +111,14 @@ if [ "$arch" = "i386" ] || [ "$arch" =
> > "x86_64" ]; then testdir=x86
> >  elif [ "$arch" = "arm" ] || [ "$arch" = "arm64" ]; then
> >      testdir=arm
> > +    if [ -z "$vmm" ] || [ "$vmm" = "qemu" ]; then
> > +        uart_early_base=0x09000000UL  
> 
> You can drop the 'UL'.
> 
> > +    elif [ "$vmm" = "kvmtool" ]; then
> > +        uart_early_base=0x3f8
> > +    else
> > +        echo '--vmm must be one of "qemu" or "kvmtool"'
> > +        usage  
> 
> You're outputting usage here, but you didn't add vmm to the help text.
> 
> > +    fi
> >  elif [ "$arch" = "ppc64" ]; then
> >      testdir=powerpc
> >      firmware="$testdir/boot_rom.bin"
> > @@ -197,4 +208,5 @@ PRETTY_PRINT_STACKS=$pretty_print_stacks
> >  ENVIRON_DEFAULT=$environ_default
> >  ERRATATXT=errata.txt
> >  U32_LONG_FMT=$u32_long
> > +UART_EARLY_BASE=$uart_early_base
> >  EOF
> > diff --git a/Makefile b/Makefile
> > index e9f02272e156..225c2a525cdf 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -72,6 +72,10 @@ COMMON_CFLAGS += $(fno_pic) $(no_pie)
> >  CFLAGS += $(COMMON_CFLAGS)
> >  CFLAGS += -Wmissing-parameter-type -Wold-style-declaration
> > -Woverride-init 
> > +ifneq ($(UART_EARLY_BASE),)
> > +CFLAGS += -DUART_EARLY_BASE=$(UART_EARLY_BASE)
> > +endif  
> 
> This type of thing is what I would like to avoid by introducing a
> config.h file. In the least we shouldn't add this -D to CFLAGS for
> all architectures. It can be added to the %.elf rule in
> arm/Makefile.common
> 
> > +
> >  CXXFLAGS += $(COMMON_CFLAGS)
> >  
> >  autodepend-flags = -MMD -MF $(dir $*).$(notdir $*).d
> >  
> 
> You'll also want to patch lib/arm/io.c with
>  
> -/*
> - * Use this guess for the pl011 base in order to make an attempt at
> - * having earlier printf support. We'll overwrite it with the real
> - * base address that we read from the device tree later. This is
> - * the address we expect QEMU's mach-virt machine type to put in
> - * its generated device tree.
> - */
> -#define UART_EARLY_BASE 0x09000000UL
> -
>  static struct spinlock uart_lock;
> -static volatile u8 *uart0_base = (u8 *)UART_EARLY_BASE;
> +static volatile u8 *uart0_base = (u8 *)(unsigned
> long)UART_EARLY_BASE;
> 
> 
> 
> This is all a bit on the ugly side, but I can't think of anything
> better. 

Cheeky question: Can't we try to somehow auto detect the VMM?
In the most simple case we could look at our load address / PC and
deduce QEMU vs. kvmtool from that. That's surely somewhat hacky, but
should be more robust than a compile time version.
Typically a good way to confirm some idea of a current platform is to
read some peripheral ID registers and check those, the GIC is typically
a good candidate. Although I see that our emulation in KVM is a bit thin
on this side ...

Cheers,
Andre.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH 2/7] lib: arm: Remove warning about uart0_base mismatch

[Andrew Jones]

On Mon, Jan 28, 2019 at 05:58:54PM +0000, Andre Przywara wrote:
> On Mon, 28 Jan 2019 17:31:01 +0100
> Andrew Jones <drjones@redhat.com> wrote:
> 
> > On Mon, Jan 28, 2019 at 02:24:29PM +0000, Alexandru Elisei wrote:
> > > On 1/25/19 4:47 PM, Andrew Jones wrote:  
> > > > On Fri, Jan 25, 2019 at 04:36:13PM +0000, Alexandru Elisei
> > > > wrote:  
> > > >> On 1/24/19 12:37 PM, Andrew Jones wrote:  
> > > >>> On Thu, Jan 24, 2019 at 11:59:43AM +0000, Andre Przywara
> > > >>> wrote:  
> > > >>>> On Thu, 24 Jan 2019 11:16:29 +0000
> > > >>>> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> > > >>>>  
> > > >>>>> A warning is displayed if uart0_base is different from what
> > > >>>>> the code expects qemu to generate for the pl011 UART in the
> > > >>>>> device tree. However, now we support the ns16550a UART
> > > >>>>> emulated by kvmtool, which has a different address. This
> > > >>>>> leads to the  warning being displayed even though the UART is
> > > >>>>> configured and working as expected.
> > > >>>>>
> > > >>>>> Now that we support multiple UARTs, the warning serves no
> > > >>>>> purpose, so remove it.  
> > > >>>> Mmh, but we use that address before, right? So for anything not
> > > >>>> emulating an UART at this QEMU specific address we write to
> > > >>>> some random (device) memory?
> > > >>>>
> > > >>>> Drew, how important is this early print feature for
> > > >>>> kvm-unit-tests?  
> > > >>> The setup code passes through quite a few asserts before
> > > >>> getting through io_init() (including in uart0_init), so I think
> > > >>> there's still value in having a guessed UART address. Maybe we
> > > >>> can provide guesses for both QEMU and kvmtool, and some
> > > >>> selection method, that would be used until we've properly
> > > >>> assigned uart0_base from DT? 
> > > >>>> Cheers,
> > > >>>> Andre.
> > > >>>>  
> > > >>>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > > >>>>> ---
> > > >>>>>  lib/arm/io.c | 6 ------
> > > >>>>>  1 file changed, 6 deletions(-)
> > > >>>>>
> > > >>>>> diff --git a/lib/arm/io.c b/lib/arm/io.c
> > > >>>>> index 35fc05aeb4db..87435150f73e 100644
> > > >>>>> --- a/lib/arm/io.c
> > > >>>>> +++ b/lib/arm/io.c
> > > >>>>> @@ -61,12 +61,6 @@ static void uart0_init(void)
> > > >>>>>  	}
> > > >>>>>  
> > > >>>>>  	uart0_base = ioremap(base.addr, base.size);
> > > >>>>> -
> > > >>>>> -	if (uart0_base != (u8 *)UART_EARLY_BASE) {
> > > >>>>> -		printf("WARNING: early print support may not
> > > >>>>> work. "
> > > >>>>> -		       "Found uart at %p, but early base is
> > > >>>>> %p.\n",
> > > >>>>> -			uart0_base, (u8 *)UART_EARLY_BASE);
> > > >>>>> -	}
> > > >>>>>  }  
> > > >>> This warning is doing what it should, which is pointing out
> > > >>> that the UART_EARLY_BASE guess appears to be wrong. If we can
> > > >>> provide a way to support more than one guess, then we should
> > > >>> keep this warning but adjust it to match one of any of the
> > > >>> guesses.
> > > >>>
> > > >>> Thanks,
> > > >>> drew  
> > > >> I'm not really sure how to implement a selection method. I've
> > > >> looked at splitting io_init() into uart0_init() and
> > > >> chr_testdev_init() and calling uart0_init() very early in the
> > > >> setup process, but uart0_init() itself uses printf() and
> > > >> assert().
> > > >>
> > > >> I've also thought about adding another function, something like
> > > >> uart0_early_init(), that is called very early in setup() and
> > > >> gets the base address from the dtb bootargs. But that means
> > > >> calling dt_init() and dt_get_bootargs(), which can fail.
> > > >>
> > > >> One other option that could work is to make it a compile-time
> > > >> configuration.
> > > >>
> > > >> What do you think?
> > > >>  
> > > > Compile-time is fine, which I guess will result in a new
> > > > configure script option as well. I wonder if we shouldn't
> > > > consider generating a config.h file with stuff like this rather
> > > > than adding another -D to the compile line.
> > > >
> > > > drew   
> > > 
> > > I propose a new configuration option called --vmm, with possible
> > > values qemu and kvmtool, which defaults to qemu if not set.
> > > 
> > > Another possibility would be to have an --uart-base option, but
> > > that means we are expecting the user to be aware of the uart base
> > > address for the virtual machine manager, which might be
> > > unreasonable.
> > > 
> > > This is a quick prototype of how using -D for conditional
> > > compilation would look like (the configure changes are included
> > > too):
> > > 
> > > diff --git a/configure b/configure
> > > index df8581e3a906..7a56ba47707f 100755
> > > --- a/configure
> > > +++ b/configure
> > > @@ -70,6 +70,9 @@ while [[ "$1" = -* ]]; do
> > >             ;;
> > >         --ld)
> > >             ld="$arg"
> > > +        ;;
> > > +    --vmm)
> > > +        vmm="$arg"
> > >             ;;
> > >         --enable-pretty-print-stacks)
> > >             pretty_print_stacks=yes
> > > @@ -108,6 +111,14 @@ if [ "$arch" = "i386" ] || [ "$arch" =
> > > "x86_64" ]; then testdir=x86
> > >  elif [ "$arch" = "arm" ] || [ "$arch" = "arm64" ]; then
> > >      testdir=arm
> > > +    if [ -z "$vmm" ] || [ "$vmm" = "qemu" ]; then
> > > +        uart_early_base=0x09000000UL  
> > 
> > You can drop the 'UL'.
> > 
> > > +    elif [ "$vmm" = "kvmtool" ]; then
> > > +        uart_early_base=0x3f8
> > > +    else
> > > +        echo '--vmm must be one of "qemu" or "kvmtool"'
> > > +        usage  
> > 
> > You're outputting usage here, but you didn't add vmm to the help text.
> > 
> > > +    fi
> > >  elif [ "$arch" = "ppc64" ]; then
> > >      testdir=powerpc
> > >      firmware="$testdir/boot_rom.bin"
> > > @@ -197,4 +208,5 @@ PRETTY_PRINT_STACKS=$pretty_print_stacks
> > >  ENVIRON_DEFAULT=$environ_default
> > >  ERRATATXT=errata.txt
> > >  U32_LONG_FMT=$u32_long
> > > +UART_EARLY_BASE=$uart_early_base
> > >  EOF
> > > diff --git a/Makefile b/Makefile
> > > index e9f02272e156..225c2a525cdf 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -72,6 +72,10 @@ COMMON_CFLAGS += $(fno_pic) $(no_pie)
> > >  CFLAGS += $(COMMON_CFLAGS)
> > >  CFLAGS += -Wmissing-parameter-type -Wold-style-declaration
> > > -Woverride-init 
> > > +ifneq ($(UART_EARLY_BASE),)
> > > +CFLAGS += -DUART_EARLY_BASE=$(UART_EARLY_BASE)
> > > +endif  
> > 
> > This type of thing is what I would like to avoid by introducing a
> > config.h file. In the least we shouldn't add this -D to CFLAGS for
> > all architectures. It can be added to the %.elf rule in
> > arm/Makefile.common
> > 
> > > +
> > >  CXXFLAGS += $(COMMON_CFLAGS)
> > >  
> > >  autodepend-flags = -MMD -MF $(dir $*).$(notdir $*).d
> > >  
> > 
> > You'll also want to patch lib/arm/io.c with
> >  
> > -/*
> > - * Use this guess for the pl011 base in order to make an attempt at
> > - * having earlier printf support. We'll overwrite it with the real
> > - * base address that we read from the device tree later. This is
> > - * the address we expect QEMU's mach-virt machine type to put in
> > - * its generated device tree.
> > - */
> > -#define UART_EARLY_BASE 0x09000000UL
> > -
> >  static struct spinlock uart_lock;
> > -static volatile u8 *uart0_base = (u8 *)UART_EARLY_BASE;
> > +static volatile u8 *uart0_base = (u8 *)(unsigned
> > long)UART_EARLY_BASE;
> > 
> > 
> > 
> > This is all a bit on the ugly side, but I can't think of anything
> > better. 
> 
> Cheeky question: Can't we try to somehow auto detect the VMM?
> In the most simple case we could look at our load address / PC and
> deduce QEMU vs. kvmtool from that.

I thought about this one, but wasn't sure I wanted to embed a heuristic in
early boot code. It does seem that QEMU mach-virt will *always* have the
base of RAM at 1G, so if kvmtool will *never* have the base of ram at 1G,
then it would work. But what if a third vmm (something like firecracker)
comes along that matches one or the other?

> That's surely somewhat hacky, but
> should be more robust than a compile time version.

Not when considering more than two vmms could be involved, or that a vmm
could change its base of ram. Also, an explicit uart-base=<addr> should
be the easiest thing to fix if addr turns out to be guessed wrong.

> Typically a good way to confirm some idea of a current platform is to
> read some peripheral ID registers and check those, the GIC is typically
> a good candidate. Although I see that our emulation in KVM is a bit thin
> on this side ...

We need this address because we want to be able to probe for peripherals
with working asserts. So if we choose to use a heuristic it needs to be
something we can do with a few instructions right at boot. Base of RAM
was the only thing that came to my mind besides passing in what we need
through auxinfo.

Being able to control implementation defined sysregs for things like this
would be nice.

Thanks,
drew

Re: [kvm-unit-tests PATCH 2/7] lib: arm: Remove warning about uart0_base mismatch

[Alexandru Elisei]

On 1/28/19 4:31 PM, Andrew Jones wrote:
> On Mon, Jan 28, 2019 at 02:24:29PM +0000, Alexandru Elisei wrote:
>> On 1/25/19 4:47 PM, Andrew Jones wrote:
>>> On Fri, Jan 25, 2019 at 04:36:13PM +0000, Alexandru Elisei wrote:
>>>> On 1/24/19 12:37 PM, Andrew Jones wrote:
>>>>> On Thu, Jan 24, 2019 at 11:59:43AM +0000, Andre Przywara wrote:
>>>>>> On Thu, 24 Jan 2019 11:16:29 +0000
>>>>>> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>>>>>>
>>>>>>> A warning is displayed if uart0_base is different from what the code
>>>>>>> expects qemu to generate for the pl011 UART in the device tree.
>>>>>>> However, now we support the ns16550a UART emulated by kvmtool, which
>>>>>>> has a different address. This leads to the  warning being displayed
>>>>>>> even though the UART is configured and working as expected.
>>>>>>>
>>>>>>> Now that we support multiple UARTs, the warning serves no purpose, so
>>>>>>> remove it.
>>>>>> Mmh, but we use that address before, right? So for anything not
>>>>>> emulating an UART at this QEMU specific address we write to some random
>>>>>> (device) memory?
>>>>>>
>>>>>> Drew, how important is this early print feature for kvm-unit-tests?
>>>>> The setup code passes through quite a few asserts before getting through
>>>>> io_init() (including in uart0_init), so I think there's still value in
>>>>> having a guessed UART address. Maybe we can provide guesses for both
>>>>> QEMU and kvmtool, and some selection method, that would be used until
>>>>> we've properly assigned uart0_base from DT?
>>>>>
>>>>>> Cheers,
>>>>>> Andre.
>>>>>>
>>>>>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>>>>>>> ---
>>>>>>>  lib/arm/io.c | 6 ------
>>>>>>>  1 file changed, 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/lib/arm/io.c b/lib/arm/io.c
>>>>>>> index 35fc05aeb4db..87435150f73e 100644
>>>>>>> --- a/lib/arm/io.c
>>>>>>> +++ b/lib/arm/io.c
>>>>>>> @@ -61,12 +61,6 @@ static void uart0_init(void)
>>>>>>>  	}
>>>>>>>  
>>>>>>>  	uart0_base = ioremap(base.addr, base.size);
>>>>>>> -
>>>>>>> -	if (uart0_base != (u8 *)UART_EARLY_BASE) {
>>>>>>> -		printf("WARNING: early print support may not work. "
>>>>>>> -		       "Found uart at %p, but early base is %p.\n",
>>>>>>> -			uart0_base, (u8 *)UART_EARLY_BASE);
>>>>>>> -	}
>>>>>>>  }
>>>>> This warning is doing what it should, which is pointing out that the
>>>>> UART_EARLY_BASE guess appears to be wrong. If we can provide a way
>>>>> to support more than one guess, then we should keep this warning but
>>>>> adjust it to match one of any of the guesses.
>>>>>
>>>>> Thanks,
>>>>> drew
>>>> I'm not really sure how to implement a selection method. I've looked at
>>>> splitting io_init() into uart0_init() and chr_testdev_init() and calling
>>>> uart0_init() very early in the setup process, but uart0_init() itself uses
>>>> printf() and assert().
>>>>
>>>> I've also thought about adding another function, something like
>>>> uart0_early_init(), that is called very early in setup() and gets the base
>>>> address from the dtb bootargs. But that means calling dt_init() and
>>>> dt_get_bootargs(), which can fail.
>>>>
>>>> One other option that could work is to make it a compile-time configuration.
>>>>
>>>> What do you think?
>>>>
>>> Compile-time is fine, which I guess will result in a new configure script
>>> option as well. I wonder if we shouldn't consider generating a config.h
>>> file with stuff like this rather than adding another -D to the compile
>>> line.
>>>
>>> drew 
>> I propose a new configuration option called --vmm, with possible values qemu and
>> kvmtool, which defaults to qemu if not set.
>>
>> Another possibility would be to have an --uart-base option, but that means we
>> are expecting the user to be aware of the uart base address for the virtual
>> machine manager, which might be unreasonable.
>>
>> This is a quick prototype of how using -D for conditional compilation would look
>> like (the configure changes are included too):
>>
>> diff --git a/configure b/configure
>> index df8581e3a906..7a56ba47707f 100755
>> --- a/configure
>> +++ b/configure
>> @@ -70,6 +70,9 @@ while [[ "$1" = -* ]]; do
>>             ;;
>>         --ld)
>>             ld="$arg"
>> +        ;;
>> +    --vmm)
>> +        vmm="$arg"
>>             ;;
>>         --enable-pretty-print-stacks)
>>             pretty_print_stacks=yes
>> @@ -108,6 +111,14 @@ if [ "$arch" = "i386" ] || [ "$arch" = "x86_64" ]; then
>>      testdir=x86
>>  elif [ "$arch" = "arm" ] || [ "$arch" = "arm64" ]; then
>>      testdir=arm
>> +    if [ -z "$vmm" ] || [ "$vmm" = "qemu" ]; then
>> +        uart_early_base=0x09000000UL
> You can drop the 'UL'.
>
>> +    elif [ "$vmm" = "kvmtool" ]; then
>> +        uart_early_base=0x3f8
>> +    else
>> +        echo '--vmm must be one of "qemu" or "kvmtool"'
>> +        usage
> You're outputting usage here, but you didn't add vmm to the help text.
>
>> +    fi
>>  elif [ "$arch" = "ppc64" ]; then
>>      testdir=powerpc
>>      firmware="$testdir/boot_rom.bin"
>> @@ -197,4 +208,5 @@ PRETTY_PRINT_STACKS=$pretty_print_stacks
>>  ENVIRON_DEFAULT=$environ_default
>>  ERRATATXT=errata.txt
>>  U32_LONG_FMT=$u32_long
>> +UART_EARLY_BASE=$uart_early_base
>>  EOF
>> diff --git a/Makefile b/Makefile
>> index e9f02272e156..225c2a525cdf 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -72,6 +72,10 @@ COMMON_CFLAGS += $(fno_pic) $(no_pie)
>>  CFLAGS += $(COMMON_CFLAGS)
>>  CFLAGS += -Wmissing-parameter-type -Wold-style-declaration -Woverride-init
>>  
>> +ifneq ($(UART_EARLY_BASE),)
>> +CFLAGS += -DUART_EARLY_BASE=$(UART_EARLY_BASE)
>> +endif
> This type of thing is what I would like to avoid by introducing a
> config.h file. In the least we shouldn't add this -D to CFLAGS for
> all architectures. It can be added to the %.elf rule in
> arm/Makefile.common
>
>> +
>>  CXXFLAGS += $(COMMON_CFLAGS)
>>  
>>  autodepend-flags = -MMD -MF $(dir $*).$(notdir $*).d
>>
> You'll also want to patch lib/arm/io.c with
>  
> -/*
> - * Use this guess for the pl011 base in order to make an attempt at
> - * having earlier printf support. We'll overwrite it with the real
> - * base address that we read from the device tree later. This is
> - * the address we expect QEMU's mach-virt machine type to put in
> - * its generated device tree.
> - */
> -#define UART_EARLY_BASE 0x09000000UL
> -
>  static struct spinlock uart_lock;
> -static volatile u8 *uart0_base = (u8 *)UART_EARLY_BASE;
> +static volatile u8 *uart0_base = (u8 *)(unsigned long)UART_EARLY_BASE;
>
>
>
> This is all a bit on the ugly side, but I can't think of anything
> better. 
>
> Thanks,
> drew

I've also tried doing it by generating config.h This is what I came up with:

diff --git a/configure b/configure
index df8581e3a906..d77b8b0d82fa 100755
--- a/configure
+++ b/configure
@@ -16,6 +16,7 @@ endian=""
 pretty_print_stacks=yes
 environ_default=yes
 u32_long=
+vmm=qemu
 
 usage() {
     cat <<-EOF
@@ -23,6 +24,8 @@ usage() {
 
        Options include:
            --arch=ARCH            architecture to compile for ($arch)
+           --vmm=VMM              virtual machine monitor to compile for (qemu
or kvmtool,
+                                  arm/arm64 only, default is qemu)
            --processor=PROCESSOR  processor to compile for ($arch)
            --cross-prefix=PREFIX  cross compiler prefix
            --cc=CC                c compiler to use ($cc)
@@ -71,6 +74,9 @@ while [[ "$1" = -* ]]; do
        --ld)
            ld="$arg"
            ;;
+       --vmm)
+           vmm="$arg"
+           ;;
        --enable-pretty-print-stacks)
            pretty_print_stacks=yes
            ;;
@@ -108,6 +114,14 @@ if [ "$arch" = "i386" ] || [ "$arch" = "x86_64" ]; then
     testdir=x86
 elif [ "$arch" = "arm" ] || [ "$arch" = "arm64" ]; then
     testdir=arm
+    if [ "$vmm" = "qemu" ]; then
+        uart_early_base=0x09000000
+    elif [ "$vmm" = "kvmtool" ]; then
+        uart_early_base=0x3f8
+    else
+        echo '--vmm must be one of "qemu" or "kvmtool"'
+        usage
+    fi
 elif [ "$arch" = "ppc64" ]; then
     testdir=powerpc
     firmware="$testdir/boot_rom.bin"
@@ -198,3 +212,16 @@ ENVIRON_DEFAULT=$environ_default
 ERRATATXT=errata.txt
 U32_LONG_FMT=$u32_long
 EOF
+
+cat <<EOF > lib/config.h
+#ifndef CONFIG_H
+#define CONFIG_H 1
+EOF
+if [ "$arch" = "arm" ] || [ "$arch" = "arm64" ]; then
+cat <<EOF >> lib/config.h
+#define UART_EARLY_BASE ${uart_early_base}UL
+EOF
+fi
+cat <<EOF >> lib/config.h
+#endif
+EOF
diff --git a/lib/arm/io.c b/lib/arm/io.c
index 9fe9bd0bf659..0a3e8f237ab8 100644
--- a/lib/arm/io.c
+++ b/lib/arm/io.c
@@ -11,6 +11,7 @@
 #include <libcflat.h>
 #include <devicetree.h>
 #include <chr-testdev.h>
+#include <config.h>
 #include "arm/asm/psci.h"
 #include <asm/spinlock.h>
 #include <asm/io.h>
@@ -21,15 +22,6 @@ extern void halt(int code);
 
 static bool testdev_enabled;
 
-/*
- * Use this guess for the pl011 base in order to make an attempt at
- * having earlier printf support. We'll overwrite it with the real
- * base address that we read from the device tree later. This is
- * the address we expect QEMU's mach-virt machine type to put in
- * its generated device tree.
- */
-#define UART_EARLY_BASE 0x09000000UL
-
 static struct spinlock uart_lock;
 static volatile u8 *uart0_base = (u8 *)UART_EARLY_BASE;

Putting config.h in lib makes it available for other architectures, in case they
want to implement something similar. Please suggest another location if there is
a better one.

I think this looks better than using architecture-specific compile-time defines
buried in arm/Makefile.common, what do you think?

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH 2/7] lib: arm: Remove warning about uart0_base mismatch

[Andrew Jones]

On Tue, Jan 29, 2019 at 11:16:05AM +0000, Alexandru Elisei wrote:
> On 1/28/19 4:31 PM, Andrew Jones wrote:
> > On Mon, Jan 28, 2019 at 02:24:29PM +0000, Alexandru Elisei wrote:
> >> On 1/25/19 4:47 PM, Andrew Jones wrote:
> >>> On Fri, Jan 25, 2019 at 04:36:13PM +0000, Alexandru Elisei wrote:
> >>>> On 1/24/19 12:37 PM, Andrew Jones wrote:
> >>>>> On Thu, Jan 24, 2019 at 11:59:43AM +0000, Andre Przywara wrote:
> >>>>>> On Thu, 24 Jan 2019 11:16:29 +0000
> >>>>>> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> >>>>>>
> >>>>>>> A warning is displayed if uart0_base is different from what the code
> >>>>>>> expects qemu to generate for the pl011 UART in the device tree.
> >>>>>>> However, now we support the ns16550a UART emulated by kvmtool, which
> >>>>>>> has a different address. This leads to the  warning being displayed
> >>>>>>> even though the UART is configured and working as expected.
> >>>>>>>
> >>>>>>> Now that we support multiple UARTs, the warning serves no purpose, so
> >>>>>>> remove it.
> >>>>>> Mmh, but we use that address before, right? So for anything not
> >>>>>> emulating an UART at this QEMU specific address we write to some random
> >>>>>> (device) memory?
> >>>>>>
> >>>>>> Drew, how important is this early print feature for kvm-unit-tests?
> >>>>> The setup code passes through quite a few asserts before getting through
> >>>>> io_init() (including in uart0_init), so I think there's still value in
> >>>>> having a guessed UART address. Maybe we can provide guesses for both
> >>>>> QEMU and kvmtool, and some selection method, that would be used until
> >>>>> we've properly assigned uart0_base from DT?
> >>>>>
> >>>>>> Cheers,
> >>>>>> Andre.
> >>>>>>
> >>>>>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> >>>>>>> ---
> >>>>>>>  lib/arm/io.c | 6 ------
> >>>>>>>  1 file changed, 6 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/lib/arm/io.c b/lib/arm/io.c
> >>>>>>> index 35fc05aeb4db..87435150f73e 100644
> >>>>>>> --- a/lib/arm/io.c
> >>>>>>> +++ b/lib/arm/io.c
> >>>>>>> @@ -61,12 +61,6 @@ static void uart0_init(void)
> >>>>>>>  	}
> >>>>>>>  
> >>>>>>>  	uart0_base = ioremap(base.addr, base.size);
> >>>>>>> -
> >>>>>>> -	if (uart0_base != (u8 *)UART_EARLY_BASE) {
> >>>>>>> -		printf("WARNING: early print support may not work. "
> >>>>>>> -		       "Found uart at %p, but early base is %p.\n",
> >>>>>>> -			uart0_base, (u8 *)UART_EARLY_BASE);
> >>>>>>> -	}
> >>>>>>>  }
> >>>>> This warning is doing what it should, which is pointing out that the
> >>>>> UART_EARLY_BASE guess appears to be wrong. If we can provide a way
> >>>>> to support more than one guess, then we should keep this warning but
> >>>>> adjust it to match one of any of the guesses.
> >>>>>
> >>>>> Thanks,
> >>>>> drew
> >>>> I'm not really sure how to implement a selection method. I've looked at
> >>>> splitting io_init() into uart0_init() and chr_testdev_init() and calling
> >>>> uart0_init() very early in the setup process, but uart0_init() itself uses
> >>>> printf() and assert().
> >>>>
> >>>> I've also thought about adding another function, something like
> >>>> uart0_early_init(), that is called very early in setup() and gets the base
> >>>> address from the dtb bootargs. But that means calling dt_init() and
> >>>> dt_get_bootargs(), which can fail.
> >>>>
> >>>> One other option that could work is to make it a compile-time configuration.
> >>>>
> >>>> What do you think?
> >>>>
> >>> Compile-time is fine, which I guess will result in a new configure script
> >>> option as well. I wonder if we shouldn't consider generating a config.h
> >>> file with stuff like this rather than adding another -D to the compile
> >>> line.
> >>>
> >>> drew 
> >> I propose a new configuration option called --vmm, with possible values qemu and
> >> kvmtool, which defaults to qemu if not set.
> >>
> >> Another possibility would be to have an --uart-base option, but that means we
> >> are expecting the user to be aware of the uart base address for the virtual
> >> machine manager, which might be unreasonable.
> >>
> >> This is a quick prototype of how using -D for conditional compilation would look
> >> like (the configure changes are included too):
> >>
> >> diff --git a/configure b/configure
> >> index df8581e3a906..7a56ba47707f 100755
> >> --- a/configure
> >> +++ b/configure
> >> @@ -70,6 +70,9 @@ while [[ "$1" = -* ]]; do
> >>             ;;
> >>         --ld)
> >>             ld="$arg"
> >> +        ;;
> >> +    --vmm)
> >> +        vmm="$arg"
> >>             ;;
> >>         --enable-pretty-print-stacks)
> >>             pretty_print_stacks=yes
> >> @@ -108,6 +111,14 @@ if [ "$arch" = "i386" ] || [ "$arch" = "x86_64" ]; then
> >>      testdir=x86
> >>  elif [ "$arch" = "arm" ] || [ "$arch" = "arm64" ]; then
> >>      testdir=arm
> >> +    if [ -z "$vmm" ] || [ "$vmm" = "qemu" ]; then
> >> +        uart_early_base=0x09000000UL
> > You can drop the 'UL'.
> >
> >> +    elif [ "$vmm" = "kvmtool" ]; then
> >> +        uart_early_base=0x3f8
> >> +    else
> >> +        echo '--vmm must be one of "qemu" or "kvmtool"'
> >> +        usage
> > You're outputting usage here, but you didn't add vmm to the help text.
> >
> >> +    fi
> >>  elif [ "$arch" = "ppc64" ]; then
> >>      testdir=powerpc
> >>      firmware="$testdir/boot_rom.bin"
> >> @@ -197,4 +208,5 @@ PRETTY_PRINT_STACKS=$pretty_print_stacks
> >>  ENVIRON_DEFAULT=$environ_default
> >>  ERRATATXT=errata.txt
> >>  U32_LONG_FMT=$u32_long
> >> +UART_EARLY_BASE=$uart_early_base
> >>  EOF
> >> diff --git a/Makefile b/Makefile
> >> index e9f02272e156..225c2a525cdf 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -72,6 +72,10 @@ COMMON_CFLAGS += $(fno_pic) $(no_pie)
> >>  CFLAGS += $(COMMON_CFLAGS)
> >>  CFLAGS += -Wmissing-parameter-type -Wold-style-declaration -Woverride-init
> >>  
> >> +ifneq ($(UART_EARLY_BASE),)
> >> +CFLAGS += -DUART_EARLY_BASE=$(UART_EARLY_BASE)
> >> +endif
> > This type of thing is what I would like to avoid by introducing a
> > config.h file. In the least we shouldn't add this -D to CFLAGS for
> > all architectures. It can be added to the %.elf rule in
> > arm/Makefile.common
> >
> >> +
> >>  CXXFLAGS += $(COMMON_CFLAGS)
> >>  
> >>  autodepend-flags = -MMD -MF $(dir $*).$(notdir $*).d
> >>
> > You'll also want to patch lib/arm/io.c with
> >  
> > -/*
> > - * Use this guess for the pl011 base in order to make an attempt at
> > - * having earlier printf support. We'll overwrite it with the real
> > - * base address that we read from the device tree later. This is
> > - * the address we expect QEMU's mach-virt machine type to put in
> > - * its generated device tree.
> > - */
> > -#define UART_EARLY_BASE 0x09000000UL
> > -
> >  static struct spinlock uart_lock;
> > -static volatile u8 *uart0_base = (u8 *)UART_EARLY_BASE;
> > +static volatile u8 *uart0_base = (u8 *)(unsigned long)UART_EARLY_BASE;
> >
> >
> >
> > This is all a bit on the ugly side, but I can't think of anything
> > better. 
> >
> > Thanks,
> > drew
> 
> I've also tried doing it by generating config.h This is what I came up with:
> 
> diff --git a/configure b/configure
> index df8581e3a906..d77b8b0d82fa 100755
> --- a/configure
> +++ b/configure
> @@ -16,6 +16,7 @@ endian=""
>  pretty_print_stacks=yes
>  environ_default=yes
>  u32_long=
> +vmm=qemu
>  
>  usage() {
>      cat <<-EOF
> @@ -23,6 +24,8 @@ usage() {
>  
>         Options include:
>             --arch=ARCH            architecture to compile for ($arch)
> +           --vmm=VMM              virtual machine monitor to compile for (qemu
> or kvmtool,

Long line?

> +                                  arm/arm64 only, default is qemu)

Why arm/arm64 only? No plans to use kvmtool for x86 tests?

>             --processor=PROCESSOR  processor to compile for ($arch)
>             --cross-prefix=PREFIX  cross compiler prefix
>             --cc=CC                c compiler to use ($cc)
> @@ -71,6 +74,9 @@ while [[ "$1" = -* ]]; do
>         --ld)
>             ld="$arg"
>             ;;
> +       --vmm)
> +           vmm="$arg"
> +           ;;
>         --enable-pretty-print-stacks)
>             pretty_print_stacks=yes
>             ;;
> @@ -108,6 +114,14 @@ if [ "$arch" = "i386" ] || [ "$arch" = "x86_64" ]; then
>      testdir=x86
>  elif [ "$arch" = "arm" ] || [ "$arch" = "arm64" ]; then
>      testdir=arm
> +    if [ "$vmm" = "qemu" ]; then
> +        uart_early_base=0x09000000
> +    elif [ "$vmm" = "kvmtool" ]; then
> +        uart_early_base=0x3f8
> +    else
> +        echo '--vmm must be one of "qemu" or "kvmtool"'
> +        usage

Checking the vmm is qemu or kvmtool can be moved out of the if-arm block
if we decide it can be for other architectures. If uart_early_base is only
and arm thing, then 'arm' should probably be in its name. Could also
rename 'base' to 'addr' to accommodate ioports.


> +    fi
>  elif [ "$arch" = "ppc64" ]; then
>      testdir=powerpc
>      firmware="$testdir/boot_rom.bin"
> @@ -198,3 +212,16 @@ ENVIRON_DEFAULT=$environ_default
>  ERRATATXT=errata.txt
>  U32_LONG_FMT=$u32_long
>  EOF
> +
> +cat <<EOF > lib/config.h
> +#ifndef CONFIG_H
> +#define CONFIG_H 1

Should add a header stating this is a generated file like
make_asm_offsets has in scripts/asm-offsets.mak

> +EOF
> +if [ "$arch" = "arm" ] || [ "$arch" = "arm64" ]; then
> +cat <<EOF >> lib/config.h
> +#define UART_EARLY_BASE ${uart_early_base}UL

Drop the 'UL'. If somebody attempted to supply it themselves it'd end up
being ULUL. Just add another cast to where the value gets used to handle
the type.

> +EOF
> +fi
> +cat <<EOF >> lib/config.h
> +#endif
> +EOF

The one line appends could use 'echo' to take 2 less lines each.

> diff --git a/lib/arm/io.c b/lib/arm/io.c
> index 9fe9bd0bf659..0a3e8f237ab8 100644
> --- a/lib/arm/io.c
> +++ b/lib/arm/io.c
> @@ -11,6 +11,7 @@
>  #include <libcflat.h>
>  #include <devicetree.h>
>  #include <chr-testdev.h>
> +#include <config.h>
>  #include "arm/asm/psci.h"
>  #include <asm/spinlock.h>
>  #include <asm/io.h>
> @@ -21,15 +22,6 @@ extern void halt(int code);
>  
>  static bool testdev_enabled;
>  
> -/*
> - * Use this guess for the pl011 base in order to make an attempt at
> - * having earlier printf support. We'll overwrite it with the real
> - * base address that we read from the device tree later. This is
> - * the address we expect QEMU's mach-virt machine type to put in
> - * its generated device tree.
> - */
> -#define UART_EARLY_BASE 0x09000000UL
> -
>  static struct spinlock uart_lock;
>  static volatile u8 *uart0_base = (u8 *)UART_EARLY_BASE;
> 
> Putting config.h in lib makes it available for other architectures, in case they
> want to implement something similar. Please suggest another location if there is
> a better one.
> 
> I think this looks better than using architecture-specific compile-time defines
> buried in arm/Makefile.common, what do you think?
>

Yes, I agree this looks better. You'll probably want to add a
lib/.gitignore for the file too and also remove it when
'make distclean' is run.

Thanks,
drew

Re: [kvm-unit-tests PATCH 2/7] lib: arm: Remove warning about uart0_base mismatch

[Alexandru Elisei]

On 1/29/19 12:23 PM, Andrew Jones wrote:
> On Tue, Jan 29, 2019 at 11:16:05AM +0000, Alexandru Elisei wrote:
>> On 1/28/19 4:31 PM, Andrew Jones wrote:
>>> On Mon, Jan 28, 2019 at 02:24:29PM +0000, Alexandru Elisei wrote:
>>>> On 1/25/19 4:47 PM, Andrew Jones wrote:
>>>>> On Fri, Jan 25, 2019 at 04:36:13PM +0000, Alexandru Elisei wrote:
>>>>>> On 1/24/19 12:37 PM, Andrew Jones wrote:
>>>>>>> On Thu, Jan 24, 2019 at 11:59:43AM +0000, Andre Przywara wrote:
>>>>>>>> On Thu, 24 Jan 2019 11:16:29 +0000
>>>>>>>> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>>>>>>>>
>>>>>>>>> A warning is displayed if uart0_base is different from what the code
>>>>>>>>> expects qemu to generate for the pl011 UART in the device tree.
>>>>>>>>> However, now we support the ns16550a UART emulated by kvmtool, which
>>>>>>>>> has a different address. This leads to the  warning being displayed
>>>>>>>>> even though the UART is configured and working as expected.
>>>>>>>>>
>>>>>>>>> Now that we support multiple UARTs, the warning serves no purpose, so
>>>>>>>>> remove it.
>>>>>>>> Mmh, but we use that address before, right? So for anything not
>>>>>>>> emulating an UART at this QEMU specific address we write to some random
>>>>>>>> (device) memory?
>>>>>>>>
>>>>>>>> Drew, how important is this early print feature for kvm-unit-tests?
>>>>>>> The setup code passes through quite a few asserts before getting through
>>>>>>> io_init() (including in uart0_init), so I think there's still value in
>>>>>>> having a guessed UART address. Maybe we can provide guesses for both
>>>>>>> QEMU and kvmtool, and some selection method, that would be used until
>>>>>>> we've properly assigned uart0_base from DT?
>>>>>>>
>>>>>>>> Cheers,
>>>>>>>> Andre.
>>>>>>>>
>>>>>>>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>>>>>>>>> ---
>>>>>>>>>  lib/arm/io.c | 6 ------
>>>>>>>>>  1 file changed, 6 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/lib/arm/io.c b/lib/arm/io.c
>>>>>>>>> index 35fc05aeb4db..87435150f73e 100644
>>>>>>>>> --- a/lib/arm/io.c
>>>>>>>>> +++ b/lib/arm/io.c
>>>>>>>>> @@ -61,12 +61,6 @@ static void uart0_init(void)
>>>>>>>>>  	}
>>>>>>>>>  
>>>>>>>>>  	uart0_base = ioremap(base.addr, base.size);
>>>>>>>>> -
>>>>>>>>> -	if (uart0_base != (u8 *)UART_EARLY_BASE) {
>>>>>>>>> -		printf("WARNING: early print support may not work. "
>>>>>>>>> -		       "Found uart at %p, but early base is %p.\n",
>>>>>>>>> -			uart0_base, (u8 *)UART_EARLY_BASE);
>>>>>>>>> -	}
>>>>>>>>>  }
>>>>>>> This warning is doing what it should, which is pointing out that the
>>>>>>> UART_EARLY_BASE guess appears to be wrong. If we can provide a way
>>>>>>> to support more than one guess, then we should keep this warning but
>>>>>>> adjust it to match one of any of the guesses.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> drew
>>>>>> I'm not really sure how to implement a selection method. I've looked at
>>>>>> splitting io_init() into uart0_init() and chr_testdev_init() and calling
>>>>>> uart0_init() very early in the setup process, but uart0_init() itself uses
>>>>>> printf() and assert().
>>>>>>
>>>>>> I've also thought about adding another function, something like
>>>>>> uart0_early_init(), that is called very early in setup() and gets the base
>>>>>> address from the dtb bootargs. But that means calling dt_init() and
>>>>>> dt_get_bootargs(), which can fail.
>>>>>>
>>>>>> One other option that could work is to make it a compile-time configuration.
>>>>>>
>>>>>> What do you think?
>>>>>>
>>>>> Compile-time is fine, which I guess will result in a new configure script
>>>>> option as well. I wonder if we shouldn't consider generating a config.h
>>>>> file with stuff like this rather than adding another -D to the compile
>>>>> line.
>>>>>
>>>>> drew 
>>>> I propose a new configuration option called --vmm, with possible values qemu and
>>>> kvmtool, which defaults to qemu if not set.
>>>>
>>>> Another possibility would be to have an --uart-base option, but that means we
>>>> are expecting the user to be aware of the uart base address for the virtual
>>>> machine manager, which might be unreasonable.
>>>>
>>>> This is a quick prototype of how using -D for conditional compilation would look
>>>> like (the configure changes are included too):
>>>>
>>>> diff --git a/configure b/configure
>>>> index df8581e3a906..7a56ba47707f 100755
>>>> --- a/configure
>>>> +++ b/configure
>>>> @@ -70,6 +70,9 @@ while [[ "$1" = -* ]]; do
>>>>             ;;
>>>>         --ld)
>>>>             ld="$arg"
>>>> +        ;;
>>>> +    --vmm)
>>>> +        vmm="$arg"
>>>>             ;;
>>>>         --enable-pretty-print-stacks)
>>>>             pretty_print_stacks=yes
>>>> @@ -108,6 +111,14 @@ if [ "$arch" = "i386" ] || [ "$arch" = "x86_64" ]; then
>>>>      testdir=x86
>>>>  elif [ "$arch" = "arm" ] || [ "$arch" = "arm64" ]; then
>>>>      testdir=arm
>>>> +    if [ -z "$vmm" ] || [ "$vmm" = "qemu" ]; then
>>>> +        uart_early_base=0x09000000UL
>>> You can drop the 'UL'.
>>>
>>>> +    elif [ "$vmm" = "kvmtool" ]; then
>>>> +        uart_early_base=0x3f8
>>>> +    else
>>>> +        echo '--vmm must be one of "qemu" or "kvmtool"'
>>>> +        usage
>>> You're outputting usage here, but you didn't add vmm to the help text.
>>>
>>>> +    fi
>>>>  elif [ "$arch" = "ppc64" ]; then
>>>>      testdir=powerpc
>>>>      firmware="$testdir/boot_rom.bin"
>>>> @@ -197,4 +208,5 @@ PRETTY_PRINT_STACKS=$pretty_print_stacks
>>>>  ENVIRON_DEFAULT=$environ_default
>>>>  ERRATATXT=errata.txt
>>>>  U32_LONG_FMT=$u32_long
>>>> +UART_EARLY_BASE=$uart_early_base
>>>>  EOF
>>>> diff --git a/Makefile b/Makefile
>>>> index e9f02272e156..225c2a525cdf 100644
>>>> --- a/Makefile
>>>> +++ b/Makefile
>>>> @@ -72,6 +72,10 @@ COMMON_CFLAGS += $(fno_pic) $(no_pie)
>>>>  CFLAGS += $(COMMON_CFLAGS)
>>>>  CFLAGS += -Wmissing-parameter-type -Wold-style-declaration -Woverride-init
>>>>  
>>>> +ifneq ($(UART_EARLY_BASE),)
>>>> +CFLAGS += -DUART_EARLY_BASE=$(UART_EARLY_BASE)
>>>> +endif
>>> This type of thing is what I would like to avoid by introducing a
>>> config.h file. In the least we shouldn't add this -D to CFLAGS for
>>> all architectures. It can be added to the %.elf rule in
>>> arm/Makefile.common
>>>
>>>> +
>>>>  CXXFLAGS += $(COMMON_CFLAGS)
>>>>  
>>>>  autodepend-flags = -MMD -MF $(dir $*).$(notdir $*).d
>>>>
>>> You'll also want to patch lib/arm/io.c with
>>>  
>>> -/*
>>> - * Use this guess for the pl011 base in order to make an attempt at
>>> - * having earlier printf support. We'll overwrite it with the real
>>> - * base address that we read from the device tree later. This is
>>> - * the address we expect QEMU's mach-virt machine type to put in
>>> - * its generated device tree.
>>> - */
>>> -#define UART_EARLY_BASE 0x09000000UL
>>> -
>>>  static struct spinlock uart_lock;
>>> -static volatile u8 *uart0_base = (u8 *)UART_EARLY_BASE;
>>> +static volatile u8 *uart0_base = (u8 *)(unsigned long)UART_EARLY_BASE;
>>>
>>>
>>>
>>> This is all a bit on the ugly side, but I can't think of anything
>>> better. 
>>>
>>> Thanks,
>>> drew
>> I've also tried doing it by generating config.h This is what I came up with:
>>
>> diff --git a/configure b/configure
>> index df8581e3a906..d77b8b0d82fa 100755
>> --- a/configure
>> +++ b/configure
>> @@ -16,6 +16,7 @@ endian=""
>>  pretty_print_stacks=yes
>>  environ_default=yes
>>  u32_long=
>> +vmm=qemu
>>  
>>  usage() {
>>      cat <<-EOF
>> @@ -23,6 +24,8 @@ usage() {
>>  
>>         Options include:
>>             --arch=ARCH            architecture to compile for ($arch)
>> +           --vmm=VMM              virtual machine monitor to compile for (qemu
>> or kvmtool,
> Long line?
I will fix it.
>
>> +                                  arm/arm64 only, default is qemu)
> Why arm/arm64 only? No plans to use kvmtool for x86 tests?
The patches target only the arm and arm64 architectures.
>
>>             --processor=PROCESSOR  processor to compile for ($arch)
>>             --cross-prefix=PREFIX  cross compiler prefix
>>             --cc=CC                c compiler to use ($cc)
>> @@ -71,6 +74,9 @@ while [[ "$1" = -* ]]; do
>>         --ld)
>>             ld="$arg"
>>             ;;
>> +       --vmm)
>> +           vmm="$arg"
>> +           ;;
>>         --enable-pretty-print-stacks)
>>             pretty_print_stacks=yes
>>             ;;
>> @@ -108,6 +114,14 @@ if [ "$arch" = "i386" ] || [ "$arch" = "x86_64" ]; then
>>      testdir=x86
>>  elif [ "$arch" = "arm" ] || [ "$arch" = "arm64" ]; then
>>      testdir=arm
>> +    if [ "$vmm" = "qemu" ]; then
>> +        uart_early_base=0x09000000
>> +    elif [ "$vmm" = "kvmtool" ]; then
>> +        uart_early_base=0x3f8
>> +    else
>> +        echo '--vmm must be one of "qemu" or "kvmtool"'
>> +        usage
> Checking the vmm is qemu or kvmtool can be moved out of the if-arm block
> if we decide it can be for other architectures. If uart_early_base is only
> and arm thing, then 'arm' should probably be in its name. Could also
> rename 'base' to 'addr' to accommodate ioports.
It's arm only for now, I will rename the variable.
>
>
>> +    fi
>>  elif [ "$arch" = "ppc64" ]; then
>>      testdir=powerpc
>>      firmware="$testdir/boot_rom.bin"
>> @@ -198,3 +212,16 @@ ENVIRON_DEFAULT=$environ_default
>>  ERRATATXT=errata.txt
>>  U32_LONG_FMT=$u32_long
>>  EOF
>> +
>> +cat <<EOF > lib/config.h
>> +#ifndef CONFIG_H
>> +#define CONFIG_H 1
> Should add a header stating this is a generated file like
> make_asm_offsets has in scripts/asm-offsets.mak
I will do that.
>
>> +EOF
>> +if [ "$arch" = "arm" ] || [ "$arch" = "arm64" ]; then
>> +cat <<EOF >> lib/config.h
>> +#define UART_EARLY_BASE ${uart_early_base}UL
> Drop the 'UL'. If somebody attempted to supply it themselves it'd end up
> being ULUL. Just add another cast to where the value gets used to handle
> the type.
Good point.
>
>> +EOF
>> +fi
>> +cat <<EOF >> lib/config.h
>> +#endif
>> +EOF
> The one line appends could use 'echo' to take 2 less lines each.
I will change it to an echo.
>
>> diff --git a/lib/arm/io.c b/lib/arm/io.c
>> index 9fe9bd0bf659..0a3e8f237ab8 100644
>> --- a/lib/arm/io.c
>> +++ b/lib/arm/io.c
>> @@ -11,6 +11,7 @@
>>  #include <libcflat.h>
>>  #include <devicetree.h>
>>  #include <chr-testdev.h>
>> +#include <config.h>
>>  #include "arm/asm/psci.h"
>>  #include <asm/spinlock.h>
>>  #include <asm/io.h>
>> @@ -21,15 +22,6 @@ extern void halt(int code);
>>  
>>  static bool testdev_enabled;
>>  
>> -/*
>> - * Use this guess for the pl011 base in order to make an attempt at
>> - * having earlier printf support. We'll overwrite it with the real
>> - * base address that we read from the device tree later. This is
>> - * the address we expect QEMU's mach-virt machine type to put in
>> - * its generated device tree.
>> - */
>> -#define UART_EARLY_BASE 0x09000000UL
>> -
>>  static struct spinlock uart_lock;
>>  static volatile u8 *uart0_base = (u8 *)UART_EARLY_BASE;
>>
>> Putting config.h in lib makes it available for other architectures, in case they
>> want to implement something similar. Please suggest another location if there is
>> a better one.
>>
>> I think this looks better than using architecture-specific compile-time defines
>> buried in arm/Makefile.common, what do you think?
>>
> Yes, I agree this looks better. You'll probably want to add a
> lib/.gitignore for the file too and also remove it when
> 'make distclean' is run.
>
> Thanks,
> drew
Thank you for the feedback, I will incorporate these changes into the next
version of the patches.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm