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

login
register
mail settings
Submitter Alexandru Elisei
Date Jan. 29, 2019, 11:16 a.m.
Message ID <39bc7b61-ddac-3180-2cb8-410e1d400cb3@arm.com>
Download mbox | patch
Permalink /patch/712273/
State New
Headers show

Comments

Alexandru Elisei - Jan. 29, 2019, 11:16 a.m.
On 1/28/19 4:31 PM, Andrew Jones 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. 
>
> Thanks,
> drew

I've also tried doing it by generating config.h This is what I came up with:

Putting config.h in lib makes it available for other architectures, in case they
want to implement something similar. Please suggest another location if there is
a better one.

I think this looks better than using architecture-specific compile-time defines
buried in arm/Makefile.common, what do you think?
Andrew Jones - Jan. 29, 2019, 12:23 p.m.
On Tue, Jan 29, 2019 at 11:16:05AM +0000, Alexandru Elisei wrote:
> On 1/28/19 4:31 PM, Andrew Jones 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. 
> >
> > Thanks,
> > drew
> 
> I've also tried doing it by generating config.h This is what I came up with:
> 
> diff --git a/configure b/configure
> index df8581e3a906..d77b8b0d82fa 100755
> --- a/configure
> +++ b/configure
> @@ -16,6 +16,7 @@ endian=""
>  pretty_print_stacks=yes
>  environ_default=yes
>  u32_long=
> +vmm=qemu
>  
>  usage() {
>      cat <<-EOF
> @@ -23,6 +24,8 @@ usage() {
>  
>         Options include:
>             --arch=ARCH            architecture to compile for ($arch)
> +           --vmm=VMM              virtual machine monitor to compile for (qemu
> or kvmtool,

Long line?

> +                                  arm/arm64 only, default is qemu)

Why arm/arm64 only? No plans to use kvmtool for x86 tests?

>             --processor=PROCESSOR  processor to compile for ($arch)
>             --cross-prefix=PREFIX  cross compiler prefix
>             --cc=CC                c compiler to use ($cc)
> @@ -71,6 +74,9 @@ while [[ "$1" = -* ]]; do
>         --ld)
>             ld="$arg"
>             ;;
> +       --vmm)
> +           vmm="$arg"
> +           ;;
>         --enable-pretty-print-stacks)
>             pretty_print_stacks=yes
>             ;;
> @@ -108,6 +114,14 @@ if [ "$arch" = "i386" ] || [ "$arch" = "x86_64" ]; then
>      testdir=x86
>  elif [ "$arch" = "arm" ] || [ "$arch" = "arm64" ]; then
>      testdir=arm
> +    if [ "$vmm" = "qemu" ]; then
> +        uart_early_base=0x09000000
> +    elif [ "$vmm" = "kvmtool" ]; then
> +        uart_early_base=0x3f8
> +    else
> +        echo '--vmm must be one of "qemu" or "kvmtool"'
> +        usage

Checking the vmm is qemu or kvmtool can be moved out of the if-arm block
if we decide it can be for other architectures. If uart_early_base is only
and arm thing, then 'arm' should probably be in its name. Could also
rename 'base' to 'addr' to accommodate ioports.


> +    fi
>  elif [ "$arch" = "ppc64" ]; then
>      testdir=powerpc
>      firmware="$testdir/boot_rom.bin"
> @@ -198,3 +212,16 @@ ENVIRON_DEFAULT=$environ_default
>  ERRATATXT=errata.txt
>  U32_LONG_FMT=$u32_long
>  EOF
> +
> +cat <<EOF > lib/config.h
> +#ifndef CONFIG_H
> +#define CONFIG_H 1

Should add a header stating this is a generated file like
make_asm_offsets has in scripts/asm-offsets.mak

> +EOF
> +if [ "$arch" = "arm" ] || [ "$arch" = "arm64" ]; then
> +cat <<EOF >> lib/config.h
> +#define UART_EARLY_BASE ${uart_early_base}UL

Drop the 'UL'. If somebody attempted to supply it themselves it'd end up
being ULUL. Just add another cast to where the value gets used to handle
the type.

> +EOF
> +fi
> +cat <<EOF >> lib/config.h
> +#endif
> +EOF

The one line appends could use 'echo' to take 2 less lines each.

> diff --git a/lib/arm/io.c b/lib/arm/io.c
> index 9fe9bd0bf659..0a3e8f237ab8 100644
> --- a/lib/arm/io.c
> +++ b/lib/arm/io.c
> @@ -11,6 +11,7 @@
>  #include <libcflat.h>
>  #include <devicetree.h>
>  #include <chr-testdev.h>
> +#include <config.h>
>  #include "arm/asm/psci.h"
>  #include <asm/spinlock.h>
>  #include <asm/io.h>
> @@ -21,15 +22,6 @@ extern void halt(int code);
>  
>  static bool testdev_enabled;
>  
> -/*
> - * 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;
> 
> Putting config.h in lib makes it available for other architectures, in case they
> want to implement something similar. Please suggest another location if there is
> a better one.
> 
> I think this looks better than using architecture-specific compile-time defines
> buried in arm/Makefile.common, what do you think?
>

Yes, I agree this looks better. You'll probably want to add a
lib/.gitignore for the file too and also remove it when
'make distclean' is run.

Thanks,
drew
Alexandru Elisei - Jan. 29, 2019, 1:40 p.m.
On 1/29/19 12:23 PM, Andrew Jones wrote:
> On Tue, Jan 29, 2019 at 11:16:05AM +0000, Alexandru Elisei wrote:
>> On 1/28/19 4:31 PM, Andrew Jones 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. 
>>>
>>> Thanks,
>>> drew
>> I've also tried doing it by generating config.h This is what I came up with:
>>
>> diff --git a/configure b/configure
>> index df8581e3a906..d77b8b0d82fa 100755
>> --- a/configure
>> +++ b/configure
>> @@ -16,6 +16,7 @@ endian=""
>>  pretty_print_stacks=yes
>>  environ_default=yes
>>  u32_long=
>> +vmm=qemu
>>  
>>  usage() {
>>      cat <<-EOF
>> @@ -23,6 +24,8 @@ usage() {
>>  
>>         Options include:
>>             --arch=ARCH            architecture to compile for ($arch)
>> +           --vmm=VMM              virtual machine monitor to compile for (qemu
>> or kvmtool,
> Long line?
I will fix it.
>
>> +                                  arm/arm64 only, default is qemu)
> Why arm/arm64 only? No plans to use kvmtool for x86 tests?
The patches target only the arm and arm64 architectures.
>
>>             --processor=PROCESSOR  processor to compile for ($arch)
>>             --cross-prefix=PREFIX  cross compiler prefix
>>             --cc=CC                c compiler to use ($cc)
>> @@ -71,6 +74,9 @@ while [[ "$1" = -* ]]; do
>>         --ld)
>>             ld="$arg"
>>             ;;
>> +       --vmm)
>> +           vmm="$arg"
>> +           ;;
>>         --enable-pretty-print-stacks)
>>             pretty_print_stacks=yes
>>             ;;
>> @@ -108,6 +114,14 @@ if [ "$arch" = "i386" ] || [ "$arch" = "x86_64" ]; then
>>      testdir=x86
>>  elif [ "$arch" = "arm" ] || [ "$arch" = "arm64" ]; then
>>      testdir=arm
>> +    if [ "$vmm" = "qemu" ]; then
>> +        uart_early_base=0x09000000
>> +    elif [ "$vmm" = "kvmtool" ]; then
>> +        uart_early_base=0x3f8
>> +    else
>> +        echo '--vmm must be one of "qemu" or "kvmtool"'
>> +        usage
> Checking the vmm is qemu or kvmtool can be moved out of the if-arm block
> if we decide it can be for other architectures. If uart_early_base is only
> and arm thing, then 'arm' should probably be in its name. Could also
> rename 'base' to 'addr' to accommodate ioports.
It's arm only for now, I will rename the variable.
>
>
>> +    fi
>>  elif [ "$arch" = "ppc64" ]; then
>>      testdir=powerpc
>>      firmware="$testdir/boot_rom.bin"
>> @@ -198,3 +212,16 @@ ENVIRON_DEFAULT=$environ_default
>>  ERRATATXT=errata.txt
>>  U32_LONG_FMT=$u32_long
>>  EOF
>> +
>> +cat <<EOF > lib/config.h
>> +#ifndef CONFIG_H
>> +#define CONFIG_H 1
> Should add a header stating this is a generated file like
> make_asm_offsets has in scripts/asm-offsets.mak
I will do that.
>
>> +EOF
>> +if [ "$arch" = "arm" ] || [ "$arch" = "arm64" ]; then
>> +cat <<EOF >> lib/config.h
>> +#define UART_EARLY_BASE ${uart_early_base}UL
> Drop the 'UL'. If somebody attempted to supply it themselves it'd end up
> being ULUL. Just add another cast to where the value gets used to handle
> the type.
Good point.
>
>> +EOF
>> +fi
>> +cat <<EOF >> lib/config.h
>> +#endif
>> +EOF
> The one line appends could use 'echo' to take 2 less lines each.
I will change it to an echo.
>
>> diff --git a/lib/arm/io.c b/lib/arm/io.c
>> index 9fe9bd0bf659..0a3e8f237ab8 100644
>> --- a/lib/arm/io.c
>> +++ b/lib/arm/io.c
>> @@ -11,6 +11,7 @@
>>  #include <libcflat.h>
>>  #include <devicetree.h>
>>  #include <chr-testdev.h>
>> +#include <config.h>
>>  #include "arm/asm/psci.h"
>>  #include <asm/spinlock.h>
>>  #include <asm/io.h>
>> @@ -21,15 +22,6 @@ extern void halt(int code);
>>  
>>  static bool testdev_enabled;
>>  
>> -/*
>> - * 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;
>>
>> Putting config.h in lib makes it available for other architectures, in case they
>> want to implement something similar. Please suggest another location if there is
>> a better one.
>>
>> I think this looks better than using architecture-specific compile-time defines
>> buried in arm/Makefile.common, what do you think?
>>
> Yes, I agree this looks better. You'll probably want to add a
> lib/.gitignore for the file too and also remove it when
> 'make distclean' is run.
>
> Thanks,
> drew
Thank you for the feedback, I will incorporate these changes into the next
version of the patches.

Patch

diff --git a/configure b/configure
index df8581e3a906..d77b8b0d82fa 100755
--- a/configure
+++ b/configure
@@ -16,6 +16,7 @@  endian=""
 pretty_print_stacks=yes
 environ_default=yes
 u32_long=
+vmm=qemu
 
 usage() {
     cat <<-EOF
@@ -23,6 +24,8 @@  usage() {
 
        Options include:
            --arch=ARCH            architecture to compile for ($arch)
+           --vmm=VMM              virtual machine monitor to compile for (qemu
or kvmtool,
+                                  arm/arm64 only, default is qemu)
            --processor=PROCESSOR  processor to compile for ($arch)
            --cross-prefix=PREFIX  cross compiler prefix
            --cc=CC                c compiler to use ($cc)
@@ -71,6 +74,9 @@  while [[ "$1" = -* ]]; do
        --ld)
            ld="$arg"
            ;;
+       --vmm)
+           vmm="$arg"
+           ;;
        --enable-pretty-print-stacks)
            pretty_print_stacks=yes
            ;;
@@ -108,6 +114,14 @@  if [ "$arch" = "i386" ] || [ "$arch" = "x86_64" ]; then
     testdir=x86
 elif [ "$arch" = "arm" ] || [ "$arch" = "arm64" ]; then
     testdir=arm
+    if [ "$vmm" = "qemu" ]; then
+        uart_early_base=0x09000000
+    elif [ "$vmm" = "kvmtool" ]; then
+        uart_early_base=0x3f8
+    else
+        echo '--vmm must be one of "qemu" or "kvmtool"'
+        usage
+    fi
 elif [ "$arch" = "ppc64" ]; then
     testdir=powerpc
     firmware="$testdir/boot_rom.bin"
@@ -198,3 +212,16 @@  ENVIRON_DEFAULT=$environ_default
 ERRATATXT=errata.txt
 U32_LONG_FMT=$u32_long
 EOF
+
+cat <<EOF > lib/config.h
+#ifndef CONFIG_H
+#define CONFIG_H 1
+EOF
+if [ "$arch" = "arm" ] || [ "$arch" = "arm64" ]; then
+cat <<EOF >> lib/config.h
+#define UART_EARLY_BASE ${uart_early_base}UL
+EOF
+fi
+cat <<EOF >> lib/config.h
+#endif
+EOF
diff --git a/lib/arm/io.c b/lib/arm/io.c
index 9fe9bd0bf659..0a3e8f237ab8 100644
--- a/lib/arm/io.c
+++ b/lib/arm/io.c
@@ -11,6 +11,7 @@ 
 #include <libcflat.h>
 #include <devicetree.h>
 #include <chr-testdev.h>
+#include <config.h>
 #include "arm/asm/psci.h"
 #include <asm/spinlock.h>
 #include <asm/io.h>
@@ -21,15 +22,6 @@  extern void halt(int code);
 
 static bool testdev_enabled;
 
-/*
- * 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;