Re: [PATCH 1/2] acpi: x86: pcihp: cleanup devfn usage in build_append_pci_bus_devices()

[Michael S. Tsirkin]

I would add a description: we want to scan all functions
not just function 0 to describe hotplug into bridges
at function != 0. in preparation for this, refactor code to not
skip functions != 0.

On Thu, Jul 22, 2021 at 06:59:44AM -0400, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/i386/acpi-build.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 17836149fe..b40e284b72 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -374,7 +374,7 @@ static void build_append_pci_bus_devices(Aml 
> *parent_scope, PCIBus *bus,
>      Aml *dev, *notify_method = NULL, *method;
>      QObject *bsel;
>      PCIBus *sec;
> -    int i;
> +    int devfn;
>  
>      bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, 
> NULL);
>      if (bsel) {
> @@ -384,11 +384,11 @@ static void build_append_pci_bus_devices(Aml 
> *parent_scope, PCIBus *bus,
>          notify_method = aml_method("DVNT", 2, AML_NOTSERIALIZED);
>      }
>  
> -    for (i = 0; i < ARRAY_SIZE(bus->devices); i += PCI_FUNC_MAX) {
> +    for (devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) {
>          DeviceClass *dc;
>          PCIDeviceClass *pc;
> -        PCIDevice *pdev = bus->devices[i];
> -        int slot = PCI_SLOT(i);
> +        PCIDevice *pdev = bus->devices[devfn];
> +        int slot = PCI_SLOT(devfn);
>          bool hotplug_enabled_dev;
>          bool bridge_in_acpi;
>          bool cold_plugged_bridge;

I am a bit puzzled about why this is equivalent. so we used to scan just
function 0 on each slot. now we are scanning them all.
won't this generate a different AML code? in fact duplicate
descriptions?
I suspect you need to move the check for slot == 0 from the next patch
to this one otherwise bisect will be broken.

Or just squash this part into next patch. up to you.


> @@ -525,13 +525,12 @@ static void build_append_pci_bus_devices(Aml 
> *parent_scope, PCIBus *bus,
>          /* Notify about child bus events in any case */
>          if (pcihp_bridge_en) {
>              QLIST_FOREACH(sec, &bus->child, sibling) {
> -                int32_t devfn = sec->parent_dev->devfn;
> -
>                  if (pci_bus_is_root(sec)) {
>                      continue;
>                  }
>  
> -                aml_append(method, aml_name("^S%.02X.PCNT", devfn));
> +                aml_append(method, aml_name("^S%.02X.PCNT",
> +                                            sec->parent_dev->devfn));
>              }
>          }
>  


this is a refactor, sure.

> -- 
> 2.27.0