Re: [PATCH v3] hw/acpi: add an assertion check for non-null return from acpi_get_i386_pci_host

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Jul 26, 2021 at 10:27:43PM +0530, Ani Sinha wrote:
> All existing code using acpi_get_i386_pci_host() checks for a non-null
> return value from this function call. Instead of returning early when the value
> returned is NULL, assert instead. Since there are only two possible host buses
> for i386 - q35 and i440fx, a null value return from the function does not make
> sense in most cases and is likely an error situation.

add "on i386"?

> Fixes: c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")

This that seems inappropriate, this is not a bugfix.

> 
> Signed-off-by: Ani Sinha <ani@anisinha.ca>


Frankly I don't see this as a useful cleanup.
assert is generally a last resort thing.

> ---
>  hw/acpi/pcihp.c      |  8 ++++++++
>  hw/i386/acpi-build.c | 15 ++++++---------
>  2 files changed, 14 insertions(+), 9 deletions(-)
> 
> changelog:
> v1: initial patch
> v2: removed comment addition - that can be sent as a separate patch.
> v3: added assertion for null host values for all cases except one.
> 
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index f4d706e47d..054ee8cbc5 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -116,6 +116,12 @@ static void acpi_set_pci_info(void)
>      bsel_is_set = true;
>  
>      if (!host) {
> +        /*
> +         * This function can be eventually called from
> +         * qemu_devices_reset() -> acpi_pcihp_reset() even
> +         * for architectures other than i386. Hence, we need

why call out i386 here? well because currently host
is only non-null for q35 and i440fx which are both i386.
all the above is not a given and we won't remember to update
the comment if we change it. Generally graceful failure
is the default or should be.



> +         * to ignore null values for host here.
> +         */
>          return;
>      }
>  
> @@ -136,6 +142,8 @@ static void acpi_pcihp_disable_root_bus(void)
>          return;
>      }
>  
> +    assert(host);
> +
>      bus = PCI_HOST_BRIDGE(host)->bus;
>      if (bus) {
>          /* setting the hotplug handler to NULL makes the bus non-hotpluggable */
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 17836149fe..83fb1d55c0 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -321,9 +321,7 @@ static void acpi_get_pci_holes(Range *hole, Range *hole64)
>  
>      pci_host = acpi_get_i386_pci_host();
>  
> -    if (!pci_host) {
> -        return;
> -    }
> +    assert(pci_host);
>  
>      range_set_bounds1(hole,
>                        object_property_get_uint(pci_host,
> @@ -1769,9 +1767,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>  
>          pci_host = acpi_get_i386_pci_host();
>  
> -        if (pci_host) {
> -            bus = PCI_HOST_BRIDGE(pci_host)->bus;
> -        }
> +        assert(pci_host);
> +
> +        bus = PCI_HOST_BRIDGE(pci_host)->bus;
>  
>          if (bus) {
>              Aml *scope = aml_scope("PCI0");
> @@ -2389,9 +2387,8 @@ static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
>      QObject *o;
>  
>      pci_host = acpi_get_i386_pci_host();
> -    if (!pci_host) {
> -        return false;
> -    }
> +
> +    assert(pci_host);
>  
>      o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_BASE, NULL);
>      if (!o) {
> -- 
> 2.25.1