Jonathan Zhang has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43336 )
Change subject: soc/intel/xeon_sp/cpx: display SystemMemoryMapHob fields ......................................................................
soc/intel/xeon_sp/cpx: display SystemMemoryMapHob fields
SystemMemoryMapHob is necessary for SMBIOS type 17 among other things. It is a fairly large structure, so the pointer to the data instead of the structure itself, is included in the HOB. Use pointer to SystemMemoryMapHob structure to interpret SystemMemoryHob HOB body.
Adjust the structure definition to match with CPX-SP ww28 release.
Display more fields to ensure the structure definition is correct.
TEST=Boot DeltaLake server, and check field values of SystemMemoryMapHob to make sure they are correct: 0x7590a090, 0x00000020 bytes: HOB_TYPE_GUID_EXTENSION f8870015-6994-4b98-95a2bd56da91c07f: FSP_SYSTEM_MEMORYMAP_HOB_GUID ================== MEMORY MAP HOB DATA ================== hob: 0x777f7000, structure size: 0x187a lowMemBase: 0x0, lowMemSize: 0x20, highMemBase: 0x40, highMemSize: 0x5d0 memSize: 0x600, memFreq: 0xb76 NumChPerMC: 3 SystemMemoryMapElement Entries: 2, entry size: 16 memory_map 0 BaseAddress: 0x0, ElementSize: 0x20, Type: 0x1 memory_map 1 BaseAddress: 0x40, ElementSize: 0x5d0, Type: 0x1 BiosFisVersion: 0x0 MmiohBase: 0x0 0x777f7000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ ... Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: I271bcbd6030276b8fcd99d5b4f2c93f034dd9b52 --- M src/soc/intel/xeon_sp/cpx/hob_display.c M src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/hob_memmap.h 2 files changed, 18 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/43336/1
diff --git a/src/soc/intel/xeon_sp/cpx/hob_display.c b/src/soc/intel/xeon_sp/cpx/hob_display.c index 0263472..3d61a82 100644 --- a/src/soc/intel/xeon_sp/cpx/hob_display.c +++ b/src/soc/intel/xeon_sp/cpx/hob_display.c @@ -33,9 +33,9 @@ return NULL; }
-static void soc_display_memmap_hob(const struct SystemMemoryMapHob *hob_addr) +static void soc_display_memmap_hob(const struct SystemMemoryMapHob **hob_addr) { - struct SystemMemoryMapHob *hob = (struct SystemMemoryMapHob *)hob_addr; + struct SystemMemoryMapHob *hob = (struct SystemMemoryMapHob *)*hob_addr;
printk(BIOS_DEBUG, "================== MEMORY MAP HOB DATA ==================\n"); printk(BIOS_DEBUG, "hob: %p, structure size: 0x%lx\n", @@ -47,6 +47,7 @@ printk(BIOS_DEBUG, "\tmemSize: 0x%x, memFreq: 0x%x\n", hob->memSize, hob->memFreq);
+ printk(BIOS_DEBUG, "\tNumChPerMC: %d\n", hob->NumChPerMC); printk(BIOS_DEBUG, "\tSystemMemoryMapElement Entries: %d, entry size: %ld\n", hob->numberEntries, sizeof(SYSTEM_MEMORY_MAP_ELEMENT)); for (int e = 0; e < hob->numberEntries; ++e) { @@ -56,6 +57,9 @@ mem_element->ElementSize, mem_element->Type); }
+ printk(BIOS_DEBUG, "\tBiosFisVersion: 0x%x\n", hob->BiosFisVersion); + printk(BIOS_DEBUG, "\tMmiohBase: 0x%x\n", hob->MmiohBase); + hexdump(hob, sizeof(*hob)); }
@@ -181,5 +185,5 @@ if (fsp_guid_compare(guid, fsp_hob_iio_uds_guid)) soc_display_iio_universal_data_hob((const IIO_UDS *)(guid + 16)); else if (fsp_guid_compare(guid, fsp_hob_memmap_guid)) - soc_display_memmap_hob((const struct SystemMemoryMapHob *)(guid + 16)); + soc_display_memmap_hob((const struct SystemMemoryMapHob **)(guid + 16)); } diff --git a/src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/hob_memmap.h b/src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/hob_memmap.h index df7787a..51aa051 100644 --- a/src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/hob_memmap.h +++ b/src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/hob_memmap.h @@ -40,6 +40,8 @@ #define MEMTYPE_2LM_MASK (1 << 1) #define MEMTYPE_VOLATILE_MASK (MEMTYPE_1LM_MASK | MEMTYPE_2LM_MASK)
+#define MAX_FPGA_REMOTE_SAD_RULES 2 // Maximum FPGA sockets exists on ICX platform + #define MAX_SAD_RULES 24 #define MAX_DRAM_CLUSTERS 1 #define MAX_IMC_PER_SOCKET 2 @@ -85,7 +87,7 @@
/* NOTE - Reserved sizes need to be calibrated if any of the above #define values change */ typedef struct SystemMemoryMapHob { - UINT8 reserved1[58]; + UINT8 reserved1[61];
UINT32 lowMemBase; // Mem base in 64MB units for below 4GB mem. UINT32 lowMemSize; // Mem size in 64MB units for below 4GB mem. @@ -96,14 +98,19 @@
UINT8 reserved2[61];
+ UINT8 NumChPerMC; UINT8 numberEntries; // Number of Memory Map Elements - SYSTEM_MEMORY_MAP_ELEMENT Element[MAX_SOCKET * MAX_DRAM_CLUSTERS * MAX_SAD_RULES]; + SYSTEM_MEMORY_MAP_ELEMENT Element[(MAX_SOCKET * MAX_DRAM_CLUSTERS * MAX_SAD_RULES) + MAX_FPGA_REMOTE_SAD_RULES];
- UINT8 reserved3[24514]; + UINT8 reserved3[30000]; + + UINT16 BiosFisVersion; // Firmware Interface Specification version currently supported by BIOS + + UINT8 reserved4[8];
UINT32 MmiohBase; // MMIOH base in 64MB granularity
- UINT8 reserved4[2]; + UINT8 reserved5[2];
} SYSTEM_MEMORY_MAP_HOB;
Hello Philipp Deppenwiese, build bot (Jenkins), Anjaneya "Reddy" Chagam, Nate DeSimone, Christian Walter, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43336
to look at the new patch set (#2).
Change subject: soc/intel/xeon_sp/cpx: display SystemMemoryMapHob fields ......................................................................
soc/intel/xeon_sp/cpx: display SystemMemoryMapHob fields
SystemMemoryMapHob is necessary for SMBIOS type 17 among other things. It is a fairly large structure, so the pointer to the data instead of the structure itself, is included in the HOB. Use pointer to SystemMemoryMapHob structure to interpret SystemMemoryHob HOB body.
Adjust the structure definition to match with CPX-SP ww28 release.
Display more fields to ensure the structure definition is correct.
TEST=Boot DeltaLake server, and check field values of SystemMemoryMapHob to make sure they are correct: 0x7590a090, 0x00000020 bytes: HOB_TYPE_GUID_EXTENSION f8870015-6994-4b98-95a2bd56da91c07f: FSP_SYSTEM_MEMORYMAP_HOB_GUID ================== MEMORY MAP HOB DATA ================== hob: 0x777f7000, structure size: 0x187a lowMemBase: 0x0, lowMemSize: 0x20, highMemBase: 0x40, highMemSize: 0x5d0 memSize: 0x600, memFreq: 0xb76 NumChPerMC: 3 SystemMemoryMapElement Entries: 2, entry size: 16 memory_map 0 BaseAddress: 0x0, ElementSize: 0x20, Type: 0x1 memory_map 1 BaseAddress: 0x40, ElementSize: 0x5d0, Type: 0x1 BiosFisVersion: 0x0 MmiohBase: 0x0 0x777f7000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ ... Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: I271bcbd6030276b8fcd99d5b4f2c93f034dd9b52 --- M src/soc/intel/xeon_sp/cpx/hob_display.c M src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/hob_memmap.h 2 files changed, 18 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/43336/2
Johnny Lin has uploaded a new patch set (#3) to the change originally created by Jonathan Zhang. ( https://review.coreboot.org/c/coreboot/+/43336 )
Change subject: soc/intel/xeon_sp/cpx: display SystemMemoryMapHob fields ......................................................................
soc/intel/xeon_sp/cpx: display SystemMemoryMapHob fields
SystemMemoryMapHob is necessary for SMBIOS type 17 among other things. It is a fairly large structure, so the pointer to the data instead of the structure itself, is included in the HOB. Use pointer to SystemMemoryMapHob structure to interpret SystemMemoryHob HOB body.
Adjust the structure definition to match with CPX-SP ww28 release.
Display more fields to ensure the structure definition is correct.
TEST=Boot DeltaLake server, and check field values of SystemMemoryMapHob to make sure they are correct: 0x7590a090, 0x00000020 bytes: HOB_TYPE_GUID_EXTENSION f8870015-6994-4b98-95a2bd56da91c07f: FSP_SYSTEM_MEMORYMAP_HOB_GUID ================== MEMORY MAP HOB DATA ================== hob: 0x777f7000, structure size: 0x187a lowMemBase: 0x0, lowMemSize: 0x20, highMemBase: 0x40, highMemSize: 0x5d0 memSize: 0x600, memFreq: 0xb76 NumChPerMC: 3 SystemMemoryMapElement Entries: 2, entry size: 16 memory_map 0 BaseAddress: 0x0, ElementSize: 0x20, Type: 0x1 memory_map 1 BaseAddress: 0x40, ElementSize: 0x5d0, Type: 0x1 BiosFisVersion: 0x0 MmiohBase: 0x0 0x777f7000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ ... Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: I271bcbd6030276b8fcd99d5b4f2c93f034dd9b52 --- M src/soc/intel/xeon_sp/cpx/hob_display.c M src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/hob_memmap.h 2 files changed, 18 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/43336/3
Johnny Lin has uploaded a new patch set (#4) to the change originally created by Jonathan Zhang. ( https://review.coreboot.org/c/coreboot/+/43336 )
Change subject: soc/intel/xeon_sp/cpx: display SystemMemoryMapHob fields ......................................................................
soc/intel/xeon_sp/cpx: display SystemMemoryMapHob fields
SystemMemoryMapHob is necessary for SMBIOS type 17 among other things. It is a fairly large structure, so the pointer to the data instead of the structure itself, is included in the HOB. Use pointer to SystemMemoryMapHob structure to interpret SystemMemoryHob HOB body.
Adjust the structure definition to match with CPX-SP ww28 release.
Display more fields to ensure the structure definition is correct.
TEST=Boot DeltaLake server, and check field values of SystemMemoryMapHob to make sure they are correct: 0x7590a090, 0x00000020 bytes: HOB_TYPE_GUID_EXTENSION f8870015-6994-4b98-95a2bd56da91c07f: FSP_SYSTEM_MEMORYMAP_HOB_GUID ================== MEMORY MAP HOB DATA ================== hob: 0x777f7000, structure size: 0x6c88 lowMemBase: 0x0, lowMemSize: 0x20, highMemBase: 0x40, highMemSize: 0x5d0 memSize: 0x600, memFreq: 0xb76 NumChPerMC: 3 SystemMemoryMapElement Entries: 2, entry size: 16 memory_map 0 BaseAddress: 0x0, ElementSize: 0x20, Type: 0x1 memory_map 1 BaseAddress: 0x40, ElementSize: 0x5d0, Type: 0x1 BiosFisVersion: 0x0 MmiohBase: 0x80000 0x777f7000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ ... Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: I271bcbd6030276b8fcd99d5b4f2c93f034dd9b52 --- M src/soc/intel/xeon_sp/cpx/hob_display.c M src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/hob_memmap.h 2 files changed, 18 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/43336/4
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43336 )
Change subject: soc/intel/xeon_sp/cpx: display SystemMemoryMapHob fields ......................................................................
Patch Set 4: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43336 )
Change subject: soc/intel/xeon_sp/cpx: display SystemMemoryMapHob fields ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43336/4/src/soc/intel/xeon_sp/cpx/h... File src/soc/intel/xeon_sp/cpx/hob_display.c:
https://review.coreboot.org/c/coreboot/+/43336/4/src/soc/intel/xeon_sp/cpx/h... PS4, Line 188: soc_display_memmap_hob((const struct SystemMemoryMapHob **)(guid + 16)); So this wasn't working properly before?
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43336 )
Change subject: soc/intel/xeon_sp/cpx: display SystemMemoryMapHob fields ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43336/4/src/soc/intel/xeon_sp/cpx/h... File src/soc/intel/xeon_sp/cpx/hob_display.c:
https://review.coreboot.org/c/coreboot/+/43336/4/src/soc/intel/xeon_sp/cpx/h... PS4, Line 188: soc_display_memmap_hob((const struct SystemMemoryMapHob **)(guid + 16));
So this wasn't working properly before?
We had thought what is stored in the HOB structure (following 16 bytes of GUID) is the HOB data. Intel clarified that what is stored in the HOB structure is actually a 8 byte address pointing to the HOB data.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43336 )
Change subject: soc/intel/xeon_sp/cpx: display SystemMemoryMapHob fields ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/43336/4/src/soc/intel/xeon_sp/cpx/h... File src/soc/intel/xeon_sp/cpx/hob_display.c:
https://review.coreboot.org/c/coreboot/+/43336/4/src/soc/intel/xeon_sp/cpx/h... PS4, Line 188: soc_display_memmap_hob((const struct SystemMemoryMapHob **)(guid + 16));
We had thought what is stored in the HOB structure (following 16 bytes of GUID) is the HOB data. […]
Ah, I see. Thanks for the clarification.
Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43336 )
Change subject: soc/intel/xeon_sp/cpx: display SystemMemoryMapHob fields ......................................................................
soc/intel/xeon_sp/cpx: display SystemMemoryMapHob fields
SystemMemoryMapHob is necessary for SMBIOS type 17 among other things. It is a fairly large structure, so the pointer to the data instead of the structure itself, is included in the HOB. Use pointer to SystemMemoryMapHob structure to interpret SystemMemoryHob HOB body.
Adjust the structure definition to match with CPX-SP ww28 release.
Display more fields to ensure the structure definition is correct.
TEST=Boot DeltaLake server, and check field values of SystemMemoryMapHob to make sure they are correct: 0x7590a090, 0x00000020 bytes: HOB_TYPE_GUID_EXTENSION f8870015-6994-4b98-95a2bd56da91c07f: FSP_SYSTEM_MEMORYMAP_HOB_GUID ================== MEMORY MAP HOB DATA ================== hob: 0x777f7000, structure size: 0x6c88 lowMemBase: 0x0, lowMemSize: 0x20, highMemBase: 0x40, highMemSize: 0x5d0 memSize: 0x600, memFreq: 0xb76 NumChPerMC: 3 SystemMemoryMapElement Entries: 2, entry size: 16 memory_map 0 BaseAddress: 0x0, ElementSize: 0x20, Type: 0x1 memory_map 1 BaseAddress: 0x40, ElementSize: 0x5d0, Type: 0x1 BiosFisVersion: 0x0 MmiohBase: 0x80000 0x777f7000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ ... Signed-off-by: Jonathan Zhang jonzhang@fb.com Change-Id: I271bcbd6030276b8fcd99d5b4f2c93f034dd9b52 Reviewed-on: https://review.coreboot.org/c/coreboot/+/43336 Reviewed-by: Philipp Deppenwiese zaolin.daisuki@gmail.com Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/xeon_sp/cpx/hob_display.c M src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/hob_memmap.h 2 files changed, 18 insertions(+), 7 deletions(-)
Approvals: build bot (Jenkins): Verified Philipp Deppenwiese: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/src/soc/intel/xeon_sp/cpx/hob_display.c b/src/soc/intel/xeon_sp/cpx/hob_display.c index 0263472..3d61a82 100644 --- a/src/soc/intel/xeon_sp/cpx/hob_display.c +++ b/src/soc/intel/xeon_sp/cpx/hob_display.c @@ -33,9 +33,9 @@ return NULL; }
-static void soc_display_memmap_hob(const struct SystemMemoryMapHob *hob_addr) +static void soc_display_memmap_hob(const struct SystemMemoryMapHob **hob_addr) { - struct SystemMemoryMapHob *hob = (struct SystemMemoryMapHob *)hob_addr; + struct SystemMemoryMapHob *hob = (struct SystemMemoryMapHob *)*hob_addr;
printk(BIOS_DEBUG, "================== MEMORY MAP HOB DATA ==================\n"); printk(BIOS_DEBUG, "hob: %p, structure size: 0x%lx\n", @@ -47,6 +47,7 @@ printk(BIOS_DEBUG, "\tmemSize: 0x%x, memFreq: 0x%x\n", hob->memSize, hob->memFreq);
+ printk(BIOS_DEBUG, "\tNumChPerMC: %d\n", hob->NumChPerMC); printk(BIOS_DEBUG, "\tSystemMemoryMapElement Entries: %d, entry size: %ld\n", hob->numberEntries, sizeof(SYSTEM_MEMORY_MAP_ELEMENT)); for (int e = 0; e < hob->numberEntries; ++e) { @@ -56,6 +57,9 @@ mem_element->ElementSize, mem_element->Type); }
+ printk(BIOS_DEBUG, "\tBiosFisVersion: 0x%x\n", hob->BiosFisVersion); + printk(BIOS_DEBUG, "\tMmiohBase: 0x%x\n", hob->MmiohBase); + hexdump(hob, sizeof(*hob)); }
@@ -181,5 +185,5 @@ if (fsp_guid_compare(guid, fsp_hob_iio_uds_guid)) soc_display_iio_universal_data_hob((const IIO_UDS *)(guid + 16)); else if (fsp_guid_compare(guid, fsp_hob_memmap_guid)) - soc_display_memmap_hob((const struct SystemMemoryMapHob *)(guid + 16)); + soc_display_memmap_hob((const struct SystemMemoryMapHob **)(guid + 16)); } diff --git a/src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/hob_memmap.h b/src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/hob_memmap.h index df7787a..71ff6d5 100644 --- a/src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/hob_memmap.h +++ b/src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/hob_memmap.h @@ -40,6 +40,8 @@ #define MEMTYPE_2LM_MASK (1 << 1) #define MEMTYPE_VOLATILE_MASK (MEMTYPE_1LM_MASK | MEMTYPE_2LM_MASK)
+#define MAX_FPGA_REMOTE_SAD_RULES 2 // Maximum FPGA sockets exists on ICX platform + #define MAX_SAD_RULES 24 #define MAX_DRAM_CLUSTERS 1 #define MAX_IMC_PER_SOCKET 2 @@ -85,7 +87,7 @@
/* NOTE - Reserved sizes need to be calibrated if any of the above #define values change */ typedef struct SystemMemoryMapHob { - UINT8 reserved1[58]; + UINT8 reserved1[61];
UINT32 lowMemBase; // Mem base in 64MB units for below 4GB mem. UINT32 lowMemSize; // Mem size in 64MB units for below 4GB mem. @@ -96,14 +98,19 @@
UINT8 reserved2[61];
+ UINT8 NumChPerMC; UINT8 numberEntries; // Number of Memory Map Elements - SYSTEM_MEMORY_MAP_ELEMENT Element[MAX_SOCKET * MAX_DRAM_CLUSTERS * MAX_SAD_RULES]; + SYSTEM_MEMORY_MAP_ELEMENT Element[(MAX_SOCKET * MAX_DRAM_CLUSTERS * MAX_SAD_RULES) + MAX_FPGA_REMOTE_SAD_RULES];
- UINT8 reserved3[24514]; + UINT8 reserved3[24518]; + + UINT16 BiosFisVersion; // Firmware Interface Specification version currently supported by BIOS + + UINT8 reserved4[8];
UINT32 MmiohBase; // MMIOH base in 64MB granularity
- UINT8 reserved4[2]; + UINT8 reserved5[2];
} SYSTEM_MEMORY_MAP_HOB;
-
Angel Pons (Code Review) -
Johnny Lin (Code Review)
-
Jonathan Zhang (Code Review)
-
Philipp Deppenwiese (Code Review)