Change in ...coreboot[master]: src/arch: An upgrade of SMBIOS to latest version 3.2

[Francois Toguo Fotso (Code Review)]

Francois Toguo Fotso has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31863

Change subject: src/arch: An upgrade of SMBIOS to latest version 3.2
......................................................................

src/arch: An upgrade of SMBIOS to latest version 3.2

This is the second of 2 patches upgrading the SMBIOS interface to the latest 3.2
First patch is in mosys. Newer required fields are added to various types definitions

BUG=NONE
TEST=Boot to OS

Signed-off-by: Francois Toguo 
Change-Id: Idd2675ddd7196300019057f80a51bdd3b0e5b4e1
---
M src/arch/x86/smbios.c
M src/include/smbios.h
2 files changed, 64 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/31863/1

diff --git a/src/arch/x86/smbios.c b/src/arch/x86/smbios.c
index 7d3cfae..6e474a4 100644
--- a/src/arch/x86/smbios.c
+++ b/src/arch/x86/smbios.c
@@ -328,6 +328,17 @@
    t->handle = *handle;
    *handle += 1;
    t->length = sizeof(struct smbios_type17) - 2;
+	t->memory_technology = MEMORY_TECHNOLOGY_UNKNOWN;
+	t->operating_mode_capability = MEMORY_OPERATING_MODE_CAP_UNKNOWN;
+	t->fw_version = 0xff;
+	t->manufacturer_id = dimm->mod_id;
+	t->product_id = 0x0000;
+	t->sub_ctrl_manufacturer_id = 0x0000;
+	t->sub_ctrl_product_id = 0x0000;
+	t->non_volatile_size = 0xffffffffffffffff;
+	t->volatile_size = 0xffffffffffffffff;
+	t->cache_size = 0xffffffffffffffff;
+	t->logical_size = 0xffffffffffffffff;
    return t->length + smbios_string_table_len(t->eos);
 }

@@ -537,6 +548,40 @@
    return len;
 }

+u16 __weak smbios_processor_core_count(void) {
+	u16 core_count = 0;
+	int ecx = 0;
+
+	while (1) {
+		struct cpuid_result leaf_b;
+		leaf_b = cpuid_ext(0xb, ecx);
+		if ((leaf_b.ecx & 0xff00) == 0x0200) {
+			core_count = leaf_b.ebx & 0xffff;
+			break;
+		}
+		ecx++;
+	}
+	
+	return core_count;
+}
+
+u16 __weak smbios_processor_thread_count(void) {
+	u16 thread_count = 0;
+	int ecx = 0;
+ 
+	while (1) {
+		struct cpuid_result leaf_b;
+		leaf_b = cpuid_ext(0xb, ecx);
+		if ((leaf_b.ecx & 0xff00) == 0x0100) {
+			thread_count = leaf_b.ebx & 0xffff;
+			break;
+		}
+		ecx++;
+	}
+	
+	return thread_count;
+}
+
 static int smbios_write_type4(unsigned long *current, int handle)
 {
    struct cpuid_result res;
@@ -560,13 +605,25 @@
    t->processor_version = smbios_processor_name(t->eos);
    t->processor_family = (res.eax > 0) ? 0x0c : 0x6;
    t->processor_type = 3; /* System Processor */
-	if (((res.ebx >> 16) & 0xffff) > 255) {
+
+	u16 core_count = smbios_processor_core_count();
+	u16 thread_count = smbios_processor_thread_count();
+
+	if (core_count > 255) {
      t->core_count = 0xff;
-	  t->core_count2 = (res.ebx >> 16) & 0xffff;
+	  t->core_count2 = core_count;
    } else {
-	  t->core_count = (res.ebx >> 16) & 0xff;
-	  t->core_count2 = (res.ebx >> 16) & 0x00ff;
+	  t->core_count = core_count & 0x00ff;
+	  t->core_count2 = 0x00ff;
    }
+	if (thread_count > 255) {
+	  t->thread_count = 0xff;
+	  t->thread_count2 = thread_count;
+	} else {
+	  t->thread_count = thread_count & 0x00ff;
+	  t->thread_count2 = 0x00ff;
+	}
+
    t->l1_cache_handle = 0xffff;
    t->l2_cache_handle = 0xffff;
    t->l3_cache_handle = 0xffff;
diff --git a/src/include/smbios.h b/src/include/smbios.h
index 4937dd6..24011ed 100644
--- a/src/include/smbios.h
+++ b/src/include/smbios.h
@@ -48,6 +48,8 @@

const char *smbios_mainboard_sku(void);
 u8 smbios_mainboard_enclosure_type(void);
+u16 smbios_processor_core_count(void);
+u16 smbios_processor_thread_count(void);
 #ifdef CONFIG_MAINBOARD_FAMILY
 const char *smbios_mainboard_family(void);
 #endif
@@ -424,7 +426,7 @@
    u8 header_format;
    u8 log_type_descriptors;
    u8 log_type_descriptor_length;
-	log_type_descriptor descriptor[0];
+	log_type_descriptor descriptor[256];
    u8 eos[2];
 } __packed;

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31863 )

Change subject: src/arch: An upgrade of SMBIOS to latest version 3.2
......................................................................

Patch Set 1:

(10 comments)

https://review.coreboot.org/#/c/31863/1/src/arch/x86/smbios.c 
File src/arch/x86/smbios.c:

https://review.coreboot.org/#/c/31863/1/src/arch/x86/smbios.c@551 
PS1, Line 551: u16 __weak smbios_processor_core_count(void) {
open brace '{' following function definitions go on the next line

https://review.coreboot.org/#/c/31863/1/src/arch/x86/smbios.c@564 
PS1, Line 564: 	
trailing whitespace

https://review.coreboot.org/#/c/31863/1/src/arch/x86/smbios.c@568 
PS1, Line 568: u16 __weak smbios_processor_thread_count(void) {
open brace '{' following function definitions go on the next line

https://review.coreboot.org/#/c/31863/1/src/arch/x86/smbios.c@571 
PS1, Line 571:  
trailing whitespace

https://review.coreboot.org/#/c/31863/1/src/arch/x86/smbios.c@571 
PS1, Line 571:  
please, no spaces at the start of a line

https://review.coreboot.org/#/c/31863/1/src/arch/x86/smbios.c@581 
PS1, Line 581: 	
trailing whitespace

https://review.coreboot.org/#/c/31863/1/src/arch/x86/smbios.c@612 
PS1, Line 612: 	if (core_count > 255) {
suspect code indent for conditional statements (8, 10)

https://review.coreboot.org/#/c/31863/1/src/arch/x86/smbios.c@615 
PS1, Line 615: 	} else {
suspect code indent for conditional statements (8, 10)

https://review.coreboot.org/#/c/31863/1/src/arch/x86/smbios.c@619 
PS1, Line 619: 	if (thread_count > 255) {
suspect code indent for conditional statements (8, 10)

https://review.coreboot.org/#/c/31863/1/src/arch/x86/smbios.c@622 
PS1, Line 622: 	} else {
suspect code indent for conditional statements (8, 10)

[Francois Toguo Fotso (Code Review)]

Francois Toguo Fotso has uploaded a new patch set (#2) to the change originally created by Francois Toguo Fotso. ( https://review.coreboot.org/c/coreboot/+/31863 )

Change subject: src/arch: An upgrade of SMBIOS to latest version 3.2
......................................................................

src/arch: An upgrade of SMBIOS to latest version 3.2

This is the second of 2 patches upgrading the SMBIOS interface to the latest 3.2
First patch is in mosys. Newer required fields are added to various types definitions

BUG=NONE
TEST=Boot to OS

Signed-off-by: Francois Toguo 
Change-Id: Idd2675ddd7196300019057f80a51bdd3b0e5b4e1
---
M src/arch/x86/smbios.c
M src/include/smbios.h
2 files changed, 67 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/31863/2

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31863 )

Change subject: src/arch: An upgrade of SMBIOS to latest version 3.2
......................................................................

Patch Set 2:

(14 comments)

https://review.coreboot.org/#/c/31863/2/src/arch/x86/smbios.c 
File src/arch/x86/smbios.c:

https://review.coreboot.org/#/c/31863/2/src/arch/x86/smbios.c@551 
PS2, Line 551: u16 __weak smbios_processor_core_count(void) 
trailing whitespace

https://review.coreboot.org/#/c/31863/2/src/arch/x86/smbios.c@565 
PS2, Line 565: 	
trailing whitespace

https://review.coreboot.org/#/c/31863/2/src/arch/x86/smbios.c@569 
PS2, Line 569: u16 __weak smbios_processor_thread_count(void) 
trailing whitespace

https://review.coreboot.org/#/c/31863/2/src/arch/x86/smbios.c@573 
PS2, Line 573:  
trailing whitespace

https://review.coreboot.org/#/c/31863/2/src/arch/x86/smbios.c@573 
PS2, Line 573:  
please, no spaces at the start of a line

https://review.coreboot.org/#/c/31863/2/src/arch/x86/smbios.c@574 
PS2, Line 574: 	while (1) 
trailing whitespace

https://review.coreboot.org/#/c/31863/2/src/arch/x86/smbios.c@574 
PS2, Line 574: 	while (1) 
that open brace { should be on the previous line

https://review.coreboot.org/#/c/31863/2/src/arch/x86/smbios.c@575 
PS2, Line 575:         {
code indent should use tabs where possible

https://review.coreboot.org/#/c/31863/2/src/arch/x86/smbios.c@575 
PS2, Line 575:         {
please, no spaces at the start of a line

https://review.coreboot.org/#/c/31863/2/src/arch/x86/smbios.c@584 
PS2, Line 584: 	
trailing whitespace

https://review.coreboot.org/#/c/31863/2/src/arch/x86/smbios.c@615 
PS2, Line 615: 	if (core_count > 255) {
suspect code indent for conditional statements (8, 10)

https://review.coreboot.org/#/c/31863/2/src/arch/x86/smbios.c@618 
PS2, Line 618: 	} else {
suspect code indent for conditional statements (8, 10)

https://review.coreboot.org/#/c/31863/2/src/arch/x86/smbios.c@622 
PS2, Line 622: 	if (thread_count > 255) {
suspect code indent for conditional statements (8, 10)

https://review.coreboot.org/#/c/31863/2/src/arch/x86/smbios.c@625 
PS2, Line 625: 	} else {
suspect code indent for conditional statements (8, 10)

[Francois Toguo Fotso (Code Review)]

Francois Toguo Fotso has uploaded a new patch set (#3) to the change originally created by Francois Toguo Fotso. ( https://review.coreboot.org/c/coreboot/+/31863 )

Change subject: src/arch: An upgrade of SMBIOS to latest version 3.2
......................................................................

src/arch: An upgrade of SMBIOS to latest version 3.2

This is the second of 2 patches upgrading the SMBIOS interface to the latest 3.2
First patch is in mosys. Newer required fields are added to various types definitions

BUG=NONE
TEST=Boot to OS

Signed-off-by: Francois Toguo 
Change-Id: Idd2675ddd7196300019057f80a51bdd3b0e5b4e1
---
M src/arch/x86/smbios.c
M src/include/smbios.h
2 files changed, 67 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/31863/3

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31863 )

Change subject: src/arch: An upgrade of SMBIOS to latest version 3.2
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/#/c/31863/3/src/arch/x86/smbios.c 
File src/arch/x86/smbios.c:

https://review.coreboot.org/#/c/31863/3/src/arch/x86/smbios.c@574 
PS3, Line 574: 	while (1){
space required before the open brace '{'

https://review.coreboot.org/#/c/31863/3/src/arch/x86/smbios.c@583 
PS3, Line 583: 	
trailing whitespace

[Francois Toguo Fotso (Code Review)]

Francois Toguo Fotso has uploaded a new patch set (#4) to the change originally created by Francois Toguo Fotso. ( https://review.coreboot.org/c/coreboot/+/31863 )

Change subject: src/arch: An upgrade of SMBIOS to latest version 3.2
......................................................................

src/arch: An upgrade of SMBIOS to latest version 3.2

This is the second of 2 patches upgrading the SMBIOS interface to the latest 3.2
First patch is in mosys. Newer required fields are added to various types definitions

BUG=NONE
TEST=Boot to OS

Signed-off-by: Francois Toguo 
Change-Id: Idd2675ddd7196300019057f80a51bdd3b0e5b4e1
---
M src/arch/x86/smbios.c
M src/include/smbios.h
2 files changed, 67 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/31863/4

[Francois Toguo Fotso (Code Review)]

Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31863 )

Change subject: src/arch: An upgrade of SMBIOS to latest version 3.2
......................................................................

Patch Set 4:

This is a follow up of CL :  https://review.coreboot.org/c/coreboot/+/31725/4
which is being abandoned. Previous finding are implemented here.

Please note core count are being computed with leaf 0xb per page 768 of
https://www.intel.com/content/www/us/en/architecture-and-technology/64-ia-32...
Result verified with a GLK platform.
Those new APIs can be overwriten as needed per platform.

Some new fields are set to their default/unknown values. They will populated later on after fsp upgrades.

Francois

[Richard Spiegel (Code Review)]

Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31863 )

Change subject: src/arch: An upgrade of SMBIOS to latest version 3.2
......................................................................

Patch Set 4: Code-Review+1

(2 comments)

Because the procedure can be overwritten, and in normal conditions should produce the proper numbers, I will not block it. But I can't approve it either (see comment I placed on line 560). I'll leave to someone with more coreboot experience to say if this is acceptable under coreboot normal use.
For those that did not received the invitation email, the document (not the full path) is:
architecture-and-technology/64-ia-32-architectures-software-developer-vol-2a-manual.html, and I copied some of the comment ** on page 196.

https://review.coreboot.org/#/c/31863/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31863/4//COMMIT_MSG@13 
PS4, Line 13: TEST=Boot to OS
Would be nice to also include the platform used.

https://review.coreboot.org/#/c/31863/4/src/arch/x86/smbios.c 
File src/arch/x86/smbios.c:

https://review.coreboot.org/#/c/31863/4/src/arch/x86/smbios.c@560 
PS4, Line 560: core_count = leaf_b.ebx & 0xffff;
From the manual you pointed on your email:
 The actual number of logical processors available to BIOS/OS/Applications may be different from the value of EBX[15:0], depending on software and platform hardware configurations.
This was the very reason  https://review.coreboot.org/c/coreboot/+/31725/4 was abandoned. The value computed is the number of cores as shipped (after fuse burning), not the actual number of used cores. Though I agree that both numbers will tend to be the same (and therefore better than the abandoned version), I believe that a better option would be to actually count the cores as they are being initialized, save the value in memory for use within SMBIOS.

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31863 )

Change subject: src/arch: An upgrade of SMBIOS to latest version 3.2
......................................................................

Patch Set 4:

(7 comments)

If you are going to abandon the parent change, you'd
mean to squash them, right? Maybe it would make sense
to move the CPUID additions into its own change, though,
e.g. one change that adds the 3.2 fields with current
values, and one that enhances the core/thread_count
detection.

https://review.coreboot.org/#/c/31863/4/src/arch/x86/smbios.c 
File src/arch/x86/smbios.c:

https://review.coreboot.org/#/c/31863/4/src/arch/x86/smbios.c@551 
PS4, Line 551: u16 __weak smbios_processor_core_count(void)
Generally, non-empty weak implementations are discouraged. OTOH,
this has no side-effects, so I would accept it.

https://review.coreboot.org/#/c/31863/4/src/arch/x86/smbios.c@556 
PS4, Line 556: 	while (1) {
please, use a for loop

https://review.coreboot.org/#/c/31863/4/src/arch/x86/smbios.c@558 
PS4, Line 558: 		leaf_b = cpuid_ext(0xb, ecx);
Should we check if leaf 0xb is implemented and return 1 if not?

Are there processors that have leaf 0x1 implemented but not 0xb?
would it make sense to fall back to the old behavior in this case?

https://review.coreboot.org/#/c/31863/4/src/arch/x86/smbios.c@569 
PS4, Line 569: u16 __weak smbios_processor_thread_count(void)
This is basically the same function as the one above. How about
adding a `level_type` parameter instead of repeating the whole
implementation?

https://review.coreboot.org/#/c/31863/4/src/arch/x86/smbios.c@618 
PS4, Line 618:  & 0x00ff
Nit, explicit masking isn't necessary.

https://review.coreboot.org/#/c/31863/4/src/arch/x86/smbios.c@619 
PS4, Line 619: 		t->core_count2 = 0x00ff;
AFAIR, the spec says this field always contains the correct
count if present.

https://review.coreboot.org/#/c/31863/4/src/include/smbios.h 
File src/include/smbios.h:

https://review.coreboot.org/#/c/31863/4/src/include/smbios.h@429 
PS4, Line 429: 	log_type_descriptor descriptor[256];
any plans to use it?

[Francois Toguo Fotso (Code Review)]

Francois Toguo Fotso has uploaded a new patch set (#5) to the change originally created by Francois Toguo Fotso. ( https://review.coreboot.org/c/coreboot/+/31863 )

Change subject: src/arch: An upgrade of SMBIOS to latest version 3.2
......................................................................

src/arch: An upgrade of SMBIOS to latest version 3.2

This is the second of 2 patches upgrading the SMBIOS interface to the latest 3.2
First patch is in mosys. Newer required fields are added to various types definitions

BUG=NONE
TEST=Boot to OS on GLK Sparky

Signed-off-by: Francois Toguo 
Change-Id: Idd2675ddd7196300019057f80a51bdd3b0e5b4e1
---
M src/arch/x86/smbios.c
M src/include/smbios.h
2 files changed, 67 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/31863/5

[Francois Toguo Fotso (Code Review)]

Francois Toguo Fotso has uploaded a new patch set (#6) to the change originally created by Francois Toguo Fotso. ( https://review.coreboot.org/c/coreboot/+/31863 )

Change subject: src/arch: An upgrade of SMBIOS to latest version 3.2
......................................................................

src/arch: An upgrade of SMBIOS to latest version 3.2

This is the second of 2 patches upgrading the SMBIOS interface to the latest 3.2
First patch is in mosys. Newer required fields are added to various types definitions

BUG=NONE
TEST=Boot to OS on GLK Sparky

Signed-off-by: Francois Toguo 
Change-Id: Idd2675ddd7196300019057f80a51bdd3b0e5b4e1
---
M src/arch/x86/smbios.c
M src/include/smbios.h
2 files changed, 43 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/31863/6

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31863 )

Change subject: src/arch: An upgrade of SMBIOS to latest version 3.2
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/#/c/31863/6/src/arch/x86/smbios.c 
File src/arch/x86/smbios.c:

https://review.coreboot.org/#/c/31863/6/src/arch/x86/smbios.c@559 
PS6, Line 559: 		if (leaf_b.eax == 0x0000 && leaf_b.ebx == 0x0000 
trailing whitespace

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31863 )

Change subject: src/arch: An upgrade of SMBIOS to latest version 3.2
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/#/c/31863/6/src/arch/x86/smbios.c 
File src/arch/x86/smbios.c:

https://review.coreboot.org/#/c/31863/6/src/arch/x86/smbios.c@560 
PS6, Line 560: 		&& leaf_b.ecx == 0x0000 && leaf_b.edx == 0x0000) {
I guess you have to check the eax result of cpuid leaf 0. If there
is no function for that in coreboot yet, it's worth to add it to
`arch/x86/cpuid`.

"If a value entered for CPUID.EAX is higher than the maximum input
  value for basic or extended function for that processor then the
  data for the highest basic information leaf is returned."

[Francois Toguo Fotso (Code Review)]

Francois Toguo Fotso has uploaded a new patch set (#7) to the change originally created by Francois Toguo Fotso. ( https://review.coreboot.org/c/coreboot/+/31863 )

Change subject: src/arch: An upgrade of SMBIOS to latest version 3.2
......................................................................

src/arch: An upgrade of SMBIOS to latest version 3.2

This is the second of 2 patches upgrading the SMBIOS interface to the latest 3.2
First patch is in mosys. Newer required fields are added to various types definitions

BUG=NONE
TEST=Boot to OS on GLK Sparky

Signed-off-by: Francois Toguo 
Change-Id: Idd2675ddd7196300019057f80a51bdd3b0e5b4e1
---
M src/arch/x86/smbios.c
M src/include/smbios.h
2 files changed, 42 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/31863/7

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31863 )

Change subject: src/arch: An upgrade of SMBIOS to latest version 3.2
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/#/c/31863/7/src/arch/x86/smbios.c 
File src/arch/x86/smbios.c:

https://review.coreboot.org/#/c/31863/7/src/arch/x86/smbios.c@559 
PS7, Line 559: 		if (!(leaf_b.eax | leaf_b.ebx | leaf_b.ecx | leaf_b.edx ) {
space prohibited before that close parenthesis ')'

[Francois Toguo Fotso (Code Review)]

Francois Toguo Fotso has uploaded a new patch set (#8) to the change originally created by Francois Toguo Fotso. ( https://review.coreboot.org/c/coreboot/+/31863 )

Change subject: src/arch: An upgrade of SMBIOS to latest version 3.2
......................................................................

src/arch: An upgrade of SMBIOS to latest version 3.2

This is the second of 2 patches upgrading the SMBIOS interface to the latest 3.2
First patch is in mosys. Newer required fields are added to various types definitions

BUG=NONE
TEST=Boot to OS on GLK Sparky

Signed-off-by: Francois Toguo 
Change-Id: Idd2675ddd7196300019057f80a51bdd3b0e5b4e1
---
M src/arch/x86/smbios.c
M src/include/smbios.h
2 files changed, 42 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/31863/8

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31863 )

Change subject: src/arch: An upgrade of SMBIOS to latest version 3.2
......................................................................

Patch Set 8:

(2 comments)

https://review.coreboot.org/#/c/31863/8/src/arch/x86/smbios.c 
File src/arch/x86/smbios.c:

https://review.coreboot.org/#/c/31863/8/src/arch/x86/smbios.c@559 
PS8, Line 559: 		if (!(leaf_b.eax | leaf_b.ebx | leaf_b.ecx | leaf_b.edx )) {
space prohibited before that close parenthesis ')'

https://review.coreboot.org/#/c/31863/8/src/arch/x86/smbios.c@559 
PS8, Line 559: 		if (!(leaf_b.eax | leaf_b.ebx | leaf_b.ecx | leaf_b.edx )) {
braces {} are not necessary for single statement blocks

[Francois Toguo Fotso (Code Review)]

Francois Toguo Fotso has uploaded a new patch set (#9) to the change originally created by Francois Toguo Fotso. ( https://review.coreboot.org/c/coreboot/+/31863 )

Change subject: src/arch: An upgrade of SMBIOS to latest version 3.2
......................................................................

src/arch: An upgrade of SMBIOS to latest version 3.2

This is the second of 2 patches upgrading the SMBIOS interface to the latest 3.2
First patch is in mosys. Newer required fields are added to various types definitions

BUG=NONE
TEST=Boot to OS on GLK Sparky

Signed-off-by: Francois Toguo 
Change-Id: Idd2675ddd7196300019057f80a51bdd3b0e5b4e1
---
M src/arch/x86/smbios.c
M src/include/smbios.h
2 files changed, 42 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/31863/9

[Francois Toguo Fotso (Code Review)]

Francois Toguo Fotso has uploaded a new patch set (#10) to the change originally created by Francois Toguo Fotso. ( https://review.coreboot.org/c/coreboot/+/31863 )

Change subject: src/arch: An upgrade of SMBIOS to latest version 3.2
......................................................................

src/arch: An upgrade of SMBIOS to latest version 3.2

This is the second of 2 patches upgrading the SMBIOS interface to the latest 3.2
First patch is in mosys. Newer required fields are added to various types definitions

BUG=NONE
TEST=Boot to OS on GLK Sparky

Signed-off-by: Francois Toguo 
Change-Id: Idd2675ddd7196300019057f80a51bdd3b0e5b4e1
---
M src/arch/x86/smbios.c
M src/include/smbios.h
2 files changed, 42 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/31863/10

[Francois Toguo Fotso (Code Review)]

Francois Toguo Fotso has uploaded a new patch set (#11) to the change originally created by Francois Toguo Fotso. ( https://review.coreboot.org/c/coreboot/+/31863 )

Change subject: src/arch: An upgrade of SMBIOS to latest version 3.2
......................................................................

src/arch: An upgrade of SMBIOS to latest version 3.2

This is the second of 2 patches upgrading the SMBIOS interface to the latest 3.2
First patch is in mosys. Newer required fields are added to various types definitions

BUG=NONE
TEST=Boot to OS on GLK Sparky

Signed-off-by: Francois Toguo 
Change-Id: Idd2675ddd7196300019057f80a51bdd3b0e5b4e1
---
M src/arch/x86/smbios.c
M src/include/smbios.h
2 files changed, 41 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/31863/11

[Lijian Zhao (Code Review)]

Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31863 )

Change subject: src/arch: An upgrade of SMBIOS to latest version 3.2
......................................................................

Patch Set 11:

(1 comment)

https://review.coreboot.org/#/c/31863/11/src/arch/x86/smbios.c 
File src/arch/x86/smbios.c:

https://review.coreboot.org/#/c/31863/11/src/arch/x86/smbios.c@556 
PS11, Line 556: int ecx = 0 
just ecx

[Francois Toguo Fotso (Code Review)]

Francois Toguo Fotso has uploaded a new patch set (#12) to the change originally created by Francois Toguo Fotso. ( https://review.coreboot.org/c/coreboot/+/31863 )

Change subject: src/arch: An upgrade of SMBIOS to latest version 3.2
......................................................................

src/arch: An upgrade of SMBIOS to latest version 3.2

This is the second of 2 patches upgrading the SMBIOS interface to the latest 3.2
First patch is in mosys. Newer required fields are added to various types definitions

BUG=NONE
TEST=Boot to OS on GLK Sparky

Signed-off-by: Francois Toguo 
Change-Id: Idd2675ddd7196300019057f80a51bdd3b0e5b4e1
---
M src/arch/x86/smbios.c
M src/include/smbios.h
2 files changed, 41 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/31863/12

[Francois Toguo Fotso (Code Review)]

Francois Toguo Fotso has uploaded a new patch set (#13) to the change originally created by Francois Toguo Fotso. ( https://review.coreboot.org/c/coreboot/+/31863 )

Change subject: src/arch: An upgrade of SMBIOS to latest version 3.2
......................................................................

src/arch: An upgrade of SMBIOS to latest version 3.2

This is the second of 2 patches upgrading the SMBIOS interface to the latest 3.2
First patch is in mosys. Newer required fields are added to various types definitions

BUG=NONE
TEST=Boot to OS on GLK Sparky

Signed-off-by: Francois Toguo 
Change-Id: Idd2675ddd7196300019057f80a51bdd3b0e5b4e1
---
M src/arch/x86/smbios.c
M src/include/smbios.h
2 files changed, 41 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/31863/13

[Francois Toguo Fotso (Code Review)]

Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31863 )

Change subject: src/arch: An upgrade of SMBIOS to latest version 3.2
......................................................................

Patch Set 13:

(10 comments)
> Patch Set 6:
>
> (1 comment)

Done.
Sure the function can be added in futures patches, since this upgrade will be done in several steps.But in this case I am checking to see if the leaf_b is implemented per your previous suggestion. This sentence from the same document captures the approach I am taking:

"If a value entered for CPUID.EAX is less than or equal to the maximum input value and the leaf is not supported on that processor then 0 is returned in all the registers"

https://review.coreboot.org/#/c/31863/4/src/arch/x86/smbios.c 
File src/arch/x86/smbios.c:

https://review.coreboot.org/#/c/31863/4/src/arch/x86/smbios.c@551 
PS4, Line 551: u16 __weak smbios_processor_core_count(void)
> Generally, non-empty weak implementations are discouraged. OTOH, […]

Done

https://review.coreboot.org/#/c/31863/4/src/arch/x86/smbios.c@556 
PS4, Line 556: 	while (1) {
> please, use a for loop

Done

https://review.coreboot.org/#/c/31863/4/src/arch/x86/smbios.c@558 
PS4, Line 558: 		leaf_b = cpuid_ext(0xb, ecx);
> Should we check if leaf 0xb is implemented and return 1 if not? […]

Done

https://review.coreboot.org/#/c/31863/4/src/arch/x86/smbios.c@569 
PS4, Line 569: u16 __weak smbios_processor_thread_count(void)
> This is basically the same function as the one above. How about […]

Done

https://review.coreboot.org/#/c/31863/4/src/arch/x86/smbios.c@618 
PS4, Line 618:  & 0x00ff
> Nit, explicit masking isn't necessary.

Done

https://review.coreboot.org/#/c/31863/4/src/arch/x86/smbios.c@619 
PS4, Line 619: 		t->core_count2 = 0x00ff;
> AFAIR, the spec says this field always contains the correct […]

Done

https://review.coreboot.org/#/c/31863/6/src/arch/x86/smbios.c 
File src/arch/x86/smbios.c:

https://review.coreboot.org/#/c/31863/6/src/arch/x86/smbios.c@559 
PS6, Line 559: 		if (leaf_b.eax == 0x0000 && leaf_b.ebx == 0x0000
> trailing whitespace

Done

https://review.coreboot.org/#/c/31863/6/src/arch/x86/smbios.c@560 
PS6, Line 560: 		&& leaf_b.ecx == 0x0000 && leaf_b.edx == 0x0000) {
> I guess you have to check the eax result of cpuid leaf 0. If there […]

Sure the function can be added in futures patches, since this upgrade will be done in several steps.But in this case I am checkind to see if the leaf_b is implemented per your previous suggestion. This sentence from the same document captures the approach I am taking:
"If a value entered for CPUID.EAX is less than or equal to the maximum input value and the leaf is not supported on that processor then 0 is returned in all the registers"

https://review.coreboot.org/#/c/31863/11/src/arch/x86/smbios.c 
File src/arch/x86/smbios.c:

https://review.coreboot.org/#/c/31863/11/src/arch/x86/smbios.c@556 
PS11, Line 556: int ecx = 0
> just ecx

Done

https://review.coreboot.org/#/c/31863/4/src/include/smbios.h 
File src/include/smbios.h:

https://review.coreboot.org/#/c/31863/4/src/include/smbios.h@429 
PS4, Line 429: 	log_type_descriptor descriptor[256];
> any plans to use it?

Done

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31863 )

Change subject: src/arch: An upgrade of SMBIOS to latest version 3.2
......................................................................

Patch Set 13:

(1 comment)

We have a Cherry-Pick only rule in our Gerrit setup.
So no merging and every commit to be submitted has
to apply to the master branch. That's not the case
for this commit, currently, because half of the
changes were in the other, abandoned commit. If you
want to submit everything as one change, you'll have
to squash the two commits into one. If you do,
please retain the Change-Id of the second commit.

https://review.coreboot.org/#/c/31863/6/src/arch/x86/smbios.c 
File src/arch/x86/smbios.c:

https://review.coreboot.org/#/c/31863/6/src/arch/x86/smbios.c@560 
PS6, Line 560: 		&& leaf_b.ecx == 0x0000 && leaf_b.edx == 0x0000) {
> Sure the function can be added in futures patches, since this upgrade will be done in several steps. […]

Ah I missed that there are two cases of unsupported leafs.
I don't know any database that would tell me since when
leafs >=0xb are implemented. If we can't tell that for sure
for all coreboot supported x86 processors (I think we have
Pentium 2, still), we should implement both checks, I guess.

[Francois Toguo Fotso (Code Review)]

Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31863 )

Change subject: src/arch: An upgrade of SMBIOS to latest version 3.2
......................................................................

Patch Set 13:
> Patch Set 13:
>
> (1 comment)
>
> We have a Cherry-Pick only rule in our Gerrit setup.
> So no merging and every commit to be submitted has
> to apply to the master branch. That's not the case
> for this commit, currently, because half of the
> changes were in the other, abandoned commit. If you
> want to submit everything as one change, you'll have
> to squash the two commits into one. If you do,
> please retain the Change-Id of the second commit.

The first check of eax fear leaf 0 have been added in new patch which now combines changes from this one and the first abandoned one. This one is also being abandoned to resolve the conflict encountered in the squash attempt.

new patch: https://review.coreboot.org/c/coreboot/+/31997

[Francois Toguo Fotso (Code Review)]

Francois Toguo Fotso has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/31863 )

Change subject: src/arch: An upgrade of SMBIOS to latest version 3.2
......................................................................

Abandoned

new patch: https://review.coreboot.org/c/coreboot/+/31997