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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux