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

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org