Re: [PATCH v6 2/6] hw/acpi/ich9: Enable ACPI PCI hot-plug

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2739 bytes --]

On Tue, Jul 13, 2021 at 02:42:01AM +0200, Julia Suvorova wrote:
> Add acpi_pcihp to ich9_pm as part of
> 'acpi-pci-hotplug-with-bridge-support' option. Set default to false.
> 
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> Signed-off-by: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Since it looks safe, however I think there are a couple of unnecessary
changes here:


[snip]
> @@ -103,6 +105,7 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
>  static void acpi_set_pci_info(void)
>  {
>      static bool bsel_is_set;
> +    Object *host = acpi_get_i386_pci_host();
>      PCIBus *bus;
>      unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT;
>  
> @@ -111,7 +114,11 @@ static void acpi_set_pci_info(void)
>      }
>      bsel_is_set = true;
>  
> -    bus = find_i440fx(); /* TODO: Q35 support */
> +    if (!host) {

AFAICT acpi_get_i386_pci_host() still can't return NULL, so I'm not
sure this test is necessary.

[snip]
> -static Object *acpi_get_i386_pci_host(void)
> +Object *acpi_get_i386_pci_host(void)
>  {
>      PCIHostState *host;
>  
> @@ -320,7 +320,10 @@ static void acpi_get_pci_holes(Range *hole, Range *hole64)
>      Object *pci_host;
>  
>      pci_host = acpi_get_i386_pci_host();
> -    g_assert(pci_host);
> +
> +    if (!pci_host) {
> +        return;
> +    }

Likewise this change.

>  
>      range_set_bounds1(hole,
>                        object_property_get_uint(pci_host,
> @@ -1765,6 +1768,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>          PCIBus *bus = NULL;
>  
>          pci_host = acpi_get_i386_pci_host();
> +
>          if (pci_host) {
>              bus = PCI_HOST_BRIDGE(pci_host)->bus;
>          }
> @@ -2321,7 +2325,9 @@ static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
>      QObject *o;
>  
>      pci_host = acpi_get_i386_pci_host();
> -    g_assert(pci_host);
> +    if (!pci_host) {
> +        return false;
> +    }

And this one.

>  
>      o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_BASE, NULL);
>      if (!o) {
> @@ -2351,7 +2357,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>      AcpiPmInfo pm;
>      AcpiMiscInfo misc;
>      AcpiMcfgInfo mcfg;
> -    Range pci_hole, pci_hole64;
> +    Range pci_hole = {}, pci_hole64 = {};
>      uint8_t *u;
>      size_t aml_len = 0;
>      GArray *tables_blob = tables->table_data;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]