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

[Julien Grall <julien@xxxxxxx>]

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,

--
Julien Grall