Hello Marshall Dawson, Marshall Dawson,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/40295
to review the following change.
Change subject: soc/amd/common/psp: Add SmmInfo command ......................................................................
soc/amd/common/psp: Add SmmInfo command
Implement the MboxBiosCmdSmmInfo function to inform the PSP of the soc's SMM configuration. Once the BootDone command is sent, the PSP only responds to commands where the buffer is in SM memory.
Set aside a region for the core-to-PSP command buffer and the PSP-to-core mailbox. Also add an SMM flag, which the PSP expects to read as non-zero during an SMI.
The soc should call the new function only from an SMI handler. Add calls to soc functions for the soc to populate the trigger info and register info (v2 only).
Change-Id: I10088a53e786db788740e4b388650641339dae75 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/common/block/include/amdblocks/psp.h M src/soc/amd/common/block/psp/psp.c M src/soc/amd/common/block/psp/psp_def.h 3 files changed, 147 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/40295/1
diff --git a/src/soc/amd/common/block/include/amdblocks/psp.h b/src/soc/amd/common/block/include/amdblocks/psp.h index 53946fb..c084bb1 100644 --- a/src/soc/amd/common/block/include/amdblocks/psp.h +++ b/src/soc/amd/common/block/include/amdblocks/psp.h @@ -7,6 +7,39 @@ /* Get the mailbox base address - specific to family of device. */ void *soc_get_mbox_address(void);
+#define SMM_TRIGGER_IO 0 +#define SMM_TRIGGER_MEM 1 + +#define SMM_TRIGGER_BYTE 0 +#define SMM_TRIGGER_WORD 1 +#define SMM_TRIGGER_DWORD 2 + +struct smm_trigger_info { + uint64_t address; /* Memory or IO address */ + uint32_t address_type; /* 0=I/O, 1=memory */ + uint32_t value_width; /* 0=byte, 1=word, 2=qword */ + uint32_t value_and_mask; + uint32_t value_or_mask; +} __packed; + +struct smm_register { + uint64_t address; /* Memory or IO address */ + uint32_t address_type; /* 0=I/O, 1=memory */ + uint32_t value_width; /* 0=byte, 1=word, 2=qword */ + uint32_t reg_bit_mask; + uint32_t expect_value; +} __packed; + +struct smm_register_info { + struct smm_register smi_enb; + struct smm_register eos; + struct smm_register psp_smi_en; + struct smm_register reserved[5]; +} __packed; + +void soc_fill_smm_trig_info(struct smm_trigger_info *trig); +void soc_fill_smm_reg_info(struct smm_register_info *reg); /* v2 only */ + /* BIOS-to-PSP functions return 0 if successful, else negative value */ #define PSPSTS_SUCCESS 0 #define PSPSTS_NOBASE 1 @@ -22,6 +55,8 @@
int psp_notify_dram(void);
+int psp_notify_smm(void); + /* * type: identical to the corresponding PSP command, e.g. pass * MBOX_BIOS_CMD_SMU_FW2 to load SMU FW2 blob. diff --git a/src/soc/amd/common/block/psp/psp.c b/src/soc/amd/common/block/psp/psp.c index 77517c0..9baabf7 100644 --- a/src/soc/amd/common/block/psp/psp.c +++ b/src/soc/amd/common/block/psp/psp.c @@ -3,11 +3,13 @@
#include <device/mmio.h> #include <cpu/x86/msr.h> +#include <cpu/amd/msr.h> #include <cbfs.h> #include <region_file.h> #include <timer.h> #include <device/pci_def.h> #include <bootstate.h> +#include <rules.h> #include <console/console.h> #include <device/pci_ops.h> #include <amdblocks/psp.h> @@ -15,6 +17,33 @@ #include <soc/northbridge.h> #include "psp_def.h"
+struct _c2p_buffer { + u8 buffer[C2P_BUFFER_MAXSIZE]; +} __attribute__((aligned(32))); + +struct _p2c_buffer { + u8 buffer[P2C_BUFFER_MAXSIZE]; +} __attribute__((aligned(32))); + +static struct _p2c_buffer p2c_buffer; +static struct _c2p_buffer c2p_buffer; + +static uint32_t smm_flag; /* Non-zero for SMM, clear when not */ + +static void set_smm_flag(void) +{ + if (!(ENV_SMM)) + return; + smm_flag = 1; +} + +static void clear_smm_flag(void) +{ + if (!(ENV_SMM)) + return; + smm_flag = 0; +} + static const char *psp_status_nobase = "error: PSP BAR3 not assigned"; static const char *psp_status_halted = "error: PSP in halted state"; static const char *psp_status_recovery = "error: PSP recovery required"; @@ -111,13 +140,18 @@ if (v1_wait_command(mbox)) return -PSPSTS_CMD_TIMEOUT;
- /* set address of command-response buffer and write command register */ + /* set smm flag, address of command-response buffer and write command */ + set_smm_flag(); v1_wr_mbox_cmd_resp(mbox, buffer); v1_wr_mbox_cmd(mbox, command);
/* PSP clears command register when complete */ - if (v1_wait_command(mbox)) + if (v1_wait_command(mbox)) { + clear_smm_flag(); return -PSPSTS_CMD_TIMEOUT; + } + + clear_smm_flag();
/* check delivery status */ if (v1_rd_mbox_sts(mbox) & (PSPV1_STATUS_ERROR | PSPV1_STATUS_TERMINATED)) @@ -192,15 +226,20 @@ if (v2_wait_command(mbox, true)) return -PSPSTS_CMD_TIMEOUT;
- /* set address of command-response buffer and write command register */ + /* set smm flag, address of command-response buffer and write command */ + set_smm_flag(); v2_wr_mbox_cmd_resp(mbox, buffer); v2_wr_mbox_cmd(mbox, command);
/* PSP clears command register when complete. All commands except * SxInfo set the Ready bit. */ if (v2_wait_command(mbox, - command == MBOX_BIOS_CMD_SX_INFO ? false : true)) + command == MBOX_BIOS_CMD_SX_INFO ? false : true)) { + clear_smm_flag(); return -PSPSTS_CMD_TIMEOUT; + } + + clear_smm_flag();
/* check delivery status */ if (v2_rd_mbox_sts(mbox)) @@ -282,6 +321,48 @@ print_cmd_status(cmd_status, &buffer); }
+int psp_notify_smm(void) +{ + msr_t msr; + int cmd_status; + struct mbox_cmd_smm_info_buffer buffer = { + .header = { + .size = sizeof(buffer) + } + }; + + if (!(ENV_SMM)) { + printk (BIOS_ERR, "PSP: Error, cannot send SMM info from outside SMM\n"); + return PSPSTS_UNSUPPORTED; + } + + msr = rdmsr(SMM_ADDR_MSR); + buffer.req.smm_base = ((uint64_t)msr.hi << 32) | msr.lo; + msr = rdmsr(SMM_MASK_MSR); + msr.lo &= 0xfffff000; + buffer.req.smm_mask = ((uint64_t)msr.hi << 32) | msr.lo; + + soc_fill_smm_trig_info(&buffer.req.smm_trig_info); +#if (CONFIG(SOC_AMD_COMMON_BLOCK_PSP_GEN2)) + soc_fill_smm_reg_info(&buffer.req.smm_reg_info); +#endif + + buffer.req.psp_smm_data_region = (uintptr_t)p2c_buffer.buffer; + buffer.req.psp_smm_data_length = sizeof(p2c_buffer); + + buffer.req.psp_mbox_smm_buffer_address = (uintptr_t)c2p_buffer.buffer; + buffer.req.psp_mbox_smm_flag_address = (uintptr_t)&smm_flag; + + printk(BIOS_DEBUG, "PSP: Notify SMM info... "); + + cmd_status = send_psp_command(MBOX_BIOS_CMD_SMM_INFO, &buffer); + + /* buffer's status shouldn't change but report it if it does */ + print_cmd_status(cmd_status, (struct mbox_default_buffer *)&buffer); + + return cmd_status; +} + /* * Tell the PSP to load a firmware blob from a location in the BIOS image. */ diff --git a/src/soc/amd/common/block/psp/psp_def.h b/src/soc/amd/common/block/psp/psp_def.h index dd0c907..f101fb3 100644 --- a/src/soc/amd/common/block/psp/psp_def.h +++ b/src/soc/amd/common/block/psp/psp_def.h @@ -6,6 +6,7 @@
#include <types.h> #include <commonlib/helpers.h> +#include <amdblocks/psp.h>
/* x86 to PSP commands */ #define MBOX_BIOS_CMD_DRAM_INFO 0x01 @@ -81,6 +82,24 @@ struct mbox_buffer_header header; } __attribute__((packed, aligned(32)));
+struct smm_req_buffer { + uint64_t smm_base; /* TSEG base */ + uint64_t smm_mask; /* TSEG mask */ + uint64_t psp_smm_data_region; /* PSP region in SMM space */ + uint64_t psp_smm_data_length; /* PSP region length in SMM space */ + struct smm_trigger_info smm_trig_info; +#if CONFIG(SOC_AMD_COMMON_BLOCK_PSP_GEN2) + struct smm_register_info smm_reg_info; +#endif + uint64_t psp_mbox_smm_buffer_address; + uint64_t psp_mbox_smm_flag_address; +} __packed; + +struct mbox_cmd_smm_info_buffer { + struct mbox_buffer_header header; + struct smm_req_buffer req; +} __attribute__((packed, aligned(32))); + struct mbox_cmd_sx_info_buffer { struct mbox_buffer_header header; u8 sleep_type; @@ -89,4 +108,12 @@ #define PSP_INIT_TIMEOUT 10000 /* 10 seconds */ #define PSP_CMD_TIMEOUT 1000 /* 1 second */
+#if ENV_SMM +#define C2P_BUFFER_MAXSIZE 0xc00 /* Core-to-PSP buffer */ +#define P2C_BUFFER_MAXSIZE 0xc00 /* PSP-to-core buffer */ +#else +#define C2P_BUFFER_MAXSIZE 0 +#define P2C_BUFFER_MAXSIZE 0 +#endif + #endif /* __AMD_PSP_DEF_H__ */
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40295 )
Change subject: soc/amd/common/psp: Add SmmInfo command ......................................................................
Patch Set 1:
(23 comments)
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/psp.h:
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/in... PS1, Line 18: uint64_t address; /* Memory or IO address */ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/in... PS1, Line 19: uint32_t address_type; /* 0=I/O, 1=memory */ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/in... PS1, Line 20: uint32_t value_width; /* 0=byte, 1=word, 2=qword */ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/in... PS1, Line 21: uint32_t value_and_mask; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/in... PS1, Line 22: uint32_t value_or_mask; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/in... PS1, Line 26: uint64_t address; /* Memory or IO address */ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/in... PS1, Line 27: uint32_t address_type; /* 0=I/O, 1=memory */ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/in... PS1, Line 28: uint32_t value_width; /* 0=byte, 1=word, 2=qword */ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/in... PS1, Line 29: uint32_t reg_bit_mask; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/in... PS1, Line 30: uint32_t expect_value; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/in... PS1, Line 34: struct smm_register smi_enb; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/in... PS1, Line 35: struct smm_register eos; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/in... PS1, Line 36: struct smm_register psp_smi_en; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/in... PS1, Line 37: struct smm_register reserved[5]; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/ps... File src/soc/amd/common/block/psp/psp.c:
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/ps... PS1, Line 335: printk (BIOS_ERR, "PSP: Error, cannot send SMM info from outside SMM\n"); space prohibited between function name and open parenthesis '('
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/ps... File src/soc/amd/common/block/psp/psp_def.h:
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/ps... PS1, Line 86: uint64_t smm_base; /* TSEG base */ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/ps... PS1, Line 87: uint64_t smm_mask; /* TSEG mask */ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/ps... PS1, Line 88: uint64_t psp_smm_data_region; /* PSP region in SMM space */ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/ps... PS1, Line 89: uint64_t psp_smm_data_length; /* PSP region length in SMM space */ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/ps... PS1, Line 90: struct smm_trigger_info smm_trig_info; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/ps... PS1, Line 92: struct smm_register_info smm_reg_info; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/ps... PS1, Line 94: uint64_t psp_mbox_smm_buffer_address; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/ps... PS1, Line 95: uint64_t psp_mbox_smm_flag_address; please, no spaces at the start of a line
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40295 )
Change subject: soc/amd/common/psp: Add SmmInfo command ......................................................................
Patch Set 1: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/40295/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40295/1//COMMIT_MSG@17 PS1, Line 17: calls A tad bit too long? limit is 72 chars
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/ps... File src/soc/amd/common/block/psp/psp_def.h:
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/ps... PS1, Line 86: uint64_t smm_base; /* TSEG base */
please, no spaces at the start of a line
Mr. Jenkins has a point.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40295 )
Change subject: soc/amd/common/psp: Add SmmInfo command ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/ps... File src/soc/amd/common/block/psp/psp_def.h:
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/ps... PS1, Line 86: uint64_t smm_base; /* TSEG base */
Mr. Jenkins has a point.
ouch, didn't notice that when cherry-picking the patches
Marshall Dawson has uploaded a new patch set (#2) to the change originally created by Felix Held. ( https://review.coreboot.org/c/coreboot/+/40295 )
Change subject: soc/amd/common/psp: Add SmmInfo command ......................................................................
soc/amd/common/psp: Add SmmInfo command
Implement the MboxBiosCmdSmmInfo function to inform the PSP of the soc's SMM configuration. Once the BootDone command is sent, the PSP only responds to commands where the buffer is in SM memory.
Set aside a region for the core-to-PSP command buffer and the PSP-to-core mailbox. Also add an SMM flag, which the PSP expects to read as non-zero during an SMI.
The soc should call the new function only from an SMI handler. Add calls to soc functions for the soc to populate the trigger info and register info (v2 only).
Change-Id: I10088a53e786db788740e4b388650641339dae75 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/common/block/include/amdblocks/psp.h M src/soc/amd/common/block/psp/psp.c M src/soc/amd/common/block/psp/psp_def.h 3 files changed, 147 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/40295/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40295 )
Change subject: soc/amd/common/psp: Add SmmInfo command ......................................................................
Patch Set 2:
(23 comments)
https://review.coreboot.org/c/coreboot/+/40295/2/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/psp.h:
https://review.coreboot.org/c/coreboot/+/40295/2/src/soc/amd/common/block/in... PS2, Line 18: uint64_t address; /* Memory or IO address */ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/2/src/soc/amd/common/block/in... PS2, Line 19: uint32_t address_type; /* 0=I/O, 1=memory */ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/2/src/soc/amd/common/block/in... PS2, Line 20: uint32_t value_width; /* 0=byte, 1=word, 2=qword */ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/2/src/soc/amd/common/block/in... PS2, Line 21: uint32_t value_and_mask; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/2/src/soc/amd/common/block/in... PS2, Line 22: uint32_t value_or_mask; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/2/src/soc/amd/common/block/in... PS2, Line 26: uint64_t address; /* Memory or IO address */ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/2/src/soc/amd/common/block/in... PS2, Line 27: uint32_t address_type; /* 0=I/O, 1=memory */ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/2/src/soc/amd/common/block/in... PS2, Line 28: uint32_t value_width; /* 0=byte, 1=word, 2=qword */ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/2/src/soc/amd/common/block/in... PS2, Line 29: uint32_t reg_bit_mask; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/2/src/soc/amd/common/block/in... PS2, Line 30: uint32_t expect_value; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/2/src/soc/amd/common/block/in... PS2, Line 34: struct smm_register smi_enb; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/2/src/soc/amd/common/block/in... PS2, Line 35: struct smm_register eos; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/2/src/soc/amd/common/block/in... PS2, Line 36: struct smm_register psp_smi_en; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/2/src/soc/amd/common/block/in... PS2, Line 37: struct smm_register reserved[5]; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/2/src/soc/amd/common/block/ps... File src/soc/amd/common/block/psp/psp.c:
https://review.coreboot.org/c/coreboot/+/40295/2/src/soc/amd/common/block/ps... PS2, Line 346: printk (BIOS_ERR, "PSP: Error, cannot send SMM info from outside SMM\n"); space prohibited between function name and open parenthesis '('
https://review.coreboot.org/c/coreboot/+/40295/2/src/soc/amd/common/block/ps... File src/soc/amd/common/block/psp/psp_def.h:
https://review.coreboot.org/c/coreboot/+/40295/2/src/soc/amd/common/block/ps... PS2, Line 86: uint64_t smm_base; /* TSEG base */ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/2/src/soc/amd/common/block/ps... PS2, Line 87: uint64_t smm_mask; /* TSEG mask */ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/2/src/soc/amd/common/block/ps... PS2, Line 88: uint64_t psp_smm_data_region; /* PSP region in SMM space */ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/2/src/soc/amd/common/block/ps... PS2, Line 89: uint64_t psp_smm_data_length; /* PSP region length in SMM space */ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/2/src/soc/amd/common/block/ps... PS2, Line 90: struct smm_trigger_info smm_trig_info; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/2/src/soc/amd/common/block/ps... PS2, Line 92: struct smm_register_info smm_reg_info; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/2/src/soc/amd/common/block/ps... PS2, Line 94: uint64_t psp_mbox_smm_buffer_address; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/40295/2/src/soc/amd/common/block/ps... PS2, Line 95: uint64_t psp_mbox_smm_flag_address; please, no spaces at the start of a line
Marshall Dawson has uploaded a new patch set (#3) to the change originally created by Felix Held. ( https://review.coreboot.org/c/coreboot/+/40295 )
Change subject: soc/amd/common/psp: Add SmmInfo command ......................................................................
soc/amd/common/psp: Add SmmInfo command
Implement the MboxBiosCmdSmmInfo function to inform the PSP of the soc's SMM configuration. Once the BootDone command is sent, the PSP only responds to commands where the buffer is in SM memory.
Set aside a region for the core-to-PSP command buffer and the PSP-to-core mailbox. Also add an SMM flag, which the PSP expects to read as non-zero during an SMI.
The soc should call the new function only from an SMI handler. Add calls to soc functions for the soc to populate the trigger info and register info (v2 only).
Change-Id: I10088a53e786db788740e4b388650641339dae75 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/common/block/include/amdblocks/psp.h M src/soc/amd/common/block/psp/psp.c M src/soc/amd/common/block/psp/psp_def.h 3 files changed, 147 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/40295/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40295 )
Change subject: soc/amd/common/psp: Add SmmInfo command ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40295/3/src/soc/amd/common/block/ps... File src/soc/amd/common/block/psp/psp.c:
https://review.coreboot.org/c/coreboot/+/40295/3/src/soc/amd/common/block/ps... PS3, Line 346: printk (BIOS_ERR, "PSP: Error, cannot send SMM info from outside SMM\n"); space prohibited between function name and open parenthesis '('
Marshall Dawson has uploaded a new patch set (#4) to the change originally created by Felix Held. ( https://review.coreboot.org/c/coreboot/+/40295 )
Change subject: soc/amd/common/psp: Add SmmInfo command ......................................................................
soc/amd/common/psp: Add SmmInfo command
Implement the MboxBiosCmdSmmInfo function to inform the PSP of the soc's SMM configuration. Once the BootDone command is sent, the PSP only responds to commands where the buffer is in SM memory.
Set aside a region for the core-to-PSP command buffer and the PSP-to-core mailbox. Also add an SMM flag, which the PSP expects to read as non-zero during an SMI.
The soc should call the new function only from an SMI handler. Add calls to soc functions for the soc to populate the trigger info and register info (v2 only).
Change-Id: I10088a53e786db788740e4b388650641339dae75 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/common/block/include/amdblocks/psp.h M src/soc/amd/common/block/psp/psp.c M src/soc/amd/common/block/psp/psp_def.h 3 files changed, 147 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/40295/4
Hello build bot (Jenkins), Marshall Dawson, Marshall Dawson, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40295
to look at the new patch set (#5).
Change subject: soc/amd/common/psp: Add SmmInfo command ......................................................................
soc/amd/common/psp: Add SmmInfo command
Implement the MboxBiosCmdSmmInfo function to inform the PSP of the SoC's SMM configuration. Once the BootDone command is sent, the PSP only responds to commands where the buffer is in SMM memory.
Set aside a region for the core-to-PSP command buffer and the PSP-to-core mailbox. Also add an SMM flag, which the PSP expects to read as non-zero during an SMI.
The soc should call the new function only from an SMI handler. Add calls to soc functions for the soc to populate the trigger info and register info (v2 only).
BUG=b:153677737
Change-Id: I10088a53e786db788740e4b388650641339dae75 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/common/block/include/amdblocks/psp.h M src/soc/amd/common/block/psp/psp.c M src/soc/amd/common/block/psp/psp_def.h M src/soc/amd/common/block/psp/psp_gen1.c M src/soc/amd/common/block/psp/psp_gen2.c 5 files changed, 160 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/40295/5
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40295 )
Change subject: soc/amd/common/psp: Add SmmInfo command ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40295/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40295/1//COMMIT_MSG@17 PS1, Line 17: calls
A tad bit too long? limit is 72 chars
Done
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/ps... File src/soc/amd/common/block/psp/psp_def.h:
https://review.coreboot.org/c/coreboot/+/40295/1/src/soc/amd/common/block/ps... PS1, Line 86: uint64_t smm_base; /* TSEG base */
ouch, didn't notice that when cherry-picking the patches
Done
Hello build bot (Jenkins), Marshall Dawson, Marshall Dawson, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40295
to look at the new patch set (#6).
Change subject: soc/amd/common/psp: Add SmmInfo command ......................................................................
soc/amd/common/psp: Add SmmInfo command
Implement the MboxBiosCmdSmmInfo function to inform the PSP of the SoC's SMM configuration. Once the BootDone command is sent, the PSP only responds to commands where the buffer is in SMM memory.
Set aside a region for the core-to-PSP command buffer and the PSP-to-core mailbox. Also add an SMM flag, which the PSP expects to read as non-zero during an SMI.
The soc should call the new function only from an SMI handler. Add calls to soc functions for the soc to populate the trigger info and register info (v2 only).
BUG=b:153677737
Change-Id: I10088a53e786db788740e4b388650641339dae75 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/common/block/include/amdblocks/psp.h M src/soc/amd/common/block/psp/psp.c M src/soc/amd/common/block/psp/psp_def.h M src/soc/amd/common/block/psp/psp_gen1.c M src/soc/amd/common/block/psp/psp_gen2.c 5 files changed, 160 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/40295/6
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40295 )
Change subject: soc/amd/common/psp: Add SmmInfo command ......................................................................
Patch Set 6: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... File src/soc/amd/common/block/psp/psp.c:
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 161: soc_fill_smm_reg_info If there's a declaration for this function in a header, you can use a regular conditional instead of preprocessor, but I don't have a preference
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40295 )
Change subject: soc/amd/common/psp: Add SmmInfo command ......................................................................
Patch Set 6:
(10 comments)
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/psp.h:
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/in... PS6, Line 17: nit: space
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... File src/soc/amd/common/block/psp/psp.c:
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 18: c2p What does c2p mean?
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 22: struct _p2c_buffer { : u8 buffer[P2C_BUFFER_MAXSIZE]; : } __attribute__((aligned(32))); : : static struct _p2c_buffer p2c_buffer; Could make this an anon stuct since it's only used once:
struct { u8 buffer[SIZE]; } __attribute__((aligned(32))) p2c_buffer;
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 31: set_smm_flag Can we make this function only get linked in the smm stage. Either a #ifdef around the method, or place it in a new file and add it to smm-y += smm-flag.c.
I don't like it silently returning.
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 38: clear_smm_flag Same
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 148: ENV_SMM Same, only link in SMM
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 156: 0xfffff000 #def the mask or comment what it means.
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 161: soc_fill_smm_reg_info
If there's a declaration for this function in a header, you can use a regular conditional instead of […]
+1
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 164: buffer.req.psp_smm_data_region = (uintptr_t)p2c_buffer.buffer; : buffer.req.psp_smm_data_length = sizeof(p2c_buffer); : : buffer.req.psp_mbox_smm_buffer_address = (uintptr_t)c2p_buffer.buffer; : buffer.req.psp_mbox_smm_flag_address = (uintptr_t)&smm_flag; nit: you could set these static fields in the buffer initializer above.
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... File src/soc/amd/common/block/psp/psp_def.h:
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 111: ENV_SMM Maybe move the buffer and the functions into a psp-smm file so you can avoid this #if.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40295 )
Change subject: soc/amd/common/psp: Add SmmInfo command ......................................................................
Patch Set 6: Code-Review-1
currently refactoring this
Hello build bot (Jenkins), Raul Rangel, Marshall Dawson, Marshall Dawson, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40295
to look at the new patch set (#7).
Change subject: soc/amd/psp: Add SmmInfo command ......................................................................
soc/amd/psp: Add SmmInfo command
Implement the MboxBiosCmdSmmInfo function to inform the PSP of the SoC's SMM configuration. Once the BootDone command is sent, the PSP only responds to commands where the buffer is in SMM memory.
Set aside a region for the core-to-PSP command buffer and the PSP-to-core mailbox. Also add an SMM flag, which the PSP expects to read as non-zero during an SMI.
The soc should call the new function only from an SMI handler. Add calls to soc functions for the soc to populate the trigger info and register info (v2 only).
Add functions to set up the structures needed for the SmmInfo function in Picasso support. Issue a SW SMI, and add a new handler to call the new PSP function.
BUG=b:153677737
Change-Id: I10088a53e786db788740e4b388650641339dae75 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/common/block/include/amdblocks/psp.h M src/soc/amd/common/block/psp/Makefile.inc M src/soc/amd/common/block/psp/psp_def.h A src/soc/amd/common/block/psp/psp_smm.c M src/soc/amd/picasso/include/soc/smi.h M src/soc/amd/picasso/psp.c M src/soc/amd/picasso/smi.c M src/soc/amd/picasso/smihandler.c 8 files changed, 192 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/40295/7
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40295 )
Change subject: soc/amd/psp: Add SmmInfo command ......................................................................
Patch Set 7:
(9 comments)
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/psp.h:
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/in... PS6, Line 17:
nit: space
Done
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... File src/soc/amd/common/block/psp/psp.c:
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 18: c2p
What does c2p mean?
moved the define for C2P_BUFFER_MAXSIZE that has a comment on this
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 22: struct _p2c_buffer { : u8 buffer[P2C_BUFFER_MAXSIZE]; : } __attribute__((aligned(32))); : : static struct _p2c_buffer p2c_buffer;
Could make this an anon stuct since it's only used once: […]
Done
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 31: set_smm_flag
Can we make this function only get linked in the smm stage. […]
Done
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 38: clear_smm_flag
Same
Done
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 148: ENV_SMM
Same, only link in SMM
Done
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 161: soc_fill_smm_reg_info
+1
tried this and the compiler complained that buffer.req has no member smm_reg_info. wontfix.
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 164: buffer.req.psp_smm_data_region = (uintptr_t)p2c_buffer.buffer; : buffer.req.psp_smm_data_length = sizeof(p2c_buffer); : : buffer.req.psp_mbox_smm_buffer_address = (uintptr_t)c2p_buffer.buffer; : buffer.req.psp_mbox_smm_flag_address = (uintptr_t)&smm_flag;
nit: you could set these static fields in the buffer initializer above.
Done
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... File src/soc/amd/common/block/psp/psp_def.h:
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 111: ENV_SMM
Maybe move the buffer and the functions into a psp-smm file so you can avoid this #if.
Done
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40295 )
Change subject: soc/amd/psp: Add SmmInfo command ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... File src/soc/amd/common/block/psp/psp.c:
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 156: 0xfffff000
#def the mask or comment what it means.
good question why it's made sure here that the mask is set in a way that the granularity is at least 4k
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40295 )
Change subject: soc/amd/psp: Add SmmInfo command ......................................................................
Patch Set 7: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40295 )
Change subject: soc/amd/psp: Add SmmInfo command ......................................................................
Patch Set 7: Code-Review+2
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40295 )
Change subject: soc/amd/psp: Add SmmInfo command ......................................................................
Patch Set 7: Code-Review-1
(1 comment)
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... File src/soc/amd/common/block/psp/psp.c:
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 156: 0xfffff000
good question why it's made sure here that the mask is set in a way that the granularity is at least […]
this masks SMM_LOCK and SMM_TSEG_VALID, but not SMM_TSEG_WB. needs more investigation; I'd suspect that this should be 0xffff0000 instead
Hello build bot (Jenkins), Raul Rangel, Patrick Georgi, Martin Roth, Marshall Dawson, Marshall Dawson, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40295
to look at the new patch set (#8).
Change subject: soc/amd/psp: Add SmmInfo command ......................................................................
soc/amd/psp: Add SmmInfo command
Implement the MboxBiosCmdSmmInfo function to inform the PSP of the SoC's SMM configuration. Once the BootDone command is sent, the PSP only responds to commands where the buffer is in SMM memory.
Set aside a region for the core-to-PSP command buffer and the PSP-to-core mailbox. Also add an SMM flag, which the PSP expects to read as non-zero during an SMI.
The soc should call the new function only from an SMI handler. Add calls to soc functions for the soc to populate the trigger info and register info (v2 only).
Add functions to set up the structures needed for the SmmInfo function in Picasso support. Issue a SW SMI, and add a new handler to call the new PSP function.
BUG=b:153677737
Change-Id: I10088a53e786db788740e4b388650641339dae75 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/common/block/include/amdblocks/psp.h M src/soc/amd/common/block/psp/Makefile.inc M src/soc/amd/common/block/psp/psp_def.h A src/soc/amd/common/block/psp/psp_smm.c M src/soc/amd/picasso/include/soc/smi.h M src/soc/amd/picasso/psp.c M src/soc/amd/picasso/smi.c M src/soc/amd/picasso/smihandler.c 8 files changed, 192 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/40295/8
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40295 )
Change subject: soc/amd/psp: Add SmmInfo command ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... File src/soc/amd/common/block/psp/psp.c:
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 156: 0xfffff000
this masks SMM_LOCK and SMM_TSEG_VALID, but not SMM_TSEG_WB. […]
fixed and added comment
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40295 )
Change subject: soc/amd/psp: Add SmmInfo command ......................................................................
Patch Set 8: Code-Review+2
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40295 )
Change subject: soc/amd/psp: Add SmmInfo command ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40295/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40295/8//COMMIT_MSG@17 PS8, Line 17: The soc should call the new function only from an SMI handler. Only a nit here. AFAIK the x86 can be in any mode when the SmmInfo call is sent to the PSP. The main reason we put it in the SMI handler is so that buffers, addresses, etc. are easily identifiable. If we tried calling it from ramstage instead, it would be difficult to know where a buffer was going to wind up inside the smm.elf.
Hello build bot (Jenkins), Raul Rangel, Patrick Georgi, Martin Roth, Marshall Dawson, Marshall Dawson, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40295
to look at the new patch set (#9).
Change subject: soc/amd/psp: Add SmmInfo command ......................................................................
soc/amd/psp: Add SmmInfo command
Implement the MboxBiosCmdSmmInfo function to inform the PSP of the SoC's SMM configuration. Once the BootDone command is sent, the PSP only responds to commands where the buffer is in SMM memory.
Set aside a region for the core-to-PSP command buffer and the PSP-to-core mailbox. Also add an SMM flag, which the PSP expects to read as non-zero during an SMI.
Add calls to soc functions for the soc to populate the trigger info and register info (v2 only).
Add functions to set up the structures needed for the SmmInfo function in Picasso support. Issue a SW SMI, and add a new handler to call the new PSP function.
BUG=b:153677737
Change-Id: I10088a53e786db788740e4b388650641339dae75 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/common/block/include/amdblocks/psp.h M src/soc/amd/common/block/psp/Makefile.inc M src/soc/amd/common/block/psp/psp_def.h A src/soc/amd/common/block/psp/psp_smm.c M src/soc/amd/picasso/include/soc/smi.h M src/soc/amd/picasso/psp.c M src/soc/amd/picasso/smi.c M src/soc/amd/picasso/smihandler.c 8 files changed, 192 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/40295/9
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40295 )
Change subject: soc/amd/psp: Add SmmInfo command ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40295/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40295/8//COMMIT_MSG@17 PS8, Line 17: The soc should call the new function only from an SMI handler.
Only a nit here. AFAIK the x86 can be in any mode when the SmmInfo call is sent to the PSP. […]
Done
-
Angel Pons (Code Review) -
build bot (Jenkins) (Code Review)
-
Felix Held (Code Review)
-
Marshall Dawson (Code Review)
-
Raul Rangel (Code Review)