[Qemu-devel] Re: [PATCH 07/18] Introduce fault tolerant VM transaction QEMUFile and ft_mode.

[ya su]

Juan:

 It's especailly important for ft to be a standalone thread, as it
may cause monitor to  be blocked by network problems.  what's your
schedule, maybe I can help some.

Yoshi:

 in the following code:

+
+s->file = qemu_fopen_ops(s, ft_trans_put_buffer, ft_trans_get_buffer,
+ ft_trans_close, ft_trans_rate_limit,
+ ft_trans_set_rate_limit, NULL);
+
+return s->file;
+}

I think you should register ft_trans_get_rate_limit function,
otherwise it will not transfer any block data at stage 2 in
block_save_live function:

if (stage == 2) {
/* control the rate of transfer */
while ((block_mig_state.submitted +
block_mig_state.read_done) * BLOCK_SIZE <
   qemu_file_get_rate_limit(f)) {

 qemu_file_get_rate_limit will return 0, then it will not proceed
to copy dirty block data.

 FYI.

Green.


2011/2/24 Yoshiaki Tamura :
> 2011/2/24 Juan Quintela :
>>
>> [ trimming cc to kvm & qemu lists]
>>
>> Yoshiaki Tamura  wrote:
>>> Juan Quintela wrote:
 Yoshiaki Tamura  wrote:
> This code implements VM transaction protocol.  Like buffered_file, it
> sits between savevm and migration layer.  With this architecture, VM
> transaction protocol is implemented mostly independent from other
> existing code.

 Could you explain what is the difference with buffered_file.c?
 I am fixing problems on buffered_file, and having something that copies
 lot of code from there makes me nervous.
>>>
>>> The objective is different:
>>>
>>> buffered_file buffers data for transmission control.
>>> ft_trans_file adds headers to the stream, and controls the transaction
>>> between sender and receiver.
>>>
>>> Although ft_trans_file sometimes buffers date, but it's not the main 
>>> objective.
>>> If you're fixing the problems on buffered_file, I'll keep eyes on them.
>>>
> +typedef ssize_t (FtTransPutBufferFunc)(void *opaque, const void *data, 
> size_t size);

 Can we get some sharing here?
 typedef ssize_t (BufferedPutFunc)(void *opaque, const void *data, size_t 
 size);

 There are not so much types for a write function that the 1st element is
 one opaque :p
>>>
>>> You're right, but I want to keep ft_trans_file independent of
>>> buffered_file at this point.  Once Kemari gets merged, I'm happy to
>>> work with you to fix the problems on buffered_file and ft_trans_file,
>>> and refactoring them.
>>
>> My goal is getting its own thread for migration on 0.15, that
>> basically means that we can do rm buffered_file.c.  I guess that
>> something similar could happen for kemari.
>
> That means both gets initiated by it's own thread, not like
> current poll based.  I'm still skeptical whether Anthony agrees,
> but I'll keep it in my mind.
>
>> But for now, this is just the start + handwaving, once I start doing the
>> work I will told you.
>
> Yes, please.
>
> Yoshi
>
>>
>> Later, Juan.
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[Qemu-devel] Re: [V7 PATCH 7/9] virtio-9p: Support for creating special files

[M. Mohan Kumar]

On Friday 04 March 2011 4:36:35 pm Stefan Hajnoczi wrote:
> On Fri, Mar 4, 2011 at 9:25 AM, M. Mohan Kumar  wrote:
> > +static int chroot_do_create_special(V9fsFileObjectRequest *request)
> > +{
> > +int cur_uid, cur_gid;
> > +int retval = -1;
> > +
> > +cur_uid = geteuid();
> > +cur_gid = getegid();
> > +
> > +if (setfsuid(request->data.uid) < 0) {
> > +return -errno;
> > +}
> > +if (setfsgid(request->data.gid) < 0) {
> > +retval = -errno;
> > +goto unset_uid;
> > +}
> > +
> > +switch (request->data.type) {
> > +case T_MKDIR:
> > +retval = mkdir(request->path.path, request->data.mode);
> > +break;
> > +case T_SYMLINK:
> > +retval = symlink(request->path.old_path, request->path.path);
> > +break;
> > +case T_LINK:
> > +retval = link(request->path.old_path, request->path.path);
> > +break;
> > +default:
> > +retval = mknod(request->path.path, request->data.mode,
> > +request->data.dev);
> > +break;
> > +}
> > +
> > +if (retval < 0) {
> > +retval = -errno;
> > +}
> > +setfsgid(cur_gid);
> > +unset_uid:
> > +setfsuid(cur_uid);
> > +return retval;
> > +}
> 
> It would be nice to take this one step further and move file create
> and open here too.  The prototype we need is:
> 
> static int chroot_handle_request(V9fsFileObjectRequest *request, int *fd)
> {
> *fd = -1;
> 
> It returns 0 on success or -errno and *fd >= 0 if a file descriptor
> was opened and -1 otherwise.
> 
> This function becomes the main request processing function called from
> v9fs_chroot() and the switch statement there can be eliminated.
>

We don't need setfsgid, setfsuid for normal open. Also I think having separate 
function based on the functionality helps better code readability. 
 
> Sending the response back to QEMU then gets a cleaned up prototype:
> chroot_sendfd(int chroot_sock, int result, int fd) where result is 0
> on success or -errno and fd >= 0 if present or -1 if not.
> 
> > +int v9fs_create_special(FsContext *fs_ctx, V9fsFileObjectRequest
> > *request) +{
> > +int retval, sock_error;
> > +qemu_mutex_lock(&fs_ctx->chroot_mutex);
> > +if (fs_ctx->chroot_ioerror) {
> > +retval = -EIO;
> > +goto unlock;
> > +}
> > +if (v9fs_write_request(fs_ctx->chroot_socket, request) < 0) {
> > +fs_ctx->chroot_ioerror = 1;
> > +retval = -EIO;
> > +goto unlock;
> > +}
> > +retval = v9fs_receivefd(fs_ctx->chroot_socket, &sock_error);
> > +if (retval < 0 && sock_error) {
> > +fs_ctx->chroot_ioerror = 1;
> > +}
> > +unlock:
> > +qemu_mutex_unlock(&fs_ctx->chroot_mutex);
> > +return retval;
> > +}
> 
> This function is a duplicate of v9fs_request().  Can't there be just
> one function?
> 
Yeah, I will make it as a single function.


M. Mohan Kumar

[Qemu-devel] Re: [V7 PATCH 9/9] virtio-9p: Chroot environment for other functions

[M. Mohan Kumar]

Thanks Stefan for your review!

On Friday 04 March 2011 5:22:00 pm Stefan Hajnoczi wrote:
> 
> Is this code supposed to build on non-Linux hosts?  If so, then please
> confirm that the *at() system calls used are standard and available on
> other hosts (e.g. FreeBSD, Darwin, Solaris).
> 
No, this (virtio-9p) is not supported on non-linux hosts.

> > +/*
> > + * Returns file descriptor of dirname(path)
> > + * This fd can be used by *at functions
> > + */
> > +static int get_dirfd(FsContext *fs_ctx, const char *path)
> > +{
> > +V9fsFileObjectRequest request;
> > +int fd;
> > +char *dpath = qemu_strdup(path);
> > +
> > +fd = fill_fileobjectrequest(&request, dirname(dpath), NULL);
> > +if (fd < 0) {
> > +return fd;
> > +}
> 
> Leaks dpath, fails to set errno, and does not return -1.
Will fix in next patchset.

> 
> > @@ -545,53 +621,146 @@ static int local_link(FsContext *fs_ctx, const
> > char *oldpath,
> > 
> >  static int local_truncate(FsContext *ctx, const char *path, off_t size)
> >  {
> > -return truncate(rpath(ctx, path), size);
> > +if (ctx->fs_sm == SM_PASSTHROUGH) {
> > +int fd, retval;
> > +fd = passthrough_open(ctx, path, O_RDWR);
> > +if (fd < 0) {
> > +return -1;
> > +}
> > +retval = ftruncate(fd, size);
> > +close(fd);
> > +return retval;
> 
> This is an example of where errno is not guaranteed to be preserved.
> When ftruncate(2) fails close(2) is allowed to affect errno and we
> cannot rely on it holding the ftruncate(2) error code.  Please check
> for other cases of this.
Will fix in next patchset.
> 
> >  static int local_rename(FsContext *ctx, const char *oldpath,
> > const char *newpath)
> >  {
> > -char *tmp;
> > -int err;
> > -
> > -tmp = qemu_strdup(rpath(ctx, oldpath));
> > +int err, serrno = 0;
> > 
> > -err = rename(tmp, rpath(ctx, newpath));
> > -if (err == -1) {
> > -int serrno = errno;
> > -qemu_free(tmp);
> > +if (ctx->fs_sm == SM_PASSTHROUGH) {
> > +int opfd, npfd;
> > +char *old_tmppath, *new_tmppath;
> > +opfd = get_dirfd(ctx, oldpath);
> > +if (opfd < 0) {
> > +return -1;
> > +}
> > +npfd = get_dirfd(ctx, newpath);
> > +if (npfd < 0) {
> > +close(opfd);
> > +return -1;
> > +}
> > +old_tmppath = qemu_strdup(oldpath);
> > +new_tmppath = qemu_strdup(newpath);
> > +err = renameat(opfd, basename(old_tmppath),
> > +npfd, basename(new_tmppath));
> > +if (err == -1) {
> > +serrno = errno;
> > +}
> > +close(npfd);
> > +close(opfd);
> > +qemu_free(old_tmppath);
> > +qemu_free(new_tmppath);
> > errno = serrno;
> 
> Why can't this be done as a chroot worker operation in a single syscall?
Ok, in next patchset, T_RENAME & T_REMOVE types will be added.

> 
> > -static int local_utimensat(FsContext *s, const char *path,
> > -   const struct timespec *buf)
> > +static int local_utimensat(FsContext *fs_ctx, const char *path,
> > +const struct timespec *buf)
> >  {
> > -return qemu_utimensat(AT_FDCWD, rpath(s, path), buf,
> > AT_SYMLINK_NOFOLLOW); +if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
> > +int fd, retval;
> > +fd = passthrough_open(fs_ctx, path, O_RDONLY | O_NONBLOCK);
> 
> This follows symlinks but the SM_PASSTHROUGH case below does not?
Will fix.
> 
> > +if (fd < 0) {
> > +return -1;
> > +}
> > +retval = futimens(fd, buf);
> > +close(fd);
> > +return retval;
> > +} else {
> > +return utimensat(AT_FDCWD, rpath(fs_ctx, path), buf,
> > +AT_SYMLINK_NOFOLLOW);
> > +}
> >  }
> > 
> > -static int local_remove(FsContext *ctx, const char *path)
> > -{
> > -return remove(rpath(ctx, path));
> > +static int local_remove(FsContext *fs_ctx, const char *path)
> > + {
> > +if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
> > +int pfd, err, serrno, flags;
> > +char *old_path;
> > +struct stat stbuf;
> > +pfd = get_dirfd(fs_ctx, path);
> > +if (pfd < 0) {
> > +return -1;
> > +}
> > +old_path = qemu_strdup(path);
> > +err = fstatat(pfd, basename(old_path), &stbuf,
> > AT_SYMLINK_NOFOLLOW); +if (err < 0) {
> 
> old_path and pfd are leaked.
> 
> > +return -1;
> > +}
> > +serrno = flags = 0;
> > +if ((stbuf.st_mode & S_IFMT) == S_IFDIR) {
> > +flags = AT_REMOVEDIR;
> > +} else {
> > +flags = 0;
> > +}
> > +err = unlinkat(pfd, basename(old_path), flags);
> > +if (err == -1) {
> > +serrno = errno;
> > +}
> > +qemu_free(old_path);
> > +close(pfd);
> > +errno =

[Qemu-devel] [PATCH V2] qemu, qmp: add keydown and keyup command for qmp

[Lai Jiangshan]

sendkey is a very good command for human using it in their monitor,
but it is not a good idea to port it to qmp, because qmp is a machine
protocol. So we introduce keydown and keyup command for qmp, they
simulate the events that keyboard send to the system.

Example, simulates ctrl+alt+f1:
{ "execute": "keydown", "arguments": { "keycode": 29 } }  #press down ctrl
{ "execute": "keydown", "arguments": { "keycode": 56 } }  #press down alt
{ "execute": "keydown", "arguments": { "keycode": 59 } }  #press down f1
{ "execute": "keyup", "arguments": { "keycode": 59 } }#release f1
{ "execute": "keyup", "arguments": { "keycode": 56 } }#release alt
{ "execute": "keyup", "arguments": { "keycode": 29 } }#release ctrl

Signed-off-by: Lai Jiangshan 
---
diff --git a/monitor.c b/monitor.c
index 22ae3bb..725df83 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1810,16 +1810,25 @@ static uint8_t keycodes[MAX_KEYCODES];
 static int nb_pending_keycodes;
 static QEMUTimer *key_timer;
 
-static void release_keys(void *opaque)
+static void keydown(uint8_t keycode)
 {
-int keycode;
+if (keycode & 0x80)
+kbd_put_keycode(0xe0);
+kbd_put_keycode(keycode & 0x7f);
+}
 
+static void keyup(uint8_t keycode)
+{
+if (keycode & 0x80)
+kbd_put_keycode(0xe0);
+kbd_put_keycode(keycode | 0x80);
+}
+
+static void release_keys(void *opaque)
+{
 while (nb_pending_keycodes > 0) {
 nb_pending_keycodes--;
-keycode = keycodes[nb_pending_keycodes];
-if (keycode & 0x80)
-kbd_put_keycode(0xe0);
-kbd_put_keycode(keycode | 0x80);
+keyup(keycodes[nb_pending_keycodes]);
 }
 }
 
@@ -1866,17 +1875,41 @@ static void do_sendkey(Monitor *mon, const QDict *qdict)
 }
 nb_pending_keycodes = i;
 /* key down events */
-for (i = 0; i < nb_pending_keycodes; i++) {
-keycode = keycodes[i];
-if (keycode & 0x80)
-kbd_put_keycode(0xe0);
-kbd_put_keycode(keycode & 0x7f);
-}
+for (i = 0; i < nb_pending_keycodes; i++)
+keydown(keycodes[i]);
 /* delayed key up events */
 qemu_mod_timer(key_timer, qemu_get_clock(vm_clock) +
muldiv64(get_ticks_per_sec(), hold_time, 1000));
 }
 
+static int qmp_keyaction(const QDict *qdict, int down)
+{
+int keycode = qdict_get_int(qdict, "keycode");
+
+if (keycode < 0 || keycode >= 256) {
+qerror_report(QERR_INVALID_PARAMETER_VALUE, "keycode",
+  "a valid keycode");
+return -1;
+}
+
+if (down)
+keydown(keycode);
+else
+keyup(keycode);
+
+return 0;
+}
+
+static int qmp_keydown(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+return qmp_keyaction(qdict, 1);
+}
+
+static int qmp_keyup(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+return qmp_keyaction(qdict, 0);
+}
+
 static int mouse_button_state;
 
 static void do_mouse_move(Monitor *mon, const QDict *qdict)
diff --git a/qmp-commands.hx b/qmp-commands.hx
index df40a3d..4c449bb 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -338,6 +338,58 @@ Example:
 EQMP
 
 {
+.name   = "keydown",
+.args_type  = "keycode:i",
+.params = "keycode",
+.help   = "press down a key",
+.user_print = monitor_user_noop,
+.mhandler.cmd_new = qmp_keydown,
+},
+
+SQMP
+keydown
+---
+
+Press down a key.
+
+Arguments:
+
+- "keycode": the code of the key to press down (json-int)
+
+Example:
+
+-> { "execute": "keydown", "arguments": { "keycode": 16 } }
+<- { "return": {} }
+
+EQMP
+
+{
+.name   = "keyup",
+.args_type  = "keycode:i",
+.params = "keycode",
+.help   = "release a key",
+.user_print = monitor_user_noop,
+.mhandler.cmd_new = qmp_keyup,
+},
+
+SQMP
+keyup
+---
+
+Release a key.
+
+Arguments:
+
+- "keycode": the code of the key to release (json-int)
+
+Example:
+
+-> { "execute": "keyup", "arguments": { "keycode": 16 } }
+<- { "return": {} }
+
+EQMP
+
+{
 .name   = "cpu",
 .args_type  = "index:i",
 .params = "index",

[Qemu-devel] Re: [PATCH 09/18] Introduce event-tap.

[ya su]

Yoshi:

I meet one problem if I killed a ft source VM, the dest ft VM will
return errors as the following:

qemu-system-x86_64: fill buffer failed, Resource temporarily unavailable
qemu-system-x86_64: recv header failed

the problem is that the dest VM can not continue to run, as it is
interrupted in the middle of a transaction, some of rams have been
updated, but the others not, do you have any plan for rolling back to
cancel the interrupted transaction? thanks.


Green.



2011/3/9 Yoshiaki Tamura :
> ya su wrote:
>>
>> Yoshi:
>>
>>     I think event-tap is a great idea, it remove the reading from disk
>> which will increase ft effiency much better as your plan in later
>> series.
>>
>>     one question: IO read/write may dirty rams, but it is difficute to
>> differ them from other dirty pages like caused by  running of
>> softwares,  whether that means you need change all the emulated device
>> realization?  actually I think it will not send too much rams caused
>> by IO Read/Write in ram_save_live, but if It can event-tap IO
>> read/write and replay on the other side, Does that means we don't need
>> call qemu_savevm_state_full in ft transactoins?
>
> I'm not expecting to remove qemu_savevm_state_full in the transaction.  Just
> reduce the number of pages to be transfered as a result.
>
> Thanks,
>
> Yoshi
>
>>
>> Green.
>>
>>
>> 2011/3/9 Yoshiaki Tamura:
>>>
>>> ya su wrote:

 2011/3/8 Yoshiaki Tamura:
>
> ya su wrote:
>>
>> Yokshiaki:
>>
>>     event-tap record block and io wirte events, and replay these on
>> the other side, so block_save_live is useless during the latter ft
>> phase, right? if so, I think it need to process the following code in
>> block_save_live function:
>
> Actually no.  It just replays the last events only.  We do have patches
> that
> enable block replication without using block live migration, like the
> way
> you described above.  In that case, we disable block live migration
> when
>  we
> go into ft mode.  We're thinking to propose it after this series get
> settled.

 so event-tap's objective is to initial a ft transaction, to start the
 sync. of ram/block/device states? if so, it need not change
 bdrv_aio_writev/bdrv_aio_flush normal process, on the other side it
 need not invokde bdrv_aio_writev either, right?
>>>
>>> Mostly yes, but because event-tap is queuing requests from block/net, it
>>> needs to flush queued requests after the transaction on the primary side.
>>>  On the secondary, it currently doesn't have to invoke bdrv_aio_writev as
>>> you mentioned.  But will change soon to enable block replication with
>>> event-tap.
>>>

>
>>
>>     if (stage == 1) {
>>         init_blk_migration(mon, f);
>>
>>         /* start track dirty blocks */
>>         set_dirty_tracking(1);
>>     }
>> --
>> the following code will send block to the other side, as this will
>> also be done by event-tap replay. I think it should placed in stage 3,
>> before the assert line. (this may affect some stage 2 rate-limit
>> then, so this can be placed in stage 2, though it looks ugly), another
>> choice is to avoid the invocation of block_save_live, right?
>> ---
>>     flush_blks(f);
>>
>>     if (qemu_file_has_error(f)) {
>>         blk_mig_cleanup(mon);
>>         return 0;
>>     }
>>
>>     blk_mig_reset_dirty_cursor();
>> 
>>     if (stage == 2) {
>>
>>
>>     another question is: since you event-tap io write(I think IO READ
>> should also be event-tapped, as read may cause io chip state to
>> change),  you then need not invoke qemu_savevm_state_full in
>> qemu_savevm_trans_complete, right? thanks.
>
> It's not necessary to tap IO READ, but you can if you like.  We also
> have
> experimental patches for this to reduce rams to be transfered.  But I
> don't
> understand why we don't have to invoke qemu_savevm_state_full although
> I
> think we may reduce number of rams by replaying IO READ on the
> secondary.
>

 I first think the objective of io-Write event-tap is to reproduce the
 same device state on the other side, though I doubt this,  so I think
 IO-Read also should be recorded and replayed. since event-tap is only
 to initial a ft transaction, the sync. of states still depend on
 qemu_save_vm_live/full,  I understand the design now, thanks.

 but I don't understand why io-write event-tap can reduce transfered
 rams as you mentioned, the amount of rams only depend on dirty pages,
 IO write don't change the normal process unlike block write, right?
>>>
>>> The point is, if we can assure that IO read retrieves the same data on
>>> both
>>> sides, instead o

Re: [Qemu-devel] Re: [PATCH] lsi53c895a: add support for ABORT messages

[Kevin Wolf]

Am 09.03.2011 00:04, schrieb Peter Lieven:
> 
> Am 07.10.2010 um 13:27 schrieb Kevin Wolf:
> 
>> Am 06.09.2010 16:42, schrieb Bernhard Kohl:
>>> If these messages are not handled correctly the guest driver may hang.
>>>
>>> Always mandatory:
>>> - ABORT
>>> - BUS DEVICE RESET
>>>
>>> Mandatory if tagged queuing is implemented (which disks usually do):
>>> - ABORT TAG
>>> - CLEAR QUEUE
>>>
>>> Signed-off-by: Bernhard Kohl 
>>
>> Nicholas, as you seem to have touched the lsi code recently, care to
>> review this one? Assuming that you are reasonably familiar with both the
>> hardware and the code, you should be quicker than me with this.
> 
> Is there a reason why this patch was never added to the stable qemu-kvm 
> release?
> At least int qemu-kvm-0.14.0 I still can't find it.

The reason is that it still didn't get a review.

Kevin

Re: [Qemu-devel] Re: segmentation fault in qemu-kvm-0.14.0

[Corentin Chary]

On Wed, Mar 9, 2011 at 7:37 AM, Jan Kiszka  wrote:
> On 2011-03-08 23:53, Peter Lieven wrote:
>> Hi,
>>
>> during testing of qemu-kvm-0.14.0 i can reproduce the following segfault. i 
>> have seen similar crash already in 0.13.0, but had no time to debug.
>> my guess is that this segfault is related to the threaded vnc server which 
>> was introduced in qemu 0.13.0. the bug is only triggerable if a vnc
>> client is attached. it might also be connected to a resolution change in the 
>> guest. i have a backtrace attached. the debugger is still running if someone
>> needs more output
>>
>
> ...
>
>> Thread 1 (Thread 0x77ff0700 (LWP 29038)):
>> #0  0x in ?? ()
>> No symbol table info available.
>> #1  0x0041d669 in main_loop_wait (nonblocking=0)
>>     at /usr/src/qemu-kvm-0.14.0/vl.c:1388
>
> So we are calling a IOHandlerRecord::fd_write handler that is NULL.
> Looking at qemu_set_fd_handler2, this may happen if that function is
> called for an existing io-handler entry with non-NULL write handler,
> passing a NULL write and a non-NULL read handler. And all this without
> the global mutex held.
>
> And there are actually calls in vnc_client_write_plain and
> vnc_client_write_locked (in contrast to vnc_write) that may generate
> this pattern. It's probably worth validating that the iothread lock is
> always held when qemu_set_fd_handler2 is invoked to confirm this race
> theory, adding something like
>
> assert(pthread_mutex_trylock(&qemu_mutex) != 0);
> (that's for qemu-kvm only)
>
> BTW, qemu with just --enable-vnc-thread, ie. without io-thread support,
> should always run into this race as it then definitely lacks a global mutex.

I'm not sure what mutex should be locked here (qemu_global_mutex,
qemu_fair_mutex, lock_iothread).
But here is where is should be locked (other vnc_write calls in this
thread should never trigger qemu_set_fd_handler):

diff --git a/ui/vnc-jobs-async.c b/ui/vnc-jobs-async.c
index 1d4c5e7..e02d891 100644
--- a/ui/vnc-jobs-async.c
+++ b/ui/vnc-jobs-async.c
@@ -258,7 +258,9 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
 goto disconnected;
 }

+/* lock */
 vnc_write(job->vs, vs.output.buffer, vs.output.offset);
+/* unlock */

 disconnected:
 /* Copy persistent encoding data */
@@ -267,7 +269,9 @@ disconnected:
 vnc_unlock_output(job->vs);

 if (flush) {
+/* lock */
 vnc_flush(job->vs);
+   /* unlock */
 }

 vnc_lock_queue(queue)

-- 
Corentin Chary
http://xf.iksaif.net

Re: [Qemu-devel] [PATCH 00/22] QAPI Round 1

[Avi Kivity]

On 03/08/2011 09:19 PM, Anthony Liguori wrote:
Both the guest and the management agent (and both can listen for 
events).  I don't see why guest->qemu RPC is problematic for 
migration - at least when qemu terminates it.



If it's terminated in QEMU, it's fine, but then it's not QMP anymore.  
Let me think about whether there's a way to achieve this without a 
guest->qemu RPC.


Why not?

{ execute: 'write-keystore' arguments: { 'key': 'foo', 'value': 'bar' } }

etc.

--
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH 09/18] Introduce event-tap.

[Yoshiaki Tamura]

ya su wrote:

Yoshi:

 I meet one problem if I killed a ft source VM, the dest ft VM will
return errors as the following:

qemu-system-x86_64: fill buffer failed, Resource temporarily unavailable
qemu-system-x86_64: recv header failed

 the problem is that the dest VM can not continue to run, as it is
interrupted in the middle of a transaction, some of rams have been
updated, but the others not, do you have any plan for rolling back to
cancel the interrupted transaction? thanks.


No it's not a problem.  This is one of FAQs I get, but just press cont or c in 
the secondary qemu, it should run.


Thanks,

Yoshi




Green.



2011/3/9 Yoshiaki Tamura:

ya su wrote:


Yoshi:

 I think event-tap is a great idea, it remove the reading from disk
which will increase ft effiency much better as your plan in later
series.

 one question: IO read/write may dirty rams, but it is difficute to
differ them from other dirty pages like caused by  running of
softwares,  whether that means you need change all the emulated device
realization?  actually I think it will not send too much rams caused
by IO Read/Write in ram_save_live, but if It can event-tap IO
read/write and replay on the other side, Does that means we don't need
call qemu_savevm_state_full in ft transactoins?


I'm not expecting to remove qemu_savevm_state_full in the transaction.  Just
reduce the number of pages to be transfered as a result.

Thanks,

Yoshi



Green.


2011/3/9 Yoshiaki Tamura:


ya su wrote:


2011/3/8 Yoshiaki Tamura:


ya su wrote:


Yokshiaki:

 event-tap record block and io wirte events, and replay these on
the other side, so block_save_live is useless during the latter ft
phase, right? if so, I think it need to process the following code in
block_save_live function:


Actually no.  It just replays the last events only.  We do have patches
that
enable block replication without using block live migration, like the
way
you described above.  In that case, we disable block live migration
when
  we
go into ft mode.  We're thinking to propose it after this series get
settled.


so event-tap's objective is to initial a ft transaction, to start the
sync. of ram/block/device states? if so, it need not change
bdrv_aio_writev/bdrv_aio_flush normal process, on the other side it
need not invokde bdrv_aio_writev either, right?


Mostly yes, but because event-tap is queuing requests from block/net, it
needs to flush queued requests after the transaction on the primary side.
  On the secondary, it currently doesn't have to invoke bdrv_aio_writev as
you mentioned.  But will change soon to enable block replication with
event-tap.







 if (stage == 1) {
 init_blk_migration(mon, f);

 /* start track dirty blocks */
 set_dirty_tracking(1);
 }
--
the following code will send block to the other side, as this will
also be done by event-tap replay. I think it should placed in stage 3,
before the assert line. (this may affect some stage 2 rate-limit
then, so this can be placed in stage 2, though it looks ugly), another
choice is to avoid the invocation of block_save_live, right?
---
 flush_blks(f);

 if (qemu_file_has_error(f)) {
 blk_mig_cleanup(mon);
 return 0;
 }

 blk_mig_reset_dirty_cursor();

 if (stage == 2) {


 another question is: since you event-tap io write(I think IO READ
should also be event-tapped, as read may cause io chip state to
change),  you then need not invoke qemu_savevm_state_full in
qemu_savevm_trans_complete, right? thanks.


It's not necessary to tap IO READ, but you can if you like.  We also
have
experimental patches for this to reduce rams to be transfered.  But I
don't
understand why we don't have to invoke qemu_savevm_state_full although
I
think we may reduce number of rams by replaying IO READ on the
secondary.



I first think the objective of io-Write event-tap is to reproduce the
same device state on the other side, though I doubt this,  so I think
IO-Read also should be recorded and replayed. since event-tap is only
to initial a ft transaction, the sync. of states still depend on
qemu_save_vm_live/full,  I understand the design now, thanks.

but I don't understand why io-write event-tap can reduce transfered
rams as you mentioned, the amount of rams only depend on dirty pages,
IO write don't change the normal process unlike block write, right?


The point is, if we can assure that IO read retrieves the same data on
both
sides, instead of dirtying the ram by read, meaning we have to transfer
in
the transaction, just replay the operation and get the same data on the
otherside. Anyway, that's just a plan :)

Thanks,

Yoshi




Thanks,

Yoshi




Green.



2011/2/24 Yoshiaki Tamura:


event-tap controls when to start FT transaction, and provides proxy
functions to called from net/block devices.  While FT transaction, it
queues 

Re: [Qemu-devel] Re: segmentation fault in qemu-kvm-0.14.0

[Jan Kiszka]

On 2011-03-09 09:50, Corentin Chary wrote:
> On Wed, Mar 9, 2011 at 7:37 AM, Jan Kiszka  wrote:
>> On 2011-03-08 23:53, Peter Lieven wrote:
>>> Hi,
>>>
>>> during testing of qemu-kvm-0.14.0 i can reproduce the following segfault. i 
>>> have seen similar crash already in 0.13.0, but had no time to debug.
>>> my guess is that this segfault is related to the threaded vnc server which 
>>> was introduced in qemu 0.13.0. the bug is only triggerable if a vnc
>>> client is attached. it might also be connected to a resolution change in 
>>> the guest. i have a backtrace attached. the debugger is still running if 
>>> someone
>>> needs more output
>>>
>>
>> ...
>>
>>> Thread 1 (Thread 0x77ff0700 (LWP 29038)):
>>> #0  0x in ?? ()
>>> No symbol table info available.
>>> #1  0x0041d669 in main_loop_wait (nonblocking=0)
>>> at /usr/src/qemu-kvm-0.14.0/vl.c:1388
>>
>> So we are calling a IOHandlerRecord::fd_write handler that is NULL.
>> Looking at qemu_set_fd_handler2, this may happen if that function is
>> called for an existing io-handler entry with non-NULL write handler,
>> passing a NULL write and a non-NULL read handler. And all this without
>> the global mutex held.
>>
>> And there are actually calls in vnc_client_write_plain and
>> vnc_client_write_locked (in contrast to vnc_write) that may generate
>> this pattern. It's probably worth validating that the iothread lock is
>> always held when qemu_set_fd_handler2 is invoked to confirm this race
>> theory, adding something like
>>
>> assert(pthread_mutex_trylock(&qemu_mutex) != 0);
>> (that's for qemu-kvm only)
>>
>> BTW, qemu with just --enable-vnc-thread, ie. without io-thread support,
>> should always run into this race as it then definitely lacks a global mutex.
> 
> I'm not sure what mutex should be locked here (qemu_global_mutex,
> qemu_fair_mutex, lock_iothread).

In upstream qemu, the latter - if it exists (which is not the case in
non-io-thread mode).

In qemu-kvm, those locks are yet unused. Rather the code in qemu-kvm.c
implements the global lock.

> But here is where is should be locked (other vnc_write calls in this
> thread should never trigger qemu_set_fd_handler):
> 
> diff --git a/ui/vnc-jobs-async.c b/ui/vnc-jobs-async.c
> index 1d4c5e7..e02d891 100644
> --- a/ui/vnc-jobs-async.c
> +++ b/ui/vnc-jobs-async.c
> @@ -258,7 +258,9 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
>  goto disconnected;
>  }
> 
> +/* lock */
>  vnc_write(job->vs, vs.output.buffer, vs.output.offset);
> +/* unlock */
> 
>  disconnected:
>  /* Copy persistent encoding data */
> @@ -267,7 +269,9 @@ disconnected:
>  vnc_unlock_output(job->vs);
> 
>  if (flush) {
> +/* lock */
>  vnc_flush(job->vs);
> +   /* unlock */
>  }

Particularly this call is critical as it will trigger
vnc_client_write_locked which may NULL'ify the write handler.

But I'm also not sure about vnc_send_framebuffer_update. Someone has to
go through the threaded vnc code again, very thoroughly. It looks fragile.

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] segmentation fault in qemu-kvm-0.14.0

[Stefan Weil]

Am 09.03.2011 08:39, schrieb Michael Tokarev:

09.03.2011 10:26, Stefan Weil wrote:

Am 08.03.2011 23:53, schrieb Peter Lieven:

Hi,

during testing of qemu-kvm-0.14.0 i can reproduce the following
segfault. i have seen similar crash already in 0.13.0, but had no time
to debug.
my guess is that this segfault is related to the threaded vnc server
which was introduced in qemu 0.13.0. the bug is only triggerable if a vnc
client is attached. it might also be connected to a resolution change
in the guest. i have a backtrace attached. the debugger is still
running if someone
needs more output


[]

Hi Peter,

did you apply this patch which fixes one of the known vnc problems
(but is still missing in qemu git master):

http://lists.nongnu.org/archive/html/qemu-devel/2011-03/msg00256.html

This patch is not suitable for 0.14 since in current qemu/master quite
alot of stuff were changed in this area (bitmaps added), there's no
similar infrastructure in 0.14.


Then you can read this thread:

http://lists.nongnu.org/archive/html/qemu-devel/2011-03/msg00313.html

And finally the following modifications of ui/vnc.c might help to see
whether you experience the same kind of crash as I get here in
my environment. They add assertions for bad memory access
which occurs sometimes when a vnc client-server connection exists and
the screen is refreshed after a resolution change.
The code line with the //~ comment also includes a fix which
works for me.

The same is true for this patch, but of a less extent: it can be applied
manually (the bitmap_empty context line).

I wonder if something similar actually exists in 0.13/0.14 too and needs
to be backported to -stable.


Regards,
Stefan W.

Thanks!

/mjt



I just tested stable-0.14. It shows the same kind of bug.
Output of qemu run with valgrind:

==18143== Conditional jump or move depends on uninitialised value(s)
==18143==at 0x4027022: bcmp (mc_replace_strmem.c:541)
==18143==by 0x80EEF96: vnc_refresh_server_surface (vnc.c:2292)
==18143==by 0x80EF0F1: vnc_refresh (vnc.c:2322)
==18143==by 0x80FA026: qemu_run_timers (qemu-timer.c:503)
==18143==by 0x80FA34E: qemu_run_all_timers (qemu-timer.c:634)
==18143==by 0x816BBB6: main_loop_wait (vl.c:1383)
==18143==by 0x816BC36: main_loop (vl.c:1424)
==18143==by 0x816FEAF: main (vl.c:3136)

Stefan

Re: [Qemu-devel] KVM call minutes for Mar 8

[Kevin Wolf]

Am 08.03.2011 16:50, schrieb Chris Wright:
> QAPI merge plans
> - should be 100% back compat
> - qmp moved over
> - hmp moved over
> - 1st pass, core infrastructure (includes test framework)
> - 2nd pass, command conversion
> - 3rd pass, more controversial bits
> - adds dependencies: glib and python
> - some testing based on kvm-unit-test micro-os instance (e.g. added a balloon
>   and run commands against it to test)
>   - add more functionality here? (kvm autotest is slow, above is quick)
> - will hit some point where full functionality is needed
>   - have a mini linux to do this (lags where driver updates are part of test)

Depending on what we want to include in such tests, we might want to
have tests with a badly behaving kernel, which Linux hopefully isn't.

Kevin

Re: [Qemu-devel] Re: [PATCH] lsi53c895a: add support for ABORT messages

[Bernhard Kohl]

Am 09.03.2011 09:47, schrieb ext Kevin Wolf:

Am 09.03.2011 00:04, schrieb Peter Lieven:

Am 07.10.2010 um 13:27 schrieb Kevin Wolf:


Am 06.09.2010 16:42, schrieb Bernhard Kohl:

If these messages are not handled correctly the guest driver may hang.

Always mandatory:
- ABORT
- BUS DEVICE RESET

Mandatory if tagged queuing is implemented (which disks usually do):
- ABORT TAG
- CLEAR QUEUE

Signed-off-by: Bernhard Kohl

Nicholas, as you seem to have touched the lsi code recently, care to
review this one? Assuming that you are reasonably familiar with both the
hardware and the code, you should be quicker than me with this.

Is there a reason why this patch was never added to the stable qemu-kvm release?
At least int qemu-kvm-0.14.0 I still can't find it.

The reason is that it still didn't get a review.


I still depend on this patch. It's needed for our legacy guest OS.
It's heavily in use here with STGT iSCSI disks via scsi-generic.

Bernhard

Re: [Qemu-devel] [PATCH v5] PING: Fix ATA SMART and CHECK POWER MODE

[Kevin Wolf]

Am 09.03.2011 05:26, schrieb Ryan Harper:
> * Brian Wheeler  [2011-03-01 07:35]:
>> This patch fixes two things:
>>
>>  1) CHECK POWER MODE
>>
>> The error return value wasn't always zero, so it would show up as
>> offline.  Error is now explicitly set to zero.
>>
>>  2) SMART
>>
>> The smart values that were returned were invalid and tools like skdump
>> would not recognize that the smart data was actually valid and would
>> dump weird output.  The data has been fixed up and raw value support
>> was added.  Tools like skdump and palimpsest work as expected.
>>
>> v5 changes:  rebase
>> v4 changes:  incorporate changes from Ryan Harper
>> v3 changes:  don't reformat code I didn't change
>> v2 changes:  use single structure instead of one for thresholds and one
>> for data.
>>
> 
> Sorry, haven't had a chance to catch up on qemu-devel, meant to respond
> sooner.  Changes look good.
> 
> Acked-by: Ryan Harper 

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] how to convert guest virtual address to host virtual address in QEMU?

[Gunasekaran Dharman]

Hi Stefan

Thanks for the reply.
Sure, I will look into TCG for improvement as a long term plan.
But for now, I have to solve this memcpy performance issue.

Regarding calculating host virtual address from guest virtual address, I
think, QEMU must be doing this somehow.
I would like to know how QEMU is handling the guest virtual address?
It will be very much helpful if you can throw some light on this.

Regards
Guna

On Wed, Mar 9, 2011 at 12:30 AM, Stefan Hajnoczi  wrote:

>  On Tue, Mar 8, 2011 at 5:30 PM, Gunasekaran Dharman 
> wrote:
> > From guest application, Iam passing some virtual addresses to QEMU
> through
> > device I/O operation. Now I want to convert these guest virtual addresses
> to
> > host virtual addresses so that I can perform some operation using them.
> In
> > QEMU, is there any macro or function available to convert guest virtual
> > address to host virtual address?
> > My objective is to perform guest's memcpy operation in host so that it
> will
> > be faster.
>
> That's not really possible.  On architectures with a software-managed
> TLB you don't know the layout of the virtual memory structures
> (because they are defined in software and not standardized).
>
> I suggest learning about and optimizing TCG instead of inventing a
> memcpy device because it can benefit all code and does not require a
> custom device and guest drivers.
>
> Stefan
>

Re: [Qemu-devel] Re: [PATCH] lsi53c895a: add support for ABORT messages

[Peter Lieven]

Am 09.03.2011 um 10:25 schrieb Bernhard Kohl:

> Am 09.03.2011 09:47, schrieb ext Kevin Wolf:
>> Am 09.03.2011 00:04, schrieb Peter Lieven:
>>> Am 07.10.2010 um 13:27 schrieb Kevin Wolf:
>>> 
 Am 06.09.2010 16:42, schrieb Bernhard Kohl:
> If these messages are not handled correctly the guest driver may hang.
> 
> Always mandatory:
> - ABORT
> - BUS DEVICE RESET
> 
> Mandatory if tagged queuing is implemented (which disks usually do):
> - ABORT TAG
> - CLEAR QUEUE
> 
> Signed-off-by: Bernhard Kohl
 Nicholas, as you seem to have touched the lsi code recently, care to
 review this one? Assuming that you are reasonably familiar with both the
 hardware and the code, you should be quicker than me with this.
>>> Is there a reason why this patch was never added to the stable qemu-kvm 
>>> release?
>>> At least int qemu-kvm-0.14.0 I still can't find it.
>> The reason is that it still didn't get a review.
>> 
> I still depend on this patch. It's needed for our legacy guest OS.
> It's heavily in use here with STGT iSCSI disks via scsi-generic.

We use it in qemu-0.12.5 as well for a very long time. I saw an assertion last 
year
with errors of unimplemented abort feature preceding. Since we use this patch
we have never seen it again.

Peter

> 
> Bernhard

Re: [Qemu-devel] Re: segmentation fault in qemu-kvm-0.14.0

[Corentin Chary]

Re-reading:

>> So we are calling a IOHandlerRecord::fd_write handler that is NULL.
>> Looking at qemu_set_fd_handler2, this may happen if that function is
>> called for an existing io-handler entry with non-NULL write handler,
>> passing a NULL write and a non-NULL read handler. And all this without
>> the global mutex held.

When using the vnc thread, nothing in the vnc thread will never be
called directly by an IO-handler. So I don't really how in would
trigger this.
And since there is a lock for accessing the output buffer (and the
network actually), there should not be any race condition either.

So the real issue, is that qemu_set_fd_handler2() is called outside of
the main thread by those two vnc_write() and vnc_flush() calls,
without any kind of locking.

> In upstream qemu, the latter - if it exists (which is not the case in
> non-io-thread mode).
> In qemu-kvm, those locks are yet unused. Rather the code in qemu-kvm.c
> implements the global lock.

So there is currently no lock for that when io-thread is disabled :/.
Spice also seems to project this kind of thing with
qemu_mutex_lock_iothread().

Maybe qemu_mutex_lock_iothread() should also be defined when
CONFIG_VNC_THREAD=y ?

> But I'm also not sure about vnc_send_framebuffer_update. Someone has to
> go through the threaded vnc code again, very thoroughly. It looks fragile.

while vnc-thread is enabled vnc_send_framebuffer_update() will always
call vnc_write() with csock = -1 in a temporary buffer. Check
vnc_async_encoding_start() and vnc_async_encoding_end(), they provide
a kind of "sandbox" that prevent the thread to write anything the
main-thread will use. You can also see that as a "transaction": the
thread compute the update in a temporary buffer, and only send it to
the network (real vnc_write calls with csock correctly set) once it's
successfully finished.

The is only two functions calls that break this isolation are the two
that I pointed out earlier.

-- 
Corentin Chary
http://xf.iksaif.net

Re: [Qemu-devel] Re: segmentation fault in qemu-kvm-0.14.0

[Jan Kiszka]

On 2011-03-09 10:54, Corentin Chary wrote:
> Re-reading:
> 
>>> So we are calling a IOHandlerRecord::fd_write handler that is NULL.
>>> Looking at qemu_set_fd_handler2, this may happen if that function is
>>> called for an existing io-handler entry with non-NULL write handler,
>>> passing a NULL write and a non-NULL read handler. And all this without
>>> the global mutex held.
> 
> When using the vnc thread, nothing in the vnc thread will never be
> called directly by an IO-handler. So I don't really how in would
> trigger this.
> And since there is a lock for accessing the output buffer (and the
> network actually), there should not be any race condition either.
> 
> So the real issue, is that qemu_set_fd_handler2() is called outside of
> the main thread by those two vnc_write() and vnc_flush() calls,
> without any kind of locking.

Yes, that's what I was referring to.

> 
>> In upstream qemu, the latter - if it exists (which is not the case in
>> non-io-thread mode).
>> In qemu-kvm, those locks are yet unused. Rather the code in qemu-kvm.c
>> implements the global lock.
> 
> So there is currently no lock for that when io-thread is disabled :/.
> Spice also seems to project this kind of thing with
> qemu_mutex_lock_iothread().
> 
> Maybe qemu_mutex_lock_iothread() should also be defined when
> CONFIG_VNC_THREAD=y ?
> 
>> But I'm also not sure about vnc_send_framebuffer_update. Someone has to
>> go through the threaded vnc code again, very thoroughly. It looks fragile.
> 
> while vnc-thread is enabled vnc_send_framebuffer_update() will always
> call vnc_write() with csock = -1 in a temporary buffer. Check
> vnc_async_encoding_start() and vnc_async_encoding_end(), they provide
> a kind of "sandbox" that prevent the thread to write anything the
> main-thread will use. You can also see that as a "transaction": the
> thread compute the update in a temporary buffer, and only send it to
> the network (real vnc_write calls with csock correctly set) once it's
> successfully finished.
> 
> The is only two functions calls that break this isolation are the two
> that I pointed out earlier.

Probably the best way is to make vnc stop fiddling with
qemu_set_fd_handler2, specifically in threaded mode. Why does it need to
set/reset the write handler all the time?

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] segmentation fault in qemu-kvm-0.14.0

[Peter Lieven]

Am 09.03.2011 um 08:26 schrieb Stefan Weil:

> Am 08.03.2011 23:53, schrieb Peter Lieven:
>> Hi,
>> 
>> during testing of qemu-kvm-0.14.0 i can reproduce the following segfault. i 
>> have seen similar crash already in 0.13.0, but had no time to debug.
>> my guess is that this segfault is related to the threaded vnc server which 
>> was introduced in qemu 0.13.0. the bug is only triggerable if a vnc
>> client is attached. it might also be connected to a resolution change in the 
>> guest. i have a backtrace attached. the debugger is still running if someone
>> needs more output
>> 
>> Reading symbols from /usr/local/bin/qemu-system-x86_64...done.
>> (gdb) r -net tap,vlan=141,script=no,downscript=no,ifname=tap0 -net 
>> nic,vlan=141,model=rtl8139,macaddr=52:54:00:ff:00:93 -drive 
>> format=host_device,file=/dev/mapper/iqn.2001-05.co
>> m.equallogic:0-8a0906-e6b70e107-e87000e7acf4d4e5-lieven-winxp-r17453,if=ide,boot=on,cache=none,aio=native
>>  -m 1024 -monitor tcp:0:4001,server,nowait -vnc :1 -name 'lieven-winxp-te
>> st' -boot order=c,menu=on -k de -pidfile /var/run/qemu/vm-265.pid -mem-path 
>> /hugepages -mem-prealloc -cpu qemu64,model_id='Intel(R) Xeon(R) CPU E5640 @ 
>> 2.67GHz',-n
>> x -rtc base=localtime,clock=vm -vga cirrus -usb -usbdevice tablet
>> Starting program: /usr/local/bin/qemu-system-x86_64 -net 
>> tap,vlan=141,script=no,downscript=no,ifname=tap0 -net 
>> nic,vlan=141,model=rtl8139,macaddr=52:54:00:ff:00:93 -drive format
>> =host_device,file=/dev/mapper/iqn.2001-05.com.equallogic:0-8a0906-e6b70e107-e87000e7acf4d4e5-lieven-winxp-r17453,if=ide,boot=on,cache=none,aio=native
>>  -m 1024 -monitor tcp:0:4001,
>> server,nowait -vnc :1 -name 'lieven-winxp-test' -boot order=c,menu=on -k de 
>> -pidfile /var/run/qemu/vm-265.pid -mem-path /hugepages -mem-prealloc -cpu 
>> qemu64,model_id='Intel(R
>> ) Xeon(R) CPU E5640 @ 2.67GHz',-nx -rtc base=localtime,clock=vm -vga cirrus 
>> -usb -usbdevice tablet
>> [Thread debugging using libthread_db enabled]
>> 
>> [New Thread 0x7694e700 (LWP 29042)]
>> [New Thread 0x76020700 (LWP 29043)]
>> [New Thread 0x7581f700 (LWP 29074)]
>> [Thread 0x7581f700 (LWP 29074) exited]
>> [New Thread 0x7581f700 (LWP 29124)]
>> [Thread 0x7581f700 (LWP 29124) exited]
>> [New Thread 0x7581f700 (LWP 29170)]
>> [Thread 0x7581f700 (LWP 29170) exited]
>> [New Thread 0x7581f700 (LWP 29246)]
>> [Thread 0x7581f700 (LWP 29246) exited]
>> [New Thread 0x7581f700 (LWP 29303)]
>> [Thread 0x7581f700 (LWP 29303) exited]
>> [New Thread 0x7581f700 (LWP 29349)]
>> [Thread 0x7581f700 (LWP 29349) exited]
>> [New Thread 0x7581f700 (LWP 29399)]
>> [Thread 0x7581f700 (LWP 29399) exited]
>> [New Thread 0x7581f700 (LWP 29471)]
>> [Thread 0x7581f700 (LWP 29471) exited]
>> [New Thread 0x7581f700 (LWP 29521)]
>> [Thread 0x7581f700 (LWP 29521) exited]
>> [New Thread 0x7581f700 (LWP 29593)]
>> [Thread 0x7581f700 (LWP 29593) exited]
>> [New Thread 0x7581f700 (LWP 29703)]
>> [Thread 0x7581f700 (LWP 29703) exited]
>> 
>> Program received signal SIGSEGV, Segmentation fault.
>> 0x in ?? ()
>> (gdb)
>> 
>> (gdb) thread apply all bt full
>> 
>> Thread 3 (Thread 0x76020700 (LWP 29043)):
>> #0 0x779c385c in pthread_cond_wait@@GLIBC_2.3.2 ()
>> from /lib/libpthread.so.0
>> No symbol table info available.
>> #1 0x004d3ae1 in qemu_cond_wait (cond=0x1612d50, mutex=0x1612d80)
>> at qemu-thread.c:133
>> err = 0
>> __func__ = "qemu_cond_wait"
>> #2 0x004d2b39 in vnc_worker_thread_loop (queue=0x1612d50)
>> at ui/vnc-jobs-async.c:198
>> job = 0x7058cd20
>> entry = 0x0
>> tmp = 0x0
>> vs = {csock = -1, ds = 0x15cb380, dirty = {{0, 0, 0, 0,
>> 0} }, vd = 0x1607ff0, need_update = 0,
>> force_update = 0, features = 243, absolute = 0, last_x = 0,
>> last_y = 0, client_width = 0, client_height = 0, vnc_encoding = 7,
>> major = 0, minor = 0, challenge = '\000' ,
>> info = 0x0, output = {capacity = 3194, offset = 2723,
>> buffer = 0x1fbbfd0 ""}, input = {capacity = 0, offset = 0,
>> buffer = 0x0}, write_pixels = 0x4c4bc9 ,
>> clientds = {flags = 0 '\000', width = 720, height = 400,
>> linesize = 2880,
>> data = 0x76021010 ,
>> pf = {bits_per_pixel = 32 ' ', bytes_per_pixel = 4 '\004',
>> depth = 24 '\030', rmask = 0, gmask = 0, bmask = 0, amask = 0,
>> rshift = 16 '\020', gshift = 8 '\b', bshift = 0 '\000',
>> ashift = 24 '\030', rmax = 255 '\377', gmax = 255 '\377',
>> bmax = 255 '\377', amax = 255 '\377', rbits = 8 '\b',
>> gbits = 8 '\b', bbits = 8 '\b', abits = 8 '\b'}},
>> audio_cap = 0x0, as = {freq = 0, nchannels = 0, fmt = AUD_FMT_U8,
>> endianness = 0}, read_handler = 0, read_handler_expect = 0,
>> modifiers_state = '\000' , led = 0x0,
>> abort = false, output_mutex = {lock = {__data = {__lock = 0,
>> __count = 0, __owner = 0, __nusers = 0, __kind = 0,
>> __spins = 0, __list = {__prev = 0x0, __next = 0x0}},
>> __size = '\000' , __align = 0}}, tight = {
>> type = 7, quality = 255

Re: [Qemu-devel] Re: segmentation fault in qemu-kvm-0.14.0

[Jan Kiszka]

On 2011-03-09 10:58, Jan Kiszka wrote:
> On 2011-03-09 10:54, Corentin Chary wrote:
>> Re-reading:
>>
 So we are calling a IOHandlerRecord::fd_write handler that is NULL.
 Looking at qemu_set_fd_handler2, this may happen if that function is
 called for an existing io-handler entry with non-NULL write handler,
 passing a NULL write and a non-NULL read handler. And all this without
 the global mutex held.
>>
>> When using the vnc thread, nothing in the vnc thread will never be
>> called directly by an IO-handler. So I don't really how in would
>> trigger this.
>> And since there is a lock for accessing the output buffer (and the
>> network actually), there should not be any race condition either.
>>
>> So the real issue, is that qemu_set_fd_handler2() is called outside of
>> the main thread by those two vnc_write() and vnc_flush() calls,
>> without any kind of locking.
> 
> Yes, that's what I was referring to.
> 
>>
>>> In upstream qemu, the latter - if it exists (which is not the case in
>>> non-io-thread mode).
>>> In qemu-kvm, those locks are yet unused. Rather the code in qemu-kvm.c
>>> implements the global lock.
>>
>> So there is currently no lock for that when io-thread is disabled :/.
>> Spice also seems to project this kind of thing with
>> qemu_mutex_lock_iothread().
>>
>> Maybe qemu_mutex_lock_iothread() should also be defined when
>> CONFIG_VNC_THREAD=y ?
>>
>>> But I'm also not sure about vnc_send_framebuffer_update. Someone has to
>>> go through the threaded vnc code again, very thoroughly. It looks fragile.
>>
>> while vnc-thread is enabled vnc_send_framebuffer_update() will always
>> call vnc_write() with csock = -1 in a temporary buffer. Check
>> vnc_async_encoding_start() and vnc_async_encoding_end(), they provide
>> a kind of "sandbox" that prevent the thread to write anything the
>> main-thread will use. You can also see that as a "transaction": the
>> thread compute the update in a temporary buffer, and only send it to
>> the network (real vnc_write calls with csock correctly set) once it's
>> successfully finished.
>>
>> The is only two functions calls that break this isolation are the two
>> that I pointed out earlier.
> 
> Probably the best way is to make vnc stop fiddling with
> qemu_set_fd_handler2, specifically in threaded mode. Why does it need to
> set/reset the write handler all the time?

The other question is: Who's responsible for writing to the client
socket in threaded mode? Only the vnc thread(s), or also other qemu
threads? In the former case, just avoid setting a write handler at all.

Jan



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] Re: segmentation fault in qemu-kvm-0.14.0

[Peter Lieven]

Am 09.03.2011 um 08:37 schrieb Jan Kiszka:

> On 2011-03-08 23:53, Peter Lieven wrote:
>> Hi,
>> 
>> during testing of qemu-kvm-0.14.0 i can reproduce the following segfault. i 
>> have seen similar crash already in 0.13.0, but had no time to debug.
>> my guess is that this segfault is related to the threaded vnc server which 
>> was introduced in qemu 0.13.0. the bug is only triggerable if a vnc
>> client is attached. it might also be connected to a resolution change in the 
>> guest. i have a backtrace attached. the debugger is still running if someone
>> needs more output
>> 
> 
> ...
> 
>> Thread 1 (Thread 0x77ff0700 (LWP 29038)):
>> #0  0x in ?? ()
>> No symbol table info available.
>> #1  0x0041d669 in main_loop_wait (nonblocking=0)
>>at /usr/src/qemu-kvm-0.14.0/vl.c:1388
> 
> So we are calling a IOHandlerRecord::fd_write handler that is NULL.
> Looking at qemu_set_fd_handler2, this may happen if that function is
> called for an existing io-handler entry with non-NULL write handler,
> passing a NULL write and a non-NULL read handler. And all this without
> the global mutex held.
> 
> And there are actually calls in vnc_client_write_plain and
> vnc_client_write_locked (in contrast to vnc_write) that may generate
> this pattern. It's probably worth validating that the iothread lock is
> always held when qemu_set_fd_handler2 is invoked to confirm this race
> theory, adding something like
> 
> assert(pthread_mutex_trylock(&qemu_mutex) != 0);
> (that's for qemu-kvm only)

i added it and will run tests now.

thanks,
peter

> 
> BTW, qemu with just --enable-vnc-thread, ie. without io-thread support,
> should always run into this race as it then definitely lacks a global mutex.
> 
> Jan
> 

Re: [Qemu-devel] Re: segmentation fault in qemu-kvm-0.14.0

[Corentin Chary]

> Probably the best way is to make vnc stop fiddling with
> qemu_set_fd_handler2, specifically in threaded mode.
> Why does it need to set/reset the write handler all the time?

I didn't write the original code, but it's probably to avoid calling a
write handler when there is no data to write. That would make select()
busy loop when there are actually no data to write.
There is also the vnc_disconnect_start() case when the socket seems to
be broken and we remove all handlers.

> The other question is: Who's responsible for writing to the client
> socket in threaded mode? Only the vnc thread(s), or also other qemu
> threads? In the former case, just avoid setting a write handler at all.

Cheap stuff is done by the main thread (cursor, etc...). The thread
only do framebuffer updates.

-- 
Corentin Chary
http://xf.iksaif.net

Re: [Qemu-devel] Re: segmentation fault in qemu-kvm-0.14.0

[Jan Kiszka]

On 2011-03-09 11:06, Corentin Chary wrote:
>> Probably the best way is to make vnc stop fiddling with
>> qemu_set_fd_handler2, specifically in threaded mode.
>> Why does it need to set/reset the write handler all the time?
> 
> I didn't write the original code, but it's probably to avoid calling a
> write handler when there is no data to write. That would make select()
> busy loop when there are actually no data to write.
> There is also the vnc_disconnect_start() case when the socket seems to
> be broken and we remove all handlers.
> 
>> The other question is: Who's responsible for writing to the client
>> socket in threaded mode? Only the vnc thread(s), or also other qemu
>> threads? In the former case, just avoid setting a write handler at all.
> 
> Cheap stuff is done by the main thread (cursor, etc...). The thread
> only do framebuffer updates.

And both are synchronized with a vnc-private lock only?

The problem with this model is the non-threaded qemu execution model.
Even if we acquire the global mutex to protect handler updates against
the main select loop, this won't help here. So vnc-threading likely
needs to be made dependent on --enable-io-thread.

Jan




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] Re: segmentation fault in qemu-kvm-0.14.0

[Corentin Chary]

>> Cheap stuff is done by the main thread (cursor, etc...). The thread
>> only do framebuffer updates.
>
> And both are synchronized with a vnc-private lock only?

Yes

> The problem with this model is the non-threaded qemu execution model.
> Even if we acquire the global mutex to protect handler updates against
> the main select loop, this won't help here. So vnc-threading likely
> needs to be made dependent on --enable-io-thread.

Plus proper lock_iothread() calls right ?

If yes I'll send a patch for that (or you can do it, as you want).

Thanks,

-- 
Corentin Chary
http://xf.iksaif.net

Re: [Qemu-devel] Re: segmentation fault in qemu-kvm-0.14.0

[Jan Kiszka]

On 2011-03-09 11:14, Corentin Chary wrote:
>>> Cheap stuff is done by the main thread (cursor, etc...). The thread
>>> only do framebuffer updates.
>>
>> And both are synchronized with a vnc-private lock only?
> 
> Yes
> 
>> The problem with this model is the non-threaded qemu execution model.
>> Even if we acquire the global mutex to protect handler updates against
>> the main select loop, this won't help here. So vnc-threading likely
>> needs to be made dependent on --enable-io-thread.
> 
> Plus proper lock_iothread() calls right ?

Yep.

> 
> If yes I'll send a patch for that (or you can do it, as you want).

Don't wait for me. :)

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] OVMF Google Summer of Code ideas

[Gleb Natapov]

On Tue, Mar 08, 2011 at 09:13:38AM -0800, Jordan Justen wrote:
> 2011/3/8 Gleb Natapov :
> > On Tue, Mar 08, 2011 at 07:18:09AM +, Stefan Hajnoczi wrote:
> >> > Regarding the non-volatile variables issue, I have been trying to
> >> > develop a proposal for addressing this with a change to QEMU's
> >> > hardware support of bios.bin.  But, I don't have the suggestion (or
> >> > implementation) ready at this time.
> >>
> >> Sounds like something to keep discussing with the QEMU and SeaBIOS
> >> communities.  Gleb Natapov and Kevin O'Connor have done a lot of the
> >> recent BIOS and firmware interface work.  I think persistent CMOS has
> >> come up several times and might be similar to non-volatile UEFI
> >> storage.
> >>
> > What kind of information OVMF stores on a persistent storage?
> 
> Non-volatile variables are a general system wide environment variable
> storage facility, but one key thing to store (for instance) is the
> path to the boot image.
> 
> > CMOS
> > memory is less them 512 byte IIRC and this may not be enough. What OVMF
> > uses on real HW for non-volatile storage?
> 
> Regarding CMOS, I think qemu exposes 128 bytes of non persistent CMOS
> RAM.  Many current chipsets provide another bank of 128 bytes, meaning
> a total of 256 bytes (minus the RTC registers).
> 
> Regarding UEFI non-volatile variables on real HW:
> Most systems today have at least 1MB of flash storage located just
> below 4GB.  The entire contents can be modified, which is how firmware
> updates happen.
> 
How this flash storage is programmed? May be we can emulate something
similar.

> For a UEFI based system, the non-volatile variables generally occupy
> 8KB~64KB of the flash depending on flash space availability.
> 
> -Jordan

--
Gleb.

[Qemu-devel] [PATCH v4] Improve error handling in do_snapshot_blkdev()

[Jes . Sorensen]

From: Jes Sorensen 

In case we cannot open the newly created snapshot image, try to fall
back to the original image file and continue running on that, which
should prevent the guest from aborting.

This is a corner case which can happen if the admin by mistake
specifies the snapshot file on a virtual file system which does not
support O_DIRECT. bdrv_create() does not use O_DIRECT, but the
following open in bdrv_open() does and will then fail.

Signed-off-by: Jes Sorensen 
---
 blockdev.c |   23 +--
 1 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 0690cc8..ecf2252 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -574,9 +574,10 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, 
QObject **ret_data)
 const char *filename = qdict_get_try_str(qdict, "snapshot_file");
 const char *format = qdict_get_try_str(qdict, "format");
 BlockDriverState *bs;
-BlockDriver *drv, *proto_drv;
+BlockDriver *drv, *old_drv, *proto_drv;
 int ret = 0;
 int flags;
+char old_filename[1024];
 
 if (!filename) {
 qerror_report(QERR_MISSING_PARAMETER, "snapshot_file");
@@ -591,6 +592,11 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, 
QObject **ret_data)
 goto out;
 }
 
+pstrcpy(old_filename, sizeof(old_filename), bs->filename);
+
+old_drv = bs->drv;
+flags = bs->open_flags;
+
 if (!format) {
 format = "qcow2";
 }
@@ -610,7 +616,7 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, 
QObject **ret_data)
 }
 
 ret = bdrv_img_create(filename, format, bs->filename,
-  bs->drv->format_name, NULL, -1, bs->open_flags);
+  bs->drv->format_name, NULL, -1, flags);
 if (ret) {
 goto out;
 }
@@ -618,15 +624,20 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, 
QObject **ret_data)
 qemu_aio_flush();
 bdrv_flush(bs);
 
-flags = bs->open_flags;
 bdrv_close(bs);
 ret = bdrv_open(bs, filename, flags, drv);
 /*
- * If reopening the image file we just created fails, we really
- * are in trouble :(
+ * If reopening the image file we just created fails, fall back
+ * and try to re-open the original image. If that fails too, we
+ * are in serious trouble.
  */
 if (ret != 0) {
-abort();
+ret = bdrv_open(bs, old_filename, flags, old_drv);
+if (ret != 0) {
+qerror_report(QERR_OPEN_FILE_FAILED, old_filename);
+} else {
+qerror_report(QERR_OPEN_FILE_FAILED, filename);
+}
 }
 out:
 if (ret) {
-- 
1.7.4

[Qemu-devel] Re: segmentation fault in qemu-kvm-0.14.0

[Jan Kiszka]

On 2011-03-09 11:16, Peter Lieven wrote:
> 
> Am 09.03.2011 um 08:37 schrieb Jan Kiszka:
> 
>> On 2011-03-08 23:53, Peter Lieven wrote:
>>> Hi,
>>>
>>> during testing of qemu-kvm-0.14.0 i can reproduce the following segfault. i 
>>> have seen similar crash already in 0.13.0, but had no time to debug.
>>> my guess is that this segfault is related to the threaded vnc server which 
>>> was introduced in qemu 0.13.0. the bug is only triggerable if a vnc
>>> client is attached. it might also be connected to a resolution change in 
>>> the guest. i have a backtrace attached. the debugger is still running if 
>>> someone
>>> needs more output
>>>
>>
>> ...
>>
>>> Thread 1 (Thread 0x77ff0700 (LWP 29038)):
>>> #0  0x in ?? ()
>>> No symbol table info available.
>>> #1  0x0041d669 in main_loop_wait (nonblocking=0)
>>>at /usr/src/qemu-kvm-0.14.0/vl.c:1388
>>
>> So we are calling a IOHandlerRecord::fd_write handler that is NULL.
>> Looking at qemu_set_fd_handler2, this may happen if that function is
>> called for an existing io-handler entry with non-NULL write handler,
>> passing a NULL write and a non-NULL read handler. And all this without
>> the global mutex held.
>>
>> And there are actually calls in vnc_client_write_plain and
>> vnc_client_write_locked (in contrast to vnc_write) that may generate
>> this pattern. It's probably worth validating that the iothread lock is
>> always held when qemu_set_fd_handler2 is invoked to confirm this race
>> theory, adding something like
>>
>> assert(pthread_mutex_trylock(&qemu_mutex) != 0);
>> (that's for qemu-kvm only)
> 
> qemu_mutex doesn't exists (anymore). i can only find qemu_global_mutex and 
> qemu_fair_mutex.

In qemu-kvm, qemu-kvm.c contains that lock. In upstream, it's
qemu_global_mutex, but you will have to enable --enable-io-thread and
export it.

> 
> i try with qemu_global_mutex now. if thats not correct please tell.

I think we knokw know the call path, so you could also wait for Corentin
to send a patch and test that one.

Jan



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] Re: segmentation fault in qemu-kvm-0.14.0

[Peter Lieven]

Am 09.03.2011 um 08:37 schrieb Jan Kiszka:

> On 2011-03-08 23:53, Peter Lieven wrote:
>> Hi,
>> 
>> during testing of qemu-kvm-0.14.0 i can reproduce the following segfault. i 
>> have seen similar crash already in 0.13.0, but had no time to debug.
>> my guess is that this segfault is related to the threaded vnc server which 
>> was introduced in qemu 0.13.0. the bug is only triggerable if a vnc
>> client is attached. it might also be connected to a resolution change in the 
>> guest. i have a backtrace attached. the debugger is still running if someone
>> needs more output
>> 
> 
> ...
> 
>> Thread 1 (Thread 0x77ff0700 (LWP 29038)):
>> #0  0x in ?? ()
>> No symbol table info available.
>> #1  0x0041d669 in main_loop_wait (nonblocking=0)
>>at /usr/src/qemu-kvm-0.14.0/vl.c:1388
> 
> So we are calling a IOHandlerRecord::fd_write handler that is NULL.
> Looking at qemu_set_fd_handler2, this may happen if that function is
> called for an existing io-handler entry with non-NULL write handler,
> passing a NULL write and a non-NULL read handler. And all this without
> the global mutex held.
> 
> And there are actually calls in vnc_client_write_plain and
> vnc_client_write_locked (in contrast to vnc_write) that may generate
> this pattern. It's probably worth validating that the iothread lock is
> always held when qemu_set_fd_handler2 is invoked to confirm this race
> theory, adding something like
> 
> assert(pthread_mutex_trylock(&qemu_mutex) != 0);
> (that's for qemu-kvm only)

qemu_mutex doesn't exists (anymore). i can only find qemu_global_mutex and 
qemu_fair_mutex.

i try with qemu_global_mutex now. if thats not correct please tell.

peter

> 
> BTW, qemu with just --enable-vnc-thread, ie. without io-thread support,
> should always run into this race as it then definitely lacks a global mutex.
> 
> Jan
> 

Re: [Qemu-devel] [PATCH v3] Improve error handling in do_snapshot_blkdev()

[Jes Sorensen]

On 03/08/11 18:46, Anthony Liguori wrote:
> On 03/08/2011 10:44 AM, Jes Sorensen wrote:
>> On 03/08/11 14:42, Anthony Liguori wrote:
>> It kinda sorta covers it. The problem with that is that you then have to
>> do a string match of the return values to determine which of the cases
>> happened, which isn't very nice. But I guess we can do that for now.
> 
> Right, but this can be done in the HMP command such that the HMP command
> still prints out the warning message.
> 
> The key is to have well documented error semantics where the various
> cases can be distinguished because then we can ensure that we can not
> only print out a nice error message in HMP, but that a remote QMP client
> (like libvirt) can also generate a high quality error message.

Have a look at v4 then, I've changed it to report errors back according
to this.

Jes

[Qemu-devel] Re: segmentation fault in qemu-kvm-0.14.0

[Peter Lieven]

Am 09.03.2011 um 11:20 schrieb Jan Kiszka:

> On 2011-03-09 11:16, Peter Lieven wrote:
>> 
>> Am 09.03.2011 um 08:37 schrieb Jan Kiszka:
>> 
>>> On 2011-03-08 23:53, Peter Lieven wrote:
 Hi,
 
 during testing of qemu-kvm-0.14.0 i can reproduce the following segfault. 
 i have seen similar crash already in 0.13.0, but had no time to debug.
 my guess is that this segfault is related to the threaded vnc server which 
 was introduced in qemu 0.13.0. the bug is only triggerable if a vnc
 client is attached. it might also be connected to a resolution change in 
 the guest. i have a backtrace attached. the debugger is still running if 
 someone
 needs more output
 
>>> 
>>> ...
>>> 
 Thread 1 (Thread 0x77ff0700 (LWP 29038)):
 #0  0x in ?? ()
 No symbol table info available.
 #1  0x0041d669 in main_loop_wait (nonblocking=0)
   at /usr/src/qemu-kvm-0.14.0/vl.c:1388
>>> 
>>> So we are calling a IOHandlerRecord::fd_write handler that is NULL.
>>> Looking at qemu_set_fd_handler2, this may happen if that function is
>>> called for an existing io-handler entry with non-NULL write handler,
>>> passing a NULL write and a non-NULL read handler. And all this without
>>> the global mutex held.
>>> 
>>> And there are actually calls in vnc_client_write_plain and
>>> vnc_client_write_locked (in contrast to vnc_write) that may generate
>>> this pattern. It's probably worth validating that the iothread lock is
>>> always held when qemu_set_fd_handler2 is invoked to confirm this race
>>> theory, adding something like
>>> 
>>> assert(pthread_mutex_trylock(&qemu_mutex) != 0);
>>> (that's for qemu-kvm only)
>> 
>> qemu_mutex doesn't exists (anymore). i can only find qemu_global_mutex and 
>> qemu_fair_mutex.
> 
> In qemu-kvm, qemu-kvm.c contains that lock. In upstream, it's
> qemu_global_mutex, but you will have to enable --enable-io-thread and
> export it.

qemu-kvm doesn't compile with --enable-io-thread

  LINK  x86_64-softmmu/qemu-system-x86_64
kvm-all.o: In function `qemu_mutex_unlock_iothread':
/usr/src/qemu-kvm-0.14.0/qemu-kvm.c:1735: multiple definition of 
`qemu_mutex_unlock_iothread'
cpus.o:/usr/src/qemu-kvm-0.14.0/cpus.c:752: first defined here
kvm-all.o: In function `qemu_mutex_lock_iothread':
/usr/src/qemu-kvm-0.14.0/qemu-kvm.c:1742: multiple definition of 
`qemu_mutex_lock_iothread'
cpus.o:/usr/src/qemu-kvm-0.14.0/cpus.c:738: first defined here
../compatfd.o: In function `qemu_signalfd':
/usr/src/qemu-kvm-0.14.0/compatfd.c:105: multiple definition of `qemu_signalfd'
../compatfd.o:/usr/src/qemu-kvm-0.14.0/compatfd.c:105: first defined here
collect2: ld returned 1 exit status
make[1]: *** [qemu-system-x86_64] Error 1
make: *** [subdir-x86_64-softmmu] Error 2


> 
>> 
>> i try with qemu_global_mutex now. if thats not correct please tell.
> 
> I think we knokw know the call path, so you could also wait for Corentin
> to send a patch and test that one.

will do as soon as i get it ;-)

> 
> Jan
> 

Peter

[Qemu-devel] Re: [RFC][PATCH v7 03/16] Make qemu timers available for tools

[Jes Sorensen]

On 03/07/11 21:10, Michael Roth wrote:
> To be able to use qemu_mod_timer() and friends to register timeout
> events for virtagent's qemu-va tool, we need to do the following:
> 
> Move several blocks of code out of cpus.c that handle initialization
> of qemu's io_thread_fd and working with it via
> qemu_notify_event()/qemu_event_read()/etc, and make them accessible
> as backend functions to both the emulator code and qemu-tool.c via
> wrapper functions within cpus.c and qemu-tool.c, respectively. These
> have been added to qemu-ioh.c, where similar treatment was given to
> qemu_set_fd_handler() and friends.
> 
> Some of these wrapper functions lack declarations when being
> built into tools, so we add those via qemu-tool.h, which can be included
> by a tool to access them. With these changes we can drive timers in a
> tool linking it against qemu-timer.o and then implementing something
> similar to the main i/o loop in vl.c:
> 

[snip]

> diff --git a/qemu-ioh.c b/qemu-ioh.c
> index cc71470..5c3f94c 100644
> --- a/qemu-ioh.c
> +++ b/qemu-ioh.c
> @@ -113,3 +117,94 @@ void qemu_process_fd_handlers2(void *ioh_record_list, 
> const fd_set *rfds,
>  }
>  }
>  }
> +
> +#ifndef _WIN32
> +void iothread_event_increment(int *io_thread_fd)

Please move these functions into posix/w32 specific files so we don't
get anymore ugly #ifdefs. It would be good if we could use a wrapper
struct as well to hide the different data types so we don't need #ifdefs
in the calling code as well.

Cheers,
Jes

[Qemu-devel] Re: [RFC][PATCH v7 05/16] virtagent: common helpers and init routines

[Jes Sorensen]

On 03/07/11 21:10, Michael Roth wrote:
> +#define VA_PIDFILE "/var/run/qemu-va.pid"
> +#define VA_HDR_LEN_MAX 4096 /* http header limit */
> +#define VA_CONTENT_LEN_MAX 2*1024*1024 /* rpc/http send limit */
> +#define VA_CLIENT_JOBS_MAX 5 /* max client rpcs we can queue */
> +#define VA_SERVER_JOBS_MAX 5 /* max server rpcs we can queue */
> +#define VA_SERVER_TIMEOUT_MS 5 * 1000
> +#define VA_CLIENT_TIMEOUT_MS 5 * 1000
> +#define VA_SENTINEL 0xFF
> +#define VA_BAUDRATE B38400 /* for isa-serial channels */
> +

I've been after these before - please put the ones that make sense to
tune into a config file, and the same with the pidfile.

Cheers,
Jes

[Qemu-devel] [PATCH] vnc: threaded server depends on io-thread

[Corentin Chary]

The threaded VNC servers messed up with QEMU fd handlers without
any kind of locking, and that can cause some nasty race conditions.

The IO-Thread provides appropriate locking primitives to avoid that.
This patch makes CONFIG_VNC_THREAD depends on CONFIG_IO_THREAD,
and add lock and unlock calls around the two faulty calls.

Thanks to Jan Kiszka for helping me solve this issue.

Cc: Jan Kiszka 
Signed-off-by: Corentin Chary 
---
 configure   |9 +
 ui/vnc-jobs-async.c |4 
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/configure b/configure
index 5513d3e..c8c1ac1 100755
--- a/configure
+++ b/configure
@@ -2455,6 +2455,15 @@ if test \( "$cpu" = "i386" -o "$cpu" = "x86_64" \) -a \
   roms="optionrom"
 fi
 
+# VNC Thread depends on IO Thread
+if test "$vnc_thread" = "yes" -a "$io_thread" != "yes"; then
+  echo
+  echo "ERROR: VNC thread depends on IO thread which isn't enabled."
+  echo "Please use --enable-io-thread if you want to enable it."
+  echo
+  exit 1
+fi
+
 
 echo "Install prefix$prefix"
 echo "BIOS directory`eval echo $datadir`"
diff --git a/ui/vnc-jobs-async.c b/ui/vnc-jobs-async.c
index f596247..093c0d4 100644
--- a/ui/vnc-jobs-async.c
+++ b/ui/vnc-jobs-async.c
@@ -260,7 +260,9 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
 goto disconnected;
 }
 
+qemu_mutex_lock_iothread();
 vnc_write(job->vs, vs.output.buffer, vs.output.offset);
+qemu_mutex_unlock_iothread();
 
 disconnected:
 /* Copy persistent encoding data */
@@ -269,7 +271,9 @@ disconnected:
 vnc_unlock_output(job->vs);
 
 if (flush) {
+qemu_mutex_lock_iothread();
 vnc_flush(job->vs);
+qemu_mutex_unlock_iothread();
 }
 
 vnc_lock_queue(queue);
-- 
1.7.3.4

[Qemu-devel] Re: [RFC][PATCH v7 15/16] virtagent: qemu-va, system-level virtagent guest agent

[Jes Sorensen]

On 03/07/11 21:10, Michael Roth wrote:
> Signed-off-by: Michael Roth 
> ---
>  qemu-va.c |  247 
> +
>  1 files changed, 247 insertions(+), 0 deletions(-)
>  create mode 100644 qemu-va.c
> 
> diff --git a/qemu-va.c b/qemu-va.c
> new file mode 100644
> index 000..a9ff56f
> --- /dev/null
> +++ b/qemu-va.c
> @@ -0,0 +1,247 @@
[snip]
> +static void become_daemon(void)
> +{
> +pid_t pid, sid;
> +int pidfd;
> +char *pidstr;
> +
> +pid = fork();
> +if (pid < 0)
> +exit(EXIT_FAILURE);
> +if (pid > 0) {
> +exit(EXIT_SUCCESS);
> +}
> +
> +pidfd = open(VA_PIDFILE, O_CREAT|O_RDWR, S_IRUSR|S_IWUSR);
> +if (!pidfd || lockf(pidfd, F_TLOCK, 0))
> +errx(EXIT_FAILURE, "Cannot lock pid file");
> +
> +if (ftruncate(pidfd, 0) || lseek(pidfd, 0, SEEK_SET))
> +   errx(EXIT_FAILURE, "Cannot truncate pid file");
> +if (asprintf(&pidstr, "%d", getpid()) == -1)
> +errx(EXIT_FAILURE, "Cannot allocate memory");
> +if (write(pidfd, pidstr, strlen(pidstr)) != strlen(pidstr))
> +errx(EXIT_FAILURE, "Failed to write pid file");
> +free(pidstr);

Coding style - this needs to be fixed.

> +umask(0);
> +sid = setsid();
> +if (sid < 0)
> +goto fail;
> +if ((chdir("/")) < 0)
> +goto fail;

and again

Cheers,
Jes

[Qemu-devel] Re: [PATCH] vnc: threaded server depends on io-thread

[Peter Lieven]

Am 09.03.2011 um 11:41 schrieb Corentin Chary:

> The threaded VNC servers messed up with QEMU fd handlers without
> any kind of locking, and that can cause some nasty race conditions.
> 
> The IO-Thread provides appropriate locking primitives to avoid that.
> This patch makes CONFIG_VNC_THREAD depends on CONFIG_IO_THREAD,
> and add lock and unlock calls around the two faulty calls.

qemu-kvm currently doesn't compile with --enable-io-thread. is there an easy fix
for this?

thanks,
peter

> 
> Thanks to Jan Kiszka for helping me solve this issue.
> 
> Cc: Jan Kiszka 
> Signed-off-by: Corentin Chary 
> ---
> configure   |9 +
> ui/vnc-jobs-async.c |4 
> 2 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/configure b/configure
> index 5513d3e..c8c1ac1 100755
> --- a/configure
> +++ b/configure
> @@ -2455,6 +2455,15 @@ if test \( "$cpu" = "i386" -o "$cpu" = "x86_64" \) -a \
>   roms="optionrom"
> fi
> 
> +# VNC Thread depends on IO Thread
> +if test "$vnc_thread" = "yes" -a "$io_thread" != "yes"; then
> +  echo
> +  echo "ERROR: VNC thread depends on IO thread which isn't enabled."
> +  echo "Please use --enable-io-thread if you want to enable it."
> +  echo
> +  exit 1
> +fi
> +
> 
> echo "Install prefix$prefix"
> echo "BIOS directory`eval echo $datadir`"
> diff --git a/ui/vnc-jobs-async.c b/ui/vnc-jobs-async.c
> index f596247..093c0d4 100644
> --- a/ui/vnc-jobs-async.c
> +++ b/ui/vnc-jobs-async.c
> @@ -260,7 +260,9 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
> goto disconnected;
> }
> 
> +qemu_mutex_lock_iothread();
> vnc_write(job->vs, vs.output.buffer, vs.output.offset);
> +qemu_mutex_unlock_iothread();
> 
> disconnected:
> /* Copy persistent encoding data */
> @@ -269,7 +271,9 @@ disconnected:
> vnc_unlock_output(job->vs);
> 
> if (flush) {
> +qemu_mutex_lock_iothread();
> vnc_flush(job->vs);
> +qemu_mutex_unlock_iothread();
> }
> 
> vnc_lock_queue(queue);
> -- 
> 1.7.3.4
> 

[Qemu-devel] Re: [PATCH] vnc: threaded server depends on io-thread

[Corentin Chary]

>> The threaded VNC servers messed up with QEMU fd handlers without
>> any kind of locking, and that can cause some nasty race conditions.
>>
>> The IO-Thread provides appropriate locking primitives to avoid that.
>> This patch makes CONFIG_VNC_THREAD depends on CONFIG_IO_THREAD,
>> and add lock and unlock calls around the two faulty calls.
>
> qemu-kvm currently doesn't compile with --enable-io-thread. is there an easy 
> fix
> for this?

If IO Thread is not available, I'm afraid that --disable-vnc-thread is
the only fix.
Or, you can try to define some global mutex acting like iothread
locks, but that doesn't sounds like an easy fix.

-- 
Corentin Chary
http://xf.iksaif.net

[Qemu-devel] Re: [PATCH] vnc: threaded server depends on io-thread

[Stefan Hajnoczi]

On Wed, Mar 9, 2011 at 10:57 AM, Corentin Chary
 wrote:
>>> The threaded VNC servers messed up with QEMU fd handlers without
>>> any kind of locking, and that can cause some nasty race conditions.
>>>
>>> The IO-Thread provides appropriate locking primitives to avoid that.
>>> This patch makes CONFIG_VNC_THREAD depends on CONFIG_IO_THREAD,
>>> and add lock and unlock calls around the two faulty calls.
>>
>> qemu-kvm currently doesn't compile with --enable-io-thread. is there an easy 
>> fix
>> for this?
>
> If IO Thread is not available, I'm afraid that --disable-vnc-thread is
> the only fix.
> Or, you can try to define some global mutex acting like iothread
> locks, but that doesn't sounds like an easy fix.

Jan or Marcelo can help here but qemu-kvm has an iothread equivalent
built in by default.  It should be possible to use that.

Stefan

[Qemu-devel] Re: segmentation fault in qemu-kvm-0.14.0

[Paolo Bonzini]

On 03/09/2011 08:37 AM, Jan Kiszka wrote:

It's probably worth validating that the iothread lock is
always held when qemu_set_fd_handler2 is invoked to confirm this race
theory, adding something like

assert(pthread_mutex_trylock(&qemu_mutex) != 0);
(that's for qemu-kvm only)


Alternatively, iohandlers could be a(nother) good place to start 
introducing fine-grained locks or rwlocks.


Paolo

[Qemu-devel] Re: [PATCH] vnc: threaded server depends on io-thread

[Peter Lieven]

Am 09.03.2011 um 12:25 schrieb Jan Kiszka:

> On 2011-03-09 12:05, Stefan Hajnoczi wrote:
>> On Wed, Mar 9, 2011 at 10:57 AM, Corentin Chary
>>  wrote:
> The threaded VNC servers messed up with QEMU fd handlers without
> any kind of locking, and that can cause some nasty race conditions.
> 
> The IO-Thread provides appropriate locking primitives to avoid that.
> This patch makes CONFIG_VNC_THREAD depends on CONFIG_IO_THREAD,
> and add lock and unlock calls around the two faulty calls.
 
 qemu-kvm currently doesn't compile with --enable-io-thread. is there an 
 easy fix
 for this?
>>> 
>>> If IO Thread is not available, I'm afraid that --disable-vnc-thread is
>>> the only fix.
>>> Or, you can try to define some global mutex acting like iothread
>>> locks, but that doesn't sounds like an easy fix.
>> 
>> Jan or Marcelo can help here but qemu-kvm has an iothread equivalent
>> built in by default.  It should be possible to use that.
> 
> qemu_mutex_lock/unlock_iothread is properly provided in qemu-kvm even
> without --enable-io-thread. So that tree could temporarily disable the
> new configure check until we got rid of the special qemu-kvm bits.
> Corentin's patch is against upstream, that adjustment need to be made
> once the commit is merged into qemu-kvm.

do i understand you right, that i should be able to use vnc-thread together 
with qemu-kvm
just now if I add Corentin's patch without the io-thread dependency?

if yes, i will do and try if I can force a crash again.

br,
peter

> 
> Jan
> 

[Qemu-devel] Re: [PATCH] vnc: threaded server depends on io-thread

[Jan Kiszka]

On 2011-03-09 12:05, Stefan Hajnoczi wrote:
> On Wed, Mar 9, 2011 at 10:57 AM, Corentin Chary
>  wrote:
 The threaded VNC servers messed up with QEMU fd handlers without
 any kind of locking, and that can cause some nasty race conditions.

 The IO-Thread provides appropriate locking primitives to avoid that.
 This patch makes CONFIG_VNC_THREAD depends on CONFIG_IO_THREAD,
 and add lock and unlock calls around the two faulty calls.
>>>
>>> qemu-kvm currently doesn't compile with --enable-io-thread. is there an 
>>> easy fix
>>> for this?
>>
>> If IO Thread is not available, I'm afraid that --disable-vnc-thread is
>> the only fix.
>> Or, you can try to define some global mutex acting like iothread
>> locks, but that doesn't sounds like an easy fix.
> 
> Jan or Marcelo can help here but qemu-kvm has an iothread equivalent
> built in by default.  It should be possible to use that.

qemu_mutex_lock/unlock_iothread is properly provided in qemu-kvm even
without --enable-io-thread. So that tree could temporarily disable the
new configure check until we got rid of the special qemu-kvm bits.
Corentin's patch is against upstream, that adjustment need to be made
once the commit is merged into qemu-kvm.

Jan



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] Re: [PATCH] vnc: threaded server depends on io-thread

[Jan Kiszka]

On 2011-03-09 12:32, Peter Lieven wrote:
> 
> Am 09.03.2011 um 12:25 schrieb Jan Kiszka:
> 
>> On 2011-03-09 12:05, Stefan Hajnoczi wrote:
>>> On Wed, Mar 9, 2011 at 10:57 AM, Corentin Chary
>>>  wrote:
>> The threaded VNC servers messed up with QEMU fd handlers without
>> any kind of locking, and that can cause some nasty race conditions.
>>
>> The IO-Thread provides appropriate locking primitives to avoid that.
>> This patch makes CONFIG_VNC_THREAD depends on CONFIG_IO_THREAD,
>> and add lock and unlock calls around the two faulty calls.
>
> qemu-kvm currently doesn't compile with --enable-io-thread. is there an 
> easy fix
> for this?

 If IO Thread is not available, I'm afraid that --disable-vnc-thread is
 the only fix.
 Or, you can try to define some global mutex acting like iothread
 locks, but that doesn't sounds like an easy fix.
>>>
>>> Jan or Marcelo can help here but qemu-kvm has an iothread equivalent
>>> built in by default.  It should be possible to use that.
>>
>> qemu_mutex_lock/unlock_iothread is properly provided in qemu-kvm even
>> without --enable-io-thread. So that tree could temporarily disable the
>> new configure check until we got rid of the special qemu-kvm bits.
>> Corentin's patch is against upstream, that adjustment need to be made
>> once the commit is merged into qemu-kvm.
> 
> do i understand you right, that i should be able to use vnc-thread together 
> with qemu-kvm
> just now if I add Corentin's patch without the io-thread dependency?

Yep.

> 
> if yes, i will do and try if I can force a crash again.
> 

TIA,
Jan



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] how to convert guest virtual address to host virtual address in QEMU?

[Stefan Hajnoczi]

On Wed, Mar 9, 2011 at 9:34 AM, Gunasekaran Dharman  wrote:
> Regarding calculating host virtual address from guest virtual address, I
> think, QEMU must be doing this somehow.
> I would like to know how QEMU is handling the guest virtual address?
> It will be very much helpful if you can throw some light on this.

Sorry but I've tried to explain twice that this approach is not the
way to go.  The hardware does not necessarily know how to translate
any virtual address by itself and QEMU doesn't either, guest
assistance may be required.  It's not as simple as handing a (kernel
or userspace?) virtual address to a custom device and having it
perform a memcpy.

If you really want to do a custom hack try adding a custom memcpy
instruction and look at how x86 targets rep; mov or other instances of
memory copy on other architectures are done.  You'll need to write a
custom memcpy implementation in the guest to make use of this
function.

Stefan

[Qemu-devel] Re: [PATCH] vnc: threaded server depends on io-thread

[Jan Kiszka]

On 2011-03-09 11:41, Corentin Chary wrote:
> The threaded VNC servers messed up with QEMU fd handlers without
> any kind of locking, and that can cause some nasty race conditions.
> 
> The IO-Thread provides appropriate locking primitives to avoid that.
> This patch makes CONFIG_VNC_THREAD depends on CONFIG_IO_THREAD,
> and add lock and unlock calls around the two faulty calls.
> 
> Thanks to Jan Kiszka for helping me solve this issue.
> 
> Cc: Jan Kiszka 
> Signed-off-by: Corentin Chary 
> ---
>  configure   |9 +
>  ui/vnc-jobs-async.c |4 
>  2 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/configure b/configure
> index 5513d3e..c8c1ac1 100755
> --- a/configure
> +++ b/configure
> @@ -2455,6 +2455,15 @@ if test \( "$cpu" = "i386" -o "$cpu" = "x86_64" \) -a \
>roms="optionrom"
>  fi
>  
> +# VNC Thread depends on IO Thread
> +if test "$vnc_thread" = "yes" -a "$io_thread" != "yes"; then
> +  echo
> +  echo "ERROR: VNC thread depends on IO thread which isn't enabled."
> +  echo "Please use --enable-io-thread if you want to enable it."
> +  echo
> +  exit 1
> +fi
> +
>  
>  echo "Install prefix$prefix"
>  echo "BIOS directory`eval echo $datadir`"
> diff --git a/ui/vnc-jobs-async.c b/ui/vnc-jobs-async.c
> index f596247..093c0d4 100644
> --- a/ui/vnc-jobs-async.c
> +++ b/ui/vnc-jobs-async.c
> @@ -260,7 +260,9 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
>  goto disconnected;
>  }
>  
> +qemu_mutex_lock_iothread();

Doesn't this comes with a risk of an ABBA deadlock between output_mutex
and the global lock? Here you take the global lock while holding
output_mutex, but I bet there is also the other way around when vnc
services are called from the main thread or a vcpu.

>  vnc_write(job->vs, vs.output.buffer, vs.output.offset);
> +qemu_mutex_unlock_iothread();
>  
>  disconnected:
>  /* Copy persistent encoding data */
> @@ -269,7 +271,9 @@ disconnected:
>  vnc_unlock_output(job->vs);
>  
>  if (flush) {
> +qemu_mutex_lock_iothread();
>  vnc_flush(job->vs);
> +qemu_mutex_unlock_iothread();
>  }
>  
>  vnc_lock_queue(queue);

The second hunk looks safe.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH 20/58] vmstate: port pxa2xx_pic

[Juan Quintela]

andrzej zaborowski  wrote:
> On 24 February 2011 18:57, Juan Quintela  wrote:
>> Signed-off-by: Juan Quintela 
>> ---
>>  hw/pxa2xx_pic.c |   52 
>>  1 files changed, 20 insertions(+), 32 deletions(-)
>
> Hi Juan,

Hi andrzej

> I pushed an earlier patch from Dmitry Eremin-Solenikov that converted
> this device, so this patch can be skipped.  Unfortunately some of your
> other patches may need rebasing now.

Remove from my tree.  Please take a look at the other pxa2xx* patches if you are
planning to port the other devices.

Later, Juan.

[Qemu-devel] Re: segmentation fault in qemu-kvm-0.14.0

[Jan Kiszka]

On 2011-03-09 12:20, Paolo Bonzini wrote:
> On 03/09/2011 08:37 AM, Jan Kiszka wrote:
>> It's probably worth validating that the iothread lock is
>> always held when qemu_set_fd_handler2 is invoked to confirm this race
>> theory, adding something like
>>
>> assert(pthread_mutex_trylock(&qemu_mutex) != 0);
>> (that's for qemu-kvm only)
> 
> Alternatively, iohandlers could be a(nother) good place to start
> introducing fine-grained locks or rwlocks.

Yeah, could be a good idea. It's a fairly confined area here that needs
protection.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] vhost: force vhost off for non-MSI guests

[rukhsana ansari]

Hi,

On Thu, Jan 20, 2011 at 9:05 PM, Michael S. Tsirkin  wrote:

> When MSI is off, each interrupt needs to be bounced through the io
> thread when it's set/cleared, so vhost-net causes more context switches and
> higher CPU utilization than userspace virtio which handles networking in
> the same thread.
>
> We'll need to fix this by adding level irq support in kvm irqfd,
> for now disable vhost-net in these configurations.
>
>
> This maybe a novice question - Would appreciate it if you can you provide a
pointer to documentation or relevant code that explains what is the
limitation in supporting level irq support in kvm irqfd.


Thanks
-Rukhsana

[Qemu-devel] [PATCH v2] Add qcow2 documentation

[Kevin Wolf]

This adds a description of the qcow2 file format to the docs/ directory.
Besides documenting what's there, which is never wrong, the document should
provide a good basis for the discussion of format extensions (called "qcow3"
in previous discussions)

Signed-off-by: Kevin Wolf 
---
 docs/specs/qcow2.txt |  233 ++
 1 files changed, 233 insertions(+), 0 deletions(-)
 create mode 100644 docs/specs/qcow2.txt

v2:
- Added limits to cluster_bits

diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
new file mode 100644
index 000..b3e9b4e
--- /dev/null
+++ b/docs/specs/qcow2.txt
@@ -0,0 +1,233 @@
+== Clusters ==
+
+A qcow2 image file is organized in units of constant size, which are called
+(host) clusters. A cluster is the unit in which all allocations are done,
+both for actual guest data and for image metadata.
+
+Likewise, the virtual disk as seen by the guest is divided into (guest)
+clusters of the same size.
+
+
+== Header ==
+
+The first cluster of a qcow2 image contains the file header:
+
+Byte  0 -  3:   magic
+QCOW magic string ("QFI\xfb")
+
+  4 -  7:   version
+Version number (only valid value is 2)
+
+  8 - 15:   backing_file_offset
+Offset into the image file at which the backing file name
+is stored (NB: The string is not null terminated). 0 if the
+image doesn't have a backing file.
+
+ 16 - 19:   backing_file_size
+Length of the backing file name in bytes. Must not be
+longer than 1023 bytes. Undefined if the image doesn't have
+a backing file.
+
+ 20 - 23:   cluster_bits
+Number of bits that are used for addressing an offset
+within a cluster (1 << cluster_bits is the cluster size).
+Must not be less than 9 (i.e. 512 byte clusters).
+
+Note: qemu as of today has an implementation limit of 2 MB
+as the maximum cluster size and won't be able to open 
images
+with larger cluster sizes.
+
+ 24 - 31:   size
+Virtual disk size in bytes
+
+ 32 - 35:   crypt_method
+0 for no encryption
+1 for AES encryption
+
+ 36 - 39:   l1_size
+Number of entries in the active L1 table
+
+ 40 - 47:   l1_table_offset
+Offset into the image file at which the active L1 table
+starts. Must be aligned to a cluster boundary.
+
+ 48 - 55:   refcount_table_offset
+Offset into the image file at which the refcount table
+starts. Must be aligned to a cluster boundary.
+
+ 56 - 59:   refcount_table_clusters
+Number of clusters that the refcount table occupies
+
+ 60 - 63:   nb_snapshots
+Number of snapshots contained in the image
+
+ 64 - 71:   snapshots_offset
+Offset into the image file at which the snapshot table
+starts. Must be aligned to a cluster boundary.
+
+All numbers in qcow2 are stored in Big Endian byte order.
+
+
+== Host cluster management ==
+
+qcow2 manages the allocation of host clusters by maintaining a reference count
+for each host cluster. A refcount of 0 means that the cluster is free, 1 means
+that it is used, and >= 2 means that it is used and any write access must
+perform a COW (copy on write) operation.
+
+The refcounts are managed in a two-level table. The first level is called
+refcount table and has a variable size (which is stored in the header). The
+refcount table can cover multiple clusters, however it needs to be contiguous
+in the image file.
+
+It contains pointers to the second level structures which are called refcount
+blocks and are exactly one cluster in size.
+
+Given a offset into the image file, the refcount of its cluster can be obtained
+as follows:
+
+refcount_block_entries = (cluster_size / sizeof(uint16_t))
+
+refcount_block_index = (offset / cluster_size) % refcount_table_entries
+refcount_table_index = (offset / cluster_size) / refcount_table_entries
+
+refcount_block = load_cluster(refcount_table[refcount_table_index]);
+return refcount_block[refcount_block_index];
+
+Refcount table entry:
+
+Bit  0 -  8:Reserved (set to 0)
+
+ 9 - 63:Bits 9-63 of the offset into the image file at which the
+refcount block starts. Must be aligned to a cluster
+boundary.
+
+If this is 0, the corresponding refcount block has not yet
+been allocated. All refcounts managed by this refcount 
block
+are 0.
+
+Refcount block entry:
+
+Bit  0 - 15:Reference count of the cluster
+
+
+== Cluste

[Qemu-devel] Re: [PATCH] vnc: threaded server depends on io-thread

[Peter Lieven]

Am 09.03.2011 um 12:42 schrieb Jan Kiszka:

> On 2011-03-09 11:41, Corentin Chary wrote:
>> The threaded VNC servers messed up with QEMU fd handlers without
>> any kind of locking, and that can cause some nasty race conditions.
>> 
>> The IO-Thread provides appropriate locking primitives to avoid that.
>> This patch makes CONFIG_VNC_THREAD depends on CONFIG_IO_THREAD,
>> and add lock and unlock calls around the two faulty calls.
>> 
>> Thanks to Jan Kiszka for helping me solve this issue.
>> 
>> Cc: Jan Kiszka 
>> Signed-off-by: Corentin Chary 
>> ---
>> configure   |9 +
>> ui/vnc-jobs-async.c |4 
>> 2 files changed, 13 insertions(+), 0 deletions(-)
>> 
>> diff --git a/configure b/configure
>> index 5513d3e..c8c1ac1 100755
>> --- a/configure
>> +++ b/configure
>> @@ -2455,6 +2455,15 @@ if test \( "$cpu" = "i386" -o "$cpu" = "x86_64" \) -a 
>> \
>>   roms="optionrom"
>> fi
>> 
>> +# VNC Thread depends on IO Thread
>> +if test "$vnc_thread" = "yes" -a "$io_thread" != "yes"; then
>> +  echo
>> +  echo "ERROR: VNC thread depends on IO thread which isn't enabled."
>> +  echo "Please use --enable-io-thread if you want to enable it."
>> +  echo
>> +  exit 1
>> +fi
>> +
>> 
>> echo "Install prefix$prefix"
>> echo "BIOS directory`eval echo $datadir`"
>> diff --git a/ui/vnc-jobs-async.c b/ui/vnc-jobs-async.c
>> index f596247..093c0d4 100644
>> --- a/ui/vnc-jobs-async.c
>> +++ b/ui/vnc-jobs-async.c
>> @@ -260,7 +260,9 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
>> goto disconnected;
>> }
>> 
>> +qemu_mutex_lock_iothread();
> 
> Doesn't this comes with a risk of an ABBA deadlock between output_mutex
> and the global lock? Here you take the global lock while holding
> output_mutex, but I bet there is also the other way around when vnc
> services are called from the main thread or a vcpu.

so after all i should stay with disabled vnc-thread in qemu-kvm until there is 
a solution?

br,
peter


> 
>> vnc_write(job->vs, vs.output.buffer, vs.output.offset);
>> +qemu_mutex_unlock_iothread();
>> 
>> disconnected:
>> /* Copy persistent encoding data */
>> @@ -269,7 +271,9 @@ disconnected:
>> vnc_unlock_output(job->vs);
>> 
>> if (flush) {
>> +qemu_mutex_lock_iothread();
>> vnc_flush(job->vs);
>> +qemu_mutex_unlock_iothread();
>> }
>> 
>> vnc_lock_queue(queue);
> 
> The second hunk looks safe.
> 
> Jan
> 
> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [RFC][PATCH v7 03/16] Make qemu timers available for tools

[Michael Roth]

On 03/09/2011 04:33 AM, Jes Sorensen wrote:

On 03/07/11 21:10, Michael Roth wrote:

To be able to use qemu_mod_timer() and friends to register timeout
events for virtagent's qemu-va tool, we need to do the following:

Move several blocks of code out of cpus.c that handle initialization
of qemu's io_thread_fd and working with it via
qemu_notify_event()/qemu_event_read()/etc, and make them accessible
as backend functions to both the emulator code and qemu-tool.c via
wrapper functions within cpus.c and qemu-tool.c, respectively. These
have been added to qemu-ioh.c, where similar treatment was given to
qemu_set_fd_handler() and friends.

Some of these wrapper functions lack declarations when being
built into tools, so we add those via qemu-tool.h, which can be included
by a tool to access them. With these changes we can drive timers in a
tool linking it against qemu-timer.o and then implementing something
similar to the main i/o loop in vl.c:



[snip]


diff --git a/qemu-ioh.c b/qemu-ioh.c
index cc71470..5c3f94c 100644
--- a/qemu-ioh.c
+++ b/qemu-ioh.c
@@ -113,3 +117,94 @@ void qemu_process_fd_handlers2(void *ioh_record_list, 
const fd_set *rfds,
  }
  }
  }
+
+#ifndef _WIN32
+void iothread_event_increment(int *io_thread_fd)


Please move these functions into posix/w32 specific files so we don't
get anymore ugly #ifdefs. It would be good if we could use a wrapper
struct as well to hide the different data types so we don't need #ifdefs
in the calling code as well.


Yup, meant to add this to the TODO. I may end up sending these general 
tools changes out in a separate patchset since they seem to be in 
conflict with quite of few patches floating around the list. Either way 
I'll make sure to get these cleaned up and tested a bit a more.




Cheers,
Jes

Re: [Qemu-devel] Re: [RFC][PATCH v7 03/16] Make qemu timers available for tools

[Jes Sorensen]

On 03/09/11 14:04, Michael Roth wrote:
> On 03/09/2011 04:33 AM, Jes Sorensen wrote:
>>> diff --git a/qemu-ioh.c b/qemu-ioh.c
>>> index cc71470..5c3f94c 100644
>>> --- a/qemu-ioh.c
>>> +++ b/qemu-ioh.c
>>> @@ -113,3 +117,94 @@ void qemu_process_fd_handlers2(void
>>> *ioh_record_list, const fd_set *rfds,
>>>   }
>>>   }
>>>   }
>>> +
>>> +#ifndef _WIN32
>>> +void iothread_event_increment(int *io_thread_fd)
>>
>> Please move these functions into posix/w32 specific files so we don't
>> get anymore ugly #ifdefs. It would be good if we could use a wrapper
>> struct as well to hide the different data types so we don't need #ifdefs
>> in the calling code as well.
> 
> Yup, meant to add this to the TODO. I may end up sending these general
> tools changes out in a separate patchset since they seem to be in
> conflict with quite of few patches floating around the list. Either way
> I'll make sure to get these cleaned up and tested a bit a more.

Sounds great! Since they are not directly part of virtagent you should
be able to push them in soon too.

Cheers,
Jes

Re: [Qemu-devel] [PATCH v2] Add qcow2 documentation

[Stefan Hajnoczi]

On Wed, Mar 9, 2011 at 12:30 PM, Kevin Wolf  wrote:
> This adds a description of the qcow2 file format to the docs/ directory.
> Besides documenting what's there, which is never wrong, the document should
> provide a good basis for the discussion of format extensions (called "qcow3"
> in previous discussions)
>
> Signed-off-by: Kevin Wolf 
> ---
>  docs/specs/qcow2.txt |  233 
> ++
>  1 files changed, 233 insertions(+), 0 deletions(-)
>  create mode 100644 docs/specs/qcow2.txt

QCOW2_EXT_MAGIC_BACKING_FORMAT is not documented.  Did you decide that
is a QEMU internal extension and you don't want it documented?

Stefan

Re: [Qemu-devel] [PATCH V2] qemu, qmp: add keydown and keyup command for qmp

[Anthony Liguori]

On 03/09/2011 02:24 AM, Lai Jiangshan wrote:

sendkey is a very good command for human using it in their monitor,
but it is not a good idea to port it to qmp, because qmp is a machine
protocol. So we introduce keydown and keyup command for qmp, they
simulate the events that keyboard send to the system.

Example, simulates ctrl+alt+f1:
{ "execute": "keydown", "arguments": { "keycode": 29 } }  #press down ctrl
{ "execute": "keydown", "arguments": { "keycode": 56 } }  #press down alt
{ "execute": "keydown", "arguments": { "keycode": 59 } }  #press down f1
{ "execute": "keyup", "arguments": { "keycode": 59 } }#release f1
{ "execute": "keyup", "arguments": { "keycode": 56 } }#release alt
{ "execute": "keyup", "arguments": { "keycode": 29 } }#release ctrl

Signed-off-by: Lai Jiangshan




---
diff --git a/monitor.c b/monitor.c
index 22ae3bb..725df83 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1810,16 +1810,25 @@ static uint8_t keycodes[MAX_KEYCODES];
  static int nb_pending_keycodes;
  static QEMUTimer *key_timer;

-static void release_keys(void *opaque)
+static void keydown(uint8_t keycode)
  {
-int keycode;
+if (keycode&  0x80)
+kbd_put_keycode(0xe0);
+kbd_put_keycode(keycode&  0x7f);
+}

+static void keyup(uint8_t keycode)
+{
+if (keycode&  0x80)
+kbd_put_keycode(0xe0);
+kbd_put_keycode(keycode | 0x80);
+}
+
+static void release_keys(void *opaque)
+{
  while (nb_pending_keycodes>  0) {
  nb_pending_keycodes--;
-keycode = keycodes[nb_pending_keycodes];
-if (keycode&  0x80)
-kbd_put_keycode(0xe0);
-kbd_put_keycode(keycode | 0x80);
+keyup(keycodes[nb_pending_keycodes]);
  }
  }

@@ -1866,17 +1875,41 @@ static void do_sendkey(Monitor *mon, const QDict *qdict)
  }
  nb_pending_keycodes = i;
  /* key down events */
-for (i = 0; i<  nb_pending_keycodes; i++) {
-keycode = keycodes[i];
-if (keycode&  0x80)
-kbd_put_keycode(0xe0);
-kbd_put_keycode(keycode&  0x7f);
-}
+for (i = 0; i<  nb_pending_keycodes; i++)
+keydown(keycodes[i]);
  /* delayed key up events */
  qemu_mod_timer(key_timer, qemu_get_clock(vm_clock) +
 muldiv64(get_ticks_per_sec(), hold_time, 1000));
  }

+static int qmp_keyaction(const QDict *qdict, int down)
+{
+int keycode = qdict_get_int(qdict, "keycode");
+
+if (keycode<  0 || keycode>= 256) {
+qerror_report(QERR_INVALID_PARAMETER_VALUE, "keycode",
+  "a valid keycode");
+return -1;
+}
+
+if (down)
+keydown(keycode);
+else
+keyup(keycode);
+
+return 0;
+}
+
+static int qmp_keydown(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+return qmp_keyaction(qdict, 1);
+}
+
+static int qmp_keyup(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+return qmp_keyaction(qdict, 0);
+}
+
  static int mouse_button_state;

  static void do_mouse_move(Monitor *mon, const QDict *qdict)
diff --git a/qmp-commands.hx b/qmp-commands.hx
index df40a3d..4c449bb 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -338,6 +338,58 @@ Example:
  EQMP

  {
+.name   = "keydown",
+.args_type  = "keycode:i",
+.params = "keycode",
+.help   = "press down a key",
+.user_print = monitor_user_noop,
+.mhandler.cmd_new = qmp_keydown,
+},
+
+SQMP
+keydown
+---
+
+Press down a key.
+
+Arguments:
+
+- "keycode": the code of the key to press down (json-int)
+
+Example:
+
+->  { "execute": "keydown", "arguments": { "keycode": 16 } }
+<- { "return": {} }
+
+EQMP
+
+{
+.name   = "keyup",
+.args_type  = "keycode:i",
+.params = "keycode",
+.help   = "release a key",
+.user_print = monitor_user_noop,
+.mhandler.cmd_new = qmp_keyup,
+},
+
+SQMP
+keyup
+---
+
+Release a key.
+
+Arguments:
+
+- "keycode": the code of the key to release (json-int)


What is the value of "keycode" and what happens if you pass an invalid 
keycode?


This interface uses QEMU-style compressed XT scan codes.  I'd suggest 
making the interface take two optional arguments one being the an 
integer keycode and then another being a symbolic name where the 
symbolic name used the current keymap to translate the keycode.


For the XT scan code, I wouldn't use the one-byte encoding.  I'd use a 
32-bit encoding.


Regards,

Anthony Liguori


+Example:
+
+->  { "execute": "keyup", "arguments": { "keycode": 16 } }
+<- { "return": {} }
+
+EQMP
+
+{
  .name   = "cpu",
  .args_type  = "index:i",
  .params = "index",

Re: [Qemu-devel] [PATCH 00/22] QAPI Round 1

[Anthony Liguori]

On 03/09/2011 02:51 AM, Avi Kivity wrote:

On 03/08/2011 09:19 PM, Anthony Liguori wrote:
Both the guest and the management agent (and both can listen for 
events).  I don't see why guest->qemu RPC is problematic for 
migration - at least when qemu terminates it.



If it's terminated in QEMU, it's fine, but then it's not QMP 
anymore.  Let me think about whether there's a way to achieve this 
without a guest->qemu RPC.


Why not?

{ execute: 'write-keystore' arguments: { 'key': 'foo', 'value': 'bar' } }


This is coming from the guest?  QMP doesn't do bidirectional RPC today.  
It could, but that is a fundamental change in the protocol.


Regards,

Anthony Liguori


etc.

Re: [Qemu-devel] KVM call minutes for Mar 8

[Anthony Liguori]

On 03/09/2011 03:25 AM, Kevin Wolf wrote:

Am 08.03.2011 16:50, schrieb Chris Wright:

QAPI merge plans
- should be 100% back compat
- qmp moved over
- hmp moved over
- 1st pass, core infrastructure (includes test framework)
- 2nd pass, command conversion
- 3rd pass, more controversial bits
- adds dependencies: glib and python
- some testing based on kvm-unit-test micro-os instance (e.g. added a balloon
   and run commands against it to test)
   - add more functionality here? (kvm autotest is slow, above is quick)
 - will hit some point where full functionality is needed
   - have a mini linux to do this (lags where driver updates are part of test)

Depending on what we want to include in such tests, we might want to
have tests with a badly behaving kernel, which Linux hopefully isn't.


Yeah, that's a big advantage of kvm-unit-test is that you can't really 
hack the upstream version of virtio-balloon to do evil things.


Regards,

Anthony Liguori


Kevin

Re: [Qemu-devel] [PATCH 00/22] QAPI Round 1

[Avi Kivity]

On 03/09/2011 03:12 PM, Anthony Liguori wrote:

On 03/09/2011 02:51 AM, Avi Kivity wrote:

On 03/08/2011 09:19 PM, Anthony Liguori wrote:
Both the guest and the management agent (and both can listen for 
events).  I don't see why guest->qemu RPC is problematic for 
migration - at least when qemu terminates it.



If it's terminated in QEMU, it's fine, but then it's not QMP 
anymore.  Let me think about whether there's a way to achieve this 
without a guest->qemu RPC.


Why not?

{ execute: 'write-keystore' arguments: { 'key': 'foo', 'value': 'bar' 
} }


This is coming from the guest? 


Yes.

QMP doesn't do bidirectional RPC today.  It could, but that is a 
fundamental change in the protocol.




Could use a separate channel for talking to qemu.

--
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH v4] Improve error handling in do_snapshot_blkdev()

[Anthony Liguori]

On 03/09/2011 04:20 AM, jes.soren...@redhat.com wrote:

From: Jes Sorensen

In case we cannot open the newly created snapshot image, try to fall
back to the original image file and continue running on that, which
should prevent the guest from aborting.

This is a corner case which can happen if the admin by mistake
specifies the snapshot file on a virtual file system which does not
support O_DIRECT. bdrv_create() does not use O_DIRECT, but the
following open in bdrv_open() does and will then fail.

Signed-off-by: Jes Sorensen
---
  blockdev.c |   23 +--
  1 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 0690cc8..ecf2252 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -574,9 +574,10 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, 
QObject **ret_data)
  const char *filename = qdict_get_try_str(qdict, "snapshot_file");
  const char *format = qdict_get_try_str(qdict, "format");
  BlockDriverState *bs;
-BlockDriver *drv, *proto_drv;
+BlockDriver *drv, *old_drv, *proto_drv;
  int ret = 0;
  int flags;
+char old_filename[1024];

  if (!filename) {
  qerror_report(QERR_MISSING_PARAMETER, "snapshot_file");
@@ -591,6 +592,11 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, 
QObject **ret_data)
  goto out;
  }

+pstrcpy(old_filename, sizeof(old_filename), bs->filename);
+
+old_drv = bs->drv;
+flags = bs->open_flags;
+
  if (!format) {
  format = "qcow2";
  }
@@ -610,7 +616,7 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, 
QObject **ret_data)
  }

  ret = bdrv_img_create(filename, format, bs->filename,
-  bs->drv->format_name, NULL, -1, bs->open_flags);
+  bs->drv->format_name, NULL, -1, flags);
  if (ret) {
  goto out;
  }
@@ -618,15 +624,20 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, 
QObject **ret_data)
  qemu_aio_flush();
  bdrv_flush(bs);

-flags = bs->open_flags;
  bdrv_close(bs);
  ret = bdrv_open(bs, filename, flags, drv);
  /*
- * If reopening the image file we just created fails, we really
- * are in trouble :(
+ * If reopening the image file we just created fails, fall back
+ * and try to re-open the original image. If that fails too, we
+ * are in serious trouble.
   */
  if (ret != 0) {
-abort();
+ret = bdrv_open(bs, old_filename, flags, old_drv);
+if (ret != 0) {
+qerror_report(QERR_OPEN_FILE_FAILED, old_filename);
+} else {
+qerror_report(QERR_OPEN_FILE_FAILED, filename);
+}
  }


Looks good.

Regards,

Anthony Liguori


  out:
  if (ret) {

Re: [Qemu-devel] Re: [RFC][PATCH v7 05/16] virtagent: common helpers and init routines

[Michael Roth]

On 03/09/2011 04:38 AM, Jes Sorensen wrote:

On 03/07/11 21:10, Michael Roth wrote:

+#define VA_PIDFILE "/var/run/qemu-va.pid"
+#define VA_HDR_LEN_MAX 4096 /* http header limit */
+#define VA_CONTENT_LEN_MAX 2*1024*1024 /* rpc/http send limit */
+#define VA_CLIENT_JOBS_MAX 5 /* max client rpcs we can queue */
+#define VA_SERVER_JOBS_MAX 5 /* max server rpcs we can queue */
+#define VA_SERVER_TIMEOUT_MS 5 * 1000
+#define VA_CLIENT_TIMEOUT_MS 5 * 1000
+#define VA_SENTINEL 0xFF
+#define VA_BAUDRATE B38400 /* for isa-serial channels */
+


I've been after these before - please put the ones that make sense to
tune into a config file, and the same with the pidfile.


I think my contention last time was that most of these weren't meant to 
be tweakable by an end-user, they're mainly just to avoid using magic 
numbers everywhere.


For stuff that is, like the pid file and socket/port paths, these would 
be the defaults, and the option to override them would be provided via 
the command line (virtagent chardev options on the host, command options 
on the guest).


I did plan to make the distinction between the 2 clearer though, by 
adding a DEFAULT_* or something along that line. Will get those in for 
the next pass.




Cheers,
Jes

[Qemu-devel] [PATCH v2] vnc: threaded server depends on io-thread

[Corentin Chary]

The threaded VNC servers messed up with QEMU fd handlers without
any kind of locking, and that can cause some nasty race conditions.

The IO-Thread provides appropriate locking primitives to avoid that.
This patch makes CONFIG_VNC_THREAD depends on CONFIG_IO_THREAD,
and add lock and unlock calls around the two faulty calls.

Thanks to Jan Kiszka for helping me solve this issue.

Cc: Jan Kiszka 
Signed-off-by: Corentin Chary 
---
The previous patch was total crap, introduced race conditions,
and probably crashs on client disconnections.

 configure   |9 +
 ui/vnc-jobs-async.c |   24 +++-
 2 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/configure b/configure
index 5513d3e..c8c1ac1 100755
--- a/configure
+++ b/configure
@@ -2455,6 +2455,15 @@ if test \( "$cpu" = "i386" -o "$cpu" = "x86_64" \) -a \
   roms="optionrom"
 fi
 
+# VNC Thread depends on IO Thread
+if test "$vnc_thread" = "yes" -a "$io_thread" != "yes"; then
+  echo
+  echo "ERROR: VNC thread depends on IO thread which isn't enabled."
+  echo "Please use --enable-io-thread if you want to enable it."
+  echo
+  exit 1
+fi
+
 
 echo "Install prefix$prefix"
 echo "BIOS directory`eval echo $datadir`"
diff --git a/ui/vnc-jobs-async.c b/ui/vnc-jobs-async.c
index f596247..d0c6f61 100644
--- a/ui/vnc-jobs-async.c
+++ b/ui/vnc-jobs-async.c
@@ -190,6 +190,18 @@ static void vnc_async_encoding_end(VncState *orig, 
VncState *local)
 queue->buffer = local->output;
 }
 
+static void vnc_worker_lock_output(VncState *vs)
+{
+qemu_mutex_lock_iothread();
+vnc_lock_output(vs);
+}
+
+static void vnc_worker_unlock_output(VncState *vs)
+{
+vnc_unlock_output(vs);
+qemu_mutex_unlock_iothread();
+}
+
 static int vnc_worker_thread_loop(VncJobQueue *queue)
 {
 VncJob *job;
@@ -211,11 +223,11 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
 return -1;
 }
 
-vnc_lock_output(job->vs);
+vnc_worker_lock_output(job->vs);
 if (job->vs->csock == -1 || job->vs->abort == true) {
 goto disconnected;
 }
-vnc_unlock_output(job->vs);
+vnc_worker_unlock_output(job->vs);
 
 /* Make a local copy of vs and switch output buffers */
 vnc_async_encoding_start(job->vs, &vs);
@@ -236,7 +248,7 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
 /* output mutex must be locked before going to
  * disconnected:
  */
-vnc_lock_output(job->vs);
+vnc_worker_lock_output(job->vs);
 goto disconnected;
 }
 
@@ -255,7 +267,7 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
 vs.output.buffer[saved_offset + 1] = n_rectangles & 0xFF;
 
 /* Switch back buffers */
-vnc_lock_output(job->vs);
+vnc_worker_lock_output(job->vs);
 if (job->vs->csock == -1) {
 goto disconnected;
 }
@@ -266,10 +278,12 @@ disconnected:
 /* Copy persistent encoding data */
 vnc_async_encoding_end(job->vs, &vs);
 flush = (job->vs->csock != -1 && job->vs->abort != true);
-vnc_unlock_output(job->vs);
+vnc_worker_unlock_output(job->vs);
 
 if (flush) {
+qemu_mutex_lock_iothread();
 vnc_flush(job->vs);
+qemu_mutex_unlock_iothread();
 }
 
 vnc_lock_queue(queue);
-- 
1.7.3.4

[Qemu-devel] [PATCH]hw/xen_disk: aio_inflight not released in handling ioreq when nr_segments==0

[Feiran Zheng]

In hw/xen_disk.c, async writing ioreq is leaked when
ioreq->req.nr_segments==0, because `aio_inflight` flag is not released
properly (skipped by misplaced "break").

Signed-off-by: Feiran Zheng 
---
 hw/xen_disk.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/xen_disk.c b/hw/xen_disk.c
index ed9e5eb..445bf03 100644
--- a/hw/xen_disk.c
+++ b/hw/xen_disk.c
@@ -408,9 +408,9 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
break;
 case BLKIF_OP_WRITE:
 case BLKIF_OP_WRITE_BARRIER:
-ioreq->aio_inflight++;
 if (!ioreq->req.nr_segments)
 break;
+ioreq->aio_inflight++;
 bdrv_aio_writev(blkdev->bs, ioreq->start / BLOCK_SIZE,
 &ioreq->v, ioreq->v.size / BLOCK_SIZE,
 qemu_aio_complete, ioreq);

Re: [Qemu-devel] [PATCH 14/22] qapi: add query-version QMP command

[Avi Kivity]

On 03/07/2011 03:41 PM, Anthony Liguori wrote:

On 03/07/2011 07:35 AM, Stefan Hajnoczi wrote:
On Mon, Mar 7, 2011 at 1:22 AM, Anthony Liguori  
wrote:

diff --git a/qmp-schema.json b/qmp-schema.json
index e69de29..b343f5e 100644
--- a/qmp-schema.json
+++ b/qmp-schema.json
@@ -0,0 +1,38 @@
+# *-*- Mode: Python -*-*

By the way JSON does not seem to support comments (neither /* */ nor
#).  So this comment feature you're using is an extension to JSON.


Yeah, it's only loosely JSON as I don't use a JSON parser.


Goes kind of against all the buzzwords you're letting fly here...

--
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 19/22] qapi: add QMP put-event command

[Avi Kivity]

On 03/07/2011 03:23 AM, Anthony Liguori wrote:

This is needed for libqmp to support events.  put-event is used to disconnect
from signals.

Signed-off-by: Anthony Liguori

diff --git a/qmp-schema.json b/qmp-schema.json
index 3f2dd4e..a13885f 100644
--- a/qmp-schema.json
+++ b/qmp-schema.json
@@ -58,3 +58,18 @@
  # Since: 0.14.0
  ##
  [ 'qmp_capabilities', {}, {}, 'none' ]
+
+##
+# @put_event:
+#
+# Disconnect a signal.  This command is used to disconnect from a signal based
+# on the handle returned by a signal accessor.
+#
+# @tag: the handle returned by a signal accessor.
+#
+# Returns: Nothing on success.
+#  If @tag is not a valid handle, InvalidParameterValue
+#
+# Since: 0.15.0
+##
+[ 'put-event', {'tag': 'int'}, {}, 'none' ]


Why is tag an int?  don't we use strings for command ids and similar?

Also could be better named, disconnect-event or unlisten-event.

--
error compiling committee.c: too many arguments to function

[Qemu-devel] [PATCH] qmp-commands.hx: Clean up mess of client_migrate_info

[Jes . Sorensen]

From: Jes Sorensen 

client_migrate_info was put into qmp-commands.hx in the middle of
migrate_set_speed, between the command and it's description. In
addition client_migrate_info put the description before the command
itself, which is the wrong order.

Signed-off-by: Jes Sorensen 
---
 qmp-commands.hx |   68 +++---
 1 files changed, 34 insertions(+), 34 deletions(-)

diff --git a/qmp-commands.hx b/qmp-commands.hx
index df40a3d..9d3cc31 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -503,79 +503,79 @@ EQMP
 },
 
 SQMP
-client_migrate_info
---
+migrate_set_speed
+-
 
-Set the spice/vnc connection info for the migration target.  The spice/vnc
-server will ask the spice/vnc client to automatically reconnect using the
-new parameters (if specified) once the vm migration finished successfully.
+Set maximum speed for migrations.
 
 Arguments:
 
-- "protocol": protocol: "spice" or "vnc" (json-string)
-- "hostname": migration target hostname (json-string)
-- "port": spice/vnc tcp port for plaintext channels (json-int, 
optional)
-- "tls-port": spice tcp port for tls-secured channels (json-int, optional)
-- "cert-subject": server certificate subject (json-string, optional)
+- "value": maximum speed, in bytes per second (json-int)
 
 Example:
 
--> { "execute": "client_migrate_info",
- "arguments": { "protocol": "spice",
-"hostname": "virt42.lab.kraxel.org",
-"port": 1234 } }
+-> { "execute": "migrate_set_speed", "arguments": { "value": 1024 } }
 <- { "return": {} }
 
 EQMP
 
 {
-.name   = "client_migrate_info",
-.args_type  = 
"protocol:s,hostname:s,port:i?,tls-port:i?,cert-subject:s?",
-.params = "protocol hostname port tls-port cert-subject",
-.help   = "send migration info to spice/vnc client",
+.name   = "migrate_set_downtime",
+.args_type  = "value:T",
+.params = "value",
+.help   = "set maximum tolerated downtime (in seconds) for 
migrations",
 .user_print = monitor_user_noop,
-.mhandler.cmd_new = client_migrate_info,
+.mhandler.cmd_new = do_migrate_set_downtime,
 },
 
 SQMP
-migrate_set_speed
--
+migrate_set_downtime
+
 
-Set maximum speed for migrations.
+Set maximum tolerated downtime (in seconds) for migrations.
 
 Arguments:
 
-- "value": maximum speed, in bytes per second (json-int)
+- "value": maximum downtime (json-number)
 
 Example:
 
--> { "execute": "migrate_set_speed", "arguments": { "value": 1024 } }
+-> { "execute": "migrate_set_downtime", "arguments": { "value": 0.1 } }
 <- { "return": {} }
 
 EQMP
 
 {
-.name   = "migrate_set_downtime",
-.args_type  = "value:T",
-.params = "value",
-.help   = "set maximum tolerated downtime (in seconds) for 
migrations",
+.name   = "client_migrate_info",
+.args_type  = 
"protocol:s,hostname:s,port:i?,tls-port:i?,cert-subject:s?",
+.params = "protocol hostname port tls-port cert-subject",
+.help   = "send migration info to spice/vnc client",
 .user_print = monitor_user_noop,
-.mhandler.cmd_new = do_migrate_set_downtime,
+.mhandler.cmd_new = client_migrate_info,
 },
 
 SQMP
-migrate_set_downtime
-
+client_migrate_info
+--
 
-Set maximum tolerated downtime (in seconds) for migrations.
+Set the spice/vnc connection info for the migration target.  The spice/vnc
+server will ask the spice/vnc client to automatically reconnect using the
+new parameters (if specified) once the vm migration finished successfully.
 
 Arguments:
 
-- "value": maximum downtime (json-number)
+- "protocol": protocol: "spice" or "vnc" (json-string)
+- "hostname": migration target hostname (json-string)
+- "port": spice/vnc tcp port for plaintext channels (json-int, 
optional)
+- "tls-port": spice tcp port for tls-secured channels (json-int, optional)
+- "cert-subject": server certificate subject (json-string, optional)
 
 Example:
 
--> { "execute": "migrate_set_downtime", "arguments": { "value": 0.1 } }
+-> { "execute": "client_migrate_info",
+ "arguments": { "protocol": "spice",
+"hostname": "virt42.lab.kraxel.org",
+"port": 1234 } }
 <- { "return": {} }
 
 EQMP
-- 
1.7.4

Re: [Qemu-devel] [PATCH 14/22] qapi: add query-version QMP command

[Avi Kivity]

On 03/07/2011 03:22 AM, Anthony Liguori wrote:

This is used internally by QMP.  It's also a pretty good example of a typical
command conversion.

+##
+{ 'VersionInfo': {'qemu': {'major': 'int', 'minor': 'int', 'micro': 'int'},
+  'package': 'str'} }
+
+##
+# @query-version:
+#
+# Returns the current version of QEMU.
+#
+# Returns:  A @VersionInfo object describing the current version of QEMU.
+#
+# Since: 0.14.0
+##
+[ 'query-version', {}, {}, 'VersionInfo' ]


(just picking on a patch that has a bit of schema in it)

Something that can be added to the schema are default values for newly 
added parameters.  This way we can avoid an explosion of commands where 
adding an optional parameter suffices; should be easier for the user to 
pick the right command and easier for us to document and support.


--
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH 1/3] qemu_next_deadline should not consider host-time timers

[Paolo Bonzini]

On 03/05/2011 07:07 PM, Jan Kiszka wrote:

On 2011-03-05 18:14, Paolo Bonzini wrote:

It is purely for icount-based virtual timers.


How about renaming the function to clarify its scope?


Will do.

Paolo

[Qemu-devel] Re: [PATCH v2] vnc: threaded server depends on io-thread

[Corentin Chary]

On Wed, Mar 9, 2011 at 1:21 PM, Corentin Chary  wrote:
> The threaded VNC servers messed up with QEMU fd handlers without
> any kind of locking, and that can cause some nasty race conditions.
>
> The IO-Thread provides appropriate locking primitives to avoid that.
> This patch makes CONFIG_VNC_THREAD depends on CONFIG_IO_THREAD,
> and add lock and unlock calls around the two faulty calls.
>
> Thanks to Jan Kiszka for helping me solve this issue.
>
> Cc: Jan Kiszka 
> Signed-off-by: Corentin Chary 
> ---
> The previous patch was total crap, introduced race conditions,
> and probably crashs on client disconnections.

Forget that one too, also deadlock on vnc_jobs_join(). I'll need some
more time to fix that.

-- 
Corentin Chary
http://xf.iksaif.net

Re: [Qemu-devel] [PATCH 14/22] qapi: add query-version QMP command

[Anthony Liguori]

On 03/09/2011 07:28 AM, Avi Kivity wrote:

On 03/07/2011 03:41 PM, Anthony Liguori wrote:

On 03/07/2011 07:35 AM, Stefan Hajnoczi wrote:
On Mon, Mar 7, 2011 at 1:22 AM, Anthony 
Liguori  wrote:

diff --git a/qmp-schema.json b/qmp-schema.json
index e69de29..b343f5e 100644
--- a/qmp-schema.json
+++ b/qmp-schema.json
@@ -0,0 +1,38 @@
+# *-*- Mode: Python -*-*

By the way JSON does not seem to support comments (neither /* */ nor
#).  So this comment feature you're using is an extension to JSON.


Yeah, it's only loosely JSON as I don't use a JSON parser.


Goes kind of against all the buzzwords you're letting fly here...


The schema defines arguments in a dictionary because in QMP, the 
argument order doesn't matter.  But the argument order matters in C so I 
need to use a custom parser to preserve dictionary order.


There's no way to do commenting in JSON and I really wanted to have 
inline documentation.


But otherwise, it's valid JSON.

Regards,

Anthony Liguori

Re: [Qemu-devel] OVMF Google Summer of Code ideas

[Natalia Portillo]

Hi all,

This may come late in the discussion, but, has OVMF been tested with Mac OS X?

A decent Intel Macintosh emulation requires of course EFI + HFS.

Regards,
Natalia Portillo

El 09/03/2011, a las 05:34, Jordan Justen escribió:

> On Tue, Mar 8, 2011 at 18:23, Kevin O'Connor  wrote:
>> On Tue, Mar 08, 2011 at 09:00:05AM -0800, Jordan Justen wrote:
>>> Yes, the UEFI system is still in place.  The UEFI part still handles
>>> the majority of platform init, and calls into the CSM at various
>>> points.  The CSM returns back to UEFI for all CSM calls, except the
>>> legacy boot.
>> 
>> Is there a concise list of these various callbacks between UEFI and
>> CSM?
>> 
>> If SeaBIOS just needs to be loaded up for legacy boots, that doesn't
>> sound too difficult.  However, if SeaBIOS would need to translate
>> various BIOS calls into UEFI calls - that sounds like it could be
>> complex.
> 
> A CSM does not really know about UEFI for the most part.  Rather it
> carries out some tasks when request by the UEFI environment.  The UEFI
> side still manages the high level boot flow (even for legacy boots).
> The CSM does not call into UEFI services, but just returns back to
> whoever invoked the CSM call.
> 
> The 16-bit CSM component interface is described in this file:
> https://edk2.svn.sourceforge.net/svnroot/edk2/trunk/edk2/IntelFrameworkPkg/Include/Protocol/LegacyBios.h
> 
> The full CSM specification document is available here:
> http://www.intel.com/technology/framework/spec.htm
> 
> Thanks,
> 
> -Jordan
> 

Re: [Qemu-devel] [PATCH 19/22] qapi: add QMP put-event command

[Anthony Liguori]

On 03/09/2011 07:31 AM, Avi Kivity wrote:

On 03/07/2011 03:23 AM, Anthony Liguori wrote:
This is needed for libqmp to support events.  put-event is used to 
disconnect

from signals.

Signed-off-by: Anthony Liguori

diff --git a/qmp-schema.json b/qmp-schema.json
index 3f2dd4e..a13885f 100644
--- a/qmp-schema.json
+++ b/qmp-schema.json
@@ -58,3 +58,18 @@
  # Since: 0.14.0
  ##
  [ 'qmp_capabilities', {}, {}, 'none' ]
+
+##
+# @put_event:
+#
+# Disconnect a signal.  This command is used to disconnect from a 
signal based

+# on the handle returned by a signal accessor.
+#
+# @tag: the handle returned by a signal accessor.
+#
+# Returns: Nothing on success.
+#  If @tag is not a valid handle, InvalidParameterValue
+#
+# Since: 0.15.0
+##
+[ 'put-event', {'tag': 'int'}, {}, 'none' ]


Why is tag an int?


It's a handle so the type doesn't matter as long as I can make sure 
values are unique.  ints are easier to work with because they don't 
require memory allocation.



  don't we use strings for command ids and similar?


id's can be any valid JSON value.

But a handle is not the same thing as an id.


Also could be better named, disconnect-event or unlisten-event.


I was going for symmetry with the signal accessors which are typically 
in the format 'get-block-io-error-event'.


Maybe it would be better to do 'connect-block-io-error-event' and 
'disconnect-event'?


Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 00/22] QAPI Round 1

[Anthony Liguori]

On 03/09/2011 07:14 AM, Avi Kivity wrote:

On 03/09/2011 03:12 PM, Anthony Liguori wrote:

On 03/09/2011 02:51 AM, Avi Kivity wrote:

On 03/08/2011 09:19 PM, Anthony Liguori wrote:
Both the guest and the management agent (and both can listen for 
events).  I don't see why guest->qemu RPC is problematic for 
migration - at least when qemu terminates it.



If it's terminated in QEMU, it's fine, but then it's not QMP 
anymore.  Let me think about whether there's a way to achieve this 
without a guest->qemu RPC.


Why not?

{ execute: 'write-keystore' arguments: { 'key': 'foo', 'value': 
'bar' } }


This is coming from the guest? 


Yes.


So what about:

{ 'KeyStore': { '*foo': 'str', '*otherkey': 'str' } }
[ 'guest-write-keystore', { 'keystore': 'KeyStore' }, 'none' ]

{ 'GUEST_UPDATE_KEYSTORE': { 'keys': [ 'str' ] } }

In this model, the key store is actually stored in the guest.  If the 
guest wants the latest version of a value, it sends an event to get keys 
updated.  The host can update keys at any point in time.


Regards,

Anthony Liguori



QMP doesn't do bidirectional RPC today.  It could, but that is a 
fundamental change in the protocol.




Could use a separate channel for talking to qemu.

[Qemu-devel] Re: [PATCH v2] vnc: threaded server depends on io-thread

[Paolo Bonzini]

On 03/09/2011 02:21 PM, Corentin Chary wrote:

The threaded VNC servers messed up with QEMU fd handlers without
any kind of locking, and that can cause some nasty race conditions.

The IO-Thread provides appropriate locking primitives to avoid that.
This patch makes CONFIG_VNC_THREAD depends on CONFIG_IO_THREAD,
and add lock and unlock calls around the two faulty calls.

Thanks to Jan Kiszka for helping me solve this issue.

Cc: Jan Kiszka
Signed-off-by: Corentin Chary
---
The previous patch was total crap, introduced race conditions,
and probably crashs on client disconnections.

  configure   |9 +
  ui/vnc-jobs-async.c |   24 +++-
  2 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/configure b/configure
index 5513d3e..c8c1ac1 100755
--- a/configure
+++ b/configure
@@ -2455,6 +2455,15 @@ if test \( "$cpu" = "i386" -o "$cpu" = "x86_64" \) -a \
roms="optionrom"
  fi

+# VNC Thread depends on IO Thread
+if test "$vnc_thread" = "yes" -a "$io_thread" != "yes"; then
+  echo
+  echo "ERROR: VNC thread depends on IO thread which isn't enabled."
+  echo "Please use --enable-io-thread if you want to enable it."
+  echo
+  exit 1
+fi
+

  echo "Install prefix$prefix"
  echo "BIOS directory`eval echo $datadir`"
diff --git a/ui/vnc-jobs-async.c b/ui/vnc-jobs-async.c
index f596247..d0c6f61 100644
--- a/ui/vnc-jobs-async.c
+++ b/ui/vnc-jobs-async.c
@@ -190,6 +190,18 @@ static void vnc_async_encoding_end(VncState *orig, 
VncState *local)
  queue->buffer = local->output;
  }

+static void vnc_worker_lock_output(VncState *vs)
+{
+qemu_mutex_lock_iothread();
+vnc_lock_output(vs);
+}
+
+static void vnc_worker_unlock_output(VncState *vs)
+{
+vnc_unlock_output(vs);
+qemu_mutex_unlock_iothread();
+}
+
  static int vnc_worker_thread_loop(VncJobQueue *queue)
  {
  VncJob *job;
@@ -211,11 +223,11 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
  return -1;
  }

-vnc_lock_output(job->vs);
+vnc_worker_lock_output(job->vs);
  if (job->vs->csock == -1 || job->vs->abort == true) {
  goto disconnected;
  }
-vnc_unlock_output(job->vs);
+vnc_worker_unlock_output(job->vs);

  /* Make a local copy of vs and switch output buffers */
  vnc_async_encoding_start(job->vs,&vs);
@@ -236,7 +248,7 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
  /* output mutex must be locked before going to
   * disconnected:
   */
-vnc_lock_output(job->vs);
+vnc_worker_lock_output(job->vs);
  goto disconnected;
  }

@@ -255,7 +267,7 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
  vs.output.buffer[saved_offset + 1] = n_rectangles&  0xFF;

  /* Switch back buffers */
-vnc_lock_output(job->vs);
+vnc_worker_lock_output(job->vs);
  if (job->vs->csock == -1) {
  goto disconnected;
  }
@@ -266,10 +278,12 @@ disconnected:
  /* Copy persistent encoding data */
  vnc_async_encoding_end(job->vs,&vs);
  flush = (job->vs->csock != -1&&  job->vs->abort != true);
-vnc_unlock_output(job->vs);
+vnc_worker_unlock_output(job->vs);

  if (flush) {
+qemu_mutex_lock_iothread();
  vnc_flush(job->vs);
+qemu_mutex_unlock_iothread();
  }

  vnc_lock_queue(queue);


Acked-by: Paolo Bonzini  for stable.

For 0.15, I believe an iohandler-list lock is a better solution.

Paolo

Re: [Qemu-devel] [PATCH 00/22] QAPI Round 1

[Michael Roth]

On 03/09/2011 07:14 AM, Avi Kivity wrote:

On 03/09/2011 03:12 PM, Anthony Liguori wrote:

On 03/09/2011 02:51 AM, Avi Kivity wrote:

On 03/08/2011 09:19 PM, Anthony Liguori wrote:

Both the guest and the management agent (and both can listen for
events). I don't see why guest->qemu RPC is problematic for
migration - at least when qemu terminates it.



If it's terminated in QEMU, it's fine, but then it's not QMP
anymore. Let me think about whether there's a way to achieve this
without a guest->qemu RPC.


Why not?

{ execute: 'write-keystore' arguments: { 'key': 'foo', 'value': 'bar'
} }


This is coming from the guest?


Yes.


QMP doesn't do bidirectional RPC today. It could, but that is a
fundamental change in the protocol.



Could use a separate channel for talking to qemu.



That's a possibility. Might be tricky to do right though...we wouldn't 
want to have a guest get a normal QMP session over the channel, so we'd 
likely end up with a separate QMP server that uses a different 
guest-specific dispatch table for commands.


Would be nice to not have to rely on multiple channels 
though...particularly in the case of isa-serial channels where available 
ports might be scarce. But I wouldn't consider that a showstopper either.

Re: [Qemu-devel] [PATCH v2] Add qcow2 documentation

[Kevin Wolf]

Am 09.03.2011 14:08, schrieb Stefan Hajnoczi:
> On Wed, Mar 9, 2011 at 12:30 PM, Kevin Wolf  wrote:
>> This adds a description of the qcow2 file format to the docs/ directory.
>> Besides documenting what's there, which is never wrong, the document should
>> provide a good basis for the discussion of format extensions (called "qcow3"
>> in previous discussions)
>>
>> Signed-off-by: Kevin Wolf 
>> ---
>>  docs/specs/qcow2.txt |  233 
>> ++
>>  1 files changed, 233 insertions(+), 0 deletions(-)
>>  create mode 100644 docs/specs/qcow2.txt
> 
> QCOW2_EXT_MAGIC_BACKING_FORMAT is not documented.  Did you decide that
> is a QEMU internal extension and you don't want it documented?

I just completely forgot to include header extensions. Though I think
the allowed values for the backing file format are implementation specific.

Anything else to consider for v3?

Kevin

Re: [Qemu-devel] [PATCH 00/22] QAPI Round 1

[Avi Kivity]

On 03/09/2011 03:51 PM, Anthony Liguori wrote:
{ execute: 'write-keystore' arguments: { 'key': 'foo', 'value': 
'bar' } }


This is coming from the guest? 


Yes.



So what about:

{ 'KeyStore': { '*foo': 'str', '*otherkey': 'str' } }
[ 'guest-write-keystore', { 'keystore': 'KeyStore' }, 'none' ]

{ 'GUEST_UPDATE_KEYSTORE': { 'keys': [ 'str' ] } }


(why the CAPS?)



In this model, the key store is actually stored in the guest.  If the 
guest wants the latest version of a value, it sends an event to get 
keys updated.  The host can update keys at any point in time.


This survives live migration, but not guest reboots or crashes.

--
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 14/22] qapi: add query-version QMP command

[Avi Kivity]

On 03/09/2011 03:44 PM, Anthony Liguori wrote:

Yeah, it's only loosely JSON as I don't use a JSON parser.


Goes kind of against all the buzzwords you're letting fly here...



The schema defines arguments in a dictionary because in QMP, the 
argument order doesn't matter.  But the argument order matters in C so 
I need to use a custom parser to preserve dictionary order.


We could extend our parser to annotate the dictionary with the original 
order.  Not worth it though.




There's no way to do commenting in JSON and I really wanted to have 
inline documentation.


But otherwise, it's valid JSON.



We should then have a transformation that generates a valid json for 
clients to use.  We could even include the documentation as a 'doc': key.


--
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [RFC][PATCH v7 01/16] Move code related to fd handlers into utility functions

[Paolo Bonzini]

On 03/07/2011 09:10 PM, Michael Roth wrote:

This allows us to implement an i/o loop outside of vl.c that can
interact with objects that use qemu_set_fd_handler()


I must say I really dislike the patches 1..3.  It's _really_ getting the 
QEMU NIH worse.  While it is not really possible to get a new shiny 
mainloop infrastructure in QEMU like snapping fingers (and I'm not sure 
the glib mainloop will ever happen there), there is no reason not to 
adopt glib's infrastructure in virtagent.  While cooperation between 
QEMU and virtagent is close, it is IMHO a substantially separate project 
that can afford starting from a clean slate.


If anybody disagrees, I'd be happy to hear their opinion anyway!

I'm sorry I'm saying this only now and I've been ignoring this series 
until v7.


Paolo

Re: [Qemu-devel] [PATCH 19/22] qapi: add QMP put-event command

[Avi Kivity]

On 03/09/2011 03:48 PM, Anthony Liguori wrote:

+[ 'put-event', {'tag': 'int'}, {}, 'none' ]


Why is tag an int?

+##

It's a handle so the type doesn't matter as long as I can make sure 
values are unique.  ints are easier to work with because they don't 
require memory allocation.


I think it's nicer for the client to use a string.  Instead of a global 
ID allocator, it can use unique IDs or unique prefixes + local IDs.  
Should also aid a little in debugging.





  don't we use strings for command ids and similar?


id's can be any valid JSON value.

But a handle is not the same thing as an id.


Why not?

I hope handles are client-provided?




Also could be better named, disconnect-event or unlisten-event.


I was going for symmetry with the signal accessors which are typically 
in the format 'get-block-io-error-event'.


Maybe it would be better to do 'connect-block-io-error-event' and 
'disconnect-event'?


Yes.

But I'm confused, do we have a per-event command on the wire?  Or just C 
stubs?


--
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH v2] vnc: threaded server depends on io-thread

[Corentin Chary]

On Wed, Mar 9, 2011 at 1:51 PM, Paolo Bonzini  wrote:
> On 03/09/2011 02:21 PM, Corentin Chary wrote:
>>
>> The threaded VNC servers messed up with QEMU fd handlers without
>> any kind of locking, and that can cause some nasty race conditions.
>>
>> The IO-Thread provides appropriate locking primitives to avoid that.
>> This patch makes CONFIG_VNC_THREAD depends on CONFIG_IO_THREAD,
>> and add lock and unlock calls around the two faulty calls.
>>
>> Thanks to Jan Kiszka for helping me solve this issue.
>>
>> Cc: Jan Kiszka
>> Signed-off-by: Corentin Chary
>> ---
>> The previous patch was total crap, introduced race conditions,
>> and probably crashs on client disconnections.
>>
>>  configure           |    9 +
>>  ui/vnc-jobs-async.c |   24 +++-
>>  2 files changed, 28 insertions(+), 5 deletions(-)
>>
>> diff --git a/configure b/configure
>> index 5513d3e..c8c1ac1 100755
>> --- a/configure
>> +++ b/configure
>> @@ -2455,6 +2455,15 @@ if test \( "$cpu" = "i386" -o "$cpu" = "x86_64" \)
>> -a \
>>    roms="optionrom"
>>  fi
>>
>> +# VNC Thread depends on IO Thread
>> +if test "$vnc_thread" = "yes" -a "$io_thread" != "yes"; then
>> +  echo
>> +  echo "ERROR: VNC thread depends on IO thread which isn't enabled."
>> +  echo "Please use --enable-io-thread if you want to enable it."
>> +  echo
>> +  exit 1
>> +fi
>> +
>>
>>  echo "Install prefix    $prefix"
>>  echo "BIOS directory    `eval echo $datadir`"
>> diff --git a/ui/vnc-jobs-async.c b/ui/vnc-jobs-async.c
>> index f596247..d0c6f61 100644
>> --- a/ui/vnc-jobs-async.c
>> +++ b/ui/vnc-jobs-async.c
>> @@ -190,6 +190,18 @@ static void vnc_async_encoding_end(VncState *orig,
>> VncState *local)
>>      queue->buffer = local->output;
>>  }
>>
>> +static void vnc_worker_lock_output(VncState *vs)
>> +{
>> +    qemu_mutex_lock_iothread();
>> +    vnc_lock_output(vs);
>> +}
>> +
>> +static void vnc_worker_unlock_output(VncState *vs)
>> +{
>> +    vnc_unlock_output(vs);
>> +    qemu_mutex_unlock_iothread();
>> +}
>> +
>>  static int vnc_worker_thread_loop(VncJobQueue *queue)
>>  {
>>      VncJob *job;
>> @@ -211,11 +223,11 @@ static int vnc_worker_thread_loop(VncJobQueue
>> *queue)
>>          return -1;
>>      }
>>
>> -    vnc_lock_output(job->vs);
>> +    vnc_worker_lock_output(job->vs);
>>      if (job->vs->csock == -1 || job->vs->abort == true) {
>>          goto disconnected;
>>      }
>> -    vnc_unlock_output(job->vs);
>> +    vnc_worker_unlock_output(job->vs);
>>
>>      /* Make a local copy of vs and switch output buffers */
>>      vnc_async_encoding_start(job->vs,&vs);
>> @@ -236,7 +248,7 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
>>              /* output mutex must be locked before going to
>>               * disconnected:
>>               */
>> -            vnc_lock_output(job->vs);
>> +            vnc_worker_lock_output(job->vs);
>>              goto disconnected;
>>          }
>>
>> @@ -255,7 +267,7 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
>>      vs.output.buffer[saved_offset + 1] = n_rectangles&  0xFF;
>>
>>      /* Switch back buffers */
>> -    vnc_lock_output(job->vs);
>> +    vnc_worker_lock_output(job->vs);
>>      if (job->vs->csock == -1) {
>>          goto disconnected;
>>      }
>> @@ -266,10 +278,12 @@ disconnected:
>>      /* Copy persistent encoding data */
>>      vnc_async_encoding_end(job->vs,&vs);
>>      flush = (job->vs->csock != -1&&  job->vs->abort != true);
>> -    vnc_unlock_output(job->vs);
>> +    vnc_worker_unlock_output(job->vs);
>>
>>      if (flush) {
>> +        qemu_mutex_lock_iothread();
>>          vnc_flush(job->vs);
>> +        qemu_mutex_unlock_iothread();
>>      }
>>
>>      vnc_lock_queue(queue);
>
> Acked-by: Paolo Bonzini  for stable.

Nacked-by: Corentin Chary 

> For 0.15, I believe an iohandler-list lock is a better solution.

I believe that's the only solution. Looks at that deadlock:

(gdb) bt
#0  0x7f70c067ad74 in __lll_lock_wait () from /lib/libpthread.so.0
#1  0x7f70c0676219 in _L_lock_508 () from /lib/libpthread.so.0
#2  0x7f70c067603b in pthread_mutex_lock () from /lib/libpthread.so.0
#3  0x004c7559 in qemu_mutex_lock (mutex=0x10f4340) at qemu-thread.c:50
#4  0x005644ef in main_loop_wait (nonblocking=) at /home/iksaif/dev/qemu/vl.c:1374
#5  0x005653a8 in main_loop (argc=,
argv=, envp=) at
/home/iksaif/dev/qemu/vl.c:1437
#6  main (argc=, argv=,
envp=) at /home/iksaif/dev/qemu/vl.c:3148
(gdb) t 2
[Switching to thread 2 (Thread 0x7f709262b710 (LWP 19546))]#0
0x7f70c067ad74 in __lll_lock_wait () from /lib/libpthread.so.0
(gdb) bt
#0  0x7f70c067ad74 in __lll_lock_wait () from /lib/libpthread.so.0
#1  0x7f70c0676219 in _L_lock_508 () from /lib/libpthread.so.0
#2  0x7f70c067603b in pthread_mutex_lock () from /lib/libpthread.so.0
#3  0x004c7559 in qemu_mutex_lock (mutex=0x10f4340) at qemu-thread.c:50
#4  0x004c65de in vnc_worker_thread_loop (queue=0x33561d0) at
ui/vnc-jobs-async.c:254

[Qemu-devel] Re: [RFC][PATCH v7 01/16] Move code related to fd handlers into utility functions

[Paolo Bonzini]

On 03/07/2011 09:10 PM, Michael Roth wrote:

+
+/* XXX: fd_read_poll should be suppressed, but an API change is
+   necessary in the character devices to suppress fd_can_read(). */
+int qemu_set_fd_handler3(void *ioh_record_list,
+ int fd,
+ IOCanReadHandler *fd_read_poll,
+ IOHandler *fd_read,
+ IOHandler *fd_write,
+ void *opaque)


What's the reason to introduce this additional indirection (and with a 
void rather than opaque pointer)?  A global iohandlers list would be 
fine in qemu-ioh.c (and it would be a worthwhile patch anyway for QEMU).


Paolo

[Qemu-devel] Re: [RFC][PATCH v7 01/16] Move code related to fd handlers into utility functions

[Michael Roth]

On 03/09/2011 07:58 AM, Paolo Bonzini wrote:

On 03/07/2011 09:10 PM, Michael Roth wrote:

This allows us to implement an i/o loop outside of vl.c that can
interact with objects that use qemu_set_fd_handler()


I must say I really dislike the patches 1..3. It's _really_ getting the
QEMU NIH worse. While it is not really possible to get a new shiny
mainloop infrastructure in QEMU like snapping fingers (and I'm not sure
the glib mainloop will ever happen there), there is no reason not to
adopt glib's infrastructure in virtagent. While cooperation between QEMU
and virtagent is close, it is IMHO a substantially separate project that
can afford starting from a clean slate.

If anybody disagrees, I'd be happy to hear their opinion anyway!

I'm sorry I'm saying this only now and I've been ignoring this series
until v7.


In the context of virtagent I would agree. The only complication there 
being that a large part of the event-driven code (the async read/write 
handlers for instance) is shared between virtagent and the host. 
Possibility this could be worked around with a set of wrappers..but it's 
hard to say.


But more importantly, I wouldn't think of these changes as being 
specific to virtagent though. Currently we have a lot of qemu tools that 
stub out portions of the block code they pull in (qemu_set_fd_handler 
and whatnot). I think it might be beneficial to future tools/test 
utilities that they actually be able to drive things like aio and timer 
events. We just keep stubbing more and more things out in these cases, 
which I would argue is even worse because it can place artificial 
constraints on how code is written that happens to get used by such tools.




Paolo

Re: [Qemu-devel] [PATCH 14/22] qapi: add query-version QMP command

[Anthony Liguori]

On 03/09/2011 07:51 AM, Avi Kivity wrote:

On 03/09/2011 03:44 PM, Anthony Liguori wrote:

Yeah, it's only loosely JSON as I don't use a JSON parser.


Goes kind of against all the buzzwords you're letting fly here...



The schema defines arguments in a dictionary because in QMP, the 
argument order doesn't matter.  But the argument order matters in C 
so I need to use a custom parser to preserve dictionary order.


We could extend our parser to annotate the dictionary with the 
original order.  Not worth it though.




There's no way to do commenting in JSON and I really wanted to have 
inline documentation.


But otherwise, it's valid JSON.



We should then have a transformation that generates a valid json for 
clients to use.  We could even include the documentation as a 'doc': key.


Yes, as I mentioned on the call, that's my plan for 0.15.  I hadn't 
thought about the doc bit though.


Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 14/22] qapi: add query-version QMP command

[Anthony Liguori]

On 03/09/2011 07:36 AM, Avi Kivity wrote:

On 03/07/2011 03:22 AM, Anthony Liguori wrote:
This is used internally by QMP.  It's also a pretty good example of a 
typical

command conversion.

+##
+{ 'VersionInfo': {'qemu': {'major': 'int', 'minor': 'int', 'micro': 
'int'},

+  'package': 'str'} }
+
+##
+# @query-version:
+#
+# Returns the current version of QEMU.
+#
+# Returns:  A @VersionInfo object describing the current version of 
QEMU.

+#
+# Since: 0.14.0
+##
+[ 'query-version', {}, {}, 'VersionInfo' ]


(just picking on a patch that has a bit of schema in it)


If you want to see more of the schema in action 
http://repo.or.cz/w/qemu/aliguori.git/blob/refs/heads/glib:/qmp-schema.json


Something that can be added to the schema are default values for newly 
added parameters.  This way we can avoid an explosion of commands 
where adding an optional parameter suffices; should be easier for the 
user to pick the right command and easier for us to document and support.


Adding a parameter to a command, even if the schema specifies a default 
value, is going to break the C library ABI so it's something we should 
strongly discourage.


We definitely want to systematically document defaults but the question 
is whether that belongs in the documentation for the command or the 
schema itself.  Since a default doesn't affect the wire protocol in any 
way, shape, or form, I think there a pretty strong argument that it 
belongs in the documentation and not the schema.


gtk-doc typically uses a tag to identify defaults.  I think it's 
something like #default although that is purely a marking concept (the 
value isn't parsed out or anything).  Whether we'd want to automatically 
parse the value following the #default tag and change the internal 
signature is probably a discussion we can defer.  Since this only really 
should apply to structures, I suspect it's not a huge win.


Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 00/22] QAPI Round 1

[Anthony Liguori]

On 03/09/2011 07:55 AM, Avi Kivity wrote:

On 03/09/2011 03:51 PM, Anthony Liguori wrote:
{ execute: 'write-keystore' arguments: { 'key': 'foo', 'value': 
'bar' } }


This is coming from the guest? 


Yes.



So what about:

{ 'KeyStore': { '*foo': 'str', '*otherkey': 'str' } }
[ 'guest-write-keystore', { 'keystore': 'KeyStore' }, 'none' ]

{ 'GUEST_UPDATE_KEYSTORE': { 'keys': [ 'str' ] } }


(why the CAPS?)


Sad to say, but legacy.  That's how we do events today.



In this model, the key store is actually stored in the guest.  If the 
guest wants the latest version of a value, it sends an event to get 
keys updated.  The host can update keys at any point in time.


This survives live migration, but not guest reboots or crashes.


Right, management tool (or QEMU) needs to keep a copy if it's to survive 
reset.


But since we don't have a concrete use-case, I'm really just trying to 
show that we don't need bidirectional RPC for these types of things.


Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 19/22] qapi: add QMP put-event command

[Anthony Liguori]

On 03/09/2011 07:58 AM, Avi Kivity wrote:

On 03/09/2011 03:48 PM, Anthony Liguori wrote:

+[ 'put-event', {'tag': 'int'}, {}, 'none' ]


Why is tag an int?

+##

It's a handle so the type doesn't matter as long as I can make sure 
values are unique.  ints are easier to work with because they don't 
require memory allocation.


I think it's nicer for the client to use a string.  Instead of a 
global ID allocator, it can use unique IDs or unique prefixes + local 
IDs.  Should also aid a little in debugging.


handle's are opaque to clients.

I don't have a huge objection to using strings and it may make sense to 
do a prefix like you're suggesting.  I won't do that initially but since 
handles are opaque, we can make that change later.



  don't we use strings for command ids and similar?


id's can be any valid JSON value.

But a handle is not the same thing as an id.


Why not?

I hope handles are client-provided?


No, they are generated by the server.  It makes sense because really a 
handle is a marshalled version of a signal.  The signal accessor looks 
something like this:


{ 'BLOCK_IO_ERROR': { 'device': 'str', 'action': 'str', 'operation': 
'str' } }

[ 'get-block-io-error-event': {}, 'BLOCK_IO_ERROR' }

The way we marshal a 'BLOCK_IO_ERROR' type is by generating a unique 
handle and returning that.


While this looks like an int on the wire, at both the server and libqmp 
level, it looks like a BlockIoErrorEvent object.  So in QEMU:


BlockIoErrorEvent *qmp_get_block_io_error_event(Error **errp)
{
}

And in libqmp:

BlockIoErrorEvent *libqmp_get_block_io_error_event(QmpSession *sess, 
Error **errp)

{
}


Also could be better named, disconnect-event or unlisten-event.


I was going for symmetry with the signal accessors which are 
typically in the format 'get-block-io-error-event'.


Maybe it would be better to do 'connect-block-io-error-event' and 
'disconnect-event'?


Yes.

But I'm confused, do we have a per-event command on the wire?  Or just 
C stubs?


Ignoring default events, you'll never see an event until you execute a 
signal accessor function.  When you execute this function, you will 
start receiving the events and those events will carry a tag containing 
the handle returned by the signal accessor.


Within libqmp, any time you execute a signal accessor, a new signal 
object is created of the appropriate type.  When that object is 
destroyed, you send a put-event to stop receiving the signal.


When you connect to a signal object (via libqmp), you don't execute the 
signal accessor because the object is already receiving the signal.


Default events (which exist to preserve compatibility) are a set of 
events that are automatically connected to after qmp_capabilities is 
executed.  Because these connections are implicit, they arrive without a 
handle in the event object.


At this point, libqmp just ignores default events.  In the future, I'd 
like to add a command that can be executed before qmp_capabilities that 
will avoid connecting to default events.


Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH v2] Add qcow2 documentation

[Stefan Hajnoczi]

On Wed, Mar 9, 2011 at 1:55 PM, Kevin Wolf  wrote:
> Am 09.03.2011 14:08, schrieb Stefan Hajnoczi:
>> On Wed, Mar 9, 2011 at 12:30 PM, Kevin Wolf  wrote:
>>> This adds a description of the qcow2 file format to the docs/ directory.
>>> Besides documenting what's there, which is never wrong, the document should
>>> provide a good basis for the discussion of format extensions (called "qcow3"
>>> in previous discussions)
>>>
>>> Signed-off-by: Kevin Wolf 
>>> ---
>>>  docs/specs/qcow2.txt |  233 
>>> ++
>>>  1 files changed, 233 insertions(+), 0 deletions(-)
>>>  create mode 100644 docs/specs/qcow2.txt
>>
>> QCOW2_EXT_MAGIC_BACKING_FORMAT is not documented.  Did you decide that
>> is a QEMU internal extension and you don't want it documented?
>
> I just completely forgot to include header extensions. Though I think
> the allowed values for the backing file format are implementation specific.
>
> Anything else to consider for v3?

I have to admit I haven't review most of the text fully yet.  I will
later today though.

Stefan

Re: [Qemu-devel] Re: [RFC][PATCH v7 01/16] Move code related to fd handlers into utility functions

[Anthony Liguori]

On 03/09/2011 07:58 AM, Paolo Bonzini wrote:

On 03/07/2011 09:10 PM, Michael Roth wrote:

This allows us to implement an i/o loop outside of vl.c that can
interact with objects that use qemu_set_fd_handler()


I must say I really dislike the patches 1..3.  It's _really_ getting 
the QEMU NIH worse.  While it is not really possible to get a new 
shiny mainloop infrastructure in QEMU like snapping fingers (and I'm 
not sure the glib mainloop will ever happen there), there is no reason 
not to adopt glib's infrastructure in virtagent.


I'm 90% in agreement with you but in terms of delivering a Windows guest 
agent, instead of just having an exe, we're now talking about quite a 
few extra DLLs.  It's not a huge problem and probably makes a ton of 
sense if virt-agent ever adopts more sophisticated functionality but I 
wanted to at least raise this point.


Regards,

Anthony Liguori

  While cooperation between QEMU and virtagent is close, it is IMHO a 
substantially separate project that can afford starting from a clean 
slate.


If anybody disagrees, I'd be happy to hear their opinion anyway!

I'm sorry I'm saying this only now and I've been ignoring this series 
until v7.


Paolo

[Qemu-devel] fdc: refactor device creation causes guest kernel panic

[Stefan Hajnoczi]

The following kernel panic occurs when the RHEL6 installer starts on
qemu.git/master:

BUG: unable to handle kernel NULL pointer dereference at (null)
IP: [] floppy_ready+0xfb/0x730 [floppy]

For full details see http://pastebin.com/SYE5A6LA.

git-bisect revealed that the following commit causes this panic:

commit 63ffb564dca94f8bda01ed6d209784104630a4d2
Author: Blue Swirl 
Date:   Sat Feb 5 16:32:23 2011 +

fdc: refactor device creation

Turn fdc_init_isa into an inline function.

Get floppy geometry directly from the drives.

Don't expose FDCtrl.

Signed-off-by: Blue Swirl 

The CMOS value at 0x10 has changed from 0x00 to 0x40 but I have not
located the root cause of the problem.

Blue Swirl: Any thoughts on this bug?

Stefan

Re: [Qemu-devel] [PATCH 14/22] qapi: add query-version QMP command

[Avi Kivity]

On 03/09/2011 04:11 PM, Anthony Liguori wrote:

(just picking on a patch that has a bit of schema in it)



If you want to see more of the schema in action 
http://repo.or.cz/w/qemu/aliguori.git/blob/refs/heads/glib:/qmp-schema.json


Thanks.  Something a little worrying is the reliance on capitalization 
and punctuation ( {} vs [] ) do distinguish among the different types of 
declarations.  It's not immediately clear if something is a type, event, 
or command.


We could do

[

{ 'type': 'MyType', fields: [['a', 'str'], ['b', 'int'], ['c', 
'AnotherType']] }

{ 'event': 'MY_EVENT', 'arguments': [ ... ] }
{ 'command': 'my-command', 'arguments': [ ... ], 'return': ... }

]

which leaves us room for additional metainformation.

The concern is more about non-qemu consumers of the schema.



Something that can be added to the schema are default values for 
newly added parameters.  This way we can avoid an explosion of 
commands where adding an optional parameter suffices; should be 
easier for the user to pick the right command and easier for us to 
document and support.


Adding a parameter to a command, even if the schema specifies a 
default value, is going to break the C library ABI so it's something 
we should strongly discourage.


We could add compatibility signatures when we extend a command:


{ 'command': 'x', arguments: [['a', 'str']], return: ...,
   'signatures': { 'x': [], 'x2': ['a'] } }

That lets the wire protocol extend x without introducing a new command, 
but for libqmp it adds a new x2() API with the new parameter.




We definitely want to systematically document defaults but the 
question is whether that belongs in the documentation for the command 
or the schema itself.  Since a default doesn't affect the wire 
protocol in any way, shape, or form, I think there a pretty strong 
argument that it belongs in the documentation and not the schema.


Agree.



gtk-doc typically uses a tag to identify defaults.  I think it's 
something like #default although that is purely a marking concept (the 
value isn't parsed out or anything).  Whether we'd want to 
automatically parse the value following the #default tag and change 
the internal signature is probably a discussion we can defer.  Since 
this only really should apply to structures, I suspect it's not a huge 
win.


--
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [RFC][PATCH v7 01/16] Move code related to fd handlers into utility functions

[Paolo Bonzini]

On 03/09/2011 03:11 PM, Michael Roth wrote:


In the context of virtagent I would agree. The only complication there
being that a large part of the event-driven code (the async read/write
handlers for instance) is shared between virtagent and the host.


What exactly?  The dependencies in 16/16 give:

qemu-tool.o qemu-error.o qemu-sockets.c $(oslib-obj-y) $(trace-obj-y)
$(block-obj-y) $(qobject-obj-y) $(version-obj-y) qemu-timer-common.o
qemu-timer.o

Compared to other tools, only qemu-sockets.c is added (and timers); 
overall it is quite self contained and interfaces well with glib's 
GIOChannels, which provide qemu_set_fd_handler-equivalent functionality.


In addition, qemu iohandlers have a lot of unwritten assumptions, for 
example on Win32 they only work with sockets and not other kinds of file 
descriptors.


Paolo

[Qemu-devel] Re: [RFC][PATCH v7 01/16] Move code related to fd handlers into utility functions

[Anthony Liguori]

On 03/09/2011 07:58 AM, Paolo Bonzini wrote:

On 03/07/2011 09:10 PM, Michael Roth wrote:

This allows us to implement an i/o loop outside of vl.c that can
interact with objects that use qemu_set_fd_handler()


I must say I really dislike the patches 1..3.  It's _really_ getting 
the QEMU NIH worse.  While it is not really possible to get a new 
shiny mainloop infrastructure in QEMU like snapping fingers (and I'm 
not sure the glib mainloop will ever happen there


While it's not at the immediate top at my MUST DO list, it's still 
pretty high FWIW.  I think the benefits are huge because it means we can 
refactor things like the VNC server to just interact with glib which 
means it can become generally useful outside of QEMU.


Regards,

Anthony Liguori

), there is no reason not to adopt glib's infrastructure in 
virtagent.  While cooperation between QEMU and virtagent is close, it 
is IMHO a substantially separate project that can afford starting from 
a clean slate.


If anybody disagrees, I'd be happy to hear their opinion anyway!

I'm sorry I'm saying this only now and I've been ignoring this series 
until v7.


Paolo

[Qemu-devel] Re: [PATCH]hw/xen_disk: aio_inflight not released in handling ioreq when nr_segments==0

[Stefano Stabellini]

On Wed, 9 Mar 2011, Feiran Zheng wrote:
> In hw/xen_disk.c, async writing ioreq is leaked when
> ioreq->req.nr_segments==0, because `aio_inflight` flag is not released
> properly (skipped by misplaced "break").
> 
> Signed-off-by: Feiran Zheng 

Acked-by: Stefano Stabellini 

Re: [Qemu-devel] [PATCH 14/22] qapi: add query-version QMP command

[Anthony Liguori]

On 03/09/2011 08:37 AM, Avi Kivity wrote:

On 03/09/2011 04:11 PM, Anthony Liguori wrote:

(just picking on a patch that has a bit of schema in it)



If you want to see more of the schema in action 
http://repo.or.cz/w/qemu/aliguori.git/blob/refs/heads/glib:/qmp-schema.json


Thanks.  Something a little worrying is the reliance on capitalization 
and punctuation ( {} vs [] ) do distinguish among the different types 
of declarations.  It's not immediately clear if something is a type, 
event, or command.


We could do

[

{ 'type': 'MyType', fields: [['a', 'str'], ['b', 'int'], ['c', 
'AnotherType']] }

{ 'event': 'MY_EVENT', 'arguments': [ ... ] }
{ 'command': 'my-command', 'arguments': [ ... ], 'return': ... }

]

which leaves us room for additional metainformation.

The concern is more about non-qemu consumers of the schema.


Yeah, I dislike it too and I've been leaning towards changing it.

My preference would be:

{ 'type': 'MyType', 'fields': { 'a': 'str', 'b': 'int', 'c': 
'AnotherType' } }

{ 'event': 'MY_EVENT', 'arguments': {...} }
{ 'command': 'my-command', 'arguments': {...}, 'returns': 'int' }

I do prefer the dictionary syntax for arguments over a list because a 
list implies order.  Plus I think the syntax is just awkward and a whole 
lot easier to get wrong (too many/few elements in list).


I don't think I want to make this sort of change just yet.  Also note 
that the schema that will be exposed over the wire is not directly 
related to the schema we use for code generation.


Something that can be added to the schema are default values for 
newly added parameters.  This way we can avoid an explosion of 
commands where adding an optional parameter suffices; should be 
easier for the user to pick the right command and easier for us to 
document and support.


Adding a parameter to a command, even if the schema specifies a 
default value, is going to break the C library ABI so it's something 
we should strongly discourage.


We could add compatibility signatures when we extend a command:


{ 'command': 'x', arguments: [['a', 'str']], return: ...,
   'signatures': { 'x': [], 'x2': ['a'] } }

That lets the wire protocol extend x without introducing a new 
command, but for libqmp it adds a new x2() API with the new parameter.


I'd rather just not add arguments.

Treating QMP as a C API makes it easier for us to maintain compatibility 
both internally and externally.


Regards,

Anthony Liguori

[Qemu-devel] Re: [RFC][PATCH v7 01/16] Move code related to fd handlers into utility functions

[Paolo Bonzini]

On 03/09/2011 03:40 PM, Anthony Liguori wrote:


I must say I really dislike the patches 1..3.  It's _really_ getting
the QEMU NIH worse.  While it is not really possible to get a new
shiny mainloop infrastructure in QEMU like snapping fingers (and I'm
not sure the glib mainloop will ever happen there


While it's not at the immediate top at my MUST DO list, it's still
pretty high FWIW.  I think the benefits are huge because it means we can
refactor things like the VNC server to just interact with glib which
means it can become generally useful outside of QEMU.


I actually agree, but there are a lot of cleanups to do to the code 
before it becomes viable.  I would be surprised to see it before 0.17 
say (maybe a pleasant surprise, but still).


In any case, introducing more dependencies from the tools to core QEMU 
would mean needing wrappers over wrappers over wrappers when QEMU itself 
is refactored.


Perhaps for virtagent something like libnih would be more appropriate? 
Not sure about its Win32 portability though.


Paolo

Re: [Qemu-devel] Re: [PATCH]hw/xen_disk: aio_inflight not released in handling ioreq when nr_segments==0

[Stefan Hajnoczi]

On Wed, Mar 9, 2011 at 2:42 PM, Stefano Stabellini
 wrote:
> On Wed, 9 Mar 2011, Feiran Zheng wrote:
>> In hw/xen_disk.c, async writing ioreq is leaked when
>> ioreq->req.nr_segments==0, because `aio_inflight` flag is not released
>> properly (skipped by misplaced "break").
>>
>> Signed-off-by: Feiran Zheng 
>
> Acked-by: Stefano Stabellini 

Included Kevin so this can be taken up into the block subsystem branch.

Stefan

[Qemu-devel] Re: [RFC][PATCH v7 01/16] Move code related to fd handlers into utility functions

[Michael Roth]

On 03/09/2011 08:38 AM, Paolo Bonzini wrote:

On 03/09/2011 03:11 PM, Michael Roth wrote:


In the context of virtagent I would agree. The only complication there
being that a large part of the event-driven code (the async read/write
handlers for instance) is shared between virtagent and the host.


What exactly? The dependencies in 16/16 give:

qemu-tool.o qemu-error.o qemu-sockets.c $(oslib-obj-y) $(trace-obj-y)
$(block-obj-y) $(qobject-obj-y) $(version-obj-y) qemu-timer-common.o
qemu-timer.o


These objs: virtagent.o virtagent-server.o virtagent-common.o 
virtagent-transport.o virtagent-manager.o


Are shared by qemu and qemu-va. virtagent.o uses the common timer 
infrastructure introduced in patch 3, and 
virtagent-transport/virtagent-common use the iohandler stuff from patch 1/2.


On the host, qemu's event loop drives them, and on the guest, qemu-va's 
event loop drives them.


Not sure what level of sharing we can maintain with 2 different event 
loops. I'm sure it's doable, just not sure what it would end up looking 
like.


I should note that initially all the qemu_set_fd_handler() stuff was 
wrapped to provide compatibility between separate event loop 
implementations in qemu/qemu-va. Sharing the event loop code was a 
widely-held consensus from earlier reviews. I'm not sure glib is so nice 
that it's worth back-peddling on that. And if we do eventually make 
qemu's event loop glib-based, consumers of the common code here would 
get migrated over for free.




Compared to other tools, only qemu-sockets.c is added (and timers);
overall it is quite self contained and interfaces well with glib's
GIOChannels, which provide qemu_set_fd_handler-equivalent functionality.

In addition, qemu iohandlers have a lot of unwritten assumptions, for
example on Win32 they only work with sockets and not other kinds of file
descriptors.


Hmm, that could be a problem... It seems like a more general one though, 
that might benefit consumers other than virtagent. So if this is 
addressed at some point, consumers of the common infrastructure proposed 
here would all benefit.




Paolo

[Qemu-devel] Re: [RFC][PATCH v7 01/16] Move code related to fd handlers into utility functions

[Paolo Bonzini]

On 03/09/2011 04:01 PM, Michael Roth wrote:


These objs: virtagent.o virtagent-server.o virtagent-common.o
virtagent-transport.o virtagent-manager.o

Are shared by qemu and qemu-va.


Okay, that's what I missed.  Then I guess it's a pity but there's a good
reason.


It seems like a more general one though, that might benefit consumers
other than virtagent. So if this is addressed at some point,
consumers of the common infrastructure proposed here would all
benefit.


I doubt, Win32 sockets are almost unusable (QEMU does "select" on them 
when it has an event from something else) and chardevs use a separate 
polling mechanism.


Paolo

[Qemu-devel] [PATCH] QMP: add snapshot_blkdev_sync command

[Jes . Sorensen]

From: Jes Sorensen 

Add QMP bits for snapshot_blkdev_sync command. This is the same as
snapshot_blkdev in the human monitor, but added _sync to the name to
make it explicit that the command is synchronous and leave space for a
future async version.

Signed-off-by: Jes Sorensen 
---
 qmp-commands.hx |   19 +++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/qmp-commands.hx b/qmp-commands.hx
index 9d3cc31..e32187e 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -667,6 +667,25 @@ Example:
 EQMP
 
 {
+.name   = "snapshot_blkdev_sync",
+.args_type  = "device:B,snapshot_file:s?,format:s?",
+.params = "device [new-image-file] [format]",
+.help   = "initiates a live snapshot\n\t\t\t"
+  "of device. If a new image file is specified, 
the\n\t\t\t"
+  "new image file will become the new root image.\n\t\t\t"
+  "If format is specified, the snapshot file will\n\t\t\t"
+  "be created in that format. Otherwise the\n\t\t\t"
+  "snapshot will be internal! (currently unsupported)",
+.user_print = monitor_user_noop,
+.mhandler.cmd_new = do_snapshot_blkdev,
+},
+
+SQMP
+Synchronous snapshot of block device, using snapshot file as target
+if provided. 
+EQMP
+
+{
 .name   = "balloon",
 .args_type  = "value:M",
 .params = "target",
-- 
1.7.4

[Qemu-devel] Re: [RFC][PATCH v7 01/16] Move code related to fd handlers into utility functions

[Anthony Liguori]

On 03/09/2011 08:45 AM, Paolo Bonzini wrote:

On 03/09/2011 03:40 PM, Anthony Liguori wrote:


I must say I really dislike the patches 1..3.  It's _really_ getting
the QEMU NIH worse.  While it is not really possible to get a new
shiny mainloop infrastructure in QEMU like snapping fingers (and I'm
not sure the glib mainloop will ever happen there


While it's not at the immediate top at my MUST DO list, it's still
pretty high FWIW.  I think the benefits are huge because it means we can
refactor things like the VNC server to just interact with glib which
means it can become generally useful outside of QEMU.


I actually agree, but there are a lot of cleanups to do to the code 
before it becomes viable.  I would be surprised to see it before 0.17 
say (maybe a pleasant surprise, but still).


In any case, introducing more dependencies from the tools to core QEMU 
would mean needing wrappers over wrappers over wrappers when QEMU 
itself is refactored.


Perhaps for virtagent something like libnih would be more appropriate? 
Not sure about its Win32 portability though.


If we do any lib, it should be glib.  I'm just playing devil's advocate 
here in pointing out that adding DLL dependences on Windows is a bit 
painful although probably unavoidable.


Regards,

Anthony Liguori


Paolo

Re: [Qemu-devel] [PATCH] QMP: add snapshot_blkdev_sync command

[Anthony Liguori]

On 03/09/2011 09:37 AM, jes.soren...@redhat.com wrote:

From: Jes Sorensen

Add QMP bits for snapshot_blkdev_sync command. This is the same as
snapshot_blkdev in the human monitor, but added _sync to the name to
make it explicit that the command is synchronous and leave space for a
future async version.

Signed-off-by: Jes Sorensen
---
  qmp-commands.hx |   19 +++
  1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/qmp-commands.hx b/qmp-commands.hx
index 9d3cc31..e32187e 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -667,6 +667,25 @@ Example:
  EQMP

  {
+.name   = "snapshot_blkdev_sync",
+.args_type  = "device:B,snapshot_file:s?,format:s?",
+.params = "device [new-image-file] [format]",
+.help   = "initiates a live snapshot\n\t\t\t"
+  "of device. If a new image file is specified, 
the\n\t\t\t"
+  "new image file will become the new root image.\n\t\t\t"
+  "If format is specified, the snapshot file will\n\t\t\t"
+  "be created in that format. Otherwise the\n\t\t\t"
+  "snapshot will be internal! (currently unsupported)",
+.user_print = monitor_user_noop,
+.mhandler.cmd_new = do_snapshot_blkdev,
+},
+
+SQMP
+Synchronous snapshot of block device, using snapshot file as target
+if provided.


Please document the error semantics.

The documentation in .help is discarded for QMP.  You should put the 
docs in the SQMP section.


Also, QMP should use '-' instead of '_'.  We should also try to follow 
the form 'noun'-'verb' so the name would be better as 'blkdev-snapshot-sync'


I'm not sure blkdev is the right prefix.  Kevin, what are your thoughts 
here?  Does 'blkdev' make sense for any command operating on a block 
device (that is, a qdev device that happens to have a block drive, not 
the same thing as -blockdev that we've discussed in the past).


Regards,

Anthony Liguori


+EQMP
+
+{
  .name   = "balloon",
  .args_type  = "value:M",
  .params = "target",

Re: [Qemu-devel] [PATCH]hw/xen_disk: aio_inflight not released in handling ioreq when nr_segments==0

[Kevin Wolf]

Am 09.03.2011 14:19, schrieb Feiran Zheng:
> In hw/xen_disk.c, async writing ioreq is leaked when
> ioreq->req.nr_segments==0, because `aio_inflight` flag is not released
> properly (skipped by misplaced "break").
> 
> Signed-off-by: Feiran Zheng 

Thanks, applied to the block branch.

Kevin