Patchwork [RFC,v6,03/23] Make qemu timers available for tools

login
register
mail settings
Submitter Michael Roth
Date Jan. 17, 2011, 1:14 p.m.
Message ID <1295270117-24760-4-git-send-email-mdroth@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/79161/
State New
Headers show

Comments

Michael Roth - Jan. 17, 2011, 1:14 p.m.
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:

init_clocks();
configure_alarms("dynticks");
if (init_timer_alarm() < 0) {
    errx(EXIT_FAILURE, "could not initialize alarm timer");
}

while (running) {
    //do work
    qemu_run_all_timers();
}

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 cpus.c      |   83 +++++++---------------------------------------------
 qemu-ioh.c  |   93 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-ioh.h  |    9 ++++++
 qemu-tool.c |   92 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 qemu-tool.h |   26 ++++++++++++++++
 5 files changed, 229 insertions(+), 74 deletions(-)
 create mode 100644 qemu-tool.h
Jes Sorensen - Jan. 21, 2011, 4:30 p.m.
On 01/17/11 14:14, Michael Roth wrote:
> diff --git a/qemu-ioh.c b/qemu-ioh.c
> index cc71470..001e7a2 100644
> --- a/qemu-ioh.c
> +++ b/qemu-ioh.c
> @@ -22,7 +22,11 @@
>   * THE SOFTWARE.
>   */
>  #include "qemu-ioh.h"
> +#include "qemu-char.h"
>  #include "qlist.h"
> +#ifdef CONFIG_EVENTFD
> +#include <sys/eventfd.h>
> +#endif
>  
>  /* XXX: fd_read_poll should be suppressed, but an API change is
>     necessary in the character devices to suppress fd_can_read(). */
> @@ -113,3 +117,92 @@ void qemu_process_fd_handlers2(void *ioh_record_list, const fd_set *rfds,
>          }
>      }
>  }
> +
> +#ifndef _WIN32
> +void iothread_event_increment(int *io_thread_fd)

Please split the WIN32 stuff into it's own file, similar to oslib-posix
and oslib-win32.c etc.

> diff --git a/qemu-ioh.h b/qemu-ioh.h
> index 7c6e833..2c714a9 100644
> --- a/qemu-ioh.h
> +++ b/qemu-ioh.h
> @@ -31,4 +31,13 @@ void qemu_get_fdset2(void *ioh_record_list, int *nfds, fd_set *rfds,
>  void qemu_process_fd_handlers2(void *ioh_record_list, const fd_set *rfds,
>                                 const fd_set *wfds, const fd_set *xfds);
>  
> +
> +#ifndef _WIN32
> +void iothread_event_increment(int *io_thread_fd);
> +int iothread_event_init(int *io_thread_fd);
> +#else
> +int win32_event_init(HANDLE *qemu_event_handle);
> +void win32_event_increment(HANDLE *qemu_event_handle);
> +#endif

Can you not do something slightly nicer that allows for those to be the
same prototype for all users? Like define a event_handle_t?

> +
> +#ifndef _WIN32
> +static int io_thread_fd = -1;

Needs splitting into separate files too.

> diff --git a/qemu-tool.h b/qemu-tool.h
> new file mode 100644
> index 0000000..fd693cf
> --- /dev/null
> +++ b/qemu-tool.h
> @@ -0,0 +1,26 @@
> +#ifndef QEMU_TOOL_H
> +#define QEMU_TOOL_H
> +
> +#include "qemu-common.h"
> +
> +#ifdef CONFIG_EVENTFD
> +#include <sys/eventfd.h>
> +#endif
> +
> +typedef void VMStateDescription;
> +typedef int VMStateInfo;
> +
> +#ifndef _WIN32
> +void qemu_event_increment(void);
> +int qemu_event_init(void);
> +#else
> +int qemu_event_init(void);
> +void qemu_event_increment(void);
> +#endif

No matter how long I stare at those prototypes, I fail to see the
difference between the win32 and the posix version :)

Cheers,
Jes
Michael Roth - Jan. 21, 2011, 5:26 p.m.
On 01/21/2011 10:30 AM, Jes Sorensen wrote:
> On 01/17/11 14:14, Michael Roth wrote:
>> diff --git a/qemu-ioh.c b/qemu-ioh.c
>> index cc71470..001e7a2 100644
>> --- a/qemu-ioh.c
>> +++ b/qemu-ioh.c
>> @@ -22,7 +22,11 @@
>>    * THE SOFTWARE.
>>    */
>>   #include "qemu-ioh.h"
>> +#include "qemu-char.h"
>>   #include "qlist.h"
>> +#ifdef CONFIG_EVENTFD
>> +#include<sys/eventfd.h>
>> +#endif
>>
>>   /* XXX: fd_read_poll should be suppressed, but an API change is
>>      necessary in the character devices to suppress fd_can_read(). */
>> @@ -113,3 +117,92 @@ void qemu_process_fd_handlers2(void *ioh_record_list, const fd_set *rfds,
>>           }
>>       }
>>   }
>> +
>> +#ifndef _WIN32
>> +void iothread_event_increment(int *io_thread_fd)
>
> Please split the WIN32 stuff into it's own file, similar to oslib-posix
> and oslib-win32.c etc.

Will look into this, but qemu-ioh.c has common code too so we'd end up 
with qemu-ioh/qemu-ioh-posix/qemu-ioh-win2.c. We could alternatively 
have a "#ifndef _WIN32" around functions in qemu-ioh.c that would be 
replaced by win32-specific ones from qemu-ioh-win32. No strong 
preference either way, but sometimes I find navigating across too many 
files more annoying that #ifdefs, and there's not a whole lot in these.

>
>> diff --git a/qemu-ioh.h b/qemu-ioh.h
>> index 7c6e833..2c714a9 100644
>> --- a/qemu-ioh.h
>> +++ b/qemu-ioh.h
>> @@ -31,4 +31,13 @@ void qemu_get_fdset2(void *ioh_record_list, int *nfds, fd_set *rfds,
>>   void qemu_process_fd_handlers2(void *ioh_record_list, const fd_set *rfds,
>>                                  const fd_set *wfds, const fd_set *xfds);
>>
>> +
>> +#ifndef _WIN32
>> +void iothread_event_increment(int *io_thread_fd);
>> +int iothread_event_init(int *io_thread_fd);
>> +#else
>> +int win32_event_init(HANDLE *qemu_event_handle);
>> +void win32_event_increment(HANDLE *qemu_event_handle);
>> +#endif
>
> Can you not do something slightly nicer that allows for those to be the
> same prototype for all users? Like define a event_handle_t?
>

Don't see why not.

>> +
>> +#ifndef _WIN32
>> +static int io_thread_fd = -1;
>
> Needs splitting into separate files too.
>
>> diff --git a/qemu-tool.h b/qemu-tool.h
>> new file mode 100644
>> index 0000000..fd693cf
>> --- /dev/null
>> +++ b/qemu-tool.h
>> @@ -0,0 +1,26 @@
>> +#ifndef QEMU_TOOL_H
>> +#define QEMU_TOOL_H
>> +
>> +#include "qemu-common.h"
>> +
>> +#ifdef CONFIG_EVENTFD
>> +#include<sys/eventfd.h>
>> +#endif
>> +
>> +typedef void VMStateDescription;
>> +typedef int VMStateInfo;
>> +
>> +#ifndef _WIN32
>> +void qemu_event_increment(void);
>> +int qemu_event_init(void);
>> +#else
>> +int qemu_event_init(void);
>> +void qemu_event_increment(void);
>> +#endif
>
> No matter how long I stare at those prototypes, I fail to see the
> difference between the win32 and the posix version :)

Heh, the ordering of course! :) Not sure how I missed this one.

The patch is pretty rough in general, I'll see what I can do about 
cleaning things up a bit.

>
> Cheers,
> Jes
Jes Sorensen - Jan. 24, 2011, 7:56 a.m.
On 01/21/11 18:26, Michael Roth wrote:
> On 01/21/2011 10:30 AM, Jes Sorensen wrote:
>> On 01/17/11 14:14, Michael Roth wrote:
>>> diff --git a/qemu-ioh.c b/qemu-ioh.c
>>> index cc71470..001e7a2 100644
>>> --- a/qemu-ioh.c
>>> +++ b/qemu-ioh.c
>>> @@ -22,7 +22,11 @@
>>>    * THE SOFTWARE.
>>>    */
>>>   #include "qemu-ioh.h"
>>> +#include "qemu-char.h"
>>>   #include "qlist.h"
>>> +#ifdef CONFIG_EVENTFD
>>> +#include<sys/eventfd.h>
>>> +#endif
>>>
>>>   /* XXX: fd_read_poll should be suppressed, but an API change is
>>>      necessary in the character devices to suppress fd_can_read(). */
>>> @@ -113,3 +117,92 @@ void qemu_process_fd_handlers2(void
>>> *ioh_record_list, const fd_set *rfds,
>>>           }
>>>       }
>>>   }
>>> +
>>> +#ifndef _WIN32
>>> +void iothread_event_increment(int *io_thread_fd)
>>
>> Please split the WIN32 stuff into it's own file, similar to oslib-posix
>> and oslib-win32.c etc.
> 
> Will look into this, but qemu-ioh.c has common code too so we'd end up
> with qemu-ioh/qemu-ioh-posix/qemu-ioh-win2.c. We could alternatively
> have a "#ifndef _WIN32" around functions in qemu-ioh.c that would be
> replaced by win32-specific ones from qemu-ioh-win32. No strong
> preference either way, but sometimes I find navigating across too many
> files more annoying that #ifdefs, and there's not a whole lot in these.

No problem having the three files - it is far better than having
#ifdefs. Having the #ifndef that is overloaded by a win32 specific file
is bad, it will make it very confusing for anyone reading the code.

Cheers,
Jes

Patch

diff --git a/cpus.c b/cpus.c
index 0309189..2f1adf6 100644
--- a/cpus.c
+++ b/cpus.c
@@ -163,90 +163,24 @@  static int io_thread_fd = -1;
 
 static void qemu_event_increment(void)
 {
-    /* Write 8 bytes to be compatible with eventfd.  */
-    static const uint64_t val = 1;
-    ssize_t ret;
-
-    if (io_thread_fd == -1)
-        return;
-
-    do {
-        ret = write(io_thread_fd, &val, sizeof(val));
-    } while (ret < 0 && errno == EINTR);
-
-    /* EAGAIN is fine, a read must be pending.  */
-    if (ret < 0 && errno != EAGAIN) {
-        fprintf(stderr, "qemu_event_increment: write() filed: %s\n",
-                strerror(errno));
-        exit (1);
-    }
-}
-
-static void qemu_event_read(void *opaque)
-{
-    int fd = (unsigned long)opaque;
-    ssize_t len;
-    char buffer[512];
-
-    /* Drain the notify pipe.  For eventfd, only 8 bytes will be read.  */
-    do {
-        len = read(fd, buffer, sizeof(buffer));
-    } while ((len == -1 && errno == EINTR) || len == sizeof(buffer));
+    return iothread_event_increment(&io_thread_fd);
 }
 
 static int qemu_event_init(void)
 {
-    int err;
-    int fds[2];
-
-    err = qemu_eventfd(fds);
-    if (err == -1)
-        return -errno;
-
-    err = fcntl_setfl(fds[0], O_NONBLOCK);
-    if (err < 0)
-        goto fail;
-
-    err = fcntl_setfl(fds[1], O_NONBLOCK);
-    if (err < 0)
-        goto fail;
-
-    qemu_set_fd_handler2(fds[0], NULL, qemu_event_read, NULL,
-                         (void *)(unsigned long)fds[0]);
-
-    io_thread_fd = fds[1];
-    return 0;
-
-fail:
-    close(fds[0]);
-    close(fds[1]);
-    return err;
+    return iothread_event_init(&io_thread_fd);
 }
 #else
 HANDLE qemu_event_handle;
 
-static void dummy_event_handler(void *opaque)
-{
-}
-
 static int qemu_event_init(void)
 {
-    qemu_event_handle = CreateEvent(NULL, FALSE, FALSE, NULL);
-    if (!qemu_event_handle) {
-        fprintf(stderr, "Failed CreateEvent: %ld\n", GetLastError());
-        return -1;
-    }
-    qemu_add_wait_object(qemu_event_handle, dummy_event_handler, NULL);
-    return 0;
+    return win32_event_init(&qemu_event_handle);
 }
 
 static void qemu_event_increment(void)
 {
-    if (!SetEvent(qemu_event_handle)) {
-        fprintf(stderr, "qemu_event_increment: SetEvent failed: %ld\n",
-                GetLastError());
-        exit (1);
-    }
+    win32_event_increment(&qemu_event_handle);
 }
 #endif
 
@@ -296,11 +230,10 @@  void qemu_cpu_kick(void *env)
     return;
 }
 
-void qemu_notify_event(void)
+static void qemu_stop_all_vcpus(void)
 {
     CPUState *env = cpu_single_env;
 
-    qemu_event_increment ();
     if (env) {
         cpu_exit(env);
     }
@@ -309,6 +242,12 @@  void qemu_notify_event(void)
     }
 }
 
+void qemu_notify_event(void)
+{
+    qemu_event_increment();
+    qemu_stop_all_vcpus();
+}
+
 void qemu_mutex_lock_iothread(void) {}
 void qemu_mutex_unlock_iothread(void) {}
 
diff --git a/qemu-ioh.c b/qemu-ioh.c
index cc71470..001e7a2 100644
--- a/qemu-ioh.c
+++ b/qemu-ioh.c
@@ -22,7 +22,11 @@ 
  * THE SOFTWARE.
  */
 #include "qemu-ioh.h"
+#include "qemu-char.h"
 #include "qlist.h"
+#ifdef CONFIG_EVENTFD
+#include <sys/eventfd.h>
+#endif
 
 /* XXX: fd_read_poll should be suppressed, but an API change is
    necessary in the character devices to suppress fd_can_read(). */
@@ -113,3 +117,92 @@  void qemu_process_fd_handlers2(void *ioh_record_list, const fd_set *rfds,
         }
     }
 }
+
+#ifndef _WIN32
+void iothread_event_increment(int *io_thread_fd)
+{
+    /* Write 8 bytes to be compatible with eventfd.  */
+    static const uint64_t val = 1;
+    ssize_t ret;
+
+    if (*io_thread_fd == -1)
+        return;
+
+    do {
+        ret = write(*io_thread_fd, &val, sizeof(val));
+    } while (ret < 0 && errno == EINTR);
+
+    /* EAGAIN is fine, a read must be pending.  */
+    if (ret < 0 && errno != EAGAIN) {
+        fprintf(stderr, "qemu_event_increment: write() filed: %s\n",
+                strerror(errno));
+        exit (1);
+    }
+}
+
+static void qemu_event_read(void *opaque)
+{
+    int fd = (unsigned long)opaque;
+    ssize_t len;
+    char buffer[512];
+
+    /* Drain the notify pipe.  For eventfd, only 8 bytes will be read.  */
+    do {
+        len = read(fd, buffer, sizeof(buffer));
+    } while (len == -1 && errno == EINTR);
+}
+
+
+int iothread_event_init(int *io_thread_fd)
+{
+    int err;
+    int fds[2];
+
+    err = qemu_eventfd(fds);
+    if (err == -1)
+        return -errno;
+
+    err = fcntl_setfl(fds[0], O_NONBLOCK);
+    if (err < 0)
+        goto fail;
+
+    err = fcntl_setfl(fds[1], O_NONBLOCK);
+    if (err < 0)
+        goto fail;
+
+    qemu_set_fd_handler2(fds[0], NULL, qemu_event_read, NULL,
+                         (void *)(unsigned long)fds[0]);
+
+    *io_thread_fd = fds[1];
+    return 0;
+
+fail:
+    close(fds[0]);
+    close(fds[1]);
+    return err;
+}
+#else
+static void dummy_event_handler(void *opaque)
+{
+}
+
+int win32_event_init(HANDLE *qemu_event_handle)
+{
+    *qemu_event_handle = CreateEvent(NULL, FALSE, FALSE, NULL);
+    if (!qemu_event_handle) {
+        fprintf(stderr, "Failed CreateEvent: %ld\n", GetLastError());
+        return -1;
+    }
+    qemu_add_wait_object(*qemu_event_handle, dummy_event_handler, NULL);
+    return 0;
+}
+
+void win32_event_increment(HANDLE *qemu_event_handle)
+{
+    if (!SetEvent(*qemu_event_handle)) {
+        fprintf(stderr, "qemu_event_increment: SetEvent failed: %ld\n",
+                GetLastError());
+        exit (1);
+    }
+}
+#endif
diff --git a/qemu-ioh.h b/qemu-ioh.h
index 7c6e833..2c714a9 100644
--- a/qemu-ioh.h
+++ b/qemu-ioh.h
@@ -31,4 +31,13 @@  void qemu_get_fdset2(void *ioh_record_list, int *nfds, fd_set *rfds,
 void qemu_process_fd_handlers2(void *ioh_record_list, const fd_set *rfds,
                                const fd_set *wfds, const fd_set *xfds);
 
+
+#ifndef _WIN32
+void iothread_event_increment(int *io_thread_fd);
+int iothread_event_init(int *io_thread_fd);
+#else
+int win32_event_init(HANDLE *qemu_event_handle);
+void win32_event_increment(HANDLE *qemu_event_handle);
+#endif
+
 #endif
diff --git a/qemu-tool.c b/qemu-tool.c
index 78d3532..027ea31 100644
--- a/qemu-tool.c
+++ b/qemu-tool.c
@@ -12,6 +12,7 @@ 
  */
 
 #include "qemu-common.h"
+#include "qemu-tool.h"
 #include "monitor.h"
 #include "qemu-timer.h"
 #include "qemu-log.h"
@@ -19,12 +20,11 @@ 
 
 #include <sys/time.h>
 
-QEMUClock *rt_clock;
+QEMUClock *rtc_clock;
 
 FILE *logfile;
 static QLIST_HEAD(, IOHandlerRecord) io_handlers =
     QLIST_HEAD_INITIALIZER(io_handlers);
-
 struct QEMUBH
 {
     QEMUBHFunc *cb;
@@ -134,3 +134,91 @@  void qemu_process_fd_handlers(const fd_set *rfds, const fd_set *wfds,
 {
     return qemu_process_fd_handlers2(&io_handlers, rfds, wfds, xfds);
 }
+
+#ifndef _WIN32
+static int io_thread_fd = -1;
+
+void qemu_event_increment(void)
+{
+    return iothread_event_increment(&io_thread_fd);
+}
+
+int qemu_event_init(void)
+{
+    return iothread_event_init(&io_thread_fd);
+}
+#else
+HANDLE qemu_event_handle;
+
+int qemu_event_init(void)
+{
+    return win32_event_init(&qemu_event_handle);
+}
+
+void qemu_event_increment(void)
+{
+    win32_event_increment(&qemu_event_handle);
+}
+#endif
+
+void qemu_notify_event(void)
+{
+    qemu_event_increment ();
+}
+
+/*
+ * Creates an eventfd that looks like a pipe and has EFD_CLOEXEC set.
+ */
+int qemu_eventfd(int fds[2])
+{
+#ifdef CONFIG_EVENTFD
+    int ret;
+
+    ret = eventfd(0, 0);
+    if (ret >= 0) {
+        fds[0] = ret;
+        qemu_set_cloexec(ret);
+        if ((fds[1] = dup(ret)) == -1) {
+            close(ret);
+            return -1;
+        }
+        qemu_set_cloexec(fds[1]);
+        return 0;
+    }
+
+    if (errno != ENOSYS) {
+        return -1;
+    }
+#endif
+
+    return qemu_pipe(fds);
+}
+
+void qemu_put_be64(QEMUFile *f, uint64_t v)
+{
+}
+
+uint64_t qemu_get_be64(QEMUFile *f)
+{
+    return 0;
+}
+
+const VMStateInfo vmstate_info_int64;
+int use_icount = 0;
+int vm_running = 1;
+int64_t qemu_icount;
+
+int vmstate_register(DeviceState *dev, int instance_id,
+                     const VMStateDescription *vmsd, void *opaque)
+{
+    return 0;
+}
+int64_t cpu_get_icount(void) {
+    return 0;
+}
+
+VMChangeStateEntry *qemu_add_vm_change_state_handler(VMChangeStateHandler *cb,
+                                                     void *opaque)
+{
+    return NULL;
+}
diff --git a/qemu-tool.h b/qemu-tool.h
new file mode 100644
index 0000000..fd693cf
--- /dev/null
+++ b/qemu-tool.h
@@ -0,0 +1,26 @@ 
+#ifndef QEMU_TOOL_H
+#define QEMU_TOOL_H
+
+#include "qemu-common.h"
+
+#ifdef CONFIG_EVENTFD
+#include <sys/eventfd.h>
+#endif
+
+typedef void VMStateDescription;
+typedef int VMStateInfo;
+
+#ifndef _WIN32
+void qemu_event_increment(void);
+int qemu_event_init(void);
+#else
+int qemu_event_init(void);
+void qemu_event_increment(void);
+#endif
+
+void qemu_put_be64(QEMUFile *f, uint64_t v);
+uint64_t qemu_get_be64(QEMUFile *f);
+int vmstate_register(DeviceState *dev, int instance_id,
+                     const VMStateDescription *vmsd, void *opaque);
+
+#endif