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

[Eric Auger <eric.auger@redhat.com>]

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.auger@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
>