xen/arch/arm/Makefile | 5 ++--- xen/arch/arm/arm32/head.S | 4 ++-- xen/arch/arm/arm64/head.S | 4 ++-- xen/arch/arm/xen.lds.S | 4 +--- xen/common/Kconfig | 10 ++++++++++ 5 files changed, 17 insertions(+), 10 deletions(-)
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.
Make a change in the linker script from:
_sdtb = .;
to:
PROVIDE(_sdtb = .);
to avoid creation of _sdtb if there is no reference to it.
Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
xen/arch/arm/Makefile | 5 ++---
xen/arch/arm/arm32/head.S | 4 ++--
xen/arch/arm/arm64/head.S | 4 ++--
xen/arch/arm/xen.lds.S | 4 +---
xen/common/Kconfig | 10 ++++++++++
5 files changed, 17 insertions(+), 10 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/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index c404fa973e..50f019ed98 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -156,10 +156,10 @@ past_zImage:
sub r10, r9, r0 /* r10 := phys-offset */
/* Using the DTB in the .dtb section? */
-#ifdef CONFIG_DTB_FILE
+.ifnes CONFIG_DTB_FILE,""
ldr r8, =_sdtb
add r8, r10 /* r8 := paddr(DTB) */
-#endif
+.endif
/* Initialize the UART if earlyprintk has been enabled. */
#ifdef CONFIG_EARLY_PRINTK
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 5d44667bd8..f38a8dfca7 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -296,9 +296,9 @@ real_start_efi:
sub x20, x19, x0 /* x20 := phys-offset */
/* Using the DTB in the .dtb section? */
-#ifdef CONFIG_DTB_FILE
+.ifnes CONFIG_DTB_FILE,""
load_paddr x21, _sdtb
-#endif
+.endif
/* Initialize the UART if earlyprintk has been enabled. */
#ifdef CONFIG_EARLY_PRINTK
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 004b182acb..540a7ccc9d 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -220,11 +220,9 @@ SECTIONS
} :text
_end = . ;
-#ifdef CONFIG_DTB_FILE
/* Section for the device tree blob (if any). */
- _sdtb = .;
+ PROVIDE(_sdtb = .);
.dtb : { *(.dtb) } :text
-#endif
/* Sections to be discarded */
/DISCARD/ : {
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index eb953d171e..a1755cd380 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -400,6 +400,16 @@ 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.
+
+ This is an optional config. Leave empty if not needed.
+
config TRACEBUFFER
bool "Enable tracing infrastructure" if EXPERT
default y
--
2.29.0
Hi Michal, > On 15 Mar 2021, at 09:23, Michal Orzel <Michal.Orzel@arm.com> 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. > > Make a change in the linker script from: > _sdtb = .; > to: > PROVIDE(_sdtb = .); > to avoid creation of _sdtb if there is no reference to it. > > Signed-off-by: Michal Orzel <michal.orzel@arm.com> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> The use of ifnes and PROVIDE is very clever :-) Cheers Bertrand > --- > xen/arch/arm/Makefile | 5 ++--- > xen/arch/arm/arm32/head.S | 4 ++-- > xen/arch/arm/arm64/head.S | 4 ++-- > xen/arch/arm/xen.lds.S | 4 +--- > xen/common/Kconfig | 10 ++++++++++ > 5 files changed, 17 insertions(+), 10 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/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S > index c404fa973e..50f019ed98 100644 > --- a/xen/arch/arm/arm32/head.S > +++ b/xen/arch/arm/arm32/head.S > @@ -156,10 +156,10 @@ past_zImage: > sub r10, r9, r0 /* r10 := phys-offset */ > > /* Using the DTB in the .dtb section? */ > -#ifdef CONFIG_DTB_FILE > +.ifnes CONFIG_DTB_FILE,"" > ldr r8, =_sdtb > add r8, r10 /* r8 := paddr(DTB) */ > -#endif > +.endif > > /* Initialize the UART if earlyprintk has been enabled. */ > #ifdef CONFIG_EARLY_PRINTK > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > index 5d44667bd8..f38a8dfca7 100644 > --- a/xen/arch/arm/arm64/head.S > +++ b/xen/arch/arm/arm64/head.S > @@ -296,9 +296,9 @@ real_start_efi: > sub x20, x19, x0 /* x20 := phys-offset */ > > /* Using the DTB in the .dtb section? */ > -#ifdef CONFIG_DTB_FILE > +.ifnes CONFIG_DTB_FILE,"" > load_paddr x21, _sdtb > -#endif > +.endif > > /* Initialize the UART if earlyprintk has been enabled. */ > #ifdef CONFIG_EARLY_PRINTK > diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S > index 004b182acb..540a7ccc9d 100644 > --- a/xen/arch/arm/xen.lds.S > +++ b/xen/arch/arm/xen.lds.S > @@ -220,11 +220,9 @@ SECTIONS > } :text > _end = . ; > > -#ifdef CONFIG_DTB_FILE > /* Section for the device tree blob (if any). */ > - _sdtb = .; > + PROVIDE(_sdtb = .); > .dtb : { *(.dtb) } :text > -#endif > > /* Sections to be discarded */ > /DISCARD/ : { > diff --git a/xen/common/Kconfig b/xen/common/Kconfig > index eb953d171e..a1755cd380 100644 > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -400,6 +400,16 @@ 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. > + > + This is an optional config. Leave empty if not needed. > + > config TRACEBUFFER > bool "Enable tracing infrastructure" if EXPERT > default y > -- > 2.29.0 >
Hi Michal, On 15/03/2021 09:23, 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. > > Make a change in the linker script from: > _sdtb = .; > to: > PROVIDE(_sdtb = .); > to avoid creation of _sdtb if there is no reference to it. This means that if someone is using #ifdef CONFIG_DTB_FILE rather than .ifnes, _sdtb will get defined. The difference between two is quite subttle and can be easily missed during review. How about defining _sdtb in dtb.S? With that approach, we would get a compiler error if someone protect _sdtb with #ifdef rather than .ifnes. Cheers, -- Julien Grall
Hi Julien, On 16.03.2021 15:54, Julien Grall wrote: > Hi Michal, > > On 15/03/2021 09:23, 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. >> >> Make a change in the linker script from: >> _sdtb = .; >> to: >> PROVIDE(_sdtb = .); >> to avoid creation of _sdtb if there is no reference to it. > > This means that if someone is using #ifdef CONFIG_DTB_FILE rather than .ifnes, _sdtb will get defined. Do we really want to overengineer something that simple? Why would someone use #ifdef CONFIG_DTB_FILE? In my opinion the rule of thumb is that you don't use #ifdef on configs of string type. Using #ifdef CONFIG_DTB_FILE means that someone modifying the code did not even spend 1 minute checking the Kconfig. If you do not agree, I can modify the code so _sdtb will be created in dtb.S. In that case I would like Jan Beulich to give his opinion before I will send v8. > > The difference between two is quite subttle and can be easily missed during review. > > How about defining _sdtb in dtb.S? With that approach, we would get a compiler error if someone protect _sdtb with #ifdef rather than .ifnes. > > Cheers, > Cheers, Michal
On 18.03.2021 08:21, Michal Orzel wrote: > Hi Julien, > > On 16.03.2021 15:54, Julien Grall wrote: >> Hi Michal, >> >> On 15/03/2021 09:23, 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. >>> >>> Make a change in the linker script from: >>> _sdtb = .; >>> to: >>> PROVIDE(_sdtb = .); >>> to avoid creation of _sdtb if there is no reference to it. >> >> This means that if someone is using #ifdef CONFIG_DTB_FILE rather than .ifnes, _sdtb will get defined. > > Do we really want to overengineer something that simple? > Why would someone use #ifdef CONFIG_DTB_FILE? > In my opinion the rule of thumb is that you don't use #ifdef on configs of string type. > Using #ifdef CONFIG_DTB_FILE means that someone modifying the code did not even spend 1 minute checking the Kconfig. > > If you do not agree, I can modify the code so _sdtb will be created in dtb.S. > In that case I would like Jan Beulich to give his opinion before I will send v8. TBH I'd find it more natural in any event if the symbol came from dtb.S. So far I was assuming there was some (hidden) reason why this wouldn't work. Jan
On 18.03.2021 10:20, Jan Beulich wrote: > On 18.03.2021 08:21, Michal Orzel wrote: >> Hi Julien, >> >> On 16.03.2021 15:54, Julien Grall wrote: >>> Hi Michal, >>> >>> On 15/03/2021 09:23, 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. >>>> >>>> Make a change in the linker script from: >>>> _sdtb = .; >>>> to: >>>> PROVIDE(_sdtb = .); >>>> to avoid creation of _sdtb if there is no reference to it. >>> >>> This means that if someone is using #ifdef CONFIG_DTB_FILE rather than .ifnes, _sdtb will get defined. >> >> Do we really want to overengineer something that simple? >> Why would someone use #ifdef CONFIG_DTB_FILE? >> In my opinion the rule of thumb is that you don't use #ifdef on configs of string type. >> Using #ifdef CONFIG_DTB_FILE means that someone modifying the code did not even spend 1 minute checking the Kconfig. >> >> If you do not agree, I can modify the code so _sdtb will be created in dtb.S. >> In that case I would like Jan Beulich to give his opinion before I will send v8. > > TBH I'd find it more natural in any event if the symbol came from > dtb.S. So far I was assuming there was some (hidden) reason why > this wouldn't work. > > Jan > Then if both of you agree that _sdtb should be defined in dtb.S, I will make it in v8 Michal
On 18/03/2021 07:21, Michal Orzel wrote: > Hi Julien, > > On 16.03.2021 15:54, Julien Grall wrote: >> Hi Michal, >> >> On 15/03/2021 09:23, 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. >>> >>> Make a change in the linker script from: >>> _sdtb = .; >>> to: >>> PROVIDE(_sdtb = .); >>> to avoid creation of _sdtb if there is no reference to it. >> >> This means that if someone is using #ifdef CONFIG_DTB_FILE rather than .ifnes, _sdtb will get defined. > > Do we really want to overengineer something that simple? Interesting, I would argue that using PROVIDE is over-engineered (and error-prone) when you likely can define _sdtb in .S. > Why would someone use #ifdef CONFIG_DTB_FILE? > In my opinion the rule of thumb is that you don't use #ifdef on configs of string type. > Using #ifdef CONFIG_DTB_FILE means that someone modifying the code did not even spend 1 minute checking the Kconfig. I'd like to point out that your first approach didn't replace #ifdef. This was fully reviewed and nearly committed. When I pointed out the issue, I had to argue that this was broken. This is not quite the same as adding #ifdef but it shows that such mistake can be easily slipped in master. Cheers, -- Julien Grall
© 2016 - 2024 Red Hat, Inc.