Attention is currently required from: Bao Zheng, Martin Roth, Nikolai Vyssotski, Zheng Bao.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/78274?usp=email )
Change subject: amdfwtool: Set the table size for L1 separately ......................................................................
Patch Set 12: Code-Review+1
(4 comments)
Patchset:
PS12: would be good to add a comment to the code to clarify things a bit, but apart from that the code looks correct to me
File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/78274/comment/992be749_26967440 : PS11, Line 615: 1
This patch is only for family which PSP header points to ISH header. […]
i thought of having one patch that changes the alignments and one that addresses the psp l1 size calculation in the ish case, but since this patch is about making sure that the l1 table size is correctly reported, that is something to maybe look into in a separate patch
https://review.coreboot.org/c/coreboot/+/78274/comment/1bf8b44a_7a91cbcf : PS11, Line 655: if (cookie == PSP_COOKIE && cb_config->need_ish) : table_size = TABLE_ALIGNMENT; : else
In integrate_psp_firmwares the new_ish_dir is called. […]
good point. had a look at the documentation and it says that in the ISH case, the PSP L1 directory table can have at most 32 entries, so setting table_size to TABLE_ALIGNMENT is valid in all cases so the code will result in the correct behavior. would be good to a comment here to make it easier to understand why this is done and correct by the spec
File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/78274/comment/b9180903_305ba8fa : PS12, Line 576: uint32_t align_end = cb_config->need_ish ? TABLE_ALIGNMENT : 1; shouldn't the table be always aligned to TABLE_ALIGNMENT? i'm fine with leaving this as it is right now and doing that change in a follow-up patch; possibly together with the other alignment changes i suggested looking into
-
Felix Held (Code Review)