Change in coreboot[master]: include/memory_info.h: Change serial number field from 5 bytes to 4

[Raul Rangel (Code Review)]

Raul Rangel has uploaded this change for review. ( https://review.coreboot.org/25343

Change subject: include/memory_info.h: Change serial number field from 5 bytes to 4
......................................................................

include/memory_info.h: Change serial number field from 5 bytes to 4

dimm_info.serial had a strange contract. The SPD spec defines a 4 byte
serial number. dimm_info.serial required a 4 character ascii string with
a null terminator.

This change makes the serial field so it matches the SPD spec.
smbios.c will then translate the byte array into hex and set it on the
smbios table.

There were only two callers that set the serial number:
* haswell/raminit.c: already does a memcpy(serial, spd->serial, 4), so
  it already matches the new contract.
* amd_late_init.c: Previously copied the last 4 characters. Requires
  decoding the serial number into a byte array.

google/cyan/spd/spd.c: This could be updated to pass the serial number,
but it uses a hard coded spd.bin.

Testing this on grunt, dmidecode now shows the full serial number:
        Serial Number: 00000000

BUG=b:65403853
TEST=tested on grunt

Change-Id: Ifc58ad9ea4cdd2abe06a170a39b1f32680e7b299
Signed-off-by: Raul E Rangel 
---
M src/arch/x86/smbios.c
A src/include/hexutil.h
M src/include/memory_info.h
M src/lib/Makefile.inc
A src/lib/hexutil.c
M src/soc/amd/common/block/pi/amd_late_init.c
6 files changed, 154 insertions(+), 33 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/25343/1

diff --git a/src/arch/x86/smbios.c b/src/arch/x86/smbios.c
index 7079374..21125c4 100644
--- a/src/arch/x86/smbios.c
+++ b/src/arch/x86/smbios.c
@@ -29,6 +29,7 @@
 #include <memory_info.h>
 #include <spd.h>
 #include <cbmem.h>
+#include <hexutil.h>
 #if IS_ENABLED(CONFIG_CHROMEOS)
 #include <vendorcode/google/chromeos/gnvs.h>
 #endif
@@ -251,6 +252,16 @@
    }
 }

+/* Encodes the SPD serial number into hex */
+static void smbios_fill_dimm_serial_number(const struct dimm_info *dimm,
+					   struct smbios_type17 *t)
+{
+	char serial[sizeof(dimm->serial) * 2 + 1];
+	encode_as_hex_string(dimm->serial, sizeof(dimm->serial), serial,
+			     sizeof(serial));
+	t->serial_number = smbios_add_string(t->eos, serial);
+}
+
 static int create_smbios_type17_for_dimm(struct dimm_info *dimm,
    				 unsigned long *current, int *handle)
 {
@@ -290,13 +301,7 @@
    }

smbios_fill_dimm_manufacturer_from_id(dimm->mod_id, t);
-	/* put '\0' in the end of data */
-	dimm->serial[DIMM_INFO_SERIAL_SIZE - 1] = '\0';
-	if (dimm->serial[0] == 0)
-		t->serial_number = smbios_add_string(t->eos, "None");
-	else
-		t->serial_number = smbios_add_string(t->eos,
-						   (const char *)dimm->serial);
+	smbios_fill_dimm_serial_number(dimm, t);

snprintf(locator, sizeof(locator), "Channel-%d-DIMM-%d",
    	dimm->channel_num, dimm->dimm_num);
diff --git a/src/include/hexutil.h b/src/include/hexutil.h
new file mode 100644
index 0000000..e57cd34
--- /dev/null
+++ b/src/include/hexutil.h
@@ -0,0 +1,53 @@
+/*
+ * This file is part of the coreboot project.
+ *
+ * Copyright 2018 Google Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef HEXUTIL_H
+#define HEXUTIL_H
+
+#include <stddef.h>
+#include <stdint.h>
+
+/**
+ * Decodes a hex character [0-9A-Za-z] into a byte.
+ *
+ * Invalid characters are mapped to 0.
+ * */
+uint8_t decode_hex_character(char hex);
+
+/**
+ * Decodes a hex string into a byte array.
+ *
+ * Examples:
+ *  * "01AF" => {0x01, 0xAF}
+ *  * "1AF" => {0x01, 0xAF}
+ *  * "1AF" => {0x00, 0x00, 0x01, 0xAF} w/ 4 byte dest buffer.
+ *
+ * @param src_size size of the src buffer
+ * @param dest_size must be at least (strlen(src) + 1) / 2 in size. If the
+ * buffer is bigger than required the most significant bytes are cleared.
+ */
+void decode_hex_string(const char *src, size_t src_size, uint8_t *dest,
+		       size_t dest_size);
+
+/**
+ * Encodes a byte array into a hex string.
+ *
+ * e.g., {0x01, 0x02} => "0102"
+ *
+ * @param dest_size must be at least (src_size * 2 + 1).
+ */
+void encode_as_hex_string(const uint8_t *src, size_t src_size, char *dest,
+			  size_t dest_size);
+#endif
diff --git a/src/include/memory_info.h b/src/include/memory_info.h
index 4613015..d26d789 100644
--- a/src/include/memory_info.h
+++ b/src/include/memory_info.h
@@ -19,7 +19,7 @@
 #include <stdint.h>
 #include <compiler.h>

-#define DIMM_INFO_SERIAL_SIZE		5
+#define DIMM_INFO_SERIAL_SIZE		4
 #define DIMM_INFO_PART_NUMBER_SIZE	19
 #define DIMM_INFO_TOTAL			8	/* Maximum num of dimm is 8 */

@@ -46,11 +46,7 @@
    uint8_t dimm_num;
    uint8_t bank_locator;
    /*
-	 * The last byte is '\0' for the end of string.
-	 *
-	 * Even though the SPD spec defines this field as a byte array the value
-	 * is passed directly to SMBIOS as a string, and thus must be printable
-	 * ASCII.
+	 * SPD serial number.
     */
    uint8_t serial[DIMM_INFO_SERIAL_SIZE];
    /*
diff --git a/src/lib/Makefile.inc b/src/lib/Makefile.inc
index 960b5cb..3ae77a6 100644
--- a/src/lib/Makefile.inc
+++ b/src/lib/Makefile.inc
@@ -133,6 +133,7 @@
 ramstage-$(CONFIG_GENERIC_GPIO_LIB) += gpio.c
 ramstage-$(CONFIG_GENERIC_UDELAY) += timer.c
 ramstage-y += b64_decode.c
+ramstage-y += hexutil.c
 ramstage-$(CONFIG_ACPI_NHLT) += nhlt.c

romstage-y += cbmem_common.c
diff --git a/src/lib/hexutil.c b/src/lib/hexutil.c
new file mode 100644
index 0000000..7c335a1
--- /dev/null
+++ b/src/lib/hexutil.c
@@ -0,0 +1,73 @@
+/*
+ * This file is part of the coreboot project.
+ *
+ * Copyright 2018 Google Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <hexutil.h>
+#include <string.h>
+
+uint8_t decode_hex_character(char hex)
+{
+	if (isdigit(hex))
+		return hex - '0';
+	else if (isxdigit(hex) && isupper(hex))
+		return hex - 'A' + 10;
+	else if (isxdigit(hex) && islower(hex))
+		return hex - 'a' + 10;
+	else
+		return 0;
+}
+
+void decode_hex_string(const char *src, size_t src_size, uint8_t *dest,
+		       size_t dest_size)
+{
+	const size_t src_len = strnlen(src, src_size - 1);
+
+	for (size_t i = 0; i < dest_size; ++i) {
+		int offset = (src_len - 1) - i * 2;
+
+		uint8_t byte = 0;
+
+		if (offset >= 0) {
+			char low = src[offset];
+			byte |= decode_hex_character(low);
+		}
+
+		if (offset >= 1) {
+			char high = src[offset - 1];
+			byte |= decode_hex_character(high) << 4;
+		}
+
+		dest[dest_size - i - 1] = byte;
+	}
+}
+
+void encode_as_hex_string(const uint8_t *src, size_t src_size, char *dest,
+			  size_t dest_size)
+{
+	for (size_t i = 0; i < src_size; ++i) {
+		size_t dest_offset = i * 2;
+
+		if (dest_offset < dest_size) {
+			if (dest_offset + 3 <= dest_size) {
+				snprintf(dest + dest_offset, 3, "%02hhx",
+					 src[i]);
+			} else if (dest_offset + 2 <= dest_size) {
+				snprintf(dest + dest_offset, 2, "%01hhx",
+					 src[i] >> 4);
+			} else {
+				dest[dest_offset] = '\0';
+			}
+		}
+	}
+}
diff --git a/src/soc/amd/common/block/pi/amd_late_init.c b/src/soc/amd/common/block/pi/amd_late_init.c
index 48c1718..6b09987 100644
--- a/src/soc/amd/common/block/pi/amd_late_init.c
+++ b/src/soc/amd/common/block/pi/amd_late_init.c
@@ -15,11 +15,12 @@

#include <arch/acpi.h>
 #include <bootstate.h>
+#include <cbmem.h>
 #include <console/console.h>
 #include <device/device.h>
 #include <device/pci_def.h>
 #include <device/pci_ops.h>
-#include <cbmem.h>
+#include <hexutil.h>
 #include <memory_info.h>

#include <amdblocks/agesawrapper.h>
@@ -27,20 +28,9 @@

static void transfer_memory_info(TYPE17_DMI_INFO *dmi17, struct dimm_info *dimm)
 {
-	size_t len, destlen;
-	uint32_t offset;
+	decode_hex_string(dmi17->SerialNumber, sizeof(dmi17->SerialNumber),
+			  dimm->serial, sizeof(dimm->serial));

-
-	len = strnlen(dmi17->SerialNumber, sizeof(dmi17->SerialNumber)) + 1;
-	destlen = sizeof(dimm->serial);
-
-	if (len > destlen) {
-		offset = len - destlen;
-		len = destlen;
-	} else
-		offset = 0;
-
-	strncpy((char *)dimm->serial, &dmi17->SerialNumber[offset], len);
    dimm->dimm_size = dmi17->ExtSize;
    dimm->ddr_type = dmi17->MemoryType;
    dimm->ddr_frequency = dmi17->Speed;
@@ -55,6 +45,10 @@

static void print_dimm_info(const struct dimm_info *dimm)
 {
+	char serial[sizeof(dimm->serial) * 2 + 1];
+	encode_as_hex_string(dimm->serial, sizeof(dimm->serial), serial,
+			     sizeof(serial));
+
    printk(RAM_SPEW,
           "CBMEM_ID_MEMINFO:\n"
           "  dimm_size: %u\n"
@@ -67,8 +61,8 @@
           "  mod_id: %hu\n"
           "  mod_type: 0x%hhx\n"
           "  bus_width: %hhu\n"
-	       "  serial(%zu): %s\n"
-	       "  module_part_number(%zu): %s\n",
+	       "  serial: 0x%s\n"
+	       "  module_part_number(%zu): '%s'\n",
           dimm->dimm_size,
           dimm->ddr_type,
           dimm->ddr_frequency,
@@ -79,8 +73,7 @@
           dimm->mod_id,
           dimm->mod_type,
           dimm->bus_width,
-	       strlen((char *) dimm->serial),
-	       (char *) dimm->serial,
+	       serial,
           strlen((char *) dimm->module_part_number),
           (char *) dimm->module_part_number
    );
@@ -104,8 +97,8 @@
           "  FormFactor: 0x%x\n"
           "  DeviceLocator: %8s\n"
           "  BankLocator: %10s\n"
-	       "  SerialNumber(%zu): %9s\n"
-	       "  PartNumber(%zu): %19s\n",
+	       "  SerialNumber(%zu): '%9s'\n"
+	       "  PartNumber(%zu): '%19s'\n",
           dmi17->Handle,
           dmi17->TotalWidth,
           dmi17->DataWidth,