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

[zhanghailiang <zhang.zhanghailiang@huawei.com>]

On 2014/12/22 4:10, Michael Roth wrote:
> 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?
>

It should be in 'guest', will fix it in next version. Thanks.

>> +#
>> +# @can-offline: #optional Whether offlining the MEMORY BLOCK  is possible. This member
>
> This line and a few below go beyond 80-char limit
>

Will fix that.

>> +#               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
>

OK, that is a typo, fix it in next version.

>> +
>> +##
>> +# @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
>

OK.

>> +# 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

Yes, compared with set-vcpus, we may repeat more times for set-memory.

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

Hmm, maybe this is really useful, for memory logical hotplug, it is a little complex.
For some old kernel, it does not supporting this operation (online/offline by sysfs) at all.
Sometimes we may not find the corresponding memoryXXX directory because we do physical memory hot-remove
at the same time or a physical memory hot-add is still in fly.
We can choose different action (retry/report error, etc) according to the result.

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

We should give definite meaning to the different error types.

> { '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.
>

That is really a good idea, i will look into it, and try to implement it in
next version. ;)

Thanks,
zhanghailiang

>> +#
>> +# 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
>
>
> .
>