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