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

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?

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.