[PATCH v7] arm: Add Kconfig entry to select CONFIG_DTB_FILE

Michal Orzel posted 1 patch 3 years ago
Test gitlab-ci passed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20210315092342.26533-1-michal.orzel@arm.com
There is a newer version of this series
xen/arch/arm/Makefile     |  5 ++---
xen/arch/arm/arm32/head.S |  4 ++--
xen/arch/arm/arm64/head.S |  4 ++--
xen/arch/arm/xen.lds.S    |  4 +---
xen/common/Kconfig        | 10 ++++++++++
5 files changed, 17 insertions(+), 10 deletions(-)
[PATCH v7] arm: Add Kconfig entry to select CONFIG_DTB_FILE
Posted by Michal Orzel 3 years ago
Currently in order to link existing DTB into Xen image
we need to either specify option CONFIG_DTB_FILE on the
command line or manually add it into .config.
Add Kconfig entry: CONFIG_DTB_FILE
to be able to provide the path to DTB we want to embed
into Xen image. If no path provided - the dtb will not
be embedded.

Remove the line: AFLAGS-y += -DCONFIG_DTB_FILE=\"$(CONFIG_DTB_FILE)\"
as it is not needed since Kconfig will define it in a header
with all the other config options.

Make a change in the linker script from:
_sdtb = .;
to:
PROVIDE(_sdtb = .);
to avoid creation of _sdtb if there is no reference to it.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
 xen/arch/arm/Makefile     |  5 ++---
 xen/arch/arm/arm32/head.S |  4 ++--
 xen/arch/arm/arm64/head.S |  4 ++--
 xen/arch/arm/xen.lds.S    |  4 +---
 xen/common/Kconfig        | 10 ++++++++++
 5 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 16e6523e2c..46e6a95fec 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -68,9 +68,8 @@ extra-y += $(TARGET_SUBARCH)/head.o
 
 #obj-bin-y += ....o
 
-ifdef CONFIG_DTB_FILE
+ifneq ($(CONFIG_DTB_FILE),"")
 obj-y += dtb.o
-AFLAGS-y += -DCONFIG_DTB_FILE=\"$(CONFIG_DTB_FILE)\"
 endif
 
 ALL_OBJS := $(TARGET_SUBARCH)/head.o $(ALL_OBJS)
@@ -137,7 +136,7 @@ asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c
 xen.lds: xen.lds.S
 	$(CPP) -P $(a_flags) -MQ $@ -o $@ $<
 
-dtb.o: $(CONFIG_DTB_FILE)
+dtb.o: $(patsubst "%",%,$(CONFIG_DTB_FILE))
 
 .PHONY: clean
 clean::
diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index c404fa973e..50f019ed98 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -156,10 +156,10 @@ past_zImage:
         sub   r10, r9, r0            /* r10 := phys-offset */
 
         /* Using the DTB in the .dtb section? */
-#ifdef CONFIG_DTB_FILE
+.ifnes CONFIG_DTB_FILE,""
         ldr   r8, =_sdtb
         add   r8, r10                /* r8 := paddr(DTB) */
-#endif
+.endif
 
         /* Initialize the UART if earlyprintk has been enabled. */
 #ifdef CONFIG_EARLY_PRINTK
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 5d44667bd8..f38a8dfca7 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -296,9 +296,9 @@ real_start_efi:
         sub   x20, x19, x0           /* x20 := phys-offset */
 
         /* Using the DTB in the .dtb section? */
-#ifdef CONFIG_DTB_FILE
+.ifnes CONFIG_DTB_FILE,""
         load_paddr x21, _sdtb
-#endif
+.endif
 
         /* Initialize the UART if earlyprintk has been enabled. */
 #ifdef CONFIG_EARLY_PRINTK
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 004b182acb..540a7ccc9d 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -220,11 +220,9 @@ SECTIONS
   } :text
   _end = . ;
 
-#ifdef CONFIG_DTB_FILE
   /* Section for the device tree blob (if any). */
-  _sdtb = .;
+  PROVIDE(_sdtb = .);
   .dtb : { *(.dtb) } :text
-#endif
 
   /* Sections to be discarded */
   /DISCARD/ : {
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index eb953d171e..a1755cd380 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -400,6 +400,16 @@ config DOM0_MEM
 
 	  Leave empty if you are not sure what to specify.
 
+config DTB_FILE
+	string "Absolute path to device tree blob"
+	depends on HAS_DEVICE_TREE
+	help
+	  When using a bootloader that has no device tree support or when there
+	  is no bootloader at all, use this option to specify the absolute path
+	  to a device tree that will be linked directly inside Xen binary.
+
+	  This is an optional config. Leave empty if not needed.
+
 config TRACEBUFFER
 	bool "Enable tracing infrastructure" if EXPERT
 	default y
-- 
2.29.0


Re: [PATCH v7] arm: Add Kconfig entry to select CONFIG_DTB_FILE
Posted by Bertrand Marquis 3 years ago
Hi Michal,

> On 15 Mar 2021, at 09:23, Michal Orzel <Michal.Orzel@arm.com> wrote:
> 
> Currently in order to link existing DTB into Xen image
> we need to either specify option CONFIG_DTB_FILE on the
> command line or manually add it into .config.
> Add Kconfig entry: CONFIG_DTB_FILE
> to be able to provide the path to DTB we want to embed
> into Xen image. If no path provided - the dtb will not
> be embedded.
> 
> Remove the line: AFLAGS-y += -DCONFIG_DTB_FILE=\"$(CONFIG_DTB_FILE)\"
> as it is not needed since Kconfig will define it in a header
> with all the other config options.
> 
> Make a change in the linker script from:
> _sdtb = .;
> to:
> PROVIDE(_sdtb = .);
> to avoid creation of _sdtb if there is no reference to it.
> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

The use of ifnes and PROVIDE is very clever :-)

Cheers
Bertrand


> ---
> xen/arch/arm/Makefile     |  5 ++---
> xen/arch/arm/arm32/head.S |  4 ++--
> xen/arch/arm/arm64/head.S |  4 ++--
> xen/arch/arm/xen.lds.S    |  4 +---
> xen/common/Kconfig        | 10 ++++++++++
> 5 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 16e6523e2c..46e6a95fec 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -68,9 +68,8 @@ extra-y += $(TARGET_SUBARCH)/head.o
> 
> #obj-bin-y += ....o
> 
> -ifdef CONFIG_DTB_FILE
> +ifneq ($(CONFIG_DTB_FILE),"")
> obj-y += dtb.o
> -AFLAGS-y += -DCONFIG_DTB_FILE=\"$(CONFIG_DTB_FILE)\"
> endif
> 
> ALL_OBJS := $(TARGET_SUBARCH)/head.o $(ALL_OBJS)
> @@ -137,7 +136,7 @@ asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c
> xen.lds: xen.lds.S
> 	$(CPP) -P $(a_flags) -MQ $@ -o $@ $<
> 
> -dtb.o: $(CONFIG_DTB_FILE)
> +dtb.o: $(patsubst "%",%,$(CONFIG_DTB_FILE))
> 
> .PHONY: clean
> clean::
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index c404fa973e..50f019ed98 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -156,10 +156,10 @@ past_zImage:
>         sub   r10, r9, r0            /* r10 := phys-offset */
> 
>         /* Using the DTB in the .dtb section? */
> -#ifdef CONFIG_DTB_FILE
> +.ifnes CONFIG_DTB_FILE,""
>         ldr   r8, =_sdtb
>         add   r8, r10                /* r8 := paddr(DTB) */
> -#endif
> +.endif
> 
>         /* Initialize the UART if earlyprintk has been enabled. */
> #ifdef CONFIG_EARLY_PRINTK
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 5d44667bd8..f38a8dfca7 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -296,9 +296,9 @@ real_start_efi:
>         sub   x20, x19, x0           /* x20 := phys-offset */
> 
>         /* Using the DTB in the .dtb section? */
> -#ifdef CONFIG_DTB_FILE
> +.ifnes CONFIG_DTB_FILE,""
>         load_paddr x21, _sdtb
> -#endif
> +.endif
> 
>         /* Initialize the UART if earlyprintk has been enabled. */
> #ifdef CONFIG_EARLY_PRINTK
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index 004b182acb..540a7ccc9d 100644
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -220,11 +220,9 @@ SECTIONS
>   } :text
>   _end = . ;
> 
> -#ifdef CONFIG_DTB_FILE
>   /* Section for the device tree blob (if any). */
> -  _sdtb = .;
> +  PROVIDE(_sdtb = .);
>   .dtb : { *(.dtb) } :text
> -#endif
> 
>   /* Sections to be discarded */
>   /DISCARD/ : {
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index eb953d171e..a1755cd380 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -400,6 +400,16 @@ config DOM0_MEM
> 
> 	  Leave empty if you are not sure what to specify.
> 
> +config DTB_FILE
> +	string "Absolute path to device tree blob"
> +	depends on HAS_DEVICE_TREE
> +	help
> +	  When using a bootloader that has no device tree support or when there
> +	  is no bootloader at all, use this option to specify the absolute path
> +	  to a device tree that will be linked directly inside Xen binary.
> +
> +	  This is an optional config. Leave empty if not needed.
> +
> config TRACEBUFFER
> 	bool "Enable tracing infrastructure" if EXPERT
> 	default y
> -- 
> 2.29.0
> 


Re: [PATCH v7] arm: Add Kconfig entry to select CONFIG_DTB_FILE
Posted by Julien Grall 3 years ago
Hi Michal,

On 15/03/2021 09:23, Michal Orzel wrote:
> Currently in order to link existing DTB into Xen image
> we need to either specify option CONFIG_DTB_FILE on the
> command line or manually add it into .config.
> Add Kconfig entry: CONFIG_DTB_FILE
> to be able to provide the path to DTB we want to embed
> into Xen image. If no path provided - the dtb will not
> be embedded.
> 
> Remove the line: AFLAGS-y += -DCONFIG_DTB_FILE=\"$(CONFIG_DTB_FILE)\"
> as it is not needed since Kconfig will define it in a header
> with all the other config options.
> 
> Make a change in the linker script from:
> _sdtb = .;
> to:
> PROVIDE(_sdtb = .);
> to avoid creation of _sdtb if there is no reference to it.

This means that if someone is using #ifdef CONFIG_DTB_FILE rather than 
.ifnes, _sdtb will get defined.

The difference between two is quite subttle and can be easily missed 
during review.

How about defining _sdtb in dtb.S? With that approach, we would get a 
compiler error if someone protect _sdtb with #ifdef rather than .ifnes.

Cheers,

-- 
Julien Grall

Re: [PATCH v7] arm: Add Kconfig entry to select CONFIG_DTB_FILE
Posted by Michal Orzel 3 years ago
Hi Julien,

On 16.03.2021 15:54, Julien Grall wrote:
> Hi Michal,
> 
> On 15/03/2021 09:23, Michal Orzel wrote:
>> Currently in order to link existing DTB into Xen image
>> we need to either specify option CONFIG_DTB_FILE on the
>> command line or manually add it into .config.
>> Add Kconfig entry: CONFIG_DTB_FILE
>> to be able to provide the path to DTB we want to embed
>> into Xen image. If no path provided - the dtb will not
>> be embedded.
>>
>> Remove the line: AFLAGS-y += -DCONFIG_DTB_FILE=\"$(CONFIG_DTB_FILE)\"
>> as it is not needed since Kconfig will define it in a header
>> with all the other config options.
>>
>> Make a change in the linker script from:
>> _sdtb = .;
>> to:
>> PROVIDE(_sdtb = .);
>> to avoid creation of _sdtb if there is no reference to it.
> 
> This means that if someone is using #ifdef CONFIG_DTB_FILE rather than .ifnes, _sdtb will get defined.

Do we really want to overengineer something that simple?
Why would someone use #ifdef CONFIG_DTB_FILE?
In my opinion the rule of thumb is that you don't use #ifdef on configs of string type.
Using #ifdef CONFIG_DTB_FILE means that someone modifying the code did not even spend 1 minute checking the Kconfig.

If you do not agree, I can modify the code so _sdtb will be created in dtb.S.
In that case I would like Jan Beulich to give his opinion before I will send v8.
> 
> The difference between two is quite subttle and can be easily missed during review.
> 
> How about defining _sdtb in dtb.S? With that approach, we would get a compiler error if someone protect _sdtb with #ifdef rather than .ifnes.
> 
> Cheers,
> 

Cheers,
Michal

Re: [PATCH v7] arm: Add Kconfig entry to select CONFIG_DTB_FILE
Posted by Jan Beulich 3 years ago
On 18.03.2021 08:21, Michal Orzel wrote:
> Hi Julien,
> 
> On 16.03.2021 15:54, Julien Grall wrote:
>> Hi Michal,
>>
>> On 15/03/2021 09:23, Michal Orzel wrote:
>>> Currently in order to link existing DTB into Xen image
>>> we need to either specify option CONFIG_DTB_FILE on the
>>> command line or manually add it into .config.
>>> Add Kconfig entry: CONFIG_DTB_FILE
>>> to be able to provide the path to DTB we want to embed
>>> into Xen image. If no path provided - the dtb will not
>>> be embedded.
>>>
>>> Remove the line: AFLAGS-y += -DCONFIG_DTB_FILE=\"$(CONFIG_DTB_FILE)\"
>>> as it is not needed since Kconfig will define it in a header
>>> with all the other config options.
>>>
>>> Make a change in the linker script from:
>>> _sdtb = .;
>>> to:
>>> PROVIDE(_sdtb = .);
>>> to avoid creation of _sdtb if there is no reference to it.
>>
>> This means that if someone is using #ifdef CONFIG_DTB_FILE rather than .ifnes, _sdtb will get defined.
> 
> Do we really want to overengineer something that simple?
> Why would someone use #ifdef CONFIG_DTB_FILE?
> In my opinion the rule of thumb is that you don't use #ifdef on configs of string type.
> Using #ifdef CONFIG_DTB_FILE means that someone modifying the code did not even spend 1 minute checking the Kconfig.
> 
> If you do not agree, I can modify the code so _sdtb will be created in dtb.S.
> In that case I would like Jan Beulich to give his opinion before I will send v8.

TBH I'd find it more natural in any event if the symbol came from
dtb.S. So far I was assuming there was some (hidden) reason why
this wouldn't work.

Jan

Re: [PATCH v7] arm: Add Kconfig entry to select CONFIG_DTB_FILE
Posted by Michal Orzel 3 years ago

On 18.03.2021 10:20, Jan Beulich wrote:
> On 18.03.2021 08:21, Michal Orzel wrote:
>> Hi Julien,
>>
>> On 16.03.2021 15:54, Julien Grall wrote:
>>> Hi Michal,
>>>
>>> On 15/03/2021 09:23, Michal Orzel wrote:
>>>> Currently in order to link existing DTB into Xen image
>>>> we need to either specify option CONFIG_DTB_FILE on the
>>>> command line or manually add it into .config.
>>>> Add Kconfig entry: CONFIG_DTB_FILE
>>>> to be able to provide the path to DTB we want to embed
>>>> into Xen image. If no path provided - the dtb will not
>>>> be embedded.
>>>>
>>>> Remove the line: AFLAGS-y += -DCONFIG_DTB_FILE=\"$(CONFIG_DTB_FILE)\"
>>>> as it is not needed since Kconfig will define it in a header
>>>> with all the other config options.
>>>>
>>>> Make a change in the linker script from:
>>>> _sdtb = .;
>>>> to:
>>>> PROVIDE(_sdtb = .);
>>>> to avoid creation of _sdtb if there is no reference to it.
>>>
>>> This means that if someone is using #ifdef CONFIG_DTB_FILE rather than .ifnes, _sdtb will get defined.
>>
>> Do we really want to overengineer something that simple?
>> Why would someone use #ifdef CONFIG_DTB_FILE?
>> In my opinion the rule of thumb is that you don't use #ifdef on configs of string type.
>> Using #ifdef CONFIG_DTB_FILE means that someone modifying the code did not even spend 1 minute checking the Kconfig.
>>
>> If you do not agree, I can modify the code so _sdtb will be created in dtb.S.
>> In that case I would like Jan Beulich to give his opinion before I will send v8.
> 
> TBH I'd find it more natural in any event if the symbol came from
> dtb.S. So far I was assuming there was some (hidden) reason why
> this wouldn't work.
> 
> Jan
> 
Then if both of you agree that _sdtb should be defined in dtb.S, I will make it in v8

Michal

Re: [PATCH v7] arm: Add Kconfig entry to select CONFIG_DTB_FILE
Posted by Julien Grall 3 years ago

On 18/03/2021 07:21, Michal Orzel wrote:
> Hi Julien,
> 
> On 16.03.2021 15:54, Julien Grall wrote:
>> Hi Michal,
>>
>> On 15/03/2021 09:23, Michal Orzel wrote:
>>> Currently in order to link existing DTB into Xen image
>>> we need to either specify option CONFIG_DTB_FILE on the
>>> command line or manually add it into .config.
>>> Add Kconfig entry: CONFIG_DTB_FILE
>>> to be able to provide the path to DTB we want to embed
>>> into Xen image. If no path provided - the dtb will not
>>> be embedded.
>>>
>>> Remove the line: AFLAGS-y += -DCONFIG_DTB_FILE=\"$(CONFIG_DTB_FILE)\"
>>> as it is not needed since Kconfig will define it in a header
>>> with all the other config options.
>>>
>>> Make a change in the linker script from:
>>> _sdtb = .;
>>> to:
>>> PROVIDE(_sdtb = .);
>>> to avoid creation of _sdtb if there is no reference to it.
>>
>> This means that if someone is using #ifdef CONFIG_DTB_FILE rather than .ifnes, _sdtb will get defined.
> 
> Do we really want to overengineer something that simple?

Interesting, I would argue that using PROVIDE is over-engineered (and 
error-prone) when you likely can define _sdtb in .S.

> Why would someone use #ifdef CONFIG_DTB_FILE?
> In my opinion the rule of thumb is that you don't use #ifdef on configs of string type.
> Using #ifdef CONFIG_DTB_FILE means that someone modifying the code did not even spend 1 minute checking the Kconfig.
I'd like to point out that your first approach didn't replace #ifdef. 
This was fully reviewed and nearly committed. When I pointed out the 
issue, I had to argue that this was broken.

This is not quite the same as adding #ifdef but it shows that such 
mistake can be easily slipped in master.

Cheers,

-- 
Julien Grall