Re: [PATCH v4] arm: Add Kconfig entry to select CONFIG_DTB_FILE

[Julien Grall <julien@xen.org>]

On 11/03/2021 10:41, Michal Orzel wrote:
> Hi Julien,

Hi,

> 
> On 11.03.2021 11:34, Julien Grall wrote:
>> Hi Michal,
>>
>> On 10/03/2021 06:58, 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.
>>>
>>> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
>>> ---
>>>    xen/arch/arm/Makefile | 5 ++---
>>>    xen/common/Kconfig    | 8 ++++++++
>>>    2 files changed, 10 insertions(+), 3 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/common/Kconfig b/xen/common/Kconfig
>>> index eb953d171e..a27836bf47 100644
>>> --- a/xen/common/Kconfig
>>> +++ b/xen/common/Kconfig
>>> @@ -400,6 +400,14 @@ 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.
>>
>> With this approach, CONFIG_DTB_FILE will always be defined. This means that Xen will always be compiled to use the "embedded" DTB. When the string is "", it will be garbagge.
>>
>> So I think we need a second config to that indicates whether the string is empty or not.
>>
>> Interestingly, your first version of patch didn't expose the problem because CONFIG_DTB_FILE would not be defined if the CONFIG_LINK_DTB is not selected. Although, it would still happily build if CONFIG_DTB_FILE is "".
>>
>> Cheers,
>>
> I do not agrree. We do not need another config.

Did you test that Xen will still boot if the string is empty?

> If string is empty - the dtb.o will not be created and there will be no dtb section in xen binary.

The dtb.o will not be created but the section will because the linker 
use #ifdef CONFIG_DTB_FILE:

42sh> grep CONFIG_DTB .config
CONFIG_DTB_FILE=""
42sh> nm xen-syms | grep _sdtb
00000000003560f8 B _sdtb

And to show this is going to be used:

42sh> git diff
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 5d44667bd89d..2b680b8226d2 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -297,6 +297,7 @@ real_start_efi:

          /* Using the DTB in the .dtb section? */
  #ifdef CONFIG_DTB_FILE
+        e
          load_paddr x21, _sdtb
  #endif

42hs> make
[...]
   CC      arm64/head.o
arm64/head.S: Assembler messages:
arm64/head.S:300: Error: unknown mnemonic `e' -- `e'
/home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/Rules.mk:204: recipe for 
target 'arm64/head.o' failed

So _sdtb is going to always be used...

Cheers,

-- 
Julien Grall