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

[David Gibson]

On Tue, Feb 14, 2017 at 02:53:08PM +0200, Marcel Apfelbaum wrote:
> On 02/14/2017 06:15 AM, David Gibson wrote:
> > On Mon, Feb 13, 2017 at 12:14:23PM +0200, Marcel Apfelbaum wrote:
> > > On 02/13/2017 06:33 AM, David Gibson wrote:
> > > > On Sun, Feb 12, 2017 at 09:05:46PM +0200, Marcel Apfelbaum wrote:
> > > > > On 02/10/2017 02:37 AM, David Gibson wrote:
> > > > > > 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?
> > > > > > 
> > > > > 
> > > > > Hi David,
> > > > > Sorry for the delay, I just came back from PTO.
> > > > > 
> > > > > > 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.
> > > > > > 
> > > > > 
> > > > > There are two reasons for keeping virtio Integrated Endpoints as PCI 
> > > > > devices.
> > > > > 1. The first point is generic; even if having PCIe devices as 
> > > > > Integrated Endpoints should be OK,
> > > > >    is not recommended because some guests may miss-behave (*). X86 
> > > > > arch supports a large number
> > > > >    of guests and we don't want to check and fix everything if *we 
> > > > > don't have to*.
> > > > >    Even if is not written anywhere and there are actually some PCI 
> > > > > Express Integrated Endpoints,
> > > > >    most of them are legacy PCI devices (I actually think this is why 
> > > > > we have Integrated Endpoints
> > > > >    at all, but I might be wrong).
> > > > 
> > > > Hm, ok.  Could we implement that restriction in the pci/pcie core
> > > > rather than in the virtio device?
> > > 
> > > I am not sure if we should do that. Most of the devices are PCI or PCIe.
> > > Only some devices are "hybrid", the virtio devices, XHCI and I am not 
> > > sure we have more.
> > 
> > Ok, I see your point, the pcie core might not be right.  But it still
> > seems really weird to have it explicitly in each hybrid device, even
> > if it's just the two.  As the code stands right now, XHCI and virtio
> > have different behaviour, without a clear reason for it.
> > 
> 
> I suppose XHCI can behave the same as virtio if Gerd has nothing against it
> (remain PCI if plugged into the Root Complex), but I don't know how it will 
> help your case.

> > Maybe a hybrid_setup() helper function?
> > 
> 
> How would it help PAPR scenario?

It doesn't, directly, but it means there would only be a single place
to fix with a PAPR hack, instead of both virtio and XHCI and any
future hybrid devices.

> Anyway, Eduardo is working on supplying
> new query interfaces for libvirt and he touches this subject, I think
> he does plan to mark the hybrid devices in some way.
> 
> 
> > > > That would then protect things like
> > > > XHCI as well.
> > > 
> > > I don't see a reason to have XHCI as Integrated Endpoint, I think it 
> > > should be always
> > > plugged into a root port. (for x86. arm and power)
> > 
> > No, not for Power.  Well, ok, yes for most ppc machine types, but not
> > for the paravirtualized 'pseries' machine (which is the one we most
> > care about).
> > 
> > Well.. I guess it doesn't need to be an Integrated Endpoint per se,
> > but we should be able to have it appear on the guest root bus.
> > 
> > The thing to realize is that the paravirtualized PCI interfaces in
> > PAPR mean there's very little guest visible difference between PCI and
> > PCIe.  In fact in most ways you could say that the paravirtualized bus
> > operates like a vanilla PCI bus.. except that it does provide a way to
> > access PCIe extended config space.
> 
> Maybe we can have a new bus type deriving from PCIBus and sibling of the PCIe 
> Bus.
> Then we can have the same rules as the PCI bus and add tweaks when
> necessary.

Sounds possible.  I guess it would need to return true for
pci_bus_is_express(), so that PCIe devices will attach to it.  In
which case I'm not entirely sure how it would differ from a PCIe bus
from the qemu side.  AFAICT with the exception of the virtio check and
maybe a handful of others, the PCIe placement rules are handled by
libvirt, rather than by qemu.

> This does not solve the virtio problem since the code is actively looking for
> a PCIe Root Port and we don't have it for PAPR.

Technically, it's not even looking for a root port, just excluding
being on the root bus itself.  I guess any kind of bridge that has
something PCI-ish on the upstream side and PCIe on the downstream side
more or less has to be either a root port or a PCIe switch downstream
port, though.

>   Which means that you can use it to
> > drive PCIe devices just fine.  "Bus level" PCIe extensions like AER
> > and PCIe standard hotplug won't work, but PAPR has its own mechanisms
> > for those (common between PCI and PCIe).
> > 
> > I did float the idea of having the pseries PCI bus remain plain PCI
> > but with a special flag to allow PCIe devices to be attached to it
> > anyway.  It wasn't greeted with much enthusiasm..
> > 
> 
> Can you point me to the discussion please? It seems similar to what I 
> proposed above.

Sorry, I was misleading.  I think I just raised that idea with Andrea
and a few other people internally, not on one of the lists at large.

> As you properly described it, is much closer to PCI then PCIe, even the only 
> characteristic
> that makes it "a little" PCIe, the Extended Configuration Space support,
> is done with an alternative interface.
> 
> I agree the PAPR bus is not PCIe.

Ok, so if we take that direction, the question becomes how do we let
PCIe devices plug into this mostly-not-PCIe bus.  Maybe introduce a
"pci_bus_accepts_express()" function that will replace many, but not
all current uses of "pci_bus_is_express()"?

Such a helper could maybe simplify the logic in virtio-pci (and XHCI?)
by returning false on an x86 root bus.

> 
> > >   And for my purposes it would also make it easier to
> > > > implement aa machine type specific hook to re-allow that configuration
> > > > on pseries.
> > > 
> > > I agree we need a solution for PAPR.
> > > 
> > > What about a pcie_papr() function and then:
> > > 
> > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > index 5ce42af..2c646ae 100644
> > > --- a/hw/virtio/virtio-pci.c
> > > +++ b/hw/virtio/virtio-pci.c
> > > @@ -1804,7 +1804,7 @@ static void virtio_pci_realize(PCIDevice *pci_dev, 
> > > Error **errp)
> > >          return;
> > >      }
> > > 
> > > -    if (pcie_port && pci_is_express(pci_dev)) {
> > > +    if ((pcie_port || pcie_parp()) && pci_is_express(pci_dev)) {
> > >          int pos;
> > > 
> > >          pos = pcie_endpoint_cap_init(pci_dev, 0);
> > 
> > That would be sufficient, yes, so I'll take it if we don't come to any
> > other solutions.
> 
> OK
> 
>  It still seems weird to me to have this logic within
> > a specific device implementation though.
> > 
> 
> I suppose you have a point, let's wait for Gerd and maybe Michael to comment
> on that, but anyway it will not help your case. (it will only make XHCI PCI 
> if plugged into Root Complex)

It won't help directly, but it means we could put that pcie_papr()
test in just the common code, rather than having to look at every
hybrid and PCIe device.

> > > > 
> > > > At the moment XHCI and virtio-pci behave differently, which seems less
> > > > than ideal.
> > > > 
> > > > > 2. The second point is virtio specific. Not all the guests have 
> > > > > virtio 1.0 support (e.g RHEL 6) and we allow them
> > > > >    to use legacy virtio devices as Integrated Endpoints (following 
> > > > > the thought that this is why we have Integrated Endpoints)
> > > > >    Making the virtio devices PCI Express, but not virtio 1.0 is also 
> > > > > problematic since now we will have too much
> > > > >    types of virtio devices. We want to keep it simple: virtio legacy
> > > > >    -> PCI, virtio modern -> PCIe.
> > > > 
> > > > Ok.. it's not obvious to me why integrated endpoint vs. under a root
> > > > port is relevant to this.  Can't we enable/disable PCIe mode based
> > > > directly on the legacy/modern settings?
> > > > 
> > > 
> > > Yes, we can, but we don't want to do that. Previous setups will stop 
> > > working
> > > and we will need libvirt to mange the disable-* properties.
> > > As a matter of fact the code today is after some discussions with libvirt 
> > > guys.
> > 
> > Uh.. I think I'm missing something.. but it's still not clear to me
> > why this would break existing setups or impose more work on libvirt.
> > 
> 
> Long story short, libvirt guys don't want to manually set the disable-* 
> properties
> of virtio devices to make them comply to our goal (legacy -> PCI, modern 
> PCIe).
> They also don't want to look at QEMU version/machine type to make a decision 
> on the above properties.
> 
> I agree the solution is not perfect, but at least it makes our testing matrix 
> smaller
> and makes our PCIe guidelines a little easier to understand (e.g. all 
> supported devices are PCIe,
> but if want legacy PCI devices put them on the Root Complex)

Ok.. I'm still missing something.  Are you saying that instead of the
legacy/modern status determining PCIe support, that PCIe status is
determining legacy/modern support by default?  That would actually
seem to simplify things: whatever method we end up allowing PCIe
virtio on PAPR, that should automatically enable modern mode, which is
fine.

Although.. I do wonder about legacy/modern for PAPR in general.
Current kernels will support virtio 1.0 fine, but we have older
released versions which only support legacy.  Unlike PC we won't (I
hope) have a whole machine type switch to handle this.  At this stage
I think we want virtio devices (whatever their bus type) on PAPR to
default to allowing both legacy and modern for the forseeable future.
How does that affect the matrix?

> XHCI being PCIe on Root Complex is an unintended exception, but we want it 
> connected to a
> Root Port anyway, we don't have anything to gain from having it as Integrated 
> Endpoint.
> We only loose a slot that can be used by 8 Root Ports assembled into one 
> multi-function device.
> 
> PAPR bus should not be considered PCIe and should have a different set of 
> rules allowing PCIe
> devices to be plugged into Root Complex.

Alright, I can work with that.  Michael, Andrea, how does this idea
seem to you?

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

signature.asc
Description: PGP signature