Re: [Qemu-devel] [PATCH 0/4] pcie: Add support for Single Root I/O Virtualization

[Knut Omang]

On Fri, 2014-08-29 at 18:17 +0200, Michael S. Tsirkin wrote:
> On Fri, Aug 29, 2014 at 09:17:05AM +0200, Knut Omang wrote:
> > This patch set consists of two parts:
> > 
> >  - The two first patches implements SR/IOV building blocks in pcie/pci.
> >    I have held this patch back for a while because I haven't had a good
> >    example test case to accompany it, but in the light of the latest 
> >    developments such as the discussion we have had around ARI and downstream
> >    switches and root ports, I think it would be valuable to get this in,
> >    to make it easier for people to experiment with creating devices with
> >    many functions. Hopefully, I can also get some help to fix the hotplug
> >    issues described below.
> > 
> >  - The two last patches contains an example to illustrate the usage
> >    of the new SR/IOV support. The example leverages the e1000 
> >    code and the fact that registers between E1000 and the PCIe based 
> >    Intel 82576 Gigabit Ethernet Adapter (which supports SR/IOV) are quite 
> > similar, 
> >    but so far without considering much of the differences beyond the 
> >    bare minimum needed to trick the igb driver into loading to the point 
> >    where VFs can be enabled.
> > 
> >    So you cannot yet use these PF or VF Ethernet devices to send ethernet 
> > packets,
> >    and the implementation do not in any way attempt to model the internals
> >    of igb such as the multiple queues or any multiplexing onto the same 
> > device, 
> >    it only instantiates the VFs as well as the PFs mostly directly by 
> > inheritance
> >    (and as separate devices) from E1000, but it should hopefully be 
> > relatively
> >    easy to understand how to proceed to make "true" VFs.
> >    It was also a nice exercise in using QOM.
> > 
> >    The changes to E1000 to accommodate igb (patch 3) should be fairly 
> >    non-intrusive, nevertheless I suppose it should not be applied 
> >    unless it will eventually lead to a new derived device which is enough
> >    different from E1000 to qualify for a separate set of source files.
> >    So if someone with more detailed knowledge of the internal differences
> >    between igb and e1000 on the functional level might have input, that 
> > would be
> >    great, an alternative of course be to only apply the two first 
> >    patches, and leave any usage examples for the future.
> > 
> >    To test and see how it plays out, you can add something like this
> >    to the command line:
> > 
> >      -device ioh3420,slot=0,id=pcie_port.0
> >      -device igb,mac=DE:AD:BE:EE:04:18,vlan=1,bus=pcie_port.0
> >      -net tap,vlan=1
> >      
> >    The Linux igb driver does not yet support changing the VF setup via sysfs
> >    (the preferred way since kernel v.3.8) so to see some VFs on the guest, 
> >    you need to set up a modprobe file with something like this:
> > 
> >       options igb max_vfs=4
> > 
> >    For some reason I dont yet understand, removal of the igb driver
> >    does not cause a 0 write to sr/iov vf_enable to disable the VFs again, 
> > eg.
> > 
> >        # lspci | grep '^01:00'
> >        01:00.0 Ethernet controller: Intel Corporation 82576 Gigabit Network 
> > Connection (rev 01)
> >        01:00.1 Ethernet controller: Intel Corporation 82576 Virtual 
> > Function (rev 01)
> >        01:00.2 Ethernet controller: Intel Corporation 82576 Virtual 
> > Function (rev 01)
> >        01:00.3 Ethernet controller: Intel Corporation 82576 Virtual 
> > Function (rev 01)
> >        01:00.4 Ethernet controller: Intel Corporation 82576 Virtual 
> > Function (rev 01)
> > 
> >    Writing directly to vf_enable using setpci works though, but after the 
> > later hotplug 
> >    changes, a little too well, as after the VF removals have succeded, 
> >    some way a slot power down gets triggered, also removing the PF:
> > 
> >       # setpci -s 01:00.0 168.b
> >       0x19
> >       # setpci -s 01:00.0 168.b=18
> >       # lspci | grep '^01:00'
> >       <nothing>
> > 
> >    For similar reasons attaching the igb device directly on the root 
> > complex (just remove
> >    bus parameter above) attempts to enable VFs will fail with 
> >       qdev.c:89: bus_add_child: Assertion `bus->allow_hotplug' failed
> > 
> >    I imagine this could for instance be a matter of defining a property 
> > "subfunction" 
> >    or similar that allows qdev (QOM?) to discriminate between main devices 
> > and devices 
> >    representing subfunctions of another main device. Suggestions on how to 
> > proceed on this
> >    welcome.
> > 
> > This patch depends (by diff context only) on my patch:
> > 
> > [PATCH v2 1/4] pcie: Fix incorrect write to the ari capability next 
> > function field
> > 
> > and for stability on Paolo's 
> > 
> > [PATCH] pci_bridge: manually destroy memory regions within PCIBridgeWindows
> > 
> > both of which Michael has pulled, but which are not in master yet.
> > 
> > Thanks,
> 
> So practically you would like patches 1 and 2 applied,
> and 3 and 4 are RFC?

Yes, I suppose that's a better way of formulating my intentions, thanks!

Knut

> > Knut Omang (4):
> >   pci: Update pci_regs header
> >   pcie: Add support for Single Root I/O Virtualization (SR/IOV)
> >   e1000: Refactor to allow subclassing from other source file
> >   igb: Example code to illustrate the SR/IOV support.
> > 
> >  hw/i386/kvm/pci-assign.c  |   4 +-
> >  hw/misc/vfio.c            |   8 +-
> >  hw/net/Makefile.objs      |   2 +-
> >  hw/net/e1000.c            | 126 ++--------------
> >  hw/net/e1000.h            | 139 +++++++++++++++++
> >  hw/net/igb.c              | 293 ++++++++++++++++++++++++++++++++++++
> >  hw/net/igb_regs.h         |  27 ++++
> >  hw/pci/msi.c              |   4 -
> >  hw/pci/msix.c             |   2 +-
> >  hw/pci/pci.c              | 107 +++++++++----
> >  hw/pci/pcie.c             | 205 ++++++++++++++++++++++++-
> >  include/hw/pci/pci.h      |   6 +-
> >  include/hw/pci/pci_regs.h | 371 
> > ++++++++++++++++++++++++++++++++++------------
> >  include/hw/pci/pcie.h     |  26 ++++
> >  14 files changed, 1072 insertions(+), 248 deletions(-)
> >  create mode 100644 hw/net/e1000.h
> >  create mode 100644 hw/net/igb.c
> >  create mode 100644 hw/net/igb_regs.h
> > 
> > -- 
> > 1.9.0