Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33387 )
Change subject: mainboard/emulation/qemu-aarch64: Add new board for ARMv8 ......................................................................
Patch Set 15:
(13 comments)
Right now it won't work, as the payload will be loaded to _dram.
SELF payloads contain their own load address, so they should work as long as they got built for the right address. FIT payloads should automatically find a free spot, but you're right that it's better to leave some room for it at the beginning of DRAM.
It would be nice to use the FDT passed by qemu to coreboot, as right now we have to manually specify a FDT when building the uImage/FIT.
I'd like to avoid adding infrastructure to receive an FDT from masked ROM and pipe it through all stages for use in a FIT image, because that's a very QEMU-specific use case and doesn't make much sense on real hardware. An FDT in ROM can't get updated so it's generally a bad idea, and the FIT model of bundling the FDT with the kernel works much better. For the QEMU case, I think it would be easier to just take whatever FDT they would pass and manually add it to FIT images you want to run on it.
https://review.coreboot.org/c/coreboot/+/33387/15/src/cpu/armltd/cortex-a53/... File src/cpu/armltd/cortex-a53/Kconfig:
https://review.coreboot.org/c/coreboot/+/33387/15/src/cpu/armltd/cortex-a53/... PS15, Line 1: config CPU_ARMLTD_CORTEX_A53 Note that src/cpu is pretty much deprecated, at least for non-x86. The remaining Arm stuff there is just still there because nobody had the time to clean it up, but all Arm chips added in the last ~5 years only use src/soc. For QEMU, I think it would be best to either keep everything in the mainboard directory, or create a new src/soc/qemu/arm64.
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/Kconfig:
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 19: select ARCH_RAMSTAGE_ARMV8_64 If you select these here directly, what does the CPU_ARMLTD_CORTEX_A53 even do?
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/bootblock_custom.S:
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 26: b copy_code nit: I think the branch should be unnecessary because .p2align should fill with NOPs in a text section.
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 32: ldp q0, q1, [x1], 32 /* Load 32 bytes */ Should be okay to use memcpy() here instead? You can put the stuff you want preserved into x19-x29 instead.
Also, we don't really use SIMD registers anywhere else, even if it works for QEMU I'd just stick to GPRs instead here.
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 50: main Sorry, where's this main()? I don't see it.
It would be best if you could reuse src/lib/bootblock.c here, but you'd have to change the Makefile first because that currently doesn't get built for BOOTBLOCK_CUSTOM (we can probably just remove that restriction and have the boards that want a custom C entry point use a different function name instead).
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/cbmem.c:
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 40: int probe_ramsize(void) Can this reuse what Patrick is doing in CB:33934?
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/debug.S:
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 11: * Just debugging. I will remove it later. When is later? Looks like this is unused.
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 10: device i2c 6 on end # Fake component for testing Note that Arm devices usually don't use devicetree.cb for peripherals or board components, so having this here is odd.
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/include/mainboard/addressmap.h:
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 28: #define VIRT_FLASH_START 0x00000000 nit: convention is to call these _BASE, not _START
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 36: #define VIRT_DRAM_START 0x40000000 For things that appear in memlayout, the convention is to have the constant directly in memlayout rather than using defines (and using the memlayout symbol to reference it in code). The idea is that you can see where what is from just looking at the memlayout file, without having to cross-reference it with a bunch of headers.
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/mainboard.c:
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 1: /*
cbmem_recovery needs to be called in romstage, too
arm64/romstage.c main() calls cbmem_initialize_empty(), that should be enough. Arm devices usually don't recover CBMEM in romstage (they just initialize it empty), because they don't wake from S3 through firmware.
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 29: cbmem_recovery(0); Why is this here? main() from hardwaremain.c calls cbmem_initialize(), that should be all you need.
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 17: DRAM_START(VIRT_DRAM_START) Like Patrick said, it's best to leave some free space at the beginning of DRAM for FIT images because the Linux kernel likes to be loaded at the start of physical memory on arm64. I'd recommend 512MB if you can spare that much.