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.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.
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
>>> *
