From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56012) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y2zpT-0007Q6-Pv for qemu-devel@nongnu.org; Mon, 22 Dec 2014 05:03:12 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y2zpO-0004D0-3n for qemu-devel@nongnu.org; Mon, 22 Dec 2014 05:03:07 -0500 Received: from szxga03-in.huawei.com ([119.145.14.66]:60664) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y2zpN-0004CH-4r for qemu-devel@nongnu.org; Mon, 22 Dec 2014 05:03:02 -0500 Message-ID: <5497EC39.3080003@huawei.com> Date: Mon, 22 Dec 2014 18:02:33 +0800 From: zhanghailiang MIME-Version: 1.0 References: <1417849159-6568-1-git-send-email-zhang.zhanghailiang@huawei.com> <1417849159-6568-2-git-send-email-zhang.zhanghailiang@huawei.com> <20141221201056.15420.68865@loki> In-Reply-To: <20141221201056.15420.68865@loki> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH RFC for-2.3 1/6] qga: introduce three guest memory block commands with stubs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Roth , qemu-devel@nongnu.org Cc: hangaohuai@huawei.com, peter.huangpeng@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 >> --- >> 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 > > > . >