Gleb Natapov wrote:
> On Wed, Sep 09, 2009 at 05:47:13PM +0200, Jan Kiszka wrote:
>> Gleb Natapov wrote:
>>> regs_modified logic doesn't work if io thread calls
>>> kvm_cpu_synchronize_state() since kvm_arch_get_registers()
>>> returns only after vcpu thread is back to kernel. Setting
>>> regs_modified to 1 at this stage causes loading of wrong vcpu
>>> state on the next vcpu_run().
>> We need this upstream too, right? Could you file the corresponding patch?
>>
> Upstream is single threaded. It shouldn't suffer from this bug.
Not if you enable iothread support (though I don't remember if that
works now for kvm) + you are also touching shared code here. So qemu-kvm
would benefit from keeping the diff small.
Jan
>
>> Thanks,
>> Jan
>>
>>> Signed-off-by: Gleb Natapov <gleb@xxxxxxxxxx>
>>> diff --git a/qemu-kvm.c b/qemu-kvm.c
>>> index 06efd41..9ab0cec 100644
>>> --- a/qemu-kvm.c
>>> +++ b/qemu-kvm.c
>>> @@ -874,14 +874,6 @@ int kvm_set_mpstate(kvm_vcpu_context_t vcpu, struct kvm_mp_state *mp_state)
>>> }
>>> #endif
>>>
>>> -void kvm_cpu_synchronize_state(CPUState *env)
>>> -{
>>> - if (!env->kvm_cpu_state.regs_modified) {
>>> - kvm_arch_get_registers(env);
>>> - env->kvm_cpu_state.regs_modified = 1;
>>> - }
>>> -}
>>> -
>>> static int handle_mmio(kvm_vcpu_context_t vcpu)
>>> {
>>> unsigned long addr = vcpu->run->mmio.phys_addr;
>>> @@ -1539,6 +1531,22 @@ static void on_vcpu(CPUState *env, void (*func)(void *data), void *data)
>>> qemu_cond_wait(&qemu_work_cond);
>>> }
>>>
>>> +static void do_kvm_cpu_synchronize_state(void *_env)
>>> +{
>>> + CPUState *env = _env;
>>> + if (!env->kvm_cpu_state.regs_modified) {
>>> + kvm_arch_save_regs(env);
>>> + kvm_arch_load_mpstate(env);
>>> + env->kvm_cpu_state.regs_modified = 1;
>>> + }
>>> +}
>>> +
>>> +void kvm_cpu_synchronize_state(CPUState *env)
>>> +{
>>> + if (!env->kvm_cpu_state.regs_modified)
>>> + on_vcpu(env, do_kvm_cpu_synchronize_state, env);
>>> +}
>>> +
>>> static void inject_interrupt(void *data)
>>> {
>>> cpu_interrupt(current_env, (long) data);
>>> diff --git a/qemu-kvm.h b/qemu-kvm.h
>>> index 2c1730b..32f74b3 100644
>>> --- a/qemu-kvm.h
>>> +++ b/qemu-kvm.h
>>> @@ -1153,12 +1153,6 @@ static inline int kvm_sync_vcpus(void)
>>> return 0;
>>> }
>>>
>>> -static inline void kvm_arch_get_registers(CPUState *env)
>>> -{
>>> - kvm_save_registers(env);
>>> - kvm_save_mpstate(env);
>>> -}
>>> -
>>> static inline void kvm_arch_put_registers(CPUState *env)
>>> {
>>> kvm_load_registers(env);
>>> diff --git a/target-i386/helper.c b/target-i386/helper.c
>>> index 4a16887..57c74a2 100644
>>> --- a/target-i386/helper.c
>>> +++ b/target-i386/helper.c
>>> @@ -746,7 +746,7 @@ void cpu_dump_state(CPUState *env, FILE *f,
>>> static const char *seg_name[6] = { "ES", "CS", "SS", "DS", "FS", "GS" };
>>>
>>> if (kvm_enabled())
>>> - kvm_arch_get_registers(env);
>>> + kvm_cpu_synchronize_state(env);
>>>
>>> eflags = env->eflags;
>>> #ifdef TARGET_X86_64
>>> --
>>> Gleb.
>> --
>> Siemens AG, Corporate Technology, CT SE 2
>> Corporate Competence Center Embedded Linux
>
> --
> Gleb.
--
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
