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