* [Qemu-devel] [PATCH v8 0/8] Add limited support of VMware's hyper-call rpc
@ 2015-06-23 23:39 Don Slutz
2015-06-23 23:39 ` [Qemu-devel] [PATCH v8 1/8] vmport: Switch to trace Don Slutz
` (8 more replies)
0 siblings, 9 replies; 19+ messages in thread
From: Don Slutz @ 2015-06-23 23:39 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S. Tsirkin, Markus Armbruster, Luiz Capitulino, Don Slutz,
Anthony Liguori, Paolo Bonzini, Andreas Färber,
Richard Henderson
Changes v7 to v8:
Rebase to master
Drop patch #1 since commit 965eb2fcdfe919ecced6c34803535ad32dc1249c
fixed this.
Paolo Bonzini
Ok to pull v7 1,2,3, and 9.
Added Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Moved v7 #9 to v8 #3
Eric Blake
s/it's/its/
Done
No space after **.
Done
Don't know if any static checkers will complain about the break being
dead code.
Dropped break since this is the last case.
Would it help if struct keyValue declared 'const char *key_data'?
It does, so switch to this.
This cast is spurious (foreach_dynamic_vmport_rpc_device)
Dropped all casts using this routine.
Do you want g_malloc0, or g_new0?
Switched to g_new0.
Adjust /* FIXME should check for...
Added "Or cause an error to be raised."
Michael S. Tsirkin
To me it looks like this will break cross-version migration
Dropped code to always add the new device.
Add code to check for vmport enabled.
Quite possibly but personally I'm confused.
No partial PULL request sent.
Changes v6 to v7:
Rebase to master
Fixed a bug caused by commit c3c1bb99d1c11978d9ce94d1bdcf0705378c1459 now patch #1
Added patch #2 to switch to using trace in vm,port.c.
Delay call on g_strndup till after key length check.
Switched e-mail address in MAINTAINERS.
Eric Blake
Why not assert(find) instead of leaving it to the comment?
Switch to assert.
Is it worth marking arg const here and in the VMPortRpcFind struct
Switch to const.
I'd rather abort() if someone compiled with -NDEBUG
Done.
Still mismatches on ---- line length (several sites).
Done
Changes v5 to v6:
Rebase to master
Eric Blake
Returning a non-dictionary is not extensible.
Added new type VmportGuestInfoValue.
s/VmportGuestInfo/VmportGuestInfoKey/
s/type/struct/
Issues with examples
Fixed.
Changes v4 to v5:
Paolo Bonzini
What is VMPORT_SHORT about?
Dropped this.
Why not use a bool in CPUX86State?
Took his sugestion and moved to a bool in X86CPU.
Changes v3 to v4:
Paolo Bonzini on "vmort_rpc: Add QMP access to vmport_rpc"
Does this compile on non-x86 targets?
Nope. Fixed.
Changes v2 to v3:
s/2.3/2.4
Changes v1 to v2:
Added live migration code.
Adjust data structures for migration.
Switch to GHashTable.
Eric Blake
s/spawened/spawned/
Done
s/traceing/tracing/
Done
Change "error_set(errp, ERROR_CLASS_GENERIC_ERROR, " to
"error_setg(errp, "
Done
Why two commands (inject-vmport-reboot, inject-vmport-halt)?
Switched to inject-vmport-action.
format=base64 "bug" statements.
Dropped.
Much more on format=base64:
If there is a bug it is in GLIB. However the Glib reference manual
refers to RFC 1421 and RFC 2045 and MIME encoding. Based on all
that (which seems to match:
http://en.wikipedia.org/wiki/Base64
) MIME states that all characters outside the (base64) alphabet are
to be ignored. Testing shows that g_base64_decode() does this.
The confusion is that most non-MIME uses reject a base64 string that
contain characters outside the alphabet. I was just following the
other uses of base64 in this file.
DataFormat refers to RFC 3548, which has the info:
"
Implementations MUST reject the encoding if it contains
characters outside the base alphabet when interpreting base
encoded data, unless the specification referring to this document
explicitly states otherwise. Such specifications may, as MIME
does, instead state that characters outside the base encoding
alphabet should simply be ignored when interpreting data ("be
liberal in what you accept").
"
So with GLIB going the MIME way, I do not think this is a QEMU bug
(you could consider this a GLIB bug, but the document I found says
that GLIB goes the MIME way and so does not reject anything).
---
The support included is enough to allow VMware tools to install in a
guest and provide guestinfo support. guestinfo support is provided
by what is known as VMware RPC support.
One of the better on-line references is:
https://sites.google.com/site/chitchatvmback/backdoor
As a place to get more accurate information by studying:
http://open-vm-tools.sourceforge.net/
With vmware tools installed, you can do:
-------------------------------------------------------------------------------
Last login: Fri Jan 30 16:03:08 2015
[root@C63-min-tools ~]# vmtoolsd --cmd "info-get guestinfo.joejoel"
No value found
[root@C63-min-tools ~]# vmtoolsd --cmd "info-set guestinfo.joejoel bar"
[root@C63-min-tools ~]# vmtoolsd --cmd "info-get guestinfo.joejoel"
bar
[root@C63-min-tools ~]#
-------------------------------------------------------------------------------
to access guest info. QMP access is also provided.
The live migration code is still in progress.
Don Slutz (8):
vmport: Switch to trace
vmport: Fix vmport_cmd_ram_size
MAINTAINERS: add VMware port
vmport_rpc: Add the object vmport_rpc
vmport_rpc: Add limited support of VMware's hyper-call rpc
vmport_rpc: Add QMP access to vmport_rpc object.
vmport_rpc: Add migration
vmport: Add VMware all ring hack
MAINTAINERS | 7 +
hw/i386/pc.c | 26 +-
hw/i386/pc_piix.c | 2 +-
hw/i386/pc_q35.c | 2 +-
hw/misc/Makefile.objs | 1 +
hw/misc/vmport.c | 16 +-
hw/misc/vmport_rpc.c | 1461 ++++++++++++++++++++++++++++++++++++++++++++++
include/hw/i386/pc.h | 8 +-
monitor.c | 24 +
qapi-schema.json | 101 ++++
qmp-commands.hx | 119 ++++
target-i386/cpu-qom.h | 3 +
target-i386/seg_helper.c | 9 +
trace-events | 27 +
14 files changed, 1793 insertions(+), 13 deletions(-)
create mode 100644 hw/misc/vmport_rpc.c
--
1.8.3.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v8 1/8] vmport: Switch to trace
2015-06-23 23:39 [Qemu-devel] [PATCH v8 0/8] Add limited support of VMware's hyper-call rpc Don Slutz
@ 2015-06-23 23:39 ` Don Slutz
2015-06-23 23:39 ` [Qemu-devel] [BUGFIX][PATCH v8 2/8] vmport: Fix vmport_cmd_ram_size Don Slutz
` (7 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Don Slutz @ 2015-06-23 23:39 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S. Tsirkin, Markus Armbruster, Don Slutz, Luiz Capitulino,
Don Slutz, Anthony Liguori, Paolo Bonzini, Andreas Färber,
Richard Henderson
From: Don Slutz <dslutz@verizon.com>
Signed-off-by: Don Slutz <dslutz@verizon.com>
CC: Don Slutz <don.slutz@gmail.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
---
v8:
Added Acked-by: Paolo Bonzini
hw/misc/vmport.c | 6 ++----
trace-events | 3 +++
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/hw/misc/vmport.c b/hw/misc/vmport.c
index 7fcc00d..3b81d04 100644
--- a/hw/misc/vmport.c
+++ b/hw/misc/vmport.c
@@ -27,7 +27,7 @@
#include "sysemu/kvm.h"
#include "hw/qdev.h"
-//#define VMPORT_DEBUG
+#include "trace.h"
#define VMPORT_CMD_GETVERSION 0x0a
#define VMPORT_CMD_GETRAMSIZE 0x14
@@ -80,9 +80,7 @@ static uint64_t vmport_ioport_read(void *opaque, hwaddr addr,
return eax;
if (!s->func[command])
{
-#ifdef VMPORT_DEBUG
- fprintf(stderr, "vmport: unknown command %x\n", command);
-#endif
+ trace_vmport_ioport_unknown_command(command);
return eax;
}
diff --git a/trace-events b/trace-events
index 90099e6..80e8a76 100644
--- a/trace-events
+++ b/trace-events
@@ -889,6 +889,9 @@ pc87312_info_ide(uint32_t base) "base 0x%x"
pc87312_info_parallel(uint32_t base, uint32_t irq) "base 0x%x, irq %u"
pc87312_info_serial(int n, uint32_t base, uint32_t irq) "id=%d, base 0x%x, irq %u"
+# hw/misc/vmport.c
+vmport_ioport_unknown_command(unsigned char command) "%x"
+
# hw/scsi/vmw_pvscsi.c
pvscsi_ring_init_data(uint32_t txr_len_log2, uint32_t rxr_len_log2) "TX/RX rings logarithms set to %d/%d"
pvscsi_ring_init_msg(uint32_t len_log2) "MSG ring logarithm set to %d"
--
1.8.3.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [BUGFIX][PATCH v8 2/8] vmport: Fix vmport_cmd_ram_size
2015-06-23 23:39 [Qemu-devel] [PATCH v8 0/8] Add limited support of VMware's hyper-call rpc Don Slutz
2015-06-23 23:39 ` [Qemu-devel] [PATCH v8 1/8] vmport: Switch to trace Don Slutz
@ 2015-06-23 23:39 ` Don Slutz
2015-06-23 23:39 ` [Qemu-devel] [PATCH v8 3/8] MAINTAINERS: add VMware port Don Slutz
` (6 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Don Slutz @ 2015-06-23 23:39 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S. Tsirkin, Markus Armbruster, Don Slutz, Luiz Capitulino,
Don Slutz, Anthony Liguori, Paolo Bonzini, Andreas Färber,
Richard Henderson
From: Don Slutz <dslutz@verizon.com>
Based on
https://sites.google.com/site/chitchatvmback/backdoor
and testing on ESXi, this should be in MB not bytes.
Signed-off-by: Don Slutz <dslutz@verizon.com>
CC: Don Slutz <don.slutz@gmail.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
---
v8:
Added Acked-by: Paolo Bonzini
hw/misc/vmport.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/misc/vmport.c b/hw/misc/vmport.c
index 3b81d04..783827d 100644
--- a/hw/misc/vmport.c
+++ b/hw/misc/vmport.c
@@ -108,7 +108,7 @@ static uint32_t vmport_cmd_ram_size(void *opaque, uint32_t addr)
X86CPU *cpu = X86_CPU(current_cpu);
cpu->env.regs[R_EBX] = 0x1177;
- return ram_size;
+ return ram_size >> 20; /* in MB */
}
static uint32_t vmport_cmd_xtest(void *opaque, uint32_t addr)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v8 3/8] MAINTAINERS: add VMware port
2015-06-23 23:39 [Qemu-devel] [PATCH v8 0/8] Add limited support of VMware's hyper-call rpc Don Slutz
2015-06-23 23:39 ` [Qemu-devel] [PATCH v8 1/8] vmport: Switch to trace Don Slutz
2015-06-23 23:39 ` [Qemu-devel] [BUGFIX][PATCH v8 2/8] vmport: Fix vmport_cmd_ram_size Don Slutz
@ 2015-06-23 23:39 ` Don Slutz
2015-06-23 23:39 ` [Qemu-devel] [PATCH v8 4/8] vmport_rpc: Add the object vmport_rpc Don Slutz
` (5 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Don Slutz @ 2015-06-23 23:39 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S. Tsirkin, Markus Armbruster, Don Slutz, Luiz Capitulino,
Don Slutz, Anthony Liguori, Paolo Bonzini, Andreas Färber,
Richard Henderson
From: Don Slutz <don.slutz@gmail.com>
Signed-off-by: Don Slutz <don.slutz@gmail.com>
CC: Don Slutz <dslutz@verizon.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
---
v8:
Added Acked-by: Paolo Bonzini
v7:
Switched e-mail address in MAINTAINERS.
MAINTAINERS | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 3639173..1872a9c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -781,6 +781,13 @@ M: Jiri Pirko <jiri@resnulli.us>
S: Maintained
F: hw/net/rocker/
+VMware port
+M: Don Slutz <Don.Slutz@Gmail.com>
+S: Maintained
+F: hw/misc/vmport.c
+F: hw/input/vmmouse.c
+F: hw/misc/vmport_rpc.c
+
Subsystems
----------
Audio
--
1.8.3.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v8 4/8] vmport_rpc: Add the object vmport_rpc
2015-06-23 23:39 [Qemu-devel] [PATCH v8 0/8] Add limited support of VMware's hyper-call rpc Don Slutz
` (2 preceding siblings ...)
2015-06-23 23:39 ` [Qemu-devel] [PATCH v8 3/8] MAINTAINERS: add VMware port Don Slutz
@ 2015-06-23 23:39 ` Don Slutz
2015-06-23 23:39 ` [Qemu-devel] [PATCH v8 5/8] vmport_rpc: Add limited support of VMware's hyper-call rpc Don Slutz
` (4 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Don Slutz @ 2015-06-23 23:39 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S. Tsirkin, Markus Armbruster, Don Slutz, Luiz Capitulino,
Don Slutz, Anthony Liguori, Paolo Bonzini, Andreas Färber,
Richard Henderson
From: Don Slutz <dslutz@verizon.com>
This is the 1st part of "Add limited support of VMware's hyper-call
rpc".
This patch uses existing infrastructure used by vmmouse.c (provided
by vmport.c) to handle the VMware backdoor command 30.
One of the better on-line references is:
https://sites.google.com/site/chitchatvmback/backdoor
More in next patch.
Signed-off-by: Don Slutz <dslutz@verizon.com>
CC: Don Slutz <don.slutz@gmail.com>
---
v8:
Dropped code to always add the new device.
Add code to check for vmport enabled.
hw/misc/Makefile.objs | 1 +
hw/misc/vmport.c | 8 ++--
hw/misc/vmport_rpc.c | 130 ++++++++++++++++++++++++++++++++++++++++++++++++++
include/hw/i386/pc.h | 2 +-
4 files changed, 137 insertions(+), 4 deletions(-)
create mode 100644 hw/misc/vmport_rpc.c
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 4aa76ff..e04c8ac 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -7,6 +7,7 @@ common-obj-$(CONFIG_ISA_TESTDEV) += pc-testdev.o
common-obj-$(CONFIG_PCI_TESTDEV) += pci-testdev.o
obj-$(CONFIG_VMPORT) += vmport.o
+obj-$(CONFIG_VMPORT) += vmport_rpc.o
# ARM devices
common-obj-$(CONFIG_PL310) += arm_l2x0.o
diff --git a/hw/misc/vmport.c b/hw/misc/vmport.c
index 783827d..6794ecf 100644
--- a/hw/misc/vmport.c
+++ b/hw/misc/vmport.c
@@ -50,13 +50,15 @@ typedef struct VMPortState
static VMPortState *port_state;
-void vmport_register(unsigned char command, VMPortReadFunc *func, void *opaque)
+bool vmport_register(unsigned char command, VMPortReadFunc *func, void *opaque)
{
- if (command >= VMPORT_ENTRIES)
- return;
+ if (command >= VMPORT_ENTRIES || !port_state) {
+ return false;
+ }
port_state->func[command] = func;
port_state->opaque[command] = opaque;
+ return true;
}
static uint64_t vmport_ioport_read(void *opaque, hwaddr addr,
diff --git a/hw/misc/vmport_rpc.c b/hw/misc/vmport_rpc.c
new file mode 100644
index 0000000..3b1baea
--- /dev/null
+++ b/hw/misc/vmport_rpc.c
@@ -0,0 +1,130 @@
+/*
+ * QEMU VMPORT RPC emulation
+ *
+ * Copyright (C) 2015 Verizon Corporation
+ *
+ * This file is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License Version 2 (GPLv2)
+ * as published by the Free Software Foundation.
+ *
+ * This file is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details. <http://www.gnu.org/licenses/>.
+ */
+
+/*
+ * One of the better on-line references is:
+ *
+ * https://sites.google.com/site/chitchatvmback/backdoor
+ *
+ * Which points you to:
+ *
+ * http://open-vm-tools.sourceforge.net/
+ *
+ * as a place to get more accurate information by studying.
+ */
+
+#include "hw/hw.h"
+#include "hw/i386/pc.h"
+#include "hw/qdev.h"
+#include "trace.h"
+#include "qmp-commands.h"
+#include "qapi/qmp/qerror.h"
+
+/* #define VMPORT_RPC_DEBUG */
+
+#define TYPE_VMPORT_RPC "vmport_rpc"
+#define VMPORT_RPC(obj) OBJECT_CHECK(VMPortRpcState, (obj), TYPE_VMPORT_RPC)
+
+/* VMPORT RPC Command */
+#define VMPORT_RPC_COMMAND 30
+
+/* The vmport_rpc object. */
+typedef struct VMPortRpcState {
+ ISADevice parent_obj;
+
+ /* Properties */
+ uint64_t reset_time;
+ uint64_t build_number_value;
+ uint64_t build_number_time;
+
+ /* Private data */
+} VMPortRpcState;
+
+typedef struct {
+ uint32_t eax;
+ uint32_t ebx;
+ uint32_t ecx;
+ uint32_t edx;
+ uint32_t esi;
+ uint32_t edi;
+} vregs;
+
+static uint32_t vmport_rpc_ioport_read(void *opaque, uint32_t addr)
+{
+ VMPortRpcState *s = opaque;
+ union {
+ uint32_t data[6];
+ vregs regs;
+ } ur;
+
+ vmmouse_get_data(ur.data);
+
+ s->build_number_time++;
+
+ vmmouse_set_data(ur.data);
+ return ur.data[0];
+}
+
+static void vmport_rpc_reset(DeviceState *d)
+{
+ VMPortRpcState *s = VMPORT_RPC(d);
+
+ s->reset_time = 14;
+ s->build_number_value = 0;
+ s->build_number_time = 0;
+}
+
+static void vmport_rpc_realize(DeviceState *dev, Error **errp)
+{
+ VMPortRpcState *s = VMPORT_RPC(dev);
+
+ if (!vmport_register(VMPORT_RPC_COMMAND, vmport_rpc_ioport_read, s)) {
+ error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+ "vmport_rpc needs vmport enabled");
+ }
+}
+
+static Property vmport_rpc_properties[] = {
+ DEFINE_PROP_UINT64("reset-time", VMPortRpcState, reset_time, 14),
+ DEFINE_PROP_UINT64("build-number-value", VMPortRpcState,
+ build_number_value, 0),
+ DEFINE_PROP_UINT64("build-number-time", VMPortRpcState,
+ build_number_time, 0),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static void vmport_rpc_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->realize = vmport_rpc_realize;
+ dc->reset = vmport_rpc_reset;
+ dc->desc = "Enable VMware's hyper-call rpc";
+ dc->props = vmport_rpc_properties;
+}
+
+static const TypeInfo vmport_rpc_info = {
+ .name = TYPE_VMPORT_RPC,
+ .parent = TYPE_ISA_DEVICE,
+ .instance_size = sizeof(VMPortRpcState),
+ .class_init = vmport_rpc_class_init,
+};
+
+static void vmport_rpc_register_types(void)
+{
+ type_register_static(&vmport_rpc_info);
+}
+
+type_init(vmport_rpc_register_types)
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 86c5651..3270a97 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -142,7 +142,7 @@ static inline void vmport_init(ISABus *bus)
isa_create_simple(bus, "vmport");
}
-void vmport_register(unsigned char command, VMPortReadFunc *func, void *opaque);
+bool vmport_register(unsigned char command, VMPortReadFunc *func, void *opaque);
void vmmouse_get_data(uint32_t *data);
void vmmouse_set_data(const uint32_t *data);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v8 5/8] vmport_rpc: Add limited support of VMware's hyper-call rpc
2015-06-23 23:39 [Qemu-devel] [PATCH v8 0/8] Add limited support of VMware's hyper-call rpc Don Slutz
` (3 preceding siblings ...)
2015-06-23 23:39 ` [Qemu-devel] [PATCH v8 4/8] vmport_rpc: Add the object vmport_rpc Don Slutz
@ 2015-06-23 23:39 ` Don Slutz
2015-06-23 23:39 ` [Qemu-devel] [PATCH v8 7/8] vmport_rpc: Add migration Don Slutz
` (3 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Don Slutz @ 2015-06-23 23:39 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S. Tsirkin, Markus Armbruster, Don Slutz, Luiz Capitulino,
Don Slutz, Anthony Liguori, Paolo Bonzini, Andreas Färber,
Richard Henderson
From: Don Slutz <dslutz@verizon.com>
To enable you must add "-device vmport_rpc" to command line.
The support included is enough to allow VMware tools to install in a
guest and provide guestinfo support. guestinfo support is provided
by what is known as VMware RPC support.
If the guest is running VMware tools, then the "build version" of
the tools is also available via the property build-number-value of
the vmport_rpc object. The build-number-time property is changed
every time the VMware tools running in the guest sends the
"build version" via the rpc.
The property reset-time controls how often to request them in
seconds minus 1. The minus 1 is to handle to 0 case. I.E. the
fastest that can be selected is every second. The default is 4
times a minute.
The VMware RPC support includes the notion of channels that are
opened, active and closed. All RPC messages sent via a channel
starts with normal ASCII text. The message some times does include
binary data.
Currently there are 2 protocols defined for VMware RPC. They
determine the direction for data flow, guest to tool stack or tool
stack to guest.
There is no provided interrupt for VMware RPC.
Getting VMPORT_RPC_DEBUG will provide a higher level trace calls
that are simpler to understand.
Signed-off-by: Don Slutz <dslutz@verizon.com>
CC: Don Slutz <don.slutz@gmail.com>
---
hw/misc/vmport_rpc.c | 797 ++++++++++++++++++++++++++++++++++++++++++++++++++-
trace-events | 22 ++
2 files changed, 818 insertions(+), 1 deletion(-)
diff --git a/hw/misc/vmport_rpc.c b/hw/misc/vmport_rpc.c
index 3b1baea..f157425 100644
--- a/hw/misc/vmport_rpc.c
+++ b/hw/misc/vmport_rpc.c
@@ -40,6 +40,123 @@
/* VMPORT RPC Command */
#define VMPORT_RPC_COMMAND 30
+/* Limits on amount of non guest memory to use */
+#define MAX_KEY_LEN 128
+#define MIN_VAL_LEN 64
+#define MAX_VAL_LEN 8192
+#define MAX_NUM_KEY 256
+#define MAX_BKTS 4
+/* Max number of channels. */
+#define GUESTMSG_MAX_CHANNEL 8
+
+/*
+ * All of VMware's rpc is based on 32bit registers. So this is the
+ * number of bytes in ebx.
+ */
+#define CHAR_PER_CALL sizeof(uint32_t)
+/* Room for basic commands */
+#define EXTRA_SEND 22
+/* Status code and NULL */
+#define EXTRA_RECV 2
+#define MAX_SEND_BUF DIV_ROUND_UP(EXTRA_SEND + MAX_KEY_LEN + MAX_VAL_LEN, \
+ CHAR_PER_CALL)
+#define MAX_RECV_BUF DIV_ROUND_UP(EXTRA_RECV + MAX_VAL_LEN, CHAR_PER_CALL)
+#define MIN_SEND_BUF DIV_ROUND_UP(EXTRA_SEND + MAX_KEY_LEN + MIN_VAL_LEN, \
+ CHAR_PER_CALL)
+
+/* Reply statuses */
+/* The basic request succeeded */
+#define MESSAGE_STATUS_SUCCESS 0x0001
+/* vmware has a message available for its party */
+#define MESSAGE_STATUS_DORECV 0x0002
+/* The channel has been closed */
+#define MESSAGE_STATUS_CLOSED 0x0004
+/* vmware removed the message before the party fetched it */
+#define MESSAGE_STATUS_UNSENT 0x0008
+/* A checkpoint occurred */
+#define MESSAGE_STATUS_CPT 0x0010
+/* An underlying device is powering off */
+#define MESSAGE_STATUS_POWEROFF 0x0020
+/* vmware has detected a timeout on the channel */
+#define MESSAGE_STATUS_TIMEOUT 0x0040
+/* vmware supports high-bandwidth for sending and receiving the payload */
+#define MESSAGE_STATUS_HB 0x0080
+
+/* Max number of channels. */
+#define GUESTMSG_MAX_CHANNEL 8
+
+/* Flags to open a channel. */
+#define GUESTMSG_FLAG_COOKIE 0x80000000
+
+/* Data to guest */
+#define VMWARE_PROTO_TO_GUEST 0x4f4c4354
+/* Data from guest */
+#define VMWARE_PROTO_FROM_GUEST 0x49435052
+
+/*
+ * Error return values used only in this file. The routine
+ * convert_local_rc() is used to convert these to an Error
+ * object.
+ */
+#define VMPORT_DEVICE_NOT_FOUND -1
+#define SEND_NOT_OPEN -2
+#define SEND_SKIPPED -3
+#define SEND_TRUCATED -4
+#define SEND_NO_MEMORY -5
+#define GUESTINFO_NOTFOUND -6
+#define GUESTINFO_VALTOOLONG -7
+#define GUESTINFO_KEYTOOLONG -8
+#define GUESTINFO_TOOMANYKEYS -9
+#define GUESTINFO_NO_MEMORY -10
+
+
+/* The VMware RPC guest info storage . */
+typedef struct {
+ char *val_data;
+ uint16_t val_len;
+ uint16_t val_max;
+} guestinfo_t;
+
+/* The VMware RPC bucket control. */
+typedef struct {
+ uint16_t recv_len;
+ uint16_t recv_idx;
+ uint16_t recv_buf_max;
+} bucket_control_t;
+
+/* The VMware RPC bucket info. */
+typedef struct {
+ union {
+ uint32_t *words;
+ char *bytes;
+ } recv;
+ bucket_control_t ctl;
+} bucket_t;
+
+
+/* The VMware RPC channel control. */
+typedef struct {
+ uint64_t active_time;
+ uint32_t chan_id;
+ uint32_t cookie;
+ uint32_t proto_num;
+ uint16_t send_len;
+ uint16_t send_idx;
+ uint16_t send_buf_max;
+ uint8_t recv_read;
+ uint8_t recv_write;
+} channel_control_t;
+
+/* The VMware RPC channel info. */
+typedef struct {
+ union {
+ uint32_t *words;
+ char *bytes;
+ } send;
+ channel_control_t ctl;
+ bucket_t recv_bkt[MAX_BKTS];
+} channel_t;
+
/* The vmport_rpc object. */
typedef struct VMPortRpcState {
ISADevice parent_obj;
@@ -50,8 +167,32 @@ typedef struct VMPortRpcState {
uint64_t build_number_time;
/* Private data */
+ uint64_t ping_time;
+ uint32_t open_cookie;
+ channel_t chans[GUESTMSG_MAX_CHANNEL];
+ GHashTable *guestinfo;
+#ifdef VMPORT_RPC_DEBUG
+ unsigned int end;
+ unsigned int last;
+ char out[2048];
+#endif
} VMPortRpcState;
+/* Basic request types */
+typedef enum {
+ MESSAGE_TYPE_OPEN,
+ MESSAGE_TYPE_SENDSIZE,
+ MESSAGE_TYPE_SENDPAYLOAD,
+ MESSAGE_TYPE_RECVSIZE,
+ MESSAGE_TYPE_RECVPAYLOAD,
+ MESSAGE_TYPE_RECVSTATUS,
+ MESSAGE_TYPE_CLOSE,
+} MessageType;
+
+/*
+ * Overlay on the array that vmmouse_get_data() returns. The code is
+ * easier to read using register names.
+ */
typedef struct {
uint32_t eax;
uint32_t ebx;
@@ -61,6 +202,636 @@ typedef struct {
uint32_t edi;
} vregs;
+#ifdef VMPORT_RPC_DEBUG
+/*
+ * Add helper function for tracing. This routine will convert
+ * binary data into more normal characters so that the trace data is
+ * earier to read and will not have nulls in it.
+ */
+static void vmport_rpc_safe_print(VMPortRpcState *s, int len, const char *msg)
+{
+ unsigned char c;
+ unsigned int i, k;
+
+ s->end = len;
+ /* 1 byte can cnvert to 3 bytes, and save room at the end. */
+ if (s->end > (sizeof(s->out) / 3 - 6)) {
+ s->end = sizeof(s->out) / 3 - 6;
+ }
+ s->out[0] = '<';
+ k = 1;
+ for (i = 0; i < s->end; ++i) {
+ c = msg[i];
+ if ((c == '^') || (c == '\\') || (c == '>')) {
+ s->out[k++] = '\\';
+ s->out[k++] = c;
+ } else if ((c >= ' ') && (c <= '~')) {
+ s->out[k++] = c;
+ } else if (c < ' ') {
+ s->out[k++] = '^';
+ s->out[k++] = c ^ 0x40;
+ } else {
+ snprintf(&s->out[k], sizeof(s->out) - k, "\\%02x", c);
+ k += 3;
+ }
+ }
+ s->out[k++] = '>';
+ if (len > s->end) {
+ s->out[k++] = '.';
+ s->out[k++] = '.';
+ s->out[k++] = '.';
+ }
+ s->out[k++] = 0;
+ s->last = k;
+}
+#endif
+
+/*
+ * Copy message into a free receive bucket buffer which vmtools will
+ * use to read from 4 (CHAR_PER_CALL) bytes at a time until done
+ * with it.
+ */
+static int vmport_rpc_send(VMPortRpcState *s, channel_t *c,
+ const char *msg, unsigned int cur_recv_len)
+{
+ int rc;
+ unsigned int my_bkt = c->ctl.recv_write;
+ unsigned int next_bkt = my_bkt + 1;
+ bucket_t *b;
+
+ if (next_bkt >= MAX_BKTS) {
+ next_bkt = 0;
+ }
+
+ if (next_bkt == c->ctl.recv_read) {
+#ifdef VMPORT_RPC_DEBUG
+ {
+ char prefix[30];
+
+ snprintf(prefix, sizeof(prefix),
+ "VMware _send skipped %d (%d, %d) ",
+ c->ctl.chan_id, my_bkt, c->ctl.recv_read);
+ prefix[sizeof(prefix) - 1] = 0;
+ vmport_rpc_safe_print(s, cur_recv_len, msg);
+ trace_vmport_rpc_send_skip(prefix, s->end, cur_recv_len, s->last,
+ sizeof(s->out), s->out);
+ }
+#endif
+ return SEND_SKIPPED;
+ }
+
+ c->ctl.recv_write = next_bkt;
+ b = &c->recv_bkt[my_bkt];
+#ifdef VMPORT_RPC_DEBUG
+ {
+ char prefix[30];
+
+ snprintf(prefix, sizeof(prefix), "VMware _send %d (%d) ",
+ c->ctl.chan_id, my_bkt);
+ prefix[sizeof(prefix) - 1] = 0;
+ vmport_rpc_safe_print(s, cur_recv_len, msg);
+ trace_vmport_rpc_send_normal(prefix, s->end, cur_recv_len, s->last,
+ sizeof(s->out), s->out);
+ }
+#endif
+
+ if (b->ctl.recv_buf_max < MAX_RECV_BUF) {
+ size_t new_recv_buf_max = DIV_ROUND_UP(cur_recv_len, CHAR_PER_CALL);
+
+ if (new_recv_buf_max > b->ctl.recv_buf_max) {
+ uint32_t *new_recv_buf =
+ g_try_malloc((new_recv_buf_max + 1) * CHAR_PER_CALL);
+
+ if (new_recv_buf) {
+ g_free(b->recv.words);
+ b->recv.words = new_recv_buf;
+ b->ctl.recv_buf_max = new_recv_buf_max;
+ }
+ }
+ }
+ if (!b->recv.words) {
+ return SEND_NO_MEMORY;
+ }
+ b->ctl.recv_len = cur_recv_len;
+ b->ctl.recv_idx = 0;
+ rc = 0;
+ if (cur_recv_len > (b->ctl.recv_buf_max * CHAR_PER_CALL)) {
+ trace_vmport_rpc_send_big(cur_recv_len,
+ b->ctl.recv_buf_max * CHAR_PER_CALL);
+ cur_recv_len = b->ctl.recv_buf_max * CHAR_PER_CALL;
+ b->recv.words[b->ctl.recv_buf_max] = 0;
+ rc = SEND_TRUCATED;
+ } else {
+ b->recv.words[cur_recv_len / CHAR_PER_CALL] = 0;
+ b->recv.words[DIV_ROUND_UP(cur_recv_len, CHAR_PER_CALL)] = 0;
+ }
+ memcpy(b->recv.words, msg, cur_recv_len);
+ return rc;
+}
+
+static int vmport_rpc_ctrl_send(VMPortRpcState *s, char *msg)
+{
+ int rc = SEND_NOT_OPEN;
+ unsigned int i;
+
+ if (!s) {
+ return rc;
+ }
+ s->ping_time = get_clock() / 1000000000LL;
+ for (i = 0; i < GUESTMSG_MAX_CHANNEL; ++i) {
+ if (s->chans[i].ctl.proto_num == VMWARE_PROTO_TO_GUEST) {
+ rc = vmport_rpc_send(s, &s->chans[i], msg, strlen(msg) + 1);
+ }
+ }
+ return rc;
+}
+
+static void vmport_rpc_sweep(VMPortRpcState *s, unsigned long now_time)
+{
+ unsigned int i;
+
+ for (i = 0; i < GUESTMSG_MAX_CHANNEL; ++i) {
+ if (s->chans[i].ctl.proto_num) {
+ channel_t *c = &s->chans[i];
+ long delta = now_time - c->ctl.active_time;
+
+ if (delta >= 80) {
+ trace_vmport_rpc_sweep(c->ctl.chan_id, delta);
+ /* Return channel to free pool */
+ c->ctl.proto_num = 0;
+ }
+ }
+ }
+}
+
+static channel_t *vmport_rpc_new_chan(VMPortRpcState *s, unsigned long now_time)
+{
+ unsigned int i;
+
+ for (i = 0; i < GUESTMSG_MAX_CHANNEL; ++i) {
+ if (!s->chans[i].ctl.proto_num) {
+ channel_t *c = &s->chans[i];
+
+ c->ctl.chan_id = i;
+ c->ctl.cookie = s->open_cookie++;
+ c->ctl.active_time = now_time;
+ c->ctl.send_len = 0;
+ c->ctl.send_idx = 0;
+ c->ctl.recv_read = 0;
+ c->ctl.recv_write = 0;
+ if (!c->send.words) {
+ uint32_t *new_send_buf =
+ g_try_malloc0((MIN_SEND_BUF + 1) * CHAR_PER_CALL);
+ if (new_send_buf) {
+ c->send.words = new_send_buf;
+ c->ctl.send_buf_max = MIN_SEND_BUF;
+ }
+ }
+ if (!c->send.words) {
+ return NULL;
+ }
+ return c;
+ }
+ }
+ return NULL;
+}
+
+static void process_send_size(VMPortRpcState *s, channel_t *c,
+ vregs *ur)
+{
+ /* vmware tools often send a 0 byte request size. */
+ c->ctl.send_len = ur->ebx;
+ c->ctl.send_idx = 0;
+
+ if (c->ctl.send_buf_max < MAX_SEND_BUF) {
+ size_t new_send_max = DIV_ROUND_UP(c->ctl.send_len, CHAR_PER_CALL);
+
+ if (new_send_max > c->ctl.send_buf_max) {
+ uint32_t *new_send_buf =
+ g_try_malloc0((new_send_max + 1) * CHAR_PER_CALL);
+
+ if (new_send_buf) {
+ g_free(c->send.words);
+ c->send.words = new_send_buf;
+ c->ctl.send_buf_max = new_send_max;
+ }
+ }
+ }
+ ur->ecx = (MESSAGE_STATUS_SUCCESS << 16) | (ur->ecx & 0xffff);
+ trace_vmport_detail_rpc_process_send_size(c->ctl.chan_id,
+ c->ctl.send_len);
+}
+
+/* ret_buffer is in/out param */
+static int get_guestinfo(VMPortRpcState *s,
+ char *a_info_key, unsigned int a_key_len,
+ char *ret_buffer, unsigned int ret_buffer_len)
+{
+ guestinfo_t *gi = NULL;
+
+ trace_vmport_rpc_get_guestinfo(a_key_len, a_key_len, a_info_key);
+
+ if (a_key_len <= MAX_KEY_LEN) {
+ gpointer key = g_strndup(a_info_key, a_key_len);
+
+ gi = (guestinfo_t *)g_hash_table_lookup(s->guestinfo, key);
+ g_free(key);
+ } else {
+ return GUESTINFO_KEYTOOLONG;
+ }
+ if (gi) {
+ unsigned int ret_len = 2 + gi->val_len;
+
+ trace_vmport_rpc_get_guestinfo_found(gi->val_len, gi->val_len,
+ gi->val_data);
+
+ ret_buffer[0] = '1';
+ ret_buffer[1] = ' ';
+ if (ret_len > ret_buffer_len - 1) {
+ ret_len = ret_buffer_len - 1;
+ }
+ memcpy(ret_buffer + 2, gi->val_data, ret_len);
+ return ret_len;
+ }
+
+ return GUESTINFO_NOTFOUND;
+}
+
+static int set_guestinfo(VMPortRpcState *s, int a_key_len,
+ unsigned int a_val_len, const char *a_info_key,
+ char *val)
+{
+ gpointer key = NULL;
+ int rc = 0;
+
+ trace_vmport_rpc_set_guestinfo(a_key_len, a_key_len, a_info_key,
+ a_val_len, a_val_len, val);
+
+ if (a_key_len <= MAX_KEY_LEN) {
+ guestinfo_t *gi;
+
+ key = g_strndup(a_info_key, a_key_len);
+ gi = (guestinfo_t *)g_hash_table_lookup(s->guestinfo, key);
+ if (a_val_len <= MAX_VAL_LEN) {
+ if (gi) {
+ if (a_val_len > gi->val_max) {
+ char *new_val = g_try_malloc0(a_val_len);
+
+ if (!new_val) {
+ g_free(key);
+ return GUESTINFO_NO_MEMORY;
+ }
+ g_free(gi->val_data);
+ gi->val_max = a_val_len;
+ gi->val_data = new_val;
+ }
+ gi->val_len = a_val_len;
+ memcpy(gi->val_data, val, a_val_len);
+ } else {
+ int new_val_len = a_val_len;
+
+ if (new_val_len < MIN_VAL_LEN) {
+ new_val_len = MIN_VAL_LEN;
+ }
+ if (g_hash_table_size(s->guestinfo) >= MAX_NUM_KEY) {
+ g_free(key);
+ return GUESTINFO_TOOMANYKEYS;
+ }
+ gi = g_try_malloc(sizeof(guestinfo_t));
+ if (!gi) {
+ g_free(key);
+ return GUESTINFO_NO_MEMORY;
+ }
+ gi->val_data = g_try_malloc0(new_val_len);
+ if (!gi->val_data) {
+ g_free(gi);
+ g_free(key);
+ return GUESTINFO_NO_MEMORY;
+ }
+ gi->val_len = a_val_len;
+ gi->val_max = new_val_len;
+ memcpy(gi->val_data, val, a_val_len);
+ g_hash_table_insert(s->guestinfo, key, gi);
+ key = NULL; /* Do not free key below */
+ }
+ } else {
+ rc = GUESTINFO_VALTOOLONG;
+ }
+ } else {
+ rc = GUESTINFO_KEYTOOLONG;
+ }
+ g_free(key);
+ return rc;
+}
+
+static void process_send_payload(VMPortRpcState *s,
+ channel_t *c,
+ vregs *ur,
+ unsigned long now_time)
+{
+ /* Accumulate 4 (CHAR_PER_CALL) bytes of paload into send_buf
+ * using offset */
+ if (c->ctl.send_idx < c->ctl.send_buf_max) {
+ c->send.words[c->ctl.send_idx] = ur->ebx;
+ }
+
+ c->ctl.send_idx++;
+ ur->ecx = (MESSAGE_STATUS_SUCCESS << 16) | (ur->ecx & 0xffff);
+
+ if (c->ctl.send_idx * CHAR_PER_CALL >= c->ctl.send_len) {
+
+ /* We are done accumulating so handle the command */
+
+ if (c->ctl.send_idx <= c->ctl.send_buf_max) {
+ c->send.words[c->ctl.send_idx] = 0;
+ }
+#ifdef VMPORT_RPC_DEBUG
+ {
+ char prefix[30];
+
+ snprintf(prefix, sizeof(prefix),
+ "VMware RECV %d (%d) ",
+ c->ctl.chan_id, c->ctl.recv_read);
+ prefix[sizeof(prefix) - 1] = 0;
+ vmport_rpc_safe_print(s, MIN(c->ctl.send_len,
+ c->ctl.send_buf_max * CHAR_PER_CALL),
+ c->send.bytes);
+ trace_vmport_rpc_recv_normal(prefix, s->end, c->ctl.send_len,
+ s->last, sizeof(s->out), s->out);
+ }
+#endif
+ if (c->ctl.proto_num == VMWARE_PROTO_FROM_GUEST) {
+ /*
+ * Eaxmples of messages:
+ *
+ * log toolbox: Version: build-341836
+ * SetGuestInfo 4 build-341836
+ * info-get guestinfo.ip
+ * info-set guestinfo.ip joe
+ *
+ */
+
+ char *build = NULL;
+ char *info_key = NULL;
+ char *ret_msg = (char *)"1 ";
+ char ret_buffer[2 + MAX_VAL_LEN + 2];
+ unsigned int ret_len = strlen(ret_msg) + 1;
+
+ if (strncmp(c->send.bytes, "log toolbox: Version: build-",
+ strlen("log toolbox: Version: build-")) == 0) {
+ build =
+ c->send.bytes + strlen("log toolbox: Version: build-");
+ } else if (strncmp(c->send.bytes, "SetGuestInfo 4 build-",
+ strlen("SetGuestInfo 4 build-")) == 0) {
+ build = c->send.bytes + strlen("SetGuestInfo 4 build-");
+ } else if (strncmp(c->send.bytes, "info-get guestinfo.",
+ strlen("info-get guestinfo.")) == 0) {
+ unsigned int a_key_len =
+ c->ctl.send_len - strlen("info-get guestinfo.");
+ int rc;
+
+ info_key = c->send.bytes + strlen("info-get guestinfo.");
+ if (a_key_len <= MAX_KEY_LEN) {
+
+ rc = get_guestinfo(s, info_key, a_key_len,
+ ret_buffer, sizeof(ret_buffer));
+ if (rc == GUESTINFO_NOTFOUND) {
+ ret_msg = (char *)"0 No value found";
+ ret_len = strlen(ret_msg) + 1;
+ } else {
+ ret_msg = ret_buffer;
+ ret_len = rc;
+ }
+ } else {
+ ret_msg = (char *)"0 Key is too long";
+ ret_len = strlen(ret_msg) + 1;
+ }
+ } else if (strncmp(c->send.bytes, "info-set guestinfo.",
+ strlen("info-set guestinfo.")) == 0) {
+ char *val;
+ unsigned int rest_len =
+ c->ctl.send_len - strlen("info-set guestinfo.");
+
+ info_key = c->send.bytes + strlen("info-set guestinfo.");
+ val = strstr(info_key, " ");
+ if (val) {
+ unsigned int a_key_len = val - info_key;
+ unsigned int a_val_len = rest_len - a_key_len - 1;
+ int rc;
+
+ val++;
+ rc = set_guestinfo(s, a_key_len, a_val_len,
+ info_key, val);
+ switch (rc) {
+ case 0:
+ ret_msg = (char *)"1 ";
+ break;
+ case GUESTINFO_VALTOOLONG:
+ ret_msg = (char *)"0 Value too long";
+ break;
+ case GUESTINFO_KEYTOOLONG:
+ ret_msg = (char *)"0 Key is too long";
+ break;
+ case GUESTINFO_TOOMANYKEYS:
+ ret_msg = (char *)"0 Too many keys";
+ break;
+ case GUESTINFO_NO_MEMORY:
+ ret_msg = (char *)"0 Out of memory";
+ break;
+ }
+ ret_len = strlen(ret_msg) + 1;
+ } else {
+ ret_msg =
+ (char *)"0 Two and exactly two arguments expected";
+ ret_len = strlen(ret_msg) + 1;
+ }
+ }
+
+ vmport_rpc_send(s, c, ret_msg, ret_len);
+
+ if (build) {
+ s->build_number_value = strtol(build, NULL, 10);
+ s->build_number_time = now_time;
+ }
+ } else {
+ unsigned int my_bkt = c->ctl.recv_read - 1;
+ bucket_t *b;
+
+ if (my_bkt >= MAX_BKTS) {
+ my_bkt = MAX_BKTS - 1;
+ }
+ b = &c->recv_bkt[my_bkt];
+ b->ctl.recv_len = 0;
+ }
+ }
+}
+
+static void process_recv_size(VMPortRpcState *s, channel_t *c,
+ vregs *ur)
+{
+ bucket_t *b;
+ int16_t recv_len;
+
+ b = &c->recv_bkt[c->ctl.recv_read];
+ recv_len = b->ctl.recv_len;
+ if (recv_len) {
+ ur->ecx = ((MESSAGE_STATUS_DORECV | MESSAGE_STATUS_SUCCESS) << 16) |
+ (ur->ecx & 0xffff);
+ ur->edx = (ur->edx & 0xffff) | (MESSAGE_TYPE_SENDSIZE << 16);
+ ur->ebx = recv_len;
+ } else {
+ ur->ecx = (MESSAGE_STATUS_SUCCESS << 16) | (ur->ecx & 0xffff);
+ }
+ trace_vmport_detail_rpc_process_recv_size(c->ctl.chan_id, recv_len);
+}
+
+static void process_recv_payload(VMPortRpcState *s,
+ channel_t *c,
+ vregs *ur)
+{
+ bucket_t *b;
+
+ b = &c->recv_bkt[c->ctl.recv_read];
+ if (b->ctl.recv_idx < b->ctl.recv_buf_max) {
+ ur->ebx = b->recv.words[b->ctl.recv_idx++];
+ } else {
+ ur->ebx = 0;
+ }
+ ur->ecx = (MESSAGE_STATUS_SUCCESS << 16) | (ur->ecx & 0xffff);
+ ur->edx = (ur->edx & 0xffff) | (MESSAGE_TYPE_SENDPAYLOAD << 16);
+}
+
+static void process_recv_status(VMPortRpcState *s,
+ channel_t *c,
+ vregs *ur)
+{
+ ur->ecx = (MESSAGE_STATUS_SUCCESS << 16) | (ur->ecx & 0xffff);
+ c->ctl.recv_read++;
+ if (c->ctl.recv_read >= MAX_BKTS) {
+ c->ctl.recv_read = 0;
+ }
+}
+
+static void process_close(VMPortRpcState *s, channel_t *c,
+ vregs *ur)
+{
+ /* Return channel to free pool */
+ c->ctl.proto_num = 0;
+ ur->ecx = (MESSAGE_STATUS_SUCCESS << 16) | (ur->ecx & 0xffff);
+ trace_vmport_rpc_process_close(c->ctl.chan_id);
+}
+
+static void process_packet(VMPortRpcState *s, channel_t *c,
+ vregs *ur, unsigned int sub_cmd,
+ unsigned long now_time)
+{
+ c->ctl.active_time = now_time;
+
+ switch (sub_cmd) {
+ case MESSAGE_TYPE_SENDSIZE:
+ process_send_size(s, c, ur);
+ break;
+
+ case MESSAGE_TYPE_SENDPAYLOAD:
+ process_send_payload(s, c, ur, now_time);
+ break;
+
+ case MESSAGE_TYPE_RECVSIZE:
+ process_recv_size(s, c, ur);
+ break;
+
+ case MESSAGE_TYPE_RECVPAYLOAD:
+ process_recv_payload(s, c, ur);
+ break;
+
+ case MESSAGE_TYPE_RECVSTATUS:
+ process_recv_status(s, c, ur);
+ break;
+
+ case MESSAGE_TYPE_CLOSE:
+ process_close(s, c, ur);
+ break;
+
+ default:
+ ur->ecx = 0;
+ break;
+ }
+}
+
+static void vmport_rpc(VMPortRpcState *s , vregs *ur)
+{
+ unsigned int sub_cmd = (ur->ecx >> 16) & 0xffff;
+ channel_t *c = NULL;
+ uint16_t msg_id;
+ uint32_t msg_cookie;
+ unsigned long now_time = get_clock() / 1000000000LL;
+ long delta = now_time - s->ping_time;
+
+ trace_vmport_detail_rpc_start(sub_cmd, ur->eax, ur->ebx, ur->ecx, ur->edx,
+ ur->esi, ur->edi);
+
+ if (!s) {
+ return;
+ }
+ if (delta > s->reset_time) {
+ trace_vmport_rpc_ping(delta);
+ vmport_rpc_ctrl_send(s, (char *)"reset");
+ }
+ vmport_rpc_sweep(s, now_time);
+ do {
+ /* Check to see if a new open request is happening... */
+ if (MESSAGE_TYPE_OPEN == sub_cmd) {
+ c = vmport_rpc_new_chan(s, now_time);
+ if (!c) {
+ trace_vmport_rpc_nofree();
+ break;
+ }
+
+ /* Attach the apropriate protocol the the channel */
+ c->ctl.proto_num = ur->ebx & ~GUESTMSG_FLAG_COOKIE;
+ ur->ecx = (MESSAGE_STATUS_SUCCESS << 16) | (ur->ecx & 0xffff);
+ ur->edx = (ur->edx & 0xffff) | (c->ctl.chan_id << 16);
+ ur->edi = c->ctl.cookie & 0xffff;
+ ur->esi = (c->ctl.cookie >> 16) & 0xffff;
+ trace_vmport_rpc_process_open(c->ctl.chan_id, c->ctl.proto_num);
+ if (c->ctl.proto_num == VMWARE_PROTO_TO_GUEST) {
+ vmport_rpc_send(s, c, "reset", strlen("reset") + 1);
+ }
+ break;
+ }
+
+ msg_id = (ur->edx >> 16) & 0xffff;
+ msg_cookie = (ur->edi & 0xffff) | (ur->esi << 16);
+ if (msg_id >= GUESTMSG_MAX_CHANNEL) {
+ trace_vmport_rpc_bad_chan(msg_id, GUESTMSG_MAX_CHANNEL);
+ break;
+ }
+ c = &s->chans[msg_id];
+ if (!c->ctl.proto_num) {
+ trace_vmport_rpc_chan_not_open(msg_id);
+ break;
+ }
+
+ /* We check the cookie here since it's possible that the
+ * connection timed out on us and another channel was opened
+ * if this happens, return error and the vmware tool will
+ * need to reopen the connection
+ */
+ if (msg_cookie != c->ctl.cookie) {
+ trace_vmport_rpc_bad_cookie(msg_cookie, c->ctl.cookie);
+ break;
+ }
+ process_packet(s, c, ur, sub_cmd, now_time);
+ } while (0);
+
+ if (!c) {
+ ur->ecx &= 0xffff;
+ }
+
+ trace_vmport_detail_rpc_end(sub_cmd, ur->eax, ur->ebx, ur->ecx, ur->edx,
+ ur->esi, ur->edi);
+}
+
static uint32_t vmport_rpc_ioport_read(void *opaque, uint32_t addr)
{
VMPortRpcState *s = opaque;
@@ -71,7 +842,7 @@ static uint32_t vmport_rpc_ioport_read(void *opaque, uint32_t addr)
vmmouse_get_data(ur.data);
- s->build_number_time++;
+ vmport_rpc(s, &ur.regs);
vmmouse_set_data(ur.data);
return ur.data[0];
@@ -79,11 +850,32 @@ static uint32_t vmport_rpc_ioport_read(void *opaque, uint32_t addr)
static void vmport_rpc_reset(DeviceState *d)
{
+ unsigned int i;
VMPortRpcState *s = VMPORT_RPC(d);
s->reset_time = 14;
s->build_number_value = 0;
s->build_number_time = 0;
+ for (i = 0; i < GUESTMSG_MAX_CHANNEL; ++i) {
+ unsigned int j;
+ channel_t *c = &s->chans[i];
+
+ for (j = 0; j < MAX_BKTS; ++j) {
+ bucket_t *b = &c->recv_bkt[j];
+
+ g_free(b->recv.words);
+ }
+ g_free(c->send.words);
+ memset(c, 0, sizeof(*c));
+ }
+ g_hash_table_remove_all(s->guestinfo);
+}
+
+static void free_guestinfo(gpointer opaque)
+{
+ guestinfo_t *gi = (guestinfo_t *)opaque;
+ g_free(gi->val_data);
+ g_free(gi);
}
static void vmport_rpc_realize(DeviceState *dev, Error **errp)
@@ -93,6 +885,9 @@ static void vmport_rpc_realize(DeviceState *dev, Error **errp)
if (!vmport_register(VMPORT_RPC_COMMAND, vmport_rpc_ioport_read, s)) {
error_set(errp, ERROR_CLASS_GENERIC_ERROR,
"vmport_rpc needs vmport enabled");
+ } else {
+ s->guestinfo = g_hash_table_new_full(g_str_hash, g_str_equal, g_free,
+ free_guestinfo);
}
}
diff --git a/trace-events b/trace-events
index 80e8a76..bfcbc06 100644
--- a/trace-events
+++ b/trace-events
@@ -892,6 +892,28 @@ pc87312_info_serial(int n, uint32_t base, uint32_t irq) "id=%d, base 0x%x, irq %
# hw/misc/vmport.c
vmport_ioport_unknown_command(unsigned char command) "%x"
+# hw/misc/vmport_rpc.c
+vmport_detail_rpc_start(int sub_cmd, unsigned int ax, unsigned int bx, unsigned int cx, unsigned int dx, unsigned int si, unsigned int di) "sub_cmd=0x%x ax=%x bx=%x cx=%x dx=%x si=%x di=%x"
+vmport_detail_rpc_end(int sub_cmd, unsigned int ax, unsigned int bx, unsigned int cx, unsigned int dx, unsigned int si, unsigned int di) "sub_cmd=0x%x ax=%x bx=%x cx=%x dx=%x si=%x di=%x"
+vmport_rpc_send_skip(char *prefix, unsigned int end, unsigned int len, unsigned int k, size_t out_size, char *out) "%s%d[%d,%d,%zu]%s"
+vmport_rpc_send_normal(char *prefix, unsigned int end, unsigned int len, unsigned int k, size_t out_size, char *out) "%s%d[%d,%d,%zu]%s"
+vmport_rpc_recv_normal(char *prefix, unsigned int end, unsigned int len, unsigned int k, size_t out_size, char *out) "%s%d[%d,%d,%zu]%s"
+vmport_rpc_get_guestinfo(short klen, int kmaxlen, char *key) "klen=%d key=%.*s"
+vmport_rpc_get_guestinfo_found(short vlen, int vmaxlen, char *val) "vlen=%d val=%.*s"
+vmport_rpc_set_guestinfo(short klen, int kmaxlen, const char *key, short vlen, int vmaxlen, char *val) "klen=%d key=%.*s vlen=%d val=%.*s"
+vmport_rpc_set_guestinfo_bad(int i, int used_guestinfo) "i=%d not allocated, but used_guestinfo=%d"
+vmport_rpc_send_big(int len, size_t maxlen) "recv_len=%d >= %zd"
+vmport_rpc_sweep(int chan_id, long delta) "flush %d. delta=%ld"
+vmport_rpc_ping(long delta) "delta=%ld"
+vmport_rpc_nofree(void) "failed to find a free channel"
+vmport_rpc_bad_chan(int chan_id, int max_chan_id) "chan_id=%d >= %d"
+vmport_rpc_chan_not_open(int chan_id) "chan_id=%d"
+vmport_rpc_bad_cookie(int msg_cookie, int ctl_cookie) "%x vs %x"
+vmport_detail_rpc_process_send_size(int chan_id, int send_len) "chan %d is %d"
+vmport_detail_rpc_process_recv_size(int chan_id, int send_len) "chan %d is %d"
+vmport_rpc_process_close(int chan_id) "chan %d"
+vmport_rpc_process_open(int chan_id, int proto_num) "chan %d proto 0x%x"
+
# hw/scsi/vmw_pvscsi.c
pvscsi_ring_init_data(uint32_t txr_len_log2, uint32_t rxr_len_log2) "TX/RX rings logarithms set to %d/%d"
pvscsi_ring_init_msg(uint32_t len_log2) "MSG ring logarithm set to %d"
--
1.8.3.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v8 7/8] vmport_rpc: Add migration
2015-06-23 23:39 [Qemu-devel] [PATCH v8 0/8] Add limited support of VMware's hyper-call rpc Don Slutz
` (4 preceding siblings ...)
2015-06-23 23:39 ` [Qemu-devel] [PATCH v8 5/8] vmport_rpc: Add limited support of VMware's hyper-call rpc Don Slutz
@ 2015-06-23 23:39 ` Don Slutz
2015-06-23 23:39 ` [Qemu-devel] [PATCH v8 8/8] vmport: Add VMware all ring hack Don Slutz
` (2 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Don Slutz @ 2015-06-23 23:39 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S. Tsirkin, Markus Armbruster, Don Slutz, Luiz Capitulino,
Don Slutz, Anthony Liguori, Paolo Bonzini, Andreas Färber,
Richard Henderson
From: Don Slutz <dslutz@verizon.com>
Signed-off-by: Don Slutz <dslutz@verizon.com>
CC: Don Slutz <don.slutz@gmail.com>
---
hw/misc/vmport_rpc.c | 250 +++++++++++++++++++++++++++++++++++++++++++++++++++
trace-events | 2 +
2 files changed, 252 insertions(+)
diff --git a/hw/misc/vmport_rpc.c b/hw/misc/vmport_rpc.c
index 917898b..7198e30 100644
--- a/hw/misc/vmport_rpc.c
+++ b/hw/misc/vmport_rpc.c
@@ -171,6 +171,14 @@ typedef struct VMPortRpcState {
uint32_t open_cookie;
channel_t chans[GUESTMSG_MAX_CHANNEL];
GHashTable *guestinfo;
+ /* Temporary cache for migration purposes */
+ int32_t mig_chan_num;
+ int32_t mig_bucket_num;
+ uint32_t mig_guestinfo_size;
+ uint32_t mig_guestinfo_off;
+ uint8_t *mig_guestinfo_buf;
+ channel_control_t *mig_chans;
+ bucket_control_t *mig_buckets;
#ifdef VMPORT_RPC_DEBUG
unsigned int end;
unsigned int last;
@@ -1186,6 +1194,247 @@ static Property vmport_rpc_properties[] = {
DEFINE_PROP_END_OF_LIST(),
};
+static const VMStateDescription vmstate_vmport_rpc_chan = {
+ .name = "vmport_rpc/chan",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField [])
+ {
+ VMSTATE_UINT64(active_time, channel_control_t),
+ VMSTATE_UINT32(chan_id, channel_control_t),
+ VMSTATE_UINT32(cookie, channel_control_t),
+ VMSTATE_UINT32(proto_num, channel_control_t),
+ VMSTATE_UINT16(send_len, channel_control_t),
+ VMSTATE_UINT16(send_idx, channel_control_t),
+ VMSTATE_UINT16(send_buf_max, channel_control_t),
+ VMSTATE_UINT8(recv_read, channel_control_t),
+ VMSTATE_UINT8(recv_write, channel_control_t),
+ VMSTATE_END_OF_LIST()
+ },
+};
+
+static const VMStateDescription vmstate_vmport_rpc_bucket = {
+ .name = "vmport_rpc/bucket",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField [])
+ {
+ VMSTATE_UINT16(recv_len, bucket_control_t),
+ VMSTATE_UINT16(recv_idx, bucket_control_t),
+ VMSTATE_UINT16(recv_buf_max, bucket_control_t),
+ VMSTATE_END_OF_LIST()
+ },
+};
+
+static void vmport_rpc_size_mig_guestinfo(gpointer key, gpointer value,
+ gpointer opaque)
+{
+ VMPortRpcState *s = opaque;
+ unsigned int key_len = strlen(key) + 1;
+ guestinfo_t *gi = value;
+
+ s->mig_guestinfo_size += 1 + key_len + 4 + gi->val_max;
+}
+
+static void vmport_rpc_fill_mig_guestinfo(gpointer key, gpointer value,
+ gpointer opaque)
+{
+ VMPortRpcState *s = opaque;
+ unsigned int key_len = strlen(key) + 1;
+ guestinfo_t *gi = value;
+
+ assert(gi->val_len <= gi->val_max);
+ trace_vmport_rpc_fill_mig_guestinfo(key_len, key_len, key, gi->val_len,
+ gi->val_len, gi->val_data);
+ s->mig_guestinfo_buf[s->mig_guestinfo_off++] = key_len;
+ memcpy(s->mig_guestinfo_buf + s->mig_guestinfo_off, key, key_len);
+ s->mig_guestinfo_off += key_len;
+ s->mig_guestinfo_buf[s->mig_guestinfo_off++] = gi->val_len >> 8;
+ s->mig_guestinfo_buf[s->mig_guestinfo_off++] = gi->val_len;
+ s->mig_guestinfo_buf[s->mig_guestinfo_off++] = gi->val_max >> 8;
+ s->mig_guestinfo_buf[s->mig_guestinfo_off++] = gi->val_max;
+ memcpy(s->mig_guestinfo_buf + s->mig_guestinfo_off, gi->val_data,
+ gi->val_max);
+ s->mig_guestinfo_off += gi->val_max;
+}
+
+static int vmport_rpc_pre_load(void *opaque)
+{
+ VMPortRpcState *s = opaque;
+
+ g_free(s->mig_guestinfo_buf);
+ s->mig_guestinfo_buf = NULL;
+ s->mig_guestinfo_size = 0;
+ s->mig_guestinfo_off = 0;
+ g_free(s->mig_chans);
+ s->mig_chans = NULL;
+ s->mig_chan_num = 0;
+ g_free(s->mig_buckets);
+ s->mig_buckets = NULL;
+ s->mig_bucket_num = 0;
+
+ return 0;
+}
+
+static void vmport_rpc_pre_save(void *opaque)
+{
+ VMPortRpcState *s = opaque;
+ unsigned int i;
+ unsigned int mig_chan_idx = 0;
+ unsigned int mig_bucket_idx = 0;
+
+ (void)vmport_rpc_pre_load(opaque);
+ for (i = 0; i < GUESTMSG_MAX_CHANNEL; ++i) {
+ channel_t *c = &s->chans[i];
+
+ if (c->ctl.proto_num) {
+ unsigned int j;
+
+ s->mig_chan_num++;
+ for (j = 0; j < MAX_BKTS; ++j) {
+ bucket_t *b = &c->recv_bkt[j];
+
+ s->mig_bucket_num++;
+ s->mig_guestinfo_size +=
+ (b->ctl.recv_buf_max + 1) * CHAR_PER_CALL;
+ }
+ s->mig_guestinfo_size += (c->ctl.send_buf_max + 1) * CHAR_PER_CALL;
+ }
+ }
+ g_hash_table_foreach(s->guestinfo, vmport_rpc_size_mig_guestinfo, s);
+ s->mig_guestinfo_size++;
+ s->mig_guestinfo_buf = g_malloc(s->mig_guestinfo_size);
+ s->mig_chans = g_malloc(s->mig_chan_num * sizeof(channel_control_t));
+ s->mig_buckets = g_malloc(s->mig_bucket_num * sizeof(bucket_control_t));
+
+ for (i = 0; i < GUESTMSG_MAX_CHANNEL; ++i) {
+ channel_t *c = &s->chans[i];
+
+ if (c->ctl.proto_num) {
+ unsigned int j;
+ channel_control_t *cm = s->mig_chans + mig_chan_idx++;
+ unsigned int send_chars = (c->ctl.send_buf_max + 1) * CHAR_PER_CALL;
+
+ *cm = c->ctl;
+ for (j = 0; j < MAX_BKTS; ++j) {
+ bucket_t *b = &c->recv_bkt[j];
+ bucket_control_t *bm = s->mig_buckets + mig_bucket_idx++;
+ unsigned int recv_chars =
+ (b->ctl.recv_buf_max + 1) * CHAR_PER_CALL;
+
+ *bm = b->ctl;
+ memcpy(s->mig_guestinfo_buf + s->mig_guestinfo_off,
+ b->recv.words, recv_chars);
+ s->mig_guestinfo_off += recv_chars;
+ }
+ memcpy(s->mig_guestinfo_buf + s->mig_guestinfo_off,
+ c->send.words, send_chars);
+ s->mig_guestinfo_off += send_chars;
+ }
+ }
+ g_hash_table_foreach(s->guestinfo, vmport_rpc_fill_mig_guestinfo, s);
+ s->mig_guestinfo_buf[s->mig_guestinfo_off++] = 0;
+ assert(s->mig_guestinfo_size == s->mig_guestinfo_off);
+ assert(s->mig_chan_num == mig_chan_idx);
+ assert(s->mig_bucket_num == mig_bucket_idx);
+}
+
+static int vmport_rpc_post_load(void *opaque, int version_id)
+{
+ VMPortRpcState *s = opaque;
+ unsigned int i;
+ unsigned int key_len;
+ unsigned int mig_bucket_idx = 0;
+
+ s->mig_guestinfo_off = 0;
+ for (i = 0; i < s->mig_chan_num; ++i) {
+ channel_control_t *cm = s->mig_chans + i;
+ channel_t *c = &s->chans[cm->chan_id];
+ unsigned int j;
+ unsigned int send_chars;
+
+ c->ctl = *cm;
+ for (j = 0; j < MAX_BKTS; ++j) {
+ bucket_t *b = &c->recv_bkt[j];
+ bucket_control_t *bm = s->mig_buckets + mig_bucket_idx++;
+ unsigned int recv_chars;
+
+ b->ctl = *bm;
+ recv_chars = (b->ctl.recv_buf_max + 1) * CHAR_PER_CALL;
+ b->recv.words =
+ g_memdup(s->mig_guestinfo_buf + s->mig_guestinfo_off,
+ recv_chars);
+ s->mig_guestinfo_off += recv_chars;
+ }
+ send_chars = (c->ctl.send_buf_max + 1) * CHAR_PER_CALL;
+ c->send.words = g_memdup(s->mig_guestinfo_buf + s->mig_guestinfo_off,
+ send_chars);
+ s->mig_guestinfo_off += send_chars;
+ }
+ assert(s->mig_bucket_num == mig_bucket_idx);
+
+ do {
+ key_len = s->mig_guestinfo_buf[s->mig_guestinfo_off++];
+ if (key_len) {
+ gpointer key = g_memdup(s->mig_guestinfo_buf + s->mig_guestinfo_off,
+ key_len);
+ guestinfo_t *gi = g_malloc(sizeof(guestinfo_t));
+ unsigned int bhi, blow;
+
+ s->mig_guestinfo_off += key_len;
+ bhi = s->mig_guestinfo_buf[s->mig_guestinfo_off++];
+ blow = s->mig_guestinfo_buf[s->mig_guestinfo_off++];
+ gi->val_len = (bhi << 8) + blow;
+ bhi = s->mig_guestinfo_buf[s->mig_guestinfo_off++];
+ blow = s->mig_guestinfo_buf[s->mig_guestinfo_off++];
+ gi->val_max = (bhi << 8) + blow;
+ assert(gi->val_len <= gi->val_max);
+ gi->val_data = g_memdup(s->mig_guestinfo_buf +
+ s->mig_guestinfo_off,
+ gi->val_max);
+ s->mig_guestinfo_off += gi->val_max;
+ trace_vmport_rpc_post_load(key_len, key_len, key, gi->val_len,
+ gi->val_len, gi->val_data);
+ assert(!g_hash_table_lookup(s->guestinfo, key));
+ g_hash_table_insert(s->guestinfo, key, gi);
+ }
+ } while (key_len);
+ assert(s->mig_guestinfo_size == s->mig_guestinfo_off);
+
+ (void)vmport_rpc_pre_load(opaque);
+ return 0;
+}
+
+static const VMStateDescription vmstate_vmport_rpc = {
+ .name = "vmport_rpc",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .pre_save = vmport_rpc_pre_save,
+ .pre_load = vmport_rpc_pre_load,
+ .post_load = vmport_rpc_post_load,
+ .fields = (VMStateField[])
+ {
+ VMSTATE_UINT64(reset_time, VMPortRpcState),
+ VMSTATE_UINT64(build_number_value, VMPortRpcState),
+ VMSTATE_UINT64(build_number_time, VMPortRpcState),
+ VMSTATE_UINT64(ping_time, VMPortRpcState),
+ VMSTATE_UINT32(open_cookie, VMPortRpcState),
+ VMSTATE_INT32(mig_chan_num, VMPortRpcState),
+ VMSTATE_STRUCT_VARRAY_ALLOC(mig_chans, VMPortRpcState, mig_chan_num,
+ 0, vmstate_vmport_rpc_chan,
+ channel_control_t),
+ VMSTATE_INT32(mig_bucket_num, VMPortRpcState),
+ VMSTATE_STRUCT_VARRAY_ALLOC(mig_buckets, VMPortRpcState,
+ mig_bucket_num, 0,
+ vmstate_vmport_rpc_bucket,
+ bucket_control_t),
+ VMSTATE_UINT32(mig_guestinfo_size, VMPortRpcState),
+ VMSTATE_VBUFFER_ALLOC_UINT32(mig_guestinfo_buf, VMPortRpcState, 1,
+ NULL, 0, mig_guestinfo_size),
+ VMSTATE_END_OF_LIST()
+ },
+};
+
static void vmport_rpc_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
@@ -1194,6 +1443,7 @@ static void vmport_rpc_class_init(ObjectClass *klass, void *data)
dc->reset = vmport_rpc_reset;
dc->desc = "Enable VMware's hyper-call rpc";
dc->props = vmport_rpc_properties;
+ dc->vmsd = &vmstate_vmport_rpc;
}
static const TypeInfo vmport_rpc_info = {
diff --git a/trace-events b/trace-events
index bfcbc06..72a53b2 100644
--- a/trace-events
+++ b/trace-events
@@ -913,6 +913,8 @@ vmport_detail_rpc_process_send_size(int chan_id, int send_len) "chan %d is %d"
vmport_detail_rpc_process_recv_size(int chan_id, int send_len) "chan %d is %d"
vmport_rpc_process_close(int chan_id) "chan %d"
vmport_rpc_process_open(int chan_id, int proto_num) "chan %d proto 0x%x"
+vmport_rpc_fill_mig_guestinfo(short klen, int kmaxlen, char *key, short vlen, int vmaxlen, char *val) "klen=%d key=%.*s vlen=%d val=%.*s"
+vmport_rpc_post_load(short klen, int kmaxlen, char *key, short vlen, int vmaxlen, char *val) "klen=%d key=%.*s vlen=%d val=%.*s"
# hw/scsi/vmw_pvscsi.c
pvscsi_ring_init_data(uint32_t txr_len_log2, uint32_t rxr_len_log2) "TX/RX rings logarithms set to %d/%d"
--
1.8.3.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v8 8/8] vmport: Add VMware all ring hack
2015-06-23 23:39 [Qemu-devel] [PATCH v8 0/8] Add limited support of VMware's hyper-call rpc Don Slutz
` (5 preceding siblings ...)
2015-06-23 23:39 ` [Qemu-devel] [PATCH v8 7/8] vmport_rpc: Add migration Don Slutz
@ 2015-06-23 23:39 ` Don Slutz
2015-07-01 6:50 ` Michael S. Tsirkin
2015-06-30 18:16 ` [Qemu-devel] [ping][PATCH v8 0/8] Add limited support of VMware's hyper-call rpc Don Slutz
[not found] ` <1435102773-3498-7-git-send-email-Don.Slutz@Gmail.com>
8 siblings, 1 reply; 19+ messages in thread
From: Don Slutz @ 2015-06-23 23:39 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S. Tsirkin, Markus Armbruster, Don Slutz, Luiz Capitulino,
Don Slutz, Anthony Liguori, Paolo Bonzini, Andreas Färber,
Richard Henderson
From: Don Slutz <dslutz@verizon.com>
This is done by adding a new machine property vmware-port-ring3 that
needs to be enabled to have any effect. It only effects accel=tcg
mode. It is needed if you want to use VMware tools in accel=tcg
mode.
Signed-off-by: Don Slutz <dslutz@verizon.com>
CC: Don Slutz <don.slutz@gmail.com>
---
hw/i386/pc.c | 26 +++++++++++++++++++++++++-
hw/i386/pc_piix.c | 2 +-
hw/i386/pc_q35.c | 2 +-
include/hw/i386/pc.h | 6 +++++-
target-i386/cpu-qom.h | 3 +++
target-i386/seg_helper.c | 9 +++++++++
6 files changed, 44 insertions(+), 4 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 7072930..3ae4476 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1042,7 +1042,9 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
object_unref(OBJECT(cpu));
}
-void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
+/* vmware_port_ring3 true says enable VMware port access in ring3. */
+void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge,
+ bool vmware_port_ring3)
{
int i;
X86CPU *cpu = NULL;
@@ -1073,6 +1075,7 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
error_report_err(error);
exit(1);
}
+ cpu->allow_vmport_ring3 = vmware_port_ring3;
object_unref(OBJECT(cpu));
}
@@ -1835,6 +1838,21 @@ static bool pc_machine_get_aligned_dimm(Object *obj, Error **errp)
return pcms->enforce_aligned_dimm;
}
+static bool pc_machine_get_vmware_port_ring3(Object *obj, Error **errp)
+{
+ PCMachineState *pcms = PC_MACHINE(obj);
+
+ return pcms->vmware_port_ring3;
+}
+
+static void pc_machine_set_vmware_port_ring3(Object *obj, bool value,
+ Error **errp)
+{
+ PCMachineState *pcms = PC_MACHINE(obj);
+
+ pcms->vmware_port_ring3 = value;
+}
+
static void pc_machine_initfn(Object *obj)
{
PCMachineState *pcms = PC_MACHINE(obj);
@@ -1865,6 +1883,12 @@ static void pc_machine_initfn(Object *obj)
object_property_add_bool(obj, PC_MACHINE_ENFORCE_ALIGNED_DIMM,
pc_machine_get_aligned_dimm,
NULL, NULL);
+
+ pcms->vmware_port_ring3 = false;
+ object_property_add_bool(obj, PC_MACHINE_VMWARE_PORT_RING3,
+ pc_machine_get_vmware_port_ring3,
+ pc_machine_set_vmware_port_ring3,
+ NULL);
}
static unsigned pc_cpu_index_to_socket_id(unsigned cpu_index)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index e142f75..890c45e 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -146,7 +146,7 @@ static void pc_init1(MachineState *machine)
object_property_add_child(qdev_get_machine(), "icc-bridge",
OBJECT(icc_bridge), NULL);
- pc_cpus_init(machine->cpu_model, icc_bridge);
+ pc_cpus_init(machine->cpu_model, icc_bridge, pc_machine->vmware_port_ring3);
if (kvm_enabled() && kvmclock_enabled) {
kvmclock_create();
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 082cd93..8288a5f 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -137,7 +137,7 @@ static void pc_q35_init(MachineState *machine)
object_property_add_child(qdev_get_machine(), "icc-bridge",
OBJECT(icc_bridge), NULL);
- pc_cpus_init(machine->cpu_model, icc_bridge);
+ pc_cpus_init(machine->cpu_model, icc_bridge, pc_machine->vmware_port_ring3);
pc_acpi_init("q35-acpi-dsdt.aml");
kvmclock_create();
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 3270a97..f9e74c7 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -40,6 +40,7 @@ struct PCMachineState {
uint64_t max_ram_below_4g;
OnOffAuto vmport;
+ bool vmware_port_ring3;
bool enforce_aligned_dimm;
};
@@ -48,6 +49,7 @@ struct PCMachineState {
#define PC_MACHINE_MAX_RAM_BELOW_4G "max-ram-below-4g"
#define PC_MACHINE_VMPORT "vmport"
#define PC_MACHINE_ENFORCE_ALIGNED_DIMM "enforce-aligned-dimm"
+#define PC_MACHINE_VMWARE_PORT_RING3 "vmware-port-ring3"
/**
* PCMachineClass:
@@ -161,7 +163,9 @@ extern int fd_bootchk;
void pc_register_ferr_irq(qemu_irq irq);
void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
-void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge);
+/* vmware_port_ring3 true says enable VMware port access in ring3. */
+void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge,
+ bool vmware_port_ring3);
void pc_hot_add_cpu(const int64_t id, Error **errp);
void pc_acpi_init(const char *default_dsdt);
diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
index 7a4fddd..ccbc5bb 100644
--- a/target-i386/cpu-qom.h
+++ b/target-i386/cpu-qom.h
@@ -109,6 +109,9 @@ typedef struct X86CPU {
*/
bool enable_pmu;
+ /* allow_vmport_ring3 true says enable VMware port access in ring3 */
+ bool allow_vmport_ring3;
+
/* in order to simplify APIC support, we leave this pointer to the
user */
struct DeviceState *apic_state;
diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c
index 8a4271e..2ddb84f 100644
--- a/target-i386/seg_helper.c
+++ b/target-i386/seg_helper.c
@@ -2566,6 +2566,15 @@ static inline void check_io(CPUX86State *env, int addr, int size)
{
int io_offset, val, mask;
+ /* vmport hack: skip iopl checking for VMware port 0x5658 (see
+ * vmport_realizefn()) */
+ if (addr == 0x5658) {
+ X86CPU *cpu = x86_env_get_cpu(env);
+ if (cpu->allow_vmport_ring3) {
+ return;
+ }
+ }
+
/* TSS must be a valid 32 bit one */
if (!(env->tr.flags & DESC_P_MASK) ||
((env->tr.flags >> DESC_TYPE_SHIFT) & 0xf) != 9 ||
--
1.8.3.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [ping][PATCH v8 0/8] Add limited support of VMware's hyper-call rpc
2015-06-23 23:39 [Qemu-devel] [PATCH v8 0/8] Add limited support of VMware's hyper-call rpc Don Slutz
` (6 preceding siblings ...)
2015-06-23 23:39 ` [Qemu-devel] [PATCH v8 8/8] vmport: Add VMware all ring hack Don Slutz
@ 2015-06-30 18:16 ` Don Slutz
[not found] ` <1435102773-3498-7-git-send-email-Don.Slutz@Gmail.com>
8 siblings, 0 replies; 19+ messages in thread
From: Don Slutz @ 2015-06-30 18:16 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S. Tsirkin, Markus Armbruster, Luiz Capitulino,
Anthony Liguori, Paolo Bonzini, Andreas Färber,
Richard Henderson
Ping
On 06/23/15 19:39, Don Slutz wrote:
> Changes v7 to v8:
> Rebase to master
>
> Drop patch #1 since commit 965eb2fcdfe919ecced6c34803535ad32dc1249c
> fixed this.
>
> Paolo Bonzini
> Ok to pull v7 1,2,3, and 9.
> Added Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> Moved v7 #9 to v8 #3
>
> Eric Blake
> s/it's/its/
> Done
> No space after **.
> Done
> Don't know if any static checkers will complain about the break being
> dead code.
> Dropped break since this is the last case.
> Would it help if struct keyValue declared 'const char *key_data'?
> It does, so switch to this.
> This cast is spurious (foreach_dynamic_vmport_rpc_device)
> Dropped all casts using this routine.
> Do you want g_malloc0, or g_new0?
> Switched to g_new0.
> Adjust /* FIXME should check for...
> Added "Or cause an error to be raised."
>
> Michael S. Tsirkin
> To me it looks like this will break cross-version migration
> Dropped code to always add the new device.
> Add code to check for vmport enabled.
> Quite possibly but personally I'm confused.
> No partial PULL request sent.
>
>
> Changes v6 to v7:
> Rebase to master
>
> Fixed a bug caused by commit c3c1bb99d1c11978d9ce94d1bdcf0705378c1459 now patch #1
>
> Added patch #2 to switch to using trace in vm,port.c.
>
> Delay call on g_strndup till after key length check.
>
> Switched e-mail address in MAINTAINERS.
>
> Eric Blake
> Why not assert(find) instead of leaving it to the comment?
> Switch to assert.
> Is it worth marking arg const here and in the VMPortRpcFind struct
> Switch to const.
> I'd rather abort() if someone compiled with -NDEBUG
> Done.
> Still mismatches on ---- line length (several sites).
> Done
>
>
> Changes v5 to v6:
>
> Rebase to master
>
> Eric Blake
> Returning a non-dictionary is not extensible.
> Added new type VmportGuestInfoValue.
> s/VmportGuestInfo/VmportGuestInfoKey/
> s/type/struct/
> Issues with examples
> Fixed.
>
>
> Changes v4 to v5:
>
> Paolo Bonzini
> What is VMPORT_SHORT about?
> Dropped this.
> Why not use a bool in CPUX86State?
> Took his sugestion and moved to a bool in X86CPU.
>
> Changes v3 to v4:
>
> Paolo Bonzini on "vmort_rpc: Add QMP access to vmport_rpc"
> Does this compile on non-x86 targets?
> Nope. Fixed.
>
> Changes v2 to v3:
>
> s/2.3/2.4
>
> Changes v1 to v2:
>
> Added live migration code.
> Adjust data structures for migration.
> Switch to GHashTable.
>
> Eric Blake
> s/spawened/spawned/
> Done
> s/traceing/tracing/
> Done
> Change "error_set(errp, ERROR_CLASS_GENERIC_ERROR, " to
> "error_setg(errp, "
> Done
> Why two commands (inject-vmport-reboot, inject-vmport-halt)?
> Switched to inject-vmport-action.
> format=base64 "bug" statements.
> Dropped.
>
> Much more on format=base64:
>
> If there is a bug it is in GLIB. However the Glib reference manual
> refers to RFC 1421 and RFC 2045 and MIME encoding. Based on all
> that (which seems to match:
>
> http://en.wikipedia.org/wiki/Base64
>
> ) MIME states that all characters outside the (base64) alphabet are
> to be ignored. Testing shows that g_base64_decode() does this.
>
> The confusion is that most non-MIME uses reject a base64 string that
> contain characters outside the alphabet. I was just following the
> other uses of base64 in this file.
>
> DataFormat refers to RFC 3548, which has the info:
>
> "
> Implementations MUST reject the encoding if it contains
> characters outside the base alphabet when interpreting base
> encoded data, unless the specification referring to this document
> explicitly states otherwise. Such specifications may, as MIME
> does, instead state that characters outside the base encoding
> alphabet should simply be ignored when interpreting data ("be
> liberal in what you accept").
> "
>
> So with GLIB going the MIME way, I do not think this is a QEMU bug
> (you could consider this a GLIB bug, but the document I found says
> that GLIB goes the MIME way and so does not reject anything).
>
> ---
>
>
> The support included is enough to allow VMware tools to install in a
> guest and provide guestinfo support. guestinfo support is provided
> by what is known as VMware RPC support.
>
> One of the better on-line references is:
>
> https://sites.google.com/site/chitchatvmback/backdoor
>
> As a place to get more accurate information by studying:
>
> http://open-vm-tools.sourceforge.net/
>
> With vmware tools installed, you can do:
>
> -------------------------------------------------------------------------------
> Last login: Fri Jan 30 16:03:08 2015
> [root@C63-min-tools ~]# vmtoolsd --cmd "info-get guestinfo.joejoel"
> No value found
> [root@C63-min-tools ~]# vmtoolsd --cmd "info-set guestinfo.joejoel bar"
>
> [root@C63-min-tools ~]# vmtoolsd --cmd "info-get guestinfo.joejoel"
> bar
> [root@C63-min-tools ~]#
> -------------------------------------------------------------------------------
>
> to access guest info. QMP access is also provided.
>
> The live migration code is still in progress.
>
> Don Slutz (8):
> vmport: Switch to trace
> vmport: Fix vmport_cmd_ram_size
> MAINTAINERS: add VMware port
> vmport_rpc: Add the object vmport_rpc
> vmport_rpc: Add limited support of VMware's hyper-call rpc
> vmport_rpc: Add QMP access to vmport_rpc object.
> vmport_rpc: Add migration
> vmport: Add VMware all ring hack
>
> MAINTAINERS | 7 +
> hw/i386/pc.c | 26 +-
> hw/i386/pc_piix.c | 2 +-
> hw/i386/pc_q35.c | 2 +-
> hw/misc/Makefile.objs | 1 +
> hw/misc/vmport.c | 16 +-
> hw/misc/vmport_rpc.c | 1461 ++++++++++++++++++++++++++++++++++++++++++++++
> include/hw/i386/pc.h | 8 +-
> monitor.c | 24 +
> qapi-schema.json | 101 ++++
> qmp-commands.hx | 119 ++++
> target-i386/cpu-qom.h | 3 +
> target-i386/seg_helper.c | 9 +
> trace-events | 27 +
> 14 files changed, 1793 insertions(+), 13 deletions(-)
> create mode 100644 hw/misc/vmport_rpc.c
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v8 8/8] vmport: Add VMware all ring hack
2015-06-23 23:39 ` [Qemu-devel] [PATCH v8 8/8] vmport: Add VMware all ring hack Don Slutz
@ 2015-07-01 6:50 ` Michael S. Tsirkin
2015-07-03 13:08 ` Don Slutz
0 siblings, 1 reply; 19+ messages in thread
From: Michael S. Tsirkin @ 2015-07-01 6:50 UTC (permalink / raw)
To: Don Slutz
Cc: qemu-devel, Markus Armbruster, Don Slutz, Luiz Capitulino,
Anthony Liguori, Paolo Bonzini, Andreas Färber,
Richard Henderson
On Tue, Jun 23, 2015 at 07:39:33PM -0400, Don Slutz wrote:
> From: Don Slutz <dslutz@verizon.com>
>
> This is done by adding a new machine property vmware-port-ring3 that
> needs to be enabled to have any effect. It only effects accel=tcg
> mode. It is needed if you want to use VMware tools in accel=tcg
> mode.
>
> Signed-off-by: Don Slutz <dslutz@verizon.com>
> CC: Don Slutz <don.slutz@gmail.com>
IMO this patch isn't ready yet.
> ---
> hw/i386/pc.c | 26 +++++++++++++++++++++++++-
> hw/i386/pc_piix.c | 2 +-
> hw/i386/pc_q35.c | 2 +-
> include/hw/i386/pc.h | 6 +++++-
> target-i386/cpu-qom.h | 3 +++
> target-i386/seg_helper.c | 9 +++++++++
> 6 files changed, 44 insertions(+), 4 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 7072930..3ae4476 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1042,7 +1042,9 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
> object_unref(OBJECT(cpu));
> }
>
> -void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
> +/* vmware_port_ring3 true says enable VMware port access in ring3. */
> +void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge,
> + bool vmware_port_ring3)
> {
> int i;
> X86CPU *cpu = NULL;
> @@ -1073,6 +1075,7 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
> error_report_err(error);
> exit(1);
> }
> + cpu->allow_vmport_ring3 = vmware_port_ring3;
> object_unref(OBJECT(cpu));
> }
>
> @@ -1835,6 +1838,21 @@ static bool pc_machine_get_aligned_dimm(Object *obj, Error **errp)
> return pcms->enforce_aligned_dimm;
> }
>
> +static bool pc_machine_get_vmware_port_ring3(Object *obj, Error **errp)
> +{
> + PCMachineState *pcms = PC_MACHINE(obj);
> +
> + return pcms->vmware_port_ring3;
> +}
> +
> +static void pc_machine_set_vmware_port_ring3(Object *obj, bool value,
> + Error **errp)
> +{
> + PCMachineState *pcms = PC_MACHINE(obj);
> +
> + pcms->vmware_port_ring3 = value;
> +}
> +
> static void pc_machine_initfn(Object *obj)
> {
> PCMachineState *pcms = PC_MACHINE(obj);
> @@ -1865,6 +1883,12 @@ static void pc_machine_initfn(Object *obj)
> object_property_add_bool(obj, PC_MACHINE_ENFORCE_ALIGNED_DIMM,
> pc_machine_get_aligned_dimm,
> NULL, NULL);
> +
> + pcms->vmware_port_ring3 = false;
> + object_property_add_bool(obj, PC_MACHINE_VMWARE_PORT_RING3,
> + pc_machine_get_vmware_port_ring3,
> + pc_machine_set_vmware_port_ring3,
> + NULL);
New properties should have a description.
> }
>
> static unsigned pc_cpu_index_to_socket_id(unsigned cpu_index)
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index e142f75..890c45e 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -146,7 +146,7 @@ static void pc_init1(MachineState *machine)
> object_property_add_child(qdev_get_machine(), "icc-bridge",
> OBJECT(icc_bridge), NULL);
>
> - pc_cpus_init(machine->cpu_model, icc_bridge);
> + pc_cpus_init(machine->cpu_model, icc_bridge, pc_machine->vmware_port_ring3);
>
> if (kvm_enabled() && kvmclock_enabled) {
> kvmclock_create();
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 082cd93..8288a5f 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -137,7 +137,7 @@ static void pc_q35_init(MachineState *machine)
> object_property_add_child(qdev_get_machine(), "icc-bridge",
> OBJECT(icc_bridge), NULL);
>
> - pc_cpus_init(machine->cpu_model, icc_bridge);
> + pc_cpus_init(machine->cpu_model, icc_bridge, pc_machine->vmware_port_ring3);
> pc_acpi_init("q35-acpi-dsdt.aml");
>
> kvmclock_create();
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 3270a97..f9e74c7 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -40,6 +40,7 @@ struct PCMachineState {
>
> uint64_t max_ram_below_4g;
> OnOffAuto vmport;
> + bool vmware_port_ring3;
> bool enforce_aligned_dimm;
> };
>
> @@ -48,6 +49,7 @@ struct PCMachineState {
> #define PC_MACHINE_MAX_RAM_BELOW_4G "max-ram-below-4g"
> #define PC_MACHINE_VMPORT "vmport"
> #define PC_MACHINE_ENFORCE_ALIGNED_DIMM "enforce-aligned-dimm"
> +#define PC_MACHINE_VMWARE_PORT_RING3 "vmware-port-ring3"
>
> /**
> * PCMachineClass:
Creating it at the pc level and propagating makes code
messy but I'd go along with it if it made sense from
user's point of view, but it does not seem to make sense:
to me this looks more like a CPU feature than a machine property.
Or the property of the vmport rpc device that you created.
If it's required for the rpc device to be used -
why not set this unconditionally if the rpc device
is created? Not saying it's a good idea - just asking.
> @@ -161,7 +163,9 @@ extern int fd_bootchk;
> void pc_register_ferr_irq(qemu_irq irq);
> void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
>
> -void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge);
> +/* vmware_port_ring3 true says enable VMware port access in ring3. */
> +void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge,
> + bool vmware_port_ring3);
> void pc_hot_add_cpu(const int64_t id, Error **errp);
> void pc_acpi_init(const char *default_dsdt);
>
> diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
> index 7a4fddd..ccbc5bb 100644
> --- a/target-i386/cpu-qom.h
> +++ b/target-i386/cpu-qom.h
> @@ -109,6 +109,9 @@ typedef struct X86CPU {
> */
> bool enable_pmu;
>
> + /* allow_vmport_ring3 true says enable VMware port access in ring3 */
> + bool allow_vmport_ring3;
> +
> /* in order to simplify APIC support, we leave this pointer to the
> user */
> struct DeviceState *apic_state;
> diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c
> index 8a4271e..2ddb84f 100644
> --- a/target-i386/seg_helper.c
> +++ b/target-i386/seg_helper.c
> @@ -2566,6 +2566,15 @@ static inline void check_io(CPUX86State *env, int addr, int size)
> {
> int io_offset, val, mask;
>
> + /* vmport hack: skip iopl checking for VMware port 0x5658 (see
> + * vmport_realizefn()) */
> + if (addr == 0x5658) {
> + X86CPU *cpu = x86_env_get_cpu(env);
> + if (cpu->allow_vmport_ring3) {
> + return;
> + }
> + }
> +
> /* TSS must be a valid 32 bit one */
> if (!(env->tr.flags & DESC_P_MASK) ||
> ((env->tr.flags >> DESC_TYPE_SHIFT) & 0xf) != 9 ||
> --
> 1.8.3.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v8 6/8] vmport_rpc: Add QMP access to vmport_rpc object.
[not found] ` <1435102773-3498-7-git-send-email-Don.Slutz@Gmail.com>
@ 2015-07-02 14:11 ` Markus Armbruster
2015-07-03 16:53 ` Don Slutz
0 siblings, 1 reply; 19+ messages in thread
From: Markus Armbruster @ 2015-07-02 14:11 UTC (permalink / raw)
To: Don Slutz
Cc: Michael S. Tsirkin, qemu-devel, Don Slutz, Luiz Capitulino,
Anthony Liguori, Paolo Bonzini, Andreas Färber,
Richard Henderson
Don Slutz <don.slutz@gmail.com> writes:
> From: Don Slutz <dslutz@verizon.com>
>
> This adds one new inject command:
>
> inject-vmport-action
>
> And three guest info commands:
>
> vmport-guestinfo-set
> vmport-guestinfo-get
> query-vmport-guestinfo
>
> More details in qmp-commands.hx
>
> Signed-off-by: Don Slutz <dslutz@verizon.com>
> CC: Don Slutz <don.slutz@gmail.com>
Reviewing only the QAPI/QMP interface, plus a bit of drive-by shooting.
> diff --git a/hw/misc/vmport_rpc.c b/hw/misc/vmport_rpc.c
> index f157425..917898b 100644
> --- a/hw/misc/vmport_rpc.c
> +++ b/hw/misc/vmport_rpc.c
> @@ -178,6 +178,19 @@ typedef struct VMPortRpcState {
> #endif
> } VMPortRpcState;
>
> +typedef int FindVMPortRpcDeviceFunc(VMPortRpcState *s, const void *arg);
> +
> +/*
> + * The input and output argument that find_VMPortRpc_device() uses
> + * to do its work.
> + */
> +typedef struct VMPortRpcFind {
> + const void *arg;
> + FindVMPortRpcDeviceFunc *func;
> + int found;
> + int rc;
> +} VMPortRpcFind;
> +
> /* Basic request types */
> typedef enum {
> MESSAGE_TYPE_OPEN,
> @@ -202,6 +215,56 @@ typedef struct {
> uint32_t edi;
> } vregs;
>
> +/*
> + * Run func() for every VMPortRpc device, traverse the tree for
> + * everything else.
> + */
> +static int find_VMPortRpc_device(Object *obj, void *opaque)
> +{
> + VMPortRpcFind *find = opaque;
> + Object *dev;
> + VMPortRpcState *s;
> +
> + assert(find);
> + if (find->found) {
> + return 0;
> + }
> + dev = object_dynamic_cast(obj, TYPE_VMPORT_RPC);
> + s = (VMPortRpcState *)dev;
> +
> + if (!s) {
> + /* Container, traverse it for children */
> + return object_child_foreach(obj, find_VMPortRpc_device, opaque);
> + }
> +
> + find->found++;
> + find->rc = find->func(s, find->arg);
> +
> + return 0;
> +}
> +
> +/*
> + * Loop through all dynamically created VMPortRpc devices and call
> + * func() for each instance.
> + */
> +static int foreach_dynamic_vmport_rpc_device(FindVMPortRpcDeviceFunc *func,
> + const void *arg)
> +{
> + VMPortRpcFind find = {
> + .func = func,
> + .arg = arg,
> + };
> +
> + /* Loop through all VMPortRpc devices that were spawned outside
> + * the machine */
> + find_VMPortRpc_device(qdev_get_machine(), &find);
> + if (find.found) {
> + return find.rc;
> + } else {
> + return VMPORT_DEVICE_NOT_FOUND;
> + }
> +}
> +
> #ifdef VMPORT_RPC_DEBUG
> /*
> * Add helper function for tracing. This routine will convert
> @@ -329,7 +392,7 @@ static int vmport_rpc_send(VMPortRpcState *s, channel_t *c,
> return rc;
> }
>
> -static int vmport_rpc_ctrl_send(VMPortRpcState *s, char *msg)
> +static int vmport_rpc_ctrl_send(VMPortRpcState *s, const char *msg)
> {
> int rc = SEND_NOT_OPEN;
> unsigned int i;
> @@ -457,6 +520,29 @@ static int get_guestinfo(VMPortRpcState *s,
> return GUESTINFO_NOTFOUND;
> }
>
> +static int get_qmp_guestinfo(VMPortRpcState *s,
> + unsigned int a_key_len, const char *a_info_key,
> + unsigned int *a_value_len, void **a_value_data)
> +{
> + guestinfo_t *gi = NULL;
> +
> + if (a_key_len <= MAX_KEY_LEN) {
> + gpointer key = g_strndup(a_info_key, a_key_len);
> +
> + gi = (guestinfo_t *)g_hash_table_lookup(s->guestinfo, key);
> + g_free(key);
> + } else {
> + return GUESTINFO_KEYTOOLONG;
> + }
> + if (gi) {
> + *a_value_len = gi->val_len;
> + *a_value_data = gi->val_data;
> + return 0;
> + }
> +
> + return GUESTINFO_NOTFOUND;
> +}
> +
> static int set_guestinfo(VMPortRpcState *s, int a_key_len,
> unsigned int a_val_len, const char *a_info_key,
> char *val)
> @@ -775,7 +861,7 @@ static void vmport_rpc(VMPortRpcState *s , vregs *ur)
> }
> if (delta > s->reset_time) {
> trace_vmport_rpc_ping(delta);
> - vmport_rpc_ctrl_send(s, (char *)"reset");
> + vmport_rpc_ctrl_send(s, "reset");
> }
> vmport_rpc_sweep(s, now_time);
> do {
> @@ -848,6 +934,206 @@ static uint32_t vmport_rpc_ioport_read(void *opaque, uint32_t addr)
> return ur.data[0];
> }
>
> +static int vmport_rpc_find_send(VMPortRpcState *s, const void *arg)
> +{
> + return vmport_rpc_ctrl_send(s, arg);
> +}
> +
> +static void convert_local_rc(Error **errp, int rc)
> +{
> + switch (rc) {
> + case 0:
> + break;
> + case VMPORT_DEVICE_NOT_FOUND:
> + error_set(errp, QERR_DEVICE_NOT_FOUND, TYPE_VMPORT_RPC);
> + break;
Conflicts with
commit 75158ebbe259f0bd8bf435e8f4827a43ec89c877
Author: Markus Armbruster <armbru@redhat.com>
Date: Mon Mar 16 08:57:47 2015 +0100
qerror: Eliminate QERR_DEVICE_NOT_FOUND
Error classes other than ERROR_CLASS_GENERIC_ERROR should not be used
in new code. Hiding them in QERR_ macros makes new uses hard to spot.
Fortunately, there's just one such macro left. Eliminate it with this
coccinelle semantic patch:
@@
expression EP, E;
@@
-error_set(EP, QERR_DEVICE_NOT_FOUND, E)
+error_set(EP, ERROR_CLASS_DEVICE_NOT_FOUND, "Device '%s' not found", E)
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
Sorry if you explained this already for previous revisions: what's the
justification for ERROR_CLASS_DEVICE_NOT_FOUND? Can you stick to
ERROR_CLASS_GENERIC_ERROR?
> + case SEND_NOT_OPEN:
> + error_setg(errp, "VMWare rpc not open");
> + break;
> + case SEND_SKIPPED:
> + error_setg(errp, "VMWare rpc send skipped");
> + break;
> + case SEND_TRUCATED:
> + error_setg(errp, "VMWare rpc send trucated");
> + break;
> + case SEND_NO_MEMORY:
> + error_setg(errp, "VMWare rpc send out of memory");
> + break;
> + case GUESTINFO_NOTFOUND:
> + error_setg(errp, "VMWare guestinfo not found");
> + break;
> + case GUESTINFO_VALTOOLONG:
> + error_setg(errp, "VMWare guestinfo value too long");
> + break;
> + case GUESTINFO_KEYTOOLONG:
> + error_setg(errp, "VMWare guestinfo key too long");
> + break;
> + case GUESTINFO_TOOMANYKEYS:
> + error_setg(errp, "VMWare guestinfo too many keys");
> + break;
> + case GUESTINFO_NO_MEMORY:
> + error_setg(errp, "VMWare guestinfo out of memory");
> + break;
> + default:
> + error_setg(errp, "VMWare rpc send rc=%d unknown", rc);
> + break;
> + }
> +}
> +
> +void qmp_inject_vmport_action(enum VmportAction action, Error **errp)
> +{
> + int rc;
> +
> + switch (action) {
> + case VMPORT_ACTION_REBOOT:
> + rc = foreach_dynamic_vmport_rpc_device(vmport_rpc_find_send,
> + "OS_Reboot");
> + break;
> + case VMPORT_ACTION_HALT:
> + rc = foreach_dynamic_vmport_rpc_device(vmport_rpc_find_send,
> + "OS_Halt");
> + break;
> + case VMPORT_ACTION_MAX:
> + assert(action != VMPORT_ACTION_MAX);
If this fails, you likely found a major compiler bug.
> + abort(); /* Should be impossible to get here. */
Why not simply
default:
abort();
> + }
> + convert_local_rc(errp, rc);
> +}
> +
> +typedef struct keyValue {
> + const char *key_data;
> + void *value_data;
> + unsigned int key_len;
> + unsigned int value_len;
> +} keyValue;
> +
> +static int find_set(VMPortRpcState *s, const void *arg)
> +{
> + const keyValue *key_value = arg;
> +
> + return set_guestinfo(s, key_value->key_len, key_value->value_len,
> + key_value->key_data, key_value->value_data);
> +}
> +
> +static int find_get(VMPortRpcState *s, const void *arg)
> +{
> + keyValue *key_value = (void *)arg;
> +
> + return get_qmp_guestinfo(s, key_value->key_len, key_value->key_data,
> + &key_value->value_len, &key_value->value_data);
> +}
> +
> +void qmp_vmport_guestinfo_set(const char *key, const char *value,
> + bool has_format, enum DataFormat format,
> + Error **errp)
> +{
> + int rc;
> + keyValue key_value;
> +
> + if (strncmp(key, "guestinfo.", strlen("guestinfo.")) == 0) {
> + key_value.key_data = (key + strlen("guestinfo."));
> + key_value.key_len = strlen(key) - strlen("guestinfo.");
> + } else {
> + key_value.key_data = key;
> + key_value.key_len = strlen(key);
> + }
> + if (has_format && (format == DATA_FORMAT_BASE64)) {
> + gsize write_count;
> +
> + key_value.value_data = g_base64_decode(value, &write_count);
> + key_value.value_len = write_count;
> + } else {
> + key_value.value_data = (void *)value;
> + key_value.value_len = strlen(value);
> + }
> +
> + rc = foreach_dynamic_vmport_rpc_device(find_set, &key_value);
> +
> + if (key_value.value_data != value) {
> + g_free(key_value.value_data);
> + }
> +
> + if (rc) {
> + convert_local_rc(errp, rc);
> + return;
> + }
> +}
> +
> +VmportGuestInfoValue *qmp_vmport_guestinfo_get(const char *key,
> + bool has_format,
> + enum DataFormat format,
> + Error **errp)
> +{
> + int rc;
> + keyValue key_value;
> + VmportGuestInfoValue *ret_value;
> +
> + if (strncmp(key, "guestinfo.", strlen("guestinfo.")) == 0) {
> + key_value.key_data = (key + strlen("guestinfo."));
> + key_value.key_len = strlen(key) - strlen("guestinfo.");
> + } else {
> + key_value.key_data = key;
> + key_value.key_len = strlen(key);
> + }
> +
> + rc = foreach_dynamic_vmport_rpc_device(find_get, &key_value);
> + if (rc) {
> + convert_local_rc(errp, rc);
> + return NULL;
> + }
> +
> + ret_value = g_new0(VmportGuestInfoValue, 1);
> + if (has_format && (format == DATA_FORMAT_BASE64)) {
> + ret_value->value = g_base64_encode(key_value.value_data,
> + key_value.value_len);
> + } else {
> + /*
> + * FIXME should check for complete, valid UTF-8 characters.
> + * Invalid sequences should be replaced by a suitable
> + * replacement character. Or cause an error to be raised.
> + */
Thanks for copying the FIXME from qmp_ringbuf_read(), I appreciate it.
> + ret_value->value = g_malloc(key_value.value_len + 1);
> + memcpy(ret_value->value, key_value.value_data, key_value.value_len);
> + ret_value->value[key_value.value_len] = 0;
> + }
> +
> + return ret_value;
> +}
> +
> +
> +static void vmport_rpc_find_list_one(gpointer key, gpointer value,
> + gpointer opaque)
> +{
> + VmportGuestInfoKeyList **keys = opaque;
> + VmportGuestInfoKeyList *info = g_new0(VmportGuestInfoKeyList, 1);
> + char *ckey = key;
> +
> + info->value = g_new0(VmportGuestInfoKey, 1);
> + info->value->key = g_strdup_printf("guestinfo.%s", ckey);
> + info->next = *keys;
> + *keys = info;
> +}
> +
> +static int vmport_rpc_find_list(VMPortRpcState *s, const void *arg)
> +{
> + g_hash_table_foreach(s->guestinfo, vmport_rpc_find_list_one, (gpointer)arg);
> + return 0;
> +}
> +
> +VmportGuestInfoKeyList *qmp_query_vmport_guestinfo(Error **errp)
> +{
> + VmportGuestInfoKeyList *keys = NULL;
> + int rc = foreach_dynamic_vmport_rpc_device(vmport_rpc_find_list,
> + (void *)&keys);
> +
> + if (rc) {
> + convert_local_rc(errp, rc);
> + }
> +
> + return keys;
> +}
> +
> +
> static void vmport_rpc_reset(DeviceState *d)
> {
> unsigned int i;
> diff --git a/monitor.c b/monitor.c
> index aeea2b5..4224314 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -5360,4 +5360,28 @@ void qmp_rtc_reset_reinjection(Error **errp)
> {
> error_setg(errp, QERR_FEATURE_DISABLED, "rtc-reset-reinjection");
> }
> +void qmp_inject_vmport_action(enum VmportAction action, Error **errp)
> +{
> + error_set(errp, QERR_FEATURE_DISABLED, "inject-vmport-action");
> +}
> +void qmp_vmport_guestinfo_set(const char *key, const char *value,
> + bool has_format, enum DataFormat format,
> + Error **errp)
> +{
> + error_set(errp, QERR_FEATURE_DISABLED, "vmport-guestinfo-set");
> +}
> +VmportGuestInfoValue *qmp_vmport_guestinfo_get(const char *key,
> + bool has_format,
> + enum DataFormat format,
> + Error **errp)
> +{
> + error_set(errp, QERR_FEATURE_DISABLED, "vmport-guestinfo-get");
> + return NULL;
> +}
> +VmportGuestInfoKeyList *qmp_query_vmport_guestinfo(Error **errp)
> +{
> + error_set(errp, QERR_FEATURE_DISABLED, "query-vmport-guestinfo");
> + return NULL;
> +}
I understand you're just following existing practice here, but I can
help to wonder whether the practice is smart. The command exists in all
compile-time configurations, but it works in only some. To figure out
whether it works, you have to try it.
If we add the command only when it actually works, you can use
query-commands instead.
This isn't a demand for you to change anything now. We QMP guys need to
decide how we want this done.
> #endif
Since the #ifdef section is now longer than a few lines,
-#endif
+#endif /* !TARGET_I386 */
would be nice
> +
No blank line at end of file, please.
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 106008c..5d9e9e2 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1429,6 +1429,107 @@
> { 'command': 'inject-nmi' }
>
> ##
> +# @VmportAction:
> +#
> +# An enumeration of actions that can be requested via vmport RPC.
> +#
> +# @reboot: Ask the guest via VMware tools to reboot
> +#
> +# @halt: Ask the guest via VMware tools to halt
> +#
> +# Since: 2.4
> +##
> +{ 'enum': 'VmportAction',
> + 'data': [ 'reboot', 'halt' ] }
> +
> +##
> +# @inject-vmport-action:
> +#
> +# Injects a VMWare Tools action to the guest.
> +#
> +# Returns: If successful, nothing
> +#
> +# Since: 2.4
> +#
> +##
> +{ 'command': 'inject-vmport-action',
> + 'data': {'action': 'VmportAction'} }
> +
> +##
> +# @vmport-guestinfo-set:
> +#
> +# Set a VMWare Tools guestinfo key to a value
> +#
> +# @key: the key to set
> +#
> +# @value: The data to set the key to
> +#
> +# @format: #optional value encoding (default 'utf8').
> +# - base64: value is assumed to be base64 encoded text. Its binary
> +# decoding gets set.
Shouldn't you copy ringbuf-write's bug note?
# Bug: invalid base64 is currently not rejected.
> +# - utf8: value's UTF-8 encoding is used to set.
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 2.4
> +##
> +{ 'command': 'vmport-guestinfo-set',
> + 'data': {'key': 'str', 'value': 'str',
> + '*format': 'DataFormat'} }
> +
> +##
> +# @VmportGuestInfoValue:
> +#
> +# Information about a single VMWare Tools guestinfo
> +#
> +# @value: The value for a key
> +#
> +# Since: 2.4
> +##
> +{ 'struct': 'VmportGuestInfoValue', 'data': {'value': 'str'} }
> +
> +##
> +# @vmport-guestinfo-get:
> +#
> +# Get a VMWare Tools guestinfo value for a key
> +#
> +# @key: the key to get
Ignorant question: is the set of keys fixed (e.g. specified by some
authority), completely dynamic (i.e. whatever has been set), or
something else?
> +#
> +# @format: #optional data encoding (default 'utf8').
> +# - base64: the value is returned in base64 encoding.
> +# - utf8: the value is interpreted as UTF-8.
> +#
> +# Returns: value for the guest info key
ringbuf-read carries this bug note:
# Bug: can screw up when the buffer contains invalid UTF-8
# sequences, NUL characters, after the ring buffer lost
# data, and when reading stops because the size limit is
# reached.
Doesn't it apply here, too, with "the buffer" replaced?
> +#
> +# Since: 2.4
> +##
> +{ 'command': 'vmport-guestinfo-get',
> + 'data': {'key': 'str', '*format': 'DataFormat'},
> + 'returns': 'VmportGuestInfoValue' }
I gather you wrap vmport-guestinfo-get's return value in a struct for
extensibility. Can't see why and how we'd want to extend it, but better
safe than sorry.
> +
> +##
> +# @VmportGuestInfoKey:
> +#
> +# Information about a single VMWare Tools guestinfo
> +#
> +# @key: The known key
> +#
> +# Since: 2.4
> +##
> +{ 'struct': 'VmportGuestInfoKey', 'data': {'key': 'str'} }
> +
> +##
> +# @query-vmport-guestinfo:
> +#
> +# Returns information about VMWare Tools guestinfo
> +#
> +# Returns: a list of @VmportGuestInfoKey
> +#
> +# Since: 2.4
> +##
> +{ 'command': 'query-vmport-guestinfo', 'returns': ['VmportGuestInfoKey'] }
Similar. Imagining an extension of a string key is even harder. Okay.
> +
> +##
> # @set_link:
> #
> # Sets the link status of a virtual network adapter.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index a05d25f..a43db8b 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -490,6 +490,125 @@ Note: inject-nmi fails when the guest doesn't support injecting.
> EQMP
>
> {
> + .name = "inject-vmport-action",
> + .args_type = "action:s",
> + .mhandler.cmd_new = qmp_marshal_input_inject_vmport_action,
> + },
> +
> +SQMP
> +inject-vmport-action
> +--------------------
> +
> +Inject a VMWare Tools action to the guest.
> +
> +Arguments:
> +
> +- "action": vmport action (json-string)
> + - Possible values: "reboot", "halt"
> +
> +Example:
> +
> +-> { "execute": "inject-vmport-action",
> + "arguments": { "action": "reboot" } }
> +<- { "return": {} }
> +
> +Note: inject-vmport-action fails when the guest doesn't support injecting.
> +
> +EQMP
> +
> + {
> + .name = "vmport-guestinfo-set",
> + .args_type = "key:s,value:s,format:s?",
> + .mhandler.cmd_new = qmp_marshal_input_vmport_guestinfo_set,
> + },
> +
> +SQMP
> +vmport-guestinfo-set
> +--------------------
> +
> +Set a VMWare Tools guestinfo key to a value
> +
> +Arguments:
> +
> +- "key": the key to set (json-string)
> +- "value": data to write (json-string)
> +- "format": data format (json-string, optional)
> + - Possible values: "utf8" (default), "base64"
Shouldn't you copy ringbuf-write's bug note?
Bug: invalid base64 is currently not rejected.
Whitespace *is* invalid.
> +
> +Example:
> +
> +-> { "execute": "vmport-guestinfo-set",
> + "arguments": { "key": "guestinfo.foo",
> + "value": "abcdefgh",
> + "format": "utf8" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> + {
> + .name = "vmport-guestinfo-get",
> + .args_type = "key:s,format:s?",
> + .mhandler.cmd_new = qmp_marshal_input_vmport_guestinfo_get,
> + },
> +
> +SQMP
> +vmport-guestinfo-get
> +--------------------
> +
> +Get a VMWare Tools guestinfo value for a key
> +
> +Arguments:
> +
> +- "key": the key to get (json-string)
> +- "format": data format (json-string, optional)
> + - Possible values: "utf8" (default), "base64"
> + - Naturally, format "utf8" works only when the value
> + contains valid UTF-8 text. Bug: replacement doesn't work.
> + Bug: can screw up on encountering NUL characters.
You adapted ringbuf-read's bug note. Good.
> +
> +Example:
> +
> +-> { "execute": "vmport-guestinfo-get",
> + "arguments": { "key": "guestinfo.foo",
> + "format": "utf8" } }
> +<- {"return": {"value": "abcdefgh"}}
> +
> +
> +EQMP
> +
> + {
> + .name = "query-vmport-guestinfo",
> + .args_type = "",
> + .mhandler.cmd_new = qmp_marshal_input_query_vmport_guestinfo,
> + },
> +
> +SQMP
> +query-vmport-guestinfo
> +----------------------
> +
> +Returns information about VMWare Tools guestinfo. The returned value is a json-array
Long line, please wrap.
> +of all keys.
> +
> +Example:
> +
> +-> { "execute": "query-vmport-guestinfo" }
> +<- {
> + "return": [
> + {
> + "key": "guestinfo.ip"
> + },
> + {
> + "key": "guestinfo.foo"
> + },
> + {
> + "key": "guestinfo.long"
> + }
> + ]
> + }
> +
> +EQMP
> +
> + {
> .name = "ringbuf-write",
> .args_type = "device:s,data:s,format:s?",
> .mhandler.cmd_new = qmp_marshal_input_ringbuf_write,
I guess I'd split this into inject-vmport-action and the guestinfo
stuff. Matter of taste.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v8 8/8] vmport: Add VMware all ring hack
2015-07-01 6:50 ` Michael S. Tsirkin
@ 2015-07-03 13:08 ` Don Slutz
2015-07-03 13:50 ` Paolo Bonzini
0 siblings, 1 reply; 19+ messages in thread
From: Don Slutz @ 2015-07-03 13:08 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, Markus Armbruster, Don Slutz, Luiz Capitulino,
Anthony Liguori, Paolo Bonzini, Andreas Färber,
Richard Henderson
On 07/01/15 02:50, Michael S. Tsirkin wrote:
> On Tue, Jun 23, 2015 at 07:39:33PM -0400, Don Slutz wrote:
>> From: Don Slutz <dslutz@verizon.com>
>>
>> This is done by adding a new machine property vmware-port-ring3 that
>> needs to be enabled to have any effect. It only effects accel=tcg
>> mode. It is needed if you want to use VMware tools in accel=tcg
>> mode.
>>
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>> CC: Don Slutz <don.slutz@gmail.com>
> IMO this patch isn't ready yet.
>
>> ---
>> hw/i386/pc.c | 26 +++++++++++++++++++++++++-
>> hw/i386/pc_piix.c | 2 +-
>> hw/i386/pc_q35.c | 2 +-
>> include/hw/i386/pc.h | 6 +++++-
>> target-i386/cpu-qom.h | 3 +++
>> target-i386/seg_helper.c | 9 +++++++++
>> 6 files changed, 44 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 7072930..3ae4476 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1042,7 +1042,9 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
>> object_unref(OBJECT(cpu));
>> }
>>
>> -void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
>> +/* vmware_port_ring3 true says enable VMware port access in ring3. */
>> +void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge,
>> + bool vmware_port_ring3)
>> {
>> int i;
>> X86CPU *cpu = NULL;
>> @@ -1073,6 +1075,7 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
>> error_report_err(error);
>> exit(1);
>> }
>> + cpu->allow_vmport_ring3 = vmware_port_ring3;
>> object_unref(OBJECT(cpu));
>> }
>>
>> @@ -1835,6 +1838,21 @@ static bool pc_machine_get_aligned_dimm(Object *obj, Error **errp)
>> return pcms->enforce_aligned_dimm;
>> }
>>
>> +static bool pc_machine_get_vmware_port_ring3(Object *obj, Error **errp)
>> +{
>> + PCMachineState *pcms = PC_MACHINE(obj);
>> +
>> + return pcms->vmware_port_ring3;
>> +}
>> +
>> +static void pc_machine_set_vmware_port_ring3(Object *obj, bool value,
>> + Error **errp)
>> +{
>> + PCMachineState *pcms = PC_MACHINE(obj);
>> +
>> + pcms->vmware_port_ring3 = value;
>> +}
>> +
>> static void pc_machine_initfn(Object *obj)
>> {
>> PCMachineState *pcms = PC_MACHINE(obj);
>> @@ -1865,6 +1883,12 @@ static void pc_machine_initfn(Object *obj)
>> object_property_add_bool(obj, PC_MACHINE_ENFORCE_ALIGNED_DIMM,
>> pc_machine_get_aligned_dimm,
>> NULL, NULL);
>> +
>> + pcms->vmware_port_ring3 = false;
>> + object_property_add_bool(obj, PC_MACHINE_VMWARE_PORT_RING3,
>> + pc_machine_get_vmware_port_ring3,
>> + pc_machine_set_vmware_port_ring3,
>> + NULL);
> New properties should have a description.
Ok.
>
>> }
>>
>> static unsigned pc_cpu_index_to_socket_id(unsigned cpu_index)
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index e142f75..890c45e 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -146,7 +146,7 @@ static void pc_init1(MachineState *machine)
>> object_property_add_child(qdev_get_machine(), "icc-bridge",
>> OBJECT(icc_bridge), NULL);
>>
>> - pc_cpus_init(machine->cpu_model, icc_bridge);
>> + pc_cpus_init(machine->cpu_model, icc_bridge, pc_machine->vmware_port_ring3);
>>
>> if (kvm_enabled() && kvmclock_enabled) {
>> kvmclock_create();
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index 082cd93..8288a5f 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -137,7 +137,7 @@ static void pc_q35_init(MachineState *machine)
>> object_property_add_child(qdev_get_machine(), "icc-bridge",
>> OBJECT(icc_bridge), NULL);
>>
>> - pc_cpus_init(machine->cpu_model, icc_bridge);
>> + pc_cpus_init(machine->cpu_model, icc_bridge, pc_machine->vmware_port_ring3);
>> pc_acpi_init("q35-acpi-dsdt.aml");
>>
>> kvmclock_create();
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 3270a97..f9e74c7 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -40,6 +40,7 @@ struct PCMachineState {
>>
>> uint64_t max_ram_below_4g;
>> OnOffAuto vmport;
>> + bool vmware_port_ring3;
>> bool enforce_aligned_dimm;
>> };
>>
>> @@ -48,6 +49,7 @@ struct PCMachineState {
>> #define PC_MACHINE_MAX_RAM_BELOW_4G "max-ram-below-4g"
>> #define PC_MACHINE_VMPORT "vmport"
>> #define PC_MACHINE_ENFORCE_ALIGNED_DIMM "enforce-aligned-dimm"
>> +#define PC_MACHINE_VMWARE_PORT_RING3 "vmware-port-ring3"
>>
>> /**
>> * PCMachineClass:
> Creating it at the pc level and propagating makes code
> messy but I'd go along with it if it made sense from
> user's point of view, but it does not seem to make sense:
> to me this looks more like a CPU feature than a machine property.
> Or the property of the vmport rpc device that you created.
Well this is not a clear area. It does look more like a CPU feature. I
went with a machine feature because that is where "vmport" currently is.
>
> If it's required for the rpc device to be used -
> why not set this unconditionally if the rpc device
> is created? Not saying it's a good idea - just asking.
This is a complex question.
The VMware provided tools and open-vm-tools
(https://github.com/vmware/open-vm-tools) currently require it. However
arranging IOPL(3) in linux for these is not that hard if the user has
the correct privilege.
However the other vmport available actions do not require this to be
enabled, but can be accessed if enabled.
Xen does not need or use this patch. KVM (which I do not know a lot
about) also needs a change similar to this. So this is only for TCG case.
Paolo Bonzini said:
-------- Forwarded Message --------
Subject: Re: [PATCH v4 6/7] vmport: Add VMware all ring hack
Date: Thu, 30 Apr 2015 11:25:26 -0400
From: Paolo Bonzini <pbonzini@redhat.com>
To: Slutz, Donald Christopher <donald.slutz@one.verizon.com>,
qemu-devel@nongnu.org <qemu-devel@nongnu.org>
CC: Michael S. Tsirkin <mst@redhat.com>, Markus Armbruster
<armbru@redhat.com>, Luiz Capitulino <lcapitulino@redhat.com>, Anthony
Liguori <aliguori@amazon.com>, Andreas Färber <afaerber@suse.de>,
Richard Henderson <rth@twiddle.net>
On 30/04/2015 17:15, Don Slutz wrote:
>>> This is done by adding a new machine property vmware-port-ring3 that
>>> needs to be enabled to have any effect. It only effects accel=tcg
>>> mode. It is needed if you want to use VMware tools in accel=tcg
>>> mode.
>>
>> How does it work on KVM or Xen?
>
> Xen change is in progress (patch posted). I have not done any work on
> HVM yet.
Ok, no problem. Just wanted to be sure this is simply the TCG case.
For KVM we can piggyback on the same X86CPU variable, so it's better if
you do not use hflags2.
Paolo
So it may make more sense to switch to the CPU feature.
I do not think it makes sense to set this by default if the vmware_rpc
device exists (not at all sure how to get something like this coded).
-Don Slutz
>
>> @@ -161,7 +163,9 @@ extern int fd_bootchk;
>> void pc_register_ferr_irq(qemu_irq irq);
>> void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
>>
>> -void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge);
>> +/* vmware_port_ring3 true says enable VMware port access in ring3. */
>> +void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge,
>> + bool vmware_port_ring3);
>> void pc_hot_add_cpu(const int64_t id, Error **errp);
>> void pc_acpi_init(const char *default_dsdt);
>>
>> diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
>> index 7a4fddd..ccbc5bb 100644
>> --- a/target-i386/cpu-qom.h
>> +++ b/target-i386/cpu-qom.h
>> @@ -109,6 +109,9 @@ typedef struct X86CPU {
>> */
>> bool enable_pmu;
>>
>> + /* allow_vmport_ring3 true says enable VMware port access in ring3 */
>> + bool allow_vmport_ring3;
>> +
>> /* in order to simplify APIC support, we leave this pointer to the
>> user */
>> struct DeviceState *apic_state;
>> diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c
>> index 8a4271e..2ddb84f 100644
>> --- a/target-i386/seg_helper.c
>> +++ b/target-i386/seg_helper.c
>> @@ -2566,6 +2566,15 @@ static inline void check_io(CPUX86State *env, int addr, int size)
>> {
>> int io_offset, val, mask;
>>
>> + /* vmport hack: skip iopl checking for VMware port 0x5658 (see
>> + * vmport_realizefn()) */
>> + if (addr == 0x5658) {
>> + X86CPU *cpu = x86_env_get_cpu(env);
>> + if (cpu->allow_vmport_ring3) {
>> + return;
>> + }
>> + }
>> +
>> /* TSS must be a valid 32 bit one */
>> if (!(env->tr.flags & DESC_P_MASK) ||
>> ((env->tr.flags >> DESC_TYPE_SHIFT) & 0xf) != 9 ||
>> --
>> 1.8.3.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v8 8/8] vmport: Add VMware all ring hack
2015-07-03 13:08 ` Don Slutz
@ 2015-07-03 13:50 ` Paolo Bonzini
2015-07-05 6:40 ` Michael S. Tsirkin
0 siblings, 1 reply; 19+ messages in thread
From: Paolo Bonzini @ 2015-07-03 13:50 UTC (permalink / raw)
To: Don Slutz, Michael S. Tsirkin
Cc: qemu-devel, Markus Armbruster, Don Slutz, Luiz Capitulino,
Anthony Liguori, Andreas Färber, Richard Henderson
On 03/07/2015 15:08, Don Slutz wrote:
>>>
>> Creating it at the pc level and propagating makes code
>> messy but I'd go along with it if it made sense from
>> user's point of view, but it does not seem to make sense:
>> to me this looks more like a CPU feature than a machine property.
>> Or the property of the vmport rpc device that you created.
>
> Well this is not a clear area. It does look more like a CPU feature. I
> went with a machine feature because that is where "vmport" currently is.
I think it should be a vmport property.
It would be cleaner to implement it using the MemTxAttrs mechanism, but
for KVM I'm afraid it would be too slow (and the slowness would affect
all I/O accesses, not just vmport ones, unless really ugly hacks are done).
Paolo
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v8 6/8] vmport_rpc: Add QMP access to vmport_rpc object.
2015-07-02 14:11 ` [Qemu-devel] [PATCH v8 6/8] vmport_rpc: Add QMP access to vmport_rpc object Markus Armbruster
@ 2015-07-03 16:53 ` Don Slutz
2015-07-03 17:56 ` Markus Armbruster
0 siblings, 1 reply; 19+ messages in thread
From: Don Slutz @ 2015-07-03 16:53 UTC (permalink / raw)
To: Markus Armbruster
Cc: Michael S. Tsirkin, qemu-devel, Don Slutz, Luiz Capitulino,
Anthony Liguori, Paolo Bonzini, Andreas Färber,
Richard Henderson
On 07/02/15 10:11, Markus Armbruster wrote:
> Don Slutz <don.slutz@gmail.com> writes:
>
>> From: Don Slutz <dslutz@verizon.com>
>>
>> This adds one new inject command:
>>
>> inject-vmport-action
>>
>> And three guest info commands:
>>
>> vmport-guestinfo-set
>> vmport-guestinfo-get
>> query-vmport-guestinfo
>>
>> More details in qmp-commands.hx
>>
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>> CC: Don Slutz <don.slutz@gmail.com>
> Reviewing only the QAPI/QMP interface, plus a bit of drive-by shooting.
>
>> diff --git a/hw/misc/vmport_rpc.c b/hw/misc/vmport_rpc.c
>> index f157425..917898b 100644
>> --- a/hw/misc/vmport_rpc.c
>> +++ b/hw/misc/vmport_rpc.c
>> @@ -178,6 +178,19 @@ typedef struct VMPortRpcState {
>> #endif
>> } VMPortRpcState;
>>
>> +typedef int FindVMPortRpcDeviceFunc(VMPortRpcState *s, const void *arg);
>> +
>> +/*
>> + * The input and output argument that find_VMPortRpc_device() uses
>> + * to do its work.
>> + */
>> +typedef struct VMPortRpcFind {
>> + const void *arg;
>> + FindVMPortRpcDeviceFunc *func;
>> + int found;
>> + int rc;
>> +} VMPortRpcFind;
>> +
>> /* Basic request types */
>> typedef enum {
>> MESSAGE_TYPE_OPEN,
>> @@ -202,6 +215,56 @@ typedef struct {
>> uint32_t edi;
>> } vregs;
>>
>> +/*
>> + * Run func() for every VMPortRpc device, traverse the tree for
>> + * everything else.
>> + */
>> +static int find_VMPortRpc_device(Object *obj, void *opaque)
>> +{
>> + VMPortRpcFind *find = opaque;
>> + Object *dev;
>> + VMPortRpcState *s;
>> +
>> + assert(find);
>> + if (find->found) {
>> + return 0;
>> + }
>> + dev = object_dynamic_cast(obj, TYPE_VMPORT_RPC);
>> + s = (VMPortRpcState *)dev;
>> +
>> + if (!s) {
>> + /* Container, traverse it for children */
>> + return object_child_foreach(obj, find_VMPortRpc_device, opaque);
>> + }
>> +
>> + find->found++;
>> + find->rc = find->func(s, find->arg);
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * Loop through all dynamically created VMPortRpc devices and call
>> + * func() for each instance.
>> + */
>> +static int foreach_dynamic_vmport_rpc_device(FindVMPortRpcDeviceFunc *func,
>> + const void *arg)
>> +{
>> + VMPortRpcFind find = {
>> + .func = func,
>> + .arg = arg,
>> + };
>> +
>> + /* Loop through all VMPortRpc devices that were spawned outside
>> + * the machine */
>> + find_VMPortRpc_device(qdev_get_machine(), &find);
>> + if (find.found) {
>> + return find.rc;
>> + } else {
>> + return VMPORT_DEVICE_NOT_FOUND;
>> + }
>> +}
>> +
>> #ifdef VMPORT_RPC_DEBUG
>> /*
>> * Add helper function for tracing. This routine will convert
>> @@ -329,7 +392,7 @@ static int vmport_rpc_send(VMPortRpcState *s, channel_t *c,
>> return rc;
>> }
>>
>> -static int vmport_rpc_ctrl_send(VMPortRpcState *s, char *msg)
>> +static int vmport_rpc_ctrl_send(VMPortRpcState *s, const char *msg)
>> {
>> int rc = SEND_NOT_OPEN;
>> unsigned int i;
>> @@ -457,6 +520,29 @@ static int get_guestinfo(VMPortRpcState *s,
>> return GUESTINFO_NOTFOUND;
>> }
>>
>> +static int get_qmp_guestinfo(VMPortRpcState *s,
>> + unsigned int a_key_len, const char *a_info_key,
>> + unsigned int *a_value_len, void **a_value_data)
>> +{
>> + guestinfo_t *gi = NULL;
>> +
>> + if (a_key_len <= MAX_KEY_LEN) {
>> + gpointer key = g_strndup(a_info_key, a_key_len);
>> +
>> + gi = (guestinfo_t *)g_hash_table_lookup(s->guestinfo, key);
>> + g_free(key);
>> + } else {
>> + return GUESTINFO_KEYTOOLONG;
>> + }
>> + if (gi) {
>> + *a_value_len = gi->val_len;
>> + *a_value_data = gi->val_data;
>> + return 0;
>> + }
>> +
>> + return GUESTINFO_NOTFOUND;
>> +}
>> +
>> static int set_guestinfo(VMPortRpcState *s, int a_key_len,
>> unsigned int a_val_len, const char *a_info_key,
>> char *val)
>> @@ -775,7 +861,7 @@ static void vmport_rpc(VMPortRpcState *s , vregs *ur)
>> }
>> if (delta > s->reset_time) {
>> trace_vmport_rpc_ping(delta);
>> - vmport_rpc_ctrl_send(s, (char *)"reset");
>> + vmport_rpc_ctrl_send(s, "reset");
>> }
>> vmport_rpc_sweep(s, now_time);
>> do {
>> @@ -848,6 +934,206 @@ static uint32_t vmport_rpc_ioport_read(void *opaque, uint32_t addr)
>> return ur.data[0];
>> }
>>
>> +static int vmport_rpc_find_send(VMPortRpcState *s, const void *arg)
>> +{
>> + return vmport_rpc_ctrl_send(s, arg);
>> +}
>> +
>> +static void convert_local_rc(Error **errp, int rc)
>> +{
>> + switch (rc) {
>> + case 0:
>> + break;
>> + case VMPORT_DEVICE_NOT_FOUND:
>> + error_set(errp, QERR_DEVICE_NOT_FOUND, TYPE_VMPORT_RPC);
>> + break;
> Conflicts with
>
> commit 75158ebbe259f0bd8bf435e8f4827a43ec89c877
> Author: Markus Armbruster <armbru@redhat.com>
> Date: Mon Mar 16 08:57:47 2015 +0100
>
> qerror: Eliminate QERR_DEVICE_NOT_FOUND
>
> Error classes other than ERROR_CLASS_GENERIC_ERROR should not be used
> in new code. Hiding them in QERR_ macros makes new uses hard to spot.
> Fortunately, there's just one such macro left. Eliminate it with this
> coccinelle semantic patch:
>
> @@
> expression EP, E;
> @@
> -error_set(EP, QERR_DEVICE_NOT_FOUND, E)
> +error_set(EP, ERROR_CLASS_DEVICE_NOT_FOUND, "Device '%s' not found", E)
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
>
> Sorry if you explained this already for previous revisions: what's the
> justification for ERROR_CLASS_DEVICE_NOT_FOUND? Can you stick to
> ERROR_CLASS_GENERIC_ERROR?
I was only coping code from other places. Happy to convert to error_setg().
>> + case SEND_NOT_OPEN:
>> + error_setg(errp, "VMWare rpc not open");
>> + break;
>> + case SEND_SKIPPED:
>> + error_setg(errp, "VMWare rpc send skipped");
>> + break;
>> + case SEND_TRUCATED:
>> + error_setg(errp, "VMWare rpc send trucated");
>> + break;
>> + case SEND_NO_MEMORY:
>> + error_setg(errp, "VMWare rpc send out of memory");
>> + break;
>> + case GUESTINFO_NOTFOUND:
>> + error_setg(errp, "VMWare guestinfo not found");
>> + break;
>> + case GUESTINFO_VALTOOLONG:
>> + error_setg(errp, "VMWare guestinfo value too long");
>> + break;
>> + case GUESTINFO_KEYTOOLONG:
>> + error_setg(errp, "VMWare guestinfo key too long");
>> + break;
>> + case GUESTINFO_TOOMANYKEYS:
>> + error_setg(errp, "VMWare guestinfo too many keys");
>> + break;
>> + case GUESTINFO_NO_MEMORY:
>> + error_setg(errp, "VMWare guestinfo out of memory");
>> + break;
>> + default:
>> + error_setg(errp, "VMWare rpc send rc=%d unknown", rc);
>> + break;
>> + }
>> +}
>> +
>> +void qmp_inject_vmport_action(enum VmportAction action, Error **errp)
>> +{
>> + int rc;
>> +
>> + switch (action) {
>> + case VMPORT_ACTION_REBOOT:
>> + rc = foreach_dynamic_vmport_rpc_device(vmport_rpc_find_send,
>> + "OS_Reboot");
>> + break;
>> + case VMPORT_ACTION_HALT:
>> + rc = foreach_dynamic_vmport_rpc_device(vmport_rpc_find_send,
>> + "OS_Halt");
>> + break;
>> + case VMPORT_ACTION_MAX:
>> + assert(action != VMPORT_ACTION_MAX);
> If this fails, you likely found a major compiler bug.
Nope. That assert() does happen:
TestCloud1:~/tmp>cat zza.c
#include <stdio.h>
#include <assert.h>
#include <stdlib.h>
typedef enum VmportAction
{
VMPORT_ACTION_REBOOT = 0,
VMPORT_ACTION_HALT = 1,
VMPORT_ACTION_MAX = 2,
} VmportAction;
int
main(void)
{
enum VmportAction action;
for (action = VMPORT_ACTION_REBOOT; ; action++) {
switch (action) {
case VMPORT_ACTION_REBOOT:
printf("reboot\n");
break;
case VMPORT_ACTION_HALT:
printf("halt\n");
break;
case VMPORT_ACTION_MAX:
printf("max\n");
assert(action != VMPORT_ACTION_MAX);
abort(); /* Should be impossible to get here. */
}
}
return 0;
}
TestCloud1:~/tmp>gcc -Wall -o zza zza.c
TestCloud1:~/tmp>./zza
reboot
halt
max
zza: zza.c:28: main: Assertion `action != VMPORT_ACTION_MAX' failed.
Aborted (core dumped)
>
>> + abort(); /* Should be impossible to get here. */
> Why not simply
>
> default:
> abort();
Will change to this. abort() was added in v7.
>> + }
>> + convert_local_rc(errp, rc);
>> +}
>> +
>> +typedef struct keyValue {
>> + const char *key_data;
>> + void *value_data;
>> + unsigned int key_len;
>> + unsigned int value_len;
>> +} keyValue;
>> +
>> +static int find_set(VMPortRpcState *s, const void *arg)
>> +{
>> + const keyValue *key_value = arg;
>> +
>> + return set_guestinfo(s, key_value->key_len, key_value->value_len,
>> + key_value->key_data, key_value->value_data);
>> +}
>> +
>> +static int find_get(VMPortRpcState *s, const void *arg)
>> +{
>> + keyValue *key_value = (void *)arg;
>> +
>> + return get_qmp_guestinfo(s, key_value->key_len, key_value->key_data,
>> + &key_value->value_len, &key_value->value_data);
>> +}
>> +
>> +void qmp_vmport_guestinfo_set(const char *key, const char *value,
>> + bool has_format, enum DataFormat format,
>> + Error **errp)
>> +{
>> + int rc;
>> + keyValue key_value;
>> +
>> + if (strncmp(key, "guestinfo.", strlen("guestinfo.")) == 0) {
>> + key_value.key_data = (key + strlen("guestinfo."));
>> + key_value.key_len = strlen(key) - strlen("guestinfo.");
>> + } else {
>> + key_value.key_data = key;
>> + key_value.key_len = strlen(key);
>> + }
>> + if (has_format && (format == DATA_FORMAT_BASE64)) {
>> + gsize write_count;
>> +
>> + key_value.value_data = g_base64_decode(value, &write_count);
>> + key_value.value_len = write_count;
>> + } else {
>> + key_value.value_data = (void *)value;
>> + key_value.value_len = strlen(value);
>> + }
>> +
>> + rc = foreach_dynamic_vmport_rpc_device(find_set, &key_value);
>> +
>> + if (key_value.value_data != value) {
>> + g_free(key_value.value_data);
>> + }
>> +
>> + if (rc) {
>> + convert_local_rc(errp, rc);
>> + return;
>> + }
>> +}
>> +
>> +VmportGuestInfoValue *qmp_vmport_guestinfo_get(const char *key,
>> + bool has_format,
>> + enum DataFormat format,
>> + Error **errp)
>> +{
>> + int rc;
>> + keyValue key_value;
>> + VmportGuestInfoValue *ret_value;
>> +
>> + if (strncmp(key, "guestinfo.", strlen("guestinfo.")) == 0) {
>> + key_value.key_data = (key + strlen("guestinfo."));
>> + key_value.key_len = strlen(key) - strlen("guestinfo.");
>> + } else {
>> + key_value.key_data = key;
>> + key_value.key_len = strlen(key);
>> + }
>> +
>> + rc = foreach_dynamic_vmport_rpc_device(find_get, &key_value);
>> + if (rc) {
>> + convert_local_rc(errp, rc);
>> + return NULL;
>> + }
>> +
>> + ret_value = g_new0(VmportGuestInfoValue, 1);
>> + if (has_format && (format == DATA_FORMAT_BASE64)) {
>> + ret_value->value = g_base64_encode(key_value.value_data,
>> + key_value.value_len);
>> + } else {
>> + /*
>> + * FIXME should check for complete, valid UTF-8 characters.
>> + * Invalid sequences should be replaced by a suitable
>> + * replacement character. Or cause an error to be raised.
>> + */
> Thanks for copying the FIXME from qmp_ringbuf_read(), I appreciate it.
>
>> + ret_value->value = g_malloc(key_value.value_len + 1);
>> + memcpy(ret_value->value, key_value.value_data, key_value.value_len);
>> + ret_value->value[key_value.value_len] = 0;
>> + }
>> +
>> + return ret_value;
>> +}
>> +
>> +
>> +static void vmport_rpc_find_list_one(gpointer key, gpointer value,
>> + gpointer opaque)
>> +{
>> + VmportGuestInfoKeyList **keys = opaque;
>> + VmportGuestInfoKeyList *info = g_new0(VmportGuestInfoKeyList, 1);
>> + char *ckey = key;
>> +
>> + info->value = g_new0(VmportGuestInfoKey, 1);
>> + info->value->key = g_strdup_printf("guestinfo.%s", ckey);
>> + info->next = *keys;
>> + *keys = info;
>> +}
>> +
>> +static int vmport_rpc_find_list(VMPortRpcState *s, const void *arg)
>> +{
>> + g_hash_table_foreach(s->guestinfo, vmport_rpc_find_list_one, (gpointer)arg);
>> + return 0;
>> +}
>> +
>> +VmportGuestInfoKeyList *qmp_query_vmport_guestinfo(Error **errp)
>> +{
>> + VmportGuestInfoKeyList *keys = NULL;
>> + int rc = foreach_dynamic_vmport_rpc_device(vmport_rpc_find_list,
>> + (void *)&keys);
>> +
>> + if (rc) {
>> + convert_local_rc(errp, rc);
>> + }
>> +
>> + return keys;
>> +}
>> +
>> +
>> static void vmport_rpc_reset(DeviceState *d)
>> {
>> unsigned int i;
>> diff --git a/monitor.c b/monitor.c
>> index aeea2b5..4224314 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -5360,4 +5360,28 @@ void qmp_rtc_reset_reinjection(Error **errp)
>> {
>> error_setg(errp, QERR_FEATURE_DISABLED, "rtc-reset-reinjection");
>> }
>> +void qmp_inject_vmport_action(enum VmportAction action, Error **errp)
>> +{
>> + error_set(errp, QERR_FEATURE_DISABLED, "inject-vmport-action");
>> +}
>> +void qmp_vmport_guestinfo_set(const char *key, const char *value,
>> + bool has_format, enum DataFormat format,
>> + Error **errp)
>> +{
>> + error_set(errp, QERR_FEATURE_DISABLED, "vmport-guestinfo-set");
>> +}
>> +VmportGuestInfoValue *qmp_vmport_guestinfo_get(const char *key,
>> + bool has_format,
>> + enum DataFormat format,
>> + Error **errp)
>> +{
>> + error_set(errp, QERR_FEATURE_DISABLED, "vmport-guestinfo-get");
>> + return NULL;
>> +}
>> +VmportGuestInfoKeyList *qmp_query_vmport_guestinfo(Error **errp)
>> +{
>> + error_set(errp, QERR_FEATURE_DISABLED, "query-vmport-guestinfo");
>> + return NULL;
>> +}
> I understand you're just following existing practice here, but I can
> help to wonder whether the practice is smart. The command exists in all
> compile-time configurations, but it works in only some. To figure out
> whether it works, you have to try it.
>
> If we add the command only when it actually works, you can use
> query-commands instead.
>
> This isn't a demand for you to change anything now. We QMP guys need to
> decide how we want this done.
>
>> #endif
> Since the #ifdef section is now longer than a few lines,
>
> -#endif
> +#endif /* !TARGET_I386 */
>
> would be nice
Will add.
>> +
> No blank line at end of file, please.
Will drop.
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 106008c..5d9e9e2 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -1429,6 +1429,107 @@
>> { 'command': 'inject-nmi' }
>>
>> ##
>> +# @VmportAction:
>> +#
>> +# An enumeration of actions that can be requested via vmport RPC.
>> +#
>> +# @reboot: Ask the guest via VMware tools to reboot
>> +#
>> +# @halt: Ask the guest via VMware tools to halt
>> +#
>> +# Since: 2.4
>> +##
>> +{ 'enum': 'VmportAction',
>> + 'data': [ 'reboot', 'halt' ] }
>> +
>> +##
>> +# @inject-vmport-action:
>> +#
>> +# Injects a VMWare Tools action to the guest.
>> +#
>> +# Returns: If successful, nothing
>> +#
>> +# Since: 2.4
>> +#
>> +##
>> +{ 'command': 'inject-vmport-action',
>> + 'data': {'action': 'VmportAction'} }
>> +
>> +##
>> +# @vmport-guestinfo-set:
>> +#
>> +# Set a VMWare Tools guestinfo key to a value
>> +#
>> +# @key: the key to set
>> +#
>> +# @value: The data to set the key to
>> +#
>> +# @format: #optional value encoding (default 'utf8').
>> +# - base64: value is assumed to be base64 encoded text. Its binary
>> +# decoding gets set.
> Shouldn't you copy ringbuf-write's bug note?
>
> # Bug: invalid base64 is currently not rejected.
>
Nope. From the cover letter:
----------------------------------------------------------------------------------------
Much more on format=base64:
If there is a bug it is in GLIB. However the Glib reference manual
refers to RFC 1421 and RFC 2045 and MIME encoding. Based on all
that (which seems to match:
http://en.wikipedia.org/wiki/Base64
) MIME states that all characters outside the (base64) alphabet are
to be ignored. Testing shows that g_base64_decode() does this.
The confusion is that most non-MIME uses reject a base64 string that
contain characters outside the alphabet. I was just following the
other uses of base64 in this file.
DataFormat refers to RFC 3548, which has the info:
"
Implementations MUST reject the encoding if it contains
characters outside the base alphabet when interpreting base
encoded data, unless the specification referring to this document
explicitly states otherwise. Such specifications may, as MIME
does, instead state that characters outside the base encoding
alphabet should simply be ignored when interpreting data ("be
liberal in what you accept").
"
So with GLIB going the MIME way, I do not think this is a QEMU bug
(you could consider this a GLIB bug, but the document I found says
that GLIB goes the MIME way and so does not reject anything).
----------------------------------------------------------------------------------------
Based on all this, I did drop it.
>> +# - utf8: value's UTF-8 encoding is used to set.
>> +#
>> +# Returns: Nothing on success
>> +#
>> +# Since: 2.4
>> +##
>> +{ 'command': 'vmport-guestinfo-set',
>> + 'data': {'key': 'str', 'value': 'str',
>> + '*format': 'DataFormat'} }
>> +
>> +##
>> +# @VmportGuestInfoValue:
>> +#
>> +# Information about a single VMWare Tools guestinfo
>> +#
>> +# @value: The value for a key
>> +#
>> +# Since: 2.4
>> +##
>> +{ 'struct': 'VmportGuestInfoValue', 'data': {'value': 'str'} }
>> +
>> +##
>> +# @vmport-guestinfo-get:
>> +#
>> +# Get a VMWare Tools guestinfo value for a key
>> +#
>> +# @key: the key to get
> Ignorant question: is the set of keys fixed (e.g. specified by some
> authority), completely dynamic (i.e. whatever has been set), or
> something else?
completely dynamic
>> +#
>> +# @format: #optional data encoding (default 'utf8').
>> +# - base64: the value is returned in base64 encoding.
>> +# - utf8: the value is interpreted as UTF-8.
>> +#
>> +# Returns: value for the guest info key
> ringbuf-read carries this bug note:
>
> # Bug: can screw up when the buffer contains invalid UTF-8
> # sequences, NUL characters, after the ring buffer lost
> # data, and when reading stops because the size limit is
> # reached.
>
> Doesn't it apply here, too, with "the buffer" replaced?
Not directly. The best description I have so far to match the FIXME
would be:
-----------------------------------------------------------------------------------
Bug: format=utf8 can cause QEMU to screw up (may crash) when the value
contains invalid UTF-8
sequences. NUL characters will cause loss of data.
----------------------------------------------------------------------------------
I have not spent the time investigating the issues with QMP and invalid
utf8 because it looks to be very wide spread and not just restricted to
this one routine.
>> +#
>> +# Since: 2.4
>> +##
>> +{ 'command': 'vmport-guestinfo-get',
>> + 'data': {'key': 'str', '*format': 'DataFormat'},
>> + 'returns': 'VmportGuestInfoValue' }
> I gather you wrap vmport-guestinfo-get's return value in a struct for
> extensibility. Can't see why and how we'd want to extend it, but better
> safe than sorry.
>
>> +
>> +##
>> +# @VmportGuestInfoKey:
>> +#
>> +# Information about a single VMWare Tools guestinfo
>> +#
>> +# @key: The known key
>> +#
>> +# Since: 2.4
>> +##
>> +{ 'struct': 'VmportGuestInfoKey', 'data': {'key': 'str'} }
>> +
>> +##
>> +# @query-vmport-guestinfo:
>> +#
>> +# Returns information about VMWare Tools guestinfo
>> +#
>> +# Returns: a list of @VmportGuestInfoKey
>> +#
>> +# Since: 2.4
>> +##
>> +{ 'command': 'query-vmport-guestinfo', 'returns': ['VmportGuestInfoKey'] }
> Similar. Imagining an extension of a string key is even harder. Okay.
>
>> +
>> +##
>> # @set_link:
>> #
>> # Sets the link status of a virtual network adapter.
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index a05d25f..a43db8b 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -490,6 +490,125 @@ Note: inject-nmi fails when the guest doesn't support injecting.
>> EQMP
>>
>> {
>> + .name = "inject-vmport-action",
>> + .args_type = "action:s",
>> + .mhandler.cmd_new = qmp_marshal_input_inject_vmport_action,
>> + },
>> +
>> +SQMP
>> +inject-vmport-action
>> +--------------------
>> +
>> +Inject a VMWare Tools action to the guest.
>> +
>> +Arguments:
>> +
>> +- "action": vmport action (json-string)
>> + - Possible values: "reboot", "halt"
>> +
>> +Example:
>> +
>> +-> { "execute": "inject-vmport-action",
>> + "arguments": { "action": "reboot" } }
>> +<- { "return": {} }
>> +
>> +Note: inject-vmport-action fails when the guest doesn't support injecting.
>> +
>> +EQMP
>> +
>> + {
>> + .name = "vmport-guestinfo-set",
>> + .args_type = "key:s,value:s,format:s?",
>> + .mhandler.cmd_new = qmp_marshal_input_vmport_guestinfo_set,
>> + },
>> +
>> +SQMP
>> +vmport-guestinfo-set
>> +--------------------
>> +
>> +Set a VMWare Tools guestinfo key to a value
>> +
>> +Arguments:
>> +
>> +- "key": the key to set (json-string)
>> +- "value": data to write (json-string)
>> +- "format": data format (json-string, optional)
>> + - Possible values: "utf8" (default), "base64"
> Shouldn't you copy ringbuf-write's bug note?
>
> Bug: invalid base64 is currently not rejected.
> Whitespace *is* invalid.
See above about format=base64
>> +
>> +Example:
>> +
>> +-> { "execute": "vmport-guestinfo-set",
>> + "arguments": { "key": "guestinfo.foo",
>> + "value": "abcdefgh",
>> + "format": "utf8" } }
>> +<- { "return": {} }
>> +
>> +EQMP
>> +
>> + {
>> + .name = "vmport-guestinfo-get",
>> + .args_type = "key:s,format:s?",
>> + .mhandler.cmd_new = qmp_marshal_input_vmport_guestinfo_get,
>> + },
>> +
>> +SQMP
>> +vmport-guestinfo-get
>> +--------------------
>> +
>> +Get a VMWare Tools guestinfo value for a key
>> +
>> +Arguments:
>> +
>> +- "key": the key to get (json-string)
>> +- "format": data format (json-string, optional)
>> + - Possible values: "utf8" (default), "base64"
>> + - Naturally, format "utf8" works only when the value
>> + contains valid UTF-8 text. Bug: replacement doesn't work.
>> + Bug: can screw up on encountering NUL characters.
> You adapted ringbuf-read's bug note. Good.
>
>> +
>> +Example:
>> +
>> +-> { "execute": "vmport-guestinfo-get",
>> + "arguments": { "key": "guestinfo.foo",
>> + "format": "utf8" } }
>> +<- {"return": {"value": "abcdefgh"}}
>> +
>> +
>> +EQMP
>> +
>> + {
>> + .name = "query-vmport-guestinfo",
>> + .args_type = "",
>> + .mhandler.cmd_new = qmp_marshal_input_query_vmport_guestinfo,
>> + },
>> +
>> +SQMP
>> +query-vmport-guestinfo
>> +----------------------
>> +
>> +Returns information about VMWare Tools guestinfo. The returned value is a json-array
> Long line, please wrap.
Will do.
>> +of all keys.
>> +
>> +Example:
>> +
>> +-> { "execute": "query-vmport-guestinfo" }
>> +<- {
>> + "return": [
>> + {
>> + "key": "guestinfo.ip"
>> + },
>> + {
>> + "key": "guestinfo.foo"
>> + },
>> + {
>> + "key": "guestinfo.long"
>> + }
>> + ]
>> + }
>> +
>> +EQMP
>> +
>> + {
>> .name = "ringbuf-write",
>> .args_type = "device:s,data:s,format:s?",
>> .mhandler.cmd_new = qmp_marshal_input_ringbuf_write,
> I guess I'd split this into inject-vmport-action and the guestinfo
> stuff. Matter of taste.
ok. No plans to do so now.
-Don Slutz
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v8 6/8] vmport_rpc: Add QMP access to vmport_rpc object.
2015-07-03 16:53 ` Don Slutz
@ 2015-07-03 17:56 ` Markus Armbruster
2015-07-03 21:26 ` Don Slutz
0 siblings, 1 reply; 19+ messages in thread
From: Markus Armbruster @ 2015-07-03 17:56 UTC (permalink / raw)
To: Don Slutz
Cc: Michael S. Tsirkin, qemu-devel, Don Slutz, Luiz Capitulino,
Anthony Liguori, Paolo Bonzini, Andreas Färber,
Richard Henderson
Don Slutz <don.slutz@gmail.com> writes:
> On 07/02/15 10:11, Markus Armbruster wrote:
>> Don Slutz <don.slutz@gmail.com> writes:
>>
>>> From: Don Slutz <dslutz@verizon.com>
>>>
>>> This adds one new inject command:
>>>
>>> inject-vmport-action
>>>
>>> And three guest info commands:
>>>
>>> vmport-guestinfo-set
>>> vmport-guestinfo-get
>>> query-vmport-guestinfo
>>>
>>> More details in qmp-commands.hx
>>>
>>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>>> CC: Don Slutz <don.slutz@gmail.com>
>> Reviewing only the QAPI/QMP interface, plus a bit of drive-by shooting.
>>
>>> diff --git a/hw/misc/vmport_rpc.c b/hw/misc/vmport_rpc.c
>>> index f157425..917898b 100644
>>> --- a/hw/misc/vmport_rpc.c
>>> +++ b/hw/misc/vmport_rpc.c
>>> @@ -178,6 +178,19 @@ typedef struct VMPortRpcState {
>>> #endif
>>> } VMPortRpcState;
>>> +typedef int FindVMPortRpcDeviceFunc(VMPortRpcState *s, const
>>> void *arg);
>>> +
>>> +/*
>>> + * The input and output argument that find_VMPortRpc_device() uses
>>> + * to do its work.
>>> + */
>>> +typedef struct VMPortRpcFind {
>>> + const void *arg;
>>> + FindVMPortRpcDeviceFunc *func;
>>> + int found;
>>> + int rc;
>>> +} VMPortRpcFind;
>>> +
>>> /* Basic request types */
>>> typedef enum {
>>> MESSAGE_TYPE_OPEN,
>>> @@ -202,6 +215,56 @@ typedef struct {
>>> uint32_t edi;
>>> } vregs;
>>> +/*
>>> + * Run func() for every VMPortRpc device, traverse the tree for
>>> + * everything else.
>>> + */
>>> +static int find_VMPortRpc_device(Object *obj, void *opaque)
>>> +{
>>> + VMPortRpcFind *find = opaque;
>>> + Object *dev;
>>> + VMPortRpcState *s;
>>> +
>>> + assert(find);
>>> + if (find->found) {
>>> + return 0;
>>> + }
>>> + dev = object_dynamic_cast(obj, TYPE_VMPORT_RPC);
>>> + s = (VMPortRpcState *)dev;
>>> +
>>> + if (!s) {
>>> + /* Container, traverse it for children */
>>> + return object_child_foreach(obj, find_VMPortRpc_device, opaque);
>>> + }
>>> +
>>> + find->found++;
>>> + find->rc = find->func(s, find->arg);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +/*
>>> + * Loop through all dynamically created VMPortRpc devices and call
>>> + * func() for each instance.
>>> + */
>>> +static int foreach_dynamic_vmport_rpc_device(FindVMPortRpcDeviceFunc *func,
>>> + const void *arg)
>>> +{
>>> + VMPortRpcFind find = {
>>> + .func = func,
>>> + .arg = arg,
>>> + };
>>> +
>>> + /* Loop through all VMPortRpc devices that were spawned outside
>>> + * the machine */
>>> + find_VMPortRpc_device(qdev_get_machine(), &find);
>>> + if (find.found) {
>>> + return find.rc;
>>> + } else {
>>> + return VMPORT_DEVICE_NOT_FOUND;
>>> + }
>>> +}
>>> +
>>> #ifdef VMPORT_RPC_DEBUG
>>> /*
>>> * Add helper function for tracing. This routine will convert
>>> @@ -329,7 +392,7 @@ static int vmport_rpc_send(VMPortRpcState *s, channel_t *c,
>>> return rc;
>>> }
>>> -static int vmport_rpc_ctrl_send(VMPortRpcState *s, char *msg)
>>> +static int vmport_rpc_ctrl_send(VMPortRpcState *s, const char *msg)
>>> {
>>> int rc = SEND_NOT_OPEN;
>>> unsigned int i;
>>> @@ -457,6 +520,29 @@ static int get_guestinfo(VMPortRpcState *s,
>>> return GUESTINFO_NOTFOUND;
>>> }
>>> +static int get_qmp_guestinfo(VMPortRpcState *s,
>>> + unsigned int a_key_len, const char *a_info_key,
>>> + unsigned int *a_value_len, void **a_value_data)
>>> +{
>>> + guestinfo_t *gi = NULL;
>>> +
>>> + if (a_key_len <= MAX_KEY_LEN) {
>>> + gpointer key = g_strndup(a_info_key, a_key_len);
>>> +
>>> + gi = (guestinfo_t *)g_hash_table_lookup(s->guestinfo, key);
>>> + g_free(key);
>>> + } else {
>>> + return GUESTINFO_KEYTOOLONG;
>>> + }
>>> + if (gi) {
>>> + *a_value_len = gi->val_len;
>>> + *a_value_data = gi->val_data;
>>> + return 0;
>>> + }
>>> +
>>> + return GUESTINFO_NOTFOUND;
>>> +}
>>> +
>>> static int set_guestinfo(VMPortRpcState *s, int a_key_len,
>>> unsigned int a_val_len, const char *a_info_key,
>>> char *val)
>>> @@ -775,7 +861,7 @@ static void vmport_rpc(VMPortRpcState *s , vregs *ur)
>>> }
>>> if (delta > s->reset_time) {
>>> trace_vmport_rpc_ping(delta);
>>> - vmport_rpc_ctrl_send(s, (char *)"reset");
>>> + vmport_rpc_ctrl_send(s, "reset");
>>> }
>>> vmport_rpc_sweep(s, now_time);
>>> do {
>>> @@ -848,6 +934,206 @@ static uint32_t vmport_rpc_ioport_read(void *opaque, uint32_t addr)
>>> return ur.data[0];
>>> }
>>> +static int vmport_rpc_find_send(VMPortRpcState *s, const void
>>> *arg)
>>> +{
>>> + return vmport_rpc_ctrl_send(s, arg);
>>> +}
>>> +
>>> +static void convert_local_rc(Error **errp, int rc)
>>> +{
>>> + switch (rc) {
>>> + case 0:
>>> + break;
>>> + case VMPORT_DEVICE_NOT_FOUND:
>>> + error_set(errp, QERR_DEVICE_NOT_FOUND, TYPE_VMPORT_RPC);
>>> + break;
>> Conflicts with
>>
>> commit 75158ebbe259f0bd8bf435e8f4827a43ec89c877
>> Author: Markus Armbruster <armbru@redhat.com>
>> Date: Mon Mar 16 08:57:47 2015 +0100
>>
>> qerror: Eliminate QERR_DEVICE_NOT_FOUND
>> Error classes other than ERROR_CLASS_GENERIC_ERROR should
>> not be used
>> in new code. Hiding them in QERR_ macros makes new uses hard to spot.
>> Fortunately, there's just one such macro left. Eliminate it with this
>> coccinelle semantic patch:
>> @@
>> expression EP, E;
>> @@
>> -error_set(EP, QERR_DEVICE_NOT_FOUND, E)
>> +error_set(EP, ERROR_CLASS_DEVICE_NOT_FOUND, "Device '%s' not found", E)
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
>>
>> Sorry if you explained this already for previous revisions: what's the
>> justification for ERROR_CLASS_DEVICE_NOT_FOUND? Can you stick to
>> ERROR_CLASS_GENERIC_ERROR?
>
> I was only coping code from other places. Happy to convert to error_setg().
Please do.
>>> + case SEND_NOT_OPEN:
>>> + error_setg(errp, "VMWare rpc not open");
>>> + break;
>>> + case SEND_SKIPPED:
>>> + error_setg(errp, "VMWare rpc send skipped");
>>> + break;
>>> + case SEND_TRUCATED:
>>> + error_setg(errp, "VMWare rpc send trucated");
>>> + break;
>>> + case SEND_NO_MEMORY:
>>> + error_setg(errp, "VMWare rpc send out of memory");
>>> + break;
>>> + case GUESTINFO_NOTFOUND:
>>> + error_setg(errp, "VMWare guestinfo not found");
>>> + break;
>>> + case GUESTINFO_VALTOOLONG:
>>> + error_setg(errp, "VMWare guestinfo value too long");
>>> + break;
>>> + case GUESTINFO_KEYTOOLONG:
>>> + error_setg(errp, "VMWare guestinfo key too long");
>>> + break;
>>> + case GUESTINFO_TOOMANYKEYS:
>>> + error_setg(errp, "VMWare guestinfo too many keys");
>>> + break;
>>> + case GUESTINFO_NO_MEMORY:
>>> + error_setg(errp, "VMWare guestinfo out of memory");
>>> + break;
>>> + default:
>>> + error_setg(errp, "VMWare rpc send rc=%d unknown", rc);
>>> + break;
>>> + }
>>> +}
>>> +
>>> +void qmp_inject_vmport_action(enum VmportAction action, Error **errp)
>>> +{
>>> + int rc;
>>> +
>>> + switch (action) {
>>> + case VMPORT_ACTION_REBOOT:
>>> + rc = foreach_dynamic_vmport_rpc_device(vmport_rpc_find_send,
>>> + "OS_Reboot");
>>> + break;
>>> + case VMPORT_ACTION_HALT:
>>> + rc = foreach_dynamic_vmport_rpc_device(vmport_rpc_find_send,
>>> + "OS_Halt");
>>> + break;
>>> + case VMPORT_ACTION_MAX:
>>> + assert(action != VMPORT_ACTION_MAX);
>> If this fails, you likely found a major compiler bug.
>
> Nope. That assert() does happen:
Uh, I misread != for ==. The assert *always* fails. Sorry!
>>> + abort(); /* Should be impossible to get here. */
>> Why not simply
>>
>> default:
>> abort();
>
> Will change to this. abort() was added in v7.
Thanks!
>>> + }
>>> + convert_local_rc(errp, rc);
>>> +}
>>> +
>>> +typedef struct keyValue {
>>> + const char *key_data;
>>> + void *value_data;
>>> + unsigned int key_len;
>>> + unsigned int value_len;
>>> +} keyValue;
>>> +
>>> +static int find_set(VMPortRpcState *s, const void *arg)
>>> +{
>>> + const keyValue *key_value = arg;
>>> +
>>> + return set_guestinfo(s, key_value->key_len, key_value->value_len,
>>> + key_value->key_data, key_value->value_data);
>>> +}
>>> +
>>> +static int find_get(VMPortRpcState *s, const void *arg)
>>> +{
>>> + keyValue *key_value = (void *)arg;
>>> +
>>> + return get_qmp_guestinfo(s, key_value->key_len, key_value->key_data,
>>> + &key_value->value_len, &key_value->value_data);
>>> +}
>>> +
>>> +void qmp_vmport_guestinfo_set(const char *key, const char *value,
>>> + bool has_format, enum DataFormat format,
>>> + Error **errp)
>>> +{
>>> + int rc;
>>> + keyValue key_value;
>>> +
>>> + if (strncmp(key, "guestinfo.", strlen("guestinfo.")) == 0) {
>>> + key_value.key_data = (key + strlen("guestinfo."));
>>> + key_value.key_len = strlen(key) - strlen("guestinfo.");
>>> + } else {
>>> + key_value.key_data = key;
>>> + key_value.key_len = strlen(key);
>>> + }
>>> + if (has_format && (format == DATA_FORMAT_BASE64)) {
>>> + gsize write_count;
>>> +
>>> + key_value.value_data = g_base64_decode(value, &write_count);
>>> + key_value.value_len = write_count;
>>> + } else {
>>> + key_value.value_data = (void *)value;
>>> + key_value.value_len = strlen(value);
>>> + }
>>> +
>>> + rc = foreach_dynamic_vmport_rpc_device(find_set, &key_value);
>>> +
>>> + if (key_value.value_data != value) {
>>> + g_free(key_value.value_data);
>>> + }
>>> +
>>> + if (rc) {
>>> + convert_local_rc(errp, rc);
>>> + return;
>>> + }
>>> +}
>>> +
>>> +VmportGuestInfoValue *qmp_vmport_guestinfo_get(const char *key,
>>> + bool has_format,
>>> + enum DataFormat format,
>>> + Error **errp)
>>> +{
>>> + int rc;
>>> + keyValue key_value;
>>> + VmportGuestInfoValue *ret_value;
>>> +
>>> + if (strncmp(key, "guestinfo.", strlen("guestinfo.")) == 0) {
>>> + key_value.key_data = (key + strlen("guestinfo."));
>>> + key_value.key_len = strlen(key) - strlen("guestinfo.");
>>> + } else {
>>> + key_value.key_data = key;
>>> + key_value.key_len = strlen(key);
>>> + }
>>> +
>>> + rc = foreach_dynamic_vmport_rpc_device(find_get, &key_value);
>>> + if (rc) {
>>> + convert_local_rc(errp, rc);
>>> + return NULL;
>>> + }
>>> +
>>> + ret_value = g_new0(VmportGuestInfoValue, 1);
>>> + if (has_format && (format == DATA_FORMAT_BASE64)) {
>>> + ret_value->value = g_base64_encode(key_value.value_data,
>>> + key_value.value_len);
>>> + } else {
>>> + /*
>>> + * FIXME should check for complete, valid UTF-8 characters.
>>> + * Invalid sequences should be replaced by a suitable
>>> + * replacement character. Or cause an error to be raised.
>>> + */
>> Thanks for copying the FIXME from qmp_ringbuf_read(), I appreciate it.
>>
>>> + ret_value->value = g_malloc(key_value.value_len + 1);
>>> + memcpy(ret_value->value, key_value.value_data, key_value.value_len);
>>> + ret_value->value[key_value.value_len] = 0;
>>> + }
>>> +
>>> + return ret_value;
>>> +}
>>> +
>>> +
>>> +static void vmport_rpc_find_list_one(gpointer key, gpointer value,
>>> + gpointer opaque)
>>> +{
>>> + VmportGuestInfoKeyList **keys = opaque;
>>> + VmportGuestInfoKeyList *info = g_new0(VmportGuestInfoKeyList, 1);
>>> + char *ckey = key;
>>> +
>>> + info->value = g_new0(VmportGuestInfoKey, 1);
>>> + info->value->key = g_strdup_printf("guestinfo.%s", ckey);
>>> + info->next = *keys;
>>> + *keys = info;
>>> +}
>>> +
>>> +static int vmport_rpc_find_list(VMPortRpcState *s, const void *arg)
>>> +{
>>> + g_hash_table_foreach(s->guestinfo, vmport_rpc_find_list_one, (gpointer)arg);
>>> + return 0;
>>> +}
>>> +
>>> +VmportGuestInfoKeyList *qmp_query_vmport_guestinfo(Error **errp)
>>> +{
>>> + VmportGuestInfoKeyList *keys = NULL;
>>> + int rc = foreach_dynamic_vmport_rpc_device(vmport_rpc_find_list,
>>> + (void *)&keys);
>>> +
>>> + if (rc) {
>>> + convert_local_rc(errp, rc);
>>> + }
>>> +
>>> + return keys;
>>> +}
>>> +
>>> +
>>> static void vmport_rpc_reset(DeviceState *d)
>>> {
>>> unsigned int i;
>>> diff --git a/monitor.c b/monitor.c
>>> index aeea2b5..4224314 100644
>>> --- a/monitor.c
>>> +++ b/monitor.c
>>> @@ -5360,4 +5360,28 @@ void qmp_rtc_reset_reinjection(Error **errp)
>>> {
>>> error_setg(errp, QERR_FEATURE_DISABLED, "rtc-reset-reinjection");
>>> }
>>> +void qmp_inject_vmport_action(enum VmportAction action, Error **errp)
>>> +{
>>> + error_set(errp, QERR_FEATURE_DISABLED, "inject-vmport-action");
>>> +}
>>> +void qmp_vmport_guestinfo_set(const char *key, const char *value,
>>> + bool has_format, enum DataFormat format,
>>> + Error **errp)
>>> +{
>>> + error_set(errp, QERR_FEATURE_DISABLED, "vmport-guestinfo-set");
>>> +}
>>> +VmportGuestInfoValue *qmp_vmport_guestinfo_get(const char *key,
>>> + bool has_format,
>>> + enum DataFormat format,
>>> + Error **errp)
>>> +{
>>> + error_set(errp, QERR_FEATURE_DISABLED, "vmport-guestinfo-get");
>>> + return NULL;
>>> +}
>>> +VmportGuestInfoKeyList *qmp_query_vmport_guestinfo(Error **errp)
>>> +{
>>> + error_set(errp, QERR_FEATURE_DISABLED, "query-vmport-guestinfo");
>>> + return NULL;
>>> +}
>> I understand you're just following existing practice here, but I can
>> help to wonder whether the practice is smart. The command exists in all
>> compile-time configurations, but it works in only some. To figure out
>> whether it works, you have to try it.
>>
>> If we add the command only when it actually works, you can use
>> query-commands instead.
>>
>> This isn't a demand for you to change anything now. We QMP guys need to
>> decide how we want this done.
>>
>>> #endif
>> Since the #ifdef section is now longer than a few lines,
>>
>> -#endif
>> +#endif /* !TARGET_I386 */
>>
>> would be nice
>
> Will add.
>
>>> +
>> No blank line at end of file, please.
>
> Will drop.
>
>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>> index 106008c..5d9e9e2 100644
>>> --- a/qapi-schema.json
>>> +++ b/qapi-schema.json
>>> @@ -1429,6 +1429,107 @@
>>> { 'command': 'inject-nmi' }
>>> ##
>>> +# @VmportAction:
>>> +#
>>> +# An enumeration of actions that can be requested via vmport RPC.
>>> +#
>>> +# @reboot: Ask the guest via VMware tools to reboot
>>> +#
>>> +# @halt: Ask the guest via VMware tools to halt
>>> +#
>>> +# Since: 2.4
>>> +##
>>> +{ 'enum': 'VmportAction',
>>> + 'data': [ 'reboot', 'halt' ] }
>>> +
>>> +##
>>> +# @inject-vmport-action:
>>> +#
>>> +# Injects a VMWare Tools action to the guest.
>>> +#
>>> +# Returns: If successful, nothing
>>> +#
>>> +# Since: 2.4
>>> +#
>>> +##
>>> +{ 'command': 'inject-vmport-action',
>>> + 'data': {'action': 'VmportAction'} }
>>> +
>>> +##
>>> +# @vmport-guestinfo-set:
>>> +#
>>> +# Set a VMWare Tools guestinfo key to a value
>>> +#
>>> +# @key: the key to set
>>> +#
>>> +# @value: The data to set the key to
>>> +#
>>> +# @format: #optional value encoding (default 'utf8').
>>> +# - base64: value is assumed to be base64 encoded text. Its binary
>>> +# decoding gets set.
>> Shouldn't you copy ringbuf-write's bug note?
>>
>> # Bug: invalid base64 is currently not rejected.
>>
>
> Nope. From the cover letter:
> ----------------------------------------------------------------------------------------
>
> Much more on format=base64:
>
> If there is a bug it is in GLIB. However the Glib reference manual
> refers to RFC 1421 and RFC 2045 and MIME encoding. Based on all
> that (which seems to match:
>
> http://en.wikipedia.org/wiki/Base64
>
> ) MIME states that all characters outside the (base64) alphabet are
> to be ignored. Testing shows that g_base64_decode() does this.
>
> The confusion is that most non-MIME uses reject a base64 string that
> contain characters outside the alphabet. I was just following the
> other uses of base64 in this file.
>
> DataFormat refers to RFC 3548, which has the info:
>
> "
> Implementations MUST reject the encoding if it contains
> characters outside the base alphabet when interpreting base
> encoded data, unless the specification referring to this document
> explicitly states otherwise. Such specifications may, as MIME
> does, instead state that characters outside the base encoding
> alphabet should simply be ignored when interpreting data ("be
> liberal in what you accept").
> "
>
> So with GLIB going the MIME way, I do not think this is a QEMU bug
> (you could consider this a GLIB bug, but the document I found says
> that GLIB goes the MIME way and so does not reject anything).
> ----------------------------------------------------------------------------------------
>
>
> Based on all this, I did drop it.
The QMP user isn't at all interested whether QEMU or GLib or any other
library screws this up, only in what the command's specification is, and
whatever bugs its implementation may have.
We obviously have leeway with the specification. We *can* override RFC
3548 by "explicitly stating otherwise".
For ringbuf-write, we deliberately chose not to. Instead we declared
the non-rejection of invalid characters a bug.
The exact same reasoning applies to your command. If you disagree with
the reasoning, it's certainly open to debate.
Regardless of what consensus emerges, all uses of base64 DataFormat in
QMP should be consistent. Right now they aren't with your patch
applied.
>>> +# - utf8: value's UTF-8 encoding is used to set.
>>> +#
>>> +# Returns: Nothing on success
>>> +#
>>> +# Since: 2.4
>>> +##
>>> +{ 'command': 'vmport-guestinfo-set',
>>> + 'data': {'key': 'str', 'value': 'str',
>>> + '*format': 'DataFormat'} }
>>> +
>>> +##
>>> +# @VmportGuestInfoValue:
>>> +#
>>> +# Information about a single VMWare Tools guestinfo
>>> +#
>>> +# @value: The value for a key
>>> +#
>>> +# Since: 2.4
>>> +##
>>> +{ 'struct': 'VmportGuestInfoValue', 'data': {'value': 'str'} }
>>> +
>>> +##
>>> +# @vmport-guestinfo-get:
>>> +#
>>> +# Get a VMWare Tools guestinfo value for a key
>>> +#
>>> +# @key: the key to get
>> Ignorant question: is the set of keys fixed (e.g. specified by some
>> authority), completely dynamic (i.e. whatever has been set), or
>> something else?
>
> completely dynamic
Thanks.
>>> +#
>>> +# @format: #optional data encoding (default 'utf8').
>>> +# - base64: the value is returned in base64 encoding.
>>> +# - utf8: the value is interpreted as UTF-8.
>>> +#
>>> +# Returns: value for the guest info key
>> ringbuf-read carries this bug note:
>>
>> # Bug: can screw up when the buffer contains invalid UTF-8
>> # sequences, NUL characters, after the ring buffer lost
>> # data, and when reading stops because the size limit is
>> # reached.
>>
>> Doesn't it apply here, too, with "the buffer" replaced?
>
> Not directly. The best description I have so far to match the FIXME
> would be:
>
> -----------------------------------------------------------------------------------
> Bug: format=utf8 can cause QEMU to screw up (may crash) when the value
> contains invalid UTF-8
>
> sequences. NUL characters will cause loss of data.
>
> ----------------------------------------------------------------------------------
>
> I have not spent the time investigating the issues with QMP and
> invalid utf8 because it looks to be very wide spread and not just
> restricted to this one routine.
The problem with ringbuf-read with format=utf8 is that it attempts to
interpret the ring buffer's contents, which could be anything, as UTF-8
by design (not a problem), and the code doing so is insufficiently
robust (this is the bug; patch welcome, as usual).
As far as I can tell, vmport-guestinfo-get is a similar case: the value
associated with the key could again be anything (if written with
format=base64), and we attempt to interpret it as UTF-8 by design.
What makes you think the problem is very widespread?
>>> +#
>>> +# Since: 2.4
>>> +##
>>> +{ 'command': 'vmport-guestinfo-get',
>>> + 'data': {'key': 'str', '*format': 'DataFormat'},
>>> + 'returns': 'VmportGuestInfoValue' }
>> I gather you wrap vmport-guestinfo-get's return value in a struct for
>> extensibility. Can't see why and how we'd want to extend it, but better
>> safe than sorry.
>>
>>> +
>>> +##
>>> +# @VmportGuestInfoKey:
>>> +#
>>> +# Information about a single VMWare Tools guestinfo
>>> +#
>>> +# @key: The known key
>>> +#
>>> +# Since: 2.4
>>> +##
>>> +{ 'struct': 'VmportGuestInfoKey', 'data': {'key': 'str'} }
>>> +
>>> +##
>>> +# @query-vmport-guestinfo:
>>> +#
>>> +# Returns information about VMWare Tools guestinfo
>>> +#
>>> +# Returns: a list of @VmportGuestInfoKey
>>> +#
>>> +# Since: 2.4
>>> +##
>>> +{ 'command': 'query-vmport-guestinfo', 'returns': ['VmportGuestInfoKey'] }
>> Similar. Imagining an extension of a string key is even harder. Okay.
>>
>>> +
>>> +##
>>> # @set_link:
>>> #
>>> # Sets the link status of a virtual network adapter.
>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>> index a05d25f..a43db8b 100644
[...]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v8 6/8] vmport_rpc: Add QMP access to vmport_rpc object.
2015-07-03 17:56 ` Markus Armbruster
@ 2015-07-03 21:26 ` Don Slutz
2015-07-04 7:00 ` Markus Armbruster
0 siblings, 1 reply; 19+ messages in thread
From: Don Slutz @ 2015-07-03 21:26 UTC (permalink / raw)
To: Markus Armbruster
Cc: Michael S. Tsirkin, qemu-devel, Don Slutz, Luiz Capitulino,
Anthony Liguori, Paolo Bonzini, Andreas Färber,
Richard Henderson
On 07/03/15 13:56, Markus Armbruster wrote:
> Don Slutz <don.slutz@gmail.com> writes:
>
>> On 07/02/15 10:11, Markus Armbruster wrote:
>>> Don Slutz <don.slutz@gmail.com> writes:
>>>
>>>> From: Don Slutz <dslutz@verizon.com>
>>>>
>>>> This adds one new inject command:
>>>>
>>>> inject-vmport-action
>>>>
>>>> And three guest info commands:
>>>>
>>>> vmport-guestinfo-set
>>>> vmport-guestinfo-get
>>>> query-vmport-guestinfo
>>>>
>>>> More details in qmp-commands.hx
>>>>
>>>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>>>> CC: Don Slutz <don.slutz@gmail.com>
>>> Reviewing only the QAPI/QMP interface, plus a bit of drive-by shooting.
>>>
>>>> diff --git a/hw/misc/vmport_rpc.c b/hw/misc/vmport_rpc.c
>>>> index f157425..917898b 100644
>>>> --- a/hw/misc/vmport_rpc.c
>>>> +++ b/hw/misc/vmport_rpc.c
>>>> @@ -178,6 +178,19 @@ typedef struct VMPortRpcState {
>>>> #endif
>>>> } VMPortRpcState;
>>>> +typedef int FindVMPortRpcDeviceFunc(VMPortRpcState *s, const
>>>> void *arg);
>>>> +
>>>> +/*
>>>> + * The input and output argument that find_VMPortRpc_device() uses
>>>> + * to do its work.
>>>> + */
>>>> +typedef struct VMPortRpcFind {
>>>> + const void *arg;
>>>> + FindVMPortRpcDeviceFunc *func;
>>>> + int found;
>>>> + int rc;
>>>> +} VMPortRpcFind;
>>>> +
>>>> /* Basic request types */
>>>> typedef enum {
>>>> MESSAGE_TYPE_OPEN,
>>>> @@ -202,6 +215,56 @@ typedef struct {
>>>> uint32_t edi;
>>>> } vregs;
>>>> +/*
>>>> + * Run func() for every VMPortRpc device, traverse the tree for
>>>> + * everything else.
>>>> + */
>>>> +static int find_VMPortRpc_device(Object *obj, void *opaque)
>>>> +{
>>>> + VMPortRpcFind *find = opaque;
>>>> + Object *dev;
>>>> + VMPortRpcState *s;
>>>> +
>>>> + assert(find);
>>>> + if (find->found) {
>>>> + return 0;
>>>> + }
>>>> + dev = object_dynamic_cast(obj, TYPE_VMPORT_RPC);
>>>> + s = (VMPortRpcState *)dev;
>>>> +
>>>> + if (!s) {
>>>> + /* Container, traverse it for children */
>>>> + return object_child_foreach(obj, find_VMPortRpc_device, opaque);
>>>> + }
>>>> +
>>>> + find->found++;
>>>> + find->rc = find->func(s, find->arg);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Loop through all dynamically created VMPortRpc devices and call
>>>> + * func() for each instance.
>>>> + */
>>>> +static int foreach_dynamic_vmport_rpc_device(FindVMPortRpcDeviceFunc *func,
>>>> + const void *arg)
>>>> +{
>>>> + VMPortRpcFind find = {
>>>> + .func = func,
>>>> + .arg = arg,
>>>> + };
>>>> +
>>>> + /* Loop through all VMPortRpc devices that were spawned outside
>>>> + * the machine */
>>>> + find_VMPortRpc_device(qdev_get_machine(), &find);
>>>> + if (find.found) {
>>>> + return find.rc;
>>>> + } else {
>>>> + return VMPORT_DEVICE_NOT_FOUND;
>>>> + }
>>>> +}
>>>> +
>>>> #ifdef VMPORT_RPC_DEBUG
>>>> /*
>>>> * Add helper function for tracing. This routine will convert
>>>> @@ -329,7 +392,7 @@ static int vmport_rpc_send(VMPortRpcState *s, channel_t *c,
>>>> return rc;
>>>> }
>>>> -static int vmport_rpc_ctrl_send(VMPortRpcState *s, char *msg)
>>>> +static int vmport_rpc_ctrl_send(VMPortRpcState *s, const char *msg)
>>>> {
>>>> int rc = SEND_NOT_OPEN;
>>>> unsigned int i;
>>>> @@ -457,6 +520,29 @@ static int get_guestinfo(VMPortRpcState *s,
>>>> return GUESTINFO_NOTFOUND;
>>>> }
>>>> +static int get_qmp_guestinfo(VMPortRpcState *s,
>>>> + unsigned int a_key_len, const char *a_info_key,
>>>> + unsigned int *a_value_len, void **a_value_data)
>>>> +{
>>>> + guestinfo_t *gi = NULL;
>>>> +
>>>> + if (a_key_len <= MAX_KEY_LEN) {
>>>> + gpointer key = g_strndup(a_info_key, a_key_len);
>>>> +
>>>> + gi = (guestinfo_t *)g_hash_table_lookup(s->guestinfo, key);
>>>> + g_free(key);
>>>> + } else {
>>>> + return GUESTINFO_KEYTOOLONG;
>>>> + }
>>>> + if (gi) {
>>>> + *a_value_len = gi->val_len;
>>>> + *a_value_data = gi->val_data;
>>>> + return 0;
>>>> + }
>>>> +
>>>> + return GUESTINFO_NOTFOUND;
>>>> +}
>>>> +
>>>> static int set_guestinfo(VMPortRpcState *s, int a_key_len,
>>>> unsigned int a_val_len, const char *a_info_key,
>>>> char *val)
>>>> @@ -775,7 +861,7 @@ static void vmport_rpc(VMPortRpcState *s , vregs *ur)
>>>> }
>>>> if (delta > s->reset_time) {
>>>> trace_vmport_rpc_ping(delta);
>>>> - vmport_rpc_ctrl_send(s, (char *)"reset");
>>>> + vmport_rpc_ctrl_send(s, "reset");
>>>> }
>>>> vmport_rpc_sweep(s, now_time);
>>>> do {
>>>> @@ -848,6 +934,206 @@ static uint32_t vmport_rpc_ioport_read(void *opaque, uint32_t addr)
>>>> return ur.data[0];
>>>> }
>>>> +static int vmport_rpc_find_send(VMPortRpcState *s, const void
>>>> *arg)
>>>> +{
>>>> + return vmport_rpc_ctrl_send(s, arg);
>>>> +}
>>>> +
>>>> +static void convert_local_rc(Error **errp, int rc)
>>>> +{
>>>> + switch (rc) {
>>>> + case 0:
>>>> + break;
>>>> + case VMPORT_DEVICE_NOT_FOUND:
>>>> + error_set(errp, QERR_DEVICE_NOT_FOUND, TYPE_VMPORT_RPC);
>>>> + break;
>>> Conflicts with
>>>
>>> commit 75158ebbe259f0bd8bf435e8f4827a43ec89c877
>>> Author: Markus Armbruster <armbru@redhat.com>
>>> Date: Mon Mar 16 08:57:47 2015 +0100
>>>
>>> qerror: Eliminate QERR_DEVICE_NOT_FOUND
>>> Error classes other than ERROR_CLASS_GENERIC_ERROR should
>>> not be used
>>> in new code. Hiding them in QERR_ macros makes new uses hard to spot.
>>> Fortunately, there's just one such macro left. Eliminate it with this
>>> coccinelle semantic patch:
>>> @@
>>> expression EP, E;
>>> @@
>>> -error_set(EP, QERR_DEVICE_NOT_FOUND, E)
>>> +error_set(EP, ERROR_CLASS_DEVICE_NOT_FOUND, "Device '%s' not found", E)
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
>>>
>>> Sorry if you explained this already for previous revisions: what's the
>>> justification for ERROR_CLASS_DEVICE_NOT_FOUND? Can you stick to
>>> ERROR_CLASS_GENERIC_ERROR?
>> I was only coping code from other places. Happy to convert to error_setg().
> Please do.
Ok, will do.
>
>>>> + case SEND_NOT_OPEN:
>>>> + error_setg(errp, "VMWare rpc not open");
>>>> + break;
>>>> + case SEND_SKIPPED:
>>>> + error_setg(errp, "VMWare rpc send skipped");
>>>> + break;
>>>> + case SEND_TRUCATED:
>>>> + error_setg(errp, "VMWare rpc send trucated");
>>>> + break;
>>>> + case SEND_NO_MEMORY:
>>>> + error_setg(errp, "VMWare rpc send out of memory");
>>>> + break;
>>>> + case GUESTINFO_NOTFOUND:
>>>> + error_setg(errp, "VMWare guestinfo not found");
>>>> + break;
>>>> + case GUESTINFO_VALTOOLONG:
>>>> + error_setg(errp, "VMWare guestinfo value too long");
>>>> + break;
>>>> + case GUESTINFO_KEYTOOLONG:
>>>> + error_setg(errp, "VMWare guestinfo key too long");
>>>> + break;
>>>> + case GUESTINFO_TOOMANYKEYS:
>>>> + error_setg(errp, "VMWare guestinfo too many keys");
>>>> + break;
>>>> + case GUESTINFO_NO_MEMORY:
>>>> + error_setg(errp, "VMWare guestinfo out of memory");
>>>> + break;
>>>> + default:
>>>> + error_setg(errp, "VMWare rpc send rc=%d unknown", rc);
>>>> + break;
>>>> + }
>>>> +}
>>>> +
>>>> +void qmp_inject_vmport_action(enum VmportAction action, Error **errp)
>>>> +{
>>>> + int rc;
>>>> +
>>>> + switch (action) {
>>>> + case VMPORT_ACTION_REBOOT:
>>>> + rc = foreach_dynamic_vmport_rpc_device(vmport_rpc_find_send,
>>>> + "OS_Reboot");
>>>> + break;
>>>> + case VMPORT_ACTION_HALT:
>>>> + rc = foreach_dynamic_vmport_rpc_device(vmport_rpc_find_send,
>>>> + "OS_Halt");
>>>> + break;
>>>> + case VMPORT_ACTION_MAX:
>>>> + assert(action != VMPORT_ACTION_MAX);
>>> If this fails, you likely found a major compiler bug.
>> Nope. That assert() does happen:
> Uh, I misread != for ==. The assert *always* fails. Sorry!
>
>>>> + abort(); /* Should be impossible to get here. */
>>> Why not simply
>>>
>>> default:
>>> abort();
>> Will change to this. abort() was added in v7.
> Thanks!
>
>>>> + }
>>>> + convert_local_rc(errp, rc);
>>>> +}
>>>> +
>>>> +typedef struct keyValue {
>>>> + const char *key_data;
>>>> + void *value_data;
>>>> + unsigned int key_len;
>>>> + unsigned int value_len;
>>>> +} keyValue;
>>>> +
>>>> +static int find_set(VMPortRpcState *s, const void *arg)
>>>> +{
>>>> + const keyValue *key_value = arg;
>>>> +
>>>> + return set_guestinfo(s, key_value->key_len, key_value->value_len,
>>>> + key_value->key_data, key_value->value_data);
>>>> +}
>>>> +
>>>> +static int find_get(VMPortRpcState *s, const void *arg)
>>>> +{
>>>> + keyValue *key_value = (void *)arg;
>>>> +
>>>> + return get_qmp_guestinfo(s, key_value->key_len, key_value->key_data,
>>>> + &key_value->value_len, &key_value->value_data);
>>>> +}
>>>> +
>>>> +void qmp_vmport_guestinfo_set(const char *key, const char *value,
>>>> + bool has_format, enum DataFormat format,
>>>> + Error **errp)
>>>> +{
>>>> + int rc;
>>>> + keyValue key_value;
>>>> +
>>>> + if (strncmp(key, "guestinfo.", strlen("guestinfo.")) == 0) {
>>>> + key_value.key_data = (key + strlen("guestinfo."));
>>>> + key_value.key_len = strlen(key) - strlen("guestinfo.");
>>>> + } else {
>>>> + key_value.key_data = key;
>>>> + key_value.key_len = strlen(key);
>>>> + }
>>>> + if (has_format && (format == DATA_FORMAT_BASE64)) {
>>>> + gsize write_count;
>>>> +
>>>> + key_value.value_data = g_base64_decode(value, &write_count);
>>>> + key_value.value_len = write_count;
>>>> + } else {
>>>> + key_value.value_data = (void *)value;
>>>> + key_value.value_len = strlen(value);
>>>> + }
>>>> +
>>>> + rc = foreach_dynamic_vmport_rpc_device(find_set, &key_value);
>>>> +
>>>> + if (key_value.value_data != value) {
>>>> + g_free(key_value.value_data);
>>>> + }
>>>> +
>>>> + if (rc) {
>>>> + convert_local_rc(errp, rc);
>>>> + return;
>>>> + }
>>>> +}
>>>> +
>>>> +VmportGuestInfoValue *qmp_vmport_guestinfo_get(const char *key,
>>>> + bool has_format,
>>>> + enum DataFormat format,
>>>> + Error **errp)
>>>> +{
>>>> + int rc;
>>>> + keyValue key_value;
>>>> + VmportGuestInfoValue *ret_value;
>>>> +
>>>> + if (strncmp(key, "guestinfo.", strlen("guestinfo.")) == 0) {
>>>> + key_value.key_data = (key + strlen("guestinfo."));
>>>> + key_value.key_len = strlen(key) - strlen("guestinfo.");
>>>> + } else {
>>>> + key_value.key_data = key;
>>>> + key_value.key_len = strlen(key);
>>>> + }
>>>> +
>>>> + rc = foreach_dynamic_vmport_rpc_device(find_get, &key_value);
>>>> + if (rc) {
>>>> + convert_local_rc(errp, rc);
>>>> + return NULL;
>>>> + }
>>>> +
>>>> + ret_value = g_new0(VmportGuestInfoValue, 1);
>>>> + if (has_format && (format == DATA_FORMAT_BASE64)) {
>>>> + ret_value->value = g_base64_encode(key_value.value_data,
>>>> + key_value.value_len);
>>>> + } else {
>>>> + /*
>>>> + * FIXME should check for complete, valid UTF-8 characters.
>>>> + * Invalid sequences should be replaced by a suitable
>>>> + * replacement character. Or cause an error to be raised.
>>>> + */
>>> Thanks for copying the FIXME from qmp_ringbuf_read(), I appreciate it.
>>>
>>>> + ret_value->value = g_malloc(key_value.value_len + 1);
>>>> + memcpy(ret_value->value, key_value.value_data, key_value.value_len);
>>>> + ret_value->value[key_value.value_len] = 0;
>>>> + }
>>>> +
>>>> + return ret_value;
>>>> +}
>>>> +
>>>> +
>>>> +static void vmport_rpc_find_list_one(gpointer key, gpointer value,
>>>> + gpointer opaque)
>>>> +{
>>>> + VmportGuestInfoKeyList **keys = opaque;
>>>> + VmportGuestInfoKeyList *info = g_new0(VmportGuestInfoKeyList, 1);
>>>> + char *ckey = key;
>>>> +
>>>> + info->value = g_new0(VmportGuestInfoKey, 1);
>>>> + info->value->key = g_strdup_printf("guestinfo.%s", ckey);
>>>> + info->next = *keys;
>>>> + *keys = info;
>>>> +}
>>>> +
>>>> +static int vmport_rpc_find_list(VMPortRpcState *s, const void *arg)
>>>> +{
>>>> + g_hash_table_foreach(s->guestinfo, vmport_rpc_find_list_one, (gpointer)arg);
>>>> + return 0;
>>>> +}
>>>> +
>>>> +VmportGuestInfoKeyList *qmp_query_vmport_guestinfo(Error **errp)
>>>> +{
>>>> + VmportGuestInfoKeyList *keys = NULL;
>>>> + int rc = foreach_dynamic_vmport_rpc_device(vmport_rpc_find_list,
>>>> + (void *)&keys);
>>>> +
>>>> + if (rc) {
>>>> + convert_local_rc(errp, rc);
>>>> + }
>>>> +
>>>> + return keys;
>>>> +}
>>>> +
>>>> +
>>>> static void vmport_rpc_reset(DeviceState *d)
>>>> {
>>>> unsigned int i;
>>>> diff --git a/monitor.c b/monitor.c
>>>> index aeea2b5..4224314 100644
>>>> --- a/monitor.c
>>>> +++ b/monitor.c
>>>> @@ -5360,4 +5360,28 @@ void qmp_rtc_reset_reinjection(Error **errp)
>>>> {
>>>> error_setg(errp, QERR_FEATURE_DISABLED, "rtc-reset-reinjection");
>>>> }
>>>> +void qmp_inject_vmport_action(enum VmportAction action, Error **errp)
>>>> +{
>>>> + error_set(errp, QERR_FEATURE_DISABLED, "inject-vmport-action");
>>>> +}
>>>> +void qmp_vmport_guestinfo_set(const char *key, const char *value,
>>>> + bool has_format, enum DataFormat format,
>>>> + Error **errp)
>>>> +{
>>>> + error_set(errp, QERR_FEATURE_DISABLED, "vmport-guestinfo-set");
>>>> +}
>>>> +VmportGuestInfoValue *qmp_vmport_guestinfo_get(const char *key,
>>>> + bool has_format,
>>>> + enum DataFormat format,
>>>> + Error **errp)
>>>> +{
>>>> + error_set(errp, QERR_FEATURE_DISABLED, "vmport-guestinfo-get");
>>>> + return NULL;
>>>> +}
>>>> +VmportGuestInfoKeyList *qmp_query_vmport_guestinfo(Error **errp)
>>>> +{
>>>> + error_set(errp, QERR_FEATURE_DISABLED, "query-vmport-guestinfo");
>>>> + return NULL;
>>>> +}
>>> I understand you're just following existing practice here, but I can
>>> help to wonder whether the practice is smart. The command exists in all
>>> compile-time configurations, but it works in only some. To figure out
>>> whether it works, you have to try it.
>>>
>>> If we add the command only when it actually works, you can use
>>> query-commands instead.
>>>
>>> This isn't a demand for you to change anything now. We QMP guys need to
>>> decide how we want this done.
>>>
>>>> #endif
>>> Since the #ifdef section is now longer than a few lines,
>>>
>>> -#endif
>>> +#endif /* !TARGET_I386 */
>>>
>>> would be nice
>> Will add.
>>
>>>> +
>>> No blank line at end of file, please.
>> Will drop.
>>
>>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>>> index 106008c..5d9e9e2 100644
>>>> --- a/qapi-schema.json
>>>> +++ b/qapi-schema.json
>>>> @@ -1429,6 +1429,107 @@
>>>> { 'command': 'inject-nmi' }
>>>> ##
>>>> +# @VmportAction:
>>>> +#
>>>> +# An enumeration of actions that can be requested via vmport RPC.
>>>> +#
>>>> +# @reboot: Ask the guest via VMware tools to reboot
>>>> +#
>>>> +# @halt: Ask the guest via VMware tools to halt
>>>> +#
>>>> +# Since: 2.4
>>>> +##
>>>> +{ 'enum': 'VmportAction',
>>>> + 'data': [ 'reboot', 'halt' ] }
>>>> +
>>>> +##
>>>> +# @inject-vmport-action:
>>>> +#
>>>> +# Injects a VMWare Tools action to the guest.
>>>> +#
>>>> +# Returns: If successful, nothing
>>>> +#
>>>> +# Since: 2.4
>>>> +#
>>>> +##
>>>> +{ 'command': 'inject-vmport-action',
>>>> + 'data': {'action': 'VmportAction'} }
>>>> +
>>>> +##
>>>> +# @vmport-guestinfo-set:
>>>> +#
>>>> +# Set a VMWare Tools guestinfo key to a value
>>>> +#
>>>> +# @key: the key to set
>>>> +#
>>>> +# @value: The data to set the key to
>>>> +#
>>>> +# @format: #optional value encoding (default 'utf8').
>>>> +# - base64: value is assumed to be base64 encoded text. Its binary
>>>> +# decoding gets set.
>>> Shouldn't you copy ringbuf-write's bug note?
>>>
>>> # Bug: invalid base64 is currently not rejected.
>>>
>> Nope. From the cover letter:
>> ----------------------------------------------------------------------------------------
>>
>> Much more on format=base64:
>>
>> If there is a bug it is in GLIB. However the Glib reference manual
>> refers to RFC 1421 and RFC 2045 and MIME encoding. Based on all
>> that (which seems to match:
>>
>> http://en.wikipedia.org/wiki/Base64
>>
>> ) MIME states that all characters outside the (base64) alphabet are
>> to be ignored. Testing shows that g_base64_decode() does this.
>>
>> The confusion is that most non-MIME uses reject a base64 string that
>> contain characters outside the alphabet. I was just following the
>> other uses of base64 in this file.
>>
>> DataFormat refers to RFC 3548, which has the info:
>>
>> "
>> Implementations MUST reject the encoding if it contains
>> characters outside the base alphabet when interpreting base
>> encoded data, unless the specification referring to this document
>> explicitly states otherwise. Such specifications may, as MIME
>> does, instead state that characters outside the base encoding
>> alphabet should simply be ignored when interpreting data ("be
>> liberal in what you accept").
>> "
>>
>> So with GLIB going the MIME way, I do not think this is a QEMU bug
>> (you could consider this a GLIB bug, but the document I found says
>> that GLIB goes the MIME way and so does not reject anything).
>> ----------------------------------------------------------------------------------------
>>
>>
>> Based on all this, I did drop it.
> The QMP user isn't at all interested whether QEMU or GLib or any other
> library screws this up, only in what the command's specification is, and
> whatever bugs its implementation may have.
>
> We obviously have leeway with the specification. We *can* override RFC
> 3548 by "explicitly stating otherwise".
>
> For ringbuf-write, we deliberately chose not to. Instead we declared
> the non-rejection of invalid characters a bug.
>
> The exact same reasoning applies to your command. If you disagree with
> the reasoning, it's certainly open to debate.
I feel that in real use (not testing) only valid base64 would be passed
in. However I do not want to delay this patch on this issue.
> Regardless of what consensus emerges, all uses of base64 DataFormat in
> QMP should be consistent. Right now they aren't with your patch
> applied.
Ok, I will copy the "bug comment line" so that it is consistent.
>>>> +# - utf8: value's UTF-8 encoding is used to set.
>>>> +#
>>>> +# Returns: Nothing on success
>>>> +#
>>>> +# Since: 2.4
>>>> +##
>>>> +{ 'command': 'vmport-guestinfo-set',
>>>> + 'data': {'key': 'str', 'value': 'str',
>>>> + '*format': 'DataFormat'} }
>>>> +
>>>> +##
>>>> +# @VmportGuestInfoValue:
>>>> +#
>>>> +# Information about a single VMWare Tools guestinfo
>>>> +#
>>>> +# @value: The value for a key
>>>> +#
>>>> +# Since: 2.4
>>>> +##
>>>> +{ 'struct': 'VmportGuestInfoValue', 'data': {'value': 'str'} }
>>>> +
>>>> +##
>>>> +# @vmport-guestinfo-get:
>>>> +#
>>>> +# Get a VMWare Tools guestinfo value for a key
>>>> +#
>>>> +# @key: the key to get
>>> Ignorant question: is the set of keys fixed (e.g. specified by some
>>> authority), completely dynamic (i.e. whatever has been set), or
>>> something else?
>> completely dynamic
> Thanks.
>
>>>> +#
>>>> +# @format: #optional data encoding (default 'utf8').
>>>> +# - base64: the value is returned in base64 encoding.
>>>> +# - utf8: the value is interpreted as UTF-8.
>>>> +#
>>>> +# Returns: value for the guest info key
>>> ringbuf-read carries this bug note:
>>>
>>> # Bug: can screw up when the buffer contains invalid UTF-8
>>> # sequences, NUL characters, after the ring buffer lost
>>> # data, and when reading stops because the size limit is
>>> # reached.
>>>
>>> Doesn't it apply here, too, with "the buffer" replaced?
>> Not directly. The best description I have so far to match the FIXME
>> would be:
>>
>> -----------------------------------------------------------------------------------
>> Bug: format=utf8 can cause QEMU to screw up (may crash) when the value
>> contains invalid UTF-8
>>
>> sequences. NUL characters will cause loss of data.
>>
>> ----------------------------------------------------------------------------------
>>
>> I have not spent the time investigating the issues with QMP and
>> invalid utf8 because it looks to be very wide spread and not just
>> restricted to this one routine.
> The problem with ringbuf-read with format=utf8 is that it attempts to
> interpret the ring buffer's contents, which could be anything, as UTF-8
> by design (not a problem), and the code doing so is insufficiently
> robust (this is the bug; patch welcome, as usual).
>
> As far as I can tell, vmport-guestinfo-get is a similar case: the value
> associated with the key could again be anything (if written with
> format=base64), and we attempt to interpret it as UTF-8 by design.
>
> What makes you think the problem is very widespread?
I only did a quick look and did not find what I would have expected. I
will do more looking and reply with those results.
I am happy to apply either the above Bug comment or an adjusted version
of the ringbuf-read one for now (I have been working on getting this
accepted for a while now, 1st posted on Fri, 30 Jan 2015 16:06:24 -0500).
-Don Slutz
>>>> +#
>>>> +# Since: 2.4
>>>> +##
>>>> +{ 'command': 'vmport-guestinfo-get',
>>>> + 'data': {'key': 'str', '*format': 'DataFormat'},
>>>> + 'returns': 'VmportGuestInfoValue' }
>>> I gather you wrap vmport-guestinfo-get's return value in a struct for
>>> extensibility. Can't see why and how we'd want to extend it, but better
>>> safe than sorry.
>>>
>>>> +
>>>> +##
>>>> +# @VmportGuestInfoKey:
>>>> +#
>>>> +# Information about a single VMWare Tools guestinfo
>>>> +#
>>>> +# @key: The known key
>>>> +#
>>>> +# Since: 2.4
>>>> +##
>>>> +{ 'struct': 'VmportGuestInfoKey', 'data': {'key': 'str'} }
>>>> +
>>>> +##
>>>> +# @query-vmport-guestinfo:
>>>> +#
>>>> +# Returns information about VMWare Tools guestinfo
>>>> +#
>>>> +# Returns: a list of @VmportGuestInfoKey
>>>> +#
>>>> +# Since: 2.4
>>>> +##
>>>> +{ 'command': 'query-vmport-guestinfo', 'returns': ['VmportGuestInfoKey'] }
>>> Similar. Imagining an extension of a string key is even harder. Okay.
>>>
>>>> +
>>>> +##
>>>> # @set_link:
>>>> #
>>>> # Sets the link status of a virtual network adapter.
>>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>>> index a05d25f..a43db8b 100644
> [...]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v8 6/8] vmport_rpc: Add QMP access to vmport_rpc object.
2015-07-03 21:26 ` Don Slutz
@ 2015-07-04 7:00 ` Markus Armbruster
0 siblings, 0 replies; 19+ messages in thread
From: Markus Armbruster @ 2015-07-04 7:00 UTC (permalink / raw)
To: Don Slutz
Cc: Michael S. Tsirkin, qemu-devel, Don Slutz, Luiz Capitulino,
Anthony Liguori, Paolo Bonzini, Andreas Färber,
Richard Henderson
Don Slutz <don.slutz@gmail.com> writes:
> On 07/03/15 13:56, Markus Armbruster wrote:
>> Don Slutz <don.slutz@gmail.com> writes:
>>
>>> On 07/02/15 10:11, Markus Armbruster wrote:
>>>> Don Slutz <don.slutz@gmail.com> writes:
[...]
>>>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>>>> index 106008c..5d9e9e2 100644
>>>>> --- a/qapi-schema.json
>>>>> +++ b/qapi-schema.json
>>>>> @@ -1429,6 +1429,107 @@
>>>>> { 'command': 'inject-nmi' }
>>>>> ##
>>>>> +# @VmportAction:
>>>>> +#
>>>>> +# An enumeration of actions that can be requested via vmport RPC.
>>>>> +#
>>>>> +# @reboot: Ask the guest via VMware tools to reboot
>>>>> +#
>>>>> +# @halt: Ask the guest via VMware tools to halt
>>>>> +#
>>>>> +# Since: 2.4
>>>>> +##
>>>>> +{ 'enum': 'VmportAction',
>>>>> + 'data': [ 'reboot', 'halt' ] }
>>>>> +
>>>>> +##
>>>>> +# @inject-vmport-action:
>>>>> +#
>>>>> +# Injects a VMWare Tools action to the guest.
>>>>> +#
>>>>> +# Returns: If successful, nothing
>>>>> +#
>>>>> +# Since: 2.4
>>>>> +#
>>>>> +##
>>>>> +{ 'command': 'inject-vmport-action',
>>>>> + 'data': {'action': 'VmportAction'} }
>>>>> +
>>>>> +##
>>>>> +# @vmport-guestinfo-set:
>>>>> +#
>>>>> +# Set a VMWare Tools guestinfo key to a value
>>>>> +#
>>>>> +# @key: the key to set
>>>>> +#
>>>>> +# @value: The data to set the key to
>>>>> +#
>>>>> +# @format: #optional value encoding (default 'utf8').
>>>>> +# - base64: value is assumed to be base64 encoded text. Its binary
>>>>> +# decoding gets set.
>>>> Shouldn't you copy ringbuf-write's bug note?
>>>>
>>>> # Bug: invalid base64 is currently not rejected.
>>>>
>>> Nope. From the cover letter:
>>> ----------------------------------------------------------------------------------------
>>>
>>> Much more on format=base64:
>>>
>>> If there is a bug it is in GLIB. However the Glib reference manual
>>> refers to RFC 1421 and RFC 2045 and MIME encoding. Based on all
>>> that (which seems to match:
>>>
>>> http://en.wikipedia.org/wiki/Base64
>>>
>>> ) MIME states that all characters outside the (base64) alphabet are
>>> to be ignored. Testing shows that g_base64_decode() does this.
>>>
>>> The confusion is that most non-MIME uses reject a base64 string that
>>> contain characters outside the alphabet. I was just following the
>>> other uses of base64 in this file.
>>>
>>> DataFormat refers to RFC 3548, which has the info:
>>>
>>> "
>>> Implementations MUST reject the encoding if it contains
>>> characters outside the base alphabet when interpreting base
>>> encoded data, unless the specification referring to this document
>>> explicitly states otherwise. Such specifications may, as MIME
>>> does, instead state that characters outside the base encoding
>>> alphabet should simply be ignored when interpreting data ("be
>>> liberal in what you accept").
>>> "
>>>
>>> So with GLIB going the MIME way, I do not think this is a QEMU bug
>>> (you could consider this a GLIB bug, but the document I found says
>>> that GLIB goes the MIME way and so does not reject anything).
>>> ----------------------------------------------------------------------------------------
>>>
>>>
>>> Based on all this, I did drop it.
>> The QMP user isn't at all interested whether QEMU or GLib or any other
>> library screws this up, only in what the command's specification is, and
>> whatever bugs its implementation may have.
>>
>> We obviously have leeway with the specification. We *can* override RFC
>> 3548 by "explicitly stating otherwise".
>>
>> For ringbuf-write, we deliberately chose not to. Instead we declared
>> the non-rejection of invalid characters a bug.
>>
>> The exact same reasoning applies to your command. If you disagree with
>> the reasoning, it's certainly open to debate.
>
> I feel that in real use (not testing) only valid base64 would be passed
> in. However I do not want to delay this patch on this issue.
>
>
>> Regardless of what consensus emerges, all uses of base64 DataFormat in
>> QMP should be consistent. Right now they aren't with your patch
>> applied.
>
> Ok, I will copy the "bug comment line" so that it is consistent.
Thanks!
>>>>> +# - utf8: value's UTF-8 encoding is used to set.
>>>>> +#
>>>>> +# Returns: Nothing on success
>>>>> +#
>>>>> +# Since: 2.4
>>>>> +##
>>>>> +{ 'command': 'vmport-guestinfo-set',
>>>>> + 'data': {'key': 'str', 'value': 'str',
>>>>> + '*format': 'DataFormat'} }
>>>>> +
>>>>> +##
>>>>> +# @VmportGuestInfoValue:
>>>>> +#
>>>>> +# Information about a single VMWare Tools guestinfo
>>>>> +#
>>>>> +# @value: The value for a key
>>>>> +#
>>>>> +# Since: 2.4
>>>>> +##
>>>>> +{ 'struct': 'VmportGuestInfoValue', 'data': {'value': 'str'} }
>>>>> +
>>>>> +##
>>>>> +# @vmport-guestinfo-get:
>>>>> +#
>>>>> +# Get a VMWare Tools guestinfo value for a key
>>>>> +#
>>>>> +# @key: the key to get
>>>> Ignorant question: is the set of keys fixed (e.g. specified by some
>>>> authority), completely dynamic (i.e. whatever has been set), or
>>>> something else?
>>> completely dynamic
>> Thanks.
>>
>>>>> +#
>>>>> +# @format: #optional data encoding (default 'utf8').
>>>>> +# - base64: the value is returned in base64 encoding.
>>>>> +# - utf8: the value is interpreted as UTF-8.
>>>>> +#
>>>>> +# Returns: value for the guest info key
>>>> ringbuf-read carries this bug note:
>>>>
>>>> # Bug: can screw up when the buffer contains invalid UTF-8
>>>> # sequences, NUL characters, after the ring buffer lost
>>>> # data, and when reading stops because the size limit is
>>>> # reached.
>>>>
>>>> Doesn't it apply here, too, with "the buffer" replaced?
>>> Not directly. The best description I have so far to match the FIXME
>>> would be:
>>>
>>> -----------------------------------------------------------------------------------
>>> Bug: format=utf8 can cause QEMU to screw up (may crash) when the value
>>> contains invalid UTF-8
>>>
>>> sequences. NUL characters will cause loss of data.
>>>
>>> ----------------------------------------------------------------------------------
>>>
>>> I have not spent the time investigating the issues with QMP and
>>> invalid utf8 because it looks to be very wide spread and not just
>>> restricted to this one routine.
>> The problem with ringbuf-read with format=utf8 is that it attempts to
>> interpret the ring buffer's contents, which could be anything, as UTF-8
>> by design (not a problem), and the code doing so is insufficiently
>> robust (this is the bug; patch welcome, as usual).
>>
>> As far as I can tell, vmport-guestinfo-get is a similar case: the value
>> associated with the key could again be anything (if written with
>> format=base64), and we attempt to interpret it as UTF-8 by design.
>>
>> What makes you think the problem is very widespread?
>
> I only did a quick look and did not find what I would have expected. I
> will do more looking and reply with those results.
Appreciated!
> I am happy to apply either the above Bug comment or an adjusted version
> of the ringbuf-read one for now (I have been working on getting this
> accepted for a while now, 1st posted on Fri, 30 Jan 2015 16:06:24 -0500).
We can certainly discuss declaring the sloppy base64 parsing a feature
across the board. You can choose to make your patch series depend on
the outcome of that discussion, but you absolutely don't have to: just
follow ringbuf-read precedence now. If we later decide to change our
base64 parsing rules, we'll have to fix up two places rather than one,
but that's fine.
Getting patches in can be arduous and frustrating. Thanks for hanging
in. I wish I had found the time to review an earlier revision, could
have saved you an iteration.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v8 8/8] vmport: Add VMware all ring hack
2015-07-03 13:50 ` Paolo Bonzini
@ 2015-07-05 6:40 ` Michael S. Tsirkin
2015-07-29 15:25 ` Don Slutz
0 siblings, 1 reply; 19+ messages in thread
From: Michael S. Tsirkin @ 2015-07-05 6:40 UTC (permalink / raw)
To: Paolo Bonzini
Cc: qemu-devel, Markus Armbruster, Don Slutz, Don Slutz,
Luiz Capitulino, Anthony Liguori, Andreas Färber,
Richard Henderson
On Fri, Jul 03, 2015 at 03:50:19PM +0200, Paolo Bonzini wrote:
>
>
> On 03/07/2015 15:08, Don Slutz wrote:
> >>>
> >> Creating it at the pc level and propagating makes code
> >> messy but I'd go along with it if it made sense from
> >> user's point of view, but it does not seem to make sense:
> >> to me this looks more like a CPU feature than a machine property.
> >> Or the property of the vmport rpc device that you created.
> >
> > Well this is not a clear area. It does look more like a CPU feature. I
> > went with a machine feature because that is where "vmport" currently is.
>
> I think it should be a vmport property.
>
> It would be cleaner to implement it using the MemTxAttrs mechanism, but
> for KVM I'm afraid it would be too slow (and the slowness would affect
> all I/O accesses, not just vmport ones, unless really ugly hacks are done).
>
> Paolo
Can it ever be implemented for kvm? I couldn't think of a clean way to do it.
--
MST
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v8 8/8] vmport: Add VMware all ring hack
2015-07-05 6:40 ` Michael S. Tsirkin
@ 2015-07-29 15:25 ` Don Slutz
0 siblings, 0 replies; 19+ messages in thread
From: Don Slutz @ 2015-07-29 15:25 UTC (permalink / raw)
To: Michael S. Tsirkin, Paolo Bonzini
Cc: qemu-devel, Markus Armbruster, Don Slutz, Luiz Capitulino,
Anthony Liguori, Andreas Färber, Richard Henderson
On 7/5/2015 2:40 AM, Michael S. Tsirkin wrote:
> On Fri, Jul 03, 2015 at 03:50:19PM +0200, Paolo Bonzini wrote:
>>
>> On 03/07/2015 15:08, Don Slutz wrote:
>>>> Creating it at the pc level and propagating makes code
>>>> messy but I'd go along with it if it made sense from
>>>> user's point of view, but it does not seem to make sense:
>>>> to me this looks more like a CPU feature than a machine property.
>>>> Or the property of the vmport rpc device that you created.
>>> Well this is not a clear area. It does look more like a CPU feature. I
>>> went with a machine feature because that is where "vmport" currently is.
>> I think it should be a vmport property.
>>
>> It would be cleaner to implement it using the MemTxAttrs mechanism, but
>> for KVM I'm afraid it would be too slow (and the slowness would affect
>> all I/O accesses, not just vmport ones, unless really ugly hacks are done).
>>
>> Paolo
> Can it ever be implemented for kvm? I couldn't think of a clean way to do it.
>
I do not know of a clean way. For Xen I need to enable #GP fault
reporting and add code to check in/out to the magic VMware port. I
would assume that some thing similar would be needed in kvm. Hope this
helps.
-Don Slutz
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2015-07-29 15:25 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-06-23 23:39 [Qemu-devel] [PATCH v8 0/8] Add limited support of VMware's hyper-call rpc Don Slutz
2015-06-23 23:39 ` [Qemu-devel] [PATCH v8 1/8] vmport: Switch to trace Don Slutz
2015-06-23 23:39 ` [Qemu-devel] [BUGFIX][PATCH v8 2/8] vmport: Fix vmport_cmd_ram_size Don Slutz
2015-06-23 23:39 ` [Qemu-devel] [PATCH v8 3/8] MAINTAINERS: add VMware port Don Slutz
2015-06-23 23:39 ` [Qemu-devel] [PATCH v8 4/8] vmport_rpc: Add the object vmport_rpc Don Slutz
2015-06-23 23:39 ` [Qemu-devel] [PATCH v8 5/8] vmport_rpc: Add limited support of VMware's hyper-call rpc Don Slutz
2015-06-23 23:39 ` [Qemu-devel] [PATCH v8 7/8] vmport_rpc: Add migration Don Slutz
2015-06-23 23:39 ` [Qemu-devel] [PATCH v8 8/8] vmport: Add VMware all ring hack Don Slutz
2015-07-01 6:50 ` Michael S. Tsirkin
2015-07-03 13:08 ` Don Slutz
2015-07-03 13:50 ` Paolo Bonzini
2015-07-05 6:40 ` Michael S. Tsirkin
2015-07-29 15:25 ` Don Slutz
2015-06-30 18:16 ` [Qemu-devel] [ping][PATCH v8 0/8] Add limited support of VMware's hyper-call rpc Don Slutz
[not found] ` <1435102773-3498-7-git-send-email-Don.Slutz@Gmail.com>
2015-07-02 14:11 ` [Qemu-devel] [PATCH v8 6/8] vmport_rpc: Add QMP access to vmport_rpc object Markus Armbruster
2015-07-03 16:53 ` Don Slutz
2015-07-03 17:56 ` Markus Armbruster
2015-07-03 21:26 ` Don Slutz
2015-07-04 7:00 ` Markus Armbruster
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).