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

[Philippe Mathieu-Daudé]

On 11/7/20 1:22 PM, Ani Sinha wrote:
> On Sat, Nov 7, 2020 at 3:40 PM Philippe Mathieu-Daudé <philmd@redhat.com> 
> 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
>>
...
>>>  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.
> 
> Hmm, ok will do that in my spare time.
> 
>>
>> So here, before 'method' was always reinitialized. Now not always,
>> so it can be any value set in the big for() loop before...
> 
> In line 467 above, method is initialized when bsel is available or
> pcihp is enabled. In line 496, it is appended to the parent scope only
> under those conditions as well. Basically, in hunks
> 
> +    if (bsel || pcihp_bridge_en) {
> +        method = aml_method("PCNT", 0, AML_NOTSERIALIZED);
> +    }
> 
> and
> +
> +    if (bsel || pcihp_bridge_en) {
> +        aml_append(parent_scope, method);
> +    }
> 
> the conditions are exactly the same.

The problem is in the (!bsel && !pcihp_bridge_en) case,
what 'method' is used there?