Re: [PATCH v2 07/31] qapi/qom: Add ObjectOptions for memory-backend-*

2021-03-02 Thread Kevin Wolf
Am 26.02.2021 um 17:23 hat Eric Blake geschrieben:
> On 2/24/21 7:52 AM, Kevin Wolf wrote:
> > This adds a QAPI schema for the properties of the memory-backend-*
> > objects.
> > 
> > HostMemPolicy has to be moved to an include file that can be used by the
> > storage daemon, too, because ObjectOptions must be the same in all
> > binaries if we don't want to compile the whole code multiple times.
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >  qapi/common.json  |  20 
> >  qapi/machine.json |  22 +
> >  qapi/qom.json | 118 +-
> >  3 files changed, 138 insertions(+), 22 deletions(-)
> > 
> 
> > +++ b/qapi/qom.json
> 
> > +##
> > +# @MemoryBackendProperties:
> > +#
> > +# Properties for objects of classes derived from memory-backend.
> > +#
> > +# @merge: if true, mark the memory as mergeable (default depends on the 
> > machine
> > +# type)
> > +#
> > +# @dump: if true, include the memory in core dumps (default depends on the
> > +#machine type)
> 
> Interesting choice to flip the description text from its previous
> wording, but fine by me:
> object_class_property_set_description(oc, "dump",
> "Set to 'off' to exclude from core dump");

I feel that for booleans, describing what happens if they are false
often turns out a bit confusing with double negatives etc. But if you
think that in some cases, describing the negative is actually better,
I'm open for that.

> > +#
> > +# @host-nodes: the list of NUMA host nodes to bind the memory to
> > +#
> > +# @policy: the NUMA policy (default: 'default')
> > +#
> > +# @prealloc: if true, preallocate memory (default: false)
> 
> Not quite in the same order as
> backends/hostmem.c:host_memory_backend_class_init() (alphabetic here
> instead of matching the C code declaration order), but that doesn't
> impact QMP semantics, and I was able to match everything up in the end.
> 
> > +#
> > +# @prealloc-threads: number of CPU threads to use for prealloc (default: 1)
> > +#
> > +# @share: if false, the memory is private to QEMU; if true, it is shared
> > +# (default: false)
> > +#
> > +# @size: size of the memory region in bytes
> > +#
> > +# @x-use-canonical-path-for-ramblock-id: if true, the canoncial path is 
> > used
> > +#for ramblock-id. Disable this for 
> > 4.0
> > +#machine types or older to allow
> > +#migration with newer QEMU 
> > versions.
> > +#(default: false generally, but 
> > true
> > +#for machine types <= 4.0)
> 
> The comment in the C code mentions that in spite of the x- prefix, we
> have to treat this as a stable interface until 4.0 machines disappear.
> Do we need any of that sentiment in the documentation here?

"This option is considered stable despite the x- prefix."

Does this work or should I be more verbose? (The indentation makes me
want to keep it terse... :-))

> > +#
> > +# Since: 2.1
> > +##
> > +{ 'struct': 'MemoryBackendProperties',
> > +  'data': { '*dump': 'bool',
> > +'*host-nodes': ['uint16'],
> > +'*merge': 'bool',
> > +'*policy': 'HostMemPolicy',
> > +'*prealloc': 'bool',
> > +'*prealloc-threads': 'uint32',
> > +'*share': 'bool',
> > +'size': 'size',
> > +'*x-use-canonical-path-for-ramblock-id': 'bool' } }
> > +
> > +##
> > +# @MemoryBackendFileProperties:
> > +#
> > +# Properties for memory-backend-file objects.
> > +#
> > +# @align: the base address alignment when QEMU mmap(2) @mem-path. Some
> > +# backend store specified by @mem-path requires an alignment 
> > different
> 
> Grammar feels off.  Would it read better as
> 
> ...when QEMU mmap(2)s @mem-path.  Some backend stores specified by
> @mem-path require an...

This description is stolen from qemu-options.hx (I actually tried to
copy existing documentation whenever it seemed to explain things well),
but that's no reason not to improve it.

> > +# than the default one used by QEMU, e.g. the device DAX 
> > /dev/dax0.0
> > +# requires 2M alignment rather than 4K. In such cases, users can
> > +# specify the required alignment via this option.
> > +# 0 selects a default alignment (currently the page size). 
> > (default: 0)
> 
> Again, not in the same order as
> backends/hostmem-file.c:file_backend_class_init(), but it matches up.
> 
> > +#
> > +# @discard-data: if true, the file contents can be destroyed when QEMU 
> > exits,
> > +#to avoid unnecessarily flushing data to the backing file. 
> > Note
> > +#that ``discard-data`` is only an optimization, and QEMU 
> > might
> > +#not discard file contents if it aborts unexpectedly or is
> > +#terminated using SIGKILL. (default: false)
> > 

Re: [PATCH v2 07/31] qapi/qom: Add ObjectOptions for memory-backend-*

2021-02-26 Thread Eric Blake
On 2/24/21 7:52 AM, Kevin Wolf wrote:
> This adds a QAPI schema for the properties of the memory-backend-*
> objects.
> 
> HostMemPolicy has to be moved to an include file that can be used by the
> storage daemon, too, because ObjectOptions must be the same in all
> binaries if we don't want to compile the whole code multiple times.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  qapi/common.json  |  20 
>  qapi/machine.json |  22 +
>  qapi/qom.json | 118 +-
>  3 files changed, 138 insertions(+), 22 deletions(-)
> 

> +++ b/qapi/qom.json

> +##
> +# @MemoryBackendProperties:
> +#
> +# Properties for objects of classes derived from memory-backend.
> +#
> +# @merge: if true, mark the memory as mergeable (default depends on the 
> machine
> +# type)
> +#
> +# @dump: if true, include the memory in core dumps (default depends on the
> +#machine type)

Interesting choice to flip the description text from its previous
wording, but fine by me:
object_class_property_set_description(oc, "dump",
"Set to 'off' to exclude from core dump");

> +#
> +# @host-nodes: the list of NUMA host nodes to bind the memory to
> +#
> +# @policy: the NUMA policy (default: 'default')
> +#
> +# @prealloc: if true, preallocate memory (default: false)

Not quite in the same order as
backends/hostmem.c:host_memory_backend_class_init() (alphabetic here
instead of matching the C code declaration order), but that doesn't
impact QMP semantics, and I was able to match everything up in the end.

> +#
> +# @prealloc-threads: number of CPU threads to use for prealloc (default: 1)
> +#
> +# @share: if false, the memory is private to QEMU; if true, it is shared
> +# (default: false)
> +#
> +# @size: size of the memory region in bytes
> +#
> +# @x-use-canonical-path-for-ramblock-id: if true, the canoncial path is used
> +#for ramblock-id. Disable this for 
> 4.0
> +#machine types or older to allow
> +#migration with newer QEMU versions.
> +#(default: false generally, but true
> +#for machine types <= 4.0)

The comment in the C code mentions that in spite of the x- prefix, we
have to treat this as a stable interface until 4.0 machines disappear.
Do we need any of that sentiment in the documentation here?

> +#
> +# Since: 2.1
> +##
> +{ 'struct': 'MemoryBackendProperties',
> +  'data': { '*dump': 'bool',
> +'*host-nodes': ['uint16'],
> +'*merge': 'bool',
> +'*policy': 'HostMemPolicy',
> +'*prealloc': 'bool',
> +'*prealloc-threads': 'uint32',
> +'*share': 'bool',
> +'size': 'size',
> +'*x-use-canonical-path-for-ramblock-id': 'bool' } }
> +
> +##
> +# @MemoryBackendFileProperties:
> +#
> +# Properties for memory-backend-file objects.
> +#
> +# @align: the base address alignment when QEMU mmap(2) @mem-path. Some
> +# backend store specified by @mem-path requires an alignment 
> different

Grammar feels off.  Would it read better as

...when QEMU mmap(2)s @mem-path.  Some backend stores specified by
@mem-path require an...

> +# than the default one used by QEMU, e.g. the device DAX /dev/dax0.0
> +# requires 2M alignment rather than 4K. In such cases, users can
> +# specify the required alignment via this option.
> +# 0 selects a default alignment (currently the page size). (default: 
> 0)

Again, not in the same order as
backends/hostmem-file.c:file_backend_class_init(), but it matches up.

> +#
> +# @discard-data: if true, the file contents can be destroyed when QEMU exits,
> +#to avoid unnecessarily flushing data to the backing file. 
> Note
> +#that ``discard-data`` is only an optimization, and QEMU 
> might
> +#not discard file contents if it aborts unexpectedly or is
> +#terminated using SIGKILL. (default: false)
> +#
> +# @mem-path: the path to either a shared memory or huge page filesystem mount
> +#
> +# @pmem: specifies whether the backing file specified by @mem-path is in
> +#host persistent memory that can be accessed using the SNIA NVM
> +#programming model (e.g. Intel NVDIMM).
> +#
> +# @readonly: if true, the backing file is opened read-only; if false, it is
> +#opened read-write. (default: false)
> +#
> +# Since: 2.1
> +##
> +{ 'struct': 'MemoryBackendFileProperties',
> +  'base': 'MemoryBackendProperties',
> +  'data': { '*align': 'size',
> +'*discard-data': 'bool',
> +'mem-path': 'str',
> +'*pmem': 'bool',

To match the C code, this should be
 '*pmem': { 'type':'bool', 'if':'defined(CONFIG_LIBPMEM)' },

> +'*readonly': 'bool' } }
> +
> +##
> +# @MemoryBackendMemfdProperties:
> 

[PATCH v2 07/31] qapi/qom: Add ObjectOptions for memory-backend-*

2021-02-24 Thread Kevin Wolf
This adds a QAPI schema for the properties of the memory-backend-*
objects.

HostMemPolicy has to be moved to an include file that can be used by the
storage daemon, too, because ObjectOptions must be the same in all
binaries if we don't want to compile the whole code multiple times.

Signed-off-by: Kevin Wolf 
---
 qapi/common.json  |  20 
 qapi/machine.json |  22 +
 qapi/qom.json | 118 +-
 3 files changed, 138 insertions(+), 22 deletions(-)

diff --git a/qapi/common.json b/qapi/common.json
index 716712d4b3..2dad4fadc3 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -145,3 +145,23 @@
 ##
 { 'enum': 'PCIELinkWidth',
   'data': [ '1', '2', '4', '8', '12', '16', '32' ] }
+
+##
+# @HostMemPolicy:
+#
+# Host memory policy types
+#
+# @default: restore default policy, remove any nondefault policy
+#
+# @preferred: set the preferred host nodes for allocation
+#
+# @bind: a strict policy that restricts memory allocation to the
+#host nodes specified
+#
+# @interleave: memory allocations are interleaved across the set
+#  of host nodes specified
+#
+# Since: 2.1
+##
+{ 'enum': 'HostMemPolicy',
+  'data': [ 'default', 'preferred', 'bind', 'interleave' ] }
diff --git a/qapi/machine.json b/qapi/machine.json
index 330189efe3..4322aee782 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -8,6 +8,8 @@
 # = Machines
 ##
 
+{ 'include': 'common.json' }
+
 ##
 # @SysEmuTarget:
 #
@@ -897,26 +899,6 @@
'policy': 'HmatCacheWritePolicy',
'line': 'uint16' }}
 
-##
-# @HostMemPolicy:
-#
-# Host memory policy types
-#
-# @default: restore default policy, remove any nondefault policy
-#
-# @preferred: set the preferred host nodes for allocation
-#
-# @bind: a strict policy that restricts memory allocation to the
-#host nodes specified
-#
-# @interleave: memory allocations are interleaved across the set
-#  of host nodes specified
-#
-# Since: 2.1
-##
-{ 'enum': 'HostMemPolicy',
-  'data': [ 'default', 'preferred', 'bind', 'interleave' ] }
-
 ##
 # @memsave:
 #
diff --git a/qapi/qom.json b/qapi/qom.json
index a6a5049707..1a869006a1 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -5,6 +5,7 @@
 # See the COPYING file in the top-level directory.
 
 { 'include': 'authz.json' }
+{ 'include': 'common.json' }
 
 ##
 # = QEMU Object Model (QOM)
@@ -272,6 +273,113 @@
 '*poll-grow': 'int',
 '*poll-shrink': 'int' } }
 
+##
+# @MemoryBackendProperties:
+#
+# Properties for objects of classes derived from memory-backend.
+#
+# @merge: if true, mark the memory as mergeable (default depends on the machine
+# type)
+#
+# @dump: if true, include the memory in core dumps (default depends on the
+#machine type)
+#
+# @host-nodes: the list of NUMA host nodes to bind the memory to
+#
+# @policy: the NUMA policy (default: 'default')
+#
+# @prealloc: if true, preallocate memory (default: false)
+#
+# @prealloc-threads: number of CPU threads to use for prealloc (default: 1)
+#
+# @share: if false, the memory is private to QEMU; if true, it is shared
+# (default: false)
+#
+# @size: size of the memory region in bytes
+#
+# @x-use-canonical-path-for-ramblock-id: if true, the canoncial path is used
+#for ramblock-id. Disable this for 4.0
+#machine types or older to allow
+#migration with newer QEMU versions.
+#(default: false generally, but true
+#for machine types <= 4.0)
+#
+# Since: 2.1
+##
+{ 'struct': 'MemoryBackendProperties',
+  'data': { '*dump': 'bool',
+'*host-nodes': ['uint16'],
+'*merge': 'bool',
+'*policy': 'HostMemPolicy',
+'*prealloc': 'bool',
+'*prealloc-threads': 'uint32',
+'*share': 'bool',
+'size': 'size',
+'*x-use-canonical-path-for-ramblock-id': 'bool' } }
+
+##
+# @MemoryBackendFileProperties:
+#
+# Properties for memory-backend-file objects.
+#
+# @align: the base address alignment when QEMU mmap(2) @mem-path. Some
+# backend store specified by @mem-path requires an alignment different
+# than the default one used by QEMU, e.g. the device DAX /dev/dax0.0
+# requires 2M alignment rather than 4K. In such cases, users can
+# specify the required alignment via this option.
+# 0 selects a default alignment (currently the page size). (default: 0)
+#
+# @discard-data: if true, the file contents can be destroyed when QEMU exits,
+#to avoid unnecessarily flushing data to the backing file. Note
+#that ``discard-data`` is only an optimization, and QEMU might
+#not discard file contents if it aborts unexpectedly or is
+#terminated using SIGKILL. (default: false)
+#
+# @mem-path: