[coreboot-gerrit] New patch to review for coreboot: drivers/intel/fsp2_0: Re-engineer handling of SOC-specific structs

[Alexandru Gagniuc (alexandrux.gagniuc@intel.com)]

Alexandru Gagniuc (alexandrux.gagniuc at intel.com) just uploaded a new patch set to gerrit, which you can find at https://review.coreboot.org/15098

-gerrit

commit 6929e7ef3e9dd64af2fe976912d8ffbc495a4d78
Author: Alexandru Gagniuc <alexandrux.gagniuc at intel.com>
Date:   Tue Jun 7 15:02:14 2016 -0700

    drivers/intel/fsp2_0: Re-engineer handling of SOC-specific structs
    
    The generic code needed to have the definitions of SOC-specific
    FSPS_UPD and FSPM_UPD structures. Since these structures are defined
    in the SOC, this creates a layering violation (soc/ should be layered
    on top of drivers/).
    
    The generic code only used the definitions of the UPD structures
    through the sizeof() operator. However, that information is already
    available through the FSP header as cfg_region_offset. If we instead
    treat the UPDs as external data blocks of a known size, we do not need
    to know the exact structure, and we do not need the FSPM_UPD, and
    FSPS_UPD definitions. Thus we can avoid the layering violation.
    
    However, for this to be complete, we need to pass the size of the UPD
    structures to the users. This is done by adjusting the platform_
    callbacks to include a size field. The platform callbacks can use that
    information to verify compatibility with the FSP binary.
    
    This seems convoluted, but it is actually a superior approach. It adds
    a very important check which did not exist previously. It checks that
    the expected size of the UPD, i.e. the structure from the FSP header,
    matches the actual size of the UPD, i.e. the config region size as
    declared in the FSP header.
    
    In order for all of this to work, we also need to remove the inclusion
    of SOC-specific FSP headers in fsp/api.h. This means that all users of
    the UPD structure must now explicitly include the respective headers.
    Half the users did, half didn't. Now all of them do.
    
    It's a big patch, but it has to go in as one unit.
    PHEW!
    
    Change-Id: I521729451912f3b3ffcadcd70ff74d6eab384615
    Signed-off-by: Alexandru Gagniuc <alexandrux.gagniuc at intel.com>
---
 src/drivers/intel/fsp2_0/include/fsp/api.h |  9 +++++----
 src/drivers/intel/fsp2_0/memory_init.c     | 20 ++++++++++++++------
 src/drivers/intel/fsp2_0/silicon_init.c    | 18 +++++++++++++-----
 src/soc/intel/apollolake/chip.c            |  7 ++++++-
 src/soc/intel/apollolake/romstage.c        |  6 +++++-
 5 files changed, 43 insertions(+), 17 deletions(-)

diff --git a/src/drivers/intel/fsp2_0/include/fsp/api.h b/src/drivers/intel/fsp2_0/include/fsp/api.h
index a7b9951..7b93b71 100644
--- a/src/drivers/intel/fsp2_0/include/fsp/api.h
+++ b/src/drivers/intel/fsp2_0/include/fsp/api.h
@@ -16,8 +16,6 @@
 #include <stddef.h>
 #include <memrange.h>
 #include <fsp/info_header.h>
-#include <soc/fsp/FspmUpd.h>
-#include <soc/fsp/FspsUpd.h>
 
 enum fsp_status {
 	FSP_SUCCESS = 0x00000000,
@@ -51,6 +49,9 @@ enum fsp_notify_phase {
 	END_OF_FIRMWARE = 0xF0
 };
 
+/* SOC-specific UPD structures. These are opaque to the generic code. */
+struct FSPM_UPD;
+struct FSPS_UPD;
 
 /* Main FSP stages */
 enum fsp_status fsp_memory_init(void **hob_list, struct range_entry *r);
@@ -58,8 +59,8 @@ enum fsp_status fsp_silicon_init(struct range_entry *r);
 enum fsp_status fsp_notify(enum fsp_notify_phase phase);
 
 /* Callbacks for updating stage-specific parameters */
-void platform_fsp_memory_init_params_cb(struct FSPM_UPD *mupd);
-void platform_fsp_silicon_init_params_cb(struct FSPS_UPD *supd);
+void platform_fsp_memory_init_params_cb(struct FSPM_UPD *mupd, size_t upd_size);
+void platform_fsp_silicon_init_params_cb(struct FSPS_UPD *supd, size_t upd_size);
 
 /*
  * # DOCUMENTATION:
diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c
index 3bd1566..1218660 100644
--- a/src/drivers/intel/fsp2_0/memory_init.c
+++ b/src/drivers/intel/fsp2_0/memory_init.c
@@ -28,27 +28,35 @@ static enum fsp_status do_fsp_memory_init(void **hob_list_ptr,
 {
 	enum fsp_status status;
 	fsp_memory_init_fn fsp_raminit;
-	struct FSPM_UPD fspm_upd, *upd;
+	uint8_t upd_buffer[1024];
+	const struct FSPM_UPD *upd;
+	struct FSPM_UPD *upd_copy = (void *)upd_buffer;
 
 	post_code(0x34);
 
-	upd = (struct FSPM_UPD *)(hdr->cfg_region_offset + hdr->image_base);
+	if (hdr->cfg_region_size > sizeof(upd_buffer)) {
+		printk(BIOS_ERR, "FSP UPD region too large (%zx > %zx)\n",
+			hdr->cfg_region_size, sizeof(upd_buffer));
+		return FSP_OUT_OF_RESOURCES;
+	}
+
+	upd = (void *)(hdr->cfg_region_offset + hdr->image_base);
 
 	/* Copy the default values from the UPD area */
-	memcpy(&fspm_upd, upd, sizeof(fspm_upd));
+	memcpy(upd_copy, upd, hdr->cfg_region_size);
 
 	/* Give SoC and mainboard a chance to update the UPD */
-	platform_fsp_memory_init_params_cb(&fspm_upd);
+	platform_fsp_memory_init_params_cb(upd_copy, hdr->cfg_region_size);
 
 	/* Call FspMemoryInit */
 	fsp_raminit = (void *)(hdr->image_base + hdr->memory_init_entry_offset);
 	printk(BIOS_DEBUG, "Calling FspMemoryInit: 0x%p\n", fsp_raminit);
-	printk(BIOS_SPEW, "\t%p: raminit_upd\n", &fspm_upd);
+	printk(BIOS_SPEW, "\t%p: raminit_upd\n", upd_copy);
 	printk(BIOS_SPEW, "\t%p: hob_list ptr\n", hob_list_ptr);
 
 	post_code(POST_FSP_MEMORY_INIT);
 	timestamp_add_now(TS_FSP_MEMORY_INIT_START);
-	status = fsp_raminit(&fspm_upd, hob_list_ptr);
+	status = fsp_raminit(upd_copy, hob_list_ptr);
 	post_code(POST_FSP_MEMORY_INIT);
 	timestamp_add_now(TS_FSP_MEMORY_INIT_END);
 
diff --git a/src/drivers/intel/fsp2_0/silicon_init.c b/src/drivers/intel/fsp2_0/silicon_init.c
index bde91da..5274e3f 100644
--- a/src/drivers/intel/fsp2_0/silicon_init.c
+++ b/src/drivers/intel/fsp2_0/silicon_init.c
@@ -25,22 +25,30 @@ typedef asmlinkage enum fsp_status (*fsp_silicon_init_fn)
 
 static enum fsp_status do_silicon_init(struct fsp_header *hdr)
 {
-	struct FSPS_UPD upd, *supd;
 	fsp_silicon_init_fn silicon_init;
 	enum fsp_status status;
+	uint8_t upd_buffer[1024];
+	const struct FSPS_UPD *upd;
+	struct FSPS_UPD *upd_copy = (void *)upd_buffer;
 
-	supd = (struct FSPS_UPD *) (hdr->cfg_region_offset + hdr->image_base);
+	if (hdr->cfg_region_size > sizeof(upd_buffer)) {
+		printk(BIOS_ERR, "FSP UPD region too large (%zx > %zx)\n",
+			hdr->cfg_region_size, sizeof(upd_buffer));
+		return FSP_OUT_OF_RESOURCES;
+	}
 
-	memcpy(&upd, supd, sizeof(upd));
+	upd = (struct FSPS_UPD *) (hdr->cfg_region_offset + hdr->image_base);
+
+	memcpy(upd_copy, upd, hdr->cfg_region_size);
 
 	/* Give SoC/mainboard a chance to populate entries */
-	platform_fsp_silicon_init_params_cb(&upd);
+	platform_fsp_silicon_init_params_cb(upd_copy, hdr->cfg_region_size);
 
 	timestamp_add_now(TS_FSP_SILICON_INIT_START);
 	post_code(POST_FSP_SILICON_INIT);
 	silicon_init = (void *) (hdr->image_base +
 				 hdr->silicon_init_entry_offset);
-	status = silicon_init(&upd);
+	status = silicon_init(upd_copy);
 	timestamp_add_now(TS_FSP_SILICON_INIT_END);
 	post_code(POST_FSP_SILICON_INIT);
 
diff --git a/src/soc/intel/apollolake/chip.c b/src/soc/intel/apollolake/chip.c
index 17bceec..43ca659 100644
--- a/src/soc/intel/apollolake/chip.c
+++ b/src/soc/intel/apollolake/chip.c
@@ -30,6 +30,7 @@
 #include <soc/intel/common/vbt.h>
 #include <soc/nvs.h>
 #include <soc/pci_devs.h>
+#include <soc/fsp/FspsUpd.h>
 
 #include "chip.h"
 
@@ -92,11 +93,15 @@ static void soc_final(void *data)
 		rdev_munmap(&vbt_rdev, vbt);
 }
 
-void platform_fsp_silicon_init_params_cb(struct FSPS_UPD *silupd)
+void platform_fsp_silicon_init_params_cb(struct FSPS_UPD *silupd,
+					 size_t upd_size)
 {
         struct FSP_S_CONFIG *silconfig = &silupd->FspsConfig;
 	static struct soc_intel_apollolake_config *cfg;
 
+	if (sizeof(*silupd) != upd_size)
+		die("Mismatch between expected and actual UPD size");
+
 	/* Load VBT before devicetree-specific config. */
 	silconfig->GraphicsConfigPtr = (uintptr_t)vbt;
 
diff --git a/src/soc/intel/apollolake/romstage.c b/src/soc/intel/apollolake/romstage.c
index 434d11f..d962030 100644
--- a/src/soc/intel/apollolake/romstage.c
+++ b/src/soc/intel/apollolake/romstage.c
@@ -38,6 +38,7 @@
 #include <soc/pm.h>
 #include <soc/romstage.h>
 #include <soc/uart.h>
+#include <soc/fsp/FspmUpd.h>
 #include <string.h>
 #include <timestamp.h>
 
@@ -215,13 +216,16 @@ static void fill_console_params(struct FSPM_UPD *mupd)
 	}
 }
 
-void platform_fsp_memory_init_params_cb(struct FSPM_UPD *mupd)
+void platform_fsp_memory_init_params_cb(struct FSPM_UPD *mupd, size_t upd_size)
 {
 	const struct mrc_saved_data *mrc_cache;
 	struct FSP_M_ARCH_UPD *arch_upd = &mupd->FspmArchUpd;
 	struct chipset_power_state *ps = car_get_var_ptr(&power_state);
 	int prev_sleep_state = chipset_prev_sleep_state(ps);
 
+	if (sizeof(*mupd) != upd_size)
+		die("Mismatch between expected and actual UPD size");
+
 	fill_console_params(mupd);
 	mainboard_memory_init_params(mupd);