* qemu-kvm requires apic initialized before vcpu main loop @ 2009-12-09 17:46 Marcelo Tosatti 2009-12-09 18:23 ` Jan Kiszka ` (3 more replies) 0 siblings, 4 replies; 29+ messages in thread From: Marcelo Tosatti @ 2009-12-09 17:46 UTC (permalink / raw) To: Avi Kivity, Glauber de Oliveira Costa; +Cc: kvm 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; } ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: qemu-kvm requires apic initialized before vcpu main loop 2009-12-09 17:46 qemu-kvm requires apic initialized before vcpu main loop Marcelo Tosatti @ 2009-12-09 18:23 ` Jan Kiszka 2009-12-09 19:23 ` Gleb Natapov ` (2 more replies) 2009-12-09 18:25 ` Glauber Costa ` (2 subsequent siblings) 3 siblings, 3 replies; 29+ messages in thread From: Jan Kiszka @ 2009-12-09 18:23 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: Avi Kivity, Glauber de Oliveira Costa, kvm 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: qemu-kvm requires apic initialized before vcpu main loop 2009-12-09 18:23 ` Jan Kiszka @ 2009-12-09 19:23 ` Gleb Natapov 2009-12-09 20:09 ` Jan Kiszka 2009-12-09 20:02 ` Marcelo Tosatti 2009-12-09 20:22 ` Marcelo Tosatti 2 siblings, 1 reply; 29+ messages in thread From: Gleb Natapov @ 2009-12-09 19:23 UTC (permalink / raw) To: Jan Kiszka; +Cc: Marcelo Tosatti, Avi Kivity, Glauber de Oliveira Costa, kvm 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: qemu-kvm requires apic initialized before vcpu main loop 2009-12-09 19:23 ` Gleb Natapov @ 2009-12-09 20:09 ` Jan Kiszka 2009-12-09 20:13 ` Marcelo Tosatti 2009-12-09 20:50 ` Gleb Natapov 0 siblings, 2 replies; 29+ messages in thread From: Jan Kiszka @ 2009-12-09 20:09 UTC (permalink / raw) To: Gleb Natapov; +Cc: Marcelo Tosatti, Avi Kivity, Glauber de Oliveira Costa, kvm [-- 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 --] ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: qemu-kvm requires apic initialized before vcpu main loop 2009-12-09 20:09 ` Jan Kiszka @ 2009-12-09 20:13 ` Marcelo Tosatti 2009-12-09 20:21 ` Jan Kiszka 2009-12-09 20:50 ` Gleb Natapov 1 sibling, 1 reply; 29+ messages in thread From: Marcelo Tosatti @ 2009-12-09 20:13 UTC (permalink / raw) To: Jan Kiszka; +Cc: Gleb Natapov, Avi Kivity, Glauber de Oliveira Costa, kvm 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 > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: qemu-kvm requires apic initialized before vcpu main loop 2009-12-09 20:13 ` Marcelo Tosatti @ 2009-12-09 20:21 ` Jan Kiszka 0 siblings, 0 replies; 29+ messages in thread From: Jan Kiszka @ 2009-12-09 20:21 UTC (permalink / raw) To: Marcelo Tosatti Cc: Jan Kiszka, Gleb Natapov, Avi Kivity, Glauber de Oliveira Costa, kvm [-- 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 --] ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: qemu-kvm requires apic initialized before vcpu main loop 2009-12-09 20:09 ` Jan Kiszka 2009-12-09 20:13 ` Marcelo Tosatti @ 2009-12-09 20:50 ` Gleb Natapov 2009-12-09 21:01 ` Jan Kiszka 1 sibling, 1 reply; 29+ messages in thread From: Gleb Natapov @ 2009-12-09 20:50 UTC (permalink / raw) To: Jan Kiszka; +Cc: Marcelo Tosatti, Avi Kivity, Glauber de Oliveira Costa, kvm 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: qemu-kvm requires apic initialized before vcpu main loop 2009-12-09 20:50 ` Gleb Natapov @ 2009-12-09 21:01 ` Jan Kiszka 2009-12-10 6:36 ` Gleb Natapov 0 siblings, 1 reply; 29+ messages in thread From: Jan Kiszka @ 2009-12-09 21:01 UTC (permalink / raw) To: Gleb Natapov; +Cc: Marcelo Tosatti, Avi Kivity, Glauber de Oliveira Costa, kvm [-- 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 --] ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: qemu-kvm requires apic initialized before vcpu main loop 2009-12-09 21:01 ` Jan Kiszka @ 2009-12-10 6:36 ` Gleb Natapov 2009-12-10 8:12 ` Jan Kiszka 0 siblings, 1 reply; 29+ messages in thread From: Gleb Natapov @ 2009-12-10 6:36 UTC (permalink / raw) To: Jan Kiszka; +Cc: Marcelo Tosatti, Avi Kivity, Glauber de Oliveira Costa, kvm 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: qemu-kvm requires apic initialized before vcpu main loop 2009-12-10 6:36 ` Gleb Natapov @ 2009-12-10 8:12 ` Jan Kiszka 0 siblings, 0 replies; 29+ messages in thread From: Jan Kiszka @ 2009-12-10 8:12 UTC (permalink / raw) To: Gleb Natapov; +Cc: Marcelo Tosatti, Avi Kivity, Glauber de Oliveira Costa, kvm [-- 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 --] ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: qemu-kvm requires apic initialized before vcpu main loop 2009-12-09 18:23 ` Jan Kiszka 2009-12-09 19:23 ` Gleb Natapov @ 2009-12-09 20:02 ` Marcelo Tosatti 2009-12-09 20:22 ` Marcelo Tosatti 2 siblings, 0 replies; 29+ messages in thread From: Marcelo Tosatti @ 2009-12-09 20:02 UTC (permalink / raw) To: Jan Kiszka; +Cc: Avi Kivity, Glauber de Oliveira Costa, kvm 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: qemu-kvm requires apic initialized before vcpu main loop 2009-12-09 18:23 ` Jan Kiszka 2009-12-09 19:23 ` Gleb Natapov 2009-12-09 20:02 ` Marcelo Tosatti @ 2009-12-09 20:22 ` Marcelo Tosatti 2 siblings, 0 replies; 29+ messages in thread From: Marcelo Tosatti @ 2009-12-09 20:22 UTC (permalink / raw) To: Jan Kiszka; +Cc: Avi Kivity, Glauber de Oliveira Costa, kvm 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: qemu-kvm requires apic initialized before vcpu main loop 2009-12-09 17:46 qemu-kvm requires apic initialized before vcpu main loop Marcelo Tosatti 2009-12-09 18:23 ` Jan Kiszka @ 2009-12-09 18:25 ` Glauber Costa 2009-12-09 19:00 ` Jan Kiszka 2009-12-09 20:09 ` Marcelo Tosatti 2009-12-09 19:20 ` Gleb Natapov 2009-12-10 9:33 ` Avi Kivity 3 siblings, 2 replies; 29+ messages in thread From: Glauber Costa @ 2009-12-09 18:25 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: Avi Kivity, Glauber de Oliveira Costa, kvm 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? ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: qemu-kvm requires apic initialized before vcpu main loop 2009-12-09 18:25 ` Glauber Costa @ 2009-12-09 19:00 ` Jan Kiszka 2009-12-09 20:21 ` Marcelo Tosatti 2009-12-09 20:09 ` Marcelo Tosatti 1 sibling, 1 reply; 29+ messages in thread From: Jan Kiszka @ 2009-12-09 19:00 UTC (permalink / raw) To: Glauber Costa; +Cc: Marcelo Tosatti, Avi Kivity, Glauber de Oliveira Costa, kvm 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: qemu-kvm requires apic initialized before vcpu main loop 2009-12-09 19:00 ` Jan Kiszka @ 2009-12-09 20:21 ` Marcelo Tosatti 2009-12-10 11:06 ` Glauber Costa 0 siblings, 1 reply; 29+ messages in thread From: Marcelo Tosatti @ 2009-12-09 20:21 UTC (permalink / raw) To: Jan Kiszka; +Cc: Glauber Costa, Avi Kivity, Glauber de Oliveira Costa, kvm 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). ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: qemu-kvm requires apic initialized before vcpu main loop 2009-12-09 20:21 ` Marcelo Tosatti @ 2009-12-10 11:06 ` Glauber Costa 0 siblings, 0 replies; 29+ messages in thread From: Glauber Costa @ 2009-12-10 11:06 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: Jan Kiszka, Avi Kivity, Glauber de Oliveira Costa, kvm 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: qemu-kvm requires apic initialized before vcpu main loop 2009-12-09 18:25 ` Glauber Costa 2009-12-09 19:00 ` Jan Kiszka @ 2009-12-09 20:09 ` Marcelo Tosatti 1 sibling, 0 replies; 29+ messages in thread From: Marcelo Tosatti @ 2009-12-09 20:09 UTC (permalink / raw) To: Glauber Costa; +Cc: Avi Kivity, Glauber de Oliveira Costa, kvm 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; ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: qemu-kvm requires apic initialized before vcpu main loop 2009-12-09 17:46 qemu-kvm requires apic initialized before vcpu main loop Marcelo Tosatti 2009-12-09 18:23 ` Jan Kiszka 2009-12-09 18:25 ` Glauber Costa @ 2009-12-09 19:20 ` Gleb Natapov 2009-12-09 20:26 ` Marcelo Tosatti 2009-12-10 9:33 ` Avi Kivity 3 siblings, 1 reply; 29+ messages in thread From: Gleb Natapov @ 2009-12-09 19:20 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: Avi Kivity, Glauber de Oliveira Costa, kvm 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: qemu-kvm requires apic initialized before vcpu main loop 2009-12-09 19:20 ` Gleb Natapov @ 2009-12-09 20:26 ` Marcelo Tosatti 0 siblings, 0 replies; 29+ messages in thread From: Marcelo Tosatti @ 2009-12-09 20:26 UTC (permalink / raw) To: Gleb Natapov; +Cc: Avi Kivity, Glauber de Oliveira Costa, kvm 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: qemu-kvm requires apic initialized before vcpu main loop 2009-12-09 17:46 qemu-kvm requires apic initialized before vcpu main loop Marcelo Tosatti ` (2 preceding siblings ...) 2009-12-09 19:20 ` Gleb Natapov @ 2009-12-10 9:33 ` Avi Kivity 2009-12-10 9:45 ` Avi Kivity 3 siblings, 1 reply; 29+ messages in thread From: Avi Kivity @ 2009-12-10 9:33 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: Glauber de Oliveira Costa, kvm 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: qemu-kvm requires apic initialized before vcpu main loop 2009-12-10 9:33 ` Avi Kivity @ 2009-12-10 9:45 ` Avi Kivity 2009-12-14 20:36 ` [PATCH] qemu-kvm initialize vcpu state after machine initialization Marcelo Tosatti 0 siblings, 1 reply; 29+ messages in thread From: Avi Kivity @ 2009-12-10 9:45 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: Glauber de Oliveira Costa, kvm 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH] qemu-kvm initialize vcpu state after machine initialization 2009-12-10 9:45 ` Avi Kivity @ 2009-12-14 20:36 ` Marcelo Tosatti 2009-12-15 10:16 ` Avi Kivity 2009-12-15 11:20 ` Gleb Natapov 0 siblings, 2 replies; 29+ messages in thread From: Marcelo Tosatti @ 2009-12-14 20:36 UTC (permalink / raw) To: Avi Kivity; +Cc: Glauber de Oliveira Costa, kvm, Gleb Natapov 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; ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH] qemu-kvm initialize vcpu state after machine initialization 2009-12-14 20:36 ` [PATCH] qemu-kvm initialize vcpu state after machine initialization Marcelo Tosatti @ 2009-12-15 10:16 ` Avi Kivity 2009-12-15 11:20 ` Gleb Natapov 1 sibling, 0 replies; 29+ messages in thread From: Avi Kivity @ 2009-12-15 10:16 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: Glauber de Oliveira Costa, kvm, Gleb Natapov 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] qemu-kvm initialize vcpu state after machine initialization 2009-12-14 20:36 ` [PATCH] qemu-kvm initialize vcpu state after machine initialization Marcelo Tosatti 2009-12-15 10:16 ` Avi Kivity @ 2009-12-15 11:20 ` Gleb Natapov 2009-12-15 12:24 ` Marcelo Tosatti 1 sibling, 1 reply; 29+ messages in thread From: Gleb Natapov @ 2009-12-15 11:20 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: Avi Kivity, Glauber de Oliveira Costa, kvm 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] qemu-kvm initialize vcpu state after machine initialization 2009-12-15 11:20 ` Gleb Natapov @ 2009-12-15 12:24 ` Marcelo Tosatti 2009-12-15 12:31 ` Avi Kivity 2009-12-15 12:33 ` Gleb Natapov 0 siblings, 2 replies; 29+ messages in thread From: Marcelo Tosatti @ 2009-12-15 12:24 UTC (permalink / raw) To: Gleb Natapov; +Cc: Avi Kivity, Glauber de Oliveira Costa, kvm 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] qemu-kvm initialize vcpu state after machine initialization 2009-12-15 12:24 ` Marcelo Tosatti @ 2009-12-15 12:31 ` Avi Kivity 2009-12-15 12:51 ` Marcelo Tosatti 2009-12-15 12:33 ` Gleb Natapov 1 sibling, 1 reply; 29+ messages in thread From: Avi Kivity @ 2009-12-15 12:31 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: Gleb Natapov, Glauber de Oliveira Costa, kvm 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] qemu-kvm initialize vcpu state after machine initialization 2009-12-15 12:31 ` Avi Kivity @ 2009-12-15 12:51 ` Marcelo Tosatti 0 siblings, 0 replies; 29+ messages in thread From: Marcelo Tosatti @ 2009-12-15 12:51 UTC (permalink / raw) To: Avi Kivity; +Cc: Gleb Natapov, Glauber de Oliveira Costa, kvm 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] qemu-kvm initialize vcpu state after machine initialization 2009-12-15 12:24 ` Marcelo Tosatti 2009-12-15 12:31 ` Avi Kivity @ 2009-12-15 12:33 ` Gleb Natapov 2009-12-16 14:12 ` Marcelo Tosatti 1 sibling, 1 reply; 29+ messages in thread From: Gleb Natapov @ 2009-12-15 12:33 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: Avi Kivity, Glauber de Oliveira Costa, kvm 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. ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH] qemu-kvm initialize vcpu state after machine initialization 2009-12-15 12:33 ` Gleb Natapov @ 2009-12-16 14:12 ` Marcelo Tosatti 0 siblings, 0 replies; 29+ messages in thread From: Marcelo Tosatti @ 2009-12-16 14:12 UTC (permalink / raw) To: Gleb Natapov; +Cc: Avi Kivity, Glauber de Oliveira Costa, kvm 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2009-12-16 14:12 UTC | newest] Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-12-09 17:46 qemu-kvm requires apic initialized before vcpu main loop Marcelo Tosatti 2009-12-09 18:23 ` Jan Kiszka 2009-12-09 19:23 ` Gleb Natapov 2009-12-09 20:09 ` Jan Kiszka 2009-12-09 20:13 ` Marcelo Tosatti 2009-12-09 20:21 ` Jan Kiszka 2009-12-09 20:50 ` Gleb Natapov 2009-12-09 21:01 ` Jan Kiszka 2009-12-10 6:36 ` Gleb Natapov 2009-12-10 8:12 ` Jan Kiszka 2009-12-09 20:02 ` Marcelo Tosatti 2009-12-09 20:22 ` Marcelo Tosatti 2009-12-09 18:25 ` Glauber Costa 2009-12-09 19:00 ` Jan Kiszka 2009-12-09 20:21 ` Marcelo Tosatti 2009-12-10 11:06 ` Glauber Costa 2009-12-09 20:09 ` Marcelo Tosatti 2009-12-09 19:20 ` Gleb Natapov 2009-12-09 20:26 ` Marcelo Tosatti 2009-12-10 9:33 ` Avi Kivity 2009-12-10 9:45 ` Avi Kivity 2009-12-14 20:36 ` [PATCH] qemu-kvm initialize vcpu state after machine initialization Marcelo Tosatti 2009-12-15 10:16 ` Avi Kivity 2009-12-15 11:20 ` Gleb Natapov 2009-12-15 12:24 ` Marcelo Tosatti 2009-12-15 12:31 ` Avi Kivity 2009-12-15 12:51 ` Marcelo Tosatti 2009-12-15 12:33 ` Gleb Natapov 2009-12-16 14:12 ` Marcelo Tosatti
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.