[PATCH] imx: hab: fix size of IVT+CSF blob tacked on to u-boot.itb

[PATCH] imx: hab: fix size of IVT+CSF blob tacked on to u-boot.itb

[Rasmus Villemoes]

Loading flash.bin using uuu fails when flash.bin does not have the
right size.

When flash.bin is loaded from some storage medium (sd card/emmc), SPL
just loads some random garbage bytes from beyond what has been
populated when flash.bin was written, but when loaded via uuu, SPL
hangs waiting for the host to send the expected number of bytes. Which
is (size of FIT image aligned to 0x1000)+CONFIG_CSF_SIZE. The
alignment to 0x1000 is already done and is necessary in all cases
because that's the exact expected location of the 32 byte IVT
header. But the IVT+CSF blob tacked onto the end must be a total of
CONFIG_CSF_SIZE.

This is exactly the same fix as 89f19f45d650, except that this time
around I don't know how to cleanly get CONFIG_CSF_SIZE.

Fixes: bc6beae7c55f (binman: Add nxp_imx8mcst etype for i.MX8M flash.bin signing)
Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
---
Heiko, can you check if this works for you?

And if somebody wants to pick this up and knows how to get at CONFIG_
values, feel free to fix up and take authorship. But perhaps it's not
really configurable at all; imx8mimage.c has the value 0x2000
hard-coded, so I don't think anything good could ever come from
modifying CONFIG_CSF_SIZE. If so, the right fix is probably just to
make that knob non-settable.

 tools/binman/etype/nxp_imx8mcst.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/binman/etype/nxp_imx8mcst.py b/tools/binman/etype/nxp_imx8mcst.py
index 8221517b0c4..9a1974cc522 100644
--- a/tools/binman/etype/nxp_imx8mcst.py
+++ b/tools/binman/etype/nxp_imx8mcst.py
@@ -137,6 +137,8 @@ class Entry_nxp_imx8mcst(Entry_mkimage):
         args = ['-i', cfg_fname, '-o', output_fname]
         if self.cst.run_cmd(*args) is not None:
             outdata = tools.read_file(output_fname)
+            # fixme: 0x2000 should be CONFIG_CSF_SIZE
+            outdata += tools.get_bytes(0, 0x2000 - 0x20 - len(outdata))
             return data + outdata
         else:
             # Bintool is missing; just use the input data as the output
-- 
2.47.0

Re: [PATCH] imx: hab: fix size of IVT+CSF blob tacked on to u-boot.itb

[Marek Vasut]

On 10/24/24 2:27 PM, Rasmus Villemoes wrote:
> Loading flash.bin using uuu fails when flash.bin does not have the
> right size.
> 
> When flash.bin is loaded from some storage medium (sd card/emmc), SPL
> just loads some random garbage bytes from beyond what has been
> populated when flash.bin was written, but when loaded via uuu, SPL
> hangs waiting for the host to send the expected number of bytes. Which
> is (size of FIT image aligned to 0x1000)+CONFIG_CSF_SIZE. The
> alignment to 0x1000 is already done and is necessary in all cases
> because that's the exact expected location of the 32 byte IVT
> header. But the IVT+CSF blob tacked onto the end must be a total of
> CONFIG_CSF_SIZE.
> 
> This is exactly the same fix as 89f19f45d650, except that this time
> around I don't know how to cleanly get CONFIG_CSF_SIZE.
> 
> Fixes: bc6beae7c55f (binman: Add nxp_imx8mcst etype for i.MX8M flash.bin signing)
> Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
> ---
> Heiko, can you check if this works for you?
> 
> And if somebody wants to pick this up and knows how to get at CONFIG_
> values, feel free to fix up and take authorship. But perhaps it's not
> really configurable at all; imx8mimage.c has the value 0x2000
> hard-coded, so I don't think anything good could ever come from
> modifying CONFIG_CSF_SIZE. If so, the right fix is probably just to
> make that knob non-settable.
> 
>   tools/binman/etype/nxp_imx8mcst.py | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/tools/binman/etype/nxp_imx8mcst.py b/tools/binman/etype/nxp_imx8mcst.py
> index 8221517b0c4..9a1974cc522 100644
> --- a/tools/binman/etype/nxp_imx8mcst.py
> +++ b/tools/binman/etype/nxp_imx8mcst.py
> @@ -137,6 +137,8 @@ class Entry_nxp_imx8mcst(Entry_mkimage):
>           args = ['-i', cfg_fname, '-o', output_fname]
>           if self.cst.run_cmd(*args) is not None:
>               outdata = tools.read_file(output_fname)
> +            # fixme: 0x2000 should be CONFIG_CSF_SIZE
> +            outdata += tools.get_bytes(0, 0x2000 - 0x20 - len(outdata))
>               return data + outdata
>           else:
>               # Bintool is missing; just use the input data as the output

I have to admit, I never really figured out this binman stuff, but 
shouldn't the fix be also in tools/binman/etype/nxp_imx8mimage.py ? And 
... shouldn't it somehow use SetImagePos() ?

Re: [PATCH] imx: hab: fix size of IVT+CSF blob tacked on to u-boot.itb

[Fabio Estevam]

Adding Simon.

On Thu, Oct 24, 2024 at 11:14 AM Marek Vasut <marex@denx.de> wrote:
>
> On 10/24/24 2:27 PM, Rasmus Villemoes wrote:
> > Loading flash.bin using uuu fails when flash.bin does not have the
> > right size.
> >
> > When flash.bin is loaded from some storage medium (sd card/emmc), SPL
> > just loads some random garbage bytes from beyond what has been
> > populated when flash.bin was written, but when loaded via uuu, SPL
> > hangs waiting for the host to send the expected number of bytes. Which
> > is (size of FIT image aligned to 0x1000)+CONFIG_CSF_SIZE. The
> > alignment to 0x1000 is already done and is necessary in all cases
> > because that's the exact expected location of the 32 byte IVT
> > header. But the IVT+CSF blob tacked onto the end must be a total of
> > CONFIG_CSF_SIZE.
> >
> > This is exactly the same fix as 89f19f45d650, except that this time
> > around I don't know how to cleanly get CONFIG_CSF_SIZE.
> >
> > Fixes: bc6beae7c55f (binman: Add nxp_imx8mcst etype for i.MX8M flash.bin signing)
> > Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
> > ---
> > Heiko, can you check if this works for you?
> >
> > And if somebody wants to pick this up and knows how to get at CONFIG_
> > values, feel free to fix up and take authorship. But perhaps it's not
> > really configurable at all; imx8mimage.c has the value 0x2000
> > hard-coded, so I don't think anything good could ever come from
> > modifying CONFIG_CSF_SIZE. If so, the right fix is probably just to
> > make that knob non-settable.
> >
> >   tools/binman/etype/nxp_imx8mcst.py | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/tools/binman/etype/nxp_imx8mcst.py b/tools/binman/etype/nxp_imx8mcst.py
> > index 8221517b0c4..9a1974cc522 100644
> > --- a/tools/binman/etype/nxp_imx8mcst.py
> > +++ b/tools/binman/etype/nxp_imx8mcst.py
> > @@ -137,6 +137,8 @@ class Entry_nxp_imx8mcst(Entry_mkimage):
> >           args = ['-i', cfg_fname, '-o', output_fname]
> >           if self.cst.run_cmd(*args) is not None:
> >               outdata = tools.read_file(output_fname)
> > +            # fixme: 0x2000 should be CONFIG_CSF_SIZE
> > +            outdata += tools.get_bytes(0, 0x2000 - 0x20 - len(outdata))
> >               return data + outdata
> >           else:
> >               # Bintool is missing; just use the input data as the output
>
> I have to admit, I never really figured out this binman stuff, but
> shouldn't the fix be also in tools/binman/etype/nxp_imx8mimage.py ? And
> ... shouldn't it somehow use SetImagePos() ?

Re: [PATCH] imx: hab: fix size of IVT+CSF blob tacked on to u-boot.itb

[Rasmus Villemoes]

On Thu, Oct 24 2024, Marek Vasut <marex@denx.de> wrote:

> On 10/24/24 2:27 PM, Rasmus Villemoes wrote:
>> Loading flash.bin using uuu fails when flash.bin does not have the
>> right size.
>> When flash.bin is loaded from some storage medium (sd card/emmc),
>> SPL
>> just loads some random garbage bytes from beyond what has been
>> populated when flash.bin was written, but when loaded via uuu, SPL
>> hangs waiting for the host to send the expected number of bytes. Which
>> is (size of FIT image aligned to 0x1000)+CONFIG_CSF_SIZE. The
>> alignment to 0x1000 is already done and is necessary in all cases
>> because that's the exact expected location of the 32 byte IVT
>> header. But the IVT+CSF blob tacked onto the end must be a total of
>> CONFIG_CSF_SIZE.
>> This is exactly the same fix as 89f19f45d650, except that this time
>> around I don't know how to cleanly get CONFIG_CSF_SIZE.
>> Fixes: bc6beae7c55f (binman: Add nxp_imx8mcst etype for i.MX8M
>> flash.bin signing)
>> Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
>> ---
>> Heiko, can you check if this works for you?
>> And if somebody wants to pick this up and knows how to get at
>> CONFIG_
>> values, feel free to fix up and take authorship. But perhaps it's not
>> really configurable at all; imx8mimage.c has the value 0x2000
>> hard-coded, so I don't think anything good could ever come from
>> modifying CONFIG_CSF_SIZE. If so, the right fix is probably just to
>> make that knob non-settable.
>>   tools/binman/etype/nxp_imx8mcst.py | 2 ++
>>   1 file changed, 2 insertions(+)
>> diff --git a/tools/binman/etype/nxp_imx8mcst.py
>> b/tools/binman/etype/nxp_imx8mcst.py
>> index 8221517b0c4..9a1974cc522 100644
>> --- a/tools/binman/etype/nxp_imx8mcst.py
>> +++ b/tools/binman/etype/nxp_imx8mcst.py
>> @@ -137,6 +137,8 @@ class Entry_nxp_imx8mcst(Entry_mkimage):
>>           args = ['-i', cfg_fname, '-o', output_fname]
>>           if self.cst.run_cmd(*args) is not None:
>>               outdata = tools.read_file(output_fname)
>> +            # fixme: 0x2000 should be CONFIG_CSF_SIZE
>> +            outdata += tools.get_bytes(0, 0x2000 - 0x20 - len(outdata))
>>               return data + outdata
>>           else:
>>               # Bintool is missing; just use the input data as the output
>
> I have to admit, I never really figured out this binman stuff, but
> shouldn't the fix be also in tools/binman/etype/nxp_imx8mimage.py ?

No, why? That logic is all about generating the imx-specific header in
front of SPL.bin, there's no CSF being generated. Or maybe mkimage tacks
on some dummy bytes, but then that's explicitly ignored by the signing
step, I think that's what the

            signbase -= 0x40
            signsize = struct.unpack('<I', data[24:28])[0] - signbase
            # Remove mkimage generated padding from the end of data
            data = data[:signsize]

is about, so the signing step generates a new IVT and a new (real) CSF
for the SPL. And none of that matters for the size of the final
flash.bin, because the FIT image is located far ahead at a fixed 0x58000
position.

> And ... shouldn't it somehow use SetImagePos() ?

Again, why? I'm padding a blob (in this case the CSF data) to a required
size before tacking it on. Exactly as the existing code does

            # Align fitImage to 4k
            signsize = tools.align(len(data), 0x1000)
            data += tools.get_bytes(0, signsize - len(data))

before writing out the (padded FIT image + 32 byte IVT) for cst to chew
on and generate the CSF data.

This is all about ensuring that the FIT+IVT+CSF blob has (exactly) the size
computed by board_spl_fit_size_align, which is

        size = ALIGN(size, 0x1000);
        size += CONFIG_CSF_SIZE;

The 0x1000 part is done in the existing code, I'm just making sure that
the data we append after that is exactly CONFIG_CSF_SIZE. The CSF blob
itself is what is originally in outdata, and because the IVT must be
included in the data which is signed, the IVT has already been appended
after the 0x1000 padded FIT image; that's why the computation of the
extra bytes ends up being a little complicated. [If we just appended a
full CONFIG_CSF_SIZE bytes, the board would boot, but then uuu would be
hanging while waiting to deliver the remaining part of the file, so the
size should match exactly.]

I also don't grok the binman stuff, but there's really no separate image
to set a "position" for, the csf blob (or ivt+csf blob) is not
represented as its own image in the binman description.

Rasmus

Re: [PATCH] imx: hab: fix size of IVT+CSF blob tacked on to u-boot.itb

[Marek Vasut]

On 10/25/24 9:10 AM, Rasmus Villemoes wrote:
> On Thu, Oct 24 2024, Marek Vasut <marex@denx.de> wrote:
> 
>> On 10/24/24 2:27 PM, Rasmus Villemoes wrote:
>>> Loading flash.bin using uuu fails when flash.bin does not have the
>>> right size.
>>> When flash.bin is loaded from some storage medium (sd card/emmc),
>>> SPL
>>> just loads some random garbage bytes from beyond what has been
>>> populated when flash.bin was written, but when loaded via uuu, SPL
>>> hangs waiting for the host to send the expected number of bytes. Which
>>> is (size of FIT image aligned to 0x1000)+CONFIG_CSF_SIZE. The
>>> alignment to 0x1000 is already done and is necessary in all cases
>>> because that's the exact expected location of the 32 byte IVT
>>> header. But the IVT+CSF blob tacked onto the end must be a total of
>>> CONFIG_CSF_SIZE.
>>> This is exactly the same fix as 89f19f45d650, except that this time
>>> around I don't know how to cleanly get CONFIG_CSF_SIZE.
>>> Fixes: bc6beae7c55f (binman: Add nxp_imx8mcst etype for i.MX8M
>>> flash.bin signing)
>>> Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
>>> ---
>>> Heiko, can you check if this works for you?
>>> And if somebody wants to pick this up and knows how to get at
>>> CONFIG_
>>> values, feel free to fix up and take authorship. But perhaps it's not
>>> really configurable at all; imx8mimage.c has the value 0x2000
>>> hard-coded, so I don't think anything good could ever come from
>>> modifying CONFIG_CSF_SIZE. If so, the right fix is probably just to
>>> make that knob non-settable.
>>>    tools/binman/etype/nxp_imx8mcst.py | 2 ++
>>>    1 file changed, 2 insertions(+)
>>> diff --git a/tools/binman/etype/nxp_imx8mcst.py
>>> b/tools/binman/etype/nxp_imx8mcst.py
>>> index 8221517b0c4..9a1974cc522 100644
>>> --- a/tools/binman/etype/nxp_imx8mcst.py
>>> +++ b/tools/binman/etype/nxp_imx8mcst.py
>>> @@ -137,6 +137,8 @@ class Entry_nxp_imx8mcst(Entry_mkimage):
>>>            args = ['-i', cfg_fname, '-o', output_fname]
>>>            if self.cst.run_cmd(*args) is not None:
>>>                outdata = tools.read_file(output_fname)
>>> +            # fixme: 0x2000 should be CONFIG_CSF_SIZE
>>> +            outdata += tools.get_bytes(0, 0x2000 - 0x20 - len(outdata))
>>>                return data + outdata
>>>            else:
>>>                # Bintool is missing; just use the input data as the output
>>
>> I have to admit, I never really figured out this binman stuff, but
>> shouldn't the fix be also in tools/binman/etype/nxp_imx8mimage.py ?
> 
> No, why? That logic is all about generating the imx-specific header in
> front of SPL.bin, there's no CSF being generated.

This patch is modifying tools/binman/etype/nxp_imx8mcst.py which is the 
Code-Signing-Tool binman etype, the thing used for generating SIGNED 
images during HAB use.

Shouldn't this patch modify tools/binman/etype/nxp_imx8mimage.py which 
is the etype used when generating flash.bin itself ?

My understanding was that the problem Heiko reported was alignment of 
flash.bin , independently of it being signed or not ?

> Or maybe mkimage tacks
> on some dummy bytes, but then that's explicitly ignored by the signing
> step, I think that's what the
> 
>              signbase -= 0x40
>              signsize = struct.unpack('<I', data[24:28])[0] - signbase
>              # Remove mkimage generated padding from the end of data
>              data = data[:signsize]
> 
> is about, so the signing step generates a new IVT and a new (real) CSF
> for the SPL. And none of that matters for the size of the final
> flash.bin, because the FIT image is located far ahead at a fixed 0x58000
> position.
> 
>> And ... shouldn't it somehow use SetImagePos() ?
> 
> Again, why? I'm padding a blob (in this case the CSF data) to a required
> size before tacking it on. Exactly as the existing code does
> 
>              # Align fitImage to 4k
>              signsize = tools.align(len(data), 0x1000)
>              data += tools.get_bytes(0, signsize - len(data))
> 
> before writing out the (padded FIT image + 32 byte IVT) for cst to chew
> on and generate the CSF data.
> 
> This is all about ensuring that the FIT+IVT+CSF blob has (exactly) the size
> computed by board_spl_fit_size_align, which is
> 
>          size = ALIGN(size, 0x1000);
>          size += CONFIG_CSF_SIZE;
> 
> The 0x1000 part is done in the existing code, I'm just making sure that
> the data we append after that is exactly CONFIG_CSF_SIZE. The CSF blob
> itself is what is originally in outdata, and because the IVT must be
> included in the data which is signed, the IVT has already been appended
> after the 0x1000 padded FIT image; that's why the computation of the
> extra bytes ends up being a little complicated. [If we just appended a
> full CONFIG_CSF_SIZE bytes, the board would boot, but then uuu would be
> hanging while waiting to deliver the remaining part of the file, so the
> size should match exactly.]
> 
> I also don't grok the binman stuff, but there's really no separate image
> to set a "position" for, the csf blob (or ivt+csf blob) is not
> represented as its own image in the binman description.
It is probably best to let Simon comment on this.

Re: [PATCH] imx: hab: fix size of IVT+CSF blob tacked on to u-boot.itb

[Rasmus Villemoes]

On Fri, Oct 25 2024, Marek Vasut <marex@denx.de> wrote:

> On 10/25/24 9:10 AM, Rasmus Villemoes wrote:
>> On Thu, Oct 24 2024, Marek Vasut <marex@denx.de> wrote:
>> 
>>> On 10/24/24 2:27 PM, Rasmus Villemoes wrote:
>>>> Loading flash.bin using uuu fails when flash.bin does not have the
>>>> right size.
>>>> When flash.bin is loaded from some storage medium (sd card/emmc),
>>>> SPL
>>>> just loads some random garbage bytes from beyond what has been
>>>> populated when flash.bin was written, but when loaded via uuu, SPL
>>>> hangs waiting for the host to send the expected number of bytes. Which
>>>> is (size of FIT image aligned to 0x1000)+CONFIG_CSF_SIZE. The
>>>> alignment to 0x1000 is already done and is necessary in all cases
>>>> because that's the exact expected location of the 32 byte IVT
>>>> header. But the IVT+CSF blob tacked onto the end must be a total of
>>>> CONFIG_CSF_SIZE.
>>>> This is exactly the same fix as 89f19f45d650, except that this time
>>>> around I don't know how to cleanly get CONFIG_CSF_SIZE.
>>>> Fixes: bc6beae7c55f (binman: Add nxp_imx8mcst etype for i.MX8M
>>>> flash.bin signing)
>>>> Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
>>>> ---
>>>> Heiko, can you check if this works for you?
>>>> And if somebody wants to pick this up and knows how to get at
>>>> CONFIG_
>>>> values, feel free to fix up and take authorship. But perhaps it's not
>>>> really configurable at all; imx8mimage.c has the value 0x2000
>>>> hard-coded, so I don't think anything good could ever come from
>>>> modifying CONFIG_CSF_SIZE. If so, the right fix is probably just to
>>>> make that knob non-settable.
>>>>    tools/binman/etype/nxp_imx8mcst.py | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>> diff --git a/tools/binman/etype/nxp_imx8mcst.py
>>>> b/tools/binman/etype/nxp_imx8mcst.py
>>>> index 8221517b0c4..9a1974cc522 100644
>>>> --- a/tools/binman/etype/nxp_imx8mcst.py
>>>> +++ b/tools/binman/etype/nxp_imx8mcst.py
>>>> @@ -137,6 +137,8 @@ class Entry_nxp_imx8mcst(Entry_mkimage):
>>>>            args = ['-i', cfg_fname, '-o', output_fname]
>>>>            if self.cst.run_cmd(*args) is not None:
>>>>                outdata = tools.read_file(output_fname)
>>>> +            # fixme: 0x2000 should be CONFIG_CSF_SIZE
>>>> +            outdata += tools.get_bytes(0, 0x2000 - 0x20 - len(outdata))
>>>>                return data + outdata
>>>>            else:
>>>>                # Bintool is missing; just use the input data as the output
>>>
>>> I have to admit, I never really figured out this binman stuff, but
>>> shouldn't the fix be also in tools/binman/etype/nxp_imx8mimage.py ?
>> No, why? That logic is all about generating the imx-specific header
>> in
>> front of SPL.bin, there's no CSF being generated.
>
> This patch is modifying tools/binman/etype/nxp_imx8mcst.py which is
> the Code-Signing-Tool binman etype, the thing used for generating
> SIGNED images during HAB use.
>
> Shouldn't this patch modify tools/binman/etype/nxp_imx8mimage.py which
> is the etype used when generating flash.bin itself ?

No, because this is not about flash.bin per se, but about the (u-boot
FIT+IVT+CSF) blob that forms part of flash.bin, in the IMX_HAB case.

> My understanding was that the problem Heiko reported was alignment of
> flash.bin , independently of it being signed or not ?

Read the thread again. Heiko's problem was very much related to IMX_HAB
being enabled. I've already explained what the problem actually is
and why this fixes it. Not only once, but as I said, this is equivalent
to a fix I sent for the shell script implementation, so to answer a
question you asked elsewhere: Yes, this is missing in the binman
implementation.

Extending the size of flash.bin in the !IMX_HAB case will have the
opposite problem (as will adding the wrong amount of padding bytes in
the IMX_HAB case), namely that the board will boot, but uuu on the host
will "hang" waiting for the target to consume the remaining bytes.

Rasmus

Re: [PATCH] imx: hab: fix size of IVT+CSF blob tacked on to u-boot.itb

[Marek Vasut]

On 10/27/24 7:38 PM, Rasmus Villemoes wrote:
> On Fri, Oct 25 2024, Marek Vasut <marex@denx.de> wrote:
> 
>> On 10/25/24 9:10 AM, Rasmus Villemoes wrote:
>>> On Thu, Oct 24 2024, Marek Vasut <marex@denx.de> wrote:
>>>
>>>> On 10/24/24 2:27 PM, Rasmus Villemoes wrote:
>>>>> Loading flash.bin using uuu fails when flash.bin does not have the
>>>>> right size.
>>>>> When flash.bin is loaded from some storage medium (sd card/emmc),
>>>>> SPL
>>>>> just loads some random garbage bytes from beyond what has been
>>>>> populated when flash.bin was written, but when loaded via uuu, SPL
>>>>> hangs waiting for the host to send the expected number of bytes. Which
>>>>> is (size of FIT image aligned to 0x1000)+CONFIG_CSF_SIZE. The
>>>>> alignment to 0x1000 is already done and is necessary in all cases
>>>>> because that's the exact expected location of the 32 byte IVT
>>>>> header. But the IVT+CSF blob tacked onto the end must be a total of
>>>>> CONFIG_CSF_SIZE.
>>>>> This is exactly the same fix as 89f19f45d650, except that this time
>>>>> around I don't know how to cleanly get CONFIG_CSF_SIZE.
>>>>> Fixes: bc6beae7c55f (binman: Add nxp_imx8mcst etype for i.MX8M
>>>>> flash.bin signing)
>>>>> Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
>>>>> ---
>>>>> Heiko, can you check if this works for you?
>>>>> And if somebody wants to pick this up and knows how to get at
>>>>> CONFIG_
>>>>> values, feel free to fix up and take authorship. But perhaps it's not
>>>>> really configurable at all; imx8mimage.c has the value 0x2000
>>>>> hard-coded, so I don't think anything good could ever come from
>>>>> modifying CONFIG_CSF_SIZE. If so, the right fix is probably just to
>>>>> make that knob non-settable.
>>>>>     tools/binman/etype/nxp_imx8mcst.py | 2 ++
>>>>>     1 file changed, 2 insertions(+)
>>>>> diff --git a/tools/binman/etype/nxp_imx8mcst.py
>>>>> b/tools/binman/etype/nxp_imx8mcst.py
>>>>> index 8221517b0c4..9a1974cc522 100644
>>>>> --- a/tools/binman/etype/nxp_imx8mcst.py
>>>>> +++ b/tools/binman/etype/nxp_imx8mcst.py
>>>>> @@ -137,6 +137,8 @@ class Entry_nxp_imx8mcst(Entry_mkimage):
>>>>>             args = ['-i', cfg_fname, '-o', output_fname]
>>>>>             if self.cst.run_cmd(*args) is not None:
>>>>>                 outdata = tools.read_file(output_fname)
>>>>> +            # fixme: 0x2000 should be CONFIG_CSF_SIZE
>>>>> +            outdata += tools.get_bytes(0, 0x2000 - 0x20 - len(outdata))
>>>>>                 return data + outdata
>>>>>             else:
>>>>>                 # Bintool is missing; just use the input data as the output
>>>>
>>>> I have to admit, I never really figured out this binman stuff, but
>>>> shouldn't the fix be also in tools/binman/etype/nxp_imx8mimage.py ?
>>> No, why? That logic is all about generating the imx-specific header
>>> in
>>> front of SPL.bin, there's no CSF being generated.
>>
>> This patch is modifying tools/binman/etype/nxp_imx8mcst.py which is
>> the Code-Signing-Tool binman etype, the thing used for generating
>> SIGNED images during HAB use.
>>
>> Shouldn't this patch modify tools/binman/etype/nxp_imx8mimage.py which
>> is the etype used when generating flash.bin itself ?
> 
> No, because this is not about flash.bin per se, but about the (u-boot
> FIT+IVT+CSF) blob that forms part of flash.bin, in the IMX_HAB case.
> 
>> My understanding was that the problem Heiko reported was alignment of
>> flash.bin , independently of it being signed or not ?
> 
> Read the thread again. Heiko's problem was very much related to IMX_HAB
> being enabled. I've already explained what the problem actually is
> and why this fixes it. Not only once, but as I said, this is equivalent
> to a fix I sent for the shell script implementation, so to answer a
> question you asked elsewhere: Yes, this is missing in the binman
> implementation.
> 
> Extending the size of flash.bin in the !IMX_HAB case will have the
> opposite problem (as will adding the wrong amount of padding bytes in
> the IMX_HAB case), namely that the board will boot, but uuu on the host
> will "hang" waiting for the target to consume the remaining bytes.
Oh, I see, all right then. Thanks for clarifying.

Re: [PATCH] imx: hab: fix size of IVT+CSF blob tacked on to u-boot.itb

[Heiko Schocher]

Hello Rasmus,

On 24.10.24 14:27, Rasmus Villemoes wrote:
> Loading flash.bin using uuu fails when flash.bin does not have the
> right size.
> 
> When flash.bin is loaded from some storage medium (sd card/emmc), SPL
> just loads some random garbage bytes from beyond what has been
> populated when flash.bin was written, but when loaded via uuu, SPL
> hangs waiting for the host to send the expected number of bytes. Which
> is (size of FIT image aligned to 0x1000)+CONFIG_CSF_SIZE. The
> alignment to 0x1000 is already done and is necessary in all cases
> because that's the exact expected location of the 32 byte IVT
> header. But the IVT+CSF blob tacked onto the end must be a total of
> CONFIG_CSF_SIZE.
> 
> This is exactly the same fix as 89f19f45d650, except that this time
> around I don't know how to cleanly get CONFIG_CSF_SIZE.
> 
> Fixes: bc6beae7c55f (binman: Add nxp_imx8mcst etype for i.MX8M flash.bin signing)
> Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
> ---
> Heiko, can you check if this works for you?

Works fine for me, thanks!

> And if somebody wants to pick this up and knows how to get at CONFIG_
> values, feel free to fix up and take authorship. But perhaps it's not
> really configurable at all; imx8mimage.c has the value 0x2000
> hard-coded, so I don't think anything good could ever come from
> modifying CONFIG_CSF_SIZE. If so, the right fix is probably just to
> make that knob non-settable.

Hmm... I don not know if it is a fix value... added Simon,
may he knows how to use a CONFIG_ option in buildman. If I
find time, I will take a look...

bye,
Heiko
> 
>   tools/binman/etype/nxp_imx8mcst.py | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/tools/binman/etype/nxp_imx8mcst.py b/tools/binman/etype/nxp_imx8mcst.py
> index 8221517b0c4..9a1974cc522 100644
> --- a/tools/binman/etype/nxp_imx8mcst.py
> +++ b/tools/binman/etype/nxp_imx8mcst.py
> @@ -137,6 +137,8 @@ class Entry_nxp_imx8mcst(Entry_mkimage):
>           args = ['-i', cfg_fname, '-o', output_fname]
>           if self.cst.run_cmd(*args) is not None:
>               outdata = tools.read_file(output_fname)
> +            # fixme: 0x2000 should be CONFIG_CSF_SIZE
> +            outdata += tools.get_bytes(0, 0x2000 - 0x20 - len(outdata))
>               return data + outdata
>           else:
>               # Bintool is missing; just use the input data as the output
> 

-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs@denx.de

Re: [PATCH] imx: hab: fix size of IVT+CSF blob tacked on to u-boot.itb

[Fabio Estevam]

On Fri, Oct 25, 2024 at 1:36 AM Heiko Schocher <hs@denx.de> wrote:
>
> Hello Rasmus,
>
> On 24.10.24 14:27, Rasmus Villemoes wrote:
> > Loading flash.bin using uuu fails when flash.bin does not have the
> > right size.
> >
> > When flash.bin is loaded from some storage medium (sd card/emmc), SPL
> > just loads some random garbage bytes from beyond what has been
> > populated when flash.bin was written, but when loaded via uuu, SPL
> > hangs waiting for the host to send the expected number of bytes. Which
> > is (size of FIT image aligned to 0x1000)+CONFIG_CSF_SIZE. The
> > alignment to 0x1000 is already done and is necessary in all cases
> > because that's the exact expected location of the 32 byte IVT
> > header. But the IVT+CSF blob tacked onto the end must be a total of
> > CONFIG_CSF_SIZE.
> >
> > This is exactly the same fix as 89f19f45d650, except that this time
> > around I don't know how to cleanly get CONFIG_CSF_SIZE.
> >
> > Fixes: bc6beae7c55f (binman: Add nxp_imx8mcst etype for i.MX8M flash.bin signing)
> > Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
> > ---
> > Heiko, can you check if this works for you?
>
> Works fine for me, thanks!

Applied, thanks.

Re: [PATCH] imx: hab: fix size of IVT+CSF blob tacked on to u-boot.itb

[Fabio Estevam]

On Fri, Oct 25, 2024 at 11:51 AM Fabio Estevam <festevam@gmail.com> wrote:

> > > Heiko, can you check if this works for you?
> >
> > Works fine for me, thanks!
>
> Applied, thanks.

Sorry, applied it too soon. I see there is an ongoing discussion.

I will wait for more time.

Re: [PATCH] imx: hab: fix size of IVT+CSF blob tacked on to u-boot.itb

[Marek Vasut]

On 10/25/24 5:09 PM, Fabio Estevam wrote:
> On Fri, Oct 25, 2024 at 11:51 AM Fabio Estevam <festevam@gmail.com> wrote:
> 
>>>> Heiko, can you check if this works for you?
>>>
>>> Works fine for me, thanks!
>>
>> Applied, thanks.
> 
> Sorry, applied it too soon. I see there is an ongoing discussion.
> 
> I will wait for more time.

Go ahead, nothing to add from my side, but it would be good to get a TB 
from Heiko maybe ?

Re: [PATCH] imx: hab: fix size of IVT+CSF blob tacked on to u-boot.itb

[Heiko Schocher]

Hello Marek,

On 27.10.24 22:01, Marek Vasut wrote:
> On 10/25/24 5:09 PM, Fabio Estevam wrote:
>> On Fri, Oct 25, 2024 at 11:51 AM Fabio Estevam <festevam@gmail.com> wrote:
>>
>>>>> Heiko, can you check if this works for you?
>>>>
>>>> Works fine for me, thanks!
>>>
>>> Applied, thanks.
>>
>> Sorry, applied it too soon. I see there is an ongoing discussion.
>>
>> I will wait for more time.
> 
> Go ahead, nothing to add from my side, but it would be good to get a TB from Heiko maybe ?

I tested it already, ah, and forgot the TB, so

Tested-by: Heiko Schocher <hs@denx.de>

Thanks!

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs@denx.de

Re: [PATCH] imx: hab: fix size of IVT+CSF blob tacked on to u-boot.itb

[Marek Vasut]

On 10/28/24 6:13 AM, Heiko Schocher wrote:
> Hello Marek,
> 
> On 27.10.24 22:01, Marek Vasut wrote:
>> On 10/25/24 5:09 PM, Fabio Estevam wrote:
>>> On Fri, Oct 25, 2024 at 11:51 AM Fabio Estevam <festevam@gmail.com> 
>>> wrote:
>>>
>>>>>> Heiko, can you check if this works for you?
>>>>>
>>>>> Works fine for me, thanks!
>>>>
>>>> Applied, thanks.
>>>
>>> Sorry, applied it too soon. I see there is an ongoing discussion.
>>>
>>> I will wait for more time.
>>
>> Go ahead, nothing to add from my side, but it would be good to get a 
>> TB from Heiko maybe ?
> 
> I tested it already, ah, and forgot the TB, so
> 
> Tested-by: Heiko Schocher <hs@denx.de>
Understood, thank you. Nothing to add from my side.