On Mon, Jan 28, 2019 at 05:58:54PM +0000, Andre Przywara wrote:
> 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.
I thought about this one, but wasn't sure I wanted to embed a heuristic in
early boot code. It does seem that QEMU mach-virt will *always* have the
base of RAM at 1G, so if kvmtool will *never* have the base of ram at 1G,
then it would work. But what if a third vmm (something like firecracker)
comes along that matches one or the other?
> That's surely somewhat hacky, but
> should be more robust than a compile time version.
Not when considering more than two vmms could be involved, or that a vmm
could change its base of ram. Also, an explicit uart-base=<addr> should
be the easiest thing to fix if addr turns out to be guessed wrong.
> 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 ...
We need this address because we want to be able to probe for peripherals
with working asserts. So if we choose to use a heuristic it needs to be
something we can do with a few instructions right at boot. Base of RAM
was the only thing that came to my mind besides passing in what we need
through auxinfo.
Being able to control implementation defined sysregs for things like this
would be nice.
Thanks,
drew
