| Message ID | 20230622103255.267289-6-anisinha@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | test and QEMU fixes to ensure proper PCIE device usage | expand |
On Thu, Jun 22, 2023 at 12:34 PM Ani Sinha <anisinha@redhat.com> wrote: > > PCI Express ports only have one slot, so PCI Express devices can only be > plugged into slot 0 on a PCIE port. Enforce it. > > 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 | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index bf38905b7d..5f25ab9f5e 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,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, > name); > > return NULL; > + } else if (pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) { > + error_setg(errp, "PCI: slot %d is not valid for %s," > + " PCI express devices can only be plugged into slot 0.", This is not technically correct, because downstream ports and root ports are also PCIe devices, and they can have different slots under upstream ports and RC. But this error will never be shown for them, so it seems fine. Reviewed-by: Julia Suvorova <jusual@redhat.com> > + PCI_SLOT(devfn), name); > + return NULL; > } > > pci_dev->devfn = devfn; > -- > 2.39.1 >
On Thu, Jun 22, 2023 at 05:46:40PM +0200, Julia Suvorova wrote: > On Thu, Jun 22, 2023 at 12:34 PM Ani Sinha <anisinha@redhat.com> wrote: > > > > PCI Express ports only have one slot, so PCI Express devices can only be > > plugged into slot 0 on a PCIE port. Enforce it. > > > > 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 | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > index bf38905b7d..5f25ab9f5e 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,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, > > name); > > > > return NULL; > > + } else if (pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) { > > + error_setg(errp, "PCI: slot %d is not valid for %s," > > + " PCI express devices can only be plugged into slot 0.", > > This is not technically correct, because downstream ports and root > ports are also PCIe devices, and they can have different slots under > upstream ports and RC. But this error will never be shown for them, so > it seems fine. Hmm. Confusing users is not nice ... I agree this might make people think they can not use root ports in slot !=0 either. Would you add "with an upstream port"? E.g. "PCI Express devices with an upstream port" ? > > Reviewed-by: Julia Suvorova <jusual@redhat.com> > > > > > > + PCI_SLOT(devfn), name); > > + return NULL; > > } > > > > pci_dev->devfn = devfn; > > -- > > 2.39.1 > >
On Thu, Jun 22, 2023 at 7:48 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Thu, Jun 22, 2023 at 05:46:40PM +0200, Julia Suvorova wrote: > > On Thu, Jun 22, 2023 at 12:34 PM Ani Sinha <anisinha@redhat.com> wrote: > > > > > > PCI Express ports only have one slot, so PCI Express devices can only be > > > plugged into slot 0 on a PCIE port. Enforce it. > > > > > > 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 | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > > index bf38905b7d..5f25ab9f5e 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,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, > > > name); > > > > > > return NULL; > > > + } else if (pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) { > > > + error_setg(errp, "PCI: slot %d is not valid for %s," > > > + " PCI express devices can only be plugged into slot 0.", > > > > This is not technically correct, because downstream ports and root > > ports are also PCIe devices, and they can have different slots under > > upstream ports and RC. But this error will never be shown for them, so > > it seems fine. > > Hmm. Confusing users is not nice ... I agree this might > make people think they can not use root ports in slot !=0 either. > > Would you add "with an upstream port"? > E.g. "PCI Express devices with an upstream port" ? This whole upstream port conditioning is quite confusing. How about "%parent device% only allows plugging into slot 0"? Best regards, Julia Suvorova. > > > > Reviewed-by: Julia Suvorova <jusual@redhat.com> > > > > > > > > > > > + PCI_SLOT(devfn), name); > > > + return NULL; > > > } > > > > > > pci_dev->devfn = devfn; > > > -- > > > 2.39.1 > > > >
On Fri, Jun 23, 2023 at 04:57:20PM +0200, Julia Suvorova wrote: > On Thu, Jun 22, 2023 at 7:48 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Thu, Jun 22, 2023 at 05:46:40PM +0200, Julia Suvorova wrote: > > > On Thu, Jun 22, 2023 at 12:34 PM Ani Sinha <anisinha@redhat.com> wrote: > > > > > > > > PCI Express ports only have one slot, so PCI Express devices can only be > > > > plugged into slot 0 on a PCIE port. Enforce it. > > > > > > > > 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 | 6 ++++++ > > > > 1 file changed, 6 insertions(+) > > > > > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > > > index bf38905b7d..5f25ab9f5e 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,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, > > > > name); > > > > > > > > return NULL; > > > > + } else if (pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) { > > > > + error_setg(errp, "PCI: slot %d is not valid for %s," > > > > + " PCI express devices can only be plugged into slot 0.", > > > > > > This is not technically correct, because downstream ports and root > > > ports are also PCIe devices, and they can have different slots under > > > upstream ports and RC. But this error will never be shown for them, so > > > it seems fine. > > > > Hmm. Confusing users is not nice ... I agree this might > > make people think they can not use root ports in slot !=0 either. > > > > Would you add "with an upstream port"? > > E.g. "PCI Express devices with an upstream port" ? > > This whole upstream port conditioning is quite confusing. > How about "%parent device% only allows plugging into slot 0"? > > Best regards, Julia Suvorova. Good idea! > > > > > > Reviewed-by: Julia Suvorova <jusual@redhat.com> > > > > > > > > > > > > > > > > + PCI_SLOT(devfn), name); > > > > + return NULL; > > > > } > > > > > > > > pci_dev->devfn = devfn; > > > > -- > > > > 2.39.1 > > > > > >
> On 26-Jun-2023, at 2:41 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Fri, Jun 23, 2023 at 04:57:20PM +0200, Julia Suvorova wrote: >> On Thu, Jun 22, 2023 at 7:48 PM Michael S. Tsirkin <mst@redhat.com> wrote: >>> >>> On Thu, Jun 22, 2023 at 05:46:40PM +0200, Julia Suvorova wrote: >>>> On Thu, Jun 22, 2023 at 12:34 PM Ani Sinha <anisinha@redhat.com> wrote: >>>>> >>>>> PCI Express ports only have one slot, so PCI Express devices can only be >>>>> plugged into slot 0 on a PCIE port. Enforce it. >>>>> >>>>> 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 | 6 ++++++ >>>>> 1 file changed, 6 insertions(+) >>>>> >>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >>>>> index bf38905b7d..5f25ab9f5e 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,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, >>>>> name); >>>>> >>>>> return NULL; >>>>> + } else if (pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) { >>>>> + error_setg(errp, "PCI: slot %d is not valid for %s," >>>>> + " PCI express devices can only be plugged into slot 0.", >>>> >>>> This is not technically correct, because downstream ports and root >>>> ports are also PCIe devices, and they can have different slots under >>>> upstream ports and RC. But this error will never be shown for them, so >>>> it seems fine. >>> >>> Hmm. Confusing users is not nice ... I agree this might >>> make people think they can not use root ports in slot !=0 either. >>> >>> Would you add "with an upstream port"? >>> E.g. "PCI Express devices with an upstream port" ? >> >> This whole upstream port conditioning is quite confusing. >> How about "%parent device% only allows plugging into slot 0"? >> >> Best regards, Julia Suvorova. > > Good idea! OK I will send out another iteration once Igor has a chance to take a look at the test fixes or someone else reviews them. > >>>> >>>> Reviewed-by: Julia Suvorova <jusual@redhat.com> >>>> >>>> >>>> >>>> >>>>> + 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..5f25ab9f5e 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,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, name); return NULL; + } else if (pcie_has_upstream_port(pci_dev) && PCI_SLOT(devfn)) { + error_setg(errp, "PCI: slot %d is not valid for %s," + " PCI express devices can only be plugged into slot 0.", + PCI_SLOT(devfn), name); + return NULL; } pci_dev->devfn = devfn;
PCI Express ports only have one slot, so PCI Express devices can only be plugged into slot 0 on a PCIE port. Enforce it. 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 | 6 ++++++ 1 file changed, 6 insertions(+)