Re: [PATCH] qemu: Extend qemu.conf with PCR banks to activate during 'TPM manufacturing'

2021-10-28 Thread Daniel P . Berrangé 
On Thu, Oct 28, 2021 at 01:51:33PM -0400, Stefan Berger wrote:
> 
> On 10/28/21 07:04, Daniel P. Berrangé wrote:
> > On Wed, Oct 27, 2021 at 05:48:19PM -0400, Stefan Berger wrote:
> > > On 10/27/21 14:17, Marc-André Lureau wrote:
> > > > Hi
> > > > 
> > > > On Wed, Oct 27, 2021 at 9:00 PM Stefan Berger  
> > > > wrote:
> > > > > Extend qemu.conf with a configration option swtpm_active_pcr_banks 
> > > > > that
> > > > > allows a user to set a comma-separated list of PCR banks to activate
> > > > > during 'TPM manufacturing'. Valid PCR banks are sha1,sha256,sha384 and
> > > > > sha512.
> > > > > 
> > > > Why not put this option in swtpm_setup.conf instead?
> > > That is another option but it depends on when one wants to see the effect 
> > > or
> > > how one wants to control it. With newer libvirt or newer swtpm?
> > The obvious reason for putting it in swtpm_setup.conf is that it also
> > benefits people using swtpm in a non-libvirt scenario.
> > 
> > IMHO, we should put it in swtpm_setup.conf, and *also* have a build
> > time option in swtpm to configure the built-in default.
> > 
> > IOW, I'd expect RHEL-9 RPM swtpm.spec to pass
> > 
> >%configure --default-pcr-banks=sha256
> > 
> > and then have the swtpm_setup.conf option to allow admins to override
> > the distro default if they need a weaker setup on a host.
> 
> I now have a pending PR to swtpm that does this modulo using
> --enable-default-pcr-banks=sha256. The selection of the PCR banks to
> activate can then be done via swtpm_setup.conf active_pcr_banks =  PCR banks> entry, if provided, otherwise it's back to the configure line
> default.
> 
> https://github.com/stefanberger/swtpm/pull/615

Great, that looks good.

> > On the libvirt side, I think we could have a domain XML config option
> > for PCR banks, to allow the built-in default or admin local default to
> > be override per-VM.
> 
> Is there an example of an attribute that can only be set once in the domain
> XML and cannot be modified after? The choice of active PCR banks is limited
> to 'TPM manufacturing' time, which means swtpm_setup runs once only when the
> swtpm's state directory does not exist because later it would overwrite the
> entire state and erase all keys etc.. Later manipulations of the PCR banks
> would have to be done using the firmware menu, which exist in EDK2, SeaBIOS
> and SLOF.

Yeah, it is a little unusual, but then I guess we have the similarish
with other firmware selection, where setting "secure=yes|no" determines
which OVMF binary we pick to use.

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] qemu: Extend qemu.conf with PCR banks to activate during 'TPM manufacturing'

2021-10-28 Thread Stefan Berger 



On 10/28/21 07:04, Daniel P. Berrangé wrote:

On Wed, Oct 27, 2021 at 05:48:19PM -0400, Stefan Berger wrote:

On 10/27/21 14:17, Marc-André Lureau wrote:

Hi

On Wed, Oct 27, 2021 at 9:00 PM Stefan Berger  wrote:

Extend qemu.conf with a configration option swtpm_active_pcr_banks that
allows a user to set a comma-separated list of PCR banks to activate
during 'TPM manufacturing'. Valid PCR banks are sha1,sha256,sha384 and
sha512.


Why not put this option in swtpm_setup.conf instead?

That is another option but it depends on when one wants to see the effect or
how one wants to control it. With newer libvirt or newer swtpm?

The obvious reason for putting it in swtpm_setup.conf is that it also
benefits people using swtpm in a non-libvirt scenario.

IMHO, we should put it in swtpm_setup.conf, and *also* have a build
time option in swtpm to configure the built-in default.

IOW, I'd expect RHEL-9 RPM swtpm.spec to pass

   %configure --default-pcr-banks=sha256

and then have the swtpm_setup.conf option to allow admins to override
the distro default if they need a weaker setup on a host.


I now have a pending PR to swtpm that does this modulo using 
--enable-default-pcr-banks=sha256. The selection of the PCR banks to 
activate can then be done via swtpm_setup.conf active_pcr_banks = of PCR banks> entry, if provided, otherwise it's back to the configure 
line default.


https://github.com/stefanberger/swtpm/pull/615





On the libvirt side, I think we could have a domain XML config option
for PCR banks, to allow the built-in default or admin local default to
be override per-VM.


Is there an example of an attribute that can only be set once in the 
domain XML and cannot be modified after? The choice of active PCR banks 
is limited to 'TPM manufacturing' time, which means swtpm_setup runs 
once only when the swtpm's state directory does not exist because later 
it would overwrite the entire state and erase all keys etc.. Later 
manipulations of the PCR banks would have to be done using the firmware 
menu, which exist in EDK2, SeaBIOS and SLOF.


  Stefan



Regards,
Daniel

Re: [PATCH] qemu: Extend qemu.conf with PCR banks to activate during 'TPM manufacturing'

2021-10-28 Thread Daniel P . Berrangé 
On Wed, Oct 27, 2021 at 05:48:19PM -0400, Stefan Berger wrote:
> 
> On 10/27/21 14:17, Marc-André Lureau wrote:
> > Hi
> > 
> > On Wed, Oct 27, 2021 at 9:00 PM Stefan Berger  wrote:
> > > Extend qemu.conf with a configration option swtpm_active_pcr_banks that
> > > allows a user to set a comma-separated list of PCR banks to activate
> > > during 'TPM manufacturing'. Valid PCR banks are sha1,sha256,sha384 and
> > > sha512.
> > > 
> > Why not put this option in swtpm_setup.conf instead?
> 
> That is another option but it depends on when one wants to see the effect or
> how one wants to control it. With newer libvirt or newer swtpm?

The obvious reason for putting it in swtpm_setup.conf is that it also
benefits people using swtpm in a non-libvirt scenario.

IMHO, we should put it in swtpm_setup.conf, and *also* have a build
time option in swtpm to configure the built-in default.

IOW, I'd expect RHEL-9 RPM swtpm.spec to pass

  %configure --default-pcr-banks=sha256

and then have the swtpm_setup.conf option to allow admins to override
the distro default if they need a weaker setup on a host.


On the libvirt side, I think we could have a domain XML config option
for PCR banks, to allow the built-in default or admin local default to
be override per-VM.

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 9/9] qapi: Extend -compat to set policy for unstable interfaces

2021-10-28 Thread Markus Armbruster 
New option parameters unstable-input and unstable-output set policy
for unstable interfaces just like deprecated-input and
deprecated-output set policy for deprecated interfaces (see commit
6dd75472d5 "qemu-options: New -compat to set policy for deprecated
interfaces").  This is intended for testing users of the management
interfaces.  It is experimental.

For now, this covers only syntactic aspects of QMP, i.e. stuff tagged
with feature 'unstable'.  We may want to extend it to cover semantic
aspects, or the command line.

Note that there is no good way for management application to detect
presence of these new option parameters: they are not visible output
of query-qmp-schema or query-command-line-options.  Tolerable, because
it's meant for testing.  If running with -compat fails, skip the test.

Signed-off-by: Markus Armbruster 
Acked-by: John Snow 
---
 qapi/compat.json  |  6 +-
 include/qapi/util.h   |  1 +
 qapi/qmp-dispatch.c   |  6 ++
 qapi/qobject-output-visitor.c |  8 ++--
 qemu-options.hx   | 20 +++-
 scripts/qapi/events.py| 10 ++
 scripts/qapi/schema.py| 10 ++
 7 files changed, 49 insertions(+), 12 deletions(-)

diff --git a/qapi/compat.json b/qapi/compat.json
index 74a8493d3d..9bc9804abb 100644
--- a/qapi/compat.json
+++ b/qapi/compat.json
@@ -47,9 +47,13 @@
 #
 # @deprecated-input: how to handle deprecated input (default 'accept')
 # @deprecated-output: how to handle deprecated output (default 'accept')
+# @unstable-input: how to handle unstable input (default 'accept')
+# @unstable-output: how to handle unstable output (default 'accept')
 #
 # Since: 6.0
 ##
 { 'struct': 'CompatPolicy',
   'data': { '*deprecated-input': 'CompatPolicyInput',
-'*deprecated-output': 'CompatPolicyOutput' } }
+'*deprecated-output': 'CompatPolicyOutput',
+'*unstable-input': 'CompatPolicyInput',
+'*unstable-output': 'CompatPolicyOutput' } }
diff --git a/include/qapi/util.h b/include/qapi/util.h
index 0cc98db9f9..81a2b13a33 100644
--- a/include/qapi/util.h
+++ b/include/qapi/util.h
@@ -13,6 +13,7 @@
 
 typedef enum {
 QAPI_DEPRECATED,
+QAPI_UNSTABLE,
 } QapiSpecialFeature;
 
 typedef struct QEnumLookup {
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index e29ade134c..c5c6e521a2 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -59,6 +59,12 @@ bool compat_policy_input_ok(unsigned special_features,
 error_class, kind, name, errp)) {
 return false;
 }
+if ((special_features & (1u << QAPI_UNSTABLE))
+&& !compat_policy_input_ok1("Unstable",
+policy->unstable_input,
+error_class, kind, name, errp)) {
+return false;
+}
 return true;
 }
 
diff --git a/qapi/qobject-output-visitor.c b/qapi/qobject-output-visitor.c
index b155bf4149..74770edd73 100644
--- a/qapi/qobject-output-visitor.c
+++ b/qapi/qobject-output-visitor.c
@@ -212,8 +212,12 @@ static bool qobject_output_type_null(Visitor *v, const 
char *name,
 static bool qobject_output_policy_skip(Visitor *v, const char *name,
unsigned special_features)
 {
-return !(special_features & 1u << QAPI_DEPRECATED)
-|| v->compat_policy.deprecated_output == COMPAT_POLICY_OUTPUT_HIDE;
+CompatPolicy *pol = >compat_policy;
+
+return ((special_features & 1u << QAPI_DEPRECATED)
+&& pol->deprecated_output == COMPAT_POLICY_OUTPUT_HIDE)
+|| ((special_features & 1u << QAPI_UNSTABLE)
+&& pol->unstable_output == COMPAT_POLICY_OUTPUT_HIDE);
 }
 
 /* Finish building, and return the root object.
diff --git a/qemu-options.hx b/qemu-options.hx
index 5f375bbfa6..f051536b63 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3641,7 +3641,9 @@ DEFHEADING(Debug/Expert options:)
 
 DEF("compat", HAS_ARG, QEMU_OPTION_compat,
 "-compat 
[deprecated-input=accept|reject|crash][,deprecated-output=accept|hide]\n"
-"Policy for handling deprecated management interfaces\n",
+"Policy for handling deprecated management interfaces\n"
+"-compat 
[unstable-input=accept|reject|crash][,unstable-output=accept|hide]\n"
+"Policy for handling unstable management interfaces\n",
 QEMU_ARCH_ALL)
 SRST
 ``-compat 
[deprecated-input=@var{input-policy}][,deprecated-output=@var{output-policy}]``
@@ -3659,6 +3661,22 @@ SRST
 Suppress deprecated command results and events
 
 Limitation: covers only syntactic aspects of QMP.
+
+``-compat 
[unstable-input=@var{input-policy}][,unstable-output=@var{output-policy}]``
+Set policy for handling unstable management interfaces (experimental):
+
+``unstable-input=accept`` (default)
+Accept unstable commands and arguments
+``unstable-input=reject``
+Reject unstable commands

[PATCH v2 4/9] qapi: Tools for sets of special feature flags in generated code

2021-10-28 Thread Markus Armbruster 
New enum QapiSpecialFeature enumerates the special feature flags.

New helper gen_special_features() returns code to represent a
collection of special feature flags as a bitset.

The next few commits will put them to use.

Signed-off-by: Markus Armbruster 
Reviewed-by: John Snow 
---
 include/qapi/util.h| 4 
 scripts/qapi/gen.py| 8 
 scripts/qapi/schema.py | 3 +++
 3 files changed, 15 insertions(+)

diff --git a/include/qapi/util.h b/include/qapi/util.h
index 257c600f99..7a8d5c7d72 100644
--- a/include/qapi/util.h
+++ b/include/qapi/util.h
@@ -11,6 +11,10 @@
 #ifndef QAPI_UTIL_H
 #define QAPI_UTIL_H
 
+typedef enum {
+QAPI_DEPRECATED,
+} QapiSpecialFeature;
+
 /* QEnumLookup flags */
 #define QAPI_ENUM_DEPRECATED 1
 
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 2ec1e7b3b6..995a97d2b8 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -18,6 +18,7 @@
 Dict,
 Iterator,
 Optional,
+Sequence,
 Tuple,
 )
 
@@ -29,6 +30,7 @@
 mcgen,
 )
 from .schema import (
+QAPISchemaFeature,
 QAPISchemaIfCond,
 QAPISchemaModule,
 QAPISchemaObjectType,
@@ -37,6 +39,12 @@
 from .source import QAPISourceInfo
 
 
+def gen_special_features(features: Sequence[QAPISchemaFeature]) -> str:
+special_features = [f"1u << QAPI_{feat.name.upper()}"
+for feat in features if feat.is_special()]
+return ' | '.join(special_features) or '0'
+
+
 class QAPIGen:
 def __init__(self, fname: str):
 self.fname = fname
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 6d5f46509a..55f82d7389 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -725,6 +725,9 @@ def connect_doc(self, doc):
 class QAPISchemaFeature(QAPISchemaMember):
 role = 'feature'
 
+def is_special(self):
+return self.name in ('deprecated')
+
 
 class QAPISchemaObjectTypeMember(QAPISchemaMember):
 def __init__(self, name, info, typ, optional, ifcond=None, features=None):
-- 
2.31.1

[PATCH v2 8/9] qapi: Factor out compat_policy_input_ok()

2021-10-28 Thread Markus Armbruster 
The code to check policy for handling deprecated input is triplicated.
Factor it out into compat_policy_input_ok() before I mess with it in
the next commit.

Signed-off-by: Markus Armbruster 
Reviewed-by: Philippe Mathieu-Daudé 
---
 include/qapi/compat-policy.h |  7 +
 qapi/qapi-visit-core.c   | 18 +
 qapi/qmp-dispatch.c  | 51 +++-
 qapi/qobject-input-visitor.c | 19 +++---
 4 files changed, 55 insertions(+), 40 deletions(-)

diff --git a/include/qapi/compat-policy.h b/include/qapi/compat-policy.h
index 1083f95122..8b7b25c0b5 100644
--- a/include/qapi/compat-policy.h
+++ b/include/qapi/compat-policy.h
@@ -13,10 +13,17 @@
 #ifndef QAPI_COMPAT_POLICY_H
 #define QAPI_COMPAT_POLICY_H
 
+#include "qapi/error.h"
 #include "qapi/qapi-types-compat.h"
 
 extern CompatPolicy compat_policy;
 
+bool compat_policy_input_ok(unsigned special_features,
+const CompatPolicy *policy,
+ErrorClass error_class,
+const char *kind, const char *name,
+Error **errp);
+
 /*
  * Create a QObject input visitor for @obj for use with QMP
  *
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 34c59286b2..6c13510a2b 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -13,6 +13,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/compat-policy.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/visitor.h"
@@ -409,18 +410,11 @@ static bool input_type_enum(Visitor *v, const char *name, 
int *obj,
 }
 
 if (lookup->special_features
-&& (lookup->special_features[value] & QAPI_DEPRECATED)) {
-switch (v->compat_policy.deprecated_input) {
-case COMPAT_POLICY_INPUT_ACCEPT:
-break;
-case COMPAT_POLICY_INPUT_REJECT:
-error_setg(errp, "Deprecated value '%s' disabled by policy",
-   enum_str);
-return false;
-case COMPAT_POLICY_INPUT_CRASH:
-default:
-abort();
-}
+&& !compat_policy_input_ok(lookup->special_features[value],
+   >compat_policy,
+   ERROR_CLASS_GENERIC_ERROR,
+   "value", enum_str, errp)) {
+return false;
 }
 
 *obj = value;
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 8cca18c891..e29ade134c 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -28,6 +28,40 @@
 
 CompatPolicy compat_policy;
 
+static bool compat_policy_input_ok1(const char *adjective,
+CompatPolicyInput policy,
+ErrorClass error_class,
+const char *kind, const char *name,
+Error **errp)
+{
+switch (policy) {
+case COMPAT_POLICY_INPUT_ACCEPT:
+return true;
+case COMPAT_POLICY_INPUT_REJECT:
+error_set(errp, error_class, "%s %s %s disabled by policy",
+  adjective, kind, name);
+return false;
+case COMPAT_POLICY_INPUT_CRASH:
+default:
+abort();
+}
+}
+
+bool compat_policy_input_ok(unsigned special_features,
+const CompatPolicy *policy,
+ErrorClass error_class,
+const char *kind, const char *name,
+Error **errp)
+{
+if ((special_features & 1u << QAPI_DEPRECATED)
+&& !compat_policy_input_ok1("Deprecated",
+policy->deprecated_input,
+error_class, kind, name, errp)) {
+return false;
+}
+return true;
+}
+
 Visitor *qobject_input_visitor_new_qmp(QObject *obj)
 {
 Visitor *v = qobject_input_visitor_new(obj);
@@ -176,19 +210,10 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject 
*request,
   "The command %s has not been found", command);
 goto out;
 }
-if (cmd->special_features & 1u << QAPI_DEPRECATED) {
-switch (compat_policy.deprecated_input) {
-case COMPAT_POLICY_INPUT_ACCEPT:
-break;
-case COMPAT_POLICY_INPUT_REJECT:
-error_set(, ERROR_CLASS_COMMAND_NOT_FOUND,
-  "Deprecated command %s disabled by policy",
-  command);
-goto out;
-case COMPAT_POLICY_INPUT_CRASH:
-default:
-abort();
-}
+if (!compat_policy_input_ok(cmd->special_features, _policy,
+ERROR_CLASS_COMMAND_NOT_FOUND,
+"command", command, )) {
+goto out;
 }
 if (!cmd->enabled) {
 error_set(, ERROR_CLASS_COMMAND_NOT_FOUND,
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index c120dbdd92..f0b4c7ca9d 100644
---

[PATCH v2 2/9] qapi: Mark unstable QMP parts with feature 'unstable'

2021-10-28 Thread Markus Armbruster 
Add special feature 'unstable' everywhere the name starts with 'x-',
except for InputBarrierProperties member x-origin and
MemoryBackendProperties member x-use-canonical-path-for-ramblock-id,
because these two are actually stable.

Signed-off-by: Markus Armbruster 
Reviewed-by: Juan Quintela 
Acked-by: John Snow 
---
 qapi/block-core.json | 123 +++
 qapi/migration.json  |  35 +---
 qapi/misc.json   |   6 ++-
 qapi/qom.json|  11 ++--
 4 files changed, 130 insertions(+), 45 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 6d3217abb6..ce2c1352cb 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1438,6 +1438,9 @@
 #
 # @x-perf: Performance options. (Since 6.0)
 #
+# Features:
+# @unstable: Member @x-perf is experimental.
+#
 # Note: @on-source-error and @on-target-error only affect background
 #   I/O.  If an error occurs during a guest write request, the device's
 #   rerror/werror actions will be used.
@@ -1452,7 +1455,9 @@
 '*on-source-error': 'BlockdevOnError',
 '*on-target-error': 'BlockdevOnError',
 '*auto-finalize': 'bool', '*auto-dismiss': 'bool',
-'*filter-node-name': 'str', '*x-perf': 'BackupPerf'  } }
+'*filter-node-name': 'str',
+'*x-perf': { 'type': 'BackupPerf',
+ 'features': [ 'unstable' ] } } }
 
 ##
 # @DriveBackup:
@@ -1916,9 +1921,13 @@
 #
 # Get the block graph.
 #
+# Features:
+# @unstable: This command is meant for debugging.
+#
 # Since: 4.0
 ##
-{ 'command': 'x-debug-query-block-graph', 'returns': 'XDbgBlockGraph' }
+{ 'command': 'x-debug-query-block-graph', 'returns': 'XDbgBlockGraph',
+  'features': [ 'unstable' ] }
 
 ##
 # @drive-mirror:
@@ -2257,6 +2266,9 @@
 #
 # Get bitmap SHA256.
 #
+# Features:
+# @unstable: This command is meant for debugging.
+#
 # Returns: - BlockDirtyBitmapSha256 on success
 #  - If @node is not a valid block device, DeviceNotFound
 #  - If @name is not found or if hashing has failed, GenericError with 
an
@@ -2265,7 +2277,8 @@
 # Since: 2.10
 ##
 { 'command': 'x-debug-block-dirty-bitmap-sha256',
-  'data': 'BlockDirtyBitmap', 'returns': 'BlockDirtyBitmapSha256' }
+  'data': 'BlockDirtyBitmap', 'returns': 'BlockDirtyBitmapSha256',
+  'features': [ 'unstable' ] }
 
 ##
 # @blockdev-mirror:
@@ -2495,27 +2508,57 @@
 #
 # Properties for throttle-group objects.
 #
-# The options starting with x- are aliases for the same key without x- in
-# the @limits object. As indicated by the x- prefix, this is not a stable
-# interface and may be removed or changed incompatibly in the future. Use
-# @limits for a supported stable interface.
-#
 # @limits: limits to apply for this throttle group
 #
+# Features:
+# @unstable: All members starting with x- are aliases for the same key
+#without x- in the @limits object.  This is not a stable
+#interface and may be removed or changed incompatibly in
+#the future.  Use @limits for a supported stable
+#interface.
+#
 # Since: 2.11
 ##
 { 'struct': 'ThrottleGroupProperties',
   'data': { '*limits': 'ThrottleLimits',
-'*x-iops-total' : 'int', '*x-iops-total-max' : 'int',
-'*x-iops-total-max-length' : 'int', '*x-iops-read' : 'int',
-'*x-iops-read-max' : 'int', '*x-iops-read-max-length' : 'int',
-'*x-iops-write' : 'int', '*x-iops-write-max' : 'int',
-'*x-iops-write-max-length' : 'int', '*x-bps-total' : 'int',
-'*x-bps-total-max' : 'int', '*x-bps-total-max-length' : 'int',
-'*x-bps-read' : 'int', '*x-bps-read-max' : 'int',
-'*x-bps-read-max-length' : 'int', '*x-bps-write' : 'int',
-'*x-bps-write-max' : 'int', '*x-bps-write-max-length' : 'int',
-'*x-iops-size' : 'int' } }
+'*x-iops-total': { 'type': 'int',
+   'features': [ 'unstable' ] },
+'*x-iops-total-max': { 'type': 'int',
+   'features': [ 'unstable' ] },
+'*x-iops-total-max-length': { 'type': 'int',
+  'features': [ 'unstable' ] },
+'*x-iops-read': { 'type': 'int',
+  'features': [ 'unstable' ] },
+'*x-iops-read-max': { 'type': 'int',
+  'features': [ 'unstable' ] },
+'*x-iops-read-max-length': { 'type': 'int',
+ 'features': [ 'unstable' ] },
+'*x-iops-write': { 'type': 'int',
+   'features': [ 'unstable' ] },
+'*x-iops-write-max': { 'type': 'int',
+   'features': [ 'unstable' ] },
+'*x-iops-write-max-length': { 'type': 'int',
+  'features': [ 'unstable' ] },
+'*x-bps-total': {

[PATCH v2 5/9] qapi: Generalize struct member policy checking

2021-10-28 Thread Markus Armbruster 
The generated visitor functions call visit_deprecated_accept() and
visit_deprecated() when visiting a struct member with special feature
flag 'deprecated'.  This makes the feature flag visible to the actual
visitors.  I want to make feature flag 'unstable' visible there as
well, so I can add policy for it.

To let me make it visible, replace these functions by
visit_policy_reject() and visit_policy_skip(), which take the member's
special features as an argument.  Note that the new functions have the
opposite sense, i.e. the return value flips.

Signed-off-by: Markus Armbruster 
---
 include/qapi/visitor-impl.h   |  6 --
 include/qapi/visitor.h| 17 +
 qapi/qapi-forward-visitor.c   | 16 +---
 qapi/qapi-visit-core.c| 22 --
 qapi/qobject-input-visitor.c  | 15 ++-
 qapi/qobject-output-visitor.c |  9 ++---
 qapi/trace-events |  4 ++--
 scripts/qapi/visit.py | 14 +++---
 8 files changed, 63 insertions(+), 40 deletions(-)

diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 72b6537bef..2badec5ba4 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -114,10 +114,12 @@ struct Visitor
 void (*optional)(Visitor *v, const char *name, bool *present);
 
 /* Optional */
-bool (*deprecated_accept)(Visitor *v, const char *name, Error **errp);
+bool (*policy_reject)(Visitor *v, const char *name,
+  unsigned special_features, Error **errp);
 
 /* Optional */
-bool (*deprecated)(Visitor *v, const char *name);
+bool (*policy_skip)(Visitor *v, const char *name,
+unsigned special_features);
 
 /* Must be set */
 VisitorType type;
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index dcb96018a9..d53a84c9ba 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -461,22 +461,31 @@ void visit_end_alternate(Visitor *v, void **obj);
 bool visit_optional(Visitor *v, const char *name, bool *present);
 
 /*
- * Should we reject deprecated member @name?
+ * Should we reject member @name due to policy?
+ *
+ * @special_features is the member's special features encoded as a
+ * bitset of QapiSpecialFeature.
  *
  * @name must not be NULL.  This function is only useful between
  * visit_start_struct() and visit_end_struct(), since only objects
  * have deprecated members.
  */
-bool visit_deprecated_accept(Visitor *v, const char *name, Error **errp);
+bool visit_policy_reject(Visitor *v, const char *name,
+ unsigned special_features, Error **errp);
 
 /*
- * Should we visit deprecated member @name?
+ *
+ * Should we skip member @name due to policy?
+ *
+ * @special_features is the member's special features encoded as a
+ * bitset of QapiSpecialFeature.
  *
  * @name must not be NULL.  This function is only useful between
  * visit_start_struct() and visit_end_struct(), since only objects
  * have deprecated members.
  */
-bool visit_deprecated(Visitor *v, const char *name);
+bool visit_policy_skip(Visitor *v, const char *name,
+   unsigned special_features);
 
 /*
  * Set policy for handling deprecated management interfaces.
diff --git a/qapi/qapi-forward-visitor.c b/qapi/qapi-forward-visitor.c
index a4b111e22a..25d098aa8a 100644
--- a/qapi/qapi-forward-visitor.c
+++ b/qapi/qapi-forward-visitor.c
@@ -246,25 +246,27 @@ static void forward_field_optional(Visitor *v, const char 
*name, bool *present)
 visit_optional(ffv->target, name, present);
 }
 
-static bool forward_field_deprecated_accept(Visitor *v, const char *name,
-Error **errp)
+static bool forward_field_policy_reject(Visitor *v, const char *name,
+unsigned special_features,
+Error **errp)
 {
 ForwardFieldVisitor *ffv = to_ffv(v);
 
 if (!forward_field_translate_name(ffv, , errp)) {
 return false;
 }
-return visit_deprecated_accept(ffv->target, name, errp);
+return visit_policy_reject(ffv->target, name, special_features, errp);
 }
 
-static bool forward_field_deprecated(Visitor *v, const char *name)
+static bool forward_field_policy_skip(Visitor *v, const char *name,
+  unsigned special_features)
 {
 ForwardFieldVisitor *ffv = to_ffv(v);
 
 if (!forward_field_translate_name(ffv, , NULL)) {
 return false;
 }
-return visit_deprecated(ffv->target, name);
+return visit_policy_skip(ffv->target, name, special_features);
 }
 
 static void forward_field_complete(Visitor *v, void *opaque)
@@ -313,8 +315,8 @@ Visitor *visitor_forward_field(Visitor *target, const char 
*from, const char *to
 v->visitor.type_any = forward_field_type_any;
 v->visitor.type_null = forward_field_type_null;
 v->visitor.optional = forward_field_optional;
-

[PATCH v2 0/9] Configurable policy for handling unstable interfaces

2021-10-28 Thread Markus Armbruster 
Option -compat lets you configure what to do when deprecated
interfaces get used.  This series extends this to unstable interfaces.
Works the same way.  Intended for testing users of the management
interfaces.  It is experimental.

To make it possible, I replace the "x-" naming convention by special
feature flag "unstable".  See PATCH 1 for rationale.

v2:
* Rebased
* PATCH 1: Commit message revamped [Kevin], R-bys kept
* PATCH 6: gen_special_features() rewritten [John]
* PATCH 7: disastrous typos fixed [Philippe]

Markus Armbruster (9):
  qapi: New special feature flag "unstable"
  qapi: Mark unstable QMP parts with feature 'unstable'
  qapi: Eliminate QCO_NO_OPTIONS for a slight simplification
  qapi: Tools for sets of special feature flags in generated code
  qapi: Generalize struct member policy checking
  qapi: Generalize command policy checking
  qapi: Generalize enum member policy checking
  qapi: Factor out compat_policy_input_ok()
  qapi: Extend -compat to set policy for unstable interfaces

 docs/devel/qapi-code-gen.rst|   9 +-
 qapi/block-core.json| 123 +---
 qapi/compat.json|   6 +-
 qapi/migration.json |  35 +--
 qapi/misc.json  |   6 +-
 qapi/qom.json   |  11 ++-
 include/qapi/compat-policy.h|   7 ++
 include/qapi/qmp/dispatch.h |   6 +-
 include/qapi/util.h |   8 +-
 include/qapi/visitor-impl.h |   6 +-
 include/qapi/visitor.h  |  17 +++-
 monitor/misc.c  |   7 +-
 qapi/qapi-forward-visitor.c |  16 +--
 qapi/qapi-visit-core.c  |  41 
 qapi/qmp-dispatch.c |  57 ---
 qapi/qmp-registry.c |   4 +-
 qapi/qobject-input-visitor.c|  22 ++---
 qapi/qobject-output-visitor.c   |  13 ++-
 storage-daemon/qemu-storage-daemon.c|   3 +-
 qapi/trace-events   |   4 +-
 qemu-options.hx |  20 +++-
 scripts/qapi/commands.py|  12 +--
 scripts/qapi/events.py  |  10 +-
 scripts/qapi/gen.py |   8 ++
 scripts/qapi/schema.py  |  11 ++-
 scripts/qapi/types.py   |  22 +++--
 scripts/qapi/visit.py   |  14 +--
 tests/qapi-schema/qapi-schema-test.json |   7 +-
 tests/qapi-schema/qapi-schema-test.out  |   5 +
 29 files changed, 348 insertions(+), 162 deletions(-)

-- 
2.31.1

[PATCH v2 6/9] qapi: Generalize command policy checking

2021-10-28 Thread Markus Armbruster 
The code to check command policy can see special feature flag
'deprecated' as command flag QCO_DEPRECATED.  I want to make feature
flag 'unstable' visible there as well, so I can add policy for it.

To let me make it visible, add member @special_features (a bitset of
QapiSpecialFeature) to QmpCommand, and adjust the generator to pass it
through qmp_register_command().  Then replace "QCO_DEPRECATED in
@flags" by QAPI_DEPRECATED in @special_features", and drop
QCO_DEPRECATED.

Signed-off-by: Markus Armbruster 
Reviewed-by: Philippe Mathieu-Daudé 
Acked-by: John Snow 
---
 include/qapi/qmp/dispatch.h  | 5 +++--
 monitor/misc.c   | 6 --
 qapi/qmp-dispatch.c  | 2 +-
 qapi/qmp-registry.c  | 4 +++-
 storage-daemon/qemu-storage-daemon.c | 3 ++-
 scripts/qapi/commands.py | 9 -
 6 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 0ce88200b9..1e4240fd0d 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -25,7 +25,6 @@ typedef enum QmpCommandOptions
 QCO_ALLOW_OOB =  (1U << 1),
 QCO_ALLOW_PRECONFIG   =  (1U << 2),
 QCO_COROUTINE =  (1U << 3),
-QCO_DEPRECATED=  (1U << 4),
 } QmpCommandOptions;
 
 typedef struct QmpCommand
@@ -34,6 +33,7 @@ typedef struct QmpCommand
 /* Runs in coroutine context if QCO_COROUTINE is set */
 QmpCommandFunc *fn;
 QmpCommandOptions options;
+unsigned special_features;
 QTAILQ_ENTRY(QmpCommand) node;
 bool enabled;
 const char *disable_reason;
@@ -42,7 +42,8 @@ typedef struct QmpCommand
 typedef QTAILQ_HEAD(QmpCommandList, QmpCommand) QmpCommandList;
 
 void qmp_register_command(QmpCommandList *cmds, const char *name,
-  QmpCommandFunc *fn, QmpCommandOptions options);
+  QmpCommandFunc *fn, QmpCommandOptions options,
+  unsigned special_features);
 const QmpCommand *qmp_find_command(const QmpCommandList *cmds,
const char *name);
 void qmp_disable_command(QmpCommandList *cmds, const char *name,
diff --git a/monitor/misc.c b/monitor/misc.c
index 3556b177f6..c2d227a07c 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -230,11 +230,13 @@ static void monitor_init_qmp_commands(void)
 
 qmp_init_marshal(_commands);
 
-qmp_register_command(_commands, "device_add", qmp_device_add, 0);
+qmp_register_command(_commands, "device_add",
+ qmp_device_add, 0, 0);
 
 QTAILQ_INIT(_cap_negotiation_commands);
 qmp_register_command(_cap_negotiation_commands, "qmp_capabilities",
- qmp_marshal_qmp_capabilities, QCO_ALLOW_PRECONFIG);
+ qmp_marshal_qmp_capabilities,
+ QCO_ALLOW_PRECONFIG, 0);
 }
 
 /* Set the current CPU defined by the user. Callers must hold BQL. */
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 7e943a0af5..8cca18c891 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -176,7 +176,7 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject 
*request,
   "The command %s has not been found", command);
 goto out;
 }
-if (cmd->options & QCO_DEPRECATED) {
+if (cmd->special_features & 1u << QAPI_DEPRECATED) {
 switch (compat_policy.deprecated_input) {
 case COMPAT_POLICY_INPUT_ACCEPT:
 break;
diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c
index f78c064aae..485bc5e6fc 100644
--- a/qapi/qmp-registry.c
+++ b/qapi/qmp-registry.c
@@ -16,7 +16,8 @@
 #include "qapi/qmp/dispatch.h"
 
 void qmp_register_command(QmpCommandList *cmds, const char *name,
-  QmpCommandFunc *fn, QmpCommandOptions options)
+  QmpCommandFunc *fn, QmpCommandOptions options,
+  unsigned special_features)
 {
 QmpCommand *cmd = g_malloc0(sizeof(*cmd));
 
@@ -27,6 +28,7 @@ void qmp_register_command(QmpCommandList *cmds, const char 
*name,
 cmd->fn = fn;
 cmd->enabled = true;
 cmd->options = options;
+cmd->special_features = special_features;
 QTAILQ_INSERT_TAIL(cmds, cmd, node);
 }
 
diff --git a/storage-daemon/qemu-storage-daemon.c 
b/storage-daemon/qemu-storage-daemon.c
index 10a1a33761..52cf17e8ac 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -146,7 +146,8 @@ static void init_qmp_commands(void)
 
 QTAILQ_INIT(_cap_negotiation_commands);
 qmp_register_command(_cap_negotiation_commands, "qmp_capabilities",
- qmp_marshal_qmp_capabilities, QCO_ALLOW_PRECONFIG);
+ qmp_marshal_qmp_capabilities,
+ QCO_ALLOW_PRECONFIG, 0);
 }
 
 static int getopt_set_loc(int argc, char **argv, const char *optstring,
diff --git a/scripts/qapi/commands.py

[PATCH v2 1/9] qapi: New special feature flag "unstable"

2021-10-28 Thread Markus Armbruster 
By convention, names starting with "x-" are experimental.  The parts
of external interfaces so named may be withdrawn or changed
incompatibly in future releases.

The naming convention makes unstable interfaces easy to recognize.
Promoting something from experimental to stable involves a name
change.  Client code needs to be updated.  Occasionally bothersome.

Worse, the convention is not universally observed:

* QOM type "input-barrier" has properties "x-origin", "y-origin".
  Looks accidental, but it's ABI since 4.2.

* QOM types "memory-backend-file", "memory-backend-memfd",
  "memory-backend-ram", and "memory-backend-epc" have a property
  "x-use-canonical-path-for-ramblock-id" that is documented to be
  stable despite its name.

We could document these exceptions, but documentation helps only
humans.  We want to recognize "unstable" in code, like "deprecated".

So support recognizing it the same way: introduce new special feature
flag "unstable".  It will be treated specially by the QAPI generator,
like the existing feature flag "deprecated", and unlike regular
feature flags.

This commit updates documentation and prepares tests.  The next commit
updates the QAPI schema.  The remaining patches update the QAPI
generator and wire up -compat policy checking.

Management applications can then use query-qmp-schema and -compat to
manage or guard against use of unstable interfaces the same way as for
deprecated interfaces.

docs/devel/qapi-code-gen.txt no longer mandates the naming convention.
Using it anyway might help writers of programs that aren't
full-fledged management applications.  Not using it can save us
bothersome renames.  We'll see how that shakes out.

Signed-off-by: Markus Armbruster 
Reviewed-by: Juan Quintela 
Reviewed-by: John Snow 
---
 docs/devel/qapi-code-gen.rst| 9 ++---
 tests/qapi-schema/qapi-schema-test.json | 7 +--
 tests/qapi-schema/qapi-schema-test.out  | 5 +
 3 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
index 4071c9074a..38f2d7aad3 100644
--- a/docs/devel/qapi-code-gen.rst
+++ b/docs/devel/qapi-code-gen.rst
@@ -713,6 +713,10 @@ member as deprecated.  It is not supported elsewhere so 
far.
 Interfaces so marked may be withdrawn in future releases in accordance
 with QEMU's deprecation policy.
 
+Feature "unstable" marks a command, event, enum value, or struct
+member as unstable.  It is not supported elsewhere so far.  Interfaces
+so marked may be withdrawn or changed incompatibly in future releases.
+
 
 Naming rules and reserved names
 ---
@@ -746,9 +750,8 @@ Member name ``u`` and names starting with ``has-`` or 
``has_`` are reserved
 for the generator, which uses them for unions and for tracking
 optional members.
 
-Any name (command, event, type, member, or enum value) beginning with
-``x-`` is marked experimental, and may be withdrawn or changed
-incompatibly in a future release.
+Names beginning with ``x-`` used to signify "experimental".  This
+convention has been replaced by special feature "unstable".
 
 Pragmas ``command-name-exceptions`` and ``member-name-exceptions`` let
 you violate naming rules.  Use for new code is strongly discouraged. See
diff --git a/tests/qapi-schema/qapi-schema-test.json 
b/tests/qapi-schema/qapi-schema-test.json
index b677ab861d..43b8697002 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -273,7 +273,7 @@
   'data': { 'foo': { 'type': 'int', 'features': [ 'deprecated' ] } },
   'features': [ 'feature1' ] }
 { 'struct': 'FeatureStruct2',
-  'data': { 'foo': 'int' },
+  'data': { 'foo': { 'type': 'int', 'features': [ 'unstable' ] } },
   'features': [ { 'name': 'feature1' } ] }
 { 'struct': 'FeatureStruct3',
   'data': { 'foo': 'int' },
@@ -331,7 +331,7 @@
 { 'command': 'test-command-features1',
   'features': [ 'deprecated' ] }
 { 'command': 'test-command-features3',
-  'features': [ 'feature1', 'feature2' ] }
+  'features': [ 'unstable', 'feature1', 'feature2' ] }
 
 { 'command': 'test-command-cond-features1',
   'features': [ { 'name': 'feature1', 'if': 'TEST_IF_FEATURE_1'} ] }
@@ -348,3 +348,6 @@
 
 { 'event': 'TEST_EVENT_FEATURES1',
   'features': [ 'deprecated' ] }
+
+{ 'event': 'TEST_EVENT_FEATURES2',
+  'features': [ 'unstable' ] }
diff --git a/tests/qapi-schema/qapi-schema-test.out 
b/tests/qapi-schema/qapi-schema-test.out
index 16846dbeb8..1f9585fa9b 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -308,6 +308,7 @@ object FeatureStruct1
 feature feature1
 object FeatureStruct2
 member foo: int optional=False
+feature unstable
 feature feature1
 object FeatureStruct3
 member foo: int optional=False
@@ -373,6 +374,7 @@ command test-command-features1 None -> None
 feature deprecated
 command test-command-features3 None -> None
 gen=True success_response=True boxed=False

[PATCH v2 7/9] qapi: Generalize enum member policy checking

2021-10-28 Thread Markus Armbruster 
The code to check enumeration value policy can see special feature
flag 'deprecated' in QEnumLookup member flags[value].  I want to make
feature flag 'unstable' visible there as well, so I can add policy for
it.

Instead of extending flags[], replace it by @special_features (a
bitset of QapiSpecialFeature), because that's how special features get
passed around elsewhere.

Signed-off-by: Markus Armbruster 
Acked-by: John Snow 
---
 include/qapi/util.h|  5 +
 qapi/qapi-visit-core.c |  3 ++-
 scripts/qapi/types.py  | 22 --
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/include/qapi/util.h b/include/qapi/util.h
index 7a8d5c7d72..0cc98db9f9 100644
--- a/include/qapi/util.h
+++ b/include/qapi/util.h
@@ -15,12 +15,9 @@ typedef enum {
 QAPI_DEPRECATED,
 } QapiSpecialFeature;
 
-/* QEnumLookup flags */
-#define QAPI_ENUM_DEPRECATED 1
-
 typedef struct QEnumLookup {
 const char *const *array;
-const unsigned char *const flags;
+const unsigned char *const special_features;
 const int size;
 } QEnumLookup;
 
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index f95503cbec..34c59286b2 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -408,7 +408,8 @@ static bool input_type_enum(Visitor *v, const char *name, 
int *obj,
 return false;
 }
 
-if (lookup->flags && (lookup->flags[value] & QAPI_ENUM_DEPRECATED)) {
+if (lookup->special_features
+&& (lookup->special_features[value] & QAPI_DEPRECATED)) {
 switch (v->compat_policy.deprecated_input) {
 case COMPAT_POLICY_INPUT_ACCEPT:
 break;
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index ab2441adc9..3013329c24 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -16,7 +16,7 @@
 from typing import List, Optional
 
 from .common import c_enum_const, c_name, mcgen
-from .gen import QAPISchemaModularCVisitor, ifcontext
+from .gen import QAPISchemaModularCVisitor, gen_special_features, ifcontext
 from .schema import (
 QAPISchema,
 QAPISchemaEnumMember,
@@ -39,7 +39,7 @@ def gen_enum_lookup(name: str,
 members: List[QAPISchemaEnumMember],
 prefix: Optional[str] = None) -> str:
 max_index = c_enum_const(name, '_MAX', prefix)
-flags = ''
+feats = ''
 ret = mcgen('''
 
 const QEnumLookup %(c_name)s_lookup = {
@@ -54,19 +54,21 @@ def gen_enum_lookup(name: str,
 ''',
  index=index, name=memb.name)
 ret += memb.ifcond.gen_endif()
-if 'deprecated' in (f.name for f in memb.features):
-flags += mcgen('''
-[%(index)s] = QAPI_ENUM_DEPRECATED,
-''',
-   index=index)
 
-if flags:
+special_features = gen_special_features(memb.features)
+if special_features != '0':
+feats += mcgen('''
+[%(index)s] = %(special_features)s,
+''',
+   index=index, special_features=special_features)
+
+if feats:
 ret += mcgen('''
 },
-.flags = (const unsigned char[%(max_index)s]) {
+.special_features = (const unsigned char[%(max_index)s]) {
 ''',
  max_index=max_index)
-ret += flags
+ret += feats
 
 ret += mcgen('''
 },
-- 
2.31.1

[PATCH v2 3/9] qapi: Eliminate QCO_NO_OPTIONS for a slight simplification

2021-10-28 Thread Markus Armbruster 
Signed-off-by: Markus Armbruster 
Reviewed-by: Juan Quintela 
Reviewed-by: John Snow 
---
 include/qapi/qmp/dispatch.h | 1 -
 monitor/misc.c  | 3 +--
 scripts/qapi/commands.py| 5 +
 3 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 075203dc67..0ce88200b9 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -21,7 +21,6 @@ typedef void (QmpCommandFunc)(QDict *, QObject **, Error **);
 
 typedef enum QmpCommandOptions
 {
-QCO_NO_OPTIONS=  0x0,
 QCO_NO_SUCCESS_RESP   =  (1U << 0),
 QCO_ALLOW_OOB =  (1U << 1),
 QCO_ALLOW_PRECONFIG   =  (1U << 2),
diff --git a/monitor/misc.c b/monitor/misc.c
index ffe7966870..3556b177f6 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -230,8 +230,7 @@ static void monitor_init_qmp_commands(void)
 
 qmp_init_marshal(_commands);
 
-qmp_register_command(_commands, "device_add", qmp_device_add,
- QCO_NO_OPTIONS);
+qmp_register_command(_commands, "device_add", qmp_device_add, 0);
 
 QTAILQ_INIT(_cap_negotiation_commands);
 qmp_register_command(_cap_negotiation_commands, "qmp_capabilities",
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 3654825968..c8a975528f 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -229,15 +229,12 @@ def gen_register_command(name: str,
 if coroutine:
 options += ['QCO_COROUTINE']
 
-if not options:
-options = ['QCO_NO_OPTIONS']
-
 ret = mcgen('''
 qmp_register_command(cmds, "%(name)s",
  qmp_marshal_%(c_name)s, %(opts)s);
 ''',
 name=name, c_name=c_name(name),
-opts=" | ".join(options))
+opts=' | '.join(options) or 0)
 return ret
 
 
-- 
2.31.1

Re: [PATCH 1/9] qapi: New special feature flag "unstable"

2021-10-28 Thread Markus Armbruster 
Kevin Wolf  writes:

> Am 26.10.2021 um 11:37 hat Markus Armbruster geschrieben:
>> Kevin Wolf  writes:
>> 
>> > Am 25.10.2021 um 07:25 hat Markus Armbruster geschrieben:
>> >> By convention, names starting with "x-" are experimental.  The parts
>> >> of external interfaces so named may be withdrawn or changed
>> >> incompatibly in future releases.
>> >> 
>> >> Drawback: promoting something from experimental to stable involves a
>> >> name change.  Client code needs to be updated.
>> >> 
>> >> Moreover, the convention is not universally observed:
>> >> 
>> >> * QOM type "input-barrier" has properties "x-origin", "y-origin".
>> >>   Looks accidental, but it's ABI since 4.2.
>> >> 
>> >> * QOM types "memory-backend-file", "memory-backend-memfd",
>> >>   "memory-backend-ram", and "memory-backend-epc" have a property
>> >>   "x-use-canonical-path-for-ramblock-id" that is documented to be
>> >>   stable despite its name.
>> >> 
>> >> We could document these exceptions, but documentation helps only
>> >> humans.  We want to recognize "unstable" in code, like "deprecated".
>> >> 
>> >> Replace the convention by a new special feature flag "unstable".  It
>> >> will be recognized by the QAPI generator, like the existing feature
>> >> flag "deprecated", and unlike regular feature flags.
>> >> 
>> >> This commit updates documentation and prepares tests.  The next commit
>> >> updates the QAPI schema.  The remaining patches update the QAPI
>> >> generator and wire up -compat policy checking.
>> >> 
>> >> Signed-off-by: Markus Armbruster 
>> >
>> > Obviously, replacing the old convention gets rid of the old drawbacks,
>> > but adds a new one: While using x- makes it very obvious for a human
>> > user that this is an unstable feature, a feature flag in the schema will
>> > almost certainly go unnoticed in manual use.
>> 
>> I thought about this, but neglected to put it in writing.  My bad.
>> 
>> Manual use of unstable interfaces is mostly fine.  Human users can adapt
>> to changing interfaces.  HMP works that way.
>> 
>> Management applications are better off with a feature flag than with a
>> naming convention we sometimes ignore.
>> 
>> The most potential for trouble is in between: programs that aren't
>> full-fledged management applications.
>> 
>> If we want to keep "unstable" obvious to the humans who write such
>> programs, we can continue to require "x-", in addition to the feature
>> flag.  We pay for it with renames, and the risk of forgetting to rename
>> in time (which is what got us the awkward stable
>> "x-use-canonical-path-for-ramblock-id").  Tradeoff.  I chose not to, but
>> if y'all think we should...
>
> Just to clarify, I'm not implying that we should keep it. I'm merely
> pointing out that there is a tradeoff that requires us to make a choice.
> The decision for one of the options should be explicit rather than just
> happening as a side effect. Documenting that it was a conscious decision
> is probably best done by adding the reasoning for it to the commit
> message.

I rewrote the commit message for v2.

Thanks!

[...]

答复: [PATCH v3 0/4] Add qemu support setting qos via ovs on ovs interface

2021-10-28 Thread 张金生 
Hi Yalan,
It seems that there is no output error abount inbound settings from your 
statistics. 100KB is short for 100 kilobytes, and 1 byte is 8 bit, therefore 
100 kilobytes is 800 kilobit and is also 1024*800 bit which is 819200 bit or 
800 Kbit for short. Similarly, 200 KB is equal to 1600Kbit.

From your test results, inbound.average is set to 400 KB which is 400 * 1024 * 
8 bit(approximately 3.2*10^6 bits). outbound.average is set to 100 KB which is 
approximately 0.8*10^6 bits. Considering peek and burst is larger than average. 
The netperf test result is meaningful.

For the second bug mentioned, after create the ovs-net, tc rules are created. 
But when attach an interface to an instance, qos settings is not add to port 
neither in xml or tc . It is a bug, I think. I will think about fixing this.

---
Best Regards,
Jinsheng Zhang
发件人: Yalan Zhang [mailto:yalzh...@redhat.com]
发送时间: 2021年10月27日 18:35
收件人: Jinsheng Zhang (张金生)-云服务集团
抄送: libvir-list@redhat.com; Norman Shen(申嘉童)
主题: Re: [PATCH v3 0/4] Add qemu support setting qos via ovs on ovs interface


Hi Jinsheng,

Thank you for the explanation. From the statistics above, the tc outputs for 
outbound matches. But I'm confused about the inbound statistics:

# virsh domiftune rhel vnet5

inbound. approximately 3.2*10^6 bits: 100

inbound.peak   : 200

inbound.burst  : 256

...

#  tc -d class show  dev vnet5

class htb 1:1 parent 1:fffe prio 0 quantum 10240 rate 819200bit ceil 1638Kbit 
linklayer ethernet burst 256Kb/1 mpu 0b cburst 256Kb/1 mpu 0b level 0

class htb 1:fffe root rate 1638Kbit ceil 1638Kbit linklayer ethernet burst 
1499b/1 mpu 0b cburst 1499b/1 mpu 0b level 7



As the value in libvirt xml is KB, inbound.average: *100 KB* can not match with 
*"rate 819200bit"* in tc outputs, I supposed it should be 800Kbit. Please help 
to confirm.

And so does "ceil 1638Kbit" (may be it should be 1600Kbit as "inbound.peak   : 
200").



I have run netperf to test the actual rate, the result is pass. 2 vm connected 
to the same bridge, set one vm with Qos, see test results below:

# virsh domiftune rhel vnet0
inbound.average: 400
inbound.peak   : 500
inbound.burst  : 125
inbound.floor  : 0
outbound.average: 100
outbound.peak  : 200
outbound.burst : 256

Throughput for inbound:  3.92 * 10^6bits/sec

Throughput for outbound:  0.93 * 10^6bits/sec

These patches fixed the bug [1] which closed with deferred resolution. Thank 
you!
And this reminds me of another ovs Qos related bug [2], which was about network.
And I tried with the scenarios in [2], there are no changes(not fixed). Just 
for information. :-)

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1510237
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1826168


---
Best Regards,
Yalan Zhang
IRC: yalzhang


On Tue, Oct 26, 2021 at 3:23 PM Jinsheng Zhang (张金生)-云服务集团 
mailto:zhangj...@inspur.com>> wrote:
Hi Yalan,


1)   For inbound, we can use `ovs-vsctl list qos` and `ovs-vsctl list 
queue`  to check them from the openvswitch side. Values can be found in 
other_config.  Inbound is in kbyte when set qos with `virshdomiftune …`, 
well it is in bit in ovs, Therefore, when inbound.average is set to 100, the 
corresponding value will be  set to 819200 in ovs.

2)   For outbound, it is in kbyte in libvirt and ingress_policing_XX in ovs 
interface is in kbit.

3)   Ovs use tc to set qos, so we can see output from tc command.
This patch is to unify the qos control and query on ovs ports.
The conversion explanation is added in this patch: 
https://listman.redhat.com/archives/libvir-list/2021-August/msg00422.html
And there are 6 following patches to fix some bugs. See 
https://listman.redhat.com/archives/libvir-list/2021-August/msg00423.html

---
Best Regards,
Jinsheng Zhang

发件人: Yalan Zhang [mailto:yalzh...@redhat.com]
发送时间: 2021年10月25日 17:54
收件人: Michal Prívozník; Jinsheng Zhang (张金生)-云服务集团
抄送: libvir-list@redhat.com; Norman Shen(申嘉童); 
zhangjl02
主题: Re: [PATCH v3 0/4] Add qemu support setting qos via ovs on ovs interface

Hi Jinsheng,

I have tested the patch and have some questions, could you please help to 
confirm?
1) For inbound, how to check it from the openvswitch side? tc will still show 
the statistics, is that expected?
2) For outbound, the peak is ignored. I just can not understand the 
"ingress_policing_burst: 2048", how can it come from the setting 
"outbound.burst : 256"?
3) Is the output from tc command expected?

Test inbound:
1. start vm with setting as below:
 
  
  





  

...


2.

# virsh domiftune rhel vnet5

inbound.average: 100

inbound.peak   : 200

inbound.burst  : 256

inbound.floor  : 0

outbound.average: 0

outbound.peak  : 0

outbound.burst : 0

# ip l

17: vnet5:  mtu 1500 qdisc htb master 
ovs-system state UNKNOWN mode DEFAULT group default qlen 1000

link/ether fe:54:00:4d:43:5a brd ff:ff:ff:ff:ff:ff

# ovs-vsctl show interface

…...

ingress_policing_burst: 0

[PATCH 4/4] news: Add support for librbd encryption

2021-10-28 Thread Han Han 
Signed-off-by: Han Han 
---
 NEWS.rst | 5 +
 1 file changed, 5 insertions(+)

diff --git a/NEWS.rst b/NEWS.rst
index 177e3dfd8f..6401713fa6 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -64,6 +64,11 @@ v7.9.0 (unreleased)
 notification capabilities exposed to the guest. It is recommended for the
 vDPA devices.
 
+  * qemu: Support librbd encryption
+
+Add an encryption engine ``librbd``. It will provides the image-level
+encryption of librbd. It requires QEMU >= 6.1.0 and librbd >= 16.1.0.
+
 * **Improvements**
 
   * Use of JSON syntax with ``-device`` with upcoming QEMU-6.2
-- 
2.33.1

[PATCH 3/4] docs: Make the version requirement more clear for rbd encryption

2021-10-28 Thread Han Han 
Signed-off-by: Han Han 
---
 docs/formatstorageencryption.html.in | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/docs/formatstorageencryption.html.in 
b/docs/formatstorageencryption.html.in
index 86d884f93d..66bf95ab0b 100644
--- a/docs/formatstorageencryption.html.in
+++ b/docs/formatstorageencryption.html.in
@@ -30,8 +30,9 @@
   qemu and librbd.
   Both qemu and librbd require using the qemu
   driver.
-  The librbd engine requires qemu version >= 6.1.0,
-  and is only applicable for RBD network disks.
+  The librbd engine requires qemu version >= 6.1.0, both
+  ceph cluster and librbd1 >= 16.1.0, and is only applicable for RBD
+  network disks.
   If the engine tag is not specified, the qemu engine will be
   used by default (assuming the qemu driver is used).
   Note that librbd engine is currently only supported by the
-- 
2.33.1

[PATCH 2/4] news: News for the new virtio attribute page_per_vq

2021-10-28 Thread Han Han 
Signed-off-by: Han Han 
---
 NEWS.rst | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/NEWS.rst b/NEWS.rst
index 5a7570d0db..177e3dfd8f 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -58,6 +58,12 @@ v7.9.0 (unreleased)
 called "vpd" is introduced holding string resources and keyword resources
 found in PCI VPD.
 
+  * qemu: Support page_per_vq for driver element
+
+This optional virtio attribute ``page_per_vq`` controls the layout of the
+notification capabilities exposed to the guest. It is recommended for the
+vDPA devices.
+
 * **Improvements**
 
   * Use of JSON syntax with ``-device`` with upcoming QEMU-6.2
-- 
2.33.1

[PATCH 0/4] Some news and docs update for v7.9.0

2021-10-28 Thread Han Han 


Han Han (4):
  docs: Fix a typo of page_per_vq
  news: News for the new virtio attribute page_per_vq
  docs: Make the version requirement more clear for rbd encryption
  news: Add support for librbd encryption

 NEWS.rst | 11 +++
 docs/formatdomain.rst|  2 +-
 docs/formatstorageencryption.html.in |  5 +++--
 3 files changed, 15 insertions(+), 3 deletions(-)

-- 
2.33.1

[PATCH 1/4] docs: Fix a typo of page_per_vq

2021-10-28 Thread Han Han 
The page_per_vq is added since v7.9.0 not v7.8.0.

Signed-off-by: Han Han 
---
 docs/formatdomain.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 58768f7e5e..b500d35f5b 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -3553,7 +3553,7 @@ capabilities exposed to the guest. When enabled, each 
virtio queue will have a
 dedicated page on the device BAR exposed to the guest. It is recommended to be
 used when vDPA is enabled on the hypervisor, as it enables mapping the
 notification area to the physical device, which is only supported in page
-granularity. The default is determined by QEMU. :since:`Since 7.8.0 (QEMU 2.8)`
+granularity. The default is determined by QEMU. :since:`Since 7.9.0 (QEMU 2.8)`
 Note: In general you should leave this option alone, unless you are very 
certain
 you know what you are doing.
 
-- 
2.33.1

RE: [PATCH v5 0/5] Add support for librbd encryption

2021-10-28 Thread Or Ozeri 
The first Ceph version to support RBD encryption is 16.1.0.Smaller versions will cause qemu (>=6.1.0) to return -ENOTSUP, "RBD library does not support image encryption".Also, this only works on linux machines (e.g. will not work on BSD/windows).-"Han Han"  wrote: ->To: "Or Ozeri" >From: "Han Han" >Date: 10/28/2021 05:58AM>Cc: libvir-list@redhat.com, idryo...@gmail.com,>to.my.troc...@gmail.com, dan...@il.ibm.com>Subject: [EXTERNAL] Re: [PATCH v5 0/5] Add support for librbd>encryption>>Hi Or, I have a question about this feature. For>rbd encryption in ceph, is it introduced from ceph-v16.2.0? Does it>require the ceph cluster side >= this version? On Sun, Oct 24, 2021>at 5:54 PM Or Ozeri  wrote: ‍ ‍>> >Hi Or,>I have a question about this feature. For rbd encryption in ceph, is>it introduced from ceph-v16.2.0?>Does it require the ceph cluster side >= this version?>>On Sun, Oct 24, 2021 at 5:54 PM Or Ozeri  wrote:>v5: rebased + nit fixes suggested by Peter> v4:>  - added disk post parse to image creation flow in qemublocktest>(since more tests failed after adding engine validation)>  - removed symlink changes>  - added luks2 and engine documentation>  - switched to using enum engine instead of int>  - added validation for encryption engine and formats> v3: rebased on master> v2: addressed (hopefully) all of Peter's v1 comments (thanks Peter!)> > Feel free to make any other changes before pushing. Thanks!> > Or Ozeri (5):>   qemu: add disk post parse to qemublocktest>   qemu: capablities: Detect presence of 'rbd-encryption' as>     QEMU_CAPS_RBD_ENCRYPTION>   conf: add encryption engine property>   qemu: add librbd encryption engine>   conf: add luks2 encryption format> >  docs/formatstorageencryption.html.in          | 29 ++->  docs/schemas/domainbackup.rng                 |  7 ++>  docs/schemas/storagecommon.rng                |  9 ++>  src/conf/storage_encryption_conf.c            | 28 ++->  src/conf/storage_encryption_conf.h            | 11 +++>  src/qemu/qemu_block.c                         | 41 +>  src/qemu/qemu_capabilities.c                  |  2 +>  src/qemu/qemu_capabilities.h                  |  1 +>  src/qemu/qemu_domain.c                        | 69 ++->  src/qemu/qemu_domain.h                        |  3 +>  tests/qemublocktest.c                         | 29 +++>  .../caps_6.1.0.x86_64.xml                     |  1 +>  .../caps_6.2.0.x86_64.xml                     |  1 +>  tests/qemustatusxml2xmldata/upgrade-out.xml   |  6 +->  ...sk-network-rbd-encryption.x86_64-6.0.0.err |  1 +>  ...-network-rbd-encryption.x86_64-latest.args | 49 +++>  .../disk-network-rbd-encryption.xml           | 75>+>  tests/qemuxml2argvdata/disk-nvme.xml          |  2 +->  .../qemuxml2argvdata/encrypted-disk-usage.xml |  2 +->  tests/qemuxml2argvdata/luks-disks.xml         |  4 +->  tests/qemuxml2argvdata/user-aliases.xml       |  2 +->  tests/qemuxml2argvtest.c                      |  2 +>  ...k-network-rbd-encryption.x86_64-latest.xml | 83>+++>  .../disk-slices.x86_64-latest.xml             |  4 +->  tests/qemuxml2xmloutdata/encrypted-disk.xml   |  2 +->  .../luks-disks-source-qcow2.x86_64-latest.xml | 14 ++-->  .../qemuxml2xmloutdata/luks-disks-source.xml  | 10 +-->  tests/qemuxml2xmltest.c                       |  1 +>  28 files changed, 443 insertions(+), 45 deletions(-)>  create mode 100644>tests/qemuxml2argvdata/disk-network-rbd-encryption.x86_64-6.0.0.err>  create mode 100644>tests/qemuxml2argvdata/disk-network-rbd-encryption.x86_64-latest.args>  create mode 100644>tests/qemuxml2argvdata/disk-network-rbd-encryption.xml>  create mode 100644>tests/qemuxml2xmloutdata/disk-network-rbd-encryption.x86_64-latest.xm>l> > -- > 2.25.1> >