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

[PATCH v3 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.

v3:
 - switch to two properties: 'deprecated' and 'not secure' flags.
 - add rfc patch implementing policies for devices with flags.

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

Gerd Hoffmann (4):
  qom: allow to mark objects as deprecated or not secure.
  usb/hub: mark as deprecated
  vga/cirrus: mark as not secure
  qdev: add device policy [RfC]

 include/qom/object.h        |  3 ++
 hw/core/qdev.c              | 60 ++++++++++++++++++++++++++++++++++++-
 hw/display/cirrus_vga.c     |  1 +
 hw/display/cirrus_vga_isa.c |  1 +
 hw/usb/dev-hub.c            |  1 +
 qom/qom-qmp-cmds.c          |  8 +++++
 system/qdev-monitor.c       |  8 +++++
 qapi/qom.json               |  8 ++++-
 8 files changed, 88 insertions(+), 2 deletions(-)

-- 
2.45.2

[PATCH v3 1/4] qom: allow to mark objects as deprecated or not secure.

[Gerd Hoffmann]

Add flags to ObjectClass for objects which are deprecated or not secure.
Add 'deprecated' and 'not-secure' bools to ObjectTypeInfo, report in
'qom-list-types'.  Print the flags when listing devices via '-device
help'.

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

diff --git a/include/qom/object.h b/include/qom/object.h
index 13d3a655ddf9..419bd9a4b219 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -136,6 +136,9 @@ struct ObjectClass
     ObjectUnparent *unparent;
 
     GHashTable *properties;
+
+    bool deprecated;
+    bool not_secure;
 };
 
 /**
diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
index e91a2353472a..325ff0ba2a25 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -101,6 +101,14 @@ static void qom_list_types_tramp(ObjectClass *klass, void *data)
     if (parent) {
         info->parent = g_strdup(object_class_get_name(parent));
     }
+    if (klass->deprecated) {
+        info->has_deprecated = true;
+        info->deprecated = true;
+    }
+    if (klass->not_secure) {
+        info->has_not_secure = true;
+        info->not_secure = true;
+    }
 
     QAPI_LIST_PREPEND(*pret, info);
 }
diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index 6af6ef7d667f..effdc95d21d3 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,12 @@ static void qdev_print_devinfo(DeviceClass *dc)
     if (!dc->user_creatable) {
         qemu_printf(", no-user");
     }
+    if (klass->deprecated) {
+        qemu_printf(", deprecated");
+    }
+    if (klass->not_secure) {
+        qemu_printf(", not-secure");
+    }
     qemu_printf("\n");
 }
 
diff --git a/qapi/qom.json b/qapi/qom.json
index 8bd299265e39..3f20d4c6413b 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -163,10 +163,16 @@
 #
 # @parent: Name of parent type, if any (since 2.10)
 #
+# @deprecated: the type is deprecated (since 9.1)
+#
+# @not-secure: the type (typically a device) is not considered
+#     a security boundary (since 9.1)
+#
 # Since: 1.1
 ##
 { 'struct': 'ObjectTypeInfo',
-  'data': { 'name': 'str', '*abstract': 'bool', '*parent': 'str' } }
+  'data': { 'name': 'str', '*abstract': 'bool', '*parent': 'str',
+            '*deprecated': 'bool', '*not-secure': 'bool' } }
 
 ##
 # @qom-list-types:
-- 
2.45.2

[PATCH v3 2/4] usb/hub: mark as deprecated

[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 +
 1 file changed, 1 insertion(+)

diff --git a/hw/usb/dev-hub.c b/hw/usb/dev-hub.c
index 06e9537d0356..bc8d0ba4cfcf 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->deprecated = true;
     device_class_set_props(dc, usb_hub_properties);
 }
 
-- 
2.45.2

[PATCH v3 3/4] vga/cirrus: mark as not secure

[Gerd Hoffmann]

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

diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index 150883a97166..1f4c55b21415 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->not_secure = true;
 }
 
 static const TypeInfo cirrus_vga_info = {
diff --git a/hw/display/cirrus_vga_isa.c b/hw/display/cirrus_vga_isa.c
index 84be51670ed8..535a631b4b09 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->not_secure = true;
 }
 
 static const TypeInfo isa_cirrus_vga_info = {
-- 
2.45.2

[PATCH v3 4/4] qdev: add device policy [RfC]

[Gerd Hoffmann]

Add policies for devices which are deprecated or not secure.
There are three options: allow, warn and deny.

It's implemented for devices only.  Devices will probably be the main
user of this.  Also object_new() can't fail as of today so it's a bit
hard to implement policy checking at object level, especially the 'deny'
part of it.

TODO: add a command line option to actually set these policies.

Comments are welcome.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/core/qdev.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 59 insertions(+), 1 deletion(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index f3a996f57dee..0c4e5cec743c 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -43,6 +43,15 @@
 static bool qdev_hot_added = false;
 bool qdev_hot_removed = false;
 
+enum qdev_policy {
+    QDEV_ALLOW = 0,
+    QDEV_WARN  = 1,
+    QDEV_DENY  = 2,
+};
+
+static enum qdev_policy qdev_deprecated_policy;
+static enum qdev_policy qdev_not_secure_policy;
+
 const VMStateDescription *qdev_get_vmsd(DeviceState *dev)
 {
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
@@ -144,6 +153,43 @@ bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp)
     return true;
 }
 
+static bool qdev_class_check(const char *name, ObjectClass *oc)
+{
+    bool allow = true;
+
+    if (oc->deprecated) {
+        switch (qdev_deprecated_policy) {
+        case QDEV_WARN:
+            warn_report("device \"%s\" is deprecated", name);
+            break;
+        case QDEV_DENY:
+            error_report("device \"%s\" is deprecated", name);
+            allow = false;
+            break;
+        default:
+            /* nothing */
+            break;
+        }
+    }
+
+    if (oc->not_secure) {
+        switch (qdev_not_secure_policy) {
+        case QDEV_WARN:
+            warn_report("device \"%s\" is not secure", name);
+            break;
+        case QDEV_DENY:
+            error_report("device \"%s\" is not secure", name);
+            allow = false;
+            break;
+        default:
+            /* nothing */
+            break;
+        }
+    }
+
+    return allow;
+}
+
 DeviceState *qdev_new(const char *name)
 {
     ObjectClass *oc = object_class_by_name(name);
@@ -162,14 +208,26 @@ DeviceState *qdev_new(const char *name)
         error_report("unknown type '%s'", name);
         abort();
     }
+
+    if (!qdev_class_check(name, oc)) {
+        exit(1);
+    }
+
     return DEVICE(object_new(name));
 }
 
 DeviceState *qdev_try_new(const char *name)
 {
-    if (!module_object_class_by_name(name)) {
+    ObjectClass *oc = module_object_class_by_name(name);
+
+    if (!oc) {
         return NULL;
     }
+
+    if (!qdev_class_check(name, oc)) {
+        return NULL;
+    }
+
     return DEVICE(object_new(name));
 }
 
-- 
2.45.2

Re: [PATCH v3 3/4] vga/cirrus: mark as not secure

[Daniel P. Berrangé]

On Thu, Jun 06, 2024 at 04:30:09PM +0200, Gerd Hoffmann wrote:

What's the justification for declaring cirrus to be insecure ?

It is shipped as a driver in RHEL for years, was the default graphics
adapter for most of this time, and bugs in it are considered CVEs
still.

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/display/cirrus_vga.c     | 1 +
>  hw/display/cirrus_vga_isa.c | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
> index 150883a97166..1f4c55b21415 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->not_secure = true;
>  }
>  
>  static const TypeInfo cirrus_vga_info = {
> diff --git a/hw/display/cirrus_vga_isa.c b/hw/display/cirrus_vga_isa.c
> index 84be51670ed8..535a631b4b09 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->not_secure = true;
>  }
>  
>  static const TypeInfo isa_cirrus_vga_info = {
> -- 
> 2.45.2
> 

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 v3 1/4] qom: allow to mark objects as deprecated or not secure.

[Daniel P. Berrangé]

On Thu, Jun 06, 2024 at 04:30:07PM +0200, Gerd Hoffmann wrote:
> Add flags to ObjectClass for objects which are deprecated or not secure.
> Add 'deprecated' and 'not-secure' bools to ObjectTypeInfo, report in
> 'qom-list-types'.  Print the flags when listing devices via '-device
> help'.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/qom/object.h  | 3 +++
>  qom/qom-qmp-cmds.c    | 8 ++++++++
>  system/qdev-monitor.c | 8 ++++++++
>  qapi/qom.json         | 8 +++++++-
>  4 files changed, 26 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 v3 2/4] usb/hub: mark as deprecated

[Daniel P. Berrangé]

On Thu, Jun 06, 2024 at 04:30:08PM +0200, Gerd Hoffmann wrote:
> 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.

Is that actually a strong enough reason to delete this device though ?
This reads like its merely something we don't expect to be commonly
used, rather than something we would actively want to delete.

> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/usb/dev-hub.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/usb/dev-hub.c b/hw/usb/dev-hub.c
> index 06e9537d0356..bc8d0ba4cfcf 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->deprecated = true;
>      device_class_set_props(dc, usb_hub_properties);
>  }

Deprecations should also have an entry in docs/about/deprecated.rst to
warn users about the intent to delete the code in future.

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 v3 4/4] qdev: add device policy [RfC]

[Peter Maydell]

On Thu, 6 Jun 2024 at 15:31, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> Add policies for devices which are deprecated or not secure.
> There are three options: allow, warn and deny.
>
> It's implemented for devices only.  Devices will probably be the main
> user of this.  Also object_new() can't fail as of today so it's a bit
> hard to implement policy checking at object level, especially the 'deny'
> part of it.
>
> TODO: add a command line option to actually set these policies.
>
> Comments are welcome.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

> @@ -162,14 +208,26 @@ DeviceState *qdev_new(const char *name)
>          error_report("unknown type '%s'", name);
>          abort();
>      }
> +
> +    if (!qdev_class_check(name, oc)) {
> +        exit(1);
> +    }
> +
>      return DEVICE(object_new(name));
>  }
>
>  DeviceState *qdev_try_new(const char *name)
>  {
> -    if (!module_object_class_by_name(name)) {
> +    ObjectClass *oc = module_object_class_by_name(name);
> +
> +    if (!oc) {
>          return NULL;
>      }
> +
> +    if (!qdev_class_check(name, oc)) {
> +        return NULL;
> +    }
> +
>      return DEVICE(object_new(name));
>  }

It's valid to create a qdev device via object_new(), so
this doesn't work as a place to put the check. My suggestion
would be to restrict the deprecation handling to qdev only,
not to objects in general. Then you can do it in the
qdev device base class realize method, and fail realize
if it's not supported.

(qdev_try_new() is one of those "we use this in just 4
places" APIs that always tempts me to wonder if we should
really have it...)

thanks
-- PMM

Re: [PATCH v3 1/4] qom: allow to mark objects as deprecated or not secure.

[Philippe Mathieu-Daudé]

On 6/6/24 16:30, Gerd Hoffmann wrote:
> Add flags to ObjectClass for objects which are deprecated or not secure.
> Add 'deprecated' and 'not-secure' bools to ObjectTypeInfo, report in
> 'qom-list-types'.  Print the flags when listing devices via '-device
> help'.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   include/qom/object.h  | 3 +++
>   qom/qom-qmp-cmds.c    | 8 ++++++++
>   system/qdev-monitor.c | 8 ++++++++
>   qapi/qom.json         | 8 +++++++-
>   4 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 13d3a655ddf9..419bd9a4b219 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -136,6 +136,9 @@ struct ObjectClass
>       ObjectUnparent *unparent;
>   
>       GHashTable *properties;
> +
> +    bool deprecated;
> +    bool not_secure;

LGTM but I'd rather use a reason string instead of a boolean,
so we are forced to justify.

That would be in line with MachineClass::deprecation_reason:

  * MachineClass:
  * @deprecation_reason: If set, the machine is marked as deprecated.
  *    The string should provide some clear information about what to
  *    use instead.

>   };
>   
>   /**
> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
> index e91a2353472a..325ff0ba2a25 100644
> --- a/qom/qom-qmp-cmds.c
> +++ b/qom/qom-qmp-cmds.c
> @@ -101,6 +101,14 @@ static void qom_list_types_tramp(ObjectClass *klass, void *data)
>       if (parent) {
>           info->parent = g_strdup(object_class_get_name(parent));
>       }
> +    if (klass->deprecated) {
> +        info->has_deprecated = true;
> +        info->deprecated = true;
> +    }
> +    if (klass->not_secure) {
> +        info->has_not_secure = true;
> +        info->not_secure = true;
> +    }
>   
>       QAPI_LIST_PREPEND(*pret, info);
>   }
> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> index 6af6ef7d667f..effdc95d21d3 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,12 @@ static void qdev_print_devinfo(DeviceClass *dc)
>       if (!dc->user_creatable) {
>           qemu_printf(", no-user");
>       }
> +    if (klass->deprecated) {
> +        qemu_printf(", deprecated");
> +    }
> +    if (klass->not_secure) {
> +        qemu_printf(", not-secure");
> +    }
>       qemu_printf("\n");
>   }
>   
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 8bd299265e39..3f20d4c6413b 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -163,10 +163,16 @@
>   #
>   # @parent: Name of parent type, if any (since 2.10)
>   #
> +# @deprecated: the type is deprecated (since 9.1)
> +#
> +# @not-secure: the type (typically a device) is not considered
> +#     a security boundary (since 9.1)
> +#
>   # Since: 1.1
>   ##
>   { 'struct': 'ObjectTypeInfo',
> -  'data': { 'name': 'str', '*abstract': 'bool', '*parent': 'str' } }
> +  'data': { 'name': 'str', '*abstract': 'bool', '*parent': 'str',
> +            '*deprecated': 'bool', '*not-secure': 'bool' } }
>   
>   ##
>   # @qom-list-types:

Re: [PATCH v3 4/4] qdev: add device policy [RfC]

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> Add policies for devices which are deprecated or not secure.
> There are three options: allow, warn and deny.
>
> It's implemented for devices only.  Devices will probably be the main
> user of this.  Also object_new() can't fail as of today so it's a bit
> hard to implement policy checking at object level, especially the 'deny'
> part of it.
>
> TODO: add a command line option to actually set these policies.
>
> Comments are welcome.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/core/qdev.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 59 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index f3a996f57dee..0c4e5cec743c 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -43,6 +43,15 @@
>  static bool qdev_hot_added = false;
>  bool qdev_hot_removed = false;
>  
> +enum qdev_policy {
> +    QDEV_ALLOW = 0,
> +    QDEV_WARN  = 1,
> +    QDEV_DENY  = 2,
> +};
> +
> +static enum qdev_policy qdev_deprecated_policy;
> +static enum qdev_policy qdev_not_secure_policy;
> +
>  const VMStateDescription *qdev_get_vmsd(DeviceState *dev)
>  {
>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
> @@ -144,6 +153,43 @@ bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp)
>      return true;
>  }
>  
> +static bool qdev_class_check(const char *name, ObjectClass *oc)
> +{
> +    bool allow = true;
> +
> +    if (oc->deprecated) {
> +        switch (qdev_deprecated_policy) {
> +        case QDEV_WARN:
> +            warn_report("device \"%s\" is deprecated", name);
> +            break;
> +        case QDEV_DENY:
> +            error_report("device \"%s\" is deprecated", name);
> +            allow = false;
> +            break;
> +        default:
> +            /* nothing */
> +            break;
> +        }
> +    }
> +
> +    if (oc->not_secure) {
> +        switch (qdev_not_secure_policy) {
> +        case QDEV_WARN:
> +            warn_report("device \"%s\" is not secure", name);
> +            break;
> +        case QDEV_DENY:
> +            error_report("device \"%s\" is not secure", name);
> +            allow = false;
> +            break;
> +        default:
> +            /* nothing */
> +            break;
> +        }
> +    }
> +
> +    return allow;
> +}
> +
>  DeviceState *qdev_new(const char *name)
>  {
>      ObjectClass *oc = object_class_by_name(name);
> @@ -162,14 +208,26 @@ DeviceState *qdev_new(const char *name)
>          error_report("unknown type '%s'", name);
>          abort();
>      }
> +
> +    if (!qdev_class_check(name, oc)) {

Anti-pattern: use of error_report() from within a function that returns
an error via Error **errp argument.

One such call chain: QMP core -> qmp_device_add() -> qdev_device_add()
-> qdev_device_add_from_qdict() -> qdev_new().  Your error message goes
to stderr, which is wrong.

> +        exit(1);

Worse, QMP command device_add can now kill the guest instantly.

You need to lift the check up the call chains some.

> +    }
> +
>      return DEVICE(object_new(name));
>  }
>  
>  DeviceState *qdev_try_new(const char *name)
>  {
> -    if (!module_object_class_by_name(name)) {
> +    ObjectClass *oc = module_object_class_by_name(name);
> +
> +    if (!oc) {
>          return NULL;
>      }
> +
> +    if (!qdev_class_check(name, oc)) {
> +        return NULL;
> +    }
> +
>      return DEVICE(object_new(name));
>  }

Re: [PATCH v3 1/4] qom: allow to mark objects as deprecated or not secure.

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> Add flags to ObjectClass for objects which are deprecated or not secure.
> Add 'deprecated' and 'not-secure' bools to ObjectTypeInfo, report in
> 'qom-list-types'.  Print the flags when listing devices via '-device
> help'.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/qom/object.h  | 3 +++
>  qom/qom-qmp-cmds.c    | 8 ++++++++
>  system/qdev-monitor.c | 8 ++++++++
>  qapi/qom.json         | 8 +++++++-
>  4 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 13d3a655ddf9..419bd9a4b219 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -136,6 +136,9 @@ struct ObjectClass
>      ObjectUnparent *unparent;
>  
>      GHashTable *properties;
> +
> +    bool deprecated;
> +    bool not_secure;
>  };

Ignorant question: should this be in struct TypeImpl instead?

>  
>  /**
> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
> index e91a2353472a..325ff0ba2a25 100644
> --- a/qom/qom-qmp-cmds.c
> +++ b/qom/qom-qmp-cmds.c
> @@ -101,6 +101,14 @@ static void qom_list_types_tramp(ObjectClass *klass, void *data)
>      if (parent) {
>          info->parent = g_strdup(object_class_get_name(parent));
>      }
> +    if (klass->deprecated) {
> +        info->has_deprecated = true;
> +        info->deprecated = true;
> +    }
> +    if (klass->not_secure) {
> +        info->has_not_secure = true;
> +        info->not_secure = true;
> +    }
>  
>      QAPI_LIST_PREPEND(*pret, info);
>  }
> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> index 6af6ef7d667f..effdc95d21d3 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,12 @@ static void qdev_print_devinfo(DeviceClass *dc)
>      if (!dc->user_creatable) {
>          qemu_printf(", no-user");
>      }
> +    if (klass->deprecated) {
> +        qemu_printf(", deprecated");
> +    }
> +    if (klass->not_secure) {
> +        qemu_printf(", not-secure");
> +    }
>      qemu_printf("\n");
>  }
>  
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 8bd299265e39..3f20d4c6413b 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -163,10 +163,16 @@
>  #
>  # @parent: Name of parent type, if any (since 2.10)
>  #
> +# @deprecated: the type is deprecated (since 9.1)
> +#
> +# @not-secure: the type (typically a device) is not considered
> +#     a security boundary (since 9.1)

What does this mean?  Does it mean "do not add an instance of this
device the guest unless you trust the guest"?

> +#
>  # Since: 1.1
>  ##
>  { 'struct': 'ObjectTypeInfo',
> -  'data': { 'name': 'str', '*abstract': 'bool', '*parent': 'str' } }
> +  'data': { 'name': 'str', '*abstract': 'bool', '*parent': 'str',
> +            '*deprecated': 'bool', '*not-secure': 'bool' } }
>  
>  ##
>  # @qom-list-types:

I dislike booleans named "no-FOO" or "not-FOO", because they lead to
double-negation.

Re: [PATCH v3 1/4] qom: allow to mark objects as deprecated or not secure.

[Daniel P. Berrangé]

On Wed, Jun 12, 2024 at 01:07:44PM +0200, Markus Armbruster wrote:
> Gerd Hoffmann <kraxel@redhat.com> writes:
> 
> > Add flags to ObjectClass for objects which are deprecated or not secure.
> > Add 'deprecated' and 'not-secure' bools to ObjectTypeInfo, report in
> > 'qom-list-types'.  Print the flags when listing devices via '-device
> > help'.
> >
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >  include/qom/object.h  | 3 +++
> >  qom/qom-qmp-cmds.c    | 8 ++++++++
> >  system/qdev-monitor.c | 8 ++++++++
> >  qapi/qom.json         | 8 +++++++-
> >  4 files changed, 26 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/qom/object.h b/include/qom/object.h
> > index 13d3a655ddf9..419bd9a4b219 100644
> > --- a/include/qom/object.h
> > +++ b/include/qom/object.h
> > @@ -136,6 +136,9 @@ struct ObjectClass
> >      ObjectUnparent *unparent;
> >  
> >      GHashTable *properties;
> > +
> > +    bool deprecated;
> > +    bool not_secure;
> >  };
> 
> Ignorant question: should this be in struct TypeImpl instead?
> 
> >  
> >  /**
> > diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
> > index e91a2353472a..325ff0ba2a25 100644
> > --- a/qom/qom-qmp-cmds.c
> > +++ b/qom/qom-qmp-cmds.c
> > @@ -101,6 +101,14 @@ static void qom_list_types_tramp(ObjectClass *klass, void *data)
> >      if (parent) {
> >          info->parent = g_strdup(object_class_get_name(parent));
> >      }
> > +    if (klass->deprecated) {
> > +        info->has_deprecated = true;
> > +        info->deprecated = true;
> > +    }
> > +    if (klass->not_secure) {
> > +        info->has_not_secure = true;
> > +        info->not_secure = true;
> > +    }
> >  
> >      QAPI_LIST_PREPEND(*pret, info);
> >  }
> > diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> > index 6af6ef7d667f..effdc95d21d3 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,12 @@ static void qdev_print_devinfo(DeviceClass *dc)
> >      if (!dc->user_creatable) {
> >          qemu_printf(", no-user");
> >      }
> > +    if (klass->deprecated) {
> > +        qemu_printf(", deprecated");
> > +    }
> > +    if (klass->not_secure) {
> > +        qemu_printf(", not-secure");
> > +    }
> >      qemu_printf("\n");
> >  }
> >  
> > diff --git a/qapi/qom.json b/qapi/qom.json
> > index 8bd299265e39..3f20d4c6413b 100644
> > --- a/qapi/qom.json
> > +++ b/qapi/qom.json
> > @@ -163,10 +163,16 @@
> >  #
> >  # @parent: Name of parent type, if any (since 2.10)
> >  #
> > +# @deprecated: the type is deprecated (since 9.1)
> > +#
> > +# @not-secure: the type (typically a device) is not considered
> > +#     a security boundary (since 9.1)
> 
> What does this mean?  Does it mean "do not add an instance of this
> device the guest unless you trust the guest"?

Essentially yes. This ties to our security doc where we declare
we won't consider non-virtualization use cases as being security
bugs (CVEs) as large parts of QEMU haven't been designed to
provide a guest security boundary

  https://www.qemu.org/docs/master/system/security.html


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 v3 0/4] allow to deprecate objects and devices

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

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

I can see usb-hub [PATCH 2], but not "ohci pci host adapter".  Peeking
at the change log below... dropped in v2?

> v3:
>  - switch to two properties: 'deprecated' and 'not secure' flags.
>  - add rfc patch implementing policies for devices with flags.
>
> v2:
>  - pick up reviews.
>  - drop ohci patch.
>  - add cirrus vga patch.
>
> Gerd Hoffmann (4):
>   qom: allow to mark objects as deprecated or not secure.
>   usb/hub: mark as deprecated
>   vga/cirrus: mark as not secure

This part isn't mentioned in the cover letter.

>   qdev: add device policy [RfC]

There's overlap with QAPI special feature 'deprecated'.

QMP command object_add has argument @qom-type, which is an enumeration
of (user-creatable) object types.  The proper way to mark one of these
deprecated is to tack feature 'deprecated' to it.  It is then subject to
policy set with -compat deprecated-input=XXX, and is visible in
query-qmp-schema.  Modern management applications should already know
how to deal with it there.

This is definitely how we should deprecate user-creatable objects.
Adding a second way to do it seems undesirable to me.

Trouble is QMP command device_add still mostly bypasses QAPI.  Its
argument @driver is a string.

QAPIfying device_add properly has been on our (unwritten) wishlist since
forever.  Kevin (cc'ed) explored it some not too long ago.

I figure you want the means to deprecate devices now rather than after
we figure out how to QAPIfy device_add.  That's fair.

I think we should limit this series just to devices.  It'll become
redundant if we ever succeed at QAPIfying device_add.  No need to worry
about that now.

Re: [PATCH v3 1/4] qom: allow to mark objects as deprecated or not secure.

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Jun 12, 2024 at 01:07:44PM +0200, Markus Armbruster wrote:
>> Gerd Hoffmann <kraxel@redhat.com> writes:
>> 
>> > Add flags to ObjectClass for objects which are deprecated or not secure.
>> > Add 'deprecated' and 'not-secure' bools to ObjectTypeInfo, report in
>> > 'qom-list-types'.  Print the flags when listing devices via '-device
>> > help'.
>> >
>> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

[...]

>> > diff --git a/qapi/qom.json b/qapi/qom.json
>> > index 8bd299265e39..3f20d4c6413b 100644
>> > --- a/qapi/qom.json
>> > +++ b/qapi/qom.json
>> > @@ -163,10 +163,16 @@
>> >  #
>> >  # @parent: Name of parent type, if any (since 2.10)
>> >  #
>> > +# @deprecated: the type is deprecated (since 9.1)
>> > +#
>> > +# @not-secure: the type (typically a device) is not considered
>> > +#     a security boundary (since 9.1)
>> 
>> What does this mean?  Does it mean "do not add an instance of this
>> device the guest unless you trust the guest"?
>
> Essentially yes. This ties to our security doc where we declare
> we won't consider non-virtualization use cases as being security
> bugs (CVEs) as large parts of QEMU haven't been designed to
> provide a guest security boundary
>
>   https://www.qemu.org/docs/master/system/security.html

Would it make sense to add a suitable pointer to the doc comment?

Re: [PATCH v3 2/4] usb/hub: mark as deprecated

[Alex Bennée]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Jun 06, 2024 at 04:30:08PM +0200, Gerd Hoffmann wrote:
>> 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.
>
> Is that actually a strong enough reason to delete this device though ?
> This reads like its merely something we don't expect to be commonly
> used, rather than something we would actively want to delete.

This does seem quite aggressive because there may be cases when users
explicitly want to use old devices. Maybe there is need for a third
state (better_alternatives?) so we can steer users away from old command
lines they may have picked up from the web to the modern alternative?


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v3 2/4] usb/hub: mark as deprecated

[Markus Armbruster]

Alex Bennée <alex.bennee@linaro.org> writes:

> Daniel P. Berrangé <berrange@redhat.com> writes:
>
>> On Thu, Jun 06, 2024 at 04:30:08PM +0200, Gerd Hoffmann wrote:
>>> 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.
>>
>> Is that actually a strong enough reason to delete this device though ?
>> This reads like its merely something we don't expect to be commonly
>> used, rather than something we would actively want to delete.
>
> This does seem quite aggressive because there may be cases when users
> explicitly want to use old devices. Maybe there is need for a third
> state (better_alternatives?) so we can steer users away from old command
> lines they may have picked up from the web to the modern alternative?

What exactly do we mean when we call something deprecated?

For me, it means "you should not normally use this".

Important special case: "because we intend to remove it."

But there can be other cases, say "because <alternative> will serve you
better".

Re: [PATCH v3 2/4] usb/hub: mark as deprecated

[Daniel P. Berrangé]

On Thu, Jun 13, 2024 at 10:31:56AM +0200, Markus Armbruster wrote:
> Alex Bennée <alex.bennee@linaro.org> writes:
> 
> > Daniel P. Berrangé <berrange@redhat.com> writes:
> >
> >> On Thu, Jun 06, 2024 at 04:30:08PM +0200, Gerd Hoffmann wrote:
> >>> 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.
> >>
> >> Is that actually a strong enough reason to delete this device though ?
> >> This reads like its merely something we don't expect to be commonly
> >> used, rather than something we would actively want to delete.
> >
> > This does seem quite aggressive because there may be cases when users
> > explicitly want to use old devices. Maybe there is need for a third
> > state (better_alternatives?) so we can steer users away from old command
> > lines they may have picked up from the web to the modern alternative?
> 
> What exactly do we mean when we call something deprecated?
> 
> For me, it means "you should not normally use this".
> 
> Important special case: "because we intend to remove it."

That's not the special case, it is the regular case - the documented
meaning of 'deprecated' in QEMU. When we deprecate something, it is
a warning that we intend to delete it in 2 releases time.

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 v3 2/4] usb/hub: mark as deprecated

[Daniel P. Berrangé]

On Wed, Jun 12, 2024 at 04:52:50PM +0100, Alex Bennée wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Thu, Jun 06, 2024 at 04:30:08PM +0200, Gerd Hoffmann wrote:
> >> 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.
> >
> > Is that actually a strong enough reason to delete this device though ?
> > This reads like its merely something we don't expect to be commonly
> > used, rather than something we would actively want to delete.
> 
> This does seem quite aggressive because there may be cases when users
> explicitly want to use old devices. Maybe there is need for a third
> state (better_alternatives?) so we can steer users away from old command
> lines they may have picked up from the web to the modern alternative?

We've got 2 flags proposed in patch 1 - "deprecated" and "not_secure" -
which we formally expose to mgmt apps/users. Both of these are actionable
in an unambiguous way. An application can query this info, and can also
tell QEMU to enforce policy on this. Both are good.

The "better alternatives" conceptable, however, is an inherantly fuzzy
concept, because "better" is very much a use-case depedent / eye of the
beholder concept. This would makes it difficult/impractical for an
application to take action based on such a "better-alternatives' schema
marker. It would imply the application has to understands the better
alternatives ahead of time, as well as understanding the end users'
intent and that's not really viable.



This is a long winded way of saying that while "better alternatives"
is certainly a concept that is relevant, I'm not convinced it belongs
in the schema, as opposed to being a documentation task.

We haven't consistently provided documentation in the manual for every
device, so in many cases '-device help' is all that exists, but in the
case of USB we do actually have docs:

  https://www.qemu.org/docs/master/system/devices/usb.html

and those docs give little guidance to users about 'usb-hub', so IMHO
that's where we should be documenting the tradeoffs of the different
USB config scenarios.

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 v3 2/4] usb/hub: mark as deprecated

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Jun 13, 2024 at 10:31:56AM +0200, Markus Armbruster wrote:
>> Alex Bennée <alex.bennee@linaro.org> writes:
>> 
>> > Daniel P. Berrangé <berrange@redhat.com> writes:
>> >
>> >> On Thu, Jun 06, 2024 at 04:30:08PM +0200, Gerd Hoffmann wrote:
>> >>> 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.
>> >>
>> >> Is that actually a strong enough reason to delete this device though ?
>> >> This reads like its merely something we don't expect to be commonly
>> >> used, rather than something we would actively want to delete.
>> >
>> > This does seem quite aggressive because there may be cases when users
>> > explicitly want to use old devices. Maybe there is need for a third
>> > state (better_alternatives?) so we can steer users away from old command
>> > lines they may have picked up from the web to the modern alternative?
>> 
>> What exactly do we mean when we call something deprecated?
>> 
>> For me, it means "you should not normally use this".
>> 
>> Important special case: "because we intend to remove it."
>
> That's not the special case, it is the regular case - the documented
> meaning of 'deprecated' in QEMU. When we deprecate something, it is
> a warning that we intend to delete it in 2 releases time.

It's the regular case in QEMU today because we made it so there, by
electing to limit deprecation to "because we intend to remove it."

Re: [PATCH v3 2/4] usb/hub: mark as deprecated

[Daniel P. Berrangé]

On Thu, Jun 13, 2024 at 12:38:31PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Thu, Jun 13, 2024 at 10:31:56AM +0200, Markus Armbruster wrote:
> >> Alex Bennée <alex.bennee@linaro.org> writes:
> >> 
> >> > Daniel P. Berrangé <berrange@redhat.com> writes:
> >> >
> >> >> On Thu, Jun 06, 2024 at 04:30:08PM +0200, Gerd Hoffmann wrote:
> >> >>> 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.
> >> >>
> >> >> Is that actually a strong enough reason to delete this device though ?
> >> >> This reads like its merely something we don't expect to be commonly
> >> >> used, rather than something we would actively want to delete.
> >> >
> >> > This does seem quite aggressive because there may be cases when users
> >> > explicitly want to use old devices. Maybe there is need for a third
> >> > state (better_alternatives?) so we can steer users away from old command
> >> > lines they may have picked up from the web to the modern alternative?
> >> 
> >> What exactly do we mean when we call something deprecated?
> >> 
> >> For me, it means "you should not normally use this".
> >> 
> >> Important special case: "because we intend to remove it."
> >
> > That's not the special case, it is the regular case - the documented
> > meaning of 'deprecated' in QEMU. When we deprecate something, it is
> > a warning that we intend to delete it in 2 releases time.
> 
> It's the regular case in QEMU today because we made it so there, by
> electing to limit deprecation to "because we intend to remove it."

Fair point, but assigning additional meanings to the existing 'deprecation'
concept will undermine the value QEMU & its consumers obtain from current
usage.

Consumers know if they see the "deprecated" marker, it has started a clock
ticking and they must immediately plan work to stop using the feature.

QEMU maintainers know if they see the 'deprecated' marker and it has been
2 releases, then we can delete it.

I don't want to loose that clear & easily understood meaning, by overloading
"deprecated" for scenarios like "it is sometimes better to use a different
device instead of this one, depending on factors X, Y & Z".

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 v3 2/4] usb/hub: mark as deprecated

[Alex Bennée]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Jun 13, 2024 at 12:38:31PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > On Thu, Jun 13, 2024 at 10:31:56AM +0200, Markus Armbruster wrote:
>> >> Alex Bennée <alex.bennee@linaro.org> writes:
>> >> 
>> >> > Daniel P. Berrangé <berrange@redhat.com> writes:
>> >> >
>> >> >> On Thu, Jun 06, 2024 at 04:30:08PM +0200, Gerd Hoffmann wrote:
>> >> >>> 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.
>> >> >>
>> >> >> Is that actually a strong enough reason to delete this device though ?
>> >> >> This reads like its merely something we don't expect to be commonly
>> >> >> used, rather than something we would actively want to delete.
>> >> >
>> >> > This does seem quite aggressive because there may be cases when users
>> >> > explicitly want to use old devices. Maybe there is need for a third
>> >> > state (better_alternatives?) so we can steer users away from old command
>> >> > lines they may have picked up from the web to the modern alternative?
>> >> 
>> >> What exactly do we mean when we call something deprecated?
>> >> 
>> >> For me, it means "you should not normally use this".
>> >> 
>> >> Important special case: "because we intend to remove it."
>> >
>> > That's not the special case, it is the regular case - the documented
>> > meaning of 'deprecated' in QEMU. When we deprecate something, it is
>> > a warning that we intend to delete it in 2 releases time.
>> 
>> It's the regular case in QEMU today because we made it so there, by
>> electing to limit deprecation to "because we intend to remove it."
>
> Fair point, but assigning additional meanings to the existing 'deprecation'
> concept will undermine the value QEMU & its consumers obtain from current
> usage.
>
> Consumers know if they see the "deprecated" marker, it has started a clock
> ticking and they must immediately plan work to stop using the feature.
>
> QEMU maintainers know if they see the 'deprecated' marker and it has been
> 2 releases, then we can delete it.
>
> I don't want to loose that clear & easily understood meaning, by overloading
> "deprecated" for scenarios like "it is sometimes better to use a different
> device instead of this one, depending on factors X, Y & Z".

I agree we shouldn't overload the meaning if deprecated.

So to this case in point. Is there anything you can do with usb/hub that
you can't do with the newer xhci based one? Is it backward compatible
enough that an old kernel would work? Or are we talking really old
kernels at this point?

Is there non-PC hardware that utilises these hubs?

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v3 2/4] usb/hub: mark as deprecated

[Gerd Hoffmann]

On Thu, Jun 13, 2024 at 03:49:23PM GMT, Alex Bennée wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > I don't want to loose that clear & easily understood meaning, by overloading
> > "deprecated" for scenarios like "it is sometimes better to use a different
> > device instead of this one, depending on factors X, Y & Z".
> 
> I agree we shouldn't overload the meaning if deprecated.
> 
> So to this case in point. Is there anything you can do with usb/hub that
> you can't do with the newer xhci based one?

Well, it's a hub.  You can (virtually) plug it into a usb root port of a
ohci/uhci/ehci/xhci host adapter and get 8 more usb ports.  Those new
ports are usb 1.1 only though, so not very useful in a modern world,
except maybe for HID devices which don't need much bandwidth.

> Is it backward compatible
> enough that an old kernel would work? Or are we talking really old
> kernels at this point?

Pretty much everything should be able to drive the usb hub.

take care,
  Gerd

Re: [PATCH v3 2/4] usb/hub: mark as deprecated

[Gerd Hoffmann]

  Hi,

> > This does seem quite aggressive because there may be cases when users
> > explicitly want to use old devices. Maybe there is need for a third
> > state (better_alternatives?) so we can steer users away from old command
> > lines they may have picked up from the web to the modern alternative?
> 
> We've got 2 flags proposed in patch 1 - "deprecated" and "not_secure" -
> which we formally expose to mgmt apps/users. Both of these are actionable
> in an unambiguous way. An application can query this info, and can also
> tell QEMU to enforce policy on this. Both are good.

Well, given that people apparently don't want actually delete stuff
I'm not sure the 'deprecated' flag actually makes sense.

> The "better alternatives" conceptable, however, is an inherantly fuzzy
> concept, because "better" is very much a use-case depedent / eye of the
> beholder concept.

Well.  I think the use cases can be grouped into two cases:

  (1) Operating system museum.  People using qemu qemu to keep their
      beloved (historical) OS alive on modern hardware.

  (2) Virtualization.  Run your production workload in VMs.

For (1) "better alternatives" doesn't make sense because you have to
consider what your (old) guest OS supports.

For (2) I think it does make sense.  I'd be conservative here and would
define possible production workloads as "OS which is still supported
with security updates".  Hardware old enough to be supported by all
production workloads (which should roughly being 10-15 years in the
market) can go into the "better alternative" bucket.  Examples:

 - xhci is old enough and can replace ohci/uhci/ehci.
 - intel-hda is old enough and can replace ac97/es1370/sb/...
 - sata is old enough and can replace ide.
 - nvme isn't old enough yet I'd say (also no cdrom support so it
   can't fully replace ide/sata).
 - q35 is old enough and can replace pc.

> This would makes it difficult/impractical for an application to take
> action based on such a "better-alternatives' schema marker.  It would
> imply the application has to understands the better alternatives ahead
> of time, as well as understanding the end users' intent and that's not
> really viable.

Well, with the conservative classification it should be possible to
simply apply it universally.  Throw warnings or errors in case a device
with a "better-alternative" tag is used.  Or build qemu without these
devices.  Or move these devices to a sub-rpm (if modularized), so they
are not present by default but can be installed for the (1) use cases.

take care,
  Gerd