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