[RFC][PATCH] qemu-kvm: x86: Refactor persistent CPU state

[RFC][PATCH] qemu-kvm: x86: Refactor persistent CPU state

[Jan Kiszka]

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

This patch reworks the KVM-related layout and encoding of the CPU state
to be saved to disk or migrated. The goal is to define a format, version
9, that is also acceptable for upstream and can later be merged into
QEMU. Besides unconditionally writing KVM states, this format compresses
interrupt_bitmap into a single number as there can be no more than one
pending IRQ at a time.

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

 target-i386/cpu.h     |    2 +-
 target-i386/machine.c |   57 +++++++++++++++++++++++++++++++++++--------------
 2 files changed, 42 insertions(+), 17 deletions(-)

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index f054af1..e07b504 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -838,7 +838,7 @@ static inline int cpu_get_time_fast(void)
 #define cpu_signal_handler cpu_x86_signal_handler
 #define cpu_list x86_cpu_list
 
-#define CPU_SAVE_VERSION 8
+#define CPU_SAVE_VERSION 9
 
 /* MMU modes definitions */
 #define MMU_MODE0_SUFFIX _kernel
diff --git a/target-i386/machine.c b/target-i386/machine.c
index 399204d..18ba983 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -37,7 +37,8 @@ void cpu_save(QEMUFile *f, void *opaque)
     uint16_t fptag, fpus, fpuc, fpregs_format;
     uint32_t hflags;
     int32_t a20_mask;
-    int i;
+    int32_t pending_irq;
+    int i, bit;
 
     if (kvm_enabled()) {
         kvm_save_registers(env);
@@ -152,13 +153,21 @@ void cpu_save(QEMUFile *f, void *opaque)
         qemu_put_be64s(f, &env->mtrr_var[i].mask);
     }
 
-    if (kvm_enabled()) {
-        for (i = 0; i < sizeof(env->interrupt_bitmap)/8 ; i++) {
-            qemu_put_be64s(f, &env->interrupt_bitmap[i]);
+    /* KVM-related states */
+
+    /* 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). */
+    pending_irq = -1;
+    for (i = 0; i < sizeof(env->interrupt_bitmap)/2; i++) {
+        bit = ffs(((uint16_t *)env->interrupt_bitmap)[i]);
+        if (bit) {
+            pending_irq = i * 16 + bit;
+            break;
         }
-        qemu_put_be64s(f, &env->tsc);
-        qemu_put_be32s(f, &env->mp_state);
     }
+    qemu_put_sbe32s(f, &pending_irq);
+    qemu_put_be32s(f, &env->mp_state);
+    qemu_put_be64s(f, &env->tsc);
 }
 
 #ifdef USE_X86LDOUBLE
@@ -192,9 +201,9 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id)
     uint32_t hflags;
     uint16_t fpus, fpuc, fptag, fpregs_format;
     int32_t a20_mask;
+    int32_t pending_irq;
 
-    if (version_id != 3 && version_id != 4 && version_id != 5
-        && version_id != 6 && version_id != 7 && version_id != 8)
+    if (version_id < 3 || version_id > CPU_SAVE_VERSION)
         return -EINVAL;
     for(i = 0; i < CPU_NB_REGS; i++)
         qemu_get_betls(f, &env->regs[i]);
@@ -339,6 +348,16 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id)
         }
     }
 
+    if (version_id >= 9) {
+        qemu_get_sbe32s(f, &pending_irq);
+        memset(&env->interrupt_bitmap, 0, sizeof(env->interrupt_bitmap));
+        if (pending_irq >= 0) {
+            env->interrupt_bitmap[pending_irq / 64] |= 1 << (pending_irq % 64);
+        }
+        qemu_get_be32s(f, &env->mp_state);
+        qemu_get_be64s(f, &env->tsc);
+    }
+
     /* XXX: ensure compatiblity for halted bit ? */
     /* XXX: compute redundant hflags bits */
     env->hflags = hflags;
@@ -347,14 +366,20 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id)
         /* when in-kernel irqchip is used, env->halted causes deadlock
            because no userspace IRQs will ever clear this flag */
         env->halted = 0;
-        for (i = 0; i < sizeof(env->interrupt_bitmap)/8; i++) {
-            qemu_get_be64s(f, &env->interrupt_bitmap[i]);
-        }
-        qemu_get_be64s(f, &env->tsc);
-        kvm_load_registers(env);
-        kvm_load_tsc(env);
-        if (version_id >= 5) {
-            qemu_get_be32s(f, &env->mp_state);
+        if (version_id < 9) {
+            for (i = 0; i < sizeof(env->interrupt_bitmap)/8; i++) {
+                qemu_get_be64s(f, &env->interrupt_bitmap[i]);
+            }
+            qemu_get_be64s(f, &env->tsc);
+            kvm_load_registers(env);
+            kvm_load_tsc(env);
+            if (version_id >= 5) {
+                qemu_get_be32s(f, &env->mp_state);
+                kvm_load_mpstate(env);
+            }
+        } else {
+            kvm_load_registers(env);
+            kvm_load_tsc(env);
             kvm_load_mpstate(env);
         }
     }


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

Re: [RFC][PATCH] qemu-kvm: x86: Refactor persistent CPU state

[Avi Kivity]

Jan Kiszka wrote:
> This patch reworks the KVM-related layout and encoding of the CPU state
> to be saved to disk or migrated. The goal is to define a format, version
> 9, that is also acceptable for upstream and can later be merged into
> QEMU. Besides unconditionally writing KVM states, this format compresses
> interrupt_bitmap into a single number as there can be no more than one
> pending IRQ at a time.
>
>  
> -    if (kvm_enabled()) {
> -        for (i = 0; i < sizeof(env->interrupt_bitmap)/8 ; i++) {
> -            qemu_put_be64s(f, &env->interrupt_bitmap[i]);
> +    /* KVM-related states */
> +
> +    /* 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). */
> +    pending_irq = -1;
> +    for (i = 0; i < sizeof(env->interrupt_bitmap)/2; i++) {
> +        bit = ffs(((uint16_t *)env->interrupt_bitmap)[i]);
> +        if (bit) {
> +            pending_irq = i * 16 + bit;
>   

bit - 1

> +            break;
>          }
>   

Why are you using uint64_t?  It would be more natural to use ffsll and 
the natural type of the bitmap vector.

> +        } else {
> +            kvm_load_registers(env);
> +            kvm_load_tsc(env);
>              kvm_load_mpstate(env);
>          }
>      }
>   

Don't you need to load the pending interrupt?

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

[PATCH v2] qemu-kvm: x86: Refactor persistent CPU state

[Jan Kiszka]

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

Avi Kivity wrote:
> Jan Kiszka wrote:
>> This patch reworks the KVM-related layout and encoding of the CPU state
>> to be saved to disk or migrated. The goal is to define a format, version
>> 9, that is also acceptable for upstream and can later be merged into
>> QEMU. Besides unconditionally writing KVM states, this format compresses
>> interrupt_bitmap into a single number as there can be no more than one
>> pending IRQ at a time.
>>
>>  
>> -    if (kvm_enabled()) {
>> -        for (i = 0; i < sizeof(env->interrupt_bitmap)/8 ; i++) {
>> -            qemu_put_be64s(f, &env->interrupt_bitmap[i]);
>> +    /* KVM-related states */
>> +
>> +    /* 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). */
>> +    pending_irq = -1;
>> +    for (i = 0; i < sizeof(env->interrupt_bitmap)/2; i++) {
>> +        bit = ffs(((uint16_t *)env->interrupt_bitmap)[i]);
>> +        if (bit) {
>> +            pending_irq = i * 16 + bit;
>>   
> 
> bit - 1

Fixed.

> 
>> +            break;
>>          }
>>   
> 
> Why are you using uint64_t?  It would be more natural to use ffsll and
> the natural type of the bitmap vector.

Better don't ask how I interpreted the ffs doc.

> 
>> +        } else {
>> +            kvm_load_registers(env);
>> +            kvm_load_tsc(env);
>>              kvm_load_mpstate(env);
>>          }
>>      }
>>   
> 
> Don't you need to load the pending interrupt?
> 

For >= v9 this is done in the previous hunk.

------------>

This patch reworks the KVM-related layout and encoding of the CPU state
to be saved to disk or migrated. The goal is to define a format, version
9, that is also acceptable for upstream and can later be merged into
QEMU. Besides unconditionally writing KVM states, this format compresses
interrupt_bitmap into a single number as there can be no more than one
pending IRQ at a time.

Changes in v2:
 - fixed and simplified interrupt_bitmap parsing

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

 target-i386/cpu.h     |    2 +-
 target-i386/machine.c |   57 +++++++++++++++++++++++++++++++++++--------------
 2 files changed, 42 insertions(+), 17 deletions(-)

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index f054af1..e07b504 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -838,7 +838,7 @@ static inline int cpu_get_time_fast(void)
 #define cpu_signal_handler cpu_x86_signal_handler
 #define cpu_list x86_cpu_list
 
-#define CPU_SAVE_VERSION 8
+#define CPU_SAVE_VERSION 9
 
 /* MMU modes definitions */
 #define MMU_MODE0_SUFFIX _kernel
diff --git a/target-i386/machine.c b/target-i386/machine.c
index 59472d0..67a6e60 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -28,7 +28,8 @@ void cpu_save(QEMUFile *f, void *opaque)
     uint16_t fptag, fpus, fpuc, fpregs_format;
     uint32_t hflags;
     int32_t a20_mask;
-    int i;
+    int32_t pending_irq;
+    int i, bit;
 
     if (kvm_enabled()) {
         kvm_save_registers(env);
@@ -143,13 +144,21 @@ void cpu_save(QEMUFile *f, void *opaque)
         qemu_put_be64s(f, &env->mtrr_var[i].mask);
     }
 
-    if (kvm_enabled()) {
-        for (i = 0; i < sizeof(env->interrupt_bitmap)/8 ; i++) {
-            qemu_put_be64s(f, &env->interrupt_bitmap[i]);
+    /* KVM-related states */
+
+    /* 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). */
+    pending_irq = -1;
+    for (i = 0; i < ARRAY_SIZE(env->interrupt_bitmap); i++) {
+        bit = ffsll(env->interrupt_bitmap[i]);
+        if (bit) {
+            pending_irq = i * 64 + bit - 1;
+            break;
         }
-        qemu_put_be64s(f, &env->tsc);
-        qemu_put_be32s(f, &env->mp_state);
     }
+    qemu_put_sbe32s(f, &pending_irq);
+    qemu_put_be32s(f, &env->mp_state);
+    qemu_put_be64s(f, &env->tsc);
 }
 
 #ifdef USE_X86LDOUBLE
@@ -183,9 +192,9 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id)
     uint32_t hflags;
     uint16_t fpus, fpuc, fptag, fpregs_format;
     int32_t a20_mask;
+    int32_t pending_irq;
 
-    if (version_id != 3 && version_id != 4 && version_id != 5
-        && version_id != 6 && version_id != 7 && version_id != 8)
+    if (version_id < 3 || version_id > CPU_SAVE_VERSION)
         return -EINVAL;
     for(i = 0; i < CPU_NB_REGS; i++)
         qemu_get_betls(f, &env->regs[i]);
@@ -330,6 +339,16 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id)
         }
     }
 
+    if (version_id >= 9) {
+        qemu_get_sbe32s(f, &pending_irq);
+        memset(&env->interrupt_bitmap, 0, sizeof(env->interrupt_bitmap));
+        if (pending_irq >= 0) {
+            env->interrupt_bitmap[pending_irq / 64] |= 1 << (pending_irq % 64);
+        }
+        qemu_get_be32s(f, &env->mp_state);
+        qemu_get_be64s(f, &env->tsc);
+    }
+
     /* XXX: ensure compatiblity for halted bit ? */
     /* XXX: compute redundant hflags bits */
     env->hflags = hflags;
@@ -338,14 +357,20 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id)
         /* when in-kernel irqchip is used, env->halted causes deadlock
            because no userspace IRQs will ever clear this flag */
         env->halted = 0;
-        for (i = 0; i < sizeof(env->interrupt_bitmap)/8; i++) {
-            qemu_get_be64s(f, &env->interrupt_bitmap[i]);
-        }
-        qemu_get_be64s(f, &env->tsc);
-        kvm_load_registers(env);
-        kvm_load_tsc(env);
-        if (version_id >= 5) {
-            qemu_get_be32s(f, &env->mp_state);
+        if (version_id < 9) {
+            for (i = 0; i < sizeof(env->interrupt_bitmap)/8; i++) {
+                qemu_get_be64s(f, &env->interrupt_bitmap[i]);
+            }
+            qemu_get_be64s(f, &env->tsc);
+            kvm_load_registers(env);
+            kvm_load_tsc(env);
+            if (version_id >= 5) {
+                qemu_get_be32s(f, &env->mp_state);
+                kvm_load_mpstate(env);
+            }
+        } else {
+            kvm_load_registers(env);
+            kvm_load_tsc(env);
             kvm_load_mpstate(env);
         }
     }


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

Re: [PATCH v2] qemu-kvm: x86: Refactor persistent CPU state

[Avi Kivity]

Jan Kiszka wrote:
>> Why are you using uint64_t?  It would be more natural to use ffsll and
>> the natural type of the bitmap vector.
>>     
>
> Better don't ask how I interpreted the ffs doc.
>   

It's a misdesigned API anyway.

>  
> +    if (version_id >= 9) {
> +        qemu_get_sbe32s(f, &pending_irq);
> +        memset(&env->interrupt_bitmap, 0, sizeof(env->interrupt_bitmap));
> +        if (pending_irq >= 0) {
> +            env->interrupt_bitmap[pending_irq / 64] |= 1 << (pending_irq % 64);
>   

(uint64_t)1 so we don't lose the upper word.



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

[PATCH v3] qemu-kvm: x86: Refactor persistent CPU state

[Jan Kiszka]

This patch reworks the KVM-related layout and encoding of the CPU state
to be saved to disk or migrated. The goal is to define a format, version
9, that is also acceptable for upstream and can later be merged into
QEMU. Besides unconditionally writing KVM states, this format compresses
interrupt_bitmap into a single number as there can be no more than one
pending IRQ at a time.

Changes in v3:
 - fixed bit-shift overflow
 - replace upstream v9 format (should be merged upstream quickly)

Changes in v2:
 - fixed and simplified interrupt_bitmap parsing

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

 target-i386/machine.c |   51 ++++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/target-i386/machine.c b/target-i386/machine.c
index 124f8dc..f280d3d 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -29,7 +29,8 @@ void cpu_save(QEMUFile *f, void *opaque)
     uint16_t fptag, fpus, fpuc, fpregs_format;
     uint32_t hflags;
     int32_t a20_mask;
-    int i;
+    int32_t pending_irq;
+    int i, bit;
 
     cpu_synchronize_state(env, 0);
 
@@ -141,11 +142,21 @@ void cpu_save(QEMUFile *f, void *opaque)
         qemu_put_be64s(f, &env->mtrr_var[i].mask);
     }
 
-    for (i = 0; i < sizeof(env->interrupt_bitmap)/8; i++) {
-        qemu_put_be64s(f, &env->interrupt_bitmap[i]);
+    /* KVM-related states */
+
+    /* 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). */
+    pending_irq = -1;
+    for (i = 0; i < ARRAY_SIZE(env->interrupt_bitmap); i++) {
+        bit = ffsll(env->interrupt_bitmap[i]);
+        if (bit) {
+            pending_irq = i * 64 + bit - 1;
+            break;
+        }
     }
-    qemu_put_be64s(f, &env->tsc);
+    qemu_put_sbe32s(f, &pending_irq);
     qemu_put_be32s(f, &env->mp_state);
+    qemu_put_be64s(f, &env->tsc);
 }
 
 #ifdef USE_X86LDOUBLE
@@ -179,6 +190,7 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id)
     uint32_t hflags;
     uint16_t fpus, fpuc, fptag, fpregs_format;
     int32_t a20_mask;
+    int32_t pending_irq;
 
     if (version_id < 3 || version_id > CPU_SAVE_VERSION)
         return -EINVAL;
@@ -324,12 +336,16 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id)
             qemu_get_be64s(f, &env->mtrr_var[i].mask);
         }
     }
+
     if (version_id >= 9) {
-        for (i = 0; i < sizeof(env->interrupt_bitmap)/8; i++) {
-            qemu_get_be64s(f, &env->interrupt_bitmap[i]);
+        qemu_get_sbe32s(f, &pending_irq);
+        memset(&env->interrupt_bitmap, 0, sizeof(env->interrupt_bitmap));
+        if (pending_irq >= 0) {
+            env->interrupt_bitmap[pending_irq / 64] |=
+                (uint64_t)1 << (pending_irq % 64);
         }
-        qemu_get_be64s(f, &env->tsc);
         qemu_get_be32s(f, &env->mp_state);
+        qemu_get_be64s(f, &env->tsc);
     }
 
     /* XXX: ensure compatiblity for halted bit ? */
@@ -337,5 +353,26 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id)
     env->hflags = hflags;
     tlb_flush(env, 1);
     cpu_synchronize_state(env, 1);
+    if (kvm_enabled()) {
+        /* when in-kernel irqchip is used, env->halted causes deadlock
+           because no userspace IRQs will ever clear this flag */
+        env->halted = 0;
+        if (version_id < 9) {
+            for (i = 0; i < sizeof(env->interrupt_bitmap)/8; i++) {
+                qemu_get_be64s(f, &env->interrupt_bitmap[i]);
+            }
+            qemu_get_be64s(f, &env->tsc);
+            kvm_load_registers(env);
+            kvm_load_tsc(env);
+            if (version_id >= 5) {
+                qemu_get_be32s(f, &env->mp_state);
+                kvm_load_mpstate(env);
+            }
+        } else {
+            kvm_load_registers(env);
+            kvm_load_tsc(env);
+            kvm_load_mpstate(env);
+        }
+    }
     return 0;
 }

Re: [PATCH v3] qemu-kvm: x86: Refactor persistent CPU state

[Avi Kivity]

Jan Kiszka wrote:
> This patch reworks the KVM-related layout and encoding of the CPU state
> to be saved to disk or migrated. The goal is to define a format, version
> 9, that is also acceptable for upstream and can later be merged into
> QEMU. Besides unconditionally writing KVM states, this format compresses
> interrupt_bitmap into a single number as there can be no more than one
> pending IRQ at a time.
>   

Applied, thanks.

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

Re: [PATCH v3] qemu-kvm: x86: Refactor persistent CPU state

[Jan Kiszka]

Avi Kivity wrote:
> Jan Kiszka wrote:
>> This patch reworks the KVM-related layout and encoding of the CPU state
>> to be saved to disk or migrated. The goal is to define a format, version
>> 9, that is also acceptable for upstream and can later be merged into
>> QEMU. Besides unconditionally writing KVM states, this format compresses
>> interrupt_bitmap into a single number as there can be no more than one
>> pending IRQ at a time.
>>   
> 
> Applied, thanks.

OK, will push its core upstream now.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux