Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37695 )
Change subject: acpi: Be more ACPI compliant when generating _UID ......................................................................
acpi: Be more ACPI compliant when generating _UID
Use the ACPI path of devices to calculate a unique _UID that doesn't change accross reboots. Add a CRC32 generator and add a function to write the _UID based on a device's ACPI path.
Change-Id: I47cd5396060d325f9ce338afced6af021e7ff2b4 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/arch/x86/acpi_device.c M src/arch/x86/include/arch/acpi_device.h M src/drivers/crb/tis.c M src/drivers/net/r8168.c M src/drivers/pc80/tpm/tis.c M src/drivers/wifi/generic.c M src/lib/Makefile.inc A src/lib/crc32.c M src/superio/common/ssdt.c 9 files changed, 193 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/37695/1
diff --git a/src/arch/x86/acpi_device.c b/src/arch/x86/acpi_device.c index 5c77953..a496832 100644 --- a/src/arch/x86/acpi_device.c +++ b/src/arch/x86/acpi_device.c @@ -18,6 +18,8 @@ #include <device/device.h> #include <device/path.h> #include <stdlib.h> +#include <crc32.h> + #if CONFIG(GENERIC_GPIO_LIB) #include <gpio.h> #endif @@ -98,6 +100,16 @@ return NULL; }
+/* Generate unique ID bases on ACPI path. Collisions on the same _HID are possible but unlikely */ +uint32_t acpi_device_uid(struct device *dev) +{ + const char *path = acpi_device_path(dev); + if (!path) + return 0; + + return xcrc32 ((unsigned char *)path, strlen(path), ~0); +} + /* Recursive function to find the root device and print a path from there */ static ssize_t acpi_device_path_fill(const struct device *dev, char *buf, size_t buf_len, size_t cur) @@ -192,6 +204,13 @@ return ACPI_STATUS_DEVICE_ALL_ON; }
+ +/* Write the unique _UID based on ACPI device path. */ +void acpi_device_write_uid(struct device *dev) +{ + acpigen_write_name_integer("_UID", acpi_device_uid(dev)); +} + /* ACPI 6.1 section 6.4.3.6: Extended Interrupt Descriptor */ void acpi_device_write_interrupt(const struct acpi_irq *irq) { diff --git a/src/arch/x86/include/arch/acpi_device.h b/src/arch/x86/include/arch/acpi_device.h index 382ef15..0a702c9 100644 --- a/src/arch/x86/include/arch/acpi_device.h +++ b/src/arch/x86/include/arch/acpi_device.h @@ -62,10 +62,12 @@ struct device; const char *acpi_device_name(const struct device *dev); const char *acpi_device_hid(const struct device *dev); +uint32_t acpi_device_uid(struct device *dev); const char *acpi_device_path(const struct device *dev); const char *acpi_device_scope(const struct device *dev); const char *acpi_device_path_join(const struct device *dev, const char *name); int acpi_device_status(const struct device *dev); +void acpi_device_write_uid(struct device *dev);
/* * ACPI Descriptor for extended Interrupt() diff --git a/src/drivers/crb/tis.c b/src/drivers/crb/tis.c index f2aba48..b1fbad0 100644 --- a/src/drivers/crb/tis.c +++ b/src/drivers/crb/tis.c @@ -117,7 +117,7 @@ acpigen_write_name_string("_HID", "MSFT0101"); acpigen_write_name_string("_CID", "MSFT0101");
- acpigen_write_name_integer("_UID", 1); + acpi_device_write_uid(dev);
acpigen_write_STA(ACPI_STATUS_DEVICE_ALL_ON);
diff --git a/src/drivers/net/r8168.c b/src/drivers/net/r8168.c index bc0132f..1bca879 100644 --- a/src/drivers/net/r8168.c +++ b/src/drivers/net/r8168.c @@ -312,7 +312,8 @@ acpigen_write_scope(path); acpigen_write_device(acpi_device_name(dev)); acpigen_write_name_string("_HID", R8168_ACPI_HID); - acpigen_write_name_integer("_UID", 0); + acpi_device_write_uid(dev); + if (dev->chip_ops) acpigen_write_name_string("_DDN", dev->chip_ops->name);
diff --git a/src/drivers/pc80/tpm/tis.c b/src/drivers/pc80/tpm/tis.c index 39fa70d..24944ea 100644 --- a/src/drivers/pc80/tpm/tis.c +++ b/src/drivers/pc80/tpm/tis.c @@ -902,7 +902,7 @@ acpigen_write_name("_CID"); acpigen_emit_eisaid("PNP0C31");
- acpigen_write_name_integer("_UID", 1); + acpi_device_write_uid(dev);
u32 did_vid = tpm_read_did_vid(0); if (did_vid > 0 && did_vid < 0xffffffff) diff --git a/src/drivers/wifi/generic.c b/src/drivers/wifi/generic.c index 9f9f4fa..fe2e39d 100644 --- a/src/drivers/wifi/generic.c +++ b/src/drivers/wifi/generic.c @@ -187,7 +187,8 @@ /* Device */ acpigen_write_scope(path); acpigen_write_device(acpi_device_name(dev)); - acpigen_write_name_integer("_UID", 0); + acpi_device_write_uid(dev); + if (dev->chip_ops) acpigen_write_name_string("_DDN", dev->chip_ops->name);
diff --git a/src/lib/Makefile.inc b/src/lib/Makefile.inc index b444ea3..4a3289d 100644 --- a/src/lib/Makefile.inc +++ b/src/lib/Makefile.inc @@ -133,6 +133,7 @@ postcar-$(CONFIG_TRACE) += trace.c ramstage-$(CONFIG_COLLECT_TIMESTAMPS) += timestamp.c ramstage-$(CONFIG_COVERAGE) += libgcov.c +ramstage-y += crc32.c ramstage-y += edid.c ifneq ($(CONFIG_NO_EDID_FILL_FB),y) ramstage-y += edid_fill_fb.c diff --git a/src/lib/crc32.c b/src/lib/crc32.c new file mode 100644 index 0000000..7c2c61d --- /dev/null +++ b/src/lib/crc32.c @@ -0,0 +1,163 @@ +/* + * Copyright (C) 2009-2019 Free Software Foundation, Inc. + * + * This file is part of the libiberty library. + * + * 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 <stdint.h> +#include <crc32.h> + +/* This table was generated by the following program. + + #include <stdio.h> + + int + main () + { + unsigned int i, j; + unsigned int c; + int table[256]; + + for (i = 0; i < 256; i++) + { + for (c = i << 24, j = 8; j > 0; --j) + c = c & 0x80000000 ? (c << 1) ^ 0x04c11db7 : (c << 1); + table[i] = c; + } + + printf ("static const unsigned int crc32_table[] =\n{\n"); + for (i = 0; i < 256; i += 4) + { + printf (" 0x%08x, 0x%08x, 0x%08x, 0x%08x", + table[i + 0], table[i + 1], table[i + 2], table[i + 3]); + if (i + 4 < 256) + putchar (','); + putchar ('\n'); + } + printf ("};\n"); + return 0; + } + + For more information on CRC, see, e.g., + http://www.ross.net/crc/download/crc_v3.txt. */ + +static const unsigned int crc32_table[] = +{ + 0x00000000, 0x04c11db7, 0x09823b6e, 0x0d4326d9, + 0x130476dc, 0x17c56b6b, 0x1a864db2, 0x1e475005, + 0x2608edb8, 0x22c9f00f, 0x2f8ad6d6, 0x2b4bcb61, + 0x350c9b64, 0x31cd86d3, 0x3c8ea00a, 0x384fbdbd, + 0x4c11db70, 0x48d0c6c7, 0x4593e01e, 0x4152fda9, + 0x5f15adac, 0x5bd4b01b, 0x569796c2, 0x52568b75, + 0x6a1936c8, 0x6ed82b7f, 0x639b0da6, 0x675a1011, + 0x791d4014, 0x7ddc5da3, 0x709f7b7a, 0x745e66cd, + 0x9823b6e0, 0x9ce2ab57, 0x91a18d8e, 0x95609039, + 0x8b27c03c, 0x8fe6dd8b, 0x82a5fb52, 0x8664e6e5, + 0xbe2b5b58, 0xbaea46ef, 0xb7a96036, 0xb3687d81, + 0xad2f2d84, 0xa9ee3033, 0xa4ad16ea, 0xa06c0b5d, + 0xd4326d90, 0xd0f37027, 0xddb056fe, 0xd9714b49, + 0xc7361b4c, 0xc3f706fb, 0xceb42022, 0xca753d95, + 0xf23a8028, 0xf6fb9d9f, 0xfbb8bb46, 0xff79a6f1, + 0xe13ef6f4, 0xe5ffeb43, 0xe8bccd9a, 0xec7dd02d, + 0x34867077, 0x30476dc0, 0x3d044b19, 0x39c556ae, + 0x278206ab, 0x23431b1c, 0x2e003dc5, 0x2ac12072, + 0x128e9dcf, 0x164f8078, 0x1b0ca6a1, 0x1fcdbb16, + 0x018aeb13, 0x054bf6a4, 0x0808d07d, 0x0cc9cdca, + 0x7897ab07, 0x7c56b6b0, 0x71159069, 0x75d48dde, + 0x6b93dddb, 0x6f52c06c, 0x6211e6b5, 0x66d0fb02, + 0x5e9f46bf, 0x5a5e5b08, 0x571d7dd1, 0x53dc6066, + 0x4d9b3063, 0x495a2dd4, 0x44190b0d, 0x40d816ba, + 0xaca5c697, 0xa864db20, 0xa527fdf9, 0xa1e6e04e, + 0xbfa1b04b, 0xbb60adfc, 0xb6238b25, 0xb2e29692, + 0x8aad2b2f, 0x8e6c3698, 0x832f1041, 0x87ee0df6, + 0x99a95df3, 0x9d684044, 0x902b669d, 0x94ea7b2a, + 0xe0b41de7, 0xe4750050, 0xe9362689, 0xedf73b3e, + 0xf3b06b3b, 0xf771768c, 0xfa325055, 0xfef34de2, + 0xc6bcf05f, 0xc27dede8, 0xcf3ecb31, 0xcbffd686, + 0xd5b88683, 0xd1799b34, 0xdc3abded, 0xd8fba05a, + 0x690ce0ee, 0x6dcdfd59, 0x608edb80, 0x644fc637, + 0x7a089632, 0x7ec98b85, 0x738aad5c, 0x774bb0eb, + 0x4f040d56, 0x4bc510e1, 0x46863638, 0x42472b8f, + 0x5c007b8a, 0x58c1663d, 0x558240e4, 0x51435d53, + 0x251d3b9e, 0x21dc2629, 0x2c9f00f0, 0x285e1d47, + 0x36194d42, 0x32d850f5, 0x3f9b762c, 0x3b5a6b9b, + 0x0315d626, 0x07d4cb91, 0x0a97ed48, 0x0e56f0ff, + 0x1011a0fa, 0x14d0bd4d, 0x19939b94, 0x1d528623, + 0xf12f560e, 0xf5ee4bb9, 0xf8ad6d60, 0xfc6c70d7, + 0xe22b20d2, 0xe6ea3d65, 0xeba91bbc, 0xef68060b, + 0xd727bbb6, 0xd3e6a601, 0xdea580d8, 0xda649d6f, + 0xc423cd6a, 0xc0e2d0dd, 0xcda1f604, 0xc960ebb3, + 0xbd3e8d7e, 0xb9ff90c9, 0xb4bcb610, 0xb07daba7, + 0xae3afba2, 0xaafbe615, 0xa7b8c0cc, 0xa379dd7b, + 0x9b3660c6, 0x9ff77d71, 0x92b45ba8, 0x9675461f, + 0x8832161a, 0x8cf30bad, 0x81b02d74, 0x857130c3, + 0x5d8a9099, 0x594b8d2e, 0x5408abf7, 0x50c9b640, + 0x4e8ee645, 0x4a4ffbf2, 0x470cdd2b, 0x43cdc09c, + 0x7b827d21, 0x7f436096, 0x7200464f, 0x76c15bf8, + 0x68860bfd, 0x6c47164a, 0x61043093, 0x65c52d24, + 0x119b4be9, 0x155a565e, 0x18197087, 0x1cd86d30, + 0x029f3d35, 0x065e2082, 0x0b1d065b, 0x0fdc1bec, + 0x3793a651, 0x3352bbe6, 0x3e119d3f, 0x3ad08088, + 0x2497d08d, 0x2056cd3a, 0x2d15ebe3, 0x29d4f654, + 0xc5a92679, 0xc1683bce, 0xcc2b1d17, 0xc8ea00a0, + 0xd6ad50a5, 0xd26c4d12, 0xdf2f6bcb, 0xdbee767c, + 0xe3a1cbc1, 0xe760d676, 0xea23f0af, 0xeee2ed18, + 0xf0a5bd1d, 0xf464a0aa, 0xf9278673, 0xfde69bc4, + 0x89b8fd09, 0x8d79e0be, 0x803ac667, 0x84fbdbd0, + 0x9abc8bd5, 0x9e7d9662, 0x933eb0bb, 0x97ffad0c, + 0xafb010b1, 0xab710d06, 0xa6322bdf, 0xa2f33668, + 0xbcb4666d, 0xb8757bda, 0xb5365d03, 0xb1f740b4 +}; + +/* + +@deftypefn Extension {unsigned int} crc32 (const unsigned char *@var{buf}, @ + int @var{len}, unsigned int @var{init}) + +Compute the 32-bit CRC of @var{buf} which has length @var{len}. The +starting value is @var{init}; this may be used to compute the CRC of +data split across multiple buffers by passing the return value of each +call as the @var{init} parameter of the next. + +This is used by the @command{gdb} remote protocol for the @samp{qCRC} +command. In order to get the same results as gdb for a block of data, +you must pass the first CRC parameter as @code{0xffffffff}. + +This CRC can be specified as: + + Width : 32 + Poly : 0x04c11db7 + Init : parameter, typically 0xffffffff + RefIn : false + RefOut : false + XorOut : 0 + +This differs from the "standard" CRC-32 algorithm in that the values +are not reflected, and there is no final XOR value. These differences +make it easy to compose the values of multiple blocks. + +@end deftypefn + +*/ + +unsigned int +xcrc32 (const unsigned char *buf, int len, unsigned int init) +{ + unsigned int crc = init; + while (len--) + { + crc = (crc << 8) ^ crc32_table[((crc >> 24) ^ *buf) & 255]; + buf++; + } + return crc; +} + diff --git a/src/superio/common/ssdt.c b/src/superio/common/ssdt.c index a919aa5..6912140 100644 --- a/src/superio/common/ssdt.c +++ b/src/superio/common/ssdt.c @@ -196,7 +196,8 @@ /* Device */ acpigen_write_device(name);
- acpigen_write_name_byte("_UID", 0); + acpi_device_write_uid(dev); + acpigen_write_name_byte("LDN", ldn); acpigen_write_name_byte("VLDN", vldn);
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37695 )
Change subject: acpi: Be more ACPI compliant when generating _UID ......................................................................
Patch Set 1:
(5 comments)
https://review.coreboot.org/c/coreboot/+/37695/1/src/arch/x86/acpi_device.c File src/arch/x86/acpi_device.c:
https://review.coreboot.org/c/coreboot/+/37695/1/src/arch/x86/acpi_device.c@... PS1, Line 103: /* Generate unique ID bases on ACPI path. Collisions on the same _HID are possible but unlikely */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/37695/1/src/arch/x86/acpi_device.c@... PS1, Line 110: return xcrc32 ((unsigned char *)path, strlen(path), ~0); space prohibited between function name and open parenthesis '('
https://review.coreboot.org/c/coreboot/+/37695/1/src/lib/crc32.c File src/lib/crc32.c:
https://review.coreboot.org/c/coreboot/+/37695/1/src/lib/crc32.c@54 PS1, Line 54: { that open brace { should be on the previous line
https://review.coreboot.org/c/coreboot/+/37695/1/src/lib/crc32.c@153 PS1, Line 153: xcrc32 (const unsigned char *buf, int len, unsigned int init) space prohibited between function name and open parenthesis '('
https://review.coreboot.org/c/coreboot/+/37695/1/src/lib/crc32.c@156 PS1, Line 156: while (len--) that open brace { should be on the previous line
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37695 )
Change subject: acpi: Be more ACPI compliant when generating _UID ......................................................................
Patch Set 3:
This change is ready for review.
Hello Stef van Os, Felix Held, wouter.eckhardt@prodrive-technologies.com, Philipp Deppenwiese, Guido Beyer @ Prodrive Technologies, build bot (Jenkins), Justin van Son, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37695
to look at the new patch set (#4).
Change subject: acpi: Be more ACPI compliant when generating _UID ......................................................................
acpi: Be more ACPI compliant when generating _UID
* Add a CRC32 generator * Add function to generate unique _UID using CRC32 * Add function to write the _UID based on a device's ACPI path
ACPI devices that have the same _HID must use different _UID. Linux doesn't care about _UID if it's not used. Windows 10 verifies the ACPI code on boot and BSODs if two devices with the same _HID share the same _UID.
Fixes BSOD seen on Windows 10.
Change-Id: I47cd5396060d325f9ce338afced6af021e7ff2b4 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/arch/x86/acpi_device.c M src/arch/x86/include/arch/acpi_device.h M src/drivers/crb/tis.c M src/drivers/net/r8168.c M src/drivers/pc80/tpm/tis.c M src/drivers/wifi/generic.c M src/include/crc_byte.h M src/lib/crc_byte.c M src/superio/common/ssdt.c 9 files changed, 121 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/37695/4
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37695 )
Change subject: acpi: Be more ACPI compliant when generating _UID ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/37695/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37695/4//COMMIT_MSG@9 PS4, Line 9: * Add a CRC32 generator Maybe split this out into a separate patch?
https://review.coreboot.org/c/coreboot/+/37695/4/src/include/crc_byte.h File src/include/crc_byte.h:
https://review.coreboot.org/c/coreboot/+/37695/4/src/include/crc_byte.h@40 PS4, Line 40: * 0x04c11db7. Please use the same notation as the other functions above.
https://review.coreboot.org/c/coreboot/+/37695/4/src/include/crc_byte.h@47 PS4, Line 47: uint32_t crc32_byte(const uint8_t *buf, size_t len, const uint32_t init); Let's stick to a single signature for all CRC functions (i.e. make this byte-at-a-time like the two above). If you want something to easily CRC a whole buffer, how about adding a macro helper for that:
#define CRC(buf, size, crc_func) do { \ const uint8_t *_crc_local_buf = (const uint8_t *)buf; \ size_t _crc_local_size = size; \ __typeof__(crc_func(0, 0)) _crc_local_result = 0; \ while (_crc_local_size--) { \ _crc_local_result = crc_func(_crc_local_result, *_crc_local_buf++); \ } \ }
https://review.coreboot.org/c/coreboot/+/37695/4/src/lib/crc_byte.c File src/lib/crc_byte.c:
https://review.coreboot.org/c/coreboot/+/37695/4/src/lib/crc_byte.c@107 PS4, Line 107: 0xbcb4666d, 0xb8757bda, 0xb5365d03, 0xb1f740b4 Table-based CRC implementations are usually not worth it for firmware (we don't calculate enough of these to warrant the extra code size). Can you please rewrite this to a bit-by-bit implementation like the two CRCs above?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37695 )
Change subject: acpi: Be more ACPI compliant when generating _UID ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37695/4/src/include/crc_byte.h File src/include/crc_byte.h:
https://review.coreboot.org/c/coreboot/+/37695/4/src/include/crc_byte.h@47 PS4, Line 47: uint32_t crc32_byte(const uint8_t *buf, size_t len, const uint32_t init);
Let's stick to a single signature for all CRC functions (i.e. […]
While I agree that the function signatures should be consistent, I would rather not see that macro in coreboot. A function would make more sense, though.
Patrick Rudolph has uploaded a new patch set (#5) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/37695 )
Change subject: acpi: Be more ACPI compliant when generating _UID ......................................................................
acpi: Be more ACPI compliant when generating _UID
* Add function to generate unique _UID using CRC32 * Add function to write the _UID based on a device's ACPI path
ACPI devices that have the same _HID must use different _UID. Linux doesn't care about _UID if it's not used. Windows 10 verifies the ACPI code on boot and BSODs if two devices with the same _HID share the same _UID.
Fixes BSOD seen on Windows 10.
Change-Id: I47cd5396060d325f9ce338afced6af021e7ff2b4 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/arch/x86/acpi_device.c M src/arch/x86/include/arch/acpi_device.h M src/drivers/crb/tis.c M src/drivers/net/r8168.c M src/drivers/pc80/tpm/tis.c M src/drivers/wifi/generic.c M src/superio/common/ssdt.c 7 files changed, 32 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/37695/5
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37695 )
Change subject: acpi: Be more ACPI compliant when generating _UID ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/c/coreboot/+/37695/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37695/4//COMMIT_MSG@9 PS4, Line 9: * Add a CRC32 generator
Maybe split this out into a separate patch?
done in CB:37753
https://review.coreboot.org/c/coreboot/+/37695/4/src/include/crc_byte.h File src/include/crc_byte.h:
https://review.coreboot.org/c/coreboot/+/37695/4/src/include/crc_byte.h@40 PS4, Line 40: * 0x04c11db7.
Please use the same notation as the other functions above.
done in CB:37753
https://review.coreboot.org/c/coreboot/+/37695/4/src/include/crc_byte.h@47 PS4, Line 47: uint32_t crc32_byte(const uint8_t *buf, size_t len, const uint32_t init);
While I agree that the function signatures should be consistent, I would rather not see that macro i […]
done in CB:37753
https://review.coreboot.org/c/coreboot/+/37695/4/src/lib/crc_byte.c File src/lib/crc_byte.c:
https://review.coreboot.org/c/coreboot/+/37695/4/src/lib/crc_byte.c@107 PS4, Line 107: 0xbcb4666d, 0xb8757bda, 0xb5365d03, 0xb1f740b4
Table-based CRC implementations are usually not worth it for firmware (we don't calculate enough of […]
done in CB:37753
Christian Walter has uploaded a new patch set (#6) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/37695 )
Change subject: acpi: Be more ACPI compliant when generating _UID ......................................................................
acpi: Be more ACPI compliant when generating _UID
Use the ACPI path of devices to calculate a unique _UID that doesn't change accross reboots. Add a CRC32 generator and add a function to write the _UID based on a device's ACPI path.
Change-Id: I47cd5396060d325f9ce338afced6af021e7ff2b4 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/arch/x86/acpi_device.c M src/arch/x86/include/arch/acpi_device.h M src/drivers/crb/tis.c M src/drivers/net/r8168.c M src/drivers/pc80/tpm/tis.c M src/drivers/wifi/generic.c A src/include/crc32.h M src/lib/Makefile.inc A src/lib/crc32.c M src/superio/common/ssdt.c 10 files changed, 213 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/37695/6
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37695 )
Change subject: acpi: Be more ACPI compliant when generating _UID ......................................................................
Patch Set 6:
(5 comments)
https://review.coreboot.org/c/coreboot/+/37695/6/src/arch/x86/acpi_device.c File src/arch/x86/acpi_device.c:
https://review.coreboot.org/c/coreboot/+/37695/6/src/arch/x86/acpi_device.c@... PS6, Line 103: /* Generate unique ID bases on ACPI path. Collisions on the same _HID are possible but unlikely */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/37695/6/src/arch/x86/acpi_device.c@... PS6, Line 110: return xcrc32 ((unsigned char *)path, strlen(path), ~0); space prohibited between function name and open parenthesis '('
https://review.coreboot.org/c/coreboot/+/37695/6/src/include/crc32.h File src/include/crc32.h:
https://review.coreboot.org/c/coreboot/+/37695/6/src/include/crc32.h@20 PS6, Line 20: xcrc32 (const unsigned char *buf, int len, unsigned int init); space prohibited between function name and open parenthesis '('
https://review.coreboot.org/c/coreboot/+/37695/6/src/lib/crc32.c File src/lib/crc32.c:
https://review.coreboot.org/c/coreboot/+/37695/6/src/lib/crc32.c@54 PS6, Line 54: { that open brace { should be on the previous line
https://review.coreboot.org/c/coreboot/+/37695/6/src/lib/crc32.c@153 PS6, Line 153: xcrc32 (const unsigned char *buf, int len, unsigned int init) space prohibited between function name and open parenthesis '('
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37695 )
Change subject: acpi: Be more ACPI compliant when generating _UID ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37695/4/src/include/crc_byte.h File src/include/crc_byte.h:
https://review.coreboot.org/c/coreboot/+/37695/4/src/include/crc_byte.h@47 PS4, Line 47: uint32_t crc32_byte(const uint8_t *buf, size_t len, const uint32_t init);
done in CB:37753
Note that the point of suggesting a macro was that the types could work out right for different CRC lengths. You can't do that with a function. Of course, we could instead have individual wrapper functions for each CRC type if you prefer. (If the CRC32 needs a different initialization value than the others -- the 0 vs ~0 thing -- then maybe that's the better option anyway.)
Hello Patrick Rudolph, Stef van Os, Felix Held, Christian Walter, wouter.eckhardt@prodrive-technologies.com, Philipp Deppenwiese, build bot (Jenkins), Guido Beyer @ Prodrive Technologies, Justin van Son, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37695
to look at the new patch set (#7).
Change subject: acpi: Be more ACPI compliant when generating _UID ......................................................................
acpi: Be more ACPI compliant when generating _UID
* Add function to generate unique _UID using CRC32 * Add function to write the _UID based on a device's ACPI path
ACPI devices that have the same _HID must use different _UID. Linux doesn't care about _UID if it's not used. Windows 10 verifies the ACPI code on boot and BSODs if two devices with the same _HID share the same _UID.
Fixes BSOD seen on Windows 10.
Change-Id: I47cd5396060d325f9ce338afced6af021e7ff2b4 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/arch/x86/acpi_device.c M src/arch/x86/include/arch/acpi_device.h M src/drivers/crb/tis.c M src/drivers/net/r8168.c M src/drivers/pc80/tpm/tis.c M src/drivers/wifi/generic.c M src/superio/common/ssdt.c 7 files changed, 32 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/37695/7
Hello Patrick Rudolph, Stef van Os, Felix Held, Christian Walter, wouter.eckhardt@prodrive-technologies.com, Philipp Deppenwiese, build bot (Jenkins), Guido Beyer @ Prodrive Technologies, Justin van Son, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37695
to look at the new patch set (#11).
Change subject: acpi: Be more ACPI compliant when generating _UID ......................................................................
acpi: Be more ACPI compliant when generating _UID
* Add function to generate unique _UID using CRC32 * Add function to write the _UID based on a device's ACPI path
ACPI devices that have the same _HID must use different _UID. Linux doesn't care about _UID if it's not used. Windows 10 verifies the ACPI code on boot and BSODs if two devices with the same _HID share the same _UID.
Fixes BSOD seen on Windows 10.
Change-Id: I47cd5396060d325f9ce338afced6af021e7ff2b4 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M Documentation/acpi/index.md A Documentation/acpi/uid.md M src/arch/x86/acpi_device.c M src/arch/x86/include/arch/acpi_device.h M src/drivers/crb/tis.c M src/drivers/net/r8168.c M src/drivers/pc80/tpm/tis.c M src/drivers/wifi/generic.c M src/superio/common/ssdt.c 9 files changed, 48 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/37695/11
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37695 )
Change subject: acpi: Be more ACPI compliant when generating _UID ......................................................................
Patch Set 11:
Added documentation as well.
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37695 )
Change subject: acpi: Be more ACPI compliant when generating _UID ......................................................................
Patch Set 11: Code-Review+2
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37695 )
Change subject: acpi: Be more ACPI compliant when generating _UID ......................................................................
acpi: Be more ACPI compliant when generating _UID
* Add function to generate unique _UID using CRC32 * Add function to write the _UID based on a device's ACPI path
ACPI devices that have the same _HID must use different _UID. Linux doesn't care about _UID if it's not used. Windows 10 verifies the ACPI code on boot and BSODs if two devices with the same _HID share the same _UID.
Fixes BSOD seen on Windows 10.
Change-Id: I47cd5396060d325f9ce338afced6af021e7ff2b4 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/37695 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M Documentation/acpi/index.md A Documentation/acpi/uid.md M src/arch/x86/acpi_device.c M src/arch/x86/include/arch/acpi_device.h M src/drivers/crb/tis.c M src/drivers/net/r8168.c M src/drivers/pc80/tpm/tis.c M src/drivers/wifi/generic.c M src/superio/common/ssdt.c 9 files changed, 48 insertions(+), 5 deletions(-)
Approvals: build bot (Jenkins): Verified Philipp Deppenwiese: Looks good to me, approved
diff --git a/Documentation/acpi/index.md b/Documentation/acpi/index.md index 8add8db..c378722 100644 --- a/Documentation/acpi/index.md +++ b/Documentation/acpi/index.md @@ -2,6 +2,8 @@
This section contains documentation about coreboot on ACPI.
+- [SSDT UID generation](uid.md) + ## GPIO
- [GPIO toggling in ACPI AML](gpio.md) diff --git a/Documentation/acpi/uid.md b/Documentation/acpi/uid.md new file mode 100644 index 0000000..0354bc5 --- /dev/null +++ b/Documentation/acpi/uid.md @@ -0,0 +1,14 @@ +# ACPI SSDT _UID generation + +According to the ACPI spec: + +> The _UID must be unique across all devices with either a common _HID or _CID. + + +When generating SSDTs in coreboot the independent drivers don't know +which _UID is already in use for a specific _HID or _CID. To generate +unique _UIDs the ACPI device's path is hashed and used as ID. As every ACPI +device has a different path, the hash will be also different for every device. + +Windows 10 verifies all devices with the same _HID or _CID and makes +sure that no _UID is duplicated. If it is it'll BSOD. diff --git a/src/arch/x86/acpi_device.c b/src/arch/x86/acpi_device.c index 5c77953..1d79170 100644 --- a/src/arch/x86/acpi_device.c +++ b/src/arch/x86/acpi_device.c @@ -18,6 +18,8 @@ #include <device/device.h> #include <device/path.h> #include <stdlib.h> +#include <crc_byte.h> + #if CONFIG(GENERIC_GPIO_LIB) #include <gpio.h> #endif @@ -98,6 +100,19 @@ return NULL; }
+/* + * Generate unique ID based on the ACPI path. + * Collisions on the same _HID are possible but very unlikely. + */ +uint32_t acpi_device_uid(struct device *dev) +{ + const char *path = acpi_device_path(dev); + if (!path) + return 0; + + return CRC(path, strlen(path), crc32_byte); +} + /* Recursive function to find the root device and print a path from there */ static ssize_t acpi_device_path_fill(const struct device *dev, char *buf, size_t buf_len, size_t cur) @@ -192,6 +207,13 @@ return ACPI_STATUS_DEVICE_ALL_ON; }
+ +/* Write the unique _UID based on ACPI device path. */ +void acpi_device_write_uid(struct device *dev) +{ + acpigen_write_name_integer("_UID", acpi_device_uid(dev)); +} + /* ACPI 6.1 section 6.4.3.6: Extended Interrupt Descriptor */ void acpi_device_write_interrupt(const struct acpi_irq *irq) { diff --git a/src/arch/x86/include/arch/acpi_device.h b/src/arch/x86/include/arch/acpi_device.h index 382ef15..0a702c9 100644 --- a/src/arch/x86/include/arch/acpi_device.h +++ b/src/arch/x86/include/arch/acpi_device.h @@ -62,10 +62,12 @@ struct device; const char *acpi_device_name(const struct device *dev); const char *acpi_device_hid(const struct device *dev); +uint32_t acpi_device_uid(struct device *dev); const char *acpi_device_path(const struct device *dev); const char *acpi_device_scope(const struct device *dev); const char *acpi_device_path_join(const struct device *dev, const char *name); int acpi_device_status(const struct device *dev); +void acpi_device_write_uid(struct device *dev);
/* * ACPI Descriptor for extended Interrupt() diff --git a/src/drivers/crb/tis.c b/src/drivers/crb/tis.c index f2aba48..b1fbad0 100644 --- a/src/drivers/crb/tis.c +++ b/src/drivers/crb/tis.c @@ -117,7 +117,7 @@ acpigen_write_name_string("_HID", "MSFT0101"); acpigen_write_name_string("_CID", "MSFT0101");
- acpigen_write_name_integer("_UID", 1); + acpi_device_write_uid(dev);
acpigen_write_STA(ACPI_STATUS_DEVICE_ALL_ON);
diff --git a/src/drivers/net/r8168.c b/src/drivers/net/r8168.c index bc0132f..1bca879 100644 --- a/src/drivers/net/r8168.c +++ b/src/drivers/net/r8168.c @@ -312,7 +312,8 @@ acpigen_write_scope(path); acpigen_write_device(acpi_device_name(dev)); acpigen_write_name_string("_HID", R8168_ACPI_HID); - acpigen_write_name_integer("_UID", 0); + acpi_device_write_uid(dev); + if (dev->chip_ops) acpigen_write_name_string("_DDN", dev->chip_ops->name);
diff --git a/src/drivers/pc80/tpm/tis.c b/src/drivers/pc80/tpm/tis.c index e9f1485..a35ef83 100644 --- a/src/drivers/pc80/tpm/tis.c +++ b/src/drivers/pc80/tpm/tis.c @@ -902,7 +902,7 @@ acpigen_write_name("_CID"); acpigen_emit_eisaid("PNP0C31");
- acpigen_write_name_integer("_UID", 1); + acpi_device_write_uid(dev);
u32 did_vid = tpm_read_did_vid(0); if (did_vid > 0 && did_vid < 0xffffffff) diff --git a/src/drivers/wifi/generic.c b/src/drivers/wifi/generic.c index 9f9f4fa..fe2e39d 100644 --- a/src/drivers/wifi/generic.c +++ b/src/drivers/wifi/generic.c @@ -187,7 +187,8 @@ /* Device */ acpigen_write_scope(path); acpigen_write_device(acpi_device_name(dev)); - acpigen_write_name_integer("_UID", 0); + acpi_device_write_uid(dev); + if (dev->chip_ops) acpigen_write_name_string("_DDN", dev->chip_ops->name);
diff --git a/src/superio/common/ssdt.c b/src/superio/common/ssdt.c index 41cafdf..541fa99 100644 --- a/src/superio/common/ssdt.c +++ b/src/superio/common/ssdt.c @@ -196,7 +196,8 @@ /* Device */ acpigen_write_device(name);
- acpigen_write_name_byte("_UID", 0); + acpi_device_write_uid(dev); + acpigen_write_name_byte("LDN", ldn); acpigen_write_name_byte("VLDN", vldn);