Change in coreboot[master]: src/soc/amd/common/fsp: check FSP binary version.

[Name of user not set (Code Review)]

Attention is currently required from: Julian Schroeder.
Hello Julian Schroeder,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/58869

to review the following change.

Change subject: src/soc/amd/common/fsp: check FSP binary version.
......................................................................

src/soc/amd/common/fsp: check FSP binary version.

The FSP encodes image revision data that is use to determine
if the coreboot header file fspmupd.h and the binary are com-
patible. If the image revision major version does not match
coreboot will die().
In case of of image revision minor version not matching, only
a warning is output.

BUG=b:184650244
TEST=verify image revision output in the coreboot serial log.
     compare to FSP version shown in serial debug output.

Signed-off-by: Julian Schroeder 
Change-Id: Ibf50f16b5e9793d946a95970fcdabc4c07289646
---
M src/drivers/intel/fsp2_0/include/fsp/info_header.h
M src/drivers/intel/fsp2_0/include/fsp/util.h
M src/drivers/intel/fsp2_0/util.c
M src/soc/amd/common/fsp/fsp_validate.c
M src/vendorcode/amd/fsp/cezanne/FspmUpd.h
5 files changed, 67 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/58869/1

diff --git a/src/drivers/intel/fsp2_0/include/fsp/info_header.h b/src/drivers/intel/fsp2_0/include/fsp/info_header.h
index 5b6318c..85e4c3f 100644
--- a/src/drivers/intel/fsp2_0/include/fsp/info_header.h
+++ b/src/drivers/intel/fsp2_0/include/fsp/info_header.h
@@ -10,7 +10,7 @@
 #define FSP_HDR_ATTRIB_FSPT		1
 #define FSP_HDR_ATTRIB_FSPM		2
 #define FSP_HDR_ATTRIB_FSPS		3
-
+//jsd
 #if CONFIG(PLATFORM_USES_FSP2_X86_32)
 struct fsp_header {
    uint32_t fsp_revision;
@@ -30,6 +30,29 @@
    char image_id[sizeof(uint64_t) + 1];
    uint8_t revision;
 } __packed;
+
+#define  FSP_INFO_HEADER_OFF    0x94
+struct fsp_info_header {
+uint32_t  signature; //FSPH
+uint32_t  header_length;
+uint8_t   res1[3];
+uint8_t   header_revision;
+uint32_t  image_revision;
+char      image_id[8];
+uint32_t  image_size;
+uint32_t  image_base;
+uint32_t  image_attribute;
+uint32_t  cfg_region_offset;
+uint32_t  cfg_region_size;
+uint32_t  api_entry_num;
+uint32_t  temp_ram_init_entry_offset; //initial stack
+uint32_t  fsp_init_entry_offset;
+uint32_t  notify_phase_entry_offset;
+uint32_t  fsp_memory_init_entry_offset;
+uint32_t  temp_ram_exit_entry_offset;
+uint32_t  fsp_silicon_init_entry_offset;
+}  __packed;
+
 #else
 #error You need to implement this struct for x86_64 FSP
 #endif
diff --git a/src/drivers/intel/fsp2_0/include/fsp/util.h b/src/drivers/intel/fsp2_0/include/fsp/util.h
index d05b644..cdec105 100644
--- a/src/drivers/intel/fsp2_0/include/fsp/util.h
+++ b/src/drivers/intel/fsp2_0/include/fsp/util.h
@@ -109,7 +109,7 @@
 void fsp_verify_upd_header_signature(uint64_t upd_signature, uint64_t expected_signature);
 void lb_string_platform_blob_version(struct lb_header *header);
 void report_fspt_output(void);
-void soc_validate_fspm_header(const struct fsp_header *hdr);
+void soc_validate_fspm_header(const struct fsp_info_header *hdr);

/* Fill in header and validate a loaded FSP component. */
 enum cb_err fsp_validate_component(struct fsp_header *hdr, void *fsp_blob, size_t size);
diff --git a/src/drivers/intel/fsp2_0/util.c b/src/drivers/intel/fsp2_0/util.c
index 1128013..e0818d6 100644
--- a/src/drivers/intel/fsp2_0/util.c
+++ b/src/drivers/intel/fsp2_0/util.c
@@ -77,6 +77,7 @@
 enum cb_err fsp_validate_component(struct fsp_header *hdr, void *fsp_file, size_t file_size)
 {
    void *raw_hdr = fsp_file + FSP_HDR_OFFSET;
+	struct fsp_info_header *info = (struct fsp_info_header *) (fsp_file + FSP_INFO_HEADER_OFF);

if (file_size < FSP_HDR_OFFSET + fsp_hdr_get_expected_min_length()) {
    	printk(BIOS_CRIT, "FSP blob too small.\n");
@@ -98,7 +99,7 @@
    }

if (ENV_ROMSTAGE)
-		soc_validate_fspm_header(hdr);
+		soc_validate_fspm_header(info);

return CB_SUCCESS;
 }
@@ -224,6 +225,6 @@
    memcpy(rec->string, fsp_version, len+1);
 }

-__weak void soc_validate_fspm_header(const struct fsp_header *hdr)
+__weak void soc_validate_fspm_header(const struct fsp_info_header *hdr)
 {
 }
diff --git a/src/soc/amd/common/fsp/fsp_validate.c b/src/soc/amd/common/fsp/fsp_validate.c
index 4d6a8f7..dba11ce 100644
--- a/src/soc/amd/common/fsp/fsp_validate.c
+++ b/src/soc/amd/common/fsp/fsp_validate.c
@@ -4,9 +4,42 @@
 #include <fsp/util.h>
 #include <types.h>

+
+struct amd_image_revision {
+uint8_t build;
+uint8_t revision;
+uint8_t minor;
+uint8_t major;
+};
+
 /* Validate the FSP-M header in romstage */
-void soc_validate_fspm_header(const struct fsp_header *hdr)
+void soc_validate_fspm_header(const struct fsp_info_header *hdr)
 {
+	struct amd_image_revision *rev = (struct amd_image_revision *) &(hdr->image_revision);
+
+	/* a coding bug on the AMD FSP side makes this value 1 in current versions of the FSP.
+	Update pushed. */
+	if (hdr->image_revision != 1) {
+		printk(BIOS_ERR, "FSPM major=%d\n", rev->major);
+		printk(BIOS_ERR, "FSPM minor=%d\n", rev->minor);
+		printk(BIOS_ERR, "FSPM revision=%d\n", rev->revision);
+		printk(BIOS_ERR, "FSPM build=%d\n", rev->build);
+
+		if (rev->major != IMAGE_REVISION_MAJOR_VERSION) {
+			printk(BIOS_ERR, "FSPM binary and fspmupd.h header file IMAGE_REVISION_MAJOR_VERSION don't match.\n");
+			printk(BIOS_ERR, "#include file ImageRevisionMajorVersion=%d\n", IMAGE_REVISION_MAJOR_VERSION);
+			printk(BIOS_ERR, "Please update FspmUpd.h based on the corresponding FSP build's FspmUpd.h\n");
+			die("Goodbye now\n");
+		}
+
+		if (rev->minor != IMAGE_REVISION_MINOR_VERSION) {
+			printk(BIOS_ERR, "FSPM binary and fspmupd.h header file version don't match.\n");
+			printk(BIOS_ERR, "#include file ImageRevisionMinorVersion=%d\n", IMAGE_REVISION_MINOR_VERSION);
+			printk(BIOS_ERR, "Please update FspmUpd.h based on the corresponding FSP build's FspmUpd.h\n");
+			}
+	} else
+		printk(BIOS_ERR, "No AMD FSP image revision information available\n");
+
    /* Check if the image fits into the reserved memory region */
    if (hdr->image_size > CONFIG_FSP_M_SIZE)
    	die("The FSP-M binary is %u bytes larger than the memory region allocated for "
diff --git a/src/vendorcode/amd/fsp/cezanne/FspmUpd.h b/src/vendorcode/amd/fsp/cezanne/FspmUpd.h
index f21ca42..f42ed43 100644
--- a/src/vendorcode/amd/fsp/cezanne/FspmUpd.h
+++ b/src/vendorcode/amd/fsp/cezanne/FspmUpd.h
@@ -107,4 +107,9 @@
    /** Offset 0x0040**/	FSP_M_CONFIG                FspmConfig;
 } FSPM_UPD;

+#define IMAGE_REVISION_MAJOR_VERSION 0x01
+#define IMAGE_REVISION_MINOR_VERSION 0x00
+#define IMAGE_REVISION_REVISION 0x05
+#define IMAGE_REVISION_BUILD_NUMBER 0x00
+
 #endif