Asami Doi 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 34: Code-Review+1
(47 comments)
Sorry for lots of "Done" comments. It's just to make all comments marked as a resolved.
Supporting QEMU/AArch64 is not perfect yet but I think it's enough to support a base feature.
I will create 2 follow-up patches in a different CL to support QEMU/AArch64 perfectly: 1. Update probe_ramsize() for armv8 to avoid an exception happening when it tries to access an address more than its memory. 2. The integrated LinuxBoot doesn't work well yet. I will investigate why it doesn't work and fix it.
https://review.coreboot.org/c/coreboot/+/33387/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33387/11//COMMIT_MSG@9 PS11, Line 9: WIP
Sorry, this is still WIP. The state changed to ready automatically when I replied.
Done
https://review.coreboot.org/c/coreboot/+/33387/21/Documentation/mainboard/em... File Documentation/mainboard/emulation/qemu-aarch64.md:
https://review.coreboot.org/c/coreboot/+/33387/21/Documentation/mainboard/em... PS21, Line 30: ### 4. Add the payload to coreboot.rom by util/cbfstool
Sorry, what did you mean?
Done
https://review.coreboot.org/c/coreboot/+/33387/21/Documentation/mainboard/em... PS21, Line 38: - The default CPU in QEMU for AArch64 is a cortex-a15 which is 32-bit ARM CPU. You need to specify 64-bit ARM CPU via `-cpu cortex-a53`.
I moved the "Running coreboot in QEMU" column to the top of this page.
Done
https://review.coreboot.org/c/coreboot/+/33387/11/src/arch/arm64/armv8/cache... File src/arch/arm64/armv8/cache.c:
https://review.coreboot.org/c/coreboot/+/33387/11/src/arch/arm64/armv8/cache... PS11, Line 42:
OK. Thank you for your advice. […]
Done
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
I deleted cpu/armld/cortex-a53 because they have nothing functions for now. […]
Done
https://review.coreboot.org/c/coreboot/+/33387/6/src/mainboard/emulation/qem... File src/mainboard/emulation/qemu-aarch64/Kconfig:
https://review.coreboot.org/c/coreboot/+/33387/6/src/mainboard/emulation/qem... PS6, Line 6: ## This software is licensed under the terms of the GNU General Public
OK. I updated all files to use the SPDX identifier. […]
Done
https://review.coreboot.org/c/coreboot/+/33387/6/src/mainboard/emulation/qem... PS6, Line 20: # qemu-system-aarch64 -M virt -m 1024M -cpu cortex-a53 -nographic -bios build/coreboot.rom
Yes. I moved it to a help section in Kconfig.name.
Done
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
cpu/armld/cortex-a53 have nothing functions for now. So I deleted them.
Done
https://review.coreboot.org/c/coreboot/+/33387/29/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/Kconfig:
https://review.coreboot.org/c/coreboot/+/33387/29/src/mainboard/emulation/qe... PS29, Line 48: config DRAM_SIZE_MB
Yes, but this still needs to be integrated there, not hacked around it. […]
I see. I will update probe_ramsize() in a different patch. Thank you.
https://review.coreboot.org/c/coreboot/+/33387/18/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/33387/18/src/mainboard/emulation/qe... PS18, Line 27: bootblock-y += ../../../lib/bootblock.c
That's good to know! I rebased this CL on the CL 34250 and deleted the TODO comment and the bootbloc […]
Done
https://review.coreboot.org/c/coreboot/+/33387/6/src/mainboard/emulation/qem... File src/mainboard/emulation/qemu-aarch64/bootblock_custom.S:
https://review.coreboot.org/c/coreboot/+/33387/6/src/mainboard/emulation/qem... PS6, Line 36: works
Yes, it does. I will remove this function.
Done
https://review.coreboot.org/c/coreboot/+/33387/11/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/bootblock_custom.S:
https://review.coreboot.org/c/coreboot/+/33387/11/src/mainboard/emulation/qe... PS11, Line 6: * SPDX-License-Identifier: GPL-2.0-or-later
According to a lint checker, coreboot seems to support a GPL-2.0-or-later. […]
Done
https://review.coreboot.org/c/coreboot/+/33387/11/src/mainboard/emulation/qe... PS11, Line 20: EL1
Yes, I could run QEMU in EL3. I will remove this function.
Done
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 11: ENTRY(_start)
Right, sounds good.
Done
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 32: ldp q0, q1, [x1], 32 /* Load 32 bytes */
Yes, it's ok. I used normal instructions and general purpose registers instead.
Done
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 45: /* Jump to code in DRAM */
Sorry, where is the FDT pointer?
Done
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 50: main
I see. OK, I created a new CL to deal with the problem. […]
Done
https://review.coreboot.org/c/coreboot/+/33387/16/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/bootblock_custom.S:
https://review.coreboot.org/c/coreboot/+/33387/16/src/mainboard/emulation/qe... PS16, Line 12: .org 0
No, it's just explicit for developers. I removed it.
Done
https://review.coreboot.org/c/coreboot/+/33387/16/src/mainboard/emulation/qe... PS16, Line 13: ic ialluis
No, it doesn't affect the output. Thank you for pointing it out.
Done
https://review.coreboot.org/c/coreboot/+/33387/16/src/mainboard/emulation/qe... PS16, Line 30: x19
I didn't notice that there is memcpy in src/arch/arm64/memcpy.S. It can be simpler. […]
Done
https://review.coreboot.org/c/coreboot/+/33387/16/src/mainboard/emulation/qe... PS16, Line 43: ldr x1, =_bootblock
Thank you for your simpler way. I changed your code a little bit but I used it. […]
Done
https://review.coreboot.org/c/coreboot/+/33387/6/src/mainboard/emulation/qem... File src/mainboard/emulation/qemu-aarch64/cbmem.c:
https://review.coreboot.org/c/coreboot/+/33387/6/src/mainboard/emulation/qem... PS6, Line 28: _eprogram
Yes, cbmem_top() must return the same address in every stage. If your code does that it's fine.
Done
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)
It seems good. I will reuse it. […]
Done
https://review.coreboot.org/c/coreboot/+/33387/27/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/cbmem.c:
https://review.coreboot.org/c/coreboot/+/33387/27/src/mainboard/emulation/qe... PS27, Line 49: 1
Instead of the function, I used new probe_ramsize() and probe_mb() defined at src/lib/ramdetect. […]
Done
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.
Sorry, this is already unused. I removed it.
Done
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
Thank you for your comment. Is it enough to pass FDT for ARM to setup device configurations? […]
Done
https://review.coreboot.org/c/coreboot/+/33387/18/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33387/18/src/mainboard/emulation/qe... PS18, Line 8: chip drivers/generic/generic # I2C0 controller
When I deleted this file or made this file empty, I got the error: […]
Done
https://review.coreboot.org/c/coreboot/+/33387/33/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/exception.c:
https://review.coreboot.org/c/coreboot/+/33387/33/src/mainboard/emulation/qe... PS33, Line 19: return EXC_RET_IGNORED;
When you want it to skip the exception, it needs to return EXC_RET_HANDLED. […]
Sorry, I'm still not sure what EXC_RET_HANDLED and EXC_RET_IGNORED are doing.
Is my understanding correct? 1. Define 2 global functions for returning EXC_RET_HANDLED/EXC_RET_IGNORED. 2. At the beginning of the probe_ramsize(), set the exception handler with EXC_RET_HANDLED. 3. At the end of the probe_ramsize(), set the exception handler with EXC_RET_IGNORED because we don't want to skip an exception anymore.
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 36: #define VIRT_DRAM_START 0x40000000
I moved FLASH_BASE and DRAM_BASE to memlayout.ld and other definitions remain in this file.
Done
https://review.coreboot.org/c/coreboot/+/33387/6/src/mainboard/emulation/qem... File src/mainboard/emulation/qemu-aarch64/mainboard.c:
https://review.coreboot.org/c/coreboot/+/33387/6/src/mainboard/emulation/qem... PS6, Line 32: 0x4c000000
Can you #define the constants
Done
https://review.coreboot.org/c/coreboot/+/33387/6/src/mainboard/emulation/qem... PS6, Line 54: discovered
s/discovered/ram_size_mb/
Done
https://review.coreboot.org/c/coreboot/+/33387/6/src/mainboard/emulation/qem... PS6, Line 62: 0x60000000
#define the constant
Done
https://review.coreboot.org/c/coreboot/+/33387/11/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/mainboard.c:
https://review.coreboot.org/c/coreboot/+/33387/11/src/mainboard/emulation/qe... PS11, Line 31: 4
I'm still working on this file so I will do it later. Thank you for your advice.
Done
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: /*
I wasn't aware of that, thanks.
Done
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 22: if (!dev) {
OK. I removed the if-statement.
Done
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 28: ram_resource(dev, 0, VIRT_DRAM_START >> 10, discovered << 10);
Thank you for the reference. I updated this file but I'm not sure it's perfect. […]
Done
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 29: cbmem_recovery(0);
Sorry, I copied this file from mainboard/emulation/qemu-armv7/. I removed this line.
Done
https://review.coreboot.org/c/coreboot/+/33387/18/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/mainboard.c:
https://review.coreboot.org/c/coreboot/+/33387/18/src/mainboard/emulation/qe... PS18, Line 18: #define VIRT_DRAM_BASE 0x40000000
You shouldn't need this, it's already in memlayout
Done
https://review.coreboot.org/c/coreboot/+/33387/18/src/mainboard/emulation/qe... PS18, Line 20: static void mainboard_read_resources(struct device *dev)
looks good
Done
https://review.coreboot.org/c/coreboot/+/33387/6/src/mainboard/emulation/qem... File src/mainboard/emulation/qemu-aarch64/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/33387/6/src/mainboard/emulation/qem... PS6, Line 47: VIRT_FLASH_START
Does the placing BOOTBLOCK in DRAM mean it doesn't use Cache-As-RAM? […]
Done
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)
OK, thank you for your advice. […]
Done
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 19: 0x10000
This is an arbitrary number as long as a region doesn't overlap. […]
Done
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 24: 0x01040000
This is the number just after the end of RAMSTAGE. […]
Done
https://review.coreboot.org/c/coreboot/+/33387/16/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/33387/16/src/mainboard/emulation/qe... PS16, Line 28: VIRT_DRAM_BASE + FIT_SPACE + GT_128KiB
Thank you for the references. I defined addresses in a simpler way.
Done
https://review.coreboot.org/c/coreboot/+/33387/18/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/33387/18/src/mainboard/emulation/qe... PS18, Line 37: * 0x6111_0000..: TTB
I removed lines from "0x0000_0000..0x0080_0000: Flash memory" to "0x6111_0000..: TTB".
Done
https://review.coreboot.org/c/coreboot/+/33387/18/src/mainboard/emulation/qe... PS18, Line 43: DRAM_START(VIRT_DRAM_BASE)
OK. I put on the number instead of using #define.
Done
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/timer.c:
https://review.coreboot.org/c/coreboot/+/33387/15/src/mainboard/emulation/qe... PS15, Line 9: void udelay(unsigned int n);
Oh yeah, you are right. CNTFRQ is already set by qemu.
Done