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

[Michal Orzel <michal.orzel@xxxxxxx>]

On 11.03.2021 12:11, Julien Grall wrote:
> 
> 
> 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@xxxxxxx>
>>>> ---
>>>>    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,
> 

Yes you are right. So I could add another config like:
config DTB_VALID
        def_bool y if DTB_FILE != ""
and change all the lines containing:
#ifdef CONFIG_DTB_FILE
to
#ifdef CONFIG_DTB_VALID

What do u think?

Michal