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



[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