| Message ID | 20210224135255.253837-8-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi/qom: QAPIfy --object and object-add | expand |
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 <kwolf@redhat.com> > --- > 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: > +# > +# Properties for memory-backend-memfd objects. > +# > +# The @share boolean option is true by default with memfd. > +# > +# @hugetlb: if true, the file to be created resides in the hugetlbfs filesystem > +# (default: false) > +# > +# @hugetlbsize: the hugetlb page size on systems that support multiple hugetlb > +# page sizes (it must be a power of 2 value supported by the > +# system). 0 selects a default page size. This option is ignored > +# if @hugetlb is false. (default: 0) > +# > +# @seal: if true, create a sealed-file, which will block further resizing of > +# the memory (default: true) > +# > +# Since: 2.12 > +## > +{ 'struct': 'MemoryBackendMemfdProperties', > + 'base': 'MemoryBackendProperties', > + 'data': { '*hugetlb': 'bool', > + '*hugetlbsize': 'size', > + '*seal': 'bool' } } backends/hostmem-memfd.c makes 'hugetlb' and 'hugetlbsize' conditional on qemu_memfd_check(MFD_HUGETLB), and only registers the overal type based on qemu_memfd_check(MFD_ALLOW_SEALING). In turn, qemu_memfd_check returns false except for CONFIG_LINUX,... > + > ## > # @ObjectType: > # > @@ -287,7 +395,10 @@ > 'cryptodev-backend-builtin', > 'cryptodev-vhost-user', > 'dbus-vmstate', > - 'iothread' > + 'iothread', > + 'memory-backend-file', > + 'memory-backend-memfd', > + 'memory-backend-ram' > ] } > > ## > @@ -314,7 +425,10 @@ > 'cryptodev-backend-builtin': 'CryptodevBackendProperties', > 'cryptodev-vhost-user': 'CryptodevVhostUserProperties', > 'dbus-vmstate': 'DBusVMStateProperties', > - 'iothread': 'IothreadProperties' > + 'iothread': 'IothreadProperties', > + 'memory-backend-file': 'MemoryBackendFileProperties', > + 'memory-backend-memfd': 'MemoryBackendMemfdProperties', ...so I'm wondering if this branch should be: 'memory-backend-memfd', { 'type':'MemoryBackendMemfdProperties', 'if': 'defined(CONFIG_LINUX)' }, and whether we are risking problems by always having the 'hugetlb*' fields even when the runtime does not register them. > + 'memory-backend-ram': 'MemoryBackendProperties' > } } > > ## > Because of my questions on conditional compilation, I'm not comfortable with R-b yet.
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 <kwolf@redhat.com> > > --- > > 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) > > +# > > +# @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)' }, Good catch, will fix. > > + '*readonly': 'bool' } } > > + > > +## > > +# @MemoryBackendMemfdProperties: > > +# > > +# Properties for memory-backend-memfd objects. > > +# > > +# The @share boolean option is true by default with memfd. > > +# > > +# @hugetlb: if true, the file to be created resides in the hugetlbfs filesystem > > +# (default: false) > > +# > > +# @hugetlbsize: the hugetlb page size on systems that support multiple hugetlb > > +# page sizes (it must be a power of 2 value supported by the > > +# system). 0 selects a default page size. This option is ignored > > +# if @hugetlb is false. (default: 0) > > +# > > +# @seal: if true, create a sealed-file, which will block further resizing of > > +# the memory (default: true) > > +# > > +# Since: 2.12 > > +## > > +{ 'struct': 'MemoryBackendMemfdProperties', > > + 'base': 'MemoryBackendProperties', > > + 'data': { '*hugetlb': 'bool', > > + '*hugetlbsize': 'size', > > + '*seal': 'bool' } } > > backends/hostmem-memfd.c makes 'hugetlb' and 'hugetlbsize' conditional > on qemu_memfd_check(MFD_HUGETLB), and only registers the overal type > based on qemu_memfd_check(MFD_ALLOW_SEALING). In turn, qemu_memfd_check > returns false except for CONFIG_LINUX,... > > > + > > ## > > # @ObjectType: > > # > > @@ -287,7 +395,10 @@ > > 'cryptodev-backend-builtin', > > 'cryptodev-vhost-user', > > 'dbus-vmstate', > > - 'iothread' > > + 'iothread', > > + 'memory-backend-file', > > + 'memory-backend-memfd', > > + 'memory-backend-ram' > > ] } > > > > ## > > @@ -314,7 +425,10 @@ > > 'cryptodev-backend-builtin': 'CryptodevBackendProperties', > > 'cryptodev-vhost-user': 'CryptodevVhostUserProperties', > > 'dbus-vmstate': 'DBusVMStateProperties', > > - 'iothread': 'IothreadProperties' > > + 'iothread': 'IothreadProperties', > > + 'memory-backend-file': 'MemoryBackendFileProperties', > > + 'memory-backend-memfd': 'MemoryBackendMemfdProperties', > > ...so I'm wondering if this branch should be: > > 'memory-backend-memfd', { 'type':'MemoryBackendMemfdProperties', > 'if': 'defined(CONFIG_LINUX)' }, > > and whether we are risking problems by always having the 'hugetlb*' > fields even when the runtime does not register them. I don't think that's necessarily a problem. Later in the series we'll have some more object types in here that don't actually work: Some of them are target dependent, but the code generated from the schema is compiled only once. So if you configured multiple targets, you'll get all of them in the schema for all system emulators, even those that emulate a different target. I'm hesitant to change this one because it feels a bit indirect. It would be a much clearer case if we only compiled the source file for CONFIG_LINUX instead of deciding at runtime. *checks meson.build* Wait, scratch that... We already do that in addition, so you can get your 'if'. :-) And while I'm at it, cryptodev-vhost-user is conditional on CONFIG_VIRTIO_CRYPTO and CONFIG_VHOST_CRYPTO. The QAPI schema language doesn't have a way to split strings across multiple lines? Because I'll need more than 80 characters for this line then... Kevin
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: 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', + '*readonly': 'bool' } } + +## +# @MemoryBackendMemfdProperties: +# +# Properties for memory-backend-memfd objects. +# +# The @share boolean option is true by default with memfd. +# +# @hugetlb: if true, the file to be created resides in the hugetlbfs filesystem +# (default: false) +# +# @hugetlbsize: the hugetlb page size on systems that support multiple hugetlb +# page sizes (it must be a power of 2 value supported by the +# system). 0 selects a default page size. This option is ignored +# if @hugetlb is false. (default: 0) +# +# @seal: if true, create a sealed-file, which will block further resizing of +# the memory (default: true) +# +# Since: 2.12 +## +{ 'struct': 'MemoryBackendMemfdProperties', + 'base': 'MemoryBackendProperties', + 'data': { '*hugetlb': 'bool', + '*hugetlbsize': 'size', + '*seal': 'bool' } } + ## # @ObjectType: # @@ -287,7 +395,10 @@ 'cryptodev-backend-builtin', 'cryptodev-vhost-user', 'dbus-vmstate', - 'iothread' + 'iothread', + 'memory-backend-file', + 'memory-backend-memfd', + 'memory-backend-ram' ] } ## @@ -314,7 +425,10 @@ 'cryptodev-backend-builtin': 'CryptodevBackendProperties', 'cryptodev-vhost-user': 'CryptodevVhostUserProperties', 'dbus-vmstate': 'DBusVMStateProperties', - 'iothread': 'IothreadProperties' + 'iothread': 'IothreadProperties', + 'memory-backend-file': 'MemoryBackendFileProperties', + 'memory-backend-memfd': 'MemoryBackendMemfdProperties', + 'memory-backend-ram': 'MemoryBackendProperties' } } ##
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 <kwolf@redhat.com> --- qapi/common.json | 20 ++++++++ qapi/machine.json | 22 +-------- qapi/qom.json | 118 +++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 138 insertions(+), 22 deletions(-)