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

[zhanghailiang]

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 <address@hidden>
> > ---
> >   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
>
>
> .