Skip to contentSkip to conversation

qemu-kvm: x86: Refactor use of interrupt_bitmap

PATCH In review Jan Kiszka 3 participants 14 messages Nov 12, 2009 – Dec 10, 2009 KVM

lore
Share & tools
lore

Jan Kiszka

author
#0 lore raw

PATCH 1/2 qemu-kvm: x86: Refactor use of interrupt_bitmap

Drop interrupt_bitmap from the cpustate and solely rely on the integer interrupt_injected. This prepares us for the new injected-interrupt interface, which will deprecate the bitmap, while preserving compatibility.

Signed-off-by Jan Kiszka <[email protected]>

---

 qemu-kvm-x86.c        |   19 +++++++++++++++++--
 target-i386/cpu.h     |    3 +--
 target-i386/kvm.c     |   24 +++++++++++++++++-------
 target-i386/machine.c |   24 ++----------------------
 4 files changed, 37 insertions(+), 33 deletions(-)
qemu-kvm-x86.c +17 -2
index 9df0d83..e03a4ba 100644
@@ -946,7 +946,11 @@ void kvm_arch_load_regs(CPUState *env)
     fpu.mxcsr = env->mxcsr;
     kvm_set_fpu(env, &fpu);
 
-    memcpy(sregs.interrupt_bitmap, env->interrupt_bitmap, sizeof(sregs.interrupt_bitmap));
+    memset(sregs.interrupt_bitmap, 0, sizeof(sregs.interrupt_bitmap));
+    if (env->interrupt_injected >= 0) {
+        sregs.interrupt_bitmap[env->interrupt_injected / 64] |=
+                (uint64_t)1 << (env->interrupt_injected % 64);
+    }
 
     if ((env->eflags & VM_MASK)) {
 	    set_v8086_seg(&sregs.cs, &env->segs[R_CS]);

@@ -1104,7 +1108,14 @@ void kvm_arch_save_regs(CPUState *env)
 
     kvm_get_sregs(env, &sregs);
 
-    memcpy(env->interrupt_bitmap, sregs.interrupt_bitmap, sizeof(env->interrupt_bitmap));
+    env->interrupt_injected = -1;
+    for (i = 0; i < ARRAY_SIZE(sregs.interrupt_bitmap); i++) {
+        if (sregs.interrupt_bitmap[i]) {
+            n = ctz64(sregs.interrupt_bitmap[i]);
+            env->interrupt_injected = i * 64 + n;
+            break;
+        }
+    }
 
     get_seg(&env->segs[R_CS], &sregs.cs);
     get_seg(&env->segs[R_DS], &sregs.ds);

@@ -1371,6 +1382,9 @@ int kvm_arch_init_vcpu(CPUState *cenv)
 #ifdef KVM_EXIT_TPR_ACCESS
     kvm_tpr_vcpu_start(cenv);
 #endif
+
+    cenv->interrupt_injected = -1;
+
     return 0;
 }
 

@@ -1439,6 +1453,7 @@ void kvm_arch_push_nmi(void *opaque)
 
 void kvm_arch_cpu_reset(CPUState *env)
 {
+    env->interrupt_injected = -1;
     kvm_arch_load_regs(env);
     if (!cpu_is_bsp(env)) {
 	if (kvm_irqchip_in_kernel()) {
target-i386/cpu.h +1 -2
index 4605fd2..a638e70 100644
@@ -709,8 +709,8 @@ typedef struct CPUX86State {
     MTRRVar mtrr_var[8];
 
     /* For KVM */
-    uint64_t interrupt_bitmap[256 / 64];
     uint32_t mp_state;
+    int32_t interrupt_injected;
 
     /* in order to simplify APIC support, we leave this pointer to the
        user */

@@ -727,7 +727,6 @@ typedef struct CPUX86State {
     uint16_t fpus_vmstate;
     uint16_t fptag_vmstate;
     uint16_t fpregs_format_vmstate;
-    int32_t pending_irq_vmstate;
 } CPUX86State;
 
 CPUX86State *cpu_x86_init(const char *cpu_model);
target-i386/kvm.c +17 -7
index 24c9903..33f7d65 100644
@@ -23,6 +23,7 @@
 #include "kvm.h"
 #include "cpu.h"
 #include "gdbstub.h"
+#include "host-utils.h"
 
 #ifdef KVM_UPSTREAM
 //#define DEBUG_KVM

@@ -408,9 +409,11 @@ static int kvm_put_sregs(CPUState *env)
 {
     struct kvm_sregs sregs;
 
-    memcpy(sregs.interrupt_bitmap,
-           env->interrupt_bitmap,
-           sizeof(sregs.interrupt_bitmap));
+    memset(sregs.interrupt_bitmap, 0, sizeof(sregs.interrupt_bitmap));
+    if (env->interrupt_injected >= 0) {
+        sregs.interrupt_bitmap[env->interrupt_injected / 64] |=
+                (uint64_t)1 << (env->interrupt_injected % 64);
+    }
 
     if ((env->eflags & VM_MASK)) {
 	    set_v8086_seg(&sregs.cs, &env->segs[R_CS]);

@@ -518,15 +521,22 @@ static int kvm_get_sregs(CPUState *env)
 {
     struct kvm_sregs sregs;
     uint32_t hflags;
-    int ret;
+    int bit, i, ret;
 
     ret = kvm_vcpu_ioctl(env, KVM_GET_SREGS, &sregs);
     if (ret < 0)
         return ret;
 
-    memcpy(env->interrupt_bitmap, 
-           sregs.interrupt_bitmap,
-           sizeof(sregs.interrupt_bitmap));
+    /* There can only be one pending IRQ set in the bitmap at a time, so try
+       to find it and save its number instead (-1 for none). */
+    env->interrupt_injected = -1;
+    for (i = 0; i < ARRAY_SIZE(sregs.interrupt_bitmap); i++) {
+        if (sregs.interrupt_bitmap[i]) {
+            bit = ctz64(sregs.interrupt_bitmap[i]);
+            env->interrupt_injected = i * 64 + bit;
+            break;
+        }
+    }
 
     get_seg(&env->segs[R_CS], &sregs.cs);
     get_seg(&env->segs[R_DS], &sregs.ds);
target-i386/machine.c +2 -22
index 2b88fea..6bd447f 100644
@@ -2,7 +2,6 @@
 #include "hw/boards.h"
 #include "hw/pc.h"
 #include "hw/isa.h"
-#include "host-utils.h"
 
 #include "exec-all.h"
 #include "kvm.h"

@@ -321,7 +320,7 @@ static const VMStateInfo vmstate_hack_uint64_as_uint32 = {
 static void cpu_pre_save(void *opaque)
 {
     CPUState *env = opaque;
-    int i, bit;
+    int i;
 
     cpu_synchronize_state(env);
     kvm_save_mpstate(env);

@@ -338,17 +337,6 @@ static void cpu_pre_save(void *opaque)
 #else
     env->fpregs_format_vmstate = 1;
 #endif
-
-    /* There can only be one pending IRQ set in the bitmap at a time, so try
-       to find it and save its number instead (-1 for none). */
-    env->pending_irq_vmstate = -1;
-    for (i = 0; i < ARRAY_SIZE(env->interrupt_bitmap); i++) {
-        if (env->interrupt_bitmap[i]) {
-            bit = ctz64(env->interrupt_bitmap[i]);
-            env->pending_irq_vmstate = i * 64 + bit;
-            break;
-        }
-    }
 }
 
 static int cpu_pre_load(void *opaque)

@@ -377,14 +365,6 @@ static int cpu_post_load(void *opaque, int version_id)
     for (i = 0; i < 4; i++)
         hw_breakpoint_insert(env, i);
 
-    if (version_id >= 9) {
-        memset(&env->interrupt_bitmap, 0, sizeof(env->interrupt_bitmap));
-        if (env->pending_irq_vmstate >= 0) {
-            env->interrupt_bitmap[env->pending_irq_vmstate / 64] |=
-                (uint64_t)1 << (env->pending_irq_vmstate % 64);
-        }
-    }
-
     tlb_flush(env, 1);
     kvm_load_mpstate(env);
 

@@ -469,7 +449,7 @@ static const VMStateDescription vmstate_cpu = {
         VMSTATE_UINT64_V(mtrr_deftype, CPUState, 8),
         VMSTATE_MTRR_VARS(mtrr_var, CPUState, 8, 8),
         /* KVM-related states */
-        VMSTATE_INT32_V(pending_irq_vmstate, CPUState, 9),
+        VMSTATE_INT32_V(interrupt_injected, CPUState, 9),
         VMSTATE_UINT32_V(mp_state, CPUState, 9),
         VMSTATE_UINT64_V(tsc, CPUState, 9),
         /* MCE */
author
#1 lore raw

PATCH 2/2 qemu-kvm: x86: Add support for VCPU event states

This patch extends the qemu-kvm state sync logic with support for KVM_GET/SET_VCPU_EVENTS, giving access to yet missing exception, interrupt and NMI states.

Signed-off-by Jan Kiszka <[email protected]>

---

 qemu-kvm-x86.c        |   64 +++++++++++++++++++++++++++++++++++++++++++++++++
 target-i386/cpu.h     |    5 ++++
 target-i386/machine.c |    5 ++++
 3 files changed, 74 insertions(+), 0 deletions(-)
qemu-kvm-x86.c +64 -0
index e03a4ba..577c5db 100644
@@ -903,6 +903,60 @@ static void get_seg(SegmentCache *lhs, const struct kvm_segment *rhs)
 	| (rhs->avl * DESC_AVL_MASK);
 }
 
+static void kvm_get_vcpu_events(CPUState *env)
+{
+#ifdef KVM_CAP_VCPU_EVENTS
+    struct kvm_vcpu_events events;
+    int r;
+
+    r = kvm_vcpu_ioctl(env, KVM_GET_VCPU_EVENTS, &events);
+    if (r == 0) {
+        env->exception_index =
+            events.exception.injected ? events.exception.nr : -1;
+        env->has_error_code = events.exception.has_error_code;
+        env->error_code = events.exception.error_code;
+
+        env->interrupt_injected =
+            events.interrupt.injected ? events.interrupt.nr : -1;
+        env->soft_interrupt = events.interrupt.soft;
+
+        env->nmi_injected = events.nmi.injected;
+        env->nmi_pending = events.nmi.pending;
+        if (events.nmi.masked) {
+            env->hflags2 |= HF2_NMI_MASK;
+        } else {
+            env->hflags2 &= ~HF2_NMI_MASK;
+        }
+
+        env->sipi_vector = events.sipi_vector;
+    }
+#endif
+}
+
+static void kvm_set_vcpu_events(CPUState *env)
+{
+#ifdef KVM_CAP_VCPU_EVENTS
+    struct kvm_vcpu_events events;
+
+    events.exception.injected = (env->exception_index >= 0);
+    events.exception.nr = env->exception_index;
+    events.exception.has_error_code = env->has_error_code;
+    events.exception.error_code = env->error_code;
+
+    events.interrupt.injected = (env->interrupt_injected >= 0);
+    events.interrupt.nr = env->interrupt_injected;
+    events.interrupt.soft = env->soft_interrupt;
+
+    events.nmi.injected = env->nmi_injected;
+    events.nmi.pending = env->nmi_pending;
+    events.nmi.masked = !!(env->hflags2 & HF2_NMI_MASK);
+
+    events.sipi_vector = env->sipi_vector;
+
+    kvm_vcpu_ioctl(env, KVM_SET_VCPU_EVENTS, &events);
+#endif
+}
+
 void kvm_arch_load_regs(CPUState *env)
 {
     struct kvm_regs regs;

@@ -1019,6 +1073,8 @@ void kvm_arch_load_regs(CPUState *env)
     rc = kvm_set_msrs(env, msrs, n);
     if (rc == -1)
         perror("kvm_set_msrs FAILED");
+
+    kvm_set_vcpu_events(env);
 }
 
 void kvm_load_tsc(CPUState *env)

@@ -1215,6 +1271,8 @@ void kvm_arch_save_regs(CPUState *env)
                 return;
         }
     }
+
+    kvm_get_vcpu_events(env);
 }
 
 static void do_cpuid_ent(struct kvm_cpuid_entry2 *e, uint32_t function,

@@ -1383,7 +1441,10 @@ int kvm_arch_init_vcpu(CPUState *cenv)
     kvm_tpr_vcpu_start(cenv);
 #endif
 
+    cenv->exception_index = -1;
     cenv->interrupt_injected = -1;
+    cenv->nmi_injected = 0;
+    cenv->nmi_pending = 0;
 
     return 0;
 }

@@ -1453,7 +1514,10 @@ void kvm_arch_push_nmi(void *opaque)
 
 void kvm_arch_cpu_reset(CPUState *env)
 {
+    env->exception_index = -1;
     env->interrupt_injected = -1;
+    env->nmi_injected = 0;
+    env->nmi_pending = 0;
     kvm_arch_load_regs(env);
     if (!cpu_is_bsp(env)) {
 	if (kvm_irqchip_in_kernel()) {
target-i386/cpu.h +5 -0
index a638e70..31412a8 100644
@@ -711,6 +711,11 @@ typedef struct CPUX86State {
     /* For KVM */
     uint32_t mp_state;
     int32_t interrupt_injected;
+    uint8_t soft_interrupt;
+    uint8_t nmi_injected;
+    uint8_t nmi_pending;
+    uint8_t has_error_code;
+    uint32_t sipi_vector;
 
     /* in order to simplify APIC support, we leave this pointer to the
        user */
target-i386/machine.c +5 -0
index 6bd447f..1eda7c5 100644
@@ -452,6 +452,11 @@ static const VMStateDescription vmstate_cpu = {
         VMSTATE_INT32_V(interrupt_injected, CPUState, 9),
         VMSTATE_UINT32_V(mp_state, CPUState, 9),
         VMSTATE_UINT64_V(tsc, CPUState, 9),
+        VMSTATE_UINT8_V(soft_interrupt, CPUState, 11),
+        VMSTATE_UINT8_V(nmi_injected, CPUState, 11),
+        VMSTATE_UINT8_V(nmi_pending, CPUState, 11),
+        VMSTATE_UINT8_V(has_error_code, CPUState, 11),
+        VMSTATE_UINT32_V(sipi_vector, CPUState, 11),
         /* MCE */
         VMSTATE_UINT64_V(mcg_cap, CPUState, 10),
         VMSTATE_UINT64_V(mcg_status, CPUState, 10),
#2 lore raw

Re: [PATCH 2/2] qemu-kvm: x86: Add support for VCPU event states

Quoted 12 lines · Jan Kiszka

This patch extends the qemu-kvm state sync logic with support for KVM_GET/SET_VCPU_EVENTS, giving access to yet missing exception, interrupt and NMI states.

target-i386/machine.c +0 -0
index 6bd447f..1eda7c5 100644
@@ -452,6 +452,11 @@ static const VMStateDescription vmstate_cpu = {
          VMSTATE_INT32_V(interrupt_injected, CPUState, 9),

target-i386/machine.c static const VMStateDescription vmstate_cpu = { · line 462

Quoted 10 lines
target-i386/machine.c +5 -0
          VMSTATE_UINT32_V(mp_state, CPUState, 9),
          VMSTATE_UINT64_V(tsc, CPUState, 9),
+        VMSTATE_UINT8_V(soft_interrupt, CPUState, 11),
+        VMSTATE_UINT8_V(nmi_injected, CPUState, 11),
+        VMSTATE_UINT8_V(nmi_pending, CPUState, 11),
+        VMSTATE_UINT8_V(has_error_code, CPUState, 11),
+        VMSTATE_UINT32_V(sipi_vector, CPUState, 11),
          /* MCE */
          VMSTATE_UINT64_V(mcg_cap, CPUState, 10),
          VMSTATE_UINT64_V(mcg_status, CPUState, 10),

Is there a reason why you add 11 between 9 and 10? We'll probably see another 11 when someone else adds the next state.

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#3 lore raw

Re: [PATCH 2/2] qemu-kvm: x86: Add support for VCPU event states

Quoted 18 lines · Avi Kivity
Quoted 17 lines · Jan Kiszka

This patch extends the qemu-kvm state sync logic with support for KVM_GET/SET_VCPU_EVENTS, giving access to yet missing exception, interrupt and NMI states.

target-i386/machine.c +5 -0
index 6bd447f..1eda7c5 100644
@@ -452,6 +452,11 @@ static const VMStateDescription vmstate_cpu = {
          VMSTATE_INT32_V(interrupt_injected, CPUState, 9),
          VMSTATE_UINT32_V(mp_state, CPUState, 9),
          VMSTATE_UINT64_V(tsc, CPUState, 9),
+        VMSTATE_UINT8_V(soft_interrupt, CPUState, 11),
+        VMSTATE_UINT8_V(nmi_injected, CPUState, 11),
+        VMSTATE_UINT8_V(nmi_pending, CPUState, 11),
+        VMSTATE_UINT8_V(has_error_code, CPUState, 11),
+        VMSTATE_UINT32_V(sipi_vector, CPUState, 11),
Quoted 9 lines

target-i386/machine.c static const VMStateDescription vmstate_cpu = { · line 462

Quoted 5 lines
target-i386/machine.c +0 -0
          /* MCE */
          VMSTATE_UINT64_V(mcg_cap, CPUState, 10),
          VMSTATE_UINT64_V(mcg_status, CPUState, 10),

Is there a reason why you add 11 between 9 and 10? We'll probably see another 11 when someone else adds the next state.

Logical grouping ("/* KVM-related states */"). If anyone once tries to add non-KVM stuff here just because it's version 12, it should be rejected. I don't think you have to sort VMSTATE entries by their version number. Am I right, Juan?

Jan

#4 lore raw

Re: [PATCH 2/2] qemu-kvm: x86: Add support for VCPU event states

Quoted 31 lines · Jan Kiszka
Quoted 19 lines · Avi Kivity
Quoted 17 lines · Jan Kiszka

This patch extends the qemu-kvm state sync logic with support for KVM_GET/SET_VCPU_EVENTS, giving access to yet missing exception, interrupt and NMI states.

target-i386/machine.c +5 -0
index 6bd447f..1eda7c5 100644
@@ -452,6 +452,11 @@ static const VMStateDescription vmstate_cpu = {
           VMSTATE_INT32_V(interrupt_injected, CPUState, 9),
           VMSTATE_UINT32_V(mp_state, CPUState, 9),
           VMSTATE_UINT64_V(tsc, CPUState, 9),
+        VMSTATE_UINT8_V(soft_interrupt, CPUState, 11),
+        VMSTATE_UINT8_V(nmi_injected, CPUState, 11),
+        VMSTATE_UINT8_V(nmi_pending, CPUState, 11),
+        VMSTATE_UINT8_V(has_error_code, CPUState, 11),
+        VMSTATE_UINT32_V(sipi_vector, CPUState, 11),
Quoted 10 lines

target-i386/machine.c static const VMStateDescription vmstate_cpu = { · line 462

Quoted 6 lines
target-i386/machine.c +0 -0
           /* MCE */
           VMSTATE_UINT64_V(mcg_cap, CPUState, 10),
           VMSTATE_UINT64_V(mcg_status, CPUState, 10),

Is there a reason why you add 11 between 9 and 10? We'll probably see another 11 when someone else adds the next state.

Quoted 1 line

Logical grouping ("/* KVM-related states */").

These aren't kvm-related, just not implemented in tcg yet. Nothing kvmish about them - it's all architectural state.

Quoted 5 lines

If anyone once tries to
add non-KVM stuff here just because it's version 12, it should be rejected. I don't think you have to sort VMSTATE entries by their version number. Am I right, Juan?

I'm worried about something else - someone looking at the end, seeing version 10, and appending new state with version 11.

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#5 lore raw

Re: [PATCH 2/2] qemu-kvm: x86: Add support for VCPU event states

Quoted 32 lines · Avi Kivity
Quoted 31 lines · Jan Kiszka
Quoted 29 lines · Avi Kivity
Quoted 13 lines · Jan Kiszka

This patch extends the qemu-kvm state sync logic with support for KVM_GET/SET_VCPU_EVENTS, giving access to yet missing exception, interrupt and NMI states.

target-i386/machine.c +0 -0
index 6bd447f..1eda7c5 100644
@@ -452,6 +452,11 @@ static const VMStateDescription vmstate_cpu = {
           VMSTATE_INT32_V(interrupt_injected, CPUState, 9),

target-i386/machine.c static const VMStateDescription vmstate_cpu = { · line 462

Quoted 10 lines
target-i386/machine.c +5 -0
           VMSTATE_UINT32_V(mp_state, CPUState, 9),
           VMSTATE_UINT64_V(tsc, CPUState, 9),
+        VMSTATE_UINT8_V(soft_interrupt, CPUState, 11),
+        VMSTATE_UINT8_V(nmi_injected, CPUState, 11),
+        VMSTATE_UINT8_V(nmi_pending, CPUState, 11),
+        VMSTATE_UINT8_V(has_error_code, CPUState, 11),
+        VMSTATE_UINT32_V(sipi_vector, CPUState, 11),
           /* MCE */
           VMSTATE_UINT64_V(mcg_cap, CPUState, 10),
           VMSTATE_UINT64_V(mcg_status, CPUState, 10),

Is there a reason why you add 11 between 9 and 10? We'll probably see another 11 when someone else adds the next state.

Quoted 4 lines
Quoted 1 line

Logical grouping ("/* KVM-related states */").

These aren't kvm-related, just not implemented in tcg yet. Nothing kvmish about them - it's all architectural state.

Most of them are KVM-specific. TCG don't have to deal with event re-injection due to host page faults etc. on first try.

Quoted 9 lines
Quoted 6 lines

If anyone once tries to
add non-KVM stuff here just because it's version 12, it should be rejected. I don't think you have to sort VMSTATE entries by their version number. Am I right, Juan?

I'm worried about something else - someone looking at the end, seeing version 10, and appending new state with version 11.

Again, that's something proper review should catch (just like checking for the right place when adding a new IOCTL to kvm.h).

Jan

#6 lore raw

Re: [PATCH 2/2] qemu-kvm: x86: Add support for VCPU event states

Quoted 6 lines · Jan Kiszka
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines

Is there a reason why you add 11 between 9 and 10? We'll probably see another 11 when someone else adds the next state.

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

Logical grouping ("/* KVM-related states */").

These aren't kvm-related, just not implemented in tcg yet. Nothing kvmish about them - it's all architectural state.

Most of them are KVM-specific. TCG don't have to deal with event re-injection due to host page faults etc. on first try.

Right, tcg can do some things atomically. There's some non-kvm state in there.

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines

If anyone once tries to
add non-KVM stuff here just because it's version 12, it should be rejected. I don't think you have to sort VMSTATE entries by their version number. Am I right, Juan?

Quoted 6 lines
Quoted 3 lines

I'm worried about something else - someone looking at the end, seeing version 10, and appending new state with version 11.

Again, that's something proper review should catch (just like checking for the right place when adding a new IOCTL to kvm.h).

And we know how well that works. We should try to make things work by default and leave the review to catch the really difficult things (like trailing whitespace).

At least add a comment at the end warning people that simple append will fail.

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#7 lore raw

Re: [PATCH 2/2] qemu-kvm: x86: Add support for VCPU event states

Quoted 32 lines · Avi Kivity
Quoted 6 lines · Jan Kiszka
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines

Is there a reason why you add 11 between 9 and 10? We'll probably see another 11 when someone else adds the next state.

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

Logical grouping ("/* KVM-related states */").

These aren't kvm-related, just not implemented in tcg yet. Nothing kvmish about them - it's all architectural state.

Most of them are KVM-specific. TCG don't have to deal with event re-injection due to host page faults etc. on first try.

Right, tcg can do some things atomically. There's some non-kvm state in there.

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines

If anyone once tries to
add non-KVM stuff here just because it's version 12, it should be rejected. I don't think you have to sort VMSTATE entries by their version number. Am I right, Juan?

Quoted 6 lines
Quoted 3 lines

I'm worried about something else - someone looking at the end, seeing version 10, and appending new state with version 11.

Again, that's something proper review should catch (just like checking for the right place when adding a new IOCTL to kvm.h).

Quoted 7 lines

And we know how well that works. We should try to make things work by default and leave the review to catch the really difficult things (like trailing whitespace).

At least add a comment at the end warning people that simple append will fail.

Where should I add "/* next version to use: 13 */", and who will take care that this comment will also be kept up to date? The CPU vmstate is already ordered according to logical groups, just look at earlier field. Only recent KVM additions happened to create some version ordering as well.

Jan

#8 lore raw

Re: [PATCH 2/2] qemu-kvm: x86: Add support for VCPU event states

Quoted 6 lines · Jan Kiszka

Where should I add "/* next version to use: 13 */", and who will take care that this comment will also be kept up to date? The CPU vmstate is already ordered according to logical groups, just look at earlier field. Only recent KVM additions happened to create some version ordering as well.

Er, now I'm confused. 11 and 12 indeed do already exist, so how can you update 11 retroactively?

Shouldn't you create 13 now?

(and I meant: /* The above list is not sorted wrt version, watch out! */, but now I feel I'm missing something).

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#9 lore raw

Re: [PATCH 2/2] qemu-kvm: x86: Add support for VCPU event states

Quoted 11 lines · Avi Kivity
Quoted 7 lines · Jan Kiszka

Where should I add "/* next version to use: 13 */", and who will take care that this comment will also be kept up to date? The CPU vmstate is already ordered according to logical groups, just look at earlier field. Only recent KVM additions happened to create some version ordering as well.

Er, now I'm confused. 11 and 12 indeed do already exist, so how can you update 11 retroactively?

Oh, right, good that we discuss this. My patch dated back before the kvmclock addition, which IMHO incorrectly bumped the version numbers. I think the current policy in upstream is that we only increment once per qemu release, not per bit added.

Quoted 2 lines

Shouldn't you create 13 now?

No, I rather think Glauber's 12 should be downgraded to 11 - unless it misses the qemu merge windows for 0.12. My extensions definitely target that release, thus will likely carry 11 in upstream. And we should really try to avoid diverging again.

Quoted 4 lines

(and I meant: /* The above list is not sorted wrt version, watch out! */, but now I feel I'm missing something).

Ok, can add such a note at the end.

Jan

#10 lore raw

Re: [PATCH 2/2] qemu-kvm: x86: Add support for VCPU event states

Quoted 11 lines · Jan Kiszka
Quoted 9 lines · Avi Kivity
Quoted 7 lines · Jan Kiszka

Where should I add "/* next version to use: 13 */", and who will take care that this comment will also be kept up to date? The CPU vmstate is already ordered according to logical groups, just look at earlier field. Only recent KVM additions happened to create some version ordering as well.

Quoted 8 lines
Quoted 3 lines

Er, now I'm confused. 11 and 12 indeed do already exist, so how can you update 11 retroactively?

Oh, right, good that we discuss this. My patch dated back before the kvmclock addition, which IMHO incorrectly bumped the version numbers. I think the current policy in upstream is that we only increment once per qemu release, not per bit added.

What about stable releases? If we need to port a commit which adds a bit, we need to port the entire thing.

(well version numbers don't work with nonlinear development anyway).

Quoted 7 lines
Quoted 2 lines

Shouldn't you create 13 now?

No, I rather think Glauber's 12 should be downgraded to 11 - unless it misses the qemu merge windows for 0.12. My extensions definitely target that release, thus will likely carry 11 in upstream. And we should really try to avoid diverging again.

Agree. Will commit the patches.

Quoted 7 lines
Quoted 4 lines

(and I meant: /* The above list is not sorted wrt version, watch out! */, but now I feel I'm missing something).

Ok, can add such a note at the end.

In upstream...

Signature · 1 line
error compiling committee.c: too many arguments to function
#11 lore raw

Re: [PATCH 2/2] qemu-kvm: x86: Add support for VCPU event states

Quoted 8 lines · Avi Kivity
Quoted 6 lines
Quoted 2 lines

Shouldn't you create 13 now?

No, I rather think Glauber's 12 should be downgraded to 11 - unless it misses the qemu merge windows for 0.12. My extensions definitely target that release, thus will likely carry 11 in upstream. And we should really try to avoid diverging again.

Agree. Will commit the patches.

Ugh, so I did it the hard way by merging upstream and porting it (incorrectly), then going back to my queue to see that you had already nicely prepared this for me.

Signature · 1 line
error compiling committee.c: too many arguments to function
#12 lore raw

Re: [PATCH 2/2] qemu-kvm: x86: Add support for VCPU event states

In reply to Jan Kiszka
Quoted 19 lines · Jan Kiszka
Quoted 11 lines · Avi Kivity
Quoted 7 lines · Jan Kiszka

Where should I add "/* next version to use: 13 */", and who will take care that this comment will also be kept up to date? The CPU vmstate is already ordered according to logical groups, just look at earlier field. Only recent KVM additions happened to create some version ordering as well.

Er, now I'm confused. 11 and 12 indeed do already exist, so how can you update 11 retroactively?

Oh, right, good that we discuss this. My patch dated back before the kvmclock addition, which IMHO incorrectly bumped the version numbers. I think the current policy in upstream is that we only increment once per qemu release, not per bit added.

Quoted 1 line
Quoted 6 lines
Quoted 1 line

Shouldn't you create 13 now?

No, I rather think Glauber's 12 should be downgraded to 11 - unless it misses the qemu merge windows for 0.12. My extensions definitely target that release, thus will likely carry 11 in upstream. And we should really try to avoid diverging again.

Agree.

Anthony,

Are Glauber patches going to make it for 0.12, so we can revert current qemu-kvm's CPU_SAVE_VERSION from 12 to 11?

Thanks.

Quoted 10 lines
Quoted 5 lines

(and I meant: /* The above list is not sorted wrt version, watch out! */, but now I feel I'm missing something).

Ok, can add such a note at the end.

Jan

#13 lore raw

Re: [PATCH 2/2] qemu-kvm: x86: Add support for VCPU event states

Quoted 27 lines · Marcelo Tosatti
Quoted 19 lines · Jan Kiszka
Quoted 11 lines · Avi Kivity
Quoted 7 lines · Jan Kiszka

Where should I add "/* next version to use: 13 */", and who will take care that this comment will also be kept up to date? The CPU vmstate is already ordered according to logical groups, just look at earlier field. Only recent KVM additions happened to create some version ordering as well.

Er, now I'm confused. 11 and 12 indeed do already exist, so how can you update 11 retroactively?

Oh, right, good that we discuss this. My patch dated back before the kvmclock addition, which IMHO incorrectly bumped the version numbers. I think the current policy in upstream is that we only increment once per qemu release, not per bit added.

Quoted 1 line
Quoted 6 lines
Quoted 1 line

Shouldn't you create 13 now?

No, I rather think Glauber's 12 should be downgraded to 11 - unless it misses the qemu merge windows for 0.12. My extensions definitely target that release, thus will likely carry 11 in upstream. And we should really try to avoid diverging again.

Quoted 8 lines

Agree.

Anthony,

Are Glauber patches going to make it for 0.12, so we can revert current qemu-kvm's CPU_SAVE_VERSION from 12 to 11?

Thanks.

Anthony,

I still don't see this patch in for qemu 0.12. Please merge it.

Date: Thu, 22 Oct 2009 10:26:56 -0200
From: Glauber Costa <[email protected]>
To: [email protected]
Subject: [Qemu-devel] [PATCH] v2: properly save kvm system time msr registers
Message-Id: <[email protected]>

TIA