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

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 6802 bytes --]

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 <jan.kiszka@siemens.com>
---

 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(-)

diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
index 9df0d83..e03a4ba 100644
--- a/qemu-kvm-x86.c
+++ b/qemu-kvm-x86.c
@@ -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()) {
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 4605fd2..a638e70 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -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);
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 24c9903..33f7d65 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -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);
diff --git a/target-i386/machine.c b/target-i386/machine.c
index 2b88fea..6bd447f 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -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 */


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 4822 bytes --]

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 <jan.kiszka@siemens.com>
---

 qemu-kvm-x86.c        |   64 +++++++++++++++++++++++++++++++++++++++++++++++++
 target-i386/cpu.h     |    5 ++++
 target-i386/machine.c |    5 ++++
 3 files changed, 74 insertions(+), 0 deletions(-)

diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
index e03a4ba..577c5db 100644
--- a/qemu-kvm-x86.c
+++ b/qemu-kvm-x86.c
@@ -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()) {
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index a638e70..31412a8 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -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 */
diff --git a/target-i386/machine.c b/target-i386/machine.c
index 6bd447f..1eda7c5 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -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),


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

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

[Avi Kivity]

On 11/12/2009 02:05 AM, Jan Kiszka wrote:
> 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.
>
> diff --git a/target-i386/machine.c b/target-i386/machine.c
> index 6bd447f..1eda7c5 100644
> --- a/target-i386/machine.c
> +++ b/target-i386/machine.c
> @@ -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),
>
>    

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.

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

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1469 bytes --]

Avi Kivity wrote:
> On 11/12/2009 02:05 AM, Jan Kiszka wrote:
>> 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.
>>
>> diff --git a/target-i386/machine.c b/target-i386/machine.c
>> index 6bd447f..1eda7c5 100644
>> --- a/target-i386/machine.c
>> +++ b/target-i386/machine.c
>> @@ -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),
>>
>>    
> 
> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

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

[Avi Kivity]

On 11/15/2009 04:21 PM, Jan Kiszka wrote:
> Avi Kivity wrote:
>    
>> On 11/12/2009 02:05 AM, Jan Kiszka wrote:
>>      
>>> 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.
>>>
>>> diff --git a/target-i386/machine.c b/target-i386/machine.c
>>> index 6bd447f..1eda7c5 100644
>>> --- a/target-i386/machine.c
>>> +++ b/target-i386/machine.c
>>> @@ -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),
>>>
>>>
>>>        
>> 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 */").

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

> 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.

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

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2171 bytes --]

Avi Kivity wrote:
> On 11/15/2009 04:21 PM, Jan Kiszka wrote:
>> Avi Kivity wrote:
>>   
>>> On 11/12/2009 02:05 AM, Jan Kiszka wrote:
>>>     
>>>> 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.
>>>>
>>>> diff --git a/target-i386/machine.c b/target-i386/machine.c
>>>> index 6bd447f..1eda7c5 100644
>>>> --- a/target-i386/machine.c
>>>> +++ b/target-i386/machine.c
>>>> @@ -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),
>>>>
>>>>
>>>>        
>>> 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 */").
> 
> 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.

> 
>> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

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

[Avi Kivity]

On 11/15/2009 04:31 PM, Jan Kiszka wrote:
>>>>
>>>> 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 */").
>>>        
>> 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.

>>> 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).
>    

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.

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

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1771 bytes --]

Avi Kivity wrote:
> On 11/15/2009 04:31 PM, Jan Kiszka wrote:
>>>>>
>>>>> 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 */").
>>>>        
>>> 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.
> 
>>>> 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).
>>    
> 
> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

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

[Avi Kivity]

On 11/15/2009 05:02 PM, Jan Kiszka wrote:
>
> 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).

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

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1215 bytes --]

Avi Kivity wrote:
> On 11/15/2009 05:02 PM, Jan Kiszka wrote:
>>
>> 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.

> 
> 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.

> 
> (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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

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

[Avi Kivity]

On 11/15/2009 05:41 PM, Jan Kiszka wrote:
> Avi Kivity wrote:
>    
>> On 11/15/2009 05:02 PM, Jan Kiszka wrote:
>>      
>>> 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.
>    

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).

>> 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.

>> (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...

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

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

[Avi Kivity]

On 11/15/2009 05:50 PM, Avi Kivity wrote:
>
>>> 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.

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

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

[Marcelo Tosatti]

On Sun, Nov 15, 2009 at 04:41:26PM +0100, Jan Kiszka wrote:
> Avi Kivity wrote:
> > On 11/15/2009 05:02 PM, Jan Kiszka wrote:
> >>
> >> 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.
> 
> > 
> > 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.

> 
> > 
> > (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
> 

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

[Marcelo Tosatti]

On Tue, Nov 24, 2009 at 08:08:37PM -0200, Marcelo Tosatti wrote:
> On Sun, Nov 15, 2009 at 04:41:26PM +0100, Jan Kiszka wrote:
> > Avi Kivity wrote:
> > > On 11/15/2009 05:02 PM, Jan Kiszka wrote:
> > >>
> > >> 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.
> > 
> > > 
> > > 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.

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 <glommer@redhat.com>
To: qemu-devel@nongnu.org
Subject: [Qemu-devel] [PATCH] v2: properly save kvm system time msr
registers
Message-Id: <1256214416-20554-1-git-send-email-glommer@redhat.com>

TIA

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

[Jan Kiszka]

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

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Note: A corresponding version for upstream is on the way as well.

 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(-)

diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
index 9df0d83..e03a4ba 100644
--- a/qemu-kvm-x86.c
+++ b/qemu-kvm-x86.c
@@ -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()) {
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 4605fd2..a638e70 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -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);
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 24c9903..33f7d65 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -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);
diff --git a/target-i386/machine.c b/target-i386/machine.c
index 2b88fea..6bd447f 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -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 */