[Qemu-devel] [PATCH RFC for-2.3 0/6] qga: add three logical memory hotplug related commands

[Qemu-devel] [PATCH RFC for-2.3 0/6] qga: add three logical memory hotplug related commands

[zhanghailiang]

Hi,

This patch series add three guest commands about memory block:
guest-get-memory-blocks, guest-set-memory-blocks, guest-get-memory-block-size.

With these three commands, we can get information about guest's memory block
online/offline status and memory block size (unit of memory online/offline 
operation ). Also, we can change guest's memory block status (Logical memory
hotplug/unplug) from host.

zhanghailiang (6):
  qga: introduce three guest memory block commands with stubs
  qga: introduce three help functions for memory block functions
  qga: implement qmp_guest_get_memory_blocks() for Linux with sysfs
  qga: implement qmp_guest_set_memory_blocks() for Linux with sysfs
  qga: implement qmp_guest_get_memory_block_size() for Linux with sysfs
  qga: add memory block command that unsupported to blacklist

 qga/commands-posix.c | 266 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 qga/commands-win32.c |  21 ++++
 qga/qapi-schema.json |  88 +++++++++++++++++
 3 files changed, 374 insertions(+), 1 deletion(-)

-- 
1.7.12.4

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

[zhanghailiang]

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.
+#
+# @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' }
+
-- 
1.7.12.4

[Qemu-devel] [PATCH RFC for-2.3 2/6] qga: introduce three help functions for memory block functions

[zhanghailiang]

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 qga/commands-posix.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 130 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index b0d6a5d..8917dca 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1875,6 +1875,136 @@ int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
     return processed;
 }
 
+static void ga_read_sysfs_file(int dirfd, const char *pathname, char *buf,
+                               int size, Error **errp)
+{
+    int fd;
+    int res;
+
+    errno = 0;
+    fd = openat(dirfd, pathname, O_RDONLY);
+    if (fd == -1) {
+        error_setg_errno(errp, errno, "open sysfs file \"%s\"", pathname);
+        return;
+    }
+
+    res = pread(fd, buf, size, 0);
+    if (res == -1) {
+        error_setg_errno(errp, errno, "pread sysfs file \"%s\"", pathname);
+    } else if (res == 0) {
+        error_setg(errp, "pread sysfs file \"%s\": unexpected EOF", pathname);
+    }
+    res = close(fd);
+    g_assert(res == 0);
+}
+
+static void ga_write_sysfs_file(int dirfd, const char *pathname,
+                                const char *buf, int size, Error **errp)
+{
+    int fd;
+    int res;
+
+    errno = 0;
+    fd = openat(dirfd, pathname, O_WRONLY);
+    if (fd == -1) {
+        error_setg_errno(errp, errno, "open sysfs file \"%s\"", pathname);
+        return;
+    }
+
+    if (pwrite(fd, buf, size, 0) == -1) {
+        error_setg_errno(errp, errno, "pwrite sysfs file \"%s\"", pathname);
+    }
+
+    res = close(fd);
+    g_assert(res == 0);
+}
+
+/* Transfer online/offline status between @mem_blk and the guest system.
+ *
+ * On input either @errp or *@errp must be NULL.
+ *
+ * In system-to-@mem_blk direction, the following @mem_blk fields are accessed:
+ * - R: mem_blk->phys_index
+ * - W: mem_blk->online
+ * - W: mem_blk->can_offline
+ *
+ * In @mem_blk-to-system direction, the following @mem_blk fields are accessed:
+ * - R: mem_blk->phys_index
+ * - R: mem_blk->online
+ *-  R: mem_blk->can_offline
+ * Written members remain unmodified on error.
+ */
+static void transfer_memory_block(GuestMemoryBlock *mem_blk, bool sys2memblk,
+                                  Error **errp)
+{
+    char *dirpath;
+    int dirfd;
+    int res;
+    char *status;
+    Error *local_err = NULL;
+
+    dirpath = g_strdup_printf("/sys/devices/system/memory/memory%" PRId64 "/",
+                              mem_blk->phys_index);
+    dirfd = open(dirpath, O_RDONLY | O_DIRECTORY);
+    if (dirfd == -1) {
+        error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
+        g_free(dirpath);
+        return;
+    }
+    g_free(dirpath);
+
+    status = g_malloc0(10);
+    ga_read_sysfs_file(dirfd, "state", status, 10, &local_err);
+    if (local_err) {
+        /* treat with sysfs file that not exist in old kernel */
+        if (errno == ENOENT) {
+            error_free(local_err);
+            if (sys2memblk) {
+                mem_blk->online = true;
+                mem_blk->can_offline = false;
+            } else if (!mem_blk->online) {
+                error_setg(errp, "memory block #%" PRId64 " can't be "
+                           "offlined", mem_blk->phys_index);
+            }
+        } else {
+            error_propagate(errp, local_err);
+        }
+        return;
+    }
+
+    if (sys2memblk) {
+        char removable = '0';
+
+        mem_blk->online = (strncmp(status, "online", 6) == 0);
+
+        ga_read_sysfs_file(dirfd, "removable", &removable, 1, &local_err);
+        if (local_err) {
+            /* if no 'removable' file, it does't support offline mem blk */
+            if (errno == ENOENT) {
+                error_free(local_err);
+                mem_blk->can_offline = false;
+            } else {
+                error_propagate(errp, local_err);
+            }
+        } else {
+            mem_blk->can_offline = (removable != '0');
+        }
+    } else {
+        if (mem_blk->online != (strncmp(status, "online", 6) == 0)) {
+            char *new_state = mem_blk->online ? g_strdup("online") :
+                                                g_strdup("offline");
+
+            ga_write_sysfs_file(dirfd, "state", new_state, strlen(new_state),
+                                errp);
+            g_free(new_state);
+        } /* otherwise pretend successful re-(on|off)-lining */
+    }
+
+    g_free(status);
+    res = close(dirfd);
+    g_assert(res == 0);
+}
+
 #else /* defined(__linux__) */
 
 void qmp_guest_suspend_disk(Error **errp)
-- 
1.7.12.4

[Qemu-devel] [PATCH RFC for-2.3 3/6] qga: implement qmp_guest_get_memory_blocks() for Linux with sysfs

[zhanghailiang]

We can get guest's memory block information by using command
"guest-get-memory-blocks", the returned value contains a list of memory block
info, such as phys_index, online state, can-offline info.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 qga/commands-posix.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 60 insertions(+), 6 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 8917dca..d3f7d4f 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2005,6 +2005,60 @@ static void transfer_memory_block(GuestMemoryBlock *mem_blk, bool sys2memblk,
     g_assert(res == 0);
 }
 
+GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
+{
+    GuestMemoryBlockList *head, **link;
+    Error *local_err = NULL;
+    struct dirent *de;
+    DIR *dp;
+
+    head = NULL;
+    link = &head;
+
+    dp = opendir("/sys/devices/system/memory/");
+    if (!dp) {
+        error_setg_errno(errp, errno, "Can't open directory"
+                         "\"/sys/devices/system/memory/\"\n");
+        return NULL;
+    }
+
+    /* Note: the phys_index of memory block may be discontinuous,
+     * this is because a memblk is the unit of the Sparse Memory design, which
+     * allows discontinuous memory ranges (ex. NUMA), so here we should
+     * traverse the memory block directory.
+     */
+    while ((de = readdir(dp)) != NULL) {
+        GuestMemoryBlock *mem_blk;
+        GuestMemoryBlockList *entry;
+
+        if ((strncmp(de->d_name, "memory", 6) != 0) ||
+            !(de->d_type & DT_DIR)) {
+            continue;
+        }
+
+        mem_blk = g_malloc0(sizeof *mem_blk);
+        /* The d_name is "memoryXXX",  phys_index is block id, same as XXX */
+        mem_blk->phys_index = strtoul(&de->d_name[6], NULL, 10);
+        mem_blk->has_can_offline = true; /* lolspeak ftw */
+        transfer_memory_block(mem_blk, true, &local_err);
+
+        entry = g_malloc0(sizeof *entry);
+        entry->value = mem_blk;
+
+        *link = entry;
+        link = &entry->next;
+    }
+
+    if (local_err == NULL) {
+        /* there's no guest with zero memroy blocks */
+        g_assert(head != NULL);
+        return head;
+    }
+
+    qapi_free_GuestMemoryBlockList(head);
+    error_propagate(errp, local_err);
+    return NULL;
+}
 #else /* defined(__linux__) */
 
 void qmp_guest_suspend_disk(Error **errp)
@@ -2040,6 +2094,12 @@ 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;
+}
+
 #endif
 
 #if !defined(CONFIG_FSFREEZE)
@@ -2126,12 +2186,6 @@ 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)
 {
-- 
1.7.12.4

[Qemu-devel] [PATCH RFC for-2.3 4/6] qga: implement qmp_guest_set_memory_blocks() for Linux with sysfs

[zhanghailiang]

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 qga/commands-posix.c | 42 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 7 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index d3f7d4f..1010e86 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2059,6 +2059,34 @@ GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
     error_propagate(errp, local_err);
     return NULL;
 }
+
+int64_t qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks,
+                                    Error **errp)
+{
+    int64_t processed;
+    Error *local_err = NULL;
+
+    processed = 0;
+    while (mem_blks != NULL) {
+        transfer_memory_block(mem_blks->value, false, &local_err);
+        if (local_err) {
+            break;
+        }
+        ++processed;
+        mem_blks = mem_blks->next;
+    }
+
+    if (local_err) {
+        if (processed == 0) {
+            error_propagate(errp, local_err);
+        } else {
+            error_free(local_err);
+        }
+    }
+
+    return processed;
+}
+
 #else /* defined(__linux__) */
 
 void qmp_guest_suspend_disk(Error **errp)
@@ -2100,6 +2128,13 @@ GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
     return NULL;
 }
 
+int64_t qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks,
+                                    Error **errp)
+{
+    error_set(errp, QERR_UNSUPPORTED);
+    return -1;
+}
+
 #endif
 
 #if !defined(CONFIG_FSFREEZE)
@@ -2186,13 +2221,6 @@ GList *ga_command_blacklist_init(GList *blacklist)
     return blacklist;
 }
 
-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);
-- 
1.7.12.4

[Qemu-devel] [PATCH RFC for-2.3 5/6] qga: implement qmp_guest_get_memory_block_size() for Linux with sysfs

[zhanghailiang]

The size of a memory block is architecture dependent,
For example, power uses 16MiB, ia64 uses 1GiB, x86 uses 128M.
It represents the logical unit upon which memory online/offline operations are
to be performed.

This function will return the value to host user.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 qga/commands-posix.c | 43 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 6 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 1010e86..02c6b44 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2087,6 +2087,37 @@ int64_t qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks,
     return processed;
 }
 
+int64_t qmp_guest_get_memory_block_size(Error **errp)
+{
+    Error *local_err = NULL;
+    char *dirpath;
+    int dirfd;
+    char *buf;
+    int64_t block_size;
+
+    dirpath = g_strdup_printf("/sys/devices/system/memory/");
+    dirfd = open(dirpath, O_RDONLY | O_DIRECTORY);
+    if (dirfd == -1) {
+        error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
+        g_free(dirpath);
+        return -1;
+    }
+    g_free(dirpath);
+
+    buf = g_malloc0(20);
+    ga_read_sysfs_file(dirfd, "block_size_bytes", buf, 20, &local_err);
+    if (local_err) {
+        g_free(buf);
+        error_propagate(errp, local_err);
+        return -1;
+    }
+
+    block_size = strtol(buf, NULL, 16); /* the unit is bytes */
+    g_free(buf);
+
+    return block_size;
+}
+
 #else /* defined(__linux__) */
 
 void qmp_guest_suspend_disk(Error **errp)
@@ -2135,6 +2166,12 @@ int64_t qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks,
     return -1;
 }
 
+int64_t qmp_guest_get_memory_block_size(Error **errp)
+{
+    error_set(errp, QERR_UNSUPPORTED);
+    return -1;
+}
+
 #endif
 
 #if !defined(CONFIG_FSFREEZE)
@@ -2221,12 +2258,6 @@ GList *ga_command_blacklist_init(GList *blacklist)
     return blacklist;
 }
 
-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)
 {
-- 
1.7.12.4

[Qemu-devel] [PATCH RFC for-2.3 6/6] qga: add memory block command that unsupported to blacklist

[zhanghailiang]

For memory block command, we only support for linux with sysfs.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 qga/commands-posix.c | 4 +++-
 qga/commands-win32.c | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 02c6b44..b701108 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2228,7 +2228,9 @@ GList *ga_command_blacklist_init(GList *blacklist)
         const char *list[] = {
             "guest-suspend-disk", "guest-suspend-ram",
             "guest-suspend-hybrid", "guest-network-get-interfaces",
-            "guest-get-vcpus", "guest-set-vcpus", NULL};
+            "guest-get-vcpus", "guest-set-vcpus",
+            "guest-get-memory-blocks", "guest-set-memory-blocks",
+            "guest-get-memory-block-size", NULL};
         char **p = (char **)list;
 
         while (*p) {
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index b53b679..dd7031a 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -473,6 +473,8 @@ GList *ga_command_blacklist_init(GList *blacklist)
         "guest-file-write", "guest-file-seek", "guest-file-flush",
         "guest-suspend-hybrid", "guest-network-get-interfaces",
         "guest-get-vcpus", "guest-set-vcpus",
+        "guest-get-memory-blocks", "guest-set-memory-blocks",
+        "guest-get-memory-block-size",
         "guest-fsfreeze-freeze-list", "guest-get-fsinfo",
         "guest-fstrim", NULL};
     char **p = (char **)list_unsupported;
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH RFC for-2.3 0/6] qga: add three logical memory hotplug related commands

[zhanghailiang]

Ping... ;)

On 2014/12/6 14:59, zhanghailiang wrote:
> Hi,
>
> This patch series add three guest commands about memory block:
> guest-get-memory-blocks, guest-set-memory-blocks, guest-get-memory-block-size.
>
> With these three commands, we can get information about guest's memory block
> online/offline status and memory block size (unit of memory online/offline
> operation ). Also, we can change guest's memory block status (Logical memory
> hotplug/unplug) from host.
>
> zhanghailiang (6):
>    qga: introduce three guest memory block commands with stubs
>    qga: introduce three help functions for memory block functions
>    qga: implement qmp_guest_get_memory_blocks() for Linux with sysfs
>    qga: implement qmp_guest_set_memory_blocks() for Linux with sysfs
>    qga: implement qmp_guest_get_memory_block_size() for Linux with sysfs
>    qga: add memory block command that unsupported to blacklist
>
>   qga/commands-posix.c | 266 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>   qga/commands-win32.c |  21 ++++
>   qga/qapi-schema.json |  88 +++++++++++++++++
>   3 files changed, 374 insertions(+), 1 deletion(-)
>

Re: [Qemu-devel] [PATCH RFC for-2.3 0/6] qga: add three logical memory hotplug related commands

[zhanghailiang]

Ping again...

Any comments are warmly welcome! Thanks.

On 2014/12/6 14:59, zhanghailiang wrote:
> Hi,
>
> This patch series add three guest commands about memory block:
> guest-get-memory-blocks, guest-set-memory-blocks, guest-get-memory-block-size.
>
> With these three commands, we can get information about guest's memory block
> online/offline status and memory block size (unit of memory online/offline
> operation ). Also, we can change guest's memory block status (Logical memory
> hotplug/unplug) from host.
>
> zhanghailiang (6):
>    qga: introduce three guest memory block commands with stubs
>    qga: introduce three help functions for memory block functions
>    qga: implement qmp_guest_get_memory_blocks() for Linux with sysfs
>    qga: implement qmp_guest_set_memory_blocks() for Linux with sysfs
>    qga: implement qmp_guest_get_memory_block_size() for Linux with sysfs
>    qga: add memory block command that unsupported to blacklist
>
>   qga/commands-posix.c | 266 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>   qga/commands-win32.c |  21 ++++
>   qga/qapi-schema.json |  88 +++++++++++++++++
>   3 files changed, 374 insertions(+), 1 deletion(-)
>

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

[Michael Roth]

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

Re: [Qemu-devel] [PATCH RFC for-2.3 2/6] qga: introduce three help functions for memory block functions

[Michael Roth]

Quoting zhanghailiang (2014-12-06 00:59:15)
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  qga/commands-posix.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 130 insertions(+)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index b0d6a5d..8917dca 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1875,6 +1875,136 @@ int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
>      return processed;
>  }

I think these would break build bisectability. Please move these into the
patches which introduce the first users.

> 
> +static void ga_read_sysfs_file(int dirfd, const char *pathname, char *buf,
> +                               int size, Error **errp)
> +{
> +    int fd;
> +    int res;
> +
> +    errno = 0;
> +    fd = openat(dirfd, pathname, O_RDONLY);
> +    if (fd == -1) {
> +        error_setg_errno(errp, errno, "open sysfs file \"%s\"", pathname);
> +        return;
> +    }
> +
> +    res = pread(fd, buf, size, 0);
> +    if (res == -1) {
> +        error_setg_errno(errp, errno, "pread sysfs file \"%s\"", pathname);
> +    } else if (res == 0) {
> +        error_setg(errp, "pread sysfs file \"%s\": unexpected EOF", pathname);
> +    }
> +    res = close(fd);
> +    g_assert(res == 0);

This should also be propagated up as an error, killing the guest agent should be
avoided when possible.

> +}
> +
> +static void ga_write_sysfs_file(int dirfd, const char *pathname,
> +                                const char *buf, int size, Error **errp)
> +{
> +    int fd;
> +    int res;
> +
> +    errno = 0;
> +    fd = openat(dirfd, pathname, O_WRONLY);
> +    if (fd == -1) {
> +        error_setg_errno(errp, errno, "open sysfs file \"%s\"", pathname);
> +        return;
> +    }
> +
> +    if (pwrite(fd, buf, size, 0) == -1) {
> +        error_setg_errno(errp, errno, "pwrite sysfs file \"%s\"", pathname);
> +    }
> +
> +    res = close(fd);
> +    g_assert(res == 0);
> +}
> +
> +/* Transfer online/offline status between @mem_blk and the guest system.
> + *
> + * On input either @errp or *@errp must be NULL.
> + *
> + * In system-to-@mem_blk direction, the following @mem_blk fields are accessed:
> + * - R: mem_blk->phys_index
> + * - W: mem_blk->online
> + * - W: mem_blk->can_offline
> + *
> + * In @mem_blk-to-system direction, the following @mem_blk fields are accessed:
> + * - R: mem_blk->phys_index
> + * - R: mem_blk->online
> + *-  R: mem_blk->can_offline
> + * Written members remain unmodified on error.
> + */
> +static void transfer_memory_block(GuestMemoryBlock *mem_blk, bool sys2memblk,
> +                                  Error **errp)
> +{
> +    char *dirpath;
> +    int dirfd;
> +    int res;
> +    char *status;
> +    Error *local_err = NULL;
> +
> +    dirpath = g_strdup_printf("/sys/devices/system/memory/memory%" PRId64 "/",
> +                              mem_blk->phys_index);
> +    dirfd = open(dirpath, O_RDONLY | O_DIRECTORY);
> +    if (dirfd == -1) {
> +        error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
> +        g_free(dirpath);
> +        return;
> +    }
> +    g_free(dirpath);
> +
> +    status = g_malloc0(10);
> +    ga_read_sysfs_file(dirfd, "state", status, 10, &local_err);
> +    if (local_err) {
> +        /* treat with sysfs file that not exist in old kernel */
> +        if (errno == ENOENT) {
> +            error_free(local_err);
> +            if (sys2memblk) {
> +                mem_blk->online = true;
> +                mem_blk->can_offline = false;
> +            } else if (!mem_blk->online) {
> +                error_setg(errp, "memory block #%" PRId64 " can't be "
> +                           "offlined", mem_blk->phys_index);
> +            }
> +        } else {
> +            error_propagate(errp, local_err);
> +        }
> +        return;

Need to free 'status' here, and close 'dirfd'. perhaps just a goto
to the end of the function since you already have common cleanup there.
And, if you use 'local_err' in place of 'err' for error_set, you can
maybe move all your error_propagate calls there as well.

> +    }
> +
> +    if (sys2memblk) {
> +        char removable = '0';
> +
> +        mem_blk->online = (strncmp(status, "online", 6) == 0);
> +
> +        ga_read_sysfs_file(dirfd, "removable", &removable, 1, &local_err);
> +        if (local_err) {
> +            /* if no 'removable' file, it does't support offline mem blk */
> +            if (errno == ENOENT) {
> +                error_free(local_err);
> +                mem_blk->can_offline = false;
> +            } else {
> +                error_propagate(errp, local_err);
> +            }
> +        } else {
> +            mem_blk->can_offline = (removable != '0');
> +        }
> +    } else {
> +        if (mem_blk->online != (strncmp(status, "online", 6) == 0)) {
> +            char *new_state = mem_blk->online ? g_strdup("online") :
> +                                                g_strdup("offline");
> +
> +            ga_write_sysfs_file(dirfd, "state", new_state, strlen(new_state),
> +                                errp);
> +            g_free(new_state);
> +        } /* otherwise pretend successful re-(on|off)-lining */
> +    }
> +
> +    g_free(status);
> +    res = close(dirfd);
> +    g_assert(res == 0);
> +}
> +
>  #else /* defined(__linux__) */
> 
>  void qmp_guest_suspend_disk(Error **errp)
> -- 
> 1.7.12.4

Re: [Qemu-devel] [PATCH RFC for-2.3 3/6] qga: implement qmp_guest_get_memory_blocks() for Linux with sysfs

[Michael Roth]

Quoting zhanghailiang (2014-12-06 00:59:16)
> We can get guest's memory block information by using command
> "guest-get-memory-blocks", the returned value contains a list of memory block
> info, such as phys_index, online state, can-offline info.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  qga/commands-posix.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 60 insertions(+), 6 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 8917dca..d3f7d4f 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2005,6 +2005,60 @@ static void transfer_memory_block(GuestMemoryBlock *mem_blk, bool sys2memblk,
>      g_assert(res == 0);
>  }
> 
> +GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
> +{
> +    GuestMemoryBlockList *head, **link;
> +    Error *local_err = NULL;
> +    struct dirent *de;
> +    DIR *dp;
> +
> +    head = NULL;
> +    link = &head;
> +
> +    dp = opendir("/sys/devices/system/memory/");
> +    if (!dp) {
> +        error_setg_errno(errp, errno, "Can't open directory"
> +                         "\"/sys/devices/system/memory/\"\n");
> +        return NULL;
> +    }
> +
> +    /* Note: the phys_index of memory block may be discontinuous,
> +     * this is because a memblk is the unit of the Sparse Memory design, which
> +     * allows discontinuous memory ranges (ex. NUMA), so here we should
> +     * traverse the memory block directory.
> +     */
> +    while ((de = readdir(dp)) != NULL) {
> +        GuestMemoryBlock *mem_blk;
> +        GuestMemoryBlockList *entry;
> +
> +        if ((strncmp(de->d_name, "memory", 6) != 0) ||
> +            !(de->d_type & DT_DIR)) {
> +            continue;
> +        }
> +
> +        mem_blk = g_malloc0(sizeof *mem_blk);
> +        /* The d_name is "memoryXXX",  phys_index is block id, same as XXX */
> +        mem_blk->phys_index = strtoul(&de->d_name[6], NULL, 10);
> +        mem_blk->has_can_offline = true; /* lolspeak ftw */

My initial thought as well :P

> +        transfer_memory_block(mem_blk, true, &local_err);
> +
> +        entry = g_malloc0(sizeof *entry);
> +        entry->value = mem_blk;
> +
> +        *link = entry;
> +        link = &entry->next;
> +    }
> +
> +    if (local_err == NULL) {
> +        /* there's no guest with zero memroy blocks */

typo

> +        g_assert(head != NULL);

As commented on in earlier patches, guest errors shouldnt crash the guest agent
if it can be avoided.

> +        return head;
> +    }
> +
> +    qapi_free_GuestMemoryBlockList(head);
> +    error_propagate(errp, local_err);
> +    return NULL;
> +}
>  #else /* defined(__linux__) */
> 
>  void qmp_guest_suspend_disk(Error **errp)
> @@ -2040,6 +2094,12 @@ 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;
> +}
> +
>  #endif
> 
>  #if !defined(CONFIG_FSFREEZE)
> @@ -2126,12 +2186,6 @@ GList *ga_command_blacklist_init(GList *blacklist)
>      return blacklist;
>  }
> 
> -GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
> -{
> -    error_set(errp, QERR_UNSUPPORTED);
> -    return NULL;
> -}
> -

Looks like some unecessary code movement made it's way into the patch. Please
squash this change into original patch

>  int64_t qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks,
>                                      Error **errp)
>  {
> -- 
> 1.7.12.4

Re: [Qemu-devel] [PATCH RFC for-2.3 4/6] qga: implement qmp_guest_set_memory_blocks() for Linux with sysfs

[Michael Roth]

Quoting zhanghailiang (2014-12-06 00:59:17)
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  qga/commands-posix.c | 42 +++++++++++++++++++++++++++++++++++-------
>  1 file changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index d3f7d4f..1010e86 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2059,6 +2059,34 @@ GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
>      error_propagate(errp, local_err);
>      return NULL;
>  }
> +
> +int64_t qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks,
> +                                    Error **errp)
> +{
> +    int64_t processed;
> +    Error *local_err = NULL;
> +
> +    processed = 0;
> +    while (mem_blks != NULL) {
> +        transfer_memory_block(mem_blks->value, false, &local_err);
> +        if (local_err) {
> +            break;
> +        }

The documentation claims to maximize the number of blocks we set in the event
of an error, which would suggest we continue processing the remainder of the
list rather than breaking here.

> +        ++processed;
> +        mem_blks = mem_blks->next;
> +    }
> +
> +    if (local_err) {
> +        if (processed == 0) {
> +            error_propagate(errp, local_err);
> +        } else {
> +            error_free(local_err);
> +        }

Really think a list of error/status would make it easier for management to make
sense of things.

> +    }
> +
> +    return processed;
> +}
> +
>  #else /* defined(__linux__) */
> 
>  void qmp_guest_suspend_disk(Error **errp)
> @@ -2100,6 +2128,13 @@ GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
>      return NULL;
>  }
> 
> +int64_t qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks,
> +                                    Error **errp)
> +{
> +    error_set(errp, QERR_UNSUPPORTED);
> +    return -1;
> +}
> +
>  #endif
> 
>  #if !defined(CONFIG_FSFREEZE)
> @@ -2186,13 +2221,6 @@ GList *ga_command_blacklist_init(GList *blacklist)
>      return blacklist;
>  }
> 
> -int64_t qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks,
> -                                    Error **errp)
> -{
> -    error_set(errp, QERR_UNSUPPORTED);
> -    return -1;
> -}
> -

Same comment here as with prior patch

>  int64_t qmp_guest_get_memory_block_size(Error **errp)
>  {
>      error_set(errp, QERR_UNSUPPORTED);
> -- 
> 1.7.12.4

Re: [Qemu-devel] [PATCH RFC for-2.3 5/6] qga: implement qmp_guest_get_memory_block_size() for Linux with sysfs

[Michael Roth]

Quoting zhanghailiang (2014-12-06 00:59:18)
> The size of a memory block is architecture dependent,
> For example, power uses 16MiB, ia64 uses 1GiB, x86 uses 128M.

That's the minimum for Power, but the size can be set via device tree and
can differ from one platform to the next.

Should either clarify in the commit or maybe just drop the statement to
avoid confusion.

> It represents the logical unit upon which memory online/offline operations are
> to be performed.
> 
> This function will return the value to host user.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  qga/commands-posix.c | 43 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 37 insertions(+), 6 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 1010e86..02c6b44 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2087,6 +2087,37 @@ int64_t qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks,
>      return processed;
>  }
> 
> +int64_t qmp_guest_get_memory_block_size(Error **errp)
> +{
> +    Error *local_err = NULL;
> +    char *dirpath;
> +    int dirfd;
> +    char *buf;
> +    int64_t block_size;
> +
> +    dirpath = g_strdup_printf("/sys/devices/system/memory/");
> +    dirfd = open(dirpath, O_RDONLY | O_DIRECTORY);
> +    if (dirfd == -1) {
> +        error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
> +        g_free(dirpath);
> +        return -1;
> +    }
> +    g_free(dirpath);
> +
> +    buf = g_malloc0(20);
> +    ga_read_sysfs_file(dirfd, "block_size_bytes", buf, 20, &local_err);
> +    if (local_err) {
> +        g_free(buf);
> +        error_propagate(errp, local_err);
> +        return -1;
> +    }
> +
> +    block_size = strtol(buf, NULL, 16); /* the unit is bytes */
> +    g_free(buf);
> +
> +    return block_size;
> +}
> +
>  #else /* defined(__linux__) */
> 
>  void qmp_guest_suspend_disk(Error **errp)
> @@ -2135,6 +2166,12 @@ int64_t qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks,
>      return -1;
>  }
> 
> +int64_t qmp_guest_get_memory_block_size(Error **errp)
> +{
> +    error_set(errp, QERR_UNSUPPORTED);
> +    return -1;
> +}
> +
>  #endif
> 
>  #if !defined(CONFIG_FSFREEZE)
> @@ -2221,12 +2258,6 @@ GList *ga_command_blacklist_init(GList *blacklist)
>      return blacklist;
>  }
> 
> -int64_t qmp_guest_get_memory_block_size(Error **errp)
> -{
> -    error_set(errp, QERR_UNSUPPORTED);
> -    return -1;
> -}
> -

Same as with above 2 patches, please squash into the initial stub patch

>  /* register init/cleanup routines for stateful command groups */
>  void ga_command_state_init(GAState *s, GACommandState *cs)
>  {
> -- 
> 1.7.12.4

Re: [Qemu-devel] [PATCH RFC for-2.3 6/6] qga: add memory block command that unsupported to blacklist

[Michael Roth]

Quoting zhanghailiang (2014-12-06 00:59:19)
> For memory block command, we only support for linux with sysfs.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
>  qga/commands-posix.c | 4 +++-
>  qga/commands-win32.c | 2 ++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 02c6b44..b701108 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2228,7 +2228,9 @@ GList *ga_command_blacklist_init(GList *blacklist)
>          const char *list[] = {
>              "guest-suspend-disk", "guest-suspend-ram",
>              "guest-suspend-hybrid", "guest-network-get-interfaces",
> -            "guest-get-vcpus", "guest-set-vcpus", NULL};
> +            "guest-get-vcpus", "guest-set-vcpus",
> +            "guest-get-memory-blocks", "guest-set-memory-blocks",
> +            "guest-get-memory-block-size", NULL};
>          char **p = (char **)list;
> 
>          while (*p) {
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index b53b679..dd7031a 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -473,6 +473,8 @@ GList *ga_command_blacklist_init(GList *blacklist)
>          "guest-file-write", "guest-file-seek", "guest-file-flush",
>          "guest-suspend-hybrid", "guest-network-get-interfaces",
>          "guest-get-vcpus", "guest-set-vcpus",
> +        "guest-get-memory-blocks", "guest-set-memory-blocks",
> +        "guest-get-memory-block-size",
>          "guest-fsfreeze-freeze-list", "guest-get-fsinfo",
>          "guest-fstrim", NULL};
>      char **p = (char **)list_unsupported;
> -- 
> 1.7.12.4

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

Re: [Qemu-devel] [PATCH RFC for-2.3 2/6] qga: introduce three help functions for memory block functions

[zhanghailiang]

On 2014/12/22 4:58, Michael Roth wrote:
> Quoting zhanghailiang (2014-12-06 00:59:15)
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> ---
>>   qga/commands-posix.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 130 insertions(+)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index b0d6a5d..8917dca 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -1875,6 +1875,136 @@ int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
>>       return processed;
>>   }
>
> I think these would break build bisectability. Please move these into the
> patches which introduce the first users.
>

OK, will squash patch into patch 3.

>>
>> +static void ga_read_sysfs_file(int dirfd, const char *pathname, char *buf,
>> +                               int size, Error **errp)
>> +{
>> +    int fd;
>> +    int res;
>> +
>> +    errno = 0;
>> +    fd = openat(dirfd, pathname, O_RDONLY);
>> +    if (fd == -1) {
>> +        error_setg_errno(errp, errno, "open sysfs file \"%s\"", pathname);
>> +        return;
>> +    }
>> +
>> +    res = pread(fd, buf, size, 0);
>> +    if (res == -1) {
>> +        error_setg_errno(errp, errno, "pread sysfs file \"%s\"", pathname);
>> +    } else if (res == 0) {
>> +        error_setg(errp, "pread sysfs file \"%s\": unexpected EOF", pathname);
>> +    }
>> +    res = close(fd);
>> +    g_assert(res == 0);
>
> This should also be propagated up as an error, killing the guest agent should be
> avoided when possible.
>

Yes, you are right. I noted that, in most cases, we ignore the return value of close() in qemu codes,
so i will choose to ignore here too, is it OK?

>> +}
>> +
>> +static void ga_write_sysfs_file(int dirfd, const char *pathname,
>> +                                const char *buf, int size, Error **errp)
>> +{
>> +    int fd;
>> +    int res;
>> +
>> +    errno = 0;
>> +    fd = openat(dirfd, pathname, O_WRONLY);
>> +    if (fd == -1) {
>> +        error_setg_errno(errp, errno, "open sysfs file \"%s\"", pathname);
>> +        return;
>> +    }
>> +
>> +    if (pwrite(fd, buf, size, 0) == -1) {
>> +        error_setg_errno(errp, errno, "pwrite sysfs file \"%s\"", pathname);
>> +    }
>> +
>> +    res = close(fd);
>> +    g_assert(res == 0);
>> +}
>> +
>> +/* Transfer online/offline status between @mem_blk and the guest system.
>> + *
>> + * On input either @errp or *@errp must be NULL.
>> + *
>> + * In system-to-@mem_blk direction, the following @mem_blk fields are accessed:
>> + * - R: mem_blk->phys_index
>> + * - W: mem_blk->online
>> + * - W: mem_blk->can_offline
>> + *
>> + * In @mem_blk-to-system direction, the following @mem_blk fields are accessed:
>> + * - R: mem_blk->phys_index
>> + * - R: mem_blk->online
>> + *-  R: mem_blk->can_offline
>> + * Written members remain unmodified on error.
>> + */
>> +static void transfer_memory_block(GuestMemoryBlock *mem_blk, bool sys2memblk,
>> +                                  Error **errp)
>> +{
>> +    char *dirpath;
>> +    int dirfd;
>> +    int res;
>> +    char *status;
>> +    Error *local_err = NULL;
>> +
>> +    dirpath = g_strdup_printf("/sys/devices/system/memory/memory%" PRId64 "/",
>> +                              mem_blk->phys_index);
>> +    dirfd = open(dirpath, O_RDONLY | O_DIRECTORY);
>> +    if (dirfd == -1) {
>> +        error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
>> +        g_free(dirpath);
>> +        return;
>> +    }
>> +    g_free(dirpath);
>> +
>> +    status = g_malloc0(10);
>> +    ga_read_sysfs_file(dirfd, "state", status, 10, &local_err);
>> +    if (local_err) {
>> +        /* treat with sysfs file that not exist in old kernel */
>> +        if (errno == ENOENT) {
>> +            error_free(local_err);
>> +            if (sys2memblk) {
>> +                mem_blk->online = true;
>> +                mem_blk->can_offline = false;
>> +            } else if (!mem_blk->online) {
>> +                error_setg(errp, "memory block #%" PRId64 " can't be "
>> +                           "offlined", mem_blk->phys_index);
>> +            }
>> +        } else {
>> +            error_propagate(errp, local_err);
>> +        }
>> +        return;
>
> Need to free 'status' here, and close 'dirfd'. perhaps just a goto
> to the end of the function since you already have common cleanup there.
> And, if you use 'local_err' in place of 'err' for error_set, you can
> maybe move all your error_propagate calls there as well.
>

Good catch, will fix it in next version, thanks.

>> +    }
>> +
>> +    if (sys2memblk) {
>> +        char removable = '0';
>> +
>> +        mem_blk->online = (strncmp(status, "online", 6) == 0);
>> +
>> +        ga_read_sysfs_file(dirfd, "removable", &removable, 1, &local_err);
>> +        if (local_err) {
>> +            /* if no 'removable' file, it does't support offline mem blk */
>> +            if (errno == ENOENT) {
>> +                error_free(local_err);
>> +                mem_blk->can_offline = false;
>> +            } else {
>> +                error_propagate(errp, local_err);
>> +            }
>> +        } else {
>> +            mem_blk->can_offline = (removable != '0');
>> +        }
>> +    } else {
>> +        if (mem_blk->online != (strncmp(status, "online", 6) == 0)) {
>> +            char *new_state = mem_blk->online ? g_strdup("online") :
>> +                                                g_strdup("offline");
>> +
>> +            ga_write_sysfs_file(dirfd, "state", new_state, strlen(new_state),
>> +                                errp);
>> +            g_free(new_state);
>> +        } /* otherwise pretend successful re-(on|off)-lining */
>> +    }
>> +
>> +    g_free(status);
>> +    res = close(dirfd);
>> +    g_assert(res == 0);
>> +}
>> +
>>   #else /* defined(__linux__) */
>>
>>   void qmp_guest_suspend_disk(Error **errp)
>> --
>> 1.7.12.4
>
>
> .
>

Re: [Qemu-devel] [PATCH RFC for-2.3 3/6] qga: implement qmp_guest_get_memory_blocks() for Linux with sysfs

[zhanghailiang]

On 2014/12/22 5:17, Michael Roth wrote:
> Quoting zhanghailiang (2014-12-06 00:59:16)
>> We can get guest's memory block information by using command
>> "guest-get-memory-blocks", the returned value contains a list of memory block
>> info, such as phys_index, online state, can-offline info.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> ---
>>   qga/commands-posix.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 60 insertions(+), 6 deletions(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index 8917dca..d3f7d4f 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -2005,6 +2005,60 @@ static void transfer_memory_block(GuestMemoryBlock *mem_blk, bool sys2memblk,
>>       g_assert(res == 0);
>>   }
>>
>> +GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
>> +{
>> +    GuestMemoryBlockList *head, **link;
>> +    Error *local_err = NULL;
>> +    struct dirent *de;
>> +    DIR *dp;
>> +
>> +    head = NULL;
>> +    link = &head;
>> +
>> +    dp = opendir("/sys/devices/system/memory/");
>> +    if (!dp) {
>> +        error_setg_errno(errp, errno, "Can't open directory"
>> +                         "\"/sys/devices/system/memory/\"\n");
>> +        return NULL;
>> +    }
>> +
>> +    /* Note: the phys_index of memory block may be discontinuous,
>> +     * this is because a memblk is the unit of the Sparse Memory design, which
>> +     * allows discontinuous memory ranges (ex. NUMA), so here we should
>> +     * traverse the memory block directory.
>> +     */
>> +    while ((de = readdir(dp)) != NULL) {
>> +        GuestMemoryBlock *mem_blk;
>> +        GuestMemoryBlockList *entry;
>> +
>> +        if ((strncmp(de->d_name, "memory", 6) != 0) ||
>> +            !(de->d_type & DT_DIR)) {
>> +            continue;
>> +        }
>> +
>> +        mem_blk = g_malloc0(sizeof *mem_blk);
>> +        /* The d_name is "memoryXXX",  phys_index is block id, same as XXX */
>> +        mem_blk->phys_index = strtoul(&de->d_name[6], NULL, 10);
>> +        mem_blk->has_can_offline = true; /* lolspeak ftw */
>
> My initial thought as well :P
>

Ha, yes. ;)

>> +        transfer_memory_block(mem_blk, true, &local_err);
>> +
>> +        entry = g_malloc0(sizeof *entry);
>> +        entry->value = mem_blk;
>> +
>> +        *link = entry;
>> +        link = &entry->next;
>> +    }
>> +
>> +    if (local_err == NULL) {
>> +        /* there's no guest with zero memroy blocks */
>
> typo
>
>> +        g_assert(head != NULL);
>
> As commented on in earlier patches, guest errors shouldnt crash the guest agent
> if it can be avoided.
>

OK.

>> +        return head;
>> +    }
>> +
>> +    qapi_free_GuestMemoryBlockList(head);
>> +    error_propagate(errp, local_err);
>> +    return NULL;
>> +}
>>   #else /* defined(__linux__) */
>>
>>   void qmp_guest_suspend_disk(Error **errp)
>> @@ -2040,6 +2094,12 @@ 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;
>> +}
>> +
>>   #endif
>>
>>   #if !defined(CONFIG_FSFREEZE)
>> @@ -2126,12 +2186,6 @@ GList *ga_command_blacklist_init(GList *blacklist)
>>       return blacklist;
>>   }
>>
>> -GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
>> -{
>> -    error_set(errp, QERR_UNSUPPORTED);
>> -    return NULL;
>> -}
>> -
>
> Looks like some unecessary code movement made it's way into the patch. Please
> squash this change into original patch
>

OK, will fix that. Thanks.

>>   int64_t qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks,
>>                                       Error **errp)
>>   {
>> --
>> 1.7.12.4
>
>
> .
>

Re: [Qemu-devel] [PATCH RFC for-2.3 4/6] qga: implement qmp_guest_set_memory_blocks() for Linux with sysfs

[zhanghailiang]

On 2014/12/22 5:23, Michael Roth wrote:
> Quoting zhanghailiang (2014-12-06 00:59:17)
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> ---
>>   qga/commands-posix.c | 42 +++++++++++++++++++++++++++++++++++-------
>>   1 file changed, 35 insertions(+), 7 deletions(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index d3f7d4f..1010e86 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -2059,6 +2059,34 @@ GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
>>       error_propagate(errp, local_err);
>>       return NULL;
>>   }
>> +
>> +int64_t qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks,
>> +                                    Error **errp)
>> +{
>> +    int64_t processed;
>> +    Error *local_err = NULL;
>> +
>> +    processed = 0;
>> +    while (mem_blks != NULL) {
>> +        transfer_memory_block(mem_blks->value, false, &local_err);
>> +        if (local_err) {
>> +            break;
>> +        }
>
> The documentation claims to maximize the number of blocks we set in the event
> of an error, which would suggest we continue processing the remainder of the
> list rather than breaking here.
>

OK, one more thing, should we fix like that for set-vcpus ?

>> +        ++processed;
>> +        mem_blks = mem_blks->next;
>> +    }
>> +
>> +    if (local_err) {
>> +        if (processed == 0) {
>> +            error_propagate(errp, local_err);
>> +        } else {
>> +            error_free(local_err);
>> +        }
>
> Really think a list of error/status would make it easier for management to make
> sense of things.
>

Agreed :)

>> +    }
>> +
>> +    return processed;
>> +}
>> +
>>   #else /* defined(__linux__) */
>>
>>   void qmp_guest_suspend_disk(Error **errp)
>> @@ -2100,6 +2128,13 @@ GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
>>       return NULL;
>>   }
>>
>> +int64_t qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks,
>> +                                    Error **errp)
>> +{
>> +    error_set(errp, QERR_UNSUPPORTED);
>> +    return -1;
>> +}
>> +
>>   #endif
>>
>>   #if !defined(CONFIG_FSFREEZE)
>> @@ -2186,13 +2221,6 @@ GList *ga_command_blacklist_init(GList *blacklist)
>>       return blacklist;
>>   }
>>
>> -int64_t qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks,
>> -                                    Error **errp)
>> -{
>> -    error_set(errp, QERR_UNSUPPORTED);
>> -    return -1;
>> -}
>> -
>
> Same comment here as with prior patch
>

OK, thanks.

>>   int64_t qmp_guest_get_memory_block_size(Error **errp)
>>   {
>>       error_set(errp, QERR_UNSUPPORTED);
>> --
>> 1.7.12.4
>
>
> .
>

Re: [Qemu-devel] [PATCH RFC for-2.3 5/6] qga: implement qmp_guest_get_memory_block_size() for Linux with sysfs

[zhanghailiang]

On 2014/12/22 5:41, Michael Roth wrote:
> Quoting zhanghailiang (2014-12-06 00:59:18)
>> The size of a memory block is architecture dependent,
>> For example, power uses 16MiB, ia64 uses 1GiB, x86 uses 128M.
>
> That's the minimum for Power, but the size can be set via device tree and
> can differ from one platform to the next.
>
> Should either clarify in the commit or maybe just drop the statement to
> avoid confusion.
>

OK, i will drop that statement.

>> It represents the logical unit upon which memory online/offline operations are
>> to be performed.
>>
>> This function will return the value to host user.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> ---
>>   qga/commands-posix.c | 43 +++++++++++++++++++++++++++++++++++++------
>>   1 file changed, 37 insertions(+), 6 deletions(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index 1010e86..02c6b44 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -2087,6 +2087,37 @@ int64_t qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks,
>>       return processed;
>>   }
>>
>> +int64_t qmp_guest_get_memory_block_size(Error **errp)
>> +{
>> +    Error *local_err = NULL;
>> +    char *dirpath;
>> +    int dirfd;
>> +    char *buf;
>> +    int64_t block_size;
>> +
>> +    dirpath = g_strdup_printf("/sys/devices/system/memory/");
>> +    dirfd = open(dirpath, O_RDONLY | O_DIRECTORY);
>> +    if (dirfd == -1) {
>> +        error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
>> +        g_free(dirpath);
>> +        return -1;
>> +    }
>> +    g_free(dirpath);
>> +
>> +    buf = g_malloc0(20);
>> +    ga_read_sysfs_file(dirfd, "block_size_bytes", buf, 20, &local_err);
>> +    if (local_err) {
>> +        g_free(buf);
>> +        error_propagate(errp, local_err);
>> +        return -1;
>> +    }
>> +
>> +    block_size = strtol(buf, NULL, 16); /* the unit is bytes */
>> +    g_free(buf);
>> +
>> +    return block_size;
>> +}
>> +
>>   #else /* defined(__linux__) */
>>
>>   void qmp_guest_suspend_disk(Error **errp)
>> @@ -2135,6 +2166,12 @@ int64_t qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks,
>>       return -1;
>>   }
>>
>> +int64_t qmp_guest_get_memory_block_size(Error **errp)
>> +{
>> +    error_set(errp, QERR_UNSUPPORTED);
>> +    return -1;
>> +}
>> +
>>   #endif
>>
>>   #if !defined(CONFIG_FSFREEZE)
>> @@ -2221,12 +2258,6 @@ GList *ga_command_blacklist_init(GList *blacklist)
>>       return blacklist;
>>   }
>>
>> -int64_t qmp_guest_get_memory_block_size(Error **errp)
>> -{
>> -    error_set(errp, QERR_UNSUPPORTED);
>> -    return -1;
>> -}
>> -
>
> Same as with above 2 patches, please squash into the initial stub patch
>

OK.

>>   /* register init/cleanup routines for stateful command groups */
>>   void ga_command_state_init(GAState *s, GACommandState *cs)
>>   {
>> --
>> 1.7.12.4
>
>
> .
>