Re: [libvirt] [PATCHv3 04/11] Add virtio revision attribute to controllers

On 08/12/2016 04:59 PM, Ján Tomko wrote: > On Fri, Aug 12, 2016 at 10:47:13AM +0200, Cornelia Huck wrote: >> On Thu, 11 Aug 2016 16:17:10 +0200 >> Ján Tomko <jtomko(a)redhat.com&gt; wrote: >> >>> On Thu, Aug 11, 2016 at 02:31:55PM +0100, Daniel P. Berrange wrote: >>> >On Thu, Aug 11, 2016 at 03:25:53PM +0200, Ján Tomko wrote: >>> >> On Thu, Aug 11, 2016 at 01:00:08PM +0100, Daniel P. Berrange wrote: >>> >> > On Wed, Aug 10, 2016 at 03:27:15PM +0200, Ján Tomko wrote: >>> >> > > <controller type='scsi' index='0' model='virtio-scsi'> >>> >> > > <virtio revision='0.9'/> >>> >> > >>> >> > I'm wondering about generalizing this. eg what if there are >>> >> > other device models where we want the ability to set a >>> >> > revision. We don't really want to invent a new sub-elment >>> >> > named after each device model >>> >> >>> >> Not even a new attribute? :) >>> >> <revision virtio='0.9'/> >>> >> >>> >> How about: >>> >> <revision type='virtio' version='0.9'/> >>> > >>> >Both of those are quite repetative - we already know its virtio. >>> > >>> >>> I guess one device having <revision>s of different types is unlikely. >>> >>> >Most devices we have alrady include a <driver> or <model> sub-element, >>> >so we should really just add a revision= attrbute to those existing >>> >>> What I liked about having it as a separate element is that it can be >>> repeated, e.g.: >>> <revision type='virtio' version='0.9'/> >>> <revision type='virtio' version='1.0'/> >>> >>> for a device with both 0.9 and 1.0. >>> >>> I could not come up with a nice way to represent that in a single >>> attribute: >>> * '0.9+1.0' feels like the two values should rather be separated >>> at the XML level >>> * 'all' will not be true if there happens to be another virtio >>> revision in the future >> >> [not a libvirt developer, but let me comment from the qemu virtio >> perspective] >> >> I don't think you are expressing the concept of virtio (standard) >> revisions (more like releases!) here correctly. Let me elaborate: >> >> - The disable-legacy/disable-modern attributes are virtio-pci only. >> Moreover, they don't express 'compliant to virtio-1.0' or so: They do >> exactly What It Says On The Tin. A device with both disable attributes >> off is in fact virtio-1.0 compliant (transitional devices are >> compliant), as is a device with disable-legacy off. But it might also >> be virtio-1.1 compliant! (That's the most likely release of the >> standard in the near future.) >> >> - virtio-ccw does not have the concept of these disable switches. >> Instead, there are virtio-ccw specific 'revisions' which count upwards >> and may be limited by the 'max_revision' attribute. However, this is >> not an attribute that is supposed to be set by the user, but for >> backwards compatibility only. Unlike pci, ccw has nothing to gain by >> disabling legacy support. >> >> - We may actually want to add some kind of versioning scheme to virtio >> devices in future versions of the standard. But that's just a very >> vague idea right now. >> >> Am I right in assuming that you simply want to be able to control >> whether your virtio-pci devices are legacy, transitional or modern? > > Yes. > >> Then I think you'd be best off adding these as virtio-pci attributes >> only and leave the concept of a 'virtio revision' for the future when >> we might introduce it in the standard. >> > > So XML like this: > <model legacy='on/off' modern='on/off'/> > or > <model compatibility='legacy/transitional/modern'/> > (which could possibly be reused for other hypervisors with a similar > concept, not just QEMU and virtio) Sorry to be a pain in the bud... but... both above proposals are virtio-PCI only.