diff mbox series

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

Message ID 20190913103953.8182-1-julien.grall@arm.com
State New
Headers show
Series [Xen-devel,for-4.13] xen/arm: Add Skeleton for using configuring early printk using Kconfig | expand

Commit Message

Julien Grall Sept. 13, 2019, 10:39 a.m. UTC 
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@arm.com>

---

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

Comments

Wei Liu Sept. 16, 2019, 10:23 a.m. UTC | #1 
On Fri, Sep 13, 2019 at 11:39:53AM +0100, 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@arm.com>
> 
> ---
> 
> I have sent it as RFC because this is not complete. I will convert the
> rest once we agree the approach is correct.

Having a top-level Kconfig.debug and includes arch specific rules is how
Linux does it (albeit with different directory structure), so I think
we're just following the norm here. No objection from me.

Wei.
Stefano Stabellini Oct. 1, 2019, 6:30 p.m. UTC | #2 
On Mon, 16 Sep 2019, Wei Liu wrote:
> On Fri, Sep 13, 2019 at 11:39:53AM +0100, 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@arm.com>
> > 
> > ---
> > 
> > I have sent it as RFC because this is not complete. I will convert the
> > rest once we agree the approach is correct.
> 
> Having a top-level Kconfig.debug and includes arch specific rules is how
> Linux does it (albeit with different directory structure), so I think
> we're just following the norm here. No objection from me.

I agree
Julien Grall Oct. 1, 2019, 6:52 p.m. UTC | #3 
Hi,

On 10/1/19 7:30 PM, Stefano Stabellini wrote:
> On Mon, 16 Sep 2019, Wei Liu wrote:
>> On Fri, Sep 13, 2019 at 11:39:53AM +0100, 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@arm.com>
>>>
>>> ---
>>>
>>> I have sent it as RFC because this is not complete. I will convert the
>>> rest once we agree the approach is correct.
>>
>> Having a top-level Kconfig.debug and includes arch specific rules is how
>> Linux does it (albeit with different directory structure), so I think
>> we're just following the norm here. No objection from me.
> 
> I agree

I know this is an RFC, but do you any opinion on the rest of the patch? 
Is the interface with the user good for you?

Cheers,
Stefano Stabellini Oct. 1, 2019, 7:33 p.m. UTC | #4 
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@arm.com>
> 
> ---
> 
> 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?


> +		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.


> +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. 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.


>  CFLAGS-$(EARLY_PRINTK) += -DEARLY_UART_REG_SHIFT=$(EARLY_UART_REG_SHIFT)
>  
>  else # !CONFIG_DEBUG
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 8f945d318a..0c7e405299 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -32,8 +32,8 @@
>  #define PT_UPPER(x) (PT_##x & 0xf00)
>  #define PT_LOWER(x) (PT_##x & 0x0ff)
>  
> -#if (defined (CONFIG_EARLY_PRINTK)) && (defined (EARLY_PRINTK_INC))
> -#include EARLY_PRINTK_INC
> +#if (defined (CONFIG_EARLY_PRINTK)) && (defined (CONFIG_EARLY_PRINTK_INC))
> +#include CONFIG_EARLY_PRINTK_INC
>  #endif
>  
>  /*
> @@ -190,7 +190,7 @@ GLOBAL(init_secondary)
>  1:
>  
>  #ifdef CONFIG_EARLY_PRINTK
> -        mov_w r11, EARLY_UART_BASE_ADDRESS   /* r11 := UART base address */
> +        mov_w r11, CONFIG_EARLY_UART_BASE_ADDRESS   /* r11 := UART base address */
>          PRINT("- CPU ")
>          print_reg r7
>          PRINT(" booting -\r\n")
> @@ -580,7 +580,7 @@ ENTRY(switch_ttbr)
>   * Clobbers r0 - r3
>   */
>  init_uart:
> -        mov_w r11, EARLY_UART_BASE_ADDRESS
> +        mov_w r11, CONFIG_EARLY_UART_BASE_ADDRESS
>  #ifdef EARLY_PRINTK_INIT_UART
>          early_uart_init r11, r1, r2
>  #endif
> diff --git a/xen/arch/arm/arm64/debug.S b/xen/arch/arm/arm64/debug.S
> index b7f53ac051..71cad9d762 100644
> --- a/xen/arch/arm/arm64/debug.S
> +++ b/xen/arch/arm/arm64/debug.S
> @@ -19,8 +19,8 @@
>  
>  #include <asm/early_printk.h>
>  
> -#ifdef EARLY_PRINTK_INC
> -#include EARLY_PRINTK_INC
> +#ifdef CONFIG_EARLY_PRINTK_INC
> +#include CONFIG_EARLY_PRINTK_INC
>  #endif
>  
>  /*
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 790b485f04..32b895ecea 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -40,8 +40,8 @@
>  #define __HEAD_FLAGS            ((__HEAD_FLAG_PAGE_SIZE << 1) | \
>                                   (__HEAD_FLAG_PHYS_BASE << 3))
>  
> -#if (defined (CONFIG_EARLY_PRINTK)) && (defined (EARLY_PRINTK_INC))
> -#include EARLY_PRINTK_INC
> +#if (defined (CONFIG_EARLY_PRINTK)) && (defined (CONFIG_EARLY_PRINTK_INC))
> +#include CONFIG_EARLY_PRINTK_INC
>  #endif
>  
>  /*
> @@ -351,7 +351,7 @@ GLOBAL(init_secondary)
>  1:
>  
>  #ifdef CONFIG_EARLY_PRINTK
> -        ldr   x23, =EARLY_UART_BASE_ADDRESS /* x23 := UART base address */
> +        ldr   x23, =CONFIG_EARLY_UART_BASE_ADDRESS /* x23 := UART base address */
>          PRINT("- CPU ")
>          print_reg x24
>          PRINT(" booting -\r\n")
> @@ -753,7 +753,7 @@ ENTRY(switch_ttbr)
>   * Clobbers x0 - x1
>   */
>  init_uart:
> -        ldr   x23, =EARLY_UART_BASE_ADDRESS
> +        ldr   x23, =CONFIG_EARLY_UART_BASE_ADDRESS
>  #ifdef EARLY_PRINTK_INIT_UART
>          early_uart_init x23, 0
>  #endif
> diff --git a/xen/arch/x86/Kconfig.debug b/xen/arch/x86/Kconfig.debug
> new file mode 100644
> index 0000000000..e69de29bb2
> diff --git a/xen/include/asm-arm/early_printk.h b/xen/include/asm-arm/early_printk.h
> index 078cf701dc..d5485decfa 100644
> --- a/xen/include/asm-arm/early_printk.h
> +++ b/xen/include/asm-arm/early_printk.h
> @@ -15,7 +15,7 @@
>  
>  /* need to add the uart address offset in page to the fixmap address */
>  #define EARLY_UART_VIRTUAL_ADDRESS \
> -    (FIXMAP_ADDR(FIXMAP_CONSOLE) +(EARLY_UART_BASE_ADDRESS & ~PAGE_MASK))
> +    (FIXMAP_ADDR(FIXMAP_CONSOLE) + (CONFIG_EARLY_UART_BASE_ADDRESS & ~PAGE_MASK))
>  
>  #endif /* !CONFIG_EARLY_PRINTK */
>  
> -- 
> 2.11.0
>
Julien Grall Oct. 1, 2019, 7:58 p.m. UTC | #5 
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@arm.com>
>>
>> ---
>>
>> 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.

Cheers,
Stefano Stabellini Oct. 2, 2019, 12:53 a.m. UTC | #6 
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@arm.com>
> > > 
> > > ---
> > > 
> > > 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.
diff mbox series

Patch

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
+		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
+
+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)
 CFLAGS-$(EARLY_PRINTK) += -DEARLY_UART_REG_SHIFT=$(EARLY_UART_REG_SHIFT)
 
 else # !CONFIG_DEBUG
diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 8f945d318a..0c7e405299 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -32,8 +32,8 @@ 
 #define PT_UPPER(x) (PT_##x & 0xf00)
 #define PT_LOWER(x) (PT_##x & 0x0ff)
 
-#if (defined (CONFIG_EARLY_PRINTK)) && (defined (EARLY_PRINTK_INC))
-#include EARLY_PRINTK_INC
+#if (defined (CONFIG_EARLY_PRINTK)) && (defined (CONFIG_EARLY_PRINTK_INC))
+#include CONFIG_EARLY_PRINTK_INC
 #endif
 
 /*
@@ -190,7 +190,7 @@  GLOBAL(init_secondary)
 1:
 
 #ifdef CONFIG_EARLY_PRINTK
-        mov_w r11, EARLY_UART_BASE_ADDRESS   /* r11 := UART base address */
+        mov_w r11, CONFIG_EARLY_UART_BASE_ADDRESS   /* r11 := UART base address */
         PRINT("- CPU ")
         print_reg r7
         PRINT(" booting -\r\n")
@@ -580,7 +580,7 @@  ENTRY(switch_ttbr)
  * Clobbers r0 - r3
  */
 init_uart:
-        mov_w r11, EARLY_UART_BASE_ADDRESS
+        mov_w r11, CONFIG_EARLY_UART_BASE_ADDRESS
 #ifdef EARLY_PRINTK_INIT_UART
         early_uart_init r11, r1, r2
 #endif
diff --git a/xen/arch/arm/arm64/debug.S b/xen/arch/arm/arm64/debug.S
index b7f53ac051..71cad9d762 100644
--- a/xen/arch/arm/arm64/debug.S
+++ b/xen/arch/arm/arm64/debug.S
@@ -19,8 +19,8 @@ 
 
 #include <asm/early_printk.h>
 
-#ifdef EARLY_PRINTK_INC
-#include EARLY_PRINTK_INC
+#ifdef CONFIG_EARLY_PRINTK_INC
+#include CONFIG_EARLY_PRINTK_INC
 #endif
 
 /*
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 790b485f04..32b895ecea 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -40,8 +40,8 @@ 
 #define __HEAD_FLAGS            ((__HEAD_FLAG_PAGE_SIZE << 1) | \
                                  (__HEAD_FLAG_PHYS_BASE << 3))
 
-#if (defined (CONFIG_EARLY_PRINTK)) && (defined (EARLY_PRINTK_INC))
-#include EARLY_PRINTK_INC
+#if (defined (CONFIG_EARLY_PRINTK)) && (defined (CONFIG_EARLY_PRINTK_INC))
+#include CONFIG_EARLY_PRINTK_INC
 #endif
 
 /*
@@ -351,7 +351,7 @@  GLOBAL(init_secondary)
 1:
 
 #ifdef CONFIG_EARLY_PRINTK
-        ldr   x23, =EARLY_UART_BASE_ADDRESS /* x23 := UART base address */
+        ldr   x23, =CONFIG_EARLY_UART_BASE_ADDRESS /* x23 := UART base address */
         PRINT("- CPU ")
         print_reg x24
         PRINT(" booting -\r\n")
@@ -753,7 +753,7 @@  ENTRY(switch_ttbr)
  * Clobbers x0 - x1
  */
 init_uart:
-        ldr   x23, =EARLY_UART_BASE_ADDRESS
+        ldr   x23, =CONFIG_EARLY_UART_BASE_ADDRESS
 #ifdef EARLY_PRINTK_INIT_UART
         early_uart_init x23, 0
 #endif
diff --git a/xen/arch/x86/Kconfig.debug b/xen/arch/x86/Kconfig.debug
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/xen/include/asm-arm/early_printk.h b/xen/include/asm-arm/early_printk.h
index 078cf701dc..d5485decfa 100644
--- a/xen/include/asm-arm/early_printk.h
+++ b/xen/include/asm-arm/early_printk.h
@@ -15,7 +15,7 @@ 
 
 /* need to add the uart address offset in page to the fixmap address */
 #define EARLY_UART_VIRTUAL_ADDRESS \
-    (FIXMAP_ADDR(FIXMAP_CONSOLE) +(EARLY_UART_BASE_ADDRESS & ~PAGE_MASK))
+    (FIXMAP_ADDR(FIXMAP_CONSOLE) + (CONFIG_EARLY_UART_BASE_ADDRESS & ~PAGE_MASK))
 
 #endif /* !CONFIG_EARLY_PRINTK */