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: 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. > + */ 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