Change in coreboot[master]: soc/intel/xeon_sp: Add get_system_memory_map()

[Marc Jones (Code Review)]

Marc Jones has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45847 )

Change subject: soc/intel/xeon_sp: Add get_system_memory_map()
......................................................................

soc/intel/xeon_sp: Add get_system_memory_map()

Prepare for common ACPI. Add get_system_memory_map() helper
function to soc_util.c and use it in the SRAT ACPI code
to match the cpx code.

Change-Id: I54675b52aaf2999d884b3c20ccb143fbbf8b138a
Signed-off-by: Marc Jones 
---
M src/soc/intel/xeon_sp/skx/include/soc/soc_util.h
M src/soc/intel/xeon_sp/skx/soc_acpi.c
M src/soc/intel/xeon_sp/skx/soc_util.c
3 files changed, 19 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/45847/1

diff --git a/src/soc/intel/xeon_sp/skx/include/soc/soc_util.h b/src/soc/intel/xeon_sp/skx/include/soc/soc_util.h
index c7f7383..0eac836 100644
--- a/src/soc/intel/xeon_sp/skx/include/soc/soc_util.h
+++ b/src/soc/intel/xeon_sp/skx/include/soc/soc_util.h
@@ -30,6 +30,7 @@
 void get_cpu_info_from_apicid(uint32_t apicid, uint32_t core_bits,
    uint32_t thread_bits, uint8_t *package, uint8_t *core, uint8_t *thread);

+const struct SystemMemoryMapHob *get_system_memory_map(void);
 void xeonsp_init_cpu_config(void);
 void set_bios_init_completion(void);
 void config_reset_cpl3_csrs(void);
diff --git a/src/soc/intel/xeon_sp/skx/soc_acpi.c b/src/soc/intel/xeon_sp/skx/soc_acpi.c
index 4296f22..c116506 100644
--- a/src/soc/intel/xeon_sp/skx/soc_acpi.c
+++ b/src/soc/intel/xeon_sp/skx/soc_acpi.c
@@ -179,13 +179,11 @@
 static unsigned int get_srat_memory_entries(acpi_srat_mem_t *srat_mem)
 {
    const struct SystemMemoryMapHob *memory_map;
-	size_t hob_size;
-	const uint8_t mem_hob_guid[16] = FSP_SYSTEM_MEMORYMAP_HOB_GUID;
    unsigned int mmap_index;

-	memory_map = fsp_find_extension_hob_by_guid(mem_hob_guid, &hob_size);
-	assert(memory_map != NULL && hob_size != 0);
-	printk(BIOS_DEBUG, "FSP_SYSTEM_MEMORYMAP_HOB_GUID hob_size: %ld\n", hob_size);
+	memory_map = get_system_memory_map();
+	assert(memory_map != NULL);
+	printk(BIOS_DEBUG, "memory_map: %p\n", memory_map);

mmap_index = 0;
    for (int e = 0; e < memory_map->numberEntries; ++e) {
diff --git a/src/soc/intel/xeon_sp/skx/soc_util.c b/src/soc/intel/xeon_sp/skx/soc_util.c
index e21edbc..179b08f 100644
--- a/src/soc/intel/xeon_sp/skx/soc_util.c
+++ b/src/soc/intel/xeon_sp/skx/soc_util.c
@@ -373,6 +373,21 @@
 }

#if ENV_RAMSTAGE
+const struct SystemMemoryMapHob *get_system_memory_map(void)
+{
+	size_t hob_size;
+	const uint8_t mem_hob_guid[16] = FSP_SYSTEM_MEMORYMAP_HOB_GUID;
+	const struct SystemMemoryMapHob **memmap_addr;
+
+	memmap_addr = (const struct SystemMemoryMapHob **)
+		fsp_find_extension_hob_by_guid(mem_hob_guid, &hob_size);
+	/* hob_size is the size of the 8-byte address not the hob data */
+	assert(memmap_addr != NULL && hob_size != 0);
+	/* assert the pointer to the hob is not NULL */
+	assert(*memmap_addr != NULL);
+
+	return *memmap_addr;
+}

void xeonsp_init_cpu_config(void)
 {

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45847 )

Change subject: soc/intel/xeon_sp: Add get_system_memory_map()
......................................................................

Patch Set 3: Code-Review+1

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45847 )

Change subject: soc/intel/xeon_sp: Add get_system_memory_map()
......................................................................

Patch Set 4: Code-Review+2

[Marc Jones (Code Review)]

Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45847 )

Change subject: soc/intel/xeon_sp: Add get_system_memory_map()
......................................................................

Patch Set 5: Code-Review-2

It looks like the new check on the memory hob fails on TiogaPass but doesn't on DeltaLake. Looking into the failure before we commit.

In get_system_memory_map(void)

/* assert the pointer to the hob is not NULL */
fail:	assert(*memmap_addr != NULL);

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45847 )

Change subject: soc/intel/xeon_sp: Add get_system_memory_map()
......................................................................

Patch Set 5: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/45847/5/src/soc/intel/xeon_sp/skx/s... 
File src/soc/intel/xeon_sp/skx/soc_util.c:

https://review.coreboot.org/c/coreboot/+/45847/5/src/soc/intel/xeon_sp/skx/s... 
PS5, Line 383: fsp_find_extension_hob_by_guid
This seems to return a `struct SystemMemoryMapHob *` for SKX.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45847 )

Change subject: soc/intel/xeon_sp: Add get_system_memory_map()
......................................................................

Patch Set 5:
> Patch Set 5: Code-Review-2
>
> It looks like the new check on the memory hob fails on TiogaPass but doesn't on DeltaLake. Looking into the failure before we commit.
>
> In get_system_memory_map(void)
>
> /* assert the pointer to the hob is not NULL */
> fail:	assert(*memmap_addr != NULL);

I think I found out why. See comment.

[Marc Jones (Code Review)]

Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45847 )

Change subject: soc/intel/xeon_sp: Add get_system_memory_map()
......................................................................

Patch Set 5: -Code-Review

(1 comment)

https://review.coreboot.org/c/coreboot/+/45847/5/src/soc/intel/xeon_sp/skx/s... 
File src/soc/intel/xeon_sp/skx/soc_util.c:

https://review.coreboot.org/c/coreboot/+/45847/5/src/soc/intel/xeon_sp/skx/s... 
PS5, Line 383: fsp_find_extension_hob_by_guid
> This seems to return a `struct SystemMemoryMapHob *` for SKX.

Yes, The HOB data access is different between the SKX and CPX FSP. Since this is split out already, we can do the correct access here. We will just need to be careful in the de-duplication.

[Marc Jones (Code Review)]

Hello build bot (Jenkins), Jonathan Zhang, Jay Talbott, Angel Pons, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#6).

Change subject: soc/intel/xeon_sp: Add get_system_memory_map()
......................................................................

soc/intel/xeon_sp: Add get_system_memory_map()

Prepare for common ACPI. Add get_system_memory_map() helper
function to soc_util.c and use it in the SRAT ACPI code
to match the cpx code.

Change-Id: I54675b52aaf2999d884b3c20ccb143fbbf8b138a
Signed-off-by: Marc Jones 
---
M src/soc/intel/xeon_sp/skx/include/soc/soc_util.h
M src/soc/intel/xeon_sp/skx/soc_acpi.c
M src/soc/intel/xeon_sp/skx/soc_util.c
3 files changed, 15 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/45847/6

[Jay Talbott (Code Review)]

Jay Talbott has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45847 )

Change subject: soc/intel/xeon_sp: Add get_system_memory_map()
......................................................................

Patch Set 6: Code-Review+1

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45847 )

Change subject: soc/intel/xeon_sp: Add get_system_memory_map()
......................................................................

Patch Set 7: Code-Review+2

[Marc Jones (Code Review)]

Marc Jones has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45847 )

Change subject: soc/intel/xeon_sp: Add get_system_memory_map()
......................................................................

soc/intel/xeon_sp: Add get_system_memory_map()

Prepare for common ACPI. Add get_system_memory_map() helper
function to soc_util.c and use it in the SRAT ACPI code
to match the cpx code.

Change-Id: I54675b52aaf2999d884b3c20ccb143fbbf8b138a
Signed-off-by: Marc Jones 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/45847
Reviewed-by: Angel Pons 
Reviewed-by: Jay Talbott 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/xeon_sp/skx/include/soc/soc_util.h
M src/soc/intel/xeon_sp/skx/soc_acpi.c
M src/soc/intel/xeon_sp/skx/soc_util.c
3 files changed, 15 insertions(+), 5 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved
  Jay Talbott: Looks good to me, but someone else must approve

diff --git a/src/soc/intel/xeon_sp/skx/include/soc/soc_util.h b/src/soc/intel/xeon_sp/skx/include/soc/soc_util.h
index c7f7383..0eac836 100644
--- a/src/soc/intel/xeon_sp/skx/include/soc/soc_util.h
+++ b/src/soc/intel/xeon_sp/skx/include/soc/soc_util.h
@@ -30,6 +30,7 @@
 void get_cpu_info_from_apicid(uint32_t apicid, uint32_t core_bits,
    uint32_t thread_bits, uint8_t *package, uint8_t *core, uint8_t *thread);

+const struct SystemMemoryMapHob *get_system_memory_map(void);
 void xeonsp_init_cpu_config(void);
 void set_bios_init_completion(void);
 void config_reset_cpl3_csrs(void);
diff --git a/src/soc/intel/xeon_sp/skx/soc_acpi.c b/src/soc/intel/xeon_sp/skx/soc_acpi.c
index c986214..9c07ec7 100644
--- a/src/soc/intel/xeon_sp/skx/soc_acpi.c
+++ b/src/soc/intel/xeon_sp/skx/soc_acpi.c
@@ -179,13 +179,11 @@
 static unsigned int get_srat_memory_entries(acpi_srat_mem_t *srat_mem)
 {
    const struct SystemMemoryMapHob *memory_map;
-	size_t hob_size;
-	const uint8_t mem_hob_guid[16] = FSP_SYSTEM_MEMORYMAP_HOB_GUID;
    unsigned int mmap_index;

-	memory_map = fsp_find_extension_hob_by_guid(mem_hob_guid, &hob_size);
-	assert(memory_map != NULL && hob_size != 0);
-	printk(BIOS_DEBUG, "FSP_SYSTEM_MEMORYMAP_HOB_GUID hob_size: %ld\n", hob_size);
+	memory_map = get_system_memory_map();
+	assert(memory_map != NULL);
+	printk(BIOS_DEBUG, "memory_map: %p\n", memory_map);

mmap_index = 0;
    for (int e = 0; e < memory_map->numberEntries; ++e) {
diff --git a/src/soc/intel/xeon_sp/skx/soc_util.c b/src/soc/intel/xeon_sp/skx/soc_util.c
index e21edbc..50f091e 100644
--- a/src/soc/intel/xeon_sp/skx/soc_util.c
+++ b/src/soc/intel/xeon_sp/skx/soc_util.c
@@ -373,6 +373,17 @@
 }

#if ENV_RAMSTAGE
+const struct SystemMemoryMapHob *get_system_memory_map(void)
+{
+	size_t hob_size;
+	const uint8_t mem_hob_guid[16] = FSP_SYSTEM_MEMORYMAP_HOB_GUID;
+	const struct SystemMemoryMapHob *memmap_addr;
+
+	memmap_addr = fsp_find_extension_hob_by_guid(mem_hob_guid, &hob_size);
+	assert(memmap_addr != NULL && hob_size != 0);
+
+	return memmap_addr;
+}

void xeonsp_init_cpu_config(void)
 {