On 10/15/2015 05:44 PM, address@hidden wrote:
From: Valerio Aimale <address@hidden>
Long subject line, and no message body. Remember, you want the subject
line to be a one-line short summary of 'what', then the commit body
message for 'why', as in:
qmp: add command for libvmi memory introspection
In the past, libvmi was relying on an out-of-tree patch to qemu that
provides a new QMP command pmemaccess. It is now time to make this
command part of qemu.
pmemaccess is used to create a side-channel communication path that can
more effectively be used to query lots of small memory chunks without
the overhead of one QMP command per chunk. ...
---
You are missing a Signed-off-by: tag. Without that, we cannot take your
patch. But at least we can still review it:
Makefile.target | 2 +-
hmp-commands.hx | 14 ++++
hmp.c | 9 +++
hmp.h | 1 +
memory-access.c | 206 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
memory-access.h | 21 ++++++
qapi-schema.json | 28 ++++++++
qmp-commands.hx | 23 +++++++
8 files changed, 303 insertions(+), 1 deletion(-)
create mode 100644 memory-access.c
create mode 100644 memory-access.h
diff --git a/Makefile.target b/Makefile.target
index 962d004..940ab51 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -131,7 +131,7 @@ endif #CONFIG_BSD_USER
#########################################################
# System emulator target
ifdef CONFIG_SOFTMMU
-obj-y += arch_init.o cpus.o monitor.o gdbstub.o balloon.o ioport.o numa.o
+obj-y += arch_init.o cpus.o monitor.o gdbstub.o balloon.o ioport.o numa.o
memory-access.o
This line is now over 80 columns; please wrap.
obj-y += qtest.o bootdevice.o
In fact, you could have just appended it into this line instead.
+++ b/hmp-commands.hx
@@ -807,6 +807,20 @@ save to disk physical memory dump starting at @var{addr}
of size @var{size}.
ETEXI
{
+ .name = "pmemaccess",
+ .args_type = "path:s",
+ .params = "file",
+ .help = "open A UNIX Socket access to physical memory at 'path'",
s/A/a/
Awkward grammar; better might be:
open a Unix socket at 'path' for use in accessing physical memory
Please also document where the user can find the protocol that will be
used across the side-channel socket thus opened.
+++ b/memory-access.c
@@ -0,0 +1,206 @@
+/*
+ * Access guest physical memory via a domain socket.
+ *
+ * Copyright (C) 2011 Sandia National Laboratories
+ * Original Author: Bryan D. Payne (address@hidden)
+ *
+ * Refurbished for modern QEMU by Valerio Aimale (address@hidden), in 2015
+ */
I would expect at least something under docs/ in addition to this file
(the protocol spoken over the socket should be well-documented, and not
just by reading the source code). Compare with docs/qmp-spec.txt.
+struct request{
+ uint8_t type; // 0 quit, 1 read, 2 write, ... rest reserved
+ uint64_t address; // address to read from OR write to
+ uint64_t length; // number of bytes to read OR write
Any particular endianness constraints to worry about?
+};
+
+static uint64_t
+connection_read_memory (uint64_t user_paddr, void *buf, uint64_t user_len)
+{
+ hwaddr paddr = (hwaddr) user_paddr;
+ hwaddr len = (hwaddr) user_len;
+ void *guestmem = cpu_physical_memory_map(paddr, &len, 0);
+ if (!guestmem){
Space before {, throughout the patch.
+static void
+send_success_ack (int connection_fd)
No space before ( in function usage.
+{
+ uint8_t success = 1;
Magic number; I'd expect an enum or #defines somewhere.
+ int nbytes = write(connection_fd, &success, 1);
+ if (1 != nbytes){
+ fprintf(stderr, "Qemu pmemaccess: failed to send success ack\n");
Is fprintf() really the best approach for error reporting?
+static void
+connection_handler (int connection_fd)
+{
+ int nbytes;
+ struct request req;
+
+ while (1){
+ // client request should match the struct request format
We prefer /* */ comments over //.
+ nbytes = read(connection_fd, &req, sizeof(struct request));
+ if (nbytes != sizeof(struct request)){
+ // error
+ continue;
Silently ignoring errors?
+ }
+ else if (req.type == 0){
+ // request to quit, goodbye
+ break;
+ }
+ else if (req.type == 1){
+ // request to read
+ char *buf = malloc(req.length + 1);
Mixing g_malloc/g_free with malloc/free makes it a bit harder to reason
about things; it might be better to use only glib allocation.
+ nbytes = connection_read_memory(req.address, buf, req.length);
+ if (nbytes != req.length){
+ // read failure, return failure message
+ buf[req.length] = 0; // set last byte to 0 for failure
+ nbytes = write(connection_fd, buf, 1);
+ }
+ else{
+ // read success, return bytes
+ buf[req.length] = 1; // set last byte to 1 for success
+ nbytes = write(connection_fd, buf, nbytes + 1);
+ }
+ free(buf);
+ }
+ else if (req.type == 2){
+ // request to write
+ void *write_buf = malloc(req.length);
+ nbytes = read(connection_fd, &write_buf, req.length);
No error checking that the allocation succeeded? How do you protect from
bogus requests?
+ if (nbytes != req.length){
+ // failed reading the message to write
+ send_fail_ack(connection_fd);
+ }
+ else{
} on the same line as else
+ // do the write
+ nbytes = connection_write_memory(req.address, write_buf,
req.length);
+ if (nbytes == req.length){
+ send_success_ack(connection_fd);
+ }
+ else{
+ send_fail_ack(connection_fd);
+ }
+ }
+ free(write_buf);
+ }
+ else{
+ // unknown command
+ fprintf(stderr, "Qemu pmemaccess: ignoring unknown command
(%d)\n", req.type);
+ char *buf = malloc(1);
+ buf[0] = 0;
+ nbytes = write(connection_fd, buf, 1);
+ free(buf);
+ }
+ }
+
+ close(connection_fd);
+}
+
+static void *
+memory_access_thread (void *p)
The most common style in qemu puts return type on the same line as the
function name.
+{
+ struct sockaddr_un address;
+ int socket_fd, connection_fd;
+ socklen_t address_length;
+ struct pmemaccess_args *pargs = (struct pmemaccess_args *)p;
This cast is not necessary in C.
+
+ socket_fd = socket(PF_UNIX, SOCK_STREAM, 0);
+ if (socket_fd < 0){
+ error_setg(pargs->errp, "Qemu pmemaccess: socket failed");
TAB damage. Also, mentioning 'Qemu' in an error message is probably
redundant. That, and we prefer 'qemu' over 'Qemu'.
+ goto error_exit;
+ }
+ unlink(pargs->path);
No check for errors?
+ address.sun_family = AF_UNIX;
+ address_length = sizeof(address.sun_family) + sprintf(address.sun_path, "%s",
(char *) pargs->path);
Long line. sprintf() is dangerous if you are not positive that
pargs->path fits. Why do you need the cast?
+
+ if (bind(socket_fd, (struct sockaddr *) &address, address_length) != 0){
+ error_setg(pargs->errp, "Qemu pmemaccess: bind failed");
More TAB damage. Please read
http://wiki.qemu.org/Contribute/SubmitAPatch for more contribution
hints, and be sure to run ./scripts/checkpatch.pl.
+void
+qmp_pmemaccess (const char *path, Error **errp)
+{
+ pthread_t thread;
+ sigset_t set, oldset;
+ struct pmemaccess_args *pargs;
+
+ // create the args struct
+ pargs = (struct pmemaccess_args *) malloc(sizeof(struct pmemaccess_args));
+ pargs->errp = errp;
+ // create a copy of path that we can safely use
+ pargs->path = malloc(strlen(path) + 1);
+ memcpy(pargs->path, path, strlen(path) + 1);
g_strdup() is your friend.
+++ b/qapi-schema.json
@@ -1427,6 +1427,34 @@
'data': {'val': 'int', 'size': 'int', 'filename': 'str'} }
##
+# @pmemaccess:
+#
+# Open A UNIX Socket access to physical memory
s/A UNIX Socket/a Unix socket/
Same wording suggestion as earlier; might be better as:
Open a Unix socket used as a side-channel for efficiently accessing
physical memory.
+#
+# @path: the name of the UNIX socket pipe
+#
+# Returns: Nothing on success
+#
+# Since: 2.4.0.1
2.5, not 2.4.0.1.
+#
+# Notes: Derived from previously existing patches.
Dead sentence that doesn't add anything to the current specification.
When command
+# succeeds connect to the open socket. Write a binary structure to
+# the socket as:
+#
+# struct request {
+# uint8_t type; // 0 quit, 1 read, 2 write, ... rest reserved
+# uint64_t address; // address to read from OR write to
+# uint64_t length; // number of bytes to read OR write
+# };
+#
+# If it is a read operation, Qemu will return lenght+1 bytes. Read lenght+1
s/lenght/length/ twice
+# bytes. the last byte will be a 1 for success, or a 0 for failure.
+#
+##
+{ 'command': 'pmemaccess',
+ 'data': {'path': 'str'} }
+
+##
# @cont:
#
# Resume guest VCPU execution.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index d2ba800..26e4a51 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -472,6 +472,29 @@ Example:
EQMP
{
+ .name = "pmemaccess",
+ .args_type = "path:s",
+ .mhandler.cmd_new = qmp_marshal_input_pmemaccess,
+ },
+
+SQMP
+pmemaccess
+----------
+
+Open A UNIX Socket access to physical memory
+
+Arguments:
+
+- "path": mount point path (json-string)
+
+Example:
+
+-> { "execute": "pmemaccess",
+ "arguments": { "path": "/tmp/guestname" } }
+<- { "return": {} }
+
+EQMP
+ {
.name = "inject-nmi",
.args_type = "",
.mhandler.cmd_new = qmp_marshal_inject_nmi,