[PATCH v2 0/3] hw/i386: Cleanup AML generation for north and south bridges

[PATCH v2 0/3] hw/i386: Cleanup AML generation for north and south bridges

[Bernhard Beschow]

This series extends Igor's "x86: clean up ACPI PCI code part 2" series, doing
some further cleanup. It also simplifies experimentation with different south
bridges (PIIX4 and VT82xx) in the pc machine [1].

Testing done:
* `mache check`
* Start a live CD using "pc" and "q35" machine types

v2:
* Rebase onto https://patchew.org/QEMU/20221017102146.2254096-1-imammedo@redhat.com/
* Squash and rework patches 3 and 4

[1] https://github.com/shentok/qemu/tree/pc-via

Based-on: <20221017102146.2254096-1-imammedo@redhat.com>

Bernhard Beschow (3):
  hw/i386/acpi-build: Remove unused struct
  hw/i386/acpi-build: Resolve redundant attribute
  hw/i386/acpi-build: Resolve north rather than south bridges

 hw/i386/acpi-build.c | 28 +++++++---------------------
 1 file changed, 7 insertions(+), 21 deletions(-)

-- 
2.38.1

[PATCH v2 1/3] hw/i386/acpi-build: Remove unused struct

[Bernhard Beschow]

Ammends commit b23046abe78f48498a423b802d6d86ba0172d57f 'pc: acpi-build:
simplify PCI bus tree generation'.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20221026133110.91828-2-shentey@gmail.com>
---
 hw/i386/acpi-build.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 960305462c..1ebf14b899 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -121,13 +121,6 @@ typedef struct AcpiMiscInfo {
     unsigned dsdt_size;
 } AcpiMiscInfo;
 
-typedef struct AcpiBuildPciBusHotplugState {
-    GArray *device_table;
-    GArray *notify_table;
-    struct AcpiBuildPciBusHotplugState *parent;
-    bool pcihp_bridge_en;
-} AcpiBuildPciBusHotplugState;
-
 typedef struct FwCfgTPMConfig {
     uint32_t tpmppi_address;
     uint8_t tpm_version;
-- 
2.38.1

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

[Bernhard Beschow]

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

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

[PATCH v2 3/3] hw/i386/acpi-build: Resolve north rather than south bridges

[Bernhard Beschow]

The code currently assumes Q35 iff ICH9 and i440fx iff PIIX. Now that more
AML generation has been moved into the south bridges and since the
machines define themselves primarily through their north bridges, let's
switch to resolving the north bridges for AML generation instead. This
also allows for easier experimentation with different south bridges in
the "pc" machine, e.g. with PIIX4 and VT82xx.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/acpi-build.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 73d8a59737..d9eaa5fc4d 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -60,6 +60,7 @@
 #include "hw/i386/fw_cfg.h"
 #include "hw/i386/ich9.h"
 #include "hw/pci/pci_bus.h"
+#include "hw/pci-host/i440fx.h"
 #include "hw/pci-host/q35.h"
 #include "hw/i386/x86-iommu.h"
 
@@ -1322,8 +1323,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);
+    Object *i440fx = object_resolve_type_unambiguous(TYPE_I440FX_PCI_HOST_BRIDGE);
+    Object *q35 = object_resolve_type_unambiguous(TYPE_Q35_HOST_DEVICE);
     CrsRangeEntry *entry;
     Aml *dsdt, *sb_scope, *scope, *dev, *method, *field, *pkg, *crs;
     CrsRangeSet crs_range_set;
@@ -1344,13 +1345,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);
+    assert(!!i440fx != !!q35);
 
     acpi_table_begin(&table, table_data);
     dsdt = init_aml_allocator();
 
     build_dbg_aml(dsdt);
-    if (piix) {
+    if (i440fx) {
         sb_scope = aml_scope("_SB");
         dev = aml_device("PCI0");
         aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
@@ -1363,7 +1364,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
             build_x86_acpi_pci_hotplug(dsdt, pm->pcihp_io_base);
         }
         build_piix4_pci0_int(dsdt);
-    } else if (lpc) {
+    } else if (q35) {
         sb_scope = aml_scope("_SB");
         dev = aml_device("PCI0");
         aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));
-- 
2.38.1

Re: [PATCH v2 3/3] hw/i386/acpi-build: Resolve north rather than south bridges

[Ani Sinha]

On Fri, Oct 28, 2022 at 4:05 PM Bernhard Beschow <shentey@gmail.com> wrote:
>
> The code currently assumes Q35 iff ICH9 and i440fx iff PIIX. Now that more
> AML generation has been moved into the south bridges and since the
> machines define themselves primarily through their north bridges, let's
> switch to resolving the north bridges for AML generation instead. This
> also allows for easier experimentation with different south bridges in
> the "pc" machine, e.g. with PIIX4 and VT82xx.

Unfortunately this patch does not apply on the latest master. Also the
code seems to be off. Can you rebase and rework the patch?

>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>  hw/i386/acpi-build.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 73d8a59737..d9eaa5fc4d 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -60,6 +60,7 @@
>  #include "hw/i386/fw_cfg.h"
>  #include "hw/i386/ich9.h"
>  #include "hw/pci/pci_bus.h"
> +#include "hw/pci-host/i440fx.h"
>  #include "hw/pci-host/q35.h"
>  #include "hw/i386/x86-iommu.h"
>
> @@ -1322,8 +1323,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);
> +    Object *i440fx = object_resolve_type_unambiguous(TYPE_I440FX_PCI_HOST_BRIDGE);
> +    Object *q35 = object_resolve_type_unambiguous(TYPE_Q35_HOST_DEVICE);
>      CrsRangeEntry *entry;
>      Aml *dsdt, *sb_scope, *scope, *dev, *method, *field, *pkg, *crs;
>      CrsRangeSet crs_range_set;
> @@ -1344,13 +1345,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);
> +    assert(!!i440fx != !!q35);
>
>      acpi_table_begin(&table, table_data);
>      dsdt = init_aml_allocator();
>
>      build_dbg_aml(dsdt);
> -    if (piix) {
> +    if (i440fx) {
>          sb_scope = aml_scope("_SB");
>          dev = aml_device("PCI0");
>          aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
> @@ -1363,7 +1364,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>              build_x86_acpi_pci_hotplug(dsdt, pm->pcihp_io_base);
>          }
>          build_piix4_pci0_int(dsdt);
> -    } else if (lpc) {
> +    } else if (q35) {
>          sb_scope = aml_scope("_SB");
>          dev = aml_device("PCI0");
>          aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));
> --
> 2.38.1
>

Re: [PATCH v2 3/3] hw/i386/acpi-build: Resolve north rather than south bridges

[B]

Am 28. Oktober 2022 10:58:07 UTC schrieb Ani Sinha <ani@anisinha.ca>:
>On Fri, Oct 28, 2022 at 4:05 PM Bernhard Beschow <shentey@gmail.com> wrote:
>>
>> The code currently assumes Q35 iff ICH9 and i440fx iff PIIX. Now that more
>> AML generation has been moved into the south bridges and since the
>> machines define themselves primarily through their north bridges, let's
>> switch to resolving the north bridges for AML generation instead. This
>> also allows for easier experimentation with different south bridges in
>> the "pc" machine, e.g. with PIIX4 and VT82xx.
>
>Unfortunately this patch does not apply on the latest master. Also the
>code seems to be off. Can you rebase and rework the patch?

I've rebased onto Igor's series to avoid merge conflicts, that's why it doesn't apply onto master. It applies fine there [1].

The first two patches of this series apply fine on both branches, so could possibly be pulled already if Igor's series doesn't make it for 7.2.

Best regards,
Bernhard

[1] https://github.com/patchew-project/qemu/commits/patchew/20221028103419.93398-1-shentey%40gmail.com
>
>>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>  hw/i386/acpi-build.c | 11 ++++++-----
>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 73d8a59737..d9eaa5fc4d 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -60,6 +60,7 @@
>>  #include "hw/i386/fw_cfg.h"
>>  #include "hw/i386/ich9.h"
>>  #include "hw/pci/pci_bus.h"
>> +#include "hw/pci-host/i440fx.h"
>>  #include "hw/pci-host/q35.h"
>>  #include "hw/i386/x86-iommu.h"
>>
>> @@ -1322,8 +1323,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);
>> +    Object *i440fx = object_resolve_type_unambiguous(TYPE_I440FX_PCI_HOST_BRIDGE);
>> +    Object *q35 = object_resolve_type_unambiguous(TYPE_Q35_HOST_DEVICE);
>>      CrsRangeEntry *entry;
>>      Aml *dsdt, *sb_scope, *scope, *dev, *method, *field, *pkg, *crs;
>>      CrsRangeSet crs_range_set;
>> @@ -1344,13 +1345,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);
>> +    assert(!!i440fx != !!q35);
>>
>>      acpi_table_begin(&table, table_data);
>>      dsdt = init_aml_allocator();
>>
>>      build_dbg_aml(dsdt);
>> -    if (piix) {
>> +    if (i440fx) {
>>          sb_scope = aml_scope("_SB");
>>          dev = aml_device("PCI0");
>>          aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
>> @@ -1363,7 +1364,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>>              build_x86_acpi_pci_hotplug(dsdt, pm->pcihp_io_base);
>>          }
>>          build_piix4_pci0_int(dsdt);
>> -    } else if (lpc) {
>> +    } else if (q35) {
>>          sb_scope = aml_scope("_SB");
>>          dev = aml_device("PCI0");
>>          aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));
>> --
>> 2.38.1
>>

Re: [PATCH v2 3/3] hw/i386/acpi-build: Resolve north rather than south bridges

[Ani Sinha]

On Fri, Oct 28, 2022 at 9:45 PM B <shentey@gmail.com> wrote:
>
>
>
> Am 28. Oktober 2022 10:58:07 UTC schrieb Ani Sinha <ani@anisinha.ca>:
> >On Fri, Oct 28, 2022 at 4:05 PM Bernhard Beschow <shentey@gmail.com> wrote:
> >>
> >> The code currently assumes Q35 iff ICH9 and i440fx iff PIIX. Now that more
> >> AML generation has been moved into the south bridges and since the
> >> machines define themselves primarily through their north bridges, let's
> >> switch to resolving the north bridges for AML generation instead. This
> >> also allows for easier experimentation with different south bridges in
> >> the "pc" machine, e.g. with PIIX4 and VT82xx.
> >
> >Unfortunately this patch does not apply on the latest master. Also the
> >code seems to be off. Can you rebase and rework the patch?
>
> I've rebased onto Igor's series to avoid merge conflicts,

Ok I will let Igor deal with this then since I have not followed his patchset.

> that's why it doesn't apply onto master. It applies fine there [1].
>
> The first two patches of this series apply fine on both branches, so could possibly be pulled already if Igor's series doesn't make it for 7.2.
>
> Best regards,
> Bernhard
>
> [1] https://github.com/patchew-project/qemu/commits/patchew/20221028103419.93398-1-shentey%40gmail.com
> >
> >>
> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> >> ---
> >>  hw/i386/acpi-build.c | 11 ++++++-----
> >>  1 file changed, 6 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >> index 73d8a59737..d9eaa5fc4d 100644
> >> --- a/hw/i386/acpi-build.c
> >> +++ b/hw/i386/acpi-build.c
> >> @@ -60,6 +60,7 @@
> >>  #include "hw/i386/fw_cfg.h"
> >>  #include "hw/i386/ich9.h"
> >>  #include "hw/pci/pci_bus.h"
> >> +#include "hw/pci-host/i440fx.h"
> >>  #include "hw/pci-host/q35.h"
> >>  #include "hw/i386/x86-iommu.h"
> >>
> >> @@ -1322,8 +1323,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);
> >> +    Object *i440fx = object_resolve_type_unambiguous(TYPE_I440FX_PCI_HOST_BRIDGE);
> >> +    Object *q35 = object_resolve_type_unambiguous(TYPE_Q35_HOST_DEVICE);
> >>      CrsRangeEntry *entry;
> >>      Aml *dsdt, *sb_scope, *scope, *dev, *method, *field, *pkg, *crs;
> >>      CrsRangeSet crs_range_set;
> >> @@ -1344,13 +1345,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);
> >> +    assert(!!i440fx != !!q35);
> >>
> >>      acpi_table_begin(&table, table_data);
> >>      dsdt = init_aml_allocator();
> >>
> >>      build_dbg_aml(dsdt);
> >> -    if (piix) {
> >> +    if (i440fx) {
> >>          sb_scope = aml_scope("_SB");
> >>          dev = aml_device("PCI0");
> >>          aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
> >> @@ -1363,7 +1364,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >>              build_x86_acpi_pci_hotplug(dsdt, pm->pcihp_io_base);
> >>          }
> >>          build_piix4_pci0_int(dsdt);
> >> -    } else if (lpc) {
> >> +    } else if (q35) {
> >>          sb_scope = aml_scope("_SB");
> >>          dev = aml_device("PCI0");
> >>          aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));
> >> --
> >> 2.38.1
> >>

Re: [PATCH v2 3/3] hw/i386/acpi-build: Resolve north rather than south bridges

[Michael S. Tsirkin]

On Fri, Oct 28, 2022 at 10:18:43PM +0530, Ani Sinha wrote:
> On Fri, Oct 28, 2022 at 9:45 PM B <shentey@gmail.com> wrote:
> >
> >
> >
> > Am 28. Oktober 2022 10:58:07 UTC schrieb Ani Sinha <ani@anisinha.ca>:
> > >On Fri, Oct 28, 2022 at 4:05 PM Bernhard Beschow <shentey@gmail.com> wrote:
> > >>
> > >> The code currently assumes Q35 iff ICH9 and i440fx iff PIIX. Now that more
> > >> AML generation has been moved into the south bridges and since the
> > >> machines define themselves primarily through their north bridges, let's
> > >> switch to resolving the north bridges for AML generation instead. This
> > >> also allows for easier experimentation with different south bridges in
> > >> the "pc" machine, e.g. with PIIX4 and VT82xx.
> > >
> > >Unfortunately this patch does not apply on the latest master. Also the
> > >code seems to be off. Can you rebase and rework the patch?
> >
> > I've rebased onto Igor's series to avoid merge conflicts,
> 
> Ok I will let Igor deal with this then since I have not followed his patchset.

should you want to review this, it's all in my tree right now.

> > that's why it doesn't apply onto master. It applies fine there [1].
> >
> > The first two patches of this series apply fine on both branches, so could possibly be pulled already if Igor's series doesn't make it for 7.2.
> >
> > Best regards,
> > Bernhard
> >
> > [1] https://github.com/patchew-project/qemu/commits/patchew/20221028103419.93398-1-shentey%40gmail.com
> > >
> > >>
> > >> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > >> ---
> > >>  hw/i386/acpi-build.c | 11 ++++++-----
> > >>  1 file changed, 6 insertions(+), 5 deletions(-)
> > >>
> > >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > >> index 73d8a59737..d9eaa5fc4d 100644
> > >> --- a/hw/i386/acpi-build.c
> > >> +++ b/hw/i386/acpi-build.c
> > >> @@ -60,6 +60,7 @@
> > >>  #include "hw/i386/fw_cfg.h"
> > >>  #include "hw/i386/ich9.h"
> > >>  #include "hw/pci/pci_bus.h"
> > >> +#include "hw/pci-host/i440fx.h"
> > >>  #include "hw/pci-host/q35.h"
> > >>  #include "hw/i386/x86-iommu.h"
> > >>
> > >> @@ -1322,8 +1323,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);
> > >> +    Object *i440fx = object_resolve_type_unambiguous(TYPE_I440FX_PCI_HOST_BRIDGE);
> > >> +    Object *q35 = object_resolve_type_unambiguous(TYPE_Q35_HOST_DEVICE);
> > >>      CrsRangeEntry *entry;
> > >>      Aml *dsdt, *sb_scope, *scope, *dev, *method, *field, *pkg, *crs;
> > >>      CrsRangeSet crs_range_set;
> > >> @@ -1344,13 +1345,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);
> > >> +    assert(!!i440fx != !!q35);
> > >>
> > >>      acpi_table_begin(&table, table_data);
> > >>      dsdt = init_aml_allocator();
> > >>
> > >>      build_dbg_aml(dsdt);
> > >> -    if (piix) {
> > >> +    if (i440fx) {
> > >>          sb_scope = aml_scope("_SB");
> > >>          dev = aml_device("PCI0");
> > >>          aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
> > >> @@ -1363,7 +1364,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > >>              build_x86_acpi_pci_hotplug(dsdt, pm->pcihp_io_base);
> > >>          }
> > >>          build_piix4_pci0_int(dsdt);
> > >> -    } else if (lpc) {
> > >> +    } else if (q35) {
> > >>          sb_scope = aml_scope("_SB");
> > >>          dev = aml_device("PCI0");
> > >>          aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));
> > >> --
> > >> 2.38.1
> > >>

Re: [PATCH v2 3/3] hw/i386/acpi-build: Resolve north rather than south bridges

[Ani Sinha]

On Sat, 29 Oct 2022, Michael S. Tsirkin wrote:

> On Fri, Oct 28, 2022 at 10:18:43PM +0530, Ani Sinha wrote:
> > On Fri, Oct 28, 2022 at 9:45 PM B <shentey@gmail.com> wrote:
> > >
> > >
> > >
> > > Am 28. Oktober 2022 10:58:07 UTC schrieb Ani Sinha <ani@anisinha.ca>:
> > > >On Fri, Oct 28, 2022 at 4:05 PM Bernhard Beschow <shentey@gmail.com> wrote:
> > > >>
> > > >> The code currently assumes Q35 iff ICH9 and i440fx iff PIIX. Now that more
> > > >> AML generation has been moved into the south bridges and since the
> > > >> machines define themselves primarily through their north bridges, let's
> > > >> switch to resolving the north bridges for AML generation instead. This
> > > >> also allows for easier experimentation with different south bridges in
> > > >> the "pc" machine, e.g. with PIIX4 and VT82xx.
> > > >
> > > >Unfortunately this patch does not apply on the latest master. Also the
> > > >code seems to be off. Can you rebase and rework the patch?
> > >
> > > I've rebased onto Igor's series to avoid merge conflicts,
> >
> > Ok I will let Igor deal with this then since I have not followed his patchset.
>
> should you want to review this, it's all in my tree right now.

I tried your "next" branch from
git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git

and it does not apply there either.

On another note, seems you have picked up all the bits patches except
the one that adds the documentation. I wonder why.

>
> > > that's why it doesn't apply onto master. It applies fine there [1].
> > >
> > > The first two patches of this series apply fine on both branches, so could possibly be pulled already if Igor's series doesn't make it for 7.2.
> > >
> > > Best regards,
> > > Bernhard
> > >
> > > [1] https://github.com/patchew-project/qemu/commits/patchew/20221028103419.93398-1-shentey%40gmail.com
> > > >
> > > >>
> > > >> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > > >> ---
> > > >>  hw/i386/acpi-build.c | 11 ++++++-----
> > > >>  1 file changed, 6 insertions(+), 5 deletions(-)
> > > >>
> > > >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > >> index 73d8a59737..d9eaa5fc4d 100644
> > > >> --- a/hw/i386/acpi-build.c
> > > >> +++ b/hw/i386/acpi-build.c
> > > >> @@ -60,6 +60,7 @@
> > > >>  #include "hw/i386/fw_cfg.h"
> > > >>  #include "hw/i386/ich9.h"
> > > >>  #include "hw/pci/pci_bus.h"
> > > >> +#include "hw/pci-host/i440fx.h"
> > > >>  #include "hw/pci-host/q35.h"
> > > >>  #include "hw/i386/x86-iommu.h"
> > > >>
> > > >> @@ -1322,8 +1323,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);
> > > >> +    Object *i440fx = object_resolve_type_unambiguous(TYPE_I440FX_PCI_HOST_BRIDGE);
> > > >> +    Object *q35 = object_resolve_type_unambiguous(TYPE_Q35_HOST_DEVICE);
> > > >>      CrsRangeEntry *entry;
> > > >>      Aml *dsdt, *sb_scope, *scope, *dev, *method, *field, *pkg, *crs;
> > > >>      CrsRangeSet crs_range_set;
> > > >> @@ -1344,13 +1345,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);
> > > >> +    assert(!!i440fx != !!q35);
> > > >>
> > > >>      acpi_table_begin(&table, table_data);
> > > >>      dsdt = init_aml_allocator();
> > > >>
> > > >>      build_dbg_aml(dsdt);
> > > >> -    if (piix) {
> > > >> +    if (i440fx) {
> > > >>          sb_scope = aml_scope("_SB");
> > > >>          dev = aml_device("PCI0");
> > > >>          aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
> > > >> @@ -1363,7 +1364,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > > >>              build_x86_acpi_pci_hotplug(dsdt, pm->pcihp_io_base);
> > > >>          }
> > > >>          build_piix4_pci0_int(dsdt);
> > > >> -    } else if (lpc) {
> > > >> +    } else if (q35) {
> > > >>          sb_scope = aml_scope("_SB");
> > > >>          dev = aml_device("PCI0");
> > > >>          aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));
> > > >> --
> > > >> 2.38.1
> > > >>
>
>

Re: [PATCH v2 3/3] hw/i386/acpi-build: Resolve north rather than south bridges

[Michael S. Tsirkin]

On Sun, Oct 30, 2022 at 09:15:44PM +0530, Ani Sinha wrote:
> 
> 
> On Sat, 29 Oct 2022, Michael S. Tsirkin wrote:
> 
> > On Fri, Oct 28, 2022 at 10:18:43PM +0530, Ani Sinha wrote:
> > > On Fri, Oct 28, 2022 at 9:45 PM B <shentey@gmail.com> wrote:
> > > >
> > > >
> > > >
> > > > Am 28. Oktober 2022 10:58:07 UTC schrieb Ani Sinha <ani@anisinha.ca>:
> > > > >On Fri, Oct 28, 2022 at 4:05 PM Bernhard Beschow <shentey@gmail.com> wrote:
> > > > >>
> > > > >> The code currently assumes Q35 iff ICH9 and i440fx iff PIIX. Now that more
> > > > >> AML generation has been moved into the south bridges and since the
> > > > >> machines define themselves primarily through their north bridges, let's
> > > > >> switch to resolving the north bridges for AML generation instead. This
> > > > >> also allows for easier experimentation with different south bridges in
> > > > >> the "pc" machine, e.g. with PIIX4 and VT82xx.
> > > > >
> > > > >Unfortunately this patch does not apply on the latest master. Also the
> > > > >code seems to be off. Can you rebase and rework the patch?
> > > >
> > > > I've rebased onto Igor's series to avoid merge conflicts,
> > >
> > > Ok I will let Igor deal with this then since I have not followed his patchset.
> >
> > should you want to review this, it's all in my tree right now.
> 
> I tried your "next" branch from
> git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git
> 
> and it does not apply there either.


commit 87bbbe87c259414864a02e8385a0c8becd269ea5
It is already applied there.


> On another note, seems you have picked up all the bits patches except
> the one that adds the documentation. I wonder why.

Not sure, I tried to pick them all up. Will check.

> >
> > > > that's why it doesn't apply onto master. It applies fine there [1].
> > > >
> > > > The first two patches of this series apply fine on both branches, so could possibly be pulled already if Igor's series doesn't make it for 7.2.
> > > >
> > > > Best regards,
> > > > Bernhard
> > > >
> > > > [1] https://github.com/patchew-project/qemu/commits/patchew/20221028103419.93398-1-shentey%40gmail.com
> > > > >
> > > > >>
> > > > >> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > > > >> ---
> > > > >>  hw/i386/acpi-build.c | 11 ++++++-----
> > > > >>  1 file changed, 6 insertions(+), 5 deletions(-)
> > > > >>
> > > > >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > >> index 73d8a59737..d9eaa5fc4d 100644
> > > > >> --- a/hw/i386/acpi-build.c
> > > > >> +++ b/hw/i386/acpi-build.c
> > > > >> @@ -60,6 +60,7 @@
> > > > >>  #include "hw/i386/fw_cfg.h"
> > > > >>  #include "hw/i386/ich9.h"
> > > > >>  #include "hw/pci/pci_bus.h"
> > > > >> +#include "hw/pci-host/i440fx.h"
> > > > >>  #include "hw/pci-host/q35.h"
> > > > >>  #include "hw/i386/x86-iommu.h"
> > > > >>
> > > > >> @@ -1322,8 +1323,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);
> > > > >> +    Object *i440fx = object_resolve_type_unambiguous(TYPE_I440FX_PCI_HOST_BRIDGE);
> > > > >> +    Object *q35 = object_resolve_type_unambiguous(TYPE_Q35_HOST_DEVICE);
> > > > >>      CrsRangeEntry *entry;
> > > > >>      Aml *dsdt, *sb_scope, *scope, *dev, *method, *field, *pkg, *crs;
> > > > >>      CrsRangeSet crs_range_set;
> > > > >> @@ -1344,13 +1345,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);
> > > > >> +    assert(!!i440fx != !!q35);
> > > > >>
> > > > >>      acpi_table_begin(&table, table_data);
> > > > >>      dsdt = init_aml_allocator();
> > > > >>
> > > > >>      build_dbg_aml(dsdt);
> > > > >> -    if (piix) {
> > > > >> +    if (i440fx) {
> > > > >>          sb_scope = aml_scope("_SB");
> > > > >>          dev = aml_device("PCI0");
> > > > >>          aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
> > > > >> @@ -1363,7 +1364,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > > > >>              build_x86_acpi_pci_hotplug(dsdt, pm->pcihp_io_base);
> > > > >>          }
> > > > >>          build_piix4_pci0_int(dsdt);
> > > > >> -    } else if (lpc) {
> > > > >> +    } else if (q35) {
> > > > >>          sb_scope = aml_scope("_SB");
> > > > >>          dev = aml_device("PCI0");
> > > > >>          aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));
> > > > >> --
> > > > >> 2.38.1
> > > > >>
> >
> >

Re: [PATCH v2 3/3] hw/i386/acpi-build: Resolve north rather than south bridges

[Ani Sinha]

On Sun, 30 Oct 2022, Michael S. Tsirkin wrote:

> On Sun, Oct 30, 2022 at 09:15:44PM +0530, Ani Sinha wrote:
> >
> >
> > On Sat, 29 Oct 2022, Michael S. Tsirkin wrote:
> >
> > > On Fri, Oct 28, 2022 at 10:18:43PM +0530, Ani Sinha wrote:
> > > > On Fri, Oct 28, 2022 at 9:45 PM B <shentey@gmail.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > Am 28. Oktober 2022 10:58:07 UTC schrieb Ani Sinha <ani@anisinha.ca>:
> > > > > >On Fri, Oct 28, 2022 at 4:05 PM Bernhard Beschow <shentey@gmail.com> wrote:
> > > > > >>
> > > > > >> The code currently assumes Q35 iff ICH9 and i440fx iff PIIX. Now that more
> > > > > >> AML generation has been moved into the south bridges and since the
> > > > > >> machines define themselves primarily through their north bridges, let's
> > > > > >> switch to resolving the north bridges for AML generation instead. This
> > > > > >> also allows for easier experimentation with different south bridges in
> > > > > >> the "pc" machine, e.g. with PIIX4 and VT82xx.
> > > > > >
> > > > > >Unfortunately this patch does not apply on the latest master. Also the
> > > > > >code seems to be off. Can you rebase and rework the patch?
> > > > >
> > > > > I've rebased onto Igor's series to avoid merge conflicts,
> > > >
> > > > Ok I will let Igor deal with this then since I have not followed his patchset.
> > >
> > > should you want to review this, it's all in my tree right now.
> >
> > I tried your "next" branch from
> > git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git
> >
> > and it does not apply there either.
>
>
> commit 87bbbe87c259414864a02e8385a0c8becd269ea5
> It is already applied there.

Hmm, I am not seeing it :

ani@ani-ubuntu:~/workspace/qemu-mst$ git show
87bbbe87c259414864a02e8385a0c8becd269ea5
fatal: bad object 87bbbe87c259414864a02e8385a0c8becd269ea5
ani@ani-ubuntu:~/workspace/qemu-mst$ git branch -vv
  master 7457fe9541 [origin/master] Update version for v1.7.0-rc2 release
* next   e336a0d550 [origin/next] ack! hw/ide/piix: Ignore writes of hardwired PCI command register bits

Re: [PATCH v2 3/3] hw/i386/acpi-build: Resolve north rather than south bridges

[Michael S. Tsirkin]

On Sun, Oct 30, 2022 at 09:48:08PM +0530, Ani Sinha wrote:
> 
> 
> On Sun, 30 Oct 2022, Michael S. Tsirkin wrote:
> 
> > On Sun, Oct 30, 2022 at 09:15:44PM +0530, Ani Sinha wrote:
> > >
> > >
> > > On Sat, 29 Oct 2022, Michael S. Tsirkin wrote:
> > >
> > > > On Fri, Oct 28, 2022 at 10:18:43PM +0530, Ani Sinha wrote:
> > > > > On Fri, Oct 28, 2022 at 9:45 PM B <shentey@gmail.com> wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > Am 28. Oktober 2022 10:58:07 UTC schrieb Ani Sinha <ani@anisinha.ca>:
> > > > > > >On Fri, Oct 28, 2022 at 4:05 PM Bernhard Beschow <shentey@gmail.com> wrote:
> > > > > > >>
> > > > > > >> The code currently assumes Q35 iff ICH9 and i440fx iff PIIX. Now that more
> > > > > > >> AML generation has been moved into the south bridges and since the
> > > > > > >> machines define themselves primarily through their north bridges, let's
> > > > > > >> switch to resolving the north bridges for AML generation instead. This
> > > > > > >> also allows for easier experimentation with different south bridges in
> > > > > > >> the "pc" machine, e.g. with PIIX4 and VT82xx.
> > > > > > >
> > > > > > >Unfortunately this patch does not apply on the latest master. Also the
> > > > > > >code seems to be off. Can you rebase and rework the patch?
> > > > > >
> > > > > > I've rebased onto Igor's series to avoid merge conflicts,
> > > > >
> > > > > Ok I will let Igor deal with this then since I have not followed his patchset.
> > > >
> > > > should you want to review this, it's all in my tree right now.
> > >
> > > I tried your "next" branch from
> > > git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git
> > >
> > > and it does not apply there either.
> >
> >
> > commit 87bbbe87c259414864a02e8385a0c8becd269ea5
> > It is already applied there.
> 
> Hmm, I am not seeing it :
> 
> ani@ani-ubuntu:~/workspace/qemu-mst$ git show
> 87bbbe87c259414864a02e8385a0c8becd269ea5
> fatal: bad object 87bbbe87c259414864a02e8385a0c8becd269ea5
> ani@ani-ubuntu:~/workspace/qemu-mst$ git branch -vv
>   master 7457fe9541 [origin/master] Update version for v1.7.0-rc2 release
> * next   e336a0d550 [origin/next] ack! hw/ide/piix: Ignore writes of hardwired PCI command register bits

oh right. pushed now.

Re: [PATCH v2 3/3] hw/i386/acpi-build: Resolve north rather than south bridges

[Ani Sinha]

On Fri, 28 Oct 2022, Bernhard Beschow wrote:

> The code currently assumes Q35 iff ICH9 and i440fx iff PIIX. Now that more
> AML generation has been moved into the south bridges and since the
> machines define themselves primarily through their north bridges, let's
> switch to resolving the north bridges for AML generation instead. This
> also allows for easier experimentation with different south bridges in
> the "pc" machine, e.g. with PIIX4 and VT82xx.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

Reviewed-by: Ani Sinha <ani@anisinha.ca>

> ---
>  hw/i386/acpi-build.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 73d8a59737..d9eaa5fc4d 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -60,6 +60,7 @@
>  #include "hw/i386/fw_cfg.h"
>  #include "hw/i386/ich9.h"
>  #include "hw/pci/pci_bus.h"
> +#include "hw/pci-host/i440fx.h"
>  #include "hw/pci-host/q35.h"
>  #include "hw/i386/x86-iommu.h"
>
> @@ -1322,8 +1323,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);
> +    Object *i440fx = object_resolve_type_unambiguous(TYPE_I440FX_PCI_HOST_BRIDGE);
> +    Object *q35 = object_resolve_type_unambiguous(TYPE_Q35_HOST_DEVICE);
>      CrsRangeEntry *entry;
>      Aml *dsdt, *sb_scope, *scope, *dev, *method, *field, *pkg, *crs;
>      CrsRangeSet crs_range_set;
> @@ -1344,13 +1345,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);
> +    assert(!!i440fx != !!q35);
>
>      acpi_table_begin(&table, table_data);
>      dsdt = init_aml_allocator();
>
>      build_dbg_aml(dsdt);
> -    if (piix) {
> +    if (i440fx) {
>          sb_scope = aml_scope("_SB");
>          dev = aml_device("PCI0");
>          aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
> @@ -1363,7 +1364,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>              build_x86_acpi_pci_hotplug(dsdt, pm->pcihp_io_base);
>          }
>          build_piix4_pci0_int(dsdt);
> -    } else if (lpc) {
> +    } else if (q35) {
>          sb_scope = aml_scope("_SB");
>          dev = aml_device("PCI0");
>          aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));
> --
> 2.38.1
>
>

Re: [PATCH v2 1/3] hw/i386/acpi-build: Remove unused struct

[Igor Mammedov]

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

> Ammends commit b23046abe78f48498a423b802d6d86ba0172d57f 'pc: acpi-build:
> simplify PCI bus tree generation'.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Message-Id: <20221026133110.91828-2-shentey@gmail.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/i386/acpi-build.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 960305462c..1ebf14b899 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -121,13 +121,6 @@ typedef struct AcpiMiscInfo {
>      unsigned dsdt_size;
>  } AcpiMiscInfo;
>  
> -typedef struct AcpiBuildPciBusHotplugState {
> -    GArray *device_table;
> -    GArray *notify_table;
> -    struct AcpiBuildPciBusHotplugState *parent;
> -    bool pcihp_bridge_en;
> -} AcpiBuildPciBusHotplugState;
> -
>  typedef struct FwCfgTPMConfig {
>      uint32_t tpmppi_address;
>      uint8_t tpm_version;

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

[Igor Mammedov]

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

Re: [PATCH v2 3/3] hw/i386/acpi-build: Resolve north rather than south bridges

[Igor Mammedov]

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

> The code currently assumes Q35 iff ICH9 and i440fx iff PIIX. Now that more
> AML generation has been moved into the south bridges and since the
> machines define themselves primarily through their north bridges, let's
> switch to resolving the north bridges for AML generation instead. This
> also allows for easier experimentation with different south bridges in
> the "pc" machine, e.g. with PIIX4 and VT82xx.

Patch looks fine to me in a sense that either would work.

But the commit message lacks clear answer to 'why'
and what issues it resolves or would resolve/make
our easier life down to road.

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>  hw/i386/acpi-build.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 73d8a59737..d9eaa5fc4d 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -60,6 +60,7 @@
>  #include "hw/i386/fw_cfg.h"
>  #include "hw/i386/ich9.h"
>  #include "hw/pci/pci_bus.h"
> +#include "hw/pci-host/i440fx.h"
>  #include "hw/pci-host/q35.h"
>  #include "hw/i386/x86-iommu.h"
>  
> @@ -1322,8 +1323,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);
> +    Object *i440fx = object_resolve_type_unambiguous(TYPE_I440FX_PCI_HOST_BRIDGE);
> +    Object *q35 = object_resolve_type_unambiguous(TYPE_Q35_HOST_DEVICE);
>      CrsRangeEntry *entry;
>      Aml *dsdt, *sb_scope, *scope, *dev, *method, *field, *pkg, *crs;
>      CrsRangeSet crs_range_set;
> @@ -1344,13 +1345,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);
> +    assert(!!i440fx != !!q35);
>  
>      acpi_table_begin(&table, table_data);
>      dsdt = init_aml_allocator();
>  
>      build_dbg_aml(dsdt);
> -    if (piix) {
> +    if (i440fx) {
>          sb_scope = aml_scope("_SB");
>          dev = aml_device("PCI0");
>          aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
> @@ -1363,7 +1364,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>              build_x86_acpi_pci_hotplug(dsdt, pm->pcihp_io_base);
>          }
>          build_piix4_pci0_int(dsdt);
> -    } else if (lpc) {
> +    } else if (q35) {
>          sb_scope = aml_scope("_SB");
>          dev = aml_device("PCI0");
>          aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));

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

[Michael S. Tsirkin]

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

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