Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs

[Eduardo Habkost]

On Wed, Oct 20, 2021 at 03:41:38AM -0400, Michael S. Tsirkin wrote:
> On Tue, Oct 19, 2021 at 12:56:11PM -0400, Eduardo Habkost wrote:
> > On Tue, Oct 19, 2021 at 12:13:17PM -0400, Michael S. Tsirkin wrote:
> > > On Tue, Oct 19, 2021 at 11:29:13AM -0400, Eduardo Habkost wrote:
> > > > On Tue, Oct 19, 2021 at 06:59:09AM -0400, Michael S. Tsirkin wrote:
> > > > > On Tue, Oct 19, 2021 at 11:46:17AM +0100, Stefan Hajnoczi wrote:
> > > > > > On Tue, Oct 12, 2021 at 10:36:01AM +0200, Jean-Louis Dupond wrote:
> > > > > > > Forgot to CC maintainers.
> > > > > > 
> > > > > > Also CCing Jason Wang and Michael Tsirkin for VIRTIO.
> > > > > > 
> > > > > > Stefan
> > > > > 
> > > > > OMG
> > > > > where all compat properties broken all the time?
> > > > 
> > > > Compat properties that existed when commit f6e501a28ef9 ("virtio:
> > > > Provide version-specific variants of virtio PCI devices") was
> > > > merged are not broken, because virtio-*-transitional and
> > > > virtio-*-non-transitional were brand new QOM types (so there's no
> > > > compatibility to be kept with old QEMU versions).
> > > > 
> > > > Compat properties referencing "virtio-*-pci" instead of
> > > > "virtio-*-pci-base" added after commit f6e501a28ef9 are probably
> > > > broken, yes.
> > > > 
> > > > -- 
> > > > Eduardo
> > > 
> > > Oh. So just this one:
> > >     { "virtio-net-pci", "vectors", "3"},
> > > 
> > > right?
> > 
> > I think so.  That's the only post-4.0 virtio-*-pci compat property I see in
> > hw/core/machine.c.
> > 
> > pc.c doesn't have any post-4.0 virtio-*-pci compat props.  I didn't see any
> > virtio compat props on spapr.c and s390-virtio-ccw.c.
> > 
> > > 
> > > about the patch: how do people feel about virtio specific
> > > stuff in qdev core? Ok by everyone?
> > 
> > Not OK, if we have a mechanism to avoid that, already (the
> > "virtio-net-pci-base" type name).  I wonder what we can do to
> > make this kind of mistake less likely, though.
> > 
> > Jean-Louis, Jason, does the following fix work?
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index b8d95eec32d..bd9c6156c1a 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -56,7 +56,7 @@ GlobalProperty hw_compat_5_2[] = {
> >      { "ICH9-LPC", "smm-compat", "on"},
> >      { "PIIX4_PM", "smm-compat", "on"},
> >      { "virtio-blk-device", "report-discard-granularity", "off" },
> > -    { "virtio-net-pci", "vectors", "3"},
> > +    { "virtio-net-pci-base", "vectors", "3"},
> >  };
> >  const size_t hw_compat_5_2_len = G_N_ELEMENTS(hw_compat_5_2);
> 
> Hmm I'm a bit confused at this point, as to why does
> specifying properties for virtio-net-pci on command
> line with -global work, but in compat list doesn't. Do others
> understand?

I don't think that's the case.  -global behaves similarly to compat_props.

Running an unpatched QEMU 6.1.0 binary:

$ echo -e 'info qtree\nquit' | qemu-system-x86_64 -device virtio-net-pci 
-machine pc-q35-5.2 -monitor stdio | grep vectors
        vectors = 3 (0x3)
$ echo -e 'info qtree\nquit' | qemu-system-x86_64 -device 
virtio-net-pci-non-transitional -machine pc-q35-5.2 -monitor stdio | grep 
vectors
        vectors = 4 (0x4)
$ echo -e 'info qtree\nquit' | qemu-system-x86_64 -device 
virtio-net-pci-non-transitional -global virtio-net-pci.vectors=3 -monitor stdio 
| grep vectors
        vectors = 4 (0x4)
$ echo -e 'info qtree\nquit' | qemu-system-x86_64 -device 
virtio-net-pci-non-transitional -global virtio-net-pci-base.vectors=3 -monitor 
stdio | grep vectors
        vectors = 3 (0x3)


-- 
Eduardo