* Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs
2021-10-19 16:56 ` Eduardo Habkost
@ 2021-10-20 1:31 ` Jason Wang
2021-10-20 5:02 ` Jason Wang
2021-10-20 7:00 ` Jean-Louis Dupond
2021-10-20 7:41 ` Michael S. Tsirkin
2 siblings, 1 reply; 24+ messages in thread
From: Jason Wang @ 2021-10-20 1:31 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Daniel P. Berrangé,
Michael S. Tsirkin, Stefan Hajnoczi, qemu-devel,
Jean-Louis Dupond, pbonzini
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"},
> };
> const size_t hw_compat_5_2_len = G_N_ELEMENTS(hw_compat_5_2);
>
> --
> Eduardo
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs
2021-10-20 1:31 ` Jason Wang
@ 2021-10-20 5:02 ` Jason Wang
2021-10-20 14:09 ` Eduardo Habkost
0 siblings, 1 reply; 24+ messages in thread
From: Jason Wang @ 2021-10-20 5:02 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Daniel P. Berrangé,
Michael S. Tsirkin, Stefan Hajnoczi, qemu-devel,
Jean-Louis Dupond, pbonzini
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.
Thanks
> > };
> > const size_t hw_compat_5_2_len = G_N_ELEMENTS(hw_compat_5_2);
> >
> > --
> > Eduardo
> >
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs
2021-10-20 5:02 ` Jason Wang
@ 2021-10-20 14:09 ` Eduardo Habkost
2021-10-20 14:58 ` Michael S. Tsirkin
0 siblings, 1 reply; 24+ messages in thread
From: Eduardo Habkost @ 2021-10-20 14:09 UTC (permalink / raw)
To: Jason Wang
Cc: Daniel P. Berrangé,
Michael S. Tsirkin, Stefan Hajnoczi, qemu-devel,
Jean-Louis Dupond, pbonzini
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.
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)
--
Eduardo
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs
2021-10-20 14:09 ` Eduardo Habkost
@ 2021-10-20 14:58 ` Michael S. Tsirkin
2021-10-20 15:46 ` Eduardo Habkost
0 siblings, 1 reply; 24+ messages in thread
From: Michael S. Tsirkin @ 2021-10-20 14:58 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Daniel P. Berrangé,
Stefan Hajnoczi, Jason Wang, qemu-devel, Jean-Louis Dupond,
pbonzini
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs
2021-10-20 14:58 ` Michael S. Tsirkin
@ 2021-10-20 15:46 ` Eduardo Habkost
0 siblings, 0 replies; 24+ messages in thread
From: Eduardo Habkost @ 2021-10-20 15:46 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Daniel P. Berrangé,
Stefan Hajnoczi, Jason Wang, qemu-devel, Jean-Louis Dupond,
pbonzini
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 <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.
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs
2021-10-19 16:56 ` Eduardo Habkost
2021-10-20 1:31 ` Jason Wang
@ 2021-10-20 7:00 ` Jean-Louis Dupond
2021-10-20 7:41 ` Michael S. Tsirkin
2 siblings, 0 replies; 24+ messages in thread
From: Jean-Louis Dupond @ 2021-10-20 7:00 UTC (permalink / raw)
To: Eduardo Habkost, Michael S. Tsirkin
Cc: Stefan Hajnoczi, qemu-devel, pbonzini, berrange, jasowang
On 19/10/2021 18:56, 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);
>
That patch fixes it indeed!
Acked-by: Jean-Louis Dupond <jean-louis@dupond.be>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs
2021-10-19 16:56 ` Eduardo Habkost
2021-10-20 1:31 ` Jason Wang
2021-10-20 7:00 ` Jean-Louis Dupond
@ 2021-10-20 7:41 ` Michael S. Tsirkin
2021-10-20 13:57 ` Eduardo Habkost
2 siblings, 1 reply; 24+ messages in thread
From: Michael S. Tsirkin @ 2021-10-20 7:41 UTC (permalink / raw)
To: Eduardo Habkost
Cc: berrange, Stefan Hajnoczi, jasowang, qemu-devel,
Jean-Louis Dupond, pbonzini
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?
> --
> Eduardo
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs
2021-10-20 7:41 ` Michael S. Tsirkin
@ 2021-10-20 13:57 ` Eduardo Habkost
2021-10-20 14:55 ` Michael S. Tsirkin
0 siblings, 1 reply; 24+ messages in thread
From: Eduardo Habkost @ 2021-10-20 13:57 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: berrange, Stefan Hajnoczi, jasowang, qemu-devel,
Jean-Louis Dupond, pbonzini
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs
2021-10-20 13:57 ` Eduardo Habkost
@ 2021-10-20 14:55 ` Michael S. Tsirkin
2021-10-20 15:01 ` Eduardo Habkost
0 siblings, 1 reply; 24+ messages in thread
From: Michael S. Tsirkin @ 2021-10-20 14:55 UTC (permalink / raw)
To: Eduardo Habkost
Cc: berrange, Stefan Hajnoczi, jasowang, qemu-devel,
Jean-Louis Dupond, pbonzini
On Wed, Oct 20, 2021 at 09:57:37AM -0400, Eduardo Habkost wrote:
> 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)
OK so ... that's another breakage then. Suggestions how to fix?
>
> --
> Eduardo
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs
2021-10-20 14:55 ` Michael S. Tsirkin
@ 2021-10-20 15:01 ` Eduardo Habkost
2021-10-20 15:16 ` Michael S. Tsirkin
0 siblings, 1 reply; 24+ messages in thread
From: Eduardo Habkost @ 2021-10-20 15:01 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: berrange, Stefan Hajnoczi, jasowang, qemu-devel,
Jean-Louis Dupond, pbonzini
On Wed, Oct 20, 2021 at 10:55:59AM -0400, Michael S. Tsirkin wrote:
> On Wed, Oct 20, 2021 at 09:57:37AM -0400, Eduardo Habkost wrote:
> > 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)
>
> OK so ... that's another breakage then. Suggestions how to fix?
What exactly is another breakage? virtio-net-pci,
virtio-net-pci-non-transitional, and virtio-net-pci-transitional
are three distinct device types.
--
Eduardo
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] hw/qdev-core: Add compatibility for (non)-transitional devs
2021-10-20 15:01 ` Eduardo Habkost
@ 2021-10-20 15:16 ` Michael S. Tsirkin
0 siblings, 0 replies; 24+ messages in thread
From: Michael S. Tsirkin @ 2021-10-20 15:16 UTC (permalink / raw)
To: Eduardo Habkost
Cc: berrange, Stefan Hajnoczi, jasowang, qemu-devel,
Jean-Louis Dupond, pbonzini
On Wed, Oct 20, 2021 at 11:01:43AM -0400, Eduardo Habkost wrote:
> On Wed, Oct 20, 2021 at 10:55:59AM -0400, Michael S. Tsirkin wrote:
> > On Wed, Oct 20, 2021 at 09:57:37AM -0400, Eduardo Habkost wrote:
> > > 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)
> >
> > OK so ... that's another breakage then. Suggestions how to fix?
>
> What exactly is another breakage? virtio-net-pci,
> virtio-net-pci-non-transitional, and virtio-net-pci-transitional
> are three distinct device types.
Hmm. I guess ... good point.
> --
> Eduardo
^ permalink raw reply [flat|nested] 24+ messages in thread