Re: [PATCH v2 15/25] hw/i386/acpi-build: Introduce and use acpi_get_pci_host

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 27 May 2025 09:40:17 +0200
Eric Auger <eric.auger@redhat.com> wrote:

> pcihp acpi_set_pci_info() generic code currently uses
> acpi_get_i386_pci_host() to retrieve the pci host bridge.
> 
> Let's rename acpi_get_i386_pci_host into acpi_get_pci_host and
> move it in pci generic code.
> 
> The helper is augmented with the support of ARM GPEX.
> 
> Also instead of using the machine child property to retrieve
> the PCI host bridge, we search for the actual object type using
> object_resolve_type_unambiguous().
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>
> 
> ---
> 
> v1 -> v2
> - described the fact we changed the implementation of
>   acpi_get_pci_host() in the commit msg.
> ---
>  include/hw/acpi/pci.h |  2 ++
>  hw/acpi/pci.c         | 20 ++++++++++++++++++++
>  hw/acpi/pcihp.c       |  3 ++-
>  hw/i386/acpi-build.c  | 24 ++++--------------------
>  4 files changed, 28 insertions(+), 21 deletions(-)
> 
> diff --git a/include/hw/acpi/pci.h b/include/hw/acpi/pci.h
> index 4dca22c0e2..310cbd02db 100644
> --- a/include/hw/acpi/pci.h
> +++ b/include/hw/acpi/pci.h
> @@ -41,4 +41,6 @@ void build_pci_bridge_aml(AcpiDevAmlIf *adev, Aml *scope);
>  
>  void build_srat_generic_affinity_structures(GArray *table_data);
>  
> +Object *acpi_get_pci_host(void);
> +
>  #endif
> diff --git a/hw/acpi/pci.c b/hw/acpi/pci.c
> index d511a85029..4191886ebe 100644
> --- a/hw/acpi/pci.c
> +++ b/hw/acpi/pci.c
> @@ -26,6 +26,7 @@
>  #include "qemu/osdep.h"
>  #include "qemu/error-report.h"
>  #include "qom/object_interfaces.h"
> +#include "qom/object.h"
>  #include "qapi/error.h"
>  #include "hw/boards.h"
>  #include "hw/acpi/aml-build.h"
> @@ -33,6 +34,9 @@
>  #include "hw/pci/pci_bridge.h"
>  #include "hw/pci/pci_device.h"
>  #include "hw/pci/pcie_host.h"
> +#include "hw/pci-host/i440fx.h"
> +#include "hw/pci-host/q35.h"
> +#include "hw/pci-host/gpex.h"
>  
>  /*
>   * PCI Firmware Specification, Revision 3.0
> @@ -301,3 +305,19 @@ void build_srat_generic_affinity_structures(GArray *table_data)
>      object_child_foreach_recursive(object_get_root(), build_acpi_generic_port,
>                                     table_data);
>  }
> +
> +Object *acpi_get_pci_host(void)
> +{
> +    Object *host;
> +
> +    host = object_resolve_type_unambiguous(TYPE_I440FX_PCI_HOST_BRIDGE, NULL);
> +    if (host) {
> +        return host;
> +    }
> +    host = object_resolve_type_unambiguous(TYPE_Q35_HOST_DEVICE, NULL);
> +    if (host) {
> +        return host;
> +    }
> +    host = object_resolve_type_unambiguous(TYPE_GPEX_HOST, NULL);
> +    return host;

while it will work, it's getting ridiculous.
(it's not likely to affect performance but if it can be helped,
I'd rather avoid searching)

Can we just reuse host bridge bus in stored in  pcihp state?
aka we do pass root_bus to acpi_pcihp_init() and then there is
AcpiPciHpState::root

then there are other places that call acpi_get_i386_pci_host()
to get root bus, can't we just get it from machine state that
callers get and pass it down as an argument? 


> +}
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 942669ea89..d800371ddc 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -36,6 +36,7 @@
>  #include "hw/pci-bridge/xio3130_downstream.h"
>  #include "hw/i386/acpi-build.h"
>  #include "hw/acpi/acpi.h"
> +#include "hw/acpi/pci.h"
>  #include "hw/pci/pci_bus.h"
>  #include "migration/vmstate.h"
>  #include "qapi/error.h"
> @@ -102,7 +103,7 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
>  static void acpi_set_pci_info(bool has_bridge_hotplug)
>  {
>      static bool bsel_is_set;
> -    Object *host = acpi_get_i386_pci_host();
> +    Object *host = acpi_get_pci_host();
>      PCIBus *bus;
>      BSELInfo info = { .bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT,
>                        .has_bridge_hotplug = has_bridge_hotplug };
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index fe8bc62c03..6feb99e9eb 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -269,27 +269,11 @@ static void acpi_get_misc_info(AcpiMiscInfo *info)
>  #endif
>  }
>  
> -/*
> - * Because of the PXB hosts we cannot simply query TYPE_PCI_HOST_BRIDGE.
> - * On i386 arch we only have two pci hosts, so we can look only for them.
> - */
> -Object *acpi_get_i386_pci_host(void)
> -{
> -    PCIHostState *host;
> -
> -    host = PCI_HOST_BRIDGE(object_resolve_path("/machine/i440fx", NULL));
> -    if (!host) {
> -        host = PCI_HOST_BRIDGE(object_resolve_path("/machine/q35", NULL));
> -    }
> -
> -    return OBJECT(host);
> -}
> -
>  static void acpi_get_pci_holes(Range *hole, Range *hole64)
>  {
>      Object *pci_host;
>  
> -    pci_host = acpi_get_i386_pci_host();
> +    pci_host = acpi_get_pci_host();
>  
>      if (!pci_host) {
>          return;
> @@ -1245,7 +1229,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>  
>      sb_scope = aml_scope("\\_SB");
>      {
> -        Object *pci_host = acpi_get_i386_pci_host();
> +        Object *pci_host = acpi_get_pci_host();
>  
>          if (pci_host) {
>              PCIBus *pbus = PCI_HOST_BRIDGE(pci_host)->bus;
> @@ -1306,7 +1290,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>      if (pm->pcihp_bridge_en || pm->pcihp_root_en) {
>          bool has_pcnt;
>  
> -        Object *pci_host = acpi_get_i386_pci_host();
> +        Object *pci_host = acpi_get_pci_host();
>          PCIBus *b = PCI_HOST_BRIDGE(pci_host)->bus;
>  
>          scope = aml_scope("\\_SB.PCI0");
> @@ -1946,7 +1930,7 @@ static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
>      Object *pci_host;
>      QObject *o;
>  
> -    pci_host = acpi_get_i386_pci_host();
> +    pci_host = acpi_get_pci_host();
>      if (!pci_host) {
>          return false;
>      }