[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs
From: |
Michael S. Tsirkin |
Subject: |
Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs |
Date: |
Wed, 20 Oct 2021 10:58:12 -0400 |
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 <jasowang@redhat.com> wrote:
> > >
> > > On Wed, Oct 20, 2021 at 12:56 AM Eduardo Habkost <ehabkost@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 <jasowang@redhat.com>
> > >
> > > Thanks
> > >
> > > >
> > > > 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"},
> >
> > 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.
> 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?
> --
> Eduardo
- [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs, (continued)
- [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs, Jean-Louis Dupond, 2021/10/12
- Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs, Jean-Louis Dupond, 2021/10/12
- Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs, Stefan Hajnoczi, 2021/10/19
- Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs, Michael S. Tsirkin, 2021/10/19
- Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs, Eduardo Habkost, 2021/10/19
- Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs, Michael S. Tsirkin, 2021/10/19
- Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs, Eduardo Habkost, 2021/10/19
- Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs, Jason Wang, 2021/10/19
- Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs, Jason Wang, 2021/10/20
- Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs, Eduardo Habkost, 2021/10/20
- Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs,
Michael S. Tsirkin <=
- Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs, Eduardo Habkost, 2021/10/20
- Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs, Jean-Louis Dupond, 2021/10/20
- Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs, Michael S. Tsirkin, 2021/10/20
- Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs, Eduardo Habkost, 2021/10/20
- Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs, Michael S. Tsirkin, 2021/10/20
- Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs, Eduardo Habkost, 2021/10/20
- Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs, Michael S. Tsirkin, 2021/10/20
Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs, Eduardo Habkost, 2021/10/19