From mboxrd@z Thu Jan  1 00:00:00 1970
From: Gleb Natapov <gleb@redhat.com>
Subject: Re: [PATCH 2/2] Don't sync mpstate to/from kernel when unneeded.
Date: Tue, 13 Oct 2009 20:40:24 +0200
Message-ID: <20091013184024.GD25891@redhat.com>
References: <1255436240-994-1-git-send-email-gleb@redhat.com>
 <1255436240-994-2-git-send-email-gleb@redhat.com>
 <20091013183613.GA26518@amt.cnet>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: kvm@vger.kernel.org
To: Marcelo Tosatti <mtosatti@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:58747 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1757596AbZJMSkw (ORCPT <rfc822;kvm@vger.kernel.org>);
	Tue, 13 Oct 2009 14:40:52 -0400
Received: from int-mx05.intmail.prod.int.phx2.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.18])
	by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id n9DIeQ0w016927
	for <kvm@vger.kernel.org>; Tue, 13 Oct 2009 14:40:26 -0400
Content-Disposition: inline
In-Reply-To: <20091013183613.GA26518@amt.cnet>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Tue, Oct 13, 2009 at 03:36:13PM -0300, Marcelo Tosatti wrote:
> On Tue, Oct 13, 2009 at 02:17:20PM +0200, Gleb Natapov wrote:
> > mp_state, unlike other cpu state, can be changed not only from vcpu
> > context it belongs to, but by other vcpus too. That makes its loading
> > from kernel/saving back not safe if mp_state value is changed inside
> > kernel between load and save. For example vcpu 1 loads mp_sate into
> > user-space and the state is RUNNING, vcpu 0 sends INIT/SIPI to vcpu 1
> > so in-kernel mp_sate becomes SIPI, vcpu 1 save user-space copy into
> > kernel and calls vcpu_run(). SIPI sate is lost.
> > 
> > The patch copies mp_sate into kernel only when it is knows that
> > int-kernel value is outdated. This happens on reset and vmload.
> > 
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > ---
> >  hw/apic.c             |    1 +
> >  monitor.c             |    2 ++
> >  qemu-kvm.c            |    9 ++++-----
> >  qemu-kvm.h            |    1 -
> >  target-i386/machine.c |    3 +++
> >  5 files changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/apic.c b/hw/apic.c
> > index 2952675..7244449 100644
> > --- a/hw/apic.c
> > +++ b/hw/apic.c
> > @@ -512,6 +512,7 @@ void apic_init_reset(CPUState *env)
> >      if (kvm_enabled() && qemu_kvm_irqchip_in_kernel()) {
> >          env->mp_state
> >              = env->halted ? KVM_MP_STATE_UNINITIALIZED : KVM_MP_STATE_RUNNABLE;
> > +        kvm_load_mpstate(env);
> >      }
> >  #endif
> >  }
> > diff --git a/monitor.c b/monitor.c
> > index 7f0f5a9..dd8f2ca 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -350,6 +350,7 @@ static CPUState *mon_get_cpu(void)
> >          mon_set_cpu(0);
> >      }
> >      cpu_synchronize_state(cur_mon->mon_cpu);
> > +    kvm_save_mpstate(cur_mon->mon_cpu);
> >      return cur_mon->mon_cpu;
> >  }
> >  
> > @@ -377,6 +378,7 @@ static void do_info_cpus(Monitor *mon)
> >  
> >      for(env = first_cpu; env != NULL; env = env->next_cpu) {
> >          cpu_synchronize_state(env);
> > +        kvm_save_mpstate(env);
> >          monitor_printf(mon, "%c CPU #%d:",
> >                         (env == mon->mon_cpu) ? '*' : ' ',
> >                         env->cpu_index);
> > diff --git a/qemu-kvm.c b/qemu-kvm.c
> > index 3765818..2a1e0ff 100644
> > --- a/qemu-kvm.c
> > +++ b/qemu-kvm.c
> > @@ -1609,11 +1609,6 @@ static void on_vcpu(CPUState *env, void (*func)(void *data), void *data)
> >  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
> 
> Why don't you keep saving it here (so there's no need to do it
> explicitly elsewhere), and only explictly loading?
To keep kvm_arch_get_registers/kvm_arch_set_registers symmetric I guess.

--
			Gleb.