| Message ID | 20210315092342.26533-1-michal.orzel@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v7] arm: Add Kconfig entry to select CONFIG_DTB_FILE | expand |
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,
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 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,
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
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
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(-)