| Message ID | 1453130846-14064-1-git-send-email-marcel@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 01/18/16 16:27, Marcel Apfelbaum wrote: > PCI devices can't be plugged directly into PCI extra root bridges > because their resources can't be computed by firmware before the ACPI > tables are loaded. > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > --- > > Hi, > > This patch follows the discussion: > https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg01484.html > > Thanks, > Marcel > > hw/pci/pci.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 168b9cc..584f504 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -850,6 +850,13 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, > DeviceState *dev = DEVICE(pci_dev); > > pci_dev->bus = bus; > + /* Only pci bridges can be attached to extra PCI root buses */ > + if (pci_bus_is_root(bus) && bus->parent_dev && !pc->is_bridge) { > + error_setg(errp, > + "PCI: Only PCI/PCIe bridges can be plugged into %s", > + bus->parent_dev->name); > + return NULL; > + } > > if (devfn < 0) { > for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices); > Looks sensible to me (and the commit message actually enlightens me a bit :)), but you could very easily trick me in this area, so I'll leave formal review to others. :) Laszlo
On 18 January 2016 at 15:27, Marcel Apfelbaum <marcel@redhat.com> wrote: > PCI devices can't be plugged directly into PCI extra root bridges > because their resources can't be computed by firmware before the ACPI > tables are loaded. > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > --- > > Hi, > > This patch follows the discussion: > https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg01484.html Is it definitely the case that no current working command lines plug PCI devices directly into these things (including on platforms that don't have anything to do with ACPI at all) ? thanks -- PMM
On 01/18/16 19:08, Peter Maydell wrote: > On 18 January 2016 at 15:27, Marcel Apfelbaum <marcel@redhat.com> wrote: >> PCI devices can't be plugged directly into PCI extra root bridges >> because their resources can't be computed by firmware before the ACPI >> tables are loaded. >> >> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> >> --- >> >> Hi, >> >> This patch follows the discussion: >> https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg01484.html > > Is it definitely the case that no current working command lines plug > PCI devices directly into these things (including on platforms that > don't have anything to do with ACPI at all) ? No clue about "pxb-pcie", but re: "pxb", the documentation and examples by Marcel (see: "docs/pci_expander_bridge.txt") will certainly continue working, with this patch place. And, that text file is authoritative for pxb, since Marcel (et al) wrote the code directly for the purposes described in the txt. (But I'll let Marcel answer too! :)) Thanks Laszlo
On 18 January 2016 at 18:16, Laszlo Ersek <lersek@redhat.com> wrote: > On 01/18/16 19:08, Peter Maydell wrote: >> On 18 January 2016 at 15:27, Marcel Apfelbaum <marcel@redhat.com> wrote: >>> PCI devices can't be plugged directly into PCI extra root bridges >>> because their resources can't be computed by firmware before the ACPI >>> tables are loaded. >>> >>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> >>> --- >>> >>> Hi, >>> >>> This patch follows the discussion: >>> https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg01484.html >> >> Is it definitely the case that no current working command lines plug >> PCI devices directly into these things (including on platforms that >> don't have anything to do with ACPI at all) ? > > No clue about "pxb-pcie", but re: "pxb", the documentation and examples > by Marcel (see: "docs/pci_expander_bridge.txt") will certainly continue > working, with this patch place. And, that text file is authoritative for > pxb, since Marcel (et al) wrote the code directly for the purposes > described in the txt. That may be true, but the patch is changing core code in hw/pci/pci.c and it's not obvious to me that that's going to be limited to just pxb or pxb-pcie; so it seemed worth asking. thanks -- PMM
On 01/18/2016 08:16 PM, Laszlo Ersek wrote: > On 01/18/16 19:08, Peter Maydell wrote: >> On 18 January 2016 at 15:27, Marcel Apfelbaum <marcel@redhat.com> wrote: >>> PCI devices can't be plugged directly into PCI extra root bridges >>> because their resources can't be computed by firmware before the ACPI >>> tables are loaded. >>> >>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> >>> --- >>> >>> Hi, >>> >>> This patch follows the discussion: >>> https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg01484.html >> >> Is it definitely the case that no current working command lines plug >> PCI devices directly into these things (including on platforms that >> don't have anything to do with ACPI at all) ? Hi, The PXB devices can work only on ACPI based platforms, but currently work only on PC Machines. So for other platforms are out of the scope. I understand the issue in putting it generic PCI code, but: - Non ACPI platforms (implemented in QEMU) do not support extra PCI host bridges (at least yet) - Even when extra host bridges will be supported, there are are several ways to implement it and most of them will not require their pxbs to have a parent_device. The presence of a parent device is a pretty solid lead that is a "snooping bridge" and as far as I know is only typical for the existing solution. Now the explanation of the issue we want to solve: - pxb (PCI expander bridge) - it already has an internal bridge, using -device pxb,bus80,id=pxb1 -device e1000,bus=pxb1 will land the device on a built-in pci bridge. - An incorrect command-line will result in a non working device without the proposed patch. - pxb-pcie (PCIe Root Complex) - it does not have an internal bridge and trying to use: -device pxb-pcie,bus80,id=pxb1 -device e1000,bus=pxb1 will fail. This patch ensures non of that can happen. Last word: I did consider another option, adding a "bridges-only" property (defaulted to false) to PCIBus class and leverage the fact that the pxb internal buses derive from it(and it can be set to true). Then we can simply check PCI_BUS_CLASS(bus)->bridges-only but it seemed a little odd since we don't have that limitation on the real world. I am not against it, if it is preferred I'll submit a new patch. > > No clue about "pxb-pcie", but re: "pxb", the documentation and examples > by Marcel (see: "docs/pci_expander_bridge.txt") will certainly continue > working, with this patch place. And, that text file is authoritative for > pxb, since Marcel (et al) wrote the code directly for the purposes > described in the txt. and that reminds me I need to update the doc for pxb-pcie, thanks Laszlo! Marcel > > (But I'll let Marcel answer too! :)) > > Thanks > Laszlo >
On 01/19/2016 10:04 AM, Marcel Apfelbaum wrote: > On 01/18/2016 08:16 PM, Laszlo Ersek wrote: >> On 01/18/16 19:08, Peter Maydell wrote: >>> On 18 January 2016 at 15:27, Marcel Apfelbaum <marcel@redhat.com> wrote: >>>> PCI devices can't be plugged directly into PCI extra root bridges >>>> because their resources can't be computed by firmware before the ACPI >>>> tables are loaded. >>>> >>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> >>>> --- >>>> >>>> Hi, >>>> >>>> This patch follows the discussion: >>>> https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg01484.html >>> >>> Is it definitely the case that no current working command lines plug >>> PCI devices directly into these things (including on platforms that >>> don't have anything to do with ACPI at all) ? > > Hi, > > The PXB devices can work only on ACPI based platforms, but currently work only on PC Machines. > So for other platforms are out of the scope. > > I understand the issue in putting it generic PCI code, but: > - Non ACPI platforms (implemented in QEMU) do not support extra PCI host bridges (at least yet) > - Even when extra host bridges will be supported, there are are several ways to implement it > and most of them will not require their pxbs to have a parent_device. The presence of a parent device > is a pretty solid lead that is a "snooping bridge" and as far as I know is only typical for the existing solution. > > Now the explanation of the issue we want to solve: > - pxb (PCI expander bridge) - it already has an internal bridge, using > -device pxb,bus80,id=pxb1 -device e1000,bus=pxb1 > will land the device on a built-in pci bridge. > - An incorrect command-line will result in a non working device without the proposed patch. > - pxb-pcie (PCIe Root Complex) - it does not have an internal bridge and trying to use: > -device pxb-pcie,bus80,id=pxb1 -device e1000,bus=pxb1 > will fail. > > This patch ensures non of that can happen. > > Last word: > I did consider another option, adding a "bridges-only" property (defaulted to false) to PCIBus class > and leverage the fact that the pxb internal buses derive from it(and it can be set to true). > > Then we can simply check PCI_BUS_CLASS(bus)->bridges-only but it seemed a little odd since we > don't have that limitation on the real world. > I am not against it, if it is preferred I'll submit a new patch. Ping. Can we merge this patch? Thanks, Marcel > >> >> No clue about "pxb-pcie", but re: "pxb", the documentation and examples >> by Marcel (see: "docs/pci_expander_bridge.txt") will certainly continue >> working, with this patch place. And, that text file is authoritative for >> pxb, since Marcel (et al) wrote the code directly for the purposes >> described in the txt. > > and that reminds me I need to update the doc for pxb-pcie, thanks Laszlo! > Marcel > > >> >> (But I'll let Marcel answer too! :)) >> >> Thanks >> Laszlo >> > >
diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 168b9cc..584f504 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -850,6 +850,13 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, DeviceState *dev = DEVICE(pci_dev); pci_dev->bus = bus; + /* Only pci bridges can be attached to extra PCI root buses */ + if (pci_bus_is_root(bus) && bus->parent_dev && !pc->is_bridge) { + error_setg(errp, + "PCI: Only PCI/PCIe bridges can be plugged into %s", + bus->parent_dev->name); + return NULL; + } if (devfn < 0) { for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
PCI devices can't be plugged directly into PCI extra root bridges because their resources can't be computed by firmware before the ACPI tables are loaded. Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> --- Hi, This patch follows the discussion: https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg01484.html Thanks, Marcel hw/pci/pci.c | 7 +++++++ 1 file changed, 7 insertions(+)