[Qemu-devel] [PATCH] hw/pci: ensure that only PCI/PCIe bridges can be attached to pxb/pxb-pcie devices

[Qemu-devel] [PATCH] hw/pci: ensure that only PCI/PCIe bridges can be attached to pxb/pxb-pcie devices

[Marcel Apfelbaum]

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);
-- 
2.4.3

Re: [Qemu-devel] [PATCH] hw/pci: ensure that only PCI/PCIe bridges can be attached to pxb/pxb-pcie devices

[Laszlo Ersek]

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

Re: [Qemu-devel] [PATCH] hw/pci: ensure that only PCI/PCIe bridges can be attached to pxb/pxb-pcie devices

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] hw/pci: ensure that only PCI/PCIe bridges can be attached to pxb/pxb-pcie devices

[Laszlo Ersek]

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

Re: [Qemu-devel] [PATCH] hw/pci: ensure that only PCI/PCIe bridges can be attached to pxb/pxb-pcie devices

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] hw/pci: ensure that only PCI/PCIe bridges can be attached to pxb/pxb-pcie devices

[Marcel Apfelbaum]

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
>

Re: [Qemu-devel] [PATCH] hw/pci: ensure that only PCI/PCIe bridges can be attached to pxb/pxb-pcie devices

[Marcel Apfelbaum]

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
>>
>
>