Re: [Xen-devel] [XEN PATCH v3 2/2] xen/arm: Configure early printk via Kconfig

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Wed, Mar 11, 2020 at 02:18:20PM +0000, Julien Grall wrote:
> > diff --git a/docs/misc/arm/early-printk.txt b/docs/misc/arm/early-printk.txt
> > index 89e081e51eaf..c61973013097 100644
> > --- a/docs/misc/arm/early-printk.txt
> > +++ b/docs/misc/arm/early-printk.txt
> > @@ -1,64 +1,39 @@
> >   How to enable early printk
> > -Early printk can only be enabled if debug=y. You may want to enable it if
> > -you are debbuging code that executes before the console is initialized.
> > +Early printk can only be enabled if CONFIG_DEBUG=y.  You may want to enable
> 
> NIT: AFAICT, the file is using one space after full stop. I would like to
> keep it like that for consistency :).

Sound good, I should look at how to fix my vim configuration so it stop
adding extra spaces :-(

:set nojoinspaces

Won't happen again :-).

> > diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug
> > index b3511e81a275..ee6ee33b69be 100644
> > --- a/xen/Kconfig.debug
> > +++ b/xen/Kconfig.debug
> > @@ -128,6 +128,8 @@ config XMEM_POOL_POISON
> >       Poison free blocks with 0xAA bytes and verify them when a block is
> >       allocated in order to spot use-after-free issues.
> > +source "arch/$(SRCARCH)/Kconfig.debug"
> 
> To double check, this means that earlyprintk can be selected in EXPERT mode
> now. However, in the document early-printk.txt, the feature is said to only
> be enabled with CONFIG_DEBUG=y.
> 
> I like the idea of allowing a user to enable earlyprintk in EXPERT mode
> (some early boot bug may only occur in non-debug build). So I am happy to
> keep the code like. Can you update the doc accordingly?

Will do.

> > +
> >   endif # DEBUG || EXPERT
> >   endmenu
> > diff --git a/xen/arch/arm/Kconfig.debug b/xen/arch/arm/Kconfig.debug
> > new file mode 100644
> > index 000000000000..ffb21e8ac20a
> > --- /dev/null
> > +++ b/xen/arch/arm/Kconfig.debug
> > @@ -0,0 +1,287 @@
> > +choice
> > +   bool "Early printk"
> > +   optional
> > +   help
> > +           You may want to enable early printk if you are debugging code
> > +           that executes before the console is initialized.
> > +
> > +           Note that selecting this option will limit Xen to a single UART
> > +           definition. Attempting to boot Xen image on a different
> > +           platform *will not work*, so this option should not be enable
> > +           for Xens that are intended to be portable.
> > +
> > +           Choose one of the UART drivers for early printk, then you'll
> > +           have to specify the parameters, like the base address.
> > +
> > +           Alternatively, there are platform specific options which will
> > +           have default values for the various parameters.
> 
> Would it be worth to mention such options are deprecated?

Yes, I should mention that here. (And probably in the early-printk.txt
doc as well.)

How about:
    Alternatively, there are platform specific options which will
    have default values for the various parameters. But such option are
    deprecated and will soon be removed.

Or it would be better to highlight the fact that they are deprecated, so
maybe the following would be better:
    Deprecated: Alternatively, there are platform specific options which
    will have default values for the various parameters. But such option
    will soon be removed.



> > +
> > +   config EARLY_PRINTK_BRCM
> > +           bool "Early printk with 8250 on Broadcom 7445D0 boards with A15 
> > processors"
> > +           select EARLY_UART_8250
> 
> I noticed below you added "depends on ARM_64" on the Xilinx SoC. In general,
> platform specific options are tied to either arm32 or arm64, even if the
> UART "driver" is arch agnostic.

Those "depends" are only there because the early uart driver is only
available for one arch. "debug-cadence.inc" can only be found in
"arch/arm/arm64/", not in arm32, for example.

> You could technically boot Xen on Arm 32-bit on Armv8 HW provided they
> support 32-bit at the hypervisor level, but we never supported this case. So
> I am wondering whether we should add depends on each earlyprintk. Stefano,
> any opinions?
> 
> > +config EARLY_UART_BASE_ADDRESS
> > +   depends on EARLY_PRINTK
> > +   hex "Early printk, physical base address of debug UART"
> > +   default 0x87e024000000 if EARLY_PRINTK_THUNDERX
> 
> You are allowing EARLY_PRINTK_THUNDERX to be selected on Arm32 platform but
> the address is above 4G. I suspect this would break randconfig build.

gcc doesn't seems to complain :-).

(I mean "arm-none-eabi-gcc (Arch Repository) 9.2.0")

But I can have thunderx depends on arm_64.

Thanks,

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel