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

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.

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(-)

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")));

On Mon, Oct 31, 2022 at 01:45:29PM +0100, Igor Mammedov wrote:
> 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.

Well I picked this up for pull req since i had to make
a decision before freeze.  But sure, a cleanup like
this might even be acceptable before rc1 I think.
Bernhard?

> 
> > 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")));

Am 31. Oktober 2022 17:41:59 UTC schrieb "Michael S. Tsirkin" <mst@redhat.com>:
>On Mon, Oct 31, 2022 at 01:45:29PM +0100, Igor Mammedov wrote:
>> 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.
>
>Well I picked this up for pull req since i had to make
>a decision before freeze.  But sure, a cleanup like
>this might even be acceptable before rc1 I think.
>Bernhard?

In my pc-via branch which I have linked to in the cover letter I'm adding support for the VT82xx south bridges to the "pc" machine (currently hardcoded, should be configurable in the future). There I'm adding support for a third pm controller in acpi_get_pm_info() [1]. So a boolean is_piix4 is not sufficient any longer.

Furthermore, build_dsdt() does have code which is specific to the north bridges. So
I think it makes sense to distinguish between the pm controllers and the north bridges. Otherwise one would make assumptions that certain north and south bridges always go together, which gets into the way of experimenting with (and possibly upstreaming support for) new south bridges in the "pc" machine.

I hope that I decently brought across my motivation. Let me know if you have any further questions.

Best regards,
Bernhard

[1] https://github.com/shentok/qemu/commit/4815ae28788ce249cc30d7a1531805c5988ffcf6

>
>> 
>> > 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")));
>