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

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 14 Jul 2020 05:22:20 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> 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 ...
it doesn't have pci if I'm not mistaken,
and when/if it gains one someday, it may or maynot use hotplug
it will be up to its specific DSDT to include this call.

What doesn't make sense is using board specific naming
here for the same code. I don't insist on the variant I've suggested,
only on consolidating it instead of adding dummy wrapper


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