* [PATCH v4] arm: Add Kconfig entry to select CONFIG_DTB_FILE
@ 2021-03-10 6:58 Michal Orzel
2021-03-10 8:05 ` Bertrand Marquis
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Michal Orzel @ 2021-03-10 6:58 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Ian Jackson, Jan Beulich, Wei Liu,
bertrand.marquis
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@arm.com>
---
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.
+
config TRACEBUFFER
bool "Enable tracing infrastructure" if EXPERT
default y
--
2.29.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v4] arm: Add Kconfig entry to select CONFIG_DTB_FILE
2021-03-10 6:58 [PATCH v4] arm: Add Kconfig entry to select CONFIG_DTB_FILE Michal Orzel
@ 2021-03-10 8:05 ` Bertrand Marquis
2021-03-10 9:00 ` Jan Beulich
2021-03-11 10:34 ` Julien Grall
2 siblings, 0 replies; 14+ messages in thread
From: Bertrand Marquis @ 2021-03-10 8:05 UTC (permalink / raw)
To: Michal Orzel
Cc: Xen-devel, Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Ian Jackson, Jan Beulich, Wei Liu
Hi,
> On 10 Mar 2021, at 07:58, 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.
>
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Cheers
Bertrand
> ---
> 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.
> +
> config TRACEBUFFER
> bool "Enable tracing infrastructure" if EXPERT
> default y
> --
> 2.29.0
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] arm: Add Kconfig entry to select CONFIG_DTB_FILE
2021-03-10 6:58 [PATCH v4] arm: Add Kconfig entry to select CONFIG_DTB_FILE Michal Orzel
2021-03-10 8:05 ` Bertrand Marquis
@ 2021-03-10 9:00 ` Jan Beulich
2021-03-10 11:33 ` Ian Jackson
2021-03-11 10:34 ` Julien Grall
2 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2021-03-10 9:00 UTC (permalink / raw)
To: Michal Orzel
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu,
bertrand.marquis, xen-devel
On 10.03.2021 07:58, Michal Orzel wrote:
> --- 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.
Nit: Linux, iirc, has dropped support for ---help--- (allowing
only help now). To limit churn whenever we're going to re-sync
our kconfig again, I'd like to avoid introduction of new uses
of the old form. I'm sure this could be taken care of while
committing.
Jan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] arm: Add Kconfig entry to select CONFIG_DTB_FILE
2021-03-10 9:00 ` Jan Beulich
@ 2021-03-10 11:33 ` Ian Jackson
2021-03-10 11:40 ` Julien Grall
0 siblings, 1 reply; 14+ messages in thread
From: Ian Jackson @ 2021-03-10 11:33 UTC (permalink / raw)
To: Jan Beulich
Cc: Michal Orzel, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
bertrand.marquis, xen-devel
Jan Beulich writes ("Re: [PATCH v4] arm: Add Kconfig entry to select CONFIG_DTB_FILE"):
> Nit: Linux, iirc, has dropped support for ---help--- (allowing
> only help now). To limit churn whenever we're going to re-sync
> our kconfig again, I'd like to avoid introduction of new uses
> of the old form. I'm sure this could be taken care of while
> committing.
At this stage of the release I would prefer to avoid updates made by
the committer when committing. Ie the thing that is committed should
have been previously posted and had at least some minimal time to spot
howlers.
Thanks,
Ian.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] arm: Add Kconfig entry to select CONFIG_DTB_FILE
2021-03-10 11:33 ` Ian Jackson
@ 2021-03-10 11:40 ` Julien Grall
2021-03-10 13:36 ` Ian Jackson
0 siblings, 1 reply; 14+ messages in thread
From: Julien Grall @ 2021-03-10 11:40 UTC (permalink / raw)
To: Ian Jackson, Jan Beulich
Cc: Michal Orzel, Stefano Stabellini, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Wei Liu, bertrand.marquis,
xen-devel
Hi Ian,
On 10/03/2021 11:33, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH v4] arm: Add Kconfig entry to select CONFIG_DTB_FILE"):
>> Nit: Linux, iirc, has dropped support for ---help--- (allowing
>> only help now). To limit churn whenever we're going to re-sync
>> our kconfig again, I'd like to avoid introduction of new uses
>> of the old form. I'm sure this could be taken care of while
>> committing.
>
> At this stage of the release I would prefer to avoid updates made by
> the committer when committing. Ie the thing that is committed should
> have been previously posted and had at least some minimal time to spot
> howlers.
I was under the impression that this patch would not target 4.15. At
least I didn't see any request for it.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] arm: Add Kconfig entry to select CONFIG_DTB_FILE
2021-03-10 11:40 ` Julien Grall
@ 2021-03-10 13:36 ` Ian Jackson
2021-03-11 9:17 ` Michal Orzel
0 siblings, 1 reply; 14+ messages in thread
From: Ian Jackson @ 2021-03-10 13:36 UTC (permalink / raw)
To: Julien Grall
Cc: Jan Beulich, Michal Orzel, Stefano Stabellini, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Wei Liu, bertrand.marquis,
xen-devel
Julien Grall writes ("Re: [PATCH v4] arm: Add Kconfig entry to select CONFIG_DTB_FILE"):
> I was under the impression that this patch would not target 4.15. At
> least I didn't see any request for it.
OK, good :-).
Thanks,
Ian.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] arm: Add Kconfig entry to select CONFIG_DTB_FILE
2021-03-10 13:36 ` Ian Jackson
@ 2021-03-11 9:17 ` Michal Orzel
0 siblings, 0 replies; 14+ messages in thread
From: Michal Orzel @ 2021-03-11 9:17 UTC (permalink / raw)
To: Ian Jackson, Julien Grall
Cc: Jan Beulich, Stefano Stabellini, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Wei Liu, bertrand.marquis,
xen-devel
On 10.03.2021 14:36, Ian Jackson wrote:
> Julien Grall writes ("Re: [PATCH v4] arm: Add Kconfig entry to select CONFIG_DTB_FILE"):
>> I was under the impression that this patch would not target 4.15. At
>> least I didn't see any request for it.
>
> OK, good :-).
>
> Thanks,
> Ian.
>
So do you guys want me to push v5 now with fix "help" or will the commiter do this?
Michal
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] arm: Add Kconfig entry to select CONFIG_DTB_FILE
2021-03-10 6:58 [PATCH v4] arm: Add Kconfig entry to select CONFIG_DTB_FILE Michal Orzel
2021-03-10 8:05 ` Bertrand Marquis
2021-03-10 9:00 ` Jan Beulich
@ 2021-03-11 10:34 ` Julien Grall
2021-03-11 10:41 ` Michal Orzel
2 siblings, 1 reply; 14+ messages in thread
From: Julien Grall @ 2021-03-11 10:34 UTC (permalink / raw)
To: Michal Orzel, xen-devel
Cc: Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, Ian Jackson, Jan Beulich, Wei Liu,
bertrand.marquis
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@arm.com>
> ---
> 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,
--
Julien Grall
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] arm: Add Kconfig entry to select CONFIG_DTB_FILE
2021-03-11 10:34 ` Julien Grall
@ 2021-03-11 10:41 ` Michal Orzel
2021-03-11 11:11 ` Julien Grall
0 siblings, 1 reply; 14+ messages in thread
From: Michal Orzel @ 2021-03-11 10:41 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, Ian Jackson, Jan Beulich, Wei Liu,
bertrand.marquis
Hi Julien,
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@arm.com>
>> ---
>> 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.
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 be compiled and embedded into Xen if and only if the path to dtb was given.
Cheers,
Michal
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] arm: Add Kconfig entry to select CONFIG_DTB_FILE
2021-03-11 10:41 ` Michal Orzel
@ 2021-03-11 11:11 ` Julien Grall
2021-03-11 12:39 ` Michal Orzel
0 siblings, 1 reply; 14+ messages in thread
From: Julien Grall @ 2021-03-11 11:11 UTC (permalink / raw)
To: Michal Orzel, xen-devel
Cc: Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, Ian Jackson, Jan Beulich, Wei Liu,
bertrand.marquis
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@arm.com>
>>> ---
>>> 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v4] arm: Add Kconfig entry to select CONFIG_DTB_FILE
2021-03-11 11:11 ` Julien Grall
@ 2021-03-11 12:39 ` Michal Orzel
2021-03-11 13:10 ` Jan Beulich
0 siblings, 1 reply; 14+ messages in thread
From: Michal Orzel @ 2021-03-11 12:39 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, Ian Jackson, Jan Beulich, Wei Liu,
bertrand.marquis
On 11.03.2021 12:11, Julien Grall wrote:
>
>
> 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@arm.com>
>>>> ---
>>>> 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
What do u think?
Michal
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] arm: Add Kconfig entry to select CONFIG_DTB_FILE
2021-03-11 12:39 ` Michal Orzel
@ 2021-03-11 13:10 ` Jan Beulich
2021-03-11 13:32 ` Julien Grall
0 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2021-03-11 13:10 UTC (permalink / raw)
To: Michal Orzel
Cc: Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, Ian Jackson, Wei Liu, bertrand.marquis, xen-devel,
Julien Grall
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@arm.com>
>>>>> ---
>>>>> 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.)
Jan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] arm: Add Kconfig entry to select CONFIG_DTB_FILE
2021-03-11 13:10 ` Jan Beulich
@ 2021-03-11 13:32 ` Julien Grall
2021-03-11 14:16 ` Michal Orzel
0 siblings, 1 reply; 14+ messages in thread
From: Julien Grall @ 2021-03-11 13:32 UTC (permalink / raw)
To: Jan Beulich, Michal Orzel
Cc: Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, Ian Jackson, Wei Liu, bertrand.marquis, xen-devel
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@arm.com>
>>>>>> ---
>>>>>> 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] arm: Add Kconfig entry to select CONFIG_DTB_FILE
2021-03-11 13:32 ` Julien Grall
@ 2021-03-11 14:16 ` Michal Orzel
0 siblings, 0 replies; 14+ messages in thread
From: Michal Orzel @ 2021-03-11 14:16 UTC (permalink / raw)
To: Julien Grall, Jan Beulich
Cc: Stefano Stabellini, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, Ian Jackson, Wei Liu, bertrand.marquis, xen-devel
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@arm.com>
>>>>>>> ---
>>>>>>> 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.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-03-11 14:17 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-10 6:58 [PATCH v4] arm: Add Kconfig entry to select CONFIG_DTB_FILE Michal Orzel
2021-03-10 8:05 ` Bertrand Marquis
2021-03-10 9:00 ` Jan Beulich
2021-03-10 11:33 ` Ian Jackson
2021-03-10 11:40 ` Julien Grall
2021-03-10 13:36 ` Ian Jackson
2021-03-11 9:17 ` Michal Orzel
2021-03-11 10:34 ` Julien Grall
2021-03-11 10:41 ` Michal Orzel
2021-03-11 11:11 ` Julien Grall
2021-03-11 12:39 ` Michal Orzel
2021-03-11 13:10 ` Jan Beulich
2021-03-11 13:32 ` Julien Grall
2021-03-11 14:16 ` Michal Orzel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.