| Message ID | 20230614123150.7077-1-anisinha@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/pci: prevent hotplug of devices on pcie-root-ports with wrong devfn address | expand |
On Wed, 14 Jun 2023 18:01:50 +0530 Ani Sinha <anisinha@redhat.com> wrote: > PCIE root ports only allow one device on slot 0/function 0. When hotplugging a > device on a pcie root port, make sure that the device address passed is > always 0x00 that represents slot 0 and function 0. Any other slot value and > function value would be illegal on a root port. > > 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 | 9 +++++++++ > 1 file changed, 9 insertions(+) > > Note: > I tested this with both native and acpi hotplug enabled on pcie. The > check seems to work on both. > > (qemu) netdev_add socket,id=hostnet1,listen=:1234 > (qemu) device_add e1000e,netdev=hostnet1,mac=00:11:22:33:44:03,id=net1,bus=pci.6,addr=0x2.0x5 > Error: PCI: slot 2 function 5 is not valid for e1000e > (qemu) device_add e1000e,netdev=hostnet1,mac=00:11:22:33:44:03,id=net1,bus=pci.6,addr=0x0.0 > (qemu) info network > net1: index=0,type=nic,model=e1000e,macaddr=00:11:22:33:44:03 > \ hostnet1: index=0,type=socket, > (qemu) device_del net1 > (qemu) info network > hostnet1: index=0,type=socket, > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index bf38905b7d..bc39322dfd 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), > @@ -1189,6 +1190,14 @@ 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) can you clarify what above 2 lines are here for? you can't hotplug anything into upstream port (only downstream) and why _vf() is here also not clear. > && devfn) { > + /* For an upstream PCIE port, we only support one device at slot 0 */ > + error_setg(errp, "PCI: slot %d function %d is not valid for %s", > + PCI_SLOT(devfn), PCI_FUNC(devfn), name); > + return NULL; > + > } > > pci_dev->devfn = devfn;
On Wed, Jun 14, 2023 at 06:01:50PM +0530, Ani Sinha wrote: > PCIE root ports only allow one device on slot 0/function 0. Why do you say this? PCI devices can be multifunction. > When hotplugging a > device on a pcie root port, make sure that the device address passed is > always 0x00 that represents slot 0 and function 0. Any other slot value and > function value would be illegal on a root port. > > 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 | 9 +++++++++ > 1 file changed, 9 insertions(+) > > Note: > I tested this with both native and acpi hotplug enabled on pcie. The > check seems to work on both. > > (qemu) netdev_add socket,id=hostnet1,listen=:1234 > (qemu) device_add e1000e,netdev=hostnet1,mac=00:11:22:33:44:03,id=net1,bus=pci.6,addr=0x2.0x5 > Error: PCI: slot 2 function 5 is not valid for e1000e > (qemu) device_add e1000e,netdev=hostnet1,mac=00:11:22:33:44:03,id=net1,bus=pci.6,addr=0x0.0 > (qemu) info network > net1: index=0,type=nic,model=e1000e,macaddr=00:11:22:33:44:03 > \ hostnet1: index=0,type=socket, > (qemu) device_del net1 > (qemu) info network > hostnet1: index=0,type=socket, > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index bf38905b7d..bc39322dfd 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), > @@ -1189,6 +1190,14 @@ 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) && devfn) { > + /* For an upstream PCIE port, we only support one device at slot 0 */ > + error_setg(errp, "PCI: slot %d function %d is not valid for %s", > + PCI_SLOT(devfn), PCI_FUNC(devfn), name); > + return NULL; > + > } > > pci_dev->devfn = devfn; > -- > 2.39.1
> On 14-Jun-2023, at 7:52 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Wed, Jun 14, 2023 at 06:01:50PM +0530, Ani Sinha wrote: >> PCIE root ports only allow one device on slot 0/function 0. > > > Why do you say this? PCI devices can be multifunction. Yeah you are right, the language needs correction. For hot plugging, I believe only function 0 is relevant. At least that is what it looks like from the code. > >> When hotplugging a >> device on a pcie root port, make sure that the device address passed is >> always 0x00 that represents slot 0 and function 0. Any other slot value and >> function value would be illegal on a root port. >> >> 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 | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> Note: >> I tested this with both native and acpi hotplug enabled on pcie. The >> check seems to work on both. >> >> (qemu) netdev_add socket,id=hostnet1,listen=:1234 >> (qemu) device_add e1000e,netdev=hostnet1,mac=00:11:22:33:44:03,id=net1,bus=pci.6,addr=0x2.0x5 >> Error: PCI: slot 2 function 5 is not valid for e1000e >> (qemu) device_add e1000e,netdev=hostnet1,mac=00:11:22:33:44:03,id=net1,bus=pci.6,addr=0x0.0 >> (qemu) info network >> net1: index=0,type=nic,model=e1000e,macaddr=00:11:22:33:44:03 >> \ hostnet1: index=0,type=socket, >> (qemu) device_del net1 >> (qemu) info network >> hostnet1: index=0,type=socket, >> >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >> index bf38905b7d..bc39322dfd 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), >> @@ -1189,6 +1190,14 @@ 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) && devfn) { >> + /* For an upstream PCIE port, we only support one device at slot 0 */ >> + error_setg(errp, "PCI: slot %d function %d is not valid for %s", >> + PCI_SLOT(devfn), PCI_FUNC(devfn), name); >> + return NULL; >> + >> } >> >> pci_dev->devfn = devfn; >> -- >> 2.39.1 >
> On 14-Jun-2023, at 6:31 PM, Igor Mammedov <imammedo@redhat.com> wrote: > > On Wed, 14 Jun 2023 18:01:50 +0530 > Ani Sinha <anisinha@redhat.com> wrote: > >> PCIE root ports only allow one device on slot 0/function 0. When hotplugging a >> device on a pcie root port, make sure that the device address passed is >> always 0x00 that represents slot 0 and function 0. Any other slot value and >> function value would be illegal on a root port. >> >> 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 | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> Note: >> I tested this with both native and acpi hotplug enabled on pcie. The >> check seems to work on both. >> >> (qemu) netdev_add socket,id=hostnet1,listen=:1234 >> (qemu) device_add e1000e,netdev=hostnet1,mac=00:11:22:33:44:03,id=net1,bus=pci.6,addr=0x2.0x5 >> Error: PCI: slot 2 function 5 is not valid for e1000e >> (qemu) device_add e1000e,netdev=hostnet1,mac=00:11:22:33:44:03,id=net1,bus=pci.6,addr=0x0.0 >> (qemu) info network >> net1: index=0,type=nic,model=e1000e,macaddr=00:11:22:33:44:03 >> \ hostnet1: index=0,type=socket, >> (qemu) device_del net1 >> (qemu) info network >> hostnet1: index=0,type=socket, >> >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >> index bf38905b7d..bc39322dfd 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), >> @@ -1189,6 +1190,14 @@ 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) > can you clarify what above 2 lines are here for? > > you can't hotplug anything into upstream port (only downstream) This code checks if the hot plugged device is being plugged into a pcie root port. For pcie root ports, only slot 0 can be used. Please see 3f1e1478db2d67098d9 . The conditional check just above this hunk therefore checks for unoccupied function 0 in slot 0 if the device is plugged into an upstream pcie root port. I think for hot plugs, only function 0 is allowed. As for vf, I am not sure. I believe for vfs the restriction is not valid. I just kept the check based on the conditional just prior to it. > and why _vf() is here also not clear. > >> && devfn) { >> + /* For an upstream PCIE port, we only support one device at slot 0 */ >> + error_setg(errp, "PCI: slot %d function %d is not valid for %s", >> + PCI_SLOT(devfn), PCI_FUNC(devfn), name); >> + return NULL; >> + >> } >> >> pci_dev->devfn = devfn;
On Wed, Jun 14, 2023 at 10:31:40PM +0530, Ani Sinha wrote: > > > > On 14-Jun-2023, at 6:31 PM, Igor Mammedov <imammedo@redhat.com> wrote: > > > > On Wed, 14 Jun 2023 18:01:50 +0530 > > Ani Sinha <anisinha@redhat.com> wrote: > > > >> PCIE root ports only allow one device on slot 0/function 0. When hotplugging a > >> device on a pcie root port, make sure that the device address passed is > >> always 0x00 that represents slot 0 and function 0. Any other slot value and > >> function value would be illegal on a root port. > >> > >> 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 | 9 +++++++++ > >> 1 file changed, 9 insertions(+) > >> > >> Note: > >> I tested this with both native and acpi hotplug enabled on pcie. The > >> check seems to work on both. > >> > >> (qemu) netdev_add socket,id=hostnet1,listen=:1234 > >> (qemu) device_add e1000e,netdev=hostnet1,mac=00:11:22:33:44:03,id=net1,bus=pci.6,addr=0x2.0x5 > >> Error: PCI: slot 2 function 5 is not valid for e1000e > >> (qemu) device_add e1000e,netdev=hostnet1,mac=00:11:22:33:44:03,id=net1,bus=pci.6,addr=0x0.0 > >> (qemu) info network > >> net1: index=0,type=nic,model=e1000e,macaddr=00:11:22:33:44:03 > >> \ hostnet1: index=0,type=socket, > >> (qemu) device_del net1 > >> (qemu) info network > >> hostnet1: index=0,type=socket, > >> > >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c > >> index bf38905b7d..bc39322dfd 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), > >> @@ -1189,6 +1190,14 @@ 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) > > can you clarify what above 2 lines are here for? > > > > you can't hotplug anything into upstream port (only downstream) > > This code checks if the hot plugged device is being plugged into a pcie root port. For pcie root ports, only slot 0 can be used. Please see 3f1e1478db2d67098d9 . The conditional check just above this hunk therefore checks for unoccupied function 0 in slot 0 if the device is plugged into an upstream pcie root port. I think for hot plugs, only function 0 is allowed. Not really. The way we hotplug a multifunction device is by first populating all functions != 0, then finally pupulating function 0 which is when the event triggers. > > As for vf, I am not sure. I believe for vfs the restriction is not valid. I just kept the check based on the conditional just prior to it. > > > and why _vf() is here also not clear. > > > >> && devfn) { > >> + /* For an upstream PCIE port, we only support one device at slot 0 */ > >> + error_setg(errp, "PCI: slot %d function %d is not valid for %s", > >> + PCI_SLOT(devfn), PCI_FUNC(devfn), name); > >> + return NULL; > >> + > >> } > >> > >> pci_dev->devfn = devfn;
On Wed, Jun 14, 2023 at 10:09:35PM +0530, Ani Sinha wrote: > > > > On 14-Jun-2023, at 7:52 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Wed, Jun 14, 2023 at 06:01:50PM +0530, Ani Sinha wrote: > >> PCIE root ports only allow one device on slot 0/function 0. > > > > > > Why do you say this? PCI devices can be multifunction. > > Yeah you are right, the language needs correction. For hot plugging, I believe only function 0 is relevant. At least that is what it looks like from the code. Adding function 0 is what makes it visible to guest. But you can't block multifunction devices that would be a regression. > > > >> When hotplugging a > >> device on a pcie root port, make sure that the device address passed is > >> always 0x00 that represents slot 0 and function 0. Any other slot value and > >> function value would be illegal on a root port. > >> > >> 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 | 9 +++++++++ > >> 1 file changed, 9 insertions(+) > >> > >> Note: > >> I tested this with both native and acpi hotplug enabled on pcie. The > >> check seems to work on both. > >> > >> (qemu) netdev_add socket,id=hostnet1,listen=:1234 > >> (qemu) device_add e1000e,netdev=hostnet1,mac=00:11:22:33:44:03,id=net1,bus=pci.6,addr=0x2.0x5 > >> Error: PCI: slot 2 function 5 is not valid for e1000e > >> (qemu) device_add e1000e,netdev=hostnet1,mac=00:11:22:33:44:03,id=net1,bus=pci.6,addr=0x0.0 > >> (qemu) info network > >> net1: index=0,type=nic,model=e1000e,macaddr=00:11:22:33:44:03 > >> \ hostnet1: index=0,type=socket, > >> (qemu) device_del net1 > >> (qemu) info network > >> hostnet1: index=0,type=socket, > >> > >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c > >> index bf38905b7d..bc39322dfd 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), > >> @@ -1189,6 +1190,14 @@ 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) && devfn) { > >> + /* For an upstream PCIE port, we only support one device at slot 0 */ > >> + error_setg(errp, "PCI: slot %d function %d is not valid for %s", > >> + PCI_SLOT(devfn), PCI_FUNC(devfn), name); > >> + return NULL; > >> + > >> } > >> > >> pci_dev->devfn = devfn; > >> -- > >> 2.39.1 > >
> On 15-Jun-2023, at 2:22 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Wed, Jun 14, 2023 at 10:31:40PM +0530, Ani Sinha wrote: >> >> >>> On 14-Jun-2023, at 6:31 PM, Igor Mammedov <imammedo@redhat.com> wrote: >>> >>> On Wed, 14 Jun 2023 18:01:50 +0530 >>> Ani Sinha <anisinha@redhat.com> wrote: >>> >>>> PCIE root ports only allow one device on slot 0/function 0. When hotplugging a >>>> device on a pcie root port, make sure that the device address passed is >>>> always 0x00 that represents slot 0 and function 0. Any other slot value and >>>> function value would be illegal on a root port. >>>> >>>> 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 | 9 +++++++++ >>>> 1 file changed, 9 insertions(+) >>>> >>>> Note: >>>> I tested this with both native and acpi hotplug enabled on pcie. The >>>> check seems to work on both. >>>> >>>> (qemu) netdev_add socket,id=hostnet1,listen=:1234 >>>> (qemu) device_add e1000e,netdev=hostnet1,mac=00:11:22:33:44:03,id=net1,bus=pci.6,addr=0x2.0x5 >>>> Error: PCI: slot 2 function 5 is not valid for e1000e >>>> (qemu) device_add e1000e,netdev=hostnet1,mac=00:11:22:33:44:03,id=net1,bus=pci.6,addr=0x0.0 >>>> (qemu) info network >>>> net1: index=0,type=nic,model=e1000e,macaddr=00:11:22:33:44:03 >>>> \ hostnet1: index=0,type=socket, >>>> (qemu) device_del net1 >>>> (qemu) info network >>>> hostnet1: index=0,type=socket, >>>> >>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >>>> index bf38905b7d..bc39322dfd 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), >>>> @@ -1189,6 +1190,14 @@ 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) >>> can you clarify what above 2 lines are here for? >>> >>> you can't hotplug anything into upstream port (only downstream) >> >> This code checks if the hot plugged device is being plugged into a pcie root port. For pcie root ports, only slot 0 can be used. Please see 3f1e1478db2d67098d9 . The conditional check just above this hunk therefore checks for unoccupied function 0 in slot 0 if the device is plugged into an upstream pcie root port. I think for hot plugs, only function 0 is allowed. > > Not really. The way we hotplug a multifunction device is by first > populating all functions != 0, then finally pupulating function 0 > which is when the event triggers. Let me explain better .. This check currently already exists: } else if (dev->hotplugged && !pci_is_vf(pci_dev) && pci_get_function_0(pci_dev)) { 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), pci_get_function_0(pci_dev)->name, name); return NULL; } This code checks for unoccupied function 0. If you look into pci_get_function_0(), it checks for slot 0 function 0 for devices that are plugged into pcie root port. > >> >> As for vf, I am not sure. I believe for vfs the restriction is not valid. I just kept the check based on the conditional just prior to it. >> >>> and why _vf() is here also not clear. >>> >>>> && devfn) { >>>> + /* For an upstream PCIE port, we only support one device at slot 0 */ >>>> + error_setg(errp, "PCI: slot %d function %d is not valid for %s", >>>> + PCI_SLOT(devfn), PCI_FUNC(devfn), name); >>>> + return NULL; >>>> + >>>> } >>>> >>>> pci_dev->devfn = devfn;
> On 15-Jun-2023, at 2:23 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Wed, Jun 14, 2023 at 10:09:35PM +0530, Ani Sinha wrote: >> >> >>> On 14-Jun-2023, at 7:52 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >>> >>> On Wed, Jun 14, 2023 at 06:01:50PM +0530, Ani Sinha wrote: >>>> PCIE root ports only allow one device on slot 0/function 0. >>> >>> >>> Why do you say this? PCI devices can be multifunction. >> >> Yeah you are right, the language needs correction. For hot plugging, I believe only function 0 is relevant. At least that is what it looks like from the code. > > Adding function 0 is what makes it visible to guest. > But you can't block multifunction devices that would be a regression. OK fair enough and I can make a small adjustment in the code. > > >>> >>>> When hotplugging a >>>> device on a pcie root port, make sure that the device address passed is >>>> always 0x00 that represents slot 0 and function 0. Any other slot value and >>>> function value would be illegal on a root port. >>>> >>>> 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 | 9 +++++++++ >>>> 1 file changed, 9 insertions(+) >>>> >>>> Note: >>>> I tested this with both native and acpi hotplug enabled on pcie. The >>>> check seems to work on both. >>>> >>>> (qemu) netdev_add socket,id=hostnet1,listen=:1234 >>>> (qemu) device_add e1000e,netdev=hostnet1,mac=00:11:22:33:44:03,id=net1,bus=pci.6,addr=0x2.0x5 >>>> Error: PCI: slot 2 function 5 is not valid for e1000e >>>> (qemu) device_add e1000e,netdev=hostnet1,mac=00:11:22:33:44:03,id=net1,bus=pci.6,addr=0x0.0 >>>> (qemu) info network >>>> net1: index=0,type=nic,model=e1000e,macaddr=00:11:22:33:44:03 >>>> \ hostnet1: index=0,type=socket, >>>> (qemu) device_del net1 >>>> (qemu) info network >>>> hostnet1: index=0,type=socket, >>>> >>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >>>> index bf38905b7d..bc39322dfd 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), >>>> @@ -1189,6 +1190,14 @@ 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) && devfn) { >>>> + /* For an upstream PCIE port, we only support one device at slot 0 */ >>>> + error_setg(errp, "PCI: slot %d function %d is not valid for %s", >>>> + PCI_SLOT(devfn), PCI_FUNC(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..bc39322dfd 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), @@ -1189,6 +1190,14 @@ 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) && devfn) { + /* For an upstream PCIE port, we only support one device at slot 0 */ + error_setg(errp, "PCI: slot %d function %d is not valid for %s", + PCI_SLOT(devfn), PCI_FUNC(devfn), name); + return NULL; + } pci_dev->devfn = devfn;
PCIE root ports only allow one device on slot 0/function 0. When hotplugging a device on a pcie root port, make sure that the device address passed is always 0x00 that represents slot 0 and function 0. Any other slot value and function value would be illegal on a root port. 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 | 9 +++++++++ 1 file changed, 9 insertions(+) Note: I tested this with both native and acpi hotplug enabled on pcie. The check seems to work on both. (qemu) netdev_add socket,id=hostnet1,listen=:1234 (qemu) device_add e1000e,netdev=hostnet1,mac=00:11:22:33:44:03,id=net1,bus=pci.6,addr=0x2.0x5 Error: PCI: slot 2 function 5 is not valid for e1000e (qemu) device_add e1000e,netdev=hostnet1,mac=00:11:22:33:44:03,id=net1,bus=pci.6,addr=0x0.0 (qemu) info network net1: index=0,type=nic,model=e1000e,macaddr=00:11:22:33:44:03 \ hostnet1: index=0,type=socket, (qemu) device_del net1 (qemu) info network hostnet1: index=0,type=socket,