[dmidecode] [PATCH] dmidecode: Extensions to Memory Device (Type 17)

[Jerry Hoemann]

The DSP0134 v3.2.0 extended the Memory Device (Type 17) structure
starting at offset 28h continuing to 4Ch to reflect persistent memory.

Signed-off-by: Jerry Hoemann <***@hpe.com>
---
dmidecode.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 113 insertions(+)

diff --git a/dmidecode.c b/dmidecode.c
index d18a258..77feee2 100644
--- a/dmidecode.c
+++ b/dmidecode.c
@@ -2499,6 +2499,84 @@ static void dmi_memory_device_type_detail(u16 code)
}
}

+static void dmi_memory_technology(u8 code)
+{
+ /* 7.18.6 */
+ static const char * const detail[] = {
+ "Other", /* 1 */
+ "Unknown",
+ "DRAM",
+ "NVDIMM-N",
+ "NVDIMM-F",
+ "NVDIMM-P",
+ "Intel persistent memory" /* 7 */
+ };
+ if (code && code <= 0x07)
+ printf(" %s", detail[code - 0x01]);
+ else
+ printf(" %s", out_of_spec);
+}
+
+static void dmi_memory_operating_mode_capility(u16 code)
+{
+ /* 7.18.7 */
+ static const char * const detail[] = {
+ "Other", /* 1 */
+ "Unknown",
+ "Volatile memory",
+ "Byte-accessible persistent memory",
+ "Block-accessible persistent memory" /* 5 */
+ };
+
+ if ((code & 0xFFFE) == 0)
+ printf(" None");
+ else {
+ int i;
+
+ for (i = 1; i <= 5; i++)
+ if (code & (1 << i))
+ printf(" %s", detail[i - 1]);
+ }
+}
+
+static void dmi_memory_manufacturer_id(u16 code)
+{
+ /* 7.18.8 */
+ /* 7.18.10 */
+ /* LSB is 7-bit Odd Parity number of continuation codes. */
+ if (code == 0)
+ printf(" Unknown");
+ else
+ printf(" Bank %d, Hex 0x%2X", (code & 0x7f) + 1, code >> 8);
+}
+
+static void dmi_memory_module_product_id(u16 code)
+{
+ /* 7.18.9 */
+ if (code == 0)
+ printf(" Unknown");
+ else
+ printf(" 0x%04X", code);
+}
+
+static void dmi_memory_subsystem_controller_product_id(u16 code)
+{
+ /* 7.18.11 */
+ if (code == 0)
+ printf(" Unknown");
+ else
+ printf(" 0x%04X", code);
+}
+
+static void dmi_memory_size(u64 code)
+{
+ if (code.h == 0xFFFFFFFF && code.l == 0xFFFFFFFF)
+ printf(" Unknown");
+ else
+ printf(" 0x%08X%08X", code.h, code.l);
+}
+
+
static void dmi_memory_device_speed(u16 code)
{
if (code == 0)
@@ -3907,6 +3985,41 @@ static void dmi_decode(const struct dmi_header *h, u16 ver)
printf("\tConfigured Voltage:");
dmi_memory_voltage_value(WORD(data + 0x26));
printf("\n");
+
+ if (h->length < 0x4c) break;
+ printf("\tMemory Technology:");
+ dmi_memory_technology(data[0x28]);
+ printf("\n");
+ printf("\tMemory Operating Mode Capability:");
+ dmi_memory_operating_mode_capility(WORD(data + 0x29));
+ printf("\n");
+ printf("\tFirmware Version: %s\n",
+ dmi_string(h, data[0x2B]));
+ printf("\tModule Manufacturer ID:");
+ dmi_memory_manufacturer_id(WORD(data + 0x2C));
+ printf("\n");
+ printf("\tModule Product ID:");
+ dmi_memory_module_product_id(WORD(data + 0x2E));
+ printf("\n");
+ printf("\tMemory Subsystem Controller Manufacturer ID:");
+ dmi_memory_manufacturer_id(WORD(data + 0x30));
+ printf("\n");
+ printf("\tMemory Subsystem Controller Product ID:");
+ dmi_memory_subsystem_controller_product_id(WORD(data + 0x32));
+ printf("\n");
+ printf("\tNon-Volatile Size:");
+ dmi_memory_size(QWORD(data + 0x34));
+ printf("\n");
+ printf("\tVolatile Size:");
+ dmi_memory_size(QWORD(data + 0x3C));
+ printf("\n");
+ printf("\tCache Size:");
+ dmi_memory_size(QWORD(data + 0x44));
+ printf("\n");
+ printf("\tLogical Size:");
+ dmi_memory_size(QWORD(data + 0x4C));
+ printf("\n");
+
break;

case 18: /* 7.19 32-bit Memory Error Information */

[Jerry Hoemann]

Post by Jerry Hoemann
The DSP0134 v3.2.0 extended the Memory Device (Type 17) structure
starting at offset 28h continuing to 4Ch to reflect persistent memory.
---
dmidecode.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 113 insertions(+)

Jean,

I recevied a couple off list comments that I'll share inline.

Post by Jerry Hoemann
diff --git a/dmidecode.c b/dmidecode.c
index d18a258..77feee2 100644
--- a/dmidecode.c
+++ b/dmidecode.c
@@ -2499,6 +2499,84 @@ static void dmi_memory_device_type_detail(u16 code)
}
}
+static void dmi_memory_technology(u8 code)
+{
+ /* 7.18.6 */
+ static const char * const detail[] = {
+ "Other", /* 1 */
+ "Unknown",
+ "DRAM",
+ "NVDIMM-N",
+ "NVDIMM-F",
+ "NVDIMM-P",
+ "Intel persistent memory" /* 7 */
+ };
+ if (code && code <= 0x07)
+ printf(" %s", detail[code - 0x01]);
+ else
+ printf(" %s", out_of_spec);
+}
+
+static void dmi_memory_operating_mode_capility(u16 code)
+{
+ /* 7.18.7 */
+ static const char * const detail[] = {
+ "Other", /* 1 */
+ "Unknown",
+ "Volatile memory",
+ "Byte-accessible persistent memory",
+ "Block-accessible persistent memory" /* 5 */
+ };
+
+ if ((code & 0xFFFE) == 0)
+ printf(" None");
+ else {
+ int i;
+
+ for (i = 1; i <= 5; i++)
+ if (code & (1 << i))
+ printf(" %s", detail[i - 1]);
+ }
+}
+
+static void dmi_memory_manufacturer_id(u16 code)
+{
+ /* 7.18.8 */
+ /* 7.18.10 */
+ /* LSB is 7-bit Odd Parity number of continuation codes. */
+ if (code == 0)
+ printf(" Unknown");
+ else
+ printf(" Bank %d, Hex 0x%2X", (code & 0x7f) + 1, code >> 8);
+}

Robert pointed me to

] Here's a GPLv2 open-source program that includes JEP106 manufacturer decoding:
] https://github.com/ntfreak/openocd
]
] see src/helper/jep106.c, .h, and .inc.


This could be adapted to print the manufacturer name in dmidecode.
There are quite a few IDs (~1200) and more keep getting added.

I was curious as to your thoughts in decoding the names.

Post by Jerry Hoemann
+
+static void dmi_memory_module_product_id(u16 code)
+{
+ /* 7.18.9 */
+ if (code == 0)
+ printf(" Unknown");
+ else
+ printf(" 0x%04X", code);
+}
+
+static void dmi_memory_subsystem_controller_product_id(u16 code)
+{
+ /* 7.18.11 */
+ if (code == 0)
+ printf(" Unknown");
+ else
+ printf(" 0x%04X", code);
+}
+
+static void dmi_memory_size(u64 code)
+{
+ if (code.h == 0xFFFFFFFF && code.l == 0xFFFFFFFF)
+ printf(" Unknown");
+ else
+ printf(" 0x%08X%08X", code.h, code.l);
+}

It was also suggested that the size in MiB or GiB be used
here in stead.

I'll go ahead and make this change and resend the patches.

Post by Jerry Hoemann
+
+
static void dmi_memory_device_speed(u16 code)
{
if (code == 0)
@@ -3907,6 +3985,41 @@ static void dmi_decode(const struct dmi_header *h, u16 ver)
printf("\tConfigured Voltage:");
dmi_memory_voltage_value(WORD(data + 0x26));
printf("\n");
+
+ if (h->length < 0x4c) break;
+ printf("\tMemory Technology:");
+ dmi_memory_technology(data[0x28]);
+ printf("\n");
+ printf("\tMemory Operating Mode Capability:");
+ dmi_memory_operating_mode_capility(WORD(data + 0x29));
+ printf("\n");
+ printf("\tFirmware Version: %s\n",
+ dmi_string(h, data[0x2B]));
+ printf("\tModule Manufacturer ID:");
+ dmi_memory_manufacturer_id(WORD(data + 0x2C));
+ printf("\n");
+ printf("\tModule Product ID:");
+ dmi_memory_module_product_id(WORD(data + 0x2E));
+ printf("\n");
+ printf("\tMemory Subsystem Controller Manufacturer ID:");
+ dmi_memory_manufacturer_id(WORD(data + 0x30));
+ printf("\n");
+ printf("\tMemory Subsystem Controller Product ID:");
+ dmi_memory_subsystem_controller_product_id(WORD(data + 0x32));
+ printf("\n");
+ printf("\tNon-Volatile Size:");
+ dmi_memory_size(QWORD(data + 0x34));
+ printf("\n");
+ printf("\tVolatile Size:");
+ dmi_memory_size(QWORD(data + 0x3C));
+ printf("\n");
+ printf("\tCache Size:");
+ dmi_memory_size(QWORD(data + 0x44));
+ printf("\n");
+ printf("\tLogical Size:");
+ dmi_memory_size(QWORD(data + 0x4C));
+ printf("\n");
+
break;
case 18: /* 7.19 32-bit Memory Error Information */
--
2.13.6

[Jean Delvare]

Hi Jerry,

Post by Jerry Hoemann

Post by Jerry Hoemann
+static void dmi_memory_manufacturer_id(u16 code)
+{
+ /* 7.18.8 */
+ /* 7.18.10 */
+ /* LSB is 7-bit Odd Parity number of continuation codes. */
+ if (code == 0)
+ printf(" Unknown");
+ else
+ printf(" Bank %d, Hex 0x%2X", (code & 0x7f) + 1, code >> 8);
+}

Robert pointed me to
] https://github.com/ntfreak/openocd
]
] see src/helper/jep106.c, .h, and .inc.
This could be adapted to print the manufacturer name in dmidecode.
There are quite a few IDs (~1200) and more keep getting added.
I was curious as to your thoughts in decoding the names.

I didn't know about openocd, but decode-dimms from i2c-tools project
also includes that list:

https://git.kernel.org/pub/scm/utils/i2c-tools/i2c-tools.git/tree/eeprom/decode-dimms#n55

And yes, it is large and constantly growing. I'm a bit reluctant to
duplicate it once more in dmidecode.

I believe it would make sense to have that list stored in a text file
under /usr/share, like we have /usr/share/pci.ids for PCI vendors and
devices. If something like that existed, then I would be happy to let
dmidecode try and read that file to optionally include the memory
vendor names in its output. I would also update decode-dimms to make
use of it.

Post by Jerry Hoemann

Post by Jerry Hoemann
(...)
+static void dmi_memory_size(u64 code)
+{
+ if (code.h == 0xFFFFFFFF && code.l == 0xFFFFFFFF)
+ printf(" Unknown");
+ else
+ printf(" 0x%08X%08X", code.h, code.l);
+}

It was also suggested that the size in MiB or GiB be used
here in stead.

Agreed, that will make the value a lot easier to read. We already do
something like that in dmi_print_memory_size(), which you may be able
to reuse or refactor somehow.

[Jerry Hoemann]

Post by Jean Delvare
Hi Jerry,

Post by Jerry Hoemann

Post by Jerry Hoemann
+static void dmi_memory_manufacturer_id(u16 code)
+{
+ /* 7.18.8 */
+ /* 7.18.10 */
+ /* LSB is 7-bit Odd Parity number of continuation codes. */
+ if (code == 0)
+ printf(" Unknown");
+ else
+ printf(" Bank %d, Hex 0x%2X", (code & 0x7f) + 1, code >> 8);
+}

Robert pointed me to
] https://github.com/ntfreak/openocd
]
] see src/helper/jep106.c, .h, and .inc.
This could be adapted to print the manufacturer name in dmidecode.
There are quite a few IDs (~1200) and more keep getting added.
I was curious as to your thoughts in decoding the names.

I didn't know about openocd, but decode-dimms from i2c-tools project
https://git.kernel.org/pub/scm/utils/i2c-tools/i2c-tools.git/tree/eeprom/decode-dimms#n55
And yes, it is large and constantly growing. I'm a bit reluctant to
duplicate it once more in dmidecode.
I believe it would make sense to have that list stored in a text file
under /usr/share, like we have /usr/share/pci.ids for PCI vendors and
devices. If something like that existed, then I would be happy to let
dmidecode try and read that file to optionally include the memory
vendor names in its output. I would also update decode-dimms to make
use of it.

For the purpose of this patch, I'll continue with the current
approach.

We can look into something along the lines of pci.ids in
a future patch.

Post by Jean Delvare

Post by Jerry Hoemann

Post by Jerry Hoemann
(...)
+static void dmi_memory_size(u64 code)
+{
+ if (code.h == 0xFFFFFFFF && code.l == 0xFFFFFFFF)
+ printf(" Unknown");
+ else
+ printf(" 0x%08X%08X", code.h, code.l);
+}

It was also suggested that the size in MiB or GiB be used
here in stead.

Agreed, that will make the value a lot easier to read. We already do
something like that in dmi_print_memory_size(), which you may be able
to reuse or refactor somehow.
--
Jean Delvare
SUSE L3 Support

[Jean Delvare]

Hi Jerry,

Post by Jerry Hoemann
The DSP0134 v3.2.0 extended the Memory Device (Type 17) structure
starting at offset 28h continuing to 4Ch to reflect persistent memory.

Do you have access to any system implementing SMBIOS 3.2.0 already? If
so, I would appreciate if you could send a dump of the DMI table to me
(using "dmidecode --dump-bin") so that I can add it to my
non-regression test suite.

Post by Jerry Hoemann
---
dmidecode.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 113 insertions(+)
diff --git a/dmidecode.c b/dmidecode.c
index d18a258..77feee2 100644
--- a/dmidecode.c
+++ b/dmidecode.c
@@ -2499,6 +2499,84 @@ static void dmi_memory_device_type_detail(u16 code)
}
}
+static void dmi_memory_technology(u8 code)
+{
+ /* 7.18.6 */
+ static const char * const detail[] = {

I suppose you copied the name "detail" from
dmi_memory_device_type_detail(). The array was named "detail" there
because of what it was representing, but that doesn't really make sense
in your new function. "technology" would be a better name.

Post by Jerry Hoemann
+ "Other", /* 1 */

For consistency with the rest of the code, comments should be
/* 0x01 */...

Post by Jerry Hoemann
+ "Unknown",
+ "DRAM",
+ "NVDIMM-N",
+ "NVDIMM-F",
+ "NVDIMM-P",
+ "Intel persistent memory" /* 7 */

... and /* 0x07 */.

Post by Jerry Hoemann
+ };
+ if (code && code <= 0x07)

Also for consistency the first test would be code >= 0x01 (even if the
result is obviously the same.)

Post by Jerry Hoemann
+ printf(" %s", detail[code - 0x01]);
+ else
+ printf(" %s", out_of_spec);
+}
+
+static void dmi_memory_operating_mode_capility(u16 code)

"capility", really? :-D

Post by Jerry Hoemann
+{
+ /* 7.18.7 */
+ static const char * const detail[] = {

I'd name it "mode".

Post by Jerry Hoemann
+ "Other", /* 1 */
+ "Unknown",
+ "Volatile memory",
+ "Byte-accessible persistent memory",
+ "Block-accessible persistent memory" /* 5 */
+ };
+
+ if ((code & 0xFFFE) == 0)
+ printf(" None");
+ else {
+ int i;
+
+ for (i = 1; i <= 5; i++)
+ if (code & (1 << i))
+ printf(" %s", detail[i - 1]);
+ }
+}
+
+static void dmi_memory_manufacturer_id(u16 code)
+{
+ /* 7.18.8 */
+ /* 7.18.10 */
+ /* LSB is 7-bit Odd Parity number of continuation codes. */
+ if (code == 0)
+ printf(" Unknown");
+ else
+ printf(" Bank %d, Hex 0x%2X", (code & 0x7f) + 1, code >> 8);
+}
+
+static void dmi_memory_module_product_id(u16 code)
+{
+ /* 7.18.9 */
+ if (code == 0)
+ printf(" Unknown");
+ else
+ printf(" 0x%04X", code);
+}
+
+static void dmi_memory_subsystem_controller_product_id(u16 code)
+{
+ /* 7.18.11 */
+ if (code == 0)
+ printf(" Unknown");
+ else
+ printf(" 0x%04X", code);
+}

The two functions above are exactly the same, so there should be only
one of them defined, and called twice. You could call it
"dmi_memory_product_id" to show that it is somewhat generic.

Post by Jerry Hoemann
+
+static void dmi_memory_size(u64 code)
+{
+ if (code.h == 0xFFFFFFFF && code.l == 0xFFFFFFFF)
+ printf(" Unknown");
+ else
+ printf(" 0x%08X%08X", code.h, code.l);
+}

Please re-use dmi_print_memory_size() instead.

Post by Jerry Hoemann
+
+

Extra blank line.

Post by Jerry Hoemann
static void dmi_memory_device_speed(u16 code)
{
if (code == 0)
@@ -3907,6 +3985,41 @@ static void dmi_decode(const struct dmi_header *h, u16 ver)
printf("\tConfigured Voltage:");
dmi_memory_voltage_value(WORD(data + 0x26));
printf("\n");
+
+ if (h->length < 0x4c) break;

For consistency, there should be no blank line here, and 0x4C should be
with uppercase C.

Also, the specification says that length must be at least 34h for
SMBIOS version 3.2.0 and later. This suggests that the fields after
that are optional, and may not be present in all implementations. So it
would be safer to check for h->length < 0x34 first, and add another
length check later in the code flow for the remaining fields.

I think your length check is wrong too... Last field *starts* at 0x4C
and is 8 byte long, so the length check for a full record should
be < 0x54.

Post by Jerry Hoemann
+ printf("\tMemory Technology:");
+ dmi_memory_technology(data[0x28]);
+ printf("\n");
+ printf("\tMemory Operating Mode Capability:");
+ dmi_memory_operating_mode_capility(WORD(data + 0x29));
+ printf("\n");
+ printf("\tFirmware Version: %s\n",
+ dmi_string(h, data[0x2B]));
+ printf("\tModule Manufacturer ID:");
+ dmi_memory_manufacturer_id(WORD(data + 0x2C));
+ printf("\n");
+ printf("\tModule Product ID:");
+ dmi_memory_module_product_id(WORD(data + 0x2E));
+ printf("\n");
+ printf("\tMemory Subsystem Controller Manufacturer ID:");
+ dmi_memory_manufacturer_id(WORD(data + 0x30));
+ printf("\n");
+ printf("\tMemory Subsystem Controller Product ID:");
+ dmi_memory_subsystem_controller_product_id(WORD(data + 0x32));
+ printf("\n");
+ printf("\tNon-Volatile Size:");
+ dmi_memory_size(QWORD(data + 0x34));
+ printf("\n");
+ printf("\tVolatile Size:");
+ dmi_memory_size(QWORD(data + 0x3C));
+ printf("\n");
+ printf("\tCache Size:");
+ dmi_memory_size(QWORD(data + 0x44));
+ printf("\n");
+ printf("\tLogical Size:");
+ dmi_memory_size(QWORD(data + 0x4C));
+ printf("\n");
+

No blank line here either.

Post by Jerry Hoemann
break;
case 18: /* 7.19 32-bit Memory Error Information */

Thanks,

[Jerry Hoemann]

Post by Jean Delvare
Hi Jerry,

Post by Jerry Hoemann
The DSP0134 v3.2.0 extended the Memory Device (Type 17) structure
starting at offset 28h continuing to 4Ch to reflect persistent memory.

Do you have access to any system implementing SMBIOS 3.2.0 already? If
so, I would appreciate if you could send a dump of the DMI table to me
(using "dmidecode --dump-bin") so that I can add it to my
non-regression test suite.

I'll look into what I can provide.

Post by Jean Delvare

Post by Jerry Hoemann
---
dmidecode.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 113 insertions(+)
diff --git a/dmidecode.c b/dmidecode.c
index d18a258..77feee2 100644
--- a/dmidecode.c
+++ b/dmidecode.c
@@ -2499,6 +2499,84 @@ static void dmi_memory_device_type_detail(u16 code)
}
}
+static void dmi_memory_technology(u8 code)
+{
+ /* 7.18.6 */
+ static const char * const detail[] = {

I suppose you copied the name "detail" from
dmi_memory_device_type_detail(). The array was named "detail" there
because of what it was representing, but that doesn't really make sense
in your new function. "technology" would be a better name.

Done.

Post by Jean Delvare

Post by Jerry Hoemann
+ "Other", /* 1 */

For consistency with the rest of the code, comments should be
/* 0x01 */...

Post by Jerry Hoemann
+ "Unknown",
+ "DRAM",
+ "NVDIMM-N",
+ "NVDIMM-F",
+ "NVDIMM-P",
+ "Intel persistent memory" /* 7 */

... and /* 0x07 */.

Done.

Post by Jean Delvare

Post by Jerry Hoemann
+ };
+ if (code && code <= 0x07)

Also for consistency the first test would be code >= 0x01 (even if the
result is obviously the same.)

Done.

Post by Jean Delvare

Post by Jerry Hoemann
+ printf(" %s", detail[code - 0x01]);
+ else
+ printf(" %s", out_of_spec);
+}
+
+static void dmi_memory_operating_mode_capility(u16 code)

"capility", really? :-D

Fixed.

Post by Jean Delvare

Post by Jerry Hoemann
+{
+ /* 7.18.7 */
+ static const char * const detail[] = {

I'd name it "mode".

Done.

Post by Jean Delvare

Post by Jerry Hoemann
+ "Other", /* 1 */
+ "Unknown",
+ "Volatile memory",
+ "Byte-accessible persistent memory",
+ "Block-accessible persistent memory" /* 5 */
+ };
+
+ if ((code & 0xFFFE) == 0)
+ printf(" None");
+ else {
+ int i;
+
+ for (i = 1; i <= 5; i++)
+ if (code & (1 << i))
+ printf(" %s", detail[i - 1]);
+ }
+}
+
+static void dmi_memory_manufacturer_id(u16 code)
+{
+ /* 7.18.8 */
+ /* 7.18.10 */
+ /* LSB is 7-bit Odd Parity number of continuation codes. */
+ if (code == 0)
+ printf(" Unknown");
+ else
+ printf(" Bank %d, Hex 0x%2X", (code & 0x7f) + 1, code >> 8);
+}
+
+static void dmi_memory_module_product_id(u16 code)
+{
+ /* 7.18.9 */
+ if (code == 0)
+ printf(" Unknown");
+ else
+ printf(" 0x%04X", code);
+}
+
+static void dmi_memory_subsystem_controller_product_id(u16 code)
+{
+ /* 7.18.11 */
+ if (code == 0)
+ printf(" Unknown");
+ else
+ printf(" 0x%04X", code);
+}

The two functions above are exactly the same, so there should be only
one of them defined, and called twice. You could call it
"dmi_memory_product_id" to show that it is somewhat generic.

Done.

Post by Jean Delvare

Post by Jerry Hoemann
+
+static void dmi_memory_size(u64 code)
+{
+ if (code.h == 0xFFFFFFFF && code.l == 0xFFFFFFFF)
+ printf(" Unknown");
+ else
+ printf(" 0x%08X%08X", code.h, code.l);
+}

Please re-use dmi_print_memory_size() instead.

Done.

Looking at spec again, fields 34h, 3Ch, and 44h, say if the value is 0,
there is no XXXX portion. This verbiage isn't present for field 4Ch.

I'm thinking that for 34h, 3Ch, 44h, we also special case when field is "0."
This would have then two functions, dmi_memory_size() for these three fields
and dmi_memory_logical_size() for 4Ch.

Post by Jean Delvare

Post by Jerry Hoemann
+
+

Extra blank line.

Done.

Post by Jean Delvare

Post by Jerry Hoemann
static void dmi_memory_device_speed(u16 code)
{
if (code == 0)
@@ -3907,6 +3985,41 @@ static void dmi_decode(const struct dmi_header *h, u16 ver)
printf("\tConfigured Voltage:");
dmi_memory_voltage_value(WORD(data + 0x26));
printf("\n");
+
+ if (h->length < 0x4c) break;

For consistency, there should be no blank line here, and 0x4C should be
with uppercase C.

Done

Post by Jean Delvare
Also, the specification says that length must be at least 34h for
SMBIOS version 3.2.0 and later. This suggests that the fields after
that are optional, and may not be present in all implementations. So it
would be safer to check for h->length < 0x34 first, and add another
length check later in the code flow for the remaining fields.

I missed this. I was thinking it was all or nothing on the new fields.

Fixed.

Post by Jean Delvare
I think your length check is wrong too... Last field *starts* at 0x4C
and is 8 byte long, so the length check for a full record should
be < 0x54.

You're correct. Fixed.

Next version will have appropriate length checks before printing
*each* of the new fields.

Post by Jean Delvare

Post by Jerry Hoemann
+ printf("\tMemory Technology:");
+ dmi_memory_technology(data[0x28]);
+ printf("\n");
+ printf("\tMemory Operating Mode Capability:");
+ dmi_memory_operating_mode_capility(WORD(data + 0x29));
+ printf("\n");
+ printf("\tFirmware Version: %s\n",
+ dmi_string(h, data[0x2B]));
+ printf("\tModule Manufacturer ID:");
+ dmi_memory_manufacturer_id(WORD(data + 0x2C));
+ printf("\n");
+ printf("\tModule Product ID:");
+ dmi_memory_module_product_id(WORD(data + 0x2E));
+ printf("\n");
+ printf("\tMemory Subsystem Controller Manufacturer ID:");
+ dmi_memory_manufacturer_id(WORD(data + 0x30));
+ printf("\n");
+ printf("\tMemory Subsystem Controller Product ID:");
+ dmi_memory_subsystem_controller_product_id(WORD(data + 0x32));
+ printf("\n");
+ printf("\tNon-Volatile Size:");
+ dmi_memory_size(QWORD(data + 0x34));
+ printf("\n");
+ printf("\tVolatile Size:");
+ dmi_memory_size(QWORD(data + 0x3C));
+ printf("\n");
+ printf("\tCache Size:");
+ dmi_memory_size(QWORD(data + 0x44));
+ printf("\n");
+ printf("\tLogical Size:");
+ dmi_memory_size(QWORD(data + 0x4C));
+ printf("\n");
+

No blank line here either.

Done.

Post by Jean Delvare

Post by Jerry Hoemann
break;
case 18: /* 7.19 32-bit Memory Error Information */

Thanks,
--
Jean Delvare
SUSE L3 Support

[Jean Delvare]

Post by Jerry Hoemann

Post by Jean Delvare

Post by Jerry Hoemann
+static void dmi_memory_size(u64 code)
+{
+ if (code.h == 0xFFFFFFFF && code.l == 0xFFFFFFFF)
+ printf(" Unknown");
+ else
+ printf(" 0x%08X%08X", code.h, code.l);
+}

Please re-use dmi_print_memory_size() instead.

Done.
Looking at spec again, fields 34h, 3Ch, and 44h, say if the value is 0,
there is no XXXX portion. This verbiage isn't present for field 4Ch.

True, but later in the specification, is mentioned: "When Memory Type
is not Logical, Logical Size shall be 0." So that field can be 0 as well.

Post by Jerry Hoemann
I'm thinking that for 34h, 3Ch, 44h, we also special case when field is "0."
This would have then two functions, dmi_memory_size() for these three fields
and dmi_memory_logical_size() for 4Ch.

Special case how? Printing (for example):

Volatile Size: None

or not printing that line at all? The latter has my preference.

As a side note, I really don't like this Logical Memory thing. The
specification fails to explain the purpose of that abstraction layer,
which then makes it hard to grasp the actual meaning of the related
bits and fields :-(

Post by Jerry Hoemann

Post by Jean Delvare
(...)
Also, the specification says that length must be at least 34h for
SMBIOS version 3.2.0 and later. This suggests that the fields after
that are optional, and may not be present in all implementations. So it
would be safer to check for h->length < 0x34 first, and add another
length check later in the code flow for the remaining fields.

I missed this. I was thinking it was all or nothing on the new fields.

That's almost always how things are implemented in practice, and also
in most cases the specification explicitly requests that all new fields
are present. But the fact that the minimum length mentioned in the
description of the Length field is less than the new fields is an
indication that it may not be the case this time. So I would check both.

Post by Jerry Hoemann

Post by Jean Delvare
I think your length check is wrong too... Last field *starts* at 0x4C
and is 8 byte long, so the length check for a full record should
be < 0x54.

You're correct. Fixed.
Next version will have appropriate length checks before printing
*each* of the new fields.

Not fundamentally incorrect but overkill in my humble opinion. The
current code has boundary checks which essentially match the fields
grouped by the specification version in which they were added. I have a
script crawling my collection of DMI table dumps and verifying that
none of them has a record length that the code does not expect, so I
believe we are on the safe side. I think it caught only 4 unexpected
record lengths in 15 years.

[Jerry Hoemann]

Post by Jean Delvare

Post by Jerry Hoemann

Post by Jean Delvare

Post by Jerry Hoemann
+static void dmi_memory_size(u64 code)
+{
+ if (code.h == 0xFFFFFFFF && code.l == 0xFFFFFFFF)
+ printf(" Unknown");
+ else
+ printf(" 0x%08X%08X", code.h, code.l);
+}

Please re-use dmi_print_memory_size() instead.

Done.
Looking at spec again, fields 34h, 3Ch, and 44h, say if the value is 0,
there is no XXXX portion. This verbiage isn't present for field 4Ch.

True, but later in the specification, is mentioned: "When Memory Type
is not Logical, Logical Size shall be 0." So that field can be 0 as well.

Post by Jerry Hoemann
I'm thinking that for 34h, 3Ch, 44h, we also special case when field is "0."
This would have then two functions, dmi_memory_size() for these three fields
and dmi_memory_logical_size() for 4Ch.

Volatile Size: None
or not printing that line at all? The latter has my preference.

I'm printing "None" but leaving it in if the field is specified by FW.
If FW doesn't specify the field, it won't appear.

Post by Jean Delvare
As a side note, I really don't like this Logical Memory thing. The
specification fails to explain the purpose of that abstraction layer,
which then makes it hard to grasp the actual meaning of the related
bits and fields :-(

Post by Jerry Hoemann

Post by Jean Delvare
(...)
Also, the specification says that length must be at least 34h for
SMBIOS version 3.2.0 and later. This suggests that the fields after
that are optional, and may not be present in all implementations. So it
would be safer to check for h->length < 0x34 first, and add another
length check later in the code flow for the remaining fields.

I missed this. I was thinking it was all or nothing on the new fields.

That's almost always how things are implemented in practice, and also
in most cases the specification explicitly requests that all new fields
are present. But the fact that the minimum length mentioned in the
description of the Length field is less than the new fields is an
indication that it may not be the case this time. So I would check both.

Post by Jerry Hoemann

Post by Jean Delvare
I think your length check is wrong too... Last field *starts* at 0x4C
and is 8 byte long, so the length check for a full record should
be < 0x54.

You're correct. Fixed.
Next version will have appropriate length checks before printing
*each* of the new fields.

Not fundamentally incorrect but overkill in my humble opinion. The
current code has boundary checks which essentially match the fields
grouped by the specification version in which they were added. I have a
script crawling my collection of DMI table dumps and verifying that
none of them has a record length that the code does not expect, so I
believe we are on the safe side. I think it caught only 4 unexpected
record lengths in 15 years.

I'm checking each field once we're *beyond* the check for the group upto 34h.
Like you said, the spec would allow for FW to not have all the fields
beyond 34h. Better safe than sorry.