Re: [PATCH v2 11/25] hw/i386/acpi-build: Introduce build_append_pcihp_resources() helper

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

Hi Jonathan,

On 5/30/25 12:17 PM, Jonathan Cameron wrote:
> On Tue, 27 May 2025 09:40:13 +0200
> Eric Auger <eric.auger@redhat.com> wrote:
>
>> Extract the code that reserves resources for ACPI PCI hotplug
>> into a new helper named build_append_pcihp_resources() and
>> move it to pcihp.c. We will reuse it on ARM.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>
> Trivial comment inline.
>
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
>
>> ---
>>  include/hw/acpi/pcihp.h |  2 ++
>>  hw/acpi/pcihp.c         | 20 ++++++++++++++++++++
>>  hw/i386/acpi-build.c    | 15 ++-------------
>>  3 files changed, 24 insertions(+), 13 deletions(-)
>>
>> diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
>> index 971451e8ea..8a46a414cc 100644
>> --- a/include/hw/acpi/pcihp.h
>> +++ b/include/hw/acpi/pcihp.h
>> @@ -75,6 +75,8 @@ void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>>  
>>  void build_acpi_pci_hotplug(Aml *table, uint64_t pcihp_addr);
>>  void build_append_pci_dsm_func0_common(Aml *ctx, Aml *retvar);
>> +void build_append_pcihp_resources(Aml *table,
>> +                                  uint64_t io_addr, uint64_t io_len);
>>  
>>  /* Called on reset */
>>  void acpi_pcihp_reset(AcpiPciHpState *s);
>> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
>> index e0260f67e6..fb54c31f77 100644
>> --- a/hw/acpi/pcihp.c
>> +++ b/hw/acpi/pcihp.c
>> @@ -685,6 +685,26 @@ void build_acpi_pci_hotplug(Aml *table, uint64_t pcihp_addr)
>>      aml_append(table, scope);
>>  }
>>  
>> +/* Reserve PCIHP resources */
>> +void build_append_pcihp_resources(Aml *scope /* \\_SB.PCI0 */,
>> +                                  uint64_t io_addr, uint64_t io_len)
>> +{
>> +    Aml *dev, *crs;
>> +
>> +    dev = aml_device("PHPR");
>> +    aml_append(dev, aml_name_decl("_HID", aml_string("PNP0A06")));
>> +    aml_append(dev,
>> +               aml_name_decl("_UID", aml_string("PCI Hotplug resources")));
>> +    /* device present, functioning, decoding, not shown in UI */
>> +    aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
>> +    crs = aml_resource_template();
>> +    aml_append(crs,
>> +        aml_io(AML_DECODE16, io_addr, io_addr, 1, io_len)
>> +    );
> Trivial but this doesn't match local style.  It is even inconsistent with
> the _UID line above.
>> +    aml_append(crs,
>> +               aml_io(AML_DECODE16, io_addr, io_addr, 1, io_len));
> maybe?

definivitely. Will be fixed in next version

Thanks!

Eric
>
>> +    aml_append(dev, aml_name_decl("_CRS", crs));
>> +    aml_append(scope, dev);
>> +}
>> +
>>  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 91945f716c..52cef834ed 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -1432,19 +1432,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>>  
>>      /* reserve PCIHP resources */
>>      if (pm->pcihp_io_len && (pm->pcihp_bridge_en || pm->pcihp_root_en)) {
>> -        dev = aml_device("PHPR");
>> -        aml_append(dev, aml_name_decl("_HID", aml_string("PNP0A06")));
>> -        aml_append(dev,
>> -            aml_name_decl("_UID", aml_string("PCI Hotplug resources")));
>> -        /* device present, functioning, decoding, not shown in UI */
>> -        aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
>> -        crs = aml_resource_template();
>> -        aml_append(crs,
>> -            aml_io(AML_DECODE16, pm->pcihp_io_base, pm->pcihp_io_base, 1,
>> -                   pm->pcihp_io_len)
>> -        );
>> -        aml_append(dev, aml_name_decl("_CRS", crs));
>> -        aml_append(scope, dev);
>> +        build_append_pcihp_resources(scope,
>> +                                      pm->pcihp_io_base, pm->pcihp_io_len);
>>      }
>>      aml_append(dsdt, scope);
>>