From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:35738) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RB67c-0008Nx-Tx for qemu-devel@nongnu.org; Tue, 04 Oct 2011 10:37:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RB67X-0000m2-0c for qemu-devel@nongnu.org; Tue, 04 Oct 2011 10:37:28 -0400 Received: from cantor2.suse.de ([195.135.220.15]:35419 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RB67W-0000ly-Ol for qemu-devel@nongnu.org; Tue, 04 Oct 2011 10:37:22 -0400 Message-ID: <4E8B1A26.8020405@suse.de> Date: Tue, 04 Oct 2011 16:37:26 +0200 From: Alexander Graf MIME-Version: 1.0 References: <4E8AEDD9.1000401@de.ibm.com> <4E8AF7CB.9070600@suse.de> <4E8B0E8C.9030609@de.ibm.com> In-Reply-To: <4E8B0E8C.9030609@de.ibm.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] s390: Fix cpu shutdown for KVM List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Christian Borntraeger Cc: Carsten Otte , qemu-devel@nongnu.org On 10/04/2011 03:47 PM, Christian Borntraeger wrote: > Something like the following? > > s390: Fix cpu shutdown for KVM > > On s390 a shutdown is the state of all CPUs being either stopped > or disabled (for interrupts) waiting. We have to track this number > to call the shutdown sequence accordingly. This patch implements > the counting and shutdown handling for the kvm path in qemu. > > Signed-off-by: Christian Borntraeger > --- > hw/s390-virtio.c | 21 +++++++++++++++++++++ > target-s390x/cpu.h | 12 ++++++++++++ > target-s390x/kvm.c | 21 +++++++++++++++++---- > 3 files changed, 50 insertions(+), 4 deletions(-) > > Index: b/hw/s390-virtio.c > =================================================================== > --- a/hw/s390-virtio.c > +++ b/hw/s390-virtio.c > @@ -130,6 +130,26 @@ int s390_virtio_hypercall(CPUState *env, > return r; > } > > +/* > + * The number of running CPUs. On s390 a shutdown is the state of all CPUs > + * being either stopped or disabled (for interrupts) waiting. We have to > + * track this number to call the shutdown sequence accordingly. This > + * number is modified either on startup or while holding the big qemu lock. > + */ > +static unsigned s390_running_cpus; > + > +void kvm_s390_add_running_cpu(CPUState *env) > +{ > + assert(env->halted == 0); > + s390_running_cpus++; > +} > + > +unsigned kvm_s390_del_running_cpu(CPUState *env) > +{ > + assert(s390_running_cpus>= 1); > + return --s390_running_cpus; > +} Almost. It has nothing to do with KVM though. Please keep the abstraction intact! > + > /* PC hardware initialisation */ > static void s390_init(ram_addr_t my_ram_size, > const char *boot_device, > @@ -189,6 +209,7 @@ static void s390_init(ram_addr_t my_ram_ > > env->halted = 0; > env->exception_index = 0; > + kvm_s390_add_running_cpu(env); > > if (kernel_filename) { > kernel_size = load_image(kernel_filename, qemu_get_ram_ptr(0)); > Index: b/target-s390x/cpu.h > =================================================================== > --- a/target-s390x/cpu.h > +++ b/target-s390x/cpu.h > @@ -292,6 +292,8 @@ void kvm_s390_interrupt(CPUState *env, i > void kvm_s390_virtio_irq(CPUState *env, int config_change, uint64_t token); > void kvm_s390_interrupt_internal(CPUState *env, int type, uint32_t parm, > uint64_t parm64, int vm); > +void kvm_s390_add_running_cpu(CPUState *env); > +unsigned kvm_s390_del_running_cpu(CPUState *env); > #else > static inline void kvm_s390_interrupt(CPUState *env, int type, uint32_t code) > { > @@ -307,6 +309,16 @@ static inline void kvm_s390_interrupt_in > int vm) > { > } > + > +static inline void kvm_s390_add_running_cpu(CPUState *env) > +{ > +} > + > +static inline unsigned kvm_s390_del_running_cpu(CPUState *env) > +{ > + return 0; > +} The functions are defined in s390-virtio.c and thus are available anywhere now. No need to special-case KVM anywhere. > + > #endif > CPUState *s390_cpu_addr2state(uint16_t cpu_addr); > > Index: b/target-s390x/kvm.c > =================================================================== > --- a/target-s390x/kvm.c > +++ b/target-s390x/kvm.c > @@ -185,6 +185,12 @@ void kvm_s390_interrupt_internal(CPUStat > return; > } > > + /* > + * We can only deliver interrupts to (interrupt) enabled CPUs. > + * We dont call kvm_s390_add_running_cpu here, since CPUs in enabled wait > + * will wait inside the kernel (no exit). Therefore, the targeted > + * CPUs was neither disabled waiting or stopped for qemu. > + */ > env->halted = 0; > env->exception_index = -1; > qemu_cpu_kick(env); > @@ -301,6 +307,7 @@ static int s390_cpu_restart(CPUState *en > kvm_s390_interrupt(env, KVM_S390_RESTART, 0); > env->halted = 0; > env->exception_index = -1; > + kvm_s390_add_running_cpu(env); Hrm. Is add_running correct here? Maybe we should add a wrapper around the halted = 0 parts in s390-virtio.c and do the increase of the counter there? That way we can make sure that transitioning from 0 -> 0 doesn't increase the counter. Alex