[Qemu-devel] [PATCH 0/4] KVM pull request: Various fixes and cleanups

[Qemu-devel] [PATCH 0/4] KVM pull request: Various fixes and cleanups

[Jan Kiszka]

As discussed in http://www.spinics.net/lists/kvm/msg28517.html, this is
the first part of the KVM upstream patches developed while cleaning up
qemu-kvm.

Please pull from

	git://git.kiszka.org/qemu.git queues/kvm

Jan Kiszka (4):
  KVM: x86: Fix up misreported CPU features
  KVM: Make vmport KVM-compatible
  KVM: Move and rename regs_modified
  KVM: Rework of guest debug state writing

 cpu-defs.h        |    3 ++-
 hw/vmport.c       |    3 +++
 kvm-all.c         |   21 ++++++++-------------
 target-i386/cpu.h |    9 ++++++++-
 target-i386/kvm.c |   19 ++++++++++++++++++-
 5 files changed, 39 insertions(+), 16 deletions(-)

[Qemu-devel] [PATCH 1/4] KVM: x86: Fix up misreported CPU features

[Jan Kiszka]

>From qemu-kvm: Kernels before 2.6.30 misreported some essential CPU
features via KVM_GET_SUPPORTED_CPUID. Fix them up.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 target-i386/kvm.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 5b093ce..f690b2e 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -99,12 +99,18 @@ uint32_t kvm_arch_get_supported_cpuid(CPUState *env, uint32_t function, int reg)
                 break;
             case R_EDX:
                 ret = cpuid->entries[i].edx;
-                if (function == 0x80000001) {
+                switch (function) {
+                case 1:
+                    /* KVM before 2.6.30 misreports the following features */
+                    ret |= CPUID_MTRR | CPUID_PAT | CPUID_MCE | CPUID_MCA;
+                    break;
+                case 0x80000001:
                     /* On Intel, kvm returns cpuid according to the Intel spec,
                      * so add missing bits according to the AMD spec:
                      */
                     cpuid_1_edx = kvm_arch_get_supported_cpuid(env, 1, R_EDX);
                     ret |= cpuid_1_edx & 0xdfeff7ff;
+                    break;
                 }
                 break;
             }
-- 
1.6.0.2

[Qemu-devel] [PATCH 2/4] KVM: Make vmport KVM-compatible

[Jan Kiszka]

The vmport "device" accesses the VCPU registers, so it requires proper
cpu_synchronize_state. Add it to vmport_ioport_read, which also
synchronizes vmport_ioport_write.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/vmport.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/hw/vmport.c b/hw/vmport.c
index 884af3f..6c9d7c9 100644
--- a/hw/vmport.c
+++ b/hw/vmport.c
@@ -25,6 +25,7 @@
 #include "isa.h"
 #include "pc.h"
 #include "sysemu.h"
+#include "kvm.h"
 
 //#define VMPORT_DEBUG
 
@@ -58,6 +59,8 @@ static uint32_t vmport_ioport_read(void *opaque, uint32_t addr)
     unsigned char command;
     uint32_t eax;
 
+    cpu_synchronize_state(env);
+
     eax = env->regs[R_EAX];
     if (eax != VMPORT_MAGIC)
         return eax;
-- 
1.6.0.2

[Qemu-devel] [PATCH 3/4] KVM: Move and rename regs_modified

[Jan Kiszka]

Touching the user space representation of KVM's VCPU state is -
naturally - a per-VCPU thing. So move the dirty flag into KVM_CPU_COMMON
and rename it at this chance to reflect its true meaning.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 cpu-defs.h |    3 ++-
 kvm-all.c  |   13 ++++++-------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/cpu-defs.h b/cpu-defs.h
index 95068b5..7fdbe97 100644
--- a/cpu-defs.h
+++ b/cpu-defs.h
@@ -197,6 +197,7 @@ typedef struct CPUWatchpoint {
     const char *cpu_model_str;                                          \
     struct KVMState *kvm_state;                                         \
     struct kvm_run *kvm_run;                                            \
-    int kvm_fd;
+    int kvm_fd;                                                         \
+    int kvm_vcpu_dirty;
 
 #endif
diff --git a/kvm-all.c b/kvm-all.c
index 15ec38e..67b44b5 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -57,7 +57,6 @@ struct KVMState
     KVMSlot slots[32];
     int fd;
     int vmfd;
-    int regs_modified;
     int coalesced_mmio;
     int broken_set_mem_region;
     int migration_log;
@@ -567,9 +566,9 @@ static void kvm_run_coalesced_mmio(CPUState *env, struct kvm_run *run)
 
 void kvm_cpu_synchronize_state(CPUState *env)
 {
-    if (!env->kvm_state->regs_modified) {
+    if (!env->kvm_vcpu_dirty) {
         kvm_arch_get_registers(env);
-        env->kvm_state->regs_modified = 1;
+        env->kvm_vcpu_dirty = 1;
     }
 }
 
@@ -587,9 +586,9 @@ int kvm_cpu_exec(CPUState *env)
             break;
         }
 
-        if (env->kvm_state->regs_modified) {
+        if (env->kvm_vcpu_dirty) {
             kvm_arch_put_registers(env);
-            env->kvm_state->regs_modified = 0;
+            env->kvm_vcpu_dirty = 0;
         }
 
         kvm_arch_pre_run(env, run);
@@ -939,9 +938,9 @@ static void kvm_invoke_set_guest_debug(void *data)
     struct kvm_set_guest_debug_data *dbg_data = data;
     CPUState *env = dbg_data->env;
 
-    if (env->kvm_state->regs_modified) {
+    if (env->kvm_vcpu_dirty) {
         kvm_arch_put_registers(env);
-        env->kvm_state->regs_modified = 0;
+        env->kvm_vcpu_dirty = 0;
     }
     dbg_data->err = kvm_vcpu_ioctl(env, KVM_SET_GUEST_DEBUG, &dbg_data->dbg);
 }
-- 
1.6.0.2

[Qemu-devel] [PATCH 4/4] KVM: Rework of guest debug state writing

[Jan Kiszka]

So far we synchronized any dirty VCPU state back into the kernel before
updating the guest debug state. This was a tribute to a deficit in x86
kernels before 2.6.33. But as this is an arch-dependent issue, it is
better handle in the x86 part of KVM and remove the writeback point for
generic code.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 kvm-all.c         |   12 ++++--------
 target-i386/cpu.h |    9 ++++++++-
 target-i386/kvm.c |   11 +++++++++++
 3 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 67b44b5..f3e09c8 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -938,10 +938,6 @@ static void kvm_invoke_set_guest_debug(void *data)
     struct kvm_set_guest_debug_data *dbg_data = data;
     CPUState *env = dbg_data->env;
 
-    if (env->kvm_vcpu_dirty) {
-        kvm_arch_put_registers(env);
-        env->kvm_vcpu_dirty = 0;
-    }
     dbg_data->err = kvm_vcpu_ioctl(env, KVM_SET_GUEST_DEBUG, &dbg_data->dbg);
 }
 
@@ -949,12 +945,12 @@ int kvm_update_guest_debug(CPUState *env, unsigned long reinject_trap)
 {
     struct kvm_set_guest_debug_data data;
 
-    data.dbg.control = 0;
-    if (env->singlestep_enabled)
-        data.dbg.control = KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
+    data.dbg.control = reinject_trap;
 
+    if (env->singlestep_enabled) {
+        data.dbg.control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
+    }
     kvm_arch_update_guest_debug(env, &data.dbg);
-    data.dbg.control |= reinject_trap;
     data.env = env;
 
     on_vcpu(env, kvm_invoke_set_guest_debug, &data);
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 216b00e..dfc4a26 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -21,6 +21,10 @@
 
 #include "config.h"
 
+#ifdef CONFIG_KVM
+#include <linux/kvm.h>  /* for kvm_guest_debug */
+#endif
+
 #ifdef TARGET_X86_64
 #define TARGET_LONG_BITS 64
 #else
@@ -702,7 +706,10 @@ typedef struct CPUX86State {
     uint8_t has_error_code;
     uint32_t sipi_vector;
     uint32_t cpuid_kvm_features;
-    
+#if defined(CONFIG_KVM) && defined(KVM_CAP_SET_GUEST_DEBUG)
+    struct kvm_guest_debug kvm_guest_debug;
+#endif
+
     /* in order to simplify APIC support, we leave this pointer to the
        user */
     struct APICState *apic_state;
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index f690b2e..2b3d8f6 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -851,6 +851,15 @@ int kvm_arch_put_registers(CPUState *env)
     if (ret < 0)
         return ret;
 
+    /*
+     * Kernels before 2.6.33 overwrote flags.TF injected via SET_GUEST_DEBUG
+     * while updating GP regs. Work around this by updating the debug state
+     * once again.
+     */
+    ret = kvm_vcpu_ioctl(env, KVM_SET_GUEST_DEBUG, &env->kvm_guest_debug);
+    if (ret < 0)
+        return ret;
+
     ret = kvm_put_fpu(env);
     if (ret < 0)
         return ret;
@@ -1147,5 +1156,7 @@ void kvm_arch_update_guest_debug(CPUState *env, struct kvm_guest_debug *dbg)
                 (len_code[hw_breakpoint[n].len] << (18 + n*4));
         }
     }
+    /* Keep a copy for the writeback workaround in kvm_arch_put_registers */
+    memcpy(&env->kvm_guest_debug, dbg, sizeof(env->kvm_guest_debug));
 }
 #endif /* KVM_CAP_SET_GUEST_DEBUG */
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH 0/4] KVM pull request: Various fixes and cleanups

[Anthony Liguori]

Hi Jan,

Thanks for sending this out.

On 02/03/2010 03:29 PM, Jan Kiszka wrote:
> As discussed in http://www.spinics.net/lists/kvm/msg28517.html, this is
> the first part of the KVM upstream patches developed while cleaning up
> qemu-kvm.
>    

I'd prefer if Marcelo/Avi took this first into qemu-kvm.git and then did 
a pull request for me to pull it into upstream qemu.

I'm also happy to pull from multiple sources but I think the former work 
flow will do a better job at ensuring that qemu-kvm.git becomes the 
primary location of this work.

Regards,

Anthony Liguori

> Please pull from
>
> 	git://git.kiszka.org/qemu.git queues/kvm
>
> Jan Kiszka (4):
>    KVM: x86: Fix up misreported CPU features
>    KVM: Make vmport KVM-compatible
>    KVM: Move and rename regs_modified
>    KVM: Rework of guest debug state writing
>
>   cpu-defs.h        |    3 ++-
>   hw/vmport.c       |    3 +++
>   kvm-all.c         |   21 ++++++++-------------
>   target-i386/cpu.h |    9 ++++++++-
>   target-i386/kvm.c |   19 ++++++++++++++++++-
>   5 files changed, 39 insertions(+), 16 deletions(-)
>
>
>
>    

Re: [Qemu-devel] [PATCH 0/4] KVM pull request: Various fixes and cleanups

[Jan Kiszka]

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

Anthony Liguori wrote:
> Hi Jan,
> 
> Thanks for sending this out.
> 
> On 02/03/2010 03:29 PM, Jan Kiszka wrote:
>> As discussed in http://www.spinics.net/lists/kvm/msg28517.html, this is
>> the first part of the KVM upstream patches developed while cleaning up
>> qemu-kvm.
>>    
> 
> I'd prefer if Marcelo/Avi took this first into qemu-kvm.git and then did
> a pull request for me to pull it into upstream qemu.
> 
> I'm also happy to pull from multiple sources but I think the former work
> flow will do a better job at ensuring that qemu-kvm.git becomes the
> primary location of this work.
> 

Whatever is preferred - but, you all, please decide at some point what
you want here. :) This part of the series is easy to push around, but
I'm not keen on juggling too much with the heavier parts later on.

Jan


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

[Qemu-devel] Re: [PATCH 4/4] KVM: Rework of guest debug state writing

[Marcelo Tosatti]

On Wed, Feb 03, 2010 at 10:29:45PM +0100, Jan Kiszka wrote:
> So far we synchronized any dirty VCPU state back into the kernel before
> updating the guest debug state. This was a tribute to a deficit in x86
> kernels before 2.6.33. But as this is an arch-dependent issue, it is
> better handle in the x86 part of KVM and remove the writeback point for
> generic code.

Jan,

This patch breaks migration.

[Qemu-devel] Re: [PATCH 4/4] KVM: Rework of guest debug state writing

[Jan Kiszka]

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

Marcelo Tosatti wrote:
> On Wed, Feb 03, 2010 at 10:29:45PM +0100, Jan Kiszka wrote:
>> So far we synchronized any dirty VCPU state back into the kernel before
>> updating the guest debug state. This was a tribute to a deficit in x86
>> kernels before 2.6.33. But as this is an arch-dependent issue, it is
>> better handle in the x86 part of KVM and remove the writeback point for
>> generic code.
> 
> Jan,
> 
> This patch breaks migration.

Can you elaborate what you did? I can't reproduce, and I do not see any
conceptual issue (given that guest debugging conflicts with migration
anyway).

Jan


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

[Qemu-devel] Re: [PATCH 4/4] KVM: Rework of guest debug state writing

[Marcelo Tosatti]

On Thu, Feb 04, 2010 at 01:33:50AM +0100, Jan Kiszka wrote:
> Marcelo Tosatti wrote:
> > On Wed, Feb 03, 2010 at 10:29:45PM +0100, Jan Kiszka wrote:
> >> So far we synchronized any dirty VCPU state back into the kernel before
> >> updating the guest debug state. This was a tribute to a deficit in x86
> >> kernels before 2.6.33. But as this is an arch-dependent issue, it is
> >> better handle in the x86 part of KVM and remove the writeback point for
> >> generic code.
> > 
> > Jan,
> > 
> > This patch breaks migration.
> 
> Can you elaborate what you did? I can't reproduce, and I do not see any
> conceptual issue (given that guest debugging conflicts with migration
> anyway).

kvm-autotest fails (migration only, install is ok, both Linux and Win
guests). Not sure why, perhaps the unconditional KVM_SET_GUEST_DEBUG
corrupts state somehow? 

Tested with io thread enabled.

[Qemu-devel] Re: [PATCH 4/4] KVM: Rework of guest debug state writing

[Jan Kiszka]

Marcelo Tosatti wrote:
> On Thu, Feb 04, 2010 at 01:33:50AM +0100, Jan Kiszka wrote:
>> Marcelo Tosatti wrote:
>>> On Wed, Feb 03, 2010 at 10:29:45PM +0100, Jan Kiszka wrote:
>>>> So far we synchronized any dirty VCPU state back into the kernel before
>>>> updating the guest debug state. This was a tribute to a deficit in x86
>>>> kernels before 2.6.33. But as this is an arch-dependent issue, it is
>>>> better handle in the x86 part of KVM and remove the writeback point for
>>>> generic code.
>>> Jan,
>>>
>>> This patch breaks migration.
>> Can you elaborate what you did? I can't reproduce, and I do not see any
>> conceptual issue (given that guest debugging conflicts with migration
>> anyway).
> 
> kvm-autotest fails (migration only, install is ok, both Linux and Win
> guests). Not sure why, perhaps the unconditional KVM_SET_GUEST_DEBUG
> corrupts state somehow? 
> 
> Tested with io thread enabled.

That's this default-off thing, so... OK, confirmed, investigating.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH 4/4] KVM: Rework of guest debug state writing

[Jan Kiszka]

Jan Kiszka wrote:
> Marcelo Tosatti wrote:
>> On Thu, Feb 04, 2010 at 01:33:50AM +0100, Jan Kiszka wrote:
>>> Marcelo Tosatti wrote:
>>>> On Wed, Feb 03, 2010 at 10:29:45PM +0100, Jan Kiszka wrote:
>>>>> So far we synchronized any dirty VCPU state back into the kernel before
>>>>> updating the guest debug state. This was a tribute to a deficit in x86
>>>>> kernels before 2.6.33. But as this is an arch-dependent issue, it is
>>>>> better handle in the x86 part of KVM and remove the writeback point for
>>>>> generic code.
>>>> Jan,
>>>>
>>>> This patch breaks migration.
>>> Can you elaborate what you did? I can't reproduce, and I do not see any
>>> conceptual issue (given that guest debugging conflicts with migration
>>> anyway).
>> kvm-autotest fails (migration only, install is ok, both Linux and Win
>> guests). Not sure why, perhaps the unconditional KVM_SET_GUEST_DEBUG
>> corrupts state somehow? 
>>
>> Tested with io thread enabled.
> 
> That's this default-off thing, so... OK, confirmed, investigating.
> 

Heisenbug: It first also popped up (in form of a frozen migration
target) after removing this patch, but now it's totally unreproducible,
whatever patch I apply or revert from my series. Base is current master.
I tend to think there is a hidden issue of iothread vs. migration,
unrelated to this patch.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH 4/4] KVM: Rework of guest debug state writing

[Marcelo Tosatti]

On Thu, Feb 04, 2010 at 04:41:44PM +0100, Jan Kiszka wrote:
> Jan Kiszka wrote:
> > Marcelo Tosatti wrote:
> >> On Thu, Feb 04, 2010 at 01:33:50AM +0100, Jan Kiszka wrote:
> >>> Marcelo Tosatti wrote:
> >>>> On Wed, Feb 03, 2010 at 10:29:45PM +0100, Jan Kiszka wrote:
> >>>>> So far we synchronized any dirty VCPU state back into the kernel before
> >>>>> updating the guest debug state. This was a tribute to a deficit in x86
> >>>>> kernels before 2.6.33. But as this is an arch-dependent issue, it is
> >>>>> better handle in the x86 part of KVM and remove the writeback point for
> >>>>> generic code.
> >>>> Jan,
> >>>>
> >>>> This patch breaks migration.
> >>> Can you elaborate what you did? I can't reproduce, and I do not see any
> >>> conceptual issue (given that guest debugging conflicts with migration
> >>> anyway).
> >> kvm-autotest fails (migration only, install is ok, both Linux and Win
> >> guests). Not sure why, perhaps the unconditional KVM_SET_GUEST_DEBUG
> >> corrupts state somehow? 
> >>
> >> Tested with io thread enabled.
> > 
> > That's this default-off thing, so... OK, confirmed, investigating.
> > 
> 
> Heisenbug: It first also popped up (in form of a frozen migration
> target) after removing this patch, but now it's totally unreproducible,
> whatever patch I apply or revert from my series. Base is current master.
>
> I tend to think there is a hidden issue of iothread vs. migration,
> unrelated to this patch.

Probably many :)

Do you have c5f32c99c6855d466737daf1cd262e7e92062f87 (from qemu-kvm.git
uq/master) in?

With kvm-autotest the failure is not sporadic (and the above commit
applied): with KVM_SET_GUEST_DEBUG in arch_put_regs all migration 
tests fail, without, all of them succeed. 

So env->kvm_guest_debug has been zeroed by cpu_x86_init, which means
the writeback via KVM_SET_GUEST_DEBUG does almost nothing. It does
get_rflags and set_rflags in the kernel.

Test box is off, but the synchronous writeback via qemu_system_reset
in main, after machine and vcpu thread initialization, might be
problematic. But it would be nice to understand this.

Unrelated to this problem, won't put_vcpu_events, which is executed 
after KVM_SET_GUEST_DEBUG, overwrite any queued debug exceptions?

[Qemu-devel] Re: [PATCH 4/4] KVM: Rework of guest debug state writing

[Jan Kiszka]

Marcelo Tosatti wrote:
> On Thu, Feb 04, 2010 at 04:41:44PM +0100, Jan Kiszka wrote:
>> Jan Kiszka wrote:
>>> Marcelo Tosatti wrote:
>>>> On Thu, Feb 04, 2010 at 01:33:50AM +0100, Jan Kiszka wrote:
>>>>> Marcelo Tosatti wrote:
>>>>>> On Wed, Feb 03, 2010 at 10:29:45PM +0100, Jan Kiszka wrote:
>>>>>>> So far we synchronized any dirty VCPU state back into the kernel before
>>>>>>> updating the guest debug state. This was a tribute to a deficit in x86
>>>>>>> kernels before 2.6.33. But as this is an arch-dependent issue, it is
>>>>>>> better handle in the x86 part of KVM and remove the writeback point for
>>>>>>> generic code.
>>>>>> Jan,
>>>>>>
>>>>>> This patch breaks migration.
>>>>> Can you elaborate what you did? I can't reproduce, and I do not see any
>>>>> conceptual issue (given that guest debugging conflicts with migration
>>>>> anyway).
>>>> kvm-autotest fails (migration only, install is ok, both Linux and Win
>>>> guests). Not sure why, perhaps the unconditional KVM_SET_GUEST_DEBUG
>>>> corrupts state somehow? 
>>>>
>>>> Tested with io thread enabled.
>>> That's this default-off thing, so... OK, confirmed, investigating.
>>>
>> Heisenbug: It first also popped up (in form of a frozen migration
>> target) after removing this patch, but now it's totally unreproducible,
>> whatever patch I apply or revert from my series. Base is current master.
>>
>> I tend to think there is a hidden issue of iothread vs. migration,
>> unrelated to this patch.
> 
> Probably many :)
> 
> Do you have c5f32c99c6855d466737daf1cd262e7e92062f87 (from qemu-kvm.git
> uq/master) in?

Yes. And that might have been the reason why some early tests failed
when it was no yet applied here.

> 
> With kvm-autotest the failure is not sporadic (and the above commit
> applied): with KVM_SET_GUEST_DEBUG in arch_put_regs all migration 
> tests fail, without, all of them succeed. 
> 
> So env->kvm_guest_debug has been zeroed by cpu_x86_init, which means
> the writeback via KVM_SET_GUEST_DEBUG does almost nothing. It does
> get_rflags and set_rflags in the kernel.

Hmm, it also copies debug regs around... BTW, where do we save/restore
dr0..7 between kernel and user space?

But that should not be a problem, both shadow as well as effective regs
should be properly initialized, specifically for a newly created VCPU.

> 
> Test box is off, but the synchronous writeback via qemu_system_reset
> in main, after machine and vcpu thread initialization, might be
> problematic. But it would be nice to understand this.
> 
> Unrelated to this problem, won't put_vcpu_events, which is executed 
> after KVM_SET_GUEST_DEBUG, overwrite any queued debug exceptions?

Good point, SET_GUEST_DEBUG should be last in the writeback for that reason.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH 4/4] KVM: Rework of guest debug state writing

[Jan Kiszka]

Jan Kiszka wrote:
> Marcelo Tosatti wrote:
>>
>> Unrelated to this problem, won't put_vcpu_events, which is executed 
>> after KVM_SET_GUEST_DEBUG, overwrite any queued debug exceptions?
> 
> Good point, SET_GUEST_DEBUG should be last in the writeback for that reason.

Actually, we no longer need the exception injection via SET_GUEST_DEBUG
now that we have full access via vcpu_events. So this needs a cleanup,
and I'm afraid quite a few cases are broken ATM with vcpu_events
writeback overwriting the reinjected exceptions.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH 4/4] KVM: Rework of guest debug state writing

[Jan Kiszka]

Jan Kiszka wrote:
> Marcelo Tosatti wrote:
>> With kvm-autotest the failure is not sporadic (and the above commit
>> applied): with KVM_SET_GUEST_DEBUG in arch_put_regs all migration 
>> tests fail, without, all of them succeed. 
>>
>> So env->kvm_guest_debug has been zeroed by cpu_x86_init, which means
>> the writeback via KVM_SET_GUEST_DEBUG does almost nothing. It does
>> get_rflags and set_rflags in the kernel.
> 
> Hmm, it also copies debug regs around... BTW, where do we save/restore
> dr0..7 between kernel and user space?
> 
> But that should not be a problem, both shadow as well as effective regs
> should be properly initialized, specifically for a newly created VCPU.

Could you retry after pushing SET_GUEST_DEBUG at the end of
kvm_arch_put_registers? Maybe it is no good idea to run get/set_rflags
without having the sregs properly initialized.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH 4/4] KVM: Rework of guest debug state writing

[Marcelo Tosatti]

On Thu, Feb 04, 2010 at 08:21:08PM +0100, Jan Kiszka wrote:
> Jan Kiszka wrote:
> > Marcelo Tosatti wrote:
> >> With kvm-autotest the failure is not sporadic (and the above commit
> >> applied): with KVM_SET_GUEST_DEBUG in arch_put_regs all migration 
> >> tests fail, without, all of them succeed. 
> >>
> >> So env->kvm_guest_debug has been zeroed by cpu_x86_init, which means
> >> the writeback via KVM_SET_GUEST_DEBUG does almost nothing. It does
> >> get_rflags and set_rflags in the kernel.
> > 
> > Hmm, it also copies debug regs around... BTW, where do we save/restore
> > dr0..7 between kernel and user space?

They're not.

> > But that should not be a problem, both shadow as well as effective regs
> > should be properly initialized, specifically for a newly created VCPU.

Yep.

> Could you retry after pushing SET_GUEST_DEBUG at the end of
> kvm_arch_put_registers? Maybe it is no good idea to run get/set_rflags
> without having the sregs properly initialized.

Will do next week.

Another tricky thing with this is that the definition of whats the
kernel job and whats userspace job is somewhat blurry in points. For
example set_regs clears pending exceptions, which made sense in the
past, but breaks now if userspace does put_vcpu_events before set_regs 
(which is not the case with current userspace but just an example).

Makes sense to heavily document things as suggested.

[Qemu-devel] Re: [PATCH 4/4] KVM: Rework of guest debug state writing

[Marcelo Tosatti]

On Thu, Feb 04, 2010 at 08:21:08PM +0100, Jan Kiszka wrote:
> Jan Kiszka wrote:
> > Marcelo Tosatti wrote:
> >> With kvm-autotest the failure is not sporadic (and the above commit
> >> applied): with KVM_SET_GUEST_DEBUG in arch_put_regs all migration 
> >> tests fail, without, all of them succeed. 
> >>
> >> So env->kvm_guest_debug has been zeroed by cpu_x86_init, which means
> >> the writeback via KVM_SET_GUEST_DEBUG does almost nothing. It does
> >> get_rflags and set_rflags in the kernel.
> > 
> > Hmm, it also copies debug regs around... BTW, where do we save/restore
> > dr0..7 between kernel and user space?
> > 
> > But that should not be a problem, both shadow as well as effective regs
> > should be properly initialized, specifically for a newly created VCPU.
> 
> Could you retry after pushing SET_GUEST_DEBUG at the end of
> kvm_arch_put_registers? Maybe it is no good idea to run get/set_rflags
> without having the sregs properly initialized.
> 
> Jan

Can't reproduce (with the original patch, KVM_SET_GUEST_DEBUG at
beginning of arch_put_regs). Different test box though. Go figure.

Anyway, as you mentioned, the workaround can be made cleaner through
set_vcpu_events.

[Qemu-devel] Re: [PATCH 4/4] KVM: Rework of guest debug state writing

[Marcelo Tosatti]

On Thu, Feb 04, 2010 at 08:00:26PM +0100, Jan Kiszka wrote:
> Jan Kiszka wrote:
> > Marcelo Tosatti wrote:
> >>
> >> Unrelated to this problem, won't put_vcpu_events, which is executed 
> >> after KVM_SET_GUEST_DEBUG, overwrite any queued debug exceptions?
> > 
> > Good point, SET_GUEST_DEBUG should be last in the writeback for that reason.
> 
> Actually, we no longer need the exception injection via SET_GUEST_DEBUG
> now that we have full access via vcpu_events. 

> So this needs a cleanup, and I'm afraid quite a few cases are broken
> ATM with vcpu_events writeback overwriting the reinjected exceptions.

Don't see what you mean here. Can you be more explicit? What breakage?

[Qemu-devel] Re: [PATCH 4/4] KVM: Rework of guest debug state writing

[Jan Kiszka]

Marcelo Tosatti wrote:
> On Thu, Feb 04, 2010 at 08:00:26PM +0100, Jan Kiszka wrote:
>> Jan Kiszka wrote:
>>> Marcelo Tosatti wrote:
>>>> Unrelated to this problem, won't put_vcpu_events, which is executed 
>>>> after KVM_SET_GUEST_DEBUG, overwrite any queued debug exceptions?
>>> Good point, SET_GUEST_DEBUG should be last in the writeback for that reason.
>> Actually, we no longer need the exception injection via SET_GUEST_DEBUG
>> now that we have full access via vcpu_events. 
> 
>> So this needs a cleanup, and I'm afraid quite a few cases are broken
>> ATM with vcpu_events writeback overwriting the reinjected exceptions.
> 
> Don't see what you mean here. Can you be more explicit? What breakage?

SET_GUEST_DEBUG and SET_VCPU_EVENTS are mutually exclusive. If we have
the latter, don't use the former for exception injection anymore. And
this is broken already without any of my patches applied, that was my
point. Will work on this soon.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux