Re: [Qemu-devel] [PATCH RFC for-2.3 1/6] qga: introduce three guest memory block commands with stubs

[Michael Roth <mdroth@linux.vnet.ibm.com>]

Quoting zhanghailiang (2014-12-06 00:59:14)
> Introduce three new guest commands:
> guest-get-memory-blocks, guest-set-memory-blocks, guest-get-memory-block-size.
> 
> With these three commands, we can support online/offline guest's memory block
> (logical memory hotplug/unplug) as required from host.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  qga/commands-posix.c | 19 ++++++++++++
>  qga/commands-win32.c | 19 ++++++++++++
>  qga/qapi-schema.json | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 126 insertions(+)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index f6f3e3c..b0d6a5d 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1996,6 +1996,25 @@ GList *ga_command_blacklist_init(GList *blacklist)
>      return blacklist;
>  }
> 
> +GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
> +{
> +    error_set(errp, QERR_UNSUPPORTED);
> +    return NULL;
> +}
> +
> +int64_t qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks,
> +                                    Error **errp)
> +{
> +    error_set(errp, QERR_UNSUPPORTED);
> +    return -1;
> +}
> +
> +int64_t qmp_guest_get_memory_block_size(Error **errp)
> +{
> +    error_set(errp, QERR_UNSUPPORTED);
> +    return -1;
> +}
> +
>  /* register init/cleanup routines for stateful command groups */
>  void ga_command_state_init(GAState *s, GACommandState *cs)
>  {
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 3bcbeae..b53b679 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -446,6 +446,25 @@ int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
>      return -1;
>  }
> 
> +GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
> +{
> +    error_set(errp, QERR_UNSUPPORTED);
> +    return NULL;
> +}
> +
> +int64_t qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks,
> +                                    Error **errp)
> +{
> +    error_set(errp, QERR_UNSUPPORTED);
> +    return -1;
> +}
> +
> +int64_t qmp_guest_get_memory_block_size(Error **errp)
> +{
> +    error_set(errp, QERR_UNSUPPORTED);
> +    return -1;
> +}
> +
>  /* add unsupported commands to the blacklist */
>  GList *ga_command_blacklist_init(GList *blacklist)
>  {
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index 376e79f..4b81769 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -738,3 +738,91 @@
>  ##
>  { 'command': 'guest-get-fsinfo',
>    'returns': ['GuestFilesystemInfo'] }
> +
> +##
> +# @GuestMemoryBlock:
> +#
> +# @phys_index: Arbitrary guest-specific unique identifier of the MEMORY BLOCK.
> +#
> +# @online: Whether the MEMORY BLOCK is enabled in logically.

Not sure what was intended with "in logically". Logically enabled? Enabled in
guest maybe?

> +#
> +# @can-offline: #optional Whether offlining the MEMORY BLOCK  is possible. This member

This line and a few below go beyond 80-char limit

> +#               is always filled in by the guest agent when the structure is
> +#               returned, and always ignored on input (hence it can be omitted
> +#               then).
> +#
> +# Since: 2.3
> +##
> +{ 'type': 'GuestMemoryBlock',
> +  'data': {'phys_index': 'uint64',
> +           'online': 'bool',
> +           '*can-offline': 'bool'} }

'phys-index' would be the convention

> +
> +##
> +# @guest-get-memory-blocks:
> +#
> +# Retrieve the list of the guest's memory blocks.
> +#
> +# This is a read-only operation.
> +#
> +# Returns: The list of all memory blocks the guest knows about.
> +# Each memory block is put on the list exactly once, but their order
> +# is unspecified.
> +#
> +# Since: 2.3
> +##
> +{ 'command': 'guest-get-memory-blocks',
> +  'returns': ['GuestMemoryBlock'] }
> +
> +##
> +# @guest-set-memory-blocks:
> +#
> +# Attempt to reconfigure (currently: enable/disable) state of memory blocks
> +# inside the guest.
> +#
> +# The input list is processed node by node in order. In each node @phys_index
> +# is used to look up the guest MEMORY BLOCK, for which @online specifies the requested

> +# state. The set of distinct @phys_index's is only required to be a subset of
> +# the guest-supported identifiers. There's no restriction on list length or on
> +# repeating the same @phys_index (with possibly different @online field).
> +# Preferably the input list should describe a modified subset of
> +# @guest-get-memory-blocks' return value.
> +#
> +# Returns: The length of the initial sublist that has been successfully
> +#          processed. The guest agent maximizes this value. Possible cases:
> +#
> +#          0:                if the @mem-blks list was empty on input. Guest state
> +#                            has not been changed. Otherwise,
> +#
> +#          Error:            processing the first node of @mem-blks failed for the
> +#                            reason returned. Guest state has not been changed.
> +#                            Otherwise,
> +#
> +#          < length(@mem-blks): more than zero initial nodes have been processed,
> +#                            but not the entire @mem-blks list. Guest state has
> +#                            changed accordingly. To retrieve the error
> +#                            (assuming it persists), repeat the call with the
> +#                            successfully processed initial sublist removed.
> +#                            Otherwise,
> +#
> +#          length(@mem-blks):   call successful.

I know this is how we handle set-vcpus, but repeating set-memory calls to get
individual errors seems kind of painful in retrospect, and in this case, where
there multiple points in the call which may fail, I'm not sure how useful an
errno response would be. Perhaps we should return something like this instead:

{ 'enum': 'GuestMemoryBlockResponseType',
  'data': ['success', 'not-found', 'operation-not-supported', 'operation-failed', ...]}

{ 'type': 'GuestMemoryBlockResponse',
  'data': { 'response': 'GuestMemoryResponseType',
            '*error-code': 'int' }}

{ 'command': 'guest-set-memory-blocks',
  'data':    { 'mem-blks': ['GuestMemoryBlock'] },
  'returns': ['GuestMemoryBlockResponse'] }

Where there's 1 response entry for each entry in mem-blk parameter.
Alternatively, we could include the phys_index in the response entries,
but since multiple requests for a particular block are accepted I think
that would be more difficult to make sense of.

> +#
> +# Since: 2.3
> +##
> +{ 'command': 'guest-set-memory-blocks',
> +  'data':    {'mem-blks': ['GuestMemoryBlock'] },
> +  'returns': 'int' }
> +
> +##
> +# @guest-get-memory-block-size:
> +#
> +# Get the the size (in bytes) of a memory block in guest.
> +# It is the unit of Memory online/offline operation (also called Logical Memory Hotplug).
> +#
> +# Returns: memory block size in bytes.
> +#
> +# Since 2.3
> +##
> +{ 'command': 'guest-get-memory-block-size',
> +  'returns': 'int' }
> +
> -- 
> 1.7.12.4