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

[Andre Przywara <andre.przywara@arm.com>]

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