Skip to contentSkip to conversation

qemu-kvm requires apic initialized before vcpu main loop

PATCH Applied Marcelo Tosatti 6 participants 29 messages Dec 9, 2009 – Dec 16, 2009 KVM

lore
Share & tools
lore

Marcelo Tosatti

author
#0 lore raw

Otherwise a zero apic base is loaded into KVM, which results in interrupts being lost until a proper apic base with enabled bit set is loaded.

Fixes WinXP migration in qemu-kvm origin/next.

Signed-off-by Marcelo Tosatti <[email protected]>
hw/apic.c +5 -0
index 627ff98..45a4d2b 100644
@@ -1131,6 +1131,11 @@ int apic_init(CPUState *env)
     vmstate_register(s->idx, &vmstate_apic, s);
     qemu_register_reset(apic_reset, s);
 
+    /* apic_reset must be called before the vcpu threads are initialized and load 
+     * registers, in qemu-kvm.
+     */
+    apic_reset(s);
+
     local_apics[s->idx] = s;
     return 0;
 }
#1 lore raw
Quoted 14 lines · Marcelo Tosatti

Otherwise a zero apic base is loaded into KVM, which results in interrupts being lost until a proper apic base with enabled bit set is loaded.

Fixes WinXP migration in qemu-kvm origin/next.

Signed-off-by Marcelo Tosatti <[email protected]>
hw/apic.c +0 -0
index 627ff98..45a4d2b 100644
@@ -1131,6 +1131,11 @@ int apic_init(CPUState *env)
     vmstate_register(s->idx, &vmstate_apic, s);

hw/apic.c int apic_init(CPUState *env) · line 1141

Quoted 10 lines
hw/apic.c +5 -0
     qemu_register_reset(apic_reset, s);
 
+    /* apic_reset must be called before the vcpu threads are initialized and load 
+     * registers, in qemu-kvm.
+     */
+    apic_reset(s);
+
     local_apics[s->idx] = s;
     return 0;
 }

Heals the issue I saw with Win2003 Server as well.

Looks all a bit messy though. Hope we can establish a more regular and less fragile model on the midterm. I wonder if it wouldn't be better to do write-back of the local APIC state along with the register state on vmrun (and only there!). The same would apply to things like mpstate, TSC MSR, or the guest debugging state. The reset/vmloading/hw-emulation code would only declare what kind of write-back it wishes: register state only, partial (excluding everything that touches continuously running timers), full. Well, basically the model I suggested for proper mpstate write-back, just even more generalized.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
#2 lore raw
Quoted 28 lines · Jan Kiszka
Quoted 14 lines · Marcelo Tosatti

Otherwise a zero apic base is loaded into KVM, which results in interrupts being lost until a proper apic base with enabled bit set is loaded.

Fixes WinXP migration in qemu-kvm origin/next.

Signed-off-by Marcelo Tosatti <[email protected]>
hw/apic.c +0 -0
index 627ff98..45a4d2b 100644
@@ -1131,6 +1131,11 @@ int apic_init(CPUState *env)
     vmstate_register(s->idx, &vmstate_apic, s);

hw/apic.c int apic_init(CPUState *env) · line 1141

Quoted 10 lines
hw/apic.c +5 -0
     qemu_register_reset(apic_reset, s);
 
+    /* apic_reset must be called before the vcpu threads are initialized and load 
+     * registers, in qemu-kvm.
+     */
+    apic_reset(s);
+
     local_apics[s->idx] = s;
     return 0;
 }

Heals the issue I saw with Win2003 Server as well.

Quoted 4 lines

Looks all a bit messy though. Hope we can establish a more regular and less fragile model on the midterm. I wonder if it wouldn't be better to do write-back of the local APIC state along with the register state on vmrun (and only there!). The same would apply to things like mpstate,

Write back of mp state there is a bug and introduce races. Do write back of the whole APIC state there looks like a recipe for disaster.

Quoted 6 lines

TSC MSR, or the guest debugging state. The reset/vmloading/hw-emulation code would only declare what kind of write-back it wishes: register state only, partial (excluding everything that touches continuously running timers), full. Well, basically the model I suggested for proper mpstate write-back, just even more generalized.

Quoted 9 lines

Jan

Signature · 6 lines
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Gleb.

#3 lore raw
Quoted 28 lines · Gleb Natapov
Quoted 27 lines · Jan Kiszka
Quoted 14 lines · Marcelo Tosatti

Otherwise a zero apic base is loaded into KVM, which results in interrupts being lost until a proper apic base with enabled bit set is loaded.

Fixes WinXP migration in qemu-kvm origin/next.

Signed-off-by Marcelo Tosatti <[email protected]>
hw/apic.c +0 -0
index 627ff98..45a4d2b 100644
@@ -1131,6 +1131,11 @@ int apic_init(CPUState *env)
     vmstate_register(s->idx, &vmstate_apic, s);

hw/apic.c int apic_init(CPUState *env) · line 1141

Quoted 10 lines
hw/apic.c +5 -0
     qemu_register_reset(apic_reset, s);
 
+    /* apic_reset must be called before the vcpu threads are initialized and load 
+     * registers, in qemu-kvm.
+     */
+    apic_reset(s);
+
     local_apics[s->idx] = s;
     return 0;
 }

Heals the issue I saw with Win2003 Server as well.

Quoted 6 lines
Quoted 4 lines

Looks all a bit messy though. Hope we can establish a more regular and less fragile model on the midterm. I wonder if it wouldn't be better to do write-back of the local APIC state along with the register state on vmrun (and only there!). The same would apply to things like mpstate,

Write back of mp state there is a bug and introduce races. Do write back of the whole APIC state there looks like a recipe for disaster.

Please read the full suggestion: We will only write-back if we were going through a reset or vmload before. That removes the ugly kvm hooks from generic code and ensures proper ordering /wrt other write-backs. IMHO, anything else will continue to cause headache like the above to us.

Jan

Quoted 12 lines
Quoted 12 lines

TSC MSR, or the guest debugging state. The reset/vmloading/hw-emulation code would only declare what kind of write-back it wishes: register state only, partial (excluding everything that touches continuously running timers), full. Well, basically the model I suggested for proper mpstate write-back, just even more generalized.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
author
#4 lore raw
Quoted 33 lines · Jan Kiszka
Quoted 32 lines · Gleb Natapov
Quoted 31 lines · Jan Kiszka
Quoted 14 lines · Marcelo Tosatti

Otherwise a zero apic base is loaded into KVM, which results in interrupts being lost until a proper apic base with enabled bit set is loaded.

Fixes WinXP migration in qemu-kvm origin/next.

Signed-off-by Marcelo Tosatti <[email protected]>
hw/apic.c +0 -0
index 627ff98..45a4d2b 100644
@@ -1131,6 +1131,11 @@ int apic_init(CPUState *env)
     vmstate_register(s->idx, &vmstate_apic, s);

hw/apic.c int apic_init(CPUState *env) · line 1141

Quoted 10 lines
hw/apic.c +5 -0
     qemu_register_reset(apic_reset, s);
 
+    /* apic_reset must be called before the vcpu threads are initialized and load 
+     * registers, in qemu-kvm.
+     */
+    apic_reset(s);
+
     local_apics[s->idx] = s;
     return 0;
 }

Heals the issue I saw with Win2003 Server as well.

Looks all a bit messy though. Hope we can establish a more regular and less fragile model on the midterm. I wonder if it wouldn't be better to do write-back of the local APIC state along with the register state on vmrun (and only there!). The same would apply to things like mpstate,

Quoted 7 lines
Quoted 2 lines

Write back of mp state there is a bug and introduce races. Do write back of the whole APIC state there looks like a recipe for disaster.

Please read the full suggestion: We will only write-back if we were going through a reset or vmload before. That removes the ugly kvm hooks from generic code and ensures proper ordering /wrt other write-backs. IMHO, anything else will continue to cause headache like the above to us.

You still need to state explicitly that mpstate should be written back, in the reset / vmloads paths.

The advantage i think is that you unify the save/restore code in vcpu entry/exit paths.

Quoted 16 lines

Jan

Quoted 12 lines
Quoted 12 lines

TSC MSR, or the guest debugging state. The reset/vmloading/hw-emulation code would only declare what kind of write-back it wishes: register state only, partial (excluding everything that touches continuously running timers), full. Well, basically the model I suggested for proper mpstate write-back, just even more generalized.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
#5 lore raw
Quoted 36 lines · Marcelo Tosatti
Quoted 35 lines · Jan Kiszka
Quoted 34 lines · Gleb Natapov
Quoted 27 lines · Jan Kiszka
Quoted 14 lines · Marcelo Tosatti

Otherwise a zero apic base is loaded into KVM, which results in interrupts being lost until a proper apic base with enabled bit set is loaded.

Fixes WinXP migration in qemu-kvm origin/next.

Signed-off-by Marcelo Tosatti <[email protected]>
hw/apic.c +0 -0
index 627ff98..45a4d2b 100644
@@ -1131,6 +1131,11 @@ int apic_init(CPUState *env)
     vmstate_register(s->idx, &vmstate_apic, s);

hw/apic.c int apic_init(CPUState *env) · line 1141

Quoted 10 lines
hw/apic.c +5 -0
     qemu_register_reset(apic_reset, s);
 
+    /* apic_reset must be called before the vcpu threads are initialized and load 
+     * registers, in qemu-kvm.
+     */
+    apic_reset(s);
+
     local_apics[s->idx] = s;
     return 0;
 }

Heals the issue I saw with Win2003 Server as well.

Quoted 4 lines

Looks all a bit messy though. Hope we can establish a more regular and less fragile model on the midterm. I wonder if it wouldn't be better to do write-back of the local APIC state along with the register state on vmrun (and only there!). The same would apply to things like mpstate,

Write back of mp state there is a bug and introduce races. Do write back of the whole APIC state there looks like a recipe for disaster.

Quoted 7 lines
Quoted 4 lines

Please read the full suggestion: We will only write-back if we were going through a reset or vmload before. That removes the ugly kvm hooks from generic code and ensures proper ordering /wrt other write-backs. IMHO, anything else will continue to cause headache like the above to us.

You still need to state explicitly that mpstate should be written back, in the reset / vmloads paths.

Implicitly by declaring what I called "write-back scope" on cpu_synchronize_state. As the write-back handling is arch-specific, it can also decide what has to be written based on the passed scope, including "mpstate or not".

Quoted 3 lines

The advantage i think is that you unify the save/restore code in vcpu entry/exit paths.

Yep, and that you don't have to teach functions like mon_get_cpu() about kvm's mpstate.

Jan

#6 lore raw
In reply to Jan Kiszka
Quoted 33 lines · Jan Kiszka
Quoted 32 lines · Gleb Natapov
Quoted 31 lines · Jan Kiszka
Quoted 14 lines · Marcelo Tosatti

Otherwise a zero apic base is loaded into KVM, which results in interrupts being lost until a proper apic base with enabled bit set is loaded.

Fixes WinXP migration in qemu-kvm origin/next.

Signed-off-by Marcelo Tosatti <[email protected]>
hw/apic.c +0 -0
index 627ff98..45a4d2b 100644
@@ -1131,6 +1131,11 @@ int apic_init(CPUState *env)
     vmstate_register(s->idx, &vmstate_apic, s);

hw/apic.c int apic_init(CPUState *env) · line 1141

Quoted 10 lines
hw/apic.c +5 -0
     qemu_register_reset(apic_reset, s);
 
+    /* apic_reset must be called before the vcpu threads are initialized and load 
+     * registers, in qemu-kvm.
+     */
+    apic_reset(s);
+
     local_apics[s->idx] = s;
     return 0;
 }

Heals the issue I saw with Win2003 Server as well.

Looks all a bit messy though. Hope we can establish a more regular and less fragile model on the midterm. I wonder if it wouldn't be better to do write-back of the local APIC state along with the register state on vmrun (and only there!). The same would apply to things like mpstate,

Quoted 8 lines
Quoted 2 lines

Write back of mp state there is a bug and introduce races. Do write back of the whole APIC state there looks like a recipe for disaster.

Please read the full suggestion: We will only write-back if we were going through a reset or vmload before. That removes the ugly kvm hooks from generic code and ensures proper ordering /wrt other write-backs. IMHO, anything else will continue to cause headache like the above to us.

We can't postpone APIC loading till vmrun. This will race with devices/other vcpus sending interrupts to the vcpu. APIC state of all vcpus should be up-to-date _before_ any vcpu or main loop starts running.

--
Gleb.

#7 lore raw
Quoted 41 lines · Gleb Natapov
Quoted 33 lines · Jan Kiszka
Quoted 32 lines · Gleb Natapov
Quoted 31 lines · Jan Kiszka
Quoted 14 lines · Marcelo Tosatti

Otherwise a zero apic base is loaded into KVM, which results in interrupts being lost until a proper apic base with enabled bit set is loaded.

Fixes WinXP migration in qemu-kvm origin/next.

Signed-off-by Marcelo Tosatti <[email protected]>
hw/apic.c +0 -0
index 627ff98..45a4d2b 100644
@@ -1131,6 +1131,11 @@ int apic_init(CPUState *env)
     vmstate_register(s->idx, &vmstate_apic, s);

hw/apic.c int apic_init(CPUState *env) · line 1141

Quoted 10 lines
hw/apic.c +5 -0
     qemu_register_reset(apic_reset, s);
 
+    /* apic_reset must be called before the vcpu threads are initialized and load 
+     * registers, in qemu-kvm.
+     */
+    apic_reset(s);
+
     local_apics[s->idx] = s;
     return 0;
 }

Heals the issue I saw with Win2003 Server as well.

Looks all a bit messy though. Hope we can establish a more regular and less fragile model on the midterm. I wonder if it wouldn't be better to do write-back of the local APIC state along with the register state on vmrun (and only there!). The same would apply to things like mpstate,

Quoted 7 lines
Quoted 2 lines

Write back of mp state there is a bug and introduce races. Do write back of the whole APIC state there looks like a recipe for disaster.

Please read the full suggestion: We will only write-back if we were going through a reset or vmload before. That removes the ugly kvm hooks from generic code and ensures proper ordering /wrt other write-backs. IMHO, anything else will continue to cause headache like the above to us.

Quoted 4 lines

We can't postpone APIC loading till vmrun. This will race with devices/other vcpus sending interrupts to the vcpu. APIC state of all vcpus should be up-to-date _before_ any vcpu or main loop starts running.

Simple to solve, just add another write-back point: vm_start.

Jan

#8 lore raw
Quoted 42 lines · Jan Kiszka
Quoted 41 lines · Gleb Natapov
Quoted 40 lines · Jan Kiszka
Quoted 28 lines · Gleb Natapov
Quoted 27 lines · Jan Kiszka
Quoted 14 lines · Marcelo Tosatti

Otherwise a zero apic base is loaded into KVM, which results in interrupts being lost until a proper apic base with enabled bit set is loaded.

Fixes WinXP migration in qemu-kvm origin/next.

Signed-off-by Marcelo Tosatti <[email protected]>
hw/apic.c +0 -0
index 627ff98..45a4d2b 100644
@@ -1131,6 +1131,11 @@ int apic_init(CPUState *env)
     vmstate_register(s->idx, &vmstate_apic, s);

hw/apic.c int apic_init(CPUState *env) · line 1141

Quoted 10 lines
hw/apic.c +5 -0
     qemu_register_reset(apic_reset, s);
 
+    /* apic_reset must be called before the vcpu threads are initialized and load 
+     * registers, in qemu-kvm.
+     */
+    apic_reset(s);
+
     local_apics[s->idx] = s;
     return 0;
 }

Heals the issue I saw with Win2003 Server as well.

Quoted 6 lines
Quoted 4 lines

Looks all a bit messy though. Hope we can establish a more regular and less fragile model on the midterm. I wonder if it wouldn't be better to do write-back of the local APIC state along with the register state on vmrun (and only there!). The same would apply to things like mpstate,

Write back of mp state there is a bug and introduce races. Do write back of the whole APIC state there looks like a recipe for disaster.

Please read the full suggestion: We will only write-back if we were going through a reset or vmload before. That removes the ugly kvm hooks from generic code and ensures proper ordering /wrt other write-backs. IMHO, anything else will continue to cause headache like the above to us.

Quoted 7 lines
Quoted 4 lines

We can't postpone APIC loading till vmrun. This will race with devices/other vcpus sending interrupts to the vcpu. APIC state of all vcpus should be up-to-date _before_ any vcpu or main loop starts running.

Simple to solve, just add another write-back point: vm_start.

So what's the point to have write-back for APIC in vmrun? It is always wrong to do it there.

--
Gleb.

#9 lore raw
Quoted 43 lines · Gleb Natapov
Quoted 42 lines · Jan Kiszka
Quoted 41 lines · Gleb Natapov
Quoted 40 lines · Jan Kiszka
Quoted 28 lines · Gleb Natapov
Quoted 27 lines · Jan Kiszka
Quoted 14 lines · Marcelo Tosatti

Otherwise a zero apic base is loaded into KVM, which results in interrupts being lost until a proper apic base with enabled bit set is loaded.

Fixes WinXP migration in qemu-kvm origin/next.

Signed-off-by Marcelo Tosatti <[email protected]>
hw/apic.c +0 -0
index 627ff98..45a4d2b 100644
@@ -1131,6 +1131,11 @@ int apic_init(CPUState *env)
     vmstate_register(s->idx, &vmstate_apic, s);

hw/apic.c int apic_init(CPUState *env) · line 1141

Quoted 10 lines
hw/apic.c +5 -0
     qemu_register_reset(apic_reset, s);
 
+    /* apic_reset must be called before the vcpu threads are initialized and load 
+     * registers, in qemu-kvm.
+     */
+    apic_reset(s);
+
     local_apics[s->idx] = s;
     return 0;
 }

Heals the issue I saw with Win2003 Server as well.

Quoted 6 lines
Quoted 4 lines

Looks all a bit messy though. Hope we can establish a more regular and less fragile model on the midterm. I wonder if it wouldn't be better to do write-back of the local APIC state along with the register state on vmrun (and only there!). The same would apply to things like mpstate,

Write back of mp state there is a bug and introduce races. Do write back of the whole APIC state there looks like a recipe for disaster.

Please read the full suggestion: We will only write-back if we were going through a reset or vmload before. That removes the ugly kvm hooks from generic code and ensures proper ordering /wrt other write-backs. IMHO, anything else will continue to cause headache like the above to us.

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

We can't postpone APIC loading till vmrun. This will race with devices/other vcpus sending interrupts to the vcpu. APIC state of all vcpus should be up-to-date _before_ any vcpu or main loop starts running.

Simple to solve, just add another write-back point: vm_start.

So what's the point to have write-back for APIC in vmrun? It is always wrong to do it there.

Write-back is arch-specific and will never show up directly in vmrun or other generic code. Will update my original patch to make this discussion more concrete.

Jan

author
#10 lore raw
In reply to Jan Kiszka
Quoted 28 lines · Jan Kiszka
Quoted 14 lines · Marcelo Tosatti

Otherwise a zero apic base is loaded into KVM, which results in interrupts being lost until a proper apic base with enabled bit set is loaded.

Fixes WinXP migration in qemu-kvm origin/next.

Signed-off-by Marcelo Tosatti <[email protected]>
hw/apic.c +0 -0
index 627ff98..45a4d2b 100644
@@ -1131,6 +1131,11 @@ int apic_init(CPUState *env)
     vmstate_register(s->idx, &vmstate_apic, s);

hw/apic.c int apic_init(CPUState *env) · line 1141

Quoted 10 lines
hw/apic.c +5 -0
     qemu_register_reset(apic_reset, s);
 
+    /* apic_reset must be called before the vcpu threads are initialized and load 
+     * registers, in qemu-kvm.
+     */
+    apic_reset(s);
+
     local_apics[s->idx] = s;
     return 0;
 }

Heals the issue I saw with Win2003 Server as well.

Quoted 9 lines

Looks all a bit messy though. Hope we can establish a more regular and less fragile model on the midterm. I wonder if it wouldn't be better to do write-back of the local APIC state along with the register state on vmrun (and only there!). The same would apply to things like mpstate, TSC MSR, or the guest debugging state. The reset/vmloading/hw-emulation code would only declare what kind of write-back it wishes: register state only, partial (excluding everything that touches continuously running timers), full. Well, basically the model I suggested for proper mpstate write-back, just even more generalized.

I suppose that works, but "regs_modified" needs to be broken down. Something similar to kvm_register_write / kvm_register_read in the kernel.

author
#11 lore raw
In reply to Jan Kiszka
Quoted 28 lines · Jan Kiszka
Quoted 14 lines · Marcelo Tosatti

Otherwise a zero apic base is loaded into KVM, which results in interrupts being lost until a proper apic base with enabled bit set is loaded.

Fixes WinXP migration in qemu-kvm origin/next.

Signed-off-by Marcelo Tosatti <[email protected]>
hw/apic.c +0 -0
index 627ff98..45a4d2b 100644
@@ -1131,6 +1131,11 @@ int apic_init(CPUState *env)
     vmstate_register(s->idx, &vmstate_apic, s);

hw/apic.c int apic_init(CPUState *env) · line 1141

Quoted 10 lines
hw/apic.c +5 -0
     qemu_register_reset(apic_reset, s);
 
+    /* apic_reset must be called before the vcpu threads are initialized and load 
+     * registers, in qemu-kvm.
+     */
+    apic_reset(s);
+
     local_apics[s->idx] = s;
     return 0;
 }

Heals the issue I saw with Win2003 Server as well.

Quoted 2 lines

Looks all a bit messy though. Hope we can establish a more regular and less fragile model on the midterm.

Yes, its a house of cards which falls all the time :(

Quoted 10 lines

I wonder if it wouldn't be better to do write-back of the local APIC state along with the register state on vmrun (and only there!). The same would apply to things like mpstate, TSC MSR, or the guest debugging state. The reset/vmloading/hw-emulation code would only declare what kind of write-back it wishes: register state only, partial (excluding everything that touches continuously running timers), full. Well, basically the model I suggested for proper mpstate write-back, just even more generalized.

Jan

#12 lore raw
In reply to Marcelo Tosatti
Quoted 14 lines · Marcelo Tosatti

Otherwise a zero apic base is loaded into KVM, which results in interrupts being lost until a proper apic base with enabled bit set is loaded.

Fixes WinXP migration in qemu-kvm origin/next.

Signed-off-by Marcelo Tosatti <[email protected]>
hw/apic.c +0 -0
index 627ff98..45a4d2b 100644

hw/apic.c int apic_init(CPUState *env) · line 1138

Quoted 8 lines
hw/apic.c +5 -0
@@ -1131,6 +1131,11 @@ int apic_init(CPUState *env)
     vmstate_register(s->idx, &vmstate_apic, s);
     qemu_register_reset(apic_reset, s);
 
+    /* apic_reset must be called before the vcpu threads are initialized and load 
+     * registers, in qemu-kvm.
+     */
+    apic_reset(s);
+

But by doing this, the system-wide reset will re-reset the apic, possibly losing some other information.

Also, system_reset happens before we signal system_ready (or at least should). This means the vcpus should not be running and producing anything useful yet. So how does it happen, in the first place?

#13 lore raw
Quoted 25 lines · Glauber Costa
Quoted 13 lines · Marcelo Tosatti

Otherwise a zero apic base is loaded into KVM, which results in interrupts being lost until a proper apic base with enabled bit set is loaded.

Fixes WinXP migration in qemu-kvm origin/next.

Signed-off-by Marcelo Tosatti <[email protected]>
hw/apic.c +0 -0
index 627ff98..45a4d2b 100644

hw/apic.c int apic_init(CPUState *env) · line 1138

Quoted 8 lines
hw/apic.c +5 -0
@@ -1131,6 +1131,11 @@ int apic_init(CPUState *env)
     vmstate_register(s->idx, &vmstate_apic, s);
     qemu_register_reset(apic_reset, s);
 
+    /* apic_reset must be called before the vcpu threads are initialized and load 
+     * registers, in qemu-kvm.
+     */
+    apic_reset(s);
+

But by doing this, the system-wide reset will re-reset the apic, possibly losing some other information.

Quoted 3 lines

Also, system_reset happens before we signal system_ready (or at least should). This means the vcpus should not be running and producing anything useful yet. So how does it happen, in the first place?

There is

	kvm_arch_load_regs(env);

before qemu_cond_wait in ap_main_loop. Probably part of the reason. Why is it there?

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
author
#14 lore raw
Quoted 30 lines · Jan Kiszka
Quoted 25 lines · Glauber Costa
Quoted 13 lines · Marcelo Tosatti

Otherwise a zero apic base is loaded into KVM, which results in interrupts being lost until a proper apic base with enabled bit set is loaded.

Fixes WinXP migration in qemu-kvm origin/next.

Signed-off-by Marcelo Tosatti <[email protected]>
hw/apic.c +0 -0
index 627ff98..45a4d2b 100644

hw/apic.c int apic_init(CPUState *env) · line 1138

Quoted 8 lines
hw/apic.c +5 -0
@@ -1131,6 +1131,11 @@ int apic_init(CPUState *env)
     vmstate_register(s->idx, &vmstate_apic, s);
     qemu_register_reset(apic_reset, s);
 
+    /* apic_reset must be called before the vcpu threads are initialized and load 
+     * registers, in qemu-kvm.
+     */
+    apic_reset(s);
+

But by doing this, the system-wide reset will re-reset the apic, possibly losing some other information.

Quoted 3 lines

Also, system_reset happens before we signal system_ready (or at least should). This means the vcpus should not be running and producing anything useful yet. So how does it happen, in the first place?

Quoted 6 lines

There is

	kvm_arch_load_regs(env);

before qemu_cond_wait in ap_main_loop. Probably part of the reason. Why is it there?

Hum ... see how qemu_kvm_load_lapic depends on kvm_vcpu_inited.

kvm_vcpu_ioctl_set_lapic -> kvm_apic_post_state_restore relies on proper apicbase set (maybe other reasons too).

#15 lore raw
Quoted 35 lines · Marcelo Tosatti
Quoted 34 lines · Jan Kiszka
Quoted 25 lines · Glauber Costa
Quoted 13 lines · Marcelo Tosatti

Otherwise a zero apic base is loaded into KVM, which results in interrupts being lost until a proper apic base with enabled bit set is loaded.

Fixes WinXP migration in qemu-kvm origin/next.

Signed-off-by Marcelo Tosatti <[email protected]>
hw/apic.c +0 -0
index 627ff98..45a4d2b 100644

hw/apic.c int apic_init(CPUState *env) · line 1138

Quoted 8 lines
hw/apic.c +5 -0
@@ -1131,6 +1131,11 @@ int apic_init(CPUState *env)
     vmstate_register(s->idx, &vmstate_apic, s);
     qemu_register_reset(apic_reset, s);
 
+    /* apic_reset must be called before the vcpu threads are initialized and load 
+     * registers, in qemu-kvm.
+     */
+    apic_reset(s);
+

But by doing this, the system-wide reset will re-reset the apic, possibly losing some other information.

Quoted 3 lines

Also, system_reset happens before we signal system_ready (or at least should). This means the vcpus should not be running and producing anything useful yet. So how does it happen, in the first place?

There is

	kvm_arch_load_regs(env);
Quoted 7 lines
Quoted 2 lines

before qemu_cond_wait in ap_main_loop. Probably part of the reason. Why is it there?

Hum ... see how qemu_kvm_load_lapic depends on kvm_vcpu_inited.

kvm_vcpu_ioctl_set_lapic -> kvm_apic_post_state_restore relies on proper apicbase set (maybe other reasons too).

Have you tried getting rid of kvm_vcpu_inited()? Now that we are doing reset after vcpu creation, it is quite possible that it won't be needed anymore.

Btw, the whole point of this exercise is to try diminishing oportunities for nasty things like that.

author
#16 lore raw
In reply to Glauber Costa
Quoted 18 lines · Glauber Costa
Quoted 17 lines · Marcelo Tosatti

Otherwise a zero apic base is loaded into KVM, which results in interrupts being lost until a proper apic base with enabled bit set is loaded.

Fixes WinXP migration in qemu-kvm origin/next.

Signed-off-by Marcelo Tosatti <[email protected]>
hw/apic.c +0 -0
index 627ff98..45a4d2b 100644
@@ -1131,6 +1131,11 @@ int apic_init(CPUState *env)
     vmstate_register(s->idx, &vmstate_apic, s);
     qemu_register_reset(apic_reset, s);
Quoted 7 lines

hw/apic.c int apic_init(CPUState *env) · line 1138

Quoted 5 lines
hw/apic.c +5 -0
+    /* apic_reset must be called before the vcpu threads are initialized and load 
+     * registers, in qemu-kvm.
+     */
+    apic_reset(s);
+

But by doing this, the system-wide reset will re-reset the apic, possibly losing some other information.

The information is the same. vcpu did not run (so did not change any information) between apic_reset and system wide reset.

Quoted 1 line

Also, system_reset happens before we signal system_ready (or at least should).

Not in qemu-kvm.c. Even if it did, it is too late (by that time vcpu thread will have loaded APIC base of 0).

qemu-kvm.c should be updated to call system_reset, I believe, similarly to what has been done to vl.c.

Quoted 2 lines

This means the vcpus should not be running and producing anything useful yet. So how does it happen, in the first place?

The initialization of env->apic_base must happen before the vcpu thread calls kvm_arch_load_regs in qemu-kvm.c.

Otherwise the vcpu thread initializes env->apic_base with value of "0", and that in turn will result in the kernel's "apic_hw_enabled()" returning false, so interrupt injections in that period are lost, for one problem.

Its similar issue that this comment on hw/pc.c refers to:

    /* kvm needs this to run after the apic is initialized. Otherwise,
     * it can access invalid state and crash.
     */
    qemu_init_vcpu(env);
    return env;
#17 lore raw
In reply to Marcelo Tosatti
Quoted 15 lines · Marcelo Tosatti

Otherwise a zero apic base is loaded into KVM, which results in interrupts being lost until a proper apic base with enabled bit set is loaded.

Fixes WinXP migration in qemu-kvm origin/next.

Signed-off-by Marcelo Tosatti <[email protected]>
hw/apic.c +0 -0
index 627ff98..45a4d2b 100644
@@ -1131,6 +1131,11 @@ int apic_init(CPUState *env)
     vmstate_register(s->idx, &vmstate_apic, s);

hw/apic.c int apic_init(CPUState *env) · line 1141

Quoted 10 lines
hw/apic.c +5 -0
     qemu_register_reset(apic_reset, s);
 
+    /* apic_reset must be called before the vcpu threads are initialized and load 
+     * registers, in qemu-kvm.
+     */
+    apic_reset(s);
+
     local_apics[s->idx] = s;
     return 0;
 }

Didn't calls to reset90 were removed from init functions to be done in centralized manner?

--
Gleb.

author
#18 lore raw
Quoted 18 lines · Gleb Natapov
Quoted 17 lines · Marcelo Tosatti

Otherwise a zero apic base is loaded into KVM, which results in interrupts being lost until a proper apic base with enabled bit set is loaded.

Fixes WinXP migration in qemu-kvm origin/next.

Signed-off-by Marcelo Tosatti <[email protected]>
hw/apic.c +0 -0
index 627ff98..45a4d2b 100644
@@ -1131,6 +1131,11 @@ int apic_init(CPUState *env)
     vmstate_register(s->idx, &vmstate_apic, s);
     qemu_register_reset(apic_reset, s);
Quoted 10 lines

hw/apic.c int apic_init(CPUState *env) · line 1141

Quoted 8 lines
hw/apic.c +5 -0
+    /* apic_reset must be called before the vcpu threads are initialized and load 
+     * registers, in qemu-kvm.
+     */
+    apic_reset(s);
+
     local_apics[s->idx] = s;
     return 0;
 }

Didn't calls to reset90 were removed from init functions to be done in centralized manner?

Yes, but apic reset (which initialiazes the reset lapic values) conflicts with qemu-kvm's creation of vcpu threads. qemu upstream is fine.

Applied
#19 lore raw
In reply to Marcelo Tosatti
Quoted 6 lines · Marcelo Tosatti

Otherwise a zero apic base is loaded into KVM, which results in interrupts being lost until a proper apic base with enabled bit set is loaded.

Fixes WinXP migration in qemu-kvm origin/next.

Thanks, applied.

Signature · 1 line
error compiling committee.c: too many arguments to function
#20 lore raw
Quoted 9 lines · Avi Kivity
Quoted 5 lines · Marcelo Tosatti

Otherwise a zero apic base is loaded into KVM, which results in interrupts being lost until a proper apic base with enabled bit set is loaded.

Fixes WinXP migration in qemu-kvm origin/next.

Thanks, applied.

btw, generalization is still welcome, I agree that we need something better here. I applied it since the bug is blocking development.

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

PATCH 1/1 qemu-kvm initialize vcpu state after machine initialization

So that the vcpu state is initialized, from vcpu thread context, after machine initialization is settled.

This allows to revert apic_init's apic_reset call. apic_reset now happens through system_reset, similarly to qemu upstream.

Signed-off-by Marcelo Tosatti <[email protected]>
hw/apic.c +0 -5
index ae805dc..627ff98 100644
@@ -1131,11 +1131,6 @@ int apic_init(CPUState *env)
     vmstate_register(s->idx, &vmstate_apic, s);
     qemu_register_reset(apic_reset, s);
 
-    /* apic_reset must be called before the vcpu threads are initialized and load
-     * registers, in qemu-kvm.
-     */
-    apic_reset(s);
-
     local_apics[s->idx] = s;
     return 0;
 }
qemu-kvm.c +2 -5
index 44e8b75..ef8c288 100644
@@ -1918,11 +1918,6 @@ static void *ap_main_loop(void *_env)
     setup_kernel_sigmask(env);
 
     pthread_mutex_lock(&qemu_mutex);
-    cpu_single_env = env;
-
-    kvm_arch_init_vcpu(env);
-
-    kvm_arch_load_regs(env);
 
     /* signal VCPU creation */
     current_env->created = 1;

@@ -1934,6 +1929,8 @@ static void *ap_main_loop(void *_env)
 
     /* re-initialize cpu_single_env after re-acquiring qemu_mutex */
     cpu_single_env = env;
+    kvm_arch_init_vcpu(env);
+    kvm_arch_load_regs(env);
 
     kvm_main_loop_cpu(env);
     return NULL;
Applied
#22 lore raw

Re: [PATCH] qemu-kvm initialize vcpu state after machine initialization

Quoted 7 lines · Marcelo Tosatti

So that the vcpu state is initialized, from vcpu thread context, after machine initialization is settled.

This allows to revert apic_init's apic_reset call. apic_reset now happens through system_reset, similarly to qemu upstream.

Applied, thanks.

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

Re: [PATCH] qemu-kvm initialize vcpu state after machine initialization

In reply to Marcelo Tosatti
Quoted 7 lines · Marcelo Tosatti

So that the vcpu state is initialized, from vcpu thread context, after machine initialization is settled.

This allows to revert apic_init's apic_reset call. apic_reset now happens through system_reset, similarly to qemu upstream.

This patch essentially revers commit 898c51c3. This commit fixes two races. First race is like this:

vcpu0 vcpu1

starts running
loads lapic state into kernel
sends event to vcpu1
starts running
loads lapic state into kernel overwrites event from vcpu0
At the time 898c51c3 was committed the race was easily reproducible by starting VM with 16 cpus + seabios. Sometimes some vcpus lost INIT/SIPI events. Now I am not able to reproduce it even with this patch applied, so something else changed, but it doesn't make the race non existent or acceptable.

The second race is during machine start after migration. The race is between event loop and vcpu:

event loop vcpu

starts running
gets RTC timer event
sends interrupt to vcpu
starts running loads lapic state into kernel overwrites interrupt from RTC

In short vcpu state that can be influenced by sources outside vcpu thread itself should be uploaded into the kernel before signaling qemu_system_ready condition.

--
Gleb.

author
#24 lore raw

Re: [PATCH] qemu-kvm initialize vcpu state after machine initialization

Quoted 20 lines · Gleb Natapov
Quoted 7 lines · Marcelo Tosatti

So that the vcpu state is initialized, from vcpu thread context, after machine initialization is settled.

This allows to revert apic_init's apic_reset call. apic_reset now happens through system_reset, similarly to qemu upstream.

This patch essentially revers commit 898c51c3. This commit fixes two races. First race is like this:

vcpu0 vcpu1

starts running
loads lapic state into kernel
sends event to vcpu1
starts running loads lapic state into kernel overwrites event from vcpu0

Quoted 5 lines

At the time 898c51c3 was committed the race was easily reproducible by starting VM with 16 cpus + seabios. Sometimes some vcpus lost INIT/SIPI events. Now I am not able to reproduce it even with this patch applied, so something else changed, but it doesn't make the race non existent or acceptable.

Note qemu_kvm_load_lapic depends on env->created set (kvm_vcpu_inited), so having init_vcpu+load_regs before signalling vcpu creation did not fix this one (but yeah, thanks for the reminder on the races).

Quoted 23 lines

The second race is during machine start after migration. The race is between event loop and vcpu:

event loop vcpu

starts running
gets RTC timer event
sends interrupt to vcpu
starts running loads lapic state into kernel overwrites interrupt from RTC

In short vcpu state that can be influenced by sources outside vcpu thread itself should be uploaded into the kernel before signaling qemu_system_ready condition.

--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#25 lore raw

Re: [PATCH] qemu-kvm initialize vcpu state after machine initialization

Quoted 19 lines · Marcelo Tosatti
Quoted 13 lines

This patch essentially revers commit 898c51c3. This commit fixes two races. First race is like this:

vcpu0 vcpu1

starts running
loads lapic state into kernel
sends event to vcpu1
starts running loads lapic state into kernel overwrites event from vcpu0

Quoted 6 lines

At the time 898c51c3 was committed the race was easily reproducible by starting VM with 16 cpus + seabios. Sometimes some vcpus lost INIT/SIPI events. Now I am not able to reproduce it even with this patch applied, so something else changed, but it doesn't make the race non existent or acceptable.

Quoted 4 lines

Note qemu_kvm_load_lapic depends on env->created set (kvm_vcpu_inited), so having init_vcpu+load_regs before signalling vcpu creation did not fix this one (but yeah, thanks for the reminder on the races).

So safest to revert and wait for a fixed patch?

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

Re: [PATCH] qemu-kvm initialize vcpu state after machine initialization

Quoted 20 lines · Avi Kivity
Quoted 19 lines · Marcelo Tosatti
Quoted 19 lines

This patch essentially revers commit 898c51c3. This commit fixes two races. First race is like this:

vcpu0 vcpu1

starts running
loads lapic state into kernel
sends event to vcpu1
starts running loads lapic state into kernel overwrites event from vcpu0

At the time 898c51c3 was committed the race was easily reproducible by starting VM with 16 cpus + seabios. Sometimes some vcpus lost INIT/SIPI events. Now I am not able to reproduce it even with this patch applied, so something else changed, but it doesn't make the race non existent or acceptable.

Quoted 6 lines
Quoted 4 lines

Note qemu_kvm_load_lapic depends on env->created set (kvm_vcpu_inited), so having init_vcpu+load_regs before signalling vcpu creation did not fix this one (but yeah, thanks for the reminder on the races).

So safest to revert and wait for a fixed patch?

Yeah, sorry.

#27 lore raw

Re: [PATCH] qemu-kvm initialize vcpu state after machine initialization

In reply to Marcelo Tosatti
Quoted 21 lines · Marcelo Tosatti
Quoted 20 lines · Gleb Natapov
Quoted 7 lines · Marcelo Tosatti

So that the vcpu state is initialized, from vcpu thread context, after machine initialization is settled.

This allows to revert apic_init's apic_reset call. apic_reset now happens through system_reset, similarly to qemu upstream.

This patch essentially revers commit 898c51c3. This commit fixes two races. First race is like this:

vcpu0 vcpu1

starts running
loads lapic state into kernel
sends event to vcpu1
starts running loads lapic state into kernel overwrites event from vcpu0

Quoted 7 lines
Quoted 5 lines

At the time 898c51c3 was committed the race was easily reproducible by starting VM with 16 cpus + seabios. Sometimes some vcpus lost INIT/SIPI events. Now I am not able to reproduce it even with this patch applied, so something else changed, but it doesn't make the race non existent or acceptable.

Note qemu_kvm_load_lapic depends on env->created set (kvm_vcpu_inited),

Uhh. What about doing this:

qemu-kvm.c +1 -1
index 44e8b75..fa6db8e 100644
@@ -1920,12 +1920,12 @@ static void *ap_main_loop(void *_env)
     pthread_mutex_lock(&qemu_mutex);
     cpu_single_env = env;
 
+    current_env->created = 1;
     kvm_arch_init_vcpu(env);
 
     kvm_arch_load_regs(env);
 
     /* signal VCPU creation */
-    current_env->created = 1;
     pthread_cond_signal(&qemu_vcpu_cond);
 
     /* and wait for machine initialization */
Quoted 26 lines

so having init_vcpu+load_regs before signalling vcpu creation did not fix this one (but yeah, thanks for the reminder on the races).

Quoted 23 lines

The second race is during machine start after migration. The race is between event loop and vcpu:

event loop vcpu

starts running
gets RTC timer event
sends interrupt to vcpu
starts running loads lapic state into kernel overwrites interrupt from RTC

In short vcpu state that can be influenced by sources outside vcpu thread itself should be uploaded into the kernel before signaling qemu_system_ready condition.

--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Gleb.

author
#28 lore raw

Re: [PATCH] qemu-kvm initialize vcpu state after machine initialization

Quoted 39 lines · Gleb Natapov
Quoted 21 lines · Marcelo Tosatti
Quoted 20 lines · Gleb Natapov
Quoted 7 lines · Marcelo Tosatti

So that the vcpu state is initialized, from vcpu thread context, after machine initialization is settled.

This allows to revert apic_init's apic_reset call. apic_reset now happens through system_reset, similarly to qemu upstream.

This patch essentially revers commit 898c51c3. This commit fixes two races. First race is like this:

vcpu0 vcpu1

starts running
loads lapic state into kernel
sends event to vcpu1
starts running loads lapic state into kernel overwrites event from vcpu0

Quoted 7 lines
Quoted 5 lines

At the time 898c51c3 was committed the race was easily reproducible by starting VM with 16 cpus + seabios. Sometimes some vcpus lost INIT/SIPI events. Now I am not able to reproduce it even with this patch applied, so something else changed, but it doesn't make the race non existent or acceptable.

Note qemu_kvm_load_lapic depends on env->created set (kvm_vcpu_inited),

Uhh. What about doing this:

qemu-kvm.c +0 -0
index 44e8b75..fa6db8e 100644
@@ -1920,12 +1920,12 @@ static void *ap_main_loop(void *_env)
     pthread_mutex_lock(&qemu_mutex);
     cpu_single_env = env;

qemu-kvm.c static void *ap_main_loop(void *_env) · line 1931

Quoted 10 lines
qemu-kvm.c +1 -1
+    current_env->created = 1;
     kvm_arch_init_vcpu(env);
 
     kvm_arch_load_regs(env);
 
     /* signal VCPU creation */
-    current_env->created = 1;
     pthread_cond_signal(&qemu_vcpu_cond);
 
     /* and wait for machine initialization */

load_lapic ioctl is called from pc_new_cpu -> apic_reset, so its alright. No need for the patch above, then.