Change in coreboot[master]: smbios: Fix type 17 for Windows 10

[Patrick Rudolph (Code Review)]

Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43969 )

Change subject: smbios: Fix type 17 for Windows 10
......................................................................

smbios: Fix type 17 for Windows 10

This fixes an issue observed on Windows 10 where the
GetPhysicallyInstalledSystemMemory API call returned an error due to
invalid SMBIOS tables. Various tools use this API call and doesn't
operate properly if this fails, for example the
"Intel Processor Diagnostic Tool".
Windows then guesses the physical memory by accumulating entries from the
memory map provided by the firmware.

To fix this issue add the handle to a type 16 entry to all type 17 entries.

Add new fields to struct memory_info and fill them in Intel common code.
Use the introduces variables to fill type 16 in smbios.c and provide
a handle to type 17 entries.

Besides keeping the current behaviour on intel/soc/common platforms, the
type 16 table is also emitted on platforms that doesn't explicitly fill
it, by using the existing fields of struct memory_info.

Tested on Windows 10:
The GetPhysicallyInstalledSystemMemory API call doesn't return an error
any more and the installed memory is now being reported as 8192MiB.

Change-Id: Idc3a363cbc3d0654dafd4176c4f4af9005210f42
Signed-off-by: Patrick Rudolph 
---
M src/arch/x86/smbios.c
M src/include/memory_info.h
M src/soc/intel/common/block/systemagent/systemagent.c
3 files changed, 93 insertions(+), 56 deletions(-)

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

diff --git a/src/arch/x86/smbios.c b/src/arch/x86/smbios.c
index 070c7ea..6e1ad87 100644
--- a/src/arch/x86/smbios.c
+++ b/src/arch/x86/smbios.c
@@ -310,7 +310,8 @@
 }

static int create_smbios_type17_for_dimm(struct dimm_info *dimm,
-					 unsigned long *current, int *handle)
+					 unsigned long *current, int *handle,
+					 int type16_handle)
 {
    struct smbios_type17 *t = (struct smbios_type17 *)*current;

@@ -365,6 +366,8 @@
    t->memory_error_information_handle = 0xFFFE;
    t->attributes = dimm->rank_per_dimm;
    t->handle = *handle;
+	t->phys_memory_array_handle = type16_handle;
+
    *handle += 1;
    t->length = sizeof(struct smbios_type17) - 2;
    return t->length + smbios_string_table_len(t->eos);
@@ -1058,10 +1061,33 @@
    return len;
 }

+static int smbios_write_type16(unsigned long *current, int *handle, bool ecc,
+			       uint32_t max_capacity_mib, uint16_t number_of_devices)
+{
+	struct smbios_type16 *t = (struct smbios_type16 *)*current;
+	int len = sizeof(struct smbios_type16);
+
+	t->type = SMBIOS_PHYS_MEMORY_ARRAY;
+	t->handle = *handle;
+	t->length = len - 2;
+	t->location = MEMORY_ARRAY_LOCATION_SYSTEM_BOARD;
+	t->use = MEMORY_ARRAY_USE_SYSTEM;
+	t->memory_error_correction = ecc ? MEMORY_ARRAY_ECC_SINGLE_BIT :
+		MEMORY_ARRAY_ECC_NONE;
+	/* no error information handle available */
+	t->memory_error_information_handle = 0xFFFE;
+	t->maximum_capacity = max_capacity_mib / KiB;
+	t->number_of_memory_devices = number_of_devices;
+
+	*current += len;
+	(*handle)++;
+	return len;
+}
+
 static int smbios_write_type17(unsigned long *current, int *handle)
 {
    int len = sizeof(struct smbios_type17);
-	int totallen = 0;
+	int type16_handle, totallen = 0;
    int i;

struct memory_info *meminfo;
@@ -1069,12 +1095,35 @@
    if (meminfo == NULL)
    	return 0;	/* can't find mem info in cbmem */

+	printk(BIOS_INFO, "Create SMBIOS type 16\n");
+	type16_handle = *handle;
+
+	if (meminfo->max_capacity_mib == 0 || meminfo->number_of_devices == 0) {
+		/* Fill in defaults if not provided */
+		meminfo->number_of_devices = 0;
+		meminfo->max_capacity_mib = 0;
+		for (i = 0; i < meminfo->dimm_cnt && i < ARRAY_SIZE(meminfo->dimm); i++) {
+			meminfo->max_capacity_mib += meminfo->dimm[i].dimm_size;
+			meminfo->number_of_devices += !!meminfo->dimm[i].dimm_size;
+		}
+	}
+
+	len = smbios_write_type16(current, handle, meminfo->ecc_capable,
+		meminfo->max_capacity_mib, meminfo->number_of_devices);
+	totallen += len;
+
    printk(BIOS_INFO, "Create SMBIOS type 17\n");
    for (i = 0; i < meminfo->dimm_cnt && i < ARRAY_SIZE(meminfo->dimm);
    	i++) {
    	struct dimm_info *dimm;
    	dimm = &meminfo->dimm[i];
-		len = create_smbios_type17_for_dimm(dimm, current, handle);
+		/*
+		 * Windows 10 GetPhysicallyInstalledSystemMemory functions reads SMBIOS
+		 * tables type 16 and type 17. The later need to point to one type 16.
+		 * Without those tables the physical installed system memory is guessed from
+		 * system memory map, which is usually slightly less than memory present.
+		 */
+		len = create_smbios_type17_for_dimm(dimm, current, handle, type16_handle);
    	*current += len;
    	totallen += len;
    }
diff --git a/src/include/memory_info.h b/src/include/memory_info.h
index a989118..26af266 100644
--- a/src/include/memory_info.h
+++ b/src/include/memory_info.h
@@ -81,6 +81,14 @@
 } __packed;

struct memory_info {
+	/* controller specific */
+	bool ecc_capable;
+	/* Maximum capacity the DRAM controller/mainboard supports */
+	uint32_t max_capacity_mib;
+	/* Maximum number of DIMMs the DRAM controller/mainboard supports */
+	uint16_t number_of_devices;
+
+	/* active DIMM configuration */
    uint8_t dimm_cnt;
    struct dimm_info dimm[DIMM_INFO_TOTAL];
 } __packed;
diff --git a/src/soc/intel/common/block/systemagent/systemagent.c b/src/soc/intel/common/block/systemagent/systemagent.c
index e12e07c..60ca478 100644
--- a/src/soc/intel/common/block/systemagent/systemagent.c
+++ b/src/soc/intel/common/block/systemagent/systemagent.c
@@ -46,6 +46,38 @@
    return 32768;	/* 32 GiB per channel */
 }

+static bool sa_supports_ecc(const uint32_t capida)
+{
+	return !(capida & CAPID_ECCDIS);
+}
+
+static size_t sa_slots_per_channel(const uint32_t capida)
+{
+	return !(capida & CAPID_DDPCD) + 1;
+}
+
+static size_t sa_number_of_channels(const uint32_t capida)
+{
+	return !(capida & CAPID_PDCD) + 1;
+}
+
+static void sa_soc_systemagent_init(struct device *dev)
+{
+	soc_systemagent_init(dev);
+
+	struct memory_info *m = cbmem_find(CBMEM_ID_MEMINFO);
+	if (m == NULL)
+		return;
+
+	const uint32_t capida = pci_read_config32(dev, CAPID0_A);
+
+	m->ecc_capable = sa_supports_ecc(capida);
+	m->max_capacity_mib = soc_systemagent_max_chan_capacity_mib(CAPID_DDRSZ(capida)) *
+			      sa_number_of_channels(capida) * MiB;
+	m->number_of_devices = sa_slots_per_channel(capida) *
+			       sa_number_of_channels(capida);
+}
+
 /*
  * Add all known fixed MMIO ranges that hang off the host bridge/memory
  * controller device.
@@ -262,55 +294,6 @@
    	sa_add_imr_resources(dev, &index);
 }

-#if CONFIG(GENERATE_SMBIOS_TABLES)
-static bool sa_supports_ecc(const uint32_t capida)
-{
-	return !(capida & CAPID_ECCDIS);
-}
-
-static size_t sa_slots_per_channel(const uint32_t capida)
-{
-	return !(capida & CAPID_DDPCD) + 1;
-}
-
-static size_t sa_number_of_channels(const uint32_t capida)
-{
-	return !(capida & CAPID_PDCD) + 1;
-}
-
-static int sa_smbios_write_type_16(struct device *dev, int *handle,
-		unsigned long *current)
-{
-	struct smbios_type16 *t = (struct smbios_type16 *)*current;
-	int len = sizeof(struct smbios_type16);
-	const uint32_t capida = pci_read_config32(dev, CAPID0_A);
-
-	struct memory_info *meminfo;
-	meminfo = cbmem_find(CBMEM_ID_MEMINFO);
-	if (meminfo == NULL)
-		return 0;	/* can't find mem info in cbmem */
-
-	memset(t, 0, sizeof(struct smbios_type16));
-	t->type = SMBIOS_PHYS_MEMORY_ARRAY;
-	t->handle = *handle;
-	t->length = len - 2;
-	t->location = MEMORY_ARRAY_LOCATION_SYSTEM_BOARD;
-	t->use = MEMORY_ARRAY_USE_SYSTEM;
-	t->memory_error_correction = sa_supports_ecc(capida) ? MEMORY_ARRAY_ECC_SINGLE_BIT :
-		MEMORY_ARRAY_ECC_NONE;
-	/* no error information handle available */
-	t->memory_error_information_handle = 0xFFFE;
-	t->maximum_capacity = soc_systemagent_max_chan_capacity_mib(CAPID_DDRSZ(capida)) *
-			      sa_number_of_channels(capida) * (MiB / KiB);
-	t->number_of_memory_devices = sa_slots_per_channel(capida) *
-				      sa_number_of_channels(capida);
-
-	*current += len;
-	*handle += 1;
-	return len;
-}
-#endif
-
 void enable_power_aware_intr(void)
 {
    uint8_t pair;
@@ -326,14 +309,11 @@
    .read_resources   = systemagent_read_resources,
    .set_resources    = pci_dev_set_resources,
    .enable_resources = pci_dev_enable_resources,
-	.init             = soc_systemagent_init,
+	.init             = sa_soc_systemagent_init,
    .ops_pci          = &pci_dev_ops_pci,
 #if CONFIG(HAVE_ACPI_TABLES)
    .write_acpi_tables = sa_write_acpi_tables,
 #endif
-#if CONFIG(GENERATE_SMBIOS_TABLES)
-	.get_smbios_data = sa_smbios_write_type_16,
-#endif
 };

static const unsigned short systemagent_ids[] = {

[Patrick Rudolph (Code Review)]

Hello Patrick Rudolph,

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

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

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

Change subject: smbios: Fix type 17 for Windows 10
......................................................................

smbios: Fix type 17 for Windows 10

This fixes an issue observed on Windows 10 where the
GetPhysicallyInstalledSystemMemory API call returned an error due to
invalid SMBIOS tables. Various tools use this API call and doesn't
operate properly if this fails, for example the
"Intel Processor Diagnostic Tool".
Windows then guesses the physical memory by accumulating entries from
the memory map provided by the firmware.

To fix this issue add the handle to a type 16 entry to all type 17
entries.

Add new fields to struct memory_info and fill them in Intel common code.
Use the introduces variables to fill type 16 in smbios.c and provide
a handle to type 17 entries.

Besides keeping the current behaviour on intel/soc/common platforms, the
type 16 table is also emitted on platforms that doesn't explicitly fill
it, by using the existing fields of struct memory_info.

Tested on Windows 10:
The GetPhysicallyInstalledSystemMemory API call doesn't return an error
any more and the installed memory is now being reported as 8192MiB.

Change-Id: Idc3a363cbc3d0654dafd4176c4f4af9005210f42
Signed-off-by: Patrick Rudolph 
---
M src/arch/x86/smbios.c
M src/include/memory_info.h
M src/soc/intel/common/block/systemagent/systemagent.c
3 files changed, 93 insertions(+), 56 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/43969/2

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43969 )

Change subject: smbios: Fix type 17 for Windows 10
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/43969/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/43969/2//COMMIT_MSG@11 
PS2, Line 11: doesn't
don’t

[Angel Pons (Code Review)]

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

Change subject: smbios: Fix type 17 for Windows 10
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43969/2/src/soc/intel/common/block/... 
File src/soc/intel/common/block/systemagent/systemagent.c:

https://review.coreboot.org/c/coreboot/+/43969/2/src/soc/intel/common/block/... 
PS2, Line 72: capida
`capida` sounds strange to me. I'd call this variable `capid0_a` instead (I think sandybridge code uses that name, for instance)

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43969 )

Change subject: smbios: Fix type 17 for Windows 10
......................................................................

Patch Set 2:

I did something similar for type 16/19/20 tables a few years ago, not sure if anything there is useful or relevant but figure I'll put it out there:
https://github.com/MrChromebox/coreboot/commit/3e93c4f58d4e6fac5865b13f780b8...

[Angel Pons (Code Review)]

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

Change subject: smbios: Fix type 17 for Windows 10
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43969/2/src/include/memory_info.h 
File src/include/memory_info.h:

https://review.coreboot.org/c/coreboot/+/43969/2/src/include/memory_info.h@8... 
PS2, Line 85: bool
stdbool.h

[Patrick Rudolph (Code Review)]

Hello build bot (Jenkins), Paul Menzel, Patrick Rudolph,

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

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

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

Change subject: smbios: Fix type 17 for Windows 10
......................................................................

smbios: Fix type 17 for Windows 10

This fixes an issue observed on Windows 10 where the
GetPhysicallyInstalledSystemMemory API call returned an error due to
invalid SMBIOS tables. Various tools use this API call and don't
operate properly if this fails, for example the
"Intel Processor Diagnostic Tool".
Windows then guesses the physical memory by accumulating entries from
the memory map provided by the firmware.

To fix this issue add the handle to a type 16 entry to all type 17
entries.

Add new fields to struct memory_info and fill them in Intel common code.
Use the introduces variables to fill type 16 in smbios.c and provide
a handle to type 17 entries.

Besides keeping the current behaviour on intel/soc/common platforms, the
type 16 table is also emitted on platforms that doesn't explicitly fill
it, by using the existing fields of struct memory_info.

Tested on Windows 10:
The GetPhysicallyInstalledSystemMemory API call doesn't return an error
any more and the installed memory is now being reported as 8192MiB.

Change-Id: Idc3a363cbc3d0654dafd4176c4f4af9005210f42
Signed-off-by: Patrick Rudolph 
---
M src/arch/x86/smbios.c
M src/include/memory_info.h
M src/soc/intel/common/block/systemagent/systemagent.c
3 files changed, 94 insertions(+), 56 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/43969/3

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43969 )

Change subject: smbios: Fix type 17 for Windows 10
......................................................................

Patch Set 3:

(3 comments)

https://review.coreboot.org/c/coreboot/+/43969/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/43969/2//COMMIT_MSG@11 
PS2, Line 11: doesn't
> don’t

Done

https://review.coreboot.org/c/coreboot/+/43969/2/src/include/memory_info.h 
File src/include/memory_info.h:

https://review.coreboot.org/c/coreboot/+/43969/2/src/include/memory_info.h@8... 
PS2, Line 85: bool
> stdbool. […]

Done

https://review.coreboot.org/c/coreboot/+/43969/2/src/soc/intel/common/block/... 
File src/soc/intel/common/block/systemagent/systemagent.c:

https://review.coreboot.org/c/coreboot/+/43969/2/src/soc/intel/common/block/... 
PS2, Line 72: capida
> `capida` sounds strange to me. […]

used the name as defined in Document:337345-003

[Angel Pons (Code Review)]

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

Change subject: smbios: Fix type 17 for Windows 10
......................................................................

Patch Set 3: Code-Review+1

(4 comments)

https://review.coreboot.org/c/coreboot/+/43969/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/43969/3//COMMIT_MSG@21 
PS3, Line 21: introduces
introduce*d*

https://review.coreboot.org/c/coreboot/+/43969/3//COMMIT_MSG@25 
PS3, Line 25: doesn't
don't

https://review.coreboot.org/c/coreboot/+/43969/3//COMMIT_MSG@30 
PS3, Line 30: any more
anymore

https://review.coreboot.org/c/coreboot/+/43969/3/src/arch/x86/smbios.c 
File src/arch/x86/smbios.c:

https://review.coreboot.org/c/coreboot/+/43969/3/src/arch/x86/smbios.c@1122 
PS3, Line 1122:  The later need to point to one type 16.
              : 		 * Without those tables the physical installed system memory is guessed from
              : 		 * system memory map, which is usually slightly less than memory present
The type 17 tables need to point to a type 16 table. Otherwise, the physical installed memory size is guessed from the system memory map, which results in a slightly smaller value.

[Angel Pons (Code Review)]

Angel Pons has uploaded a new patch set (#4) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/43969 )

Change subject: smbios: Fix type 17 for Windows 10
......................................................................

smbios: Fix type 17 for Windows 10

The `GetPhysicallyInstalledSystemMemory` API call, at least on Windows
10, returns an error if SMBIOS tables are invalid. Various tools use
this API call and don't operate correctly if this fails. For example,
the "Intel Processor Diagnostic Tool" program is affected.

Windows then guesses the physical memory size by accumulating entries
from the firmware-provided memory map, which results in a total memory
size that is slightly lower than the actual installed memory capacity.

To fix this issue, add the handle to a type 16 entry to all type 17
entries.

Add new fields to struct memory_info and fill them in Intel common code.
Use the introduced variables to fill type 16 in smbios.c and provide
a handle to type 17 entries.

Besides keeping the current behaviour on intel/soc/common platforms, the
type 16 table is also emitted on platforms that don't explicitly fill
it, by using the existing fields of struct memory_info.

Tested on Windows 10:
The GetPhysicallyInstalledSystemMemory API call doesn't return an error
anymore and the installed memory is now being reported as 8192 MiB.

Change-Id: Idc3a363cbc3d0654dafd4176c4f4af9005210f42
Signed-off-by: Patrick Rudolph 
---
M src/arch/x86/smbios.c
M src/include/memory_info.h
M src/soc/intel/common/block/systemagent/systemagent.c
3 files changed, 94 insertions(+), 56 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/43969/4

[Angel Pons (Code Review)]

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

Change subject: smbios: Fix type 17 for Windows 10
......................................................................

Patch Set 3:

(4 comments)

https://review.coreboot.org/c/coreboot/+/43969/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/43969/3//COMMIT_MSG@21 
PS3, Line 21: introduces
> introduce*d*

Done

https://review.coreboot.org/c/coreboot/+/43969/3//COMMIT_MSG@25 
PS3, Line 25: doesn't
> don't

Done

https://review.coreboot.org/c/coreboot/+/43969/3//COMMIT_MSG@30 
PS3, Line 30: any more
> anymore

Done

https://review.coreboot.org/c/coreboot/+/43969/3/src/arch/x86/smbios.c 
File src/arch/x86/smbios.c:

https://review.coreboot.org/c/coreboot/+/43969/3/src/arch/x86/smbios.c@1122 
PS3, Line 1122:  The later need to point to one type 16.
              : 		 * Without those tables the physical installed system memory is guessed from
              : 		 * system memory map, which is usually slightly less than memory present
> The type 17 tables need to point to a type 16 table. […]

Done

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43969 )

Change subject: smbios: Fix type 17 for Windows 10
......................................................................

Patch Set 4: Code-Review+2

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43969 )

Change subject: smbios: Fix type 17 for Windows 10
......................................................................

Patch Set 4:

(3 comments)

https://review.coreboot.org/c/coreboot/+/43969/4/src/soc/intel/common/block/... 
File src/soc/intel/common/block/systemagent/systemagent.c:

https://review.coreboot.org/c/coreboot/+/43969/4/src/soc/intel/common/block/... 
PS4, Line 49:  bool 
stdbool

https://review.coreboot.org/c/coreboot/+/43969/4/src/soc/intel/common/block/... 
PS4, Line 54: size_t
stddef

https://review.coreboot.org/c/coreboot/+/43969/4/src/soc/intel/common/block/... 
PS4, Line 59: uint32_t
stdint

[Angel Pons (Code Review)]

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

Change subject: smbios: Fix type 17 for Windows 10
......................................................................

Patch Set 4:

(3 comments)

https://review.coreboot.org/c/coreboot/+/43969/4/src/soc/intel/common/block/... 
File src/soc/intel/common/block/systemagent/systemagent.c:

https://review.coreboot.org/c/coreboot/+/43969/4/src/soc/intel/common/block/... 
PS4, Line 49:  bool
> stdbool

types.h

https://review.coreboot.org/c/coreboot/+/43969/4/src/soc/intel/common/block/... 
PS4, Line 54: size_t
> stddef

types.h

https://review.coreboot.org/c/coreboot/+/43969/4/src/soc/intel/common/block/... 
PS4, Line 59: uint32_t
> stdint

types.h

[Angel Pons (Code Review)]

Angel Pons has uploaded a new patch set (#5) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/43969 )

Change subject: smbios: Fix type 17 for Windows 10
......................................................................

smbios: Fix type 17 for Windows 10

The `GetPhysicallyInstalledSystemMemory` API call, at least on Windows
10, returns an error if SMBIOS tables are invalid. Various tools use
this API call and don't operate correctly if this fails. For example,
the "Intel Processor Diagnostic Tool" program is affected.

Windows then guesses the physical memory size by accumulating entries
from the firmware-provided memory map, which results in a total memory
size that is slightly lower than the actual installed memory capacity.

To fix this issue, add the handle to a type 16 entry to all type 17
entries.

Add new fields to struct memory_info and fill them in Intel common code.
Use the introduced variables to fill type 16 in smbios.c and provide
a handle to type 17 entries.

Besides keeping the current behaviour on intel/soc/common platforms, the
type 16 table is also emitted on platforms that don't explicitly fill
it, by using the existing fields of struct memory_info.

Tested on Windows 10:
The GetPhysicallyInstalledSystemMemory API call doesn't return an error
anymore and the installed memory is now being reported as 8192 MiB.

Change-Id: Idc3a363cbc3d0654dafd4176c4f4af9005210f42
Signed-off-by: Patrick Rudolph 
---
M src/arch/x86/smbios.c
M src/include/memory_info.h
M src/soc/intel/common/block/systemagent/systemagent.c
3 files changed, 95 insertions(+), 56 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/43969/5

[Angel Pons (Code Review)]

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

Change subject: smbios: Fix type 17 for Windows 10
......................................................................

Patch Set 5:

(3 comments)

https://review.coreboot.org/c/coreboot/+/43969/4/src/soc/intel/common/block/... 
File src/soc/intel/common/block/systemagent/systemagent.c:

https://review.coreboot.org/c/coreboot/+/43969/4/src/soc/intel/common/block/... 
PS4, Line 49:  bool
> types. […]

Done

https://review.coreboot.org/c/coreboot/+/43969/4/src/soc/intel/common/block/... 
PS4, Line 54: size_t
> types. […]

Done

https://review.coreboot.org/c/coreboot/+/43969/4/src/soc/intel/common/block/... 
PS4, Line 59: uint32_t
> types. […]

Done

[Angel Pons (Code Review)]

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

Change subject: smbios: Fix type 17 for Windows 10
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43969/5/src/arch/x86/smbios.c 
File src/arch/x86/smbios.c:

https://review.coreboot.org/c/coreboot/+/43969/5/src/arch/x86/smbios.c@1079 
PS5, Line 1079: max_capacity_mib * MiB
This apparently overflows

[Angel Pons (Code Review)]

Angel Pons has uploaded a new patch set (#6) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/43969 )

Change subject: smbios: Fix type 17 for Windows 10
......................................................................

smbios: Fix type 17 for Windows 10

The `GetPhysicallyInstalledSystemMemory` API call, at least on Windows
10, returns an error if SMBIOS tables are invalid. Various tools use
this API call and don't operate correctly if this fails. For example,
the "Intel Processor Diagnostic Tool" program is affected.

Windows then guesses the physical memory size by accumulating entries
from the firmware-provided memory map, which results in a total memory
size that is slightly lower than the actual installed memory capacity.

To fix this issue, add the handle to a type 16 entry to all type 17
entries.

Add new fields to struct memory_info and fill them in Intel common code.
Use the introduced variables to fill type 16 in smbios.c and provide
a handle to type 17 entries.

Besides keeping the current behaviour on intel/soc/common platforms, the
type 16 table is also emitted on platforms that don't explicitly fill
it, by using the existing fields of struct memory_info.

Tested on Windows 10:
The GetPhysicallyInstalledSystemMemory API call doesn't return an error
anymore and the installed memory is now being reported as 8192 MiB.

Change-Id: Idc3a363cbc3d0654dafd4176c4f4af9005210f42
Signed-off-by: Patrick Rudolph 
Signed-off-by: Angel Pons 
---
M src/arch/x86/smbios.c
M src/include/memory_info.h
M src/soc/intel/common/block/systemagent/systemagent.c
3 files changed, 106 insertions(+), 57 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/43969/6

[Angel Pons (Code Review)]

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

Change subject: smbios: Fix type 17 for Windows 10
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43969/5/src/arch/x86/smbios.c 
File src/arch/x86/smbios.c:

https://review.coreboot.org/c/coreboot/+/43969/5/src/arch/x86/smbios.c@1079 
PS5, Line 1079: max_capacity_mib * MiB
> This apparently overflows

Yes, among other problems, this was indeed overflowing.

[Marcello Sylvester Bauer (Code Review)]

Marcello Sylvester Bauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43969 )

Change subject: smbios: Fix type 17 for Windows 10
......................................................................

Patch Set 6: Code-Review+1

no regression with Linux on a ThinkPad X220

[Christian Walter (Code Review)]

Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43969 )

Change subject: smbios: Fix type 17 for Windows 10
......................................................................

Patch Set 6: Code-Review+2

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43969 )

Change subject: smbios: Fix type 17 for Windows 10
......................................................................

Patch Set 6: Code-Review+2

Tested on GNU/Linux 5.6. The array handle is correcly filled.

[Angel Pons (Code Review)]

Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43969 )

Change subject: smbios: Fix type 17 for Windows 10
......................................................................

smbios: Fix type 17 for Windows 10

The `GetPhysicallyInstalledSystemMemory` API call, at least on Windows
10, returns an error if SMBIOS tables are invalid. Various tools use
this API call and don't operate correctly if this fails. For example,
the "Intel Processor Diagnostic Tool" program is affected.

Windows then guesses the physical memory size by accumulating entries
from the firmware-provided memory map, which results in a total memory
size that is slightly lower than the actual installed memory capacity.

To fix this issue, add the handle to a type 16 entry to all type 17
entries.

Add new fields to struct memory_info and fill them in Intel common code.
Use the introduced variables to fill type 16 in smbios.c and provide
a handle to type 17 entries.

Besides keeping the current behaviour on intel/soc/common platforms, the
type 16 table is also emitted on platforms that don't explicitly fill
it, by using the existing fields of struct memory_info.

Tested on Windows 10:
The GetPhysicallyInstalledSystemMemory API call doesn't return an error
anymore and the installed memory is now being reported as 8192 MiB.

Change-Id: Idc3a363cbc3d0654dafd4176c4f4af9005210f42
Signed-off-by: Patrick Rudolph 
Signed-off-by: Angel Pons 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/43969
Tested-by: build bot (Jenkins) 
Reviewed-by:  Marcello Sylvester Bauer 
Reviewed-by: Christian Walter 
Reviewed-by: Patrick Rudolph 
---
M src/arch/x86/smbios.c
M src/include/memory_info.h
M src/soc/intel/common/block/systemagent/systemagent.c
3 files changed, 106 insertions(+), 57 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Rudolph: Looks good to me, approved
  Christian Walter: Looks good to me, approved
   Marcello Sylvester Bauer: Looks good to me, but someone else must approve

diff --git a/src/arch/x86/smbios.c b/src/arch/x86/smbios.c
index d7e8747..47b54aa 100644
--- a/src/arch/x86/smbios.c
+++ b/src/arch/x86/smbios.c
@@ -310,7 +310,8 @@
 }

static int create_smbios_type17_for_dimm(struct dimm_info *dimm,
-					 unsigned long *current, int *handle)
+					 unsigned long *current, int *handle,
+					 int type16_handle)
 {
    struct smbios_type17 *t = (struct smbios_type17 *)*current;

@@ -365,6 +366,8 @@
    t->memory_error_information_handle = 0xFFFE;
    t->attributes = dimm->rank_per_dimm;
    t->handle = *handle;
+	t->phys_memory_array_handle = type16_handle;
+
    *handle += 1;
    t->length = sizeof(struct smbios_type17) - 2;
    return t->length + smbios_string_table_len(t->eos);
@@ -1063,7 +1066,53 @@
    return len;
 }

-static int smbios_write_type17(unsigned long *current, int *handle)
+static int smbios_write_type16(unsigned long *current, int *handle)
+{
+	struct smbios_type16 *t = (struct smbios_type16 *)*current;
+
+	int len;
+	int i;
+
+	struct memory_info *meminfo;
+	meminfo = cbmem_find(CBMEM_ID_MEMINFO);
+	if (meminfo == NULL)
+		return 0;	/* can't find mem info in cbmem */
+
+	printk(BIOS_INFO, "Create SMBIOS type 16\n");
+
+	if (meminfo->max_capacity_mib == 0 || meminfo->number_of_devices == 0) {
+		/* Fill in defaults if not provided */
+		meminfo->number_of_devices = 0;
+		meminfo->max_capacity_mib = 0;
+		for (i = 0; i < meminfo->dimm_cnt && i < ARRAY_SIZE(meminfo->dimm); i++) {
+			meminfo->max_capacity_mib += meminfo->dimm[i].dimm_size;
+			meminfo->number_of_devices += !!meminfo->dimm[i].dimm_size;
+		}
+	}
+
+	memset(t, 0, sizeof(*t));
+	t->type = SMBIOS_PHYS_MEMORY_ARRAY;
+	t->handle = *handle;
+	t->length = len = sizeof(*t) - 2;
+
+	t->location = MEMORY_ARRAY_LOCATION_SYSTEM_BOARD;
+	t->use = MEMORY_ARRAY_USE_SYSTEM;
+	t->memory_error_correction = meminfo->ecc_capable ?
+				MEMORY_ARRAY_ECC_SINGLE_BIT : MEMORY_ARRAY_ECC_NONE;
+
+	/* no error information handle available */
+	t->memory_error_information_handle = 0xFFFE;
+	t->maximum_capacity = meminfo->max_capacity_mib * (MiB / KiB);
+	t->number_of_memory_devices = meminfo->number_of_devices;
+
+	len += smbios_string_table_len(t->eos);
+
+	*current += len;
+	(*handle)++;
+	return len;
+}
+
+static int smbios_write_type17(unsigned long *current, int *handle, int type16)
 {
    int len = sizeof(struct smbios_type17);
    int totallen = 0;
@@ -1079,7 +1128,13 @@
    	i++) {
    	struct dimm_info *dimm;
    	dimm = &meminfo->dimm[i];
-		len = create_smbios_type17_for_dimm(dimm, current, handle);
+		/*
+		 * Windows 10 GetPhysicallyInstalledSystemMemory functions reads SMBIOS tables
+		 * type 16 and type 17. The type 17 tables need to point to a type 16 table.
+		 * Otherwise, the physical installed memory size is guessed from the system
+		 * memory map, which results in a slightly smaller value than the actual size.
+		 */
+		len = create_smbios_type17_for_dimm(dimm, current, handle, type16);
    	*current += len;
    	totallen += len;
    }
@@ -1356,8 +1411,12 @@
    if (CONFIG(ELOG))
    	update_max(len, max_struct_size,
    		elog_smbios_write_type15(&current,handle++));
-	update_max(len, max_struct_size, smbios_write_type17(&current,
+
+	const int type16 = handle;
+	update_max(len, max_struct_size, smbios_write_type16(&current,
    	&handle));
+	update_max(len, max_struct_size, smbios_write_type17(&current,
+		&handle, type16));
    update_max(len, max_struct_size, smbios_write_type19(&current,
    	&handle));
    update_max(len, max_struct_size, smbios_write_type32(&current,
diff --git a/src/include/memory_info.h b/src/include/memory_info.h
index a989118..f4a2009 100644
--- a/src/include/memory_info.h
+++ b/src/include/memory_info.h
@@ -5,6 +5,7 @@
 #define _MEMORY_INFO_H_

#include <stdint.h>
+#include <stdbool.h>

#define DIMM_INFO_SERIAL_SIZE		4
 #define DIMM_INFO_PART_NUMBER_SIZE	33
@@ -81,6 +82,14 @@
 } __packed;

struct memory_info {
+	/* controller specific */
+	bool ecc_capable;
+	/* Maximum capacity the DRAM controller/mainboard supports */
+	uint32_t max_capacity_mib;
+	/* Maximum number of DIMMs the DRAM controller/mainboard supports */
+	uint16_t number_of_devices;
+
+	/* active DIMM configuration */
    uint8_t dimm_cnt;
    struct dimm_info dimm[DIMM_INFO_TOTAL];
 } __packed;
diff --git a/src/soc/intel/common/block/systemagent/systemagent.c b/src/soc/intel/common/block/systemagent/systemagent.c
index e12e07c..5da2800 100644
--- a/src/soc/intel/common/block/systemagent/systemagent.c
+++ b/src/soc/intel/common/block/systemagent/systemagent.c
@@ -13,6 +13,7 @@
 #include <soc/iomap.h>
 #include <soc/pci_devs.h>
 #include <soc/systemagent.h>
+#include <types.h>
 #include "systemagent_def.h"

/* SoC override function */
@@ -46,6 +47,38 @@
    return 32768;	/* 32 GiB per channel */
 }

+static bool sa_supports_ecc(const uint32_t capid0_a)
+{
+	return !(capid0_a & CAPID_ECCDIS);
+}
+
+static size_t sa_slots_per_channel(const uint32_t capid0_a)
+{
+	return !(capid0_a & CAPID_DDPCD) + 1;
+}
+
+static size_t sa_number_of_channels(const uint32_t capid0_a)
+{
+	return !(capid0_a & CAPID_PDCD) + 1;
+}
+
+static void sa_soc_systemagent_init(struct device *dev)
+{
+	soc_systemagent_init(dev);
+
+	struct memory_info *m = cbmem_find(CBMEM_ID_MEMINFO);
+	if (m == NULL)
+		return;
+
+	const uint32_t capid0_a = pci_read_config32(dev, CAPID0_A);
+
+	m->ecc_capable = sa_supports_ecc(capid0_a);
+	m->max_capacity_mib = soc_systemagent_max_chan_capacity_mib(CAPID_DDRSZ(capid0_a)) *
+			      sa_number_of_channels(capid0_a);
+	m->number_of_devices = sa_slots_per_channel(capid0_a) *
+			       sa_number_of_channels(capid0_a);
+}
+
 /*
  * Add all known fixed MMIO ranges that hang off the host bridge/memory
  * controller device.
@@ -262,55 +295,6 @@
    	sa_add_imr_resources(dev, &index);
 }

-#if CONFIG(GENERATE_SMBIOS_TABLES)
-static bool sa_supports_ecc(const uint32_t capida)
-{
-	return !(capida & CAPID_ECCDIS);
-}
-
-static size_t sa_slots_per_channel(const uint32_t capida)
-{
-	return !(capida & CAPID_DDPCD) + 1;
-}
-
-static size_t sa_number_of_channels(const uint32_t capida)
-{
-	return !(capida & CAPID_PDCD) + 1;
-}
-
-static int sa_smbios_write_type_16(struct device *dev, int *handle,
-		unsigned long *current)
-{
-	struct smbios_type16 *t = (struct smbios_type16 *)*current;
-	int len = sizeof(struct smbios_type16);
-	const uint32_t capida = pci_read_config32(dev, CAPID0_A);
-
-	struct memory_info *meminfo;
-	meminfo = cbmem_find(CBMEM_ID_MEMINFO);
-	if (meminfo == NULL)
-		return 0;	/* can't find mem info in cbmem */
-
-	memset(t, 0, sizeof(struct smbios_type16));
-	t->type = SMBIOS_PHYS_MEMORY_ARRAY;
-	t->handle = *handle;
-	t->length = len - 2;
-	t->location = MEMORY_ARRAY_LOCATION_SYSTEM_BOARD;
-	t->use = MEMORY_ARRAY_USE_SYSTEM;
-	t->memory_error_correction = sa_supports_ecc(capida) ? MEMORY_ARRAY_ECC_SINGLE_BIT :
-		MEMORY_ARRAY_ECC_NONE;
-	/* no error information handle available */
-	t->memory_error_information_handle = 0xFFFE;
-	t->maximum_capacity = soc_systemagent_max_chan_capacity_mib(CAPID_DDRSZ(capida)) *
-			      sa_number_of_channels(capida) * (MiB / KiB);
-	t->number_of_memory_devices = sa_slots_per_channel(capida) *
-				      sa_number_of_channels(capida);
-
-	*current += len;
-	*handle += 1;
-	return len;
-}
-#endif
-
 void enable_power_aware_intr(void)
 {
    uint8_t pair;
@@ -326,14 +310,11 @@
    .read_resources   = systemagent_read_resources,
    .set_resources    = pci_dev_set_resources,
    .enable_resources = pci_dev_enable_resources,
-	.init             = soc_systemagent_init,
+	.init             = sa_soc_systemagent_init,
    .ops_pci          = &pci_dev_ops_pci,
 #if CONFIG(HAVE_ACPI_TABLES)
    .write_acpi_tables = sa_write_acpi_tables,
 #endif
-#if CONFIG(GENERATE_SMBIOS_TABLES)
-	.get_smbios_data = sa_smbios_write_type_16,
-#endif
 };

static const unsigned short systemagent_ids[] = {