On Mon, 28 Jan 2019 17:31:01 +0100
Andrew Jones <drjones@xxxxxxxxxx> 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.
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.
