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

[Julien Grall <julien@xxxxxxx>]

Hi Anthony,

On 11/03/2020 15:26, Anthony PERARD wrote:
> 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.
The second version looks better to me.

> > > +
> > > +       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 was expecting GAS to throw an error because the 64-bit value does not 
fit in a 32-bit register. But... it looks like GAS will silently 
truncate the value to 0x24000000 :(.
        
> (I mean "arm-none-eabi-gcc (Arch Repository) 9.2.0")
>
> But I can have thunderx depends on arm_64.
Is there a way to constrainst the address in Kconfig?

Cheers,

--
Julien Grall

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