Re: [Qemu-devel] VFIO PCIe Extended Capabilities

[Marcel Apfelbaum]

On 07/19/2016 09:30 PM, Alex Williamson wrote:
> On Tue, 19 Jul 2016 21:16:40 +0300
> Marcel Apfelbaum <address@hidden> wrote:
>
> > On 07/19/2016 08:55 PM, Alex Williamson wrote:
> > > On Tue, 19 Jul 2016 11:22:29 -0600
> > > Alex Williamson <address@hidden> wrote:
> > >
> > > > On Tue, 19 Jul 2016 17:12:45 +0000
> > > > Spenser Gilliland <address@hidden> wrote:
> > > >
> > > > > Hi,
> > > > >
> > > > > I noticed your patches "vfio: add pcie extended capability support" and "vfio/pci: 
> > > > > Hide SR-IOV capability" have gone into qemu mainline.  These look really good, and thanks so 
> > > > > much for doing these.
> > > > >
> > > > > I was wondering if there were any side effects to removing the 
> > > > > pci_bus_is_express check on line 1776 of hw/vfio/pci.c .
> > > > >
> > > > >       /* Only add extended caps if we have them and the guest can see them */
> > > > > -   if (!pci_is_express(pdev) || !pci_bus_is_express(pdev->bus) ||
> > > > > +  if (!pci_is_express(pdev) ||
> > > > >           !pci_get_long(pdev->config + PCI_CONFIG_SPACE_SIZE)) {
> > > > >           return 0;
> > > > >       }
> > > > >
> > > > > I'm asking because it looks like the defaults for libvirt/OpenStack are to create a 
> > > > > "hostdev" stanza in the libvirt xml to define this pass through condition.  However, it 
> > > > > also appears that the "hostdev" stanza only supports pci-bridge bus connections.  Thus, 
> > > > > it's not easily possible to use this patch in a libvirt/OpenStack environment as the bus is 
> > > > > technically a non-express bus.  It looks like adding PCIe bus support to libvirt/OpenStack may be a 
> > > > > lot more effort than a simple workaround here.
> > > > >
> > > > > I have tested this on my local system and it does work as intended for my use 
> > > > > case.  The following is from an OpenStack VM and shows that the 0x340 extended 
> > > > > configuration space is passed through correctly.  I've also done testing which 
> > > > > uses this space and the results are positive.
> > > >
> > > > If the bus is not express then extended capabilities on the device
> > > > should not be accessible, this would be a QEMU bug for allowing it.
> > > > Cc'ing Marcel for that.  Thanks,
> >
> > Hi Spencer,
> >
> > Indeed, if a device is attached to a PCI bus it makes no sense to advertise the 
> > extended configuration space.
> > Can you please share the QEMU command line? Maybe is possible to make the 
> > device's bus PCIe in QEMU?
>
> I think that any instance of a q35 machine where the assigned device is
> placed on the conventional PCI bridge will create this scenario.  It's
> the default for attaching devices to a libvirt managed q35 VM AFAIK.

Yes, I discussed with Laine from libvirt the possibility to assign
devices to a PCIe port instead.


> > > In fact, I've tried to fix this multiple times:
> > >
> > > https://lists.nongnu.org/archive/html/qemu-devel/2015-10/msg05384.html
> > > https://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg02422.html
> > > https://lists.nongnu.org/archive/html/qemu-devel/2016-01/msg03259.html
> > >
> > > Yet the patch remains unapplied :(
> >
> > I thought is it in already. Maybe Michael can add it as part of the hard freeze.
> > And if the patch will be applied, the tweak above wouldn't help, right Alex?
>
> The tweak Spenser suggested above should be unnecessary with my proposed
> patch applied.

I think I finally understand this. The bus is not pcie -> we return
from vfio_add_ext_cap without "breaking" the extended capabilities
chain and the bare metal SR-IOV capability will be visible to guest.
With your patch the PCI bridge will "interfere" and mask the extended
configuration space completely.

  Only now searching for that patch did I notice Michael's
> comment hidden at the bottom of his reply, which I assume is why it
> never got applied:
>
> https://patchwork.kernel.org/patch/8057411/

I just saw it too! It seems Michael wants to cache this info
in device instead of re-calculating it every time.

> Anyway, the current behavior is clearly a bug, so QEMU hard freeze
> should be irrelevant.  If anyone wants to take over the patch, feel
> free.  Thanks,

I suppose I can handle it, but sadly not for 2.7.
If Spencer has some time now I can help by testing it and reviewing it quickly 
:)

Thanks,
Marcel

> Alex