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

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.) Precedent: This is exactly patterned after the use of <model> in <video> devices though, where the specific card being emulated along with any memory/etc options are put in <model>, so at least there is precedence) [3] <controller type='pci model='pci-root-port' index='3'> <address type='pci' bus='0' slot='4' function='1'> <device type='ioh3420' chassis='3' port='0x21'/> </controller> (This one is completely new, just for a fresh start in case people think neither of the others are exactly right) At this point you can see that it all comes down to what name we want to give the subelement; within that, we give the exact name of the qemu device, and the exact name/value of any qemu options that we set to non-default values. Somebody *please* have an opinion on the name for this, because none of these strikes me as better or worse (well, I think I dislike <driver>

(P.S. There are other "automagic" qemu options being specified by libvirt that maybe could use the same treatment as these. Two that come to mind are chassis_nr for pci-bridge controllers (set to the controller's index), and "vectors" for virtio-net multiqueue support (set to (queues*2)+2).