On Mon, 5 Dec 2016 19:14:40 +0100 Halil Pasic <pa...@linux.vnet.ibm.com> wrote:
> On 12/05/2016 06:41 PM, Eduardo Habkost wrote: > > On Mon, Dec 05, 2016 at 06:25:55PM +0100, Cornelia Huck wrote: > >> > On Mon, 5 Dec 2016 14:48:29 -0200 > >> > Eduardo Habkost <ehabk...@redhat.com> wrote: > >> > > >>> > > On Mon, Dec 05, 2016 at 04:42:00PM +0100, Cornelia Huck wrote: > >>>> > > > On Mon, 05 Dec 2016 16:21:22 +0100 > >>>> > > > Greg Kurz <gr...@kaod.org> wrote: > >>>> > > > > >>>>> > > > > The current code recursively applies global properties from > >>>>> > > > > child up to > >>>>> > > > > parent. So, if you have: > >>>>> > > > > > >>>>> > > > > -global virtio-pci.disable-modern=on > >>>>> > > > > -global virtio-blk-pci.disable-modern=off > >>>>> > > > > > >>>>> > > > > Then the default value of disable-modern for a virtio-blk-pci > >>>>> > > > > device is on, > >>>>> > > > > which looks wrong from an OOP perspective. > >>>>> > > > > > >>>>> > > > > This patch reverses the logic, so that a child property always > >>>>> > > > > prevail. > >>>> > > > > >>>> > > > This sounds reasonable... > >>>> > > > > >>>>> > > > > > >>>>> > > > > This fixes a subtle bug that got introduced in 2.7 with commit > >>>>> > > > > "9a4c0e220d8a > >>>>> > > > > hw/virtio-pci: fix virtio behaviour" for older (< 2.7) machine > >>>>> > > > > types: the > >>>>> > > > > HW_COMPAT_2_6 macro contains global virtio-pci.disable-* > >>>>> > > > > properties which > >>>>> > > > > would silently override global properties passed on the command > >>>>> > > > > line for > >>>>> > > > > virtio subtypes. > >>>>> > > > > > >>>>> > > > > Signed-off-by: Greg Kurz <gr...@kaod.org> > >>>>> > > > > --- > >>>>> > > > > > >>>>> > > > > AFAIK, libvirt's XML doesn't know about modern/legacy modes for > >>>>> > > > > virtio > >>>>> > > > > devices. Early adopters of virtio 1.0 had to rely on the > >>>>> > > > > <qemu:commandline> > >>>>> > > > > tag to pass global properties to QEMU. This patch ensures that > >>>>> > > > > XML files > >>>>> > > > > used with older machine types remain valid with newer versions > >>>>> > > > > of QEMU. > >>>>> > > > > > >>>>> > > > > FWIW I guess it could help to have this fix in 2.8, and also > >>>>> > > > > probably in > >>>>> > > > > 2.7.1. > >>>> > > > > >>>> > > > ...but I'm a bit worried about doing that change this late in the > >>>> > > > cycle, as we may introduce subtle changes for other > >>>> > > > configurations. At > >>>> > > > the very least, we should look over the existing backwards compat > >>>> > > > properties (I'll look at those I'm familiar with). > >>> > > > >>> > > This patch would change the behavior for: > >>> > > -global virtio-blk-pci.disable-modern=on > >>> > > -global virtio-pci.disable-modern=off > >>> > > > >>> > > And I am not sure the new behavior would be correct. Shouldn't we > >>> > > apply the properties in the order specified in the command-line? > >> > > >> > Probably; but how should this interact with compat props? > > compat props should be always applied in the order they appear. > > -global should always be applied after compat props. > > > > So, it looks like we have two additional reasons to just follow > > the order the global properties were registered. > > > > How about a docs update? > > Given the current doc: > """ > -global driver.prop=value > -global driver=driver,property=property,value=value > Set default value of driver's property prop to value, e.g.: > > qemu-system-i386 -global ide-drive.physical_block_size=4096 -drive > file=file,if=ide,index=0,media=disk > > In particular, you can use this to set driver properties for devices which > are created automatically by the machine model. To create a device which > is > not created automatically and set properties on it, use -device. > > -global driver.prop=value is shorthand for -global > driver=driver,property=prop, > value=value. The longhand syntax works even when > driver contains a dot. > """ > I think this OOP argument, which I do not completely understand, With the current code, properties from the parent classes implicitly prevail and this has nothing to do with command line order, or order of appearance in HW_COMPAT_*. From an OOP perspective, we usually expect child classes to override parent classes behavior, not the contrary. > is not the right direction. > > Yet I do not think the current state of documentation gives a > definitive answer on what behavior should take place in the > scenarios described above. > True, the documentation doesn't mention anything about the QEMU object model. And the user cannot even think that virtio-pci exists and is the parent of virtio-blk-pci... > Maybe it's my mistake, but I did not find a statement about > the order in which global properties are to be applied > (please point me to it if I've missed it). > True, so this should be the implicit command line order. > Another problem with the doc (IMHO) is that it's not > really defined what a driver is. So I can't even tell if > -global virtio-pci.disable-modern=off is even legit on > the command line. > True, virtio-pci.disable-modern is an internal compat thing and it silentyl overrides virtio-blk-pci.disable-modern which is a legit option. Passing virtio-pci.disable-modern to the command line is just messing with undocumented internals... and should probably be disallowed. > Regarding the order compat props. applied before command line it > makes a lot of sense to me with the documentation stating "In > particular, you can use this to set driver properties for devices which > are created automatically by the machine model." > I agree. So, as written several times in this thread, the only thing we can do now is changing the virtio-pci compat props to be applied to each subtype, so that they don't silently undo -global. Cheers. -- Greg