Re: [RFC PATCH 3/5] hw/i386/acpi-build: Add ACPI PCI hot-plug methods to q35

[Michael S. Tsirkin]

On Mon, Jul 13, 2020 at 04:39:54PM +0200, Igor Mammedov wrote:
> On Thu,  9 Jul 2020 00:46:13 +0200
> Julia Suvorova <jusual@redhat.com> wrote:
> 
> > Implement notifications and gpe to support q35 ACPI PCI hot-plug.
> > The addresses specified in [1] remain the same to make fewer changes.
> > 
> > [1] docs/spec/acpi_pci_hotplug.txt
> 
> CCing Gerd his opinion on reusing piix4 IO port range for q35
> 
>  
> > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > ---
> >  hw/i386/acpi-build.c | 20 +++++++++++++-------
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 11c598f955..5c5ad88ad6 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -201,10 +201,6 @@ static void acpi_get_pm_info(MachineState *machine, 
> > AcpiPmInfo *pm)
> >          /* w2k requires FADT(rev1) or it won't boot, keep PC compatible */
> >          pm->fadt.rev = 1;
> >          pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
> > -        pm->pcihp_io_base =
> > -            object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
> > -        pm->pcihp_io_len =
> > -            object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
> >      }
> >      if (lpc) {
> >          struct AcpiGenericAddress r = { .space_id = AML_AS_SYSTEM_IO,
> > @@ -214,6 +210,10 @@ static void acpi_get_pm_info(MachineState *machine, 
> > AcpiPmInfo *pm)
> >          pm->fadt.flags |= 1 << ACPI_FADT_F_RESET_REG_SUP;
> >          pm->cpu_hp_io_base = ICH9_CPU_HOTPLUG_IO_BASE;
> >      }
> > +    pm->pcihp_io_base =
> > +        object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
> > +    pm->pcihp_io_len =
> > +        object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
> >  
> >      /* The above need not be conditional on machine type because the reset 
> > port
> >       * happens to be the same on PIIX (pc) and ICH9 (q35). */
> > @@ -472,7 +472,7 @@ static void build_append_pci_bus_devices(Aml 
> > *parent_scope, PCIBus *bus,
> >          QLIST_FOREACH(sec, &bus->child, sibling) {
> >              int32_t devfn = sec->parent_dev->devfn;
> >  
> > -            if (pci_bus_is_root(sec) || pci_bus_is_express(sec)) {
> > +            if (pci_bus_is_root(sec)) {
> >                  continue;
> >              }
> >  
> > @@ -1586,7 +1586,12 @@ static void build_piix4_pci_hotplug(Aml *table)
> >      aml_append(table, scope);
> >  }
> >  
> > -static Aml *build_q35_osc_method(void)
> > +static void build_q35_pci_hotplug(Aml *table)
> > +{
> > +    build_piix4_pci_hotplug(table);
> > +}
> 
> s/build_piix4_pci_hotplug/build_i386_acpi_pci_hotplug/
> 
> and reuse it in both cases, instead of adding wrapper?

I'm not sure about that - we have microvm too ...

> > +
> > +static Aml *build_q35_osc_method(AcpiPmInfo *pm)
> >  {
> >      Aml *if_ctx;
> >      Aml *if_ctx2;
> > @@ -1698,6 +1703,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >          build_hpet_aml(dsdt);
> >          build_q35_isa_bridge(dsdt);
> >          build_isa_devices_aml(dsdt);
> > +        build_q35_pci_hotplug(dsdt);
> >          build_q35_pci0_int(dsdt);
> >          if (pcms->smbus && !pcmc->do_not_add_smb_acpi) {
> >              build_smb0(dsdt, pcms->smbus, ICH9_SMB_DEV, ICH9_SMB_FUNC);
> > @@ -1724,7 +1730,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >      {
> >          aml_append(scope, aml_name_decl("_HID", aml_string("ACPI0006")));
> >  
> > -        if (misc->is_piix4) {
> > +        if (misc->is_piix4 || pm->pcihp_bridge_en) {
> >              method = aml_method("_E01", 0, AML_NOTSERIALIZED);
> >              aml_append(method,
> >                  aml_acquire(aml_name("\\_SB.PCI0.BLCK"), 0xFFFF));