[PATCH 0/4] mkimage: also honour -B even without external data

[PATCH 0/4] mkimage: also honour -B even without external data

[Rasmus Villemoes]

I was surprised to discover that mkimage effectively ignores -B when
used by itself - the help text suggests otherwise, and it's a
completely reasonable thing to have.

Also, binman already translates a fit,align property into a -B
argument, without requiring fit,external-offset to be set.

Rasmus Villemoes (4):
  mkimage: also honour -B even without external data
  binman: test: rename 275_fit_align.dts -> 275_fit_align_external.dts
  tools: binman: add test case for fit,align without fit,external-offset
  binman: update documentation for fit,align property

 tools/binman/entries.rst                      |  5 +-
 tools/binman/ftest.py                         | 14 ++++-
 ...t_align.dts => 275_fit_align_external.dts} |  0
 tools/binman/test/311_fit_align.dts           | 58 +++++++++++++++++++
 tools/fit_image.c                             | 40 +++++++++++++
 5 files changed, 112 insertions(+), 5 deletions(-)
 rename tools/binman/test/{275_fit_align.dts => 275_fit_align_external.dts} (100%)
 create mode 100644 tools/binman/test/311_fit_align.dts

-- 
2.37.2

[PATCH 1/4] mkimage: also honour -B even without external data

[Rasmus Villemoes]

In some cases, using the "external data" feature is impossible or
undesirable, but one may still want (or need) the FIT image to have a
certain alignment. Also, given the current 'mkimage -h' output,

  -B => align size in hex for FIT structure and header

it is quite unexpected for -B to be effectively ignored without -E.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 tools/fit_image.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/tools/fit_image.c b/tools/fit_image.c
index 9fe69ea0d9..2f5b25098a 100644
--- a/tools/fit_image.c
+++ b/tools/fit_image.c
@@ -712,6 +712,42 @@ err:
 	return ret;
 }
 
+/**
+ * fit_align() - Ensure FIT image has certain alignment
+ *
+ * This takes a normal FIT file (with embedded data) and increases its
+ * size so that it is a multiple of params->bl_len.
+ */
+static int fit_align(struct image_tool_params *params, const char *fname)
+{
+	int fit_size, new_size;
+	int fd;
+	struct stat sbuf;
+	void *fdt;
+	int ret = 0;
+	int align_size;
+
+	align_size = params->bl_len;
+	fd = mmap_fdt(params->cmdname, fname, 0, &fdt, &sbuf, false, false);
+	if (fd < 0)
+		return -EIO;
+
+	fit_size = fdt_totalsize(fdt);
+	new_size = ALIGN(fit_size, align_size);
+	fdt_set_totalsize(fdt, new_size);
+	debug("Size extended from from %x to %x\n", fit_size, new_size);
+	munmap(fdt, sbuf.st_size);
+
+	if (ftruncate(fd, new_size)) {
+		debug("%s: Failed to truncate file: %s\n", __func__,
+		      strerror(errno));
+		ret = -EIO;
+	}
+
+	close(fd);
+	return ret;
+}
+
 /**
  * fit_handle_file - main FIT file processing function
  *
@@ -817,6 +853,10 @@ static int fit_handle_file(struct image_tool_params *params)
 		ret = fit_extract_data(params, tmpfile);
 		if (ret)
 			goto err_system;
+	} else if (params->bl_len) {
+		ret = fit_align(params, tmpfile);
+		if (ret)
+			goto err_system;
 	}
 
 	if (rename (tmpfile, params->imagefile) == -1) {
-- 
2.37.2

[PATCH 2/4] binman: test: rename 275_fit_align.dts -> 275_fit_align_external.dts

[Rasmus Villemoes]

In preparation for adding a test case for fit,align without
fit,external-offset present, rename the existing test case and
corresponding file.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 tools/binman/ftest.py                                         | 4 ++--
 .../test/{275_fit_align.dts => 275_fit_align_external.dts}    | 0
 2 files changed, 2 insertions(+), 2 deletions(-)
 rename tools/binman/test/{275_fit_align.dts => 275_fit_align_external.dts} (100%)

diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 1293e9dbf4..d26e7511f7 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -6522,9 +6522,9 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
         self.assertEqual(base + 8, inset.image_pos);
         self.assertEqual(4, inset.size);
 
-    def testFitAlign(self):
+    def testFitAlignExternal(self):
         """Test an image with an FIT with aligned external data"""
-        data = self._DoReadFile('275_fit_align.dts')
+        data = self._DoReadFile('275_fit_align_external.dts')
         self.assertEqual(4096, len(data))
 
         dtb = fdt.Fdt.FromData(data)
diff --git a/tools/binman/test/275_fit_align.dts b/tools/binman/test/275_fit_align_external.dts
similarity index 100%
rename from tools/binman/test/275_fit_align.dts
rename to tools/binman/test/275_fit_align_external.dts
-- 
2.37.2

[PATCH 3/4] tools: binman: add test case for fit, align without fit, external-offset

[Rasmus Villemoes]

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 tools/binman/ftest.py               | 10 +++++
 tools/binman/test/311_fit_align.dts | 58 +++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+)
 create mode 100644 tools/binman/test/311_fit_align.dts

diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index d26e7511f7..a3c465b3d3 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -7216,5 +7216,15 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
         self.assertRegex(err,
                          "Image 'image'.*missing bintools.*: bootgen")
 
+    def testFitAlign(self):
+        """Test a FIT image with a fit,align property"""
+        data = self._DoReadFile('311_fit_align.dts')
+        self.assertEqual(2048, len(data))
+
+        dtb = fdt.Fdt.FromData(data)
+        dtb.Scan()
+
+        self.assertEqual(2048, dtb._fdt_obj.totalsize())
+
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/binman/test/311_fit_align.dts b/tools/binman/test/311_fit_align.dts
new file mode 100644
index 0000000000..4a9b95b8df
--- /dev/null
+++ b/tools/binman/test/311_fit_align.dts
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		fit {
+			description = "test desc";
+			#address-cells = <1>;
+			fit,align = <2048>;
+
+			images {
+				u-boot {
+					description = "test u-boot";
+					type = "standalone";
+					arch = "arm64";
+					os = "u-boot";
+					compression = "none";
+					load = <00000000>;
+					entry = <00000000>;
+
+					u-boot-nodtb {
+					};
+				};
+
+				fdt-1 {
+					description = "test fdt";
+					type = "flat_dt";
+					compression = "none";
+
+					u-boot-dtb {
+					};
+				};
+
+				fdt-2 {
+					description = "test fdt";
+					type = "flat_dt";
+					compression = "none";
+
+					u-boot-dtb {
+					};
+				};
+			};
+
+			configurations {
+				default = "config-1";
+				config-1 {
+					description = "test config";
+					fdt = "fdt-1";
+					firmware = "u-boot";
+				};
+			};
+		};
+	};
+};
-- 
2.37.2

[PATCH 4/4] binman: update documentation for fit,align property

[Rasmus Villemoes]

Eliminate the repetition "what alignment to use ... and provides the
alignment to use", and indicate that fit,align can both be used by
itself and together with fit,external-offset.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 tools/binman/entries.rst | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
index e7dfe6b2a3..f9ad27ce8c 100644
--- a/tools/binman/entries.rst
+++ b/tools/binman/entries.rst
@@ -691,9 +691,8 @@ The top-level 'fit' node supports the following special properties:
         external offset. This is passed to mkimage via the -E and -p flags.
 
     fit,align
-        Indicates what alignment to use for the FIT and its external data,
-        and provides the alignment to use. This is passed to mkimage via
-        the -B flag.
+        Indicates what alignment to use for the FIT and, if applicable,
+        its external data. This is passed to mkimage via the -B flag.
 
     fit,fdt-list
         Indicates the entry argument which provides the list of device tree
-- 
2.37.2

Re: [PATCH 1/4] mkimage: also honour -B even without external data

[Simon Glass]

Hi Rasmus,

On Tue, 19 Sept 2023 at 05:37, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> In some cases, using the "external data" feature is impossible or
> undesirable, but one may still want (or need) the FIT image to have a
> certain alignment. Also, given the current 'mkimage -h' output,
>
>   -B => align size in hex for FIT structure and header
>
> it is quite unexpected for -B to be effectively ignored without -E.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  tools/fit_image.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)

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

Q below

>
> diff --git a/tools/fit_image.c b/tools/fit_image.c
> index 9fe69ea0d9..2f5b25098a 100644
> --- a/tools/fit_image.c
> +++ b/tools/fit_image.c
> @@ -712,6 +712,42 @@ err:
>         return ret;
>  }
>
> +/**
> + * fit_align() - Ensure FIT image has certain alignment
> + *
> + * This takes a normal FIT file (with embedded data) and increases its
> + * size so that it is a multiple of params->bl_len.
> + */
> +static int fit_align(struct image_tool_params *params, const char *fname)
> +{
> +       int fit_size, new_size;
> +       int fd;
> +       struct stat sbuf;
> +       void *fdt;
> +       int ret = 0;
> +       int align_size;
> +
> +       align_size = params->bl_len;
> +       fd = mmap_fdt(params->cmdname, fname, 0, &fdt, &sbuf, false, false);
> +       if (fd < 0)
> +               return -EIO;
> +
> +       fit_size = fdt_totalsize(fdt);
> +       new_size = ALIGN(fit_size, align_size);
> +       fdt_set_totalsize(fdt, new_size);

Shouldn't this be fdt_open_into()?

> +       debug("Size extended from from %x to %x\n", fit_size, new_size);
> +       munmap(fdt, sbuf.st_size);
> +
> +       if (ftruncate(fd, new_size)) {
> +               debug("%s: Failed to truncate file: %s\n", __func__,
> +                     strerror(errno));
> +               ret = -EIO;
> +       }
> +
> +       close(fd);
> +       return ret;
> +}
> +
>  /**
>   * fit_handle_file - main FIT file processing function
>   *
> @@ -817,6 +853,10 @@ static int fit_handle_file(struct image_tool_params *params)
>                 ret = fit_extract_data(params, tmpfile);
>                 if (ret)
>                         goto err_system;
> +       } else if (params->bl_len) {
> +               ret = fit_align(params, tmpfile);
> +               if (ret)
> +                       goto err_system;
>         }
>
>         if (rename (tmpfile, params->imagefile) == -1) {
> --
> 2.37.2
>

Regards,
Simon

Re: [PATCH 2/4] binman: test: rename 275_fit_align.dts -> 275_fit_align_external.dts

[Simon Glass]

On Tue, 19 Sept 2023 at 05:37, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> In preparation for adding a test case for fit,align without
> fit,external-offset present, rename the existing test case and
> corresponding file.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  tools/binman/ftest.py                                         | 4 ++--
>  .../test/{275_fit_align.dts => 275_fit_align_external.dts}    | 0
>  2 files changed, 2 insertions(+), 2 deletions(-)
>  rename tools/binman/test/{275_fit_align.dts => 275_fit_align_external.dts} (100%)
>

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

Re: [PATCH 3/4] tools: binman: add test case for fit, align without fit, external-offset

[Simon Glass]

On Tue, 19 Sept 2023 at 05:37, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  tools/binman/ftest.py               | 10 +++++
>  tools/binman/test/311_fit_align.dts | 58 +++++++++++++++++++++++++++++
>  2 files changed, 68 insertions(+)
>  create mode 100644 tools/binman/test/311_fit_align.dts
>

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

Re: [PATCH 4/4] binman: update documentation for fit,align property

[Simon Glass]

On Tue, 19 Sept 2023 at 05:37, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> Eliminate the repetition "what alignment to use ... and provides the
> alignment to use", and indicate that fit,align can both be used by
> itself and together with fit,external-offset.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  tools/binman/entries.rst | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

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

Re: [PATCH 1/4] mkimage: also honour -B even without external data

[Rasmus Villemoes]

On 21/09/2023 03.02, Simon Glass wrote:
> Hi Rasmus,
> 
> On Tue, 19 Sept 2023 at 05:37, Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
>>
>> In some cases, using the "external data" feature is impossible or
>> undesirable, but one may still want (or need) the FIT image to have a
>> certain alignment. Also, given the current 'mkimage -h' output,
>>
>>   -B => align size in hex for FIT structure and header
>>
>> it is quite unexpected for -B to be effectively ignored without -E.
>>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>> ---
>>  tools/fit_image.c | 40 ++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 40 insertions(+)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Q below
> 
>>
>> diff --git a/tools/fit_image.c b/tools/fit_image.c
>> index 9fe69ea0d9..2f5b25098a 100644
>> --- a/tools/fit_image.c
>> +++ b/tools/fit_image.c
>> @@ -712,6 +712,42 @@ err:
>>         return ret;
>>  }
>>
>> +/**
>> + * fit_align() - Ensure FIT image has certain alignment
>> + *
>> + * This takes a normal FIT file (with embedded data) and increases its
>> + * size so that it is a multiple of params->bl_len.
>> + */
>> +static int fit_align(struct image_tool_params *params, const char *fname)
>> +{
>> +       int fit_size, new_size;
>> +       int fd;
>> +       struct stat sbuf;
>> +       void *fdt;
>> +       int ret = 0;
>> +       int align_size;
>> +
>> +       align_size = params->bl_len;
>> +       fd = mmap_fdt(params->cmdname, fname, 0, &fdt, &sbuf, false, false);
>> +       if (fd < 0)
>> +               return -EIO;
>> +
>> +       fit_size = fdt_totalsize(fdt);
>> +       new_size = ALIGN(fit_size, align_size);
>> +       fdt_set_totalsize(fdt, new_size);
> 
> Shouldn't this be fdt_open_into()?

I honestly just copy-pasted fit_extract_data() and shaved it down to the
part that does the "align the FDT part of the file".

I don't really understand your question. Are you saying this doesn't
work (or maybe doesn't work in some cases), or are you saying that
there's a simpler way to do this? For the latter, sure, one doesn't
really need to parse the whole FDT; we could just

  open()
  pread() length from FDT header, convert to cpu-endianness
  length = ALIGN(length)
  pwrite() the new length in fdt-endianness
  ftruncate()
  close()

but I thought it was better to stay closer to how fit_extract_data() was
done.

Rasmus

Re: [PATCH 1/4] mkimage: also honour -B even without external data

[Simon Glass]

Hi Rasmus,

On Thu, 21 Sept 2023 at 01:57, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 21/09/2023 03.02, Simon Glass wrote:
> > Hi Rasmus,
> >
> > On Tue, 19 Sept 2023 at 05:37, Rasmus Villemoes
> > <rasmus.villemoes@prevas.dk> wrote:
> >>
> >> In some cases, using the "external data" feature is impossible or
> >> undesirable, but one may still want (or need) the FIT image to have a
> >> certain alignment. Also, given the current 'mkimage -h' output,
> >>
> >>   -B => align size in hex for FIT structure and header
> >>
> >> it is quite unexpected for -B to be effectively ignored without -E.
> >>
> >> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> >> ---
> >>  tools/fit_image.c | 40 ++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 40 insertions(+)
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > Q below
> >
> >>
> >> diff --git a/tools/fit_image.c b/tools/fit_image.c
> >> index 9fe69ea0d9..2f5b25098a 100644
> >> --- a/tools/fit_image.c
> >> +++ b/tools/fit_image.c
> >> @@ -712,6 +712,42 @@ err:
> >>         return ret;
> >>  }
> >>
> >> +/**
> >> + * fit_align() - Ensure FIT image has certain alignment
> >> + *
> >> + * This takes a normal FIT file (with embedded data) and increases its
> >> + * size so that it is a multiple of params->bl_len.
> >> + */
> >> +static int fit_align(struct image_tool_params *params, const char *fname)
> >> +{
> >> +       int fit_size, new_size;
> >> +       int fd;
> >> +       struct stat sbuf;
> >> +       void *fdt;
> >> +       int ret = 0;
> >> +       int align_size;
> >> +
> >> +       align_size = params->bl_len;
> >> +       fd = mmap_fdt(params->cmdname, fname, 0, &fdt, &sbuf, false, false);
> >> +       if (fd < 0)
> >> +               return -EIO;
> >> +
> >> +       fit_size = fdt_totalsize(fdt);
> >> +       new_size = ALIGN(fit_size, align_size);
> >> +       fdt_set_totalsize(fdt, new_size);
> >
> > Shouldn't this be fdt_open_into()?
>
> I honestly just copy-pasted fit_extract_data() and shaved it down to the
> part that does the "align the FDT part of the file".
>
> I don't really understand your question. Are you saying this doesn't
> work (or maybe doesn't work in some cases), or are you saying that
> there's a simpler way to do this? For the latter, sure, one doesn't
> really need to parse the whole FDT; we could just
>
>   open()
>   pread() length from FDT header, convert to cpu-endianness
>   length = ALIGN(length)
>   pwrite() the new length in fdt-endianness
>   ftruncate()
>   close()
>
> but I thought it was better to stay closer to how fit_extract_data() was
> done.

I mean that fdt_open_into() does more than just set the size (from
what I can tell). But looking further I see other code which calls
fdt_set_totalsize() so perhaps it is fine.

Regards,
SImon

Re: [PATCH 1/4] mkimage: also honour -B even without external data

[Rasmus Villemoes]

On 22/09/2023 17.26, Simon Glass wrote:

>>> Shouldn't this be fdt_open_into()?
>>
>> I honestly just copy-pasted fit_extract_data() and shaved it down to the
>> part that does the "align the FDT part of the file".
>>
>> I don't really understand your question. Are you saying this doesn't
>> work (or maybe doesn't work in some cases), or are you saying that
>> there's a simpler way to do this? For the latter, sure, one doesn't
>> really need to parse the whole FDT; we could just
>>
>>   open()
>>   pread() length from FDT header, convert to cpu-endianness
>>   length = ALIGN(length)
>>   pwrite() the new length in fdt-endianness
>>   ftruncate()
>>   close()
>>
>> but I thought it was better to stay closer to how fit_extract_data() was
>> done.
> 
> I mean that fdt_open_into() does more than just set the size (from
> what I can tell). But looking further I see other code which calls
> fdt_set_totalsize() so perhaps it is fine.

Yes, I think it's as it should be - as a I said, this is really just a
trimmed-down copy of the function which moves the data externally, and
also needs to make the size of the base fdt structure aligned.

Since patches 2,3,4 touch binman code, could you take all four?

Thanks,
Rasmus

Re: [PATCH 1/4] mkimage: also honour -B even without external data

[Simon Glass]

Hi Rasmus,

On Mon, 25 Sept 2023 at 02:47, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 22/09/2023 17.26, Simon Glass wrote:
>
> >>> Shouldn't this be fdt_open_into()?
> >>
> >> I honestly just copy-pasted fit_extract_data() and shaved it down to the
> >> part that does the "align the FDT part of the file".
> >>
> >> I don't really understand your question. Are you saying this doesn't
> >> work (or maybe doesn't work in some cases), or are you saying that
> >> there's a simpler way to do this? For the latter, sure, one doesn't
> >> really need to parse the whole FDT; we could just
> >>
> >>   open()
> >>   pread() length from FDT header, convert to cpu-endianness
> >>   length = ALIGN(length)
> >>   pwrite() the new length in fdt-endianness
> >>   ftruncate()
> >>   close()
> >>
> >> but I thought it was better to stay closer to how fit_extract_data() was
> >> done.
> >
> > I mean that fdt_open_into() does more than just set the size (from
> > what I can tell). But looking further I see other code which calls
> > fdt_set_totalsize() so perhaps it is fine.
>
> Yes, I think it's as it should be - as a I said, this is really just a
> trimmed-down copy of the function which moves the data externally, and
> also needs to make the size of the base fdt structure aligned.
>
> Since patches 2,3,4 touch binman code, could you take all four?

Yes, will do. I didn't pick them up in the most recent PR as I try to
have things sit for a week before applying.

Regards,
Simon

Re: [PATCH 1/4] mkimage: also honour -B even without external data

[Rasmus Villemoes]

On 25/09/2023 15.10, Simon Glass wrote:
> Hi Rasmus,
> 
> On Mon, 25 Sept 2023 at 02:47, Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:

>> Since patches 2,3,4 touch binman code, could you take all four?
> 
> Yes, will do. I didn't pick them up in the most recent PR as I try to
> have things sit for a week before applying.

Oh, absolutely, no rush :)

Rasmus

Re: [PATCH 4/4] binman: update documentation for fit,align property

[Jonas Karlman]

Hi Rasmus,

On 2023-09-19 13:37, Rasmus Villemoes wrote:
> Eliminate the repetition "what alignment to use ... and provides the
> alignment to use", and indicate that fit,align can both be used by
> itself and together with fit,external-offset.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  tools/binman/entries.rst | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
> index e7dfe6b2a3..f9ad27ce8c 100644
> --- a/tools/binman/entries.rst
> +++ b/tools/binman/entries.rst
> @@ -691,9 +691,8 @@ The top-level 'fit' node supports the following special properties:
>          external offset. This is passed to mkimage via the -E and -p flags.
>  
>      fit,align
> -        Indicates what alignment to use for the FIT and its external data,
> -        and provides the alignment to use. This is passed to mkimage via
> -        the -B flag.
> +        Indicates what alignment to use for the FIT and, if applicable,
> +        its external data. This is passed to mkimage via the -B flag.

This only updates entries.rst, please update tools/binman/etype/fit.py
and re-generate entries.rst from output of running binman entry-docs.

Regards,
Jonas

>  
>      fit,fdt-list
>          Indicates the entry argument which provides the list of device tree

Re: [PATCH 4/4] binman: update documentation for fit,align property

[Rasmus Villemoes]

On 25/09/2023 17.14, Jonas Karlman wrote:

>>      fit,align
>> -        Indicates what alignment to use for the FIT and its external data,
>> -        and provides the alignment to use. This is passed to mkimage via
>> -        the -B flag.
>> +        Indicates what alignment to use for the FIT and, if applicable,
>> +        its external data. This is passed to mkimage via the -B flag.
> 
> This only updates entries.rst, please update tools/binman/etype/fit.py
> and re-generate entries.rst from output of running binman entry-docs.

Ah, I didn't know the .rst was generated.

Simon, want me to resend (this one or the whole series?), or can you
fold in this when applying:

diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index 2c14b15b03..97d3cedaf5 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -71,9 +71,8 @@ class Entry_fit(Entry_section):
             external offset. This is passed to mkimage via the -E and
-p flags.

         fit,align
-            Indicates what alignment to use for the FIT and its
external data,
-            and provides the alignment to use. This is passed to
mkimage via
-            the -B flag.
+            Indicates what alignment to use for the FIT and, if applicable,
+            its external data. This is passed to mkimage via the -B flag.

         fit,fdt-list
             Indicates the entry argument which provides the list of
device tree

Rasmus

Re: [PATCH 4/4] binman: update documentation for fit,align property

[Simon Glass]

Hi Rasmus,

On Tue, 26 Sept 2023 at 00:25, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 25/09/2023 17.14, Jonas Karlman wrote:
>
> >>      fit,align
> >> -        Indicates what alignment to use for the FIT and its external data,
> >> -        and provides the alignment to use. This is passed to mkimage via
> >> -        the -B flag.
> >> +        Indicates what alignment to use for the FIT and, if applicable,
> >> +        its external data. This is passed to mkimage via the -B flag.
> >
> > This only updates entries.rst, please update tools/binman/etype/fit.py
> > and re-generate entries.rst from output of running binman entry-docs.
>
> Ah, I didn't know the .rst was generated.
>
> Simon, want me to resend (this one or the whole series?), or can you
> fold in this when applying:

I can fold it in, so long as I remember.

>
> diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
> index 2c14b15b03..97d3cedaf5 100644
> --- a/tools/binman/etype/fit.py
> +++ b/tools/binman/etype/fit.py
> @@ -71,9 +71,8 @@ class Entry_fit(Entry_section):
>              external offset. This is passed to mkimage via the -E and
> -p flags.
>
>          fit,align
> -            Indicates what alignment to use for the FIT and its
> external data,
> -            and provides the alignment to use. This is passed to
> mkimage via
> -            the -B flag.
> +            Indicates what alignment to use for the FIT and, if applicable,
> +            its external data. This is passed to mkimage via the -B flag.
>
>          fit,fdt-list
>              Indicates the entry argument which provides the list of
> device tree
>
> Rasmus
>

Regards,
Simon

Re: [PATCH 1/4] mkimage: also honour -B even without external data

[Sean Anderson]

On 9/19/23 07:37, Rasmus Villemoes wrote:
> In some cases, using the "external data" feature is impossible or
> undesirable, but one may still want (or need) the FIT image to have a
> certain alignment. Also, given the current 'mkimage -h' output,
> 
>    -B => align size in hex for FIT structure and header
> 
> it is quite unexpected for -B to be effectively ignored without -E.

FWIW, this behavior is documented in doc/mkimage.1 (which should also be
updated if this behavior is implemented):

| The alignment, in hexadecimal, that external data will be aligned to.
| This option only has an effect when -E is specified.

And, for additional context, the documentation for -E is

| After processing, move the image data outside the FIT and store a data
| offset in the FIT. Images will be placed one after the other
| immediately after the FIT, with each one aligned to a 4-byte boundary.
| The existing ‘data’ property in each image will be replaced with
| ‘data-offset’ and ‘data-size’ properties. A ‘data-offset’ of 0
| indicates that it starts in the first (4-byte-aligned) byte after the
| FIT.

Based on this documentation and my understanding of the code as-is, -B
controls the alignment of the images themselves, not the size multiple
of the FIT. However, from what I can tell, this patch does not actually
affect the alignment of the images, but rather adjusts the size of the
overall FIT to a certain alignment. I find this rather unexpected.

--Sean

> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>   tools/fit_image.c | 40 ++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 40 insertions(+)
> 
> diff --git a/tools/fit_image.c b/tools/fit_image.c
> index 9fe69ea0d9..2f5b25098a 100644
> --- a/tools/fit_image.c
> +++ b/tools/fit_image.c
> @@ -712,6 +712,42 @@ err:
>   	return ret;
>   }
>   
> +/**
> + * fit_align() - Ensure FIT image has certain alignment
> + *
> + * This takes a normal FIT file (with embedded data) and increases its
> + * size so that it is a multiple of params->bl_len.
> + */
> +static int fit_align(struct image_tool_params *params, const char *fname)
> +{
> +	int fit_size, new_size;
> +	int fd;
> +	struct stat sbuf;
> +	void *fdt;
> +	int ret = 0;
> +	int align_size;
> +
> +	align_size = params->bl_len;
> +	fd = mmap_fdt(params->cmdname, fname, 0, &fdt, &sbuf, false, false);
> +	if (fd < 0)
> +		return -EIO;
> +
> +	fit_size = fdt_totalsize(fdt);
> +	new_size = ALIGN(fit_size, align_size);
> +	fdt_set_totalsize(fdt, new_size);
> +	debug("Size extended from from %x to %x\n", fit_size, new_size);
> +	munmap(fdt, sbuf.st_size);
> +
> +	if (ftruncate(fd, new_size)) {
> +		debug("%s: Failed to truncate file: %s\n", __func__,
> +		      strerror(errno));
> +		ret = -EIO;
> +	}
> +
> +	close(fd);
> +	return ret;
> +}
> +
>   /**
>    * fit_handle_file - main FIT file processing function
>    *
> @@ -817,6 +853,10 @@ static int fit_handle_file(struct image_tool_params *params)
>   		ret = fit_extract_data(params, tmpfile);
>   		if (ret)
>   			goto err_system;
> +	} else if (params->bl_len) {
> +		ret = fit_align(params, tmpfile);
> +		if (ret)
> +			goto err_system;
>   	}
>   
>   	if (rename (tmpfile, params->imagefile) == -1) {

Re: [PATCH 1/4] mkimage: also honour -B even without external data

[Rasmus Villemoes]

On 27/09/2023 21.02, Sean Anderson wrote:
> On 9/19/23 07:37, Rasmus Villemoes wrote:
>> In some cases, using the "external data" feature is impossible or
>> undesirable, but one may still want (or need) the FIT image to have a
>> certain alignment. Also, given the current 'mkimage -h' output,
>>
>>    -B => align size in hex for FIT structure and header
>>
>> it is quite unexpected for -B to be effectively ignored without -E.
> 
> FWIW, this behavior is documented in doc/mkimage.1 (which should also be
> updated if this behavior is implemented):
> 
> | The alignment, in hexadecimal, that external data will be aligned to.
> | This option only has an effect when -E is specified.

I'll send a follow-up to fix that, thanks.

> And, for additional context, the documentation for -E is
> 
> | After processing, move the image data outside the FIT and store a data
> | offset in the FIT. Images will be placed one after the other
> | immediately after the FIT, with each one aligned to a 4-byte boundary.
> | The existing ‘data’ property in each image will be replaced with
> | ‘data-offset’ and ‘data-size’ properties. A ‘data-offset’ of 0
> | indicates that it starts in the first (4-byte-aligned) byte after the
> | FIT.
> 
> Based on this documentation and my understanding of the code as-is, -B
> controls the alignment of the images themselves, 

Yes, when -E is in effect. My patch does not affect the case where -E is
present.

> not the size multiple of the FIT.

It is _also_ that, but mostly as a "necessary side effect" of getting
the first image aligned. In order to achieve the desired alignment of
the first external image, the FIT structure itself is aligned to the -B
value, and each image's size similarly aligned up to that value so the
next image ends on a -B multiple again. So with both -E and -B, the FIT
structure itself is indeed also padded to the -B value, as the `mkimage
-h` output suggests.

What I want, and expected from `mkimage -h`, is to make the whole FIT
have a certain size multiple, without using -E, so in that case the
alignment of the FIT is the primary goal.

> However, from what I can tell, this patch does not actually
> affect the alignment of the images,

No, because the images are (still) embedded within the FIT as ordinary
values of the data= properties, and it's a basic property of the DTB
format that those are always 4-byte aligned, and you can't (easily)
change that [1]. Which, I suppose, is one of the reasons U-Boot invented
the "external data" mechanism - so for example the .dtbs embedded in the
FIT can all be guaranteed to be on an 8-byte boundary, and thus the
selected one can be used in-place when passed to linux.

> but rather adjusts the size of the
> overall FIT to a certain alignment. I find this rather unexpected.

Well, as I said, _I_ was surprised by -B having no effect based on the
`mkimage -h` output, so the two sources of documentation were not in
sync. The man page was/is correct wrt. the actual code.

Rasmus

[1] There's a nop tag one can insert, and I think I saw somebody
actually suggesting to do that to align the embedded data= properties,
but it's quite error-prone and inflexible, as any subsequent
modification of the .dtb before that property will ruin the alignment.

[PATCH 5/4] mkimage: update man page and -h output

[Rasmus Villemoes]

The man page correctly said that -B was ignored without -E, while the
`mkimage -h` output suggested otherwise. Now that -B can actually be
used by itself, update the man page.

While at it, also amend the `mkimage -h` line to mention the
connection with -E.

The FDT header is a fixed 40 bytes, so its size cannot (and is not)
modified, while its alignment is a property of the address in RAM one
loads the FIT to, so not something mkimage can affect in any way. (In
the file itself, the header is of course at offset 0, which has all
possible alignments already.)

Reported-by: Sean Anderson <seanga2@gmail.com>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 doc/mkimage.1   | 6 ++++--
 tools/mkimage.c | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/doc/mkimage.1 b/doc/mkimage.1
index 76c7859bb0..1a4bc25936 100644
--- a/doc/mkimage.1
+++ b/doc/mkimage.1
@@ -281,8 +281,10 @@ properties.  A \(oqdata-offset\(cq of 0 indicates that it starts in the first
 .BI \-B " alignment"
 .TQ
 .BI \-\-alignment " alignment"
-The alignment, in hexadecimal, that external data will be aligned to. This
-option only has an effect when \-E is specified.
+The alignment, in hexadecimal, that the FDT structure will be aligned
+to. With
+.BR \-E ,
+also specifies the alignment for the external data.
 .
 .TP
 .BI \-p " external-position"
diff --git a/tools/mkimage.c b/tools/mkimage.c
index 6dfe3e1d42..a5979fa6fd 100644
--- a/tools/mkimage.c
+++ b/tools/mkimage.c
@@ -112,7 +112,7 @@ static void usage(const char *msg)
 		"          -f => input filename for FIT source\n"
 		"          -i => input filename for ramdisk file\n"
 		"          -E => place data outside of the FIT structure\n"
-		"          -B => align size in hex for FIT structure and header\n"
+		"          -B => align size in hex for FIT structure and, with -E, for the external data\n"
 		"          -b => append the device tree binary to the FIT\n"
 		"          -t => update the timestamp in the FIT\n");
 #ifdef CONFIG_FIT_SIGNATURE
-- 
2.37.2

Re: [PATCH 1/4] mkimage: also honour -B even without external data

[Sean Anderson]

On 9/28/23 03:10, Rasmus Villemoes wrote:
> On 27/09/2023 21.02, Sean Anderson wrote:
>> On 9/19/23 07:37, Rasmus Villemoes wrote:
>>> In some cases, using the "external data" feature is impossible or
>>> undesirable, but one may still want (or need) the FIT image to have a
>>> certain alignment. Also, given the current 'mkimage -h' output,
>>>
>>>     -B => align size in hex for FIT structure and header
>>>
>>> it is quite unexpected for -B to be effectively ignored without -E.
>>
>> FWIW, this behavior is documented in doc/mkimage.1 (which should also be
>> updated if this behavior is implemented):
>>
>> | The alignment, in hexadecimal, that external data will be aligned to.
>> | This option only has an effect when -E is specified.
> 
> I'll send a follow-up to fix that, thanks.
> 
>> And, for additional context, the documentation for -E is
>>
>> | After processing, move the image data outside the FIT and store a data
>> | offset in the FIT. Images will be placed one after the other
>> | immediately after the FIT, with each one aligned to a 4-byte boundary.
>> | The existing ‘data’ property in each image will be replaced with
>> | ‘data-offset’ and ‘data-size’ properties. A ‘data-offset’ of 0
>> | indicates that it starts in the first (4-byte-aligned) byte after the
>> | FIT.
>>
>> Based on this documentation and my understanding of the code as-is, -B
>> controls the alignment of the images themselves,
> 
> Yes, when -E is in effect. My patch does not affect the case where -E is
> present.

Wherever possible, flags should be orthogonal. That is, they should have the
same effect (or at least the same spirit) regardless of the presence of other
flags. This matches their UX, as orthogonally selectable options.

>> not the size multiple of the FIT.
> 
> It is _also_ that, but mostly as a "necessary side effect" of getting
> the first image aligned. In order to achieve the desired alignment of
> the first external image, the FIT structure itself is aligned to the -B
> value, and each image's size similarly aligned up to that value so the
> next image ends on a -B multiple again. So with both -E and -B, the FIT
> structure itself is indeed also padded to the -B value, as the `mkimage
> -h` output suggests.

The intent of -B is to align images. That the FIT itself is padded is an
implementation detail.

> What I want, and expected from `mkimage -h`, is to make the whole FIT
> have a certain size multiple, without using -E, so in that case the
> alignment of the FIT is the primary goal.

Why does mkimage have to do this? Can't you just use truncate or, in a
binman context, align-size?

>> However, from what I can tell, this patch does not actually
>> affect the alignment of the images,
> 
> No, because the images are (still) embedded within the FIT as ordinary
> values of the data= properties, and it's a basic property of the DTB
> format that those are always 4-byte aligned, and you can't (easily)
> change that [1]. Which, I suppose, is one of the reasons U-Boot invented
> the "external data" mechanism - so for example the .dtbs embedded in the
> FIT can all be guaranteed to be on an 8-byte boundary, and thus the
> selected one can be used in-place when passed to linux.
> 
>> but rather adjusts the size of the
>> overall FIT to a certain alignment. I find this rather unexpected.
> 
> Well, as I said, _I_ was surprised by -B having no effect based on the
> `mkimage -h` output, so the two sources of documentation were not in
> sync. The man page was/is correct wrt. the actual code.

The mkimage -h putput is too terse to record the complete behavior of
each option. Perhaps we should add a warning when -B is specified without
-E.

> Rasmus
> 
> [1] There's a nop tag one can insert, and I think I saw somebody
> actually suggesting to do that to align the embedded data= properties,
> but it's quite error-prone and inflexible, as any subsequent
> modification of the .dtb before that property will ruin the alignment.

This would indeed be the way to go. I don't think we should worry about
further modification of the fdt, as this could also ruin the alignment
of external images.

--Sean

Re: [PATCH 5/4] mkimage: update man page and -h output

[Simon Glass]

On Thu, 28 Sept 2023 at 02:03, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> The man page correctly said that -B was ignored without -E, while the
> `mkimage -h` output suggested otherwise. Now that -B can actually be
> used by itself, update the man page.
>
> While at it, also amend the `mkimage -h` line to mention the
> connection with -E.
>
> The FDT header is a fixed 40 bytes, so its size cannot (and is not)
> modified, while its alignment is a property of the address in RAM one
> loads the FIT to, so not something mkimage can affect in any way. (In
> the file itself, the header is of course at offset 0, which has all
> possible alignments already.)
>
> Reported-by: Sean Anderson <seanga2@gmail.com>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  doc/mkimage.1   | 6 ++++--
>  tools/mkimage.c | 2 +-
>  2 files changed, 5 insertions(+), 3 deletions(-)

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

Strange that your patch says 5/4

Re: [PATCH 5/4] mkimage: update man page and -h output

[Tom Rini]

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

On Thu, Sep 28, 2023 at 10:02:57AM +0200, Rasmus Villemoes wrote:

> The man page correctly said that -B was ignored without -E, while the
> `mkimage -h` output suggested otherwise. Now that -B can actually be
> used by itself, update the man page.
> 
> While at it, also amend the `mkimage -h` line to mention the
> connection with -E.
> 
> The FDT header is a fixed 40 bytes, so its size cannot (and is not)
> modified, while its alignment is a property of the address in RAM one
> loads the FIT to, so not something mkimage can affect in any way. (In
> the file itself, the header is of course at offset 0, which has all
> possible alignments already.)
> 
> Reported-by: Sean Anderson <seanga2@gmail.com>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> 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 --]

Re: [PATCH 5/4] mkimage: update man page and -h output

[Rasmus Villemoes]

On 11/10/2023 20.37, Tom Rini wrote:
> On Thu, Sep 28, 2023 at 10:02:57AM +0200, Rasmus Villemoes wrote:
> 
>> The man page correctly said that -B was ignored without -E, while the
>> `mkimage -h` output suggested otherwise. Now that -B can actually be
>> used by itself, update the man page.
>>
>> While at it, also amend the `mkimage -h` line to mention the
>> connection with -E.
>>
>> The FDT header is a fixed 40 bytes, so its size cannot (and is not)
>> modified, while its alignment is a property of the address in RAM one
>> loads the FIT to, so not something mkimage can affect in any way. (In
>> the file itself, the header is of course at offset 0, which has all
>> possible alignments already.)
>>
>> Reported-by: Sean Anderson <seanga2@gmail.com>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Applied to u-boot/master, thanks!
> 

Thanks, but I'm afraid that was premature.

The original series which this was a fixup/followup for hasn't been
applied, and Sean had reservations. I'm leaving it to Simon or Tom or
whoever has final say to decide if they should eventually go in, but it
would probably be good to get a verdict soonish (it really shouldn't be
too controversial), and if it's a no, this should just be reverted.

Rasmus

Re: [PATCH 5/4] mkimage: update man page and -h output

[Tom Rini]

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

On Wed, Oct 11, 2023 at 09:07:11PM +0200, Rasmus Villemoes wrote:
> On 11/10/2023 20.37, Tom Rini wrote:
> > On Thu, Sep 28, 2023 at 10:02:57AM +0200, Rasmus Villemoes wrote:
> > 
> >> The man page correctly said that -B was ignored without -E, while the
> >> `mkimage -h` output suggested otherwise. Now that -B can actually be
> >> used by itself, update the man page.
> >>
> >> While at it, also amend the `mkimage -h` line to mention the
> >> connection with -E.
> >>
> >> The FDT header is a fixed 40 bytes, so its size cannot (and is not)
> >> modified, while its alignment is a property of the address in RAM one
> >> loads the FIT to, so not something mkimage can affect in any way. (In
> >> the file itself, the header is of course at offset 0, which has all
> >> possible alignments already.)
> >>
> >> Reported-by: Sean Anderson <seanga2@gmail.com>
> >> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> >> Reviewed-by: Simon Glass <sjg@chromium.org>
> > 
> > Applied to u-boot/master, thanks!
> > 
> 
> Thanks, but I'm afraid that was premature.
> 
> The original series which this was a fixup/followup for hasn't been
> applied, and Sean had reservations. I'm leaving it to Simon or Tom or
> whoever has final say to decide if they should eventually go in, but it
> would probably be good to get a verdict soonish (it really shouldn't be
> too controversial), and if it's a no, this should just be reverted.

Ugh, thanks.  I'll push the revert with the next round of changes.

-- 
Tom

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

Re: [PATCH 5/4] mkimage: update man page and -h output

[Sean Anderson]

Hi Rasmus,

On 10/11/23 15:07, Rasmus Villemoes wrote:
> On 11/10/2023 20.37, Tom Rini wrote:
>> On Thu, Sep 28, 2023 at 10:02:57AM +0200, Rasmus Villemoes wrote:
>>
>>> The man page correctly said that -B was ignored without -E, while the
>>> `mkimage -h` output suggested otherwise. Now that -B can actually be
>>> used by itself, update the man page.
>>>
>>> While at it, also amend the `mkimage -h` line to mention the
>>> connection with -E.
>>>
>>> The FDT header is a fixed 40 bytes, so its size cannot (and is not)
>>> modified, while its alignment is a property of the address in RAM one
>>> loads the FIT to, so not something mkimage can affect in any way. (In
>>> the file itself, the header is of course at offset 0, which has all
>>> possible alignments already.)
>>>
>>> Reported-by: Sean Anderson <seanga2@gmail.com>
>>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>
>> Applied to u-boot/master, thanks!
>>
> 
> Thanks, but I'm afraid that was premature.
> 
> The original series which this was a fixup/followup for hasn't been
> applied, and Sean had reservations. I'm leaving it to Simon or Tom or
> whoever has final say to decide if they should eventually go in, but it
> would probably be good to get a verdict soonish (it really shouldn't be
> too controversial), and if it's a no, this should just be reverted.

I was hoping you would respond to my most-recent email regarding this series.
In particular:

| Why does mkimage have to do this? Can't you just use truncate or, in a
| binman context, align-size?

Presumably you have some reason for wanting this in mkimage rather than using
existing tooling.

--Sean

Re: [PATCH 5/4] mkimage: update man page and -h output

[Simon Glass]

Hi,

On Wed, 11 Oct 2023 at 12:33, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Oct 11, 2023 at 09:07:11PM +0200, Rasmus Villemoes wrote:
> > On 11/10/2023 20.37, Tom Rini wrote:
> > > On Thu, Sep 28, 2023 at 10:02:57AM +0200, Rasmus Villemoes wrote:
> > >
> > >> The man page correctly said that -B was ignored without -E, while the
> > >> `mkimage -h` output suggested otherwise. Now that -B can actually be
> > >> used by itself, update the man page.
> > >>
> > >> While at it, also amend the `mkimage -h` line to mention the
> > >> connection with -E.
> > >>
> > >> The FDT header is a fixed 40 bytes, so its size cannot (and is not)
> > >> modified, while its alignment is a property of the address in RAM one
> > >> loads the FIT to, so not something mkimage can affect in any way. (In
> > >> the file itself, the header is of course at offset 0, which has all
> > >> possible alignments already.)
> > >>
> > >> Reported-by: Sean Anderson <seanga2@gmail.com>
> > >> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > >> Reviewed-by: Simon Glass <sjg@chromium.org>
> > >
> > > Applied to u-boot/master, thanks!
> > >
> >
> > Thanks, but I'm afraid that was premature.
> >
> > The original series which this was a fixup/followup for hasn't been
> > applied, and Sean had reservations. I'm leaving it to Simon or Tom or
> > whoever has final say to decide if they should eventually go in, but it
> > would probably be good to get a verdict soonish (it really shouldn't be
> > too controversial), and if it's a no, this should just be reverted.
>
> Ugh, thanks.  I'll push the revert with the next round of changes.

My intent was to apply it, but I haven't got to it yet.

Regards,
Simon

Re: [PATCH 5/4] mkimage: update man page and -h output

[Tom Rini]

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

On Wed, Oct 11, 2023 at 10:17:50PM -0400, Sean Anderson wrote:
> Hi Rasmus,
> 
> On 10/11/23 15:07, Rasmus Villemoes wrote:
> > On 11/10/2023 20.37, Tom Rini wrote:
> > > On Thu, Sep 28, 2023 at 10:02:57AM +0200, Rasmus Villemoes wrote:
> > > 
> > > > The man page correctly said that -B was ignored without -E, while the
> > > > `mkimage -h` output suggested otherwise. Now that -B can actually be
> > > > used by itself, update the man page.
> > > > 
> > > > While at it, also amend the `mkimage -h` line to mention the
> > > > connection with -E.
> > > > 
> > > > The FDT header is a fixed 40 bytes, so its size cannot (and is not)
> > > > modified, while its alignment is a property of the address in RAM one
> > > > loads the FIT to, so not something mkimage can affect in any way. (In
> > > > the file itself, the header is of course at offset 0, which has all
> > > > possible alignments already.)
> > > > 
> > > > Reported-by: Sean Anderson <seanga2@gmail.com>
> > > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > 
> > > Applied to u-boot/master, thanks!
> > > 
> > 
> > Thanks, but I'm afraid that was premature.
> > 
> > The original series which this was a fixup/followup for hasn't been
> > applied, and Sean had reservations. I'm leaving it to Simon or Tom or
> > whoever has final say to decide if they should eventually go in, but it
> > would probably be good to get a verdict soonish (it really shouldn't be
> > too controversial), and if it's a no, this should just be reverted.
> 
> I was hoping you would respond to my most-recent email regarding this series.
> In particular:
> 
> | Why does mkimage have to do this? Can't you just use truncate or, in a
> | binman context, align-size?
> 
> Presumably you have some reason for wanting this in mkimage rather than using
> existing tooling.

I think part of the fun of trying to figure out how to have the dtb in
u-boot be properly (8 byte) aligned was that existing command wise,
something portable is tricky?

-- 
Tom

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

Re: [PATCH 5/4] mkimage: update man page and -h output

[Rasmus Villemoes]

On 12/10/2023 04.17, Sean Anderson wrote:

> I was hoping you would respond to my most-recent email regarding this
> series.
> In particular:
> 
> | Why does mkimage have to do this? Can't you just use truncate or, in a
> | binman context, align-size?

In both cases, that just affects the size of the file, but does not
affect the totalsize field in the fdt header.

Wrt. binman, just as I was somewhat misled by the short mkimage -h
output into thinking that this already worked as I expected, binman's
fit,align documentation can also be read that way - and the
_implementation_ certainly currently unconditionally translates a
fit,align property into a -B argument to mkimage. So if we don't want
mkimage to support a -B by itself, binman documentation and code would
probably need another patch to reject that as well.

Rasmus

Re: [PATCH 1/4] mkimage: also honour -B even without external data

[Simon Glass]

Hi Rasmus,

On Fri, 29 Sept 2023 at 07:16, Sean Anderson <seanga2@gmail.com> wrote:
>
> On 9/28/23 03:10, Rasmus Villemoes wrote:
> > On 27/09/2023 21.02, Sean Anderson wrote:
> >> On 9/19/23 07:37, Rasmus Villemoes wrote:
> >>> In some cases, using the "external data" feature is impossible or
> >>> undesirable, but one may still want (or need) the FIT image to have a
> >>> certain alignment. Also, given the current 'mkimage -h' output,
> >>>
> >>>     -B => align size in hex for FIT structure and header
> >>>
> >>> it is quite unexpected for -B to be effectively ignored without -E.
> >>
> >> FWIW, this behavior is documented in doc/mkimage.1 (which should also be
> >> updated if this behavior is implemented):
> >>
> >> | The alignment, in hexadecimal, that external data will be aligned to.
> >> | This option only has an effect when -E is specified.
> >
> > I'll send a follow-up to fix that, thanks.
> >
> >> And, for additional context, the documentation for -E is
> >>
> >> | After processing, move the image data outside the FIT and store a data
> >> | offset in the FIT. Images will be placed one after the other
> >> | immediately after the FIT, with each one aligned to a 4-byte boundary.
> >> | The existing ‘data’ property in each image will be replaced with
> >> | ‘data-offset’ and ‘data-size’ properties. A ‘data-offset’ of 0
> >> | indicates that it starts in the first (4-byte-aligned) byte after the
> >> | FIT.
> >>
> >> Based on this documentation and my understanding of the code as-is, -B
> >> controls the alignment of the images themselves,
> >
> > Yes, when -E is in effect. My patch does not affect the case where -E is
> > present.
>
> Wherever possible, flags should be orthogonal. That is, they should have the
> same effect (or at least the same spirit) regardless of the presence of other
> flags. This matches their UX, as orthogonally selectable options.
>
> >> not the size multiple of the FIT.
> >
> > It is _also_ that, but mostly as a "necessary side effect" of getting
> > the first image aligned. In order to achieve the desired alignment of
> > the first external image, the FIT structure itself is aligned to the -B
> > value, and each image's size similarly aligned up to that value so the
> > next image ends on a -B multiple again. So with both -E and -B, the FIT
> > structure itself is indeed also padded to the -B value, as the `mkimage
> > -h` output suggests.
>
> The intent of -B is to align images. That the FIT itself is padded is an
> implementation detail.
>
> > What I want, and expected from `mkimage -h`, is to make the whole FIT
> > have a certain size multiple, without using -E, so in that case the
> > alignment of the FIT is the primary goal.
>
> Why does mkimage have to do this? Can't you just use truncate or, in a
> binman context, align-size?
>
> >> However, from what I can tell, this patch does not actually
> >> affect the alignment of the images,
> >
> > No, because the images are (still) embedded within the FIT as ordinary
> > values of the data= properties, and it's a basic property of the DTB
> > format that those are always 4-byte aligned, and you can't (easily)
> > change that [1]. Which, I suppose, is one of the reasons U-Boot invented
> > the "external data" mechanism - so for example the .dtbs embedded in the
> > FIT can all be guaranteed to be on an 8-byte boundary, and thus the
> > selected one can be used in-place when passed to linux.
> >
> >> but rather adjusts the size of the
> >> overall FIT to a certain alignment. I find this rather unexpected.
> >
> > Well, as I said, _I_ was surprised by -B having no effect based on the
> > `mkimage -h` output, so the two sources of documentation were not in
> > sync. The man page was/is correct wrt. the actual code.
>
> The mkimage -h putput is too terse to record the complete behavior of
> each option. Perhaps we should add a warning when -B is specified without
> -E.
>
> > Rasmus
> >
> > [1] There's a nop tag one can insert, and I think I saw somebody
> > actually suggesting to do that to align the embedded data= properties,
> > but it's quite error-prone and inflexible, as any subsequent
> > modification of the .dtb before that property will ruin the alignment.
>
> This would indeed be the way to go. I don't think we should worry about
> further modification of the fdt, as this could also ruin the alignment
> of external images.

Are you planning a new version of this series?

Regards,
Simon

Re: [PATCH 1/4] mkimage: also honour -B even without external data

[Rasmus Villemoes]

On 04/11/2023 20.43, Simon Glass wrote:
> Hi Rasmus,

> Are you planning a new version of this series?

No. AFAICT there's nothing to be done on my end.

Rasmus

Re: [PATCH 5/4] mkimage: update man page and -h output

[Sean Anderson]

On 10/13/23 14:30, Rasmus Villemoes wrote:
> On 12/10/2023 04.17, Sean Anderson wrote:
> 
>> I was hoping you would respond to my most-recent email regarding this
>> series.
>> In particular:
>>
>> | Why does mkimage have to do this? Can't you just use truncate or, in a
>> | binman context, align-size?
> 
> In both cases, that just affects the size of the file, but does not
> affect the totalsize field in the fdt header.

Use `dtc -a` then.

> Wrt. binman, just as I was somewhat misled by the short mkimage -h
> output into thinking that this already worked as I expected, binman's
> fit,align documentation can also be read that way - and the
> _implementation_ certainly currently unconditionally translates a
> fit,align property into a -B argument to mkimage. So if we don't want
> mkimage to support a -B by itself, binman documentation and code would
> probably need another patch to reject that as well.

Yeah, that's a good idea.

--Sean

Re: [PATCH 5/4] mkimage: update man page and -h output

[Rasmus Villemoes]

On 07/11/2023 01.46, Sean Anderson wrote:
> On 10/13/23 14:30, Rasmus Villemoes wrote:
>> On 12/10/2023 04.17, Sean Anderson wrote:
>>
>>> I was hoping you would respond to my most-recent email regarding this
>>> series.
>>> In particular:
>>>
>>> | Why does mkimage have to do this? Can't you just use truncate or, in a
>>> | binman context, align-size?
>>
>> In both cases, that just affects the size of the file, but does not
>> affect the totalsize field in the fdt header.
> 
> Use `dtc -a` then.

Not possible, when the generator is binman which calls mkimage which
calls dtc. If I was using dtc directly, sure.

Anyway, I currently have more important things to deal with than this.
Simon, please drop the series. I'll send a revert to Tom for the
prematurely applied fixup.

Rasmus

Re: [PATCH 5/4] mkimage: update man page and -h output

[Rasmus Villemoes]

On 07/11/2023 08.30, Rasmus Villemoes wrote:

> I'll send a revert to Tom for the prematurely applied fixup.

Oh, I see that's already done. Good.

Rasmus