On 11/7/20 11:10 AM, Philippe Mathieu-Daudé wrote: > Hi, > > On 9/29/20 9:22 AM, Michael S. Tsirkin wrote: >> From: Ani Sinha <a...@anisinha.ca> >> >> When acpi hotplug is turned off for both root pci bus as well as for pci >> bridges, we should not generate the related ACPI code 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. >> >> In this change, we also make sure ACPI code for the PCNT method are only >> added when bsel is enabled for the corresponding pci bus or bridge hotplug >> is turned on. > > I'm trying to understand the following build failure using gcc 9.3.0 > on Ubuntu: > > [2567/3684] Compiling C object > libqemu-x86_64-softmmu.fa.p/hw_i386_acpi-build.c.o > FAILED: libqemu-x86_64-softmmu.fa.p/hw_i386_acpi-build.c.o > ../hw/i386/acpi-build.c: In function 'build_append_pci_bus_devices': > ../hw/i386/acpi-build.c:496:9: error: 'method' may be used uninitialized > in this function [-Werror=maybe-uninitialized] > 496 | aml_append(parent_scope, method); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > cc1: all warnings being treated as errors > >> >> As q35 machines do not use bsel for it's pci buses at this point in time, >> this >> change affects DSDT acpi table for q35 machines as well. Therefore, we will >> also need to commit the updated golden master DSDT table acpi binary blobs as >> well. Following is the list of blobs which needs updating: >> >> tests/data/acpi/q35/DSDT >> tests/data/acpi/q35/DSDT.acpihmat >> tests/data/acpi/q35/DSDT.bridge >> tests/data/acpi/q35/DSDT.cphp >> tests/data/acpi/q35/DSDT.dimmpxm >> tests/data/acpi/q35/DSDT.ipmibt >> tests/data/acpi/q35/DSDT.memhp >> tests/data/acpi/q35/DSDT.mmio64 >> tests/data/acpi/q35/DSDT.numamem >> tests/data/acpi/q35/DSDT.tis >> >> These tables are updated in the following commit. Without the updated table >> blobs, the unit tests would fail with this patch. >> >> Signed-off-by: Ani Sinha <a...@anisinha.ca> >> Reviewed-by: Igor Mammedov <imamm...@redhat.com> >> Message-Id: <20200918084111.15339-11-...@anisinha.ca> >> Reviewed-by: Michael S. Tsirkin <m...@redhat.com> >> Signed-off-by: Michael S. Tsirkin <m...@redhat.com> >> --- >> hw/acpi/piix4.c | 6 ++++-- >> hw/i386/acpi-build.c | 25 ++++++++++++++++++------- >> 2 files changed, 22 insertions(+), 9 deletions(-) >> >> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c >> index 832f8fba82..894d357f8c 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 2b17843837..8d14e4667a 100644 >> --- a/hw/i386/acpi-build.c >> +++ b/hw/i386/acpi-build.c >> @@ -96,6 +96,7 @@ typedef struct AcpiPmInfo { >> bool s4_disabled; >> bool pcihp_bridge_en; >> bool smi_on_cpuhp; >> + bool pcihp_root_en; >> uint8_t s4_val; >> AcpiFadtData fadt; >> uint16_t cpu_hp_io_base; >> @@ -251,6 +252,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) >> @@ -456,10 +460,12 @@ static void build_append_pci_bus_devices(Aml >> *parent_scope, PCIBus *bus, >> } >> >> /* Append PCNT method to notify about events on local and child buses. >> - * Add unconditionally for root since DSDT expects it. >> + * Add this method for root bus only when hotplug is enabled since DSDT >> + * expects it. >> */ >> - method = aml_method("PCNT", 0, AML_NOTSERIALIZED); >> - >> + if (bsel || pcihp_bridge_en) { >> + method = aml_method("PCNT", 0, AML_NOTSERIALIZED); >> + } > > build_append_pci_bus_devices() is not easy to follow and could certainly > benefit from a refactor. > > So here, before 'method' was always reinitialized. Now not always, > so it can be any value set in the big for() loop before... > > Something is definitively wrong.
Suggested fix: https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg01986.html