On Wed, Oct 20, 2021 at 10:58:12AM -0400, Michael S. Tsirkin wrote: > On Wed, Oct 20, 2021 at 10:09:17AM -0400, Eduardo Habkost wrote: > > On Wed, Oct 20, 2021 at 01:02:24PM +0800, Jason Wang wrote: > > > On Wed, Oct 20, 2021 at 9:31 AM Jason Wang <jasow...@redhat.com> wrote: > > > > > > > > On Wed, Oct 20, 2021 at 12:56 AM Eduardo Habkost <ehabk...@redhat.com> > > > > 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? > > > > > > > > Yes. > > > > > > > > Acked-by: Jason Wang <jasow...@redhat.com> > > > > > > > > Thanks > > > > > > > > > > > > > > Signed-off-by: Eduardo Habkost <ehabk...@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"}, > > > > > > Rethink about this, any chance that we can use "virtio-net-pci" as the > > > base_name? It looks to me this can cause less confusion and consistent > > > with the existing compat properties. > > > > It's probably too late now: we can't change the semantics of > > "-global virtio-net-pci" without breaking compatibility. > > You mean someone playing with virtio-net-pci-base and friends? > We could maybe make virtio-net-pci-base be an alias to > virtio-net-pci.
I mean someone using "-global virtio-net-pci" with a VM that has virtio-net-pci*transitional devices. > > > The original reasoning for making generic_name != base_name is at > > this comment in struct VirtioPCIDeviceTypeInfo: > > > > /* > > * Common base class for the subclasses below. > > * > > * Required only if transitional_name or non_transitional_name is set. > > * > > * We need a separate base type instead of making all types > > * inherit from generic_name for two reasons: > > * 1) generic_name implements INTERFACE_PCIE_DEVICE, but > > * transitional_name does not. > > * 2) generic_name has the "disable-legacy" and "disable-modern" > > * properties, transitional_name and non_transitional name don't. > > */ > > const char *base_name; > > > > (I had to look it up. I didn't remember the original reason for that) > > > Maybe we can find a different way to address these. Jason, any ideas? (2) above is not a big deal, but (1) was supposed to be useful for management software to identify which devices can be plugged where. I completely agree that the: * virtio-pci * virtio-net-pci-base * virtio-net-pci * virtio-net-pci-transitional * virtio-net-pci-non-transitional hierarchy is not obvious, but I do believe it is too late to change it, because the QOM type hierarchy defines user-visible behavior when using -global. -- Eduardo