[PATCH v2 0/4] allow to deprecate objects and devices

[PATCH v2 0/4] allow to deprecate objects and devices

[Gerd Hoffmann]

Put some infrastructure in place to allow tagging objects (including
devices) as deprected.  Use it to mark the ohci pci host adapter and
the usb hub as deprecated.

v2:
 - pick up reviews.
 - drop ohci patch.
 - add cirrus vga patch.

Gerd Hoffmann (4):
  qom: allow to mark objects (including devices) as deprecated.
  usb: add config options for the hub and hid devices
  usb/hub: deprecate, don't build by default
  vga/cirrus: deprecate, don't build by default

 include/qom/object.h        | 1 +
 hw/display/cirrus_vga.c     | 1 +
 hw/display/cirrus_vga_isa.c | 1 +
 hw/usb/dev-hub.c            | 1 +
 qom/qom-qmp-cmds.c          | 4 ++++
 system/qdev-monitor.c       | 5 +++++
 hw/display/Kconfig          | 1 -
 hw/usb/Kconfig              | 9 +++++++++
 hw/usb/meson.build          | 4 ++--
 qapi/qom.json               | 4 +++-
 10 files changed, 27 insertions(+), 4 deletions(-)

-- 
2.45.1

[PATCH v2 1/4] qom: allow to mark objects (including devices) as deprecated.

[Gerd Hoffmann]

Add deprecation_note field (string) to ObjectClass.
Add deprecated bool to ObjectTypeInfo, report in 'qom-list-types'.
Print the note when listing devices via '-device help'.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/qom/object.h  | 1 +
 qom/qom-qmp-cmds.c    | 4 ++++
 system/qdev-monitor.c | 5 +++++
 qapi/qom.json         | 4 +++-
 4 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index 13d3a655ddf9..6c682aa0135f 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -136,6 +136,7 @@ struct ObjectClass
     ObjectUnparent *unparent;
 
     GHashTable *properties;
+    const char *deprecation_note;
 };
 
 /**
diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
index e91a2353472a..43de9c9ae141 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -101,6 +101,10 @@ static void qom_list_types_tramp(ObjectClass *klass, void *data)
     if (parent) {
         info->parent = g_strdup(object_class_get_name(parent));
     }
+    if (klass->deprecation_note) {
+        info->has_deprecated = true;
+        info->deprecated = true;
+    }
 
     QAPI_LIST_PREPEND(*pret, info);
 }
diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index 6af6ef7d667f..704be312e1a7 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -144,6 +144,8 @@ static bool qdev_class_has_alias(DeviceClass *dc)
 
 static void qdev_print_devinfo(DeviceClass *dc)
 {
+    ObjectClass *klass = OBJECT_CLASS(dc);
+
     qemu_printf("name \"%s\"", object_class_get_name(OBJECT_CLASS(dc)));
     if (dc->bus_type) {
         qemu_printf(", bus %s", dc->bus_type);
@@ -157,6 +159,9 @@ static void qdev_print_devinfo(DeviceClass *dc)
     if (!dc->user_creatable) {
         qemu_printf(", no-user");
     }
+    if (klass->deprecation_note) {
+        qemu_printf(", deprecated \"%s\"", klass->deprecation_note);
+    }
     qemu_printf("\n");
 }
 
diff --git a/qapi/qom.json b/qapi/qom.json
index 38dde6d785ac..bd062feabaf7 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -163,10 +163,12 @@
 #
 # @parent: Name of parent type, if any (since 2.10)
 #
+# @deprecated: the type is deprecated (since 9.1)
+#
 # Since: 1.1
 ##
 { 'struct': 'ObjectTypeInfo',
-  'data': { 'name': 'str', '*abstract': 'bool', '*parent': 'str' } }
+  'data': { 'name': 'str', '*abstract': 'bool', '*parent': 'str', '*deprecated': 'bool' } }
 
 ##
 # @qom-list-types:
-- 
2.45.1

Re: [PATCH v2 1/4] qom: allow to mark objects (including devices) as deprecated.

[Eric Blake]

On Thu, May 30, 2024 at 01:27:14PM GMT, Gerd Hoffmann wrote:
> Add deprecation_note field (string) to ObjectClass.
> Add deprecated bool to ObjectTypeInfo, report in 'qom-list-types'.
> Print the note when listing devices via '-device help'.

In the subject line, I suggest s/allow to mark/allow marking/

> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/qom/object.h  | 1 +
>  qom/qom-qmp-cmds.c    | 4 ++++
>  system/qdev-monitor.c | 5 +++++
>  qapi/qom.json         | 4 +++-
>  4 files changed, 13 insertions(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH v2 1/4] qom: allow to mark objects (including devices) as deprecated.

[Daniel P. Berrangé]

On Thu, May 30, 2024 at 01:27:14PM +0200, Gerd Hoffmann wrote:
> Add deprecation_note field (string) to ObjectClass.
> Add deprecated bool to ObjectTypeInfo, report in 'qom-list-types'.
> Print the note when listing devices via '-device help'.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/qom/object.h  | 1 +
>  qom/qom-qmp-cmds.c    | 4 ++++
>  system/qdev-monitor.c | 5 +++++
>  qapi/qom.json         | 4 +++-
>  4 files changed, 13 insertions(+), 1 deletion(-)

I replied to v1 before noticing your v2, so see my comment there
about printing a warning on stderr when creating an instance.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH v2 2/4] usb: add config options for the hub and hid devices

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/usb/Kconfig     | 10 ++++++++++
 hw/usb/meson.build |  4 ++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/hw/usb/Kconfig b/hw/usb/Kconfig
index f569ed7eeaa1..84bc7fbe36cd 100644
--- a/hw/usb/Kconfig
+++ b/hw/usb/Kconfig
@@ -65,6 +65,16 @@ config TUSB6010
     bool
     select USB_MUSB
 
+config USB_HUB
+    bool
+    default y
+    depends on USB
+
+config USB_HID
+    bool
+    default y
+    depends on USB
+
 config USB_TABLET_WACOM
     bool
     default y
diff --git a/hw/usb/meson.build b/hw/usb/meson.build
index 23f7f7acb50b..d7de1003e3db 100644
--- a/hw/usb/meson.build
+++ b/hw/usb/meson.build
@@ -35,8 +35,8 @@ system_ss.add(when: 'CONFIG_XLNX_VERSAL', if_true: files('xlnx-versal-usb2-ctrl-
 system_ss.add(when: 'CONFIG_XLNX_USB_SUBSYS', if_true: files('xlnx-usb-subsystem.c'))
 
 # emulated usb devices
-system_ss.add(when: 'CONFIG_USB', if_true: files('dev-hub.c'))
-system_ss.add(when: 'CONFIG_USB', if_true: files('dev-hid.c'))
+system_ss.add(when: 'CONFIG_USB_HUB', if_true: files('dev-hub.c'))
+system_ss.add(when: 'CONFIG_USB_HID', if_true: files('dev-hid.c'))
 system_ss.add(when: 'CONFIG_USB_TABLET_WACOM', if_true: files('dev-wacom.c'))
 system_ss.add(when: 'CONFIG_USB_STORAGE_CORE', if_true: files('dev-storage.c'))
 system_ss.add(when: 'CONFIG_USB_STORAGE_BOT', if_true: files('dev-storage-bot.c'))
-- 
2.45.1

Re: [PATCH v2 2/4] usb: add config options for the hub and hid devices

[Philippe Mathieu-Daudé]

On 30/5/24 13:27, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/usb/Kconfig     | 10 ++++++++++
>   hw/usb/meson.build |  4 ++--
>   2 files changed, 12 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v2 3/4] usb/hub: deprecate, don't build by default

[Gerd Hoffmann]

The hub supports only USB 1.1.  When running out of usb ports it is in
almost all cases the much better choice to add another usb host adapter
(or increase the number of root ports when using xhci) instead of using
the usb hub.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/dev-hub.c | 1 +
 hw/usb/Kconfig   | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/dev-hub.c b/hw/usb/dev-hub.c
index 06e9537d0356..68444d39534f 100644
--- a/hw/usb/dev-hub.c
+++ b/hw/usb/dev-hub.c
@@ -686,6 +686,7 @@ static void usb_hub_class_initfn(ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
     dc->fw_name = "hub";
     dc->vmsd = &vmstate_usb_hub;
+    klass->deprecation_note = "use more root ports or additional hostadapters instead";
     device_class_set_props(dc, usb_hub_properties);
 }
 
diff --git a/hw/usb/Kconfig b/hw/usb/Kconfig
index 84bc7fbe36cd..b583f5c25d05 100644
--- a/hw/usb/Kconfig
+++ b/hw/usb/Kconfig
@@ -67,7 +67,6 @@ config TUSB6010
 
 config USB_HUB
     bool
-    default y
     depends on USB
 
 config USB_HID
-- 
2.45.1

[PATCH v2 4/4] vga/cirrus: deprecate, don't build by default

[Gerd Hoffmann]

stdvga is the much better option.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/cirrus_vga.c     | 1 +
 hw/display/cirrus_vga_isa.c | 1 +
 hw/display/Kconfig          | 1 -
 3 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index 150883a97166..81421be1f89d 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -3007,6 +3007,7 @@ static void cirrus_vga_class_init(ObjectClass *klass, void *data)
     dc->vmsd = &vmstate_pci_cirrus_vga;
     device_class_set_props(dc, pci_vga_cirrus_properties);
     dc->hotpluggable = false;
+    klass->deprecation_note = "use stdvga instead";
 }
 
 static const TypeInfo cirrus_vga_info = {
diff --git a/hw/display/cirrus_vga_isa.c b/hw/display/cirrus_vga_isa.c
index 84be51670ed8..3abbf4dddd90 100644
--- a/hw/display/cirrus_vga_isa.c
+++ b/hw/display/cirrus_vga_isa.c
@@ -85,6 +85,7 @@ static void isa_cirrus_vga_class_init(ObjectClass *klass, void *data)
     dc->realize = isa_cirrus_vga_realizefn;
     device_class_set_props(dc, isa_cirrus_vga_properties);
     set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
+    klass->deprecation_note = "use stdvga instead";
 }
 
 static const TypeInfo isa_cirrus_vga_info = {
diff --git a/hw/display/Kconfig b/hw/display/Kconfig
index a4552c8ed78d..cd0779396890 100644
--- a/hw/display/Kconfig
+++ b/hw/display/Kconfig
@@ -11,7 +11,6 @@ config FW_CFG_DMA
 
 config VGA_CIRRUS
     bool
-    default y if PCI_DEVICES
     depends on PCI
     select VGA
 
-- 
2.45.1

Re: [PATCH v2 4/4] vga/cirrus: deprecate, don't build by default

[BALATON Zoltan]

On Thu, 30 May 2024, Gerd Hoffmann wrote:
> stdvga is the much better option.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> hw/display/cirrus_vga.c     | 1 +
> hw/display/cirrus_vga_isa.c | 1 +
> hw/display/Kconfig          | 1 -
> 3 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
> index 150883a97166..81421be1f89d 100644
> --- a/hw/display/cirrus_vga.c
> +++ b/hw/display/cirrus_vga.c
> @@ -3007,6 +3007,7 @@ static void cirrus_vga_class_init(ObjectClass *klass, void *data)
>     dc->vmsd = &vmstate_pci_cirrus_vga;
>     device_class_set_props(dc, pci_vga_cirrus_properties);
>     dc->hotpluggable = false;
> +    klass->deprecation_note = "use stdvga instead";
> }
>
> static const TypeInfo cirrus_vga_info = {
> diff --git a/hw/display/cirrus_vga_isa.c b/hw/display/cirrus_vga_isa.c
> index 84be51670ed8..3abbf4dddd90 100644
> --- a/hw/display/cirrus_vga_isa.c
> +++ b/hw/display/cirrus_vga_isa.c
> @@ -85,6 +85,7 @@ static void isa_cirrus_vga_class_init(ObjectClass *klass, void *data)
>     dc->realize = isa_cirrus_vga_realizefn;
>     device_class_set_props(dc, isa_cirrus_vga_properties);
>     set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
> +    klass->deprecation_note = "use stdvga instead";

Excepr some old OSes work better with this than stdvga so could this be 
left and not removed? Does it cause a lot of work to keep this device? I 
thought it's stable already and were not many changes for it lately. If 
something works why drop it?

Regards,
BALATON Zoltan

> }
>
> static const TypeInfo isa_cirrus_vga_info = {
> diff --git a/hw/display/Kconfig b/hw/display/Kconfig
> index a4552c8ed78d..cd0779396890 100644
> --- a/hw/display/Kconfig
> +++ b/hw/display/Kconfig
> @@ -11,7 +11,6 @@ config FW_CFG_DMA
>
> config VGA_CIRRUS
>     bool
> -    default y if PCI_DEVICES
>     depends on PCI
>     select VGA
>
>

Re: [PATCH v2 4/4] vga/cirrus: deprecate, don't build by default

[Mark Cave-Ayland]

On 30/05/2024 12:40, BALATON Zoltan wrote:

> On Thu, 30 May 2024, Gerd Hoffmann wrote:
>> stdvga is the much better option.
>>
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
>> hw/display/cirrus_vga.c     | 1 +
>> hw/display/cirrus_vga_isa.c | 1 +
>> hw/display/Kconfig          | 1 -
>> 3 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
>> index 150883a97166..81421be1f89d 100644
>> --- a/hw/display/cirrus_vga.c
>> +++ b/hw/display/cirrus_vga.c
>> @@ -3007,6 +3007,7 @@ static void cirrus_vga_class_init(ObjectClass *klass, void 
>> *data)
>>     dc->vmsd = &vmstate_pci_cirrus_vga;
>>     device_class_set_props(dc, pci_vga_cirrus_properties);
>>     dc->hotpluggable = false;
>> +    klass->deprecation_note = "use stdvga instead";
>> }
>>
>> static const TypeInfo cirrus_vga_info = {
>> diff --git a/hw/display/cirrus_vga_isa.c b/hw/display/cirrus_vga_isa.c
>> index 84be51670ed8..3abbf4dddd90 100644
>> --- a/hw/display/cirrus_vga_isa.c
>> +++ b/hw/display/cirrus_vga_isa.c
>> @@ -85,6 +85,7 @@ static void isa_cirrus_vga_class_init(ObjectClass *klass, void 
>> *data)
>>     dc->realize = isa_cirrus_vga_realizefn;
>>     device_class_set_props(dc, isa_cirrus_vga_properties);
>>     set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
>> +    klass->deprecation_note = "use stdvga instead";
> 
> Excepr some old OSes work better with this than stdvga so could this be left and not 
> removed? Does it cause a lot of work to keep this device? I thought it's stable 
> already and were not many changes for it lately. If something works why drop it?

Seconded: whilst stdvga is preferred, there are a lot of older OSs that work well in 
QEMU using the Cirrus emulation. I appreciate that the code could do with a bit of 
work, but is there a more specific reason that it should be deprecated?


ATB,

Mark.

Re: [PATCH v2 4/4] vga/cirrus: deprecate, don't build by default

[Gerd Hoffmann]

  Hi,

> > > static const TypeInfo cirrus_vga_info = {
> > > diff --git a/hw/display/cirrus_vga_isa.c b/hw/display/cirrus_vga_isa.c
> > > index 84be51670ed8..3abbf4dddd90 100644
> > > --- a/hw/display/cirrus_vga_isa.c
> > > +++ b/hw/display/cirrus_vga_isa.c
> > > @@ -85,6 +85,7 @@ static void isa_cirrus_vga_class_init(ObjectClass
> > > *klass, void *data)
> > >     dc->realize = isa_cirrus_vga_realizefn;
> > >     device_class_set_props(dc, isa_cirrus_vga_properties);
> > >     set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
> > > +    klass->deprecation_note = "use stdvga instead";
> > 
> > Excepr some old OSes work better with this than stdvga so could this be
> > left and not removed? Does it cause a lot of work to keep this device? I
> > thought it's stable already and were not many changes for it lately. If
> > something works why drop it?
> 
> Seconded: whilst stdvga is preferred, there are a lot of older OSs that work
> well in QEMU using the Cirrus emulation. I appreciate that the code could do
> with a bit of work, but is there a more specific reason that it should be
> deprecated?

Well, the cirrus has a 2d blitter implementation which I'd rate
problematic from both security and correctness point of view.

Also any guest new enough to still receive security updates can surely
use stdvga instead.  The "operating system museum" is IMHO pretty much
the only use case where it possibly might make sense to continue using
cirrus.

Having sayed that maybe the boolean classification -- be deprecated
or not -- is too simple.  The number of devices we can actually
deprecate and remove is probably relatively small.  But there also is
a large number of old-ish devices which only make sense in very few use
cases.  When running an old OS.  Or when emulating an old board.  Which
also tend to be not maintained very well because there are not many
users.  Maybe we need an "unsupported" state for them, with some
infrastructure like an easy way to compile qemu without unsupported
devices and an option to get warnings from qemu in case an unsupported
device is used.

take care,
  Gerd

Re: [PATCH v2 4/4] vga/cirrus: deprecate, don't build by default

[Daniel P. Berrangé]

On Thu, May 30, 2024 at 01:22:11PM +0100, Mark Cave-Ayland wrote:
> On 30/05/2024 12:40, BALATON Zoltan wrote:
> 
> > On Thu, 30 May 2024, Gerd Hoffmann wrote:
> > > stdvga is the much better option.
> > > 
> > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > > ---
> > > hw/display/cirrus_vga.c     | 1 +
> > > hw/display/cirrus_vga_isa.c | 1 +
> > > hw/display/Kconfig          | 1 -
> > > 3 files changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
> > > index 150883a97166..81421be1f89d 100644
> > > --- a/hw/display/cirrus_vga.c
> > > +++ b/hw/display/cirrus_vga.c
> > > @@ -3007,6 +3007,7 @@ static void cirrus_vga_class_init(ObjectClass
> > > *klass, void *data)
> > >     dc->vmsd = &vmstate_pci_cirrus_vga;
> > >     device_class_set_props(dc, pci_vga_cirrus_properties);
> > >     dc->hotpluggable = false;
> > > +    klass->deprecation_note = "use stdvga instead";
> > > }
> > > 
> > > static const TypeInfo cirrus_vga_info = {
> > > diff --git a/hw/display/cirrus_vga_isa.c b/hw/display/cirrus_vga_isa.c
> > > index 84be51670ed8..3abbf4dddd90 100644
> > > --- a/hw/display/cirrus_vga_isa.c
> > > +++ b/hw/display/cirrus_vga_isa.c
> > > @@ -85,6 +85,7 @@ static void isa_cirrus_vga_class_init(ObjectClass
> > > *klass, void *data)
> > >     dc->realize = isa_cirrus_vga_realizefn;
> > >     device_class_set_props(dc, isa_cirrus_vga_properties);
> > >     set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
> > > +    klass->deprecation_note = "use stdvga instead";
> > 
> > Excepr some old OSes work better with this than stdvga so could this be
> > left and not removed? Does it cause a lot of work to keep this device? I
> > thought it's stable already and were not many changes for it lately. If
> > something works why drop it?
> 
> Seconded: whilst stdvga is preferred, there are a lot of older OSs that work
> well in QEMU using the Cirrus emulation. I appreciate that the code could do
> with a bit of work, but is there a more specific reason that it should be
> deprecated?

I think there's different answers here for upstream vs downstream.

Upstream QEMU's scope is to emulate pretty much arbitrary hardware that
may have existed at any point in time. Emulating Cirrus is very much
in scope upstream, and even if there are other better VGA devices, that
doesn't make emulation of Cirrus redundant.

Downstream is a different matter - if a downstream vendor wants to be
opinionated and limit the scope of devices they ship to customers, it
is totally valid to cull Cirrus.

IOW, I think device deprecation *framework* is relevant to include
upstream, but most actual usage of it will be downstream.

Upstream might use *object* deprecation, if we replace an backend
implementation with a different one.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 4/4] vga/cirrus: deprecate, don't build by default

[Peter Maydell]

On Mon, 3 Jun 2024 at 12:42, Daniel P. Berrangé <berrange@redhat.com> wrote:
> I think there's different answers here for upstream vs downstream.
>
> Upstream QEMU's scope is to emulate pretty much arbitrary hardware that
> may have existed at any point in time. Emulating Cirrus is very much
> in scope upstream, and even if there are other better VGA devices, that
> doesn't make emulation of Cirrus redundant.
>
> Downstream is a different matter - if a downstream vendor wants to be
> opinionated and limit the scope of devices they ship to customers, it
> is totally valid to cull Cirrus.
>
> IOW, I think device deprecation *framework* is relevant to include
> upstream, but most actual usage of it will be downstream.

Right; for upstream we should expect that we use deprecation
mostly as part of the "deprecate and then drop in a few releases"
cycle. Deprecating something we don't want to drop doesn't
seem to me to make much sense in an upstream context.

thanks
-- PMM

Re: [PATCH v2 4/4] vga/cirrus: deprecate, don't build by default

[Philippe Mathieu-Daudé]

On 3/6/24 13:40, Daniel P. Berrangé wrote:
> On Thu, May 30, 2024 at 01:22:11PM +0100, Mark Cave-Ayland wrote:
>> On 30/05/2024 12:40, BALATON Zoltan wrote:
>>
>>> On Thu, 30 May 2024, Gerd Hoffmann wrote:
>>>> stdvga is the much better option.
>>>>
>>>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>>> ---
>>>> hw/display/cirrus_vga.c     | 1 +
>>>> hw/display/cirrus_vga_isa.c | 1 +
>>>> hw/display/Kconfig          | 1 -
>>>> 3 files changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
>>>> index 150883a97166..81421be1f89d 100644
>>>> --- a/hw/display/cirrus_vga.c
>>>> +++ b/hw/display/cirrus_vga.c
>>>> @@ -3007,6 +3007,7 @@ static void cirrus_vga_class_init(ObjectClass
>>>> *klass, void *data)
>>>>      dc->vmsd = &vmstate_pci_cirrus_vga;
>>>>      device_class_set_props(dc, pci_vga_cirrus_properties);
>>>>      dc->hotpluggable = false;
>>>> +    klass->deprecation_note = "use stdvga instead";
>>>> }
>>>>
>>>> static const TypeInfo cirrus_vga_info = {
>>>> diff --git a/hw/display/cirrus_vga_isa.c b/hw/display/cirrus_vga_isa.c
>>>> index 84be51670ed8..3abbf4dddd90 100644
>>>> --- a/hw/display/cirrus_vga_isa.c
>>>> +++ b/hw/display/cirrus_vga_isa.c
>>>> @@ -85,6 +85,7 @@ static void isa_cirrus_vga_class_init(ObjectClass
>>>> *klass, void *data)
>>>>      dc->realize = isa_cirrus_vga_realizefn;
>>>>      device_class_set_props(dc, isa_cirrus_vga_properties);
>>>>      set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
>>>> +    klass->deprecation_note = "use stdvga instead";
>>>
>>> Excepr some old OSes work better with this than stdvga so could this be
>>> left and not removed? Does it cause a lot of work to keep this device? I
>>> thought it's stable already and were not many changes for it lately. If
>>> something works why drop it?
>>
>> Seconded: whilst stdvga is preferred, there are a lot of older OSs that work
>> well in QEMU using the Cirrus emulation. I appreciate that the code could do
>> with a bit of work, but is there a more specific reason that it should be
>> deprecated?
> 
> I think there's different answers here for upstream vs downstream.
> 
> Upstream QEMU's scope is to emulate pretty much arbitrary hardware that
> may have existed at any point in time. Emulating Cirrus is very much
> in scope upstream, and even if there are other better VGA devices, that
> doesn't make emulation of Cirrus redundant.
> 
> Downstream is a different matter - if a downstream vendor wants to be
> opinionated and limit the scope of devices they ship to customers, it
> is totally valid to cull Cirrus.

Few years ago I suggested qemu_security_policy_taint() "which allows
unsafe (read "not very maintained") code to 'taint' QEMU security
policy." (Gerd FYI):
https://lore.kernel.org/qemu-devel/20210908232024.2399215-1-philmd@redhat.com/


Upstream we could add a boolean in DeviceClass about device security
status / maintenance (or enum or bitfield); then downstreams could
use it to be sure unsafe devices aren't linked in.


> IOW, I think device deprecation *framework* is relevant to include
> upstream, but most actual usage of it will be downstream.
> 
> Upstream might use *object* deprecation, if we replace an backend
> implementation with a different one.
> 
> With regards,
> Daniel

Re: [PATCH v2 4/4] vga/cirrus: deprecate, don't build by default

[Gerd Hoffmann]

  Hi,

> > Upstream QEMU's scope is to emulate pretty much arbitrary hardware that
> > may have existed at any point in time. Emulating Cirrus is very much
> > in scope upstream, and even if there are other better VGA devices, that
> > doesn't make emulation of Cirrus redundant.
> > 
> > Downstream is a different matter - if a downstream vendor wants to be
> > opinionated and limit the scope of devices they ship to customers, it
> > is totally valid to cull Cirrus.
> 
> Few years ago I suggested qemu_security_policy_taint() "which allows
> unsafe (read "not very maintained") code to 'taint' QEMU security
> policy." (Gerd FYI):
> https://lore.kernel.org/qemu-devel/20210908232024.2399215-1-philmd@redhat.com/
> 
> Upstream we could add a boolean in DeviceClass about device security
> status / maintenance (or enum or bitfield); then downstreams could
> use it to be sure unsafe devices aren't linked in.

Yes, I think it makes sense to maintain that information upstream.  It
is useful information to have.  Even if upstream isn't going to enforce
something qemu could print useful hints.

So, the question is whenever we want go for a simple bool, or go for a
bitfield giving more detailed information.  Bits I think could be
useful:

  (1) OBJECT_STATUS_DEPRECATED
      Stuff we actually want remove.  Very few cases, maybe usb-hub.

  (2) OBJECT_STATUS_UNSAFE
      "not very maintained".  Probably need a better name for this.

  (3) OBJECT_STATUS_OUTDATED
      Old device where newer / better alternatives exist.

Looking at the USB host adapters I'd attach 2+3 to ohci and 3 to uhci
and ehci.  In general there is a lot of overlap for (2) + (3) though.

We might also have recommendation bits such as:

  (4) OBJECT_STATUS_VIRTUALIZATON
      Devices recommended for virtualization use cases
      (all virtio, xhci, ...).
     
> > IOW, I think device deprecation *framework* is relevant to include
> > upstream, but most actual usage of it will be downstream.

When doing it at ObjectClass not DeviceClass level we get introspection
support for (almost) free, so I'd do it for ObjectClass even if the vast
majority of users will be devices.

> > Upstream might use *object* deprecation, if we replace an backend
> > implementation with a different one.

Sounds like having OBJECT_STATUS_DEPRECATED makes sense even if we don't
have any device actually using that.

take care,
  Gerd

Re: [PATCH v2 4/4] vga/cirrus: deprecate, don't build by default

[Daniel P. Berrangé]

On Wed, Jun 05, 2024 at 09:32:01AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > > Upstream QEMU's scope is to emulate pretty much arbitrary hardware that
> > > may have existed at any point in time. Emulating Cirrus is very much
> > > in scope upstream, and even if there are other better VGA devices, that
> > > doesn't make emulation of Cirrus redundant.
> > > 
> > > Downstream is a different matter - if a downstream vendor wants to be
> > > opinionated and limit the scope of devices they ship to customers, it
> > > is totally valid to cull Cirrus.
> > 
> > Few years ago I suggested qemu_security_policy_taint() "which allows
> > unsafe (read "not very maintained") code to 'taint' QEMU security
> > policy." (Gerd FYI):
> > https://lore.kernel.org/qemu-devel/20210908232024.2399215-1-philmd@redhat.com/
> > 
> > Upstream we could add a boolean in DeviceClass about device security
> > status / maintenance (or enum or bitfield); then downstreams could
> > use it to be sure unsafe devices aren't linked in.
> 
> Yes, I think it makes sense to maintain that information upstream.  It
> is useful information to have.  Even if upstream isn't going to enforce
> something qemu could print useful hints.
> 
> So, the question is whenever we want go for a simple bool, or go for a
> bitfield giving more detailed information.  Bits I think could be
> useful:
> 
>   (1) OBJECT_STATUS_DEPRECATED
>       Stuff we actually want remove.  Very few cases, maybe usb-hub.
> 
>   (2) OBJECT_STATUS_UNSAFE
>       "not very maintained".  Probably need a better name for this.

If it reflects maintainer status, then we're obvioulsy overlapping
with the MAINTAINERS file info, but "UNSAFE" suggests something
different....

So what I think would be valuable is marking whether a device is
considered to provide a guest security boundary.  This would in turn
indicate whether we would treat flaws in its impl as being worthy of
triaging as CVEs, or merely be normal bugs.

We already document in security.rst that we consider virtualization
use cases only to provide a security boundary, but on every bug report
we have to decide whether the device in question is considered relevant
to a "virtualization"  use case.

This was a known limitation, because we had no metadata to track
which devices were considered "secure". It was always expected that
long term we should be tagging devices/machines/accelerators/etc
with their security status.

It would be nice from an end user POV to be able to have a way to tell
QEMU to enforce that a given VM provides a guest security boundary, and
get a hard error at startup, or hot-plug if any device cannot satisfy
that requirement.

>   (3) OBJECT_STATUS_OUTDATED
>       Old device where newer / better alternatives exist.

I think (3) is pretty hard to define, as "better" is very much a
use case specific decision. You could say that everything, for
which a virtio device exists, has a "better" alternative. That's
only true if running a modern OS though. If running a vintage
OS, the "outdated" thing is liley "better".

> Looking at the USB host adapters I'd attach 2+3 to ohci and 3 to uhci
> and ehci.  In general there is a lot of overlap for (2) + (3) though.
> 
> We might also have recommendation bits such as:
> 
>   (4) OBJECT_STATUS_VIRTUALIZATON
>       Devices recommended for virtualization use cases
>       (all virtio, xhci, ...).

Even that has the same problem as (3) - the recommended devices
differ depending on what OS you're intending to use.

This problem is pretty much why libosinfo exists to provide more
guided hardware recommendations tailored to OS.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 4/4] vga/cirrus: deprecate, don't build by default

[Mark Cave-Ayland]

On 03/06/2024 12:40, Daniel P. Berrangé wrote:

> On Thu, May 30, 2024 at 01:22:11PM +0100, Mark Cave-Ayland wrote:
>> On 30/05/2024 12:40, BALATON Zoltan wrote:
>>
>>> On Thu, 30 May 2024, Gerd Hoffmann wrote:
>>>> stdvga is the much better option.
>>>>
>>>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>>> ---
>>>> hw/display/cirrus_vga.c     | 1 +
>>>> hw/display/cirrus_vga_isa.c | 1 +
>>>> hw/display/Kconfig          | 1 -
>>>> 3 files changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
>>>> index 150883a97166..81421be1f89d 100644
>>>> --- a/hw/display/cirrus_vga.c
>>>> +++ b/hw/display/cirrus_vga.c
>>>> @@ -3007,6 +3007,7 @@ static void cirrus_vga_class_init(ObjectClass
>>>> *klass, void *data)
>>>>      dc->vmsd = &vmstate_pci_cirrus_vga;
>>>>      device_class_set_props(dc, pci_vga_cirrus_properties);
>>>>      dc->hotpluggable = false;
>>>> +    klass->deprecation_note = "use stdvga instead";
>>>> }
>>>>
>>>> static const TypeInfo cirrus_vga_info = {
>>>> diff --git a/hw/display/cirrus_vga_isa.c b/hw/display/cirrus_vga_isa.c
>>>> index 84be51670ed8..3abbf4dddd90 100644
>>>> --- a/hw/display/cirrus_vga_isa.c
>>>> +++ b/hw/display/cirrus_vga_isa.c
>>>> @@ -85,6 +85,7 @@ static void isa_cirrus_vga_class_init(ObjectClass
>>>> *klass, void *data)
>>>>      dc->realize = isa_cirrus_vga_realizefn;
>>>>      device_class_set_props(dc, isa_cirrus_vga_properties);
>>>>      set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
>>>> +    klass->deprecation_note = "use stdvga instead";
>>>
>>> Excepr some old OSes work better with this than stdvga so could this be
>>> left and not removed? Does it cause a lot of work to keep this device? I
>>> thought it's stable already and were not many changes for it lately. If
>>> something works why drop it?
>>
>> Seconded: whilst stdvga is preferred, there are a lot of older OSs that work
>> well in QEMU using the Cirrus emulation. I appreciate that the code could do
>> with a bit of work, but is there a more specific reason that it should be
>> deprecated?
> 
> I think there's different answers here for upstream vs downstream.
> 
> Upstream QEMU's scope is to emulate pretty much arbitrary hardware that
> may have existed at any point in time. Emulating Cirrus is very much
> in scope upstream, and even if there are other better VGA devices, that
> doesn't make emulation of Cirrus redundant.
> 
> Downstream is a different matter - if a downstream vendor wants to be
> opinionated and limit the scope of devices they ship to customers, it
> is totally valid to cull Cirrus.

The concern for me is that if someone such as RedHat decided not to ship Cirrus then 
we'd end up in a place where command lines for some legacy OSs would work on an 
upstream build, but if someone was using RHEL then they would fail due to the device 
not being present. I can see this causing confusion for users since they would report 
that a command line doesn't work whilst others would shrug and report back that it 
works for them.

I do wonder if a solution for this would be to add a blocklist file in /etc that 
prevents the listed QOM types from being instantiated. The file could contain also 
contain a machine regex to match and a reason that can be reported to the user e.g.

# QEMU QOM type blocklist
#
# QOM type regex, machine regex list, reason
"cirrus*","pc*,machine*","contains insecure blitter routines"
"fdc","pc*","may not be completely secure"

This feels like a better solution because:

- It's easy to add a message that reports "The requested QOM type <foo> cannot be 
instantiated because <reason>" for specific machine types. The machine regex also 
fixes the problem where usb-ohci should be allowed for PPC machines, but not 
generally for PC machines.

- Downstream can ship with a secure policy for production environments but also a 
less restrictive policy for more casual users

- If someone really needs a device that is not enabled by default, a privileged user 
can simply edit the blocklist file and allow it

- It should work both with or without modules


ATB,

Mark.

Re: [PATCH v2 4/4] vga/cirrus: deprecate, don't build by default

[Daniel P. Berrangé]

On Tue, Jun 04, 2024 at 07:50:38AM +0100, Mark Cave-Ayland wrote:
> On 03/06/2024 12:40, Daniel P. Berrangé wrote:
> 
> > On Thu, May 30, 2024 at 01:22:11PM +0100, Mark Cave-Ayland wrote:
> > > On 30/05/2024 12:40, BALATON Zoltan wrote:
> > > 
> > > > On Thu, 30 May 2024, Gerd Hoffmann wrote:
> > > > > stdvga is the much better option.
> > > > > 
> > > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > > > > ---
> > > > > hw/display/cirrus_vga.c     | 1 +
> > > > > hw/display/cirrus_vga_isa.c | 1 +
> > > > > hw/display/Kconfig          | 1 -
> > > > > 3 files changed, 2 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
> > > > > index 150883a97166..81421be1f89d 100644
> > > > > --- a/hw/display/cirrus_vga.c
> > > > > +++ b/hw/display/cirrus_vga.c
> > > > > @@ -3007,6 +3007,7 @@ static void cirrus_vga_class_init(ObjectClass
> > > > > *klass, void *data)
> > > > >      dc->vmsd = &vmstate_pci_cirrus_vga;
> > > > >      device_class_set_props(dc, pci_vga_cirrus_properties);
> > > > >      dc->hotpluggable = false;
> > > > > +    klass->deprecation_note = "use stdvga instead";
> > > > > }
> > > > > 
> > > > > static const TypeInfo cirrus_vga_info = {
> > > > > diff --git a/hw/display/cirrus_vga_isa.c b/hw/display/cirrus_vga_isa.c
> > > > > index 84be51670ed8..3abbf4dddd90 100644
> > > > > --- a/hw/display/cirrus_vga_isa.c
> > > > > +++ b/hw/display/cirrus_vga_isa.c
> > > > > @@ -85,6 +85,7 @@ static void isa_cirrus_vga_class_init(ObjectClass
> > > > > *klass, void *data)
> > > > >      dc->realize = isa_cirrus_vga_realizefn;
> > > > >      device_class_set_props(dc, isa_cirrus_vga_properties);
> > > > >      set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
> > > > > +    klass->deprecation_note = "use stdvga instead";
> > > > 
> > > > Excepr some old OSes work better with this than stdvga so could this be
> > > > left and not removed? Does it cause a lot of work to keep this device? I
> > > > thought it's stable already and were not many changes for it lately. If
> > > > something works why drop it?
> > > 
> > > Seconded: whilst stdvga is preferred, there are a lot of older OSs that work
> > > well in QEMU using the Cirrus emulation. I appreciate that the code could do
> > > with a bit of work, but is there a more specific reason that it should be
> > > deprecated?
> > 
> > I think there's different answers here for upstream vs downstream.
> > 
> > Upstream QEMU's scope is to emulate pretty much arbitrary hardware that
> > may have existed at any point in time. Emulating Cirrus is very much
> > in scope upstream, and even if there are other better VGA devices, that
> > doesn't make emulation of Cirrus redundant.
> > 
> > Downstream is a different matter - if a downstream vendor wants to be
> > opinionated and limit the scope of devices they ship to customers, it
> > is totally valid to cull Cirrus.
> 
> The concern for me is that if someone such as RedHat decided not to ship
> Cirrus then we'd end up in a place where command lines for some legacy OSs
> would work on an upstream build, but if someone was using RHEL then they
> would fail due to the device not being present. I can see this causing
> confusion for users since they would report that a command line doesn't work
> whilst others would shrug and report back that it works for them.

That ship sailed in RHEL over a decade and a half ago.

There is already a mountain of devices and other QEMU features that are
/not/ shipped in RHEL, disabled at build time. Essentially RHEL is only
targetting virtualization use cases, not emulation, so the priority is
virtio devices, paired with a tiny handful of emulated devices to fill
some gaps. Historically RHEL included Cirrus, but it was marked deprecated
in RHEL quite a few years ago now, and will likely be removed entirely
in RHEL-10. The combo of stdvga and virtio-vga/gpu is sufficient for the
virtualization use case.

Yes, this does cause confusion and annoyance for users who want to try
to use RHEL with QEMU cli configs they find around the web, but that's
considered acceptable pain.

> I do wonder if a solution for this would be to add a blocklist file in /etc
> that prevents the listed QOM types from being instantiated. The file could
> contain also contain a machine regex to match and a reason that can be
> reported to the user e.g.
> 
> # QEMU QOM type blocklist
> #
> # QOM type regex, machine regex list, reason
> "cirrus*","pc*,machine*","contains insecure blitter routines"
> "fdc","pc*","may not be completely secure"
> 
> This feels like a better solution because:
> 
> - It's easy to add a message that reports "The requested QOM type <foo>
> cannot be instantiated because <reason>" for specific machine types. The
> machine regex also fixes the problem where usb-ohci should be allowed for
> PPC machines, but not generally for PC machines.
> 
> - Downstream can ship with a secure policy for production environments but
> also a less restrictive policy for more casual users
>
> - If someone really needs a device that is not enabled by default, a
> privileged user can simply edit the blocklist file and allow it
> 
> - It should work both with or without modules

I don't see a user edittable config being useful in RHEL or Fedora.
For RHEL we have a strict policy of only shipping what we want to
support, and everything else must be fully disabled at build time.
Conversely in Fedora we aim to ship everything that QEMU provides.

I would be nice to have a tagging of "quality" status for devices
upstream, but that's not something that needs to be turned into a
user edittable matrix against machine types. A device impl is either
good or not - the code impl quality doesn't vary per machine usage,
nor should upstream try to artificially block usage per machine.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|