[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4] arm: Add Kconfig entry to select CONFIG_DTB_FILE
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.
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |