[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: |
Michael S. Tsirkin |
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 06:43:32 -0400 |
On Tue, Jun 20, 2023 at 12:48:05PM +0530, Ani Sinha wrote:
> When a device has an upstream PCIE port, we can only use slot 0.
Actually, it's when device is plugged into a PCIE port.
So maybe:
PCI Express ports only have one slot, so
PCI Express devices can only be plugged into
slot 0 on a PCIE port
> 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.
it also adds a comment explaining why function 0 must not exist
when function != 0 is added. or maybe split that part out.
> 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.
> + */
Pls capitalize populating. Also, comments like this should come
before the logic they document, not after. By the way it doesn't
have to be a bus scan - I'd just say "a scan" - with ACPI
guest knows what was added and can just probe the device functions.
> 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 &&
why hotplugged? Doesn't the same rule apply to all devices?
> + !pci_is_vf(pci_dev) &&
Hmm. I think you copied it from here:
} else if (dev->hotplugged &&
!pci_is_vf(pci_dev) &&
pci_get_function_0(pci_dev)) {
it makes sense there because VFs are added later
after PF exists.
But here it makes no sense that I can see.
> + pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) {
> + /*
> + * If the device has an upstream PCIE port, like a pcie root port,
no, a root port can not be an upstream 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.",
something like:
error_setg(errp, "PCI: slot %d is not valid for %s:"
" PCI Express devices can only be plugged into slot 0")
and then you don't really need a comment.
> + PCI_SLOT(devfn), name);
> + return NULL;
> }
>
> pci_dev->devfn = devfn;
> --
> 2.39.1
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