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