Re: [PATCH 9/9] piix4: don't reserve hw resources when hotplug is off globally

[Ani Sinha]

On Tue, Sep 15, 2020 at 7:33 PM Ani Sinha <ani@anisinha.ca> wrote:
>
> On Tue, Sep 15, 2020 at 7:12 PM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Tue, 15 Sep 2020 18:25:55 +0530
> > Ani Sinha <ani@anisinha.ca> wrote:
> >
> > > On Tue, Sep 15, 2020 at 6:19 PM Igor Mammedov <imammedo@redhat.com> wrote:
> > > >
> > > > On Tue, 15 Sep 2020 17:40:25 +0530
> > > > Ani Sinha <ani@anisinha.ca> wrote:
> > > >
> > > > > On Tue, Sep 15, 2020 at 5:21 PM Igor Mammedov <imammedo@redhat.com> 
> > > > > wrote:
> > > > > >
> > > > > > On Mon, 14 Sep 2020 20:20:21 +0530
> > > > > > Ani Sinha <ani@anisinha.ca> wrote:
> > > > > >
> > > > > > > On Mon, Sep 14, 2020 at 7:39 PM Igor Mammedov 
> > > > > > > <imammedo@redhat.com> wrote:
> > > > > > > >
> > > > > > > > On Mon, 14 Sep 2020 18:58:19 +0530
> > > > > > > > Ani Sinha <ani@anisinha.ca> wrote:
> > > > > > > >
> > > > > > > > > On Mon, Sep 14, 2020 at 6:40 PM Ani Sinha <ani@anisinha.ca> 
> > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > On Mon, Sep 14, 2020 at 6:36 PM Igor Mammedov 
> > > > > > > > > > <imammedo@redhat.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > On Fri, 11 Sep 2020 23:37:55 +0530
> > > > > > > > > > > Ani Sinha <ani@anisinha.ca> wrote:
> > > > > > > > > > >
> > > > > > > > > > > > When acpi hotplug is turned off for both root pci bus 
> > > > > > > > > > > > as well as for pci
> > > > > > > > > > > > bridges, we should not generate the related amls for 
> > > > > > > > > > > > DSDT table or initialize
> > > > > > > > > > > > related hw ports or reserve hw resources. This change 
> > > > > > > > > > > > makes sure all those
> > > > > > > > > > > > operations are turned off in the case acpi pci hotplug 
> > > > > > > > > > > > is off globally.
> > > > > > > > > > >
> > > > > > > > > > > it still leaves around pure PCI hotplug ACPI code:
> > > > > > > > > > >
> > > > > > > > > > >             Method (PCNT, 0, NotSerialized)
> > > > > > > > > > >             {
> > > > > > > > > > >             }
> > > > > > > > > >
> > > > > > > > > > How do you suggest we fix this?
> > > > > > > > >
> > > > > > > > > One way to fix this would be to do this:
> > > > > > > > >
> > > > > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > > > > > > index 7e3cf3b57b..2bd0c37143 100644
> > > > > > > > > --- a/hw/i386/acpi-build.c
> > > > > > > > > +++ b/hw/i386/acpi-build.c
> > > > > > > > > @@ -483,7 +483,8 @@ static void 
> > > > > > > > > build_append_pci_bus_devices(Aml
> > > > > > > > > *parent_scope, PCIBus *bus,
> > > > > > > > >              aml_append(method, aml_name("^S%.02X.PCNT", 
> > > > > > > > > devfn));
> > > > > > > > >          }
> > > > > > > > >      }
> > > > > > > > > -    aml_append(parent_scope, method);
> > > > > > > > > +    if (bsel)
> > > > > > > > > +        aml_append(parent_scope, method);
> > > > > >
> > > > > > and this also introduces mem leak
> > > > > >
> > > > > >
> > > > > > > > >      qobject_unref(bsel);
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > This means that if the hotplug on the root bus is disabled, 
> > > > > > > > > we also
> > > > > > > > > disable PCNT. This will also need the unit test update as the 
> > > > > > > > > unit
> > > > > > > > Does bridge hot-plug still work if we disable it only on root 
> > > > > > > > bus?
> > > > > > >
> > > > > > > Just did a quick test. Windows seems to eject devices on the 
> > > > > > > bridge
> > > > > > > even when the root bus hot plug is off.
> > > > > >
> > > > > > well I'm not sure it works using ACPI hotplug,
> > > > > >
> > > > > > with
> > > > > > -global PIIX4_PM.acpi-root-pci-hotplug=off
> > > > > > -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=on
> > > > > >
> > > > > > DSDT ins't what I'd expect when bridge hp is on:
> > > > > >
> > > > > >     Scope (\_SB)
> > > > > >     {
> > > > > >         Scope (PCI0)
> > > > > >         {
> > > > > >             Device (S00)
> > > > > >             {
> > > > > >                 Name (_ADR, Zero)  // _ADR: Address
> > > > > >             }
> > > > > >
> > > > > >             Device (S08)
> > > > >
> > > > > I think you should look for AMLs for Device (S18) because the bridge
> > > > > goes into slot 3.
> > > >
> > > > as you see, there weren't any here. so maybe something else gone wrong
> > > > in earlier patches
> > >
> > > Can you send the full command line you are passing? You need to attach
> > > a bridge, then the function will recurse into it.
> >
> > all your 9 pathes with this on top:
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 7e3cf3b57b..8339a3538a 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -483,7 +483,8 @@ static void build_append_pci_bus_devices(Aml 
> > *parent_scope, PCIBus *bus,
> >              aml_append(method, aml_name("^S%.02X.PCNT", devfn));
> >          }
> >      }
> > -    aml_append(parent_scope, method);
> > +    if (bsel)
> > +       aml_append(parent_scope, method);
> >      qobject_unref(bsel);
> >  }
> >
> > diff --git a/slirp b/slirp
> > --- a/slirp
> > +++ b/slirp
> > @@ -1 +1 @@
> > -Subproject commit ce94eba2042d52a0ba3d9e252ebce86715e94275
> > +Subproject commit ce94eba2042d52a0ba3d9e252ebce86715e94275-dirty
> > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> > index 7632cfe1be..fb56c1eeb0 100644
> > --- a/tests/qtest/bios-tables-test.c
> > +++ b/tests/qtest/bios-tables-test.c
> > @@ -750,7 +750,7 @@ static void test_acpi_piix4_root_hotplug(void)
> >      data.required_struct_types = base_required_struct_types;
> >      data.required_struct_types_len = 
> > ARRAY_SIZE(base_required_struct_types);
> >      test_acpi_one("-global PIIX4_PM.acpi-root-pci-hotplug=off "
> > -                  "-device pci-bridge,chassis_nr=1", &data);
> > +                  "-device pci-bridge,chassis_nr=1 -global 
> > PIIX4_PM.acpi-pci-hotplug-with-bridge-support=on", &data);
>
> That's a noop since by default its ON.
>
> I ran the test and it passed. The disassembly of
> tests/data/acpi/pc/DSDT.roothp is here:
>
> https://pastebin.ubuntu.com/p/52VS6HRPx8/
>
> Seems OK to me. The table for the bridge slots starts from line 871.

I just sent v2 for the entire patch set. I have updated patch #7 and
patch #9. Can you try again with the updated patch #9? I think it's OK
now. Maybe the previous patch had an issue.

>
> >      free_test_data(&data);
> >  }
> >
> >
> > and run test with:
> >
> > make -j32 && make tests/qtest/bios-tables-test && V=1 
> > QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 QTEST_QEMU_IMG=qemu-img 
> > tests/qtest/bios-tables-test
> >
> > >
> > > >
> > > > >
> > > > > >             {
> > > > > >                 Name (_ADR, 0x00010000)  // _ADR: Address
> > > > > >                 Method (_S1D, 0, NotSerialized)  // _S1D: S1 Device 
> > > > > > State
> > > > > >                 {
> > > > > >                     Return (Zero)
> > > > > >                 }
> > > > > >
> > > > > >                 Method (_S2D, 0, NotSerialized)  // _S2D: S2 Device 
> > > > > > State
> > > > > >                 {
> > > > > >                     Return (Zero)
> > > > > >                 }
> > > > > >
> > > > > >                 Method (_S3D, 0, NotSerialized)  // _S3D: S3 Device 
> > > > > > State
> > > > > >                 {
> > > > > >                     Return (Zero)
> > > > > >                 }
> > > > > >             }
> > > > > >         }
> > > > > >     }
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > > test
> > > > > > > > > will fail with :
> > > > > > > > >
> > > > > > > > > @@ -3113,24 +3113,20 @@
> > > > > > > > >                  Name (_ADR, 0x00010000)  // _ADR: Address
> > > > > > > > >                  Method (_S1D, 0, NotSerialized)  // _S1D: S1 
> > > > > > > > > Device State
> > > > > > > > >                  {
> > > > > > > > >                      Return (Zero)
> > > > > > > > >                  }
> > > > > > > > >
> > > > > > > > >                  Method (_S2D, 0, NotSerialized)  // _S2D: S2 
> > > > > > > > > Device State
> > > > > > > > >                  {
> > > > > > > > >                      Return (Zero)
> > > > > > > > >                  }
> > > > > > > > >
> > > > > > > > >                  Method (_S3D, 0, NotSerialized)  // _S3D: S3 
> > > > > > > > > Device State
> > > > > > > > >                  {
> > > > > > > > >                      Return (Zero)
> > > > > > > > >                  }
> > > > > > > > >              }
> > > > > > > > > -
> > > > > > > > > -            Method (PCNT, 0, NotSerialized)
> > > > > > > > > -            {
> > > > > > > > > -            }
> > > > > > > > >          }
> > > > > > > > >      }
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > Let's fix this as a separate patch.
> > > > > > > >
> > > > > > > > I'd rather fix up this patch, so it would do what it claims.
> > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > > > > > > > > > > > ---
> > > > > > > > > > > >  hw/acpi/piix4.c      |  6 ++++--
> > > > > > > > > > > >  hw/i386/acpi-build.c | 10 ++++++++--
> > > > > > > > > > > >  2 files changed, 12 insertions(+), 4 deletions(-)
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > > > > > > > > > > > index e6163bb6ce..b70b1f98af 100644
> > > > > > > > > > > > --- a/hw/acpi/piix4.c
> > > > > > > > > > > > +++ b/hw/acpi/piix4.c
> > > > > > > > > > > > @@ -596,8 +596,10 @@ static void 
> > > > > > > > > > > > piix4_acpi_system_hot_add_init(MemoryRegion *parent,
> > > > > > > > > > > >                            "acpi-gpe0", GPE_LEN);
> > > > > > > > > > > >      memory_region_add_subregion(parent, GPE_BASE, 
> > > > > > > > > > > > &s->io_gpe);
> > > > > > > > > > > >
> > > > > > > > > > > > -    acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, 
> > > > > > > > > > > > bus, parent,
> > > > > > > > > > > > -                    s->use_acpi_hotplug_bridge);
> > > > > > > > > > > > +    if (s->use_acpi_hotplug_bridge || 
> > > > > > > > > > > > s->use_acpi_root_pci_hotplug) {
> > > > > > > > > > > > +        acpi_pcihp_init(OBJECT(s), 
> > > > > > > > > > > > &s->acpi_pci_hotplug, bus, parent,
> > > > > > > > > > > > +                        s->use_acpi_hotplug_bridge);
> > > > > > > > > > > > +    }
> > > > > > > > > > > >
> > > > > > > > > > > >      s->cpu_hotplug_legacy = true;
> > > > > > > > > > > >      object_property_add_bool(OBJECT(s), 
> > > > > > > > > > > > "cpu-hotplug-legacy",
> > > > > > > > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > > > > > > > > > index e079b686f5..7e3cf3b57b 100644
> > > > > > > > > > > > --- a/hw/i386/acpi-build.c
> > > > > > > > > > > > +++ b/hw/i386/acpi-build.c
> > > > > > > > > > > > @@ -95,6 +95,7 @@ typedef struct AcpiPmInfo {
> > > > > > > > > > > >      bool s3_disabled;
> > > > > > > > > > > >      bool s4_disabled;
> > > > > > > > > > > >      bool pcihp_bridge_en;
> > > > > > > > > > > > +    bool pcihp_root_en;
> > > > > > > > > > > >      uint8_t s4_val;
> > > > > > > > > > > >      AcpiFadtData fadt;
> > > > > > > > > > > >      uint16_t cpu_hp_io_base;
> > > > > > > > > > > > @@ -245,6 +246,9 @@ static void 
> > > > > > > > > > > > acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
> > > > > > > > > > > >      pm->pcihp_bridge_en =
> > > > > > > > > > > >          object_property_get_bool(obj, 
> > > > > > > > > > > > "acpi-pci-hotplug-with-bridge-support",
> > > > > > > > > > > >                                   NULL);
> > > > > > > > > > > > +    pm->pcihp_root_en =
> > > > > > > > > > > > +        object_property_get_bool(obj, 
> > > > > > > > > > > > "acpi-root-pci-hotplug",
> > > > > > > > > > > > +                                 NULL);
> > > > > > > > > > > >  }
> > > > > > > > > > > >
> > > > > > > > > > > >  static void acpi_get_misc_info(AcpiMiscInfo *info)
> > > > > > > > > > > > @@ -1504,7 +1508,9 @@ build_dsdt(GArray *table_data, 
> > > > > > > > > > > > BIOSLinker *linker,
> > > > > > > > > > > >          build_hpet_aml(dsdt);
> > > > > > > > > > > >          build_piix4_isa_bridge(dsdt);
> > > > > > > > > > > >          build_isa_devices_aml(dsdt);
> > > > > > > > > > > > -        build_piix4_pci_hotplug(dsdt);
> > > > > > > > > > > > +        if (pm->pcihp_bridge_en || pm->pcihp_root_en) {
> > > > > > > > > > > > +            build_piix4_pci_hotplug(dsdt);
> > > > > > > > > > > > +        }
> > > > > > > > > > > >          build_piix4_pci0_int(dsdt);
> > > > > > > > > > > >      } else {
> > > > > > > > > > > >          sb_scope = aml_scope("_SB");
> > > > > > > > > > > > @@ -1698,7 +1704,7 @@ build_dsdt(GArray *table_data, 
> > > > > > > > > > > > BIOSLinker *linker,
> > > > > > > > > > > >      crs_range_set_free(&crs_range_set);
> > > > > > > > > > > >
> > > > > > > > > > > >      /* reserve PCIHP resources */
> > > > > > > > > > > > -    if (pm->pcihp_io_len) {
> > > > > > > > > > > > +    if (pm->pcihp_io_len && (pm->pcihp_bridge_en || 
> > > > > > > > > > > > pm->pcihp_root_en)) {
> > > > > > > > > > > >          dev = aml_device("PHPR");
> > > > > > > > > > > >          aml_append(dev, aml_name_decl("_HID", 
> > > > > > > > > > > > aml_string("PNP0A06")));
> > > > > > > > > > > >          aml_append(dev,
> > > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >