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

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

[Jean-Louis Dupond]

hw_compat modes only take into account their base name.
But if a device is created with (non)-transitional, then the compat
values are not used, causing migrating issues.

This commit adds their (non)-transitional entries with the same settings
as the base entry.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1999141

Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be>
---
 include/hw/qdev-core.h | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 4ff19c714b..5726825c2d 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -293,6 +293,30 @@ typedef struct GlobalProperty {
     bool optional;
 } GlobalProperty;
 
+
+/**
+ * Helper to add (non)transitional compat properties
+ */
+static inline void
+compat_props_add_transitional(GPtrArray *arr, GlobalProperty *prop)
+{
+    GlobalProperty *transitional = g_new0(typeof(*transitional), 1);
+    transitional->driver = g_strdup_printf("%s-transitional", prop->driver);
+    transitional->property = g_strdup(prop->property);
+    transitional->value = g_strdup(prop->value);
+    transitional->used = prop->used;
+    transitional->optional = prop->optional;
+    g_ptr_array_add(arr, (void *)transitional);
+
+    GlobalProperty *non_transitional = g_new0(typeof(*non_transitional), 1);
+    non_transitional->driver = g_strdup_printf("%s-non-transitional", prop->driver);
+    non_transitional->property = g_strdup(prop->property);
+    non_transitional->value = g_strdup(prop->value);
+    non_transitional->used = prop->used;
+    non_transitional->optional = prop->optional;
+    g_ptr_array_add(arr, (void *)non_transitional);
+}
+
 static inline void
 compat_props_add(GPtrArray *arr,
                  GlobalProperty props[], size_t nelem)
@@ -300,6 +324,16 @@ compat_props_add(GPtrArray *arr,
     int i;
     for (i = 0; i < nelem; i++) {
         g_ptr_array_add(arr, (void *)&props[i]);
+        if (g_str_equal(props[i].driver, "vhost-user-blk-pci") ||
+            g_str_equal(props[i].driver, "virtio-scsi-pci") ||
+            g_str_equal(props[i].driver, "virtio-blk-pci") ||
+            g_str_equal(props[i].driver, "virtio-balloon-pci") ||
+            g_str_equal(props[i].driver, "virtio-serial-pci") ||
+            g_str_equal(props[i].driver, "virtio-9p-pci") ||
+            g_str_equal(props[i].driver, "virtio-net-pci") ||
+            g_str_equal(props[i].driver, "virtio-rng-pci")) {
+            compat_props_add_transitional(arr, &props[i]);
+        }
     }
 }
 
-- 
2.33.0

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

[Jean-Louis Dupond]

Forgot to CC maintainers.

On 12/10/2021 10:24, Jean-Louis Dupond wrote:
> hw_compat modes only take into account their base name.
> But if a device is created with (non)-transitional, then the compat
> values are not used, causing migrating issues.
>
> This commit adds their (non)-transitional entries with the same settings
> as the base entry.
>
> Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1999141
>
> Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be>
> ---
>   include/hw/qdev-core.h | 34 ++++++++++++++++++++++++++++++++++
>   1 file changed, 34 insertions(+)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 4ff19c714b..5726825c2d 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -293,6 +293,30 @@ typedef struct GlobalProperty {
>       bool optional;
>   } GlobalProperty;
>   
> +
> +/**
> + * Helper to add (non)transitional compat properties
> + */
> +static inline void
> +compat_props_add_transitional(GPtrArray *arr, GlobalProperty *prop)
> +{
> +    GlobalProperty *transitional = g_new0(typeof(*transitional), 1);
> +    transitional->driver = g_strdup_printf("%s-transitional", prop->driver);
> +    transitional->property = g_strdup(prop->property);
> +    transitional->value = g_strdup(prop->value);
> +    transitional->used = prop->used;
> +    transitional->optional = prop->optional;
> +    g_ptr_array_add(arr, (void *)transitional);
> +
> +    GlobalProperty *non_transitional = g_new0(typeof(*non_transitional), 1);
> +    non_transitional->driver = g_strdup_printf("%s-non-transitional", prop->driver);
> +    non_transitional->property = g_strdup(prop->property);
> +    non_transitional->value = g_strdup(prop->value);
> +    non_transitional->used = prop->used;
> +    non_transitional->optional = prop->optional;
> +    g_ptr_array_add(arr, (void *)non_transitional);
> +}
> +
>   static inline void
>   compat_props_add(GPtrArray *arr,
>                    GlobalProperty props[], size_t nelem)
> @@ -300,6 +324,16 @@ compat_props_add(GPtrArray *arr,
>       int i;
>       for (i = 0; i < nelem; i++) {
>           g_ptr_array_add(arr, (void *)&props[i]);
> +        if (g_str_equal(props[i].driver, "vhost-user-blk-pci") ||
> +            g_str_equal(props[i].driver, "virtio-scsi-pci") ||
> +            g_str_equal(props[i].driver, "virtio-blk-pci") ||
> +            g_str_equal(props[i].driver, "virtio-balloon-pci") ||
> +            g_str_equal(props[i].driver, "virtio-serial-pci") ||
> +            g_str_equal(props[i].driver, "virtio-9p-pci") ||
> +            g_str_equal(props[i].driver, "virtio-net-pci") ||
> +            g_str_equal(props[i].driver, "virtio-rng-pci")) {
> +            compat_props_add_transitional(arr, &props[i]);
> +        }
>       }
>   }
>   

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3117 bytes --]

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

> 
> On 12/10/2021 10:24, Jean-Louis Dupond wrote:
> > hw_compat modes only take into account their base name.
> > But if a device is created with (non)-transitional, then the compat
> > values are not used, causing migrating issues.
> > 
> > This commit adds their (non)-transitional entries with the same settings
> > as the base entry.
> > 
> > Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1999141
> > 
> > Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be>
> > ---
> >   include/hw/qdev-core.h | 34 ++++++++++++++++++++++++++++++++++
> >   1 file changed, 34 insertions(+)
> > 
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index 4ff19c714b..5726825c2d 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -293,6 +293,30 @@ typedef struct GlobalProperty {
> >       bool optional;
> >   } GlobalProperty;
> > +
> > +/**
> > + * Helper to add (non)transitional compat properties
> > + */
> > +static inline void
> > +compat_props_add_transitional(GPtrArray *arr, GlobalProperty *prop)
> > +{
> > +    GlobalProperty *transitional = g_new0(typeof(*transitional), 1);
> > +    transitional->driver = g_strdup_printf("%s-transitional", prop->driver);
> > +    transitional->property = g_strdup(prop->property);
> > +    transitional->value = g_strdup(prop->value);
> > +    transitional->used = prop->used;
> > +    transitional->optional = prop->optional;
> > +    g_ptr_array_add(arr, (void *)transitional);
> > +
> > +    GlobalProperty *non_transitional = g_new0(typeof(*non_transitional), 1);
> > +    non_transitional->driver = g_strdup_printf("%s-non-transitional", prop->driver);
> > +    non_transitional->property = g_strdup(prop->property);
> > +    non_transitional->value = g_strdup(prop->value);
> > +    non_transitional->used = prop->used;
> > +    non_transitional->optional = prop->optional;
> > +    g_ptr_array_add(arr, (void *)non_transitional);
> > +}
> > +
> >   static inline void
> >   compat_props_add(GPtrArray *arr,
> >                    GlobalProperty props[], size_t nelem)
> > @@ -300,6 +324,16 @@ compat_props_add(GPtrArray *arr,
> >       int i;
> >       for (i = 0; i < nelem; i++) {
> >           g_ptr_array_add(arr, (void *)&props[i]);
> > +        if (g_str_equal(props[i].driver, "vhost-user-blk-pci") ||
> > +            g_str_equal(props[i].driver, "virtio-scsi-pci") ||
> > +            g_str_equal(props[i].driver, "virtio-blk-pci") ||
> > +            g_str_equal(props[i].driver, "virtio-balloon-pci") ||
> > +            g_str_equal(props[i].driver, "virtio-serial-pci") ||
> > +            g_str_equal(props[i].driver, "virtio-9p-pci") ||
> > +            g_str_equal(props[i].driver, "virtio-net-pci") ||
> > +            g_str_equal(props[i].driver, "virtio-rng-pci")) {
> > +            compat_props_add_transitional(arr, &props[i]);
> > +        }
> >       }
> >   }
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Michael S. Tsirkin]

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?


> > 
> > On 12/10/2021 10:24, Jean-Louis Dupond wrote:
> > > hw_compat modes only take into account their base name.
> > > But if a device is created with (non)-transitional, then the compat
> > > values are not used, causing migrating issues.
> > > 
> > > This commit adds their (non)-transitional entries with the same settings
> > > as the base entry.
> > > 
> > > Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1999141
> > > 
> > > Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be>
> > > ---
> > >   include/hw/qdev-core.h | 34 ++++++++++++++++++++++++++++++++++
> > >   1 file changed, 34 insertions(+)
> > > 
> > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > > index 4ff19c714b..5726825c2d 100644
> > > --- a/include/hw/qdev-core.h
> > > +++ b/include/hw/qdev-core.h
> > > @@ -293,6 +293,30 @@ typedef struct GlobalProperty {
> > >       bool optional;
> > >   } GlobalProperty;
> > > +
> > > +/**
> > > + * Helper to add (non)transitional compat properties
> > > + */
> > > +static inline void
> > > +compat_props_add_transitional(GPtrArray *arr, GlobalProperty *prop)
> > > +{
> > > +    GlobalProperty *transitional = g_new0(typeof(*transitional), 1);
> > > +    transitional->driver = g_strdup_printf("%s-transitional", prop->driver);
> > > +    transitional->property = g_strdup(prop->property);
> > > +    transitional->value = g_strdup(prop->value);
> > > +    transitional->used = prop->used;
> > > +    transitional->optional = prop->optional;
> > > +    g_ptr_array_add(arr, (void *)transitional);
> > > +
> > > +    GlobalProperty *non_transitional = g_new0(typeof(*non_transitional), 1);
> > > +    non_transitional->driver = g_strdup_printf("%s-non-transitional", prop->driver);
> > > +    non_transitional->property = g_strdup(prop->property);
> > > +    non_transitional->value = g_strdup(prop->value);
> > > +    non_transitional->used = prop->used;
> > > +    non_transitional->optional = prop->optional;
> > > +    g_ptr_array_add(arr, (void *)non_transitional);
> > > +}
> > > +
> > >   static inline void
> > >   compat_props_add(GPtrArray *arr,
> > >                    GlobalProperty props[], size_t nelem)
> > > @@ -300,6 +324,16 @@ compat_props_add(GPtrArray *arr,
> > >       int i;
> > >       for (i = 0; i < nelem; i++) {
> > >           g_ptr_array_add(arr, (void *)&props[i]);
> > > +        if (g_str_equal(props[i].driver, "vhost-user-blk-pci") ||
> > > +            g_str_equal(props[i].driver, "virtio-scsi-pci") ||
> > > +            g_str_equal(props[i].driver, "virtio-blk-pci") ||
> > > +            g_str_equal(props[i].driver, "virtio-balloon-pci") ||
> > > +            g_str_equal(props[i].driver, "virtio-serial-pci") ||
> > > +            g_str_equal(props[i].driver, "virtio-9p-pci") ||
> > > +            g_str_equal(props[i].driver, "virtio-net-pci") ||
> > > +            g_str_equal(props[i].driver, "virtio-rng-pci")) {
> > > +            compat_props_add_transitional(arr, &props[i]);
> > > +        }
> > >       }
> > >   }
> > 

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

[Eduardo Habkost]

On Tue, Oct 12, 2021 at 10:24:28AM +0200, Jean-Louis Dupond wrote:
> hw_compat modes only take into account their base name.

What do you mean by "base name"?

> But if a device is created with (non)-transitional, then the compat
> values are not used, causing migrating issues.
> 
> This commit adds their (non)-transitional entries with the same settings
> as the base entry.


Wouldn't it be easier to fix the incorrect compat_props arrays to
use "virtio-*-pci-base" instead?

If a piece of code is supposed to affect/support both
non-transitional and transitional subclasses, that's why
VirtioPCIDeviceTypeInfo.base_name exists.


> 
> Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1999141
> 
> Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be>
> ---
>  include/hw/qdev-core.h | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 4ff19c714b..5726825c2d 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -293,6 +293,30 @@ typedef struct GlobalProperty {
>      bool optional;
>  } GlobalProperty;
>  
> +
> +/**
> + * Helper to add (non)transitional compat properties
> + */
> +static inline void
> +compat_props_add_transitional(GPtrArray *arr, GlobalProperty *prop)
> +{
> +    GlobalProperty *transitional = g_new0(typeof(*transitional), 1);
> +    transitional->driver = g_strdup_printf("%s-transitional", prop->driver);
> +    transitional->property = g_strdup(prop->property);
> +    transitional->value = g_strdup(prop->value);
> +    transitional->used = prop->used;
> +    transitional->optional = prop->optional;
> +    g_ptr_array_add(arr, (void *)transitional);
> +
> +    GlobalProperty *non_transitional = g_new0(typeof(*non_transitional), 1);
> +    non_transitional->driver = g_strdup_printf("%s-non-transitional", prop->driver);
> +    non_transitional->property = g_strdup(prop->property);
> +    non_transitional->value = g_strdup(prop->value);
> +    non_transitional->used = prop->used;
> +    non_transitional->optional = prop->optional;
> +    g_ptr_array_add(arr, (void *)non_transitional);
> +}
> +
>  static inline void
>  compat_props_add(GPtrArray *arr,
>                   GlobalProperty props[], size_t nelem)
> @@ -300,6 +324,16 @@ compat_props_add(GPtrArray *arr,
>      int i;
>      for (i = 0; i < nelem; i++) {
>          g_ptr_array_add(arr, (void *)&props[i]);
> +        if (g_str_equal(props[i].driver, "vhost-user-blk-pci") ||
> +            g_str_equal(props[i].driver, "virtio-scsi-pci") ||
> +            g_str_equal(props[i].driver, "virtio-blk-pci") ||
> +            g_str_equal(props[i].driver, "virtio-balloon-pci") ||
> +            g_str_equal(props[i].driver, "virtio-serial-pci") ||
> +            g_str_equal(props[i].driver, "virtio-9p-pci") ||
> +            g_str_equal(props[i].driver, "virtio-net-pci") ||
> +            g_str_equal(props[i].driver, "virtio-rng-pci")) {
> +            compat_props_add_transitional(arr, &props[i]);
> +        }
>      }
>  }
>  
> -- 
> 2.33.0
> 
> 

-- 
Eduardo

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

[Eduardo Habkost]

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

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

[Michael S. Tsirkin]

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?

about the patch: how do people feel about virtio specific
stuff in qdev core? Ok by everyone?

-- 
MST

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

[Eduardo Habkost]

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);
 
-- 
Eduardo

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

[Jason Wang]

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
>

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

[Jason Wang]

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
> >

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

[Jean-Louis Dupond]

On 19/10/2021 17:27, Eduardo Habkost wrote:
> On Tue, Oct 12, 2021 at 10:24:28AM +0200, Jean-Louis Dupond wrote:
>> hw_compat modes only take into account their base name.
> What do you mean by "base name"?
virtio-net-pci (without the (non-)transitional extension.
>> But if a device is created with (non)-transitional, then the compat
>> values are not used, causing migrating issues.
>>
>> This commit adds their (non)-transitional entries with the same settings
>> as the base entry.
>
> Wouldn't it be easier to fix the incorrect compat_props arrays to
> use "virtio-*-pci-base" instead?
>
> If a piece of code is supposed to affect/support both
> non-transitional and transitional subclasses, that's why
> VirtioPCIDeviceTypeInfo.base_name exists.
>
Thats easier indeed :)
>> Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1999141
>>
>> Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be>
>> ---
>>   include/hw/qdev-core.h | 34 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 34 insertions(+)
>>
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index 4ff19c714b..5726825c2d 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -293,6 +293,30 @@ typedef struct GlobalProperty {
>>       bool optional;
>>   } GlobalProperty;
>>   
>> +
>> +/**
>> + * Helper to add (non)transitional compat properties
>> + */
>> +static inline void
>> +compat_props_add_transitional(GPtrArray *arr, GlobalProperty *prop)
>> +{
>> +    GlobalProperty *transitional = g_new0(typeof(*transitional), 1);
>> +    transitional->driver = g_strdup_printf("%s-transitional", prop->driver);
>> +    transitional->property = g_strdup(prop->property);
>> +    transitional->value = g_strdup(prop->value);
>> +    transitional->used = prop->used;
>> +    transitional->optional = prop->optional;
>> +    g_ptr_array_add(arr, (void *)transitional);
>> +
>> +    GlobalProperty *non_transitional = g_new0(typeof(*non_transitional), 1);
>> +    non_transitional->driver = g_strdup_printf("%s-non-transitional", prop->driver);
>> +    non_transitional->property = g_strdup(prop->property);
>> +    non_transitional->value = g_strdup(prop->value);
>> +    non_transitional->used = prop->used;
>> +    non_transitional->optional = prop->optional;
>> +    g_ptr_array_add(arr, (void *)non_transitional);
>> +}
>> +
>>   static inline void
>>   compat_props_add(GPtrArray *arr,
>>                    GlobalProperty props[], size_t nelem)
>> @@ -300,6 +324,16 @@ compat_props_add(GPtrArray *arr,
>>       int i;
>>       for (i = 0; i < nelem; i++) {
>>           g_ptr_array_add(arr, (void *)&props[i]);
>> +        if (g_str_equal(props[i].driver, "vhost-user-blk-pci") ||
>> +            g_str_equal(props[i].driver, "virtio-scsi-pci") ||
>> +            g_str_equal(props[i].driver, "virtio-blk-pci") ||
>> +            g_str_equal(props[i].driver, "virtio-balloon-pci") ||
>> +            g_str_equal(props[i].driver, "virtio-serial-pci") ||
>> +            g_str_equal(props[i].driver, "virtio-9p-pci") ||
>> +            g_str_equal(props[i].driver, "virtio-net-pci") ||
>> +            g_str_equal(props[i].driver, "virtio-rng-pci")) {
>> +            compat_props_add_transitional(arr, &props[i]);
>> +        }
>>       }
>>   }
>>   
>> -- 
>> 2.33.0
>>
>>

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

[Jean-Louis Dupond]

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>

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

[Michael S. Tsirkin]

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

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

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

[Eduardo Habkost]

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

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

[Michael S. Tsirkin]

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

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

[Michael S. Tsirkin]

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

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

[Eduardo Habkost]

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

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

[Michael S. Tsirkin]

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

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

[Eduardo Habkost]

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

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

[Michael S. Tsirkin]

On Tue, Oct 12, 2021 at 10:24:28AM +0200, Jean-Louis Dupond wrote:
> hw_compat modes only take into account their base name.
> But if a device is created with (non)-transitional, then the compat
> values are not used, causing migrating issues.
> 
> This commit adds their (non)-transitional entries with the same settings
> as the base entry.
> 
> Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1999141
> 
> Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be>


Jean-Louis, any chance you are going to address the comments
and post a new patch?


> ---
>  include/hw/qdev-core.h | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 4ff19c714b..5726825c2d 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -293,6 +293,30 @@ typedef struct GlobalProperty {
>      bool optional;
>  } GlobalProperty;
>  
> +
> +/**
> + * Helper to add (non)transitional compat properties
> + */
> +static inline void
> +compat_props_add_transitional(GPtrArray *arr, GlobalProperty *prop)
> +{
> +    GlobalProperty *transitional = g_new0(typeof(*transitional), 1);
> +    transitional->driver = g_strdup_printf("%s-transitional", prop->driver);
> +    transitional->property = g_strdup(prop->property);
> +    transitional->value = g_strdup(prop->value);
> +    transitional->used = prop->used;
> +    transitional->optional = prop->optional;
> +    g_ptr_array_add(arr, (void *)transitional);
> +
> +    GlobalProperty *non_transitional = g_new0(typeof(*non_transitional), 1);
> +    non_transitional->driver = g_strdup_printf("%s-non-transitional", prop->driver);
> +    non_transitional->property = g_strdup(prop->property);
> +    non_transitional->value = g_strdup(prop->value);
> +    non_transitional->used = prop->used;
> +    non_transitional->optional = prop->optional;
> +    g_ptr_array_add(arr, (void *)non_transitional);
> +}
> +
>  static inline void
>  compat_props_add(GPtrArray *arr,
>                   GlobalProperty props[], size_t nelem)
> @@ -300,6 +324,16 @@ compat_props_add(GPtrArray *arr,
>      int i;
>      for (i = 0; i < nelem; i++) {
>          g_ptr_array_add(arr, (void *)&props[i]);
> +        if (g_str_equal(props[i].driver, "vhost-user-blk-pci") ||
> +            g_str_equal(props[i].driver, "virtio-scsi-pci") ||
> +            g_str_equal(props[i].driver, "virtio-blk-pci") ||
> +            g_str_equal(props[i].driver, "virtio-balloon-pci") ||
> +            g_str_equal(props[i].driver, "virtio-serial-pci") ||
> +            g_str_equal(props[i].driver, "virtio-9p-pci") ||
> +            g_str_equal(props[i].driver, "virtio-net-pci") ||
> +            g_str_equal(props[i].driver, "virtio-rng-pci")) {
> +            compat_props_add_transitional(arr, &props[i]);
> +        }
>      }
>  }
>  
> -- 
> 2.33.0
> 
> 
> 

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

[Jean-Louis Dupond]

On 1/11/2021 23:26, Michael S. Tsirkin wrote:
> On Tue, Oct 12, 2021 at 10:24:28AM +0200, Jean-Louis Dupond wrote:
>> hw_compat modes only take into account their base name.
>> But if a device is created with (non)-transitional, then the compat
>> values are not used, causing migrating issues.
>>
>> This commit adds their (non)-transitional entries with the same settings
>> as the base entry.
>>
>> Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1999141
>>
>> Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be>
>
> Jean-Louis, any chance you are going to address the comments
> and post a new patch?
>
Should'nt we just apply the patch from Eduardo?
As this is a more elegant solution with the same result.
>> ---
>>   include/hw/qdev-core.h | 34 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 34 insertions(+)
>>
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index 4ff19c714b..5726825c2d 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -293,6 +293,30 @@ typedef struct GlobalProperty {
>>       bool optional;
>>   } GlobalProperty;
>>   
>> +
>> +/**
>> + * Helper to add (non)transitional compat properties
>> + */
>> +static inline void
>> +compat_props_add_transitional(GPtrArray *arr, GlobalProperty *prop)
>> +{
>> +    GlobalProperty *transitional = g_new0(typeof(*transitional), 1);
>> +    transitional->driver = g_strdup_printf("%s-transitional", prop->driver);
>> +    transitional->property = g_strdup(prop->property);
>> +    transitional->value = g_strdup(prop->value);
>> +    transitional->used = prop->used;
>> +    transitional->optional = prop->optional;
>> +    g_ptr_array_add(arr, (void *)transitional);
>> +
>> +    GlobalProperty *non_transitional = g_new0(typeof(*non_transitional), 1);
>> +    non_transitional->driver = g_strdup_printf("%s-non-transitional", prop->driver);
>> +    non_transitional->property = g_strdup(prop->property);
>> +    non_transitional->value = g_strdup(prop->value);
>> +    non_transitional->used = prop->used;
>> +    non_transitional->optional = prop->optional;
>> +    g_ptr_array_add(arr, (void *)non_transitional);
>> +}
>> +
>>   static inline void
>>   compat_props_add(GPtrArray *arr,
>>                    GlobalProperty props[], size_t nelem)
>> @@ -300,6 +324,16 @@ compat_props_add(GPtrArray *arr,
>>       int i;
>>       for (i = 0; i < nelem; i++) {
>>           g_ptr_array_add(arr, (void *)&props[i]);
>> +        if (g_str_equal(props[i].driver, "vhost-user-blk-pci") ||
>> +            g_str_equal(props[i].driver, "virtio-scsi-pci") ||
>> +            g_str_equal(props[i].driver, "virtio-blk-pci") ||
>> +            g_str_equal(props[i].driver, "virtio-balloon-pci") ||
>> +            g_str_equal(props[i].driver, "virtio-serial-pci") ||
>> +            g_str_equal(props[i].driver, "virtio-9p-pci") ||
>> +            g_str_equal(props[i].driver, "virtio-net-pci") ||
>> +            g_str_equal(props[i].driver, "virtio-rng-pci")) {
>> +            compat_props_add_transitional(arr, &props[i]);
>> +        }
>>       }
>>   }
>>   
>> -- 
>> 2.33.0
>>
>>
>>

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

[Michael S. Tsirkin]

On Wed, Nov 03, 2021 at 08:51:42AM +0100, Jean-Louis Dupond wrote:
> On 1/11/2021 23:26, Michael S. Tsirkin wrote:
> > On Tue, Oct 12, 2021 at 10:24:28AM +0200, Jean-Louis Dupond wrote:
> > > hw_compat modes only take into account their base name.
> > > But if a device is created with (non)-transitional, then the compat
> > > values are not used, causing migrating issues.
> > > 
> > > This commit adds their (non)-transitional entries with the same settings
> > > as the base entry.
> > > 
> > > Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1999141
> > > 
> > > Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be>
> > 
> > Jean-Louis, any chance you are going to address the comments
> > and post a new patch?
> > 
> Should'nt we just apply the patch from Eduardo?
> As this is a more elegant solution with the same result.

don't see it in queue. Repost?

> > > ---
> > >   include/hw/qdev-core.h | 34 ++++++++++++++++++++++++++++++++++
> > >   1 file changed, 34 insertions(+)
> > > 
> > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > > index 4ff19c714b..5726825c2d 100644
> > > --- a/include/hw/qdev-core.h
> > > +++ b/include/hw/qdev-core.h
> > > @@ -293,6 +293,30 @@ typedef struct GlobalProperty {
> > >       bool optional;
> > >   } GlobalProperty;
> > > +
> > > +/**
> > > + * Helper to add (non)transitional compat properties
> > > + */
> > > +static inline void
> > > +compat_props_add_transitional(GPtrArray *arr, GlobalProperty *prop)
> > > +{
> > > +    GlobalProperty *transitional = g_new0(typeof(*transitional), 1);
> > > +    transitional->driver = g_strdup_printf("%s-transitional", prop->driver);
> > > +    transitional->property = g_strdup(prop->property);
> > > +    transitional->value = g_strdup(prop->value);
> > > +    transitional->used = prop->used;
> > > +    transitional->optional = prop->optional;
> > > +    g_ptr_array_add(arr, (void *)transitional);
> > > +
> > > +    GlobalProperty *non_transitional = g_new0(typeof(*non_transitional), 1);
> > > +    non_transitional->driver = g_strdup_printf("%s-non-transitional", prop->driver);
> > > +    non_transitional->property = g_strdup(prop->property);
> > > +    non_transitional->value = g_strdup(prop->value);
> > > +    non_transitional->used = prop->used;
> > > +    non_transitional->optional = prop->optional;
> > > +    g_ptr_array_add(arr, (void *)non_transitional);
> > > +}
> > > +
> > >   static inline void
> > >   compat_props_add(GPtrArray *arr,
> > >                    GlobalProperty props[], size_t nelem)
> > > @@ -300,6 +324,16 @@ compat_props_add(GPtrArray *arr,
> > >       int i;
> > >       for (i = 0; i < nelem; i++) {
> > >           g_ptr_array_add(arr, (void *)&props[i]);
> > > +        if (g_str_equal(props[i].driver, "vhost-user-blk-pci") ||
> > > +            g_str_equal(props[i].driver, "virtio-scsi-pci") ||
> > > +            g_str_equal(props[i].driver, "virtio-blk-pci") ||
> > > +            g_str_equal(props[i].driver, "virtio-balloon-pci") ||
> > > +            g_str_equal(props[i].driver, "virtio-serial-pci") ||
> > > +            g_str_equal(props[i].driver, "virtio-9p-pci") ||
> > > +            g_str_equal(props[i].driver, "virtio-net-pci") ||
> > > +            g_str_equal(props[i].driver, "virtio-rng-pci")) {
> > > +            compat_props_add_transitional(arr, &props[i]);
> > > +        }
> > >       }
> > >   }
> > > -- 
> > > 2.33.0
> > > 
> > > 
> > > 

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

[Jean-Louis Dupond]

hw_compat modes only take into account their base name.
But if a device is created with (non)-transitional, then the compat
values are not used, causing migrating issues.

This commit adds their (non)-transitional entries with the same settings
as the base entry.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1999141
---
 include/hw/qdev-core.h | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 4ff19c714b..5726825c2d 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -293,6 +293,30 @@ typedef struct GlobalProperty {
     bool optional;
 } GlobalProperty;
 
+
+/**
+ * Helper to add (non)transitional compat properties
+ */
+static inline void
+compat_props_add_transitional(GPtrArray *arr, GlobalProperty *prop)
+{
+    GlobalProperty *transitional = g_new0(typeof(*transitional), 1);
+    transitional->driver = g_strdup_printf("%s-transitional", prop->driver);
+    transitional->property = g_strdup(prop->property);
+    transitional->value = g_strdup(prop->value);
+    transitional->used = prop->used;
+    transitional->optional = prop->optional;
+    g_ptr_array_add(arr, (void *)transitional);
+
+    GlobalProperty *non_transitional = g_new0(typeof(*non_transitional), 1);
+    non_transitional->driver = g_strdup_printf("%s-non-transitional", prop->driver);
+    non_transitional->property = g_strdup(prop->property);
+    non_transitional->value = g_strdup(prop->value);
+    non_transitional->used = prop->used;
+    non_transitional->optional = prop->optional;
+    g_ptr_array_add(arr, (void *)non_transitional);
+}
+
 static inline void
 compat_props_add(GPtrArray *arr,
                  GlobalProperty props[], size_t nelem)
@@ -300,6 +324,16 @@ compat_props_add(GPtrArray *arr,
     int i;
     for (i = 0; i < nelem; i++) {
         g_ptr_array_add(arr, (void *)&props[i]);
+        if (g_str_equal(props[i].driver, "vhost-user-blk-pci") ||
+            g_str_equal(props[i].driver, "virtio-scsi-pci") ||
+            g_str_equal(props[i].driver, "virtio-blk-pci") ||
+            g_str_equal(props[i].driver, "virtio-balloon-pci") ||
+            g_str_equal(props[i].driver, "virtio-serial-pci") ||
+            g_str_equal(props[i].driver, "virtio-9p-pci") ||
+            g_str_equal(props[i].driver, "virtio-net-pci") ||
+            g_str_equal(props[i].driver, "virtio-rng-pci")) {
+            compat_props_add_transitional(arr, &props[i]);
+        }
     }
 }
 
-- 
2.33.0