Hi Eric, On 5/27/25 04:18, Eric Auger wrote:
On 5/21/25 10:19 PM, Gustavo Romero wrote:Hi Eric, On 5/21/25 13:24, Gustavo Romero wrote:Hi Eric, On 5/21/25 12:56, Eric Auger wrote:Hi Gustavo, On 5/21/25 5:26 PM, Gustavo Romero wrote:Hi Eric, On 5/14/25 14:01, Eric Auger wrote:Move aml_pci_edsm to pcihp since we want to reuse that for ARM and acpi-index support. Signed-off-by: Eric Auger <eric.au...@redhat.com> --- include/hw/acpi/pcihp.h | 2 ++ hw/acpi/pcihp.c | 53 +++++++++++++++++++++++++++++++++++++++++ hw/i386/acpi-build.c | 53 ----------------------------------------- 3 files changed, 55 insertions(+), 53 deletions(-) diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h index 4d820b4baf..bc31dbff39 100644 --- a/include/hw/acpi/pcihp.h +++ b/include/hw/acpi/pcihp.h @@ -82,6 +82,8 @@ bool build_append_notification_callback(Aml *parent_scope, const PCIBus *bus); void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus); +Aml *aml_pci_edsm(void); + /* Called on reset */ void acpi_pcihp_reset(AcpiPciHpState *s); diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c index d800371ddc..57fe8938b1 100644 --- a/hw/acpi/pcihp.c +++ b/hw/acpi/pcihp.c @@ -937,6 +937,59 @@ void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus) } } +Aml *aml_pci_edsm(void) +{ + Aml *method, *ifctx; + Aml *zero = aml_int(0); + Aml *func = aml_arg(2); + Aml *ret = aml_local(0); + Aml *aidx = aml_local(1); + Aml *params = aml_arg(4); + + method = aml_method("EDSM", 5, AML_SERIALIZED); + + /* get supported functions */ + ifctx = aml_if(aml_equal(func, zero)); + { + /* 1: have supported functions */ + /* 7: support for function 7 */ + const uint8_t caps = 1 | BIT(7); + build_append_pci_dsm_func0_common(ifctx, ret); + aml_append(ifctx, aml_store(aml_int(caps), aml_index(ret, zero))); + aml_append(ifctx, aml_return(ret)); + } + aml_append(method, ifctx); + + /* handle specific functions requests */ + /* + * PCI Firmware Specification 3.1 + * 4.6.7. _DSM for Naming a PCI or PCI Express Device Under + * Operating Systems + */ + ifctx = aml_if(aml_equal(func, aml_int(7))); + { + Aml *pkg = aml_package(2); + aml_append(pkg, zero); + /* optional, if not impl. should return null string */ + aml_append(pkg, aml_string("%s", "")); + aml_append(ifctx, aml_store(pkg, ret)); + + /* + * IASL is fine when initializing Package with computational data, + * however it makes guest unhappy /it fails to process such AML/. + * So use runtime assignment to set acpi-index after initializer + * to make OSPM happy. + */ + aml_append(ifctx, + aml_store(aml_derefof(aml_index(params, aml_int(0))), aidx)); + aml_append(ifctx, aml_store(aidx, aml_index(ret, zero))); + aml_append(ifctx, aml_return(ret)); + } + aml_append(method, ifctx); + + return method; +} + const VMStateDescription vmstate_acpi_pcihp_pci_status = { .name = "acpi_pcihp_pci_status", .version_id = 1, diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index bcfba2ccb3..385e75d061 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -322,59 +322,6 @@ build_facs(GArray *table_data) g_array_append_vals(table_data, reserved, 40); /* Reserved */ } -static Aml *aml_pci_edsm(void) -{ - Aml *method, *ifctx; - Aml *zero = aml_int(0); - Aml *func = aml_arg(2); - Aml *ret = aml_local(0); - Aml *aidx = aml_local(1); - Aml *params = aml_arg(4); - - method = aml_method("EDSM", 5, AML_SERIALIZED); - - /* get supported functions */ - ifctx = aml_if(aml_equal(func, zero)); - { - /* 1: have supported functions */ - /* 7: support for function 7 */ - const uint8_t caps = 1 | BIT(7); - build_append_pci_dsm_func0_common(ifctx, ret); - aml_append(ifctx, aml_store(aml_int(caps), aml_index(ret, zero))); - aml_append(ifctx, aml_return(ret)); - } - aml_append(method, ifctx); - - /* handle specific functions requests */ - /* - * PCI Firmware Specification 3.1 - * 4.6.7. _DSM for Naming a PCI or PCI Express Device Under - * Operating Systems - */ - ifctx = aml_if(aml_equal(func, aml_int(7))); - { - Aml *pkg = aml_package(2); - aml_append(pkg, zero); - /* optional, if not impl. should return null string */ - aml_append(pkg, aml_string("%s", "")); - aml_append(ifctx, aml_store(pkg, ret)); - - /* - * IASL is fine when initializing Package with computational data, - * however it makes guest unhappy /it fails to process such AML/. - * So use runtime assignment to set acpi-index after initializer - * to make OSPM happy. - */ - aml_append(ifctx, - aml_store(aml_derefof(aml_index(params, aml_int(0))), aidx)); - aml_append(ifctx, aml_store(aidx, aml_index(ret, zero))); - aml_append(ifctx, aml_return(ret)); - } - aml_append(method, ifctx); - - return method; -} - /* * build_prt - Define interrupt routing rules *EDSM is not called from anywhere. _DSM method actually calls the PDSM, already defined in pcihp.c and generated by aml_pci_pdsm(). build_acpi_pci_hotplug(), which calls aml_pci_pdsm(), properly creates the PDSM method. Then in build_append_pcihp_slots() the a _DSM is defined per slot and inside it there is a call to PDSM, so I understand we should actually stick to the PDSM in pcihp.c instead of EDSM.I see build_append_pci_bus_devices -> aml_pci_static_endpoint_dsm adds a _DSM method which eventually calls EDSM.Yes, but I meant in the final generated ACPI AML code.aml_pci_device_dsm builds another _DSM implementation which calls PDSM. As we use build_append_pci_bus_devices and we are likely to have a _DSM implementation that calls EDSM method, I think we shall have it in the aml. What do I miss?Maybe some condition in build_append_pci_bus_devices() of: if (pdev->acpi_index && !object_property_get_bool(OBJECT(pdev), "hotpluggable", &error_abort)) { ... } is not met? Does your _DSM method in the machine's ACPI has a call to EDSM? I don't see it. In my case it's the PDSM that is being called: gromero@xps13:/tmp$ grep -n PDSM dsdt.dsl 1910: Method (PDSM, 5, Serialized) 2047: Return (PDSM (Arg0, Arg1, Arg2, Arg3, Local0)) gromero@xps13:/tmp$ grep -n EDSM dsdt.dsl 1959: Method (EDSM, 5, Serialized) gromero@xps13:/tmp$ I also confirmed that PDSM is what is being used by reverting this patch and dropping the aml_append(pci0_scope, aml_pci_edsm()); in 15/22. But also, why shouldn't we use the PDSM defined already in pcihp.c? This is indeed crafted for the acpi-pcihp-hotplug device as I understand it.Sorry, I see now, the EDSM is actually only for static endpoint PCI devices, i.e. non-hotpluggable, I see. So PDSM and EDSM serve different purposes, although similar. So, my point in the end is (and the one that confused me too) that EDSM can be present even if it will never be used by any static device, working pretty much like an ACPI deadcode. Could we avoid this? For instance, just emit EDSM if a _DSM that relies on it is emitted, like:I see your point and it's true that for some PCIe configs the code can be there while not used. However it looks it is done the same way on x86 and I would tend to be not more popish than the pope ;-) Maybe this can be a patch sent afterwards that would also address the x86 case then.
Got it. Well, I think it's just a matter of making
build_append_pci_bus_devices()
tell if any static endpoint is added and just call aml_pci_edsm() to emit EDSM
if
there is any, like in my diff:
+ /* Generate tables for static endpoints. */
+ if (build_append_pci_bus_devices(pci0_scope, vms->bus)) {
+ /* Has a _DSM method that requires a EDSM method. */
+ aml_append(pci0_scope, aml_pci_edsm());
+ }
But I'm not really the maintainer or the official reviewer for this part of
the code, and you certainly have more karma than I do in this area, so it's
up to you, of course. I just think ACPI code is tricky enough to review,
and having dead code makes it a tad more daunting :)
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c index e87846a1fa..4d6dd3dd96 100644 --- a/hw/acpi/pcihp.c +++ b/hw/acpi/pcihp.c @@ -905,8 +905,9 @@ void build_append_pcihp_slots(Aml *parent_scope, PCIBus *bus) aml_append(parent_scope, notify_method); }-void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus)+bool build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus) { + bool has_dsm = false; int devfn; Aml *dev;@@ -929,11 +930,14 @@ void build_append_pci_bus_devices(Aml*parent_scope, PCIBus *bus) !object_property_get_bool(OBJECT(pdev), "hotpluggable", &error_abort)) { aml_append(dev, aml_pci_static_endpoint_dsm(pdev)); + has_dsm = true; }/* device descriptor has been composed, add it into parentcontext */ aml_append(parent_scope, dev); } + + return has_dsm; }Aml *aml_pci_edsm(void)diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index 05a754d368..e4788c03ed 100644 --- a/hw/arm/virt-acpi-build.c +++ b/hw/arm/virt-acpi-build.c @@ -844,15 +844,22 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) if (vms->acpi_pcihp) { Aml *pci0_scope = aml_scope("\\_SB.PCI0");- aml_append(pci0_scope, aml_pci_edsm());build_acpi_pci_hotplug(dsdt, AML_SYSTEM_MEMORY, memmap[VIRT_ACPI_PCIHP].base); build_append_pcihp_resources(pci0_scope, memmap[VIRT_ACPI_PCIHP].base, memmap[VIRT_ACPI_PCIHP].size);- /* Scan all PCI buses. Generate tables to support hotplug. */- build_append_pci_bus_devices(pci0_scope, vms->bus); + /* + * Scan all PCI buses and devices. Generate tables to support static + * endpoint devices and hotplug. + */ + /* Generate tables for static endpoints. */ + if (build_append_pci_bus_devices(pci0_scope, vms->bus)) { + /* Has a _DSM method that requires a EDSM method. */ + aml_append(pci0_scope, aml_pci_edsm()); + } + /* Generate tables for PCI buses/slots/ */ if (object_property_find(OBJECT(vms->bus), ACPI_PCIHP_PROP_BSEL)) { build_append_pcihp_slots(pci0_scope, vms->bus); } diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h index bda5ea24b5..25cc39f4ab 100644 --- a/include/hw/acpi/pcihp.h +++ b/include/hw/acpi/pcihp.h @@ -82,7 +82,7 @@ void build_append_pcihp_resources(Aml *table, uint64_t io_addr, uint64_t io_len); bool build_append_notification_callback(Aml *parent_scope, const PCIBus *bus);-void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus);+bool build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus);Aml *aml_pci_edsm(void);Another question that I have is: could you please share a QEMU command line option that generates a static endpoint PCI device that matches the criteria for the emission of the _DSM method accordingly to build_append_pci_bus_devices()?You can instantiate a virtio-net-pci device on pci.0 and with acpi-index set. -device virtio-net-pci,bus=pcie.0,netdev=nic0,mac=6a:f5:10:b1:3d:d2,acpi-index=12 In that case EDSM is being called: Device (S08) { Name (_ADR, 0x00010000) // _ADR: Address Method (_DSM, 4, Serialized) // _DSM: Device-Specific Method { Local0 = Package (0x01) { 0x0C } Return (EDSM (Arg0, Arg1, Arg2, Arg3, Local0)) } } Putting it on a root port will let the EDSM unused.
Thanks for the example. Would it be correct to say so that EDSM is part of the coldplug and so could be left for another series, so out of scope for hotpluging? Cheers, Gustavo
