Re: [Xen-devel] [PATCH for-4.13] xen/arm: Add Skeleton for using configuring early printk using Kconfig

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 1 Oct 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 10/1/19 8:33 PM, Stefano Stabellini wrote:
> > On Fri, 13 Sep 2019, Julien Grall wrote:
> > > At the moment, early printk can only be configured on the make command
> > > line. It is not very handy because a user has to remove the option
> > > everytime it is using another command other than compiling the
> > > hypervisor.
> > > 
> > > Furthermore, early printk is one of the few odds one that are not using
> > > Kconfig.
> > > 
> > > So this is about time to move it to Kconfig. For now, a skeleton is
> > > added with one example based on Cadence UART. Follow-up will continue to
> > > convert all the options to Kconfig.
> > > 
> > > Because Kconfig will prefix all the config by CONFIG_, it is necessary
> > > to adapt the define within the code.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > > 
> > > ---
> > > 
> > > I have sent it as RFC because this is not complete. I will convert the
> > > rest once we agree the approach is correct.
> > > ---
> > >   xen/Kconfig.debug                  |  2 ++
> > >   xen/arch/arm/Kconfig.debug         | 40
> > > ++++++++++++++++++++++++++++++++++++++
> > >   xen/arch/arm/Rules.mk              |  5 ++---
> > >   xen/arch/arm/arm32/head.S          |  8 ++++----
> > >   xen/arch/arm/arm64/debug.S         |  4 ++--
> > >   xen/arch/arm/arm64/head.S          |  8 ++++----
> > >   xen/arch/x86/Kconfig.debug         |  0
> > >   xen/include/asm-arm/early_printk.h |  2 +-
> > >   8 files changed, 55 insertions(+), 14 deletions(-)
> > >   create mode 100644 xen/arch/arm/Kconfig.debug
> > >   create mode 100644 xen/arch/x86/Kconfig.debug
> > > 
> > > diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug
> > > index e10e314e25..d0806dba32 100644
> > > --- a/xen/Kconfig.debug
> > > +++ b/xen/Kconfig.debug
> > > @@ -112,6 +112,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"
> > > +
> > >   endif # DEBUG || EXPERT
> > >     endmenu
> > > diff --git a/xen/arch/arm/Kconfig.debug b/xen/arch/arm/Kconfig.debug
> > > new file mode 100644
> > > index 0000000000..bc3b622695
> > > --- /dev/null
> > > +++ b/xen/arch/arm/Kconfig.debug
> > > @@ -0,0 +1,40 @@
> > > +choice
> > > + prompt "Enable early printk"
> > > +
> > > + optional
> > > + config EARLY_PRINTK_ZYNQMP
> > > +         bool "Enable early printk on Xilinx ZynQMP"
> > > +         select EARLY_UART_CADENCE
> > > +         help
> > > +           Say Y here if you want the early printk support on Xilinx
> > > +           ZynQMP platform.
> > > +
> > > + config EARLY_PRINTK_CADENCE
> > > +         bool "Enable early printk via Cadence UART"
> > > +         select EARLY_UART_CADENCE
> > 
> > Why not select EARLY_PRINTK directly? Is EARLY_UART_CADENCE actually
> > needed?
> 
> Yes to select the proper EARLY_PRINTK_INC. For other UARTs (such as 8250), it
> will be necessary to enable more options.
> 
> The other option is to have lengthy if in some part of the Kconfig which is
> not pretty and likely a way to miss some places if we ever decide to add more
> alias (I hope not!).
> 
> > 
> > 
> > > +         help
> > > +           Say Y here if you wish the early printk to direct their
> > > +           output to a Cadence UART. You can use this option to provide
> > > +           the parameters for the Cadence UART rather than selecting
> > > +           one of the platform specific options above if you know the
> > > +           parameters for the port.
> > > +
> > > +           This option is preferred over the platform specific options;
> > > +           the platform specific options are deprecated and will soon
> > > +           be removed.
> > > +endchoice
> > > +
> > > +config EARLY_PRINTK
> > > + bool
> > > +
> > > +config EARLY_UART_CADENCE
> > > + bool
> > > + select EARLY_PRINTK
> > > +
> > > +config EARLY_UART_BASE_ADDRESS
> > > + hex "Physical base address of debug UART" if EARLY_PRINTK
> > > + default 0xff000000 if EARLY_PRINTK_ZYNQMP
> > 
> > This only works for EARLY_PRINTK_ZYNQMP and not for
> > EARLY_PRINTK_CADENCE. I imagine we need a default line for each of the
> > possible options above.
> 
> If you don't set any default value, then it will be requested when compiling
> Xen. The risk of setting a default value for "generic" config is the user may
> not notice that it needs to fill it up.
> 
> So he/she may end up to compile Xen with the wrong address and will get not
> output later on at best. This sort of things is quite difficult to debug until
> you know what you are doing.
> 
> > 
> > 
> > > +config EARLY_PRINTK_INC
> > > + string
> > > + default "debug-cadence.inc" if EARLY_UART_CADENCE
> > > diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
> > > index 3d9a0ed357..084f1725a8 100644
> > > --- a/xen/arch/arm/Rules.mk
> > > +++ b/xen/arch/arm/Rules.mk
> > > @@ -46,7 +46,6 @@ EARLY_PRINTK_thunderx       := pl011,0x87e024000000
> > >   EARLY_PRINTK_vexpress       := pl011,0x1c090000
> > >   EARLY_PRINTK_xgene-mcdivitt := 8250,0x1c021000,2
> > >   EARLY_PRINTK_xgene-storm    := 8250,0x1c020000,2
> > > -EARLY_PRINTK_zynqmp         := cadence,0xff000000
> > >     ifneq ($(EARLY_PRINTK_$(CONFIG_EARLY_PRINTK)),)
> > >   EARLY_PRINTK_CFG := $(subst $(comma),
> > > ,$(EARLY_PRINTK_$(CONFIG_EARLY_PRINTK)))
> > > @@ -82,9 +81,9 @@ endif
> > >     CFLAGS-$(EARLY_PRINTK) += -DCONFIG_EARLY_PRINTK
> > >   CFLAGS-$(EARLY_PRINTK_INIT_UART) += -DEARLY_PRINTK_INIT_UART
> > > -CFLAGS-$(EARLY_PRINTK) +=
> > > -DEARLY_PRINTK_INC=\"debug-$(EARLY_PRINTK_INC).inc\"
> > > +CFLAGS-$(EARLY_PRINTK) +=
> > > -DCONFIG_EARLY_PRINTK_INC=\"debug-$(EARLY_PRINTK_INC).inc\"
> > >   CFLAGS-$(EARLY_PRINTK) += -DEARLY_PRINTK_BAUD=$(EARLY_PRINTK_BAUD)
> > > -CFLAGS-$(EARLY_PRINTK) +=
> > > -DEARLY_UART_BASE_ADDRESS=$(EARLY_UART_BASE_ADDRESS)
> > > +CFLAGS-$(EARLY_PRINTK) +=
> > > -DCONFIG_EARLY_UART_BASE_ADDRESS=$(EARLY_UART_BASE_ADDRESS)
> > 
> > I don't have an opinion on the naming, the following is a suggestion to
> > make the patch easier to write.
> 
> I got bored enough on the plan to try to do more cleanup at the same time (see
> below). ;)
> 
> >  It looks like we could get away without
> > any of the renaming below if we choose to export
> > EARLY_UART_BASE_ADDRESS and keep using EARLY_UART_BASE_ADDRESS directly
> > in the code below.
> 
> I want to get rid of anything not starting with CONFIG_ because this is
> misleading. So they either got drop in this patch (and follow-ups) or they get
> renamed before hand.
> 
> I chose the former because some options will not be a straight prefixing of
> CONFIG_. For instance, EARLY_UART_REG_SHIFT is a specific to 8250, so I would
> like to rename it CONFIG_EARLY_UART_8250_SHIFT.
> 
> I can try to do the renaming before hand if you prefer.

Yes, I prefer the renaming to be separate if it is not a problem.

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