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

[Julien Grall <julien@xxxxxxx>]

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,

--
Julien Grall