[libvirt] [PATCH 00/13] PCIe fixes + new PCI controllers w/RFC

Wed Jun 24 16:17:33 UTC 2015

On Wed, Jun 24, 2015 at 12:04:33PM -0400, Laine Stump wrote:
> I think the only votes were for option 1 and 4 (interesting how the only
> ones that were chosen were those that I *didn't* pick personally :-).
> See comments below. In the meantime the other issue Alex pointed out may
> cause this to take a slightly different direction.
> 
> On 06/22/2015 02:44 PM, Laine Stump wrote:
> > ===
> > Idea 1: multiplex the meaning of the "model" attribute, so we currently have:
> >
> >   <controller type='pci' model='dmi-to-pci-bridge'/>
> >
> > which means "add an i82801b11-bridge device" and when we add a generic
> > version of this type of controller, we would do it with something like:
> >
> >   <controller type='pci' model='generic-dmi-to-pci-bridge'/>
> >
> > and for another vendor's mythical controller:
> >
> >   <controller type='pci' model='xyz-dmi-to-pci-bridge'/>
> >
> > Cons: This will make for ugliness in switch statements where a new
> > case will have to be added whenever a different controller with
> > similar behavior/usage is supported. And it's generally not a good idea to
> > have a single attribute be used for two different functions.
> 
> jtomko advocated this one because it would make it easier for an older
> libvirt to notice an unsupported class+model of controller during a
> migration attempt from a newer libvirt. An example would be if a newer
> libvirt had a guest with
> 
>     <controller type='pci' model='xyz'/>
> 
> (where xyz is some qemu device that implements a dmi-to-pci-bridge)
> 
> That would fail on an attempt to migrate to older libvirt, but
> 
>    <controller type='pci' model='dmi-to-pci-bridge'/>
> 
> would succeed. On the other hand, if we did this:
> 
>    <controller type='pci' model='dmi-to-pci-bridge submodel='xyz'/>
> 
> that would succeed (and create the wrong device) because submodel would
> be ignored.
> 
> Since there is currently no alternate implementation of a
> dmi-to-pci-bridge available in qemu, and it will likely be awhile until
> that happens, I think if we start filling out the XML now as:
> 
>    <controller type='pci' model='dmi-to-pci-bridge'
>                           submodel='i82801b11-bridge'/>
> 
> by the time we get to the point that we do have an alternate 'xyz'
> controller, any version of libvirt that doesn't understand "submodel"
> will be so far in the past that we wouldn't want to be able to migrate
> back to it anyway.
> 
> 
> > ===
> > Idea 4: Unlike other uses of "model" in libvirt, for pci controllers,
> > continue to use "model" to denote the subtype/class/whatever of
> > controller, and create a new attribute to denote the different
> > specific implementations of each one. So for example:
> >
> > [4]  <controller type='pci' model='dmi-to-pci-bridge'/>
> >
> > would become:
> >
> > [5]  <controller type='pci' model='dmi-to-pci-bridge'
> >                             implementation='i82801b11-bridge'/>
> >
> > (or some other name in place of "implementation" - ideas? I'm horrible
> > at thinkin up names)
> >
> > Pros: wouldn't create compatibility problems when downgrading or
> > migrating cross version.
> >
> > Cons: Is inconsistent with every other use of "model" attribute in
> > libvirt, and each new addition of a PCI controller further propagates
> > this misuse.
> >
> 
> mkletzan preferred this one, and danpb was amenable to it in IRC. I
> think I now agree, but Alex's comments about needing to keep track of
> what we put in the qemu commandline for port and chassis have me
> thinking this isn't enough.
> 
> The problem is that the "ioh3420" device needs its "port" and "chassis"
> options set; Alex recommends setting port to:
> 
>    (slot << 3)+function
> 
> Likewise, he recommends setting "port" for the xio3130-downstream device
> to the same value as slot. So, for the following:
> 
>    <controller type='pci model='pci-root-port' index='3'>
>      <address type='pci' bus='0' slot='4' function='1'>
>    </controller>
> 
> we would end up with the following commandline:
> 
>    -device ioh3420,chassis=3,port=0x21,id='pcie.3',bus='pcie.0',
>            addr=0x4.0x1
> 
> (chassis is the same as "index" in the xml (again per Alex's
> recommendation), and port is (4 << 3) + 1 = 0x21)
> 
> *However* he also says that we may change our mind on these in the
> future, so chassis and port may end up being something different. Since
> those values are visible to the guest, we can't allow them to change on
> an existing machine, as it breaks the guest ABI. So we need to store
> them in the XML. They aren't part of the PCI address though, they are
> options. So I need to figure out the best way to represent that in the
> XML, and fill it in when it is auto-generated when the controller is
> defined. A few possible ways to solve both problems at once:
> 
> [1] <controller type='pci model='pci-root-port' index='3'>
>       <address type='pci' bus='0' slot='4' function='1'>
>       <target type='ioh3420' chassis='3' port='0x21'/>
>     </controller>
> 
> Precedent: <target> is used to store the port number for serial/console
> devices, specify guest-side bus and device name for disks, specify
> guest-side mount location for filesystems, specify the *host*-side name
> of tap devices for network interfaces, memory size for <memory>. So it
> seems kind of proper. (slight variation - <target model='ioh3420' .../>
> :-/ )
> 
> [2] <controller type='pci model='pci-root-port' index='3'>
>       <address type='pci' bus='0' slot='4' function='1'>
>       <model type='ioh3420' chassis='3' port='0x21'/>
>     </controller>
> 
> (How's *that* for confusing?!? Both a model attribute *and* a model
> subelement.)

That's no worse than model & submodel really. If anything it is better
as having a child element gives more room for expansion than an
attribute, and <model> is still a fairly standard approach we've
used inpast.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|