On Tue, 20 Jun 2023 12:48:05 +0530 Ani Sinha <anisi...@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: jus...@redhat.com > CC: imamm...@redhat.com > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2128929 > Signed-off-by: Ani Sinha <anisi...@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;
