On Fri, 20 Jun 2025 10:19:36 +0100
Jonathan Cameron <jonathan.came...@huawei.com> wrote:
> On Mon, 16 Jun 2025 11:46:45 +0200
> Eric Auger <eric.au...@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.au...@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
> > *
>
