xen/arch/arm/Makefile | 4 +--- xen/common/Kconfig | 8 ++++++++ 2 files changed, 9 insertions(+), 3 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.
Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
xen/arch/arm/Makefile | 4 +---
xen/common/Kconfig | 8 ++++++++
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 16e6523e2c..0f3e99d075 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -68,7 +68,7 @@ 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
@@ -137,8 +137,6 @@ asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c
xen.lds: xen.lds.S
$(CPP) -P $(a_flags) -MQ $@ -o $@ $<
-dtb.o: $(CONFIG_DTB_FILE)
-
.PHONY: clean
clean::
rm -f asm-offsets.s xen.lds
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index eb953d171e..a4c8d09edf 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 ARM
+ ---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
Hi, On 08/03/2021 13:59, 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. > > Signed-off-by: Michal Orzel <michal.orzel@arm.com> > --- > xen/arch/arm/Makefile | 4 +--- > xen/common/Kconfig | 8 ++++++++ > 2 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile > index 16e6523e2c..0f3e99d075 100644 > --- a/xen/arch/arm/Makefile > +++ b/xen/arch/arm/Makefile > @@ -68,7 +68,7 @@ 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 > @@ -137,8 +137,6 @@ asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c > xen.lds: xen.lds.S > $(CPP) -P $(a_flags) -MQ $@ -o $@ $< > > -dtb.o: $(CONFIG_DTB_FILE) > - Why is this dropped? > .PHONY: clean > clean:: > rm -f asm-offsets.s xen.lds > diff --git a/xen/common/Kconfig b/xen/common/Kconfig > index eb953d171e..a4c8d09edf 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 May I ask why is this add in common/Kconfig rather than arm/Kconfig? > + string "Absolute path to device tree blob" > + depends on ARM If this stay in common Kconfig, shouldn't this be gated with 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 > Cheers, -- Julien Grall
Hi Julien, On 08.03.2021 15:31, Julien Grall wrote: > Hi, > > On 08/03/2021 13:59, 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. >> >> Signed-off-by: Michal Orzel <michal.orzel@arm.com> >> --- >> xen/arch/arm/Makefile | 4 +--- >> xen/common/Kconfig | 8 ++++++++ >> 2 files changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile >> index 16e6523e2c..0f3e99d075 100644 >> --- a/xen/arch/arm/Makefile >> +++ b/xen/arch/arm/Makefile >> @@ -68,7 +68,7 @@ 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 >> @@ -137,8 +137,6 @@ asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c >> xen.lds: xen.lds.S >> $(CPP) -P $(a_flags) -MQ $@ -o $@ $< >> -dtb.o: $(CONFIG_DTB_FILE) >> - > > Why is this dropped? 1)This line is not needed as it has no impact on creating dtb.o 2)It causes the build failure once CONFIG_DTB_FILE option is in the Kconfig as string within quotes. > >> .PHONY: clean >> clean:: >> rm -f asm-offsets.s xen.lds >> diff --git a/xen/common/Kconfig b/xen/common/Kconfig >> index eb953d171e..a4c8d09edf 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 > > May I ask why is this add in common/Kconfig rather than arm/Kconfig? > I wanted to have it in common features rather than architecture features. Maybe it could be later on used by other architectures. >> + string "Absolute path to device tree blob" >> + depends on ARM > > If this stay in common Kconfig, shouldn't this be gated with HAS_DEVICE_TREE? No it shouldn't as CONFIG_DTB_FILE depends on CONFIG_ARM which selects CONFIG_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 >> > > Cheers, > Cheers, Michal
On 09/03/2021 07:34, Michal Orzel wrote: > Hi Julien, Hi, > On 08.03.2021 15:31, Julien Grall wrote: >> Hi, >> >> On 08/03/2021 13:59, 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. >>> >>> Signed-off-by: Michal Orzel <michal.orzel@arm.com> >>> --- >>> xen/arch/arm/Makefile | 4 +--- >>> xen/common/Kconfig | 8 ++++++++ >>> 2 files changed, 9 insertions(+), 3 deletions(-) >>> >>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile >>> index 16e6523e2c..0f3e99d075 100644 >>> --- a/xen/arch/arm/Makefile >>> +++ b/xen/arch/arm/Makefile >>> @@ -68,7 +68,7 @@ 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 >>> @@ -137,8 +137,6 @@ asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c >>> xen.lds: xen.lds.S >>> $(CPP) -P $(a_flags) -MQ $@ -o $@ $< >>> -dtb.o: $(CONFIG_DTB_FILE) >>> - >> >> Why is this dropped? > 1)This line is not needed as it has no impact on creating dtb.o > 2)It causes the build failure once CONFIG_DTB_FILE option is in the Kconfig as string within quotes. Because of 1), this should have ideally be part of a separate patch. But I am OK to keep it in this patch so long it is explained in the commit message. >> >>> .PHONY: clean >>> clean:: >>> rm -f asm-offsets.s xen.lds >>> diff --git a/xen/common/Kconfig b/xen/common/Kconfig >>> index eb953d171e..a4c8d09edf 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 >> >> May I ask why is this add in common/Kconfig rather than arm/Kconfig? >> > I wanted to have it in common features rather than architecture features. > Maybe it could be later on used by other architectures. The same can be argued for a few CONFIG in arch/.../Kconfig. What I want to avoid is spreading depends on <ARCH> in the common/Kconfig. >>> + string "Absolute path to device tree blob" >>> + depends on ARM >> >> If this stay in common Kconfig, shouldn't this be gated with HAS_DEVICE_TREE? > No it shouldn't as CONFIG_DTB_FILE depends on CONFIG_ARM which selects CONFIG_HAS_DEVICE_TREE I think you misunderstood my point, what I suggested is replacing "depends on Arm" by "depends on HAS_DEVICE_TREE". This is for two reasons: 1) This avoids spreading depend on <ARCH> in common/kconfig 2) This avoids the assumption that Arm is always using DT If you would rather not use "depends on HAS_DEVICE_TREE", then I think this config should go in arch/arm/Kconfig until we see another users. Cheers, -- Julien Grall
Hi, On 09.03.2021 11:20, Julien Grall wrote: > > > On 09/03/2021 07:34, Michal Orzel wrote: >> Hi Julien, > > Hi, > >> On 08.03.2021 15:31, Julien Grall wrote: >>> Hi, >>> >>> On 08/03/2021 13:59, 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. >>>> >>>> Signed-off-by: Michal Orzel <michal.orzel@arm.com> >>>> --- >>>> xen/arch/arm/Makefile | 4 +--- >>>> xen/common/Kconfig | 8 ++++++++ >>>> 2 files changed, 9 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile >>>> index 16e6523e2c..0f3e99d075 100644 >>>> --- a/xen/arch/arm/Makefile >>>> +++ b/xen/arch/arm/Makefile >>>> @@ -68,7 +68,7 @@ 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 >>>> @@ -137,8 +137,6 @@ asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c >>>> xen.lds: xen.lds.S >>>> $(CPP) -P $(a_flags) -MQ $@ -o $@ $< >>>> -dtb.o: $(CONFIG_DTB_FILE) >>>> - >>> >>> Why is this dropped? >> 1)This line is not needed as it has no impact on creating dtb.o >> 2)It causes the build failure once CONFIG_DTB_FILE option is in the Kconfig as string within quotes. > > Because of 1), this should have ideally be part of a separate patch. But I am OK to keep it in this patch so long it is explained in the commit message. Ok I will explain it in the commit msg in v3 > >>> >>>> .PHONY: clean >>>> clean:: >>>> rm -f asm-offsets.s xen.lds >>>> diff --git a/xen/common/Kconfig b/xen/common/Kconfig >>>> index eb953d171e..a4c8d09edf 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 >>> >>> May I ask why is this add in common/Kconfig rather than arm/Kconfig? >>> >> I wanted to have it in common features rather than architecture features. >> Maybe it could be later on used by other architectures. > > The same can be argued for a few CONFIG in arch/.../Kconfig. What I want to avoid is spreading depends on <ARCH> in the common/Kconfig. > >>>> + string "Absolute path to device tree blob" >>>> + depends on ARM >>> >>> If this stay in common Kconfig, shouldn't this be gated with HAS_DEVICE_TREE? >> No it shouldn't as CONFIG_DTB_FILE depends on CONFIG_ARM which selects CONFIG_HAS_DEVICE_TREE > I think you misunderstood my point, what I suggested is replacing "depends on Arm" by "depends on HAS_DEVICE_TREE". > > This is for two reasons: > 1) This avoids spreading depend on <ARCH> in common/kconfig > 2) This avoids the assumption that Arm is always using DT > > If you would rather not use "depends on HAS_DEVICE_TREE", then I think this config should go in arch/arm/Kconfig until we see another users. > Ok I will keep it in common/Kconfig but switch to depends on HAS_DEVICE_TREE > Cheers, > Cheers
On 09.03.2021 11:20, Julien Grall wrote: > On 09/03/2021 07:34, Michal Orzel wrote: >> On 08.03.2021 15:31, Julien Grall wrote: >>> On 08/03/2021 13:59, Michal Orzel wrote: >>>> --- a/xen/arch/arm/Makefile >>>> +++ b/xen/arch/arm/Makefile >>>> @@ -68,7 +68,7 @@ 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 >>>> @@ -137,8 +137,6 @@ asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c >>>> xen.lds: xen.lds.S >>>> $(CPP) -P $(a_flags) -MQ $@ -o $@ $< >>>> -dtb.o: $(CONFIG_DTB_FILE) >>>> - >>> >>> Why is this dropped? >> 1)This line is not needed as it has no impact on creating dtb.o >> 2)It causes the build failure once CONFIG_DTB_FILE option is in the Kconfig as string within quotes. > > Because of 1), this should have ideally be part of a separate patch. But > I am OK to keep it in this patch so long it is explained in the commit > message. Wasn't the intention to have dtb.o re-compiled when the blob has changed? This would be lost with the removal of this line. The quotes would need stripping now, of course. Jan
On 09/03/2021 11:07, Jan Beulich wrote: > On 09.03.2021 11:20, Julien Grall wrote: >> On 09/03/2021 07:34, Michal Orzel wrote: >>> On 08.03.2021 15:31, Julien Grall wrote: >>>> On 08/03/2021 13:59, Michal Orzel wrote: >>>>> --- a/xen/arch/arm/Makefile >>>>> +++ b/xen/arch/arm/Makefile >>>>> @@ -68,7 +68,7 @@ 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 >>>>> @@ -137,8 +137,6 @@ asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c >>>>> xen.lds: xen.lds.S >>>>> $(CPP) -P $(a_flags) -MQ $@ -o $@ $< >>>>> -dtb.o: $(CONFIG_DTB_FILE) >>>>> - >>>> >>>> Why is this dropped? >>> 1)This line is not needed as it has no impact on creating dtb.o >>> 2)It causes the build failure once CONFIG_DTB_FILE option is in the Kconfig as string within quotes. >> >> Because of 1), this should have ideally be part of a separate patch. But >> I am OK to keep it in this patch so long it is explained in the commit >> message. > > Wasn't the intention to have dtb.o re-compiled when the blob > has changed? This would be lost with the removal of this line. Ah yes. I was only thinking about a name change (this would be caught via the update of the config header) and not a file update. -- Julien Grall
On 09.03.2021 14:32, Julien Grall wrote: > > > On 09/03/2021 11:07, Jan Beulich wrote: >> On 09.03.2021 11:20, Julien Grall wrote: >>> On 09/03/2021 07:34, Michal Orzel wrote: >>>> On 08.03.2021 15:31, Julien Grall wrote: >>>>> On 08/03/2021 13:59, Michal Orzel wrote: >>>>>> --- a/xen/arch/arm/Makefile >>>>>> +++ b/xen/arch/arm/Makefile >>>>>> @@ -68,7 +68,7 @@ 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 >>>>>> @@ -137,8 +137,6 @@ asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c >>>>>> xen.lds: xen.lds.S >>>>>> $(CPP) -P $(a_flags) -MQ $@ -o $@ $< >>>>>> -dtb.o: $(CONFIG_DTB_FILE) >>>>>> - >>>>> >>>>> Why is this dropped? >>>> 1)This line is not needed as it has no impact on creating dtb.o >>>> 2)It causes the build failure once CONFIG_DTB_FILE option is in the Kconfig as string within quotes. >>> >>> Because of 1), this should have ideally be part of a separate patch. But >>> I am OK to keep it in this patch so long it is explained in the commit >>> message. >> >> Wasn't the intention to have dtb.o re-compiled when the blob >> has changed? This would be lost with the removal of this line. > > Ah yes. I was only thinking about a name change (this would be caught via the update of the config header) and not a file update. > I already pushed v3 but I agree. Something like this would do the job: dtb.o: $(subst $\",,$(CONFIG_DTB_FILE)) to remove quotes
On 09.03.2021 14:55, Michal Orzel wrote: > > > On 09.03.2021 14:32, Julien Grall wrote: >> >> >> On 09/03/2021 11:07, Jan Beulich wrote: >>> On 09.03.2021 11:20, Julien Grall wrote: >>>> On 09/03/2021 07:34, Michal Orzel wrote: >>>>> On 08.03.2021 15:31, Julien Grall wrote: >>>>>> On 08/03/2021 13:59, Michal Orzel wrote: >>>>>>> --- a/xen/arch/arm/Makefile >>>>>>> +++ b/xen/arch/arm/Makefile >>>>>>> @@ -68,7 +68,7 @@ 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 >>>>>>> @@ -137,8 +137,6 @@ asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c >>>>>>> xen.lds: xen.lds.S >>>>>>> $(CPP) -P $(a_flags) -MQ $@ -o $@ $< >>>>>>> -dtb.o: $(CONFIG_DTB_FILE) >>>>>>> - >>>>>> >>>>>> Why is this dropped? >>>>> 1)This line is not needed as it has no impact on creating dtb.o >>>>> 2)It causes the build failure once CONFIG_DTB_FILE option is in the Kconfig as string within quotes. >>>> >>>> Because of 1), this should have ideally be part of a separate patch. But >>>> I am OK to keep it in this patch so long it is explained in the commit >>>> message. >>> >>> Wasn't the intention to have dtb.o re-compiled when the blob >>> has changed? This would be lost with the removal of this line. >> >> Ah yes. I was only thinking about a name change (this would be caught via the update of the config header) and not a file update. >> > I already pushed v3 but I agree. Something like this would do the job: > dtb.o: $(subst $\",,$(CONFIG_DTB_FILE)) > to remove quotes Besides struggling with the $\", may I suggest $(patsubst "%",%,$(CONFIG_DTB_FILE))? If the double quote needs special treatment, I think it wants to be done via an abstraction similar to squote (near the top of ./Config.mk). Jan
On 09.03.2021 15:18, Jan Beulich wrote: > On 09.03.2021 14:55, Michal Orzel wrote: >> >> >> On 09.03.2021 14:32, Julien Grall wrote: >>> >>> >>> On 09/03/2021 11:07, Jan Beulich wrote: >>>> On 09.03.2021 11:20, Julien Grall wrote: >>>>> On 09/03/2021 07:34, Michal Orzel wrote: >>>>>> On 08.03.2021 15:31, Julien Grall wrote: >>>>>>> On 08/03/2021 13:59, Michal Orzel wrote: >>>>>>>> --- a/xen/arch/arm/Makefile >>>>>>>> +++ b/xen/arch/arm/Makefile >>>>>>>> @@ -68,7 +68,7 @@ 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 >>>>>>>> @@ -137,8 +137,6 @@ asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c >>>>>>>> xen.lds: xen.lds.S >>>>>>>> $(CPP) -P $(a_flags) -MQ $@ -o $@ $< >>>>>>>> -dtb.o: $(CONFIG_DTB_FILE) >>>>>>>> - >>>>>>> >>>>>>> Why is this dropped? >>>>>> 1)This line is not needed as it has no impact on creating dtb.o >>>>>> 2)It causes the build failure once CONFIG_DTB_FILE option is in the Kconfig as string within quotes. >>>>> >>>>> Because of 1), this should have ideally be part of a separate patch. But >>>>> I am OK to keep it in this patch so long it is explained in the commit >>>>> message. >>>> >>>> Wasn't the intention to have dtb.o re-compiled when the blob >>>> has changed? This would be lost with the removal of this line. >>> >>> Ah yes. I was only thinking about a name change (this would be caught via the update of the config header) and not a file update. >>> >> I already pushed v3 but I agree. Something like this would do the job: >> dtb.o: $(subst $\",,$(CONFIG_DTB_FILE)) >> to remove quotes > > Besides struggling with the $\", may I suggest > $(patsubst "%",%,$(CONFIG_DTB_FILE))? If the double quote needs > special treatment, I think it wants to be done via an abstraction > similar to squote (near the top of ./Config.mk). > The line $(patsubst "%",%,$(CONFIG_DTB_FILE)) is sufficient. I checked and dtb.o is recompiled when the blob is changed. I will fix it in v4 > Jan > Michal
On 08.03.2021 14:59, Michal Orzel wrote: > --- a/xen/arch/arm/Makefile > +++ b/xen/arch/arm/Makefile > @@ -68,7 +68,7 @@ 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 Right now what I have for my Arm test builds is an unquoted string in ./.config, e.g.: CONFIG_DTB_FILE:=/usr/local/arm-linux-gnueabi/vexpress-v2p-aem-v7a.dtb While I suppose you've tested that the resulting quoting is still okay, to reduce confusion perhaps the AFLAGS-y line would better be changed to AFLAGS-y += '-DCONFIG_DTB_FILE=$(CONFIG_DTB_FILE)' at the same time? Jan
On 08.03.2021 15:26, Jan Beulich wrote: > On 08.03.2021 14:59, Michal Orzel wrote: >> --- a/xen/arch/arm/Makefile >> +++ b/xen/arch/arm/Makefile >> @@ -68,7 +68,7 @@ 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 > > Right now what I have for my Arm test builds is an unquoted > string in ./.config, e.g.: > > CONFIG_DTB_FILE:=/usr/local/arm-linux-gnueabi/vexpress-v2p-aem-v7a.dtb > > While I suppose you've tested that the resulting quoting is still > okay, to reduce confusion perhaps the AFLAGS-y line would better > be changed to > > AFLAGS-y += '-DCONFIG_DTB_FILE=$(CONFIG_DTB_FILE)' It is tested. I can change it to: AFLAGS-y += -DCONFIG_DTB_FILE='$(CONFIG_DTB_FILE)' as the -DCONFIG_DTB_FILE= does not need to be within quotes > > at the same time? > > Jan > Michal
On 09.03.2021 08:28, Michal Orzel wrote: > > > On 08.03.2021 15:26, Jan Beulich wrote: >> On 08.03.2021 14:59, Michal Orzel wrote: >>> --- a/xen/arch/arm/Makefile >>> +++ b/xen/arch/arm/Makefile >>> @@ -68,7 +68,7 @@ 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 >> >> Right now what I have for my Arm test builds is an unquoted >> string in ./.config, e.g.: >> >> CONFIG_DTB_FILE:=/usr/local/arm-linux-gnueabi/vexpress-v2p-aem-v7a.dtb >> >> While I suppose you've tested that the resulting quoting is still >> okay, to reduce confusion perhaps the AFLAGS-y line would better >> be changed to >> >> AFLAGS-y += '-DCONFIG_DTB_FILE=$(CONFIG_DTB_FILE)' > > It is tested. I can change it to: > AFLAGS-y += -DCONFIG_DTB_FILE='$(CONFIG_DTB_FILE)' > as the -DCONFIG_DTB_FILE= does not need to be within quotes Either way would seem better to me than the current use of escaped double quotes. (Personally I prefer to quote entire arguments, but that's clearly a taste aspect.) Jan
On 09/03/2021 07:28, Michal Orzel wrote: > > > On 08.03.2021 15:26, Jan Beulich wrote: >> On 08.03.2021 14:59, Michal Orzel wrote: >>> --- a/xen/arch/arm/Makefile >>> +++ b/xen/arch/arm/Makefile >>> @@ -68,7 +68,7 @@ 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 >> >> Right now what I have for my Arm test builds is an unquoted >> string in ./.config, e.g.: >> >> CONFIG_DTB_FILE:=/usr/local/arm-linux-gnueabi/vexpress-v2p-aem-v7a.dtb >> >> While I suppose you've tested that the resulting quoting is still >> okay, to reduce confusion perhaps the AFLAGS-y line would better >> be changed to >> >> AFLAGS-y += '-DCONFIG_DTB_FILE=$(CONFIG_DTB_FILE)' > > It is tested. I can change it to: > AFLAGS-y += -DCONFIG_DTB_FILE='$(CONFIG_DTB_FILE)' > as the -DCONFIG_DTB_FILE= does not need to be within quotes May I ask why do we need to keep the AFLAGS-y? Wouldn't Kconfig define it in an header with all the other config option? Cheers, -- Julien Grall
On 09.03.2021 11:22, Julien Grall wrote: > > > On 09/03/2021 07:28, Michal Orzel wrote: >> >> >> On 08.03.2021 15:26, Jan Beulich wrote: >>> On 08.03.2021 14:59, Michal Orzel wrote: >>>> --- a/xen/arch/arm/Makefile >>>> +++ b/xen/arch/arm/Makefile >>>> @@ -68,7 +68,7 @@ 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 >>> >>> Right now what I have for my Arm test builds is an unquoted >>> string in ./.config, e.g.: >>> >>> CONFIG_DTB_FILE:=/usr/local/arm-linux-gnueabi/vexpress-v2p-aem-v7a.dtb >>> >>> While I suppose you've tested that the resulting quoting is still >>> okay, to reduce confusion perhaps the AFLAGS-y line would better >>> be changed to >>> >>> AFLAGS-y += '-DCONFIG_DTB_FILE=$(CONFIG_DTB_FILE)' >> >> It is tested. I can change it to: >> AFLAGS-y += -DCONFIG_DTB_FILE='$(CONFIG_DTB_FILE)' >> as the -DCONFIG_DTB_FILE= does not need to be within quotes > > May I ask why do we need to keep the AFLAGS-y? Wouldn't Kconfig define it in an header with all the other config option? > It is interesting. I did not investigate it when creating a patch. I just tested and indeed we can get rid of AFLAGS-y line. Will do in v3 > Cheers, > Cheers
© 2016 - 2024 Red Hat, Inc.