[PATCH v3 16/29] hw/i386/acpi-build: Move aml_pci_edsm to a generic place

Eric Auger posted 29 patches 5 months ago
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Igor Mammedov <imammedo@redhat.com>, Ani Sinha <anisinha@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, Shannon Zhao <shannon.zhaosl@gmail.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Richard Henderson <richard.henderson@linaro.org>
There is a newer version of this series
[PATCH v3 16/29] hw/i386/acpi-build: Move aml_pci_edsm to a generic place
Posted by Eric Auger 5 months ago
Move aml_pci_edsm to pci-bridge.c since we want to reuse that for
ARM and acpi-index support.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

v2 -> v3:
- move to pci-bridge.c instead of pcihp.c (Igor)
---
 include/hw/acpi/pci.h |  1 +
 hw/acpi/pci-bridge.c  | 54 +++++++++++++++++++++++++++++++++++++++++++
 hw/i386/acpi-build.c  | 53 ------------------------------------------
 3 files changed, 55 insertions(+), 53 deletions(-)

diff --git a/include/hw/acpi/pci.h b/include/hw/acpi/pci.h
index 69bae95eac..05e72815c8 100644
--- a/include/hw/acpi/pci.h
+++ b/include/hw/acpi/pci.h
@@ -42,5 +42,6 @@ void build_pci_bridge_aml(AcpiDevAmlIf *adev, Aml *scope);
 void build_srat_generic_affinity_structures(GArray *table_data);
 
 Aml *build_pci_host_bridge_osc_method(bool enable_native_pcie_hotplug);
+Aml *aml_pci_edsm(void);
 
 #endif
diff --git a/hw/acpi/pci-bridge.c b/hw/acpi/pci-bridge.c
index 7baa7034a1..be68a98c34 100644
--- a/hw/acpi/pci-bridge.c
+++ b/hw/acpi/pci-bridge.c
@@ -35,3 +35,57 @@ void build_pci_bridge_aml(AcpiDevAmlIf *adev, Aml *scope)
         }
     }
 }
+
+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;
+}
+
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index fe8bc62c03..6cf623392e 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -338,59 +338,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
  *
-- 
2.49.0
Re: [PATCH v3 16/29] hw/i386/acpi-build: Move aml_pci_edsm to a generic place
Posted by Jonathan Cameron via 4 months, 4 weeks ago
On Mon, 16 Jun 2025 11:46:45 +0200
Eric Auger <eric.auger@redhat.com> wrote:

> Move aml_pci_edsm to pci-bridge.c since we want to reuse that for
> ARM and acpi-index support.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>

A request for a bit of documentation inline.  aml_pci_edsm() sounds
like we should be able to grep the spec for edsm and find it but
that's just internal method naming in qemu.


More interesting is I don't think this will ever be called as
the kernel has no idea how to call it and unlike on x86 the
blobs don't show wrapping the call in a _DSM() (see aml_pci_static_endpoint_dsm())

Did EDSM usage get dropped as this set evolved leaving this behind?



>
> ---
> 
> v2 -> v3:
> - move to pci-bridge.c instead of pcihp.c (Igor)
> ---
>  include/hw/acpi/pci.h |  1 +
>  hw/acpi/pci-bridge.c  | 54 +++++++++++++++++++++++++++++++++++++++++++
>  hw/i386/acpi-build.c  | 53 ------------------------------------------
>  3 files changed, 55 insertions(+), 53 deletions(-)
> 
> diff --git a/include/hw/acpi/pci.h b/include/hw/acpi/pci.h
> index 69bae95eac..05e72815c8 100644
> --- a/include/hw/acpi/pci.h
> +++ b/include/hw/acpi/pci.h
> @@ -42,5 +42,6 @@ void build_pci_bridge_aml(AcpiDevAmlIf *adev, Aml *scope);
>  void build_srat_generic_affinity_structures(GArray *table_data);
>  
>  Aml *build_pci_host_bridge_osc_method(bool enable_native_pcie_hotplug);
> +Aml *aml_pci_edsm(void);
>  
>  #endif
> diff --git a/hw/acpi/pci-bridge.c b/hw/acpi/pci-bridge.c
> index 7baa7034a1..be68a98c34 100644
> --- a/hw/acpi/pci-bridge.c
> +++ b/hw/acpi/pci-bridge.c
> @@ -35,3 +35,57 @@ void build_pci_bridge_aml(AcpiDevAmlIf *adev, Aml *scope)
>          }
>      }
>  }
> +
> +Aml *aml_pci_edsm(void)

Can we have some comments, or a more descriptive name than
the resulting method name?  There is stuff in the function obviously
that associates it with the naming DSM but given this is moving to
generic code maybe it needs a brief intro comment?


> +{
> +    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;
> +}
> +
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index fe8bc62c03..6cf623392e 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -338,59 +338,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
>   *
Re: [PATCH v3 16/29] hw/i386/acpi-build: Move aml_pci_edsm to a generic place
Posted by Igor Mammedov 4 months, 4 weeks ago
On Fri, 20 Jun 2025 10:19:36 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> On Mon, 16 Jun 2025 11:46:45 +0200
> Eric Auger <eric.auger@redhat.com> wrote:
> 
> > Move aml_pci_edsm to pci-bridge.c since we want to reuse that for
> > ARM and acpi-index support.
> > 
> > Signed-off-by: Eric Auger <eric.auger@redhat.com>  
> 
> A request for a bit of documentation inline.  aml_pci_edsm() sounds
> like we should be able to grep the spec for edsm and find it but
> that's just internal method naming in qemu.

agree, aml_ prefix is typically reserved for ACPI spec items. 

perhaps rename it to follow build_ prefix scheme?

> 
> 
> More interesting is I don't think this will ever be called as
> the kernel has no idea how to call it and unlike on x86 the
> blobs don't show wrapping the call in a _DSM() (see aml_pci_static_endpoint_dsm())
> 
> Did EDSM usage get dropped as this set evolved leaving this behind?
> 
> 
> 
> >
> > ---
> > 
> > v2 -> v3:
> > - move to pci-bridge.c instead of pcihp.c (Igor)
> > ---
> >  include/hw/acpi/pci.h |  1 +
> >  hw/acpi/pci-bridge.c  | 54 +++++++++++++++++++++++++++++++++++++++++++
> >  hw/i386/acpi-build.c  | 53 ------------------------------------------
> >  3 files changed, 55 insertions(+), 53 deletions(-)
> > 
> > diff --git a/include/hw/acpi/pci.h b/include/hw/acpi/pci.h
> > index 69bae95eac..05e72815c8 100644
> > --- a/include/hw/acpi/pci.h
> > +++ b/include/hw/acpi/pci.h
> > @@ -42,5 +42,6 @@ void build_pci_bridge_aml(AcpiDevAmlIf *adev, Aml *scope);
> >  void build_srat_generic_affinity_structures(GArray *table_data);
> >  
> >  Aml *build_pci_host_bridge_osc_method(bool enable_native_pcie_hotplug);
> > +Aml *aml_pci_edsm(void);
> >  
> >  #endif
> > diff --git a/hw/acpi/pci-bridge.c b/hw/acpi/pci-bridge.c
> > index 7baa7034a1..be68a98c34 100644
> > --- a/hw/acpi/pci-bridge.c
> > +++ b/hw/acpi/pci-bridge.c
> > @@ -35,3 +35,57 @@ void build_pci_bridge_aml(AcpiDevAmlIf *adev, Aml *scope)
> >          }
> >      }
> >  }
> > +
> > +Aml *aml_pci_edsm(void)  
> 
> Can we have some comments, or a more descriptive name than
> the resulting method name?  There is stuff in the function obviously
> that associates it with the naming DSM but given this is moving to
> generic code maybe it needs a brief intro comment?
> 
> 
> > +{
> > +    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;
> > +}
> > +
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index fe8bc62c03..6cf623392e 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -338,59 +338,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
> >   *  
>
Re: [PATCH v3 16/29] hw/i386/acpi-build: Move aml_pci_edsm to a generic place
Posted by Eric Auger 4 months, 3 weeks ago
Hi Igor, Jonathan,

On 6/20/25 2:27 PM, Igor Mammedov wrote:
> On Fri, 20 Jun 2025 10:19:36 +0100
> Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
>
>> On Mon, 16 Jun 2025 11:46:45 +0200
>> Eric Auger <eric.auger@redhat.com> wrote:
>>
>>> Move aml_pci_edsm to pci-bridge.c since we want to reuse that for
>>> ARM and acpi-index support.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>  
>> A request for a bit of documentation inline.  aml_pci_edsm() sounds
>> like we should be able to grep the spec for edsm and find it but
>> that's just internal method naming in qemu.
> agree, aml_ prefix is typically reserved for ACPI spec items. 
renamed into build_pci_bridge_edsm()

Thanks

Eric
>
> perhaps rename it to follow build_ prefix scheme?
>
>>
>> More interesting is I don't think this will ever be called as
>> the kernel has no idea how to call it and unlike on x86 the
>> blobs don't show wrapping the call in a _DSM() (see aml_pci_static_endpoint_dsm())
>>
>> Did EDSM usage get dropped as this set evolved leaving this behind?
>>
>>
>>
>>> ---
>>>
>>> v2 -> v3:
>>> - move to pci-bridge.c instead of pcihp.c (Igor)
>>> ---
>>>  include/hw/acpi/pci.h |  1 +
>>>  hw/acpi/pci-bridge.c  | 54 +++++++++++++++++++++++++++++++++++++++++++
>>>  hw/i386/acpi-build.c  | 53 ------------------------------------------
>>>  3 files changed, 55 insertions(+), 53 deletions(-)
>>>
>>> diff --git a/include/hw/acpi/pci.h b/include/hw/acpi/pci.h
>>> index 69bae95eac..05e72815c8 100644
>>> --- a/include/hw/acpi/pci.h
>>> +++ b/include/hw/acpi/pci.h
>>> @@ -42,5 +42,6 @@ void build_pci_bridge_aml(AcpiDevAmlIf *adev, Aml *scope);
>>>  void build_srat_generic_affinity_structures(GArray *table_data);
>>>  
>>>  Aml *build_pci_host_bridge_osc_method(bool enable_native_pcie_hotplug);
>>> +Aml *aml_pci_edsm(void);
>>>  
>>>  #endif
>>> diff --git a/hw/acpi/pci-bridge.c b/hw/acpi/pci-bridge.c
>>> index 7baa7034a1..be68a98c34 100644
>>> --- a/hw/acpi/pci-bridge.c
>>> +++ b/hw/acpi/pci-bridge.c
>>> @@ -35,3 +35,57 @@ void build_pci_bridge_aml(AcpiDevAmlIf *adev, Aml *scope)
>>>          }
>>>      }
>>>  }
>>> +
>>> +Aml *aml_pci_edsm(void)  
>> Can we have some comments, or a more descriptive name than
>> the resulting method name?  There is stuff in the function obviously
>> that associates it with the naming DSM but given this is moving to
>> generic code maybe it needs a brief intro comment?
>>
>>
>>> +{
>>> +    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;
>>> +}
>>> +
>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>> index fe8bc62c03..6cf623392e 100644
>>> --- a/hw/i386/acpi-build.c
>>> +++ b/hw/i386/acpi-build.c
>>> @@ -338,59 +338,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
>>>   *
Re: [PATCH v3 16/29] hw/i386/acpi-build: Move aml_pci_edsm to a generic place
Posted by Jonathan Cameron via 4 months, 4 weeks ago
On Fri, 20 Jun 2025 10:19:36 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> On Mon, 16 Jun 2025 11:46:45 +0200
> Eric Auger <eric.auger@redhat.com> wrote:
> 
> > Move aml_pci_edsm to pci-bridge.c since we want to reuse that for
> > ARM and acpi-index support.
> > 
> > Signed-off-by: Eric Auger <eric.auger@redhat.com>  
> 
> A request for a bit of documentation inline.  aml_pci_edsm() sounds
> like we should be able to grep the spec for edsm and find it but
> that's just internal method naming in qemu.
> 
> 
> More interesting is I don't think this will ever be called as
> the kernel has no idea how to call it and unlike on x86 the
> blobs don't show wrapping the call in a _DSM() (see aml_pci_static_endpoint_dsm())
> 
> Did EDSM usage get dropped as this set evolved leaving this behind?
Doh. You moved and use build_append_pci_bus_devices().

So this is fine.  Ignore me.

I'd still like a comment though - particularly as it turns up in the blobs
with no callers because the examples don't use it yet.

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

> 
> 
> 
> >
> > ---
> > 
> > v2 -> v3:
> > - move to pci-bridge.c instead of pcihp.c (Igor)
> > ---
> >  include/hw/acpi/pci.h |  1 +
> >  hw/acpi/pci-bridge.c  | 54 +++++++++++++++++++++++++++++++++++++++++++
> >  hw/i386/acpi-build.c  | 53 ------------------------------------------
> >  3 files changed, 55 insertions(+), 53 deletions(-)
> > 
> > diff --git a/include/hw/acpi/pci.h b/include/hw/acpi/pci.h
> > index 69bae95eac..05e72815c8 100644
> > --- a/include/hw/acpi/pci.h
> > +++ b/include/hw/acpi/pci.h
> > @@ -42,5 +42,6 @@ void build_pci_bridge_aml(AcpiDevAmlIf *adev, Aml *scope);
> >  void build_srat_generic_affinity_structures(GArray *table_data);
> >  
> >  Aml *build_pci_host_bridge_osc_method(bool enable_native_pcie_hotplug);
> > +Aml *aml_pci_edsm(void);
> >  
> >  #endif
> > diff --git a/hw/acpi/pci-bridge.c b/hw/acpi/pci-bridge.c
> > index 7baa7034a1..be68a98c34 100644
> > --- a/hw/acpi/pci-bridge.c
> > +++ b/hw/acpi/pci-bridge.c
> > @@ -35,3 +35,57 @@ void build_pci_bridge_aml(AcpiDevAmlIf *adev, Aml *scope)
> >          }
> >      }
> >  }
> > +
> > +Aml *aml_pci_edsm(void)  
> 
> Can we have some comments, or a more descriptive name than
> the resulting method name?  There is stuff in the function obviously
> that associates it with the naming DSM but given this is moving to
> generic code maybe it needs a brief intro comment?
> 
> 
> > +{
> > +    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;
> > +}
> > +
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index fe8bc62c03..6cf623392e 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -338,59 +338,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
> >   *  
>