[PATCH] Update halted state from mpstate only in case of inkernel irq chip

[PATCH] Update halted state from mpstate only in case of inkernel irq chip

[Gleb Natapov]

Otherwise cpu is always unhalted by call to kvm_arch_get_registers()

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 qemu-kvm.c |   10 ++++++++++
 qemu-kvm.h |    9 +--------
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/qemu-kvm.c b/qemu-kvm.c
index 6511cb6..2a7fe3d 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -1605,6 +1605,16 @@ static void on_vcpu(CPUState *env, void (*func)(void *data), void *data)
         qemu_cond_wait(&qemu_work_cond);
 }
 
+void kvm_arch_get_registers(CPUState *env)
+{
+	kvm_arch_save_regs(env);
+	kvm_arch_save_mpstate(env);
+#ifdef KVM_CAP_MP_STATE
+	if (kvm_irqchip_in_kernel(kvm_context))
+		env->halted = (env->mp_state == KVM_MP_STATE_HALTED);
+#endif
+}
+
 static void do_kvm_cpu_synchronize_state(void *_env)
 {
     CPUState *env = _env;
diff --git a/qemu-kvm.h b/qemu-kvm.h
index b2c8c35..4523e25 100644
--- a/qemu-kvm.h
+++ b/qemu-kvm.h
@@ -1181,14 +1181,7 @@ static inline int kvm_sync_vcpus(void)
 }
 
 #ifndef QEMU_KVM_NO_CPU
-static inline void kvm_arch_get_registers(CPUState *env)
-{
-    kvm_arch_save_regs(env);
-    kvm_arch_save_mpstate(env);
-#ifdef KVM_CAP_MP_STATE
-    env->halted = (env->mp_state == KVM_MP_STATE_HALTED);
-#endif
-}
+void kvm_arch_get_registers(CPUState *env);
 
 static inline void kvm_arch_put_registers(CPUState *env)
 {
-- 
1.6.3.3

[PATCH] Don't call kvm_cpu_synchronize_state() if there is no irqchip events to process.

[Gleb Natapov]

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 qemu-kvm-x86.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
index 32561f0..acb1b91 100644
--- a/qemu-kvm-x86.c
+++ b/qemu-kvm-x86.c
@@ -1673,9 +1673,12 @@ uint32_t kvm_arch_get_supported_cpuid(CPUState *env, uint32_t function,
 
 void kvm_arch_process_irqchip_events(CPUState *env)
 {
-    kvm_cpu_synchronize_state(env);
-    if (env->interrupt_request & CPU_INTERRUPT_INIT)
+    if (env->interrupt_request & CPU_INTERRUPT_INIT) {
+        kvm_cpu_synchronize_state(env);
         do_cpu_init(env);
-    if (env->interrupt_request & CPU_INTERRUPT_SIPI)
+    }
+    if (env->interrupt_request & CPU_INTERRUPT_SIPI) {
+        kvm_cpu_synchronize_state(env);
         do_cpu_sipi(env);
+    }
 }
-- 
1.6.3.3

[PATCH] Don't call cpu_synchronize_state() in apic_init_reset()

[Gleb Natapov]

Each caller of the function already calls it.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 hw/apic.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/hw/apic.c b/hw/apic.c
index 3a2e128..a9d1fb8 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -488,7 +488,6 @@ void apic_init_reset(CPUState *env)
     if (!s)
         return;
 
-    cpu_synchronize_state(env);
     s->tpr = 0;
     s->spurious_vec = 0xff;
     s->log_dest = 0;
-- 
1.6.3.3

Re: [PATCH] Update halted state from mpstate only in case of inkernel irq chip

[Avi Kivity]

On 09/22/2009 05:51 PM, Gleb Natapov wrote:
> Otherwise cpu is always unhalted by call to kvm_arch_get_registers()
>    

Applied, thanks.

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

Re: [PATCH] Don't call kvm_cpu_synchronize_state() if there is no irqchip events to process.

[Avi Kivity]

On 09/22/2009 05:51 PM, Gleb Natapov wrote:
> Signed-off-by: Gleb Natapov<gleb@redhat.com>
>    

Applied, thanks.

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

Re: [PATCH] Don't call cpu_synchronize_state() in apic_init_reset()

[Avi Kivity]

On 09/22/2009 05:51 PM, Gleb Natapov wrote:
> Each caller of the function already calls it.
>
> Signed-off-by: Gleb Natapov<gleb@redhat.com>
> ---
>   hw/apic.c |    1 -
>   1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/hw/apic.c b/hw/apic.c
> index 3a2e128..a9d1fb8 100644
> --- a/hw/apic.c
> +++ b/hw/apic.c
> @@ -488,7 +488,6 @@ void apic_init_reset(CPUState *env)
>       if (!s)
>           return;
>
> -    cpu_synchronize_state(env);
>       s->tpr = 0;
>       s->spurious_vec = 0xff;
>       s->log_dest = 0;
>    

Still, it's safer to live this in.

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

Re: [PATCH] Don't call cpu_synchronize_state() in apic_init_reset()

[Jan Kiszka]

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

Avi Kivity wrote:
> On 09/22/2009 05:51 PM, Gleb Natapov wrote:
>> Each caller of the function already calls it.
>>
>> Signed-off-by: Gleb Natapov<gleb@redhat.com>
>> ---
>>   hw/apic.c |    1 -
>>   1 files changed, 0 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/apic.c b/hw/apic.c
>> index 3a2e128..a9d1fb8 100644
>> --- a/hw/apic.c
>> +++ b/hw/apic.c
>> @@ -488,7 +488,6 @@ void apic_init_reset(CPUState *env)
>>       if (!s)
>>           return;
>>
>> -    cpu_synchronize_state(env);
>>       s->tpr = 0;
>>       s->spurious_vec = 0xff;
>>       s->log_dest = 0;
>>    
> 
> Still, it's safer to live this in.
> 

Yet another diff to upstream...

It's really time to stabilize this still a bit fuzzy register sync
model, also in qemu-kvm. If you think we need it, let us push it
upstream - with a sound explanation, and maybe even with more sync
points for the sake of consistency.

Jan


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

Re: [PATCH] Don't call cpu_synchronize_state() in apic_init_reset()

[Avi Kivity]

On 09/23/2009 06:07 PM, Jan Kiszka wrote:
> Avi Kivity wrote:
>    
>> On 09/22/2009 05:51 PM, Gleb Natapov wrote:
>>      
>>> Each caller of the function already calls it.
>>>
>>> Signed-off-by: Gleb Natapov<gleb@redhat.com>
>>> ---
>>>    hw/apic.c |    1 -
>>>    1 files changed, 0 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/hw/apic.c b/hw/apic.c
>>> index 3a2e128..a9d1fb8 100644
>>> --- a/hw/apic.c
>>> +++ b/hw/apic.c
>>> @@ -488,7 +488,6 @@ void apic_init_reset(CPUState *env)
>>>        if (!s)
>>>            return;
>>>
>>> -    cpu_synchronize_state(env);
>>>        s->tpr = 0;
>>>        s->spurious_vec = 0xff;
>>>        s->log_dest = 0;
>>>
>>>        
>> Still, it's safer to live this in.
>>
>>      
> Yet another diff to upstream...
>
> It's really time to stabilize this still a bit fuzzy register sync
> model, also in qemu-kvm. If you think we need it, let us push it
> upstream - with a sound explanation, and maybe even with more sync
> points for the sake of consistency.
>
>    

Functions calling each other in the same subsystem can rely on callers 
calling cpu_synchronize_state().  Across subsystems, that's another 
matter, exported functions should try not to rely on implementation 
details of their callers.

(You might argue that the apic is not separate subsystem wrt an x86 cpu, 
and I'm not sure I have a counterargument)

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

Re: [PATCH] Don't call cpu_synchronize_state() in apic_init_reset()

[Jan Kiszka]

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

Avi Kivity wrote:
> On 09/23/2009 06:07 PM, Jan Kiszka wrote:
>> Avi Kivity wrote:
>>   
>>> On 09/22/2009 05:51 PM, Gleb Natapov wrote:
>>>     
>>>> Each caller of the function already calls it.
>>>>
>>>> Signed-off-by: Gleb Natapov<gleb@redhat.com>
>>>> ---
>>>>    hw/apic.c |    1 -
>>>>    1 files changed, 0 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/hw/apic.c b/hw/apic.c
>>>> index 3a2e128..a9d1fb8 100644
>>>> --- a/hw/apic.c
>>>> +++ b/hw/apic.c
>>>> @@ -488,7 +488,6 @@ void apic_init_reset(CPUState *env)
>>>>        if (!s)
>>>>            return;
>>>>
>>>> -    cpu_synchronize_state(env);
>>>>        s->tpr = 0;
>>>>        s->spurious_vec = 0xff;
>>>>        s->log_dest = 0;
>>>>
>>>>        
>>> Still, it's safer to live this in.
>>>
>>>      
>> Yet another diff to upstream...
>>
>> It's really time to stabilize this still a bit fuzzy register sync
>> model, also in qemu-kvm. If you think we need it, let us push it
>> upstream - with a sound explanation, and maybe even with more sync
>> points for the sake of consistency.
>>
>>    
> 
> Functions calling each other in the same subsystem can rely on callers
> calling cpu_synchronize_state().  Across subsystems, that's another
> matter, exported functions should try not to rely on implementation
> details of their callers.
> 
> (You might argue that the apic is not separate subsystem wrt an x86 cpu,
> and I'm not sure I have a counterargument)
> 

I do accept this argument. It's just that my feeling is that we are
lacking proper review of the required call sites of cpu_sychronize_state
and rather put it where some regression popped up (and that only in
qemu-kvm...).

The new rule is: Synchronize the states before accessing registers (or
in-kernel devices) the first time after a vmexit to user space. But,
e.g., I do not see where we do this on CPU reset.

Jan


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

Re: [PATCH] Don't call cpu_synchronize_state() in apic_init_reset()

[Avi Kivity]

On 09/23/2009 06:45 PM, Jan Kiszka wrote:
>> Functions calling each other in the same subsystem can rely on callers
>> calling cpu_synchronize_state().  Across subsystems, that's another
>> matter, exported functions should try not to rely on implementation
>> details of their callers.
>>
>> (You might argue that the apic is not separate subsystem wrt an x86 cpu,
>> and I'm not sure I have a counterargument)
>>
>>      
> I do accept this argument. It's just that my feeling is that we are
> lacking proper review of the required call sites of cpu_sychronize_state
> and rather put it where some regression popped up (and that only in
> qemu-kvm...).
>    

That's life...

> The new rule is: Synchronize the states before accessing registers (or
> in-kernel devices) the first time after a vmexit to user space.

No, the rule is: synchronize state before accessing registers.  Extra 
synchronization is cheap, while missing synchronization is very expensive.

> But,
> e.g., I do not see where we do this on CPU reset.
>    

That's a bug.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH] Don't call cpu_synchronize_state() in apic_init_reset()

[Gleb Natapov]

On Thu, Sep 24, 2009 at 10:53:59AM +0300, Avi Kivity wrote:
> On 09/23/2009 06:45 PM, Jan Kiszka wrote:
> >>Functions calling each other in the same subsystem can rely on callers
> >>calling cpu_synchronize_state().  Across subsystems, that's another
> >>matter, exported functions should try not to rely on implementation
> >>details of their callers.
> >>
> >>(You might argue that the apic is not separate subsystem wrt an x86 cpu,
> >>and I'm not sure I have a counterargument)
> >>
> >I do accept this argument. It's just that my feeling is that we are
> >lacking proper review of the required call sites of cpu_sychronize_state
> >and rather put it where some regression popped up (and that only in
> >qemu-kvm...).
> 
> That's life...
> 
> >The new rule is: Synchronize the states before accessing registers (or
> >in-kernel devices) the first time after a vmexit to user space.
> 
> No, the rule is: synchronize state before accessing registers.
> Extra synchronization is cheap, while missing synchronization is
> very expensive.
> 
So should we stick cpu_synchronize_state() before each register
accesses? I think it is reasonable to omit it if all callers do it
already.

> >But,
> >e.g., I do not see where we do this on CPU reset.
> 
> That's a bug.
> 
Only if kvm support cpus without apic. Otherwise CPU is reset by 
apic_reset() and cpu_synchronize_state() is called there.

--
			Gleb.

Re: [PATCH] Don't call cpu_synchronize_state() in apic_init_reset()

[Jan Kiszka]

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

Gleb Natapov wrote:
> On Thu, Sep 24, 2009 at 10:53:59AM +0300, Avi Kivity wrote:
>> On 09/23/2009 06:45 PM, Jan Kiszka wrote:
>>>> Functions calling each other in the same subsystem can rely on callers
>>>> calling cpu_synchronize_state().  Across subsystems, that's another
>>>> matter, exported functions should try not to rely on implementation
>>>> details of their callers.
>>>>
>>>> (You might argue that the apic is not separate subsystem wrt an x86 cpu,
>>>> and I'm not sure I have a counterargument)
>>>>
>>> I do accept this argument. It's just that my feeling is that we are
>>> lacking proper review of the required call sites of cpu_sychronize_state
>>> and rather put it where some regression popped up (and that only in
>>> qemu-kvm...).
>> That's life...
>>
>>> The new rule is: Synchronize the states before accessing registers (or
>>> in-kernel devices) the first time after a vmexit to user space.
>> No, the rule is: synchronize state before accessing registers.
>> Extra synchronization is cheap, while missing synchronization is
>> very expensive.
>>
> So should we stick cpu_synchronize_state() before each register
> accesses? I think it is reasonable to omit it if all callers do it
> already.
> 
>>> But,
>>> e.g., I do not see where we do this on CPU reset.
>> That's a bug.
>>
> Only if kvm support cpus without apic. Otherwise CPU is reset by 
> apic_reset() and cpu_synchronize_state() is called there.

No, that's not enough if cpu_reset() first fiddles with some registers
that may later on be overwritten on cpu_synchronize_state() with the old
in-kernel state. At least in theory, haven't checked yet what happens in
reality. That's why not synchronizing properly is "expensive" (or broken
IOW).

Jan


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

Re: [PATCH] Don't call cpu_synchronize_state() in apic_init_reset()

[Avi Kivity]

On 09/24/2009 11:03 AM, Gleb Natapov wrote:
>>
>>> The new rule is: Synchronize the states before accessing registers (or
>>> in-kernel devices) the first time after a vmexit to user space.
>>>        
>> No, the rule is: synchronize state before accessing registers.
>> Extra synchronization is cheap, while missing synchronization is
>> very expensive.
>>
>>      
> So should we stick cpu_synchronize_state() before each register
> accesses? I think it is reasonable to omit it if all callers do it
> already.
>    

If the callee is static we can and should avoid it.  If the function is 
exported then we shouldn't rely on callers.

IOW, it's fine to depend on local details (which a reader can easily 
gain), but better to avoid depending on global details.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH] Don't call cpu_synchronize_state() in apic_init_reset()

[Gleb Natapov]

On Thu, Sep 24, 2009 at 10:15:15AM +0200, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Thu, Sep 24, 2009 at 10:53:59AM +0300, Avi Kivity wrote:
> >> On 09/23/2009 06:45 PM, Jan Kiszka wrote:
> >>>> Functions calling each other in the same subsystem can rely on callers
> >>>> calling cpu_synchronize_state().  Across subsystems, that's another
> >>>> matter, exported functions should try not to rely on implementation
> >>>> details of their callers.
> >>>>
> >>>> (You might argue that the apic is not separate subsystem wrt an x86 cpu,
> >>>> and I'm not sure I have a counterargument)
> >>>>
> >>> I do accept this argument. It's just that my feeling is that we are
> >>> lacking proper review of the required call sites of cpu_sychronize_state
> >>> and rather put it where some regression popped up (and that only in
> >>> qemu-kvm...).
> >> That's life...
> >>
> >>> The new rule is: Synchronize the states before accessing registers (or
> >>> in-kernel devices) the first time after a vmexit to user space.
> >> No, the rule is: synchronize state before accessing registers.
> >> Extra synchronization is cheap, while missing synchronization is
> >> very expensive.
> >>
> > So should we stick cpu_synchronize_state() before each register
> > accesses? I think it is reasonable to omit it if all callers do it
> > already.
> > 
> >>> But,
> >>> e.g., I do not see where we do this on CPU reset.
> >> That's a bug.
> >>
> > Only if kvm support cpus without apic. Otherwise CPU is reset by 
> > apic_reset() and cpu_synchronize_state() is called there.
> 
> No, that's not enough if cpu_reset() first fiddles with some registers
> that may later on be overwritten on cpu_synchronize_state() with the old
> in-kernel state. At least in theory, haven't checked yet what happens in
Can't happen. Call chain is apic_reset() -> cpu_reset() and apic_reset()
calls  cpu_synchronize_state() before calling cpu_reset().

> reality. That's why not synchronizing properly is "expensive" (or broken
> IOW).
> 
> Jan
> 



--
			Gleb.

Re: [PATCH] Don't call cpu_synchronize_state() in apic_init_reset()

[Jan Kiszka]

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

Gleb Natapov wrote:
> On Thu, Sep 24, 2009 at 10:15:15AM +0200, Jan Kiszka wrote:
>> Gleb Natapov wrote:
>>> On Thu, Sep 24, 2009 at 10:53:59AM +0300, Avi Kivity wrote:
>>>> On 09/23/2009 06:45 PM, Jan Kiszka wrote:
>>>>>> Functions calling each other in the same subsystem can rely on callers
>>>>>> calling cpu_synchronize_state().  Across subsystems, that's another
>>>>>> matter, exported functions should try not to rely on implementation
>>>>>> details of their callers.
>>>>>>
>>>>>> (You might argue that the apic is not separate subsystem wrt an x86 cpu,
>>>>>> and I'm not sure I have a counterargument)
>>>>>>
>>>>> I do accept this argument. It's just that my feeling is that we are
>>>>> lacking proper review of the required call sites of cpu_sychronize_state
>>>>> and rather put it where some regression popped up (and that only in
>>>>> qemu-kvm...).
>>>> That's life...
>>>>
>>>>> The new rule is: Synchronize the states before accessing registers (or
>>>>> in-kernel devices) the first time after a vmexit to user space.
>>>> No, the rule is: synchronize state before accessing registers.
>>>> Extra synchronization is cheap, while missing synchronization is
>>>> very expensive.
>>>>
>>> So should we stick cpu_synchronize_state() before each register
>>> accesses? I think it is reasonable to omit it if all callers do it
>>> already.
>>>
>>>>> But,
>>>>> e.g., I do not see where we do this on CPU reset.
>>>> That's a bug.
>>>>
>>> Only if kvm support cpus without apic. Otherwise CPU is reset by 
>>> apic_reset() and cpu_synchronize_state() is called there.
>> No, that's not enough if cpu_reset() first fiddles with some registers
>> that may later on be overwritten on cpu_synchronize_state() with the old
>> in-kernel state. At least in theory, haven't checked yet what happens in
> Can't happen. Call chain is apic_reset() -> cpu_reset() and apic_reset()
> calls  cpu_synchronize_state() before calling cpu_reset().

And system_reset?

Jan


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

Re: [PATCH] Don't call cpu_synchronize_state() in apic_init_reset()

[Gleb Natapov]

On Thu, Sep 24, 2009 at 10:59:46AM +0200, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Thu, Sep 24, 2009 at 10:15:15AM +0200, Jan Kiszka wrote:
> >> Gleb Natapov wrote:
> >>> On Thu, Sep 24, 2009 at 10:53:59AM +0300, Avi Kivity wrote:
> >>>> On 09/23/2009 06:45 PM, Jan Kiszka wrote:
> >>>>>> Functions calling each other in the same subsystem can rely on callers
> >>>>>> calling cpu_synchronize_state().  Across subsystems, that's another
> >>>>>> matter, exported functions should try not to rely on implementation
> >>>>>> details of their callers.
> >>>>>>
> >>>>>> (You might argue that the apic is not separate subsystem wrt an x86 cpu,
> >>>>>> and I'm not sure I have a counterargument)
> >>>>>>
> >>>>> I do accept this argument. It's just that my feeling is that we are
> >>>>> lacking proper review of the required call sites of cpu_sychronize_state
> >>>>> and rather put it where some regression popped up (and that only in
> >>>>> qemu-kvm...).
> >>>> That's life...
> >>>>
> >>>>> The new rule is: Synchronize the states before accessing registers (or
> >>>>> in-kernel devices) the first time after a vmexit to user space.
> >>>> No, the rule is: synchronize state before accessing registers.
> >>>> Extra synchronization is cheap, while missing synchronization is
> >>>> very expensive.
> >>>>
> >>> So should we stick cpu_synchronize_state() before each register
> >>> accesses? I think it is reasonable to omit it if all callers do it
> >>> already.
> >>>
> >>>>> But,
> >>>>> e.g., I do not see where we do this on CPU reset.
> >>>> That's a bug.
> >>>>
> >>> Only if kvm support cpus without apic. Otherwise CPU is reset by 
> >>> apic_reset() and cpu_synchronize_state() is called there.
> >> No, that's not enough if cpu_reset() first fiddles with some registers
> >> that may later on be overwritten on cpu_synchronize_state() with the old
> >> in-kernel state. At least in theory, haven't checked yet what happens in
> > Can't happen. Call chain is apic_reset() -> cpu_reset() and apic_reset()
> > calls  cpu_synchronize_state() before calling cpu_reset().
> 
> And system_reset?
> 
And system_reset calls apic_reset() if cpu has apic, cpu_reset()
otherwise. That is why I said that the bug is only for cpus without
apic.

--
			Gleb.

Re: [PATCH] Don't call cpu_synchronize_state() in apic_init_reset()

[Jan Kiszka]

Gleb Natapov wrote:
> On Thu, Sep 24, 2009 at 10:59:46AM +0200, Jan Kiszka wrote:
>> Gleb Natapov wrote:
>>> On Thu, Sep 24, 2009 at 10:15:15AM +0200, Jan Kiszka wrote:
>>>> Gleb Natapov wrote:
>>>>> On Thu, Sep 24, 2009 at 10:53:59AM +0300, Avi Kivity wrote:
>>>>>> On 09/23/2009 06:45 PM, Jan Kiszka wrote:
>>>>>>>> Functions calling each other in the same subsystem can rely on callers
>>>>>>>> calling cpu_synchronize_state().  Across subsystems, that's another
>>>>>>>> matter, exported functions should try not to rely on implementation
>>>>>>>> details of their callers.
>>>>>>>>
>>>>>>>> (You might argue that the apic is not separate subsystem wrt an x86 cpu,
>>>>>>>> and I'm not sure I have a counterargument)
>>>>>>>>
>>>>>>> I do accept this argument. It's just that my feeling is that we are
>>>>>>> lacking proper review of the required call sites of cpu_sychronize_state
>>>>>>> and rather put it where some regression popped up (and that only in
>>>>>>> qemu-kvm...).
>>>>>> That's life...
>>>>>>
>>>>>>> The new rule is: Synchronize the states before accessing registers (or
>>>>>>> in-kernel devices) the first time after a vmexit to user space.
>>>>>> No, the rule is: synchronize state before accessing registers.
>>>>>> Extra synchronization is cheap, while missing synchronization is
>>>>>> very expensive.
>>>>>>
>>>>> So should we stick cpu_synchronize_state() before each register
>>>>> accesses? I think it is reasonable to omit it if all callers do it
>>>>> already.
>>>>>
>>>>>>> But,
>>>>>>> e.g., I do not see where we do this on CPU reset.
>>>>>> That's a bug.
>>>>>>
>>>>> Only if kvm support cpus without apic. Otherwise CPU is reset by 
>>>>> apic_reset() and cpu_synchronize_state() is called there.
>>>> No, that's not enough if cpu_reset() first fiddles with some registers
>>>> that may later on be overwritten on cpu_synchronize_state() with the old
>>>> in-kernel state. At least in theory, haven't checked yet what happens in
>>> Can't happen. Call chain is apic_reset() -> cpu_reset() and apic_reset()
>>> calls  cpu_synchronize_state() before calling cpu_reset().
>> And system_reset?
>>
> And system_reset calls apic_reset() if cpu has apic, cpu_reset()
> otherwise. That is why I said that the bug is only for cpus without
> apic.

Yes, I see now. Still, it's not a good reference for other archs (did
anyone already checked the situation on ppc?). Will file a patch.

Jan

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