[PATCH 0/21] Remove merge artifacts from qemu-kvm

[PATCH 0/21] Remove merge artifacts from qemu-kvm

[Anthony Liguori]

Now that we've got qemu-kvm, it's pretty easy to look at what's different
between upstream QEMU and qemu-kvm.  Unfortunately, there's still a lot of
gunk that seems to keep surviving merges.

This series removes all of the gunk I could find.  I also culled out a number
of fixes that should be in upstream QEMU.  I'll take care of getting those
committed.

[PATCH 01/21] Remove memalign call for guess_disk_lchs

[Anthony Liguori]

This code doesn't exist in upstream QEMU because it is not necessary to
provide an aligned buffer to bdrv_read.  The API has always worked this way
although at one point, the bouncing was broken which is what led to this
patch.

The places where qemu_memalign() is used in QEMU are only where performance is
sensitive.  guess_disk_lchs does not fall into this category.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 block.c |   13 ++-----------
 1 files changed, 2 insertions(+), 11 deletions(-)

diff --git a/block.c b/block.c
index 8e08f32..8348cf2 100644
--- a/block.c
+++ b/block.c
@@ -30,7 +30,6 @@
 #include "qemu-common.h"
 #include "monitor.h"
 #include "block_int.h"
-#include "osdep.h"
 
 #ifdef HOST_BSD
 #include <sys/types.h>
@@ -772,26 +771,20 @@ struct partition {
 static int guess_disk_lchs(BlockDriverState *bs,
                            int *pcylinders, int *pheads, int *psectors)
 {
-    uint8_t *buf;
+    uint8_t buf[512];
     int ret, i, heads, sectors, cylinders;
     struct partition *p;
     uint32_t nr_sects;
     uint64_t nb_sectors;
 
-    buf = qemu_memalign(512, 512);
-    if (buf == NULL)
-        return -1;
-
     bdrv_get_geometry(bs, &nb_sectors);
 
     ret = bdrv_read(bs, 0, buf, 1);
     if (ret < 0)
         return -1;
     /* test msdos magic */
-    if (buf[510] != 0x55 || buf[511] != 0xaa) {
-        qemu_free(buf);
+    if (buf[510] != 0x55 || buf[511] != 0xaa)
         return -1;
-    }
     for(i = 0; i < 4; i++) {
         p = ((struct partition *)(buf + 0x1be)) + i;
         nr_sects = le32_to_cpu(p->nr_sects);
@@ -812,11 +805,9 @@ static int guess_disk_lchs(BlockDriverState *bs,
             printf("guessed geometry: LCHS=%d %d %d\n",
                    cylinders, heads, sectors);
 #endif
-            qemu_free(buf);
             return 0;
         }
     }
-    qemu_free(buf);
     return -1;
 }
 
-- 
1.6.0.6

[PATCH 02/21] Remove devfn from BlockDriverState

[Anthony Liguori]

It's no longer used.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 block_int.h |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/block_int.h b/block_int.h
index 951ff02..e10b906 100644
--- a/block_int.h
+++ b/block_int.h
@@ -150,8 +150,6 @@ struct BlockDriverState {
     int cyls, heads, secs, translation;
     int type;
     char device_name[32];
-    /* PCI devfn of parent */
-    int devfn;
     BlockDriverState *next;
     void *private;
 };
-- 
1.6.0.6

[PATCH 03/21] Remove use of signalfd in block-raw-posix.c

[Anthony Liguori]

We don't use signalfd in upstream QEMU.  Instead, we always emulate it.

It's not necessarily a bad thing to use signalfd, but this is something that
should be done upstream.  It certainly does qemu-kvm no harm to use the upstream
code.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 block-raw-posix.c |   71 +++++++++++++++++++++++++++-------------------------
 1 files changed, 37 insertions(+), 34 deletions(-)

diff --git a/block-raw-posix.c b/block-raw-posix.c
index f033bae..822839f 100644
--- a/block-raw-posix.c
+++ b/block-raw-posix.c
@@ -25,7 +25,6 @@
 #include "qemu-timer.h"
 #include "qemu-char.h"
 #include "block_int.h"
-#include "compatfd.h"
 #include <assert.h>
 #ifdef CONFIG_AIO
 #include "posix-aio-compat.h"
@@ -481,7 +480,7 @@ typedef struct RawAIOCB {
 
 typedef struct PosixAioState
 {
-    int fd;
+    int rfd, wfd;
     RawAIOCB *first_aio;
 } PosixAioState;
 
@@ -490,29 +489,18 @@ static void posix_aio_read(void *opaque)
     PosixAioState *s = opaque;
     RawAIOCB *acb, **pacb;
     int ret;
-    size_t offset;
-    union {
-        struct qemu_signalfd_siginfo siginfo;
-        char buf[128];
-    } sig;
-
-    /* try to read from signalfd, don't freak out if we can't read anything */
-    offset = 0;
-    while (offset < 128) {
-        ssize_t len;
-
-        len = read(s->fd, sig.buf + offset, 128 - offset);
-        if (len == -1 && errno == EINTR)
-            continue;
-        if (len == -1 && errno == EAGAIN) {
-            /* there is no natural reason for this to happen,
-             * so we'll spin hard until we get everything just
-             * to be on the safe side. */
-            if (offset > 0)
-                continue;
-        }
+    ssize_t len;
+
+    /* read all bytes from signal pipe */
+    for (;;) {
+        char bytes[16];
 
-        offset += len;
+        len = read(s->rfd, bytes, sizeof(bytes));
+        if (len == -1 && errno == EINTR)
+            continue; /* try again */
+        if (len == sizeof(bytes))
+            continue; /* more to read */
+        break;
     }
 
     for(;;) {
@@ -559,10 +547,22 @@ static int posix_aio_flush(void *opaque)
 
 static PosixAioState *posix_aio_state;
 
+static void aio_signal_handler(int signum)
+{
+    if (posix_aio_state) {
+        char byte = 0;
+
+        write(posix_aio_state->wfd, &byte, sizeof(byte));
+    }
+
+    qemu_service_io();
+}
+
 static int posix_aio_init(void)
 {
-    sigset_t mask;
+    struct sigaction act;
     PosixAioState *s;
+    int fds[2];
     struct qemu_paioinit ai;
   
     if (posix_aio_state)
@@ -570,21 +570,24 @@ static int posix_aio_init(void)
 
     s = qemu_malloc(sizeof(PosixAioState));
 
-    /* Make sure to block AIO signal */
-    sigemptyset(&mask);
-    sigaddset(&mask, SIGUSR2);
-    sigprocmask(SIG_BLOCK, &mask, NULL);
+    sigfillset(&act.sa_mask);
+    act.sa_flags = 0; /* do not restart syscalls to interrupt select() */
+    act.sa_handler = aio_signal_handler;
+    sigaction(SIGUSR2, &act, NULL);
 
     s->first_aio = NULL;
-    s->fd = qemu_signalfd(&mask);
-    if (s->fd == -1) {
-        fprintf(stderr, "failed to create signalfd\n");
+    if (pipe(fds) == -1) {
+        fprintf(stderr, "failed to create pipe\n");
         return -errno;
     }
 
-    fcntl(s->fd, F_SETFL, O_NONBLOCK);
+    s->rfd = fds[0];
+    s->wfd = fds[1];
+
+    fcntl(s->rfd, F_SETFL, O_NONBLOCK);
+    fcntl(s->wfd, F_SETFL, O_NONBLOCK);
 
-    qemu_aio_set_fd_handler(s->fd, posix_aio_read, NULL, posix_aio_flush, s);
+    qemu_aio_set_fd_handler(s->rfd, posix_aio_read, NULL, posix_aio_flush, s);
 
     memset(&ai, 0, sizeof(ai));
     ai.aio_threads = 64;
-- 
1.6.0.6

[PATCH 04/21] Remove stray whitespace

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 block-vmdk.c |    1 +
 vl.c         |    1 -
 2 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/block-vmdk.c b/block-vmdk.c
index ff5007c..d47d483 100644
--- a/block-vmdk.c
+++ b/block-vmdk.c
@@ -93,6 +93,7 @@ typedef struct ActiveBDRVState{
 
 static ActiveBDRVState activeBDRV;
 
+
 static int vmdk_probe(const uint8_t *buf, int buf_size, const char *filename)
 {
     uint32_t magic;
diff --git a/vl.c b/vl.c
index 9ff4a5a..15f85e2 100644
--- a/vl.c
+++ b/vl.c
@@ -21,7 +21,6 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
  * THE SOFTWARE.
  */
-
 #include <unistd.h>
 #include <fcntl.h>
 #include <signal.h>
-- 
1.6.0.6

[PATCH 05/21] Remove extra call to kvm_cpu_exec()

[Anthony Liguori]

It's just duplicated code now since we do the same thing in upstream.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 cpu-exec.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index bb77b5e..f959f4e 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -347,12 +347,6 @@ int cpu_exec(CPUState *env1)
             }
 #endif
 
-            /* kvm vcpu threads */
-            if (kvm_enabled()) {
-                kvm_cpu_exec(env);
-                longjmp(env->jmp_env, 1);
-            }
-
             if (kvm_enabled()) {
                 kvm_cpu_exec(env);
                 longjmp(env->jmp_env, 1);
-- 
1.6.0.6

[PATCH 06/21] Remove dead functions from cutils

[Anthony Liguori]

These are leftover from the old migration code.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 cutils.c      |   39 ---------------------------------------
 qemu-common.h |    2 --
 2 files changed, 0 insertions(+), 41 deletions(-)

diff --git a/cutils.c b/cutils.c
index 4bf4fcd..e2bee1e 100644
--- a/cutils.c
+++ b/cutils.c
@@ -99,45 +99,6 @@ time_t mktimegm(struct tm *tm)
     return t;
 }
 
-int hex2bin(char ch)
-{
-    if (ch >= '0' && ch <= '9')
-	return ch - '0';
-    else if (ch >= 'A' && ch <= 'Z')
-	return 10 + ch - 'A';
-    else if (ch >= 'a' && ch <= 'z')
-	return 10 + ch - 'a';
-
-    return -1;
-}
-
-char *urldecode(const char *ptr)
-{
-    char *ret;
-    int i;
-
-    ret = qemu_mallocz(strlen(ptr) + 1);
-    if (ret == NULL)
-	return NULL;
-
-    for (i = 0; *ptr; ptr++, i++) {
-	switch (*ptr) {
-	case '%':
-	    if (ptr[1] == 0 || ptr[2] == 0)
-		break;
-	    ret[i] = hex2bin(ptr[1]) << 4 | hex2bin(ptr[2]);
-	    ptr += 2;
-	    break;
-	default:
-	   ret[i] = *ptr;
-	   break;
-	}
-    }
-    ret[i] = 0;
-
-    return ret;
-}
-
 int qemu_fls(int i)
 {
     return 32 - clz32(i);
diff --git a/qemu-common.h b/qemu-common.h
index 77d4ddd..942d3a6 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -109,8 +109,6 @@ int strstart(const char *str, const char *val, const char **ptr);
 int stristart(const char *str, const char *val, const char **ptr);
 time_t mktimegm(struct tm *tm);
 int qemu_fls(int i);
-int hex2bin(char ch);
-char *urldecode(const char *ptr);
 
 #define qemu_isalnum(c)		isalnum((unsigned char)(c))
 #define qemu_isalpha(c)		isalpha((unsigned char)(c))
-- 
1.6.0.6

[PATCH 07/21] Remove dead macros likely/unlikely in exec.c

[Anthony Liguori]

More left overs from the old migration code.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 exec.c |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/exec.c b/exec.c
index 16d3cf8..e9d83a8 100644
--- a/exec.c
+++ b/exec.c
@@ -3500,14 +3500,6 @@ uint32_t lduw_phys(target_phys_addr_t addr)
     return tswap16(val);
 }
 
-#ifdef __GNUC__
-#define likely(x) __builtin_expect(!!(x), 1)
-#define unlikely(x) __builtin_expect(!!(x), 0)
-#else
-#define likely(x) x
-#define unlikely(x) x
-#endif
-
 /* warning: addr must be aligned. The ram page is not masked as dirty
    and the code inside is not invalidated. It is useful if the dirty
    bits are used to track modified PTEs */
-- 
1.6.0.6

[PATCH 08/21] Remove leftovers in device-hotplug.c

[Anthony Liguori]

Post merge into upstream, device-hotplug.c is carrying stray #includes,
 #defines, and whitespace changes.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/device-hotplug.c |    9 +--------
 1 files changed, 1 insertions(+), 8 deletions(-)

diff --git a/hw/device-hotplug.c b/hw/device-hotplug.c
index 24ab506..3bdc048 100644
--- a/hw/device-hotplug.c
+++ b/hw/device-hotplug.c
@@ -27,14 +27,6 @@
 #include "net.h"
 #include "block_int.h"
 #include "sysemu.h"
-#include "pci.h"
-#include "pc.h"
-#include "console.h"
-#include "config.h"
-#include "virtio-blk.h"
-
-#define PCI_BASE_CLASS_STORAGE          0x01
-#define PCI_BASE_CLASS_NETWORK          0x02
 
 int add_init_drive(const char *opts)
 {
@@ -91,3 +83,4 @@ void destroy_bdrvs(dev_match_fn *match_fn, void *arg)
     }
 }
 
+
-- 
1.6.0.6

[PATCH 09/21] Remove unnecessary setting of cmos smp_cpu count

[Anthony Liguori]

This is duplicate code.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/pc.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 35f9527..db34f53 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -265,7 +265,6 @@ static void cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
         rtc_set_memory(s, 0x5c, (unsigned int)above_4g_mem_size >> 24);
         rtc_set_memory(s, 0x5d, (uint64_t)above_4g_mem_size >> 32);
     }
-    rtc_set_memory(s, 0x5f, smp_cpus - 1);
 
     if (ram_size > (16 * 1024 * 1024))
         val = (ram_size / 65536) - ((16 * 1024 * 1024) / 65536);
-- 
1.6.0.6

[PATCH 10/21] Remove IBM copyright in unmodified file in upstream

[Anthony Liguori]

Presumably, it would carry an IBM copyright upstream if needed.  qemu-kvm
introduces no additional code over upstream QEMU in this file.

Avi: please let Hollis ack/nack this before committing.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/ppc4xx.h |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/hw/ppc4xx.h b/hw/ppc4xx.h
index 25a91bd..7832cd9 100644
--- a/hw/ppc4xx.h
+++ b/hw/ppc4xx.h
@@ -3,9 +3,6 @@
  *
  * Copyright (c) 2007 Jocelyn Mayer
  *
- * Copyright 2008 IBM Corp.
- * Authors: Hollis Blanchard <hollisb@us.ibm.com>
- *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to deal
  * in the Software without restriction, including without limitation the rights
-- 
1.6.0.6

[PATCH 11/21] Remove unused variables in vga.c

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/vga.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/vga.c b/hw/vga.c
index 4931b69..d96f1be 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -1585,12 +1585,11 @@ static void vga_sync_dirty_bitmap(VGAState *s)
  */
 static void vga_draw_graphic(VGAState *s, int full_update)
 {
-    int y1, y, update, linesize, y_start, double_scan, mask, depth;
-    int width, height, shift_control, line_offset, bwidth, bits;
+    int y1, y, update, page_min, page_max, linesize, y_start, double_scan, mask, depth;
+    int width, height, shift_control, line_offset, page0, page1, bwidth, bits;
     int disp_width, multi_scan, multi_run;
     uint8_t *d;
     uint32_t v, addr1, addr;
-    long page0, page1, page_min, page_max;
     vga_draw_line_func *vga_draw_line;
 
     full_update |= update_basic_params(s);
-- 
1.6.0.6

[PATCH 12/21] Remove odd hack in vga.c

[Anthony Liguori]

I looked closely at the vga code in kvm-userspace a while ago and merged
every fix I could understand into upstream QEMU.  This particular change makes
no sense to me.  I could not figure out from revision history what it actually
fixed.  I'm fairly certain it's not useful today.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/vga.c |   27 ++++-----------------------
 1 files changed, 4 insertions(+), 23 deletions(-)

diff --git a/hw/vga.c b/hw/vga.c
index d96f1be..385184a 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -2227,33 +2227,14 @@ typedef struct PCIVGAState {
     VGAState vga_state;
 } PCIVGAState;
 
-static int s1, s2;
-
-static void mark_dirty(target_phys_addr_t start, target_phys_addr_t len)
-{
-    target_phys_addr_t end = start + len;
-
-    while (start < end) {
-        cpu_physical_memory_set_dirty(cpu_get_physical_page_desc(start));
-        start += TARGET_PAGE_SIZE;
-    }
-}
-
 void vga_dirty_log_start(VGAState *s)
 {
     if (kvm_enabled() && s->map_addr)
-        if (!s1) {
-            kvm_log_start(s->map_addr, s->map_end - s->map_addr);
-            mark_dirty(s->map_addr, s->map_end - s->map_addr);
-            s1 = 1;
-        }
+        kvm_log_start(s->map_addr, s->map_end - s->map_addr);
+
     if (kvm_enabled() && s->lfb_vram_mapped) {
-        if (!s2) {
-            kvm_log_start(isa_mem_base + 0xa0000, 0x8000);
-            kvm_log_start(isa_mem_base + 0xa8000, 0x8000);
-            mark_dirty(isa_mem_base + 0xa0000, 0x10000);
-        }
-        s2 = 1;
+        kvm_log_start(isa_mem_base + 0xa0000, 0x8000);
+        kvm_log_start(isa_mem_base + 0xa8000, 0x8000);
     }
 }
 
-- 
1.6.0.6

[PATCH 13/21] Remove virtio-console PIF change

[Anthony Liguori]

If this change should happen, it should happen in upstream QEMU.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/virtio-console.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 89e8be0..b263281 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -132,7 +132,7 @@ void *virtio_console_init(PCIBus *bus, CharDriverState *chr)
                                          PCI_DEVICE_ID_VIRTIO_CONSOLE,
                                          PCI_VENDOR_ID_REDHAT_QUMRANET,
                                          VIRTIO_ID_CONSOLE,
-                                         PCI_CLASS_OTHERS, 0x00,
+                                         PCI_CLASS_DISPLAY_OTHER, 0x00,
                                          0, sizeof(VirtIOConsole));
     if (s == NULL)
         return NULL;
-- 
1.6.0.6

[PATCH 14/21] Remove -cpu-vendor-string

[Anthony Liguori]

This isn't in upstream QEMU and is of little utility to KVM.  It's unlikely
to appear in upstream QEMU either.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 linux-user/main.c    |    1 -
 qemu-options.hx      |    2 --
 target-i386/helper.c |    5 -----
 vl.c                 |    4 ----
 4 files changed, 0 insertions(+), 12 deletions(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index 5967fa3..dc39b05 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -43,7 +43,6 @@ int singlestep;
 
 static const char *interp_prefix = CONFIG_QEMU_PREFIX;
 const char *qemu_uname_release = CONFIG_UNAME_RELEASE;
-const char *cpu_vendor_string = NULL;
 
 #if defined(__i386__) && !defined(CONFIG_STATIC)
 /* Force usage of an ELF interpreter even if it is an ELF shared
diff --git a/qemu-options.hx b/qemu-options.hx
index a11ead9..f7d83c9 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1545,8 +1545,6 @@ DEF("pcidevice", HAS_ARG, QEMU_OPTION_pcidevice,
 #endif
 DEF("enable-nesting", 0, QEMU_OPTION_enable_nesting,
     "-enable-nesting enable support for running a VM inside the VM (AMD only)\n")
-DEF("cpu-vendor", HAS_ARG, QEMU_OPTION_cpu_vendor,
-    "-cpu-vendor STRING   override the cpuid vendor string\n")
 DEF("nvram", HAS_ARG, QEMU_OPTION_nvram,
     "-nvram FILE          provide ia64 nvram contents\n")
 DEF("tdf", 0, QEMU_OPTION_tdf,
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 24fcea8..7440983 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -91,8 +91,6 @@ static void add_flagname_to_bitmaps(char *flagname, uint32_t *features,
     fprintf(stderr, "CPU feature %s not found\n", flagname);
 }
 
-extern const char *cpu_vendor_string;
-
 typedef struct x86_def_t {
     const char *name;
     uint32_t level;
@@ -431,9 +429,6 @@ static int cpu_x86_register (CPUX86State *env, const char *cpu_model)
     {
         const char *model_id = def->model_id;
         int c, len, i;
-
-        if (cpu_vendor_string != NULL)
-            model_id = cpu_vendor_string;
         if (!model_id)
             model_id = "";
         len = strlen(model_id);
diff --git a/vl.c b/vl.c
index 15f85e2..3b0e3dc 100644
--- a/vl.c
+++ b/vl.c
@@ -268,7 +268,6 @@ const char *mem_path = NULL;
 int mem_prealloc = 1;	/* force preallocation of physical target memory */
 #endif
 long hpagesize = 0;
-const char *cpu_vendor_string;
 #ifdef TARGET_ARM
 int old_param = 0;
 #endif
@@ -5228,9 +5227,6 @@ int main(int argc, char **argv, char **envp)
                 nb_prom_envs++;
                 break;
 #endif
-	    case QEMU_OPTION_cpu_vendor:
-		cpu_vendor_string = optarg;
-		break;
 #ifdef TARGET_ARM
             case QEMU_OPTION_old_param:
                 old_param = 1;
-- 
1.6.0.6

[PATCH 15/21] Remove defined(TARGET_X86_64)

[Anthony Liguori]

TARGET_I386 is always defined when TARGET_X86_64 is defined.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 monitor.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/monitor.c b/monitor.c
index 11e48c7..1a531f2 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1764,7 +1764,7 @@ static const mon_cmd_t mon_cmds[] = {
       "", "cancel the current VM migration" },
     { "migrate_set_speed", "s", do_migrate_set_speed,
       "value", "set maximum speed (in bytes) for migrations" },
-#if defined(TARGET_I386) || defined(TARGET_X86_64)
+#if defined(TARGET_I386)
     { "drive_add", "ss", drive_hot_add, "pci_addr=[[<domain>:]<bus>:]<slot>\n"
                                          "[file=file][,if=type][,bus=n]\n"
                                         "[,unit=m][,media=d][index=i]\n"
-- 
1.6.0.6

[PATCH 16/21] Remove clean rule change

[Anthony Liguori]

It's not in upstream QEMU so apparently it's not useful.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 pc-bios/Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/pc-bios/Makefile b/pc-bios/Makefile
index dabeb4c..315288d 100644
--- a/pc-bios/Makefile
+++ b/pc-bios/Makefile
@@ -16,4 +16,4 @@ all: $(TARGETS)
 	dtc -I dts -O dtb -o $@ $<
 
 clean:
-	rm -f $(TARGETS) *.o *~ *.dtb
+	rm -f $(TARGETS) *.o *~
-- 
1.6.0.6

[PATCH 17/21] Remove #define __user in usb-linux.c

[Anthony Liguori]

This has been consistently nacked in upstream QEMU.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 usb-linux.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index 26643bd..70d7a1c 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -34,10 +34,6 @@
 #include "qemu-timer.h"
 #include "monitor.h"
 
-#if defined(__linux__)
-#define __user
-#endif
-
 #include <dirent.h>
 #include <sys/ioctl.h>
 #include <signal.h>
-- 
1.6.0.6

[PATCH 18/21] Remove host_alarm_timer hacks.

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 vl.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index 3b0e3dc..848a8f8 100644
--- a/vl.c
+++ b/vl.c
@@ -1367,8 +1367,7 @@ static void host_alarm_handler(int host_signum)
         last_clock = ti;
     }
 #endif
-    if (1 ||
-        alarm_has_dynticks(alarm_timer) ||
+    if (alarm_has_dynticks(alarm_timer) ||
         (!use_icount &&
             qemu_timer_expired(active_timers[QEMU_TIMER_VIRTUAL],
                                qemu_get_clock(vm_clock))) ||
-- 
1.6.0.6

[PATCH 19/21] Use QEMU_PKGVERSION to declare kvm-devel

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 configure |    4 +---
 vl.c      |    2 +-
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/configure b/configure
index c41e269..6b6cf5b 100755
--- a/configure
+++ b/configure
@@ -194,7 +194,7 @@ blobs="yes"
 fdt="yes"
 sdl_x11="no"
 xen="yes"
-pkgversion=""
+pkgversion="kvm-devel"
 signalfd="no"
 eventfd="no"
 cpu_emulation="yes"
@@ -1677,8 +1677,6 @@ fi
 qemu_version=`head $source_path/VERSION`
 echo "VERSION=$qemu_version" >>$config_mak
 echo "#define QEMU_VERSION \"$qemu_version\"" >> $config_h
-echo "#define KVM_VERSION \"kvm-devel\"" >> $config_h
-
 echo "#define QEMU_PKGVERSION \"$pkgversion\"" >> $config_h
 
 echo "SRC_PATH=$source_path" >> $config_mak
diff --git a/vl.c b/vl.c
index 848a8f8..19c1de3 100644
--- a/vl.c
+++ b/vl.c
@@ -4099,7 +4099,7 @@ static int main_loop(void)
 
 static void version(void)
 {
-    printf("QEMU PC emulator version " QEMU_VERSION " (" KVM_VERSION ")" QEMU_PKGVERSION ", Copyright (c) 2003-2008 Fabrice Bellard\n");
+    printf("QEMU PC emulator version " QEMU_VERSION QEMU_PKGVERSION ", Copyright (c) 2003-2008 Fabrice Bellard\n");
 }
 
 static void help(int exitcode)
-- 
1.6.0.6

[PATCH 20/21] Get rid of qemu_get_launch_info()

[Anthony Liguori]

This is left over from the old ssh migration implementation

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 sysemu.h |    3 ---
 vl.c     |   13 -------------
 2 files changed, 0 insertions(+), 16 deletions(-)

diff --git a/sysemu.h b/sysemu.h
index e8dd381..cf6b48a 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -46,9 +46,6 @@ void qemu_system_powerdown(void);
 #endif
 void qemu_system_reset(void);
 
-void qemu_get_launch_info(int *argc, char ***argv,
-                          int *opt_daemonize, const char **opt_incoming);
-
 void do_savevm(Monitor *mon, const char *name);
 void do_loadvm(Monitor *mon, const char *name);
 void do_delvm(Monitor *mon, const char *name);
diff --git a/vl.c b/vl.c
index 19c1de3..a38379b 100644
--- a/vl.c
+++ b/vl.c
@@ -4373,18 +4373,6 @@ int qemu_uuid_parse(const char *str, uint8_t *uuid)
 
 #define MAX_NET_CLIENTS 32
 
-static int saved_argc;
-static char **saved_argv;
-static const char *saved_incoming;
-
-void qemu_get_launch_info(int *argc, char ***argv, int *opt_daemonize, const char **opt_incoming)
-{
-    *argc = saved_argc;
-    *argv = saved_argv;
-    *opt_daemonize = daemonize;
-    *opt_incoming = saved_incoming;
-}
-
 #ifdef USE_KVM
 
 #define HUGETLBFS_MAGIC       0x958458f6
@@ -5287,7 +5275,6 @@ int main(int argc, char **argv, char **envp)
                 break;
             case QEMU_OPTION_incoming:
                 incoming = optarg;
-                saved_incoming = incoming;
                 break;
 #ifndef _WIN32
             case QEMU_OPTION_chroot:
-- 
1.6.0.6

[PATCH 21/21] Leave upstream QEMU comments intact

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 vl.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/vl.c b/vl.c
index a38379b..3d6ebd9 100644
--- a/vl.c
+++ b/vl.c
@@ -5483,10 +5483,10 @@ int main(int argc, char **argv, char **envp)
         if (bt_parse(bt_opts[i]))
             exit(1);
 
+    /* init the memory */
     if (ram_size == 0)
         ram_size = DEFAULT_RAM_SIZE * 1024 * 1024;
 
-    /* init the memory */
     if (kvm_enabled()) {
 	    if (kvm_qemu_create_context() < 0) {
 		    fprintf(stderr, "Could not create KVM context\n");
-- 
1.6.0.6

Re: [PATCH 10/21] Remove IBM copyright in unmodified file in upstream

[Hollis Blanchard]

On Wed, 2009-04-29 at 16:20 -0500, Anthony Liguori wrote:
> Presumably, it would carry an IBM copyright upstream if needed.  qemu-kvm
> introduces no additional code over upstream QEMU in this file.
> 
> Avi: please let Hollis ack/nack this before committing.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/ppc4xx.h |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc4xx.h b/hw/ppc4xx.h
> index 25a91bd..7832cd9 100644
> --- a/hw/ppc4xx.h
> +++ b/hw/ppc4xx.h
> @@ -3,9 +3,6 @@
>   *
>   * Copyright (c) 2007 Jocelyn Mayer
>   *
> - * Copyright 2008 IBM Corp.
> - * Authors: Hollis Blanchard <hollisb@us.ibm.com>
> - *
>   * Permission is hereby granted, free of charge, to any person obtaining a copy
>   * of this software and associated documentation files (the "Software"), to deal
>   * in the Software without restriction, including without limitation the rights

Hmm, I don't know how that happened. Anyways, I did add a couple
function prototypes, but I don't think that's significant copyrightable
IP.

Acked-by: Hollis Blanchard <hollisb@us.ibm.com>

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [PATCH 03/21] Remove use of signalfd in block-raw-posix.c

[Avi Kivity]

Anthony Liguori wrote:
> We don't use signalfd in upstream QEMU.  Instead, we always emulate it.
>   

With an extra thread -> so an extra context switch.

> It's not necessarily a bad thing to use signalfd, but this is something that
> should be done upstream.  It certainly does qemu-kvm no harm to use the upstream
> code.
>   

It will introduce a (likely minor, but real) performance regression.

Instead of this, why not apply the reverse patch to qemu.git?


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

Re: [PATCH 11/21] Remove unused variables in vga.c

[Avi Kivity]

Anthony Liguori wrote:
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/vga.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/hw/vga.c b/hw/vga.c
> index 4931b69..d96f1be 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -1585,12 +1585,11 @@ static void vga_sync_dirty_bitmap(VGAState *s)
>   */
>  static void vga_draw_graphic(VGAState *s, int full_update)
>  {
> -    int y1, y, update, linesize, y_start, double_scan, mask, depth;
> -    int width, height, shift_control, line_offset, bwidth, bits;
> +    int y1, y, update, page_min, page_max, linesize, y_start, double_scan, mask, depth;
> +    int width, height, shift_control, line_offset, page0, page1, bwidth, bits;
>      int disp_width, multi_scan, multi_run;
>      uint8_t *d;
>      uint32_t v, addr1, addr;
> -    long page0, page1, page_min, page_max;
>      vga_draw_line_func *vga_draw_line;
>  

This introduces a regression with >4GB guests.  I resolved this by 
posting a patch to qemu; see 12c7e75a7c.  Are you using an outdated 
checkout?

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

Re: [PATCH 12/21] Remove odd hack in vga.c

[Avi Kivity]

Anthony Liguori wrote:
> I looked closely at the vga code in kvm-userspace a while ago and merged
> every fix I could understand into upstream QEMU.  This particular change makes
> no sense to me.  I could not figure out from revision history what it actually
> fixed.  I'm fairly certain it's not useful today.
>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/vga.c |   27 ++++-----------------------
>  1 files changed, 4 insertions(+), 23 deletions(-)
>
> diff --git a/hw/vga.c b/hw/vga.c
> index d96f1be..385184a 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -2227,33 +2227,14 @@ typedef struct PCIVGAState {
>      VGAState vga_state;
>  } PCIVGAState;
>  
> -static int s1, s2;
> -
> -static void mark_dirty(target_phys_addr_t start, target_phys_addr_t len)
> -{
> -    target_phys_addr_t end = start + len;
> -
> -    while (start < end) {
> -        cpu_physical_memory_set_dirty(cpu_get_physical_page_desc(start));
> -        start += TARGET_PAGE_SIZE;
> -    }
> -}
> -
>  void vga_dirty_log_start(VGAState *s)
>  {
>      if (kvm_enabled() && s->map_addr)
> -        if (!s1) {
> -            kvm_log_start(s->map_addr, s->map_end - s->map_addr);
> -            mark_dirty(s->map_addr, s->map_end - s->map_addr);
> -            s1 = 1;
> -        }
> +        kvm_log_start(s->map_addr, s->map_end - s->map_addr);
> +
>      if (kvm_enabled() && s->lfb_vram_mapped) {
> -        if (!s2) {
> -            kvm_log_start(isa_mem_base + 0xa0000, 0x8000);
> -            kvm_log_start(isa_mem_base + 0xa8000, 0x8000);
> -            mark_dirty(isa_mem_base + 0xa0000, 0x10000);
> -        }
> -        s2 = 1;
> +        kvm_log_start(isa_mem_base + 0xa0000, 0x8000);
> +        kvm_log_start(isa_mem_base + 0xa8000, 0x8000);
>      }
>  }
>  
>   

This makes live migration and vga dirty tracking work together.  
Unfortunately since the last merge with qemu it's broken.

We have a shared resource, the log_dirty flag of memory slots.  We can't 
call log_start() and log_stop() from different users and expect things 
to work.

One cleaner way to fix this is to add a parameter containing the mask 
which will be used by the client to access the qemu bytemap.  
log_start() can OR this parameter with its own copy, and log_stop() can 
AND NOT the same thing.  When the local copy is nonzero, the slot dirty 
log is enabled.

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

Re: [PATCH 13/21] Remove virtio-console PIF change

[Avi Kivity]

Anthony Liguori wrote:
> If this change should happen, it should happen in upstream QEMU.
>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/virtio-console.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/hw/virtio-console.c b/hw/virtio-console.c
> index 89e8be0..b263281 100644
> --- a/hw/virtio-console.c
> +++ b/hw/virtio-console.c
> @@ -132,7 +132,7 @@ void *virtio_console_init(PCIBus *bus, CharDriverState *chr)
>                                           PCI_DEVICE_ID_VIRTIO_CONSOLE,
>                                           PCI_VENDOR_ID_REDHAT_QUMRANET,
>                                           VIRTIO_ID_CONSOLE,
> -                                         PCI_CLASS_OTHERS, 0x00,
> +                                         PCI_CLASS_DISPLAY_OTHER, 0x00,
>                                           0, sizeof(VirtIOConsole));
>      if (s == NULL)
>          return NULL;
>   

Since virtio-console is not enabled by default, it isn't needed, so I'll 
apply this.

But if it were needed, there's no reason to introduce regressions into 
qemu-kvm.git.

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

Re: [PATCH 14/21] Remove -cpu-vendor-string

[Avi Kivity]

Anthony Liguori wrote:
> This isn't in upstream QEMU and is of little utility to KVM.  It's unlikely
> to appear in upstream QEMU either.
>   

Since we allow overriding cpuid flags, why not the vendor string?  It's 
necessary for cpu passthrough.

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

Re: [PATCH 16/21] Remove clean rule change

[Avi Kivity]

Anthony Liguori wrote:
> It's not in upstream QEMU so apparently it's not useful.
>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  pc-bios/Makefile |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/pc-bios/Makefile b/pc-bios/Makefile
> index dabeb4c..315288d 100644
> --- a/pc-bios/Makefile
> +++ b/pc-bios/Makefile
> @@ -16,4 +16,4 @@ all: $(TARGETS)
>  	dtc -I dts -O dtb -o $@ $<
>  
>  clean:
> -	rm -f $(TARGETS) *.o *~ *.dtb
> +	rm -f $(TARGETS) *.o *~
>   

Hollis?

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

Re: [PATCH 17/21] Remove #define __user in usb-linux.c

[Avi Kivity]

Anthony Liguori wrote:
> This has been consistently nacked in upstream QEMU.
>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  usb-linux.c |    4 ----
>  1 files changed, 0 insertions(+), 4 deletions(-)
>
> diff --git a/usb-linux.c b/usb-linux.c
> index 26643bd..70d7a1c 100644
> --- a/usb-linux.c
> +++ b/usb-linux.c
> @@ -34,10 +34,6 @@
>  #include "qemu-timer.h"
>  #include "monitor.h"
>  
> -#if defined(__linux__)
> -#define __user
> -#endif
> -
>   

This will introduce a regression into qemu-kvm.git.

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

Re: [PATCH 18/21] Remove host_alarm_timer hacks.

[Avi Kivity]

Anthony Liguori wrote:
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  vl.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 3b0e3dc..848a8f8 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1367,8 +1367,7 @@ static void host_alarm_handler(int host_signum)
>          last_clock = ti;
>      }
>  #endif
> -    if (1 ||
> -        alarm_has_dynticks(alarm_timer) ||
> +    if (alarm_has_dynticks(alarm_timer) ||
>          (!use_icount &&
>              qemu_timer_expired(active_timers[QEMU_TIMER_VIRTUAL],
>                                 qemu_get_clock(vm_clock))) ||
>   

This was added to fix a problem.  Have you tested it?

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

Re: [PATCH 0/21] Remove merge artifacts from qemu-kvm

[Avi Kivity]

Anthony Liguori wrote:
> Now that we've got qemu-kvm, it's pretty easy to look at what's different
> between upstream QEMU and qemu-kvm.  

This was actually easy in kvm-userspace.git: git diff origin/master 
origin/qemu-svn/trunk.

> Unfortunately, there's still a lot of
> gunk that seems to keep surviving merges.
>
> This series removes all of the gunk I could find.  I also culled out a number
> of fixes that should be in upstream QEMU.  I'll take care of getting those
> committed.
>   

Applied all except patches for which I had objections (noted in separate 
replies); thanks.

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

Re: [PATCH 03/21] Remove use of signalfd in block-raw-posix.c

[Anthony Liguori]

Avi Kivity wrote:
> Anthony Liguori wrote:
>> We don't use signalfd in upstream QEMU.  Instead, we always emulate it.
>>   
>
> With an extra thread -> so an extra context switch.

We don't use an extra thread.  We just install a signal handler that 
writes to a pipe.  At best, the added overhead is that we get EINTRs 
more often but this is something we already handle.

>> It's not necessarily a bad thing to use signalfd, but this is 
>> something that
>> should be done upstream.  It certainly does qemu-kvm no harm to use 
>> the upstream
>> code.
>>   
>
> It will introduce a (likely minor, but real) performance regression.
>
> Instead of this, why not apply the reverse patch to qemu.git?

I'm not sure signalfd really buys us much.  To emulate it requires 
writing a bunch more data to the pipe.  When writing more than 1 byte, 
we have to worry about whether there's a partial write because the pipe 
buffers full).  We also have to make sure to read from the fd in 
properly sized chunks.

Regards,

Anthony Liguori

Re: [PATCH 03/21] Remove use of signalfd in block-raw-posix.c

[Avi Kivity]

Anthony Liguori wrote:
> Avi Kivity wrote:
>> Anthony Liguori wrote:
>>> We don't use signalfd in upstream QEMU.  Instead, we always emulate it.
>>>   
>>
>> With an extra thread -> so an extra context switch.
>
> We don't use an extra thread.  We just install a signal handler that 
> writes to a pipe.  At best, the added overhead is that we get EINTRs 
> more often but this is something we already handle.

Oh okay.  But signal delivery is slow; for example the FPU needs to be 
reset.

> I'm not sure signalfd really buys us much.  To emulate it requires 
> writing a bunch more data to the pipe.  When writing more than 1 byte, 
> we have to worry about whether there's a partial write because the 
> pipe buffers full).  We also have to make sure to read from the fd in 
> properly sized chunks.

Then we can use one byte writes (and reads) when signalfd is not 
available.  128 byte pipe read/writes should always be atomic on Linux 
though, likely on other OSes too.

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

Re: [PATCH 11/21] Remove unused variables in vga.c

[Anthony Liguori]

Avi Kivity wrote:
>> @@ -1585,12 +1585,11 @@ static void vga_sync_dirty_bitmap(VGAState *s)
>>   */
>>  static void vga_draw_graphic(VGAState *s, int full_update)
>>  {
>> -    int y1, y, update, linesize, y_start, double_scan, mask, depth;
>> -    int width, height, shift_control, line_offset, bwidth, bits;
>> +    int y1, y, update, page_min, page_max, linesize, y_start, 
>> double_scan, mask, depth;
>> +    int width, height, shift_control, line_offset, page0, page1, 
>> bwidth, bits;
>>      int disp_width, multi_scan, multi_run;
>>      uint8_t *d;
>>      uint32_t v, addr1, addr;
>> -    long page0, page1, page_min, page_max;
>>      vga_draw_line_func *vga_draw_line;
>>  
>
> This introduces a regression with >4GB guests.  I resolved this by 
> posting a patch to qemu; see 12c7e75a7c.  Are you using an outdated 
> checkout?

Oh, I understand what's happening now.  It took me a while to see that 
we're changing the type of variables from int to long.

-- 
Regards,

Anthony Liguori

Re: [PATCH 14/21] Remove -cpu-vendor-string

[Anthony Liguori]

Avi Kivity wrote:
> Anthony Liguori wrote:
>> This isn't in upstream QEMU and is of little utility to KVM.  It's 
>> unlikely
>> to appear in upstream QEMU either.
>>   
>
> Since we allow overriding cpuid flags, why not the vendor string?  
> It's necessary for cpu passthrough.
>
But we don't allow explicit override of cpuid flags today.  We support 
choosing CPU models which include vendor id and cpuid flags.

Introducing a host CPU model would be acceptable and would more 
accurately achieve cpu passthrough.

-- 
Regards,

Anthony Liguori

Re: [PATCH 17/21] Remove #define __user in usb-linux.c

[Anthony Liguori]

Avi Kivity wrote:
> Anthony Liguori wrote:
>> This has been consistently nacked in upstream QEMU.
>>
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> ---
>>  usb-linux.c |    4 ----
>>  1 files changed, 0 insertions(+), 4 deletions(-)
>>
>> diff --git a/usb-linux.c b/usb-linux.c
>> index 26643bd..70d7a1c 100644
>> --- a/usb-linux.c
>> +++ b/usb-linux.c
>> @@ -34,10 +34,6 @@
>>  #include "qemu-timer.h"
>>  #include "monitor.h"
>>  
>> -#if defined(__linux__)
>> -#define __user
>> -#endif
>> -
>>   
>
> This will introduce a regression into qemu-kvm.git.

It won't because -D__user is in CFLAGS.

-- 
Regards,

Anthony Liguori

Re: [PATCH 14/21] Remove -cpu-vendor-string

[Avi Kivity]

Anthony Liguori wrote:
>>> This isn't in upstream QEMU and is of little utility to KVM.  It's 
>>> unlikely
>>> to appear in upstream QEMU either.
>>>   
>>
>> Since we allow overriding cpuid flags, why not the vendor string?  
>> It's necessary for cpu passthrough.
>>
> But we don't allow explicit override of cpuid flags today.  We support 
> choosing CPU models which include vendor id and cpuid flags.

I think we allow "-cpu qemu64,-nx" for example.

>
> Introducing a host CPU model would be acceptable and would more 
> accurately achieve cpu passthrough.
>

I agree that "-cpu host[,modifiers]" is desirable.  But I don't see why 
we shouldn't support finegrained control.

It's probably better done through a "-cpu blah,-nx,vendorid=foobar" 
rather than a separate option.

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

Re: [PATCH 17/21] Remove #define __user in usb-linux.c

[Avi Kivity]

Anthony Liguori wrote:
> Avi Kivity wrote:
>> Anthony Liguori wrote:
>>> This has been consistently nacked in upstream QEMU.
>>>
>>> -#if defined(__linux__)
>>> -#define __user
>>> -#endif
>>> -
>>>   
>>
>> This will introduce a regression into qemu-kvm.git.
>
> It won't because -D__user is in CFLAGS.
>

Ah, ok, will apply.  But that's not in upstream either.

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

Re: [PATCH 18/21] Remove host_alarm_timer hacks.

[Anthony Liguori]

Avi Kivity wrote:
> Anthony Liguori wrote:
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> ---
>>  vl.c |    3 +--
>>  1 files changed, 1 insertions(+), 2 deletions(-)
>>
>> diff --git a/vl.c b/vl.c
>> index 3b0e3dc..848a8f8 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -1367,8 +1367,7 @@ static void host_alarm_handler(int host_signum)
>>          last_clock = ti;
>>      }
>>  #endif
>> -    if (1 ||
>> -        alarm_has_dynticks(alarm_timer) ||
>> +    if (alarm_has_dynticks(alarm_timer) ||
>>          (!use_icount &&
>>              qemu_timer_expired(active_timers[QEMU_TIMER_VIRTUAL],
>>                                 qemu_get_clock(vm_clock))) ||
>>   
>
> This was added to fix a problem.  Have you tested it?

Do you know what problem it fixes?

This goes back a very long time.  IIUC, this was added prior to the IO 
thread as an "optimization".  This ensures that any time there's a 
timer, the vcpu is interrupted to allow IO to run.  With non-dynticks, 
there can be spurious timer signals because we problem the timer with a 
fixed frequency.  It's necessary to take this path with dynticks because 
we need to rearm the timer which happens in the IO path.  It's not 
necessary to take this path with a non-dynticks timer unless there's 
been an expiration.

In modern KVM, the IO thread is capable of interrupting the CPU whenever 
it needs to process IO.  Therefore this "problem" no longer exists.

Regards,

Anthony Liguori


-- 
Regards,

Anthony Liguori

Re: [PATCH 17/21] Remove #define __user in usb-linux.c

[Anthony Liguori]

Avi Kivity wrote:
> Ah, ok, will apply.  But that's not in upstream either.

Nope, but one step at a time.

-- 
Regards,

Anthony Liguori

Re: [PATCH 18/21] Remove host_alarm_timer hacks.

[Avi Kivity]

Anthony Liguori wrote:
> Avi Kivity wrote:
>> Anthony Liguori wrote:
>>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>> ---
>>>  vl.c |    3 +--
>>>  1 files changed, 1 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/vl.c b/vl.c
>>> index 3b0e3dc..848a8f8 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -1367,8 +1367,7 @@ static void host_alarm_handler(int host_signum)
>>>          last_clock = ti;
>>>      }
>>>  #endif
>>> -    if (1 ||
>>> -        alarm_has_dynticks(alarm_timer) ||
>>> +    if (alarm_has_dynticks(alarm_timer) ||
>>>          (!use_icount &&
>>>              qemu_timer_expired(active_timers[QEMU_TIMER_VIRTUAL],
>>>                                 qemu_get_clock(vm_clock))) ||
>>>   
>>
>> This was added to fix a problem.  Have you tested it?
>
> Do you know what problem it fixes?
>
> This goes back a very long time.  IIUC, this was added prior to the IO 
> thread as an "optimization".  This ensures that any time there's a 
> timer, the vcpu is interrupted to allow IO to run.  With non-dynticks, 
> there can be spurious timer signals because we problem the timer with 
> a fixed frequency.  It's necessary to take this path with dynticks 
> because we need to rearm the timer which happens in the IO path.  It's 
> not necessary to take this path with a non-dynticks timer unless 
> there's been an expiration.
>
> In modern KVM, the IO thread is capable of interrupting the CPU 
> whenever it needs to process IO.  Therefore this "problem" no longer 
> exists.
>

It would still be good to verify that the problem no longer exists.  
This is not a cosmetic change; some testing is needed to verify it 
doesn't introduce new latencies.

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

Re: [PATCH 14/21] Remove -cpu-vendor-string

[Anthony Liguori]

Avi Kivity wrote:
> Anthony Liguori wrote:
>>>> This isn't in upstream QEMU and is of little utility to KVM.  It's 
>>>> unlikely
>>>> to appear in upstream QEMU either.
>>>>   
>>>
>>> Since we allow overriding cpuid flags, why not the vendor string?  
>>> It's necessary for cpu passthrough.
>>>
>> But we don't allow explicit override of cpuid flags today.  We 
>> support choosing CPU models which include vendor id and cpuid flags.
>
> I think we allow "-cpu qemu64,-nx" for example.

Funny enough, -cpu qemu64,vendor="AuthenticAMD" already works today.  So 
yeah, there's no reason to carry -cpu-vendor-string anymore.

-- 
Regards,

Anthony Liguori

Re: [PATCH 03/21] Remove use of signalfd in block-raw-posix.c

[Anthony Liguori]

Avi Kivity wrote:
> Anthony Liguori wrote:
>> Avi Kivity wrote:
>>> Anthony Liguori wrote:
>>>> We don't use signalfd in upstream QEMU.  Instead, we always emulate 
>>>> it.
>>>>   
>>>
>>> With an extra thread -> so an extra context switch.
>>
>> We don't use an extra thread.  We just install a signal handler that 
>> writes to a pipe.  At best, the added overhead is that we get EINTRs 
>> more often but this is something we already handle.
>
> Oh okay.  But signal delivery is slow; for example the FPU needs to be 
> reset.

Is it really justified to add all of this extra code (including signalfd 
emulation) for something that probably isn't even measurable?

I like using wiz-bang features of Linux as much as the next guy, but I 
think we're stretching to justify it here :-)

-- 
Regards,

Anthony Liguori

Re: [PATCH 14/21] Remove -cpu-vendor-string

[Avi Kivity]

Anthony Liguori wrote:
>>>>> This isn't in upstream QEMU and is of little utility to KVM.  It's 
>>>>> unlikely
>>>>> to appear in upstream QEMU either.
>>>>>   
>>>>
>>>> Since we allow overriding cpuid flags, why not the vendor string?  
>>>> It's necessary for cpu passthrough.
>>>>
>>> But we don't allow explicit override of cpuid flags today.  We 
>>> support choosing CPU models which include vendor id and cpuid flags.
>>
>> I think we allow "-cpu qemu64,-nx" for example.
>
> Funny enough, -cpu qemu64,vendor="AuthenticAMD" already works today.  
> So yeah, there's no reason to carry -cpu-vendor-string anymore.
>

Applied, but had to reverse the sense of the commit log :)

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

Re: [PATCH 03/21] Remove use of signalfd in block-raw-posix.c

[Avi Kivity]

Anthony Liguori wrote:
>>
>> Oh okay.  But signal delivery is slow; for example the FPU needs to 
>> be reset.
>
> Is it really justified to add all of this extra code (including 
> signalfd emulation) for something that probably isn't even measurable?

We don't have to add signalfd emulation; we can simply use signal+pipe 
in that case.

We won't know if it's measurable or not until we measure it (or not).

>
> I like using wiz-bang features of Linux as much as the next guy, but I 
> think we're stretching to justify it here :-)
>

I think it's worth it in this case.  It will become more important in 
time, too.

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

Re: [PATCH 18/21] Remove host_alarm_timer hacks.

[Anthony Liguori]

Avi Kivity wrote:
> Anthony Liguori wrote:
>> In modern KVM, the IO thread is capable of interrupting the CPU 
>> whenever it needs to process IO.  Therefore this "problem" no longer 
>> exists.
>>
>
> It would still be good to verify that the problem no longer exists.  
> This is not a cosmetic change; some testing is needed to verify it 
> doesn't introduce new latencies.
>

N.B. dynticks is the preferred timer in QEMU on Linux.  To even hit this 
code path, you'd have to use an explicit -clock hpet or -clock rtc.  I 
don't have an hpet on my laptop and -clock rtc boots just as fast as it 
did before.

Do we really care about optimizing latency with -clock rtc though?

-- 
Regards,

Anthony Liguori

Re: [PATCH 16/21] Remove clean rule change

[Hollis Blanchard]

On Thu, 2009-04-30 at 12:42 +0300, Avi Kivity wrote:
> Anthony Liguori wrote:
> > It's not in upstream QEMU so apparently it's not useful.
> >
> > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> > ---
> >  pc-bios/Makefile |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/pc-bios/Makefile b/pc-bios/Makefile
> > index dabeb4c..315288d 100644
> > --- a/pc-bios/Makefile
> > +++ b/pc-bios/Makefile
> > @@ -16,4 +16,4 @@ all: $(TARGETS)
> >  	dtc -I dts -O dtb -o $@ $<
> >  
> >  clean:
> > -	rm -f $(TARGETS) *.o *~ *.dtb
> > +	rm -f $(TARGETS) *.o *~
> >   
> 
> Hollis?

dtb is the compiled (binary) form of dts (source) device tree files.

Think of it like bios.bin: if make clean doesn't delete bios.bin (and it
looks like it doesn't), neither should it delete *.dtb, and we can drop
the patch.

Acked-by: Hollis Blanchard <hollisb@us.ibm.com>

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [PATCH 16/21] Remove clean rule change

[Avi Kivity]

Hollis Blanchard wrote:
> dtb is the compiled (binary) form of dts (source) device tree files.
>
> Think of it like bios.bin: if make clean doesn't delete bios.bin (and it
> looks like it doesn't), neither should it delete *.dtb, and we can drop
> the patch.
>
> Acked-by: Hollis Blanchard <hollisb@us.ibm.com>
>   

make clean doesn't delete bios.bin, because bios.bin is under source 
control (as it requires special tools to build).

I see that *.dtb is also under source control, so will apply the patch.

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

Re: [PATCH 18/21] Remove host_alarm_timer hacks.

[Avi Kivity]

Anthony Liguori wrote:
> Avi Kivity wrote:
>> Anthony Liguori wrote:
>>> In modern KVM, the IO thread is capable of interrupting the CPU 
>>> whenever it needs to process IO.  Therefore this "problem" no longer 
>>> exists.
>>>
>>
>> It would still be good to verify that the problem no longer exists.  
>> This is not a cosmetic change; some testing is needed to verify it 
>> doesn't introduce new latencies.
>>
>
> N.B. dynticks is the preferred timer in QEMU on Linux.  To even hit 
> this code path, you'd have to use an explicit -clock hpet or -clock 
> rtc.  I don't have an hpet on my laptop and -clock rtc boots just as 
> fast as it did before.

I'll apply this and see what happens.

>
> Do we really care about optimizing latency with -clock rtc though?
>

People still run kvm on RHEL 5 (or cheap clones thereof), aren't they 
affected?

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

Re: [PATCH 18/21] Remove host_alarm_timer hacks.

[Anthony Liguori]

Avi Kivity wrote:
>>
>> Do we really care about optimizing latency with -clock rtc though?
>>
>
> People still run kvm on RHEL 5 (or cheap clones thereof), aren't they 
> affected?

Do they use -clock rtc?  -clock dynticks should still work on RHEL 5 
it's just that you won't get very accurate timer events.

You can only use -clock rtc with a single guest at a time so I doubt 
people use it seriously.  The other option would be -clock unix but I 
can't see why you'd use -clock unix instead of -clock dynticks.

The only reason to keep -clock unix around is for non Linux unices.

-- 
Regards,

Anthony Liguori

Re: [PATCH 18/21] Remove host_alarm_timer hacks.

[Avi Kivity]

Anthony Liguori wrote:
> Avi Kivity wrote:
>>>
>>> Do we really care about optimizing latency with -clock rtc though?
>>>
>>
>> People still run kvm on RHEL 5 (or cheap clones thereof), aren't they 
>> affected?
>
> Do they use -clock rtc?  -clock dynticks should still work on RHEL 5 
> it's just that you won't get very accurate timer events.
>
> You can only use -clock rtc with a single guest at a time so I doubt 
> people use it seriously.  The other option would be -clock unix but I 
> can't see why you'd use -clock unix instead of -clock dynticks.
>
> The only reason to keep -clock unix around is for non Linux unices.
>

Oh, okay then.

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

Re: [PATCH 03/21] Remove use of signalfd in block-raw-posix.c

[Avi Kivity]

Avi Kivity wrote:
> Anthony Liguori wrote:
>>>
>>> Oh okay.  But signal delivery is slow; for example the FPU needs to 
>>> be reset.
>>
>> Is it really justified to add all of this extra code (including 
>> signalfd emulation) for something that probably isn't even measurable?
>
> We don't have to add signalfd emulation; we can simply use signal+pipe 
> in that case.
>
> We won't know if it's measurable or not until we measure it (or not).
>
>>
>> I like using wiz-bang features of Linux as much as the next guy, but 
>> I think we're stretching to justify it here :-)
>>
>
> I think it's worth it in this case.  It will become more important in 
> time, too.
>

Out of curiosity, I measured this:

[avi@balrog test]$ ./signal
 2777 ns/signal (pipe)
  844 ns/signal (signalfd)


At 10000 signals/sec, this will save about 2% cpu time.  It's definitely 
worthwhile for the handful of lines it takes.

test program:

#include <signal.h>
#include <unistd.h>
#include <sys/signalfd.h>
#include <sys/time.h>
#include <stdio.h>

static int wfd;

static void handler(int signum)
{
    char b;

    write(wfd, &b, 1);
}

static int create_pipe(void)
{
    int fd[2];
    sigset_t s;

    pipe(fd);
    wfd = fd[1];
    signal(SIGUSR1, handler);
    sigemptyset(&s);
    sigaddset(&s, SIGUSR1);
    sigprocmask(SIG_UNBLOCK, &s, NULL);
    return fd[0];
}

static int create_signalfd(void)
{
    sigset_t s;

    sigemptyset(&s);
    sigaddset(&s, SIGUSR1);
    sigprocmask(SIG_BLOCK, &s, NULL);
    return signalfd(-1, &s, 0);
}

static uint64_t time_usec(void)
{
    struct timeval tv;

    gettimeofday(&tv, NULL);
    return (uint64_t)tv.tv_sec * 1000000 + tv.tv_usec;
}

#define N 10000000

static void test(const char *name, int fd, int len)
{
    int i;
    uint64_t t1, t2;
    char buf[128];

    t1 = time_usec();
    for (i = 0; i < N; ++i) {
        raise(SIGUSR1);
        read(fd, buf, len);
    }
    t2 = time_usec();

    close(fd);

    printf("%5d ns/signal (%s)\n", 1000 * (t2 - t1) / N, name);
}

int main(int ac, char **av)
{
    test("pipe", create_pipe(), 1);
    test("signalfd", create_signalfd(), 128);
    return 0;
}


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