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:
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 parent context */
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()?
Cheers,
Gustavo
