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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

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.
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 be compiled and embedded into Xen if and only if the path to dtb 
was given.

Cheers,
Michal