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

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