<p>Matt DeVillier has uploaded this change for <strong>review</strong>.</p><p><a href="https://review.coreboot.org/26598">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">soc/intel/broadwell: decouple PEI memory struct from coreboot header<br><br>Recent changes to field lengths in include/memory_info.h resulted in<br>a mismatch between the memory_info struct the MRC blob writes to and<br>the struct used by coreboot to parse out data for the SMBIOS tables.<br>This mismatch caused type 17 SMBIOS tables to be filled incorrectly.<br><br>The solution used here is to define the memory_info struct as expected<br>by MRC in the pei_data header, and manually copy the data field by field<br>into the coreboot memory_info struct, observing the more restrictive<br>lengths for the two structs.<br><br>Test: build/boot google/lulu, verify SMBIOS type 17 tables correctly<br>populated.<br><br>Change-Id: I932b7b41ae1e3fd364d056a8c91f7ed5d25dbafc<br>Signed-off-by: Matt DeVillier <matt.devillier@gmail.com><br>---<br>M src/soc/intel/broadwell/include/soc/pei_data.h<br>M src/soc/intel/broadwell/romstage/raminit.c<br>2 files changed, 120 insertions(+), 3 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/26598/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/src/soc/intel/broadwell/include/soc/pei_data.h b/src/soc/intel/broadwell/include/soc/pei_data.h</span><br><span>index 339dadd..852af01 100644</span><br><span>--- a/src/soc/intel/broadwell/include/soc/pei_data.h</span><br><span>+++ b/src/soc/intel/broadwell/include/soc/pei_data.h</span><br><span>@@ -87,6 +87,84 @@</span><br><span> uint8_t fixed_eq;</span><br><span> } __packed;</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+#define PEI_DIMM_INFO_SERIAL_SIZE 5</span><br><span style="color: hsl(120, 100%, 40%);">+#define PEI_DIMM_INFO_PART_NUMBER_SIZE 19</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+/**</span><br><span style="color: hsl(120, 100%, 40%);">+ * If this table is filled and put in CBMEM,</span><br><span style="color: hsl(120, 100%, 40%);">+ * then these info in CBMEM will be used to generate smbios type 17 table</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * Values are specified according to the JEDEC SPD Standard.</span><br><span style="color: hsl(120, 100%, 40%);">+ */</span><br><span style="color: hsl(120, 100%, 40%);">+struct pei_dimm_info {</span><br><span style="color: hsl(120, 100%, 40%);">+ /*</span><br><span style="color: hsl(120, 100%, 40%);">+ * Size of the module in MiB.</span><br><span style="color: hsl(120, 100%, 40%);">+ */</span><br><span style="color: hsl(120, 100%, 40%);">+ uint32_t dimm_size;</span><br><span style="color: hsl(120, 100%, 40%);">+ /*</span><br><span style="color: hsl(120, 100%, 40%);">+ * SMBIOS (not SPD) device type.</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * See the smbios.h smbios_memory_device_type enum.</span><br><span style="color: hsl(120, 100%, 40%);">+ */</span><br><span style="color: hsl(120, 100%, 40%);">+ uint16_t ddr_type;</span><br><span style="color: hsl(120, 100%, 40%);">+ uint16_t ddr_frequency;</span><br><span style="color: hsl(120, 100%, 40%);">+ uint8_t rank_per_dimm;</span><br><span style="color: hsl(120, 100%, 40%);">+ uint8_t channel_num;</span><br><span style="color: hsl(120, 100%, 40%);">+ uint8_t dimm_num;</span><br><span style="color: hsl(120, 100%, 40%);">+ uint8_t bank_locator;</span><br><span style="color: hsl(120, 100%, 40%);">+ /*</span><br><span style="color: hsl(120, 100%, 40%);">+ * The last byte is '\0' for the end of string.</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * Even though the SPD spec defines this field as a byte array the value</span><br><span style="color: hsl(120, 100%, 40%);">+ * is passed directly to SMBIOS as a string, and thus must be printable</span><br><span style="color: hsl(120, 100%, 40%);">+ * ASCII.</span><br><span style="color: hsl(120, 100%, 40%);">+ */</span><br><span style="color: hsl(120, 100%, 40%);">+ uint8_t serial[PEI_DIMM_INFO_SERIAL_SIZE];</span><br><span style="color: hsl(120, 100%, 40%);">+ /*</span><br><span style="color: hsl(120, 100%, 40%);">+ * The last byte is '\0' for the end of string</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * Must contain only printable ASCII.</span><br><span style="color: hsl(120, 100%, 40%);">+ */</span><br><span style="color: hsl(120, 100%, 40%);">+ uint8_t module_part_number[PEI_DIMM_INFO_PART_NUMBER_SIZE];</span><br><span style="color: hsl(120, 100%, 40%);">+ /*</span><br><span style="color: hsl(120, 100%, 40%);">+ * SPD Manufacturer ID</span><br><span style="color: hsl(120, 100%, 40%);">+ */</span><br><span style="color: hsl(120, 100%, 40%);">+ uint16_t mod_id;</span><br><span style="color: hsl(120, 100%, 40%);">+ /*</span><br><span style="color: hsl(120, 100%, 40%);">+ * SPD Module Type.</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * See spd.h for valid values.</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * e.g., SPD_RDIMM, SPD_SODIMM, SPD_MICRO_DIMM</span><br><span style="color: hsl(120, 100%, 40%);">+ */</span><br><span style="color: hsl(120, 100%, 40%);">+ uint8_t mod_type;</span><br><span style="color: hsl(120, 100%, 40%);">+ /*</span><br><span style="color: hsl(120, 100%, 40%);">+ * SPD bus width.</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * Bits 0 - 2 encode the primary bus width:</span><br><span style="color: hsl(120, 100%, 40%);">+ * 0b000 = 8 bit width</span><br><span style="color: hsl(120, 100%, 40%);">+ * 0b001 = 16 bit width</span><br><span style="color: hsl(120, 100%, 40%);">+ * 0b010 = 32 bit width</span><br><span style="color: hsl(120, 100%, 40%);">+ * 0b011 = 64 bit width</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * Bits 3 - 4 encode the extension bits (ECC):</span><br><span style="color: hsl(120, 100%, 40%);">+ * 0b00 = 0 extension bits</span><br><span style="color: hsl(120, 100%, 40%);">+ * 0b01 = 8 bit of ECC</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * e.g.,</span><br><span style="color: hsl(120, 100%, 40%);">+ * 64 bit bus with 8 bits of ECC (72 bits total): 0b1011</span><br><span style="color: hsl(120, 100%, 40%);">+ * 64 bit bus with 0 bits of ECC (64 bits total): 0b0011</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * See the smbios.h smbios_memory_bus_width enum.</span><br><span style="color: hsl(120, 100%, 40%);">+ */</span><br><span style="color: hsl(120, 100%, 40%);">+ uint8_t bus_width;</span><br><span style="color: hsl(120, 100%, 40%);">+} __packed;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+struct pei_memory_info {</span><br><span style="color: hsl(120, 100%, 40%);">+ uint8_t dimm_cnt;</span><br><span style="color: hsl(120, 100%, 40%);">+ struct pei_dimm_info dimm[DIMM_INFO_TOTAL];</span><br><span style="color: hsl(120, 100%, 40%);">+} __packed;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> struct pei_data {</span><br><span> uint32_t pei_version;</span><br><span> </span><br><span>@@ -191,7 +269,7 @@</span><br><span> /* Data from MRC that should be saved to flash */</span><br><span> void *data_to_save;</span><br><span> int data_to_save_size;</span><br><span style="color: hsl(0, 100%, 40%);">- struct memory_info meminfo;</span><br><span style="color: hsl(120, 100%, 40%);">+ struct pei_memory_info meminfo;</span><br><span> } __packed;</span><br><span> </span><br><span> typedef struct pei_data PEI_DATA;</span><br><span>diff --git a/src/soc/intel/broadwell/romstage/raminit.c b/src/soc/intel/broadwell/romstage/raminit.c</span><br><span>index 665dad2..844ed7c 100644</span><br><span>--- a/src/soc/intel/broadwell/romstage/raminit.c</span><br><span>+++ b/src/soc/intel/broadwell/romstage/raminit.c</span><br><span>@@ -121,6 +121,45 @@</span><br><span> </span><br><span> printk(BIOS_DEBUG, "create cbmem for dimm information\n");</span><br><span> mem_info = cbmem_add(CBMEM_ID_MEMINFO, sizeof(struct memory_info));</span><br><span style="color: hsl(0, 100%, 40%);">- memcpy(mem_info, &pei_data->meminfo, sizeof(struct memory_info));</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(120, 100%, 40%);">+ /* Translate pei_memory_info struct data into memory_info struct */</span><br><span style="color: hsl(120, 100%, 40%);">+ memcpy(&mem_info->dimm_cnt,</span><br><span style="color: hsl(120, 100%, 40%);">+ &pei_data->meminfo.dimm_cnt, sizeof(uint8_t));</span><br><span style="color: hsl(120, 100%, 40%);">+ for (int i=0; i < DIMM_INFO_TOTAL; i++) {</span><br><span style="color: hsl(120, 100%, 40%);">+ memcpy(&mem_info->dimm[i].dimm_size,</span><br><span style="color: hsl(120, 100%, 40%);">+ &pei_data->meminfo.dimm[i].dimm_size,</span><br><span style="color: hsl(120, 100%, 40%);">+ sizeof(uint32_t));</span><br><span style="color: hsl(120, 100%, 40%);">+ memcpy(&mem_info->dimm[i].ddr_type,</span><br><span style="color: hsl(120, 100%, 40%);">+ &pei_data->meminfo.dimm[i].ddr_type,</span><br><span style="color: hsl(120, 100%, 40%);">+ sizeof(uint16_t));</span><br><span style="color: hsl(120, 100%, 40%);">+ memcpy(&mem_info->dimm[i].ddr_frequency,</span><br><span style="color: hsl(120, 100%, 40%);">+ &pei_data->meminfo.dimm[i].ddr_frequency,</span><br><span style="color: hsl(120, 100%, 40%);">+ sizeof(uint16_t));</span><br><span style="color: hsl(120, 100%, 40%);">+ memcpy(&mem_info->dimm[i].rank_per_dimm,</span><br><span style="color: hsl(120, 100%, 40%);">+ &pei_data->meminfo.dimm[i].rank_per_dimm,</span><br><span style="color: hsl(120, 100%, 40%);">+ sizeof(uint8_t));</span><br><span style="color: hsl(120, 100%, 40%);">+ memcpy(&mem_info->dimm[i].channel_num,</span><br><span style="color: hsl(120, 100%, 40%);">+ &pei_data->meminfo.dimm[i].channel_num,</span><br><span style="color: hsl(120, 100%, 40%);">+ sizeof(uint8_t));</span><br><span style="color: hsl(120, 100%, 40%);">+ memcpy(&mem_info->dimm[i].dimm_num,</span><br><span style="color: hsl(120, 100%, 40%);">+ &pei_data->meminfo.dimm[i].dimm_num,</span><br><span style="color: hsl(120, 100%, 40%);">+ sizeof(uint8_t));</span><br><span style="color: hsl(120, 100%, 40%);">+ memcpy(&mem_info->dimm[i].bank_locator,</span><br><span style="color: hsl(120, 100%, 40%);">+ &pei_data->meminfo.dimm[i].bank_locator,</span><br><span style="color: hsl(120, 100%, 40%);">+ sizeof(uint8_t));</span><br><span style="color: hsl(120, 100%, 40%);">+ memcpy(&mem_info->dimm[i].serial,</span><br><span style="color: hsl(120, 100%, 40%);">+ &pei_data->meminfo.dimm[i].serial,</span><br><span style="color: hsl(120, 100%, 40%);">+ sizeof(uint8_t) * DIMM_INFO_SERIAL_SIZE);</span><br><span style="color: hsl(120, 100%, 40%);">+ memcpy(&mem_info->dimm[i].module_part_number,</span><br><span style="color: hsl(120, 100%, 40%);">+ &pei_data->meminfo.dimm[i].module_part_number,</span><br><span style="color: hsl(120, 100%, 40%);">+ sizeof(uint32_t) * PEI_DIMM_INFO_PART_NUMBER_SIZE);</span><br><span style="color: hsl(120, 100%, 40%);">+ memcpy(&mem_info->dimm[i].mod_id,</span><br><span style="color: hsl(120, 100%, 40%);">+ &pei_data->meminfo.dimm[i].mod_id,</span><br><span style="color: hsl(120, 100%, 40%);">+ sizeof(uint16_t));</span><br><span style="color: hsl(120, 100%, 40%);">+ memcpy(&mem_info->dimm[i].mod_type,</span><br><span style="color: hsl(120, 100%, 40%);">+ &pei_data->meminfo.dimm[i].mod_type,</span><br><span style="color: hsl(120, 100%, 40%);">+ sizeof(uint8_t));</span><br><span style="color: hsl(120, 100%, 40%);">+ memcpy(&mem_info->dimm[i].bus_width,</span><br><span style="color: hsl(120, 100%, 40%);">+ &pei_data->meminfo.dimm[i].bus_width,</span><br><span style="color: hsl(120, 100%, 40%);">+ sizeof(uint8_t));</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span> }</span><br><span></span><br></pre><p>To view, visit <a href="https://review.coreboot.org/26598">change 26598</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://review.coreboot.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://review.coreboot.org/26598"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: coreboot </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>
<div style="display:none"> Gerrit-Change-Id: I932b7b41ae1e3fd364d056a8c91f7ed5d25dbafc </div>
<div style="display:none"> Gerrit-Change-Number: 26598 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Matt DeVillier <matt.devillier@gmail.com> </div>