| Message ID | 20211012082428.16222-1-jean-louis@dupond.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/qdev-core: Add compatibility for (non)-transitional devs | expand |
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]); > + } > } > } >
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]); > > + } > > } > > } >
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]); > > > + } > > > } > > > } > >
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 > >
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.
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?
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);
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 >
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 > >
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 >> >>
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>
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
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)
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)
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
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
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.
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
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.
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 > > >
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 >> >> >>
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 > > > > > > > > >
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]); + } } }
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(+)