diff mbox series

[4/4] kbuild: arm: Fix duplicate builds of dtbs

Message ID cb1348ece0d034ccd1db1b78524879d2b5c18eb9.1588595912.git.jan.kiszka@web.de
State New
Delegated to: Tom Rini
Headers show
Series Various build dependency fixes | expand

Commit Message

Jan Kiszka May 4, 2020, 12:38 p.m. UTC
From: Jan Kiszka <jan.kiszka@siemens.com>

Build the secured board dtbs (.dtb_HS) as part of the regular dtb build
on CONFIG_TI_SECURE_DEVICE targets. This avoids rebuilding them,
possibly overwriting artifacts that are in use, as it is done so far.

In the same run, fix needless rebuilding of the secured spl dtb.

Fixes: 508369672ca3 ("arm: mach-k3: Add secure device build support")
CC: Andrew F. Davis <afd@ti.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/arm/dts/Makefile             |  6 ++++--
 arch/arm/mach-k3/config_secure.mk | 19 ++++++++++++++-----
 2 files changed, 18 insertions(+), 7 deletions(-)

--
2.26.1

Comments

Andrew F. Davis May 8, 2020, 3:40 p.m. UTC | #1
On 5/4/20 8:38 AM, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Build the secured board dtbs (.dtb_HS) as part of the regular dtb build
> on CONFIG_TI_SECURE_DEVICE targets. This avoids rebuilding them,
> possibly overwriting artifacts that are in use, as it is done so far.
> 
> In the same run, fix needless rebuilding of the secured spl dtb.
> 
> Fixes: 508369672ca3 ("arm: mach-k3: Add secure device build support")
> CC: Andrew F. Davis <afd@ti.com>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  arch/arm/dts/Makefile             |  6 ++++--
>  arch/arm/mach-k3/config_secure.mk | 19 ++++++++++++++-----
>  2 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile
> index 2c123bd6da..b68e9c0726 100644
> --- a/arch/arm/dts/Makefile
> +++ b/arch/arm/dts/Makefile
> @@ -1,5 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0+
> 
> +include $(srctree)/arch/arm/mach-k3/config_secure.mk
> +


This is hacky, we should not in the top level dts makefile have a
dependency on a specific platform configuration file.

What about mach-omap/config_secure.mk, etc..

The negative of building dtbs twice for HS is very minor compared to the
extra complexity this patch adds. It will be much more difficult to
clean this up later.

Andrew


>  dtb-$(CONFIG_TARGET_SMARTWEB) += at91sam9260-smartweb.dtb
>  dtb-$(CONFIG_TARGET_TAURUS) += at91sam9g20-taurus.dtb
>  dtb-$(CONFIG_TARGET_CORVUS) += at91sam9g45-corvus.dtb
> @@ -927,13 +929,13 @@ dtb-$(CONFIG_TARGET_DURIAN) += phytium-durian.dtb
> 
>  dtb-$(CONFIG_TARGET_PRESIDIO_ASIC) += ca-presidio-engboard.dtb
> 
> -targets += $(dtb-y)
> +targets += $(dtb-y) $(TI_SECURE_DTBS)
> 
>  # Add any required device tree compiler flags here
>  DTC_FLAGS +=
> 
>  PHONY += dtbs
> -dtbs: $(addprefix $(obj)/, $(dtb-y))
> +dtbs: $(addprefix $(obj)/, $(dtb-y) $(TI_SECURE_DTBS))
>  	@:
> 
>  clean-files := *.dtb *.dtbo *_HS
> diff --git a/arch/arm/mach-k3/config_secure.mk b/arch/arm/mach-k3/config_secure.mk
> index 6d63c57665..d9141e10a0 100644
> --- a/arch/arm/mach-k3/config_secure.mk
> +++ b/arch/arm/mach-k3/config_secure.mk
> @@ -26,7 +26,12 @@ endif
>  $(obj)/u-boot-spl-nodtb.bin_HS: $(obj)/u-boot-spl-nodtb.bin FORCE
>  	$(call if_changed,k3secureimg)
> 
> -tispl.bin_HS: $(obj)/u-boot-spl-nodtb.bin_HS $(patsubst %,$(obj)/dts/%.dtb_HS,$(subst ",,$(CONFIG_SPL_OF_LIST))) $(SPL_ITS) FORCE
> +SPL_OF_LIST_TARGETS = $(patsubst %,dts/%.dtb,$(subst ",,$(CONFIG_SPL_OF_LIST)))
> +SPL_OF_LIST_TARGETS_HS = $(addsuffix _HS,$(SPL_OF_LIST_TARGETS))
> +
> +targets += $(SPL_OF_LIST_TARGETS) $(SPL_OF_LIST_TARGETS_HS)
> +
> +tispl.bin_HS: $(obj)/u-boot-spl-nodtb.bin_HS $(addprefix $(obj)/,$(SPL_OF_LIST_TARGETS_HS)) $(SPL_ITS) FORCE
>  	$(call if_changed,mkfitimage)
> 
>  MKIMAGEFLAGS_u-boot.img_HS = -f auto -A $(ARCH) -T firmware -C none -O u-boot \
> @@ -34,11 +39,15 @@ MKIMAGEFLAGS_u-boot.img_HS = -f auto -A $(ARCH) -T firmware -C none -O u-boot \
>  	-n "U-Boot $(UBOOTRELEASE) for $(BOARD) board" -E \
>  	$(patsubst %,-b arch/$(ARCH)/dts/%.dtb_HS,$(subst ",,$(CONFIG_OF_LIST)))
> 
> -OF_LIST_TARGETS = $(patsubst %,arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST)))
> -$(OF_LIST_TARGETS): dtbs
> -
>  u-boot-nodtb.bin_HS: u-boot-nodtb.bin FORCE
>  	$(call if_changed,k3secureimg)
> 
> -u-boot.img_HS: u-boot-nodtb.bin_HS u-boot.img $(patsubst %.dtb,%.dtb_HS,$(OF_LIST_TARGETS)) FORCE
> +u-boot.img_HS: u-boot-nodtb.bin_HS u-boot.img dtbs FORCE
>  	$(call if_changed,mkimage)
> +
> +# Used when included by arch-dts makefile
> +-include include/config/auto.conf
> +
> +ifeq ($(CONFIG_TI_SECURE_DEVICE),y)
> +TI_SECURE_DTBS = $(addsuffix _HS, $(dtb-y))
> +endif
> --
> 2.26.1
>
Jan Kiszka May 8, 2020, 3:54 p.m. UTC | #2
On 08.05.20 17:40, Andrew F. Davis wrote:
> On 5/4/20 8:38 AM, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Build the secured board dtbs (.dtb_HS) as part of the regular dtb build
>> on CONFIG_TI_SECURE_DEVICE targets. This avoids rebuilding them,
>> possibly overwriting artifacts that are in use, as it is done so far.
>>
>> In the same run, fix needless rebuilding of the secured spl dtb.
>>
>> Fixes: 508369672ca3 ("arm: mach-k3: Add secure device build support")
>> CC: Andrew F. Davis <afd@ti.com>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>   arch/arm/dts/Makefile             |  6 ++++--
>>   arch/arm/mach-k3/config_secure.mk | 19 ++++++++++++++-----
>>   2 files changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile
>> index 2c123bd6da..b68e9c0726 100644
>> --- a/arch/arm/dts/Makefile
>> +++ b/arch/arm/dts/Makefile
>> @@ -1,5 +1,7 @@
>>   # SPDX-License-Identifier: GPL-2.0+
>>
>> +include $(srctree)/arch/arm/mach-k3/config_secure.mk
>> +
>
>
> This is hacky, we should not in the top level dts makefile have a
> dependency on a specific platform configuration file.
>
> What about mach-omap/config_secure.mk, etc..

They don't need those special signing the k3 need. If they did, they
could be included here as well.

>
> The negative of building dtbs twice for HS is very minor compared to the
> extra complexity this patch adds. It will be much more difficult to
> clean this up later.

Overwriting a build artifacts that were already completed and may just
be in use while doing so is broken. This must be fixed.

As I wrote, I will add support for injecting public keys into the dtbs,
and that also revealed this issue.

Better suggestions welcome.

Jan
Andrew F. Davis May 18, 2020, 4:26 p.m. UTC | #3
On 5/8/20 11:54 AM, Jan Kiszka wrote:
> On 08.05.20 17:40, Andrew F. Davis wrote:
>> On 5/4/20 8:38 AM, Jan Kiszka wrote:
>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> Build the secured board dtbs (.dtb_HS) as part of the regular dtb build
>>> on CONFIG_TI_SECURE_DEVICE targets. This avoids rebuilding them,
>>> possibly overwriting artifacts that are in use, as it is done so far.
>>>
>>> In the same run, fix needless rebuilding of the secured spl dtb.
>>>
>>> Fixes: 508369672ca3 ("arm: mach-k3: Add secure device build support")
>>> CC: Andrew F. Davis <afd@ti.com>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>>   arch/arm/dts/Makefile             |  6 ++++--
>>>   arch/arm/mach-k3/config_secure.mk | 19 ++++++++++++++-----
>>>   2 files changed, 18 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile
>>> index 2c123bd6da..b68e9c0726 100644
>>> --- a/arch/arm/dts/Makefile
>>> +++ b/arch/arm/dts/Makefile
>>> @@ -1,5 +1,7 @@
>>>   # SPDX-License-Identifier: GPL-2.0+
>>>
>>> +include $(srctree)/arch/arm/mach-k3/config_secure.mk
>>> +
>>
>>
>> This is hacky, we should not in the top level dts makefile have a
>> dependency on a specific platform configuration file.
>>
>> What about mach-omap/config_secure.mk, etc..
> 
> They don't need those special signing the k3 need. If they did, they
> could be included here as well.
> 


I did everything the same in that file, so I don't see why it wouldn't
need the same treatment. But my point is that this will keep growing,
the fixes should be local to the mk files or everyone who does something
similar will need to add here.


>>
>> The negative of building dtbs twice for HS is very minor compared to the
>> extra complexity this patch adds. It will be much more difficult to
>> clean this up later.
> 
> Overwriting a build artifacts that were already completed and may just
> be in use while doing so is broken. This must be fixed.
> 


Absolutely agree it needs fixed, just not sure this just doesn't add to
the makefile complexity in the wrong way.


> As I wrote, I will add support for injecting public keys into the dtbs,
> and that also revealed this issue.
> 


If I understand right, you are adding the keys into the dtb then signing
them? Why not add them to the dts first, then nothing would need to
change with multiple builds of the dtbs.

Andrew


> Better suggestions welcome.
> 
> Jan
Jan Kiszka May 18, 2020, 5:34 p.m. UTC | #4
On 18.05.20 18:26, Andrew F. Davis wrote:
> On 5/8/20 11:54 AM, Jan Kiszka wrote:
>> On 08.05.20 17:40, Andrew F. Davis wrote:
>>> On 5/4/20 8:38 AM, Jan Kiszka wrote:
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> Build the secured board dtbs (.dtb_HS) as part of the regular dtb build
>>>> on CONFIG_TI_SECURE_DEVICE targets. This avoids rebuilding them,
>>>> possibly overwriting artifacts that are in use, as it is done so far.
>>>>
>>>> In the same run, fix needless rebuilding of the secured spl dtb.
>>>>
>>>> Fixes: 508369672ca3 ("arm: mach-k3: Add secure device build support")
>>>> CC: Andrew F. Davis <afd@ti.com>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> ---
>>>>    arch/arm/dts/Makefile             |  6 ++++--
>>>>    arch/arm/mach-k3/config_secure.mk | 19 ++++++++++++++-----
>>>>    2 files changed, 18 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile
>>>> index 2c123bd6da..b68e9c0726 100644
>>>> --- a/arch/arm/dts/Makefile
>>>> +++ b/arch/arm/dts/Makefile
>>>> @@ -1,5 +1,7 @@
>>>>    # SPDX-License-Identifier: GPL-2.0+
>>>>
>>>> +include $(srctree)/arch/arm/mach-k3/config_secure.mk
>>>> +
>>>
>>>
>>> This is hacky, we should not in the top level dts makefile have a
>>> dependency on a specific platform configuration file.
>>>
>>> What about mach-omap/config_secure.mk, etc..
>>
>> They don't need those special signing the k3 need. If they did, they
>> could be included here as well.
>>
>
>
> I did everything the same in that file, so I don't see why it wouldn't
> need the same treatment. But my point is that this will keep growing,
> the fixes should be local to the mk files or everyone who does something
> similar will need to add here.
>

The problem is that dtbs are built in a separate make session. If you
want to address specific dtbs in a rule, you must do that in that
session, not outside like you did. That applies to signing for TI K3,
just like it would for bringing in public keys as discussed below.

I think you mentioned offlist that eliminating this nesting would help,
but I do not yet see if that can be easily done.

>
>>>
>>> The negative of building dtbs twice for HS is very minor compared to the
>>> extra complexity this patch adds. It will be much more difficult to
>>> clean this up later.
>>
>> Overwriting a build artifacts that were already completed and may just
>> be in use while doing so is broken. This must be fixed.
>>
>
>
> Absolutely agree it needs fixed, just not sure this just doesn't add to
> the makefile complexity in the wrong way.
>

As I said, I'm open for other approaches that resolve this.

>
>> As I wrote, I will add support for injecting public keys into the dtbs,
>> and that also revealed this issue.
>>
>
>
> If I understand right, you are adding the keys into the dtb then signing
> them? Why not add them to the dts first, then nothing would need to
> change with multiple builds of the dtbs.

You sign the chain from right to left, while loading is left to right:
the right-side image is signed while injecting the necessary keys/certs
into the left-side image that loads it. And then that is signed as well,
in the end often with a hardware-specific mechanism (unfortunately).

But dtbs are hardware-specific, keys are deployment-specific. That's why
they are injected into existing dtbs, rather than asking the user to
patch them into dts files in the source tree. Just today I ran into
another piece of code that models this signing and key injection outside
of U-Boot, with a lot of knowledge about the particular target. It would
be way nicer to solve that generically.

Jan
diff mbox series

Patch

diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile
index 2c123bd6da..b68e9c0726 100644
--- a/arch/arm/dts/Makefile
+++ b/arch/arm/dts/Makefile
@@ -1,5 +1,7 @@ 
 # SPDX-License-Identifier: GPL-2.0+

+include $(srctree)/arch/arm/mach-k3/config_secure.mk
+
 dtb-$(CONFIG_TARGET_SMARTWEB) += at91sam9260-smartweb.dtb
 dtb-$(CONFIG_TARGET_TAURUS) += at91sam9g20-taurus.dtb
 dtb-$(CONFIG_TARGET_CORVUS) += at91sam9g45-corvus.dtb
@@ -927,13 +929,13 @@  dtb-$(CONFIG_TARGET_DURIAN) += phytium-durian.dtb

 dtb-$(CONFIG_TARGET_PRESIDIO_ASIC) += ca-presidio-engboard.dtb

-targets += $(dtb-y)
+targets += $(dtb-y) $(TI_SECURE_DTBS)

 # Add any required device tree compiler flags here
 DTC_FLAGS +=

 PHONY += dtbs
-dtbs: $(addprefix $(obj)/, $(dtb-y))
+dtbs: $(addprefix $(obj)/, $(dtb-y) $(TI_SECURE_DTBS))
 	@:

 clean-files := *.dtb *.dtbo *_HS
diff --git a/arch/arm/mach-k3/config_secure.mk b/arch/arm/mach-k3/config_secure.mk
index 6d63c57665..d9141e10a0 100644
--- a/arch/arm/mach-k3/config_secure.mk
+++ b/arch/arm/mach-k3/config_secure.mk
@@ -26,7 +26,12 @@  endif
 $(obj)/u-boot-spl-nodtb.bin_HS: $(obj)/u-boot-spl-nodtb.bin FORCE
 	$(call if_changed,k3secureimg)

-tispl.bin_HS: $(obj)/u-boot-spl-nodtb.bin_HS $(patsubst %,$(obj)/dts/%.dtb_HS,$(subst ",,$(CONFIG_SPL_OF_LIST))) $(SPL_ITS) FORCE
+SPL_OF_LIST_TARGETS = $(patsubst %,dts/%.dtb,$(subst ",,$(CONFIG_SPL_OF_LIST)))
+SPL_OF_LIST_TARGETS_HS = $(addsuffix _HS,$(SPL_OF_LIST_TARGETS))
+
+targets += $(SPL_OF_LIST_TARGETS) $(SPL_OF_LIST_TARGETS_HS)
+
+tispl.bin_HS: $(obj)/u-boot-spl-nodtb.bin_HS $(addprefix $(obj)/,$(SPL_OF_LIST_TARGETS_HS)) $(SPL_ITS) FORCE
 	$(call if_changed,mkfitimage)

 MKIMAGEFLAGS_u-boot.img_HS = -f auto -A $(ARCH) -T firmware -C none -O u-boot \
@@ -34,11 +39,15 @@  MKIMAGEFLAGS_u-boot.img_HS = -f auto -A $(ARCH) -T firmware -C none -O u-boot \
 	-n "U-Boot $(UBOOTRELEASE) for $(BOARD) board" -E \
 	$(patsubst %,-b arch/$(ARCH)/dts/%.dtb_HS,$(subst ",,$(CONFIG_OF_LIST)))

-OF_LIST_TARGETS = $(patsubst %,arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST)))
-$(OF_LIST_TARGETS): dtbs
-
 u-boot-nodtb.bin_HS: u-boot-nodtb.bin FORCE
 	$(call if_changed,k3secureimg)

-u-boot.img_HS: u-boot-nodtb.bin_HS u-boot.img $(patsubst %.dtb,%.dtb_HS,$(OF_LIST_TARGETS)) FORCE
+u-boot.img_HS: u-boot-nodtb.bin_HS u-boot.img dtbs FORCE
 	$(call if_changed,mkimage)
+
+# Used when included by arch-dts makefile
+-include include/config/auto.conf
+
+ifeq ($(CONFIG_TI_SECURE_DEVICE),y)
+TI_SECURE_DTBS = $(addsuffix _HS, $(dtb-y))
+endif