Re: [PATCH 14/22] hw/i386/acpi-build: Move aml_pci_edsm to a generic place

[Gustavo Romero]

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.


Cheers,
Gustavo

Thank you for the comprehensive review!

Cheers

Eric

EDSM, being used in i386 code, feels a outdated PDSM, so maybe PDSM
should be used there,
although a different story or a clean up for later. I'm not sure what
"static endpoint"
means in the context of EDSM.

Hence this patch can be dropped in my understanding and:

aml_append(pci0_scope, aml_pci_edsm()) in 15/22 too, without any
prejudice to the
hotplugging and unplugging in this series.


Cheers,
Gustavo