Re: [PATCH v2 2/3] hw/i386/acpi-build: Resolve redundant attribute

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 28 Oct 2022 12:34:18 +0200
Bernhard Beschow <shentey@gmail.com> wrote:

> The is_piix4 attribute is set once in one location and read once in
> another. Doing both in one location allows for removing the attribute
> altogether.

we also test for piix4 in acpi_get_pm_info(),
Perhaps we should move is_piix4 to AcpiPmInfo
and reuse it in build_dsdt()?

Also should use is_piix4 as argument to build_iqcr_method()
to make its call places self documenting. But that's another patch.

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Message-Id: <20221026133110.91828-3-shentey@gmail.com>
> ---
>  hw/i386/acpi-build.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 1ebf14b899..73d8a59737 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -112,7 +112,6 @@ typedef struct AcpiPmInfo {
>  } AcpiPmInfo;
>  
>  typedef struct AcpiMiscInfo {
> -    bool is_piix4;
>      bool has_hpet;
>  #ifdef CONFIG_TPM
>      TPMVersion tpm_version;
> @@ -281,17 +280,6 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
>  
>  static void acpi_get_misc_info(AcpiMiscInfo *info)
>  {
> -    Object *piix = object_resolve_type_unambiguous(TYPE_PIIX4_PM);
> -    Object *lpc = object_resolve_type_unambiguous(TYPE_ICH9_LPC_DEVICE);
> -    assert(!!piix != !!lpc);
> -
> -    if (piix) {
> -        info->is_piix4 = true;
> -    }
> -    if (lpc) {
> -        info->is_piix4 = false;
> -    }

ack for this hunk

>      info->has_hpet = hpet_find();
>  #ifdef CONFIG_TPM
>      info->tpm_version = tpm_get_version(tpm_find());
> @@ -1334,6 +1322,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>             AcpiPmInfo *pm, AcpiMiscInfo *misc,
>             Range *pci_hole, Range *pci_hole64, MachineState *machine)
>  {
> +    Object *piix = object_resolve_type_unambiguous(TYPE_PIIX4_PM);
> +    Object *lpc = object_resolve_type_unambiguous(TYPE_ICH9_LPC_DEVICE);
>      CrsRangeEntry *entry;
>      Aml *dsdt, *sb_scope, *scope, *dev, *method, *field, *pkg, *crs;
>      CrsRangeSet crs_range_set;
> @@ -1354,11 +1344,13 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>      AcpiTable table = { .sig = "DSDT", .rev = 1, .oem_id = x86ms->oem_id,
>                          .oem_table_id = x86ms->oem_table_id };
>  
> +    assert(!!piix != !!lpc);
> +
>      acpi_table_begin(&table, table_data);
>      dsdt = init_aml_allocator();
>  
>      build_dbg_aml(dsdt);
> -    if (misc->is_piix4) {
> +    if (piix) {
>          sb_scope = aml_scope("_SB");
>          dev = aml_device("PCI0");
>          aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
> @@ -1371,7 +1363,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>              build_x86_acpi_pci_hotplug(dsdt, pm->pcihp_io_base);
>          }
>          build_piix4_pci0_int(dsdt);
> -    } else {
> +    } else if (lpc) {
>          sb_scope = aml_scope("_SB");
>          dev = aml_device("PCI0");
>          aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));