[U-Boot] [PATCH 4/5] tools: mkimage: fix imximage header size

[Stefan Agner <stefan@agner.ch>]

On 2015-07-15 13:44, Albert ARIBAUD wrote:
> Hello Stefan (and sorry for the duplicate),
> 
> On Wed, 15 Jul 2015 12:41:59 +0200, Stefan Agner <stefan@agner.ch>
> wrote:
>> Hi Albert,
>>
>> On 2015-07-15 09:54, Albert ARIBAUD wrote:
>> > Hello Stefano,
>> >
>> > On Wed, 15 Jul 2015 09:19:55 +0200, Stefano Babic <sbabic@denx.de>
>> > wrote:
>> >
>> >> > I think my patch back then solves the issue nicer. The struct dcd_v2_t
>> >> > is not the reason the whole header is not aligned by 8-byte, it is a
>> >> > problem of the boot_data_t header which is 12 bytes long. So inserting
>> >> > the padding just after struct boot_data_t (or inside of struct
>> >> > boot_data_t) seems to be more appropriate.
>> >>
>> >> I see. Albert, can you test on your board if Stefan's patch solves your
>> >> issue, too ? I could then revert this one and apply Stefan's.
>> >
>> > I can test it, but I'm sure that boot data does not need size alignment
>> > since in my case there is no such alignment and Vybrid boots. To me
>> > this means aligning boot_data_t size would introduce a constraint that
>> > is not really there.
>>
>> I quote here from both emails, since its about the same topic.
>>
>> > After reading the U-Boot and Freescale discussions, IIUC you have come
>> > to the conclusion that the missing 4 bytes were in boot_data_t because
>> > it was the only structure in the header which was not 8-bytes-aligned.
>> >
>> > However, the available documentation does not specify this constraint,
>> > and from a more experimental vewpoint, my patch adds 4 bytes at the end
>> > of the overall header, thus leaving boot data at 12 bytes (therefore
>> > leaving the dcd_table not 8-byte-aligned) and yet Vybrid can boot.
>>
>> Yes, I agree on that. Also the boot data seems not to have the 8-byte
>> alignment requirement (see the graphics I posted on the Freescale
>> community article).
>>
>> > To me, this proves that the size alignment problem is not with
>> > boot_data_t but with the overall header.
>>
>> I agree. Still, boot_data_t is the "offender", hence I would prefer that
>> solution over your solution.
>>
>> > If Stefan would test my patch as well, I reckon he would find it to
>> > work as well as his.
>> >
>> > So we have two patches which fix Vybrid booting:
>> >
>> > - one which aligns the boot_data_t /size/ but keeps its /offset/
>> >   unaligned;
>> >
>> > - one which aligns the the boot_data_t /offset/ but keeps its /size/
>> >   unaligned.
>>
>> I don't get this classification. The complete header struct shows that
>> dcd_v2_t is _after_ boot_data_t.
>>
>> typedef struct {
>> 	flash_header_v2_t fhdr;
>> 	boot_data_t boot_data;
>> 	dcd_v2_t dcd_table;
>> } imx_header_v2_t;
>>
>> Your patch changes the size of dcd_v2_t, hence compensating the
>> "unaligned" size of boot_data_t.
> 
> Compensating the unaligned size of the overall header for sure, since
> I'm adding a last field in the last structure. But precisely since I'm
> adding 4 bytes at the very end of the construct, we cannot tell whether
> I fixed the 'size' of the flash header, boot data or DCD.
> 
> What we can tell, though, is that Vybrid will boot as long as we
> increase the size of the overall header to an 8-byte boundary, and also
> that it will boot regardless to whether these 4 bytes are added between
> boot data and DCD or after DCD.
> 
> (note, btw, that in the Vybrid RM, the IVT itself, and the DCD, contain
> their own size explicitly in their tag-length-version headers, but the
> boot data does not contain its own size, nor is it contained elsewhere.
> What we call 'the size of the boot data' is 'the difference between the
> boot data and dcd addresses', but that's a definition for convenience.)

Good point, agreed.

>> As far as I see we have these two patches which fix Vybrid booting:
>>
>> - one which changes the boot_data_t /size/ which keeps the offset
>>   of dcd_v2_t and the image aligned.
>>
>> - one which changes the dcd_v2_t /size/ which compensates the unaligned
>>   size of boot_data_t and keeps the image aligned.
>>
>> > Seems to me that the conclusion is that the actual alignment of
>> > boot_data_t does not matter and that only the alignment of the
>> > whole imx_header_v2_t size (and, consequently, offset) matters.
>> >
>> > How about just adding an attribute((align(8))) to imx_header_v2_t?
>>
>> Hm this sounds tempting, but it does not seem to work here. I think
>> because it only aligns the beginning of imx_header_v2_t in to 8-byte,
>> however it does not align the size of the whole struct to 8 bytes, I
>> guess? Hence the header size is still "unaligned".
> 
> Correct -- my fault, this aligns the address, not size, and there is
> no attribute that will control size alignment.

Actually I just discovered that "aligned" also changes the size (make
sure using the correct syntax "__attribute__((aligned(8)))").

However, imximage.c seems to do some calculations using sizeof(struct
imx_header) and sizeof(imx_header_v2_t) which lead to this error
message:
./tools/mkimage: header error

Adding __attribute__((aligned(8))) to struct imx_header seems not to
alleviate the problem... Any idea?

> 
> So we really need to add manual padding.
> 
> And if we want to not introduce any artificial constaints, I suggest we
> define the padding as a separate field in the overall structure, i.e.:
> 
> typedef struct {
>         flash_header_v2_t fhdr;
>         boot_data_t boot_data;
>         dcd_v2_t dcd_table;
> 	uint32_t padding; /* make size an 8-byte multiple */
> } imx_header_v2_t;

That change would also sound good to me.

> 
> I think this is the 'least inexact' solution: it does not enforce any
> address or size alignment constraint that is not defined in the RM, and
> it does show that the constraint is not on the boot data or DCD but on
> the header as a whole.
> 
> Functionally, it is identical to what I did, so I'm pretty sure it
> works. :)

Agreed.

--
Stefan