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

[Michal Orzel <michal.orzel@xxxxxxx>]

On 11.03.2021 14:32, Julien Grall wrote:
> Hi,
> 
> On 11/03/2021 13:10, Jan Beulich wrote:
>> On 11.03.2021 13:39, Michal Orzel wrote:
>>> On 11.03.2021 12:11, Julien Grall wrote:
>>>> On 11/03/2021 10:41, Michal Orzel wrote:
>>>>> On 11.03.2021 11:34, Julien Grall wrote:
>>>>>> 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
>>
>> I'm sorry to jump in again, but I still think a 2nd Kconfig setting
>> is not needed. I count three uses of CONFIG_DTB_VALID outside of
>> make files. The ones in .S can be replaced by using assembler
>> directives .ifeqs / .ifneqs. And the one in xen.lds.S looks to be
>> unnecessary altogether: If there's no input .dtb section, the
>> linker wouldn't allocate an output one anyway. And the _sdtb symbol,
>> if you want to avoid its creation when there's no reference, could
>> be wrapped in PROVIDE(). (I also think that symbol should be
>> defined inside the section definition, not ahead of it.)
> 
> I don't particularly care of the approach used so long it doesn't break 
> existing setup and doesn't end up to define _sdtb.
> 
> Cheers,
> 
I will send v5.