Re: [Qemu-devel] [PATCH V2 RESEND] docs: add PCIe devices placement guidelines

[Laszlo Ersek]

On 10/27/16 13:27, Marcel Apfelbaum wrote:
> On 10/14/2016 02:36 PM, Laszlo Ersek wrote:
>> On 10/13/16 16:05, Marcel Apfelbaum wrote:
>>> On 10/13/2016 04:52 PM, Marcel Apfelbaum wrote:

>>>> +      -device
>>>> pci-bridge,id=pci_bridge1,bus=dmi_pci_bridge1[,chassis_nr=x][,addr=y]
>>>> \
>>>> +      -device <dev>,bus=pci_bridge1[,addr=x]
>>
>> {13} It would be nice to spell out the valid device addresses (y and x)
>> here too -- can we use 0 for them? SHPC again?
>>
>> Can we / should we simply go with >=1 device addresses?
>>
> 
> For pci-bridges only - yes. A better idea (I think) is to disable SHPC
> by default
> from the next QEMU version. Make this slot usable. Sounds OK?

Sure.

>>>> +        Don't use PCI Express Switches if you don't have too, they use
>>>> +        an extra PCI bus that may handy to plug another device id it
>>>> comes to it.
>>>> +
>>
>> {25} I'd put it as: Don't use PCI Express Switches if you don't have
>> too, each one of those uses an extra PCI bus (for its Upstream Port)
>> that could be put to better use with another Root Port or Downstream
>> Port, which may come handy for hotplugging another device.
>>
>> {26} Another remark (important to me) in this section: the document
>> doesn't state firmware expectations. It's clear the firmware is expected
>> to reserve no IO space for PCI Express Downstream Ports and Root Ports,
>> but what about MMIO?
>>
>> We discussed this at length with Alex, but I think we didn't conclude
>> anything. It would be nice if firmware received some instructions from
>> this document in this regard, even before we implement our own ports and
>> bridges in QEMU.
>>
> 
> Hmm, I have no idea what to add here, except:
> The firmware is expected to reserve at least 2M for each pci bridge?

Just ignore {26} for now please. I'll come back to this later when we
have our own (generic) ports.

>>>> +5.3 Hot plug example:
>>>> +Using HMP: (add -monitor stdio to QEMU command line)
>>>> +  device_add <dev>,id=<id>,bus=<pcie.0/PCI Express Root Port
>>>> Id/PCI-PCI bridge Id/pxb-pcie Id>
>>
>> {27} I think the bus=<...> part is incorrect here. Based on the rest of
>> the guidelines, we have to specify the ID of:
>> - a PCI Express Root Port, or
>> - a PCI Express Downstream Port, or
>> - a PCI-PCI bridge.
>>
> 
> I don't get it, you specify what you wrote above as the bus, right?
> For example if you start the machine with
>     .... -device ioh3420,id=root_port1,
> you hotplug with: device_add e1000,bus=root_port1.

My point is that your example names

  bus=<pcie.0/PCI Express Root Port Id/PCI-PCI bridge Id/pxb-pcie Id>

which suggests that the following bus *types* can receive hotplugged
devices:
(a) main root bus ("pcie.0")
(c) root port ("PCI Express Root Port Id")
(c) PCI-PCI bridge ("PCI-PCI bridge Id")
(d) extra root bus ("pxb-pcie Id")

Based on the rest of the guidelines, suggestions (a) and (d) are invalid
-- those bus types cannot accept hotplugged devices --, plus option (e)
is missing, namely:
(e) downstream port.

In other words, your example IDs *infer* bus types, and the set of bus
types inferred is both wrong (incorrect elements) and incomplete (one
correct element missing).

Therefore my proposal is to provide the following example:

  bus=<PCI Express Root Port Id/PCI Express Downstream Port Id/PCI-PCI
bridge Id>

That's all.

Cheers
Laszlo