Attention is currently required from: Jason Glenesk, Raul Rangel, Martin L Roth, ritul guru, Marshall Dawson, Matt DeVillier, Paul Menzel, Arthur Heymans, Fred Reitberger.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/65523 )
Change subject: soc/amd/common/psp: Support PSP SMI handler ......................................................................
Patch Set 24:
(6 comments)
Patchset:
PS24: i'm still not fully done with reviewing psp_smi.c, but already found some things to address
File src/soc/amd/common/block/psp/psp_gen2.c:
https://review.coreboot.org/c/coreboot/+/65523/comment/85a64017_118767c7 PS24, Line 119: static bool is_valid_psp_spi_id(u64 id) : { : return id == SMI_TARGET_NVRAM || : id == SMI_TARGET_RPMC_NVRAM; : } : : bool is_valid_psp_spi_info(struct mbox_default_buffer *buffer) : { : struct mbox_pspv2_cmd_spi_info *cmd = (struct mbox_pspv2_cmd_spi_info *)buffer; : return is_valid_psp_spi_id(read64(&cmd->req.id)); : } : : bool is_valid_psp_spi_read_write(struct mbox_default_buffer *buffer) : { : struct mbox_pspv2_cmd_spi_read_write *cmd = : (struct mbox_pspv2_cmd_spi_read_write *)buffer; : return is_valid_psp_spi_id(read64(&cmd->req.id)); : } : : bool is_valid_psp_spi_erase(struct mbox_default_buffer *buffer) : { : struct mbox_pspv2_cmd_spi_erase *cmd = (struct mbox_pspv2_cmd_spi_erase *)buffer; : return is_valid_psp_spi_id(read64(&cmd->req.id)); : } : : u64 get_psp_spi_info_id(struct mbox_default_buffer *buffer) : { : struct mbox_pspv2_cmd_spi_info *cmd = (struct mbox_pspv2_cmd_spi_info *)buffer; : return read64(&cmd->req.id); : } : : void set_psp_spi_info(struct mbox_default_buffer *buffer, : u64 lba, u64 block_size, u64 num_blocks) : { : struct mbox_pspv2_cmd_spi_info *cmd = (struct mbox_pspv2_cmd_spi_info *)buffer; : write32(&cmd->req.lba, lba); : write32(&cmd->req.block_size, block_size); : write32(&cmd->req.num_blocks, num_blocks); : } : : void get_psp_spi_read_write(struct mbox_default_buffer *buffer, : u64 *id, u64 *lba, u64 *offset, : u64 *num_bytes, u8 **data) : { : struct mbox_pspv2_cmd_spi_read_write *cmd = : (struct mbox_pspv2_cmd_spi_read_write *)buffer; : *id = read64(&cmd->req.id); : *lba = read64(&cmd->req.lba); : *offset = read64(&cmd->req.offset); : *num_bytes = read64(&cmd->req.num_bytes); : *data = cmd->req.buffer; : } : : void get_psp_spi_erase(struct mbox_default_buffer *buffer, : u64 *id, u64 *lba, u64 *num_blocks) : { : struct mbox_pspv2_cmd_spi_erase *cmd = (struct mbox_pspv2_cmd_spi_erase *)buffer; : *id = read64(&cmd->req.id); : *lba = read64(&cmd->req.lba); : *num_blocks = read64(&cmd->req.num_blocks); : } : all these newly added functions only get used in soc/amd/common/block/psp/psp_smi.c and i don't see a case where those would be needed outside of the smi-handler, since they all operate on p2c_buffer that resides in smm memory, so i'd like those to be moved to the psp_smi.c compilation unit. after having most of the code, i agree that my initial suggestion to move those to soc/amd/common/block/psp/psp_p2c.c wouldn't have improved things
File src/soc/amd/common/block/psp/psp_smi.c:
https://review.coreboot.org/c/coreboot/+/65523/comment/b64ca0e1_31f1d0c9 PS24, Line 63: int since this basically returns a bool, i'd change the return type to bool
https://review.coreboot.org/c/coreboot/+/65523/comment/cf6643a9_f3ba14d3 PS24, Line 68: static enum mbox_p2c_status find_psp_spi_flash_device_region(const struct spi_flash **flash) this function is missing the struct region_device *store parameter and the id parameter
https://review.coreboot.org/c/coreboot/+/65523/comment/e92d6cc1_44c9fff6 PS24, Line 68: const struct spi_flash **flash only flash->sector_size is used from this struct in all cases, so i'd replace this parameter with u64 *block_size and write the value we need to that pointer: *block_size = flash->sector_size
https://review.coreboot.org/c/coreboot/+/65523/comment/5df0e42f_da629383 PS24, Line 100: find_spi_flash_device_region find_psp_spi_flash_device_region
-
Felix Held (Code Review)