Re: [Qemu-devel] [PATCH 0/2] block: fix spurious DEVICE_TRAY_MOVED events on shutdown
On Fri, 17 May 2013 16:23:51 +0200
Pavel Hrdina phrd...@redhat.com wrote:
On 25.4.2013 16:31, Luiz Capitulino wrote:
On Thu, 25 Apr 2013 16:29:45 +0200
Stefan Hajnoczi stefa...@gmail.com wrote:
On Thu, Apr 25, 2013 at 09:51:47AM -0400, Luiz Capitulino wrote:
On Mon, 22 Apr 2013 15:53:43 +0200
Stefan Hajnoczi stefa...@gmail.com wrote:
On Fri, Apr 19, 2013 at 01:47:17PM -0400, Luiz Capitulino wrote:
Hi,
This fixes a regression introduced by commit 9ca111544, as detailed in
patch 2/2, by moving bdrv_dev_change_media_cb() calls to callers of
bdrv_close() that need it, as suggested by Kevin.
Luiz Capitulino (2):
block: make bdrv_dev_change_media_cb() public
block: move bdrv_dev_change_media_cb() to callers that really need it
block.c | 5 +
blockdev.c| 2 ++
include/block/block.h | 1 +
3 files changed, 4 insertions(+), 4 deletions(-)
Looks okay but I'll wait for Markus or Kevin to review too. The media
change code is subtle, we've had a long history of fixes :).
I wouldn't say this is hugely important, but I'm targeting 1.5.
So, maybe lack of review means you could apply it? :)
Nice try :)
Hehe.
We've never gotten media change right. I really would appreciate a
second pair of eyes. There are still a couple of days until hard
freeze.
Holding off until then.
Ok, no problem.
Hi all,
I've just tested the side effect of my original commit and the
DEVICE_TRAY_MOVED event is emitted only if the CD-ROM is opened. If you
shutdown/reboot the guest with closed CD-ROM tray there is no
DEVICE_TRAY_MOVED event emitted. I think that this behavior is correct.
That's not what I'm seeing here, unless the tray is opened right before
shutdown, but even then the events are wrong as they notify the
transition closed - opened twice.
On HMP:
(qemu) info block
ide1-cd0: removable=1 locked=0 tray-open=0 io-status=ok [not inserted]
(qemu) system_powerdown
on QMP:
{
timestamp: {
seconds: 1369139052,
microseconds: 766612
},
event: DEVICE_TRAY_MOVED,
data: {
device: ide1-cd0,
tray-open: true
}
}
`
{
timestamp: {
seconds: 1369139052,
microseconds: 766798
},
event: DEVICE_TRAY_MOVED,
data: {
device: floppy0,
tray-open: true
}
}
Re: [Qemu-devel] [PATCH 7/7] block: dump to monitor for bdrv_snapshot_dump() and bdrv_image_info_dump()
On Wed, 22 May 2013 10:09:19 +0800
Wenchao Xia xiaw...@linux.vnet.ibm.com wrote:
于 2013-5-20 10:39, Wenchao Xia 写道:
于 2013-5-17 20:30, Luiz Capitulino 写道:
On Fri, 17 May 2013 11:30:31 +0800
Wenchao Xia xiaw...@linux.vnet.ibm.com wrote:
于 2013-5-16 20:17, Luiz Capitulino 写道:
On Thu, 16 May 2013 10:22:09 +0800
Wenchao Xia xiaw...@linux.vnet.ibm.com wrote:
于 2013-5-15 20:28, Luiz Capitulino 写道:
On Wed, 15 May 2013 10:10:37 +0800
Wenchao Xia xiaw...@linux.vnet.ibm.com wrote:
于 2013-5-6 21:22, Luiz Capitulino 写道:
On Mon, 06 May 2013 10:09:43 +0800
Wenchao Xia xiaw...@linux.vnet.ibm.com wrote:
于 2013-5-3 10:51, Wenchao Xia 写道:
于 2013-5-2 20:02, Luiz Capitulino 写道:
On Thu, 02 May 2013 10:05:08 +0800
Wenchao Xia xiaw...@linux.vnet.ibm.com wrote:
于 2013-4-30 3:05, Luiz Capitulino 写道:
On Fri, 26 Apr 2013 16:46:57 +0200
Stefan Hajnoczi stefa...@gmail.com wrote:
On Fri, Apr 26, 2013 at 05:31:15PM +0800, Wenchao Xia wrote:
@@ -2586,10 +2585,12 @@ void do_info_snapshots(Monitor
*mon, const
QDict *qdict)
}
if (total 0) {
-monitor_printf(mon, %s\n, bdrv_snapshot_dump(buf,
sizeof(buf), NULL));
+bdrv_snapshot_dump(NULL);
+monitor_printf(mon, \n);
Luiz: any issue with mixing monitor_printf(mon) and
monitor_vprintf(cur_mon) calls? I guess there was a
reason for
explicitly passing mon instead of relying on cur_mon.
where are they being mixed?
bdrv_snapshot_dump() used a global variable
cur_mon inside,
instead
of let caller pass in a explicit montior* mon, I guess
that is the
question.
I'd have to see the code to tell, but yes, what Stefan
described is the
best practice for the Monitor.
I think this would not be a problem until qemu wants
more than one
human monitor console, and then we may require a data
structure to tell
where to output the string: stdout, *mon, or even stderr, and
error_printf() also need to be changed.
Luiz, what is your idea? I'd like to respin v2 if no
issues for it.
As I said before, I'd have to see the code to tell. But
answering your comment,
the code does support multiple monitors.
Hi Luiz,
Sorry to ask again, do you think method above is OK now,
waiting for
your confirm.
Can you point me to the code in question?
Sure, it is
+
+/*
+ * Print to current monitor if we have one, else to stdout. It is
similar with
+ * error_printf().
+ * TODO just like error_vprintf()
+ */
+void message_printf(const char *fmt, ...)
+{
+va_list ap;
+
+va_start(ap, fmt);
+if (cur_mon) {
+monitor_vprintf(cur_mon, fmt, ap);
+} else {
+vfprintf(stdout, fmt, ap);
+}
+va_end(ap);
+}
This function used global variable cur_mon instead of input
parameter,
similar to error_printf().
Why do you need it? Why can't you you use error_printf() for example?
error_printf() will print out to stderr in qemu-img, but stdout is
wanted for those dump info function.
You can refactor the code so that you can pass a FILE *stream argument to
error_vprintf() and maybe add error_printf_stream()?
The name is a bit confusing, maybe qemu_printf()? Another problem is,
monitor have a buf[] instead of a FILE*, I think it need a structure
include those:
typdef enum QemuOutputType {
QEMU_OUTPUT_TYPE_STREAM,
QEMU_OUTPUT_TYPE_MONITOR,
} QemuOutputType;
typedef struct QemuOutput {
QemuOutputType type;
union {
FILE *file;
Monitor *mon;
};
}
It may brings some inconvienience to caller, but this is what I can
think out now.
Luiz, I am going to respin V2 as above, can I have you confirm on it?
I'm honestly a bit confused with what you're suggesting. I'd just respin the
patches and restart the discussion.
Re: [Qemu-devel] [PATCH 0/9 v3] Make monitor command 'dump-guest-memory' dump in kdump-compressed format
On Wed, 22 May 2013 14:37:37 +0800 Qiao Nuohan qiaonuo...@cn.fujitsu.com wrote: Hi eric and luiz, Does you have some comments on this version? I haven't reviewed it yet, but we need introspection support before merging this.
[Qemu-devel] [PATCH] monitor: allow to disable the default monitor
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
qemu-options.hx | 1 +
vl.c| 4 +++-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/qemu-options.hx b/qemu-options.hx
index fb3961d..bf94862 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2528,6 +2528,7 @@ Redirect the monitor to host device @var{dev} (same
devices as the
serial port).
The default device is @code{vc} in graphical mode and @code{stdio} in
non graphical mode.
+Use @code{-monitor none} to disable the default monitor.
ETEXI
DEF(qmp, HAS_ARG, QEMU_OPTION_qmp, \
-qmp devlike -monitor but opens in 'control' mode\n,
diff --git a/vl.c b/vl.c
index 59dc0b4..510d2c2 100644
--- a/vl.c
+++ b/vl.c
@@ -3366,8 +3366,10 @@ int main(int argc, char **argv, char **envp)
break;
}
case QEMU_OPTION_monitor:
-monitor_parse(optarg, readline);
default_monitor = 0;
+if (strncmp(optarg, none, 4)) {
+monitor_parse(optarg, readline);
+}
break;
case QEMU_OPTION_qmp:
monitor_parse(optarg, control);
--
1.8.1.4
Re: [Qemu-devel] RFC: Full introspection support for QMP
On Wed, 22 May 2013 21:40:07 +0800
Amos Kong ak...@redhat.com wrote:
Hi all,
We already have query-command-line-options to query details of command-line
options. As we discussed in the list, we also need full introspection of QMP
(command). The qmp-events also need to be dumped, we can define events in
qai-schema.json. We can also dump QMP errors in future if it's needed.
Command name: query-qmp-schema
Return: returns the contents of qapi-schema.json in json format.
Solution to query json content from C code:
qapi-schema.json is processed by qapi python scripts to generate C
files, I found the content is good enough for Libvirt to know the
QMP command schema. We can change qapi scripts to generate a talbe/list
to record the raw string, then we can return the raw string in
qmp_query_qmp_schema().
By default, return the complete schema in one go.
And support to query of unknown type in new command.
- { execute: query-qmp-schema arguments: { command: query-status
}}
- { return : data: { command': query-status, returns:
StatusInfo }}
- { execute: query-qmp-schema arguments: { type: StatusInfo }}
- { return : data: { type: StatusInfo, data: {running: bool,
singlestep: bool, status: RunState} }
Looks good, but as Kevin said an example of the schema output would be good.
Feel free to post patches along :)
- { execute: query-qmp-schema arguments: { event:
RX-FILTER-CHANGE }}
You'll need to add the events to the schema some way. I'm not sure I'd mix
the projects in the same series, maybe you could add event support to the
schema after query-qmp-schema gets merged.
Welcome your comments, thanks!
Target: 1.6
Related RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=557939
Re: [Qemu-devel] RFC: Full introspection support for QMP
On Thu, 23 May 2013 07:08:59 -0500 Anthony Liguori aligu...@us.ibm.com wrote: then we don't need introspection at all. There's no user for it then. Introspection is not the right approach to feature discovery. The schema does answer the question of what features are enabled, it just answers the question of what the signature of the methods are. (s/does answer/does not answer) But there's an intersection here: a new enum value or new argument can be a new feature too. If we add new commands to query features, than I'm afraid that in the long run feature discovery will be split in query-qmp-schema and the specific feature discovery commands. I'm not arguing in favor of one or another way, but we need to know why and where we're going. The real motivation behind full introspection is to allow commands to be extended.
Re: [Qemu-devel] Designing QMP APIs at KVM Forum
On Thu, 23 May 2013 13:51:22 +0200 Stefan Hajnoczi stefa...@gmail.com wrote: With better QMP introspection on the horizon and work in various subsystems pushing QMP boundaries it would be useful to bring together the latest best practices for designing QMP APIs. There are design rules for keeping QMP APIs extensible and for allowing clients to detect the presence of features. There is also QEMU-side infrastructure like event rate-limiting, which developers should make use of where appropriate. Is anyone willing to bring together the best practices and present them at KVM Forum this year? I think this is a great idea and I vote for Eric to prepare a presentation. Eric is doing an exceptional work on QMP command review, he is also experienced on the client side. I think that could help set the standard for QMP APIs. A set of slides or wiki page can be a reference to developers that stops us working from first pricinples every time a new API is added. I was working on a doc to be added to docs/. It wouldn't be anything fancy, but I ended up not finishing it.
Re: [Qemu-devel] [PATCH] ui/input.c: replace magic numbers with macros
On Thu, 16 May 2013 13:19:47 +0800
Amos Kong ak...@redhat.com wrote:
It's clearer to use defined macros than magic numbers.
Signed-off-by: Amos Kong ak...@redhat.com
Applied to the qmp branch, thanks.
---
ui/input.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/ui/input.c b/ui/input.c
index 8ca1a03..92c44ca 100644
--- a/ui/input.c
+++ b/ui/input.c
@@ -28,6 +28,7 @@
#include qapi/error.h
#include qmp-commands.h
#include qapi-types.h
+#include ui/keymaps.h
struct QEMUPutMouseEntry {
QEMUPutMouseEvent *qemu_put_mouse_event;
@@ -260,10 +261,10 @@ static void free_keycodes(void)
static void release_keys(void *opaque)
{
while (keycodes_size 0) {
-if (keycodes[--keycodes_size] 0x80) {
-kbd_put_keycode(0xe0);
+if (keycodes[--keycodes_size] SCANCODE_GREY) {
+kbd_put_keycode(SCANCODE_EMUL0);
}
-kbd_put_keycode(keycodes[keycodes_size] | 0x80);
+kbd_put_keycode(keycodes[keycodes_size] | SCANCODE_UP);
}
free_keycodes();
@@ -297,10 +298,10 @@ void qmp_send_key(KeyValueList *keys, bool
has_hold_time, int64_t hold_time,
return;
}
-if (keycode 0x80) {
-kbd_put_keycode(0xe0);
+if (keycode SCANCODE_GREY) {
+kbd_put_keycode(SCANCODE_EMUL0);
}
-kbd_put_keycode(keycode 0x7f);
+kbd_put_keycode(keycode SCANCODE_KEYCODEMASK);
keycodes = g_realloc(keycodes, sizeof(int) * (keycodes_size + 1));
keycodes[keycodes_size++] = keycode;
Re: [Qemu-devel] [PATCH v3 1/2] net: introduce RX_FILTER_CHANGED event
On Thu, 23 May 2013 13:24:59 +0300
Michael S. Tsirkin m...@redhat.com wrote:
On Thu, May 23, 2013 at 05:07:59PM +0800, Amos Kong wrote:
Introduce this new QMP event to notify management after guest changes
rx-filter configuration.
Signed-off-by: Amos Kong ak...@redhat.com
---
QMP/qmp-events.txt| 14 ++
include/monitor/monitor.h | 1 +
monitor.c | 1 +
3 files changed, 16 insertions(+)
diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
index 92fe5fb..ad6612b 100644
--- a/QMP/qmp-events.txt
+++ b/QMP/qmp-events.txt
@@ -154,6 +154,20 @@ Data:
path: /machine/peripheral/virtio-net-pci-0 },
timestamp: { seconds: 1265044230, microseconds: 450486 } }
+RX_FILTER_CHANGED
+-
+
+Emitted when rx-filter configuration is changed by the guest.
Please stress this is only for the NIC. It does not apply
to non-NIC netclients.
Stress it in the event name too, please. I find RX_FILTER_CHANGED a bit
generic.
Also, although I haven't reviewed the next patch yet, I think you
should move the event trigger to this patch.
+
+Data:
+
+- name: net client name (json-string)
Maybe a device path here as well?
+
+{ event: RX_FILTER_CHANGED,
+ data: { name: vnet0 },
+ timestamp: { seconds: 1368697518, microseconds: 326866 }}
+}
+
DEVICE_TRAY_MOVED
-
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 1a6cfcf..c495a67 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -40,6 +40,7 @@ typedef enum MonitorEvent {
QEVENT_BLOCK_JOB_ERROR,
QEVENT_BLOCK_JOB_READY,
QEVENT_DEVICE_DELETED,
+QEVENT_RX_FILTER_CHANGED,
QEVENT_DEVICE_TRAY_MOVED,
QEVENT_SUSPEND,
QEVENT_SUSPEND_DISK,
diff --git a/monitor.c b/monitor.c
index 6ce2a4e..4f7bd48 100644
--- a/monitor.c
+++ b/monitor.c
@@ -489,6 +489,7 @@ static const char *monitor_event_names[] = {
[QEVENT_BLOCK_JOB_ERROR] = BLOCK_JOB_ERROR,
[QEVENT_BLOCK_JOB_READY] = BLOCK_JOB_READY,
[QEVENT_DEVICE_DELETED] = DEVICE_DELETED,
+[QEVENT_RX_FILTER_CHANGED] = RX_FILTER_CHANGED,
[QEVENT_DEVICE_TRAY_MOVED] = DEVICE_TRAY_MOVED,
[QEVENT_SUSPEND] = SUSPEND,
[QEVENT_SUSPEND_DISK] = SUSPEND_DISK,
--
1.8.1.4
Re: [Qemu-devel] [PATCH v3 1/2] net: introduce RX_FILTER_CHANGED event
On Thu, 23 May 2013 17:57:56 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Thu, May 23, 2013 at 08:52:16AM -0600, Eric Blake wrote: On 05/23/2013 08:43 AM, Michael S. Tsirkin wrote: Please stress this is only for the NIC. It does not apply to non-NIC netclients. Stress it in the event name too, please. I find RX_FILTER_CHANGED a bit generic. What do you suggest? NIC_RX_FILTER_CHANGED ? Yes, that might work. (And whatever name we bikeshed, insert it in the correct sorted order in the file) Also, although I haven't reviewed the next patch yet, I think you should move the event trigger to this patch. That's hard because of the flag that's shared with the query command. It's fine if this patch declares and sets the flag, but the event is one-shot until the next patch adds the query to clear the flag. And again, the flag should be per-device, not global. Maybe just merge both patches together. They are pretty small anyway. That's what I was going to suggest if it's really hard to move the event to this patch.
Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event
On Thu, 16 May 2013 18:17:23 +0300 Michael S. Tsirkin m...@redhat.com wrote: The existing throttling approach ensures that if the event includes latest guest information, then the host doesn't even have to do do a query, and is guaranteed that reacting to the final event will always see the most recent request. But most importantly, if the existing throttling works, why do we have to invent a one-off approach for this event instead of reusing existing code? Sorry to restart this week old discussion, but I'm now reviewing the patch in question and I dislike how we're coupling the event and the query command. Because of the 1st issue above. A large delay because we Has this been measured? How long is this large delay? Also, is it impossible for management to issue query-rx-filter on a reasonable rate that would also cause the same problems? IOW, how can we be sure we're fixing anything without trying it on a real use-case scenario? exceed an arbitrary throttling rate would be bad for the guest. Contrast with delay in e.g. device delete event. The throttling mechanism is good for events that host cares about, not for events that guest cares about. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
On Thu, 23 May 2013 17:08:00 +0800
Amos Kong ak...@redhat.com wrote:
We want to implement mac programming over macvtap through Libvirt,
related rx-filter configuration contains main mac, some of rx-mode
and mac-table.
The previous patch adds QMP event to notify management of rx-filter
change. This patch adds a monitor command to query rx-filter
information.
A flag is used to avoid events flooding, if user don't query
rx-filter after receives one event, new events won't be sent
to qmp monitor.
(qemu) info rx-filter vnet0
vnet0:
\ promiscuous: on
\ multicast: normal
\ unicast: normal
\ broadcast-allowed: off
\ multicast-overflow: off
\ unicast-overflow: off
\ main-mac: 52:54:00:12:34:56
\ unicast-table:
\ multicast-table:
01:00:5e:00:00:01
33:33:00:00:00:01
33:33:ff:12:34:56
Signed-off-by: Amos Kong ak...@redhat.com
Looks good in general. I have some comments below and restarted the
dicussion about notify_enabled.
---
hmp-commands.hx | 2 ++
hmp.c | 49
hmp.h | 1 +
hw/net/virtio-net.c | 93
+
include/net/net.h | 2 ++
monitor.c | 8 +
net/net.c | 47 +++
qapi-schema.json| 73 +
qmp-commands.hx | 55 +++
9 files changed, 330 insertions(+)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 9cea415..b7863eb 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1639,6 +1639,8 @@ show qdev device model list
show roms
@item info tpm
show the TPM device
+@item info rx-filter [net client name]
+show the rx-filter information for all nics (or for the given nic)
@end table
ETEXI
diff --git a/hmp.c b/hmp.c
index 4fb76ec..5b47382 100644
--- a/hmp.c
+++ b/hmp.c
@@ -653,6 +653,55 @@ void hmp_info_tpm(Monitor *mon, const QDict *qdict)
qapi_free_TPMInfoList(info_list);
}
+void hmp_info_rx_filter(Monitor *mon, const QDict *qdict)
+{
+RxFilterInfoList *filter_list, *entry;
+strList *str_entry;
+bool has_name = qdict_haskey(qdict, name);
+const char *name = NULL;
+
+if (has_name) {
+name = qdict_get_str(qdict, name);
+}
+
+filter_list = qmp_query_rx_filter(has_name, name, NULL);
qmp_query_rx_filter() can fail.
+entry = filter_list;
+
+while (entry) {
+monitor_printf(mon, %s:\n, entry-value-name);
+monitor_printf(mon, \\ promiscuous: %s\n,
+ entry-value-promiscuous ? on : off);
+monitor_printf(mon, \\ multicast: %s\n,
+ RxState_lookup[entry-value-multicast]);
+monitor_printf(mon, \\ unicast: %s\n,
+ RxState_lookup[entry-value-unicast]);
+monitor_printf(mon, \\ broadcast-allowed: %s\n,
+ entry-value-broadcast_allowed ? on : off);
+monitor_printf(mon, \\ multicast-overflow: %s\n,
+ entry-value-multicast_overflow ? on : off);
+monitor_printf(mon, \\ unicast-overflow: %s\n,
+ entry-value-unicast_overflow ? on : off);
+monitor_printf(mon, \\ main-mac: %s\n, entry-value-main_mac);
+
+str_entry = entry-value-unicast_table;
+monitor_printf(mon, \\ unicast-table:\n);
+while (str_entry) {
+monitor_printf(mon, %s\n, str_entry-value);
+str_entry = str_entry-next;
+}
+
+str_entry = entry-value-multicast_table;
+monitor_printf(mon, \\ multicast-table:\n);
+while (str_entry) {
+monitor_printf(mon, %s\n, str_entry-value);
+str_entry = str_entry-next;
+}
Can't that loop be moved to a function?
+
+entry = entry-next;
+}
+qapi_free_RxFilterInfoList(filter_list);
+}
+
void hmp_quit(Monitor *mon, const QDict *qdict)
{
monitor_suspend(mon);
diff --git a/hmp.h b/hmp.h
index 95fe76e..9af733e 100644
--- a/hmp.h
+++ b/hmp.h
@@ -37,6 +37,7 @@ void hmp_info_balloon(Monitor *mon, const QDict *qdict);
void hmp_info_pci(Monitor *mon, const QDict *qdict);
void hmp_info_block_jobs(Monitor *mon, const QDict *qdict);
void hmp_info_tpm(Monitor *mon, const QDict *qdict);
+void hmp_info_rx_filter(Monitor *mon, const QDict *qdict);
void hmp_quit(Monitor *mon, const QDict *qdict);
void hmp_stop(Monitor *mon, const QDict *qdict);
void hmp_system_reset(Monitor *mon, const QDict *qdict);
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 1ea9556..f93e021 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -21,6 +21,8 @@
#include hw/virtio/virtio-net.h
#include net/vhost_net.h
#include hw/virtio/virtio-bus.h
+#include qapi/qmp/qjson.h
+#include monitor/monitor.h
#define VIRTIO_NET_VM_VERSION11
@@ -192,6 +194,90 @@
Re: [Qemu-devel] [PATCH V2 4/5] util: add new function message_printf()
On Thu, 23 May 2013 16:47:15 +0800
Wenchao Xia xiaw...@linux.vnet.ibm.com wrote:
This function takes an input parameter *output, which can be specified by
caller as stderr, stdout or a monitor. error_vprintf() now calls
message_vprintf(),
which is a static function added in this patch.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
This solution looks good to me:
Reviewed-by: Luiz Capitulino lcapitul...@redhat.com
---
include/qemu/error-report.h | 13 +
util/qemu-error.c | 28 ++--
2 files changed, 39 insertions(+), 2 deletions(-)
diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index c902cc1..cdde78b 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -14,6 +14,8 @@
#define QEMU_ERROR_H
#include stdarg.h
+#include stdio.h
+#include qemu/typedefs.h
typedef struct Location {
/* all members are private to qemu-error.c */
@@ -32,6 +34,17 @@ void loc_set_none(void);
void loc_set_cmdline(char **argv, int idx, int cnt);
void loc_set_file(const char *fname, int lno);
+typedef struct QemuOutput {
+enum { OUTPUT_STREAM, OUTPUT_MONITOR } kind;
+union {
+FILE *stream;
+Monitor *monitor;
+};
+} QemuOutput;
+
+void message_printf(const QemuOutput *output, const char *fmt, ...)
+GCC_FMT_ATTR(2, 3);
+
void error_vprintf(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
void error_printf(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
void error_printf_unless_qmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
diff --git a/util/qemu-error.c b/util/qemu-error.c
index 08a36f4..c7ff0a8 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -13,6 +13,25 @@
#include stdio.h
#include monitor/monitor.h
+static GCC_FMT_ATTR(2, 0)
+void message_vprintf(const QemuOutput *output, const char *fmt, va_list ap)
+{
+if (output-kind == OUTPUT_STREAM) {
+vfprintf(output-stream, fmt, ap);
+} else if (output-kind == OUTPUT_MONITOR) {
+monitor_vprintf(output-monitor, fmt, ap);
+}
+}
+
+void message_printf(const QemuOutput *output, const char *fmt, ...)
+{
+va_list ap;
+
+va_start(ap, fmt);
+message_vprintf(output, fmt, ap);
+va_end(ap);
+}
+
/*
* Print to current monitor if we have one, else to stderr.
* TODO should return int, so callers can calculate width, but that
@@ -20,11 +39,16 @@
*/
void error_vprintf(const char *fmt, va_list ap)
{
+QemuOutput output;
+
if (cur_mon) {
-monitor_vprintf(cur_mon, fmt, ap);
+output.kind = OUTPUT_MONITOR;
+output.monitor = cur_mon;
} else {
-vfprintf(stderr, fmt, ap);
+output.kind = OUTPUT_STREAM;
+output.stream = stderr;
}
+message_vprintf(output, fmt, ap);
}
/*
[Qemu-devel] [PULL 00/12] QMP queue
The changes (since 95de21a430f7bc4166a153b1f69b1425c8a99c7b) are available in the following repository: git://repo.or.cz/qemu/qmp-unstable.git queue/qmp Amos Kong (1): ui/input.c: replace magic numbers with macros Luiz Capitulino (1): monitor: allow to disable the default monitor Michael Roth (10): qapi: qapi-types.py, native list support qapi: qapi-visit.py, fix list handling for union types qapi: qapi-visit.py, native list support qapi: enable generation of native list code json-parser: fix handling of large whole number values qapi: add QMP input test for large integers qapi: fix visitor serialization tests for numbers/doubles qapi: add native list coverage for visitor serialization tests qapi: add native list coverage for QMP output visitor tests qapi: add native list coverage for QMP input visitor tests Makefile | 6 +- qapi-schema-test.json | 15 ++ qemu-options.hx| 1 + qobject/json-parser.c | 26 +- scripts/qapi-types.py | 45 +++- scripts/qapi-visit.py | 36 ++- scripts/qapi.py| 23 ++ tests/test-qmp-input-visitor.c | 358 tests/test-qmp-output-visitor.c| 332 ++ tests/test-visitor-serialization.c | 476 ++--- ui/input.c | 13 +- vl.c | 4 +- 12 files changed, 1278 insertions(+), 57 deletions(-) -- 1.8.1.4
[Qemu-devel] [PULL 02/12] qapi: qapi-visit.py, fix list handling for union types
From: Michael Roth mdr...@linux.vnet.ibm.com
Currently we assume non-list types when generating visitor routines for
union types. This is broken, since values like ['Type'] need to mapped
to 'TypeList'.
We already have a type_name() function to handle this that we use for
generating struct visitors, so use that here as well.
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
Reviewed-by: Laszlo Ersek ler...@redhat.com
Reviewed-by: Amos Kong ak...@redhat.com
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
scripts/qapi-visit.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index a276540..4c4de4b 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -174,7 +174,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const
char *name, Error **
''',
abbrev = de_camel_case(name).upper(),
enum = c_fun(de_camel_case(key),False).upper(),
-c_type=members[key],
+c_type=type_name(members[key]),
c_name=c_fun(key))
ret += mcgen('''
--
1.8.1.4
[Qemu-devel] [PULL 01/12] qapi: qapi-types.py, native list support
From: Michael Roth mdr...@linux.vnet.ibm.com
Teach type generators about native types so they can generate the
appropriate linked list types.
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
Reviewed-by: Laszlo Ersek ler...@redhat.com
Reviewed-by: Amos Kong ak...@redhat.com
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
scripts/qapi-types.py | 45 ++---
scripts/qapi.py | 23 +++
2 files changed, 65 insertions(+), 3 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 9e19920..fd42d71 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -16,8 +16,21 @@ import os
import getopt
import errno
-def generate_fwd_struct(name, members):
+def generate_fwd_struct(name, members, builtin_type=False):
+if builtin_type:
+return mcgen('''
+
+typedef struct %(name)sList
+{
+%(type)s value;
+struct %(name)sList *next;
+} %(name)sList;
+''',
+ type=c_type(name),
+ name=name)
+
return mcgen('''
+
typedef struct %(name)s %(name)s;
typedef struct %(name)sList
@@ -164,6 +177,7 @@ void qapi_free_%(type)s(%(c_type)s obj);
def generate_type_cleanup(name):
ret = mcgen('''
+
void qapi_free_%(type)s(%(c_type)s obj)
{
QapiDeallocVisitor *md;
@@ -184,8 +198,9 @@ void qapi_free_%(type)s(%(c_type)s obj)
try:
-opts, args = getopt.gnu_getopt(sys.argv[1:], chp:o:,
- [source, header, prefix=,
output-dir=])
+opts, args = getopt.gnu_getopt(sys.argv[1:], chbp:o:,
+ [source, header, builtins,
+prefix=, output-dir=])
except getopt.GetoptError, err:
print str(err)
sys.exit(1)
@@ -197,6 +212,7 @@ h_file = 'qapi-types.h'
do_c = False
do_h = False
+do_builtins = False
for o, a in opts:
if o in (-p, --prefix):
@@ -207,6 +223,8 @@ for o, a in opts:
do_c = True
elif o in (-h, --header):
do_h = True
+elif o in (-b, --builtins):
+do_builtins = True
if not do_c and not do_h:
do_c = True
@@ -282,6 +300,11 @@ fdecl.write(mcgen('''
exprs = parse_schema(sys.stdin)
exprs = filter(lambda expr: not expr.has_key('gen'), exprs)
+fdecl.write(guardstart(QAPI_TYPES_BUILTIN_STRUCT_DECL))
+for typename in builtin_types:
+fdecl.write(generate_fwd_struct(typename, None, builtin_type=True))
+fdecl.write(guardend(QAPI_TYPES_BUILTIN_STRUCT_DECL))
+
for expr in exprs:
ret = \n
if expr.has_key('type'):
@@ -298,6 +321,22 @@ for expr in exprs:
continue
fdecl.write(ret)
+# to avoid header dependency hell, we always generate declarations
+# for built-in types in our header files and simply guard them
+fdecl.write(guardstart(QAPI_TYPES_BUILTIN_CLEANUP_DECL))
+for typename in builtin_types:
+fdecl.write(generate_type_cleanup_decl(typename + List))
+fdecl.write(guardend(QAPI_TYPES_BUILTIN_CLEANUP_DECL))
+
+# ...this doesn't work for cases where we link in multiple objects that
+# have the functions defined, so we use -b option to provide control
+# over these cases
+if do_builtins:
+fdef.write(guardstart(QAPI_TYPES_BUILTIN_CLEANUP_DEF))
+for typename in builtin_types:
+fdef.write(generate_type_cleanup(typename + List))
+fdef.write(guardend(QAPI_TYPES_BUILTIN_CLEANUP_DEF))
+
for expr in exprs:
ret = \n
if expr.has_key('type'):
diff --git a/scripts/qapi.py b/scripts/qapi.py
index afc5f32..02ad668 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -11,6 +11,12 @@
from ordereddict import OrderedDict
+builtin_types = [
+'str', 'int', 'number', 'bool',
+'int8', 'int16', 'int32', 'int64',
+'uint8', 'uint16', 'uint32', 'uint64'
+]
+
def tokenize(data):
while len(data):
ch = data[0]
@@ -242,3 +248,20 @@ def guardname(filename):
for substr in [., , -]:
guard = guard.replace(substr, _)
return guard.upper() + '_H'
+
+def guardstart(name):
+return mcgen('''
+
+#ifndef %(name)s
+#define %(name)s
+
+''',
+ name=guardname(name))
+
+def guardend(name):
+return mcgen('''
+
+#endif /* %(name)s */
+
+''',
+ name=guardname(name))
--
1.8.1.4
[Qemu-devel] [PULL 08/12] qapi: add native list coverage for visitor serialization tests
From: Michael Roth mdr...@linux.vnet.ibm.com
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
Reviewed-by: Laszlo Ersek ler...@redhat.com
Reviewed-by: Amos Kong ak...@redhat.com
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
tests/test-visitor-serialization.c | 451 +++--
1 file changed, 433 insertions(+), 18 deletions(-)
diff --git a/tests/test-visitor-serialization.c
b/tests/test-visitor-serialization.c
index fed6810..ee7916b 100644
--- a/tests/test-visitor-serialization.c
+++ b/tests/test-visitor-serialization.c
@@ -23,6 +23,25 @@
#include qapi/qmp-output-visitor.h
#include qapi/string-input-visitor.h
#include qapi/string-output-visitor.h
+#include qapi-types.h
+#include qapi-visit.h
+#include qapi/dealloc-visitor.h
+
+enum PrimitiveTypeKind {
+PTYPE_STRING = 0,
+PTYPE_BOOLEAN,
+PTYPE_NUMBER,
+PTYPE_INTEGER,
+PTYPE_U8,
+PTYPE_U16,
+PTYPE_U32,
+PTYPE_U64,
+PTYPE_S8,
+PTYPE_S16,
+PTYPE_S32,
+PTYPE_S64,
+PTYPE_EOL,
+};
typedef struct PrimitiveType {
union {
@@ -40,26 +59,42 @@ typedef struct PrimitiveType {
int64_t s64;
intmax_t max;
} value;
-enum {
-PTYPE_STRING = 0,
-PTYPE_BOOLEAN,
-PTYPE_NUMBER,
-PTYPE_INTEGER,
-PTYPE_U8,
-PTYPE_U16,
-PTYPE_U32,
-PTYPE_U64,
-PTYPE_S8,
-PTYPE_S16,
-PTYPE_S32,
-PTYPE_S64,
-PTYPE_EOL,
-} type;
+enum PrimitiveTypeKind type;
const char *description;
} PrimitiveType;
+typedef struct PrimitiveList {
+union {
+strList *strings;
+boolList *booleans;
+numberList *numbers;
+intList *integers;
+int8List *s8_integers;
+int16List *s16_integers;
+int32List *s32_integers;
+int64List *s64_integers;
+uint8List *u8_integers;
+uint16List *u16_integers;
+uint32List *u32_integers;
+uint64List *u64_integers;
+} value;
+enum PrimitiveTypeKind type;
+const char *description;
+} PrimitiveList;
+
/* test helpers */
+typedef void (*VisitorFunc)(Visitor *v, void **native, Error **errp);
+
+static void dealloc_helper(void *native_in, VisitorFunc visit, Error **errp)
+{
+QapiDeallocVisitor *qdv = qapi_dealloc_visitor_new();
+
+visit(qapi_dealloc_get_visitor(qdv), native_in, errp);
+
+qapi_dealloc_visitor_cleanup(qdv);
+}
+
static void visit_primitive_type(Visitor *v, void **native, Error **errp)
{
PrimitiveType *pt = *native;
@@ -105,6 +140,51 @@ static void visit_primitive_type(Visitor *v, void
**native, Error **errp)
}
}
+static void visit_primitive_list(Visitor *v, void **native, Error **errp)
+{
+PrimitiveList *pl = *native;
+switch (pl-type) {
+case PTYPE_STRING:
+visit_type_strList(v, pl-value.strings, NULL, errp);
+break;
+case PTYPE_BOOLEAN:
+visit_type_boolList(v, pl-value.booleans, NULL, errp);
+break;
+case PTYPE_NUMBER:
+visit_type_numberList(v, pl-value.numbers, NULL, errp);
+break;
+case PTYPE_INTEGER:
+visit_type_intList(v, pl-value.integers, NULL, errp);
+break;
+case PTYPE_S8:
+visit_type_int8List(v, pl-value.s8_integers, NULL, errp);
+break;
+case PTYPE_S16:
+visit_type_int16List(v, pl-value.s16_integers, NULL, errp);
+break;
+case PTYPE_S32:
+visit_type_int32List(v, pl-value.s32_integers, NULL, errp);
+break;
+case PTYPE_S64:
+visit_type_int64List(v, pl-value.s64_integers, NULL, errp);
+break;
+case PTYPE_U8:
+visit_type_uint8List(v, pl-value.u8_integers, NULL, errp);
+break;
+case PTYPE_U16:
+visit_type_uint16List(v, pl-value.u16_integers, NULL, errp);
+break;
+case PTYPE_U32:
+visit_type_uint32List(v, pl-value.u32_integers, NULL, errp);
+break;
+case PTYPE_U64:
+visit_type_uint64List(v, pl-value.u64_integers, NULL, errp);
+break;
+default:
+g_assert(false);
+}
+}
+
typedef struct TestStruct
{
int64_t integer;
@@ -206,12 +286,11 @@ static void visit_nested_struct_list(Visitor *v, void
**native, Error **errp)
/* test cases */
-typedef void (*VisitorFunc)(Visitor *v, void **native, Error **errp);
-
typedef enum VisitorCapabilities {
VCAP_PRIMITIVES = 1,
VCAP_STRUCTURES = 2,
VCAP_LISTS = 4,
+VCAP_PRIMITIVE_LISTS = 8,
} VisitorCapabilities;
typedef struct SerializeOps {
@@ -270,6 +349,328 @@ static void test_primitives(gconstpointer opaque)
g_free(pt_copy);
}
+static void test_primitive_lists(gconstpointer opaque)
+{
+TestArgs *args = (TestArgs *) opaque;
+const SerializeOps *ops = args-ops;
+PrimitiveType *pt = args-test_data;
+PrimitiveList pl = { .value = { 0 } };
+PrimitiveList pl_copy = { .value = { 0 } };
+PrimitiveList *pl_copy_ptr = pl_copy
[Qemu-devel] [PULL 03/12] qapi: qapi-visit.py, native list support
From: Michael Roth mdr...@linux.vnet.ibm.com
Teach visitor generators about native types so they can generate the
appropriate visitor routines.
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
Reviewed-by: Laszlo Ersek ler...@redhat.com
Reviewed-by: Amos Kong ak...@redhat.com
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
scripts/qapi-visit.py | 34 +-
1 file changed, 29 insertions(+), 5 deletions(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 4c4de4b..6cac05a 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -202,12 +202,14 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj,
const char *name, Error **
return ret
-def generate_declaration(name, members, genlist=True):
-ret = mcgen('''
+def generate_declaration(name, members, genlist=True, builtin_type=False):
+ret =
+if not builtin_type:
+ret += mcgen('''
void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error
**errp);
''',
-name=name)
+name=name)
if genlist:
ret += mcgen('''
@@ -235,8 +237,9 @@ void visit_type_%(name)s(Visitor *m, %(name)s * obj, const
char *name, Error **e
name=name)
try:
-opts, args = getopt.gnu_getopt(sys.argv[1:], chp:o:,
- [source, header, prefix=,
output-dir=])
+opts, args = getopt.gnu_getopt(sys.argv[1:], chbp:o:,
+ [source, header, builtins, prefix=,
+output-dir=])
except getopt.GetoptError, err:
print str(err)
sys.exit(1)
@@ -248,6 +251,7 @@ h_file = 'qapi-visit.h'
do_c = False
do_h = False
+do_builtins = False
for o, a in opts:
if o in (-p, --prefix):
@@ -258,6 +262,8 @@ for o, a in opts:
do_c = True
elif o in (-h, --header):
do_h = True
+elif o in (-b, --builtins):
+do_builtins = True
if not do_c and not do_h:
do_c = True
@@ -324,11 +330,29 @@ fdecl.write(mcgen('''
#include qapi/visitor.h
#include %(prefix)sqapi-types.h
+
''',
prefix=prefix, guard=guardname(h_file)))
exprs = parse_schema(sys.stdin)
+# to avoid header dependency hell, we always generate declarations
+# for built-in types in our header files and simply guard them
+fdecl.write(guardstart(QAPI_VISIT_BUILTIN_VISITOR_DECL))
+for typename in builtin_types:
+fdecl.write(generate_declaration(typename, None, genlist=True,
+ builtin_type=True))
+fdecl.write(guardend(QAPI_VISIT_BUILTIN_VISITOR_DECL))
+
+# ...this doesn't work for cases where we link in multiple objects that
+# have the functions defined, so we use -b option to provide control
+# over these cases
+if do_builtins:
+fdef.write(guardstart(QAPI_VISIT_BUILTIN_VISITOR_DEF))
+for typename in builtin_types:
+fdef.write(generate_visit_list(typename, None))
+fdef.write(guardend(QAPI_VISIT_BUILTIN_VISITOR_DEF))
+
for expr in exprs:
if expr.has_key('type'):
ret = generate_visit_struct(expr['type'], expr['data'])
--
1.8.1.4
[Qemu-devel] [PULL 06/12] qapi: add QMP input test for large integers
From: Michael Roth mdr...@linux.vnet.ibm.com
Large integers previously got capped to LLONG_MAX/LLONG_MIN so we could
store them as int64_t. This could lead to silent errors occuring.
Now, we use a double to handle these cases.
Add a test to confirm that QMPInputVisitor handles this as expected if
we're expected an integer value: errors for out of range integer values
that got promoted to doubles in this fashion.
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
Reviewed-by: Laszlo Ersek ler...@redhat.com
Reviewed-by: Amos Kong ak...@redhat.com
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
tests/test-qmp-input-visitor.c | 20
1 file changed, 20 insertions(+)
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 955a4c0..b308cf9 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -75,6 +75,24 @@ static void test_visitor_in_int(TestInputVisitorData *data,
g_assert_cmpint(res, ==, value);
}
+static void test_visitor_in_int_overflow(TestInputVisitorData *data,
+ const void *unused)
+{
+int64_t res = 0;
+Error *errp = NULL;
+Visitor *v;
+
+/* this will overflow a Qint/int64, so should be deserialized into
+ * a QFloat/double field instead, leading to an error if we pass it
+ * to visit_type_int. confirm this.
+ */
+v = visitor_input_test_init(data, %f, DBL_MAX);
+
+visit_type_int(v, res, NULL, errp);
+g_assert(error_is_set(errp));
+error_free(errp);
+}
+
static void test_visitor_in_bool(TestInputVisitorData *data,
const void *unused)
{
@@ -292,6 +310,8 @@ int main(int argc, char **argv)
input_visitor_test_add(/visitor/input/int,
in_visitor_data, test_visitor_in_int);
+input_visitor_test_add(/visitor/input/int_overflow,
+ in_visitor_data, test_visitor_in_int_overflow);
input_visitor_test_add(/visitor/input/bool,
in_visitor_data, test_visitor_in_bool);
input_visitor_test_add(/visitor/input/number,
--
1.8.1.4
[Qemu-devel] [PULL 10/12] qapi: add native list coverage for QMP input visitor tests
From: Michael Roth mdr...@linux.vnet.ibm.com
This exercises schema-generated visitors for native list types and does
some sanity checking on validity of deserialized data.
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
Reviewed-by: Laszlo Ersek ler...@redhat.com
Reviewed-by: Amos Kong ak...@redhat.com
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
tests/test-qmp-input-visitor.c | 338 +
1 file changed, 338 insertions(+)
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index b308cf9..2741eef 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -61,6 +61,31 @@ Visitor *visitor_input_test_init(TestInputVisitorData *data,
return v;
}
+/* similar to visitor_input_test_init(), but does not expect a string
+ * literal/format json_string argument and so can be used for
+ * programatically generated strings (and we can't pass in programatically
+ * generated strings via %s format parameters since qobject_from_jsonv()
+ * will wrap those in double-quotes and treat the entire object as a
+ * string)
+ */
+static Visitor *visitor_input_test_init_raw(TestInputVisitorData *data,
+const char *json_string)
+{
+Visitor *v;
+
+data-obj = qobject_from_json(json_string);
+
+g_assert(data-obj != NULL);
+
+data-qiv = qmp_input_visitor_new(data-obj);
+g_assert(data-qiv != NULL);
+
+v = qmp_input_get_visitor(data-qiv);
+g_assert(v != NULL);
+
+return v;
+}
+
static void test_visitor_in_int(TestInputVisitorData *data,
const void *unused)
{
@@ -277,6 +302,287 @@ static void test_visitor_in_union(TestInputVisitorData
*data,
qapi_free_UserDefUnion(tmp);
}
+static void test_native_list_integer_helper(TestInputVisitorData *data,
+const void *unused,
+UserDefNativeListUnionKind kind)
+{
+UserDefNativeListUnion *cvalue = NULL;
+Error *err = NULL;
+Visitor *v;
+GString *gstr_list = g_string_new();
+GString *gstr_union = g_string_new();
+int i;
+
+for (i = 0; i 32; i++) {
+g_string_append_printf(gstr_list, %d, i);
+if (i != 31) {
+g_string_append(gstr_list, , );
+}
+}
+g_string_append_printf(gstr_union, { 'type': '%s', 'data': [ %s ] },
+ UserDefNativeListUnionKind_lookup[kind],
+ gstr_list-str);
+v = visitor_input_test_init_raw(data, gstr_union-str);
+
+visit_type_UserDefNativeListUnion(v, cvalue, NULL, err);
+g_assert(err == NULL);
+g_assert(cvalue != NULL);
+g_assert_cmpint(cvalue-kind, ==, kind);
+
+switch (kind) {
+case USER_DEF_NATIVE_LIST_UNION_KIND_INTEGER: {
+intList *elem = NULL;
+for (i = 0, elem = cvalue-integer; elem; elem = elem-next, i++) {
+g_assert_cmpint(elem-value, ==, i);
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_S8: {
+int8List *elem = NULL;
+for (i = 0, elem = cvalue-s8; elem; elem = elem-next, i++) {
+g_assert_cmpint(elem-value, ==, i);
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_S16: {
+int16List *elem = NULL;
+for (i = 0, elem = cvalue-s16; elem; elem = elem-next, i++) {
+g_assert_cmpint(elem-value, ==, i);
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_S32: {
+int32List *elem = NULL;
+for (i = 0, elem = cvalue-s32; elem; elem = elem-next, i++) {
+g_assert_cmpint(elem-value, ==, i);
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_S64: {
+int64List *elem = NULL;
+for (i = 0, elem = cvalue-s64; elem; elem = elem-next, i++) {
+g_assert_cmpint(elem-value, ==, i);
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_U8: {
+uint8List *elem = NULL;
+for (i = 0, elem = cvalue-u8; elem; elem = elem-next, i++) {
+g_assert_cmpint(elem-value, ==, i);
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_U16: {
+uint16List *elem = NULL;
+for (i = 0, elem = cvalue-u16; elem; elem = elem-next, i++) {
+g_assert_cmpint(elem-value, ==, i);
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_U32: {
+uint32List *elem = NULL;
+for (i = 0, elem = cvalue-u32; elem; elem = elem-next, i++) {
+g_assert_cmpint(elem-value, ==, i);
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_U64: {
+uint64List *elem = NULL;
+for (i = 0, elem = cvalue-u64; elem; elem = elem-next, i++) {
+g_assert_cmpint(elem-value, ==, i);
+}
+break;
+}
+default:
+g_assert(false
[Qemu-devel] [PULL 09/12] qapi: add native list coverage for QMP output visitor tests
From: Michael Roth mdr...@linux.vnet.ibm.com
This exercises schema-generated visitors for native list types and does
some sanity checking on validity of serialized data.
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
Reviewed-by: Laszlo Ersek ler...@redhat.com
Reviewed-by: Amos Kong ak...@redhat.com
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
qapi-schema-test.json | 15 ++
tests/test-qmp-output-visitor.c | 332
2 files changed, 347 insertions(+)
diff --git a/qapi-schema-test.json b/qapi-schema-test.json
index 9eae350..4434fa3 100644
--- a/qapi-schema-test.json
+++ b/qapi-schema-test.json
@@ -32,6 +32,21 @@
{ 'union': 'UserDefUnion',
'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } }
+# for testing native lists
+{ 'union': 'UserDefNativeListUnion',
+ 'data': { 'integer': ['int'],
+'s8': ['int8'],
+'s16': ['int16'],
+'s32': ['int32'],
+'s64': ['int64'],
+'u8': ['uint8'],
+'u16': ['uint16'],
+'u32': ['uint32'],
+'u64': ['uint64'],
+'number': ['number'],
+'boolean': ['bool'],
+'string': ['str'] } }
+
# testing commands
{ 'command': 'user_def_cmd', 'data': {} }
{ 'command': 'user_def_cmd1', 'data': {'ud1a': 'UserDefOne'} }
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 71367e6..0942a41 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -431,6 +431,314 @@ static void test_visitor_out_union(TestOutputVisitorData
*data,
QDECREF(qdict);
}
+static void init_native_list(UserDefNativeListUnion *cvalue)
+{
+int i;
+switch (cvalue-kind) {
+case USER_DEF_NATIVE_LIST_UNION_KIND_INTEGER: {
+intList **list = cvalue-integer;
+for (i = 0; i 32; i++) {
+*list = g_new0(intList, 1);
+(*list)-value = i;
+(*list)-next = NULL;
+list = (*list)-next;
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_S8: {
+int8List **list = cvalue-s8;
+for (i = 0; i 32; i++) {
+*list = g_new0(int8List, 1);
+(*list)-value = i;
+(*list)-next = NULL;
+list = (*list)-next;
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_S16: {
+int16List **list = cvalue-s16;
+for (i = 0; i 32; i++) {
+*list = g_new0(int16List, 1);
+(*list)-value = i;
+(*list)-next = NULL;
+list = (*list)-next;
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_S32: {
+int32List **list = cvalue-s32;
+for (i = 0; i 32; i++) {
+*list = g_new0(int32List, 1);
+(*list)-value = i;
+(*list)-next = NULL;
+list = (*list)-next;
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_S64: {
+int64List **list = cvalue-s64;
+for (i = 0; i 32; i++) {
+*list = g_new0(int64List, 1);
+(*list)-value = i;
+(*list)-next = NULL;
+list = (*list)-next;
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_U8: {
+uint8List **list = cvalue-u8;
+for (i = 0; i 32; i++) {
+*list = g_new0(uint8List, 1);
+(*list)-value = i;
+(*list)-next = NULL;
+list = (*list)-next;
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_U16: {
+uint16List **list = cvalue-u16;
+for (i = 0; i 32; i++) {
+*list = g_new0(uint16List, 1);
+(*list)-value = i;
+(*list)-next = NULL;
+list = (*list)-next;
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_U32: {
+uint32List **list = cvalue-u32;
+for (i = 0; i 32; i++) {
+*list = g_new0(uint32List, 1);
+(*list)-value = i;
+(*list)-next = NULL;
+list = (*list)-next;
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_U64: {
+uint64List **list = cvalue-u64;
+for (i = 0; i 32; i++) {
+*list = g_new0(uint64List, 1);
+(*list)-value = i;
+(*list)-next = NULL;
+list = (*list)-next;
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_BOOLEAN: {
+boolList **list = cvalue-boolean;
+for (i = 0; i 32; i++) {
+*list = g_new0(boolList, 1);
+(*list)-value = (i % 3 == 0);
+(*list)-next = NULL;
+list = (*list)-next;
+}
+break;
+}
+case USER_DEF_NATIVE_LIST_UNION_KIND_STRING: {
+strList **list = cvalue-string;
+for (i = 0; i 32; i++) {
+*list = g_new0(strList, 1);
+(*list)-value
[Qemu-devel] [PULL 07/12] qapi: fix visitor serialization tests for numbers/doubles
From: Michael Roth mdr...@linux.vnet.ibm.com
We never actually stored the stringified double values into the strings
before we did the comparisons. This left number/double values completely
uncovered in test-visitor-serialization tests.
Fixing this exposed a bug in our handling of large whole number values
in QEMU's JSON parser which is now fixed.
Simplify the code while we're at it by dropping the
calc_float_string_storage() craziness in favor of GStrings.
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
Reviewed-by: Laszlo Ersek ler...@redhat.com
Reviewed-by: Amos Kong ak...@redhat.com
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
tests/test-visitor-serialization.c | 25 -
1 file changed, 8 insertions(+), 17 deletions(-)
diff --git a/tests/test-visitor-serialization.c
b/tests/test-visitor-serialization.c
index 8c8adac..fed6810 100644
--- a/tests/test-visitor-serialization.c
+++ b/tests/test-visitor-serialization.c
@@ -229,17 +229,6 @@ typedef struct TestArgs {
void *test_data;
} TestArgs;
-#define FLOAT_STRING_PRECISION 6 /* corresponding to n in %.nf formatting */
-static gsize calc_float_string_storage(double value)
-{
-int whole_value = value;
-gsize i = 0;
-do {
-i++;
-} while (whole_value /= 10);
-return i + 2 + FLOAT_STRING_PRECISION;
-}
-
static void test_primitives(gconstpointer opaque)
{
TestArgs *args = (TestArgs *) opaque;
@@ -248,7 +237,6 @@ static void test_primitives(gconstpointer opaque)
PrimitiveType *pt_copy = g_malloc0(sizeof(*pt_copy));
Error *err = NULL;
void *serialize_data;
-char *double1, *double2;
pt_copy-type = pt-type;
ops-serialize(pt, serialize_data, visit_primitive_type, err);
@@ -260,14 +248,17 @@ static void test_primitives(gconstpointer opaque)
g_assert_cmpstr(pt-value.string, ==, pt_copy-value.string);
g_free((char *)pt_copy-value.string);
} else if (pt-type == PTYPE_NUMBER) {
+GString *double_expected = g_string_new();
+GString *double_actual = g_string_new();
/* we serialize with %f for our reference visitors, so rather than
fuzzy
* floating math to test equality, just compare the formatted values
*/
-double1 = g_malloc0(calc_float_string_storage(pt-value.number));
-double2 = g_malloc0(calc_float_string_storage(pt_copy-value.number));
-g_assert_cmpstr(double1, ==, double2);
-g_free(double1);
-g_free(double2);
+g_string_printf(double_expected, %.6f, pt-value.number);
+g_string_printf(double_actual, %.6f, pt_copy-value.number);
+g_assert_cmpstr(double_actual-str, ==, double_expected-str);
+
+g_string_free(double_expected, true);
+g_string_free(double_actual, true);
} else if (pt-type == PTYPE_BOOLEAN) {
g_assert_cmpint(!!pt-value.max, ==, !!pt-value.max);
} else {
--
1.8.1.4
[Qemu-devel] [PULL 05/12] json-parser: fix handling of large whole number values
From: Michael Roth mdr...@linux.vnet.ibm.com
Currently our JSON parser assumes that numbers lacking a fractional
value are integers and attempts to store them as QInt/int64 values. This
breaks in the case where the number overflows/underflows int64 values (which
is still valid JSON)
Fix this by detecting such cases and using a QFloat to store the value
instead.
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
Reviewed-by: Laszlo Ersek ler...@redhat.com
Reviewed-by: Amos Kong ak...@redhat.com
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
qobject/json-parser.c | 26 +++---
1 file changed, 23 insertions(+), 3 deletions(-)
diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index 05279c1..e7947b3 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -640,9 +640,29 @@ static QObject *parse_literal(JSONParserContext *ctxt)
case JSON_STRING:
obj = QOBJECT(qstring_from_escaped_str(ctxt, token));
break;
-case JSON_INTEGER:
-obj = QOBJECT(qint_from_int(strtoll(token_get_value(token), NULL,
10)));
-break;
+case JSON_INTEGER: {
+/* A possibility exists that this is a whole-valued float where the
+ * fractional part was left out due to being 0 (.0). It's not a big
+ * deal to treat these as ints in the parser, so long as users of the
+ * resulting QObject know to expect a QInt in place of a QFloat in
+ * cases like these.
+ *
+ * However, in some cases these values will overflow/underflow a
+ * QInt/int64 container, thus we should assume these are to be handled
+ * as QFloats/doubles rather than silently changing their values.
+ *
+ * strtoll() indicates these instances by setting errno to ERANGE
+ */
+int64_t value;
+
+errno = 0; /* strtoll doesn't set errno on success */
+value = strtoll(token_get_value(token), NULL, 10);
+if (errno != ERANGE) {
+obj = QOBJECT(qint_from_int(value));
+break;
+}
+/* fall through to JSON_FLOAT */
+}
case JSON_FLOAT:
/* FIXME dependent on locale */
obj = QOBJECT(qfloat_from_double(strtod(token_get_value(token),
NULL)));
--
1.8.1.4
[Qemu-devel] [PULL 11/12] ui/input.c: replace magic numbers with macros
From: Amos Kong ak...@redhat.com
It's clearer to use defined macros than magic numbers.
Signed-off-by: Amos Kong ak...@redhat.com
Reviewed-by: Lei Li li...@linux.vnet.ibm.com
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
ui/input.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/ui/input.c b/ui/input.c
index 8ca1a03..92c44ca 100644
--- a/ui/input.c
+++ b/ui/input.c
@@ -28,6 +28,7 @@
#include qapi/error.h
#include qmp-commands.h
#include qapi-types.h
+#include ui/keymaps.h
struct QEMUPutMouseEntry {
QEMUPutMouseEvent *qemu_put_mouse_event;
@@ -260,10 +261,10 @@ static void free_keycodes(void)
static void release_keys(void *opaque)
{
while (keycodes_size 0) {
-if (keycodes[--keycodes_size] 0x80) {
-kbd_put_keycode(0xe0);
+if (keycodes[--keycodes_size] SCANCODE_GREY) {
+kbd_put_keycode(SCANCODE_EMUL0);
}
-kbd_put_keycode(keycodes[keycodes_size] | 0x80);
+kbd_put_keycode(keycodes[keycodes_size] | SCANCODE_UP);
}
free_keycodes();
@@ -297,10 +298,10 @@ void qmp_send_key(KeyValueList *keys, bool has_hold_time,
int64_t hold_time,
return;
}
-if (keycode 0x80) {
-kbd_put_keycode(0xe0);
+if (keycode SCANCODE_GREY) {
+kbd_put_keycode(SCANCODE_EMUL0);
}
-kbd_put_keycode(keycode 0x7f);
+kbd_put_keycode(keycode SCANCODE_KEYCODEMASK);
keycodes = g_realloc(keycodes, sizeof(int) * (keycodes_size + 1));
keycodes[keycodes_size++] = keycode;
--
1.8.1.4
[Qemu-devel] [PULL 04/12] qapi: enable generation of native list code
From: Michael Roth mdr...@linux.vnet.ibm.com Also, fix a dependency issue with libqemuutil: qemu-sockets.c needs qapi-types.c/qapi-visit.c Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com Reviewed-by: Laszlo Ersek ler...@redhat.com Reviewed-by: Amos Kong ak...@redhat.com Signed-off-by: Luiz Capitulino lcapitul...@redhat.com --- Makefile | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Makefile b/Makefile index 7dc0204..9695c9d 100644 --- a/Makefile +++ b/Makefile @@ -178,7 +178,7 @@ Makefile: $(version-obj-y) $(version-lobj-y) # Build libraries libqemustub.a: $(stub-obj-y) -libqemuutil.a: $(util-obj-y) +libqemuutil.a: $(util-obj-y) qapi-types.o qapi-visit.o ## @@ -215,10 +215,10 @@ $(SRC_PATH)/qga/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py) qapi-types.c qapi-types.h :\ $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py) - $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py $(gen-out-type) -o . $, GEN $@) + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py $(gen-out-type) -o . -b $, GEN $@) qapi-visit.c qapi-visit.h :\ $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py $(qapi-py) - $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py $(gen-out-type) -o . $, GEN $@) + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py $(gen-out-type) -o . -b $, GEN $@) qmp-commands.h qmp-marshal.c :\ $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py) $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py $(gen-out-type) -m -o . $, GEN $@) -- 1.8.1.4
[Qemu-devel] [PULL 12/12] monitor: allow to disable the default monitor
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
qemu-options.hx | 1 +
vl.c| 4 +++-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/qemu-options.hx b/qemu-options.hx
index fb3961d..bf94862 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2528,6 +2528,7 @@ Redirect the monitor to host device @var{dev} (same
devices as the
serial port).
The default device is @code{vc} in graphical mode and @code{stdio} in
non graphical mode.
+Use @code{-monitor none} to disable the default monitor.
ETEXI
DEF(qmp, HAS_ARG, QEMU_OPTION_qmp, \
-qmp devlike -monitor but opens in 'control' mode\n,
diff --git a/vl.c b/vl.c
index 59dc0b4..510d2c2 100644
--- a/vl.c
+++ b/vl.c
@@ -3366,8 +3366,10 @@ int main(int argc, char **argv, char **envp)
break;
}
case QEMU_OPTION_monitor:
-monitor_parse(optarg, readline);
default_monitor = 0;
+if (strncmp(optarg, none, 4)) {
+monitor_parse(optarg, readline);
+}
break;
case QEMU_OPTION_qmp:
monitor_parse(optarg, control);
--
1.8.1.4
Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event
On Thu, 23 May 2013 20:18:34 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Thu, May 23, 2013 at 11:54:03AM -0400, Luiz Capitulino wrote: On Thu, 16 May 2013 18:17:23 +0300 Michael S. Tsirkin m...@redhat.com wrote: The existing throttling approach ensures that if the event includes latest guest information, then the host doesn't even have to do do a query, and is guaranteed that reacting to the final event will always see the most recent request. But most importantly, if the existing throttling works, why do we have to invent a one-off approach for this event instead of reusing existing code? Sorry to restart this week old discussion, but I'm now reviewing the patch in question and I dislike how we're coupling the event and the query command. Because of the 1st issue above. A large delay because we Has this been measured? How long is this large delay? Also, is it impossible for management to issue query-rx-filter on a reasonable rate that would also cause the same problems? IOW, how can we be sure we're fixing anything without trying it on a real use-case scenario? Play with priorities, you can make management arbitrarily slow. It's just not sane to assume any timing guarantees for tasks running on Linux. Would you mind to elaborate? I'm not sure I understand how this answers my questions.
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
On Fri, 24 May 2013 11:08:13 +0800
Amos Kong ak...@redhat.com wrote:
On Thu, May 23, 2013 at 12:03:25PM -0400, Luiz Capitulino wrote:
On Thu, 23 May 2013 17:08:00 +0800
Amos Kong ak...@redhat.com wrote:
A flag is used to avoid events flooding, if user don't query
rx-filter after receives one event, new events won't be sent
to qmp monitor.
+RxFilterInfoList *qmp_query_rx_filter(bool has_name, const char *name,
+ Error **errp)
+{
+NetClientState *nc;
+RxFilterInfoList *filter_list = NULL, *last_entry = NULL;
+
+QTAILQ_FOREACH(nc, net_clients, next) {
+RxFilterInfoList *entry;
+RxFilterInfo *info;
+
+if (nc-info-type != NET_CLIENT_OPTIONS_KIND_NIC) {
+continue;
+}
+if (has_name strcmp(nc-name, name) != 0) {
+continue;
+}
I don't think we need this argument. This command is quite simple in its
response, let's do this filtering in HMP only.
Event message contains the net client name, management might only want
to query the single net client.
The client can do the filtering itself.
And we plan to use a flag for _each nic_ to control the event notification,
querying single net client will only clear it's flag.
So we need to support query by net-client-name.
Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event
On Fri, 24 May 2013 15:10:16 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Thu, May 23, 2013 at 01:26:33PM -0400, Luiz Capitulino wrote: On Thu, 23 May 2013 20:18:34 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Thu, May 23, 2013 at 11:54:03AM -0400, Luiz Capitulino wrote: On Thu, 16 May 2013 18:17:23 +0300 Michael S. Tsirkin m...@redhat.com wrote: The existing throttling approach ensures that if the event includes latest guest information, then the host doesn't even have to do do a query, and is guaranteed that reacting to the final event will always see the most recent request. But most importantly, if the existing throttling works, why do we have to invent a one-off approach for this event instead of reusing existing code? Sorry to restart this week old discussion, but I'm now reviewing the patch in question and I dislike how we're coupling the event and the query command. Because of the 1st issue above. A large delay because we Has this been measured? How long is this large delay? Also, is it impossible for management to issue query-rx-filter on a reasonable rate that would also cause the same problems? IOW, how can we be sure we're fixing anything without trying it on a real use-case scenario? Play with priorities, you can make management arbitrarily slow. It's just not sane to assume any timing guarantees for tasks running on Linux. Would you mind to elaborate? I'm not sure I understand how this answers my questions. Maybe I don't understand the questions. You are asking why doesn't usual throttling sufficient? This was discussed in this thread already. That's because it would introduce a huge delay if guest changes the mac too often. People don't except that changing a mac is a thing the should do slowly. You meant shouldn't? If I got it correctly, all you want to avoid is to call qobject_from_jsonf() and monitor_protocol_event() in the mac change path, because this will slow down the guest. Did I get it? If I did, my main point is whether or not the solution you're proposing (which is to couple the event with the query command) is appropriate. We're in user-space already, many things could slow the guest down apart from the event generation. Two questions: 1. Do we know how slow (or how many packets are actually dropped) if the mac is changed too often *and* the event is always sent? 2. Does this solution consider what happens if the QMP client does respond timely to the event by issuing the query-rx-filter command?
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
On Fri, 24 May 2013 06:55:44 -0600 Eric Blake ebl...@redhat.com wrote: On 05/24/2013 06:23 AM, Luiz Capitulino wrote: I don't think we need this argument. This command is quite simple in its response, let's do this filtering in HMP only. Event message contains the net client name, management might only want to query the single net client. The client can do the filtering itself. If we're arguing that we want this to be as responsive as possible, then the less data we send over the wire, the faster management can react to the guest's request for a particular NIC. That is, if libvirt is listening to events that says NIC2 wants to change rx-filter, libvirt would rather do a filtered query where it knows the JSON array of 1 element matches NIC2 data, rather than do a global query and search through the returned array until it finds NIC2. This sounds like premature optimization to me, but I wonder if instead of cluttering commands with arguments to do the filtering we could add some standard way of doing this in the QAPI. It was you who suggested a filter command? Filtering is relatively easy to add, whether you do it in QMP or make every client add it. Libvirt will survive if you don't have filtering, but I don't see why we can't have it in QMP. Also, if you DO decide to rip filtering out of QMP, you STILL need to keep a per-NIC flag. Since the events say which NIC is requesting a change, even if the query reads all nics, libvirt will only change the macvtap settings of the nic(s) for which it has received an event (it doesn't make sense to waste time requesting a (no-op) change to macvtap settings on a nic that hasn't requested a change). But if you argue that having no filtering in the QMP command means that you can get away with a single flag instead of a per-nic flag, then you will fail to emit an event for NIC2 if it changes in between the time that NIC1 fired an event and libvirt finally does the query, and libvirt wouldn't realize that NIC2 also needs a macvtap change.
Re: [Qemu-devel] qmp commands get rejected
On Fri, 24 May 2013 07:50:33 +0200
Stefan Priebe s.pri...@profihost.ag wrote:
Hello list,
since upgrading from qemu 1.4.1 to 1.5.0 i've problems with qmp commands.
With Qemu 1.5 i've the following socket communication:
'{execute:qmp_capabilities,id:12125:1,arguments:{}}'
'{return: {}, id: 12125:1}'
'{execute:qom-set,id:12125:2,arguments:{value:2,path:machine/peripheral/balloon0,property:guest-stats-polling-interval}}'
'{QMP: {version: {qemu: {micro: 0, minor: 5, major: 1},
package: }, capabilities: []}}'
'{id: 12125:2, error: {class: CommandNotFound, desc: The
command qom-set has not been found}}'
It seems that the command mode (qmp_capabilities) gets resets by the
welcome banner?
It looks like you got disconnected before qom-set was issued.
Can you share more details on how those commands are being issued?
Re: [Qemu-devel] qmp commands get rejected
On Fri, 24 May 2013 15:57:59 +0200
Stefan Priebe - Profihost AG s.pri...@profihost.ag wrote:
Am 24.05.2013 um 15:23 schrieb Luiz Capitulino lcapitul...@redhat.com:
On Fri, 24 May 2013 07:50:33 +0200
Stefan Priebe s.pri...@profihost.ag wrote:
Hello list,
since upgrading from qemu 1.4.1 to 1.5.0 i've problems with qmp commands.
With Qemu 1.5 i've the following socket communication:
'{execute:qmp_capabilities,id:12125:1,arguments:{}}'
'{return: {}, id: 12125:1}'
'{execute:qom-set,id:12125:2,arguments:{value:2,path:machine/peripheral/balloon0,property:guest-stats-polling-interval}}'
'{QMP: {version: {qemu: {micro: 0, minor: 5, major: 1},
package: }, capabilities: []}}'
'{id: 12125:2, error: {class: CommandNotFound, desc: The
command qom-set has not been found}}'
It seems that the command mode (qmp_capabilities) gets resets by the
welcome banner?
It looks like you got disconnected before qom-set was issued.
No its the same socket connection. No disconnect had happened.
Can you share more details on how those commands are being issued?
They're send through socket with a perl script. What do you need?
That perl script maybe? I can't reproduce the problem.
Re: [Qemu-devel] qmp commands get rejected
On Fri, 24 May 2013 16:36:26 +0200
Stefan Priebe - Profihost AG s.pri...@profihost.ag wrote:
Am 24.05.2013 um 16:02 schrieb Luiz Capitulino lcapitul...@redhat.com:
On Fri, 24 May 2013 15:57:59 +0200
Stefan Priebe - Profihost AG s.pri...@profihost.ag wrote:
Am 24.05.2013 um 15:23 schrieb Luiz Capitulino lcapitul...@redhat.com:
On Fri, 24 May 2013 07:50:33 +0200
Stefan Priebe s.pri...@profihost.ag wrote:
Hello list,
since upgrading from qemu 1.4.1 to 1.5.0 i've problems with qmp commands.
With Qemu 1.5 i've the following socket communication:
'{execute:qmp_capabilities,id:12125:1,arguments:{}}'
'{return: {}, id: 12125:1}'
'{execute:qom-set,id:12125:2,arguments:{value:2,path:machine/peripheral/balloon0,property:guest-stats-polling-interval}}'
'{QMP: {version: {qemu: {micro: 0, minor: 5, major: 1},
package: }, capabilities: []}}'
'{id: 12125:2, error: {class: CommandNotFound, desc: The
command qom-set has not been found}}'
It seems that the command mode (qmp_capabilities) gets resets by the
welcome banner?
It looks like you got disconnected before qom-set was issued.
No its the same socket connection. No disconnect had happened.
Can you share more details on how those commands are being issued?
They're send through socket with a perl script. What do you need?
That perl script maybe? I can't reproduce the problem.
I would try to create a small example script.
I use qmp-shell and other little scripts very often.
Am this be due to the fact that I don't wait for the welcome banner right now?
If you're not reading from the socket, then you'll get the banner back when
you read your first response. But qom-set shouldn't fail because of that.
[Qemu-devel] tcg: Windows guests don't boot
Hi, Today I accidentally started qemu w/o -enable-kvm to run a Windows guest and noticed it didn't boot: sometimes it hangs on a blue screen and sometimes it keeps rebooting in a loop. I tried with Windows 2008 and Windows 8, and went back to qemu v1.2.0 to see if it's a bisectable regression, but no luck. I'm reporting case someone is interested in debugging this.
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
On Fri, 24 May 2013 12:05:12 -0600 Eric Blake ebl...@redhat.com wrote: On 05/24/2013 10:12 AM, Michael S. Tsirkin wrote: Event message contains the net client name, management might only want to query the single net client. The client can do the filtering itself. I'm not sure I buy the responsiveness argument. Sure, the fastest I/O is no I/O, but whether you read and parse 100 bytes or 1000 from a Unix domain socket once in a great while shouldn't make a difference. And the time spent malloc'ing the larger message to send from qemu, as well as the time spent malloc'ing the libvirt side that parses the qemu string into C code for use, and the time spent strcmp'ing every entry to find the right one... It really IS more efficient to filter as low down in the stack as possible, once it is determined that filtering is desirable. Whether filtering makes a difference in performance is a different question - you may be right that always returning the entire list and making libvirt do its own filtering will still not add any more noticeable delay compared to libvirt doing a filtered query, if the bottleneck lies elsewhere (such as libvirt telling macvtap its new configration). My main concern is to keep the external interface simple. I'm rather reluctant to have query commands grow options. In a case where we need the give me everything query anyway, the give me this particular part option is additional complexity. Needs justification, say arguments involving throughput, latency or client complexity. Perhaps cases exist where we never want to ask for everything. Then the give me everything query is useless, and the option should be mandatory. For this _particular_ interface, I'm not sure whether libvirt will ever use an unfiltered query - If having the argument is useful for libvirt, then it's fine to have it. But I'd be very reluctant to buy any performance argument w/o real numbers to back them up.
Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event
On Mon, 27 May 2013 17:34:25 +0800 Amos Kong ak...@redhat.com wrote: On Fri, May 24, 2013 at 08:51:36AM -0400, Luiz Capitulino wrote: On Fri, 24 May 2013 15:10:16 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Thu, May 23, 2013 at 01:26:33PM -0400, Luiz Capitulino wrote: On Thu, 23 May 2013 20:18:34 +0300 Michael S. Tsirkin m...@redhat.com wrote: On Thu, May 23, 2013 at 11:54:03AM -0400, Luiz Capitulino wrote: On Thu, 16 May 2013 18:17:23 +0300 Michael S. Tsirkin m...@redhat.com wrote: The existing throttling approach ensures that if the event includes latest guest information, then the host doesn't even have to do do a query, and is guaranteed that reacting to the final event will always see the most recent request. But most importantly, if the existing throttling works, why do we have to invent a one-off approach for this event instead of reusing existing code? Sorry to restart this week old discussion, but I'm now reviewing the patch in question and I dislike how we're coupling the event and the query command. Because of the 1st issue above. A large delay because we Has this been measured? How long is this large delay? Also, is it impossible for management to issue query-rx-filter on a reasonable rate that would also cause the same problems? IOW, how can we be sure we're fixing anything without trying it on a real use-case scenario? Play with priorities, you can make management arbitrarily slow. It's just not sane to assume any timing guarantees for tasks running on Linux. Would you mind to elaborate? I'm not sure I understand how this answers my questions. Maybe I don't understand the questions. You are asking why doesn't usual throttling sufficient? This was discussed in this thread already. That's because it would introduce a huge delay if guest changes the mac too often. People don't except that changing a mac is a thing the should do slowly. You meant shouldn't? If I got it correctly, all you want to avoid is to call qobject_from_jsonf() and monitor_protocol_event() in the mac change path, because this will slow down the guest. Did I get it? No. We use the QMP event to notify management about the mac changing. In this thread, we _wrongly_ considered to use qmp approach to delay the event for avoiding the flooding. eg: monitor_protocol_event_throttle(NIC_RX_FILTER_CHANGED, 1000); Now we have a solution (using a flag to turn on/off the notify) to avoid the flooding, only emit the event if we have no un-read event. If we want to (flag is on) emit the event, we wish the event be sent ASAP (so event_throttle isn't needed). Unfortunately this doesn't answer my question. I did understand why you're not using the event throttle API (which is because you don't want to slow down the guest, not the QMP client). My point is whether coupling the event with the query command is really justified or even if it really fixes the problem. Two points: 1. Coupling them is bad design, and will probably strike back, as we plan for a better API for events where events can be disabled 2. Can you actually show the problem does exist so that we ensure this is not premature optimization? Might be a good idea to have this in the commit log (which is to couple the event with the query command) is appropriate. We're in user-space already, many things could slow the guest down apart from the event generation. Two questions: 1. Do we know how slow (or how many packets are actually dropped) if the mac is changed too often *and* the event is always sent? We always disable interface first, then change the macaddr. But we just have patch to allow guest to change macaddr of virtio-net when the interface is running. | commit 2dcd0cce551983afe2f900125457f10bb5d980ae | Author: Jiri Pirko jpi...@redhat.com | Date: Tue Dec 11 15:33:56 2012 -0500 | | [virt] virtio_net: allow to change mac when iface is running 2. Does this solution consider what happens if the QMP client does respond timely to the event by issuing the query-rx-filter command? We assume that the QMP client (management) cares about the mac changing event, and will query the latest rx-filter state and sync to macvtap device. 1) If QMP client respond timely to the event: that's what we expected :) Won't this slow down the guest? If not, why? 2) If QMP client doesn't respond timely to the event: packets might drop. If we change mac when the interface is running, we can accept trivial packets dropping. For second condition, we need to test in real environment when libvirt finishs the work of processing events.
Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event
On Mon, 27 May 2013 09:10:11 -0400 Luiz Capitulino lcapitul...@redhat.com wrote: We use the QMP event to notify management about the mac changing. In this thread, we _wrongly_ considered to use qmp approach to delay the event for avoiding the flooding. eg: monitor_protocol_event_throttle(NIC_RX_FILTER_CHANGED, 1000); Now we have a solution (using a flag to turn on/off the notify) to avoid the flooding, only emit the event if we have no un-read event. If we want to (flag is on) emit the event, we wish the event be sent ASAP (so event_throttle isn't needed). Unfortunately this doesn't answer my question. I did understand why you're not using the event throttle API (which is because you don't want to slow down the guest, not the QMP client). My point is whether coupling the event with the query command is really justified or even if it really fixes the problem. Two points: 1. Coupling them is bad design, and will probably strike back, as we plan for a better API for events where events can be disabled I meant we may in the future, for example, introduce the ability to disable commands (and events). One could argue that the event w/o a query command is not that useful, as events can be lost. But loosing an event is one thing, not having it because it got disabled by a side effect is another. But anyway, my main point in this thread is to make sure we at least justify having this coupling. Aren't we optimizing prematurely? Aren't we optimizing for a corner case? That's what I want to see answered.
Re: [Qemu-devel] [PATCH V3 4/4] block: dump snapshot and image info to specified output
On Sat, 25 May 2013 11:09:45 +0800
Wenchao Xia xiaw...@linux.vnet.ibm.com wrote:
bdrv_snapshot_dump() and bdrv_image_info_dump() do not dump to a buffer now,
some internal buffers are still used for format control, which have no
chance to be truncated. As a result, these two functions have no more issue
of truncation, and they can be used by both qemu and qemu-img with correct
parameter specified.
Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
I don't like the casting and the void pointers very much, but I can't
suggest anything better:
Reviewed-by: Luiz Capitulino lcapitul...@redhat.com
---
block/qapi.c | 66 +++--
include/block/qapi.h |6 +++-
qemu-img.c |9 ---
savevm.c |7 +++--
4 files changed, 49 insertions(+), 39 deletions(-)
diff --git a/block/qapi.c b/block/qapi.c
index 155e77e..794dbf8 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -259,7 +259,8 @@ static char *get_human_readable_size(char *buf, int
buf_size, int64_t size)
return buf;
}
-char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn)
+void bdrv_snapshot_dump(fprintf_function func_fprintf, void *f,
+QEMUSnapshotInfo *sn)
{
char buf1[128], date_buf[128], clock_buf[128];
struct tm tm;
@@ -267,9 +268,9 @@ char *bdrv_snapshot_dump(char *buf, int buf_size,
QEMUSnapshotInfo *sn)
int64_t secs;
if (!sn) {
-snprintf(buf, buf_size,
- %-10s%-20s%7s%20s%15s,
- ID, TAG, VM SIZE, DATE, VM CLOCK);
+func_fprintf(f,
+ %-10s%-20s%7s%20s%15s,
+ ID, TAG, VM SIZE, DATE, VM CLOCK);
} else {
ti = sn-date_sec;
localtime_r(ti, tm);
@@ -282,17 +283,18 @@ char *bdrv_snapshot_dump(char *buf, int buf_size,
QEMUSnapshotInfo *sn)
(int)((secs / 60) % 60),
(int)(secs % 60),
(int)((sn-vm_clock_nsec / 100) % 1000));
-snprintf(buf, buf_size,
- %-10s%-20s%7s%20s%15s,
- sn-id_str, sn-name,
- get_human_readable_size(buf1, sizeof(buf1),
sn-vm_state_size),
- date_buf,
- clock_buf);
+func_fprintf(f,
+ %-10s%-20s%7s%20s%15s,
+ sn-id_str, sn-name,
+ get_human_readable_size(buf1, sizeof(buf1),
+ sn-vm_state_size),
+ date_buf,
+ clock_buf);
}
-return buf;
}
-void bdrv_image_info_dump(ImageInfo *info)
+void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
+ ImageInfo *info)
{
char size_buf[128], dsize_buf[128];
if (!info-has_actual_size) {
@@ -302,43 +304,46 @@ void bdrv_image_info_dump(ImageInfo *info)
info-actual_size);
}
get_human_readable_size(size_buf, sizeof(size_buf), info-virtual_size);
-printf(image: %s\n
- file format: %s\n
- virtual size: %s (% PRId64 bytes)\n
- disk size: %s\n,
- info-filename, info-format, size_buf,
- info-virtual_size,
- dsize_buf);
+func_fprintf(f,
+ image: %s\n
+ file format: %s\n
+ virtual size: %s (% PRId64 bytes)\n
+ disk size: %s\n,
+ info-filename, info-format, size_buf,
+ info-virtual_size,
+ dsize_buf);
if (info-has_encrypted info-encrypted) {
-printf(encrypted: yes\n);
+func_fprintf(f, encrypted: yes\n);
}
if (info-has_cluster_size) {
-printf(cluster_size: % PRId64 \n, info-cluster_size);
+func_fprintf(f, cluster_size: % PRId64 \n,
+ info-cluster_size);
}
if (info-has_dirty_flag info-dirty_flag) {
-printf(cleanly shut down: no\n);
+func_fprintf(f, cleanly shut down: no\n);
}
if (info-has_backing_filename) {
-printf(backing file: %s, info-backing_filename);
+func_fprintf(f, backing file: %s, info-backing_filename);
if (info-has_full_backing_filename) {
-printf( (actual path: %s), info-full_backing_filename);
+func_fprintf(f, (actual path: %s),
info-full_backing_filename);
}
-putchar('\n');
+func_fprintf(f, \n);
if (info-has_backing_filename_format) {
-printf(backing file format: %s\n,
info-backing_filename_format);
+func_fprintf(f, backing file format: %s\n,
+ info-backing_filename_format);
}
}
if (info-has_snapshots) {
SnapshotInfoList *elem;
-char buf[256
Re: [Qemu-devel] [PATCH v2] monitor: work around delayed CHR_EVENT_OPENED events
On Sun, 26 May 2013 10:33:39 -0500
Michael Roth mdr...@linux.vnet.ibm.com wrote:
In the past, CHR_EVENT_OPENED events were emitted via a pre-expired
QEMUTimer. Due to timers being processing at the tail end of each main
loop iteration, this generally meant that such events would be emitted
within the same main loop iteration, prior any client data being read
by tcp/unix socket server backends.
With 9f939df955a4152aad69a19a77e0898631bb2c18, this was switched to
a bottom-half that is registered via g_idle_add(). This makes it
likely that the event won't be sent until a subsequent iteration, and
also adds the possibility that such events will race with the
processing of client data.
In cases where we rely on the CHR_EVENT_OPENED event being delivered
prior to any client data being read, this may cause unexpected behavior.
In the case of a QMP monitor session using a socket backend, the delayed
processing of the CHR_EVENT_OPENED event can lead to a situation where
a previous session, where 'qmp_capabilities' has already processed, can
cause the rejection of 'qmp_capabilities' for a subsequent session,
since we reset capabilities in response to CHR_EVENT_OPENED, which may
not yet have been delivered. This can be reproduced with the following
command, generally within 50 or so iterations:
mdroth@loki:~$ cat cmds
{'execute':'qmp_capabilities'}
{'execute':'query-cpus'}
mdroth@loki:~$ while true; do if socat stdio unix-connect:/tmp/qmp.sock
cmds | grep CommandNotFound /dev/null; then echo failed; break; else
echo ok; fi; done;
ok
ok
failed
mdroth@loki:~$
As a work-around, we reset capabilities as part of the CHR_EVENT_CLOSED
hook, which gets called as part of the GIOChannel cb associated with the
client rather than a bottom-half, and is thus guaranteed to be delivered
prior to accepting any subsequent client sessions.
This does not address the more general problem of whether or not there
are chardev users that rely on CHR_EVENT_OPENED being delivered prior to
client data, and whether or not we should consider moving CHR_EVENT_OPENED
in-band with connection establishment as a general solution, but fixes
QMP for the time being.
Reported-by: Stefan Priebe s.pri...@profihost.ag
Cc: qemu-sta...@nongnu.org
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
Thanks for debugging this Michael. I'm going to apply your patch to the qmp
branch because we can't let this broken (esp. in -stable) but this is papering
over the real bug...
---
v1-v2:
* remove command_mode reset from CHR_EVENT_OPENED case, since this
might still cause a race
monitor.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/monitor.c b/monitor.c
index 6ce2a4e..f1953a0 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4649,7 +4649,6 @@ static void monitor_control_event(void *opaque, int
event)
switch (event) {
case CHR_EVENT_OPENED:
-mon-mc-command_mode = 0;
data = get_qmp_greeting();
monitor_json_emitter(mon, data);
qobject_decref(data);
@@ -4660,6 +4659,7 @@ static void monitor_control_event(void *opaque, int
event)
json_message_parser_init(mon-mc-parser, handle_qmp_command);
mon_refcount--;
monitor_fdsets_cleanup();
+mon-mc-command_mode = 0;
break;
}
}
Re: [Qemu-devel] [PATCH V3 4/4] block: dump snapshot and image info to specified output
On Mon, 27 May 2013 17:40:59 +0200 Kevin Wolf kw...@redhat.com wrote: Am 27.05.2013 um 17:02 hat Luiz Capitulino geschrieben: On Sat, 25 May 2013 11:09:45 +0800 Wenchao Xia xiaw...@linux.vnet.ibm.com wrote: bdrv_snapshot_dump() and bdrv_image_info_dump() do not dump to a buffer now, some internal buffers are still used for format control, which have no chance to be truncated. As a result, these two functions have no more issue of truncation, and they can be used by both qemu and qemu-img with correct parameter specified. Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com I don't like the casting and the void pointers very much, but I can't suggest anything better: Maybe introduce and use a different function pointer type that explicitly takes void* instead of FILE*. You'd still have to cast the function pointers (and now actually in all instances), but then at least nobody can accidentally misinterpret a Monitor* as FILE*. I'm not sure this helps much because we'd have two functions pointers to choose (which fails the purpose of having the function pointer IMO) and also, there's code in QEMU doing this cast/void dance already. If we want a good solution, we'd have to find a better way to print to the monitor and to standard output.
Re: [Qemu-devel] [Bug 1180970] *** affects all x86_64 soft emulation
On Fri, 24 May 2013 23:23:02 +0200
Laszlo Ersek ler...@redhat.com wrote:
--[ proposed fix ]--
diff --git a/target-i386/translate.c b/target-i386/translate.c
index 0e0356f..4fbd6c0 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -4813,7 +4813,11 @@ static target_ulong disas_insn(CPUX86State *env,
DisasContext *s,
/* 0x66 is ignored if rex.w is set */
dflag = 2;
}
-if (!(prefixes PREFIX_ADR)) {
+if (prefixes PREFIX_ADR) {
+/* flip it back, 0x67 should have no effect */
+aflag ^= 1;
+}
+else {
aflag = 2;
}
}
--[ proposed fix ]--
I'll post it separately to the list for review.
Luiz, can you please test it with Windows guests?
On Windows 8 I can get past the boot loop point and even see Windows' boot
logo, but then I get a black screen (which I guess is the evolution of the
blue screen) asking me to reboot the PC saying Error Code: 0x005D.
That error code is what I get with Windows 2008, with or without or patch.
I googled a bit about it, and it seems to be related to some CPU
incompatibility, which makes me think that this is a difference issue
(meaning that your patch does fix the boot loop bug).
Re: [Qemu-devel] [PATCH v2 1/2] net: introduce MAC_TABLE_CHANGED event
On Tue, 28 May 2013 06:43:04 +0800 Amos Kong ak...@redhat.com wrote: On Mon, May 27, 2013 at 09:24:28AM -0400, Luiz Capitulino wrote: On Mon, 27 May 2013 09:10:11 -0400 Luiz Capitulino lcapitul...@redhat.com wrote: We use the QMP event to notify management about the mac changing. In this thread, we _wrongly_ considered to use qmp approach to delay the event for avoiding the flooding. eg: monitor_protocol_event_throttle(NIC_RX_FILTER_CHANGED, 1000); Now we have a solution (using a flag to turn on/off the notify) to avoid the flooding, only emit the event if we have no un-read event. If we want to (flag is on) emit the event, we wish the event be sent ASAP (so event_throttle isn't needed). Unfortunately this doesn't answer my question. I did understand why you're not using the event throttle API (which is because you don't want to slow down the guest, not the QMP client). My point is whether coupling the event with the query command is really justified or even if it really fixes the problem. Two points: 1. Coupling them is bad design, and will probably strike back, as we plan for a better API for events where events can be disabled I meant we may in the future, for example, introduce the ability to disable commands (and events). One could argue that the event w/o a query command is not that useful, as events can be lost. But loosing an event is one thing, not having it because it got disabled by a side effect is another. event_throttle() couples the event in QMP framework, but we use flags to disabled the event from real source (emit points/senders). If we set the evstate-rate to -1, we can ignore the events in monitor_protocol_event_queue(), but we could not control the event emitting of each emit point (each nic). But anyway, my main point in this thread is to make sure we at least justify having this coupling. Aren't we optimizing prematurely? Aren't we optimizing for a corner case? That's what I want to see answered. If it's a corner case, we don't need a general API to disable event. If it's a corner case, it's really worth to fix it? I think that what we need a real world test-case to show us we're doing the right thing. We can disable this event by a flag, and introduce a new API if we have same request from other events. 2. Can you actually show the problem does exist so that we ensure this is not premature optimization? Might be a good idea to have this in the commit log (which is to couple the event with the query command) is appropriate. We're in user-space already, many things could slow the guest down apart from the event generation. Two questions: 1. Do we know how slow (or how many packets are actually dropped) if the mac is changed too often *and* the event is always sent? We always disable interface first, then change the macaddr. But we just have patch to allow guest to change macaddr of virtio-net when the interface is running. | commit 2dcd0cce551983afe2f900125457f10bb5d980ae | Author: Jiri Pirko jpi...@redhat.com | Date: Tue Dec 11 15:33:56 2012 -0500 | | [virt] virtio_net: allow to change mac when iface is running 2. Does this solution consider what happens if the QMP client does respond timely to the event by issuing the query-rx-filter command? We assume that the QMP client (management) cares about the mac changing event, and will query the latest rx-filter state and sync to macvtap device. 1) If QMP client respond timely to the event: that's what we expected :) Won't this slow down the guest? If not, why? If guest changes fx-filter configs frequently management always query the event very timely, this will slow down the guest. We should detect process the abnormal behavior from management. That's not abnormal. Management is doing what it should do. Maybe using the event throttle API can solve the mngt side of the problem, but I still think we need a reproducible test-case to ensure we're doing the right thing. Management (qmp client) always respond timely to the event in the begining. If guest changes rx-filter very frequently continuous. Then we increase the evstate-rate, even disable the event. In the normal usecase, we should consider packet losing first (caused by event delay + the delay is used by management to execute the change) --- btw, currently we could not test in real environment. If related libvirt work finishes, we can evaluate with real delays, packet losing, etc. The worst condition is we could not accept the delay(packet losing), we need to consider other solution for mac programming of macvtap. 2) If QMP client doesn't respond timely to the event: packets might drop. If we change mac
[Qemu-devel] [PATCH] walk_pml4e(): fix abort on bad PML4E/PDPTE/PDE/PTE addresses
The code used to walk IA-32e page-tables, and possibly PAE page-tables,
uses the bit mask ~0xfff to get the next PML4E/PDPTE/PDE/PTE address.
However, as we use a uint64_t to store the resulting address, that mask
gets expanded to 0xf000 which not only ends up selecting
reserved bits but also selects the XD bit (execute-disable) which
happens to be enabled by Windows 8, causing qemu_get_ram_ptr() to abort.
This commit fixes that problem by replacing ~0xfff by a correct mask
that only selects the address bit range (ie. bits 51:12).
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
PS: I (obviously) don't any more core dumps with this patch applied, but
I couldn't check if the Windows dump is correct (does anyone know
how to do this?). I did quickly check on Linux though.
target-i386/arch_memory_mapping.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/target-i386/arch_memory_mapping.c
b/target-i386/arch_memory_mapping.c
index 844893f..24884bd 100644
--- a/target-i386/arch_memory_mapping.c
+++ b/target-i386/arch_memory_mapping.c
@@ -75,6 +75,8 @@ static void walk_pte2(MemoryMappingList *list,
}
/* PAE Paging or IA-32e Paging */
+#define PLM4_ADDR_MASK 0xff000 /* selects bits 51:12 */
+
static void walk_pde(MemoryMappingList *list, hwaddr pde_start_addr,
int32_t a20_mask, target_ulong start_line_addr)
{
@@ -105,7 +107,7 @@ static void walk_pde(MemoryMappingList *list, hwaddr
pde_start_addr,
continue;
}
-pte_start_addr = (pde ~0xfff) a20_mask;
+pte_start_addr = (pde PLM4_ADDR_MASK) a20_mask;
walk_pte(list, pte_start_addr, a20_mask, line_addr);
}
}
@@ -208,7 +210,7 @@ static void walk_pdpe(MemoryMappingList *list,
continue;
}
-pde_start_addr = (pdpe ~0xfff) a20_mask;
+pde_start_addr = (pdpe PLM4_ADDR_MASK) a20_mask;
walk_pde(list, pde_start_addr, a20_mask, line_addr);
}
}
@@ -231,7 +233,7 @@ static void walk_pml4e(MemoryMappingList *list,
}
line_addr = ((i 0x1ffULL) 39) | (0xULL 48);
-pdpe_start_addr = (pml4e ~0xfff) a20_mask;
+pdpe_start_addr = (pml4e PLM4_ADDR_MASK) a20_mask;
walk_pdpe(list, pdpe_start_addr, a20_mask, line_addr);
}
}
@@ -249,7 +251,7 @@ int cpu_get_memory_mapping(MemoryMappingList *list,
CPUArchState *env)
if (env-hflags HF_LMA_MASK) {
hwaddr pml4e_addr;
-pml4e_addr = (env-cr[3] ~0xfff) env-a20_mask;
+pml4e_addr = (env-cr[3] PLM4_ADDR_MASK) env-a20_mask;
walk_pml4e(list, pml4e_addr, env-a20_mask);
} else
#endif
--
1.8.1.4
Re: [Qemu-devel] [PATCH 7/7] monitor: QMP/HMP support for retrieving VNVRAM details
On Thu, 23 May 2013 13:44:47 -0400
Corey Bryant cor...@linux.vnet.ibm.com wrote:
Signed-off-by: Corey Bryant cor...@linux.vnet.ibm.com
Looks good to me, only one small nit below.
---
hmp.c| 32
hmp.h|1 +
monitor.c|7 +
qapi-schema.json | 47 +++
qmp-commands.hx | 41 +++
vnvram.c | 71
++
6 files changed, 199 insertions(+), 0 deletions(-)
diff --git a/hmp.c b/hmp.c
index 4fb76ec..a144f73 100644
--- a/hmp.c
+++ b/hmp.c
@@ -653,6 +653,38 @@ void hmp_info_tpm(Monitor *mon, const QDict *qdict)
qapi_free_TPMInfoList(info_list);
}
+void hmp_info_vnvram(Monitor *mon, const QDict *dict)
+{
+VNVRAMInfoList *info_list, *info;
+Error *err = NULL;
+unsigned int c = 0;
+
+info_list = qmp_query_vnvram(err);
+if (err) {
+monitor_printf(mon, VNVRAM not found\n);
+error_free(err);
+return;
+}
+
+for (info = info_list; info; info = info-next) {
+VNVRAMInfo *ni = info-value;
+VNVRAMEntryInfoList *einfo_list = ni-entries, *einfo;
+unsigned int d = 0;
+monitor_printf(mon, vnvram%u: drive-id=%s
+ virtual-disk-size=%PRId64 vnvram-size=%PRIu64\n,
+ c++, ni-drive_id, ni-virtual_disk_size,
+ ni-vnvram_size);
+
+for (einfo = einfo_list; einfo; einfo = einfo-next) {
+VNVRAMEntryInfo *nei = einfo-value;
+monitor_printf(mon, entry%u: name=%s cur-size=%PRIu64
+ max-size=%PRIu64\n,
+ d++, nei-name, nei-cur_size, nei-max_size);
+}
+}
+qapi_free_VNVRAMInfoList(info_list);
+}
+
void hmp_quit(Monitor *mon, const QDict *qdict)
{
monitor_suspend(mon);
diff --git a/hmp.h b/hmp.h
index 95fe76e..e26daf2 100644
--- a/hmp.h
+++ b/hmp.h
@@ -37,6 +37,7 @@ void hmp_info_balloon(Monitor *mon, const QDict *qdict);
void hmp_info_pci(Monitor *mon, const QDict *qdict);
void hmp_info_block_jobs(Monitor *mon, const QDict *qdict);
void hmp_info_tpm(Monitor *mon, const QDict *qdict);
+void hmp_info_vnvram(Monitor *mon, const QDict *dict);
void hmp_quit(Monitor *mon, const QDict *qdict);
void hmp_stop(Monitor *mon, const QDict *qdict);
void hmp_system_reset(Monitor *mon, const QDict *qdict);
diff --git a/monitor.c b/monitor.c
index 62aaebe..c10fe15 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2764,6 +2764,13 @@ static mon_cmd_t info_cmds[] = {
.mhandler.cmd = hmp_info_tpm,
},
{
+.name = vnvram,
+.args_type = ,
+.params = ,
+.help = show VNVRAM information,
+.mhandler.cmd = hmp_info_vnvram,
+},
+{
.name = NULL,
},
};
diff --git a/qapi-schema.json b/qapi-schema.json
index 9302e7d..73d42d6 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3619,3 +3619,50 @@
'*cpuid-input-ecx': 'int',
'cpuid-register': 'X86CPURegister32',
'features': 'int' } }
+
+# @VNVRAMEntryInfo:
+#
+# Information about an entry in the VNVRAM.
+#
+# @name: name of the entry
+#
+# @cur-size: current size of the entry's blob in bytes
It's preferable not to abbreviate, you can have current-size.
+#
+# @max-size: max size of the entry's blob in bytes
+#
+# Since: 1.6
+#
+##
+{ 'type': 'VNVRAMEntryInfo',
+ 'data': {'name': 'str', 'cur-size': 'int', 'max-size': 'int', } }
+
+##
+# @VNVRAMInfo:
+#
+# Information about the VNVRAM device.
+#
+# @drive-id: ID of the VNVRAM (and associated drive)
+#
+# @virtual-disk-size: Virtual size of the associated disk drive in bytes
+#
+# @vnvram-size: Size of the VNVRAM in bytes
+#
+# @entries: Array of @VNVRAMEntryInfo
+#
+# Since: 1.6
+#
+##
+{ 'type': 'VNVRAMInfo',
+ 'data': {'drive-id': 'str', 'virtual-disk-size': 'int',
+ 'vnvram-size': 'int', 'entries' : ['VNVRAMEntryInfo']} }
+
+##
+# @query-vnvram:
+#
+# Return information about the VNVRAM devices.
+#
+# Returns: @VNVRAMInfo on success
+#
+# Since: 1.6
+##
+{ 'command': 'query-vnvram', 'returns': ['VNVRAMInfo'] }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index ffd130e..56a57b7 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2932,3 +2932,44 @@ Example:
- { return: {} }
EQMP
+
+{
+.name = query-vnvram,
+.args_type = ,
+.mhandler.cmd_new = qmp_marshal_input_query_vnvram,
+},
+
+SQMP
+query-vnvram
+
+
+Show VNVRAM info.
+
+Return a json-array of json-objects representing VNVRAMs. Each VNVRAM
+is described by a json-object with the following:
+
+- drive-id: ID of the VNVRAM (json-string)
+- vitual-disk-size: Virtual size of associated disk drive in
Re: [Qemu-devel] [PATCH v2] monitor: work around delayed CHR_EVENT_OPENED events
On Mon, 27 May 2013 12:59:25 -0500
mdroth mdr...@linux.vnet.ibm.com wrote:
On Mon, May 27, 2013 at 11:16:01AM -0500, Anthony Liguori wrote:
Luiz Capitulino lcapitul...@redhat.com writes:
On Sun, 26 May 2013 10:33:39 -0500
Michael Roth mdr...@linux.vnet.ibm.com wrote:
In the past, CHR_EVENT_OPENED events were emitted via a pre-expired
QEMUTimer. Due to timers being processing at the tail end of each main
loop iteration, this generally meant that such events would be emitted
within the same main loop iteration, prior any client data being read
by tcp/unix socket server backends.
With 9f939df955a4152aad69a19a77e0898631bb2c18, this was switched to
a bottom-half that is registered via g_idle_add(). This makes it
likely that the event won't be sent until a subsequent iteration, and
also adds the possibility that such events will race with the
processing of client data.
In cases where we rely on the CHR_EVENT_OPENED event being delivered
prior to any client data being read, this may cause unexpected behavior.
In the case of a QMP monitor session using a socket backend, the delayed
processing of the CHR_EVENT_OPENED event can lead to a situation where
a previous session, where 'qmp_capabilities' has already processed, can
cause the rejection of 'qmp_capabilities' for a subsequent session,
since we reset capabilities in response to CHR_EVENT_OPENED, which may
not yet have been delivered. This can be reproduced with the following
command, generally within 50 or so iterations:
mdroth@loki:~$ cat cmds
{'execute':'qmp_capabilities'}
{'execute':'query-cpus'}
mdroth@loki:~$ while true; do if socat stdio unix-connect:/tmp/qmp.sock
cmds | grep CommandNotFound /dev/null; then echo failed; break; else
echo ok; fi; done;
ok
ok
failed
mdroth@loki:~$
As a work-around, we reset capabilities as part of the CHR_EVENT_CLOSED
hook, which gets called as part of the GIOChannel cb associated with the
client rather than a bottom-half, and is thus guaranteed to be delivered
prior to accepting any subsequent client sessions.
This does not address the more general problem of whether or not there
are chardev users that rely on CHR_EVENT_OPENED being delivered prior to
client data, and whether or not we should consider moving
CHR_EVENT_OPENED
in-band with connection establishment as a general solution, but fixes
QMP for the time being.
Reported-by: Stefan Priebe s.pri...@profihost.ag
Cc: qemu-sta...@nongnu.org
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
Thanks for debugging this Michael. I'm going to apply your patch to the
qmp
branch because we can't let this broken (esp. in -stable) but this is
papering
over the real bug...
Do we really need OPENED to happen in a bottom half? Shouldn't the open
event handlers register the callback instead if they need it?
I think that's the more general fix, but wasn't sure if there were
historical reasons why we didn't do this in the first place.
Looking at it more closely it does seem like we need a general fix
sooner rather than later though, and I don't see any reason why we can't
move BH registration into the actual OPENED hooks as you suggest:
hw/usb/ccid-card-passthru.c
- currently affected? no
debug hook only
- needs OPENED BH? no
hw/usb/redirect.c
- currently affected? yes
can_read handler checks for dev-parser != NULL, which may be
true if CLOSED BH has been executed yet. In the past, OPENED
quiesced outstanding CLOSED events prior to us reading client data.
- need OPENED BH? possibly
seems to only be needed by CLOSED events, which can be issued by
USBDevice, so we do teardown/usb_detach in BH. For OPENED, this
may not apply. if we do issue a BH, we'd need to modify can_read
handler to avoid race.
hw/usb/dev-serial.c
- currently affected? no
can_read checks for dev.attached != NULL. set to NULL
synchronously in CLOSED, will not accept reads until OPENED gets
called and sets dev.attached
- need opened BH? no
hw/char/sclpconsole.c
- currently affected? no
guarded by can_read handler
- need opened BH? no
hw/char/ipoctal232.c
- currently affected? no
debug hook only
- need opened BH? no
hw/char/virtio-console.c
- currently affected? no
OPENED/CLOSED map to vq events handled asynchronously. can_read
checks for guest_connected state rather than anything set by OPENED
- need opened BH? no
qtest.c
- currently affected? yes
can_read handler doesn't check for qtest_opened == true, can read
data before OPENED event is processed
- need opened BH? no
monitor.c
- current affected? yes
negotiated session state can temporarilly leak into a new session
- need opened BH? no
gdbstub.c
- currently affected? yes
can fail
Re: [Qemu-devel] [PATCH] qapi: pad GenericList value fields to 64 bits
On Sun, 26 May 2013 22:20:58 -0500
Michael Roth mdr...@linux.vnet.ibm.com wrote:
With the introduction of native list types, we now have types such as
int64List where the 'value' field is not a pointer, but the actual
64-bit value.
On 32-bit architectures, this can lead to situations where 'next' field
offset in GenericList does not correspond to the 'next' field in the
types that we cast to GenericList when using the visit_next_list()
interface, causing issues when we attempt to traverse linked list
structures of these types.
To fix this, pad the 'value' field of GenericList and other
schema-defined/native *List types out to 64-bits.
This is less memory-efficient for 32-bit architectures, but allows us to
continue to rely on list-handling interfaces that target GenericList to
simply visitor implementations.
In the future we can improve efficiency by defaulting to using native C
array backends to handle list of non-pointer types, which would be more
memory efficient in itself and allow us to roll back this change.
I'm also concerned with the small complexity this change is adding.
How hard would it be to do the proper solution with arrays instead?
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
---
include/qapi/visitor.h |5 -
scripts/qapi-types.py | 10 --
tests/test-qmp-output-visitor.c |5 -
3 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 1fef18c..28c21d8 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -18,7 +18,10 @@
typedef struct GenericList
{
-void *value;
+union {
+void *value;
+uint64_t padding;
+};
struct GenericList *next;
} GenericList;
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index fd42d71..ddcfed9 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -22,7 +22,10 @@ def generate_fwd_struct(name, members, builtin_type=False):
typedef struct %(name)sList
{
-%(type)s value;
+union {
+%(type)s value;
+uint64_t padding;
+};
struct %(name)sList *next;
} %(name)sList;
''',
@@ -35,7 +38,10 @@ typedef struct %(name)s %(name)s;
typedef struct %(name)sList
{
-%(name)s *value;
+union {
+%(name)s *value;
+uint64_t padding;
+};
struct %(name)sList *next;
} %(name)sList;
''',
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 0942a41..b2fa9a7 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -295,7 +295,10 @@ static void
test_visitor_out_struct_errors(TestOutputVisitorData *data,
typedef struct TestStructList
{
-TestStruct *value;
+union {
+TestStruct *value;
+uint64_t padding;
+};
struct TestStructList *next;
} TestStructList;
Re: [Qemu-devel] [PATCH 16/16] Make qemu-io commands available in the monitor
On Wed, 29 May 2013 10:13:42 +0200
Kevin Wolf kw...@redhat.com wrote:
Am 28.05.2013 um 18:07 hat Eric Blake geschrieben:
On 05/28/2013 09:27 AM, Kevin Wolf wrote:
The QMP version is flagged with a __org.qemu.debug- prefix in order to
reinforce the statement that qemu-io is for testing and debugging only,
with no API guarantees.
Correct use of naming conventions.
Hmm, I wonder if the recent addition of an 'abort' action to
'transaction' should be renamed __org.qemu.debug-abort, to make it
obvious that it is another case of a QMP command useful mainly for
testing, and not real-life use.
Makes sense to me.
But first I'd like to get Luiz's ack for this, because I think I'm the
first one to use an __org.qemu prefix, and I'm the first one trying to
introduce a QMP command without API stability.
I think that should be fine. However, it's not a perfect match for QMP
as you don't expect mngt to use it anytime soon and that the command's
syntax is not QMP friendly:
{ execute: __org.qemu.debug-qemu-io-command, arguments:
{ device: ide0-hd0, command: write -P 0x12 4M 512k } }
What about adding a HMP-only command (the good old way) and use it
through human-monitor-command?
IMO, this matches better your current use-case and the API instability
of the command.
Re: [Qemu-devel] [PATCH] qapi: pad GenericList value fields to 64 bits
On Wed, 29 May 2013 13:12:18 -0500
mdroth mdr...@linux.vnet.ibm.com wrote:
On Wed, May 29, 2013 at 01:32:52PM -0400, Luiz Capitulino wrote:
On Sun, 26 May 2013 22:20:58 -0500
Michael Roth mdr...@linux.vnet.ibm.com wrote:
With the introduction of native list types, we now have types such as
int64List where the 'value' field is not a pointer, but the actual
64-bit value.
On 32-bit architectures, this can lead to situations where 'next' field
offset in GenericList does not correspond to the 'next' field in the
types that we cast to GenericList when using the visit_next_list()
interface, causing issues when we attempt to traverse linked list
structures of these types.
To fix this, pad the 'value' field of GenericList and other
schema-defined/native *List types out to 64-bits.
This is less memory-efficient for 32-bit architectures, but allows us to
continue to rely on list-handling interfaces that target GenericList to
simply visitor implementations.
In the future we can improve efficiency by defaulting to using native C
array backends to handle list of non-pointer types, which would be more
memory efficient in itself and allow us to roll back this change.
I'm also concerned with the small complexity this change is adding.
How hard would it be to do the proper solution with arrays instead?
It's not *too* bad, we'd need patches 9-11 from here:
http://lists.gnu.org/archive/html/qemu-devel/2012-10/threads.html#05755
Along with code generation bits, and then all the unit test stuff.
I think we should be able to get it in for 1.6, but I'd rather not leave
32-bit busted in the meantime.
Ok, I've applied this patch to the QMP branch.
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
---
include/qapi/visitor.h |5 -
scripts/qapi-types.py | 10 --
tests/test-qmp-output-visitor.c |5 -
3 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 1fef18c..28c21d8 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -18,7 +18,10 @@
typedef struct GenericList
{
-void *value;
+union {
+void *value;
+uint64_t padding;
+};
struct GenericList *next;
} GenericList;
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index fd42d71..ddcfed9 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -22,7 +22,10 @@ def generate_fwd_struct(name, members,
builtin_type=False):
typedef struct %(name)sList
{
-%(type)s value;
+union {
+%(type)s value;
+uint64_t padding;
+};
struct %(name)sList *next;
} %(name)sList;
''',
@@ -35,7 +38,10 @@ typedef struct %(name)s %(name)s;
typedef struct %(name)sList
{
-%(name)s *value;
+union {
+%(name)s *value;
+uint64_t padding;
+};
struct %(name)sList *next;
} %(name)sList;
''',
diff --git a/tests/test-qmp-output-visitor.c
b/tests/test-qmp-output-visitor.c
index 0942a41..b2fa9a7 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -295,7 +295,10 @@ static void
test_visitor_out_struct_errors(TestOutputVisitorData *data,
typedef struct TestStructList
{
-TestStruct *value;
+union {
+TestStruct *value;
+uint64_t padding;
+};
struct TestStructList *next;
} TestStructList;
Re: [Qemu-devel] [PATCH] gdbstub: do not restart crashed guest
On Thu, 30 May 2013 13:55:50 +0200
Laszlo Ersek ler...@redhat.com wrote:
On 05/30/13 13:20, Paolo Bonzini wrote:
If a guest has crashed with an internal error or similar, detaching
gdb (or any other debugger action) should not restart it.
Cc: Jan Kiszka jan.kis...@siemens.com
Signed-off-by: Paolo Bonzini pbonz...@redhat.com
---
gdbstub.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/gdbstub.c b/gdbstub.c
index e80e1d3..90e54cb 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -371,7 +371,9 @@ static inline void gdb_continue(GDBState *s)
#ifdef CONFIG_USER_ONLY
s-running_state = 1;
#else
-vm_start();
+if (runstate_check(RUN_STATE_DEBUG)) {
+vm_start();
+}
#endif
}
I sought to check the gdb_continue() call sites, and uses of
RUN_STATE_DEBUG. Seems reasonable.
Reviewed-by: Laszlo Ersek ler...@redhat.com
(
FWIW I wonder why in commit ad02b96a Luiz allowed DEBUG - SUSPENDED. As
far as I understand, when the debugger is attached, the guest is not
running, hence it can't go directly to RUN_STATE_SUSPENDED. Maybe due to
a concurrent monitor command?
I honestly don't remember if I took into account the explanation you gave
below. You're right that a stopped guest can't suspend.
Technically it does seem possible; from main_loop_should_exit():
if (qemu_debug_requested()) {
vm_stop(RUN_STATE_DEBUG);
}
if (qemu_suspend_requested()) {
qemu_system_suspend();
}
Both requests could become pending during one iteration of the loop, and
the next iteration will see both of them. OK.
)
Laszlo
Re: [Qemu-devel] [PATCH] walk_pml4e(): fix abort on bad PML4E/PDPTE/PDE/PTE addresses
On Tue, 28 May 2013 14:19:22 -0400
Luiz Capitulino lcapitul...@redhat.com wrote:
The code used to walk IA-32e page-tables, and possibly PAE page-tables,
uses the bit mask ~0xfff to get the next PML4E/PDPTE/PDE/PTE address.
However, as we use a uint64_t to store the resulting address, that mask
gets expanded to 0xf000 which not only ends up selecting
reserved bits but also selects the XD bit (execute-disable) which
happens to be enabled by Windows 8, causing qemu_get_ram_ptr() to abort.
This commit fixes that problem by replacing ~0xfff by a correct mask
that only selects the address bit range (ie. bits 51:12).
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
Ping? Wen?
Would be nice get a Reviewed-by before merging...
---
PS: I (obviously) don't any more core dumps with this patch applied, but
I couldn't check if the Windows dump is correct (does anyone know
how to do this?). I did quickly check on Linux though.
target-i386/arch_memory_mapping.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/target-i386/arch_memory_mapping.c
b/target-i386/arch_memory_mapping.c
index 844893f..24884bd 100644
--- a/target-i386/arch_memory_mapping.c
+++ b/target-i386/arch_memory_mapping.c
@@ -75,6 +75,8 @@ static void walk_pte2(MemoryMappingList *list,
}
/* PAE Paging or IA-32e Paging */
+#define PLM4_ADDR_MASK 0xff000 /* selects bits 51:12 */
+
static void walk_pde(MemoryMappingList *list, hwaddr pde_start_addr,
int32_t a20_mask, target_ulong start_line_addr)
{
@@ -105,7 +107,7 @@ static void walk_pde(MemoryMappingList *list, hwaddr
pde_start_addr,
continue;
}
-pte_start_addr = (pde ~0xfff) a20_mask;
+pte_start_addr = (pde PLM4_ADDR_MASK) a20_mask;
walk_pte(list, pte_start_addr, a20_mask, line_addr);
}
}
@@ -208,7 +210,7 @@ static void walk_pdpe(MemoryMappingList *list,
continue;
}
-pde_start_addr = (pdpe ~0xfff) a20_mask;
+pde_start_addr = (pdpe PLM4_ADDR_MASK) a20_mask;
walk_pde(list, pde_start_addr, a20_mask, line_addr);
}
}
@@ -231,7 +233,7 @@ static void walk_pml4e(MemoryMappingList *list,
}
line_addr = ((i 0x1ffULL) 39) | (0xULL 48);
-pdpe_start_addr = (pml4e ~0xfff) a20_mask;
+pdpe_start_addr = (pml4e PLM4_ADDR_MASK) a20_mask;
walk_pdpe(list, pdpe_start_addr, a20_mask, line_addr);
}
}
@@ -249,7 +251,7 @@ int cpu_get_memory_mapping(MemoryMappingList *list,
CPUArchState *env)
if (env-hflags HF_LMA_MASK) {
hwaddr pml4e_addr;
-pml4e_addr = (env-cr[3] ~0xfff) env-a20_mask;
+pml4e_addr = (env-cr[3] PLM4_ADDR_MASK) env-a20_mask;
walk_pml4e(list, pml4e_addr, env-a20_mask);
} else
#endif
Re: [Qemu-devel] [PATCH] walk_pml4e(): fix abort on bad PML4E/PDPTE/PDE/PTE addresses
On Thu, 30 May 2013 15:16:18 +0200
Laszlo Ersek ler...@redhat.com wrote:
On 05/30/13 14:59, Luiz Capitulino wrote:
On Tue, 28 May 2013 14:19:22 -0400
Luiz Capitulino lcapitul...@redhat.com wrote:
The code used to walk IA-32e page-tables, and possibly PAE page-tables,
uses the bit mask ~0xfff to get the next PML4E/PDPTE/PDE/PTE address.
However, as we use a uint64_t to store the resulting address, that mask
gets expanded to 0xf000 which not only ends up selecting
reserved bits but also selects the XD bit (execute-disable) which
happens to be enabled by Windows 8, causing qemu_get_ram_ptr() to abort.
This commit fixes that problem by replacing ~0xfff by a correct mask
that only selects the address bit range (ie. bits 51:12).
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
Ping? Wen?
Would be nice get a Reviewed-by before merging...
I didn't miss your submission and did find it OK, I just felt unsure
about stating so, because simple patches like this are prime territory
to burn someone's R-b's worth (ie. to expose a reviewer's lack of
information / experience). But hey, what can I lose? The patch does look
good to me, so
Thank you Laszlo! It's also new territory for me, that's why I'm asking
for reviews (otherwise I'd just sneak it in some pull request :-)
Reviewed-by: Laszlo Ersek ler...@redhat.com
---
PS: I (obviously) don't any more core dumps with this patch applied, but
I couldn't check if the Windows dump is correct (does anyone know
how to do this?). I did quickly check on Linux though.
target-i386/arch_memory_mapping.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/target-i386/arch_memory_mapping.c
b/target-i386/arch_memory_mapping.c
index 844893f..24884bd 100644
--- a/target-i386/arch_memory_mapping.c
+++ b/target-i386/arch_memory_mapping.c
@@ -75,6 +75,8 @@ static void walk_pte2(MemoryMappingList *list,
}
/* PAE Paging or IA-32e Paging */
+#define PLM4_ADDR_MASK 0xff000 /* selects bits 51:12 */
+
static void walk_pde(MemoryMappingList *list, hwaddr pde_start_addr,
int32_t a20_mask, target_ulong start_line_addr)
{
@@ -105,7 +107,7 @@ static void walk_pde(MemoryMappingList *list, hwaddr
pde_start_addr,
continue;
}
-pte_start_addr = (pde ~0xfff) a20_mask;
+pte_start_addr = (pde PLM4_ADDR_MASK) a20_mask;
walk_pte(list, pte_start_addr, a20_mask, line_addr);
}
}
@@ -208,7 +210,7 @@ static void walk_pdpe(MemoryMappingList *list,
continue;
}
-pde_start_addr = (pdpe ~0xfff) a20_mask;
+pde_start_addr = (pdpe PLM4_ADDR_MASK) a20_mask;
walk_pde(list, pde_start_addr, a20_mask, line_addr);
}
}
@@ -231,7 +233,7 @@ static void walk_pml4e(MemoryMappingList *list,
}
line_addr = ((i 0x1ffULL) 39) | (0xULL 48);
-pdpe_start_addr = (pml4e ~0xfff) a20_mask;
+pdpe_start_addr = (pml4e PLM4_ADDR_MASK) a20_mask;
walk_pdpe(list, pdpe_start_addr, a20_mask, line_addr);
}
}
@@ -249,7 +251,7 @@ int cpu_get_memory_mapping(MemoryMappingList *list,
CPUArchState *env)
if (env-hflags HF_LMA_MASK) {
hwaddr pml4e_addr;
-pml4e_addr = (env-cr[3] ~0xfff) env-a20_mask;
+pml4e_addr = (env-cr[3] PLM4_ADDR_MASK) env-a20_mask;
walk_pml4e(list, pml4e_addr, env-a20_mask);
} else
#endif
Re: [Qemu-devel] [PATCH] walk_pml4e(): fix abort on bad PML4E/PDPTE/PDE/PTE addresses
On Thu, 30 May 2013 16:10:28 +0200 Andreas Färber afaer...@suse.de wrote: Am 30.05.2013 15:16, schrieb Luiz Capitulino: On Thu, 30 May 2013 15:16:18 +0200 Laszlo Ersek ler...@redhat.com wrote: On 05/30/13 14:59, Luiz Capitulino wrote: On Tue, 28 May 2013 14:19:22 -0400 Luiz Capitulino lcapitul...@redhat.com wrote: The code used to walk IA-32e page-tables, and possibly PAE page-tables, uses the bit mask ~0xfff to get the next PML4E/PDPTE/PDE/PTE address. However, as we use a uint64_t to store the resulting address, that mask gets expanded to 0xf000 which not only ends up selecting reserved bits but also selects the XD bit (execute-disable) which happens to be enabled by Windows 8, causing qemu_get_ram_ptr() to abort. This commit fixes that problem by replacing ~0xfff by a correct mask that only selects the address bit range (ie. bits 51:12). Signed-off-by: Luiz Capitulino lcapitul...@redhat.com Ping? Wen? Would be nice get a Reviewed-by before merging... I didn't miss your submission and did find it OK, I just felt unsure about stating so, because simple patches like this are prime territory to burn someone's R-b's worth (ie. to expose a reviewer's lack of information / experience). But hey, what can I lose? The patch does look good to me, so Thank you Laszlo! It's also new territory for me, that's why I'm asking for reviews (otherwise I'd just sneak it in some pull request :-) Luiz, you aware aware that I have another fix by Nuohan queued that seemed orthogonal? Yes, they should be orthogonal. If someone reviews my refactoring series (which resent that patch) I would like to send out a PULL for that rather soon, since it blocks further CPU work. I would then include your fix as well to avoid merge conflicts. Thanks, although I was going to include it in my tomorrow's QMP pull request. Will this disturb your work?
Re: [Qemu-devel] [PATCH] walk_pml4e(): fix abort on bad PML4E/PDPTE/PDE/PTE addresses
On Thu, 30 May 2013 16:22:32 +0200 Andreas Färber afaer...@suse.de wrote: Am 30.05.2013 16:14, schrieb Luiz Capitulino: On Thu, 30 May 2013 16:10:28 +0200 Andreas Färber afaer...@suse.de wrote: Am 30.05.2013 15:16, schrieb Luiz Capitulino: On Thu, 30 May 2013 15:16:18 +0200 Laszlo Ersek ler...@redhat.com wrote: On 05/30/13 14:59, Luiz Capitulino wrote: On Tue, 28 May 2013 14:19:22 -0400 Luiz Capitulino lcapitul...@redhat.com wrote: The code used to walk IA-32e page-tables, and possibly PAE page-tables, uses the bit mask ~0xfff to get the next PML4E/PDPTE/PDE/PTE address. However, as we use a uint64_t to store the resulting address, that mask gets expanded to 0xf000 which not only ends up selecting reserved bits but also selects the XD bit (execute-disable) which happens to be enabled by Windows 8, causing qemu_get_ram_ptr() to abort. This commit fixes that problem by replacing ~0xfff by a correct mask that only selects the address bit range (ie. bits 51:12). Signed-off-by: Luiz Capitulino lcapitul...@redhat.com Ping? Wen? Would be nice get a Reviewed-by before merging... I didn't miss your submission and did find it OK, I just felt unsure about stating so, because simple patches like this are prime territory to burn someone's R-b's worth (ie. to expose a reviewer's lack of information / experience). But hey, what can I lose? The patch does look good to me, so Thank you Laszlo! It's also new territory for me, that's why I'm asking for reviews (otherwise I'd just sneak it in some pull request :-) Luiz, you aware aware that I have another fix by Nuohan queued that seemed orthogonal? Yes, they should be orthogonal. If someone reviews my refactoring series (which resent that patch) I would like to send out a PULL for that rather soon, since it blocks further CPU work. I would then include your fix as well to avoid merge conflicts. Thanks, although I was going to include it in my tomorrow's QMP pull request. Will this disturb your work? I hope not - could you then please pick up Nuohan's bugfix as well? Sure. Still I'd be interested in your review - and I have one more patch to add that I created offline yesterday wrt first paging-enabled CPU. You can CC me.
Re: [Qemu-devel] [PATCH v2 0/2] block: fix spurious DEVICE_TRAY_MOVED events on shutdown
On Wed, 29 May 2013 18:18:17 +0200 Pavel Hrdina phrd...@redhat.com wrote: This fixes a regression introduced by commit 9ca111544. The first commit is done by Luiz and I've just use it as it is. The second commit moves the bdrv_dev_change_media_cb() into eject_device(), called by QMP and HMP eject command, and into qmp_bdrv_open_encrypted(), called by QMP and HMP change command. These are the only place where I think that should call the bdrv_dev_change_media_cb() function. There is no reason to call this function while we are removing the device from the guest, for example while closing and deleting all devices on shutdown. I'll defer review to the block guys, but as this works for me: Tested-by: Luiz Capitulino lcapitul...@redhat.com Luiz Capitulino (1): block: make bdrv_dev_change_media_cb() public Pavel Hrdina (1): block: move the bdrv_dev_change_media_cb() block.c | 11 +-- blockdev.c| 5 + include/block/block.h | 1 + 3 files changed, 7 insertions(+), 10 deletions(-)
[Qemu-devel] [BUG]: kvm_set_phys_mem: error unregistering overlapping slot: Invalid argument
The culprit is commit: commit 235e8982ad393e5611cb892df54881c872eea9e1 Author: Jordan Justen jordan.l.jus...@intel.com Date: Wed May 29 01:27:26 2013 -0700 kvm: support using KVM_MEM_READONLY flag for regions I'm running 3.9.2-200.fc18, btw. And, error checking is missing on the first call to kvm_vm_ioctl().
Re: [Qemu-devel] [BUG]: kvm_set_phys_mem: error unregistering overlapping slot: Invalid argument
On Thu, 30 May 2013 18:03:04 +0200 Paolo Bonzini pbonz...@redhat.com wrote: Il 30/05/2013 17:46, Luiz Capitulino ha scritto: The culprit is commit: commit 235e8982ad393e5611cb892df54881c872eea9e1 Author: Jordan Justen jordan.l.jus...@intel.com Date: Wed May 29 01:27:26 2013 -0700 kvm: support using KVM_MEM_READONLY flag for regions I'm running 3.9.2-200.fc18, btw. And, error checking is missing on the first call to kvm_vm_ioctl(). Reproducer? I just try to start a VM (HEAD 87d23f7): ~/work/virt/ sudo ./qemu-qmp -drive file=disks/test.img,if=virtio,cache=none,aio=native -enable-kvm -m 1G -monitor stdio -cpu host -snapshot QEMU 1.5.50 monitor - type 'help' for more information (qemu) kvm_set_phys_mem: error unregistering overlapping slot: Invalid argument ~/work/virt/
Re: [Qemu-devel] [BUG]: kvm_set_phys_mem: error unregistering overlapping slot: Invalid argument
On Thu, 30 May 2013 09:50:10 -0700 Jordan Justen jljus...@gmail.com wrote: On Thu, May 30, 2013 at 9:08 AM, Luiz Capitulino lcapitul...@redhat.com wrote: On Thu, 30 May 2013 18:03:04 +0200 Paolo Bonzini pbonz...@redhat.com wrote: Il 30/05/2013 17:46, Luiz Capitulino ha scritto: The culprit is commit: commit 235e8982ad393e5611cb892df54881c872eea9e1 Author: Jordan Justen jordan.l.jus...@intel.com Date: Wed May 29 01:27:26 2013 -0700 kvm: support using KVM_MEM_READONLY flag for regions I'm running 3.9.2-200.fc18, btw. And, error checking is missing on the first call to kvm_vm_ioctl(). As noted in the code, the first call is for KVM commit 75d61fbc. I'm not sure we want to fail if an error occurs when making that call. (I'm pretty sure we don't want to in fact.) Xiao, any thoughts? Reproducer? I just try to start a VM (HEAD 87d23f7): ~/work/virt/ sudo ./qemu-qmp -drive file=disks/test.img,if=virtio,cache=none,aio=native -enable-kvm -m 1G -monitor stdio -cpu host -snapshot QEMU 1.5.50 monitor - type 'help' for more information (qemu) kvm_set_phys_mem: error unregistering overlapping slot: Invalid argument ~/work/virt/ Sorry. I am working with Linux 3.8.0, and I don't see this. I'll try to update my kernel. Does the firmware behave as a ROM for you? I think so: (qemu) info roms fw=genroms/kvmvapic.bin size=0x002400 name=kvmvapic.bin addr=fffe size=0x02 mem=rom name=bios.bin (qemu) Is this what you're asking?
Re: [Qemu-devel] [BUG]: kvm_set_phys_mem: error unregistering overlapping slot: Invalid argument
On Thu, 30 May 2013 10:32:36 -0700 Jordan Justen jljus...@gmail.com wrote: On Thu, May 30, 2013 at 10:03 AM, Luiz Capitulino lcapitul...@redhat.com wrote: On Thu, 30 May 2013 09:50:10 -0700 Jordan Justen jljus...@gmail.com wrote: On Thu, May 30, 2013 at 9:08 AM, Luiz Capitulino lcapitul...@redhat.com wrote: On Thu, 30 May 2013 18:03:04 +0200 Paolo Bonzini pbonz...@redhat.com wrote: Il 30/05/2013 17:46, Luiz Capitulino ha scritto: The culprit is commit: commit 235e8982ad393e5611cb892df54881c872eea9e1 Author: Jordan Justen jordan.l.jus...@intel.com Date: Wed May 29 01:27:26 2013 -0700 kvm: support using KVM_MEM_READONLY flag for regions I'm running 3.9.2-200.fc18, btw. And, error checking is missing on the first call to kvm_vm_ioctl(). As noted in the code, the first call is for KVM commit 75d61fbc. I'm not sure we want to fail if an error occurs when making that call. (I'm pretty sure we don't want to in fact.) Xiao, any thoughts? Reproducer? I just try to start a VM (HEAD 87d23f7): ~/work/virt/ sudo ./qemu-qmp -drive file=disks/test.img,if=virtio,cache=none,aio=native -enable-kvm -m 1G -monitor stdio -cpu host -snapshot QEMU 1.5.50 monitor - type 'help' for more information (qemu) kvm_set_phys_mem: error unregistering overlapping slot: Invalid argument ~/work/virt/ Sorry. I am working with Linux 3.8.0, and I don't see this. I'll try to update my kernel. Does the firmware behave as a ROM for you? I think so: (qemu) info roms fw=genroms/kvmvapic.bin size=0x002400 name=kvmvapic.bin addr=fffe size=0x02 mem=rom name=bios.bin (qemu) Is this what you're asking? I guess I was meaning ... if you write to an address such as 0xfff0, does it update as RAM, or does it retain the original value? This is easy to test in OVMF at the EFI shell, but I'm not sure how you could easily test it otherwise. I could try to hack something, but maybe not today. Does the system actually boot for you after the error message? No, I get an abort. That's what kvm_set_phys_mem() does when kvm_set_user_memory_region() fails: (gdb) bt #0 0x7f01f8592ba5 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:63 #1 0x7f01f8594358 in __GI_abort () at abort.c:90 #2 0x7f01fedb57f1 in kvm_set_phys_mem (section=0x7f01ee6975d0, add=false) at /home/lcapitulino/work/src/upstream/qmp-unstable/kvm-all.c:692 #3 0x7f01fedb5bfa in kvm_region_del (listener=0x7f01ff2bc360 kvm_memory_listener, section=0x7f01ee6975d0) at /home/lcapitulino/work/src/upstream/qmp-unstable/kvm-all.c:795 #4 0x7f01fedbc92d in address_space_update_topology_pass (as=0x7f01ffadfa60 address_space_memory, old_view=..., new_view=..., adding=false) at /home/lcapitulino/work/src/upstream/qmp-unstable/memory.c:689 #5 0x7f01fedbd125 in address_space_update_topology (as=0x7f01ffadfa60 address_space_memory) at /home/lcapitulino/work/src/upstream/qmp-unstable/memory.c:725 #6 0x7f01fedbd29c in memory_region_transaction_commit () at /home/lcapitulino/work/src/upstream/qmp-unstable/memory.c:750 #7 0x7f01fec1b977 in i440fx_update_memory_mappings (d=0x7f0200511940) at /home/lcapitulino/work/src/upstream/qmp-unstable/hw/pci-host/piix.c:131 #8 0x7f01fec1bab3 in i440fx_write_config (dev=0x7f0200511940, address=94, val=51, len=1) at /home/lcapitulino/work/src/upstream/qmp-unstable/hw/pci-host/piix.c:154 #9 0x7f01fec29a9a in pci_host_config_write_common (pci_dev=0x7f0200511940, addr=94, limit=256, val=51, len=1) at /home/lcapitulino/work/src/upstream/qmp-unstable/hw/pci/pci_host.c:54 #10 0x7f01fec29ba2 in pci_data_write (s=0x7f0200502a50, addr=2147483742, val=51, len=1) at /home/lcapitulino/work/src/upstream/qmp-unstable/hw/pci/pci_host.c:75 #11 0x7f01fec29d5e in pci_host_data_write (opaque=0x7f02004fa1c0, addr=2, val=51, len=1) at /home/lcapitulino/work/src/upstream/qmp-unstable/hw/pci/pci_host.c:128 #12 0x7f01fedbac6e in memory_region_write_accessor (opaque=0x7f02004fc598, addr=2, value=0x7f01ee697a70, size=1, shift=0, mask=255) at /home/lcapitulino/work/src/upstream/qmp-unstable/memory.c:334 #13 0x7f01fedbad50 in access_with_adjusted_size (addr=2, value=0x7f01ee697a70, size=1, access_size_min=1, access_size_max=4, access=0x7f01fedbabe9 memory_region_write_accessor, opaque=0x7f02004fc598) at /home/lcapitulino/work/src/upstream/qmp-unstable/memory.c:364 #14 0x7f01fedbb1b8 in memory_region_iorange_write (iorange=0x7f0200503550, offset=2, width=1, data=51) at /home/lcapitulino/work/src/upstream/qmp-unstable/memory.c:439 #15 0x7f01fedb33aa in ioport_writeb_thunk (opaque=0x7f0200503550, addr=3326, data=51) at /home/lcapitulino/work/src/upstream/qmp-unstable/ioport.c:212 #16 0x7f01fedb2d84 in ioport_write (index=0, address=3326, data=51) at /home/lcapitulino/work/src/upstream/qmp-unstable/ioport.c:83
Re: [Qemu-devel] [BUG]: kvm_set_phys_mem: error unregistering overlapping slot: Invalid argument
On Thu, 30 May 2013 20:05:29 +0200 Paolo Bonzini pbonz...@redhat.com wrote: Il 30/05/2013 19:56, Luiz Capitulino ha scritto: On Thu, 30 May 2013 10:32:36 -0700 Jordan Justen jljus...@gmail.com wrote: On Thu, May 30, 2013 at 10:03 AM, Luiz Capitulino lcapitul...@redhat.com wrote: On Thu, 30 May 2013 09:50:10 -0700 Jordan Justen jljus...@gmail.com wrote: On Thu, May 30, 2013 at 9:08 AM, Luiz Capitulino lcapitul...@redhat.com wrote: On Thu, 30 May 2013 18:03:04 +0200 Paolo Bonzini pbonz...@redhat.com wrote: Il 30/05/2013 17:46, Luiz Capitulino ha scritto: The culprit is commit: commit 235e8982ad393e5611cb892df54881c872eea9e1 Author: Jordan Justen jordan.l.jus...@intel.com Date: Wed May 29 01:27:26 2013 -0700 kvm: support using KVM_MEM_READONLY flag for regions I'm running 3.9.2-200.fc18, btw. And, error checking is missing on the first call to kvm_vm_ioctl(). As noted in the code, the first call is for KVM commit 75d61fbc. I'm not sure we want to fail if an error occurs when making that call. (I'm pretty sure we don't want to in fact.) Xiao, any thoughts? Reproducer? I just try to start a VM (HEAD 87d23f7): ~/work/virt/ sudo ./qemu-qmp -drive file=disks/test.img,if=virtio,cache=none,aio=native -enable-kvm -m 1G -monitor stdio -cpu host -snapshot QEMU 1.5.50 monitor - type 'help' for more information (qemu) kvm_set_phys_mem: error unregistering overlapping slot: Invalid argument ~/work/virt/ Sorry. I am working with Linux 3.8.0, and I don't see this. I'll try to update my kernel. Does the firmware behave as a ROM for you? I think so: (qemu) info roms fw=genroms/kvmvapic.bin size=0x002400 name=kvmvapic.bin addr=fffe size=0x02 mem=rom name=bios.bin (qemu) Is this what you're asking? I guess I was meaning ... if you write to an address such as 0xfff0, does it update as RAM, or does it retain the original value? This is easy to test in OVMF at the EFI shell, but I'm not sure how you could easily test it otherwise. I could try to hack something, but maybe not today. Just put a breakpoint on pflash_cfi01_register and see if it is reached. Reached on a regular boot right? It's not reached, with or without the offending commit. I cannot reproduce it, but I'm also on 3.8.x. Will look at it tomorrow. Paolo Does the system actually boot for you after the error message? No, I get an abort. That's what kvm_set_phys_mem() does when kvm_set_user_memory_region() fails: (gdb) bt #0 0x7f01f8592ba5 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:63 #1 0x7f01f8594358 in __GI_abort () at abort.c:90 #2 0x7f01fedb57f1 in kvm_set_phys_mem (section=0x7f01ee6975d0, add=false) at /home/lcapitulino/work/src/upstream/qmp-unstable/kvm-all.c:692 #3 0x7f01fedb5bfa in kvm_region_del (listener=0x7f01ff2bc360 kvm_memory_listener, section=0x7f01ee6975d0) at /home/lcapitulino/work/src/upstream/qmp-unstable/kvm-all.c:795 #4 0x7f01fedbc92d in address_space_update_topology_pass (as=0x7f01ffadfa60 address_space_memory, old_view=..., new_view=..., adding=false) at /home/lcapitulino/work/src/upstream/qmp-unstable/memory.c:689 #5 0x7f01fedbd125 in address_space_update_topology (as=0x7f01ffadfa60 address_space_memory) at /home/lcapitulino/work/src/upstream/qmp-unstable/memory.c:725 #6 0x7f01fedbd29c in memory_region_transaction_commit () at /home/lcapitulino/work/src/upstream/qmp-unstable/memory.c:750 #7 0x7f01fec1b977 in i440fx_update_memory_mappings (d=0x7f0200511940) at /home/lcapitulino/work/src/upstream/qmp-unstable/hw/pci-host/piix.c:131 #8 0x7f01fec1bab3 in i440fx_write_config (dev=0x7f0200511940, address=94, val=51, len=1) at /home/lcapitulino/work/src/upstream/qmp-unstable/hw/pci-host/piix.c:154 #9 0x7f01fec29a9a in pci_host_config_write_common (pci_dev=0x7f0200511940, addr=94, limit=256, val=51, len=1) at /home/lcapitulino/work/src/upstream/qmp-unstable/hw/pci/pci_host.c:54 #10 0x7f01fec29ba2 in pci_data_write (s=0x7f0200502a50, addr=2147483742, val=51, len=1) at /home/lcapitulino/work/src/upstream/qmp-unstable/hw/pci/pci_host.c:75 #11 0x7f01fec29d5e in pci_host_data_write (opaque=0x7f02004fa1c0, addr=2, val=51, len=1) at /home/lcapitulino/work/src/upstream/qmp-unstable/hw/pci/pci_host.c:128 #12 0x7f01fedbac6e in memory_region_write_accessor (opaque=0x7f02004fc598, addr=2, value=0x7f01ee697a70, size=1, shift=0, mask=255) at /home/lcapitulino/work/src/upstream/qmp-unstable/memory.c:334 #13 0x7f01fedbad50 in access_with_adjusted_size (addr=2, value=0x7f01ee697a70, size=1, access_size_min=1, access_size_max=4, access=0x7f01fedbabe9 memory_region_write_accessor, opaque=0x7f02004fc598) at /home/lcapitulino/work/src/upstream/qmp-unstable/memory.c:364 #14
Re: [Qemu-devel] [PATCH uq/master] fix double free the memslot in kvm_set_phys_mem
On Fri, 31 May 2013 16:52:18 +0800
Xiao Guangrong xiaoguangr...@linux.vnet.ibm.com wrote:
Luiz Capitulino reported that guest refused to boot and qemu
complained with:
kvm_set_phys_mem: error unregistering overlapping slot: Invalid argument
It is caused by commit 235e8982ad that did double free for the memslot
so that the second one raises the -EINVAL error
Fix it by reset memory size only if it is needed
Reported-by: Luiz Capitulino lcapitul...@redhat.com
Signed-off-by: Xiao Guangrong xiaoguangr...@linux.vnet.ibm.com
Tested-by: Luiz Capitulino lcapitul...@redhat.com
Thanks Xiao for the fix, and thanks everyone for debugging this issue!
---
kvm-all.c |3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/kvm-all.c b/kvm-all.c
index 8e7bbf8..405480e 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -206,7 +206,8 @@ static int kvm_set_user_memory_region(KVMState *s,
KVMSlot *slot)
if (s-migration_log) {
mem.flags |= KVM_MEM_LOG_DIRTY_PAGES;
}
-if (mem.flags KVM_MEM_READONLY) {
+
+if (slot-memory_size mem.flags KVM_MEM_READONLY) {
/* Set the slot size to 0 before setting the slot to the desired
* value. This is needed based on KVM commit 75d61fbc. */
mem.memory_size = 0;
Re: [Qemu-devel] [PATCH qom-cpu v3 4/9] cpu: Turn cpu_paging_enabled() into a CPUState hook
On Thu, 30 May 2013 17:07:56 +0200
Andreas Färber afaer...@suse.de wrote:
Signed-off-by: Andreas Färber afaer...@suse.de
Nitpick alarm on.
---
include/qom/cpu.h | 10 ++
include/sysemu/memory_mapping.h | 1 -
memory_mapping-stub.c | 6 --
memory_mapping.c | 2 +-
qom/cpu.c | 13 +
target-i386/arch_memory_mapping.c | 6 +-
target-i386/cpu.c | 11 +--
7 files changed, 34 insertions(+), 15 deletions(-)
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 7cd9442..cf5fec2 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -48,6 +48,7 @@ typedef struct CPUState CPUState;
* @reset: Callback to reset the #CPUState to its initial state.
* @do_interrupt: Callback for interrupt handling.
* @get_arch_id: Callback for getting architecture-dependent CPU ID.
+ * @get_paging_enabled: Callback for inquiring whether paging is enabled.
* @vmsd: State description for migration.
*
* Represents a CPU family or model.
@@ -62,6 +63,7 @@ typedef struct CPUClass {
void (*reset)(CPUState *cpu);
void (*do_interrupt)(CPUState *cpu);
int64_t (*get_arch_id)(CPUState *cpu);
+bool (*get_paging_enabled)(CPUState *cpu);
Argument could be const?
const struct VMStateDescription *vmsd;
int (*write_elf64_note)(WriteCoreDumpFunction f, CPUState *cpu,
@@ -138,6 +140,14 @@ struct CPUState {
};
/**
+ * cpu_paging_enabled:
+ * @cpu: The CPU whose state is to be inspected.
+ *
+ * Returns: %true if paging is enabled, %false otherwise.
+ */
+bool cpu_paging_enabled(CPUState *cpu);
+
+/**
* cpu_write_elf64_note:
* @f: pointer to a function that writes memory to a file
* @cpu: The CPU whose memory is to be dumped
diff --git a/include/sysemu/memory_mapping.h b/include/sysemu/memory_mapping.h
index 1256125..6f01524 100644
--- a/include/sysemu/memory_mapping.h
+++ b/include/sysemu/memory_mapping.h
@@ -31,7 +31,6 @@ typedef struct MemoryMappingList {
} MemoryMappingList;
int cpu_get_memory_mapping(MemoryMappingList *list, CPUArchState *env);
-bool cpu_paging_enabled(CPUArchState *env);
/*
* add or merge the memory region [phys_addr, phys_addr + length) into the
diff --git a/memory_mapping-stub.c b/memory_mapping-stub.c
index 24d5d67..6c0dfeb 100644
--- a/memory_mapping-stub.c
+++ b/memory_mapping-stub.c
@@ -25,9 +25,3 @@ int cpu_get_memory_mapping(MemoryMappingList *list,
{
return -1;
}
-
-bool cpu_paging_enabled(CPUArchState *env)
-{
-return true;
-}
-
diff --git a/memory_mapping.c b/memory_mapping.c
index ff45b3a..0790aac 100644
--- a/memory_mapping.c
+++ b/memory_mapping.c
@@ -170,7 +170,7 @@ static CPUArchState *find_paging_enabled_cpu(CPUArchState
*start_cpu)
CPUArchState *env;
for (env = start_cpu; env != NULL; env = env-next_cpu) {
-if (cpu_paging_enabled(env)) {
+if (cpu_paging_enabled(ENV_GET_CPU(env))) {
return env;
}
}
diff --git a/qom/cpu.c b/qom/cpu.c
index 04aefbb..ea7e676 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -50,6 +50,18 @@ bool cpu_exists(int64_t id)
return data.found;
}
+bool cpu_paging_enabled(CPUState *cpu)
+{
+CPUClass *cc = CPU_GET_CLASS(cpu);
+
+return cc-get_paging_enabled(cpu);
+}
+
+static bool cpu_common_get_paging_enabled(CPUState *cpu)
+{
+return true;
+}
Not sure if this is important, but I wonder if we want to do this
I mean, for all cases where you want to know if paging is enabled, what
will happen if this default method says yes, it's enabled but it
actually isn't?
+
/* CPU hot-plug notifiers */
static NotifierList cpu_added_notifiers =
NOTIFIER_LIST_INITIALIZER(cpu_add_notifiers);
@@ -176,6 +188,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
k-class_by_name = cpu_common_class_by_name;
k-reset = cpu_common_reset;
k-get_arch_id = cpu_common_get_arch_id;
+k-get_paging_enabled = cpu_common_get_paging_enabled;
k-write_elf32_qemunote = cpu_common_write_elf32_qemunote;
k-write_elf32_note = cpu_common_write_elf32_note;
k-write_elf64_qemunote = cpu_common_write_elf64_qemunote;
diff --git a/target-i386/arch_memory_mapping.c
b/target-i386/arch_memory_mapping.c
index 5096fbd..c5a10ec 100644
--- a/target-i386/arch_memory_mapping.c
+++ b/target-i386/arch_memory_mapping.c
@@ -241,7 +241,7 @@ static void walk_pml4e(MemoryMappingList *list,
int cpu_get_memory_mapping(MemoryMappingList *list, CPUArchState *env)
{
-if (!cpu_paging_enabled(env)) {
+if (!cpu_paging_enabled(ENV_GET_CPU(env))) {
/* paging is disabled */
return 0;
}
@@ -273,7 +273,3 @@ int cpu_get_memory_mapping(MemoryMappingList *list,
CPUArchState *env)
return 0;
}
-bool cpu_paging_enabled(CPUArchState *env)
-{
-return env-cr[0]
Re: [Qemu-devel] [PATCH qom-cpu v3 6/9] cpu: Turn cpu_get_memory_mapping() into a CPUState hook
On Thu, 30 May 2013 17:07:58 +0200
Andreas Färber afaer...@suse.de wrote:
Signed-off-by: Andreas Färber afaer...@suse.de
---
include/qom/cpu.h | 11 +++
include/sysemu/memory_mapping.h | 2 --
memory_mapping-stub.c | 6 --
memory_mapping.c | 2 +-
qom/cpu.c | 14 ++
target-i386/arch_memory_mapping.c | 7 +--
target-i386/cpu-qom.h | 2 ++
target-i386/cpu.c | 1 +
8 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index cf5fec2..93a4612 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -23,6 +23,7 @@
#include signal.h
#include hw/qdev-core.h
#include qemu/thread.h
+#include qemu/typedefs.h
typedef int (*WriteCoreDumpFunction)(void *buf, size_t size, void *opaque);
@@ -49,6 +50,7 @@ typedef struct CPUState CPUState;
* @do_interrupt: Callback for interrupt handling.
* @get_arch_id: Callback for getting architecture-dependent CPU ID.
* @get_paging_enabled: Callback for inquiring whether paging is enabled.
+ * @get_memory_mapping: Callback for obtaining the memory mappings.
* @vmsd: State description for migration.
*
* Represents a CPU family or model.
@@ -64,6 +66,7 @@ typedef struct CPUClass {
void (*do_interrupt)(CPUState *cpu);
int64_t (*get_arch_id)(CPUState *cpu);
bool (*get_paging_enabled)(CPUState *cpu);
+int (*get_memory_mapping)(CPUState *cpu, MemoryMappingList *list);
Would be nice to take an Error argument and fill it properly when
get_memory_mapping() is not implemented.
const struct VMStateDescription *vmsd;
int (*write_elf64_note)(WriteCoreDumpFunction f, CPUState *cpu,
@@ -148,6 +151,14 @@ struct CPUState {
bool cpu_paging_enabled(CPUState *cpu);
/**
+ * @cpu: The CPU whose memory mappings are to be obtained.
+ * @list: Where to write the memory mappings to.
+ *
+ * Returns: 0 if successful.
+ */
+int cpu_get_memory_mapping(CPUState *cpu, MemoryMappingList *list);
+
+/**
* cpu_write_elf64_note:
* @f: pointer to a function that writes memory to a file
* @cpu: The CPU whose memory is to be dumped
diff --git a/include/sysemu/memory_mapping.h b/include/sysemu/memory_mapping.h
index 1f71c32..c47e6ee 100644
--- a/include/sysemu/memory_mapping.h
+++ b/include/sysemu/memory_mapping.h
@@ -31,8 +31,6 @@ struct MemoryMappingList {
QTAILQ_HEAD(, MemoryMapping) head;
};
-int cpu_get_memory_mapping(MemoryMappingList *list, CPUArchState *env);
-
/*
* add or merge the memory region [phys_addr, phys_addr + length) into the
* memory mapping's list. The region's virtual address starts with virt_addr,
diff --git a/memory_mapping-stub.c b/memory_mapping-stub.c
index 6c0dfeb..989dc00 100644
--- a/memory_mapping-stub.c
+++ b/memory_mapping-stub.c
@@ -19,9 +19,3 @@ int qemu_get_guest_memory_mapping(MemoryMappingList *list)
{
return -2;
}
-
-int cpu_get_memory_mapping(MemoryMappingList *list,
-
CPUArchState *env)
-{
-return -1;
-}
diff --git a/memory_mapping.c b/memory_mapping.c
index 0790aac..481530a 100644
--- a/memory_mapping.c
+++ b/memory_mapping.c
@@ -188,7 +188,7 @@ int qemu_get_guest_memory_mapping(MemoryMappingList *list)
first_paging_enabled_cpu = find_paging_enabled_cpu(first_cpu);
if (first_paging_enabled_cpu) {
for (env = first_paging_enabled_cpu; env != NULL; env =
env-next_cpu) {
-ret = cpu_get_memory_mapping(list, env);
+ret = cpu_get_memory_mapping(ENV_GET_CPU(env), list);
if (ret 0) {
return -1;
}
diff --git a/qom/cpu.c b/qom/cpu.c
index ea7e676..e7e1c25 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -62,6 +62,19 @@ static bool cpu_common_get_paging_enabled(CPUState *cpu)
return true;
}
+int cpu_get_memory_mapping(CPUState *cpu, MemoryMappingList *list)
+{
+CPUClass *cc = CPU_GET_CLASS(cpu);
+
+return cc-get_memory_mapping(cpu, list);
+}
+
+static int cpu_common_get_memory_mapping(CPUState *cpu,
+ MemoryMappingList *list)
+{
+return -1;
+}
+
/* CPU hot-plug notifiers */
static NotifierList cpu_added_notifiers =
NOTIFIER_LIST_INITIALIZER(cpu_add_notifiers);
@@ -189,6 +202,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
k-reset = cpu_common_reset;
k-get_arch_id = cpu_common_get_arch_id;
k-get_paging_enabled = cpu_common_get_paging_enabled;
+k-get_memory_mapping = cpu_common_get_memory_mapping;
k-write_elf32_qemunote = cpu_common_write_elf32_qemunote;
k-write_elf32_note = cpu_common_write_elf32_note;
k-write_elf64_qemunote = cpu_common_write_elf64_qemunote;
diff --git a/target-i386/arch_memory_mapping.c
Re: [Qemu-devel] [PATCH qom-cpu v3 9/9] memory_mapping: Change qemu_get_guest_memory_mapping() semantics
On Thu, 30 May 2013 17:08:01 +0200
Andreas Färber afaer...@suse.de wrote:
Previously it would search for the first CPU with paging enabled and
retrieve memory mappings from this and any following CPUs or return an
error if that fails.
Instead walk all CPUs and if paging is enabled retrieve the memory
mappings. Fail only if retrieving memory mappings on a CPU with paging
enabled fails.
This not only allows to more easily use one qemu_for_each_cpu() instead
of open-coding two CPU loops and drops find_first_paging_enabled_cpu()
but removes the implicit assumption of heterogeneity between CPUs n..N
in having paging enabled.
Cc: Wen Congyang we...@cn.fujitsu.com
Signed-off-by: Andreas Färber afaer...@suse.de
---
memory_mapping.c | 42 +++---
1 file changed, 23 insertions(+), 19 deletions(-)
diff --git a/memory_mapping.c b/memory_mapping.c
index 481530a..55ac2b8 100644
--- a/memory_mapping.c
+++ b/memory_mapping.c
@@ -165,35 +165,39 @@ void memory_mapping_list_init(MemoryMappingList *list)
QTAILQ_INIT(list-head);
}
-static CPUArchState *find_paging_enabled_cpu(CPUArchState *start_cpu)
+typedef struct GetGuestMemoryMappingData {
+MemoryMappingList *list;
+int ret;
+} GetGuestMemoryMappingData;
+
+static void qemu_get_one_guest_memory_mapping(CPUState *cpu, void *data)
{
-CPUArchState *env;
+GetGuestMemoryMappingData *s = data;
+int ret;
-for (env = start_cpu; env != NULL; env = env-next_cpu) {
-if (cpu_paging_enabled(ENV_GET_CPU(env))) {
-return env;
-}
+if (!cpu_paging_enabled(cpu) || s-ret == -1) {
+return;
+}
+ret = cpu_get_memory_mapping(cpu, s-list);
+if (ret 0) {
+s-ret = -1;
+} else {
+s-ret = 0;
}
Does cpu_get_memory_mapping() ever return a positive value or a negative
value != -1 ?
It it doesn't, I'd do:
s-ret = cpu_get_memory_mapping();
assert(s-ret == 0 || s-ret == -1);
-
-return NULL;
}
int qemu_get_guest_memory_mapping(MemoryMappingList *list)
{
-CPUArchState *env, *first_paging_enabled_cpu;
+GetGuestMemoryMappingData s = {
+.list = list,
+.ret = -2,
+};
RAMBlock *block;
ram_addr_t offset, length;
-int ret;
-first_paging_enabled_cpu = find_paging_enabled_cpu(first_cpu);
-if (first_paging_enabled_cpu) {
-for (env = first_paging_enabled_cpu; env != NULL; env =
env-next_cpu) {
-ret = cpu_get_memory_mapping(ENV_GET_CPU(env), list);
-if (ret 0) {
-return -1;
-}
-}
-return 0;
+qemu_for_each_cpu(qemu_get_one_guest_memory_mapping, s);
+if (s.ret != -2) {
+return s.ret;
}
I see we ignore error in dump_init(). Doesn't matter today because
x86_cpu_get_memory_mapping() never fails. But as you're doing this cleanup,
shouldn't you add proper error handling?
/*
Re: [Qemu-devel] [PATCH V3 0/4] qapi and snapshot code clean up in block layer
On Fri, 31 May 2013 21:04:10 +0800 Wenchao Xia xiaw...@linux.vnet.ibm.com wrote: 于 2013-5-30 10:41, Wenchao Xia 写道: 于 2013-5-27 23:41, Kevin Wolf 写道: Am 25.05.2013 um 05:09 hat Wenchao Xia geschrieben: These patches are the common part of my hmp/qmp block query series and Pavel's qmp snapshot command converion series. It mainly does following things: 1 move snapshot related code to block/snapshot.c, qmp and info dumping code to block/qapi.c. 2 better info dumping function to get rid of buffer, avoid string truncation. Posted comments on patch 1 and 4. Patches 2 and 3 are: Reviewed-by: Kevin Wolf kw...@redhat.com It seems nothing need change, Kevin, do you think it can be merged? This serial blocks mine and Pavel's work, anything need to be improved? Respin with -typedef int (*fprintf_function)(FILE *f, const char *fmt, ...) +typedef int (*fprintf_function)(void *out, const char *fmt, ...) ? As far as my review is concerned, I'm OK with your current version.
Re: [Qemu-devel] [PATCH qom-cpu v3 0/9] dump: Build cleanups redone
On Thu, 30 May 2013 17:07:52 +0200 Andreas Färber afaer...@suse.de wrote: Hello, This series is an alternative to patches previously queued or posted, based on virgin master. As requested by Paolo, this replaces Kate's previous memory_mapping split and my follow-ups and instead goes directly for moving things to CPUState. All knowledge about dump / memory mapping are moved away from configure. v3 adds an additional patch proposing a semantic change to facilitate CPU handling and it prepends another bugfix to avoid merge conflicts. I had some minor comments, but series looks good to me. Also, Andreas, I'm going to cherry-pick mine and Qiao's fixes into QMP tree so that they're included in my today's pull request. Regards, Andreas v2 - v3: * Incorporate Luiz' x86 bugfix. * Append a patch to resolve the open-coded CPU loops. * Massage CONFIG_HAVE_* commit messages (they were previously reordered). v1 - v2: * Dropped Kate's memory_mapping split * Dropped target_ulong cleanup and replaced with typedef * Amended CPUArchState cleanups with introducing hooks in CPUClass * Drop memory_memory stubs instead of moving them Cc: Wen Congyang we...@cn.fujitsu.com Cc: Qiao Nuohan qiaonuo...@cn.fujitsu.com Cc: Jens Freimann jf...@linux.vnet.ibm.com Cc: Vincent Rabin ra...@rab.in Cc: Paolo Bonzini pbonz...@redhat.com Cc: Luiz Capitulino lcapitul...@redhat.com Andreas Färber (7): dump: Move stubs into libqemustub.a cpu: Turn cpu_paging_enabled() into a CPUState hook memory_mapping: Move MemoryMappingList typedef to qemu/typedefs.h cpu: Turn cpu_get_memory_mapping() into a CPUState hook memory_mapping: Drop qemu_get_memory_mapping() stub dump: Unconditionally compile memory_mapping: Change qemu_get_guest_memory_mapping() semantics Luiz Capitulino (1): target-i386: walk_pml4e(): fix abort on bad PML4E/PDPTE/PDE/PTE addresses Qiao Nuohan (1): target-i386: Fix mask of pte index in memory mapping Makefile.target | 8 ++-- configure | 8 hmp-commands.hx | 2 -- include/qemu/typedefs.h | 2 ++ include/qom/cpu.h | 21 include/sysemu/memory_mapping.h | 8 +++- memory_mapping-stub.c | 33 -- memory_mapping.c | 42 +-- qom/cpu.c | 27 + stubs/Makefile.objs | 1 + dump-stub.c = stubs/dump.c | 8 target-i386/arch_memory_mapping.c | 23 +++-- target-i386/cpu-qom.h | 2 ++ target-i386/cpu.c | 12 +-- 14 files changed, 103 insertions(+), 94 deletions(-) delete mode 100644 memory_mapping-stub.c rename dump-stub.c = stubs/dump.c (65%)
[Qemu-devel] [PULL 0/3] QMP queue
One qapi fix and two fixes that affect the dump-guest-memory QMP command. The changes (since 87d23f78aa79b72da022afda358bbc8a8509ca70) are available in the following repository: git://repo.or.cz/qemu/qmp-unstable.git queue/qmp Luiz Capitulino (1): target-i386: fix abort on bad PML4E/PDPTE/PDE/PTE addresses Michael Roth (1): qapi: pad GenericList value fields to 64 bits Qiao Nuohan (1): target-i386: Fix mask of pte index in memory mapping include/qapi/visitor.h| 5 - scripts/qapi-types.py | 10 -- target-i386/arch_memory_mapping.c | 12 +++- tests/test-qmp-output-visitor.c | 5 - 4 files changed, 23 insertions(+), 9 deletions(-) -- 1.8.1.4
[Qemu-devel] [PULL 1/3] qapi: pad GenericList value fields to 64 bits
From: Michael Roth mdr...@linux.vnet.ibm.com
With the introduction of native list types, we now have types such as
int64List where the 'value' field is not a pointer, but the actual
64-bit value.
On 32-bit architectures, this can lead to situations where 'next' field
offset in GenericList does not correspond to the 'next' field in the
types that we cast to GenericList when using the visit_next_list()
interface, causing issues when we attempt to traverse linked list
structures of these types.
To fix this, pad the 'value' field of GenericList and other
schema-defined/native *List types out to 64-bits.
This is less memory-efficient for 32-bit architectures, but allows us to
continue to rely on list-handling interfaces that target GenericList to
simply visitor implementations.
In the future we can improve efficiency by defaulting to using native C
array backends to handle list of non-pointer types, which would be more
memory efficient in itself and allow us to roll back this change.
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
include/qapi/visitor.h | 5 -
scripts/qapi-types.py | 10 --
tests/test-qmp-output-visitor.c | 5 -
3 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 1fef18c..28c21d8 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -18,7 +18,10 @@
typedef struct GenericList
{
-void *value;
+union {
+void *value;
+uint64_t padding;
+};
struct GenericList *next;
} GenericList;
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index fd42d71..ddcfed9 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -22,7 +22,10 @@ def generate_fwd_struct(name, members, builtin_type=False):
typedef struct %(name)sList
{
-%(type)s value;
+union {
+%(type)s value;
+uint64_t padding;
+};
struct %(name)sList *next;
} %(name)sList;
''',
@@ -35,7 +38,10 @@ typedef struct %(name)s %(name)s;
typedef struct %(name)sList
{
-%(name)s *value;
+union {
+%(name)s *value;
+uint64_t padding;
+};
struct %(name)sList *next;
} %(name)sList;
''',
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 0942a41..b2fa9a7 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -295,7 +295,10 @@ static void
test_visitor_out_struct_errors(TestOutputVisitorData *data,
typedef struct TestStructList
{
-TestStruct *value;
+union {
+TestStruct *value;
+uint64_t padding;
+};
struct TestStructList *next;
} TestStructList;
--
1.8.1.4
[Qemu-devel] [PULL 2/3] target-i386: fix abort on bad PML4E/PDPTE/PDE/PTE addresses
The code used to walk IA-32e page-tables, and possibly PAE page-tables,
uses the bit mask ~0xfff to get the next PML4E/PDPTE/PDE/PTE address.
However, as we use a uint64_t to store the resulting address, that mask
gets expanded to 0xf000 which not only ends up selecting
reserved bits but also selects the XD bit (execute-disable) which
happens to be enabled by Windows 8, causing qemu_get_ram_ptr() to abort.
This commit fixes that problem by replacing ~0xfff by a correct mask
that only selects the address bit range (ie. bits 51:12).
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
Reviewed-by: Laszlo Ersek ler...@redhat.com
---
target-i386/arch_memory_mapping.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/target-i386/arch_memory_mapping.c
b/target-i386/arch_memory_mapping.c
index 844893f..24884bd 100644
--- a/target-i386/arch_memory_mapping.c
+++ b/target-i386/arch_memory_mapping.c
@@ -75,6 +75,8 @@ static void walk_pte2(MemoryMappingList *list,
}
/* PAE Paging or IA-32e Paging */
+#define PLM4_ADDR_MASK 0xff000 /* selects bits 51:12 */
+
static void walk_pde(MemoryMappingList *list, hwaddr pde_start_addr,
int32_t a20_mask, target_ulong start_line_addr)
{
@@ -105,7 +107,7 @@ static void walk_pde(MemoryMappingList *list, hwaddr
pde_start_addr,
continue;
}
-pte_start_addr = (pde ~0xfff) a20_mask;
+pte_start_addr = (pde PLM4_ADDR_MASK) a20_mask;
walk_pte(list, pte_start_addr, a20_mask, line_addr);
}
}
@@ -208,7 +210,7 @@ static void walk_pdpe(MemoryMappingList *list,
continue;
}
-pde_start_addr = (pdpe ~0xfff) a20_mask;
+pde_start_addr = (pdpe PLM4_ADDR_MASK) a20_mask;
walk_pde(list, pde_start_addr, a20_mask, line_addr);
}
}
@@ -231,7 +233,7 @@ static void walk_pml4e(MemoryMappingList *list,
}
line_addr = ((i 0x1ffULL) 39) | (0xULL 48);
-pdpe_start_addr = (pml4e ~0xfff) a20_mask;
+pdpe_start_addr = (pml4e PLM4_ADDR_MASK) a20_mask;
walk_pdpe(list, pdpe_start_addr, a20_mask, line_addr);
}
}
@@ -249,7 +251,7 @@ int cpu_get_memory_mapping(MemoryMappingList *list,
CPUArchState *env)
if (env-hflags HF_LMA_MASK) {
hwaddr pml4e_addr;
-pml4e_addr = (env-cr[3] ~0xfff) env-a20_mask;
+pml4e_addr = (env-cr[3] PLM4_ADDR_MASK) env-a20_mask;
walk_pml4e(list, pml4e_addr, env-a20_mask);
} else
#endif
--
1.8.1.4
[Qemu-devel] [PULL 3/3] target-i386: Fix mask of pte index in memory mapping
From: Qiao Nuohan qiaonuo...@cn.fujitsu.com Function walk_pte() needs pte index to calculate virtual address. However, pte index of PAE paging or IA-32e paging is 9 bit, so the mask should be 0x1ff. Signed-off-by: Qiao Nuohan qiaonuo...@cn.fujitsu.com Reviewed-by: Jesse Larrew jlar...@linux.vnet.ibm.com Signed-off-by: Andreas Färber afaer...@suse.de Signed-off-by: Luiz Capitulino lcapitul...@redhat.com --- target-i386/arch_memory_mapping.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target-i386/arch_memory_mapping.c b/target-i386/arch_memory_mapping.c index 24884bd..5096fbd 100644 --- a/target-i386/arch_memory_mapping.c +++ b/target-i386/arch_memory_mapping.c @@ -38,7 +38,7 @@ static void walk_pte(MemoryMappingList *list, hwaddr pte_start_addr, continue; } -start_vaddr = start_line_addr | ((i 0x1fff) 12); +start_vaddr = start_line_addr | ((i 0x1ff) 12); memory_mapping_list_add_merge_sorted(list, start_paddr, start_vaddr, 1 12); } -- 1.8.1.4
Re: [Qemu-devel] [PATCH] correct RTC_CHANGE_EVENT description
On Fri, 31 May 2013 15:24:03 -0300 Marcelo Tosatti mtosa...@redhat.com wrote: Fix RTC_CHANGE event description to match implementation. Signed-off-by: Marcelo Tosatti mtosa...@redhat.com Applied to the qmp branch, thanks. diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt index 92fe5fb..00b4087 100644 --- a/QMP/qmp-events.txt +++ b/QMP/qmp-events.txt @@ -203,7 +203,8 @@ Emitted when the guest changes the RTC time. Data: -- offset: delta against the host UTC in seconds (json-number) +- offset: Offset between old RTC clock value and new one +in seconds (json-number) Example:
Re: [Qemu-devel] [RFC PATCH v3 00/11] qemu-ga: fsfreeze on Windows using VSS
On Mon, 3 Jun 2013 09:12:55 + Libaiqing libaiq...@huawei.com wrote: Hi, Thanks for your advice,it works well when using prefix=x86_64-w64-mingw32-. The dlls may download from here: http://qemu.weilnetz.de/w64/dll/. One more question: Qemu-ga.exe can't run successfully under normal account which belongs to Administration group. Qemu-ga service can start successfully under this account after Administrator installed. The output is: 1370307591.689600:critical:error opening path 1370307591.705200:critical:error opening channel 1370307591.720800:critical:failed to create guest agent channel 1370307591.736400:critical:failed to initialize guest agent channel Could you give me some advise to debug this problem ? I can provide more information if need. Maybe it's because it needs admin permissions to open the virtio device.
Re: [Qemu-devel] [PATCH 0/2] gdbstub runstate check follow-ups
On Mon, 3 Jun 2013 17:06:53 +0200 Paolo Bonzini pbonz...@redhat.com wrote: My patch committed at 87f25c12bfeaaa0c41fb857713bbc7e8a9b757dc was broken. These patches fix the problem in a better way. Looks good to me: Reviewed-by: Luiz Capitulino lcapitul...@redhat.com
[Qemu-devel] [PATCH 1/2] MAINTAINERS: new maintainers for qapi-schema.json
I'm facing two problems lately wrt QMP patch review: increasingly lack of bandwidth and lack of background in so many different areas that are getting new QMP commands almost every week. In order to help me mitigate this problem, I'm adding Eric and Markus (besides me) as maintainers of the qapi-schema.json file. Markus has been an old timer reviewer. Eric is being the most active and prolific reviewer of QMP patches for some time now. I believe Markus and Eric will keep doing their work as before, but starting now I'll require the ACK of at least one of them before appling a patch/series that touches the qapi-schema.json file. Signed-off-by: Luiz Capitulino lcapitul...@redhat.com --- MAINTAINERS | 7 +++ 1 file changed, 7 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index be02724..432185a 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -706,6 +706,13 @@ F: nbd.* F: qemu-nbd.c T: git git://github.com/bonzini/qemu.git nbd-next +QAPI Schema +M: Eric Blake ebl...@redhat.com +M: Luiz Capitulino lcapitul...@redhat.com +M: Markus Armbruster arm...@redhat.com +S: Supported +F: qapi-schema.json + SLIRP M: Jan Kiszka jan.kis...@siemens.com S: Maintained -- 1.8.1.4
[Qemu-devel] [PATCH 0/2] MAINTAINERS: QMP: entry update and new maintainers
Hi, This is an small update on QMP/HMP/QAPI mainternership. Please, check individual patches for details. Luiz Capitulino (2): MAINTAINERS: new maintainers for qapi-schema.json MAINTAINERS: split Monitor (QMP/HMP) entry MAINTAINERS | 24 ++-- 1 file changed, 22 insertions(+), 2 deletions(-) -- 1.8.1.4
[Qemu-devel] [PATCH 2/2] MAINTAINERS: split Monitor (QMP/HMP) entry
This entry doesn't reflect reality for a few years now. This commit splits it into Human Monitor (HMP), QAPI and QMP. Markus is dropped as a maintainer. This is what we have been for the last few years. Also, it's going to help me to offload some of this work to someone else in the near future. Signed-off-by: Luiz Capitulino lcapitul...@redhat.com --- MAINTAINERS | 17 +++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 432185a..45f2e68 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -685,11 +685,12 @@ M: Anthony Liguori aligu...@us.ibm.com S: Supported F: vl.c -Monitor (QMP/HMP) +Human Monitor (HMP) M: Luiz Capitulino lcapitul...@redhat.com -M: Markus Armbruster arm...@redhat.com S: Supported F: monitor.c +F: hmp.c +F: hmp-commands.hx Network device layer M: Anthony Liguori aligu...@us.ibm.com @@ -706,6 +707,11 @@ F: nbd.* F: qemu-nbd.c T: git git://github.com/bonzini/qemu.git nbd-next +QAPI +M: Luiz Capitulino lcapitul...@redhat.com +S: Supported +F: qapi/ + QAPI Schema M: Eric Blake ebl...@redhat.com M: Luiz Capitulino lcapitul...@redhat.com @@ -713,6 +719,13 @@ M: Markus Armbruster arm...@redhat.com S: Supported F: qapi-schema.json +QMP +M: Luiz Capitulino lcapitul...@redhat.com +S: Supported +F: qmp.c +F: monitor.c +F: qmp-commands.hx + SLIRP M: Jan Kiszka jan.kis...@siemens.com S: Maintained -- 1.8.1.4
Re: [Qemu-devel] [PATCH 2/2] MAINTAINERS: split Monitor (QMP/HMP) entry
On Mon, 03 Jun 2013 11:24:58 -0600 Eric Blake ebl...@redhat.com wrote: On 06/03/2013 11:17 AM, Luiz Capitulino wrote: This entry doesn't reflect reality for a few years now. This commit splits it into Human Monitor (HMP), QAPI and QMP. Markus is dropped as a maintainer. This is what we have been for the last few years. Also, it's going to help me to offload some of this work to someone else in the near future. Signed-off-by: Luiz Capitulino lcapitul...@redhat.com --- MAINTAINERS | 17 +++-- 1 file changed, 15 insertions(+), 2 deletions(-) QAPI Schema M: Eric Blake ebl...@redhat.com M: Luiz Capitulino lcapitul...@redhat.com @@ -713,6 +719,13 @@ M: Markus Armbruster arm...@redhat.com S: Supported F: qapi-schema.json +QMP +M: Luiz Capitulino lcapitul...@redhat.com +S: Supported +F: qmp.c +F: monitor.c +F: qmp-commands.hx Do we want QMP/* lumped into this category? For that matter, should QMP/qmp-events.txt be included as part of QAPI Schema? Yes, for both questions. I've fixed it for v2, but I didn't include QMP/qmp-events.txt in the QAPI Schema entry cause it would be a bit confusing. We should move the events to the schema file soon, anyway.
Re: [Qemu-devel] [PATCH 0/2] MAINTAINERS: QMP: entry update and new maintainers
On Mon, 03 Jun 2013 11:27:51 -0600 Eric Blake ebl...@redhat.com wrote: On 06/03/2013 11:17 AM, Luiz Capitulino wrote: Hi, This is an small update on QMP/HMP/QAPI mainternership. Please, check individual patches for details. Luiz Capitulino (2): MAINTAINERS: new maintainers for qapi-schema.json MAINTAINERS: split Monitor (QMP/HMP) entry Series: Reviewed-by: Eric Blake ebl...@redhat.com But see questions about whether 2/2 missed some files. Thanks. Also, be aware that I'm on vacation June 6-26, so I won't be able to do much with my increased responsibility until after that point. Enjoy! :)
Re: [Qemu-devel] [PATCH 2/2] MAINTAINERS: split Monitor (QMP/HMP) entry
On Mon, 03 Jun 2013 21:13:30 +0200 Paolo Bonzini pbonz...@redhat.com wrote: Il 03/06/2013 19:17, Luiz Capitulino ha scritto: This entry doesn't reflect reality for a few years now. This commit splits it into Human Monitor (HMP), QAPI and QMP. Markus is dropped as a maintainer. This is what we have been for the last few years. Also, it's going to help me to offload some of this work to someone else in the near future. Signed-off-by: Luiz Capitulino lcapitul...@redhat.com --- MAINTAINERS | 17 +++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 432185a..45f2e68 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -685,11 +685,12 @@ M: Anthony Liguori aligu...@us.ibm.com S: Supported F: vl.c -Monitor (QMP/HMP) +Human Monitor (HMP) M: Luiz Capitulino lcapitul...@redhat.com -M: Markus Armbruster arm...@redhat.com S: Supported F: monitor.c +F: hmp.c +F: hmp-commands.hx Network device layer M: Anthony Liguori aligu...@us.ibm.com @@ -706,6 +707,11 @@ F: nbd.* F: qemu-nbd.c T: git git://github.com/bonzini/qemu.git nbd-next +QAPI +M: Luiz Capitulino lcapitul...@redhat.com Mike Roth too, perhaps? I'd like to, but last time I asked he sort of refused :) Michael, can I add you?
Re: [Qemu-devel] [PATCH 2/2] MAINTAINERS: split Monitor (QMP/HMP) entry
On Mon, 3 Jun 2013 15:05:03 -0500 mdroth mdr...@linux.vnet.ibm.com wrote: On Mon, Jun 03, 2013 at 03:54:48PM -0400, Luiz Capitulino wrote: On Mon, 03 Jun 2013 21:13:30 +0200 Paolo Bonzini pbonz...@redhat.com wrote: Il 03/06/2013 19:17, Luiz Capitulino ha scritto: This entry doesn't reflect reality for a few years now. This commit splits it into Human Monitor (HMP), QAPI and QMP. Markus is dropped as a maintainer. This is what we have been for the last few years. Also, it's going to help me to offload some of this work to someone else in the near future. Signed-off-by: Luiz Capitulino lcapitul...@redhat.com --- MAINTAINERS | 17 +++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 432185a..45f2e68 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -685,11 +685,12 @@ M: Anthony Liguori aligu...@us.ibm.com S: Supported F: vl.c -Monitor (QMP/HMP) +Human Monitor (HMP) M: Luiz Capitulino lcapitul...@redhat.com -M: Markus Armbruster arm...@redhat.com S: Supported F: monitor.c +F: hmp.c +F: hmp-commands.hx Network device layer M: Anthony Liguori aligu...@us.ibm.com @@ -706,6 +707,11 @@ F: nbd.* F: qemu-nbd.c T: git git://github.com/bonzini/qemu.git nbd-next +QAPI +M: Luiz Capitulino lcapitul...@redhat.com Mike Roth too, perhaps? I'd like to, but last time I asked he sort of refused :) Michael, can I add you? Haha, well, I think the discussion back then was whether I should send QAPI pulls, and I was of the mindset that we should keep sending those through the QMP tree since most QAPI changes were in support of QMP. According to the MAINTAINERS documentation though, M: is meant to be a set of names that developers can consult when they have a question about a particular subset and also to provide a set of names to be CC'd when submitting a patch to obtain appropriate review., so if that's the case I'd be happy to add my name and help in that regard. Oh, that's cool. I'll add your name for v2 then.
Re: [Qemu-devel] [PATCH 16/16] Make qemu-io commands available in the monitor
On Tue, 4 Jun 2013 12:08:23 +0200
Kevin Wolf kw...@redhat.com wrote:
Am 29.05.2013 um 19:51 hat Luiz Capitulino geschrieben:
On Wed, 29 May 2013 10:13:42 +0200
Kevin Wolf kw...@redhat.com wrote:
Am 28.05.2013 um 18:07 hat Eric Blake geschrieben:
On 05/28/2013 09:27 AM, Kevin Wolf wrote:
The QMP version is flagged with a __org.qemu.debug- prefix in order to
reinforce the statement that qemu-io is for testing and debugging
only,
with no API guarantees.
Correct use of naming conventions.
Hmm, I wonder if the recent addition of an 'abort' action to
'transaction' should be renamed __org.qemu.debug-abort, to make it
obvious that it is another case of a QMP command useful mainly for
testing, and not real-life use.
Makes sense to me.
But first I'd like to get Luiz's ack for this, because I think I'm the
first one to use an __org.qemu prefix, and I'm the first one trying to
introduce a QMP command without API stability.
I think that should be fine. However, it's not a perfect match for QMP
as you don't expect mngt to use it anytime soon and that the command's
syntax is not QMP friendly:
{ execute: __org.qemu.debug-qemu-io-command, arguments:
{ device: ide0-hd0, command: write -P 0x12 4M 512k } }
What about adding a HMP-only command (the good old way) and use it
through human-monitor-command?
IMO, this matches better your current use-case and the API instability
of the command.
Works for me, but wasn't the plan to make HMP purely a wrapper around
QMP? Then adding HMP-only commands would be counterproductive. So I
assumed that QMP is a must. I didn't even know that the code still
allows you to have HMP-only commands. :-)
Yes, the long term plan is to have all HMP commands calling QMP
counterparts. But the command you're adding doesn't fit QMP's design
very well.
I suggested adding it as HMP-only for now because it's the simplest
thing to do for this very specific case.
If more test-only non-QMP-friendly commands appear, then we'll need
to think of a more general solution.
Now, something has just occurred to me. Why isn't it a good idea having
this command as a stable API? Wouldn't it be a good idea to allow
out-of-tree test tools like autotest to use it?
So you prefer a respin with the QMP part dropped?
Yes.
Re: [Qemu-devel] [PATCH 16/16] Make qemu-io commands available in the monitor
On Tue, 4 Jun 2013 14:49:55 +0200
Kevin Wolf kw...@redhat.com wrote:
Am 04.06.2013 um 14:40 hat Luiz Capitulino geschrieben:
On Tue, 4 Jun 2013 12:08:23 +0200
Kevin Wolf kw...@redhat.com wrote:
Am 29.05.2013 um 19:51 hat Luiz Capitulino geschrieben:
On Wed, 29 May 2013 10:13:42 +0200
Kevin Wolf kw...@redhat.com wrote:
Am 28.05.2013 um 18:07 hat Eric Blake geschrieben:
On 05/28/2013 09:27 AM, Kevin Wolf wrote:
The QMP version is flagged with a __org.qemu.debug- prefix in
order to
reinforce the statement that qemu-io is for testing and debugging
only,
with no API guarantees.
Correct use of naming conventions.
Hmm, I wonder if the recent addition of an 'abort' action to
'transaction' should be renamed __org.qemu.debug-abort, to make it
obvious that it is another case of a QMP command useful mainly for
testing, and not real-life use.
Makes sense to me.
But first I'd like to get Luiz's ack for this, because I think I'm the
first one to use an __org.qemu prefix, and I'm the first one trying to
introduce a QMP command without API stability.
I think that should be fine. However, it's not a perfect match for QMP
as you don't expect mngt to use it anytime soon and that the command's
syntax is not QMP friendly:
{ execute: __org.qemu.debug-qemu-io-command, arguments:
{ device: ide0-hd0, command: write -P 0x12 4M 512k } }
What about adding a HMP-only command (the good old way) and use it
through human-monitor-command?
IMO, this matches better your current use-case and the API instability
of the command.
Works for me, but wasn't the plan to make HMP purely a wrapper around
QMP? Then adding HMP-only commands would be counterproductive. So I
assumed that QMP is a must. I didn't even know that the code still
allows you to have HMP-only commands. :-)
Yes, the long term plan is to have all HMP commands calling QMP
counterparts. But the command you're adding doesn't fit QMP's design
very well.
It fits the design about as well as human-monitor-command. Both are
passthrough commands, one to HMP, the other one to qemu-io.
Yes, but human-monitor-command is a stop gap command meant to be an
exception. It's usage should decrease over time. I'd expect the opposite
for qemu-io-command.
I suggested adding it as HMP-only for now because it's the simplest
thing to do for this very specific case.
If more test-only non-QMP-friendly commands appear, then we'll need
to think of a more general solution.
What does for now mean, and what will the proper long-term solution
look like?
For now means: until we know what the proper solution should be. I Honestly
don't know what's the best/proper solution here, that's why I suggested
having this in HMP first, so that we don't commit ourselves to anything
prematurely.
Now, something has just occurred to me. Why isn't it a good idea having
this command as a stable API? Wouldn't it be a good idea to allow
out-of-tree test tools like autotest to use it?
Because qemu-io exposes internals of the block layer that we don't want
to make a stable API.
What kind of internals? In the example you posted I see just a generic
enough write command.
Autotest can have test cases using this, but
they'd have to be against a specific qemu version.
So you prefer a respin with the QMP part dropped?
Yes.
Okay, I'll do that.
Kevin
[Qemu-devel] [PATCH v2 0/2] MAINTAINERS: QMP: entry update and new maintainers
Hi, This is an small update on QMP/HMP/QAPI mainternership. Please, check individual patches for details. v2 o add QMP/ to the QMP entry o add Michael as a maintainer for the QAPI Luiz Capitulino (2): MAINTAINERS: new maintainers for qapi-schema.json MAINTAINERS: split Monitor (QMP/HMP) entry MAINTAINERS | 26 -- 1 file changed, 24 insertions(+), 2 deletions(-) -- 1.8.1.4
[Qemu-devel] [PATCH 2/2] MAINTAINERS: split Monitor (QMP/HMP) entry
This entry doesn't reflect reality for a few years now. This commit splits it into Human Monitor (HMP), QAPI and QMP. Markus is dropped as a maintainer. This is what we have been for the last few years. Also, it's going to help me to offload some of this work to someone else in the near future. Signed-off-by: Luiz Capitulino lcapitul...@redhat.com Reviewed-by: Eric Blake ebl...@redhat.com --- MAINTAINERS | 19 +-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 432185a..13c0cc5 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -685,11 +685,12 @@ M: Anthony Liguori aligu...@us.ibm.com S: Supported F: vl.c -Monitor (QMP/HMP) +Human Monitor (HMP) M: Luiz Capitulino lcapitul...@redhat.com -M: Markus Armbruster arm...@redhat.com S: Supported F: monitor.c +F: hmp.c +F: hmp-commands.hx Network device layer M: Anthony Liguori aligu...@us.ibm.com @@ -706,6 +707,12 @@ F: nbd.* F: qemu-nbd.c T: git git://github.com/bonzini/qemu.git nbd-next +QAPI +M: Luiz Capitulino lcapitul...@redhat.com +M: Michael Roth mdr...@linux.vnet.ibm.com +S: Supported +F: qapi/ + QAPI Schema M: Eric Blake ebl...@redhat.com M: Luiz Capitulino lcapitul...@redhat.com @@ -713,6 +720,14 @@ M: Markus Armbruster arm...@redhat.com S: Supported F: qapi-schema.json +QMP +M: Luiz Capitulino lcapitul...@redhat.com +S: Supported +F: qmp.c +F: monitor.c +F: qmp-commands.hx +F: QMP/ + SLIRP M: Jan Kiszka jan.kis...@siemens.com S: Maintained -- 1.8.1.4
[Qemu-devel] [PATCH 1/2] MAINTAINERS: new maintainers for qapi-schema.json
I'm facing two problems lately wrt QMP patch review: increasingly lack of bandwidth and lack of background in so many different areas that are getting new QMP commands almost every week. In order to help me mitigate this problem, I'm adding Eric and Markus (besides me) as maintainers of the qapi-schema.json file. Markus has been an old timer reviewer. Eric is being the most active and prolific reviewer of QMP patches for some time now. I believe Markus and Eric will keep doing their work as before, but starting now I'll require the ACK of at least one of them before appling a patch/series that touches the qapi-schema.json file. Signed-off-by: Luiz Capitulino lcapitul...@redhat.com Reviewed-by: Eric Blake ebl...@redhat.com --- MAINTAINERS | 7 +++ 1 file changed, 7 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index be02724..432185a 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -706,6 +706,13 @@ F: nbd.* F: qemu-nbd.c T: git git://github.com/bonzini/qemu.git nbd-next +QAPI Schema +M: Eric Blake ebl...@redhat.com +M: Luiz Capitulino lcapitul...@redhat.com +M: Markus Armbruster arm...@redhat.com +S: Supported +F: qapi-schema.json + SLIRP M: Jan Kiszka jan.kis...@siemens.com S: Maintained -- 1.8.1.4
Re: [Qemu-devel] [PATCH v4 0/9] Make 'dump-guest-memory' dump in kdump-compressed format
On Wed, 05 Jun 2013 09:29:19 +0800 Qiao Nuohan qiaonuo...@cn.fujitsu.com wrote: I haven't reviewed it yet, but we need introspection support before merging this. Hello Luiz, Is it possible to get this reviewed, or I am supposed to wait until introspection support being settled? I can review it until the end of this week. If this series is adding a new argument (which I believe is what it does) then there's only two ways to get this merged: either we wait for full introspection or you add this feature as a new command. I'd prefer to wait for full introspection, but it depends how long it's going to take to get it merged and how much time you're willing to wait. Amos, can you give us an update on that work?
Re: [Qemu-devel] [PATCH v4 0/9] Make 'dump-guest-memory' dump in kdump-compressed format
[CC'ing Amos this time] On Wed, 05 Jun 2013 09:29:19 +0800 Qiao Nuohan qiaonuo...@cn.fujitsu.com wrote: I haven't reviewed it yet, but we need introspection support before merging this. Hello Luiz, Is it possible to get this reviewed, or I am supposed to wait until introspection support being settled? I can review it until the end of this week. If this series is adding a new argument (which I believe is what it does) then there's only two ways to get this merged: either we wait for full introspection or you add this feature as a new command. I'd prefer to wait for full introspection, but it depends how long it's going to take to get it merged and how much time you're willing to wait. Amos, can you give us an update on that work?
Re: [Qemu-devel] [PATCH v5] net: add support of mac-programming over macvtap in QEMU side
On Wed, 5 Jun 2013 18:42:13 +0800
Amos Kong ak...@redhat.com wrote:
Currently macvtap based macvlan device is working in promiscuous
mode, we want to implement mac-programming over macvtap through
Libvirt for better performance.
Design:
QEMU notifies Libvirt when rx-filter config is changed in guest,
then Libvirt query the rx-filter information by a monitor command,
and sync the change to macvtap device. Related rx-filter config
of the nic contains main mac, rx-mode items and vlan table.
This patch adds a QMP event to notify management of rx-filter change,
and adds a monitor command for management to query rx-filter
information.
For reducing length of output, we just return the entries of vlan
filter table that have active vlan.
Event_throttle API can avoid the events to flood QMP client, but it
could cause an unexpected delay. So a flag for each nic is used to
avoid events flooding, if management doesn't query rx-filter after
it receives one event, new events won't be emitted to QMP monitor.
There maybe exist an uncontrollable delay if we let Libvirt do the
real change, guests normally expect rx-filter updates immediately.
But it's another separate issue, we can investigate it when the
work in Libvirt side is done.
I think I completely misunderstood your testing results.
I had understood that: 1. changing the mac often quick enough to
be a problem was a corner case and 2. you actually can overflow mngt
I hope I really got it wrong, otherwise you'll be using that
flag as a replacement for the event throttle API, which would be
a big mistake.
Can you please add your test results analysis to this commit message?
Two small comments below.
Signed-off-by: Amos Kong ak...@redhat.com
---
v2: add argument to filter mac-table info of single nic (Stefan)
update the document, add event notification
v3: rename to rx-filter, add main mac, avoid events flooding (MST)
fix error process (Stefan), fix qmp interface (Eric)
v4: process qerror in hmp, cleanup (Luiz)
set flag for each device, add device path in event, add
helper for g_strdup_printf (MST)
fix qmp document (Eric)
v5: add path in doc, define notify flag to unsigned (Eric)
add vlan table (Jason), drop monitor cmd
---
QMP/qmp-events.txt| 20 +
hw/net/virtio-net.c | 112
++
include/monitor/monitor.h | 1 +
include/net/net.h | 3 ++
monitor.c | 1 +
net/net.c | 47 +++
qapi-schema.json | 89
qmp-commands.hx | 66 +++
8 files changed, 339 insertions(+)
diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
index 92fe5fb..885230e 100644
--- a/QMP/qmp-events.txt
+++ b/QMP/qmp-events.txt
@@ -172,6 +172,26 @@ Data:
},
timestamp: { seconds: 1265044230, microseconds: 450486 } }
+NIC_RX_FILTER_CHANGED
+-
+
+Emitted when rx-filter configuration of nic is changed by the guest.
+Each nic has a flag to control event emit, the flag is set to false
+when it emits one event of the nic, the flag is set to true when
+management queries the rx-filter of the nic. This is used to avoid
+events flooding.
Having this flag is an implementation detail. I think you should only
say that the event is emitted once until the query command is executed.
+
+Data:
+
+- name: net client name (json-string)
+- path: device path (json-string)
+
+{ event: NIC_RX_FILTER_CHANGED,
+ data: { name: vnet0,
+path: /machine/peripheral/vnet0/virtio-backend },
+ timestamp: { seconds: 1368697518, microseconds: 326866 } }
+}
+
RESET
-
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 1ea9556..ae1eab6 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -21,6 +21,8 @@
#include hw/virtio/virtio-net.h
#include net/vhost_net.h
#include hw/virtio/virtio-bus.h
+#include qapi/qmp/qjson.h
+#include monitor/monitor.h
#define VIRTIO_NET_VM_VERSION11
@@ -192,6 +194,104 @@ static void virtio_net_set_link_status(NetClientState
*nc)
virtio_net_set_status(vdev, vdev-status);
}
+static void rxfilter_notify(NetClientState *nc)
+{
+QObject *event_data;
+VirtIONet *n = qemu_get_nic_opaque(nc);
+
+if (nc-rxfilter_notify_enabled) {
+event_data = qobject_from_jsonf({ 'name': %s, 'path': %s },
+ n-netclient_name,
+ object_get_canonical_path(OBJECT(n-qdev)));
+monitor_protocol_event(QEVENT_NIC_RX_FILTER_CHANGED, event_data);
+qobject_decref(event_data);
+/* disable event notification to avoid events flooding */
+nc-rxfilter_notify_enabled = 0;
+}
+}
+
+static char *mac_strdup_printf(uint8_t *mac)
+{
mac can be const. Is there more code in QEMU that could use this
function? If
Re: [Qemu-devel] [PATCH v4] qemu-char: don't issue CHR_EVENT_OPEN in a BH
On Tue, 4 Jun 2013 16:35:09 -0500
Michael Roth mdr...@linux.vnet.ibm.com wrote:
When CHR_EVENT_OPENED was initially added, it was CHR_EVENT_RESET,
and it was issued as a bottom-half:
86e94dea5b740dad65446c857f6959eae43e0ba6
Which we basically used to print out a greeting/prompt for the
monitor.
AFAICT the only reason this was ever done in a BH was because in
some cases we'd modify the chr_write handler for a new chardev
backend *after* the site where we issued the reset (see:
86e94d:qemu_chr_open_stdio())
At some point this event was renamed to CHR_EVENT_OPENED, and we've
maintained the use of this BH ever since.
However, due to 9f939df955a4152aad69a19a77e0898631bb2c18, we schedule
the BH via g_idle_add(), which is causing events to sometimes be
delivered after we've already begun processing data from backends,
leading to:
known bugs:
QMP:
session negotation resets with OPENED event, in some cases this
is causing new sessions to get sporadically reset
potential bugs:
hw/usb/redirect.c:
can_read handler checks for dev-parser != NULL, which may be
true if CLOSED BH has not been executed yet. In the past, OPENED
quiesced outstanding CLOSED events prior to us reading client
data. If it's delayed, our check may allow reads to occur even
though we haven't processed the OPENED event yet, and when we
do finally get the OPENED event, our state may get reset.
qtest.c:
can begin session before OPENED event is processed, leading to
a spurious reset of the system and irq_levels
gdbstub.c:
may start a gdb session prior to the machine being paused
To fix these, let's just drop the BH.
Since the initial reasoning for using it still applies to an extent,
work around that by deferring the delivery of CHR_EVENT_OPENED until
after the chardevs have been fully initialized, toward the end of
qmp_chardev_add() (or some cases, qemu_chr_new_from_opts()). This
defers delivery long enough that we can be assured a CharDriverState
is fully initialized before CHR_EVENT_OPENED is sent.
Also, rather than requiring each chardev to do an explicit open, do it
automatically, and allow the small few who don't desire such behavior to
suppress the OPENED-on-init behavior by setting a 'explicit_be_open'
flag.
We additionally add missing OPENED events for stdio backends on w32,
which were previously not being issued, causing us to not recieve the
banner and initial prompts for qmp/hmp.
Reported-by: Stefan Priebe s.pri...@profihost.ag
Cc: qemu-sta...@nongnu.org
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
I don't know if the QMP queue is the ideal queue for this patch, but
I'd happily apply it if I get at least one Reviewed-by from the CC'ed
people.
---
v3-v4:
* renamed 'suppress_be_open_on_init' to 'explicit_be_open' to match
existing 'explicit_fe_open' flag (Hans)
* added missing 'explicit_be_open' flags for spice vmc/port and
msmouse backends
v2-v3:
* removed artifact in from v1 in backends/baum.c, test build with
BRLAPI=y (Anthony)
* rebased on latest origin/master
v1-v2:
* default to sending OPENED on backend init, add flag to suppress
it (Anthony)
* fix missing OPENED for stdio backends on w32
* fix missing OPENED when qemu_chr_new_from_opts() doesn't use
qmp_chardev_add()
* clean up/update commit message
backends/baum.c |2 --
backends/msmouse.c|1 +
include/sysemu/char.h |2 +-
qemu-char.c | 38 +-
spice-qemu-char.c |1 +
ui/console.c |1 -
ui/gtk.c |1 -
7 files changed, 20 insertions(+), 26 deletions(-)
diff --git a/backends/baum.c b/backends/baum.c
index 4cba79f..62aa784 100644
--- a/backends/baum.c
+++ b/backends/baum.c
@@ -611,8 +611,6 @@ CharDriverState *chr_baum_init(void)
qemu_set_fd_handler(baum-brlapi_fd, baum_chr_read, NULL, baum);
-qemu_chr_be_generic_open(chr);
-
return chr;
fail:
diff --git a/backends/msmouse.c b/backends/msmouse.c
index 0ac05a0..c0dbfcd 100644
--- a/backends/msmouse.c
+++ b/backends/msmouse.c
@@ -70,6 +70,7 @@ CharDriverState *qemu_chr_open_msmouse(void)
chr = g_malloc0(sizeof(CharDriverState));
chr-chr_write = msmouse_chr_write;
chr-chr_close = msmouse_chr_close;
+chr-explicit_be_open = true;
qemu_add_mouse_event_handler(msmouse_event, chr, 0, QEMU Microsoft
Mouse);
diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index 5e42c90..066c216 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -70,12 +70,12 @@ struct CharDriverState {
void (*chr_set_echo)(struct CharDriverState *chr, bool echo);
void (*chr_set_fe_open)(struct CharDriverState *chr, int fe_open);
void *opaque;
-int idle_tag;
char *label;
char *filename;
int be_open;
int fe_open;
int
Re: [Qemu-devel] [PATCH v2 16/16] Make qemu-io commands available in HMP
On Wed, 5 Jun 2013 14:19:41 +0200
Kevin Wolf kw...@redhat.com wrote:
It was decided to not make this command available in QMP in order to
make clear that this is not supposed to be a stable API and should be
used only for testing and debugging purposes.
I like this as a temporary solution.
But I also see that commands seem to print to stderr, so this needs
work before being available in QMP. Even for HMP or the -debug
extension this is not desirable.
Signed-off-by: Kevin Wolf kw...@redhat.com
---
Makefile| 2 +-
Makefile.objs | 1 +
hmp-commands.hx | 16
hmp.c | 18 ++
hmp.h | 1 +
5 files changed, 37 insertions(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index 87298e5..9a77ae0 100644
--- a/Makefile
+++ b/Makefile
@@ -186,7 +186,7 @@ qemu-img.o: qemu-img-cmds.h
qemu-img$(EXESUF): qemu-img.o $(block-obj-y) libqemuutil.a libqemustub.a
qemu-nbd$(EXESUF): qemu-nbd.o $(block-obj-y) libqemuutil.a libqemustub.a
-qemu-io$(EXESUF): qemu-io.o qemu-io-cmds.o $(block-obj-y) libqemuutil.a
libqemustub.a
+qemu-io$(EXESUF): qemu-io.o $(block-obj-y) libqemuutil.a libqemustub.a
qemu-bridge-helper$(EXESUF): qemu-bridge-helper.o
diff --git a/Makefile.objs b/Makefile.objs
index 286ce06..5b288ba 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -13,6 +13,7 @@ block-obj-$(CONFIG_POSIX) += aio-posix.o
block-obj-$(CONFIG_WIN32) += aio-win32.o
block-obj-y += block/
block-obj-y += qapi-types.o qapi-visit.o
+block-obj-y += qemu-io-cmds.o
block-obj-y += qemu-coroutine.o qemu-coroutine-lock.o qemu-coroutine-io.o
block-obj-y += qemu-coroutine-sleep.o
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 9cea415..a6167bd 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1551,6 +1551,22 @@ Removes the chardev @var{id}.
ETEXI
{
+.name = qemu-io,
+.args_type = device:B,command:s,
+.params = [device] \[command]\,
+.help = run a qemu-io command on a block device,
+.mhandler.cmd = hmp_qemu_io,
+},
+
+STEXI
+@item qemu-io @var{device} @var{command}
+@findex qemu-io
+
+Executes a qemu-io command on the given block device.
+
+ETEXI
+
+{
.name = info,
.args_type = item:s?,
.params = [subcommand],
diff --git a/hmp.c b/hmp.c
index 4fb76ec..64e0baa 100644
--- a/hmp.c
+++ b/hmp.c
@@ -22,6 +22,7 @@
#include qemu/sockets.h
#include monitor/monitor.h
#include ui/console.h
+#include qemu-io.h
static void hmp_handle_error(Monitor *mon, Error **errp)
{
@@ -1425,3 +1426,20 @@ void hmp_chardev_remove(Monitor *mon, const QDict
*qdict)
qmp_chardev_remove(qdict_get_str(qdict, id), local_err);
hmp_handle_error(mon, local_err);
}
+
+void hmp_qemu_io(Monitor *mon, const QDict *qdict)
+{
+BlockDriverState *bs;
+const char* device = qdict_get_str(qdict, device);
+const char* command = qdict_get_str(qdict, command);
+Error *err = NULL;
+
+bs = bdrv_find(device);
+if (bs) {
+qemuio_command(bs, command);
+} else {
+error_set(err, QERR_DEVICE_NOT_FOUND, device);
+}
+
+hmp_handle_error(mon, err);
+}
diff --git a/hmp.h b/hmp.h
index 95fe76e..56d2e92 100644
--- a/hmp.h
+++ b/hmp.h
@@ -85,5 +85,6 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict);
void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
void hmp_chardev_add(Monitor *mon, const QDict *qdict);
void hmp_chardev_remove(Monitor *mon, const QDict *qdict);
+void hmp_qemu_io(Monitor *mon, const QDict *qdict);
#endif
Re: [Qemu-devel] [PATCH v4] qemu-char: don't issue CHR_EVENT_OPEN in a BH
On Fri, 7 Jun 2013 09:56:00 -0500 mdroth mdr...@linux.vnet.ibm.com wrote: On Fri, Jun 07, 2013 at 09:50:39AM -0400, Luiz Capitulino wrote: On Tue, 4 Jun 2013 16:35:09 -0500 Michael Roth mdr...@linux.vnet.ibm.com wrote: When CHR_EVENT_OPENED was initially added, it was CHR_EVENT_RESET, and it was issued as a bottom-half: 86e94dea5b740dad65446c857f6959eae43e0ba6 Which we basically used to print out a greeting/prompt for the monitor. AFAICT the only reason this was ever done in a BH was because in some cases we'd modify the chr_write handler for a new chardev backend *after* the site where we issued the reset (see: 86e94d:qemu_chr_open_stdio()) At some point this event was renamed to CHR_EVENT_OPENED, and we've maintained the use of this BH ever since. However, due to 9f939df955a4152aad69a19a77e0898631bb2c18, we schedule the BH via g_idle_add(), which is causing events to sometimes be delivered after we've already begun processing data from backends, leading to: known bugs: QMP: session negotation resets with OPENED event, in some cases this is causing new sessions to get sporadically reset potential bugs: hw/usb/redirect.c: can_read handler checks for dev-parser != NULL, which may be true if CLOSED BH has not been executed yet. In the past, OPENED quiesced outstanding CLOSED events prior to us reading client data. If it's delayed, our check may allow reads to occur even though we haven't processed the OPENED event yet, and when we do finally get the OPENED event, our state may get reset. qtest.c: can begin session before OPENED event is processed, leading to a spurious reset of the system and irq_levels gdbstub.c: may start a gdb session prior to the machine being paused To fix these, let's just drop the BH. Since the initial reasoning for using it still applies to an extent, work around that by deferring the delivery of CHR_EVENT_OPENED until after the chardevs have been fully initialized, toward the end of qmp_chardev_add() (or some cases, qemu_chr_new_from_opts()). This defers delivery long enough that we can be assured a CharDriverState is fully initialized before CHR_EVENT_OPENED is sent. Also, rather than requiring each chardev to do an explicit open, do it automatically, and allow the small few who don't desire such behavior to suppress the OPENED-on-init behavior by setting a 'explicit_be_open' flag. We additionally add missing OPENED events for stdio backends on w32, which were previously not being issued, causing us to not recieve the banner and initial prompts for qmp/hmp. Reported-by: Stefan Priebe s.pri...@profihost.ag Cc: qemu-sta...@nongnu.org Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com I don't know if the QMP queue is the ideal queue for this patch, but I'd happily apply it if I get at least one Reviewed-by from the CC'ed people. Anthony actually added his Reviewed-by for v3, but I forgot to roll it in the commit. If you do take it in through your tree can you add that? Sorry for the bureaucracy, but I don't like to add other people's tags when the patch changes. Even when I'm sure they would be OK with the new version. Anthony, can you please add your Reviewed-by again? Thanks! --- v3-v4: * renamed 'suppress_be_open_on_init' to 'explicit_be_open' to match existing 'explicit_fe_open' flag (Hans) * added missing 'explicit_be_open' flags for spice vmc/port and msmouse backends v2-v3: * removed artifact in from v1 in backends/baum.c, test build with BRLAPI=y (Anthony) * rebased on latest origin/master v1-v2: * default to sending OPENED on backend init, add flag to suppress it (Anthony) * fix missing OPENED for stdio backends on w32 * fix missing OPENED when qemu_chr_new_from_opts() doesn't use qmp_chardev_add() * clean up/update commit message backends/baum.c |2 -- backends/msmouse.c|1 + include/sysemu/char.h |2 +- qemu-char.c | 38 +- spice-qemu-char.c |1 + ui/console.c |1 - ui/gtk.c |1 - 7 files changed, 20 insertions(+), 26 deletions(-) diff --git a/backends/baum.c b/backends/baum.c index 4cba79f..62aa784 100644 --- a/backends/baum.c +++ b/backends/baum.c @@ -611,8 +611,6 @@ CharDriverState *chr_baum_init(void) qemu_set_fd_handler(baum-brlapi_fd, baum_chr_read, NULL, baum); -qemu_chr_be_generic_open(chr); - return chr; fail: diff --git a/backends/msmouse.c b/backends/msmouse.c index 0ac05a0..c0dbfcd 100644 --- a/backends/msmouse.c +++ b/backends/msmouse.c @@ -70,6 +70,7
[Qemu-devel] [PATCH 4/9] block: mirror_complete(): use error_setg_file_open()
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
block/mirror.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/mirror.c b/block/mirror.c
index 8b07dec..89d531d 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -512,7 +512,7 @@ static void mirror_complete(BlockJob *job, Error **errp)
char backing_filename[PATH_MAX];
bdrv_get_full_backing_filename(s-target, backing_filename,
sizeof(backing_filename));
-error_set(errp, QERR_OPEN_FILE_FAILED, backing_filename);
+error_setg_file_open(errp, errno, backing_filename);
return;
}
if (!s-synced) {
--
1.8.1.4
[Qemu-devel] [PATCH 1/9] error: add error_setg_file_open() helper
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
include/qapi/error.h | 5 +
util/error.c | 5 +
2 files changed, 10 insertions(+)
diff --git a/include/qapi/error.h b/include/qapi/error.h
index 5cd2f0c..ffd1cea 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -45,6 +45,11 @@ void error_set_errno(Error **err, int os_error, ErrorClass
err_class, const char
error_set_errno(err, os_error, ERROR_CLASS_GENERIC_ERROR, fmt, ##
__VA_ARGS__)
/**
+ * Helper for open() errors
+ */
+void error_setg_file_open(Error **errp, int os_errno, const char *filename);
+
+/**
* Returns true if an indirect pointer to an error is pointing to a valid
* error object.
*/
diff --git a/util/error.c b/util/error.c
index 519f6b6..53b0435 100644
--- a/util/error.c
+++ b/util/error.c
@@ -71,6 +71,11 @@ void error_set_errno(Error **errp, int os_errno, ErrorClass
err_class,
*errp = err;
}
+void error_setg_file_open(Error **errp, int os_errno, const char *filename)
+{
+error_setg_errno(errp, os_errno, Could not open '%s', filename);
+}
+
Error *error_copy(const Error *err)
{
Error *err_new;
--
1.8.1.4
[Qemu-devel] [PATCH 0/9] QMP/HMP: add error reason to open failures
Today I was surprised to find out that we still have old users of QERR_OPEN_FILE_FAILED that print errors like: (qemu) dump-guest-memory -p /lkmads/foo Could not open '/lkmads/foo' (qemu) This series converts all those users to a new helper called error_setg_file_open(), which adds error reason to open failures: (qemu) dump-guest-memory -p /sfmdkjf/foo Could not open '/sfmdkjf/foo': No such file or directory (qemu) Luiz Capitulino (9): error: add error_setg_file_open() helper rng-random: use error_setg_file_open() block: bdrv_reopen_prepare(): use error_setg_file_open() block: mirror_complete(): use error_setg_file_open() blockdev: use error_setg_file_open() cpus: use error_setg_file_open() dump: qmp_dump_guest_memory(): use error_setg_file_open() savevm: qmp_xen_save_devices_state(): use error_setg_file_open() qerror: drop QERR_OPEN_FILE_FAILED macro backends/rng-random.c | 3 +-- block.c | 3 +-- block/mirror.c| 2 +- blockdev.c| 6 +++--- cpus.c| 4 ++-- dump.c| 2 +- include/qapi/error.h | 5 + include/qapi/qmp/qerror.h | 3 --- savevm.c | 2 +- util/error.c | 5 + 10 files changed, 20 insertions(+), 15 deletions(-) -- 1.8.1.4
[Qemu-devel] [PATCH 3/9] block: bdrv_reopen_prepare(): use error_setg_file_open()
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
block.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/block.c b/block.c
index 79ad33d..c78f152 100644
--- a/block.c
+++ b/block.c
@@ -1291,8 +1291,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
BlockReopenQueue *queue,
if (local_err != NULL) {
error_propagate(errp, local_err);
} else {
-error_set(errp, QERR_OPEN_FILE_FAILED,
- reopen_state-bs-filename);
+error_setg_file_open(errp, errno, reopen_state-bs-filename);
}
goto error;
}
--
1.8.1.4
[Qemu-devel] [PULL 3/3] MAINTAINERS: split Monitor (QMP/HMP) entry
This entry doesn't reflect reality for a few years now. This commit splits it into Human Monitor (HMP), QAPI and QMP. Markus is dropped as a maintainer. This is what we have been for the last few years. Also, it's going to help me to offload some of this work to someone else in the near future. Signed-off-by: Luiz Capitulino lcapitul...@redhat.com Acked-by: Markus Armbruster arm...@redhat.com --- MAINTAINERS | 19 +-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 432185a..13c0cc5 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -685,11 +685,12 @@ M: Anthony Liguori aligu...@us.ibm.com S: Supported F: vl.c -Monitor (QMP/HMP) +Human Monitor (HMP) M: Luiz Capitulino lcapitul...@redhat.com -M: Markus Armbruster arm...@redhat.com S: Supported F: monitor.c +F: hmp.c +F: hmp-commands.hx Network device layer M: Anthony Liguori aligu...@us.ibm.com @@ -706,6 +707,12 @@ F: nbd.* F: qemu-nbd.c T: git git://github.com/bonzini/qemu.git nbd-next +QAPI +M: Luiz Capitulino lcapitul...@redhat.com +M: Michael Roth mdr...@linux.vnet.ibm.com +S: Supported +F: qapi/ + QAPI Schema M: Eric Blake ebl...@redhat.com M: Luiz Capitulino lcapitul...@redhat.com @@ -713,6 +720,14 @@ M: Markus Armbruster arm...@redhat.com S: Supported F: qapi-schema.json +QMP +M: Luiz Capitulino lcapitul...@redhat.com +S: Supported +F: qmp.c +F: monitor.c +F: qmp-commands.hx +F: QMP/ + SLIRP M: Jan Kiszka jan.kis...@siemens.com S: Maintained -- 1.8.1.4
[Qemu-devel] [PULL 0/3] QMP queue
The changes (since 7387de16d0e4d2988df350926537cd12a8e34206) are available in the following repository: git://repo.or.cz/qemu/qmp-unstable.git queue/qmp Luiz Capitulino (2): MAINTAINERS: new maintainers for qapi-schema.json MAINTAINERS: split Monitor (QMP/HMP) entry Marcelo Tosatti (1): correct RTC_CHANGE_EVENT description MAINTAINERS| 26 -- QMP/qmp-events.txt | 3 ++- 2 files changed, 26 insertions(+), 3 deletions(-) -- 1.8.1.4
[Qemu-devel] [PATCH 5/9] blockdev: use error_setg_file_open()
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
blockdev.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 9937311..c09adba 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -899,7 +899,7 @@ static void external_snapshot_prepare(BlkTransactionStates
*common,
ret = bdrv_open(states-new_bs, new_image_file, NULL,
flags | BDRV_O_NO_BACKING, drv);
if (ret != 0) {
-error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
+error_setg_file_open(errp, errno, new_image_file);
}
}
@@ -1063,7 +1063,7 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs,
const char *filename,
const char *password, Error **errp)
{
if (bdrv_open(bs, filename, NULL, bdrv_flags, drv) 0) {
-error_set(errp, QERR_OPEN_FILE_FAILED, filename);
+error_setg_file_open(errp, errno, filename);
return;
}
@@ -1483,7 +1483,7 @@ void qmp_drive_mirror(const char *device, const char
*target,
if (ret 0) {
bdrv_delete(target_bs);
-error_set(errp, QERR_OPEN_FILE_FAILED, target);
+error_setg_file_open(errp, errno, target);
return;
}
--
1.8.1.4
[Qemu-devel] [PULL 1/3] correct RTC_CHANGE_EVENT description
From: Marcelo Tosatti mtosa...@redhat.com Fix RTC_CHANGE event description to match implementation. Signed-off-by: Marcelo Tosatti mtosa...@redhat.com Signed-off-by: Luiz Capitulino lcapitul...@redhat.com --- QMP/qmp-events.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt index 92fe5fb..00b4087 100644 --- a/QMP/qmp-events.txt +++ b/QMP/qmp-events.txt @@ -203,7 +203,8 @@ Emitted when the guest changes the RTC time. Data: -- offset: delta against the host UTC in seconds (json-number) +- offset: Offset between old RTC clock value and new one +in seconds (json-number) Example: -- 1.8.1.4
[Qemu-devel] [PATCH 6/9] cpus: use error_setg_file_open()
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
cpus.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/cpus.c b/cpus.c
index c232265..c8bc8ad 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1278,7 +1278,7 @@ void qmp_memsave(int64_t addr, int64_t size, const char
*filename,
f = fopen(filename, wb);
if (!f) {
-error_set(errp, QERR_OPEN_FILE_FAILED, filename);
+error_setg_file_open(errp, errno, filename);
return;
}
@@ -1308,7 +1308,7 @@ void qmp_pmemsave(int64_t addr, int64_t size, const char
*filename,
f = fopen(filename, wb);
if (!f) {
-error_set(errp, QERR_OPEN_FILE_FAILED, filename);
+error_setg_file_open(errp, errno, filename);
return;
}
--
1.8.1.4
[Qemu-devel] [PATCH 8/9] savevm: qmp_xen_save_devices_state(): use error_setg_file_open()
Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
---
savevm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/savevm.c b/savevm.c
index 2ce439f..ff5ece6 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2410,7 +2410,7 @@ void qmp_xen_save_devices_state(const char *filename,
Error **errp)
f = qemu_fopen(filename, wb);
if (!f) {
-error_set(errp, QERR_OPEN_FILE_FAILED, filename);
+error_setg_file_open(errp, errno, filename);
goto the_end;
}
ret = qemu_save_device_state(f);
--
1.8.1.4
