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

[Michael S. Tsirkin]

On Sat, Nov 07, 2020 at 03:18:24PM +0100, Philippe Mathieu-Daudé wrote:
> 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?

Um ... where exactly?