[Qemu-devel] [PATCH v2 2/2] display: stop using DT_NOGRAPHIC, use DT_NONE
It looks like initially there was -nographic option to turn
off display, now there's another option of the same sort,
-display none. But code in other places of qemu checks for
DT_NOGRAPHIC and does not work well with -display none.
Make DT_NOGRAPHIC an internal version which selects DT_NONE,
and check for that in all other places where previously we
checked for DT_NOGRAPHIC.
While at it, rename two private variants of display (DT_DEFAULT
and DT_NOGRAPHIC) to use two underscores and make them negative,
and set DT_NONE to 0.
This should fix the issue of non-working sun serial console
with the suggested replacement of -nographic which is
-display none.
Cc: Todd T. Fries
Signed-off-by: Michael Tokarev
---
V2:
- do not touch qemu-char, fixed differently as suggested by pbonzini
- a bit more explicit comments about private DT__* constants
- documentation additions and fixes to describe actual reality
hw/lm32/milkymist-hw.h |2 +-
hw/nvram/fw_cfg.c |2 +-
hw/sparc/sun4m.c|2 +-
include/sysemu/sysemu.h |6 +++---
qemu-options.hx | 12 ++--
vl.c| 15 +++
6 files changed, 23 insertions(+), 16 deletions(-)
diff --git a/hw/lm32/milkymist-hw.h b/hw/lm32/milkymist-hw.h
index 5317ce6..59af720 100644
--- a/hw/lm32/milkymist-hw.h
+++ b/hw/lm32/milkymist-hw.h
@@ -107,7 +107,7 @@ static inline DeviceState *milkymist_tmu2_create(hwaddr
base,
int nelements;
int ver_major, ver_minor;
-if (display_type == DT_NOGRAPHIC) {
+if (display_type == DT_NONE) {
return NULL;
}
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 3c255ce..b3d163a 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -510,7 +510,7 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t
data_port,
}
fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
fw_cfg_add_bytes(s, FW_CFG_UUID, qemu_uuid, 16);
-fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)(display_type ==
DT_NOGRAPHIC));
+fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)(display_type == DT_NONE));
fw_cfg_add_i16(s, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
fw_cfg_add_i16(s, FW_CFG_BOOT_MENU, (uint16_t)boot_menu);
fw_cfg_bootsplash(s);
diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index 0e86ca7..c1d42ec 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -919,7 +919,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
ram_addr_t RAM_size,
slavio_timer_init_all(hwdef->counter_base, slavio_irq[19], slavio_cpu_irq,
smp_cpus);
slavio_serial_ms_kbd_init(hwdef->ms_kb_base, slavio_irq[14],
- display_type == DT_NOGRAPHIC, ESCC_CLOCK, 1);
+ display_type == DT_NONE, ESCC_CLOCK, 1);
/* Slavio TTYA (base+4, Linux ttyS0) is the first QEMU serial device
Slavio TTYB (base+0, Linux ttyS1) is the second QEMU serial device */
escc_init(hwdef->serial_base, slavio_irq[15], slavio_irq[15],
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 2fb71af..d1b7bd7 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -87,12 +87,12 @@ void do_info_slirp(Monitor *mon);
typedef enum DisplayType
{
-DT_DEFAULT,
+DT__DEFAULT = -1, /* used internally in vl.c */
+DT__NOGRAPHIC = -2, /* used internally in vl.c */
+DT_NONE = 0,
DT_CURSES,
DT_SDL,
DT_GTK,
-DT_NOGRAPHIC,
-DT_NONE,
} DisplayType;
extern int autostart;
diff --git a/qemu-options.hx b/qemu-options.hx
index d676b03..4669138 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -825,7 +825,11 @@ a text mode. Generally only the VGA device models support
text mode.
@item none
Do not display video output. The guest will still see an emulated
graphics card, but its output will not be displayed to the QEMU
-user. This option differs from the -nographic option in that it
+user. The fact that we have no display is passed to firmware and
+affects a few other places depending on the target architecture,
+like switching console output to serial console or disabling keyboard
+input.
+This option differs from the -nographic option in that it
only affects what is done with video output; -nographic also changes
the destination of the serial and parallel port data.
@item vnc
@@ -844,7 +848,11 @@ you can totally disable graphical output so that QEMU is a
simple
command line application. The emulated serial port is redirected on
the console and muxed with the monitor (unless redirected elsewhere
explicitly). Therefore, you can still use QEMU to debug a Linux kernel
-with a serial console.
+with a serial console. This option is equivalent for
+@example
+-display none -serial mon:stdio -parallel none
+@end example
+unless serial, parallel and/or monitor were also specified.
ETEXI
DEF("curses", 0, QEMU_OPTION_curses,
diff --git a/vl.c b/vl.c
index 6d9fd7d..98d3e62 100644
--- a/vl.c
+++ b/vl.c
@@ -183,7 +183,7 @@ static cons
[Qemu-devel] [PATCH v2 1/2] trap signals for "-serial mon:stdio"
From: Paolo Bonzini
With mon:stdio you can exit the VM by switching to the monitor and
sending the "quit" command. It is then useful to pass Ctrl-C to the
VM instead of exiting.
This in turn lets us stop tying the default signal handling behavior
to -nographic, removing gratuitous differences between "-display none"
and "-nographic".
This patch changes behavior for "-display none -serial mon:stdio", as
expected, but not for "-display none -serial stdio".
Signed-off-by: Paolo Bonzini
Signed-off-by: Michael Tokarev
---
V2: added code comments and documentation fixes by mjt
(hopefully the s-o-b stands still)
qemu-char.c | 13 +
qemu-options.hx |8 +---
2 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/qemu-char.c b/qemu-char.c
index 6cec5d7..18c42a3 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -926,7 +926,6 @@ static void qemu_chr_set_echo_stdio(CharDriverState *chr,
bool echo)
tty.c_cc[VMIN] = 1;
tty.c_cc[VTIME] = 0;
}
-/* if graphical mode, we allow Ctrl-C handling */
if (!stdio_allow_signal)
tty.c_lflag &= ~ISIG;
@@ -955,7 +954,6 @@ static CharDriverState *qemu_chr_open_stdio(ChardevStdio
*opts)
chr = qemu_chr_open_fd(0, 1);
chr->chr_close = qemu_chr_close_stdio;
chr->chr_set_echo = qemu_chr_set_echo_stdio;
-stdio_allow_signal = display_type != DT_NOGRAPHIC;
if (opts->has_signal) {
stdio_allow_signal = opts->signal;
}
@@ -2932,6 +2930,14 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const
char *filename)
if (strstart(filename, "mon:", &p)) {
filename = p;
qemu_opt_set(opts, "mux", "on");
+if (strcmp(filename, "stdio") == 0) {
+/* Monitor is muxed to stdio: do not exit on Ctrl+C by default
+ * but pass it to the guest. Handle this only for compat syntax,
+ * for -chardev syntax we have special option for this.
+ * This is what -nographic did, redirecting+muxing serial+monitor
+ * to stdio causing Ctrl+C to be passed to guest. */
+qemu_opt_set(opts, "signal", "off");
+}
}
if (strcmp(filename, "null")== 0 ||
@@ -3060,8 +3066,7 @@ static void qemu_chr_parse_stdio(QemuOpts *opts,
ChardevBackend *backend,
{
backend->stdio = g_new0(ChardevStdio, 1);
backend->stdio->has_signal = true;
-backend->stdio->signal =
-qemu_opt_get_bool(opts, "signal", display_type != DT_NOGRAPHIC);
+backend->stdio->signal = qemu_opt_get_bool(opts, "signal", true);
}
static void qemu_chr_parse_serial(QemuOpts *opts, ChardevBackend *backend,
diff --git a/qemu-options.hx b/qemu-options.hx
index 137a39b..d676b03 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -842,7 +842,8 @@ STEXI
Normally, QEMU uses SDL to display the VGA output. With this option,
you can totally disable graphical output so that QEMU is a simple
command line application. The emulated serial port is redirected on
-the console. Therefore, you can still use QEMU to debug a Linux kernel
+the console and muxed with the monitor (unless redirected elsewhere
+explicitly). Therefore, you can still use QEMU to debug a Linux kernel
with a serial console.
ETEXI
@@ -2485,14 +2486,15 @@ same as if you had specified @code{-serial tcp} except
the unix domain socket
@item mon:@var{dev_string}
This is a special option to allow the monitor to be multiplexed onto
another serial port. The monitor is accessed with key sequence of
-@key{Control-a} and then pressing @key{c}. See monitor access
-@ref{pcsys_keys} in the -nographic section for more keys.
+@key{Control-a} and then pressing @key{c}.
@var{dev_string} should be any one of the serial devices specified
above. An example to multiplex the monitor onto a telnet server
listening on port would be:
@table @code
@item -serial mon:telnet::,server,nowait
@end table
+When monitor is multiplexed to stdio this way, Ctrl+C will not terminate
+guest anymore but will be passed to the guest as is.
@item braille
Braille device. This will use BrlAPI to display the braille output on a real
--
1.7.10.4
Re: [Qemu-devel] [PATCH 3/7] nbd: use BDS refcount
On Wed, 07/03 07:58, Paolo Bonzini wrote: > Il 03/07/2013 03:10, Fam Zheng ha scritto: > > > This has the important side effect of marking the exported disk as > > > "in_use" (to use the terms before the series). Right now you can serve > > > a disk and, at the same time, stream it or mirror it or create a live > > > snapshot of it. > > > > > > Do we really want to block anything for a device being served? Perhaps > > > truncation, but maybe not even that. The NBD server is meant to be as > > > unobtrusive as possible (in some sense NBD accesses are the same as > > > guest accesses). > > > > OK, it is better to work like that. But I don't quite understand why was > > there drive_get_ref() on the device (w/o the series), as there's already > > a close notifier? And it just drive_put_ref() when bs is closed? > > The close notifier runs when the user invokes a drive_del or eject > command from the monitor. The drive_get_ref/drive_put_ref delays the > bdrv_delete until after nbd.c has cleaned up all the connections. But drive_put_ref is called by close notifier. I think it can be omitted, registering a close notifier is enough, and close the export when drive_del calls it. It doesn't make more sense w/ drive_get_ref, does it? -- Fam
Re: [Qemu-devel] [PATCH v2 5/6] net: defer nested call to BH
Il 13/06/2013 11:03, Liu Ping Fan ha scritto:
> From: Liu Ping Fan
>
> Nested call caused by ->receive() will raise issue like deadlock,
> so postphone it to BH.
When I read this patch, I am completely puzzled.
All I see is that a qemu_net_queue_flush is done in a bottom half. I
have no clue of how you get a nested call of ->receive, and I have no
clue of what this patch does to prevent it (since the bottom half is
asynchronous).
A bottom half is not a synchronization primitive. If you use a lock, or
a condition variable, or RCU, you can usually assume that the reader
understands how you're using it (though not if you're doing fancy
tricks). If you are introducing infrastructure, you can use long
comments or docs/ and expect the reviewer to read those.
But if you're doing tricky stuff (such as if you have global/local
locks, or you have parts of the code that run lockless, or if you are
modifying the behavior of an existing subsystem to prevent deadlocks),
you need to document _every single step that you do_.
For example, let's take the patches you had for hostmem.c. You had a
single patch doing all the work:
hostmem: make hostmem global and RAM hotunplg safe
The hostmem listener will translate gpa to hva quickly. Make it
global, so other component can use it.
Also this patch adopt MemoryRegionOps's ref/unref interface to
make it survive the RAM hotunplug.
No reference whatsoever to the reference counting of the hostmem itself,
and how you are using the locks. The "also" in the commit message is a
big warning sign, too (though I'm guilty of adding many "also"s in the
commit messages).
When I took your ideas and applied them to memory.c, here is how I
explained it:
memory: use a new FlatView pointer on every topology update
This is the first step towards converting as->current_map to
RCU-style updates, where the FlatView updates run concurrently
with uses of an old FlatView.
---
memory: add reference counting to FlatView
With this change, a FlatView can be used even after a concurrent
update has replaced it. Because we do not have RCU, we use a
mutex to protect the small critical sections that read/write the
as->current_map pointer. Accesses to the FlatView can be done
outside the mutex.
And in the code:
/* flat_view_mutex is taken around reading as->current_map; the
* critical section is extremely short, so I'm using a single mutex
* for every AS. We could also RCU for the read-side.
*
* The BQL is taken around transaction commits, hence both locks are
* taken while writing to as->current_map (with the BQL taken
* outside).
*/
It is still quite concise, but it explains both the concurrency to
expect, and the locking policies.
This patch is changing the behavior of existing code in a complex
scenario and in a tricky way. If you want your patches accepted (or
even reviewed), you _must_ learn how to explain the scenarios and your
fixes.
And believe me, it is not a language problem; people with much worse
English than yours manage this all the time. It is more of an attitude
problem, and I've explained it many times.
Paolo
> Signed-off-by: Liu Ping Fan
> ---
> net/queue.c | 40 ++--
> 1 file changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/net/queue.c b/net/queue.c
> index 58222b0..9c343ab 100644
> --- a/net/queue.c
> +++ b/net/queue.c
> @@ -24,6 +24,8 @@
> #include "net/queue.h"
> #include "qemu/queue.h"
> #include "net/net.h"
> +#include "block/aio.h"
> +#include "qemu/main-loop.h"
>
> /* The delivery handler may only return zero if it will call
> * qemu_net_queue_flush() when it determines that it is once again able
> @@ -183,6 +185,22 @@ static ssize_t qemu_net_queue_deliver_iov(NetQueue
> *queue,
> return ret;
> }
>
> +typedef struct NetQueBH {
> +QEMUBH *bh;
> +NetClientState *nc;
> +} NetQueBH;
> +
> +static void qemu_net_queue_send_bh(void *opaque)
> +{
> +NetQueBH *q_bh = opaque;
> +NetQueue *queue = q_bh->nc->send_queue;
> +
> +qemu_net_queue_flush(queue);
> +netclient_unref(q_bh->nc);
> +qemu_bh_delete(q_bh->bh);
> +g_slice_free(NetQueBH, q_bh);
> +}
> +
> ssize_t qemu_net_queue_send(NetQueue *queue,
> NetClientState *sender,
> unsigned flags,
> @@ -192,8 +210,17 @@ ssize_t qemu_net_queue_send(NetQueue *queue,
> {
> ssize_t ret;
>
> -if (queue->delivering || !qemu_can_send_packet_nolock(sender)) {
> +if (queue->delivering || !qemu_can_send_packet_nolock(sender)
> +|| sender->send_queue->delivering) {
> qemu_net_queue_append(queue, sender, flags, data, size, sent_cb);
> +/* Nested call will be deferred to BH */
> +if (sender->send_queue->delivering) {
> +NetQueBH *que_bh = g_slice_new(NetQueBH);
> +que_bh->bh = qemu_bh_new(qemu_net_queue_send_bh, que_bh);
> +
Re: [Qemu-devel] Wayland support
I don't known if it's what you want, but wayland have a beta spice backend http://www.phoronix.com/scan.php?page=news_item&px=MTM1NzQ - Mail original - De: "Max A Yu" À: qemu-devel@nongnu.org Envoyé: Mardi 2 Juillet 2013 05:36:50 Objet: [Qemu-devel] Wayland support Hi, Can QEMU support wayland? I’d like to know if anybody have ideas/solutions on that. Thanks, Max
[Qemu-devel] moving CHR_EVENT_OPEN out of BH broke echo for muxed mon
Before commit bd5c51ee6c4f1c79cae5ad2516d711a27b4ea8ec Author: Michael Roth Date: Fri Jun 7 15:19:53 2013 -0500 qemu-char: don't issue CHR_EVENT_OPEN in a BH we had no echo by default with -nographic, and it gave the prompt when switching to monitor: $ qemu-system-x86_64 -nographic _ (nothing is on the screen except the command line) (Ctrl+A, c) QEMU 1.5.0 monitor - type 'help' for more information (qemu) _ After this patch, we now have: $ qemu-system-x86_64 -nographic QEMU 1.5.0 monitor - type 'help' for more information (qemu) _ (note the cursor position on the _next_ line after the prompt), and the system is actually in "serial port" mode, not monitor mode (you still need to hit Ctrl+A,c to switch to monitor). Thanks, /mjt
Re: [Qemu-devel] [PATCH 04/30] add a header file for atomic operations
Il 03/07/2013 04:24, liu ping fan ha scritto: >> > +- atomic operations in Linux are always on a 32-bit int type and > 32-bit? int should be the integer type that the target processor is > most efficient working with. Has Linux ever been ported to a machine where sizeof(int) != 4? (Honest question. I don't know). Paolo
Re: [Qemu-devel] [PATCH 3/7] nbd: use BDS refcount
Il 03/07/2013 03:10, Fam Zheng ha scritto: > > This has the important side effect of marking the exported disk as > > "in_use" (to use the terms before the series). Right now you can serve > > a disk and, at the same time, stream it or mirror it or create a live > > snapshot of it. > > > > Do we really want to block anything for a device being served? Perhaps > > truncation, but maybe not even that. The NBD server is meant to be as > > unobtrusive as possible (in some sense NBD accesses are the same as > > guest accesses). > > OK, it is better to work like that. But I don't quite understand why was > there drive_get_ref() on the device (w/o the series), as there's already > a close notifier? And it just drive_put_ref() when bs is closed? The close notifier runs when the user invokes a drive_del or eject command from the monitor. The drive_get_ref/drive_put_ref delays the bdrv_delete until after nbd.c has cleaned up all the connections. Paolo
Re: [Qemu-devel] [PATCH] full introspection support for QMP
Il 02/07/2013 22:58, Anthony Liguori ha scritto: > > > We consume the schema in QEMU. No reason for us to consume it in a > > > different format than libvirt. > > > > One reason could be that qapi-schema.json, as written, lacks a schema > > that can be expressed itself using QAPI. > > Yup, but how much does that matter in practice? It matters little because we do not provide a library of QAPI parsers/visitors, so clients have to invent their own anyway. But if we did, clients would be completely oblivious of the fact that QMP is based on JSON. Sending qapi-schema.json down the wire as a JSON string would break the abstraction that we provide to the clients. > At any rate, if we wanted to solve this problem--a self-describing > schema--we should do it in qapi-schema.json too. I disagree. I also disagree that qapi-schema.json, as written, is a format designed for machine consumption. So, qapi-schema.json has to be readable/writable _mostly_ by humans. That it is valid JSON is little more than a curious accident, because overall the syntax greatly favors humans rather than computers. A format designed for computers would have a schema such that no parsing tasks (however small---I'm thinking of the "list of" and "optional" syntaxes) would be left after parsing the JSON. The example that Eric sent is not something that I would find easy to read/write. qapi-schema.json instead is more than acceptable. Paolo
Re: [Qemu-devel] [PATCHv2 02/11] iscsi: read unmap info from block limits vpd page
max_unmap :
If the target does not return an explicit limit for max_unmap it will
return 0x which means "no limit".
I think you should add a check for max_unmap and clamp it down to
something sane.
Maybe a maximum of 128M ?
Same for bdc, that should also be checked and clamped down to sane values.
On Thu, Jun 27, 2013 at 11:11 PM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven
> ---
> block/iscsi.c | 80
> -
> 1 file changed, 56 insertions(+), 24 deletions(-)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index a38a1bf..2e2455d 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -54,6 +54,8 @@ typedef struct IscsiLun {
> uint8_t lbpu;
> uint8_t lbpws;
> uint8_t lbpws10;
> +uint32_t max_unmap;
> +uint32_t max_unmap_bdc;
> } IscsiLun;
>
> typedef struct IscsiAIOCB {
> @@ -1007,6 +1009,37 @@ static QemuOptsList runtime_opts = {
> },
> };
>
> +static struct scsi_task *iscsi_do_inquiry(struct iscsi_context *iscsi,
> + int lun, int evpd, int pc) {
> +int full_size;
> +struct scsi_task *task = NULL;
> +task = iscsi_inquiry_sync(iscsi, lun, evpd, pc, 64);
> +if (task == NULL || task->status != SCSI_STATUS_GOOD) {
> +goto fail;
> +}
> +full_size = scsi_datain_getfullsize(task);
> +if (full_size > task->datain.size) {
> +scsi_free_scsi_task(task);
> +
> +/* we need more data for the full list */
> +task = iscsi_inquiry_sync(iscsi, lun, evpd, pc, full_size);
> +if (task == NULL || task->status != SCSI_STATUS_GOOD) {
> +goto fail;
> +}
> +}
> +
> +return task;
> +
> +fail:
> +error_report("iSCSI: Inquiry command failed : %s",
> + iscsi_get_error(iscsi));
> +if (task) {
> +scsi_free_scsi_task(task);
> +return NULL;
> +}
> +return NULL;
> +}
> +
> /*
> * We support iscsi url's on the form
> * iscsi://[%@][:]//
> @@ -1139,33 +1172,12 @@ static int iscsi_open(BlockDriverState *bs, QDict
> *options, int flags)
>
> if (iscsilun->lbpme) {
> struct scsi_inquiry_logical_block_provisioning *inq_lbp;
> -int full_size;
> -
> -task = iscsi_inquiry_sync(iscsi, iscsilun->lun, 1,
> -
> SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING,
> - 64);
> -if (task == NULL || task->status != SCSI_STATUS_GOOD) {
> -error_report("iSCSI: Inquiry command failed : %s",
> - iscsi_get_error(iscsilun->iscsi));
> +task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1,
> +
> SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING);
> +if (task == NULL) {
> ret = -EINVAL;
> goto out;
> }
> -full_size = scsi_datain_getfullsize(task);
> -if (full_size > task->datain.size) {
> -scsi_free_scsi_task(task);
> -
> -/* we need more data for the full list */
> -task = iscsi_inquiry_sync(iscsi, iscsilun->lun, 1,
> -
> SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING,
> - full_size);
> -if (task == NULL || task->status != SCSI_STATUS_GOOD) {
> -error_report("iSCSI: Inquiry command failed : %s",
> - iscsi_get_error(iscsilun->iscsi));
> -ret = -EINVAL;
> -goto out;
> -}
> -}
> -
> inq_lbp = scsi_datain_unmarshall(task);
> if (inq_lbp == NULL) {
> error_report("iSCSI: failed to unmarshall inquiry datain blob");
> @@ -1175,6 +1187,26 @@ static int iscsi_open(BlockDriverState *bs, QDict
> *options, int flags)
> iscsilun->lbpu = inq_lbp->lbpu;
> iscsilun->lbpws = inq_lbp->lbpws;
> iscsilun->lbpws10 = inq_lbp->lbpws10;
> +scsi_free_scsi_task(task);
> +task = NULL;
> +}
> +
> +if (iscsilun->lbpu) {
> +struct scsi_inquiry_block_limits *inq_bl;
> +task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1,
> +SCSI_INQUIRY_PAGECODE_BLOCK_LIMITS);
> +if (task == NULL) {
> +ret = -EINVAL;
> +goto out;
> +}
> +inq_bl = scsi_datain_unmarshall(task);
> +if (inq_bl == NULL) {
> +error_report("iSCSI: failed to unmarshall inquiry datain blob");
> +ret = -EINVAL;
> +goto out;
> +}
> +iscsilun->max_unmap = inq_bl->max_unmap;
> +iscsilun->max_unmap_bdc = inq_bl->max_unmap_bdc;
> }
>
> #if defined(LIBISCSI_FEATURE_NOP_COUNTER)
> --
> 1.7.9.5
>
Re: [Qemu-devel] [PATCH v2 3/6] net: make netclient re-entrant with refcnt
On Mon, Jul 1, 2013 at 7:50 PM, Stefan Hajnoczi wrote: > On Thu, Jun 20, 2013 at 05:14:56PM +0800, liu ping fan wrote: >> On Tue, Jun 18, 2013 at 8:41 PM, Stefan Hajnoczi wrote: >> > On Thu, Jun 13, 2013 at 05:03:03PM +0800, Liu Ping Fan wrote: >> >> @@ -1109,6 +1146,7 @@ void net_cleanup(void) >> >> qemu_del_net_client(nc); >> >> } >> >> } >> >> +qemu_mutex_destroy(&net_clients_lock); >> > >> > Why is it okay to iterate over net_clients here without the lock? >> >> atexit(&net_cleanup); So no other racers exist. > > What about dataplane? The device may not be reset when net_cleanup runs. > Does the func registered by atexit run after all of the other threads terminate? > It's best not to make assumptions, taking the lock is easy. > Yes, assumptions are not reliable. I will take the lock for the next version. Thx & regards, Pingfan > Stefan
Re: [Qemu-devel] [PATCH 6/9] vhost-scsi: new device supporting the tcm_vhost Linux kernel module
On Fri, Jun 21, 2013 at 10:16:48AM +, Libaiqing wrote: > Hi Asias, > > > -Original Message- > > From: Asias He [mailto:as...@redhat.com] > > Sent: Thursday, June 20, 2013 5:39 PM > > To: Libaiqing > > Cc: Paolo Bonzini; Wenchao Xia; qemu-devel@nongnu.org; > > n...@linux-iscsi.org; Michael S. Tsirkin; Haofeng > > Subject: Re: [Qemu-devel] [PATCH 6/9] vhost-scsi: new device supporting the > > tcm_vhost Linux kernel module > > > > On Thu, Jun 20, 2013 at 08:49:50AM +, Libaiqing wrote: > > > Hi Asias, > > > Thanks for your config. > > > According to you config,I test booting from vhost device with > > upstream kernel and qemu,but failed. > > > > > > 1 installing guest from cdrom,ok. > > > 2 booting vhost-scsi,guest fs error occurs. > > > 3 using fileio backstores,the error is same.. > > > 4 rebooting guest,a log printed: > > > (qemu) hw/scsi/virtio-scsi.c:533:virtio_scsi_handle_event: Object > > 0x7fccae7f2c88 is not an instance of type virtio-scsi-device > > > > Paolo, I remember you fixed a similar issue? > > > > > 5 using upstream seabios,core dumped. > > > > > > Could you give me some advise to debug this problem ? I can provide > > more information if need. > > > > Can you show me qemu commit id you used? Can you verity that if using the > > host kernel for guest helps? Does booting directly (without the install > > and reboot process) work? > Qemu commit id is "commit 4eda32f588086b6cd0ec2be6a7a6c131f8c2b427". > > Guest kernel updateing is useless. > > Booting directly doesn't work too. Hello Libaiqing, Sorry for the delay. I will try to reproduce it myself. > > > > The qemu cmd: > > > [root@fedora121 x86_64-softmmu]# ./qemu-system-x86_64 -enable-kvm > > -name fedora -M pc -m 1024 -smp 2 -drive > > file=/home/fedora18.iso,if=ide,media=cdrom -device > > vhost-scsi-pci,wwpn=naa.50014057133e25dc -monitor stdio -vga qxl > > -vnc :1 > > > > > > The vnc output: > > > Dracut-initqueue[189]:/dev/mapper/fedora-root:UNEXPECTED > > INCONSISTENCY;RUN FSCK MANUALLY. > > > Dracut-initqueue[189]: Warning: e2fsck returned with 4 > > > Dracut-initqueue[189]: Warning: ***An error occurred during the file > > system check. > > > > > > The guest kernel log: > > > Kernel: virtio-pci :00:04.0: irq 40 for MSI/MSI-X > > > Kernel: virtio-pci :00:04.0: irq 41 for MSI/MSI-X > > > Kernel: virtio-pci :00:04.0: irq 42 for MSI/MSI-X > > > Kernel: virtio-pci :00:04.0: irq 43 for MSI/MSI-X > > > Kernel: scsi2 : Virtio SCSI HBA > > > Kernel: scsi 2:0:1:0: Direct-Access LIO-ORG r0 > > > Kernel: sd 2:0:1:0: Attached scsi generic sg1 type 0 > > > Kernel: sd 2:0:1:0: [sda]1258912 512-byte logical . > > > Kernel: sd 2:0:1:0: [sda]write protect is off > > > Kernel: sd 2:0:1:0: [sda]Mode sense :43 00 00 08 > > > Kernel: sd 2:0:1:0: [sda]write cache: disabled, read . > > > Kernel: sda sda1 sda2 > > > Kernel: sd 2:0:1:0: [sda] Attached SCSI disk > > > Dracut-initqueue[189]: Scanning devices sda2 for LVM > > > Dracut-initqueue[189]: inactive '/dev/fedora/swap'... > > > Dracut-initqueue[189]: inactive '/dev/fedora/root'... > > > > > > The info of host: > > > [root@fedora121 x86_64-softmmu]# uname -a > > > Linux fedora121 3.10.0-rc6 #1 SMP Wed Jun 19 19:34:24 CST 2013 x86_64 > > x86_64 x86_64 GNU/Linux > > > [root@fedora121 x86_64-softmmu]# lsmod |grep vhost_scsi > > > vhost_scsi 49456 5 > > > target_core_mod 282163 14 > > target_core_iblock,target_core_pscsi,iscsi_target_mod,target_core_file,vh > > ost_scsi > > > [root@fedora121 x86_64-softmmu]# targetcli > > > targetcli shell version v2.1.fb26 > > > Copyright 2011 by RisingTide Systems LLC and others. > > > For help on commands, type 'help'. > > > > > > /> ls > > > o- > > / > > .. > > ... [...] > > > o- > > backstores > > ... > > ... [...] > > > | o- > > block > > .. > > [Storage Objects: 0] > > > | o- > > fileio > > . > > [Storage Objects: 0] > > > | o- > > pscsi > > .. > > [Storage Objects: 0] > > > | o- > > ramdisk > > > > [Storage Objects: 1] > > > | o- > > r0 > > ... > > [(6.0GiB) activated] > > > o- > > iscsi > > . > > ... [Targets: 0] > > > o- > > loopback > >
Re: [Qemu-devel] [PATCH 6/9] vhost-scsi: new device supporting the tcm_vhost Linux kernel module
On Wed, Jul 03, 2013 at 03:08:34AM +, Libaiqing wrote: > Hi asias, >I'm testing vhost-blk,for comparimg the performance with virtio-blk. >I got the kernel patch from this mail: https://lkml.org/lkml/2012/12/1/174 You can find the latest vhost-blk kernel bits here: git://github.com/asias/linux.git blk.vhost-blk >And I got the userspace code for qemu from this git: > https://github.com/asias/qemu/tree/blk.vhost-blk; >Used vhost-blk device as non-bootable device,the configuration is : > Qemu-kvm -enable-kvm -name win7 -M pc-0.15 -m 1024 -smp 2 -boot c > -device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 -drive > file=/home/fedora188.img,if=virtio,index=0,format=raw -drive > file=/home/fedora18.img,if=virtio,index=1,format=raw,id=hd,vhost=on > -monitor stdio -vga qxl -vnc :1 -device usb-tablet,id=input0 Please use raw block device for vhost-blk. >On fedora17 host,I used kernel 3.6.3 for building vhos_blk module,but the > vm will hang when vm starting. > >1 Is there any new patch for kernel 3.8 3.10? >2 which version of the kernel is suitable for the current kernel patch? > >Could you give me some advise to debug this problem ? I can provide more > information if need. >Or could you give me some advise to run vhost-blk,such as RHEL > version,kernel version and so on? I recommend you to use the kernel I provided above for both guest and host. > Thanks > Baiqing. -- Asias
[Qemu-devel] [PATCH 6/9] vhost-scsi: new device supporting the tcm_vhost Linux kernel module
Hi asias, I'm testing vhost-blk,for comparimg the performance with virtio-blk. I got the kernel patch from this mail: https://lkml.org/lkml/2012/12/1/174 And I got the userspace code for qemu from this git: https://github.com/asias/qemu/tree/blk.vhost-blk; Used vhost-blk device as non-bootable device,the configuration is : Qemu-kvm -enable-kvm -name win7 -M pc-0.15 -m 1024 -smp 2 -boot c -device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 -drive file=/home/fedora188.img,if=virtio,index=0,format=raw -drive file=/home/fedora18.img,if=virtio,index=1,format=raw,id=hd,vhost=on -monitor stdio -vga qxl -vnc :1 -device usb-tablet,id=input0 On fedora17 host,I used kernel 3.6.3 for building vhos_blk module,but the vm will hang when vm starting. 1 Is there any new patch for kernel 3.8 3.10? 2 which version of the kernel is suitable for the current kernel patch? Could you give me some advise to debug this problem ? I can provide more information if need. Or could you give me some advise to run vhost-blk,such as RHEL version,kernel version and so on? Thanks Baiqing.
Re: [Qemu-devel] [PATCH 04/30] add a header file for atomic operations
On Sat, Jun 29, 2013 at 2:26 AM, Paolo Bonzini wrote: > We're already using them in several places, but __sync builtins are just > too ugly to type, and do not provide seqcst load/store operations. > > Signed-off-by: Paolo Bonzini > --- > docs/atomics.txt | 345 > +++ > hw/display/qxl.c | 3 +- > hw/virtio/vhost.c| 9 +- > include/qemu/atomic.h| 190 +- > migration.c | 3 +- > tests/test-thread-pool.c | 8 +- > 6 files changed, 514 insertions(+), 44 deletions(-) > create mode 100644 docs/atomics.txt > > diff --git a/docs/atomics.txt b/docs/atomics.txt > new file mode 100644 > index 000..e2ce04b > --- /dev/null > +++ b/docs/atomics.txt > @@ -0,0 +1,345 @@ > +CPUs perform independent memory operations effectively in random order. > +but this can be a problem for CPU-CPU interaction (including interactions > +between QEMU and the guest). Multi-threaded programs use various tools > +to instruct the compiler and the CPU to restrict the order to something > +that is consistent with the expectations of the programmer. > + > +The most basic tool is locking. Mutexes, condition variables and > +semaphores are used in QEMU, and should be the default approach to > +synchronization. Anything else is considerably harder, but it's > +also justified more often than one would like. The two tools that > +are provided by qemu/atomic.h are memory barriers and atomic operations. > + > +Macros defined by qemu/atomic.h fall in three camps: > + > +- compiler barriers: barrier(); > + > +- weak atomic access and manual memory barriers: atomic_read(), > + atomic_set(), smp_rmb(), smp_wmb(), smp_mb(), smp_read_barrier_depends(); > + > +- sequentially consistent atomic access: everything else. > + > + > +COMPILER MEMORY BARRIER > +=== > + > +barrier() prevents the compiler from moving the memory accesses either > +side of it to the other side. The compiler barrier has no direct effect > +on the CPU, which may then reorder things however it wishes. > + > +barrier() is mostly used within qemu/atomic.h itself. On some > +architectures, CPU guarantees are strong enough that blocking compiler > +optimizations already ensures the correct order of execution. In this > +case, qemu/atomic.h will reduce stronger memory barriers to simple > +compiler barriers. > + > +Still, barrier() can be useful when writing code that can be interrupted > +by signal handlers. > + > + > +SEQUENTIALLY CONSISTENT ATOMIC ACCESS > += > + > +Most of the operations in the qemu/atomic.h header ensure *sequential > +consistency*, where "the result of any execution is the same as if the > +operations of all the processors were executed in some sequential order, > +and the operations of each individual processor appear in this sequence > +in the order specified by its program". > + > +qemu/atomic.h provides the following set of atomic read-modify-write > +operations: > + > +typeof(*ptr) atomic_inc(ptr) > +typeof(*ptr) atomic_dec(ptr) > +typeof(*ptr) atomic_add(ptr, val) > +typeof(*ptr) atomic_sub(ptr, val) > +typeof(*ptr) atomic_and(ptr, val) > +typeof(*ptr) atomic_or(ptr, val) > +typeof(*ptr) atomic_xchg(ptr, val > +typeof(*ptr) atomic_cmpxchg(ptr, old, new) > + > +all of which return the old value of *ptr. These operations are > +polymorphic; they operate on any type that is as wide as an int. > + > +Sequentially consistent loads and stores can be done using: > + > +atomic_add(ptr, 0) for loads > +atomic_xchg(ptr, val) for stores > + > +However, they are quite expensive on some platforms, notably POWER and > +ARM. Therefore, qemu/atomic.h provides two primitives with slightly > +weaker constraints: > + > +typeof(*ptr) atomic_mb_read(ptr) > +void atomic_mb_set(ptr, val) > + > +The semantics of these primitives map to Java volatile variables, > +and are strongly related to memory barriers as used in the Linux > +kernel (see below). > + > +As long as you use atomic_mb_read and atomic_mb_set, accesses cannot > +be reordered with each other, and it is also not possible to reorder > +"normal" accesses around them. > + > +However, and this is the important difference between > +atomic_mb_read/atomic_mb_set and sequential consistency, it is important > +for both threads to access the same volatile variable. It is not the > +case that everything visible to thread A when it writes volatile field f > +becomes visible to thread B after it reads volatile field g. The store > +and load have to "match" (i.e., be performed on the same volatile > +field) to achieve the right semantics. > + > + > +These operations operate on any type that is as wide as an int or smaller. > + > + > +WEAK ATOMIC ACCESS AND MANUAL MEMORY BARRIERS > += > + > +Compared to sequentially consistent atomic access, programmin
[Qemu-devel] 回复： Re: Which part of qemu responds to ACPI control method?
Take the method “_PTS” for example, how could I know how it access a certain hardware, and what hardware it accesses? I am a newbie in this field, thanks in advance;) 已从三星手机发送 原始邮件 发件人： Laszlo Ersek 日期: 2013-07-02 23:57 (GMT+08:00) 收件人： bobooscar 抄送： qemu-devel@nongnu.org 主题： Re: [Qemu-devel] Which part of qemu responds to ACPI control method? On 07/02/13 15:05, bobooscar wrote: > > Hi,all: > When a guest domain excutes a control method, such as “_PTS”, which > part of qemu would respond and handle the request? thank you in advance. IMHO, like any other method, _PTS (Prepare To Sleep) is executed by the guest's ACPI interpreter, and qemu responds only to the hardware accesses that the method makes. Laszlo
Re: [Qemu-devel] [PATCH V3 0/9] add internal snapshot support at block device level
Any comments for this version? >This series brings internal snapshot support at block devices level, now we > have two three methods to do block snapshot lively: 1) backing chain, > 2) internal one and 3) drive-back up approach. > > Comparation: > Advantages:Disadvantages: > 1)delta data, taken fast, export, sizeperformance, delete slow. > 2) taken fast, delete fast, performance, size delta data, format > 3) performance, export, format taken slow, delta data, size > >I think in most case, saving vmstate in an standalone file is better than > saving it inside qcow2, So suggest treat internal snapshot as block level > methods and not encourage user to savevm in qcow2 any more. > > Implemention details: >To avoid trouble, this serial have hide ID in create interfaces, this make > sure no chaos of ID and name will be introduced by these interfaces. >There is one patch may be common to Pavel's savvm transaction, patch 1/11, > others are not quite related. Patch 1/11 will not set errp when no snapshot > find, since patch 3/11 need to distinguish real error case. > > Next steps to better full VM snapshot: >Improve internal snapshot's export capability. >Better vmstate saving. > >Thanks Kevin to give advisement about how add it in qmp_transaction, oldest > version comes drom Dietmar Maurer. > > v3: >General: >Rebased after Stenfan's driver-backup patch V6. > >Address Eric's comments: >4/9: grammar fix and better doc. >5/9: parameter name is mandatory now. grammar fix. >6/9: redesiged interface: take both id and name as optional parameter, > return > the deleted snapshot's info. > >Address Stefan's comments: >4/9: add '' around %s in message. drop code comments about vm_clock. >9/9: better doc, refined the code and add more test case. > > > Wenchao Xia (9): >1 snapshot: new function bdrv_snapshot_find_by_id_and_name() >2 snapshot: add paired functions for internal snapshot id and name >3 snapshot: distinguish id and name in snapshot delete >4 qmp: add internal snapshot support in qmp_transaction >5 qmp: add interface blockdev-snapshot-internal-sync >6 qmp: add interface blockdev-snapshot-delete-internal-sync >7 hmp: add interface hmp_snapshot_blkdev_internal >8 hmp: add interface hmp_snapshot_delete_blkdev_internal >9 qemu-iotests: add 056 internal snapshot for block device test case > > block/qcow2-snapshot.c | 68 +--- > block/qcow2.h |5 +- > block/rbd.c| 23 - > block/sheepdog.c |5 +- > block/snapshot.c | 130 +- > blockdev.c | 191 +++ > hmp-commands.hx| 37 ++- > hmp.c | 22 > hmp.h |2 + > include/block/block_int.h |5 +- > include/block/snapshot.h | 14 ++- > include/qemu-common.h |3 + > qapi-schema.json | 67 +++- > qemu-img.c |5 +- > qmp-commands.hx| 104 - > savevm.c | 10 ++- > tests/qemu-iotests/056 | 267 > > tests/qemu-iotests/056.out |5 + > tests/qemu-iotests/group |1 + > 19 files changed, 926 insertions(+), 38 deletions(-) > create mode 100755 tests/qemu-iotests/056 > create mode 100644 tests/qemu-iotests/056.out > > -- Best Regards Wenchao Xia
Re: [Qemu-devel] [PATCH v2 02/26] ohci: QOM'ify some more
Am 01.07.2013 12:18, schrieb Hu Tao:
> Introduce type constant and avoid DO_UPCAST().
>
> Signed-off-by: Hu Tao
> ---
> hw/usb/hcd-ohci.c | 20
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
> index 5513924..912255d 100644
> --- a/hw/usb/hcd-ohci.c
> +++ b/hw/usb/hcd-ohci.c
> @@ -1842,6 +1842,8 @@ static int usb_ohci_init(OHCIState *ohci, DeviceState
> *dev,
> return 0;
> }
>
> +#define TYPE_PCI_OHCI "pci-ohci"
> +#define PCI_OHCI(obj) OBJECT_CHECK(OHCIPCIState, (obj), TYPE_PCI_OHCI)
I added a while line here and converted the last three remaining uses of
pci_dev in the initfn so that both fields could be renamed to parent_obj
as proof of complete conversion.
Thanks, queued this and the following one on qom-next:
https://github.com/afaerber/qemu-cpu/commits/qom-next
There was an overlap with Peter's usb/* PCIDevice patch, I believe this
one is a superset of the ohci part.
Andreas
> typedef struct {
> PCIDevice pci_dev;
> OHCIState state;
> @@ -1852,23 +1854,25 @@ typedef struct {
>
> static int usb_ohci_initfn_pci(struct PCIDevice *dev)
> {
> -OHCIPCIState *ohci = DO_UPCAST(OHCIPCIState, pci_dev, dev);
> +OHCIPCIState *ohci = PCI_OHCI(dev);
>
> ohci->pci_dev.config[PCI_CLASS_PROG] = 0x10; /* OHCI */
> ohci->pci_dev.config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin A */
>
> -if (usb_ohci_init(&ohci->state, &dev->qdev, ohci->num_ports, 0,
> +if (usb_ohci_init(&ohci->state, DEVICE(dev), ohci->num_ports, 0,
>ohci->masterbus, ohci->firstport,
>pci_get_address_space(dev)) != 0) {
> return -1;
> }
> ohci->state.irq = ohci->pci_dev.irq[0];
>
> -/* TODO: avoid cast below by using dev */
> -pci_register_bar(&ohci->pci_dev, 0, 0, &ohci->state.mem);
> +pci_register_bar(dev, 0, 0, &ohci->state.mem);
> return 0;
> }
>
> +#define TYPE_SYSBUS_OHCI "sysbus-ohci"
> +#define SYSBUS_OHCI(obj) OBJECT_CHECK(OHCISysBusState, (obj),
> TYPE_SYSBUS_OHCI)
> +
> typedef struct {
> SysBusDevice busdev;
> OHCIState ohci;
> @@ -1878,10 +1882,10 @@ typedef struct {
>
> static int ohci_init_pxa(SysBusDevice *dev)
> {
> -OHCISysBusState *s = FROM_SYSBUS(OHCISysBusState, dev);
> +OHCISysBusState *s = SYSBUS_OHCI(dev);
>
> /* Cannot fail as we pass NULL for masterbus */
> -usb_ohci_init(&s->ohci, &dev->qdev, s->num_ports, s->dma_offset, NULL, 0,
> +usb_ohci_init(&s->ohci, DEVICE(dev), s->num_ports, s->dma_offset, NULL,
> 0,
>&address_space_memory);
> sysbus_init_irq(dev, &s->ohci.irq);
> sysbus_init_mmio(dev, &s->ohci.mem);
> @@ -1911,7 +1915,7 @@ static void ohci_pci_class_init(ObjectClass *klass,
> void *data)
> }
>
> static const TypeInfo ohci_pci_info = {
> -.name = "pci-ohci",
> +.name = TYPE_PCI_OHCI,
> .parent= TYPE_PCI_DEVICE,
> .instance_size = sizeof(OHCIPCIState),
> .class_init= ohci_pci_class_init,
> @@ -1934,7 +1938,7 @@ static void ohci_sysbus_class_init(ObjectClass *klass,
> void *data)
> }
>
> static const TypeInfo ohci_sysbus_info = {
> -.name = "sysbus-ohci",
> +.name = TYPE_SYSBUS_OHCI,
> .parent= TYPE_SYS_BUS_DEVICE,
> .instance_size = sizeof(OHCISysBusState),
> .class_init= ohci_sysbus_class_init,
>
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] help me for nahanni device
Hello Cam, I am honored that receive your reply. 1 the QEMU version is showed as following, the version is 1.0.50 [root@node1 sh-vm]# /opt/qemu-kvm-rt/x86_64-softmmu/qemu-system-x86_64 -version QEMU emulator version 1.0.50 (qemu-kvm-devel), Copyright (c) 2003-2008 Fabrice Bellard [root@node1 sh-vm]# 2 ”QEMU/KVM doesn't work” My mean is that qemu has been start, but I can’t log on VM or ping VM IP. When only attach a nahanni device, all are ok. Thank you. Dai weibin From: c...@ualberta.ca [mailto:c...@ualberta.ca] On Behalf Of Cam Macdonell Sent: 2013年7月3日 0:20 To: DAI Weibin Cc: qemu-devel@nongnu.org Subject: Re: [Qemu-devel] help me for nahanni device Hello Dai, Which qemu version you are using? When you say QEMU/KVM "doesn't work", do you mean it does not start? Cam On Mon, Jul 1, 2013 at 12:39 AM, DAI Weibin mailto:weibin@alcatel-sbell.com.cn>> wrote: Hi all, When use nahanni device as following, The QEMU/KVM doesn’t work. I attach two ivshmem devices to VM, QEMU/KVM only supports a ivshmem device, why? Expect your reply! -device virtio-net-pci,vlan=0,id=net0,mac=DE:AD:BE:09:03:01,bus=pci.0,addr=0x5 \ -net tap,script=/etc/qemu-ifup \ -device ivshmem,size=256m,shm=l1shm \ -device ivshmem,size=256m,shm=l2shm \ -usbdevice tablet \ -vnc :2 \ -daemonize Best regards 戴卫彬
Re: [Qemu-devel] [PATCH v2 01/26] sysbus: document SysBusDeviceClass about @init
On Wed, Jul 03, 2013 at 03:19:53AM +0200, Andreas Färber wrote:
> Am 01.07.2013 12:18, schrieb Hu Tao:
> > Signed-off-by: Hu Tao
> > ---
> > include/hw/sysbus.h | 10 ++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
> > index 7c2e316..9614758 100644
> > --- a/include/hw/sysbus.h
> > +++ b/include/hw/sysbus.h
> > @@ -23,6 +23,16 @@ typedef struct SysBusDevice SysBusDevice;
> > #define SYS_BUS_DEVICE_GET_CLASS(obj) \
> > OBJECT_GET_CLASS(SysBusDeviceClass, (obj), TYPE_SYS_BUS_DEVICE)
> >
> > +/*
> > + * SysBusDeviceClass:
> > + * @parent_class: This is private
>
> Because it is private, the usual way is to suppress its documentation
> below via private/public markers.
>
> > + * @init: Callback function invoked when the #DeviceState::realized
> > property
> > + * is changed to %true. Deprecated, new types inheriting directly from
> > + * TYPE_SYS_BUS_DEVICE should use #DeviceClass::realize instead, new leaf
> > + * types should consult their respective parent type. SysBusDeviceClass is
> > + * not implementing #DeviceClass::realize, so deriving classes can simply
> > + * ignore it.
> > + */
> > typedef struct SysBusDeviceClass {
> > DeviceClass parent_class;
> >
>
> Thanks for documenting this. I'm queuing it with the following gtk-doc
> syntax and contential modifications, please let me know if you wish for
> any wording changes.
Thanks for the changes, looks OK to me.
>
> Andreas
>
> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
> index 9614758..8c17165 100644
> --- a/include/hw/sysbus.h
> +++ b/include/hw/sysbus.h
> @@ -23,18 +23,20 @@ typedef struct SysBusDevice SysBusDevice;
> #define SYS_BUS_DEVICE_GET_CLASS(obj) \
> OBJECT_GET_CLASS(SysBusDeviceClass, (obj), TYPE_SYS_BUS_DEVICE)
>
> -/*
> +/**
> * SysBusDeviceClass:
> - * @parent_class: This is private
> - * @init: Callback function invoked when the #DeviceState::realized
> property
> + * @init: Callback function invoked when the #DeviceState.realized property
> * is changed to %true. Deprecated, new types inheriting directly from
> - * TYPE_SYS_BUS_DEVICE should use #DeviceClass::realize instead, new leaf
> - * types should consult their respective parent type. SysBusDeviceClass is
> - * not implementing #DeviceClass::realize, so deriving classes can simply
> - * ignore it.
> + * TYPE_SYS_BUS_DEVICE should use #DeviceClass.realize instead, new leaf
> + * types should consult their respective parent type.
> + *
> + * SysBusDeviceClass is not overriding #DeviceClass.realize, so derived
> + * classes overriding it are not required to invoke its implementation.
> */
> typedef struct SysBusDeviceClass {
> +/*< private >*/
> DeviceClass parent_class;
> +/*< public >*/
>
> int (*init)(SysBusDevice *dev);
> } SysBusDeviceClass;
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH] qom: Use atomics for object refcounting
On Wed, Jul 3, 2013 at 12:36 AM, Anthony Liguori wrote:
> Paolo Bonzini writes:
>
>> Il 02/07/2013 16:47, Anthony Liguori ha scritto:
>>> Jan Kiszka writes:
>>>
Objects can soon be referenced/dereference outside the BQL. So we need
to use atomics in object_ref/unref.
Based on patch by Liu Ping Fan.
Signed-off-by: Jan Kiszka
---
qom/object.c |5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/qom/object.c b/qom/object.c
index 803b94b..a76a30b 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -683,16 +683,15 @@ GSList *object_class_get_list(const char
*implements_type,
void object_ref(Object *obj)
{
-obj->ref++;
+ __sync_fetch_and_add(&obj->ref, 1);
}
void object_unref(Object *obj)
{
g_assert(obj->ref > 0);
-obj->ref--;
/* parent always holds a reference to its children */
-if (obj->ref == 0) {
+if (__sync_sub_and_fetch(&obj->ref, 1) == 0) {
object_finalize(obj);
}
}
>>>
>>> Should we introduce something akin to kref now that referencing counting
>>> has gotten fancy?
>>
>> I'm not a big fan of kref (it seems _too_ thin a wrapper to me, i.e. it
>> doesn't really wrap enough to be useful), but I wouldn't oppose it if
>> someone else does it.
>
> I had honestly hoped Object was light enough to be used for this
> purpose. What do you think?
>
I think it is a good idea. So we can consider the object_finalize() as
the place to release everything. Take the DeviceState as example, we
will have
-- >8 --
Subject: [PATCH] qom: delay DeviceState destructor until object finialize
Until refcnt->0, we know that the DeviceState can be safely dropped,
so put the destructor there.
Signed-off-by: Liu Ping Fan
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 6985ad8..1f4e5d8 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -794,9 +794,7 @@ static void device_unparent(Object *obj)
bus = QLIST_FIRST(&dev->child_bus);
qbus_free(bus);
}
-if (dev->realized) {
-object_property_set_bool(obj, false, "realized", NULL);
-}
+
if (dev->parent_bus) {
bus_remove_child(dev->parent_bus, dev);
object_unref(OBJECT(dev->parent_bus));
diff --git a/qom/object.c b/qom/object.c
index 803b94b..2c945f0 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -393,6 +393,7 @@ static void object_finalize(void *data)
Object *obj = data;
TypeImpl *ti = obj->class->type;
+object_property_set_bool(obj, false, "realized", NULL);
object_deinit(obj, ti);
object_property_del_all(obj);
Re: [Qemu-devel] [PATCH v2 01/26] sysbus: document SysBusDeviceClass about @init
Am 01.07.2013 12:18, schrieb Hu Tao:
> Signed-off-by: Hu Tao
> ---
> include/hw/sysbus.h | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
> index 7c2e316..9614758 100644
> --- a/include/hw/sysbus.h
> +++ b/include/hw/sysbus.h
> @@ -23,6 +23,16 @@ typedef struct SysBusDevice SysBusDevice;
> #define SYS_BUS_DEVICE_GET_CLASS(obj) \
> OBJECT_GET_CLASS(SysBusDeviceClass, (obj), TYPE_SYS_BUS_DEVICE)
>
> +/*
> + * SysBusDeviceClass:
> + * @parent_class: This is private
Because it is private, the usual way is to suppress its documentation
below via private/public markers.
> + * @init: Callback function invoked when the #DeviceState::realized property
> + * is changed to %true. Deprecated, new types inheriting directly from
> + * TYPE_SYS_BUS_DEVICE should use #DeviceClass::realize instead, new leaf
> + * types should consult their respective parent type. SysBusDeviceClass is
> + * not implementing #DeviceClass::realize, so deriving classes can simply
> + * ignore it.
> + */
> typedef struct SysBusDeviceClass {
> DeviceClass parent_class;
>
Thanks for documenting this. I'm queuing it with the following gtk-doc
syntax and contential modifications, please let me know if you wish for
any wording changes.
Andreas
diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
index 9614758..8c17165 100644
--- a/include/hw/sysbus.h
+++ b/include/hw/sysbus.h
@@ -23,18 +23,20 @@ typedef struct SysBusDevice SysBusDevice;
#define SYS_BUS_DEVICE_GET_CLASS(obj) \
OBJECT_GET_CLASS(SysBusDeviceClass, (obj), TYPE_SYS_BUS_DEVICE)
-/*
+/**
* SysBusDeviceClass:
- * @parent_class: This is private
- * @init: Callback function invoked when the #DeviceState::realized
property
+ * @init: Callback function invoked when the #DeviceState.realized property
* is changed to %true. Deprecated, new types inheriting directly from
- * TYPE_SYS_BUS_DEVICE should use #DeviceClass::realize instead, new leaf
- * types should consult their respective parent type. SysBusDeviceClass is
- * not implementing #DeviceClass::realize, so deriving classes can simply
- * ignore it.
+ * TYPE_SYS_BUS_DEVICE should use #DeviceClass.realize instead, new leaf
+ * types should consult their respective parent type.
+ *
+ * SysBusDeviceClass is not overriding #DeviceClass.realize, so derived
+ * classes overriding it are not required to invoke its implementation.
*/
typedef struct SysBusDeviceClass {
+/*< private >*/
DeviceClass parent_class;
+/*< public >*/
int (*init)(SysBusDevice *dev);
} SysBusDeviceClass;
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH 3/7] nbd: use BDS refcount
On Tue, 07/02 12:16, Paolo Bonzini wrote:
> Il 02/07/2013 07:59, Fam Zheng ha scritto:
> > Previously, nbd call drive_get_ref on the drive of bs. A BDS doesn't
> > always have associated dinfo, it's more proper to use bdrv_get_ref().
>
> This has the important side effect of marking the exported disk as
> "in_use" (to use the terms before the series). Right now you can serve
> a disk and, at the same time, stream it or mirror it or create a live
> snapshot of it.
>
> Do we really want to block anything for a device being served? Perhaps
> truncation, but maybe not even that. The NBD server is meant to be as
> unobtrusive as possible (in some sense NBD accesses are the same as
> guest accesses).
>
> Paolo
>
OK, it is better to work like that. But I don't quite understand why was
there drive_get_ref() on the device (w/o the series), as there's already
a close notifier? And it just drive_put_ref() when bs is closed?
> > Signed-off-by: Fam Zheng
> > ---
> > blockdev-nbd.c | 9 +
> > nbd.c | 5 +
> > 2 files changed, 6 insertions(+), 8 deletions(-)
> >
> > diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> > index 95f10c8..d8bcd6f 100644
> > --- a/blockdev-nbd.c
> > +++ b/blockdev-nbd.c
> > @@ -69,12 +69,6 @@ static void nbd_close_notifier(Notifier *n, void *data)
> > g_free(cn);
> > }
> >
> > -static void nbd_server_put_ref(NBDExport *exp)
> > -{
> > -BlockDriverState *bs = nbd_export_get_blockdev(exp);
> > -drive_put_ref(drive_get_by_blockdev(bs));
> > -}
> > -
> > void qmp_nbd_server_add(const char *device, bool has_writable, bool
> > writable,
> > Error **errp)
> > {
> > @@ -106,10 +100,9 @@ void qmp_nbd_server_add(const char *device, bool
> > has_writable, bool writable,
> > }
> >
> > exp = nbd_export_new(bs, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY,
> > - nbd_server_put_ref);
> > + NULL);
> >
> > nbd_export_set_name(exp, device);
> > -drive_get_ref(drive_get_by_blockdev(bs));
> >
> > n = g_malloc0(sizeof(NBDCloseNotifier));
> > n->n.notify = nbd_close_notifier;
> > diff --git a/nbd.c b/nbd.c
> > index 2606403..f28b9fb 100644
> > --- a/nbd.c
> > +++ b/nbd.c
> > @@ -881,6 +881,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t
> > dev_offset,
> > exp->nbdflags = nbdflags;
> > exp->size = size == -1 ? bdrv_getlength(bs) : size;
> > exp->close = close;
> > +bdrv_get_ref(bs);
> > return exp;
> > }
> >
> > @@ -927,6 +928,10 @@ void nbd_export_close(NBDExport *exp)
> > }
> > nbd_export_set_name(exp, NULL);
> > nbd_export_put(exp);
> > +if (exp->bs) {
> > +bdrv_put_ref(exp->bs);
> > +exp->bs = NULL;
> > +}
> > }
> >
> > void nbd_export_get(NBDExport *exp)
> >
>
--
Fam
Re: [Qemu-devel] [PATCH 1/7] block: Convert BlockDriverState.in_use to refcount
On Tue, 07/02 13:41, Eric Blake wrote:
> On 07/01/2013 11:59 PM, Fam Zheng wrote:
> > Use numeric value to replace in_use flag in BDS, this will make
> > lifecycle management with ref count possible. This patch only replaces
> > existing uses of bdrv_set_in_use, so no logic change here.
> >
> > Signed-off-by: Fam Zheng
> > ---
> > block-migration.c | 4 ++--
> > block.c | 23 +++
> > blockjob.c | 6 +++---
> > hw/block/dataplane/virtio-blk.c | 4 ++--
> > include/block/block.h | 3 ++-
> > include/block/block_int.h | 2 +-
> > 6 files changed, 25 insertions(+), 17 deletions(-)
> >
> > diff --git a/block-migration.c b/block-migration.c
> > index 2fd7699..2efb6c0 100644
> > --- a/block-migration.c
> > +++ b/block-migration.c
> > @@ -321,7 +321,7 @@ static void init_blk_migration_it(void *opaque,
> > BlockDriverState *bs)
> > bmds->shared_base = block_mig_state.shared_base;
> > alloc_aio_bitmap(bmds);
> > drive_get_ref(drive_get_by_blockdev(bs));
> > -bdrv_set_in_use(bs, 1);
> > +bdrv_get_ref(bs);
>
> The old code sets the flag, the new code gets a ref.
>
> >
> > block_mig_state.total_sector_sum += sectors;
> >
> > @@ -557,7 +557,7 @@ static void blk_mig_cleanup(void)
> > blk_mig_lock();
> > while ((bmds = QSIMPLEQ_FIRST(&block_mig_state.bmds_list)) != NULL) {
> > QSIMPLEQ_REMOVE_HEAD(&block_mig_state.bmds_list, entry);
> > -bdrv_set_in_use(bmds->bs, 0);
> > +bdrv_get_ref(bmds->bs);
>
> The old code sets the flag, the new code gets a ref. Shouldn't this be
> clearing a ref, with bdrv_put_ref?
Yes, thanks for pointing out.
--
Fam
[Qemu-devel] [PATCH qom-cpu] target-ppc: Change LOG_MMU_STATE() argument to CPUState
Choose CPUState rather than PowerPCCPU since doing a CPU() cast on the
macro argument would hide type mismatches.
Signed-off-by: Andreas Färber
---
target-ppc/mmu-hash32.c | 4 ++--
target-ppc/mmu-hash64.c | 4 ++--
target-ppc/mmu_helper.c | 6 +++---
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/target-ppc/mmu-hash32.c b/target-ppc/mmu-hash32.c
index b5ebe07..6a77dc4 100644
--- a/target-ppc/mmu-hash32.c
+++ b/target-ppc/mmu-hash32.c
@@ -29,10 +29,10 @@
#ifdef DEBUG_MMU
# define LOG_MMU(...) qemu_log(__VA_ARGS__)
-# define LOG_MMU_STATE(env) log_cpu_state(CPU(ppc_env_get_cpu(env)), 0)
+# define LOG_MMU_STATE(cpu) log_cpu_state((cpu), 0)
#else
# define LOG_MMU(...) do { } while (0)
-# define LOG_MMU_STATE(...) do { } while (0)
+# define LOG_MMU_STATE(cpu) do { } while (0)
#endif
#ifdef DEBUG_BATS
diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
index 759cef3..67fc1b5 100644
--- a/target-ppc/mmu-hash64.c
+++ b/target-ppc/mmu-hash64.c
@@ -28,10 +28,10 @@
#ifdef DEBUG_MMU
# define LOG_MMU(...) qemu_log(__VA_ARGS__)
-# define LOG_MMU_STATE(env) log_cpu_state(CPU(ppc_env_get_cpu(env)), 0)
+# define LOG_MMU_STATE(cpu) log_cpu_state((cpu), 0)
#else
# define LOG_MMU(...) do { } while (0)
-# define LOG_MMU_STATE(...) do { } while (0)
+# define LOG_MMU_STATE(cpu) do { } while (0)
#endif
#ifdef DEBUG_SLB
diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
index 413e95b..77102c4 100644
--- a/target-ppc/mmu_helper.c
+++ b/target-ppc/mmu_helper.c
@@ -32,10 +32,10 @@
#ifdef DEBUG_MMU
# define LOG_MMU(...) qemu_log(__VA_ARGS__)
-# define LOG_MMU_STATE(env) log_cpu_state(CPU(ppc_env_get_cpu(env)), 0)
+# define LOG_MMU_STATE(cpu) log_cpu_state((cpu), 0)
#else
# define LOG_MMU(...) do { } while (0)
-# define LOG_MMU_STATE(...) do { } while (0)
+# define LOG_MMU_STATE(cpu) do { } while (0)
#endif
#ifdef DEBUG_SOFTWARE_TLB
@@ -1508,7 +1508,7 @@ static int cpu_ppc_handle_mmu_fault(CPUPPCState *env,
target_ulong address,
mmu_idx, TARGET_PAGE_SIZE);
ret = 0;
} else if (ret < 0) {
-LOG_MMU_STATE(env);
+LOG_MMU_STATE(CPU(ppc_env_get_cpu(env)));
if (access_type == ACCESS_CODE) {
switch (ret) {
case -1:
--
1.8.1.4
Re: [Qemu-devel] [PATCH RFC qom-cpu 03/41] gdbstub: Change GDBState::query_cpu to CPUState
Am 01.07.2013 19:05, schrieb Richard Henderson: > On 06/29/2013 01:01 PM, Andreas Färber wrote: >> Since first_cpu/next_cpu are CPUState, CPUArchState is no longer needed. >> >> Signed-off-by: Andreas Färber >> --- >> gdbstub.c | 9 - >> 1 file changed, 4 insertions(+), 5 deletions(-) > > Reviewed-by: Richard Henderson Thanks, applied to qom-cpu: https://github.com/afaerber/qemu-cpu/commits/qom-cpu Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [RFC V8 01/24] qcow2: Add journal specification.
> > 2. Byte-granularity means that read-modify-write is necessary to append > >entries to the journal. Therefore a failure could destroy previously > >committed entries. > > > >Any ideas how existing journals handle this? > > You commit only whole blocks. So in this case we can consider a block > only committed as soon as a TYPE_END entry has been written (and after > that we won't touch it any more until the journalled changes have been > flushed to disk). > > There's one "interesting" case: cache=writethrough. I'm not entirely > sure yet what to do with it, but it's slow anyway, so using one block > per entry and therefore flushing the journal very often might actually > be not totally unreasonable. This sure would finish to kill the performance because this would be an io per metadata written to disk. > > Another thing I'm not sure about is whether a fixed 4k block is good or > if we should leave it configurable. I don't think making it an option > would hurt (not necessarily modifyable with qemu-img, but as a field > in the file format). I agree. I also think about make the number of block to be flushed at once configurable. Benoît
Re: [Qemu-devel] [PATCH v2 0/9] tcg: remainder and arm runtime detection
On 2 July 2013 21:25, Anthony Liguori wrote: > Peter Maydell writes: >> On 2 July 2013 20:17, Anthony Liguori wrote: >>> Richard Henderson writes: >>> Ping. >>> >>> Peter, could you take a look and bring in through the arm tree? >> >> I can test it and put together a pullrequest, sure, though >> tcg/arm isn't really covered by either arm-devs or target-arm. > > Ah, I had assumed you also maintained ARM TCG host support. I care if it's broken, but I can't say I test it regularly, and I wouldn't go so far as to call myself actually a maintainer (partly because I feel like if I pick up any more random subtrees I won't be doing anything but reviewing patches and putting together pull requests ;-). -- PMM
Re: [Qemu-devel] [RFC V8 01/24] qcow2: Add journal specification.
> > +QCOW2 can use one or more instance of a metadata journal. > > s/instance/instances/ > > Is there a reason to use multiple journals rather than a single journal > for all entry types? The single journal area avoids seeks. Here are the main reason for this: For the deduplication some patterns like cycles of insertion/deletion could leave the hash table almost empty while filling the journal. If the journal is full and the hash table is empty a packing operation is started. Basically a new journal is created and only the entry presents in the hash table are reinserted. This is why I want to keep the deduplication journal appart from regular qcow2 journal: to avoid interferences between a pack operation and regular qcow2 journal entries. The other thing is that freezing the log store would need a replay of regular qcow2 entries as it trigger a reset of the journal. Also since deduplication will not work on spinning disk I discarded the seek time factor. Maybe commiting the dedupe journal by erase block sized chunk would be a good idea to reduce random writes to the SSD. The additional reason for having multiple journals is that the SILT paper propose a mode where prefix of the hash is used to dispatch insertions in multiples store and it easier to do with multiple journals. > > > + > > +A journal is a sequential log of journal entries appended on a previously > > +allocated and reseted area. > > I think you say "previously reset area" instead of "reseted". Another > option is "initialized area". > > > +A journal is designed like a linked list with each entry pointing to the > > next > > +so it's easy to iterate over entries. > > + > > +A journal uses the following constants to denote the type of each entry > > + > > +TYPE_NONE = 0xFF default value of any bytes in a reseted journal > > +TYPE_END = 1 the entry ends a journal cluster and point to the > > next > > + cluster > > +TYPE_HASH = 2 the entry contains a deduplication hash > > + > > +QCOW2 journal entry: > > + > > +Byte 0 :Size of the entry: size = 2 + n with size <= 254 > > This is not clear. I'm wondering if the +2 is included in the byte > value or not. I'm also wondering what a byte value of zero means and > what a byte value of 255 means. I am counting the journal entry header in the size. So yes the +2 is in the byte value. A byte value of zero, 1 or 255 is an error. Maybe this design is bogus and I should only count the payload size in the size field. It would make less tricky cases. > > Please include an example to illustrate how this field works. > > > + > > + 1 :Type of the entry > > + > > + 2 - size :The optional n bytes structure carried by entry > > + > > +A journal is divided into clusters and no journal entry can be spilled on > > two > > +clusters. This avoid having to read more than one cluster to get a single > > entry. > > + > > +For this purpose an entry with the end type is added at the end of a > > journal > > +cluster before starting to write in the next cluster. > > +The size of such an entry is set so the entry points to the next cluster. > > + > > +As any journal cluster must be ended with an end entry the size of regular > > +journal entries is limited to 254 bytes in order to always left room for > > an end > > +entry which mimimal size is two bytes. > > + > > +The only cases where size > 254 are none entries where size = 255. > > + > > +The replay of a journal stop when the first end none entry is reached. > > s/stop/stops/ > > > +The journal cluster size is 4096 bytes. > > Questions about this layout: > > 1. Journal entries have no integrity mechanism, which is especially >important if they span physical sectors where cheap disks may perform >a partial write. This would leave a corrupt journal. If the last >bytes are a checksum then you can get some confidence that the entry >was fully written and is valid. I will add a checksum mecanism. Do you have any preferences regarding the checksum function ? > >Did I miss something? > > 2. Byte-granularity means that read-modify-write is necessary to append >entries to the journal. Therefore a failure could destroy previously >committed entries. It's designed to be committed by 4KB blocks. > >Any ideas how existing journals handle this? >
Re: [Qemu-devel] [PATCH RFC qom-cpu 01/41] log: Change log_cpu_state[_mask]() argument to CPUState
Am 02.07.2013 03:26, schrieb Andreas Färber:
> Am 29.06.2013 22:01, schrieb Andreas Färber:
>> diff --git a/target-microblaze/translate.c b/target-microblaze/translate.c
>> index 54f439f..f9acdb1 100644
>> --- a/target-microblaze/translate.c
>> +++ b/target-microblaze/translate.c
>> @@ -1741,6 +1741,9 @@ static void
>> gen_intermediate_code_internal(CPUMBState *env, TranslationBlock *tb,
>> int search_pc)
>> {
>> +#if !SIM_COMPAT
>> +MicroBlazeCPU *cpu = mb_env_get_cpu(env);
>> +#endif
>> uint16_t *gen_opc_end;
>> uint32_t pc_start;
>> int j, lj;
> [snip]
>
> This hunk would benefit from the gen_intermediate_code_internal()
> argument type change prompted by gdbstub later in this series, so I
> intend to prepend those once v2 is done - inline is missing here.
Missing inline was added separately to allow backporting to stable.
This hunk was dropped via rebase onto gen_intermediate_code_internal()
MicroBlazeCPU change:
https://github.com/afaerber/qemu-cpu/commits/qom-cpu
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH qom-cpu 2/5] target-lm32: gen_intermediate_code_internal() should be inline
Am Dienstag, 2. Juli 2013, 21:31:33 schrieb Andreas Färber:
> Reported-by: Richard Henderson
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Andreas Färber
> ---
> target-lm32/translate.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/target-lm32/translate.c b/target-lm32/translate.c
> index 227a801..7d82dc7 100644
> --- a/target-lm32/translate.c
> +++ b/target-lm32/translate.c
> @@ -1011,8 +1011,9 @@ static void check_breakpoint(CPULM32State *env,
> DisasContext *dc) }
>
> /* generate intermediate code for basic block 'tb'. */
> -static void gen_intermediate_code_internal(CPULM32State *env,
> -TranslationBlock *tb, int search_pc)
> +static inline
> +void gen_intermediate_code_internal(CPULM32State *env,
> +TranslationBlock *tb, int search_pc)
> {
> struct DisasContext ctx, *dc = &ctx;
> uint16_t *gen_opc_end;
Acked-by: Michael Walle
--
michael
Re: [Qemu-devel] [PATCH RFC qom-cpu 23/41] target-xtensa: Change gen_intermediate_code_internal() arg to XtensaCPU
Am 01.07.2013 19:16, schrieb Richard Henderson: > On 06/29/2013 01:01 PM, Andreas Färber wrote: >> Also use bool type while at it. >> >> Prepares for moving singlestep_enabled field to CPUState. >> >> Signed-off-by: Andreas Färber >> --- >> target-xtensa/translate.c | 10 ++ >> 1 file changed, 6 insertions(+), 4 deletions(-) > > All of 10-23/41, > > Reviewed-by: Richard Henderson Thanks, rebased on inline additions and inserted into qom-cpu: https://github.com/afaerber/qemu-cpu/commits/qom-cpu Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH v2 0/9] tcg: remainder and arm runtime detection
Am 02.07.2013 22:25, schrieb Anthony Liguori: > Peter Maydell writes: >> On 2 July 2013 20:17, Anthony Liguori wrote: >>> Peter, could you take a look and bring in through the arm tree? >> >> I can test it and put together a pullrequest, sure, though >> tcg/arm isn't really covered by either arm-devs or target-arm. > > Ah, I had assumed you also maintained ARM TCG host support. > > MAINTAINERS is silent on TCG host support :-/ Actually it isn't: Tiny Code Generator (TCG) - Common code M: qemu-devel@nongnu.org S: Maintained F: tcg/ [...] ARM target M: Andrzej Zaborowski S: Maintained F: tcg/arm/ but that doesn't quite reflect reality anymore. ;) Richard, don't you want to put yourself there for the common code? You're the most active on core TCG these days and you could just send a PULL when there's no more comments after a ping. Regards, Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH] full introspection support for QMP
Paolo Bonzini writes: > Il 02/07/2013 20:21, Anthony Liguori ha scritto: >>> > >>> > Indeed, I wouldn't mind a one-time pass over qapi-schema.json to make it >>> > follow a more rigid format if that made it easier to use it as-is with >>> > less post-processing. It won't be very nice to backport such a >>> > conversion, but I don't know how much distros are planning on >>> > backporting introspection in the first place. >> We consume the schema in QEMU. No reason for us to consume it in a >> different format than libvirt. > > One reason could be that qapi-schema.json, as written, lacks a schema > that can be expressed itself using QAPI. Yup, but how much does that matter in practice? At any rate, if we wanted to solve this problem--a self-describing schema--we should do it in qapi-schema.json too. Regards, Anthony Liguori > > Paolo
Re: [Qemu-devel] [PATCH 01/17] pseries: move interrupt controllers to hw/intc/
Am 27.06.2013 08:45, schrieb Alexey Kardashevskiy:
> Signed-off-by: Alexey Kardashevskiy
> ---
> default-configs/ppc64-softmmu.mak |1 +
> hw/intc/Makefile.objs |1 +
> hw/{ppc => intc}/xics.c |0
> hw/ppc/Makefile.objs |2 +-
> 4 files changed, 3 insertions(+), 1 deletion(-)
> rename hw/{ppc => intc}/xics.c (100%)
Looks sensible,
Reviewed-by: Andreas Färber
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH] savevm: Fix potential memory leak
Am 02.07.2013 22:33, schrieb Stefan Weil:
> Am 16.06.2013 13:33, schrieb Stefan Weil:
>> The leak was reported by cppcheck. Fix it by moving the g_malloc0 after
>> the argument validity check.
>>
>> Signed-off-by: Stefan Weil
>> ---
>> savevm.c |3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/savevm.c b/savevm.c
>> index 2ce439f..b883714 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -479,7 +479,7 @@ static const QEMUFileOps socket_write_ops = {
>>
>> QEMUFile *qemu_fopen_socket(int fd, const char *mode)
>> {
>> -QEMUFileSocket *s = g_malloc0(sizeof(QEMUFileSocket));
>> +QEMUFileSocket *s;
>>
>> if (mode == NULL ||
>> (mode[0] != 'r' && mode[0] != 'w') ||
>> @@ -488,6 +488,7 @@ QEMUFile *qemu_fopen_socket(int fd, const char *mode)
>> return NULL;
>> }
>>
>> +s = g_malloc0(sizeof(QEMUFileSocket));
>> s->fd = fd;
>> if (mode[0] == 'w') {
>> qemu_set_block(s->fd);
>
> Ping?
Sorry, that was bad bookkeeping on my side.
The patch was already applied.
Stefan
Re: [Qemu-devel] [PATCH] fsdev: Fix potential memory leak
Am 16.06.2013 12:02, schrieb Stefan Weil:
> This leak was reported by cppcheck.
>
> Signed-off-by: Stefan Weil
> ---
> fsdev/qemu-fsdev.c |2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/fsdev/qemu-fsdev.c b/fsdev/qemu-fsdev.c
> index 6eaf36d..ccfec13 100644
> --- a/fsdev/qemu-fsdev.c
> +++ b/fsdev/qemu-fsdev.c
> @@ -76,6 +76,8 @@ int qemu_fsdev_add(QemuOpts *opts)
>
> if (fsle->fse.ops->parse_opts) {
> if (fsle->fse.ops->parse_opts(opts, &fsle->fse)) {
> +g_free(fsle->fse.fsdev_id);
> +g_free(fsle);
> return -1;
> }
> }
Ping?
Re: [Qemu-devel] [PATCH] hw/9pfs: Fix potential memory leak and avoid reuse of freed memory
Am 16.06.2013 12:14, schrieb Stefan Weil:
> The leak was reported by cppcheck.
>
> Function proxy_init also calls g_free for ctx->fs_root.
> Avoid reuse of this memory by setting ctx->fs_root to NULL.
>
> Signed-off-by: Stefan Weil
> ---
>
> Hi,
>
> I'm not sure whether ctx->fs_root should also be freed in the error case.
> Please feel free to modify my patch if needed.
>
> Regards
> Stefan Weil
>
> hw/9pfs/virtio-9p-proxy.c |2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/virtio-9p-proxy.c
> index 8ba2959..5f44bb7 100644
> --- a/hw/9pfs/virtio-9p-proxy.c
> +++ b/hw/9pfs/virtio-9p-proxy.c
> @@ -1153,10 +1153,12 @@ static int proxy_init(FsContext *ctx)
> sock_id = atoi(ctx->fs_root);
> if (sock_id < 0) {
> fprintf(stderr, "socket descriptor not initialized\n");
> +g_free(proxy);
> return -1;
> }
> }
> g_free(ctx->fs_root);
> +ctx->fs_root = NULL;
>
> proxy->in_iovec.iov_base = g_malloc(PROXY_MAX_IO_SZ + PROXY_HDR_SZ);
> proxy->in_iovec.iov_len = PROXY_MAX_IO_SZ + PROXY_HDR_SZ;
Ping?
Re: [Qemu-devel] [PATCH] savevm: Fix potential memory leak
Am 16.06.2013 13:33, schrieb Stefan Weil:
> The leak was reported by cppcheck. Fix it by moving the g_malloc0 after
> the argument validity check.
>
> Signed-off-by: Stefan Weil
> ---
> savevm.c |3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/savevm.c b/savevm.c
> index 2ce439f..b883714 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -479,7 +479,7 @@ static const QEMUFileOps socket_write_ops = {
>
> QEMUFile *qemu_fopen_socket(int fd, const char *mode)
> {
> -QEMUFileSocket *s = g_malloc0(sizeof(QEMUFileSocket));
> +QEMUFileSocket *s;
>
> if (mode == NULL ||
> (mode[0] != 'r' && mode[0] != 'w') ||
> @@ -488,6 +488,7 @@ QEMUFile *qemu_fopen_socket(int fd, const char *mode)
> return NULL;
> }
>
> +s = g_malloc0(sizeof(QEMUFileSocket));
> s->fd = fd;
> if (mode[0] == 'w') {
> qemu_set_block(s->fd);
Ping?
Re: [Qemu-devel] [PATCH 3/3] spapr: Fix compiler warning for some versions of gcc (spapr_io_read)
Am 02.07.2013 um 22:22 schrieb Stefan Weil : > Am 02.07.2013 14:45, schrieb Alexander Graf: >> On 06/24/2013 07:48 PM, Stefan Weil wrote: >>> i686-w64-mingw32-gcc (GCC) 4.6.3 from Debian wheezy reports this >>> warning: >>> >>> hw/ppc/spapr_pci.c:454:1: warning: >>> control reaches end of non-void function [-Wreturn-type] >>> >>> Adding a default case to the switch statement satisfies the compiler. >>> This modification requires moving the assert statement. >>> >>> Signed-off-by: Stefan Weil >>> --- >>> hw/ppc/spapr_pci.c |3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >>> index 04e8362..c04086c 100644 >>> --- a/hw/ppc/spapr_pci.c >>> +++ b/hw/ppc/spapr_pci.c >>> @@ -448,9 +448,10 @@ static uint64_t spapr_io_read(void *opaque, >>> hwaddr addr, >>> case 2: >>> return cpu_inw(addr); >>> case 4: >>> +default: >>> +assert(size == 4); >>> return cpu_inl(addr); >> >> Please fix this one up with g_assert_not_reached() as well. >> >> Alex > > I did, and you applied it to ppc-next. :-) > > See http://patchwork.ozlabs.org/patch/255738/. Oops, just consider me stupid then :) Alex > > Cheers > Stefan >
Re: [Qemu-devel] [PATCH v2 0/9] tcg: remainder and arm runtime detection
Peter Maydell writes: > On 2 July 2013 20:17, Anthony Liguori wrote: >> Richard Henderson writes: >> >>> Ping. >> >> Peter, could you take a look and bring in through the arm tree? > > I can test it and put together a pullrequest, sure, though > tcg/arm isn't really covered by either arm-devs or target-arm. Ah, I had assumed you also maintained ARM TCG host support. MAINTAINERS is silent on TCG host support :-/ Regards, Anthony Liguori > > -- PMM
Re: [Qemu-devel] [PATCH 3/3] spapr: Fix compiler warning for some versions of gcc (spapr_io_read)
Am 02.07.2013 14:45, schrieb Alexander Graf: > On 06/24/2013 07:48 PM, Stefan Weil wrote: >> i686-w64-mingw32-gcc (GCC) 4.6.3 from Debian wheezy reports this >> warning: >> >> hw/ppc/spapr_pci.c:454:1: warning: >> control reaches end of non-void function [-Wreturn-type] >> >> Adding a default case to the switch statement satisfies the compiler. >> This modification requires moving the assert statement. >> >> Signed-off-by: Stefan Weil >> --- >> hw/ppc/spapr_pci.c |3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >> index 04e8362..c04086c 100644 >> --- a/hw/ppc/spapr_pci.c >> +++ b/hw/ppc/spapr_pci.c >> @@ -448,9 +448,10 @@ static uint64_t spapr_io_read(void *opaque, >> hwaddr addr, >> case 2: >> return cpu_inw(addr); >> case 4: >> +default: >> +assert(size == 4); >> return cpu_inl(addr); > > Please fix this one up with g_assert_not_reached() as well. > > Alex I did, and you applied it to ppc-next. :-) See http://patchwork.ozlabs.org/patch/255738/. Cheers Stefan
Re: [Qemu-devel] [PATCH qom-cpu 0/5] TCG code generation performance fix
Am 02.07.2013 21:59, schrieb Richard Henderson: > On 07/02/2013 12:31 PM, Andreas Färber wrote: >> Hello, >> >> As Richard explained, the purpose of having separate gen_intermediate_code() >> and gen_intermediate_code_pc() functions per target is to compile-optimize >> gen_intermediate_code_internal() for the non-_pc case. >> >> Multiple targets were using static rather than static inline though, fix >> this. >> >> I've split these off from my refactorings so that we can backport them to >> stable, >> and I'm rebasing my argument refactoring patches on top. >> >> No actual performance changes have been benchmarked, these changes serve more >> to align our targets as clear examples for new targets such as rl78 and bfin. > > All: > Reviewed-by: Richard Henderson > > Although we should probably do some benchmarking at some point to see if > the duplicated code paths really do improve things over, say, unlikely(). I did wonder about unlikely() last night, but I thought that's just a branch optimization whereas inline might avoid some branches in the first place. > But failing that we should at least have conformity of implementation. Thanks, applied to qom-cpu (moving log_cpu_state to qom-cpu-next): https://github.com/afaerber/qemu-cpu/commits/qom-cpu (If maintainers want to ack/nack, please do.) Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH] full introspection support for QMP
On 07/02/2013 02:00 PM, Paolo Bonzini wrote: > Il 02/07/2013 20:21, Anthony Liguori ha scritto: Indeed, I wouldn't mind a one-time pass over qapi-schema.json to make it follow a more rigid format if that made it easier to use it as-is with less post-processing. It won't be very nice to backport such a conversion, but I don't know how much distros are planning on backporting introspection in the first place. >> We consume the schema in QEMU. No reason for us to consume it in a >> different format than libvirt. > > One reason could be that qapi-schema.json, as written, lacks a schema > that can be expressed itself using QAPI. Indeed - it was my attempt to write a structured set of QAPI that led to the more rigid and more verbose layout, rather than shorthand. On the other hand, there's nothing stopping us from stating that QAPI itself can be extended to express new concepts (after all, we want to express the full structure of events as part of qapi-schema.json someday); we even have the 'qmp_capabilities' handshake to advertise whether use of such extensions will be understood by the client. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 6/7] block: add target-id option to drive-backup QMP command
On 07/01/2013 11:59 PM, Fam Zheng wrote:
> Add target-id (optional) to drive-backup command, to make the target bs
> a named drive so that we can operate on it (e.g. export with NBD).
>
> Signed-off-by: Fam Zheng
> ---
> blockdev.c | 4 +++-
> qapi-schema.json | 7 +--
> qmp-commands.hx | 3 ++-
> 3 files changed, 10 insertions(+), 4 deletions(-)
>
> +++ b/qapi-schema.json
> @@ -1654,7 +1654,8 @@
> # Since: 1.6
> ##
> { 'type': 'DriveBackup',
> - 'data': { 'device': 'str', 'target': 'str', '*format': 'str',
> + 'data': { 'device': 'str', 'target': 'str',
> +'*target-id': 'str', '*format': 'str',
Seems undocumented...
> '*mode': 'NewImageMode', '*speed': 'int',
> '*on-source-error': 'BlockdevOnError',
> '*on-target-error': 'BlockdevOnError' } }
> @@ -1807,6 +1808,7 @@
> # is a device, the existing file/device will be used as the new
> # destination. If it does not exist, a new file will be created.
> #
> +# @target-id: #optional the drive id of the target.
...until I read this. Hmm, I think we should first consolidate things
for DriveBackup (so that documentation is listed only once, prior to the
DriveBackup 'type' declaration), by rebasing things on top of
in the same was as Kevin's series "[PATCH v3 0/3] qapi: Top-level type
reference for command definitions" does for BlockdevSnapshot.
The documentation is not incorrect, but it also isn't very helpful -
what is the "drive id of the target" and when would I want to set it?
What do I gain by overriding the drive id, and what is the default
behavior when I don't pass in the option?
> +++ b/qmp-commands.hx
> @@ -913,7 +913,7 @@ EQMP
>
> {
> .name = "drive-backup",
> -.args_type = "device:B,target:s,speed:i?,mode:s?,format:s?,"
> +.args_type =
> "device:B,target:s,target-id:s?,speed:i?,mode:s?,format:s?,"
>"on-source-error:s?,on-target-error:s?",
> .mhandler.cmd_new = qmp_marshal_input_drive_backup,
> },
> @@ -936,6 +936,7 @@ Arguments:
> device, the existing file/device will be used as the new
> destination. If it does not exist, a new file will be created.
> (json-string)
> +- "target-id": the drive id of the target image.
Should probably mention (json-string, optional), as done elsewhere in
this command.
> - "format": the format of the new destination, default is to probe if 'mode'
> is
> 'existing', else the format of the source
> (json-string, optional)
>
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] full introspection support for QMP
Il 02/07/2013 20:21, Anthony Liguori ha scritto: >> > >> > Indeed, I wouldn't mind a one-time pass over qapi-schema.json to make it >> > follow a more rigid format if that made it easier to use it as-is with >> > less post-processing. It won't be very nice to backport such a >> > conversion, but I don't know how much distros are planning on >> > backporting introspection in the first place. > We consume the schema in QEMU. No reason for us to consume it in a > different format than libvirt. One reason could be that qapi-schema.json, as written, lacks a schema that can be expressed itself using QAPI. Paolo
[Qemu-devel] [PATCH qom-cpu 5/5] target-xtensa: gen_intermediate_code_internal() should be inline
Reported-by: Richard Henderson
Cc: qemu-sta...@nongnu.org
Signed-off-by: Andreas Färber
---
target-xtensa/translate.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/target-xtensa/translate.c b/target-xtensa/translate.c
index dcb90a5..e384812 100644
--- a/target-xtensa/translate.c
+++ b/target-xtensa/translate.c
@@ -2875,8 +2875,9 @@ static void gen_ibreak_check(CPUXtensaState *env,
DisasContext *dc)
}
}
-static void gen_intermediate_code_internal(
-CPUXtensaState *env, TranslationBlock *tb, int search_pc)
+static inline
+void gen_intermediate_code_internal(CPUXtensaState *env,
+TranslationBlock *tb, int search_pc)
{
DisasContext dc;
int insn_count = 0;
--
1.8.1.4
Re: [Qemu-devel] [PATCH qom-cpu 0/5] TCG code generation performance fix
On 07/02/2013 12:31 PM, Andreas Färber wrote: > Hello, > > As Richard explained, the purpose of having separate gen_intermediate_code() > and gen_intermediate_code_pc() functions per target is to compile-optimize > gen_intermediate_code_internal() for the non-_pc case. > > Multiple targets were using static rather than static inline though, fix this. > > I've split these off from my refactorings so that we can backport them to > stable, > and I'm rebasing my argument refactoring patches on top. > > No actual performance changes have been benchmarked, these changes serve more > to align our targets as clear examples for new targets such as rl78 and bfin. All: Reviewed-by: Richard Henderson Although we should probably do some benchmarking at some point to see if the duplicated code paths really do improve things over, say, unlikely(). But failing that we should at least have conformity of implementation. r~
Re: [Qemu-devel] [PATCH v3 3/3] qapi-schema: Use BlockdevSnapshot type for blockdev-snapshot-sync
On 07/01/2013 08:31 AM, Kevin Wolf wrote: > We don't have to duplicate the definition any more now that we may refer > to a type instead. > > Signed-off-by: Kevin Wolf > --- > qapi-schema.json | 14 ++ > 1 file changed, 2 insertions(+), 12 deletions(-) Since this patch is not in yet, but DriveBackup IS in via commit 3037f364, should you expand the scope of this patch to cover that command at the same time? -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 0/9] tcg: remainder and arm runtime detection
On 2 July 2013 20:17, Anthony Liguori wrote: > Richard Henderson writes: > >> Ping. > > Peter, could you take a look and bring in through the arm tree? I can test it and put together a pullrequest, sure, though tcg/arm isn't really covered by either arm-devs or target-arm. -- PMM
[Qemu-devel] [PATCH qom-cpu 3/5] target-microblaze: gen_intermediate_code_internal() should be inline
Reported-by: Richard Henderson
Cc: qemu-sta...@nongnu.org
Signed-off-by: Andreas Färber
---
target-microblaze/translate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target-microblaze/translate.c b/target-microblaze/translate.c
index 54f439f..27da4bf 100644
--- a/target-microblaze/translate.c
+++ b/target-microblaze/translate.c
@@ -1737,7 +1737,7 @@ static void check_breakpoint(CPUMBState *env,
DisasContext *dc)
}
/* generate intermediate code for basic block 'tb'. */
-static void
+static inline void
gen_intermediate_code_internal(CPUMBState *env, TranslationBlock *tb,
int search_pc)
{
--
1.8.1.4
[Qemu-devel] [PATCH qom-cpu 4/5] target-moxie: gen_intermediate_code_internal() should be inline
Reported-by: Richard Henderson
Cc: qemu-sta...@nongnu.org
Signed-off-by: Andreas Färber
---
target-moxie/translate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target-moxie/translate.c b/target-moxie/translate.c
index b0ae38a..664d359 100644
--- a/target-moxie/translate.c
+++ b/target-moxie/translate.c
@@ -820,7 +820,7 @@ static int decode_opc(MoxieCPU *cpu, DisasContext *ctx)
}
/* generate intermediate code for basic block 'tb'. */
-static void
+static inline void
gen_intermediate_code_internal(MoxieCPU *cpu, TranslationBlock *tb,
bool search_pc)
{
--
1.8.1.4
[Qemu-devel] [PATCH qom-cpu 2/5] target-lm32: gen_intermediate_code_internal() should be inline
Reported-by: Richard Henderson
Cc: qemu-sta...@nongnu.org
Signed-off-by: Andreas Färber
---
target-lm32/translate.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/target-lm32/translate.c b/target-lm32/translate.c
index 227a801..7d82dc7 100644
--- a/target-lm32/translate.c
+++ b/target-lm32/translate.c
@@ -1011,8 +1011,9 @@ static void check_breakpoint(CPULM32State *env,
DisasContext *dc)
}
/* generate intermediate code for basic block 'tb'. */
-static void gen_intermediate_code_internal(CPULM32State *env,
-TranslationBlock *tb, int search_pc)
+static inline
+void gen_intermediate_code_internal(CPULM32State *env,
+TranslationBlock *tb, int search_pc)
{
struct DisasContext ctx, *dc = &ctx;
uint16_t *gen_opc_end;
--
1.8.1.4
Re: [Qemu-devel] [PATCH v2 0/9] tcg: remainder and arm runtime detection
Am 02.07.2013 21:17, schrieb Anthony Liguori: > Richard Henderson writes: > >> Ping. > > Peter, could you take a look and bring in through the arm tree? The ppc bits look pretty obvious and the rem concept fine to me. Didn't look too close at the arm parts. Andreas > > Regards, > > Anthony Liguori > >> >> On 06/26/2013 01:52 PM, Richard Henderson wrote: >>> This patch set includes both the remainder series and arm runtime >>> detection series that I've previouslyt posted separately, as there >>> are small conflicts between the two series. >>> >>> Aside from rebasing vs master, the only other change is to fix the >>> TCG_OPF_NOT_PRESENT problem wrt call that Claudio Fontana spotted. >>> >>> >>> r~ >>> >>> >>> Richard Henderson (9): >>> tcg: Split rem requirement from div requirement >>> tcg-arm: Don't implement rem >>> tcg-ppc: Don't implement rem >>> tcg-ppc64: Don't implement rem >>> tcg: Allow non-constant control macros >>> tcg: Simplify logic using TCG_OPF_NOT_PRESENT >>> tcg-arm: Make use of conditional availability of opcodes for divide >>> tcg-arm: Simplify logic in detecting the ARM ISA in use >>> tcg-arm: Use AT_PLATFORM to detect the host ISA >>> >>> tcg/arm/tcg-target.c | 96 >>> ++ >>> tcg/arm/tcg-target.h | 15 >>> tcg/hppa/tcg-target.h | 1 + >>> tcg/ia64/tcg-target.h | 2 ++ >>> tcg/mips/tcg-target.h | 1 + >>> tcg/ppc/tcg-target.c | 14 >>> tcg/ppc/tcg-target.h | 1 + >>> tcg/ppc64/tcg-target.c | 26 -- >>> tcg/ppc64/tcg-target.h | 2 ++ >>> tcg/sparc/tcg-target.h | 2 ++ >>> tcg/tcg-op.h | 32 ++--- >>> tcg/tcg-opc.h | 36 ++- >>> tcg/tcg.c | 4 +-- >>> tcg/tcg.h | 6 +++- >>> tcg/tci/tcg-target.h | 2 ++ >>> 15 files changed, 116 insertions(+), 124 deletions(-) >>> > > -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
[Qemu-devel] [PATCH qom-cpu 1/5] target-cris: gen_intermediate_code_internal() should be inlined
Reported-by: Richard Henderson
Cc: qemu-sta...@nongnu.org
Signed-off-by: Andreas Färber
---
target-cris/translate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target-cris/translate.c b/target-cris/translate.c
index 09d0d2b..ee9ae22 100644
--- a/target-cris/translate.c
+++ b/target-cris/translate.c
@@ -3161,7 +3161,7 @@ static void check_breakpoint(CPUCRISState *env,
DisasContext *dc)
*/
/* generate intermediate code for basic block 'tb'. */
-static void
+static inline void
gen_intermediate_code_internal(CPUCRISState *env, TranslationBlock *tb,
int search_pc)
{
--
1.8.1.4
[Qemu-devel] [PATCH qom-cpu 0/5] TCG code generation performance fix
Hello, As Richard explained, the purpose of having separate gen_intermediate_code() and gen_intermediate_code_pc() functions per target is to compile-optimize gen_intermediate_code_internal() for the non-_pc case. Multiple targets were using static rather than static inline though, fix this. I've split these off from my refactorings so that we can backport them to stable, and I'm rebasing my argument refactoring patches on top. No actual performance changes have been benchmarked, these changes serve more to align our targets as clear examples for new targets such as rl78 and bfin. Regards, Andreas Cc: Richard Henderson Cc: Edgar E. Iglesias (cris, mblaze) Cc: Michael Walle (lm32) Cc: Peter Crosthwaite (mblaze) Cc: Anthony Green (moxie) Cc: Max Filippov (xtensa) Cc: qemu-sta...@nongnu.org Cc: Mike Frysinger (bfin) Andreas Färber (5): target-cris: gen_intermediate_code_internal() should be inlined target-lm32: gen_intermediate_code_internal() should be inline target-microblaze: gen_intermediate_code_internal() should be inline target-moxie: gen_intermediate_code_internal() should be inline target-xtensa: gen_intermediate_code_internal() should be inline target-cris/translate.c | 2 +- target-lm32/translate.c | 5 +++-- target-microblaze/translate.c | 2 +- target-moxie/translate.c | 2 +- target-xtensa/translate.c | 5 +++-- 5 files changed, 9 insertions(+), 7 deletions(-) -- 1.8.1.4
Re: [Qemu-devel] [PATCH 1/7] block: Convert BlockDriverState.in_use to refcount
On 07/01/2013 11:59 PM, Fam Zheng wrote:
> Use numeric value to replace in_use flag in BDS, this will make
> lifecycle management with ref count possible. This patch only replaces
> existing uses of bdrv_set_in_use, so no logic change here.
>
> Signed-off-by: Fam Zheng
> ---
> block-migration.c | 4 ++--
> block.c | 23 +++
> blockjob.c | 6 +++---
> hw/block/dataplane/virtio-blk.c | 4 ++--
> include/block/block.h | 3 ++-
> include/block/block_int.h | 2 +-
> 6 files changed, 25 insertions(+), 17 deletions(-)
>
> diff --git a/block-migration.c b/block-migration.c
> index 2fd7699..2efb6c0 100644
> --- a/block-migration.c
> +++ b/block-migration.c
> @@ -321,7 +321,7 @@ static void init_blk_migration_it(void *opaque,
> BlockDriverState *bs)
> bmds->shared_base = block_mig_state.shared_base;
> alloc_aio_bitmap(bmds);
> drive_get_ref(drive_get_by_blockdev(bs));
> -bdrv_set_in_use(bs, 1);
> +bdrv_get_ref(bs);
The old code sets the flag, the new code gets a ref.
>
> block_mig_state.total_sector_sum += sectors;
>
> @@ -557,7 +557,7 @@ static void blk_mig_cleanup(void)
> blk_mig_lock();
> while ((bmds = QSIMPLEQ_FIRST(&block_mig_state.bmds_list)) != NULL) {
> QSIMPLEQ_REMOVE_HEAD(&block_mig_state.bmds_list, entry);
> -bdrv_set_in_use(bmds->bs, 0);
> +bdrv_get_ref(bmds->bs);
The old code sets the flag, the new code gets a ref. Shouldn't this be
clearing a ref, with bdrv_put_ref?
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 4/9] tcg-ppc64: Don't implement rem
Am 26.06.2013 22:52, schrieb Richard Henderson:
> Signed-off-by: Richard Henderson
Reviewed-by: Andreas Färber
Andreas
> ---
> tcg/ppc64/tcg-target.c | 26 --
> tcg/ppc64/tcg-target.h | 4 ++--
> 2 files changed, 2 insertions(+), 28 deletions(-)
>
> diff --git a/tcg/ppc64/tcg-target.c b/tcg/ppc64/tcg-target.c
> index 606b73d..0678de2 100644
> --- a/tcg/ppc64/tcg-target.c
> +++ b/tcg/ppc64/tcg-target.c
> @@ -1617,18 +1617,6 @@ static void tcg_out_op (TCGContext *s, TCGOpcode opc,
> const TCGArg *args,
> tcg_out32 (s, DIVWU | TAB (args[0], args[1], args[2]));
> break;
>
> -case INDEX_op_rem_i32:
> -tcg_out32 (s, DIVW | TAB (0, args[1], args[2]));
> -tcg_out32 (s, MULLW | TAB (0, 0, args[2]));
> -tcg_out32 (s, SUBF | TAB (args[0], 0, args[1]));
> -break;
> -
> -case INDEX_op_remu_i32:
> -tcg_out32 (s, DIVWU | TAB (0, args[1], args[2]));
> -tcg_out32 (s, MULLW | TAB (0, 0, args[2]));
> -tcg_out32 (s, SUBF | TAB (args[0], 0, args[1]));
> -break;
> -
> case INDEX_op_shl_i32:
> if (const_args[2]) {
> tcg_out_rlw(s, RLWINM, args[0], args[1], args[2], 0, 31 -
> args[2]);
> @@ -1786,16 +1774,6 @@ static void tcg_out_op (TCGContext *s, TCGOpcode opc,
> const TCGArg *args,
> case INDEX_op_divu_i64:
> tcg_out32 (s, DIVDU | TAB (args[0], args[1], args[2]));
> break;
> -case INDEX_op_rem_i64:
> -tcg_out32 (s, DIVD | TAB (0, args[1], args[2]));
> -tcg_out32 (s, MULLD | TAB (0, 0, args[2]));
> -tcg_out32 (s, SUBF | TAB (args[0], 0, args[1]));
> -break;
> -case INDEX_op_remu_i64:
> -tcg_out32 (s, DIVDU | TAB (0, args[1], args[2]));
> -tcg_out32 (s, MULLD | TAB (0, 0, args[2]));
> -tcg_out32 (s, SUBF | TAB (args[0], 0, args[1]));
> -break;
>
> case INDEX_op_qemu_ld8u:
> tcg_out_qemu_ld (s, args, 0);
> @@ -2064,8 +2042,6 @@ static const TCGTargetOpDef ppc_op_defs[] = {
> { INDEX_op_mul_i32, { "r", "r", "rI" } },
> { INDEX_op_div_i32, { "r", "r", "r" } },
> { INDEX_op_divu_i32, { "r", "r", "r" } },
> -{ INDEX_op_rem_i32, { "r", "r", "r" } },
> -{ INDEX_op_remu_i32, { "r", "r", "r" } },
> { INDEX_op_sub_i32, { "r", "rI", "ri" } },
> { INDEX_op_and_i32, { "r", "r", "ri" } },
> { INDEX_op_or_i32, { "r", "r", "ri" } },
> @@ -2108,8 +2084,6 @@ static const TCGTargetOpDef ppc_op_defs[] = {
> { INDEX_op_mul_i64, { "r", "r", "rI" } },
> { INDEX_op_div_i64, { "r", "r", "r" } },
> { INDEX_op_divu_i64, { "r", "r", "r" } },
> -{ INDEX_op_rem_i64, { "r", "r", "r" } },
> -{ INDEX_op_remu_i64, { "r", "r", "r" } },
>
> { INDEX_op_neg_i64, { "r", "r" } },
> { INDEX_op_not_i64, { "r", "r" } },
> diff --git a/tcg/ppc64/tcg-target.h b/tcg/ppc64/tcg-target.h
> index 7c600f1..48fc6e2 100644
> --- a/tcg/ppc64/tcg-target.h
> +++ b/tcg/ppc64/tcg-target.h
> @@ -76,7 +76,7 @@ typedef enum {
>
> /* optional instructions */
> #define TCG_TARGET_HAS_div_i32 1
> -#define TCG_TARGET_HAS_rem_i32 1
> +#define TCG_TARGET_HAS_rem_i32 0
> #define TCG_TARGET_HAS_rot_i32 1
> #define TCG_TARGET_HAS_ext8s_i321
> #define TCG_TARGET_HAS_ext16s_i32 1
> @@ -97,7 +97,7 @@ typedef enum {
> #define TCG_TARGET_HAS_muls2_i320
>
> #define TCG_TARGET_HAS_div_i64 1
> -#define TCG_TARGET_HAS_rem_i64 1
> +#define TCG_TARGET_HAS_rem_i64 0
> #define TCG_TARGET_HAS_rot_i64 1
> #define TCG_TARGET_HAS_ext8s_i641
> #define TCG_TARGET_HAS_ext16s_i64 1
>
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH v2 3/9] tcg-ppc: Don't implement rem
Am 26.06.2013 22:52, schrieb Richard Henderson:
> Signed-off-by: Richard Henderson
Reviewed-by: Andreas Färber
Andreas
> ---
> tcg/ppc/tcg-target.c | 14 --
> tcg/ppc/tcg-target.h | 2 +-
> 2 files changed, 1 insertion(+), 15 deletions(-)
>
> diff --git a/tcg/ppc/tcg-target.c b/tcg/ppc/tcg-target.c
> index 29ca934..453ab6b 100644
> --- a/tcg/ppc/tcg-target.c
> +++ b/tcg/ppc/tcg-target.c
> @@ -1671,18 +1671,6 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc,
> const TCGArg *args,
> tcg_out32 (s, DIVWU | TAB (args[0], args[1], args[2]));
> break;
>
> -case INDEX_op_rem_i32:
> -tcg_out32 (s, DIVW | TAB (0, args[1], args[2]));
> -tcg_out32 (s, MULLW | TAB (0, 0, args[2]));
> -tcg_out32 (s, SUBF | TAB (args[0], 0, args[1]));
> -break;
> -
> -case INDEX_op_remu_i32:
> -tcg_out32 (s, DIVWU | TAB (0, args[1], args[2]));
> -tcg_out32 (s, MULLW | TAB (0, 0, args[2]));
> -tcg_out32 (s, SUBF | TAB (args[0], 0, args[1]));
> -break;
> -
> case INDEX_op_mulu2_i32:
> if (args[0] == args[2] || args[0] == args[3]) {
> tcg_out32 (s, MULLW | TAB (0, args[2], args[3]));
> @@ -1992,8 +1980,6 @@ static const TCGTargetOpDef ppc_op_defs[] = {
> { INDEX_op_mul_i32, { "r", "r", "ri" } },
> { INDEX_op_div_i32, { "r", "r", "r" } },
> { INDEX_op_divu_i32, { "r", "r", "r" } },
> -{ INDEX_op_rem_i32, { "r", "r", "r" } },
> -{ INDEX_op_remu_i32, { "r", "r", "r" } },
> { INDEX_op_mulu2_i32, { "r", "r", "r", "r" } },
> { INDEX_op_sub_i32, { "r", "r", "ri" } },
> { INDEX_op_and_i32, { "r", "r", "ri" } },
> diff --git a/tcg/ppc/tcg-target.h b/tcg/ppc/tcg-target.h
> index 01b880e..b42d97c 100644
> --- a/tcg/ppc/tcg-target.h
> +++ b/tcg/ppc/tcg-target.h
> @@ -78,7 +78,7 @@ typedef enum {
>
> /* optional instructions */
> #define TCG_TARGET_HAS_div_i32 1
> -#define TCG_TARGET_HAS_rem_i32 1
> +#define TCG_TARGET_HAS_rem_i32 0
> #define TCG_TARGET_HAS_rot_i32 1
> #define TCG_TARGET_HAS_ext8s_i321
> #define TCG_TARGET_HAS_ext16s_i32 1
>
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH v2 0/9] tcg: remainder and arm runtime detection
Richard Henderson writes: > Ping. Peter, could you take a look and bring in through the arm tree? Regards, Anthony Liguori > > On 06/26/2013 01:52 PM, Richard Henderson wrote: >> This patch set includes both the remainder series and arm runtime >> detection series that I've previouslyt posted separately, as there >> are small conflicts between the two series. >> >> Aside from rebasing vs master, the only other change is to fix the >> TCG_OPF_NOT_PRESENT problem wrt call that Claudio Fontana spotted. >> >> >> r~ >> >> >> Richard Henderson (9): >> tcg: Split rem requirement from div requirement >> tcg-arm: Don't implement rem >> tcg-ppc: Don't implement rem >> tcg-ppc64: Don't implement rem >> tcg: Allow non-constant control macros >> tcg: Simplify logic using TCG_OPF_NOT_PRESENT >> tcg-arm: Make use of conditional availability of opcodes for divide >> tcg-arm: Simplify logic in detecting the ARM ISA in use >> tcg-arm: Use AT_PLATFORM to detect the host ISA >> >> tcg/arm/tcg-target.c | 96 >> ++ >> tcg/arm/tcg-target.h | 15 >> tcg/hppa/tcg-target.h | 1 + >> tcg/ia64/tcg-target.h | 2 ++ >> tcg/mips/tcg-target.h | 1 + >> tcg/ppc/tcg-target.c | 14 >> tcg/ppc/tcg-target.h | 1 + >> tcg/ppc64/tcg-target.c | 26 -- >> tcg/ppc64/tcg-target.h | 2 ++ >> tcg/sparc/tcg-target.h | 2 ++ >> tcg/tcg-op.h | 32 ++--- >> tcg/tcg-opc.h | 36 ++- >> tcg/tcg.c | 4 +-- >> tcg/tcg.h | 6 +++- >> tcg/tci/tcg-target.h | 2 ++ >> 15 files changed, 116 insertions(+), 124 deletions(-) >>
Re: [Qemu-devel] [Xen-devel] [PATCH] Citrix PV Bus device [V3]
Stefano Stabellini writes: > On Tue, 2 Jul 2013, Stefano Stabellini wrote: >> - the new device should have configurable vendor and device ids, so that >> host admins can select which vendor's PV drivers are going to be >> automatically installed on all your Windows guests. This should probably >> be a VM config option (pvdevice=, libxl would >> then pass a vendor and device id pair to QEMU via command line. >> We can come up with a config syntax that would support both numeric >> pairs as well as simple labels. > > Anthony, > would you be happy to have a PCI device in QEMU with configurable vendor > and device IDs? We already have this in other places (USB devices, for instance), so yeah, no problem there. I will say that having to force a user to choose which "vendor" drivers they use sucks. It's ashame that both Xen and KVM suffer this problem of everyone introducing their own variants of pv drivers. Regards, Anthony Liguori > If not, would you be OK with a PCI device with the Xen vendor ID but a > configurable device ID?
Re: [Qemu-devel] [PATCH] full introspection support for QMP
Eric Blake writes:
> On 07/02/2013 11:06 AM, Anthony Liguori wrote:
>>> Because qapi-schema.json requires further parsing. For example, how is
>>> a client supposed to know that '*foo':'int' means that there is an
>>> argument named 'foo' but it is optional? The rule of thumb with QMP is
>>> that if you have to post-process JSON output, then the JSON was not
>>> designed correctly.
>>
>> Then we should fix qapi-schema.json.
>>
>
>> One reasonable compromise would be:
>>
>> { "command": "foo", "arguments": { "name": "str", "id": "int" },
>> "optional": { "bar": "bool" } }
>>
>> Then libvirt only has to test 'does command["optional"] contain key
>> "bar"' to test for an optional parameter.
>
> Yes, that might be a reasonable compromise - at least that way, the
> optional parameter names are listed directly, and libvirt doesn't have
> to probe every single parameter to name to see which begin with '*'.
>
>>
>> Although to be honest, '*bar' in command['arguments'] is reasonable too
>> IMHO.
>
> It may be reasonable, but it forces every QMP client to reimplement the
> post-processing step, rather than presenting the parameter names already
> in isolation.
>
> Food for thought - suppose we wanted to start expressing in the .json
> file what the default value is for any parameter that is listed as
> optional? What representation is most compact for that purpose, while
> still being valid JSON and not requiring QMP clients to reimplement
> post-processing?
We won't do that. It would be way too hard to retrofit it for very
little value.
Regards,
Anthony Liguori
>
> --
> Eric Blake eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH] full introspection support for QMP
Eric Blake writes:
> On 07/02/2013 11:01 AM, Paolo Bonzini wrote:
Arguably that rule of thumb would apply equally to the QEMU
build scripts which already parse qapi-schema.json. It could
be possible to normalize qapi-schema.json somewhat to remove
this 2-stage parsing if we went down this route.
>>>
>>> Indeed, I wouldn't mind a one-time pass over qapi-schema.json to make it
>>> follow a more rigid format if that made it easier to use it as-is with
>>> less post-processing. It won't be very nice to backport such a
>>> conversion, but I don't know how much distros are planning on
>>> backporting introspection in the first place.
>>
>> How would the schema look like after this "one-time pass"?
>>
>> This:
>>
>>> [
>>> { "name": "protocol",
>>>"type": "str" },
>>> { "name": "fdname",
>>>"type": "str" },
>>> { "name": "skipauth",
>>>"type": "bool",
>>>"optional": true },
>>> { "name": "tls",
>>>"type": "bool",
>>>"optional": true }
>>> ]
>>
>> Looks quite awful for a human to write and read.
>
> Which puts us back in favor of my original argument that keeping
> qapi-schema.json compact for human use,
qapi-schema.json is not "for human use". It's consumed by QAPI. It
happens to be reasonably human readable but that's just a happy
coincidence and because we document and format the heck out of it.
> while expanding the QMP output
> to be verbose for machine use, is probably what we'll have to live with.
> While it is easy to document shortcuts that the qapi parser can use
> when converting .json to code, it is harder to require that all other
> QMP clients must implement those same shortcuts, instead of having
> things already directly represented.
qapi-schema.json is valid JSON except for the # style comments. Those
are trivial to remove and we could simply post process it.
Heck, if you use json_parser to parse the file, you'd end up with a list
of QObjects that could be returned directly via QMP.
I don't understand what all this talk of "post-processing" is.
Regards,
Anthony Liguori
>
> --
> Eric Blake eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH] full introspection support for QMP
Eric Blake writes:
> On 07/02/2013 09:39 AM, Daniel P. Berrange wrote:
Maybe I'm being too meta here, but why not just return qapi-schema.json
as a string and call it as day?
>>>
>>> Because qapi-schema.json requires further parsing. For example, how is
>>> a client supposed to know that '*foo':'int' means that there is an
>>> argument named 'foo' but it is optional? The rule of thumb with QMP is
>>> that if you have to post-process JSON output, then the JSON was not
>>> designed correctly.
>>
>> Arguably that rule of thumb would apply equally to the QEMU
>> build scripts which already parse qapi-schema.json. It could
>> be possible to normalize qapi-schema.json somewhat to remove
>> this 2-stage parsing if we went down this route.
>
> Indeed, I wouldn't mind a one-time pass over qapi-schema.json to make it
> follow a more rigid format if that made it easier to use it as-is with
> less post-processing. It won't be very nice to backport such a
> conversion, but I don't know how much distros are planning on
> backporting introspection in the first place.
We consume the schema in QEMU. No reason for us to consume it in a
different format than libvirt.
We're all after the same thing.
Regards,
Anthony Liguori
>
>>
>> I'm finding it hard to clearly see what the 2 different proposed
>> data formats look like against each other. Can someone give some
>> examples, showing the data that would need to be parsed in each
>> format, for a couple of examples.
>
> My proposal (but not quite what was implemented in _this_ version of
> Amos' patch) has been:
>
> qapi-schema.json:
>
> { 'command': 'add_client',
> 'data': { 'protocol': 'str', 'fdname': 'str', '*skipauth': 'bool',
> '*tls': 'bool' } }
>
> My proposal:
>
>
> => { "execute": "query-qmp-schema",
> "arguments": { "name": "add_client" } }
> <= { "return": [
> { "name": "add_client",
>"type": "command",
>"data": [
> { "name": "protocol",
>"type": "str" },
> { "name": "fdname",
>"type": "str" },
> { "name": "skipauth",
>"type": "bool",
>"optional": true },
> { "name": "tls",
>"type": "bool",
>"optional": true }
>] } ] }
>
> Note that one other benefit of breaking out "optional" into its own dict
> member: we are free to expand the dict to add new members, such as a
> '*default':'str' member (present when "optional":true, and with the
> stringized value of the default value used if "name" is not present).
> Of course, to be able to express a default value, we already have to
> modify the syntax stored in qapi-schema.json in the first place, which
> takes us back to the argument of a one-time conversion of the .json file
> to actually use a more formal typing system in the first place.
I really think that we want schema introspection to return the exact
content of qapi-schema.json.
I don't think we need an overly verbose schema though and I don't really
understand why { 'name': 'tls', 'type': 'bool', 'optional': true } is
better than { '*name': 'bool' }
Regards,
Anthony Liguori
>
> --
> Eric Blake eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [Xen-devel] [PATCH] Citrix PV Bus device [V3]
On Tue, 2 Jul 2013, Stefano Stabellini wrote: > - the new device should have configurable vendor and device ids, so that > host admins can select which vendor's PV drivers are going to be > automatically installed on all your Windows guests. This should probably > be a VM config option (pvdevice=, libxl would > then pass a vendor and device id pair to QEMU via command line. > We can come up with a config syntax that would support both numeric > pairs as well as simple labels. Anthony, would you be happy to have a PCI device in QEMU with configurable vendor and device IDs? If not, would you be OK with a PCI device with the Xen vendor ID but a configurable device ID?
Re: [Qemu-devel] [PATCH] full introspection support for QMP
On 07/02/2013 11:06 AM, Anthony Liguori wrote:
>> Because qapi-schema.json requires further parsing. For example, how is
>> a client supposed to know that '*foo':'int' means that there is an
>> argument named 'foo' but it is optional? The rule of thumb with QMP is
>> that if you have to post-process JSON output, then the JSON was not
>> designed correctly.
>
> Then we should fix qapi-schema.json.
>
> One reasonable compromise would be:
>
> { "command": "foo", "arguments": { "name": "str", "id": "int" },
> "optional": { "bar": "bool" } }
>
> Then libvirt only has to test 'does command["optional"] contain key
> "bar"' to test for an optional parameter.
Yes, that might be a reasonable compromise - at least that way, the
optional parameter names are listed directly, and libvirt doesn't have
to probe every single parameter to name to see which begin with '*'.
>
> Although to be honest, '*bar' in command['arguments'] is reasonable too
> IMHO.
It may be reasonable, but it forces every QMP client to reimplement the
post-processing step, rather than presenting the parameter names already
in isolation.
Food for thought - suppose we wanted to start expressing in the .json
file what the default value is for any parameter that is listed as
optional? What representation is most compact for that purpose, while
still being valid JSON and not requiring QMP clients to reimplement
post-processing?
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] full introspection support for QMP
On 07/02/2013 11:01 AM, Paolo Bonzini wrote:
>>> Arguably that rule of thumb would apply equally to the QEMU
>>> build scripts which already parse qapi-schema.json. It could
>>> be possible to normalize qapi-schema.json somewhat to remove
>>> this 2-stage parsing if we went down this route.
>>
>> Indeed, I wouldn't mind a one-time pass over qapi-schema.json to make it
>> follow a more rigid format if that made it easier to use it as-is with
>> less post-processing. It won't be very nice to backport such a
>> conversion, but I don't know how much distros are planning on
>> backporting introspection in the first place.
>
> How would the schema look like after this "one-time pass"?
>
> This:
>
>> [
>> { "name": "protocol",
>>"type": "str" },
>> { "name": "fdname",
>>"type": "str" },
>> { "name": "skipauth",
>>"type": "bool",
>>"optional": true },
>> { "name": "tls",
>>"type": "bool",
>>"optional": true }
>> ]
>
> Looks quite awful for a human to write and read.
Which puts us back in favor of my original argument that keeping
qapi-schema.json compact for human use, while expanding the QMP output
to be verbose for machine use, is probably what we'll have to live with.
While it is easy to document shortcuts that the qapi parser can use
when converting .json to code, it is harder to require that all other
QMP clients must implement those same shortcuts, instead of having
things already directly represented.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] full introspection support for QMP
Eric Blake writes:
> On 07/02/2013 08:51 AM, Anthony Liguori wrote:
>> Amos Kong writes:
>>
>>> Introduces new monitor command to query QMP schema information,
>>> the return data is a nested dict/list, it contains the useful
>>> metadata.
>>>
>>> we can add events definations to qapi-schema.json, then it can
>>> also be queried.
>>>
>>> Signed-off-by: Amos Kong
>>
>> Maybe I'm being too meta here, but why not just return qapi-schema.json
>> as a string and call it as day?
>
> Because qapi-schema.json requires further parsing. For example, how is
> a client supposed to know that '*foo':'int' means that there is an
> argument named 'foo' but it is optional? The rule of thumb with QMP is
> that if you have to post-process JSON output, then the JSON was not
> designed correctly.
Then we should fix qapi-schema.json.
>> It's JSON already and since QMP is JSON, the client already has a JSON
>> parser. Adding another level of complexity doesn't add much value IMHO.
>
> qapi-schema.json is not quite JSON, in that it has #comments that we'd
> have to strip before we attempted a trick like this.
Pretty easy to filter that out...
> I've also been the one arguing that the additional complexity (an array of
> {"name":"str","type":"str","optional":bool"}) is better for libvirt in
> that the JSON is then well-suited for scanning (it is easier to scan
> through an array where the key is a constant "name", and looking for the
> value that we are interested in, than it is to scan through a dictionary
> where the keys of the dictionary are the names we are interested in).
> That is, the JSON in qapi-schema.json is a nice compact representation
> that works for humans, but may be a bit TOO compact for handling via
> machines.
But adding a bunch of code to do JSON translation just adds a bunch of
additional complexity.
One reasonable compromise would be:
{ "command": "foo", "arguments": { "name": "str", "id": "int" },
"optional": { "bar": "bool" } }
Then libvirt only has to test 'does command["optional"] contain key
"bar"' to test for an optional parameter.
Although to be honest, '*bar' in command['arguments'] is reasonable too
IMHO.
Regards,
Anthony Liguori
>
> --
> Eric Blake eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH] full introspection support for QMP
> > Arguably that rule of thumb would apply equally to the QEMU
> > build scripts which already parse qapi-schema.json. It could
> > be possible to normalize qapi-schema.json somewhat to remove
> > this 2-stage parsing if we went down this route.
>
> Indeed, I wouldn't mind a one-time pass over qapi-schema.json to make it
> follow a more rigid format if that made it easier to use it as-is with
> less post-processing. It won't be very nice to backport such a
> conversion, but I don't know how much distros are planning on
> backporting introspection in the first place.
How would the schema look like after this "one-time pass"?
This:
> [
> { "name": "protocol",
>"type": "str" },
> { "name": "fdname",
>"type": "str" },
> { "name": "skipauth",
>"type": "bool",
>"optional": true },
> { "name": "tls",
>"type": "bool",
>"optional": true }
> ]
Looks quite awful for a human to write and read.
Paolo
[Qemu-devel] [PATCH v2] semaphore: fix a hangup problem under load on NetBSD hosts.
Fix following bugs in "fallback implementation of counting semaphores
with mutex+condvar" added in c166cb72f1676855816340666c3b618beef4b976:
- waiting threads are not restarted properly if more than one threads
are waiting unblock signals in qemu_sem_timedwait()
- possible missing pthread_cond_signal(3) calls when waiting threads
are returned by ETIMEDOUT
- fix an uninitialized variable
The problem is analyzed by and fix is provided by Noriyuki Soda.
Also put additional cleanup suggested by Laszlo Ersek:
- make QemuSemaphore.count unsigned (it won't be negative)
- check a return value of in pthread_cond_wait() in qemu_sem_wait()
Signed-off-by: Izumi Tsutsui
Reviewed-by: Laszlo Ersek
---
v2:
- make QemuSemaphore.count unsigned (it won't be negative)
- also eliminate checks for negative count values
- check a return value of in pthread_cond_wait() in qemu_sem_wait()
include/qemu/thread-posix.h | 2 +-
util/qemu-thread-posix.c| 26 +++---
2 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/include/qemu/thread-posix.h b/include/qemu/thread-posix.h
index 0f30dcc..361566a 100644
--- a/include/qemu/thread-posix.h
+++ b/include/qemu/thread-posix.h
@@ -15,7 +15,7 @@ struct QemuSemaphore {
#if defined(__APPLE__) || defined(__NetBSD__)
pthread_mutex_t lock;
pthread_cond_t cond;
-int count;
+unsigned int count;
#else
sem_t sem;
#endif
diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
index 4489abf..44f6f30 100644
--- a/util/qemu-thread-posix.c
+++ b/util/qemu-thread-posix.c
@@ -170,12 +170,11 @@ void qemu_sem_post(QemuSemaphore *sem)
#if defined(__APPLE__) || defined(__NetBSD__)
pthread_mutex_lock(&sem->lock);
-if (sem->count == INT_MAX) {
+if (sem->count == UINT_MAX) {
rc = EINVAL;
-} else if (sem->count++ < 0) {
-rc = pthread_cond_signal(&sem->cond);
} else {
-rc = 0;
+sem->count++;
+rc = pthread_cond_signal(&sem->cond);
}
pthread_mutex_unlock(&sem->lock);
if (rc != 0) {
@@ -207,19 +206,21 @@ int qemu_sem_timedwait(QemuSemaphore *sem, int ms)
struct timespec ts;
#if defined(__APPLE__) || defined(__NetBSD__)
+rc = 0;
compute_abs_deadline(&ts, ms);
pthread_mutex_lock(&sem->lock);
---sem->count;
-while (sem->count < 0) {
+while (sem->count == 0) {
rc = pthread_cond_timedwait(&sem->cond, &sem->lock, &ts);
if (rc == ETIMEDOUT) {
-++sem->count;
break;
}
if (rc != 0) {
error_exit(rc, __func__);
}
}
+if (rc != ETIMEDOUT) {
+--sem->count;
+}
pthread_mutex_unlock(&sem->lock);
return (rc == ETIMEDOUT ? -1 : 0);
#else
@@ -249,16 +250,19 @@ int qemu_sem_timedwait(QemuSemaphore *sem, int ms)
void qemu_sem_wait(QemuSemaphore *sem)
{
+int rc;
+
#if defined(__APPLE__) || defined(__NetBSD__)
pthread_mutex_lock(&sem->lock);
---sem->count;
-while (sem->count < 0) {
+while (sem->count == 0) {
pthread_cond_wait(&sem->cond, &sem->lock);
+if (rc != 0) {
+error_exit(rc, __func__);
+}
}
+--sem->count;
pthread_mutex_unlock(&sem->lock);
#else
-int rc;
-
do {
rc = sem_wait(&sem->sem);
} while (rc == -1 && errno == EINTR);
--
1.8.0.1
Re: [Qemu-devel] [PATCH] full introspection support for QMP
On 07/02/2013 09:39 AM, Daniel P. Berrange wrote:
>>> Maybe I'm being too meta here, but why not just return qapi-schema.json
>>> as a string and call it as day?
>>
>> Because qapi-schema.json requires further parsing. For example, how is
>> a client supposed to know that '*foo':'int' means that there is an
>> argument named 'foo' but it is optional? The rule of thumb with QMP is
>> that if you have to post-process JSON output, then the JSON was not
>> designed correctly.
>
> Arguably that rule of thumb would apply equally to the QEMU
> build scripts which already parse qapi-schema.json. It could
> be possible to normalize qapi-schema.json somewhat to remove
> this 2-stage parsing if we went down this route.
Indeed, I wouldn't mind a one-time pass over qapi-schema.json to make it
follow a more rigid format if that made it easier to use it as-is with
less post-processing. It won't be very nice to backport such a
conversion, but I don't know how much distros are planning on
backporting introspection in the first place.
>
> I'm finding it hard to clearly see what the 2 different proposed
> data formats look like against each other. Can someone give some
> examples, showing the data that would need to be parsed in each
> format, for a couple of examples.
My proposal (but not quite what was implemented in _this_ version of
Amos' patch) has been:
qapi-schema.json:
{ 'command': 'add_client',
'data': { 'protocol': 'str', 'fdname': 'str', '*skipauth': 'bool',
'*tls': 'bool' } }
My proposal:
=> { "execute": "query-qmp-schema",
"arguments": { "name": "add_client" } }
<= { "return": [
{ "name": "add_client",
"type": "command",
"data": [
{ "name": "protocol",
"type": "str" },
{ "name": "fdname",
"type": "str" },
{ "name": "skipauth",
"type": "bool",
"optional": true },
{ "name": "tls",
"type": "bool",
"optional": true }
] } ] }
Note that one other benefit of breaking out "optional" into its own dict
member: we are free to expand the dict to add new members, such as a
'*default':'str' member (present when "optional":true, and with the
stringized value of the default value used if "name" is not present).
Of course, to be able to express a default value, we already have to
modify the syntax stored in qapi-schema.json in the first place, which
takes us back to the argument of a one-time conversion of the .json file
to actually use a more formal typing system in the first place.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [Xen-devel] [PATCH] Citrix PV Bus device [V3]
On Tue, 2 Jul 2013, Paul Durrant wrote:
> This patch introduces a new PCI device which will act as the binding point
> for Citrix branded PV drivers for Xen.
> The intention is that Citrix Windows PV drivers will be available on Windows
> Update and thus using the existing Xen platform PCI device as an anchor
> point is not desirable as that device has been ubiquitous in HVM guests for
> a long time and thus existing HVM guests running Windows would start
> automatically downloading drivers from Windows Update when this may not be
> desired by either the host or guest admin. This device therefore acts as
> an opt-in for those wishing to deploy Citrix PV drivers.
>
> V3:
> - Addresses comments from Anthony Liguori and Peter Maydell
>
> V2:
> - Addresses comments from Andreas Farber and Paolo Bonzini
>
> Signed-off-by: Paul Durrant
I do realize that this is v3 and I didn't step in the discussion
before, sorry for the delay in my response. I admit I didn't read the
entire thread, so excuse me if I repeat things already said before.
> hw/misc/Makefile.objs|2 +
> hw/misc/citrix_pv_bus.c | 127
> ++
> include/hw/pci/pci_ids.h |7 ++-
> trace-events |4 ++
> 4 files changed, 138 insertions(+), 2 deletions(-)
> create mode 100644 hw/misc/citrix_pv_bus.c
>
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index 2578e29..ffd29ea 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -41,3 +41,5 @@ obj-$(CONFIG_SLAVIO) += slavio_misc.o
> obj-$(CONFIG_ZYNQ) += zynq_slcr.o
>
> obj-$(CONFIG_PVPANIC) += pvpanic.o
> +
> +obj-$(CONFIG_XEN) += citrix-pv-bus.o
> diff --git a/hw/misc/citrix_pv_bus.c b/hw/misc/citrix_pv_bus.c
> new file mode 100644
> index 000..76e5e13
> --- /dev/null
> +++ b/hw/misc/citrix_pv_bus.c
> @@ -0,0 +1,127 @@
> +
> +/* Copyright (c) Citrix Systems Inc.
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms,
> + * with or without modification, are permitted provided
> + * that the following conditions are met:
> + *
> + * * Redistributions of source code must retain the above
> + * copyright notice, this list of conditions and the
> + * following disclaimer.
> + * * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the
> + * following disclaimer in the documentation and/or other
> + * materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> + * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
> + * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR
> + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
> + * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
> + * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
> + * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
> + * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +
> +#include "hw/hw.h"
> +#include "hw/pci/pci.h"
> +#include "trace.h"
> +
> +#define TYPE_CITRIX_PV_BUS_DEVICE "citrix-pv"
> +
> +#define CITRIX_PV_BUS_DEVICE(obj) \
> + OBJECT_CHECK(CitrixPVBusDevice, (obj), TYPE_CITRIX_PV_BUS_DEVICE)
> +
> +typedef struct CitrixPVBusDevice {
> +/*< private >*/
> +PCIDevice parent_obj;
> +/*< public >*/
> +uint8_t revision;
> +uint32_tsize;
> +MemoryRegionmmio;
> +} CitrixPVBusDevice;
> +
> +static uint64_t citrix_pv_bus_mmio_read(void *opaque, hwaddr addr,
> +unsigned size)
> +{
> +trace_citrix_pv_bus_mmio_read(addr);
> +
> +return ~(uint64_t)0;
> +}
> +
> +static void citrix_pv_bus_mmio_write(void *opaque, hwaddr addr,
> + uint64_t val, unsigned size)
> +{
> +trace_citrix_pv_bus_mmio_write(addr);
> +}
> +
> +static const MemoryRegionOps citrix_pv_bus_mmio_ops = {
> +.read = &citrix_pv_bus_mmio_read,
> +.write = &citrix_pv_bus_mmio_write,
> +.endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static int citrix_pv_bus_init(PCIDevice *pci_dev)
> +{
> +CitrixPVBusDevice *d = CITRIX_PV_BUS_DEVICE(pci_dev);
> +uint8_t *pci_conf;
> +
> +pci_conf = pci_dev->config;
> +
> +pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_MEMORY);
> +pci_set_byte(pci_conf + PCI_REVISION_ID, d->revision);
> +
> +pci_config_set_prog_interface(pci_conf, 0);
> +
> +pci_conf[PCI_INTERRUPT_PIN] = 1;
> +
> +memory_region_init_io(&d->mmio, &citrix_pv_bus_mmio_ops, d,
> + "c
Re: [Qemu-devel] [PATCH] full introspection support for QMP
On 06/19/2013 06:49 AM, Amos Kong wrote:
> On Wed, Jun 19, 2013 at 08:24:37PM +0800, Amos Kong wrote:
>> Introduces new monitor command to query QMP schema information,
>> the return data is a nested dict/list, it contains the useful
>> metadata.
>>
>> we can add events definations to qapi-schema.json, then it can
>> also be queried.
>
> I didn't implement to return complete schema in one go in this
> version, will do it in next version. We have a recursive define
> 'DataObject', we only display one layer for it.
Yes, for recursive definitions, we have to stop somewhere.
>
> You can find three kind of examples(string/list/dict) in the bottom.
> Attached (query-qmp-schema--output.txt) the full output of execut
> query-qmp-schema command.
>
> String:
> { 'command': 'query-name', 'returns': 'NameInfo' }
> -
> {
> "name": "NameInfo",
> "type": "Type",
> "data": [
> {
> "name": "*name",
> "type": "str"
> }
> ]
> },
This output still requires post-processing - the user has to parse the
key "*name" to learn two bits of information - whether the parameter is
optional, and the fact that it is named "name" not "*name". I _still_
argue that we want to return 3 things, not two, as in:
"data": [
{
"name": "name",
"optional": true,
"type": "str"
}
]
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] qom: Use atomics for object refcounting
Paolo Bonzini writes:
> Il 02/07/2013 16:47, Anthony Liguori ha scritto:
>> Jan Kiszka writes:
>>
>>> Objects can soon be referenced/dereference outside the BQL. So we need
>>> to use atomics in object_ref/unref.
>>>
>>> Based on patch by Liu Ping Fan.
>>>
>>> Signed-off-by: Jan Kiszka
>>> ---
>>> qom/object.c |5 ++---
>>> 1 files changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/qom/object.c b/qom/object.c
>>> index 803b94b..a76a30b 100644
>>> --- a/qom/object.c
>>> +++ b/qom/object.c
>>> @@ -683,16 +683,15 @@ GSList *object_class_get_list(const char
>>> *implements_type,
>>>
>>> void object_ref(Object *obj)
>>> {
>>> -obj->ref++;
>>> + __sync_fetch_and_add(&obj->ref, 1);
>>> }
>>>
>>> void object_unref(Object *obj)
>>> {
>>> g_assert(obj->ref > 0);
>>> -obj->ref--;
>>>
>>> /* parent always holds a reference to its children */
>>> -if (obj->ref == 0) {
>>> +if (__sync_sub_and_fetch(&obj->ref, 1) == 0) {
>>> object_finalize(obj);
>>> }
>>> }
>>
>> Should we introduce something akin to kref now that referencing counting
>> has gotten fancy?
>
> I'm not a big fan of kref (it seems _too_ thin a wrapper to me, i.e. it
> doesn't really wrap enough to be useful), but I wouldn't oppose it if
> someone else does it.
I had honestly hoped Object was light enough to be used for this
purpose. What do you think?
Regards,
Anthony Liguori
>
> Paolo
Re: [Qemu-devel] [PATCH v3] linux-user: improve target_to_host_sock_type conversion
On 2 July 2013 15:13, Petar Jovanovic wrote: > Yet, I ended up changing 80% of the header file with this version, so I > was under impression it makes sense to change the remaining 20%. Yeah, that's fine -- but once you get to that scale of change it's nicer to have patch 1/2 fix formatting, patch 2/2 actual change. (Just a note for next time at this point, though.) >> Not using the same base as the kernel (hex vs octal) made checking >> the values a bit tedious too, but I think they're all correct. > > What values are you refering to? arch/sparc/include/uapi/asm/fcntl.h:#define O_CLOEXEC 0x40 arch/alpha/include/asm/socket.h:#define SOCK_NONBLOCK 0x4000 arch/mips/include/uapi/asm/fcntl.h:#define O_NONBLOCK 0x0080 arch/sparc/include/uapi/asm/fcntl.h:#define O_NONBLOCK 0x4000 thanks -- PMM
Re: [Qemu-devel] help me for nahanni device
Hello Dai, Which qemu version you are using? When you say QEMU/KVM "doesn't work", do you mean it does not start? Cam On Mon, Jul 1, 2013 at 12:39 AM, DAI Weibin wrote: > Hi all, > When use nahanni device as following, The QEMU/KVM doesn’t work. > I attach two ivshmem devices to VM, QEMU/KVM only supports a ivshmem > device, why? > Expect your reply! > > > *-device > virtio-net-pci,vlan=0,id=net0,mac=DE:AD:BE:09:03:01,bus=pci.0,addr=0x5 \* > *-net tap,script=/etc/qemu-ifup \* > *-device ivshmem,size=256m,shm=l1shm \* > *-device ivshmem,size=256m,shm=l2shm \* > *-usbdevice tablet \* > *-vnc :2 \* > *-daemonize* > > Best regards > 戴卫彬 > > > >
Re: [Qemu-devel] [Xen-devel] [PATCH] Citrix PV Bus device
Ian, --On 2 July 2013 14:42:41 +0100 Ian Campbell wrote: Let's say we do this, then Xirtic also produce PV drivers, and get assigned their own device ID. Will both of them be visible with a different device ID for the same device? Different device ID for the same device isn't possible, a device has exactly one device ID. I assume you mean two devices visible one with each ID? Yes. Two PCI devices talking to the same underlying thing. Will windows cope with that? Or do we need to mediate which device IDs are exposed? I'd expect that only Xirtic products would provide a device with that device ID. They may choose to make it mutually exclusive with the standard device, or they may choose to allow it to co-exist. In the latter case it is down to them what the behaviour of their drivers is. In any case this is all down to the people who make that product and their requirements etc. For people running Xirtic drivers on non-Xirtic products the rest of my mail applies -- IOW those users would use a setup.exe which binds those drivers to the normal device, there is no Xirtic device ID present anywhere in this scenario. OK. The thing I was suggesting we try to avoid was Citrix drivers binding to PCI-ID A and Xirtic drivers binding to PCI-ID B, and them both trying to work at once. Perhaps I should stop worrying about this as I'm finding it really hard to resist typing 'so don't use Windows' :-) -- Alex Bligh
Re: [Qemu-devel] Which part of qemu responds to ACPI control method?
On 07/02/13 15:05, bobooscar wrote: > > Hi,all: > When a guest domain excutes a control method, such as “_PTS”, which > part of qemu would respond and handle the request? thank you in advance. IMHO, like any other method, _PTS (Prepare To Sleep) is executed by the guest's ACPI interpreter, and qemu responds only to the hardware accesses that the method makes. Laszlo
Re: [Qemu-devel] [PATCH 25/30] exec: put memory map in AddressSpaceDispatch
On 2013-07-02 17:08, Paolo Bonzini wrote:
> Il 02/07/2013 16:42, Jan Kiszka ha scritto:
+typedef PhysPageEntry Node[L2_SIZE];
+
struct AddressSpaceDispatch {
/* This is a multi-level map on the physical address space.
* The bottom level has pointers to MemoryRegionSections.
*/
PhysPageEntry phys_map;
+Node *nodes;
+MemoryRegionSection *sections;
>> Why not sticking the whole current PhysPageMap into here? Wouldn't that
>> also allow to overcome prev_map completely (next patch)?
>
> (BTW, this is also why patch 21 has two separate arguments to
> phys_page_find).
>
> Sticking a pointer would add one useless pointer chase. Storing it by
> value makes it unclear who owns the PhysPageMap and doesn't say that
> AddressSpaceDispatch cannot extend the vectors. In the end, this seemed
> like a good compromise.
Well, PhysPageMap is still globally owned. And as long as this is not
broken up, you are likely right. So let's wait with this for the next
refactoring round.
Jan
--
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCH] semaphore: fix a hangup problem under loadon NetBSD hosts.
On 07/02/13 17:27, Izumi Tsutsui wrote: > Laszlo Ersek wrote: >> Conversely, the only time we need to send a signal is the 0->1 count >> transition (*). > > Per comments from Soda, signals could be required even on count >0, > if more than one threads are sleeping in qemu_cond_timedwait(), > and more than one qemu_sem_post() are called at once, then > the second qemu_sem_post() gets the mutex before sleeping threads > in qemu_sem_timedwait(). You're right. Laszlo
[Qemu-devel] Which part of qemu responds to ACPI control method?
Hi,all: When a guest domain excutes a control method, such as “_PTS”, which part of qemu would respond and handle the request? thank you in advance. 已从三星手机发送
Re: [Qemu-devel] [PATCH] full introspection support for QMP
On Tue, Jul 02, 2013 at 09:28:51AM -0600, Eric Blake wrote:
> On 07/02/2013 08:51 AM, Anthony Liguori wrote:
> > Amos Kong writes:
> >
> >> Introduces new monitor command to query QMP schema information,
> >> the return data is a nested dict/list, it contains the useful
> >> metadata.
> >>
> >> we can add events definations to qapi-schema.json, then it can
> >> also be queried.
> >>
> >> Signed-off-by: Amos Kong
> >
> > Maybe I'm being too meta here, but why not just return qapi-schema.json
> > as a string and call it as day?
>
> Because qapi-schema.json requires further parsing. For example, how is
> a client supposed to know that '*foo':'int' means that there is an
> argument named 'foo' but it is optional? The rule of thumb with QMP is
> that if you have to post-process JSON output, then the JSON was not
> designed correctly.
Arguably that rule of thumb would apply equally to the QEMU
build scripts which already parse qapi-schema.json. It could
be possible to normalize qapi-schema.json somewhat to remove
this 2-stage parsing if we went down this route.
> > It's JSON already and since QMP is JSON, the client already has a JSON
> > parser. Adding another level of complexity doesn't add much value IMHO.
>
> qapi-schema.json is not quite JSON, in that it has #comments that we'd
> have to strip before we attempted a trick like this. I've also been the
> one arguing that the additional complexity (an array of
> {"name":"str","type":"str","optional":bool"}) is better for libvirt in
> that the JSON is then well-suited for scanning (it is easier to scan
> through an array where the key is a constant "name", and looking for the
> value that we are interested in, than it is to scan through a dictionary
> where the keys of the dictionary are the names we are interested in).
> That is, the JSON in qapi-schema.json is a nice compact representation
> that works for humans, but may be a bit TOO compact for handling via
> machines.
I'm finding it hard to clearly see what the 2 different proposed
data formats look like against each other. Can someone give some
examples, showing the data that would need to be parsed in each
format, for a couple of examples.
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
[Qemu-devel] [PATCH] Citrix PV Bus device [V3]
This patch introduces a new PCI device which will act as the binding point
for Citrix branded PV drivers for Xen.
The intention is that Citrix Windows PV drivers will be available on Windows
Update and thus using the existing Xen platform PCI device as an anchor
point is not desirable as that device has been ubiquitous in HVM guests for
a long time and thus existing HVM guests running Windows would start
automatically downloading drivers from Windows Update when this may not be
desired by either the host or guest admin. This device therefore acts as
an opt-in for those wishing to deploy Citrix PV drivers.
V3:
- Addresses comments from Anthony Liguori and Peter Maydell
V2:
- Addresses comments from Andreas Farber and Paolo Bonzini
Signed-off-by: Paul Durrant
---
hw/misc/Makefile.objs|2 +
hw/misc/citrix_pv_bus.c | 127 ++
include/hw/pci/pci_ids.h |7 ++-
trace-events |4 ++
4 files changed, 138 insertions(+), 2 deletions(-)
create mode 100644 hw/misc/citrix_pv_bus.c
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 2578e29..ffd29ea 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -41,3 +41,5 @@ obj-$(CONFIG_SLAVIO) += slavio_misc.o
obj-$(CONFIG_ZYNQ) += zynq_slcr.o
obj-$(CONFIG_PVPANIC) += pvpanic.o
+
+obj-$(CONFIG_XEN) += citrix-pv-bus.o
diff --git a/hw/misc/citrix_pv_bus.c b/hw/misc/citrix_pv_bus.c
new file mode 100644
index 000..76e5e13
--- /dev/null
+++ b/hw/misc/citrix_pv_bus.c
@@ -0,0 +1,127 @@
+
+/* Copyright (c) Citrix Systems Inc.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms,
+ * with or without modification, are permitted provided
+ * that the following conditions are met:
+ *
+ * * Redistributions of source code must retain the above
+ * copyright notice, this list of conditions and the
+ * following disclaimer.
+ * * Redistributions in binary form must reproduce the above
+ * copyright notice, this list of conditions and the
+ * following disclaimer in the documentation and/or other
+ * materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
+ * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+ * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+ * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
+ * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+ * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
+ * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
+ * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include "hw/hw.h"
+#include "hw/pci/pci.h"
+#include "trace.h"
+
+#define TYPE_CITRIX_PV_BUS_DEVICE "citrix-pv"
+
+#define CITRIX_PV_BUS_DEVICE(obj) \
+ OBJECT_CHECK(CitrixPVBusDevice, (obj), TYPE_CITRIX_PV_BUS_DEVICE)
+
+typedef struct CitrixPVBusDevice {
+/*< private >*/
+PCIDevice parent_obj;
+/*< public >*/
+uint8_t revision;
+uint32_tsize;
+MemoryRegionmmio;
+} CitrixPVBusDevice;
+
+static uint64_t citrix_pv_bus_mmio_read(void *opaque, hwaddr addr,
+unsigned size)
+{
+trace_citrix_pv_bus_mmio_read(addr);
+
+return ~(uint64_t)0;
+}
+
+static void citrix_pv_bus_mmio_write(void *opaque, hwaddr addr,
+ uint64_t val, unsigned size)
+{
+trace_citrix_pv_bus_mmio_write(addr);
+}
+
+static const MemoryRegionOps citrix_pv_bus_mmio_ops = {
+.read = &citrix_pv_bus_mmio_read,
+.write = &citrix_pv_bus_mmio_write,
+.endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static int citrix_pv_bus_init(PCIDevice *pci_dev)
+{
+CitrixPVBusDevice *d = CITRIX_PV_BUS_DEVICE(pci_dev);
+uint8_t *pci_conf;
+
+pci_conf = pci_dev->config;
+
+pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_MEMORY);
+pci_set_byte(pci_conf + PCI_REVISION_ID, d->revision);
+
+pci_config_set_prog_interface(pci_conf, 0);
+
+pci_conf[PCI_INTERRUPT_PIN] = 1;
+
+memory_region_init_io(&d->mmio, &citrix_pv_bus_mmio_ops, d,
+ "citrix-bus-mmio", d->size);
+
+pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_MEM_PREFETCH,
+ &d->mmio);
+
+return 0;
+}
+
+static Property citrix_pv_bus_props[] = {
+DEFINE_PROP_UINT8("revision", CitrixPVBusDevice, revision, 0x01),
+DEFINE_PROP_UINT32("size", CitrixPVBusDevice, size, 0x40),
+DEFINE_PROP_END_OF_LIST()
+};
+
+static void citrix_pv_bus_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+PCIDevi
Re: [Qemu-devel] [PATCH] qom: Use atomics for object refcounting
Il 02/07/2013 16:47, Anthony Liguori ha scritto:
> Jan Kiszka writes:
>
>> Objects can soon be referenced/dereference outside the BQL. So we need
>> to use atomics in object_ref/unref.
>>
>> Based on patch by Liu Ping Fan.
>>
>> Signed-off-by: Jan Kiszka
>> ---
>> qom/object.c |5 ++---
>> 1 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/qom/object.c b/qom/object.c
>> index 803b94b..a76a30b 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -683,16 +683,15 @@ GSList *object_class_get_list(const char
>> *implements_type,
>>
>> void object_ref(Object *obj)
>> {
>> -obj->ref++;
>> + __sync_fetch_and_add(&obj->ref, 1);
>> }
>>
>> void object_unref(Object *obj)
>> {
>> g_assert(obj->ref > 0);
>> -obj->ref--;
>>
>> /* parent always holds a reference to its children */
>> -if (obj->ref == 0) {
>> +if (__sync_sub_and_fetch(&obj->ref, 1) == 0) {
>> object_finalize(obj);
>> }
>> }
>
> Should we introduce something akin to kref now that referencing counting
> has gotten fancy?
I'm not a big fan of kref (it seems _too_ thin a wrapper to me, i.e. it
doesn't really wrap enough to be useful), but I wouldn't oppose it if
someone else does it.
Paolo
Re: [Qemu-devel] [PATCH] full introspection support for QMP
On 07/02/2013 08:51 AM, Anthony Liguori wrote:
> Amos Kong writes:
>
>> Introduces new monitor command to query QMP schema information,
>> the return data is a nested dict/list, it contains the useful
>> metadata.
>>
>> we can add events definations to qapi-schema.json, then it can
>> also be queried.
>>
>> Signed-off-by: Amos Kong
>
> Maybe I'm being too meta here, but why not just return qapi-schema.json
> as a string and call it as day?
Because qapi-schema.json requires further parsing. For example, how is
a client supposed to know that '*foo':'int' means that there is an
argument named 'foo' but it is optional? The rule of thumb with QMP is
that if you have to post-process JSON output, then the JSON was not
designed correctly.
>
> It's JSON already and since QMP is JSON, the client already has a JSON
> parser. Adding another level of complexity doesn't add much value IMHO.
qapi-schema.json is not quite JSON, in that it has #comments that we'd
have to strip before we attempted a trick like this. I've also been the
one arguing that the additional complexity (an array of
{"name":"str","type":"str","optional":bool"}) is better for libvirt in
that the JSON is then well-suited for scanning (it is easier to scan
through an array where the key is a constant "name", and looking for the
value that we are interested in, than it is to scan through a dictionary
where the keys of the dictionary are the names we are interested in).
That is, the JSON in qapi-schema.json is a nice compact representation
that works for humans, but may be a bit TOO compact for handling via
machines.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v3] configure: Simplify alternate .text segment
Ping.
On 06/21/2013 07:10 PM, Richard Henderson wrote:
> For bsd-user and linux-user emulation modes QEMU needs to be linked at an
> alternate .text segment address, so that it's out of the way of the guest
> executable. Instead of including modified linker scripts for each arch,
> just set the address with -Ttext-segment if supported, or by using sed to
> edit the default linker script.
>
> Cc: Ed Maste
> Signed-off-by: Richard Henderson
> ---
> configure | 48 +++-
> 2 files changed, 31 insertions(+), 18 deletions(-)
> --
>
> Changes v2-v3:
> * Move the check for textseg_ldflags much earlier in the configure file,
> so that we've not got cflags invalid for configure time. Plus, the
> check (and generated ld script) only gets done once, not once per
> target directory.
> * Remove ia64 from the hosts that get relocation
> * Handle s390x like s390.
>
> Tested on x86_64, arm, hppa (old binutils needing config-host.ld),
> sparc64, and ia64. All various versions of linux.
>
>
> r~
>
>
>
> diff --git a/configure b/configure
> index ad32f87..63da418 100755
> --- a/configure
> +++ b/configure
> @@ -3444,6 +3444,36 @@ if test "$cpu" = "s390x" ; then
>roms="$roms s390-ccw"
> fi
>
> +# Probe for the need for relocating the user-only binary.
> +if test "$pie" = "no" ; then
> + textseg_addr=
> + case "$cpu" in
> +arm | hppa | i386 | m68k | ppc | ppc64 | s390* | sparc | sparc64 |
> x86_64)
> + textseg_addr=0x6000
> + ;;
> +mips)
> + textseg_addr=0x40
> + ;;
> + esac
> + if [ -n "$textseg_addr" ]; then
> +cat > $TMPC < +int main(void) { return 0; }
> +EOF
> +textseg_ldflags="-Wl,-Ttext-segment=$textseg_addr"
> +if ! compile_prog "" "$textseg_ldflags"; then
> + # In case ld does not support -Ttext-segment, edit the default linker
> + # script via sed to set the .text start addr. This is needed on
> FreeBSD
> + # at least.
> + $ld --verbose | sed \
> +-e '1,/==/d' \
> +-e '/==/,$d' \
> +-e "s/[.] = [0-9a-fx]* [+] SIZEOF_HEADERS/. = $textseg_addr +
> SIZEOF_HEADERS/" \
> +-e "s/__executable_start = [0-9a-fx]*/__executable_start =
> $textseg_addr/" > config-host.ld
> + textseg_ldflags="-Wl,-T../config-host.ld"
> +fi
> + fi
> +fi
> +
> # add pixman flags after all config tests are done
> QEMU_CFLAGS="$QEMU_CFLAGS $pixman_cflags $fdt_cflags"
> libs_softmmu="$libs_softmmu $pixman_libs"
> @@ -4072,9 +4102,6 @@ if test "$gcov" = "yes" ; then
>echo "GCOV=$gcov_tool" >> $config_host_mak
> fi
>
> -# generate list of library paths for linker script
> -$ld --verbose -v 2> /dev/null | grep SEARCH_DIR > config-host.ld
> -
> # use included Linux headers
> if test "$linux" = "yes" ; then
>mkdir -p linux-headers
> @@ -4437,21 +4464,8 @@ if test "$gprof" = "yes" ; then
>fi
> fi
>
> -if test "$ARCH" = "tci"; then
> - linker_script=""
> -else
> - linker_script="-Wl,-T../config-host.ld
> -Wl,-T,\$(SRC_PATH)/ldscripts/\$(ARCH).ld"
> -fi
> -
> if test "$target_linux_user" = "yes" -o "$target_bsd_user" = "yes" ; then
> - case "$ARCH" in
> - alpha | s390x | aarch64)
> -# The default placement of the application is fine.
> -;;
> - *)
> -ldflags="$linker_script $ldflags"
> -;;
> - esac
> + ldflags="$ldflags $textseg_ldflags"
> fi
>
> echo "LDFLAGS+=$ldflags" >> $config_target_mak
>
Re: [Qemu-devel] [PATCH] semaphore: fix a hangup problem under loadon NetBSD hosts.
Laszlo Ersek wrote:
> On 06/29/13 12:22, Izumi Tsutsui wrote:
> > Fix following bugs in "fallback implementation of counting semaphores
> > with mutex+condvar" added in c166cb72f1676855816340666c3b618beef4b976:
> > - waiting threads are not restarted properly if more than one threads
> >are waiting unblock signals in qemu_sem_timedwait()
> > - possible missing pthread_cond_signal(3) calls when waiting threads
> >are returned by ETIMEDOUT
> > - fix an uninitialized variable
> >
> > The problem is analyzed by and fix is provided by Noriyuki Soda.
> >
> > Signed-off-by: Izumi Tsutsui
> > ---
> > util/qemu-thread-posix.c | 17 +
> > 1 file changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
> > index 4489abf..db7a15b 100644
> > --- a/util/qemu-thread-posix.c
> > +++ b/util/qemu-thread-posix.c
> > @@ -172,10 +172,9 @@ void qemu_sem_post(QemuSemaphore *sem)
> > pthread_mutex_lock(&sem->lock);
> > if (sem->count == INT_MAX) {
> > rc = EINVAL;
> > -} else if (sem->count++ < 0) {
> > -rc = pthread_cond_signal(&sem->cond);
> > } else {
> > -rc = 0;
> > +sem->count++;
> > +rc = pthread_cond_signal(&sem->cond);
> > }
> > pthread_mutex_unlock(&sem->lock);
> > if (rc != 0) {
> > @@ -207,19 +206,21 @@ int qemu_sem_timedwait(QemuSemaphore *sem, int ms)
> > struct timespec ts;
> >
> > #if defined(__APPLE__) || defined(__NetBSD__)
> > +rc = 0;
> > compute_abs_deadline(&ts, ms);
> > pthread_mutex_lock(&sem->lock);
> > ---sem->count;
> > -while (sem->count < 0) {
> > +while (sem->count <= 0) {
> > rc = pthread_cond_timedwait(&sem->cond, &sem->lock, &ts);
> > if (rc == ETIMEDOUT) {
> > -++sem->count;
> > break;
> > }
> > if (rc != 0) {
> > error_exit(rc, __func__);
> > }
> > }
> > +if (rc != ETIMEDOUT) {
> > +--sem->count;
> > +}
> > pthread_mutex_unlock(&sem->lock);
> > return (rc == ETIMEDOUT ? -1 : 0);
> > #else
> > @@ -251,10 +252,10 @@ void qemu_sem_wait(QemuSemaphore *sem)
> > {
> > #if defined(__APPLE__) || defined(__NetBSD__)
> > pthread_mutex_lock(&sem->lock);
> > ---sem->count;
> > -while (sem->count < 0) {
> > +while (sem->count <= 0) {
> > pthread_cond_wait(&sem->cond, &sem->lock);
> > }
> > +--sem->count;
> > pthread_mutex_unlock(&sem->lock);
> > #else
> > int rc;
> >
>
> I agree with this patch, but I'd propose something more intrusive (feel
> free to ignore it anyway): "QemuSemaphore.count" has no business with
> negative values; it should be an unsigned int.
>
> The condition on which consumers block is exactly (count == 0).
Sure, I'll post an updated patch v2 later.
> Conversely, the only time we need to send a signal is the 0->1 count
> transition (*).
Per comments from Soda, signals could be required even on count >0,
if more than one threads are sleeping in qemu_cond_timedwait(),
and more than one qemu_sem_post() are called at once, then
the second qemu_sem_post() gets the mutex before sleeping threads
in qemu_sem_timedwait().
> Checks for negative values should be eliminated in
> parallel with the int->unsigned type change.
I'll also eliminate them.
> Also I'd feel safer if pthread_cond_*() and pthread_mutex_*() were
> retval-checked consistently, but that's tangential.
I'll add a retval check of pthread_cond_wait() in qemu_sem_wait()
as pthread_cond_timedwait() in qemu_sem_timedwait().
But I'll leave pthread_mutex_{lock,unlock} because there are
many other sources which don't check retvals of them.
> Reviewed-by: Laszlo Ersek
Thanks,
> (*) 100% tangential: this reminds me of when I made an attempt to
> dissect condvars & co on reddit [1]. I considered pthread_cond_signal()
> vs. pthread_cond_broadcast() too; alas my two conclusions there against
> the former were wrong. See [2] why -- in short when a wakeup signal is
> delivered, the victim thread is removed from the set of potential
> victims. In other words, pthread_cond_signal() itself (vs. broadcast)
> *is* correct here.
>
> I also like that the signal is sent with the mutex held [3] [4].
>
> [1]
> http://www.reddit.com/r/programming/comments/9ynxv/utter_verbiage_how_to_design_condition_variables/
> [2]
> http://thread.gmane.org/gmane.comp.standards.posix.austin.general/4844/focus=4850
> [3]
> http://thread.gmane.org/gmane.comp.standards.posix.austin.general/1822/focus=1823
> [4]
> http://www.domaigne.com/blog/computing/condvars-signal-with-mutex-locked-or-not/
>
> Thanks,
> Laszlo
>
---
Izumi Tsutsui
Re: [Qemu-devel] [PATCH v4 08/10] qemu-ga: call Windows VSS requester in fsfreeze command handler
On 07/02/13 17:16, Luiz Capitulino wrote:
> On Tue, 02 Jul 2013 16:44:55 +0200
> Laszlo Ersek wrote:
>
>> On 07/02/13 16:09, Luiz Capitulino wrote:
>>> On Tue, 02 Jul 2013 08:36:11 +0200
>>> Laszlo Ersek wrote:
>>>
On 07/01/13 19:59, Tomoki Sekiyama wrote:
> On 7/1/13 9:29 , "Laszlo Ersek" wrote:
>
>>> +error:
>>> +qmp_guest_fsfreeze_thaw(NULL);
>>
>> Passing NULL here as "errp" concerns me slightly. I've been assuming
>> that "errp" is never NULL due to the outermost QMP layer always wanting
>> to receive errors and to serialize them.
>>
>> Specifically, a NULL "errp" would turn all error_set*(errp, ...) calls
>> into no-ops under qmp_guest_fsfreeze_thaw(), and all error_is_set(errp)
>> questions would answer with false. That said, nothing seems to be
>> affected right now.
>>
>> Maybe a dummy local variable would be more future-proof... OTOH it would
>> be stylistically questionable :)
>
> OK, then it should be:
> Error *local_err = NULL;
> ...
> error:
> qmp_guest_fsfreeze_thaw(&local_err);
> if (error_is_set(&local_err)) {
> error_free(local_err);
> }
I think so, yes.
You could also log it before freeing it I guess:
g_debug("cleanup thaw: %s", error_get_pretty(local_err));
or some such, but it's probably not important.
>>>
>>> I'd rather do something like that, otherwise it doesn't seem to
>>> make sense to pass Error as qmp_guest_fsfreeze_thaw() does
>>> use a local Error.
>>
>> No, the win32 version of qmp_guest_fsfreeze_thaw() being added by this
>> patch doesn't.
>
> Oh, I looked into the POSIX one. But then, it's qmp_guest_fsfreeze_thaw()
> that has to be fixed, isn't it?
I didn't insist on that because the QMP command implementations in the
guest agent are all rooted in the dispatcher, and the dispatcher makes
sure for all commands that errp is never NULL. This is the only call
site where a NULL errp was manually passed, and we're discussing how to
fix it -- use a local_err, and maybe even print it.
Of course I don't insist on *not* reworking it either.
Thanks,
Laszlo
Re: [Qemu-devel] [PATCH] configure: detect endian via compile test
On 06/30/2013 08:30 PM, Mike Frysinger wrote: > This avoids needing to execute a program and keeping an (incomplete) > list when cross-compiling. > > Signed-off-by: Mike Frysinger For the benefit of the list, this is a porting of the autoconf test. Reviewed-by: Richard Henderson r~
Re: [Qemu-devel] [PATCH v4 08/10] qemu-ga: call Windows VSS requester in fsfreeze command handler
On Tue, 02 Jul 2013 16:44:55 +0200
Laszlo Ersek wrote:
> On 07/02/13 16:09, Luiz Capitulino wrote:
> > On Tue, 02 Jul 2013 08:36:11 +0200
> > Laszlo Ersek wrote:
> >
> >> On 07/01/13 19:59, Tomoki Sekiyama wrote:
> >>> On 7/1/13 9:29 , "Laszlo Ersek" wrote:
> >>>
> > +error:
> > +qmp_guest_fsfreeze_thaw(NULL);
>
> Passing NULL here as "errp" concerns me slightly. I've been assuming
> that "errp" is never NULL due to the outermost QMP layer always wanting
> to receive errors and to serialize them.
>
> Specifically, a NULL "errp" would turn all error_set*(errp, ...) calls
> into no-ops under qmp_guest_fsfreeze_thaw(), and all error_is_set(errp)
> questions would answer with false. That said, nothing seems to be
> affected right now.
>
> Maybe a dummy local variable would be more future-proof... OTOH it would
> be stylistically questionable :)
> >>>
> >>> OK, then it should be:
> >>> Error *local_err = NULL;
> >>> ...
> >>> error:
> >>> qmp_guest_fsfreeze_thaw(&local_err);
> >>> if (error_is_set(&local_err)) {
> >>> error_free(local_err);
> >>> }
> >>
> >> I think so, yes.
> >>
> >> You could also log it before freeing it I guess:
> >>
> >> g_debug("cleanup thaw: %s", error_get_pretty(local_err));
> >>
> >> or some such, but it's probably not important.
> >
> > I'd rather do something like that, otherwise it doesn't seem to
> > make sense to pass Error as qmp_guest_fsfreeze_thaw() does
> > use a local Error.
>
> No, the win32 version of qmp_guest_fsfreeze_thaw() being added by this
> patch doesn't.
Oh, I looked into the POSIX one. But then, it's qmp_guest_fsfreeze_thaw()
that has to be fixed, isn't it?
Re: [Qemu-devel] [PATCH v2 0/9] tcg: remainder and arm runtime detection
Ping. On 06/26/2013 01:52 PM, Richard Henderson wrote: > This patch set includes both the remainder series and arm runtime > detection series that I've previouslyt posted separately, as there > are small conflicts between the two series. > > Aside from rebasing vs master, the only other change is to fix the > TCG_OPF_NOT_PRESENT problem wrt call that Claudio Fontana spotted. > > > r~ > > > Richard Henderson (9): > tcg: Split rem requirement from div requirement > tcg-arm: Don't implement rem > tcg-ppc: Don't implement rem > tcg-ppc64: Don't implement rem > tcg: Allow non-constant control macros > tcg: Simplify logic using TCG_OPF_NOT_PRESENT > tcg-arm: Make use of conditional availability of opcodes for divide > tcg-arm: Simplify logic in detecting the ARM ISA in use > tcg-arm: Use AT_PLATFORM to detect the host ISA > > tcg/arm/tcg-target.c | 96 > ++ > tcg/arm/tcg-target.h | 15 > tcg/hppa/tcg-target.h | 1 + > tcg/ia64/tcg-target.h | 2 ++ > tcg/mips/tcg-target.h | 1 + > tcg/ppc/tcg-target.c | 14 > tcg/ppc/tcg-target.h | 1 + > tcg/ppc64/tcg-target.c | 26 -- > tcg/ppc64/tcg-target.h | 2 ++ > tcg/sparc/tcg-target.h | 2 ++ > tcg/tcg-op.h | 32 ++--- > tcg/tcg-opc.h | 36 ++- > tcg/tcg.c | 4 +-- > tcg/tcg.h | 6 +++- > tcg/tci/tcg-target.h | 2 ++ > 15 files changed, 116 insertions(+), 124 deletions(-) >
Re: [Qemu-devel] [PATCH v2 0/4] tcg-arm: Implement tcg_register_jit
Ping 4. Should I just send a PULL for the only partially reviewed series? r~ On 06/24/2013 08:44 PM, Richard Henderson wrote: > Ping 3. > > On 06/17/2013 08:54 AM, Richard Henderson wrote: >> Ping. >> >> >> On 06/10/2013 11:41 AM, Richard Henderson wrote: >>> Ping. >>> >>> >>> On 06/05/2013 10:29 AM, Richard Henderson wrote: Changes v1-v2: The suggestions for improvement I got from round 1 apply to all of the hosts, not just arm. r~ Richard Henderson (4): tcg: Fix high_pc fields in .debug_info tcg: Move the CIE and FDE header definitions to common code tcg-i386: Use QEMU_BUILD_BUG_ON instead of assert for frame size tcg-arm: Implement tcg_register_jit tcg/arm/tcg-target.c | 76 -- tcg/hppa/tcg-target.c | 35 +++ tcg/i386/tcg-target.c | 45 +++--- tcg/sparc/tcg-target.c | 35 +++ tcg/tcg.c | 22 +-- 5 files changed, 123 insertions(+), 90 deletions(-) >>> >> >
Re: [Qemu-devel] [PATCH 25/30] exec: put memory map in AddressSpaceDispatch
Il 02/07/2013 16:42, Jan Kiszka ha scritto:
>> > +typedef PhysPageEntry Node[L2_SIZE];
>> > +
>> > struct AddressSpaceDispatch {
>> > /* This is a multi-level map on the physical address space.
>> > * The bottom level has pointers to MemoryRegionSections.
>> > */
>> > PhysPageEntry phys_map;
>> > +Node *nodes;
>> > +MemoryRegionSection *sections;
> Why not sticking the whole current PhysPageMap into here? Wouldn't that
> also allow to overcome prev_map completely (next patch)?
(BTW, this is also why patch 21 has two separate arguments to
phys_page_find).
Sticking a pointer would add one useless pointer chase. Storing it by
value makes it unclear who owns the PhysPageMap and doesn't say that
AddressSpaceDispatch cannot extend the vectors. In the end, this seemed
like a good compromise.
Paolo
Re: [Qemu-devel] [PATCH v3] linux-user: improve target_to_host_sock_type conversion
From: Peter Maydell [peter.mayd...@linaro.org]
Sent: Tuesday, July 02, 2013 3:44 PM
To: Petar Jovanovic
Cc: qemu-devel@nongnu.org; Petar Jovanovic; aurel...@aurel32.net;
riku.voi...@linaro.org; r...@twiddle.net
Subject: Re: [PATCH v3] linux-user: improve target_to_host_sock_type conversion
On 1 July 2013 01:44, Petar Jovanovic wrote:
> The patch also includes necessary code style changes (tab to spaces) in the
> header file since most of the file has been touched by this change.
Thanks for the review, Peter.
> (The general rule of thumb is "if you're just changing a few lines
> of indent/braces in lines that the patch touches anyway" that's fine;
> larger scale changes go in their own patch if we do them at all.)
I am aware of this, and I originally tried to make these changes as small
as possible. Check for the previous two versions of the patch.
The commit message originally said:
"The patch also includes necessary code style changes (tab to spaces) in the
header file in the MIPS #ifdef block touched by the change."
Yet, I ended up changing 80% of the header file with this version, so I
was under impression it makes sense to change the remaining 20%.
> Not using the same base as the kernel (hex vs octal) made checking
> the values a bit tedious too, but I think they're all correct.
What values are you refering to?
I actually tried to copy the values from the kernel in the same format:
O_NONBLOCK 4000
http://lxr.free-electrons.com/source/include/uapi/asm-generic/fcntl.h#L37
O_CLOEXEC 0200
http://lxr.free-electrons.com/source/include/uapi/asm-generic/fcntl.h#L61
all socket type values:
http://lxr.free-electrons.com/source/include/linux/net.h#L58
enum sock_type {
SOCK_STREAM = 1,
SOCK_DGRAM = 2,
SOCK_RAW= 3,
SOCK_RDM= 4,
SOCK_SEQPACKET = 5,
SOCK_DCCP = 6,
SOCK_PACKET = 10,
};
#define SOCK_TYPE_MASK 0xf
Petar
Re: [Qemu-devel] [PATCH v4 08/10] qemu-ga: call Windows VSS requester in fsfreeze command handler
On Tue, Jul 2, 2013 at 1:36 AM, Laszlo Ersek wrote:
> On 07/01/13 19:59, Tomoki Sekiyama wrote:
>> On 7/1/13 9:29 , "Laszlo Ersek" wrote:
>>
+error:
+qmp_guest_fsfreeze_thaw(NULL);
>>>
>>> Passing NULL here as "errp" concerns me slightly. I've been assuming
>>> that "errp" is never NULL due to the outermost QMP layer always wanting
>>> to receive errors and to serialize them.
>>>
>>> Specifically, a NULL "errp" would turn all error_set*(errp, ...) calls
>>> into no-ops under qmp_guest_fsfreeze_thaw(), and all error_is_set(errp)
>>> questions would answer with false. That said, nothing seems to be
>>> affected right now.
>>>
>>> Maybe a dummy local variable would be more future-proof... OTOH it would
>>> be stylistically questionable :)
>>
>> OK, then it should be:
>> Error *local_err = NULL;
>> ...
>> error:
>> qmp_guest_fsfreeze_thaw(&local_err);
>> if (error_is_set(&local_err)) {
>> error_free(local_err);
>> }
>
> I think so, yes.
>
> You could also log it before freeing it I guess:
>
> g_debug("cleanup thaw: %s", error_get_pretty(local_err));
>
> or some such, but it's probably not important.
One thing to keep in mind is there are some failure paths in
qmp_guest_fsfreeze_thaw(), for both win32 and posix, where we might not unset
frozen state via ga_set_unfrozen(). In which case, logging will still be
disabled.
It might make sense to nest those error strings inside the one returned by
qmp_guest_fsfreeze_freeze(), since that's the only reliable way to report it
and the client will be interested in that information. This also makes
introducing local_err immediately useful.
>
> Thanks,
> Laszlo
>
Re: [Qemu-devel] [PATCH] Citrix PV Bus device
> -Original Message- > From: Peter Maydell [mailto:peter.mayd...@linaro.org] > Sent: 02 July 2013 15:58 > To: Paul Durrant > Cc: qemu-devel@nongnu.org; xen-de...@lists.xen.org > Subject: Re: [Qemu-devel] [PATCH] Citrix PV Bus device > > On 2 July 2013 15:03, Paul Durrant wrote: > > This patch introduces a new PCI device which will act as the binding point > > for Citrix branded PV drivers for Xen. > > The intention is that Citrix Windows PV drivers will be available on Windows > > Update and thus using the existing Xen platform PCI device as an anchor > > point is not desirable as that device has been ubiquitous in HVM guests for > > a long time and thus existing HVM guests running Windows would start > > automatically downloading drivers from Windows Update when this may > not be > > desired by either the host or guest admin. This device therefore acts as > > an opt-in for those wishing to deploy Citrix PV drivers. > > This commit message doesn't really make the case for having this > driver in upstream, IMHO. It sounds like it's only of use for your > branded product, which suggests that the best place for it is > probably in your product, not upstream. > No, that's not true. The drivers themselves may be Citrix branded but they are not solely for use in Citrix branded VMs. Paul
Re: [Qemu-devel] [PATCH] Citrix PV Bus device
> -Original Message-
> From: Anthony Liguori [mailto:anth...@codemonkey.ws]
> Sent: 02 July 2013 15:43
> To: Paul Durrant; qemu-devel@nongnu.org; xen-de...@lists.xen.org
> Cc: Paul Durrant
> Subject: Re: [Qemu-devel] [PATCH] Citrix PV Bus device
>
> Paul Durrant writes:
>
> > This patch introduces a new PCI device which will act as the binding point
> > for Citrix branded PV drivers for Xen.
> > The intention is that Citrix Windows PV drivers will be available on Windows
> > Update and thus using the existing Xen platform PCI device as an anchor
> > point is not desirable as that device has been ubiquitous in HVM guests for
> > a long time and thus existing HVM guests running Windows would start
> > automatically downloading drivers from Windows Update when this may
> not be
> > desired by either the host or guest admin. This device therefore acts as
> > an opt-in for those wishing to deploy Citrix PV drivers.
> >
> > Signed-off-by: Paul Durrant
> > ---
> > hw/i386/Makefile.objs|1 +
> > hw/i386/citrix_pv_bus.c | 122
> ++
> > include/hw/pci/pci_ids.h |3 ++
> > 3 files changed, 126 insertions(+)
> > create mode 100644 hw/i386/citrix_pv_bus.c
> >
> > diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> > index 205d22e..8e28831 100644
> > --- a/hw/i386/Makefile.objs
> > +++ b/hw/i386/Makefile.objs
> > @@ -4,3 +4,4 @@ obj-y += pc.o pc_piix.o pc_q35.o
> > obj-$(CONFIG_XEN) += xen_domainbuild.o xen_machine_pv.o
> >
> > obj-y += kvmvapic.o
> > +obj-y += citrix_pv_bus.o
> > diff --git a/hw/i386/citrix_pv_bus.c b/hw/i386/citrix_pv_bus.c
> > new file mode 100644
> > index 000..e1e0508
> > --- /dev/null
> > +++ b/hw/i386/citrix_pv_bus.c
> > @@ -0,0 +1,122 @@
> > +/* Copyright (c) Citrix Systems Inc.
> > + * All rights reserved.
>
> All rights reserved contradicts the grant of rights below. Obviously
> the later one is the one that wins but having the above statement is a
> little silly IMHO.
>
I lifted the license verbatim from the BSD 2-clause template at
http://opensource.org/licenses/BSD-2-Clause
> > + *
> > + * Redistribution and use in source and binary forms,
> > + * with or without modification, are permitted provided
> > + * that the following conditions are met:
> > + *
> > + * * Redistributions of source code must retain the above
> > + * copyright notice, this list of conditions and the
> > + * following disclaimer.
> > + * * Redistributions in binary form must reproduce the above
> > + * copyright notice, this list of conditions and the
> > + * following disclaimer in the documentation and/or other
> > + * materials provided with the distribution.
> > + *
> > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> > + * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
> > + * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
> > + * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> > + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR
> > + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> > + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
> > + * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
> > + * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
> > + * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
> > + * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> > + * SUCH DAMAGE.
> > + */
> > +
> > +#include "hw/hw.h"
> > +#include "hw/pci/pci.h"
> > +
> > +typedef struct _CitrixPVBusDevice {
>
> Drop the '_' please.
>
Done in the latest patch.
> > +PCIDevice dev;
> > +uint8_t revision;
> > +uint32_tpages;
> > +MemoryRegionmmio;
> > +} CitrixPVBusDevice;
> > +
> > +static uint64_t citrix_pv_bus_mmio_read(void *opaque, hwaddr addr,
> > +unsigned size)
> > +{
> > +fprintf(stderr, "WARNING: read from address 0x" TARGET_FMT_plx
> > +" in Citrix PV Bus MMIO BAR\n", addr);
> > +
> > +return ~(uint64_t)0;
> > +}
> > +
> > +static void citrix_pv_bus_mmio_write(void *opaque, hwaddr addr,
> > + uint64_t val, unsigned size)
> > +{
> > +fprintf(stderr, "WARNING: write to address 0x" TARGET_FMT_plx
> > +" in Citrix PV Bus MMIO BAR\n", addr);
> > +}
>
> Don't let guests trigger unconditional output to stdio. If a management
> tool logs stdio (libvirt does for instance), this can allow a guest to
> mount a DoS attack on the host.
>
I went for trace points in the latest patch.
> > +static const MemoryRegionOps citrix_pv_bus_mmio_ops = {
> > +.read = &citrix_pv_bus_mmio_read,
> > +.write = &citrix_pv_bus_mmio_write,
> > +.endianness = DEVICE_NATIVE_ENDIAN,
> > +};
>
> Please use DEVICE_LITTLE_ENDIAN. Nativ
Re: [Qemu-devel] [PATCH] Citrix PV Bus device
On 2 July 2013 15:03, Paul Durrant wrote: > This patch introduces a new PCI device which will act as the binding point > for Citrix branded PV drivers for Xen. > The intention is that Citrix Windows PV drivers will be available on Windows > Update and thus using the existing Xen platform PCI device as an anchor > point is not desirable as that device has been ubiquitous in HVM guests for > a long time and thus existing HVM guests running Windows would start > automatically downloading drivers from Windows Update when this may not be > desired by either the host or guest admin. This device therefore acts as > an opt-in for those wishing to deploy Citrix PV drivers. This commit message doesn't really make the case for having this driver in upstream, IMHO. It sounds like it's only of use for your branded product, which suggests that the best place for it is probably in your product, not upstream. > Signed-off-by: Paul Durrant > --- > hw/i386/Makefile.objs|1 + > hw/i386/citrix_pv_bus.c | 126 > ++ hw/i386 is also not the right place for this. hw/misc/ would be better. thanks -- PMM
Re: [Qemu-devel] [PATCH 27/30] exec: change some APIs to take AddressSpaceDispatch
On 2013-06-28 20:26, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini
> ---
> exec.c | 11 +--
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 7f87e16..528c4d7 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -236,11 +236,10 @@ bool memory_region_is_unassigned(MemoryRegion *mr)
> && mr != &io_mem_watch;
> }
>
> -static MemoryRegionSection *address_space_lookup_region(AddressSpace *as,
> +static MemoryRegionSection *address_space_lookup_region(AddressSpaceDispatch
> *d,
> hwaddr addr,
> bool resolve_subpage)
> {
> -AddressSpaceDispatch *d = as->dispatch;
> MemoryRegionSection *section;
> subpage_t *subpage;
>
> @@ -254,13 +253,13 @@ static MemoryRegionSection
> *address_space_lookup_region(AddressSpace *as,
> }
>
> static MemoryRegionSection *
> -address_space_translate_internal(AddressSpace *as, hwaddr addr, hwaddr *xlat,
> +address_space_translate_internal(AddressSpaceDispatch *d, hwaddr addr,
> hwaddr *xlat,
> hwaddr *plen, bool resolve_subpage)
> {
> MemoryRegionSection *section;
> Int128 diff;
>
> -section = address_space_lookup_region(as, addr, resolve_subpage);
> +section = address_space_lookup_region(d, addr, resolve_subpage);
> /* Compute offset within MemoryRegionSection */
> addr -= section->offset_within_address_space;
>
> @@ -282,7 +281,7 @@ MemoryRegion *address_space_translate(AddressSpace *as,
> hwaddr addr,
> hwaddr len = *plen;
>
> for (;;) {
> -section = address_space_translate_internal(as, addr, &addr, plen,
> true);
> +section = address_space_translate_internal(as->dispatch, addr,
> &addr, plen, true);
> mr = section->mr;
>
> if (!mr->iommu_ops) {
> @@ -311,7 +310,7 @@ address_space_translate_for_iotlb(AddressSpace *as,
> hwaddr addr, hwaddr *xlat,
>hwaddr *plen)
> {
> MemoryRegionSection *section;
> -section = address_space_translate_internal(as, addr, xlat, plen, false);
> +section = address_space_translate_internal(as->dispatch, addr, xlat,
> plen, false);
>
> assert(!section->mr->iommu_ops);
> return section;
>
Reviewed-by: Jan Kiszka
--
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [RFC V8 01/24] qcow2: Add journal specification.
Am 02.07.2013 um 16:42 hat Stefan Hajnoczi geschrieben: > On Thu, Jun 20, 2013 at 04:26:09PM +0200, Benoît Canet wrote: > > --- > > docs/specs/qcow2.txt | 42 ++ > > 1 file changed, 42 insertions(+) > > > > diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt > > index 36a559d..a4ffc85 100644 > > --- a/docs/specs/qcow2.txt > > +++ b/docs/specs/qcow2.txt > > @@ -350,3 +350,45 @@ Snapshot table entry: > > variable: Unique ID string for the snapshot (not null terminated) > > > > variable: Name of the snapshot (not null terminated) > > + > > +== Journal == > > + > > +QCOW2 can use one or more instance of a metadata journal. > > s/instance/instances/ > > Is there a reason to use multiple journals rather than a single journal > for all entry types? The single journal area avoids seeks. > > > + > > +A journal is a sequential log of journal entries appended on a previously > > +allocated and reseted area. > > I think you say "previously reset area" instead of "reseted". Another > option is "initialized area". > > > +A journal is designed like a linked list with each entry pointing to the > > next > > +so it's easy to iterate over entries. > > + > > +A journal uses the following constants to denote the type of each entry > > + > > +TYPE_NONE = 0xFF default value of any bytes in a reseted journal > > +TYPE_END = 1 the entry ends a journal cluster and point to the > > next > > + cluster > > +TYPE_HASH = 2 the entry contains a deduplication hash > > + > > +QCOW2 journal entry: > > + > > +Byte 0 :Size of the entry: size = 2 + n with size <= 254 > > This is not clear. I'm wondering if the +2 is included in the byte > value or not. I'm also wondering what a byte value of zero means and > what a byte value of 255 means. > > Please include an example to illustrate how this field works. > > > + > > + 1 :Type of the entry > > + > > + 2 - size :The optional n bytes structure carried by entry > > + > > +A journal is divided into clusters and no journal entry can be spilled on > > two > > +clusters. This avoid having to read more than one cluster to get a single > > entry. > > + > > +For this purpose an entry with the end type is added at the end of a > > journal > > +cluster before starting to write in the next cluster. > > +The size of such an entry is set so the entry points to the next cluster. > > + > > +As any journal cluster must be ended with an end entry the size of regular > > +journal entries is limited to 254 bytes in order to always left room for > > an end > > +entry which mimimal size is two bytes. > > + > > +The only cases where size > 254 are none entries where size = 255. > > + > > +The replay of a journal stop when the first end none entry is reached. > > s/stop/stops/ > > > +The journal cluster size is 4096 bytes. > > Questions about this layout: > > 1. Journal entries have no integrity mechanism, which is especially >important if they span physical sectors where cheap disks may perform >a partial write. This would leave a corrupt journal. If the last >bytes are a checksum then you can get some confidence that the entry >was fully written and is valid. > >Did I miss something? Adding a checksum sounds like a good idea. > 2. Byte-granularity means that read-modify-write is necessary to append >entries to the journal. Therefore a failure could destroy previously >committed entries. > >Any ideas how existing journals handle this? You commit only whole blocks. So in this case we can consider a block only committed as soon as a TYPE_END entry has been written (and after that we won't touch it any more until the journalled changes have been flushed to disk). There's one "interesting" case: cache=writethrough. I'm not entirely sure yet what to do with it, but it's slow anyway, so using one block per entry and therefore flushing the journal very often might actually be not totally unreasonable. Another thing I'm not sure about is whether a fixed 4k block is good or if we should leave it configurable. I don't think making it an option would hurt (not necessarily modifyable with qemu-img, but as a field in the file format). Kevin
