[PATCH] fit: Load DTO into temporary buffer and ignore load address

[PATCH] fit: Load DTO into temporary buffer and ignore load address

[Marek Vasut]

The current fitImage DTO implementation expects each fitImage image
subnode containing DTO to have 'load' property, pointing somewhere
into memory where the DTO will be loaded. The address in the 'load'
property must be different then the base DT load address and there
must be sufficient amount of space between those two addresses.
Selecting and using such hard-coded addresses is fragile, error
prone and difficult to port even across devices with the same SoC
and different DRAM sizes.

The DTO cannot be applied in-place because fdt_overlay_apply_verbose()
modifies the DTO when applying it onto the base DT, so if the DTO was
used in place within the fitImage, call to fdt_overlay_apply_verbose()
would corrupt the fitImage.

Instead of copying the DTO to a specific hard-coded load address,
allocate a buffer, copy the DTO into that buffer, apply the DTO onto
the base DT, and free the buffer.

The upside of this approach is that it is no longer necessary to
select and hard-code specific DTO load address into the DTO. The
slight downside is the new malloc()/free() overhead for each DTO,
but that is negligible (*).

(*) on iMX8MM/MN and STM32MP1

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@konsulko.com>
---
 common/image-fit.c | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/common/image-fit.c b/common/image-fit.c
index e614643fe39..1e0aabddec6 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -2267,10 +2267,10 @@ int boot_get_fdt_fit(bootm_headers_t *images, ulong addr,
 	ulong load, len;
 #ifdef CONFIG_OF_LIBFDT_OVERLAY
 	ulong image_start, image_end;
-	ulong ovload, ovlen;
+	ulong ovload, ovlen, ovcopylen;
 	const char *uconfig;
 	const char *uname;
-	void *base, *ov;
+	void *base, *ov, *ovcopy = NULL;
 	int i, err, noffset, ov_noffset;
 #endif
 
@@ -2360,7 +2360,7 @@ int boot_get_fdt_fit(bootm_headers_t *images, ulong addr,
 			addr, &uname, &uconfig,
 			arch, IH_TYPE_FLATDT,
 			BOOTSTAGE_ID_FIT_FDT_START,
-			FIT_LOAD_REQUIRED, &ovload, &ovlen);
+			FIT_LOAD_IGNORED, &ovload, &ovlen);
 		if (ov_noffset < 0) {
 			printf("load of %s failed\n", uname);
 			continue;
@@ -2369,6 +2369,21 @@ int boot_get_fdt_fit(bootm_headers_t *images, ulong addr,
 				uname, ovload, ovlen);
 		ov = map_sysmem(ovload, ovlen);
 
+		ovcopylen = ALIGN(fdt_totalsize(ov), SZ_4K);
+		ovcopy = malloc(ovcopylen);
+		if (!ovcopy) {
+			printf("failed to duplicate DTO before application\n");
+			fdt_noffset = -ENOMEM;
+			goto out;
+		}
+
+		err = fdt_open_into(ov, ovcopy, ovcopylen);
+		if (err < 0) {
+			printf("failed on fdt_open_into for DTO\n");
+			fdt_noffset = err;
+			goto out;
+		}
+
 		base = map_sysmem(load, len + ovlen);
 		err = fdt_open_into(base, base, len + ovlen);
 		if (err < 0) {
@@ -2376,14 +2391,18 @@ int boot_get_fdt_fit(bootm_headers_t *images, ulong addr,
 			fdt_noffset = err;
 			goto out;
 		}
+
 		/* the verbose method prints out messages on error */
-		err = fdt_overlay_apply_verbose(base, ov);
+		err = fdt_overlay_apply_verbose(base, ovcopy);
 		if (err < 0) {
 			fdt_noffset = err;
 			goto out;
 		}
 		fdt_pack(base);
 		len = fdt_totalsize(base);
+
+		free(ovcopy);
+		ovcopy = NULL;
 	}
 #else
 	printf("config with overlays but CONFIG_OF_LIBFDT_OVERLAY not set\n");
@@ -2400,6 +2419,10 @@ out:
 	if (fit_uname_configp)
 		*fit_uname_configp = fit_uname_config;
 
+#ifdef CONFIG_OF_LIBFDT_OVERLAY
+	if (ovcopy)
+		free(ovcopy);
+#endif
 	if (fit_uname_config_copy)
 		free(fit_uname_config_copy);
 	return fdt_noffset;
-- 
2.30.2

Re: [PATCH] fit: Load DTO into temporary buffer and ignore load address

[Simon Glass]

Hi,

On Thu, 10 Jun 2021 at 20:10, Marek Vasut <marex@denx.de> wrote:
>
> The current fitImage DTO implementation expects each fitImage image
> subnode containing DTO to have 'load' property, pointing somewhere
> into memory where the DTO will be loaded. The address in the 'load'
> property must be different then the base DT load address and there
> must be sufficient amount of space between those two addresses.
> Selecting and using such hard-coded addresses is fragile, error
> prone and difficult to port even across devices with the same SoC
> and different DRAM sizes.
>
> The DTO cannot be applied in-place because fdt_overlay_apply_verbose()
> modifies the DTO when applying it onto the base DT, so if the DTO was
> used in place within the fitImage, call to fdt_overlay_apply_verbose()
> would corrupt the fitImage.
>
> Instead of copying the DTO to a specific hard-coded load address,
> allocate a buffer, copy the DTO into that buffer, apply the DTO onto
> the base DT, and free the buffer.
>
> The upside of this approach is that it is no longer necessary to
> select and hard-code specific DTO load address into the DTO. The
> slight downside is the new malloc()/free() overhead for each DTO,
> but that is negligible (*).
>
> (*) on iMX8MM/MN and STM32MP1
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>
> ---
>  common/image-fit.c | 31 +++++++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)
>

Is this tested by the existing overlay test, or do we need something new?

Regards,
Simon

Re: [PATCH] fit: Load DTO into temporary buffer and ignore load address

[Marek Vasut]

On 6/26/21 8:31 PM, Simon Glass wrote:
> Hi,
> 
> On Thu, 10 Jun 2021 at 20:10, Marek Vasut <marex@denx.de> wrote:
>>
>> The current fitImage DTO implementation expects each fitImage image
>> subnode containing DTO to have 'load' property, pointing somewhere
>> into memory where the DTO will be loaded. The address in the 'load'
>> property must be different then the base DT load address and there
>> must be sufficient amount of space between those two addresses.
>> Selecting and using such hard-coded addresses is fragile, error
>> prone and difficult to port even across devices with the same SoC
>> and different DRAM sizes.
>>
>> The DTO cannot be applied in-place because fdt_overlay_apply_verbose()
>> modifies the DTO when applying it onto the base DT, so if the DTO was
>> used in place within the fitImage, call to fdt_overlay_apply_verbose()
>> would corrupt the fitImage.
>>
>> Instead of copying the DTO to a specific hard-coded load address,
>> allocate a buffer, copy the DTO into that buffer, apply the DTO onto
>> the base DT, and free the buffer.
>>
>> The upside of this approach is that it is no longer necessary to
>> select and hard-code specific DTO load address into the DTO. The
>> slight downside is the new malloc()/free() overhead for each DTO,
>> but that is negligible (*).
>>
>> (*) on iMX8MM/MN and STM32MP1
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Tom Rini <trini@konsulko.com>
>> ---
>>   common/image-fit.c | 31 +++++++++++++++++++++++++++----
>>   1 file changed, 27 insertions(+), 4 deletions(-)
>>
> 
> Is this tested by the existing overlay test, or do we need something new?

Overlay test should be sufficient.

Re: [PATCH] fit: Load DTO into temporary buffer and ignore load address

[Simon Glass]

On Sat, 26 Jun 2021 at 13:23, Marek Vasut <marex@denx.de> wrote:
>
> On 6/26/21 8:31 PM, Simon Glass wrote:
> > Hi,
> >
> > On Thu, 10 Jun 2021 at 20:10, Marek Vasut <marex@denx.de> wrote:
> >>
> >> The current fitImage DTO implementation expects each fitImage image
> >> subnode containing DTO to have 'load' property, pointing somewhere
> >> into memory where the DTO will be loaded. The address in the 'load'
> >> property must be different then the base DT load address and there
> >> must be sufficient amount of space between those two addresses.
> >> Selecting and using such hard-coded addresses is fragile, error
> >> prone and difficult to port even across devices with the same SoC
> >> and different DRAM sizes.
> >>
> >> The DTO cannot be applied in-place because fdt_overlay_apply_verbose()
> >> modifies the DTO when applying it onto the base DT, so if the DTO was
> >> used in place within the fitImage, call to fdt_overlay_apply_verbose()
> >> would corrupt the fitImage.
> >>
> >> Instead of copying the DTO to a specific hard-coded load address,
> >> allocate a buffer, copy the DTO into that buffer, apply the DTO onto
> >> the base DT, and free the buffer.
> >>
> >> The upside of this approach is that it is no longer necessary to
> >> select and hard-code specific DTO load address into the DTO. The
> >> slight downside is the new malloc()/free() overhead for each DTO,
> >> but that is negligible (*).
> >>
> >> (*) on iMX8MM/MN and STM32MP1
> >>
> >> Signed-off-by: Marek Vasut <marex@denx.de>
> >> Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> >> Cc: Simon Glass <sjg@chromium.org>
> >> Cc: Tom Rini <trini@konsulko.com>
> >> ---
> >>   common/image-fit.c | 31 +++++++++++++++++++++++++++----
> >>   1 file changed, 27 insertions(+), 4 deletions(-)
> >>
> >
> > Is this tested by the existing overlay test, or do we need something new?
>
> Overlay test should be sufficient.

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH] fit: Load DTO into temporary buffer and ignore load address

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1572 bytes --]

On Fri, Jun 11, 2021 at 04:09:56AM +0200, Marek Vasut wrote:

> The current fitImage DTO implementation expects each fitImage image
> subnode containing DTO to have 'load' property, pointing somewhere
> into memory where the DTO will be loaded. The address in the 'load'
> property must be different then the base DT load address and there
> must be sufficient amount of space between those two addresses.
> Selecting and using such hard-coded addresses is fragile, error
> prone and difficult to port even across devices with the same SoC
> and different DRAM sizes.
> 
> The DTO cannot be applied in-place because fdt_overlay_apply_verbose()
> modifies the DTO when applying it onto the base DT, so if the DTO was
> used in place within the fitImage, call to fdt_overlay_apply_verbose()
> would corrupt the fitImage.
> 
> Instead of copying the DTO to a specific hard-coded load address,
> allocate a buffer, copy the DTO into that buffer, apply the DTO onto
> the base DT, and free the buffer.
> 
> The upside of this approach is that it is no longer necessary to
> select and hard-code specific DTO load address into the DTO. The
> slight downside is the new malloc()/free() overhead for each DTO,
> but that is negligible (*).
> 
> (*) on iMX8MM/MN and STM32MP1
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]