fiptool: Pad zeros for file alignment #1166

Leo-Yan · 2017-11-22T09:03:25Z

ARM trusted firmware can specify alignment, the alignment has been
applied on every image start address when combine fip.bin; but it misses
to apply alignment to the file tail, so fip.bin file size cannot be
specified alignment.

This commit checks the tail of fip.bin file and pad zeros on the tail
for alignment.

Fixes ARM-software/tf-issues#528

Signed-off-by: Leo Yan [email protected]

ARM trusted firmware can specify alignment, the alignment has been applied on every image start address when combine fip.bin; but it misses to apply alignment to the file tail, so fip.bin file size cannot be specified alignment. This commit checks the tail of fip.bin file and pad zeros on the tail for alignment. Signed-off-by: Leo Yan <[email protected]>

ssg-bot · 2017-11-22T09:05:02Z

Can one of the admins verify this patch?

ssg-bot · 2017-11-22T09:05:03Z

Can one of the admins verify this patch?

Leo-Yan · 2017-11-22T09:05:46Z

Hi @hzhuang1, @danh-arm, @raw-bin, This patch is to fix the issue brought up by Robin: ARM-software/tf-issues#528

davidcunado-arm · 2017-11-22T22:57:28Z

@Leo-Yan
Can you add
Fixes ARM-software/tf-issues#528
to the commit message?

Leo-Yan · 2017-11-23T00:41:42Z

@davidcunado-arm, yes, have added into commit log.

ghost

Why is this change actually needed?

I would like to point out that the entry_offset of the null uuid entry is no longer equal to the size of the FIP file after this patch. Hence, if we go with this patch, I would prefer to maintain this constraint as current implementations depend on this. I understand this is not guaranteed by the TBBR spec but I don't want to break this implementation defined behaviour without good reason.

ghost · 2017-11-23T10:28:22Z

@Leo-Yan I just saw your earlier comment explaining why this patch is needed.

Are we sure this is not just working around the problem? I don't understand why the io block layer cannot handle this.

Leo-Yan · 2017-11-23T13:44:01Z

Hi Dimitris,

On Thu, Nov 23, 2017 at 10:28:28AM +0000, Dimitris Papastamos wrote: @Leo-Yan I just saw your earlier comment explaining why this patch is needed. Are we sure this is not just working around the problem? I don't understand why the io block layer cannot handle this.

I might not clearly described this issue, this issue is _NOT_ for BL1 loading fip.bin image. I don't see the problem is introduced by block layer in ARM-TF, the problem happens when we want to *flash* fip.bin image into EMMC. E.g. at the tail of fip.bin file, it needs to combine BL33 (u-boot.bin or UEFI image); so you could see the code line [1] ensures the image offset address is aligned, this also ensures the previous image has been padded properly for alignment. But it misses to pad the last one image, e.g. alignment size is 512B, uboot.bin has size 512010 bytes (512010 mod 512 = 10); so finally the fip.bin image size is not 512B alignment. So we need flash fip.bin in board recovery mode, this needs to download fip.bin into SRAM and flash into EMMC; for this case sometimes we have requirement for transferring length alignment. So I would like to check with you guys if the fip.bin length is not aligned with the size specified by FIP_ALIGN, do you think this is an issue or not? Originally I think if we can pad fip.bin file with FIP_ALIGN alignment, this will be safe for any size of BL33. So this is the purpse of this patch. [1] https://github.com/ARM-software/arm-trusted-firmware/blob/master/tools/fiptool/fiptool.c#L552

…

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #1166 (comment)

ghost · 2017-11-23T13:55:45Z

What code do you use to flash fip.bin into EMMC? Do you use the io block layer in TF or some other code? Why can't you accommodate for this case in your flashing code?

hzhuang1 · 2017-11-24T01:18:07Z

What code do you use to flash fip.bin into EMMC? Do you use the io block layer in TF or some other code? Why can't you accommodate for this case in your flashing code?

Oh, it's not related to io block layer in TF. In TF, we could force to align. But this issue is related to usb transmission.

USB is actually much like storage deivce. We need to specify the transmission size and be heavily relied on DMA. If the fip.bin isn't aligned with 512B, it causes USB driver always waiting for the last transmission that never comes.

We always hope user to do the alignment. But it seems not everyone knows this limitation. So Leo prepared this patch as a workaround.

ghost · 2017-11-29T09:59:17Z

USB is actually much like storage deivce. We need to specify the transmission size and be heavily relied on DMA. If the fip.bin isn't aligned with 512B, it causes USB driver always waiting for the last transmission that never comes.

I still don't understand why you can't fix this in your code. This is the whole purpose of having an IO block layer to hide the block size requirements of underlying devices. For example, in your flashing code you can do a write of N bytes where N is > 0 and <= 512, the block layer knows that the buffer needs to be a multiple of 512B so it will copy the data into a new buffer and issue a DMA transaction accordingly.

hzhuang1 · 2017-11-29T12:38:26Z

I still don't understand why you can't fix this in your code. This is the whole purpose of having an IO block layer to hide the block size requirements of underlying devices. For example, in your flashing code you can do a write of N bytes where N is > 0 and <= 512, the block layer knows that the buffer needs to be a multiple of 512B so it will copy the data into a new buffer and issue a DMA transaction accordingly.

The key is not in block layer. The key is in the USB driver. Image is downloaded from host to RAM by USB. And it hangs in the USB driver since it's not aligned on 512B. Block layer is only used on flushing data from RAM to storage device.

robertovargas-arm · 2017-11-29T12:43:56Z

We understood that the problem is in the USB driver. What we don't understand is why the USB driver doesn't add the padding bytes to the buffer to be able to transmit and then why you don't use the block io layer to filter these added bytes. Maybe there is some additional problem that we don't understand, but we think that this should be the correct behaviour.

hzhuang1 · 2017-11-29T13:25:35Z

We understood that the problem is in the USB driver. What we don't understand is why the USB driver doesn't add the padding bytes to the buffer to be able to transmit and then why you don't use the block io layer to filter these added bytes. Maybe there is some additional problem that we don't understand, but we think that this should be the correct behaviour.

Since DMA is used in USB driver. Only when data is accumulated to the threshold of FIFO, DMA interrupt is triggered. Otherwise, USB driver is always waiting for the interrupt that never comes. So USB driver doesn't have any chance to add the padding bytes to the buffer.

The key is that we're reading data from USB. If we're writing data to USB, we could add padding bytes.

robertovargas-arm · 2017-11-30T10:20:33Z

Can you give me more information about the USB device that you are using?. What device is it?, is it a mass storage device or a serial device? Can you give me a link to the code of the driver?

hzhuang1 · 2017-11-30T11:23:50Z

Can you give me more information about the USB device that you are using?. What device is it?, is it a mass storage device or a serial device? Can you give me a link to the code of the driver?

It's used to download data from host to target's RAM. It looks much like storage device.

Code is in this link. (
https://github.com/96boards-hikey/OpenPlatformPkg/tree/testing/hikey960_v1.3.4/Drivers/Usb/DwUsbDxe)

robertovargas-arm · 2017-11-30T13:48:47Z

Do you know if the board is the host or the device in the USB communication? What sotfware do you use in the host machine?

hzhuang1 · 2017-11-30T13:50:44Z

Do you know if the board is the host or the device in the USB communication? What sotfware do you use in the host machine?

The board is in device mode. Fastboot is running on the host machine.

robertovargas-arm · 2017-11-30T13:53:51Z

We are going to try to reproduce the problem here. Can you give us a guide how to reproduce the problem?

Leo-Yan · 2017-12-01T01:39:19Z

Hi @robertovargas-arm, except Haojian said the USB driver in UEFI, actually we also have one USB driver in atf-fastboot: https://github.com/96boards-hikey/atf-fastboot; this code is built for Hikey620 recovery mode for downloading image.

Let me take some time to prepare the script for downloading and building code and share back with you.

Leo-Yan · 2017-12-01T10:11:25Z

Hi @robertovargas-arm, you could use below method to reproduce the issue:

download the building script: http://people.linaro.org/~leo.yan/hikey/build_boot_images.sh
after download this script, place it into one folder for source code building: mkdir hikey_boot_src
cd hikey_boot_src; sh build_boot_images.sh
Finally you could see there have binaries in the folder: l-loader, so "cd l-loader";
Let the board enter into recovery mode: link "boot setting jumper" (pins 3-4), you could refer the picture [1] to find the jumper position on the board;
on the PC side, input two commands:
sudo python hisi-idt.py -d /dev/ttyUSB1 --img1=./l-loader.bin
sudo fastboot flash fastboot fip.bin
so you could see fastboot is stuck when download fip.bin file.

[1] https://github.com/96boards/documentation/blob/master/ConsumerEdition/HiKey/AdditionalDocs/Images/Images_HWUserManual/HiKey_Numbered_Front.png

robertovargas-arm · 2017-12-05T10:17:41Z

The board that we have here is the hikey960, can we reproduce the problem with it or do we need a hikey620?

hzhuang1 · 2017-12-08T00:46:57Z

Yes, you need hikey620.

ssg-bot · 2017-12-11T13:58:07Z

Can one of the admins verify this patch?

gitfineon · 2017-12-11T15:51:01Z

Hi @Leo-Yan, your patch does not fix my issue of loading fip.bin using fastboot.

Former ATF version from 96Boards had a rough implementation of TBB for HiKey (620), but this official repo only has a (more generic) version of TBB for some official development platforms from ARM. I tried to port the generic one to be used on HiKey and therefore added some flags to the Makefile.

Adding GENERATE_COT=1 MBEDTLS_DIR=$(MBEDTLS_PATH) USE_TBBR_DEFS=1 does produce an image, which causes the USB driver of fastboot waiting for the final interrupt after 1MB, as you described.

There's no difference between downloading fip.bin in recovery or uefi-fastboot mode. Here you can see the output using DEBUG=1 flag.

Press ESCAPE for boot options Installed Fat filesystem on 3C7B0118
.[Bds]Booting Android Fastboot
add-symbol-file ~/.../optee/edk2/Build/HiKey/DEBUG_GCC49/AARCH64/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp/DEBUG/AndroidFastbootApp.dll 0x381B2000
Loading driver at 0x000381B1000 EntryPoint=0x000381B2000 AndroidFastbootApp.efi
Android Fastboot mode - version 0.6.
Press RETURN or SPACE key to quit.
Fastboot platform: check for partition-type:ptable
Downloading 17408 bytes
#FastbootTransportUsbRequestReceive, 255, BufferSize:17408
   17408 /    17408 bytes downloaded (100%)
Flashing partition ptable
Done.
Fastboot platform: check for partition-type:fastboot
Downloading 1471849 bytes
#FastbootTransportUsbRequestReceive, 255, BufferSize:1471849
 1048576 /  1471849 bytes downloaded (71%)

1471849 mod 512 = 361
1471849 mod 256 = 105
1471849 mod 64 = 41

I applied your 94dc0de.patch but it does not solve the problem. Builds without additional flags work fine, no matter of using your patch or not, even if they are not aligned!?

Example: make recovery with additional certificates in fip.bin

fastboot flash ptable ~/.../optee/build/../l-loader/prm_ptable.img
target reported max download size of 268435456 bytes
sending 'ptable' (17 KB)...
OKAY [  0.001s]
writing 'ptable'...
OKAY [  0.004s]
finished. total time: 0.005s
fastboot flash fastboot ~/.../optee/build/../arm-trusted-firmware/build/hikey/release/fip.bin
target reported max download size of 268435456 bytes
sending 'fastboot' (1441 KB)...
FAILED (status read failed (No such device))
finished. total time: 20.727s
Makefile:506: recipe for target 'flash' failed
make[1]: *** [flash] Error 1
Makefile:469: recipe for target 'recovery' failed
make: *** [recovery] Error 2

To use the workaround from @raw-bin I added a Makefile target.

╰─$ make alignfip                                                                                2 ↵
~/.../optee/build/../build/align-fip.sh ~/.../optee/build/../arm-trusted-firmware/build/hikey/release/fip.bin
size=1475945
requested padding=23
23+0 records in
23+0 records out
23 bytes copied, 6,6951e-05 s, 344 kB/s
size=1475968
Successfully padded fip.bin

1475945 mod 64 = 41
1475968 mod 64 = 0 ✓
1475968 mod 256 = 128

Therefore, my assumption is fastboot requires alignment of 64B (bytes) and not 512B. If you thought of 512 bit, please use a small 'b' to prevent size confusion.

Fastboot accepted the padded file, thanks to @raw-bin.

╰─$ make recovery
Enter recovery mode to flash a new bootloader
...
fastboot flash ptable ~/.../optee/build/../l-loader/prm_ptable.img
target reported max download size of 268435456 bytes
sending 'ptable' (17 KB)...
OKAY [  0.001s]
writing 'ptable'...
OKAY [  0.004s]
finished. total time: 0.005s
fastboot flash fastboot ~/.../optee/build/../arm-trusted-firmware/build/hikey/release/fip.bin
target reported max download size of 268435456 bytes
sending 'fastboot' (1441 KB)...
OKAY [  0.057s]
writing 'fastboot'...
OKAY [  0.067s]
finished. total time: 0.124s
fastboot flash nvme ~/.../..

Patching fastboot seems not to be a good option. Aren't there devices without the possibility to update fastboot? A device specific workaround could be, to add padding in Makefiles, but my read is it should be the case on other fastboot devices, too.

See also:

Leo-Yan · 2017-12-12T07:13:18Z

Hi @gitfineon, just remind except this fip patch, there have two extra modifications so can let Hikey and Hikey960 really apply alignment for ARM-TF building. You could apply patch 591ff98 and 26bb69c for Hikey and Hikey960 respectively.

Following up question: I saw you have pointed out the linkage for the patch "Set alignment size to 512B for Hikey/Hikey960", could you confirm after you apply this patch still cannot see the fip.bin size is not alignment for 512B?

gitfineon · 2017-12-13T10:54:37Z

Hey @Leo-Yan ,

sry, I tried to pass it like this FIP_ALIGN=512 make arm-tf -j7 but the environment variable was not used ... add it to platform.mk e.g. by applying your patch worked fine. Thank you!

Please add a hint to the commit message, requiring FIP_ALIGN is set for the platform.

512 Byte are a lot and it is used for each image, although we only need it at the end.
https://github.com/ARM-software/arm-trusted-firmware/blob/master/tools/fiptool/fiptool.c#L729

/* platform.mk FIP_ALIGN := 512 */
/../arm-trusted-firmware/build/hikey/release/fip.bin
size=1465344 bytes

Using smaller FIP size of 64B (512b) for HiKey as recommended by @raw-bin, in my case saves more than 1.3KB (of zeroes !!!!) and is accepted by fastboot. Why should we use more?

/* platform.mk FIP_ALIGN := 64 */
/../arm-trusted-firmware/build/hikey/release/fip.bin
size=1464000 bytes

fastboot flash fastboot /home/brandmic/AlexAndRalph/optee/build/../arm-trusted-firmware/build/hikey/release/fip.bin
target reported max download size of 268435456 bytes
sending 'fastboot' (1429 KB)...
OKAY [  0.064s]
writing 'fastboot'...
OKAY [  0.066s]
finished. total time: 0.130s

I would prefer an additional parameter to align the overall size of fip.bin if it makes the images smaller and may save internal storage. E.g. using --pre-pad or --post-pad as @masahir0y suggested in this post #952 (comment)

Edit:
@masahir0y mentioned better performance using 512 byte alignment of the eMMC DMA (1c75d5d) and the usb driver may also use 512 byte BS for HighSpeed Mode (https://github.com/96boards-hikey/atf-fastboot/blob/master/plat/hikey/usb.c#L47)

But the documentation says image address and block size are independent from source (https://github.com/ARM-software/arm-trusted-firmware/blame/master/docs/firmware-update.rst#L234). In this case overall BS of 512 byte will speed up USB Transmission but in the end images of fip.bin get realigned to the device by FWU StateMachine?

davidcunado-arm · 2017-12-13T22:27:20Z

@Leo-Yan @gitfineon
From my brief reading of the conversation here, I'm not convinced that a change in ARM TF code is the right way to go, in particular as this may break behaviour that is being relied on.

Anyways, we'll re-review this thread and propose next steps. Thanks!

Leo-Yan · 2017-12-14T02:03:07Z

@gitfineon, thanks for the testing. I think you are right, on Hikey I can see 64B is okay; but I remembered @hzhuang1 has some optimization on Hikey960, so consider to use 512B alignment is for performance optimization. @hzhuang1 is best known for USB and EMMC drivers.

@davidcunado-arm, It's okay for us to hold on this patch, seems to me this patch is same with @masahir0y suggestion for --post-pad. If your team has any conclusion, we can follow up that for this. Thanks for reviewing!

robertovargas-arm · 2017-12-14T09:22:58Z

@hzhuang1 We only have a hikey960 here, and it means that I cannot test it. I am a bit confused, because I still don't understand why it cannot be fixed in the usb drivers that you have. You have the control over the drivers of the two devices in the communication. Is there some reason why you cannot fix it in the drivers? Maybe you can add the padding in the host driver and get rid of it in the device driver. We would prefer a solution of that type, because this PR can generate problems to other people.

ghost · 2017-12-14T15:34:09Z

What about a patch along these lines? This will maintain the constraint that the null entry should point to the end of the file, so the offset of the null entry image is the size of the file.

diff --git a/tools/fiptool/fiptool.c b/tools/fiptool/fiptool.c
index 1dcb7e8e..4ec74001 100644
--- a/tools/fiptool/fiptool.c
+++ b/tools/fiptool/fiptool.c
@@ -492,7 +492,7 @@ static int pack_images(const char *filename, uint64_t toc_flags, unsigned long a
 	fip_toc_header_t *toc_header;
 	fip_toc_entry_t *toc_entry;
 	char *buf;
-	uint64_t entry_offset, buf_size, payload_size = 0;
+	uint64_t entry_offset, buf_size, payload_size = 0, pad_size;
 	size_t nr_images = 0;
 
 	for (desc = image_desc_head; desc != NULL; desc = desc->next)
@@ -528,7 +528,7 @@ static int pack_images(const char *filename, uint64_t toc_flags, unsigned long a
 
 	/* Append a null uuid entry to mark the end of ToC entries. */
 	memset(toc_entry, 0, sizeof(*toc_entry));
-	toc_entry->offset_address = entry_offset;
+	toc_entry->offset_address = (entry_offset + align - 1) & ~(align - 1);
 
 	/* Generate the FIP file. */
 	fp = fopen(filename, "wb");
@@ -555,6 +555,13 @@ static int pack_images(const char *filename, uint64_t toc_flags, unsigned long a
 		xfwrite(image->buffer, image->toc_e.size, fp, filename);
 	}
 
+	if (fseek(fp, entry_offset, SEEK_SET))
+		log_errx("Failed to set file position");
+
+	pad_size = toc_entry->offset_address - entry_offset;
+	while (pad_size--)
+		fputc(0x0, fp);
+
 	fclose(fp);
 	return 0;
 }

davidcunado-arm · 2017-12-20T16:39:07Z

I'm going to close this PR in its current form. We've created #1204 as an alternative solution to the problem that this PR was looking to address.

ghost reviewed Nov 23, 2017

View reviewed changes

davidcunado-arm mentioned this pull request Dec 20, 2017

Add padding at the end of the last entry #1204

Merged

davidcunado-arm closed this Dec 20, 2017

gitfineon mentioned this pull request May 15, 2018

About TRUSTED_BOARD_BOOT and CryptoCell on Hikey OP-TEE/optee_os#2239

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fiptool: Pad zeros for file alignment #1166

fiptool: Pad zeros for file alignment #1166

Leo-Yan commented Nov 22, 2017 •

edited

Loading

ssg-bot commented Nov 22, 2017

ssg-bot commented Nov 22, 2017

Leo-Yan commented Nov 22, 2017

davidcunado-arm commented Nov 22, 2017

Leo-Yan commented Nov 23, 2017

ghost left a comment •

edited by ghost

Loading

ghost commented Nov 23, 2017

Leo-Yan commented Nov 23, 2017 via email

ghost commented Nov 23, 2017

hzhuang1 commented Nov 24, 2017

ghost commented Nov 29, 2017 •

edited by ghost

Loading

hzhuang1 commented Nov 29, 2017

robertovargas-arm commented Nov 29, 2017

hzhuang1 commented Nov 29, 2017

robertovargas-arm commented Nov 30, 2017

hzhuang1 commented Nov 30, 2017

robertovargas-arm commented Nov 30, 2017

hzhuang1 commented Nov 30, 2017

robertovargas-arm commented Nov 30, 2017

Leo-Yan commented Dec 1, 2017

Leo-Yan commented Dec 1, 2017

robertovargas-arm commented Dec 5, 2017

hzhuang1 commented Dec 8, 2017

ssg-bot commented Dec 11, 2017

gitfineon commented Dec 11, 2017

Leo-Yan commented Dec 12, 2017 •

edited

Loading

gitfineon commented Dec 13, 2017 •

edited

Loading

davidcunado-arm commented Dec 13, 2017

Leo-Yan commented Dec 14, 2017 •

edited

Loading

robertovargas-arm commented Dec 14, 2017

ghost commented Dec 14, 2017 •

edited by ghost

Loading

davidcunado-arm commented Dec 20, 2017

fiptool: Pad zeros for file alignment #1166

fiptool: Pad zeros for file alignment #1166

Conversation

Leo-Yan commented Nov 22, 2017 • edited Loading

ssg-bot commented Nov 22, 2017

ssg-bot commented Nov 22, 2017

Leo-Yan commented Nov 22, 2017

davidcunado-arm commented Nov 22, 2017

Leo-Yan commented Nov 23, 2017

ghost left a comment • edited by ghost Loading

Choose a reason for hiding this comment

ghost commented Nov 23, 2017

Leo-Yan commented Nov 23, 2017 via email

ghost commented Nov 23, 2017

hzhuang1 commented Nov 24, 2017

ghost commented Nov 29, 2017 • edited by ghost Loading

hzhuang1 commented Nov 29, 2017

robertovargas-arm commented Nov 29, 2017

hzhuang1 commented Nov 29, 2017

robertovargas-arm commented Nov 30, 2017

hzhuang1 commented Nov 30, 2017

robertovargas-arm commented Nov 30, 2017

hzhuang1 commented Nov 30, 2017

robertovargas-arm commented Nov 30, 2017

Leo-Yan commented Dec 1, 2017

Leo-Yan commented Dec 1, 2017

robertovargas-arm commented Dec 5, 2017

hzhuang1 commented Dec 8, 2017

ssg-bot commented Dec 11, 2017

gitfineon commented Dec 11, 2017

Leo-Yan commented Dec 12, 2017 • edited Loading

gitfineon commented Dec 13, 2017 • edited Loading

davidcunado-arm commented Dec 13, 2017

Leo-Yan commented Dec 14, 2017 • edited Loading

robertovargas-arm commented Dec 14, 2017

ghost commented Dec 14, 2017 • edited by ghost Loading

davidcunado-arm commented Dec 20, 2017

Leo-Yan commented Nov 22, 2017 •

edited

Loading

ghost left a comment •

edited by ghost

Loading

ghost commented Nov 29, 2017 •

edited by ghost

Loading

Leo-Yan commented Dec 12, 2017 •

edited

Loading

gitfineon commented Dec 13, 2017 •

edited

Loading

Leo-Yan commented Dec 14, 2017 •

edited

Loading

ghost commented Dec 14, 2017 •

edited by ghost

Loading