diff mbox

[RFC,for-2.3,1/6] qga: introduce three guest memory block commands with stubs

Message ID 1417849159-6568-2-git-send-email-zhang.zhanghailiang@huawei.com
State New
Headers show

Commit Message

Zhanghailiang Dec. 6, 2014, 6:59 a.m. UTC
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(+)

Comments

Michael Roth Dec. 21, 2014, 8:10 p.m. UTC | #1
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
Zhanghailiang Dec. 22, 2014, 10:02 a.m. UTC | #2
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
>
>
> .
>
diff mbox

Patch

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.
+#
+# @can-offline: #optional Whether offlining the MEMORY BLOCK  is possible. This member
+#               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'} }
+
+##
+# @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.
+#
+# 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' }
+