| Message ID | 20190124111634.4727-3-alexandru.elisei@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm/arm64: Add support for running under kvmtool | expand |
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? 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); > - } > } > > void io_init(void)
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
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?
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
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 + 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" @@ -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 + CXXFLAGS += $(COMMON_CFLAGS) autodepend-flags = -MMD -MF $(dir $*).$(notdir $*).d
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
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.
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@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. 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
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, + 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; 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?
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
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.
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); - } } void io_init(void)
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. Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> --- lib/arm/io.c | 6 ------ 1 file changed, 6 deletions(-)