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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 7392 bytes --]

On Thu, Feb 09, 2017 at 10:04:47AM +0100, Laszlo Ersek wrote:
> 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?

Well, first I'd like to see if Marcel knows of some reason I didn't
think of why this test is important for virtio particularly here.  But
assuming the basic idea is acceptable, then yes, I'll update pcie.txt.

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

Right, that was my understanding as well.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]