| Message ID | 20200911180755.28409-9-ani@anisinha.ca |
|---|---|
| State | New |
| Headers | show |
| Series | [1/9] tests/acpi: document addition of table DSDT.roothp for unit testing root pci hotplug on/off | expand |
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, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
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,
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. 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(-)