Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41816 )
Change subject: soc/amd/picasso: add psp_verstage ......................................................................
Patch Set 9:
(20 comments)
https://review.coreboot.org/c/coreboot/+/41816/4/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/41816/4/src/soc/amd/picasso/Kconfig... PS4, Line 385: config VERSTAGE_IN_USERSPACE
Where do we need this option to make decisions?
It was originally needed to exclude some arm files, but that's no longer required.
Removed.
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/Kconfig... PS9, Line 13: VBOOT &&
Is this necessary? Don't think VERSTAGE will do anything without VBOOT.
Done
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/Makefil... PS9, Line 26: arch/x86/memmove.c
Why not add bootblock-y += memmove.c to arch/x86/Makefile. […]
Done in a follow-on patch.
Note that this is actually pretty common practice in the coreboot code.
% grep -r --include='Make*' "+= .." | wc -l 560
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/chip.h File src/soc/amd/picasso/chip.h:
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/chip.h@... PS9, Line 15: #include <arch/x86/include/arch/smp/mpspec.h> /* point from top level */
hrmm, just delete the include. It's not used. irq_override only uses uint8_t.
I tried, but then we get errors for 'MP_IRQ_TRIGGER_LEVEL' undeclared and 'MP_IRQ_POLARITY_HIGH' undeclared
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/memlayo... File src/soc/amd/picasso/memlayout_psp_verstage.ld:
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/memlayo... PS9, Line 35: . = VERSTAGE_START; : _sram = .;
SRAM_START macro
Done
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/memlayo... PS9, Line 52: VERSTAGE_SIZE
Is this a hard limit or a self imposed one?
No, it's not a hard limit. We just needed a size.
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/memlayo... PS9, Line 53: . = VBOOT_WORK_START; : _vboot2_work = .; : _evboot2_work = VBOOT_WORK_START + VBOOT_WORK_SIZE; : . = _evboot2_work;
Maybe use the REGION macro?
Done
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/memlayo... PS9, Line 59: _fmap_cache = .; : _efmap_cache = _fmap_cache + FMAP_CACHE_SIZE;
FMAP_CACHE(. […]
Done
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/memlayo... PS9, Line 65: _stack = PSP_VERSTAGE_STACK_START; : PSP_VERSTAGE_STACK_BASE = _stack; : _estack = PSP_VERSTAGE_STACK_START + PSP_VERSTAGE_STACK_SIZE;
REGION
Done
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/memlayo... PS9, Line 69: _esram = SRAM_START + SRAM_SIZE;
SRAM_END
Done
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/psp_ver... PS9, Line 3: ifeq ($(CONFIG_VBOOT_STARTS_BEFORE_BOOTBLOCK),y)
You are already guarding the subdir in the parent dir. I would remove this.
Done
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/fch.c:
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/psp_ver... PS9, Line 34: void * bar
clang-format
Done
https://review.coreboot.org/c/coreboot/+/41816/8/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/include/arch/io.h:
https://review.coreboot.org/c/coreboot/+/41816/8/src/soc/amd/picasso/psp_ver... PS8, Line 9: outb
inb and outb are used all over, but not too much in the code that we're using for psp_verstage. […]
Done. b/159936581
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.h:
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/psp_ver... PS9, Line 8:
nit: Add tabs
Done
https://review.coreboot.org/c/coreboot/+/41816/7/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.c:
https://review.coreboot.org/c/coreboot/+/41816/7/src/soc/amd/picasso/psp_ver... PS7, Line 140: /* Do not use printk() before verstage_mainboard_init() is called */
Because IO isn't initialized until that point, and the system will hang.
Updated the comment.
https://review.coreboot.org/c/coreboot/+/41816/7/src/soc/amd/picasso/psp_ver... PS7, Line 156: svc_write_postcode(0x01);
Thanks, I meant to go back and add #defines for these. […]
Done
https://review.coreboot.org/c/coreboot/+/41816/7/src/soc/amd/picasso/psp_ver... PS7, Line 186: svc_write_postcode(0xF1);
Yes. These were the first ones implemented and they never got updated.
Done
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.c:
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/psp_ver... PS9, Line 161: svc_debug_print
No message?
Left-over debug code. Removed.
https://review.coreboot.org/c/coreboot/+/41816/9/src/soc/amd/picasso/psp_ver... PS9, Line 210: stage_entry
What's this for?
This is needed so that we don't have to drop a bunch of ARM files. It's not used directly, but because the arm/arch/header.h file marks stage_entry as the entry point, if it's not here, verstage doesn't get built. We end up with a ~300 byte verstage. This was my slightly hacky, but mostly harmless solution.
https://review.coreboot.org/c/coreboot/+/41816/7/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/vboot_crypto.c:
https://review.coreboot.org/c/coreboot/+/41816/7/src/soc/amd/picasso/psp_ver... PS7, Line 62: sha_op.DataLen = MIN(size, 1024);
That's a VERY good question. […]
Updated. Changing the size didn't seem to make a difference.