Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34367 )
Change subject: util: Add new util to make a payload for QEMU/AArch64 ......................................................................
Patch Set 1:
(7 comments)
This change is ready for review.
https://review.coreboot.org/c/coreboot/+/34367/1/util/qemu_aarch64/Makefile File util/qemu_aarch64/Makefile:
https://review.coreboot.org/c/coreboot/+/34367/1/util/qemu_aarch64/Makefile@... PS1, Line 8: payload: linux virt.dtb Ideally, all output should go into a subdirectory ("out" or "build" or such) and there should be a 'clean' target to delete that.
https://review.coreboot.org/c/coreboot/+/34367/1/util/qemu_aarch64/Makefile@... PS1, Line 13: #TODO: Look up how to compress the kernel. I don't think this should be in this Makefile TBH, it's a little far out of scope. Just tell people to pass a VMLINUZ=... variable in here (or zImage or however the kernel is called for aarch64 again).
https://review.coreboot.org/c/coreboot/+/34367/1/util/qemu_aarch64/Makefile@... PS1, Line 17: nit: maybe a 'help' target that's also the default target that explains the other available targets?
https://review.coreboot.org/c/coreboot/+/34367/1/util/qemu_aarch64/payload.i... File util/qemu_aarch64/payload.its:
https://review.coreboot.org/c/coreboot/+/34367/1/util/qemu_aarch64/payload.i... PS1, Line 10: arch = "arm"; I think this should be arm64? (coreboot doesn't actually use it.)
https://review.coreboot.org/c/coreboot/+/34367/1/util/qemu_aarch64/payload.i... PS1, Line 12: compression = "gzip"; coreboot doesn't support gzip compression, we only have "lzma" and "lz4". You can compress a file with
cbfs-compression-tool rawcompress <infile> <outfile> <algo>
Using cbfs-compression-tool has the advantage that the user doesn't need the official utility installed first. It should normally be available under coreboot/build/util/ (although if someone passed a custom obj= path to the main coreboot make invocation it would be somewhere else... I guess you'll need to give the user a way to manually override the path for that case).
https://review.coreboot.org/c/coreboot/+/34367/1/util/qemu_aarch64/payload.i... PS1, Line 13: load = <0x40000000>; Nothing below here is used by coreboot, I think you might as well leave it out (or does mkimage choke then?).
https://review.coreboot.org/c/coreboot/+/34367/1/util/qemu_aarch64/payload.i... PS1, Line 36: }; You also need a config node like this:
configurations { default = "conf@1"; conf@1 { kernel = "kernel@1"; fdt = "fdt@1"; compatible = "qemu,qemu-aarch64" } }