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

[Gustavo Romero <gustavo.romero@linaro.org>]

Hi Eric,

On 5/27/25 04:18, Eric Auger wrote:
> 
> 
> On 5/21/25 10:19 PM, Gustavo Romero wrote:
>> Hi Eric,
>>
>> On 5/21/25 13:24, Gustavo Romero wrote:
>>> Hi Eric,
>>>
>>> On 5/21/25 12:56, Eric Auger wrote:
>>>> 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.
>>>
>>> Yes, but I meant in the final generated ACPI AML code.
>>>
>>>
>>>> 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?
>>>
>>> Maybe some condition in build_append_pci_bus_devices() of:
>>>
>>>           if (pdev->acpi_index &&
>>>               !object_property_get_bool(OBJECT(pdev), "hotpluggable",
>>>                                         &error_abort)) { ... }
>>>
>>>
>>> is not met?
>>>
>>> Does your _DSM method in the machine's ACPI has a call to EDSM? I don't
>>> see it. In my case it's the PDSM that is being called:
>>>
>>> gromero@xps13:/tmp$ grep -n PDSM dsdt.dsl
>>> 1910:        Method (PDSM, 5, Serialized)
>>> 2047:                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, Local0))
>>> gromero@xps13:/tmp$ grep -n EDSM dsdt.dsl
>>> 1959:        Method (EDSM, 5, Serialized)
>>> gromero@xps13:/tmp$
>>>
>>> I also confirmed that PDSM is what is being used by reverting this
>>> patch and
>>> dropping the aml_append(pci0_scope, aml_pci_edsm()); in 15/22.
>>>
>>> But also, why shouldn't we use the PDSM defined already in pcihp.c?
>>> This is
>>> indeed crafted for the acpi-pcihp-hotplug device as I understand it.
>>
>> Sorry, I see now, the EDSM is actually only for static endpoint PCI
>> devices,
>> i.e. non-hotpluggable, I see. So PDSM and EDSM serve different
>> purposes, although
>> similar.
>>
>> So, my point in the end is (and the one that confused me too) that
>> EDSM can be
>> present even if it will never be used by any static device, working
>> pretty much
>> like an ACPI deadcode. Could we avoid this? For instance, just emit
>> EDSM if
>> a _DSM that relies on it is emitted, like:
> 
> I see your point and it's true that for some PCIe configs the code can
> be there while not used. However it looks it is done the same way on x86
> and I would tend to be not more popish than the pope ;-)
> 
> Maybe this can be a patch sent afterwards that would also address the
> x86 case then.

Got it. Well, I think it's just a matter of making build_append_pci_bus_devices()
tell if any static endpoint is added and just call aml_pci_edsm() to emit EDSM if
there is any, like in my diff:

+        /* Generate tables for static endpoints. */
+        if (build_append_pci_bus_devices(pci0_scope, vms->bus)) {
+            /* Has a _DSM method that requires a EDSM method. */
+            aml_append(pci0_scope, aml_pci_edsm());
+        }

But I'm not really the maintainer or the official reviewer for this part of
the code, and you certainly have more karma than I do in this area, so it's
up to you, of course. I just think ACPI code is tricky enough to review,
and having dead code makes it a tad more daunting :)


>>
>> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
>> index e87846a1fa..4d6dd3dd96 100644
>> --- a/hw/acpi/pcihp.c
>> +++ b/hw/acpi/pcihp.c
>> @@ -905,8 +905,9 @@ void build_append_pcihp_slots(Aml *parent_scope,
>> PCIBus *bus)
>>       aml_append(parent_scope, notify_method);
>>   }
>>   
>> -void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus)
>> +bool build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus)
>>   {
>> +    bool has_dsm = false;
>>       int devfn;
>>       Aml *dev;
>>   
>> @@ -929,11 +930,14 @@ void build_append_pci_bus_devices(Aml
>> *parent_scope, PCIBus *bus)
>>               !object_property_get_bool(OBJECT(pdev), "hotpluggable",
>>                                         &error_abort)) {
>>               aml_append(dev, aml_pci_static_endpoint_dsm(pdev));
>> +            has_dsm = true;
>>           }
>>   
>>           /* device descriptor has been composed, add it into parent
>> context */
>>           aml_append(parent_scope, dev);
>>       }
>> +
>> +    return has_dsm;
>>   }
>>   
>>   Aml *aml_pci_edsm(void)
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 05a754d368..e4788c03ed 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -844,15 +844,22 @@ build_dsdt(GArray *table_data, BIOSLinker
>> *linker, VirtMachineState *vms)
>>       if (vms->acpi_pcihp) {
>>           Aml *pci0_scope = aml_scope("\\_SB.PCI0");
>>   
>> -        aml_append(pci0_scope, aml_pci_edsm());
>>           build_acpi_pci_hotplug(dsdt, AML_SYSTEM_MEMORY,
>>                                  memmap[VIRT_ACPI_PCIHP].base);
>>           build_append_pcihp_resources(pci0_scope,
>>                                        memmap[VIRT_ACPI_PCIHP].base,
>>                                        memmap[VIRT_ACPI_PCIHP].size);
>>   
>> -        /* Scan all PCI buses. Generate tables to support hotplug. */
>> -        build_append_pci_bus_devices(pci0_scope, vms->bus);
>> +        /*
>> +         * Scan all PCI buses and devices. Generate tables to support
>> static
>> +         * endpoint devices and hotplug.
>> +         */
>> +        /* Generate tables for static endpoints. */
>> +        if (build_append_pci_bus_devices(pci0_scope, vms->bus)) {
>> +            /* Has a _DSM method that requires a EDSM method. */
>> +            aml_append(pci0_scope, aml_pci_edsm());
>> +        }
>> +        /* Generate tables for PCI buses/slots/ */
>>           if (object_property_find(OBJECT(vms->bus),
>> ACPI_PCIHP_PROP_BSEL)) {
>>               build_append_pcihp_slots(pci0_scope, vms->bus);
>>           }
>> diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
>> index bda5ea24b5..25cc39f4ab 100644
>> --- a/include/hw/acpi/pcihp.h
>> +++ b/include/hw/acpi/pcihp.h
>> @@ -82,7 +82,7 @@ void build_append_pcihp_resources(Aml *table,
>>                                     uint64_t io_addr, uint64_t io_len);
>>   bool build_append_notification_callback(Aml *parent_scope, const
>> PCIBus *bus);
>>   
>> -void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus);
>> +bool build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus);
>>   
>>   Aml *aml_pci_edsm(void);
>>
>>
>> Another question that I have is: could you please share a QEMU command
>> line option
>> that generates a static endpoint PCI device that matches the criteria
>> for the emission
>> of the _DSM method accordingly to build_append_pci_bus_devices()?
> You can instantiate a virtio-net-pci device on pci.0 and with acpi-index
> set.
> 
> -device
> virtio-net-pci,bus=pcie.0,netdev=nic0,mac=6a:f5:10:b1:3d:d2,acpi-index=12
> 
> In that case EDSM is being called:
> 
>          Device (S08)
>          {
>              Name (_ADR, 0x00010000)  // _ADR: Address
>              Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
>              {
>                  Local0 = Package (0x01)
>                      {
>                          0x0C
>                      }
>                  Return (EDSM (Arg0, Arg1, Arg2, Arg3, Local0))
>              }
>          }
> 
> Putting it on a root port will let the EDSM unused.

Thanks for the example. Would it be correct to say so that EDSM is part of
the coldplug and so could be left for another series, so out of scope for
hotpluging?


Cheers,
Gustavo