Attention is currently required from: Jason Glenesk, Raul Rangel, ritul guru, Marshall Dawson, Paul Menzel, Aamir Bohra, Fred Reitberger, Felix Held. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/60968 )
Change subject: soc/amd/common/block/psp: Add platform secure boot support ......................................................................
Patch Set 17:
(11 comments)
File src/soc/amd/common/block/psp/psb.c:
https://review.coreboot.org/c/coreboot/+/60968/comment/ea39bfbd_e17926e1 PS17, Line 18: /* PSB Test Status and Error Codes (doc#56654) */
As psb_test_status_to_string() is called in error scenarios so all error status codes are mentioned […]
See my comments below, PSB_TEST_STATUS_PASS can be used in the check before printing the error string.
https://review.coreboot.org/c/coreboot/+/60968/comment/1b491b37_b8e7741d PS17, Line 30:
As fuse_status_to_string() is called in error scenarios, so all error status codes are mentioned and […]
See my comments below, FUSE_STATUS_SUCCESS can be used in the check before printing the error string.
https://review.coreboot.org/c/coreboot/+/60968/comment/3527710d_bf808e25 PS17, Line 35: error: Is this prefix needed? These strings are printed with BIOS_ERR log level.
https://review.coreboot.org/c/coreboot/+/60968/comment/a0fc3b8f_9e54a702 PS17, Line 35: static const char *psb_test_status_fuse_read_err = "error: Error reading fuse info"; : : static const char *psb_test_status_bios_key_bad_usage = : "error: OEM BIOS signing key usage flag violation"; : : static const char *psb_test_status_bios_rtm_sig_noent = : "error: BIOS RTM signature entry not found"; : : static const char *psb_test_status_bios_rtm_copy_err = : "error: BIOS copy to DRAM failed"; : : static const char *psb_test_status_bios_rtm_bad_sig = : "error: BIOS RTM signature verification failed"; : : static const char *psb_test_status_bios_key_bad_sig = : "error: OEM BIOS signing key failed signature verification"; : : static const char *psb_test_status_platform_bad_id = : "error: Platform vendor id and/or model id binding violation"; : : static const char *psb_test_status_bios_copy_bit_unset = : "error: BIOS copy bit unset for reset image"; : : static const char *psb_test_status_bios_ca_bad_sig = : "error: OEM BIOS signing CA key failed signature verification"; : : static const char *psb_test_status_bios_ca_bad_usage = : "error: OEM BIOS signing CA key usage flag violation"; : : static const char *psb_test_status_bios_key_bad_revision = : "error: OEM BIOS signing key revision violation"; : : static const char *psb_test_status_unknown = "error: Unknown failure"; Why not use the string literals directly in the code?
https://review.coreboot.org/c/coreboot/+/60968/comment/2105fb49_1bb76560 PS17, Line 127: read32 You can use `read32p` and drop the cast
https://review.coreboot.org/c/coreboot/+/60968/comment/2c6bc98d_81b070df PS17, Line 167: psb_test_status = status & PSB_TEST_STATUS_MASK; nit: I'd make this const and declare/initialize it in one line:
const u32 psb_test_status = status & PSB_TEST_STATUS_MASK;
https://review.coreboot.org/c/coreboot/+/60968/comment/76dd6ce9_6b042def PS17, Line 169: if (psb_test_status) { This assumes that 0 means success. Why not make it explicit by defining `PSB_TEST_STATUS_PASS` as Felix Held suggested?
if (psb_test_status != PSB_TEST_STATUS_PASS)
https://review.coreboot.org/c/coreboot/+/60968/comment/1223879f_b7b26643 PS17, Line 181: cmd_status = send_psp_command(MBOX_BIOS_CMD_PSB_AUTO_FUSING, &buffer); nit: I'd make this const and declare/initialize it in one line:
const int cmd_status = send_psp_command(MBOX_BIOS_CMD_PSB_AUTO_FUSING, &buffer);
https://review.coreboot.org/c/coreboot/+/60968/comment/e094ee4f_37a280b6 PS17, Line 190: fuse_status = read32(&buffer.header.status); nit: I'd make this const and declare/initialize it in one line:
const u32 fuse_status = read32(&buffer.header.status);
https://review.coreboot.org/c/coreboot/+/60968/comment/d2de3ed8_3825e166 PS17, Line 191: if (fuse_status) { This assumes that 0 means success. Why not make it explicit by defining `FUSE_STATUS_SUCCESS` as Felix Held suggested?
if (fuse_status != FUSE_STATUS_SUCCESS)
https://review.coreboot.org/c/coreboot/+/60968/comment/81dab1ae_291564c2 PS17, Line 206: * nit: drop empty comment line