[PATCH v2] arm: Add Kconfig entry to select CONFIG_DTB_FILE

[PATCH v2] arm: Add Kconfig entry to select CONFIG_DTB_FILE

[Michal Orzel]

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

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

[Jan Beulich]

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

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

[Julien Grall]

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

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

[Michal Orzel]

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

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

[Michal Orzel]

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

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

[Jan Beulich]

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

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

[Julien Grall]

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

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

[Julien Grall]

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

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

[Michal Orzel]

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

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

[Michal Orzel]

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

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

[Jan Beulich]

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

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

[Julien Grall]

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

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

[Michal Orzel]

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

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

[Jan Beulich]

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

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

[Michal Orzel]

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