qemu-kvm requires apic initialized before vcpu main loop

qemu-kvm requires apic initialized before vcpu main loop

[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 <mtosatti@redhat.com>

diff --git a/hw/apic.c b/hw/apic.c
index 627ff98..45a4d2b 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -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;
 }

Re: qemu-kvm requires apic initialized before vcpu main loop

[Jan Kiszka]

Marcelo Tosatti wrote:
> 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 <mtosatti@redhat.com>
> 
> diff --git a/hw/apic.c b/hw/apic.c
> index 627ff98..45a4d2b 100644
> --- a/hw/apic.c
> +++ b/hw/apic.c
> @@ -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;
>  }

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

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

Re: qemu-kvm requires apic initialized before vcpu main loop

[Glauber Costa]

On Wed, Dec 09, 2009 at 03:46:54PM -0200, Marcelo Tosatti wrote:
> 
> 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 <mtosatti@redhat.com>
> 
> diff --git a/hw/apic.c b/hw/apic.c
> index 627ff98..45a4d2b 100644
> --- a/hw/apic.c
> +++ b/hw/apic.c
> @@ -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?

Re: qemu-kvm requires apic initialized before vcpu main loop

[Jan Kiszka]

Glauber Costa wrote:
> On Wed, Dec 09, 2009 at 03:46:54PM -0200, Marcelo Tosatti wrote:
>> 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 <mtosatti@redhat.com>
>>
>> diff --git a/hw/apic.c b/hw/apic.c
>> index 627ff98..45a4d2b 100644
>> --- a/hw/apic.c
>> +++ b/hw/apic.c
>> @@ -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?

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

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

Re: qemu-kvm requires apic initialized before vcpu main loop

[Gleb Natapov]

On Wed, Dec 09, 2009 at 03:46:54PM -0200, Marcelo Tosatti wrote:
> 
> 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 <mtosatti@redhat.com>
> 
> diff --git a/hw/apic.c b/hw/apic.c
> index 627ff98..45a4d2b 100644
> --- a/hw/apic.c
> +++ b/hw/apic.c
> @@ -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;
>  }
Didn't calls to reset90 were removed from init functions to be done in
centralized manner?

--
			Gleb.

Re: qemu-kvm requires apic initialized before vcpu main loop

[Gleb Natapov]

On Wed, Dec 09, 2009 at 07:23:38PM +0100, Jan Kiszka wrote:
> Marcelo Tosatti wrote:
> > 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 <mtosatti@redhat.com>
> > 
> > diff --git a/hw/apic.c b/hw/apic.c
> > index 627ff98..45a4d2b 100644
> > --- a/hw/apic.c
> > +++ b/hw/apic.c
> > @@ -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;
> >  }
> 
> 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,
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.

> 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
> 
> -- 
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.

Re: qemu-kvm requires apic initialized before vcpu main loop

[Marcelo Tosatti]

On Wed, Dec 09, 2009 at 07:23:38PM +0100, Jan Kiszka wrote:
> Marcelo Tosatti wrote:
> > 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 <mtosatti@redhat.com>
> > 
> > diff --git a/hw/apic.c b/hw/apic.c
> > index 627ff98..45a4d2b 100644
> > --- a/hw/apic.c
> > +++ b/hw/apic.c
> > @@ -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;
> >  }
> 
> 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.

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

Re: qemu-kvm requires apic initialized before vcpu main loop

[Marcelo Tosatti]

On Wed, Dec 09, 2009 at 04:25:45PM -0200, Glauber Costa wrote:
> On Wed, Dec 09, 2009 at 03:46:54PM -0200, Marcelo Tosatti wrote:
> > 
> > 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 <mtosatti@redhat.com>
> > 
> > diff --git a/hw/apic.c b/hw/apic.c
> > index 627ff98..45a4d2b 100644
> > --- a/hw/apic.c
> > +++ b/hw/apic.c
> > @@ -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.

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

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

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

Re: qemu-kvm requires apic initialized before vcpu main loop

[Jan Kiszka]

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

Gleb Natapov wrote:
> On Wed, Dec 09, 2009 at 07:23:38PM +0100, Jan Kiszka wrote:
>> Marcelo Tosatti wrote:
>>> 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 <mtosatti@redhat.com>
>>>
>>> diff --git a/hw/apic.c b/hw/apic.c
>>> index 627ff98..45a4d2b 100644
>>> --- a/hw/apic.c
>>> +++ b/hw/apic.c
>>> @@ -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;
>>>  }
>> 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,
> 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

> 
>> 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
>>
>> -- 
>> Siemens AG, Corporate Technology, CT T DE IT 1
>> Corporate Competence Center Embedded Linux


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

Re: qemu-kvm requires apic initialized before vcpu main loop

[Marcelo Tosatti]

On Wed, Dec 09, 2009 at 09:09:54PM +0100, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Wed, Dec 09, 2009 at 07:23:38PM +0100, Jan Kiszka wrote:
> >> Marcelo Tosatti wrote:
> >>> 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 <mtosatti@redhat.com>
> >>>
> >>> diff --git a/hw/apic.c b/hw/apic.c
> >>> index 627ff98..45a4d2b 100644
> >>> --- a/hw/apic.c
> >>> +++ b/hw/apic.c
> >>> @@ -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;
> >>>  }
> >> 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,
> > 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.

> 
> Jan
> 
> > 
> >> 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
> >>
> >> -- 
> >> Siemens AG, Corporate Technology, CT T DE IT 1
> >> Corporate Competence Center Embedded Linux
> 

Re: qemu-kvm requires apic initialized before vcpu main loop

[Marcelo Tosatti]

On Wed, Dec 09, 2009 at 08:00:41PM +0100, Jan Kiszka wrote:
> Glauber Costa wrote:
> > On Wed, Dec 09, 2009 at 03:46:54PM -0200, Marcelo Tosatti wrote:
> >> 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 <mtosatti@redhat.com>
> >>
> >> diff --git a/hw/apic.c b/hw/apic.c
> >> index 627ff98..45a4d2b 100644
> >> --- a/hw/apic.c
> >> +++ b/hw/apic.c
> >> @@ -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?
> 
> 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).

Re: qemu-kvm requires apic initialized before vcpu main loop

[Jan Kiszka]

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

Marcelo Tosatti wrote:
> On Wed, Dec 09, 2009 at 09:09:54PM +0100, Jan Kiszka wrote:
>> Gleb Natapov wrote:
>>> On Wed, Dec 09, 2009 at 07:23:38PM +0100, Jan Kiszka wrote:
>>>> Marcelo Tosatti wrote:
>>>>> 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 <mtosatti@redhat.com>
>>>>>
>>>>> diff --git a/hw/apic.c b/hw/apic.c
>>>>> index 627ff98..45a4d2b 100644
>>>>> --- a/hw/apic.c
>>>>> +++ b/hw/apic.c
>>>>> @@ -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;
>>>>>  }
>>>> 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,
>>> 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. 

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

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


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

Re: qemu-kvm requires apic initialized before vcpu main loop

[Marcelo Tosatti]

On Wed, Dec 09, 2009 at 07:23:38PM +0100, Jan Kiszka wrote:
> Marcelo Tosatti wrote:
> > 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 <mtosatti@redhat.com>
> > 
> > diff --git a/hw/apic.c b/hw/apic.c
> > index 627ff98..45a4d2b 100644
> > --- a/hw/apic.c
> > +++ b/hw/apic.c
> > @@ -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;
> >  }
> 
> 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. 

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

> 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

Re: qemu-kvm requires apic initialized before vcpu main loop

[Marcelo Tosatti]

On Wed, Dec 09, 2009 at 09:20:49PM +0200, Gleb Natapov wrote:
> On Wed, Dec 09, 2009 at 03:46:54PM -0200, Marcelo Tosatti wrote:
> > 
> > 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 <mtosatti@redhat.com>
> > 
> > diff --git a/hw/apic.c b/hw/apic.c
> > index 627ff98..45a4d2b 100644
> > --- a/hw/apic.c
> > +++ b/hw/apic.c
> > @@ -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;
> >  }
> 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.

Re: qemu-kvm requires apic initialized before vcpu main loop

[Gleb Natapov]

On Wed, Dec 09, 2009 at 09:09:54PM +0100, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Wed, Dec 09, 2009 at 07:23:38PM +0100, Jan Kiszka wrote:
> >> Marcelo Tosatti wrote:
> >>> 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 <mtosatti@redhat.com>
> >>>
> >>> diff --git a/hw/apic.c b/hw/apic.c
> >>> index 627ff98..45a4d2b 100644
> >>> --- a/hw/apic.c
> >>> +++ b/hw/apic.c
> >>> @@ -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;
> >>>  }
> >> 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,
> > 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.

Re: qemu-kvm requires apic initialized before vcpu main loop

[Jan Kiszka]

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

Gleb Natapov wrote:
> On Wed, Dec 09, 2009 at 09:09:54PM +0100, Jan Kiszka wrote:
>> Gleb Natapov wrote:
>>> On Wed, Dec 09, 2009 at 07:23:38PM +0100, Jan Kiszka wrote:
>>>> Marcelo Tosatti wrote:
>>>>> 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 <mtosatti@redhat.com>
>>>>>
>>>>> diff --git a/hw/apic.c b/hw/apic.c
>>>>> index 627ff98..45a4d2b 100644
>>>>> --- a/hw/apic.c
>>>>> +++ b/hw/apic.c
>>>>> @@ -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;
>>>>>  }
>>>> 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,
>>> 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.

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

Jan


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

Re: qemu-kvm requires apic initialized before vcpu main loop

[Gleb Natapov]

On Wed, Dec 09, 2009 at 10:01:36PM +0100, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Wed, Dec 09, 2009 at 09:09:54PM +0100, Jan Kiszka wrote:
> >> Gleb Natapov wrote:
> >>> On Wed, Dec 09, 2009 at 07:23:38PM +0100, Jan Kiszka wrote:
> >>>> Marcelo Tosatti wrote:
> >>>>> 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 <mtosatti@redhat.com>
> >>>>>
> >>>>> diff --git a/hw/apic.c b/hw/apic.c
> >>>>> index 627ff98..45a4d2b 100644
> >>>>> --- a/hw/apic.c
> >>>>> +++ b/hw/apic.c
> >>>>> @@ -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;
> >>>>>  }
> >>>> 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,
> >>> 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.
> 
> 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.

Re: qemu-kvm requires apic initialized before vcpu main loop

[Jan Kiszka]

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

Gleb Natapov wrote:
> On Wed, Dec 09, 2009 at 10:01:36PM +0100, Jan Kiszka wrote:
>> Gleb Natapov wrote:
>>> On Wed, Dec 09, 2009 at 09:09:54PM +0100, Jan Kiszka wrote:
>>>> Gleb Natapov wrote:
>>>>> On Wed, Dec 09, 2009 at 07:23:38PM +0100, Jan Kiszka wrote:
>>>>>> Marcelo Tosatti wrote:
>>>>>>> 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 <mtosatti@redhat.com>
>>>>>>>
>>>>>>> diff --git a/hw/apic.c b/hw/apic.c
>>>>>>> index 627ff98..45a4d2b 100644
>>>>>>> --- a/hw/apic.c
>>>>>>> +++ b/hw/apic.c
>>>>>>> @@ -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;
>>>>>>>  }
>>>>>> 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,
>>>>> 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.
>> 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


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

Re: qemu-kvm requires apic initialized before vcpu main loop

[Avi Kivity]

On 12/09/2009 07:46 PM, Marcelo Tosatti wrote:
> 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.

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

Re: qemu-kvm requires apic initialized before vcpu main loop

[Avi Kivity]

On 12/10/2009 11:33 AM, Avi Kivity wrote:
> On 12/09/2009 07:46 PM, Marcelo Tosatti wrote:
>> 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.

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

Re: qemu-kvm requires apic initialized before vcpu main loop

[Glauber Costa]

On Wed, Dec 09, 2009 at 06:21:38PM -0200, Marcelo Tosatti wrote:
> On Wed, Dec 09, 2009 at 08:00:41PM +0100, Jan Kiszka wrote:
> > Glauber Costa wrote:
> > > On Wed, Dec 09, 2009 at 03:46:54PM -0200, Marcelo Tosatti wrote:
> > >> 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 <mtosatti@redhat.com>
> > >>
> > >> diff --git a/hw/apic.c b/hw/apic.c
> > >> index 627ff98..45a4d2b 100644
> > >> --- a/hw/apic.c
> > >> +++ b/hw/apic.c
> > >> @@ -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?
> > 
> > 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).

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.

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

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

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

diff --git a/hw/apic.c b/hw/apic.c
index ae805dc..627ff98 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -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;
 }
diff --git a/qemu-kvm.c b/qemu-kvm.c
index 44e8b75..ef8c288 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -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;

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

[Avi Kivity]

On 12/14/2009 10:36 PM, Marcelo Tosatti wrote:
> 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.

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

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

[Gleb Natapov]

On Mon, Dec 14, 2009 at 06:36:37PM -0200, Marcelo Tosatti wrote:
> 
> 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.

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

[Marcelo Tosatti]

On Tue, Dec 15, 2009 at 01:20:38PM +0200, Gleb Natapov wrote:
> On Mon, Dec 14, 2009 at 06:36:37PM -0200, Marcelo Tosatti wrote:
> > 
> > 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.

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

> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Avi Kivity]

On 12/15/2009 02:24 PM, Marcelo Tosatti wrote:
>>
>> 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.
>>      
> 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?

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

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

[Gleb Natapov]

On Tue, Dec 15, 2009 at 10:24:15AM -0200, Marcelo Tosatti wrote:
> On Tue, Dec 15, 2009 at 01:20:38PM +0200, Gleb Natapov wrote:
> > On Mon, Dec 14, 2009 at 06:36:37PM -0200, Marcelo Tosatti wrote:
> > > 
> > > 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.
> 
> Note qemu_kvm_load_lapic depends on env->created set (kvm_vcpu_inited),
Uhh. What about doing this:

diff --git a/qemu-kvm.c b/qemu-kvm.c
index 44e8b75..fa6db8e 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -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 */

> so having init_vcpu+load_regs before signalling vcpu creation did not
> fix this one (but yeah, thanks for the reminder on the races).
> 
> > 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 majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.

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

[Marcelo Tosatti]

On Tue, Dec 15, 2009 at 02:31:16PM +0200, Avi Kivity wrote:
> On 12/15/2009 02:24 PM, Marcelo Tosatti wrote:
>>>
>>> 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.
>>>      
>> 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.

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

[Marcelo Tosatti]

On Tue, Dec 15, 2009 at 02:33:27PM +0200, Gleb Natapov wrote:
> On Tue, Dec 15, 2009 at 10:24:15AM -0200, Marcelo Tosatti wrote:
> > On Tue, Dec 15, 2009 at 01:20:38PM +0200, Gleb Natapov wrote:
> > > On Mon, Dec 14, 2009 at 06:36:37PM -0200, Marcelo Tosatti wrote:
> > > > 
> > > > 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.
> > 
> > Note qemu_kvm_load_lapic depends on env->created set (kvm_vcpu_inited),
> Uhh. What about doing this:
> 
> diff --git a/qemu-kvm.c b/qemu-kvm.c
> index 44e8b75..fa6db8e 100644
> --- a/qemu-kvm.c
> +++ b/qemu-kvm.c
> @@ -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 */

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