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

[Bernhard Beschow]

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