Duncan Laurie has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42047 )
Change subject: acpi: Accomodate non-standard UUIDs in device properties ......................................................................
acpi: Accomodate non-standard UUIDs in device properties
There have been changes to the way device properties are supported in Linux[1] and Windows[2] which add flexibilty:
- non-standard UUIDs can be used instead of only ACPI_DP_UUID - support for multiple different packages within the _DSD that associate different properties with unique UUIDs.
To handle this I extracted the part that does the write of UUID and properties to a separate function and defined a new PACKAGE type which has the custom UUID as a name and can be used the same way that child properties are today.
For example a PCIe root port for a USB4 port has a standard property indicating the USB4 reference, and then two custom properties which are defined for different attributes.
Example code:
/* Create property table */ acpi_dp *dsd = acpi_dp_new_table("_DSD"); acpi_dp_add_reference(dsd, "usb4-port", usb4_path);
/* Add package for hotplug */ acpi_dp *pkg = acpi_dp_new_table("6211e2c0-58a3-4af3-90e1-927a4e0c55a4"); acpi_dp_add_integer(pkg, "HotPlugSupportInD3", 1); acpi_dp_add_package(dsd, pkg);
/* Add package for external port info */ pkg = acpi_dp_new_table("efcc06cc-73ac-4bc3-bff0-76143807c389"); acpi_dp_add_integer(pkg, "ExternalFacingPort", 1); acpi_dp_add_package(dsd, pkg);
/* Write all properties */ acpi_dp_write(dsd);
Resulting ACPI:
Scope (_SB.PCI0.TRP0) { Name (_DSD, Package () { ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") Package () { Package (0x02) { "usb4-port", _SB.PCI0.TDM0.RHUB.PRTA } },
ToUUID ("6211e2c0-58a3-4af3-90e1-927a4e0c55a4"), Package () { Package () { "HotPlugSupportInD3", One } },
ToUUID ("efcc06cc-73ac-4bc3-bff0-76143807c389"), Package () { Package () { "ExternalFacingPort", One }, } }) }
[1] https://patchwork.kernel.org/patch/10599675/ [2] https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-r...
Change-Id: I75f47825bf4ffc5e9e92af2c45790d1b5945576e Signed-off-by: Duncan Laurie dlaurie@google.com --- M src/acpi/device.c M src/include/acpi/acpi_device.h 2 files changed, 77 insertions(+), 31 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/42047/1
diff --git a/src/acpi/device.c b/src/acpi/device.c index 8b03bfd..9e8efa4 100644 --- a/src/acpi/device.c +++ b/src/acpi/device.c @@ -53,7 +53,7 @@ return NULL;
/* Check for device specific handler */ - if (dev->ops->acpi_name) + if (dev->ops && dev->ops->acpi_name) return dev->ops->acpi_name(dev);
/* Walk up the tree to find if any parent can identify this device */ @@ -658,10 +658,48 @@ } }
+static bool acpi_dp_write_properties(struct acpi_dp *prop, const char *uuid) +{ + struct acpi_dp *dp; + char *prop_count = NULL; + + /* Print base properties */ + for (dp = prop; dp; dp = dp->next) { + if (dp->type == ACPI_DP_TYPE_TABLE || + dp->type == ACPI_DP_TYPE_CHILD || + dp->type == ACPI_DP_TYPE_PACKAGE) + continue; + + /* + * The UUID and package is only added when + * we come across the first property. This + * is to avoid creating a zero-length package + * in situations where there are only children. + */ + if (!prop_count) { + /* ToUUID (dp->uuid) */ + acpigen_write_uuid(uuid); + /* + * Package (PROP), element count determined as + * it is populated + */ + prop_count = acpigen_write_package(0); + } + (*prop_count)++; + acpi_dp_write_property(dp); + } + if (prop_count) { + /* Package (PROP) length, if a package was written */ + acpigen_pop_len(); + return true; + } + return false; +} + void acpi_dp_write(struct acpi_dp *table) { struct acpi_dp *dp, *prop; - char *dp_count, *prop_count = NULL; + char *dp_count; int child_count = 0;
if (!table || table->type != ACPI_DP_TYPE_TABLE || !table->next) @@ -677,37 +715,17 @@ dp_count = acpigen_write_package(0);
/* Print base properties */ - for (dp = prop; dp; dp = dp->next) { - if (dp->type == ACPI_DP_TYPE_CHILD) { - child_count++; - } else { - /* - * The UUID and package is only added when - * we come across the first property. This - * is to avoid creating a zero-length package - * in situations where there are only children. - */ - if (!prop_count) { - *dp_count += 2; - /* ToUUID (ACPI_DP_UUID) */ - acpigen_write_uuid(ACPI_DP_UUID); - /* - * Package (PROP), element count determined as - * it is populated - */ - prop_count = acpigen_write_package(0); - } - (*prop_count)++; - acpi_dp_write_property(dp); - } - } - if (prop_count) { - /* Package (PROP) length, if a package was written */ - acpigen_pop_len(); - } + if (acpi_dp_write_properties(prop, table->uuid)) + *dp_count += 2;
+ /* Count child properties */ + for (dp = prop; dp; dp = dp->next) + if (dp->type == ACPI_DP_TYPE_CHILD) + child_count++; + + /* Add child properties to the base table */ if (child_count) { - /* Update DP package count to 2 or 4 */ + /* Update DP package count */ *dp_count += 2; /* ToUUID (ACPI_DP_CHILD_UUID) */ acpigen_write_uuid(ACPI_DP_CHILD_UUID); @@ -722,6 +740,12 @@ acpigen_pop_len(); }
+ /* Write packages of properties with unique UUID */ + for (dp = prop; dp; dp = dp->next) + if (dp->type == ACPI_DP_TYPE_PACKAGE) + if (acpi_dp_write_properties(dp->child, dp->uuid)) + *dp_count += 2; + /* Package (DP) length */ acpigen_pop_len();
@@ -746,6 +770,7 @@ memset(new, 0, sizeof(*new)); new->type = type; new->name = name; + new->uuid = ACPI_DP_UUID;
if (dp) { /* Add to end of property list */ @@ -863,6 +888,22 @@ return new; }
+struct acpi_dp *acpi_dp_add_package(struct acpi_dp *dp, struct acpi_dp *package) +{ + struct acpi_dp *new; + + if (!dp || !package || package->type != ACPI_DP_TYPE_TABLE) + return NULL; + + new = acpi_dp_new(dp, ACPI_DP_TYPE_PACKAGE, NULL); + if (new) { + new->uuid = package->name; + new->child = package; + } + + return new; +} + struct acpi_dp *acpi_dp_add_array(struct acpi_dp *dp, struct acpi_dp *array) { struct acpi_dp *new; diff --git a/src/include/acpi/acpi_device.h b/src/include/acpi/acpi_device.h index 20dd8a4..2c2c67a 100644 --- a/src/include/acpi/acpi_device.h +++ b/src/include/acpi/acpi_device.h @@ -15,11 +15,13 @@ ACPI_DP_TYPE_TABLE, ACPI_DP_TYPE_ARRAY, ACPI_DP_TYPE_CHILD, + ACPI_DP_TYPE_PACKAGE, };
struct acpi_dp { enum acpi_dp_type type; const char *name; + const char *uuid; struct acpi_dp *next; union { struct acpi_dp *child; @@ -464,6 +466,9 @@ /* Start a new Device Property table with provided ACPI reference */ struct acpi_dp *acpi_dp_new_table(const char *ref);
+/* Add package of device properties with a unique UUID */ +struct acpi_dp *acpi_dp_add_package(struct acpi_dp *dp, struct acpi_dp *package); + /* Add integer Device Property */ struct acpi_dp *acpi_dp_add_integer(struct acpi_dp *dp, const char *name, uint64_t value);
Hello Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42047
to look at the new patch set (#2).
Change subject: acpi: Accomodate non-standard UUIDs in device properties ......................................................................
acpi: Accomodate non-standard UUIDs in device properties
There have been changes to the way device properties are supported in Linux[1] and Windows[2] which add flexibilty:
- non-standard UUIDs can be used instead of only ACPI_DP_UUID - support for multiple different packages within the _DSD that associate different properties with unique UUIDs.
To handle this I extracted the part that does the write of UUID and properties to a separate function and defined a new PACKAGE type which has the custom UUID as a name and can be used the same way that child properties are today.
For example a PCIe root port for a USB4 port has a standard property indicating the USB4 reference, and then two custom properties which are defined for different attributes.
Example code:
/* Create property table */ acpi_dp *dsd = acpi_dp_new_table("_DSD"); acpi_dp_add_reference(dsd, "usb4-port", usb4_path);
/* Add package for hotplug */ acpi_dp *pkg = acpi_dp_new_table("6211e2c0-58a3-4af3-90e1-927a4e0c55a4"); acpi_dp_add_integer(pkg, "HotPlugSupportInD3", 1); acpi_dp_add_package(dsd, pkg);
/* Add package for external port info */ pkg = acpi_dp_new_table("efcc06cc-73ac-4bc3-bff0-76143807c389"); acpi_dp_add_integer(pkg, "ExternalFacingPort", 1); acpi_dp_add_package(dsd, pkg);
/* Write all properties */ acpi_dp_write(dsd);
Resulting ACPI:
Scope (_SB.PCI0.TRP0) { Name (_DSD, Package () { ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") Package () { Package () { "usb4-port", _SB.PCI0.TDM0.RHUB.PRTA } },
ToUUID ("6211e2c0-58a3-4af3-90e1-927a4e0c55a4"), Package () { Package () { "HotPlugSupportInD3", One } },
ToUUID ("efcc06cc-73ac-4bc3-bff0-76143807c389"), Package () { Package () { "ExternalFacingPort", One }, } }) }
[1] https://patchwork.kernel.org/patch/10599675/ [2] https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-r...
Change-Id: I75f47825bf4ffc5e9e92af2c45790d1b5945576e Signed-off-by: Duncan Laurie dlaurie@google.com --- M src/acpi/device.c M src/include/acpi/acpi_device.h 2 files changed, 77 insertions(+), 31 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/42047/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42047 )
Change subject: acpi: Accomodate non-standard UUIDs in device properties ......................................................................
Patch Set 2: Code-Review+2
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42047 )
Change subject: acpi: Accomodate non-standard UUIDs in device properties ......................................................................
acpi: Accomodate non-standard UUIDs in device properties
There have been changes to the way device properties are supported in Linux[1] and Windows[2] which add flexibilty:
- non-standard UUIDs can be used instead of only ACPI_DP_UUID - support for multiple different packages within the _DSD that associate different properties with unique UUIDs.
To handle this I extracted the part that does the write of UUID and properties to a separate function and defined a new PACKAGE type which has the custom UUID as a name and can be used the same way that child properties are today.
For example a PCIe root port for a USB4 port has a standard property indicating the USB4 reference, and then two custom properties which are defined for different attributes.
Example code:
/* Create property table */ acpi_dp *dsd = acpi_dp_new_table("_DSD"); acpi_dp_add_reference(dsd, "usb4-port", usb4_path);
/* Add package for hotplug */ acpi_dp *pkg = acpi_dp_new_table("6211e2c0-58a3-4af3-90e1-927a4e0c55a4"); acpi_dp_add_integer(pkg, "HotPlugSupportInD3", 1); acpi_dp_add_package(dsd, pkg);
/* Add package for external port info */ pkg = acpi_dp_new_table("efcc06cc-73ac-4bc3-bff0-76143807c389"); acpi_dp_add_integer(pkg, "ExternalFacingPort", 1); acpi_dp_add_package(dsd, pkg);
/* Write all properties */ acpi_dp_write(dsd);
Resulting ACPI:
Scope (_SB.PCI0.TRP0) { Name (_DSD, Package () { ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") Package () { Package () { "usb4-port", _SB.PCI0.TDM0.RHUB.PRTA } },
ToUUID ("6211e2c0-58a3-4af3-90e1-927a4e0c55a4"), Package () { Package () { "HotPlugSupportInD3", One } },
ToUUID ("efcc06cc-73ac-4bc3-bff0-76143807c389"), Package () { Package () { "ExternalFacingPort", One }, } }) }
[1] https://patchwork.kernel.org/patch/10599675/ [2] https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-r...
Change-Id: I75f47825bf4ffc5e9e92af2c45790d1b5945576e Signed-off-by: Duncan Laurie dlaurie@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/42047 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/acpi/device.c M src/include/acpi/acpi_device.h 2 files changed, 77 insertions(+), 31 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/acpi/device.c b/src/acpi/device.c index 8b03bfd..9e8efa4 100644 --- a/src/acpi/device.c +++ b/src/acpi/device.c @@ -53,7 +53,7 @@ return NULL;
/* Check for device specific handler */ - if (dev->ops->acpi_name) + if (dev->ops && dev->ops->acpi_name) return dev->ops->acpi_name(dev);
/* Walk up the tree to find if any parent can identify this device */ @@ -658,10 +658,48 @@ } }
+static bool acpi_dp_write_properties(struct acpi_dp *prop, const char *uuid) +{ + struct acpi_dp *dp; + char *prop_count = NULL; + + /* Print base properties */ + for (dp = prop; dp; dp = dp->next) { + if (dp->type == ACPI_DP_TYPE_TABLE || + dp->type == ACPI_DP_TYPE_CHILD || + dp->type == ACPI_DP_TYPE_PACKAGE) + continue; + + /* + * The UUID and package is only added when + * we come across the first property. This + * is to avoid creating a zero-length package + * in situations where there are only children. + */ + if (!prop_count) { + /* ToUUID (dp->uuid) */ + acpigen_write_uuid(uuid); + /* + * Package (PROP), element count determined as + * it is populated + */ + prop_count = acpigen_write_package(0); + } + (*prop_count)++; + acpi_dp_write_property(dp); + } + if (prop_count) { + /* Package (PROP) length, if a package was written */ + acpigen_pop_len(); + return true; + } + return false; +} + void acpi_dp_write(struct acpi_dp *table) { struct acpi_dp *dp, *prop; - char *dp_count, *prop_count = NULL; + char *dp_count; int child_count = 0;
if (!table || table->type != ACPI_DP_TYPE_TABLE || !table->next) @@ -677,37 +715,17 @@ dp_count = acpigen_write_package(0);
/* Print base properties */ - for (dp = prop; dp; dp = dp->next) { - if (dp->type == ACPI_DP_TYPE_CHILD) { - child_count++; - } else { - /* - * The UUID and package is only added when - * we come across the first property. This - * is to avoid creating a zero-length package - * in situations where there are only children. - */ - if (!prop_count) { - *dp_count += 2; - /* ToUUID (ACPI_DP_UUID) */ - acpigen_write_uuid(ACPI_DP_UUID); - /* - * Package (PROP), element count determined as - * it is populated - */ - prop_count = acpigen_write_package(0); - } - (*prop_count)++; - acpi_dp_write_property(dp); - } - } - if (prop_count) { - /* Package (PROP) length, if a package was written */ - acpigen_pop_len(); - } + if (acpi_dp_write_properties(prop, table->uuid)) + *dp_count += 2;
+ /* Count child properties */ + for (dp = prop; dp; dp = dp->next) + if (dp->type == ACPI_DP_TYPE_CHILD) + child_count++; + + /* Add child properties to the base table */ if (child_count) { - /* Update DP package count to 2 or 4 */ + /* Update DP package count */ *dp_count += 2; /* ToUUID (ACPI_DP_CHILD_UUID) */ acpigen_write_uuid(ACPI_DP_CHILD_UUID); @@ -722,6 +740,12 @@ acpigen_pop_len(); }
+ /* Write packages of properties with unique UUID */ + for (dp = prop; dp; dp = dp->next) + if (dp->type == ACPI_DP_TYPE_PACKAGE) + if (acpi_dp_write_properties(dp->child, dp->uuid)) + *dp_count += 2; + /* Package (DP) length */ acpigen_pop_len();
@@ -746,6 +770,7 @@ memset(new, 0, sizeof(*new)); new->type = type; new->name = name; + new->uuid = ACPI_DP_UUID;
if (dp) { /* Add to end of property list */ @@ -863,6 +888,22 @@ return new; }
+struct acpi_dp *acpi_dp_add_package(struct acpi_dp *dp, struct acpi_dp *package) +{ + struct acpi_dp *new; + + if (!dp || !package || package->type != ACPI_DP_TYPE_TABLE) + return NULL; + + new = acpi_dp_new(dp, ACPI_DP_TYPE_PACKAGE, NULL); + if (new) { + new->uuid = package->name; + new->child = package; + } + + return new; +} + struct acpi_dp *acpi_dp_add_array(struct acpi_dp *dp, struct acpi_dp *array) { struct acpi_dp *new; diff --git a/src/include/acpi/acpi_device.h b/src/include/acpi/acpi_device.h index 20dd8a4..2c2c67a 100644 --- a/src/include/acpi/acpi_device.h +++ b/src/include/acpi/acpi_device.h @@ -15,11 +15,13 @@ ACPI_DP_TYPE_TABLE, ACPI_DP_TYPE_ARRAY, ACPI_DP_TYPE_CHILD, + ACPI_DP_TYPE_PACKAGE, };
struct acpi_dp { enum acpi_dp_type type; const char *name; + const char *uuid; struct acpi_dp *next; union { struct acpi_dp *child; @@ -464,6 +466,9 @@ /* Start a new Device Property table with provided ACPI reference */ struct acpi_dp *acpi_dp_new_table(const char *ref);
+/* Add package of device properties with a unique UUID */ +struct acpi_dp *acpi_dp_add_package(struct acpi_dp *dp, struct acpi_dp *package); + /* Add integer Device Property */ struct acpi_dp *acpi_dp_add_integer(struct acpi_dp *dp, const char *name, uint64_t value);
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42047 )
Change subject: acpi: Accomodate non-standard UUIDs in device properties ......................................................................
Patch Set 4:
Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/4895 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4894 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4893 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/4892
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Duncan Laurie (Code Review)
-
Furquan Shaikh (Code Review)