Change in ...coreboot[master]: src/arch: This is an upgrade of SMBIOS to latest version 3.2
Francois Toguo Fotso has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32090
Change subject: src/arch: This is an upgrade of SMBIOS to latest version 3.2 ......................................................................
src/arch: This is 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
Change-Id: I1171c20ac85a2231d949d4ac1c3e3c544ba3e5ef Signed-off-by: Francois Toguo francois.toguo.fotso@intel.com --- M src/arch/x86/smbios.c M src/include/smbios.h 2 files changed, 71 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/32090/1
diff --git a/src/arch/x86/smbios.c b/src/arch/x86/smbios.c index 8cb59df..b82780d 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); }
@@ -547,9 +558,31 @@ return len; }
+u16 __weak smbios_processor_core_thread_count(u16 level_type) +{ + u16 count = 0; + int ecx = 0; + + for (ecx = 0 ; ecx < 255 ; ecx++) { + struct cpuid_result leaf_b; + leaf_b = cpuid_ext(0xb, ecx); + if ((cpuid_eax(0) < 0xb) || + !(leaf_b.eax | leaf_b.ebx | leaf_b.ecx | leaf_b.edx)) + return (((cpuid(1).ebx) >> 16) & 0x00ff); + + if ((leaf_b.ecx & 0xff00) == level_type) { + count = leaf_b.ebx & 0xffff; + break; + } + } + + return count; +} + static int smbios_write_type4(unsigned long *current, int handle) { struct cpuid_result res; + u16 core_count = 0, thread_count = 0; struct smbios_type4 *t = (struct smbios_type4 *)*current; int len = sizeof(struct smbios_type4);
@@ -570,7 +603,15 @@ t->processor_version = smbios_processor_name(t->eos); t->processor_family = (res.eax > 0) ? 0x0c : 0x6; t->processor_type = 3; /* System Processor */ - t->core_count = (res.ebx >> 16) & 0xff; + + core_count = smbios_processor_core_thread_count(PROC_CORE_TYPE); + thread_count = smbios_processor_core_thread_count(PROC_THREAD_TYPE); + t->core_count2 = core_count; + t->core_count = (core_count > BYTE_LIMIT) ? 0xff : core_count; + t->thread_count2 = thread_count; + t->thread_count = (thread_count > BYTE_LIMIT) ? 0xff : core_count; + t->core_enabled2 = core_count; + 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 af83bfe..e1d71be 100644 --- a/src/include/smbios.h +++ b/src/include/smbios.h @@ -52,6 +52,10 @@ u8 smbios_mainboard_feature_flags(void); const char *smbios_mainboard_location_in_chassis(void); u8 smbios_mainboard_enclosure_type(void); +u16 smbios_processor_core_thread_count(u16 level_type); +#ifdef CONFIG_MAINBOARD_FAMILY +const char *smbios_mainboard_family(void); +#endif
#define BIOS_CHARACTERISTICS_PCI_SUPPORTED (1 << 7) #define BIOS_CHARACTERISTICS_PC_CARD (1 << 8) @@ -100,6 +104,11 @@ #define MEMORY_OPERATING_MODE_CAP_BYTE_ACCESS_PERSISTENT (1 << 4) #define MEMORY_OPERATING_MODE_CAP_BLOCK_ACCESS_PERSISTENT (1 << 5)
+#define PROC_THREAD_TYPE 0x1 +#define PROC_CORE_TYPE 0x2 + +#define BYTE_LIMIT 255 + typedef enum { MEMORY_BUS_WIDTH_8 = 0, MEMORY_BUS_WIDTH_16 = 1, @@ -299,6 +308,7 @@ u8 location_in_chassis; u16 chassis_handle; u8 board_type; + u8 num_cont_obj_handles; u8 eos[2]; } __packed;
@@ -390,6 +400,9 @@ u8 thread_count; u16 processor_characteristics; u16 processor_family2; + u16 core_count2; + u16 core_enabled2; + u16 thread_count2; u8 eos[2]; } __packed;
@@ -401,6 +414,11 @@ u8 eos[2]; } __packed;
+typedef struct { + u8 type; + u8 format_descriptor; +} log_type_descriptor; + struct smbios_type15 { u8 type; u8 length; @@ -471,6 +489,17 @@ u16 minimum_voltage; u16 maximum_voltage; u16 configured_voltage; + u8 memory_technology; + u16 operating_mode_capability; + u8 fw_version; + u16 manufacturer_id; + u16 product_id; + u16 sub_ctrl_manufacturer_id; + u16 sub_ctrl_product_id; + u64 non_volatile_size; + u64 volatile_size; + u64 cache_size; + u64 logical_size; u8 eos[2]; } __packed;
Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: src/arch: This is an upgrade of SMBIOS to latest version 3.2 ......................................................................
Patch Set 1:
This is the follow up of the reverted patch: https://review.coreboot.org/c/coreboot/+/32059
Please take a look.
Regards, Francois
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: src/arch: This is an upgrade of SMBIOS to latest version 3.2 ......................................................................
Patch Set 1: Code-Review-1
(4 comments)
https://review.coreboot.org/#/c/32090/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32090/1//COMMIT_MSG@7 PS1, Line 7: src/arch: This is an upgrade of SMBIOS to latest version 3.2 Please use a verb in imperative mood, cf `git log --oneline` for how to word git commit summaries.
smbios: Add and adapt fields for version 3.2
https://review.coreboot.org/#/c/32090/1//COMMIT_MSG@9 PS1, Line 9: 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 Please add dots to the end of sentences.
Better reference the commit by hash and commit message summary, if it has been submitted/committed already.
https://review.coreboot.org/#/c/32090/1//COMMIT_MSG@11 PS1, Line 11: Please elaborate more about the change. Thread counts on so on.
https://review.coreboot.org/#/c/32090/1/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/#/c/32090/1/src/arch/x86/smbios.c@570 PS1, Line 570: !(leaf_b.eax | leaf_b.ebx | leaf_b.ecx | leaf_b.edx)) Please indent the condition.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: src/arch: This is an upgrade of SMBIOS to latest version 3.2 ......................................................................
Patch Set 1:
(1 comment)
Sorry for the revert. Please, split out the functional changes (e.g. the cpuid stuff around core/thread count) into their own patch (it's not related to the SMBIOS update, is it?). This change does a lot of things, and I haven't spotted anything yet that is *required* for SMBIOS 3.2.
https://review.coreboot.org/#/c/32090/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32090/1//COMMIT_MSG@11 PS1, Line 11:
Please elaborate more about the change. Thread counts on so on.
I think most of all, this commit message needs to give a reason for the changes. If all the added fields (I haven't checked) are optional in SMBIOS 3.2, then this would just be a cosmetic change? Is it done in preperation for other changes in the queue? or just for anticipated future changes?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: src/arch: This is an upgrade of SMBIOS to latest version 3.2 ......................................................................
Patch Set 1:
(6 comments)
https://review.coreboot.org/#/c/32090/1/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/#/c/32090/1/src/arch/x86/smbios.c@279 PS1, Line 279: t->clock_speed = dimm->ddr_frequency; clock_speed and speed are now in units of MT/s instead of Mhz. Please update all callers that provide the value in Mhz.
https://review.coreboot.org/#/c/32090/1/src/arch/x86/smbios.c@331 PS1, Line 331: t->memory_technology = MEMORY_TECHNOLOGY_UNKNOWN; likely DRAM
https://review.coreboot.org/#/c/32090/1/src/arch/x86/smbios.c@561 PS1, Line 561: u16 __weak smbios_processor_core_thread_count(u16 level_type) why it it __weak?
https://review.coreboot.org/#/c/32090/1/src/arch/x86/smbios.c@824 PS1, Line 824: se->major_version = 2; update to 3
https://review.coreboot.org/#/c/32090/1/src/arch/x86/smbios.c@825 PS1, Line 825: se->minor_version = 7; update to 2
https://review.coreboot.org/#/c/32090/1/src/arch/x86/smbios.c@829 PS1, Line 829: add the 64bit entry point as specified in SMBIOS 3.0. It has an anchor "_SM3_"
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/+/32090 )
Change subject: smbios: Add and adapt fields for version 3.2 ......................................................................
smbios: Add and adapt fields for version 3.2
This is the second of 2 patches upgrading the SMBIOS interface to 3.2. First patch "platform/mosys: This upgrades SMBIOS to latest version 3.2". First patch SHA: a1943f6e5aaa4fa38208ee43576882b54471797f. The added fields, in anticipation for future change, are part of the new 3.2 spec of SMBIOS. The new procedure populates the core and thread count(s).
BUG=NONE TEST=Boot to OS on GLK Sparky
Change-Id: I1171c20ac85a2231d949d4ac1c3e3c544ba3e5ef Signed-off-by: Francois Toguo francois.toguo.fotso@intel.com --- M src/arch/x86/smbios.c M src/include/smbios.h 2 files changed, 71 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/32090/2
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/+/32090 )
Change subject: smbios: Add and adapt fields for version 3.2 ......................................................................
smbios: Add and adapt fields for version 3.2
This is the second of 2 patches upgrading the SMBIOS interface to 3.2. First patch "platform/mosys: This upgrades SMBIOS to latest version 3.2". First patch SHA: a1943f6e5aaa4fa38208ee43576882b54471797f. The added fields, in anticipation for future change, are part of the new 3.2 spec of SMBIOS. The new procedure populates the core and thread count(s).
BUG=NONE TEST=Boot to OS on GLK Sparky
Change-Id: I1171c20ac85a2231d949d4ac1c3e3c544ba3e5ef Signed-off-by: Francois Toguo francois.toguo.fotso@intel.com --- M src/arch/x86/smbios.c M src/include/smbios.h 2 files changed, 85 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/32090/3
Hello Patrick Rudolph, Paul Menzel, build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32090
to look at the new patch set (#4).
Change subject: smbios: Add and adapt fields for version 3.2 ......................................................................
smbios: Add and adapt fields for version 3.2
This is the second of 2 patches upgrading the SMBIOS interface to 3.2. First patch "platform/mosys: This upgrades SMBIOS to latest version 3.2". First patch SHA: a1943f6e5aaa4fa38208ee43576882b54471797f. The added fields, in anticipation for future change, are part of the new 3.2 spec of SMBIOS. The new procedure populates the core and thread count(s).
BUG=NONE TEST=Boot to OS on GLK Sparky
Change-Id: I1171c20ac85a2231d949d4ac1c3e3c544ba3e5ef Signed-off-by: Francois Toguo francois.toguo.fotso@intel.com --- M src/arch/x86/smbios.c M src/include/smbios.h 2 files changed, 74 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/32090/4
Hello Patrick Rudolph, Paul Menzel, build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32090
to look at the new patch set (#5).
Change subject: smbios: Add and adapt fields for version 3.2 ......................................................................
smbios: Add and adapt fields for version 3.2
This is the second of 2 patches upgrading the SMBIOS interface to 3.2. First patch "platform/mosys: This upgrades SMBIOS to latest version 3.2". First patch SHA: a1943f6e5aaa4fa38208ee43576882b54471797f. The added fields, in anticipation for future change, are part of the new 3.2 spec of SMBIOS. The new procedure populates the core and thread count(s).
BUG=NONE TEST=Boot to OS on GLK Sparky
Change-Id: I1171c20ac85a2231d949d4ac1c3e3c544ba3e5ef Signed-off-by: Francois Toguo francois.toguo.fotso@intel.com --- M src/arch/x86/smbios.c M src/include/smbios.h 2 files changed, 76 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/32090/5
Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for version 3.2 ......................................................................
Patch Set 5:
(6 comments)
Patch Set 1:
(6 comments)
Patrick, - Line 561: "_weak" so that it can be if needed for som platform/soc - Line 829: I agree that the entry point should be added as as specified in SMBIOS 3.0. it would be more disruptive in this CL, as the change will also need to go in other other files where the table entry are used. I suggest having a later patch and review exclusively for the table entry update.
https://review.coreboot.org/#/c/32090/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32090/1//COMMIT_MSG@7 PS1, Line 7: src/arch: This is an upgrade of SMBIOS to latest version 3.2
Please use a verb in imperative mood, cf `git log --oneline` for how to word git commit summaries. […]
Done
https://review.coreboot.org/#/c/32090/1//COMMIT_MSG@9 PS1, Line 9: 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
Please add dots to the end of sentences. […]
Done
https://review.coreboot.org/#/c/32090/1//COMMIT_MSG@11 PS1, Line 11:
I think most of all, this commit message needs to give a reason […]
Done
https://review.coreboot.org/#/c/32090/1/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/#/c/32090/1/src/arch/x86/smbios.c@570 PS1, Line 570: !(leaf_b.eax | leaf_b.ebx | leaf_b.ecx | leaf_b.edx))
Please indent the condition.
Done
https://review.coreboot.org/#/c/32090/1/src/arch/x86/smbios.c@824 PS1, Line 824: se->major_version = 2;
update to 3
Done
https://review.coreboot.org/#/c/32090/1/src/arch/x86/smbios.c@825 PS1, Line 825: se->minor_version = 7;
update to 2
Done
Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for version 3.2 ......................................................................
Patch Set 5:
Patch Set 1:
(1 comment)
Sorry for the revert. Please, split out the functional changes (e.g. the cpuid stuff around core/thread count) into their own patch (it's not related to the SMBIOS update, is it?). This change does a lot of things, and I haven't spotted anything yet that is *required* for SMBIOS 3.2.
Nico, From the 3.2 spec, core counts and thread counts don't seem to be optional. The newly added procedure is populating them to comply to one of your previous comment: " please don't add fields without giving them a reasonable value" I can remove it from this patch if you require it and set the values to 0. Because the count generated by the code before this upgrade was not accurate, at least not on the GKL Sparky platform.
Regards, Francois
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for version 3.2 ......................................................................
Patch Set 5:
(3 comments)
Patch Set 5:
(6 comments)
Patch Set 1:
(6 comments)
Patrick,
- Line 561: "_weak" so that it can be if needed for som platform/soc
- Line 829: I agree that the entry point should be added as as specified in SMBIOS 3.0. it would be more disruptive in this CL, as the change will also need to go in other other files where the table entry are used. I suggest having a later patch and review exclusively for the table entry update.
It's not disruptive. Just a few lines to add in this file. No need to update the callers.
https://review.coreboot.org/#/c/32090/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32090/5//COMMIT_MSG@10 PS5, Line 10: platform/mosys I can't find it on Gerrit.
https://review.coreboot.org/#/c/32090/5//COMMIT_MSG@13 PS5, Line 13: thread It does more than that.
https://review.coreboot.org/#/c/32090/5/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/#/c/32090/5/src/arch/x86/smbios.c@279 PS5, Line 279: ddr_frequency Are you sure that all callers provide the value in MHz?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for version 3.2 ......................................................................
Patch Set 5:
Sorry for the revert. Please, split out the functional changes (e.g. the cpuid stuff around core/thread count) into their own patch (it's not related to the SMBIOS update, is it?). This change does a lot of things, and I haven't spotted anything yet that is *required* for SMBIOS 3.2.
Nico, From the 3.2 spec, core counts and thread counts don't seem to be optional. The newly added procedure is populating them to comply to one of your previous comment: " please don't add fields without giving them a reasonable value"
Well, core/thread count are not added by in this change, so my comment doesn't apply to them. I was mostly refer- ring to the added fields that were left untouched (at 0) where 0 isn't a valid value. Sorry for the confusion.
Still, you could fix the cpuid code first, then add SMBIOS 3.2 stuff. I don't see why it would have to be part of the same commit.
I can remove it from this patch if you require it and set the values to 0. Because the count generated by the code before this upgrade was not accurate, at least not on the GKL Sparky platform.
I know the current code doesn't provide accurate values in all cases. But in some cases it might. So why would you want to set it to 0 in all cases?
Also, we are talking about an extension of existing fields. With less than 256 cores, core_count2 must have the same value as core_count. Whether or not the value of the latter is accurate is an orthogonal issue.
Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for version 3.2 ......................................................................
Patch Set 5:
Patch Set 5:
(3 comments)
Patch Set 5:
(6 comments)
Patch Set 1:
(6 comments)
Patrick,
- Line 561: "_weak" so that it can be if needed for som platform/soc
- Line 829: I agree that the entry point should be added as as specified in SMBIOS 3.0. it would be more disruptive in this CL, as the change will also need to go in other other files where the table entry are used. I suggest having a later patch and review exclusively for the table entry update.
|> It's not disruptive. Just a few lines to add in this file. No need to update the callers. It is disruptive indeed. For example the new "anchor" field is 5 bytes whereas the current one is 4 bytes. The 5 bytes of "formated area" , the 5 bytes of "intermediate anchor string" and the 1 byte of "intermediate checksum" all need to go away in 3.0 and one "reserve" byte need to come in. Even if it was just about updating the anchor string alone, changing its size here and not on the callers side will not work. Furthermore that smbios_entry struct is defined and used in at least two other different filee, hence my previous suggestion.
|> Commit message: Line 10: I can't find it on Gerrit. The hash was requested. Here is the full CL path, it hasn't been merge yet: https://chromium-review.googlesource.com/c/chromiumos/platform/mosys/+/15427...
|> Commit message: Line 13: It does more than that. Yes, but that was just asummary of what the procedure does. If you have a better suggestion like Paul, I am happy to add it.
|>src/arch/x86/smbios.c: Line 279: Are you sure that all callers provide the value in MHz? They should. Per previous SMBIOS version specs, that speed have consistently been defined in MHZ. Even if other callers were mistakenly not providing it in MHz, then the fix most likely in other files, should be provided in other patches. Please sync up your comments with Nico's, he wants to reduce the number of changes in the patch. This cannot be achieved while fixing other stuffs as well.
Regards, Francois
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for version 3.2 ......................................................................
Patch Set 5:
Patch Set 5:
(3 comments)
Patch Set 5:
(6 comments)
Patch Set 1:
(6 comments)
Patrick,
- Line 561: "_weak" so that it can be if needed for som
platform/soc
- Line 829: I agree that the entry point should be added as as
specified in SMBIOS 3.0. it would be more disruptive in this CL, as the change will also need to go in other other files where the table entry are used.
There's one file making use of struct smbios_entry: src/arch/x86/smbios.c Of course you would add a second struct for the 64bit entry point and keep the 32bit one.
I suggest having a later patch and review exclusively for the
table entry update.
|> It's not disruptive. Just a few lines to add in this file. No need to update the callers. It is disruptive indeed. For example the new "anchor" field is 5 bytes whereas the current one is 4 bytes. The 5 bytes of "formated area" , the 5 bytes of "intermediate anchor string" and the 1 byte of "intermediate checksum" all need to go away in 3.0 and one "reserve" byte need to come in. Even if it was just about updating the anchor string alone, changing its size here and not on the callers side will not work. Furthermore that smbios_entry struct is defined and used in at least two other different filee, hence my previous suggestion.
|> Commit message: Line 10: I can't find it on Gerrit. The hash was requested. Here is the full CL path, it hasn't been merge yet: https://chromium-review.googlesource.com/c/chromiumos/platform/mosys/+/15427...
I don't see how chromium patches relate to coreboot.
|> Commit message: Line 13: It does more than that. Yes, but that was just asummary of what the procedure does. If you have a better suggestion like Paul, I am happy to add it.
|>src/arch/x86/smbios.c: Line 279: Are you sure that all callers provide the value in MHz? They should. Per previous SMBIOS version specs, that speed have consistently been defined in MHZ. Even if other callers were mistakenly not providing it in MHz, then the fix most likely in other files, should be provided in other patches. Please sync up your comments with Nico's, he wants to reduce the number of changes in the patch. This cannot be achieved while fixing other stuffs as well.
If the callers should provide the value in the correct unit the SMBIOS spec defines, you should update all callers instead.
Regards, Francois
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for version 3.2 ......................................................................
Patch Set 5:
Repost as gerrit messed up the message:
There's one file making use of struct smbios_entry: src/arch/x86/smbios.c Of course you would add a second struct for the 64bit entry point and keep the 32bit one.
If the callers should provide the value in the correct unit the SMBIOS spec defines, you should update all callers instead.
I don't see how chromium patches relate to coreboot and why you would mention them in the commit message.
Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for version 3.2 ......................................................................
Patch Set 5:
Patch Set 5:
Sorry for the revert. Please, split out the functional changes (e.g. the cpuid stuff around core/thread count) into their own patch (it's not related to the SMBIOS update, is it?). This change does a lot of things, and I haven't spotted anything yet that is *required* for SMBIOS 3.2.
Nico, From the 3.2 spec, core counts and thread counts don't seem to be optional. The newly added procedure is populating them to comply to one of your previous comment: " please don't add fields without giving them a reasonable value"
Well, core/thread count are not added by in this change, so my comment doesn't apply to them. I was mostly refer- ring to the added fields that were left untouched (at 0) where 0 isn't a valid value. Sorry for the confusion.
Still, you could fix the cpuid code first, then add SMBIOS 3.2 stuff. I don't see why it would have to be part of the same commit.
I can remove it from this patch if you require it and set the values to 0. Because the count generated by the code before this upgrade was not accurate, at least not on the GKL Sparky platform.
I know the current code doesn't provide accurate values in all cases. But in some cases it might. So why would you want to set it to 0 in all cases?
Also, we are talking about an extension of existing fields. With less than 256 cores, core_count2 must have the same value as core_count. Whether or not the value of the latter is accurate is an orthogonal issue.
Since the comment don't apply to the procedure then I am still confused.
|> Still, you could fix the cpuid code first, then add SMBIOS 3.2 stuff. I don't see why it would have to be part of the same commit. Removing the new core/tread procedure here and keeping everything else, along with the old core count in this commit would achieve the same goal, wouldn't it? If that is your request please confirm and I will do just that.
|> I was mostly referring to the added fields that were left untouched (at 0) where 0 isn't a valid value. Sorry for the confusion. There are 3 added field which are set to 0, and 0 indeed is a valid value for each of them. Please see these lines from the SMBIOS spec: " A value of 0000h indicates the Memory Subsystem Controller Product ID is unknown" " A value of 0000h indicates the Memory Subsystem Controller Manufacturer ID is unknown" " A value of 0000h indicates the Module Product ID is unknown" The FSP code have not yet been upgraded to 3.2 to popolate those memory fields appropriately. Hence their "unknown" 0 values.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for version 3.2 ......................................................................
Patch Set 5:
|> Still, you could fix the cpuid code first, then add SMBIOS 3.2 stuff. I don't see why it would have to be part of the same commit. Removing the new core/tread procedure here and keeping everything else, along with the old core count in this commit would achieve the same goal, wouldn't it? If that is your request please confirm and I will do just that.
You can do that. My only request is that you don't fix existing fields and call that required for the SMBIOS update.
|> I was mostly referring to the added fields that were left untouched (at 0) where 0 isn't a valid value. Sorry for the confusion. There are 3 added field which are set to 0, and 0 indeed is a valid value for each of them. Please see these lines from the SMBIOS spec: " A value of 0000h indicates the Memory Subsystem Controller Product ID is unknown" " A value of 0000h indicates the Memory Subsystem Controller Manufacturer ID is unknown" " A value of 0000h indicates the Module Product ID is unknown" The FSP code have not yet been upgraded to 3.2 to popolate those memory fields appropriately. Hence their "unknown" 0 values.
That's in the past, see above "I *was* mosly referring". You fixed that already, I know.
Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for version 3.2 ......................................................................
Patch Set 5:
Patch Set 5:
Repost as gerrit messed up the message:
There's one file making use of struct smbios_entry: src/arch/x86/smbios.c Of course you would add a second struct for the 64bit entry point and keep the 32bit one.
If the callers should provide the value in the correct unit the SMBIOS spec defines, you should update all callers instead.
I don't see how chromium patches relate to coreboot and why you would mention them in the commit message.
|> I don't see how chromium patches relate to coreboot Because these SMBIOS information are ultimatly consumed in chromium, more specifically in mosys, and smbios related changes are going there.
|> why you would mention them in the commit message. Please see the earlier request/comment from Paul.
|> There's one file making use of struct smbios_entry: src/arch/x86/smbios.c See my comment above. That information is consumed outside of coreboot and the smbios_entry struct is also defined at the caller's side and elsewhere: platform/mosys/include/lib/smbios.h platform/mosys/lib/smbios/smbios.c platform/mosys/include/lib/smbios_tables.h third_party/u-boot/files/include/smbios.h Those definitions have to be consistent, otherwise the system becomes unstable.
|> Of course you would add a second struct for the 64bit entry point and keep the 32bit one. |> If the callers should provide the value in the correct unit the SMBIOS spec defines, you should update all callers instead. Maybe I am missing something, but not sure I understand the 2 sentences above. If you have 2 formats of smbios_entry which one would you return? The caller's only passes one variable into which this function "concatenate" the fields info, and the caller retrieves the information using the same standard definition of smbios_entry. Why would you need 2 definitions? Are you asking to change all the functions through which this information passes through? And if so, how would you do that in this commit? Since those files are not include here. Even if that is your request it conflict's with Nico's who wants to reduce the amount of changes going in here. In short, having one definition of smbios_entry on one side of the system and a different one on the end, would break the whole flow. Please let me know if I am missing something
Hello Patrick Rudolph, Paul Menzel, build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32090
to look at the new patch set (#6).
Change subject: smbios: Add and adapt fields for version 3.2 ......................................................................
smbios: Add and adapt fields for version 3.2
This is the second of 2 patches upgrading the SMBIOS interface to 3.2. First patch "platform/mosys: This upgrades SMBIOS to latest version 3.2". First patch SHA: a1943f6e5aaa4fa38208ee43576882b54471797f. The added fields, in anticipation for future change, are part of the new 3.2 spec of SMBIOS. The new procedure populates the core and thread count(s).
BUG=NONE TEST=Boot to OS on GLK Sparky
Change-Id: I1171c20ac85a2231d949d4ac1c3e3c544ba3e5ef Signed-off-by: Francois Toguo francois.toguo.fotso@intel.com --- M src/arch/x86/smbios.c M src/include/smbios.h 2 files changed, 55 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/32090/6
Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for version 3.2 ......................................................................
Patch Set 6:
Patch Set 5:
|> Still, you could fix the cpuid code first, then add SMBIOS 3.2 stuff. I don't see why it would have to be part of the same commit. Removing the new core/tread procedure here and keeping everything else, along with the old core count in this commit would achieve the same goal, wouldn't it? If that is your request please confirm and I will do just that.
You can do that. My only request is that you don't fix existing fields and call that required for the SMBIOS update.
|> I was mostly referring to the added fields that were left untouched (at 0) where 0 isn't a valid value. Sorry for the confusion. There are 3 added field which are set to 0, and 0 indeed is a valid value for each of them. Please see these lines from the SMBIOS spec: " A value of 0000h indicates the Memory Subsystem Controller Product ID is unknown" " A value of 0000h indicates the Memory Subsystem Controller Manufacturer ID is unknown" " A value of 0000h indicates the Module Product ID is unknown" The FSP code have not yet been upgraded to 3.2 to popolate those memory fields appropriately. Hence their "unknown" 0 values.
That's in the past, see above "I *was* mosly referring". You fixed that already, I know.
Nico, The fix for the existing field have been removed. Please take a look.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for version 3.2 ......................................................................
Patch Set 6:
Patch Set 5:
Repost as gerrit messed up the message:
There's one file making use of struct smbios_entry:
src/arch/x86/smbios.c
Of course you would add a second struct for the 64bit entry point
and keep the 32bit one.
If the callers should provide the value in the correct unit the
SMBIOS spec defines, you should update all callers instead.
I don't see how chromium patches relate to coreboot and why you
would mention them in the commit message.
|> I don't see how chromium patches relate to coreboot Because these SMBIOS information are ultimatly consumed in chromium, more specifically in mosys, and smbios related changes are going there.
I don't see how it can be the second patch, if the first never will be in this repository.
|> why you would mention them in the commit message. Please see the earlier request/comment from Paul.
|> There's one file making use of struct smbios_entry: src/arch/x86/smbios.c See my comment above. That information is consumed outside of coreboot and the smbios_entry struct is also defined at the caller's side and elsewhere: platform/mosys/include/lib/smbios.h platform/mosys/lib/smbios/smbios.c platform/mosys/include/lib/smbios_tables.h third_party/u-boot/files/include/smbios.h Those definitions have to be consistent, otherwise the system becomes unstable.
I don't care about 3rdparty software and it's totally unrelated.
|> Of course you would add a second struct for the 64bit entry point and keep the 32bit one. |> If the callers should provide the value in the correct unit the SMBIOS spec defines, you should update all callers instead. Maybe I am missing something, but not sure I understand the 2 sentences above. If you have 2 formats of smbios_entry which one would you return? The caller's only passes one variable into which this function "concatenate" the fields info, and the caller retrieves the information using the same standard definition of smbios_entry. Why would you need 2 definitions?
One struct for each entrypoint. It's not returned, so it wouldn't matter for the caller. The whole SMBIOS table including the entry point is returned.
Are you asking to change all the functions through which this information passes through? And if so, how would you do that in this commit? Since those files are not include here. Even if that is your request it conflict's with Nico's who wants to reduce the amount of changes going in here. In short, having one definition of smbios_entry on one side of the system and a different one on the end, would break the whole flow. Please let me know if I am missing something
For the type 17 CLK units, you should provide the value in the correct unit the SMBIOS spec defines, as it was also the case before the update. That means you should update all providers of CBMEM_ID_MEMINFO.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for version 3.2 ......................................................................
Patch Set 6:
tl;dr; Please focus on a commit message that provides a reasoning for the change, what it gains us and the scope of the changes. I still don't see the incentive for all this.
Francois, I think you are experiencing a lot of conflicting feedback from reviewers here, because you leave too much room for interpretation. For instance I guess, Paul asked you to properly refer to the other change because he might have thought it was a patch to coreboot. But it can't work out, as you don't know yet what commit hash the other change will have.
Also, I don't see a reason to mention the change to mosys. The SMBIOS spec is the interface between coreboot and mosys, both have to comply to SMBIOS, period. Otherwise, you might break compatibility to other programs whilst adding conflic- ting dependencies.
If I tell you, you shouldn't do this or that in this change, I merely interpret the scope of this change that you set with the commit message. And I'm still unsure what that should be. It is very important to define the scope particularly in this case because SMBIOS allows many things to be optional. I can't say that I read the full document, but so far I don't see anything that would have to change for 3.2 compatibility. The 64-bit entry point is optional too, and I guess, again, Patrick wouldn't have asked for it if you had provided a clear line around what this change should comprise and what not.
So please, before making any more changes to the code, tell us exactly what you want to achieve and why. e.g.
* Add optional fields, as a programming excercise? * SMBIOS 3.2 compliance in coreboot, because? * SMBIOS 3.2 compliance plus optional fields, because? * Keep things in sync with mosys?
And regarding existing bugs (wrong core counts, clock vs. MTs etc.), if they make it harder to update the SMBIOS code, I suggest to fix them first.
Please understand that you create a lot of reviewing work with this -- a change that doesn't gain us anything at the moment (anticipation of future changes?). So this has pro- bably lowest priority for most of us.
Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for version 3.2 ......................................................................
Patch Set 6:
(1 comment)
Patch Set 6:
Patch Set 5:
Repost as gerrit messed up the message:
There's one file making use of struct smbios_entry:
src/arch/x86/smbios.c
Of course you would add a second struct for the 64bit entry point
and keep the 32bit one.
If the callers should provide the value in the correct unit the
SMBIOS spec defines, you should update all callers instead.
I don't see how chromium patches relate to coreboot and why you
would mention them in the commit message.
|> I don't see how chromium patches relate to coreboot Because these SMBIOS information are ultimatly consumed in chromium, more specifically in mosys, and smbios related changes are going there.
I don't see how it can be the second patch, if the first never will be in this repository.
|> why you would mention them in the commit message. Please see the earlier request/comment from Paul.
|> There's one file making use of struct smbios_entry: src/arch/x86/smbios.c See my comment above. That information is consumed outside of coreboot and the smbios_entry struct is also defined at the caller's side and elsewhere: platform/mosys/include/lib/smbios.h platform/mosys/lib/smbios/smbios.c platform/mosys/include/lib/smbios_tables.h third_party/u-boot/files/include/smbios.h Those definitions have to be consistent, otherwise the system becomes unstable.
I don't care about 3rdparty software and it's totally unrelated.
Nico seems to be in agreement that we shouldn't be breaking compatibility to other programs.
|> Of course you would add a second struct for the 64bit entry point and keep the 32bit one. |> If the callers should provide the value in the correct unit the SMBIOS spec defines, you should update all callers instead. Maybe I am missing something, but not sure I understand the 2 sentences above. If you have 2 formats of smbios_entry which one would you return? The caller's only passes one variable into which this function "concatenate" the fields info, and the caller retrieves the information using the same standard definition of smbios_entry. Why would you need 2 definitions?
One struct for each entrypoint. It's not returned, so it wouldn't matter for the caller. The whole SMBIOS table including the entry point is returned.
Correct, the whole SMBIOS table, including the entry point is returned. My question was: which one of the two formats for the entry will be included to the able to be returned. Because the caller expects one format, not two. That is why in order to maintain the compatibility, that Nico is also refering to, I suggested not making that change now in this same review.
Are you asking to change all the functions through which this information passes through? And if so, how would you do that in this commit? Since those files are not include here. Even if that is your request it conflict's with Nico's who wants to reduce the amount of changes going in here. In short, having one definition of smbios_entry on one side of the system and a different one on the end, would break the whole flow. Please let me know if I am missing something
For the type 17 CLK units, you should provide the value in the correct unit the SMBIOS spec defines, as it was also the case before the update. That means you should update all providers of CBMEM_ID_MEMINFO.
The changes already present in here will take care of the type 17 new clock units. Another alternative would be to remove the change here and push it down in the FSP codes, the ultimate provider of these information. Which will need to be done for all SOCs since the FSP varies per SOC. Requests to various FSP teams for furture SOCs to update their type17 info to 3.2 spec are already in the work. Past SOCs and GLK (current) for example won't be making the change. Hence the decision to implement it here.
https://review.coreboot.org/#/c/32090/1/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/#/c/32090/1/src/arch/x86/smbios.c@279 PS1, Line 279: t->clock_speed = dimm->ddr_frequency;
clock_speed and speed are now in units of MT/s instead of Mhz. […]
Done
Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for version 3.2 ......................................................................
Patch Set 6:
Patch Set 6:
tl;dr; Please focus on a commit message that provides a reasoning for the change, what it gains us and the scope of the changes. I still don't see the incentive for all this.
Francois, I think you are experiencing a lot of conflicting feedback from reviewers here, because you leave too much room for interpretation. For instance I guess, Paul asked you to properly refer to the other change because he might have thought it was a patch to coreboot. But it can't work out, as you don't know yet what commit hash the other change will have.
Also, I don't see a reason to mention the change to mosys. The SMBIOS spec is the interface between coreboot and mosys, both have to comply to SMBIOS, period. Otherwise, you might break compatibility to other programs whilst adding conflic- ting dependencies.
Reference to the mosys change in the commit message have been removed.
If I tell you, you shouldn't do this or that in this change, I merely interpret the scope of this change that you set with the commit message. And I'm still unsure what that should be. It is very important to define the scope particularly in this case because SMBIOS allows many things to be optional. I can't say that I read the full document, but so far I don't see anything that would have to change for 3.2 compatibility. The 64-bit entry point is optional too, and I guess, again, Patrick wouldn't have asked for it if you had provided a clear line around what this change should comprise and what not.
So please, before making any more changes to the code, tell us exactly what you want to achieve and why. e.g.
No more change(s) to the code is planned in this review.
- Add optional fields, as a programming excercise?
- SMBIOS 3.2 compliance in coreboot, because?
- SMBIOS 3.2 compliance plus optional fields, because?
- Keep things in sync with mosys?
A particular reason is project or test case related, we have some internal testing that will require to support SMBIOS 3.2 to make the test pass.
And regarding existing bugs (wrong core counts, clock vs. MTs etc.), if they make it harder to update the SMBIOS code, I suggest to fix them first. Please understand that you create a lot of reviewing work with this -- a change that doesn't gain us anything at the moment (anticipation of future changes?). So this has pro- bably lowest priority for most of us.
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for version 3.2 ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/#/c/32090/6/src/include/smbios.h File src/include/smbios.h:
https://review.coreboot.org/#/c/32090/6/src/include/smbios.h@466 PS6, Line 466: smbios_type17 { do src/include/memory_info.h needs some update?
Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for version 3.2 ......................................................................
Patch Set 6:
Patch Set 6:
(1 comment)
Yes it does, but not really required now. It can done in the next step. The dimm_info struct in there is more reflective of what is coming out of the FSP. No SOC's FSP currently provide the 3.2 new field. So this is more of a future TODO task, either before or after the FSP's dimm_info are update to 3.2.
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/+/32090 )
Change subject: smbios: Add and adapt fields for version 3.2 ......................................................................
smbios: Add and adapt fields for version 3.2
This is an upgrade to the SMBIOS interface to 3.2.
BUG=NONE TEST=Boot to OS on GLK Sparky
Change-Id: I1171c20ac85a2231d949d4ac1c3e3c544ba3e5ef Signed-off-by: Francois Toguo francois.toguo.fotso@intel.com --- M src/arch/x86/smbios.c M src/include/smbios.h 2 files changed, 55 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/32090/7
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/+/32090 )
Change subject: smbios: Add and adapt fields for version 3.2 ......................................................................
smbios: Add and adapt fields for version 3.2
This is an upgrade to the SMBIOS interface to 3.2.
BUG=NONE TEST=Boot to OS on GLK Sparky
Change-Id: I1171c20ac85a2231d949d4ac1c3e3c544ba3e5ef Signed-off-by: Francois Toguo francois.toguo.fotso@intel.com --- M src/arch/x86/smbios.c M src/include/smbios.h 2 files changed, 55 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/32090/8
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/+/32090 )
Change subject: smbios: Add and adapt fields for 32 bits SMBIOS to version 3.2 ......................................................................
smbios: Add and adapt fields for 32 bits SMBIOS to version 3.2
This is an upgrade to the SMBIOS 32 bits interface to the 3.2 specs Future work will upgrade the smbio_entry to the 64 bits format
BUG=NONE TEST=Boot to OS on GLK Sparky
Change-Id: I1171c20ac85a2231d949d4ac1c3e3c544ba3e5ef Signed-off-by: Francois Toguo francois.toguo.fotso@intel.com --- M src/arch/x86/smbios.c M src/include/smbios.h 2 files changed, 55 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/32090/9
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for 32 bits SMBIOS to version 3.2 ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/#/c/32090/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32090/9//COMMIT_MSG@10 PS9, Line 10: smbio_entry smbio*s*?
https://review.coreboot.org/#/c/32090/9/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/#/c/32090/9/src/arch/x86/smbios.c@815 PS9, Line 815: Please do not mix format changes in functional commits.
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/+/32090 )
Change subject: smbios: Add and adapt fields for 32 bits SMBIOS to version 3.2 ......................................................................
smbios: Add and adapt fields for 32 bits SMBIOS to version 3.2
This is an upgrade to the SMBIOS 32 bits interface to the 3.2 specs Future work will upgrade the smbios_entry to the 64 bits format
BUG=NONE TEST=Boot to OS on GLK Sparky
Change-Id: I1171c20ac85a2231d949d4ac1c3e3c544ba3e5ef Signed-off-by: Francois Toguo francois.toguo.fotso@intel.com --- M src/arch/x86/smbios.c M src/include/smbios.h 2 files changed, 55 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/32090/10
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/+/32090 )
Change subject: smbios: Add and adapt fields for 32 bits SMBIOS to version 3.2 ......................................................................
smbios: Add and adapt fields for 32 bits SMBIOS to version 3.2
This is an upgrade to the SMBIOS 32 bits interface to the 3.2 specs Future work will upgrade the smbios_entry to the 64 bits format
BUG=NONE TEST=Boot to OS on GLK Sparky
Change-Id: I1171c20ac85a2231d949d4ac1c3e3c544ba3e5ef Signed-off-by: Francois Toguo francois.toguo.fotso@intel.com --- M src/arch/x86/smbios.c M src/include/smbios.h 2 files changed, 54 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/32090/11
Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for 32 bits SMBIOS to version 3.2 ......................................................................
Patch Set 11:
(3 comments)
Patch Set 9:
(2 comments)
Paul,
Your comment have been implemented. "No builds executed", most likely because no code change was required.
Regards, Francois
https://review.coreboot.org/#/c/32090/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32090/9//COMMIT_MSG@10 PS9, Line 10: smbio_entry
smbio*s*?
Done
https://review.coreboot.org/#/c/32090/9/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/#/c/32090/9/src/arch/x86/smbios.c@815 PS9, Line 815:
Please do not mix format changes in functional commits.
Done
https://review.coreboot.org/#/c/32090/6/src/include/smbios.h File src/include/smbios.h:
https://review.coreboot.org/#/c/32090/6/src/include/smbios.h@466 PS6, Line 466: smbios_type17 {
do src/include/memory_info. […]
Ack
Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for 32 bits SMBIOS to version 3.2 ......................................................................
Patch Set 11:
(1 comment)
Gentlemen,
The latest comments to this review have been addressed. Would you please take a look?
Regards, Francois
Patch Set 11:
(3 comments)
Patch Set 9:
(2 comments)
Paul,
Your comment have been implemented. "No builds executed", most likely because no code change was required.
Regards, Francois
https://review.coreboot.org/#/c/32090/6/src/include/smbios.h File src/include/smbios.h:
https://review.coreboot.org/#/c/32090/6/src/include/smbios.h@466 PS6, Line 466: smbios_type17 {
Ack
Done
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for 32 bits SMBIOS to version 3.2 ......................................................................
Patch Set 11: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for 32 bits SMBIOS to version 3.2 ......................................................................
Patch Set 11:
(4 comments)
https://review.coreboot.org/#/c/32090/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32090/11//COMMIT_MSG@7 PS11, Line 7: 32 bits What is "32 bits SMBIOS"?
https://review.coreboot.org/#/c/32090/11//COMMIT_MSG@10 PS11, Line 10: Future work will upgrade the smbios_entry to the 64 bits format Please provide the reason for this change.
https://review.coreboot.org/#/c/32090/11/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/#/c/32090/11/src/arch/x86/smbios.c@280 PS11, Line 280: t->speed = 2 * dimm->ddr_frequency; Did somebody check *all* callers that they report the *clock* frequency?
https://review.coreboot.org/#/c/32090/11/src/arch/x86/smbios.c@804 PS11, Line 804: se->minor_version = 2; Beside increasing the version, this change seems to only add optional fields. Did anybody check if there are any mandatory changes before increasing the version?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for 32 bits SMBIOS to version 3.2 ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/#/c/32090/11/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/#/c/32090/11/src/arch/x86/smbios.c@804 PS11, Line 804: se->minor_version = 2;
Beside increasing the version, this change seems to only add […]
*if there are any mandatory changes to make
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/+/32090 )
Change subject: smbios: Add and adapt fields for SMBIOS update to version 3.2 ......................................................................
smbios: Add and adapt fields for SMBIOS update to version 3.2
This is an upgrade to the SMBIOS 32 bits interface to the 3.2 specs
BUG=NONE TEST=Boot to OS on GLK Sparky
Change-Id: I1171c20ac85a2231d949d4ac1c3e3c544ba3e5ef Signed-off-by: Francois Toguo francois.toguo.fotso@intel.com --- M src/arch/x86/smbios.c M src/include/smbios.h 2 files changed, 54 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/32090/12
Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for SMBIOS update to version 3.2 ......................................................................
Patch Set 12:
(2 comments)
Patch Set 11:
(4 comments)
Line 7:What is "32 bits SMBIOS"?
It was reference to the 32-bit entry point mention in the section 5.2.1 Comment is now removed.
Line 10: Please provide the reason for this change. This was a request from one of the reviewer, and he did not provide a reason for it. Comment removed as well.
https://review.coreboot.org/#/c/32090/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32090/11//COMMIT_MSG@7 PS11, Line 7: 32 bits
What is "32 bits SMBIOS"?
Done
https://review.coreboot.org/#/c/32090/11//COMMIT_MSG@10 PS11, Line 10: Future work will upgrade the smbios_entry to the 64 bits format
Please provide the reason for this change.
Done
Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for SMBIOS update to version 3.2 ......................................................................
Patch Set 12:
(2 comments)
Patch Set 11:
(4 comments)
Line 280: Did somebody check *all* callers that they report the *clock*
frequency? The direct caller, I found, that immediately uses this those values, is in mosys. It is reported there as clock frequency in MHZ. Now, why would any caller report it/them in anything other than clock frequency? They have always been specified in the SMBIOS specs as clock frequencies in MHZ.
Line 804: Beside increasing the version, this change seems to only add optional fields. Did anybody check if there are any mandatory changes before increasing the version?
I did not find any indication/specification that the added fields is/are optional.
https://review.coreboot.org/#/c/32090/11/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/#/c/32090/11/src/arch/x86/smbios.c@280 PS11, Line 280: t->speed = 2 * dimm->ddr_frequency;
Did somebody check *all* callers that they report the *clock* […]
Done
https://review.coreboot.org/#/c/32090/11/src/arch/x86/smbios.c@804 PS11, Line 804: se->minor_version = 2;
*if there are any mandatory changes to make
Done
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for SMBIOS update to version 3.2 ......................................................................
Patch Set 12: -Code-Review
Patch Set 12:
(2 comments)
Patch Set 11:
(4 comments)
Line 7:What is "32 bits SMBIOS"?
It was reference to the 32-bit entry point mention in the section 5.2.1 Comment is now removed.
Line 10: Please provide the reason for this change. This was a request from one of the reviewer, and he did not provide a reason for it. Comment removed as well.
Take back my +2 until Nico's comment get addressed.
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for SMBIOS update to version 3.2 ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/#/c/32090/12/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/#/c/32090/12/src/arch/x86/smbios.c@279 PS12, Line 279: clock_speed is it Configured Memory Speed ?
Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for SMBIOS update to version 3.2 ......................................................................
Patch Set 12:
(1 comment)
Patch Set 12:
(1 comment)
Correct, that is the configured memory speed, but now in megatransfers per second (MT/s).
https://review.coreboot.org/#/c/32090/5/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/#/c/32090/5/src/arch/x86/smbios.c@279 PS5, Line 279: ddr_frequency
Are you sure that all callers provide the value in MHz?
Done
Francois Toguo Fotso has removed Lijian Zhao from this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for SMBIOS update to version 3.2 ......................................................................
Removed reviewer Lijian Zhao.
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for SMBIOS update to version 3.2 ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/#/c/32090/12/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/#/c/32090/12/src/arch/x86/smbios.c@279 PS12, Line 279: clock_speed
is it Configured Memory Speed ?
Please, do we need to make some changes here "src/include/memory_info.h"? Thx
Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for SMBIOS update to version 3.2 ......................................................................
Patch Set 12:
(1 comment)
Patch Set 12:
(1 comment)
This question was addressed in a previous comments as follows:
The changes already present in here will take care of the type 17 new clock units. Another alternative would be to remove the change here and push it down in the FSP codes, the ultimate provider of these information. Which will need to be done for all SOCs since the FSP varies per SOC. .. Past SOCs and GLK (current) for example won't be making the change. Hence the decision to implement it here.
In short, with the x 2 multiplier, no change needed in src/include/memory_info.h
https://review.coreboot.org/#/c/32090/12/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/#/c/32090/12/src/arch/x86/smbios.c@279 PS12, Line 279: clock_speed
is it Configured Memory Speed ?
Done
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for SMBIOS update to version 3.2 ......................................................................
Patch Set 12: Code-Review+1
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for SMBIOS update to version 3.2 ......................................................................
Patch Set 12: Code-Review+1
it needs a manual rebase
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for SMBIOS update to version 3.2 ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32090/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/32090/11//COMMIT_MSG@10 PS11, Line 10: Future work will upgrade the smbios_entry to the 64 bits format
Done
It’d have been great if you added your replies directly to the comments.
In what review was this requested? Please elaborate in the commit message.
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for SMBIOS update to version 3.2 ......................................................................
Patch Set 12:
any update?
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for SMBIOS update to version 3.2 ......................................................................
Patch Set 12:
Hi Francois, thanks for this commit. Will you be able to rebase it? We are waiting for it to be merged. ;-) Otherwise if you are busy and if it is okay with you, we can rebase for you.
Thanks, Jonathan
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/32090?usp=email )
Change subject: smbios: Add and adapt fields for SMBIOS update to version 3.2 ......................................................................
Abandoned
This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.
-
Francois Toguo Fotso (Code Review) -
HAOUAS Elyes (Code Review)
-
Jonathan Zhang (Code Review)
-
Lijian Zhao (Code Review)
-
Martin L Roth (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)