| Message ID | 20230620071805.4400-1-anisinha@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] hw/pci: enforce use of slot only slot 0 when devices have an upstream PCIE port | expand |
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;
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
just noticed a repeated "slot" in the subject btw.
On Tue, Jun 20, 2023 at 10:59:42AM +0200, Igor Mammedov wrote: > 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. 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;
> On 20-Jun-2023, at 4:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > 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. I thought PFs are handled only in the host OS and only VFs are passthrough into the guest? I thought this check was because VFs have a different domain address separate from other non-vf devices in the guest PCI tree. > > 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
On Wed, Jun 21, 2023 at 08:09:55AM +0530, Ani Sinha wrote: > > > > On 20-Jun-2023, at 4:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > > > 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. > > I thought PFs are handled only in the host OS and only VFs are > passthrough into the guest? This is emulated SRIOV. host and guest would be nested L2. > I thought this check was because VFs have > a different domain address separate from other non-vf devices in the > guest PCI tree. Maybe take a look at the SRIOV spec then. > > > > 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
> On 20-Jun-2023, at 4:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > 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. This patch with these changes causes failures in bios-tables-test which can be fixed easily. It also breaks hd-geo-test and I do not know enough of this test to fix them. I think I will drop this patch for now. > > >> + 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
> On 21-Jun-2023, at 4:36 PM, Ani Sinha <anisinha@redhat.com> wrote: > > > >> On 20-Jun-2023, at 4:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >> >> 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. > > This patch with these changes causes failures in bios-tables-test which can be fixed easily. It also breaks hd-geo-test and I do not know enough of this test to fix them. Specifically it breaks test_override_scsi_q35() and test_override_virtio_blk_q35(). I think these tests were wrong to begin with since they attach a pcie-to-pci bridge on a pcie root port and then attach a scsi controller not on the pcie-to-pci bridge but on the root port (which is not possible because we can only attach one device on a non-multifunction pcie root port). Even if I fix them, its failing somewhere else. I have added Thomas and Laurent, maybe they can help fix these in this test. I have pushed my patch here: https://gitlab.com/anisinha/qemu/-/commit/24b3eddb968a0739bff222bdf781f722365cc9b2 > > I think I will drop this patch for now. > >> >> >>> + 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
> On 21-Jun-2023, at 4:55 PM, Ani Sinha <anisinha@redhat.com> wrote: > > > >> On 21-Jun-2023, at 4:36 PM, Ani Sinha <anisinha@redhat.com> wrote: >> >> >> >>> On 20-Jun-2023, at 4:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >>> >>> 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. This commit essentially needs fixing: commit c46b126088b5616d8b7cd3ff83aaf5d097c36633 Author: Michael Labiuk <michael.labiuk@virtuozzo.com> Date: Fri Sep 30 01:35:42 2022 +0300 tests/x86: Add 'q35' machine type to override-tests in hd-geo-test Signed-off-by: Michael Labiuk <michael.labiuk@virtuozzo.com> Message-Id: <20220929223547.1429580-5-michael.labiuk@virtuozzo.com> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> >>>> 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. >> >> This patch with these changes causes failures in bios-tables-test which can be fixed easily. It also breaks hd-geo-test and I do not know enough of this test to fix them. > > Specifically it breaks test_override_scsi_q35() and test_override_virtio_blk_q35(). > I think these tests were wrong to begin with since they attach a pcie-to-pci bridge on a pcie root port and then attach a scsi controller not on the pcie-to-pci bridge but on the root port (which is not possible because we can only attach one device on a non-multifunction pcie root port). Even if I fix them, its failing somewhere else. > > I have added Thomas and Laurent, maybe they can help fix these in this test. > I have pushed my patch here: https://gitlab.com/anisinha/qemu/-/commit/24b3eddb968a0739bff222bdf781f722365cc9b2 > > >> >> I think I will drop this patch for now. >> >>> >>> >>>> + 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
> On 21-Jun-2023, at 5:20 PM, Ani Sinha <anisinha@redhat.com> wrote: > > > >> On 21-Jun-2023, at 4:55 PM, Ani Sinha <anisinha@redhat.com> wrote: >> >> >> >>> On 21-Jun-2023, at 4:36 PM, Ani Sinha <anisinha@redhat.com> wrote: >>> >>> >>> >>>> On 20-Jun-2023, at 4:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >>>> >>>> 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. > > This commit essentially needs fixing: > > commit c46b126088b5616d8b7cd3ff83aaf5d097c36633 > Author: Michael Labiuk <michael.labiuk@virtuozzo.com> > Date: Fri Sep 30 01:35:42 2022 +0300 > > tests/x86: Add 'q35' machine type to override-tests in hd-geo-test > > Signed-off-by: Michael Labiuk <michael.labiuk@virtuozzo.com> > Message-Id: <20220929223547.1429580-5-michael.labiuk@virtuozzo.com> > Signed-off-by: Thomas Huth <thuth@redhat.com> I have sent a patch series with a proposed fix for this test (hd-geo-test) as well as fixes for bios-tables-test. The series also includes my QEMU patch that enforces proper PCIE slot usage. The patches are part of one series and should be applied in the same sequence - test fixes first then the QEMU fix so that the QEMU fix does not break the unit tests. Please review. > >>>> >>>>> 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. >>> >>> This patch with these changes causes failures in bios-tables-test which can be fixed easily. It also breaks hd-geo-test and I do not know enough of this test to fix them. >> >> Specifically it breaks test_override_scsi_q35() and test_override_virtio_blk_q35(). >> I think these tests were wrong to begin with since they attach a pcie-to-pci bridge on a pcie root port and then attach a scsi controller not on the pcie-to-pci bridge but on the root port (which is not possible because we can only attach one device on a non-multifunction pcie root port). Even if I fix them, its failing somewhere else. >> >> I have added Thomas and Laurent, maybe they can help fix these in this test. >> I have pushed my patch here: https://gitlab.com/anisinha/qemu/-/commit/24b3eddb968a0739bff222bdf781f722365cc9b2 >> >> >>> >>> I think I will drop this patch for now. >>> >>>> >>>> >>>>> + 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
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.", + PCI_SLOT(devfn), name); + return NULL; } pci_dev->devfn = devfn;
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. 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 :-)