Re: [Qemu-devel] [RFC] virtio-pci: Allow PCIe virtio devices on root bus

[Laszlo Ersek]

On 02/09/17 05:16, David Gibson wrote:
> On Wed, Feb 08, 2017 at 11:40:50AM +0100, Laszlo Ersek wrote:
>> On 02/08/17 07:16, David Gibson wrote:
>>> Marcel,
>>>
>>> Your original patch adding PCIe support to virtio-pci.c has the
>>> limitation noted below that PCIe won't be enabled if the device is on
>>> the root bus (rather than under a root or downstream port).  As
>>> reasoned below, I think removing the check is correct, even for x86
>>> (though it would rarely be useful there).  But I could well have
>>> missed something.  Let me know if so...
>>>
>>>
>>>
>>> Virtio devices can appear as either vanilla PCI or PCI-Express devices
>>> depending on the bus they're connected to.  At the moment it will only
>>> appear as vanilla PCI if connected to the root bus of a PCIe host bridge.
>>>
>>> Presumably this is to reflect the fact that PCIe devices usually need to
>>> be connected to a root (or further downstream) port rather than directly
>>> on the root bus.  However, due to the odd requirements of the PAPR spec on 
>>> the 'pseries'
>>> machine type, it's normal for PCIe devices to appear on the root bus
>>> without root ports.
>>>
>>> Further, even on x86, there's no inherent reason we couldn't present a
>>> virtio device as an "integrated device" (typically used for things built
>>> into the PCI chipset), and those devices *do* typically appear on the root
>>> bus.
>>
>> I'm not personally making a counter-argument, just qouting some of
>> the relevant parts of "docs/pcie.txt" ("PCI EXPRESS GUIDELINES"):
> 
> So, an earlier discussion more or less concluded that the PCIe
> guidelines don't really work with PAPR guests.  That comes because
> PAPR was designed with PowerVM in mind which allows PCI passthrough
> but doesn't do any emulated PCI devices.  So they wanted to present
> passed through devices (virtual or phyical) to the guest without
> inserting virtual root ports.
> 
> Now, you can argue that this was a silly decision in PAPR, and you
> could well be right, but there it is.

I can totally accept this, but then we should state it as a fact near
the top of "docs/pcie.txt".

> 
>>> Place only the following kinds of devices directly on the Root Complex:
>>>     (1) PCI Devices (e.g. network card, graphics card, IDE controller),
>>>         not controllers. Place only legacy PCI devices on
>>>         the Root Complex. These will be considered Integrated Endpoints.
>>>         Note: Integrated Endpoints are not hot-pluggable.
>>>
>>>         Although the PCI Express spec does not forbid PCI Express devices as
>>>         Integrated Endpoints, existing hardware mostly integrates legacy PCI
>>>         devices with the Root Complex.
> 
> "Mostly".. on my laptop at least the GPU shows up as an integrated PCI
> Express endpoint, so it's certainly not the case that *all* root bus
> devices are legacy.
> 
>> Guest OSes are suspected to behave
>>>         strangely when PCI Express devices are integrated
>>>         with the Root Complex.
> 
> Clearly not that strangely, that often, since my laptop works just fine.
> 
>>>
>>>     [...]
>>>
>>> 2.2 PCI Express only hierarchy
>>> ==============================
>>> Always use PCI Express Root Ports to start PCI Express hierarchies.
>>
>> Above you mention "it's normal for PCIe devices to appear on the root bus 
>> without root ports".
> 
> Well "normal" perhaps wasn't the right word.  Let's say precedented,
> if uncommon.
> 
>> Let me turn the question around: is it a *problem* for "pseries" if
>> we require root ports? If so, why exactly?
> 
> That's.. a complex question.  At least Linux guests (and we don't
> support any others yet) might cope with the addition of root ports.
> Maybe.  I have discussed this option with BenH and others.
> 
> However it is gratuitously different from how PCIe devices will
> typically appear for the same guest running under PowerVM.  Although I
> suspect Linux would cope with the "normal standard" rather than "PAPR
> standard" presentation, I'm not as confident about it as I would like.
> 
> Another consideration here is that other PCIe capable qemu emulated
> devices, such as XHCI, will present fine as PCIe integrated endpoints
> when attached to the root bus.  Libvirt won't do that usually, of
> course, and it may not be the recommended way of doing things (on PC)
> but it's possible.  I don't see any particular reason that virtio-pci
> should enforce the root port requirement more so than any other
> device.
> 
>> On 02/08/17 07:16, David Gibson wrote:
>>>
>>> pcie_endpoint_cap_init() already automatically adjusts to advertise as
>>> an integrated device rather than a "normal" PCIe endpoint when attached to
>>> a root bus.  So we can remove the check for root bus within virtio and
>>> allow (at the user's discretion) a PCIe virtio bus to be attached to a
>>> root bus.
>>
>> If Marcel thinks this is a good change, then I think we should go
>> through "docs/pcie.txt" with a fine-toothed comb, and update all
>> relevant spots. (If Marcel agrees, perhaps you can include such
>> hunks in your patch at once.)
> 
> Actually, I think that would be a neverending process.  Maybe better
> to put in a whole different spapr-pcie.txt with the assorted ways that
> PAPR violates PCIe conventions.

That works for me too, but I think it would be a lot more work for you
and others.

I plan on consulting "docs/pcie.txt" frequently; among other things, for
deciding debates. Thus, improving the scope of "docs/pcie.txt" is very
welcome IMO.

> 
>> It also may have consequences for libvirt (but I see you addressed
>> Andrea at once, which is great).
> 
> Right, I've been discussed this with Andrea all along.  We're working
> on a proposed PAPR specific way of allocating PCI and PCIe addresses
> (different from the PCIe normal way, but the same as each other).
> That will simplify adding PCIe support to PAPR, and also has some
> other advantages for PAPR guests (related to the platform specific
> isolation, hotplug  and error recovery mechanisms - also different
> from the normal PCIe ones).

Great, if Andrea is aware, that's a relief.

Can you resubmit this patch with a small hunk for "docs/pcie.txt" that
removes PAPR from the scope? A short list of actual machine types would
be appreciated too, if that makes sense. (By default we aim at
multi-arch / multi-target with this document; we may not have stated it
explicitly, but AFAIR we intend to cover aarch64 / "virt" too.)

Thanks!
Laszlo

> 
>>
>> Thanks,
>> Laszlo
>>
>>>
>>> Signed-off-by: David Gibson <address@hidden>
>>> ---
>>>  hw/virtio/virtio-pci.c | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>>> index 5ce42af..caea44c 100644
>>> --- a/hw/virtio/virtio-pci.c
>>> +++ b/hw/virtio/virtio-pci.c
>>> @@ -1737,8 +1737,7 @@ static void virtio_pci_realize(PCIDevice *pci_dev, 
>>> Error **errp)
>>>  {
>>>      VirtIOPCIProxy *proxy = VIRTIO_PCI(pci_dev);
>>>      VirtioPCIClass *k = VIRTIO_PCI_GET_CLASS(pci_dev);
>>> -    bool pcie_port = pci_bus_is_express(pci_dev->bus) &&
>>> -                     !pci_bus_is_root(pci_dev->bus);
>>> +    bool pcie_port = pci_bus_is_express(pci_dev->bus);
>>>  
>>>      if (!kvm_has_many_ioeventfds()) {
>>>          proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;
>>>
>>
>