src/snagflash: fastboot: Add support for flashing and splitting android sparse file #53
Conversation
|
Hello, thanks a lot for the contrib! Just one note: Snagflash is already capable of flashing files larger than the bootloader buffer size. You have to use interactive-mode commands for this: https://github.com/bootlin/snagboot/blob/main/docs/snagflash.md#interactive-fastboot-mode When using interactive mode (or an interactive command file), you have to specify the target device, fastboot buffer address, and fastboot buffer size. Snagflash will then take care of splitting files into multiple chunks as needed. The Android Sparse Image approach to flashing large files is actually something I explored myself at first, but I eventually abandonned it because computing the ASI header CRC32 checksum took way too much time for large images. With this in mind, adding Android Sparse Image support to Snagflash could be quite interesting, however, adding a flash_sparse command is not required, since mechanisms to achieve this already exist in Snagflash, as explained above. I hope this helps clear up the situation, I'll give you a more detailed review soon |
hm. which mechanisms ? As mentioned in my comment in bug #54 , sparse file support would make snagflash work with any bootloaders implementing fastboot, not just u-boot, as (iirc), the
Thanks |
I hadn't considered the angle of using a different bootloader. To be honest, Snagboot is currently very heavily tailored towards U-Boot. What specific bootloader are you working with? I'm ready to consider adding a new command for compatibility with different bootloaders, but I'd like to make sure that we don't end up with two different ways of doing the same thing (taking into account that most Snagboot users use U-Boot, so larger-than-RAM-buffer-flashing is already solved for the majority of users). |
|
I've been atm testing with u-boot but at some point I'd like to test android bootloader (little kernel). fwiw, with |
The fastboot_partition_alias mechanism only works for MMC backends in U-Boot, but that could be sidestepped by using the mtdparts environment variable anyway. What's more problematic is the time it takes to compute the CRC32 checksum when generating an Android Sparse Image (taking a quick look at your code, it doesn't look like you're computing this when generating the sparse headers?). Also, one of the main reasons we had for circumventing Fastboot for write commands is that U-Boot currently only supports MMC and raw NAND backends for Fastboot. This means that SPI-NAND and NOR flash devices are not handled. Since using the Fastboot flash command isn't an option for us, I'm not too fond of adding snagflash support for bootloaders other than U-Boot. I feel like it will reduce the overall coherency of the codebase for little gain. Thus, my suggestion is to drop the image splitting and sparse image generation logic. Then, you could add logic in the existing snagflash "flash" command to detect if an image is an Android Sparse Image and flash it in sparse mode if it is, just like we already do for Bmap images. Please let me know what you think. PS: please look into reusing the generic "Header" class defined in snagrecover/firmware/zynqmp_fw.py instead of using struct pack/unpack directly. You can move this class to snagrecover/utils.py and extend it if needed. |
About mmc vs nand backends: IMHO, that's an issue in u-boot, not here. Moreover, in the nand case, it's looking for the MTD partitions names and the command is using partition names, no need for the alias. So, not sure what's the issue here. About the checksum: I'm not even sure it's used in practice. The sparse image generated with the fastboot version I have seems to be generating a checksum 0. u-boot is not even checking it.
yes, the fastboot command is supporting only nand and mmc backends but again, the missing support is an issue in u-boot not snagboot. The command on snagboot side doesn't care about that, it's even bootloader agnostic: It's only asking to flash something in a partition. I've used it to flash on MMC, so it's useful, even with u-boot. The new command not being useful for you use-case, is a different issue. If you don't want contributions about adding support for a use-case you don't care, please, say it loudly.
Not sure to understand what you're suggesting. Your proposing to create an external tool (not in snagboot?) to split the file and that use snagflash to flash it ? So the use-case would be:
Am I missing something ?
oh, sounds great. Looking at that. |
OK well in that case that changes things. If the crc doesn't need to be computed everytime then this sparse flashing method immediately becomes way more relevant in my view.
I partially disagree here, but there's some really confusing terminology in Snagflash that is probably not helping here. Basically, the "interactive mode" commands should really be considered as an "extended Fastboot+U-Boot" mode, that is specifically tailored for U-Boot. I'm planning to change this terminology in the future to make this more clear. So it is very relevant for this support if something doesn't work in U-Boot.
I do care, I just want to make sure that existing users don't become confused by the different available flashing methods. ...
Yeah no that was a bad suggestion, sorry about that. Considering what you told me about crc checking not being mandatory, I do think that flash_sparse would be a viable command to add to snagflash. However, I still don't want it in the "interactive mode" command set (which should really be called "extended Fastboot for U-Boot mode" or something similar). After discussing this issue internally, I've decided that it would be best to separate out "interactive mode" commands into a fourth protocol (passed with the -P option) and keep only "pure Fastboot commands" i.e. no U-Boot specific stuff in the "fastboot" protocol. To be clear, this would look like this: # flashes a small image using Fastboot commands
snagflash -p ... -P fastboot -f download:foo -f flash:bar
# flashes a large image using your Android Sparse Image method
snagflash -p ... -P fastboot -f download:foo -f flash_sparse:bar
# flashes using oem_run and U-Boot commands
snagflash -p ... -P uboot_fastboot -f download:foo -f flash:barSo for now, I'd like you to move your flash_sparse command to src/snagflash/fastboot.py (I'll let you figure out the integration details but don't hesitate to ask me if anything seems unclear). On my side, I'll take care of separating the uboot_fastboot commands out from the "pure fastboot" ones and cleaning up the CLI and terminology as needed. I'll probably also change the interactive mode support so that it can be used for all four protocols and not just the "extended Fastboot" one. Does this seem clear to you? I know this might seem a bit confusing, but there's clearly a terminology issue that I need to fix in this tool. |
Oh, I see. Thanks for explaining this. It was not clear to me.
Ok. Fair enough. I get your point.
It sounds a great idea. No more possible confusions
yes, thanks. I'm going to try implementing that. |
Move the Header class from the zynqmp_fw.py into snagrecover/utils.py to allow reusing this class elsewhere. Signed-off-by: Arnaud Patard <[email protected]>
apatard
force-pushed
the
fastboot_sparse_file
branch
from
b4ef34b to
bd31782
Compare
|
Pushed current work, to get some feedback about it. Mainly, to check if it's going to the wanted direction |
| @@ -0,0 +1,46 @@ | |||
|
|
|||
There was a problem hiding this comment.
You should link to this new doc file from an existing one, or people won't find it. Linking this one from docs/snagflash.md seems appropriate to me.
There was a problem hiding this comment.
I've added a small paragraph to link to this file. I hope it's fine
| CHUNK_TYPE_CRC32 = 0xcac4 | ||
|
|
||
| class SparseFileFormatError(Exception): | ||
| pass |
There was a problem hiding this comment.
I guess this is WIP. When implementing this, you can use logger.error() + sys.exit(-1)
There was a problem hiding this comment.
imho, it's better to still generate the exception and let the caller handle it.
| @@ -30,6 +35,9 @@ | |||
| for more information on fastboot support in U-Boot. | |||
| """ | |||
|
|
|||
| class FastbootError(Exception): | |||
| pass | |||
There was a problem hiding this comment.
same comment as for SparseFileFormatError. Also, could you change other Fastboot methods in this file to use this as well?
There was a problem hiding this comment.
I've updated the other places using raise to use the new exception.
| if not os.path.exists(fname): | ||
| raise FastbootError(f"File {fname} does not exist") | ||
| part = arg_list[1] | ||
| with tempfile.TemporaryDirectory() as tmp: |
There was a problem hiding this comment.
Could you make sure that this context manager automatically remove the directory when you're done with it?
There was a problem hiding this comment.
I've used TemporaryDirectory exactly for this reason: it is design to do the cleaning
|
Your current changes are going in the right direction IMO, I'll do a more thorough review when you submit your final draft. |
Instead of issuing a python backtrace in case of fastboot exception, catch the exception, log it into the logs and exit with error code. Signed-off-by: Arnaud Patard <[email protected]>
To provide better exception message, define an exception class adding a header to the exception message. Signed-off-by: Arnaud Patard <[email protected]>
Add support for flashing android sparse files with flashboot. The main purpose is to allow flashing files bigger than the bootloader buffer size by splitting them into 2 or more files. Signed-off-by: Arnaud Patard <[email protected]>
apatard
force-pushed
the
fastboot_sparse_file
branch
from
bd31782 to
9a9388e
Compare
Add support for flashing android sparse files with flashboot. The main purpose is to allow flashing files bigger than the bootloader buffer size by splitting them into 2 or more files.