Re: [PATCH v4 04/11] Fix a gap where acpi_pcihp_find_hotplug_bus() returns a non-hotpluggable bus

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 16 Sep 2020 11:43:28 +0530
Ani Sinha <ani@anisinha.ca> wrote:

> When ACPI hotplug for the root bus is disabled, the bsel property for that
> bus is not set. Please see the following commit:
> 
> 3d7e78aa7777f ("Introduce a new flag for i440fx to disable PCI hotplug on the root bus").
> 
> As a result, when acpi_pcihp_find_hotplug_bus() is called
> with bsel set to 0, it may return the root bus. This can cause devices attached to
> the root bus to get hot-unplugged if the user issues the following set of commmands:
> 
> outl 0xae10 0
> outl 0xae08 your_slot
>
> Thanks to Julia for pointing this out here:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg734548.html
> 
> In this patch, we fix the issue in this function by checking if the bus which is
> returned by the function is actually hotpluggable. If not, we simply return NULL.
> This avoids the scenario where we were returning a non-hotpluggable bus.
> 
> Signed-off-by: Ani Sinha <ani@anisinha.ca>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/acpi/pcihp.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> change log:
> v1: initial patch
> v4: commit log updated as per suggestion.
> 
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 39b1f74442..32ae8b2c0a 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -147,6 +147,21 @@ static PCIBus *acpi_pcihp_find_hotplug_bus(AcpiPciHpState *s, int bsel)
>      if (!bsel && !find.bus) {
>          find.bus = s->root;
>      }
> +
> +    /*
> +     * Check if find.bus is actually hotpluggable. If bsel is set to
> +     * NULL for example on the root bus in order to make it
> +     * non-hotpluggable, find.bus will match the root bus when bsel
> +     * is 0. See acpi_pcihp_test_hotplug_bus() above. Since the
> +     * bus is not hotpluggable however, we should not select the bus.
> +     * Instead, we should set find.bus to NULL in that case. In the check
> +     * below, we generalize this case for all buses, not just the root bus.
> +     * The callers of this function check for a null return value and
> +     * handle them appropriately.
> +     */
> +    if (find.bus && !qbus_is_hotpluggable(BUS(find.bus))) {
> +        find.bus = NULL;
> +    }
>      return find.bus;
>  }
>