Re: [PULL v4 44/48] piix4: don't reserve hw resources when hotplug is off globally

[Philippe Mathieu-Daudé]

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 <ani@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 <ani@anisinha.ca>
>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>> Message-Id: <20200918084111.15339-11-ani@anisinha.ca>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@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