[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4] hw/pci: enforce use of slot only slot 0 when devices have
From: |
Igor Mammedov |
Subject: |
Re: [PATCH v4] hw/pci: enforce use of slot only slot 0 when devices have an upstream PCIE port |
Date: |
Tue, 20 Jun 2023 10:59:42 +0200 |
On Tue, 20 Jun 2023 12:48:05 +0530
Ani Sinha <anisinha@redhat.com> wrote:
> When a device has an upstream PCIE port, we can only use slot 0. Non-zero
> slots
> are invalid.
> This change ensures that we throw an error if the user
> tries to hotplug a device with an upstream PCIE port to a non-zero slot.
Isn't the same true for coldplugged devices?
Why you limit it only to hotplug?
>
> 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 | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> changelog:
> v2: addressed issue with multifunction pcie root ports. Should allow
> hotplug on functions other than function 0.
> v3: improved commit message.
> v4: improve commit message and code comments further. Some more
> improvements might come in v5. No claims made here that this is
> the final one :-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index bf38905b7d..30ce6a78cb 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 wasn't added yet.
> + */
> 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,18 @@ 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 has an upstream PCIE port, like a pcie root port,
> + * we only support functions on slot 0.
> + */
> + error_setg(errp, "PCI: slot %d is not valid for %s,"
> + " only functions on slot 0 is supported for devices"
> + " with an upstream PCIE port.",
upstream port language is confusing here and elsewhere you mention it.
It would be better to use root-port instead.
> + PCI_SLOT(devfn), name);
> + return NULL;
> }
>
> pci_dev->devfn = devfn;
Re: [PATCH v4] hw/pci: enforce use of slot only slot 0 when devices have an upstream PCIE port, Michael S. Tsirkin, 2023/06/20