On Tue, Jun 20, 2023 at 10:59:42AM +0200, Igor Mammedov wrote: > 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.
No i do not think this is specific to root ports. it is technically any non-integrated express device but we also plug pci devices into express ports as a hack. so checking where device is plugged (this is what pcie_has_upstream_port does) seems like a reasonable approach. > > + PCI_SLOT(devfn), name); > > + return NULL; > > } > > > > pci_dev->devfn = devfn;
