Re: [PATCH v2] hw/pci: prevent hotplug of devices on pcie-root-ports on the wrong slot

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 15 Jun 2023 10:46:45 +0530
Ani Sinha <anisinha@redhat.com> wrote:

> PCIE root ports and other upstream ports only allow one device on slot 0.
> When hotplugging a device on a pcie root port, make sure that the device
> address passed always represents slot 0. Any other slot value would be
> illegal on a root port.
> 
> CC: jusual@redhat.com
> CC: imammedo@redhat.com
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2128929
> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> ---
>  hw/pci/pci.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> changelog:
> v2: feedback from mst included.
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index bf38905b7d..66999352cc 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -64,6 +64,7 @@ bool pci_available = true;
>  static char *pcibus_get_dev_path(DeviceState *dev);
>  static char *pcibus_get_fw_dev_path(DeviceState *dev);
>  static void pcibus_reset(BusState *qbus);
> +static bool pcie_has_upstream_port(PCIDevice *dev);
>  
>  static Property pci_props[] = {
>      DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
> @@ -1182,6 +1183,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
>      } else if (dev->hotplugged &&
>                 !pci_is_vf(pci_dev) &&
>                 pci_get_function_0(pci_dev)) {
> +        /*
> +         * populating function 0 triggers a bus scan from the guest that
> +         * exposes other non-zero functions. Hence we need to ensure that
> +         * function 0 is available.
> +         */
>          error_setg(errp, "PCI: slot %d function 0 already occupied by %s,"
>                     " new func %s cannot be exposed to guest.",
>                     PCI_SLOT(pci_get_function_0(pci_dev)->devfn),
> @@ -1189,6 +1195,16 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
>                     name);
>  
>         return NULL;
> +    } else if (dev->hotplugged &&
> +               !pci_is_vf(pci_dev) &&
> +               pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
> +        /*
> +         * If the device is being plugged into an upstream PCIE port,

you probably mixing up downstream port with upstream one,
the only thing that could be plugged into upstream port
is PCIE switch.

Also I'm not sure that we should do this at all.
Looking at BZ it seems that QEMU crashes inside backend
and tear down/cleanup sequence is broken somewhere.
And that is the root cause, so I'd rather fix that 1st
and only after that consider adding workarounds if any
were necessary.


> +         * like a pcie root port, we only support one device at slot 0
> +         */
> +        error_setg(errp, "PCI: slot %d is not valid for %s",
> +                   PCI_SLOT(devfn), name);
> +        return NULL;
>      }
>  
>      pci_dev->devfn = devfn;