From mboxrd@z Thu Jan  1 00:00:00 1970
From: Christian Borntraeger <borntraeger@de.ibm.com>
Subject: Re: [PATCH 3/6] kvm-s390: make sigp restart return busy when stop
 pending
Date: Mon, 06 Feb 2012 14:08:21 +0100
Message-ID: <4F2FD0C5.8050205@de.ibm.com>
References: <1328522347-54635-1-git-send-email-borntraeger@de.ibm.com> <1328522347-54635-4-git-send-email-borntraeger@de.ibm.com> <4F2FBF46.2090604@de.ibm.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-15
Content-Transfer-Encoding: 7bit
Cc: Avi Kivity <avi@redhat.com>, Marcelo Tossati <mtosatti@redhat.com>,
	Alexander Graf <agraf@suse.de>,
	Cornelia Huck <cornelia.huck@de.ibm.com>,
	Heiko Carstens <heiko.carstens@de.ibm.com>,
	Martin Schwidefsky <schwidefsky@de.ibm.com>,
	KVM <kvm@vger.kernel.org>
To: Carsten Otte <cotte@de.ibm.com>,
	Jens Freimann <jfrei@linux.vnet.ibm.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from e06smtp10.uk.ibm.com ([195.75.94.106]:57830 "EHLO
	e06smtp10.uk.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1753420Ab2BFNI6 (ORCPT <rfc822;kvm@vger.kernel.org>);
	Mon, 6 Feb 2012 08:08:58 -0500
Received: from /spool/local
	by e06smtp10.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted
	for <kvm@vger.kernel.org> from <borntraeger@de.ibm.com>;
	Mon, 6 Feb 2012 13:08:56 -0000
Received: from d06av08.portsmouth.uk.ibm.com (d06av08.portsmouth.uk.ibm.com [9.149.37.249])
	by d06nrmr1806.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q16D8OO22814182
	for <kvm@vger.kernel.org>; Mon, 6 Feb 2012 13:08:24 GMT
Received: from d06av08.portsmouth.uk.ibm.com (loopback [127.0.0.1])
	by d06av08.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q16D8NSI002039
	for <kvm@vger.kernel.org>; Mon, 6 Feb 2012 13:08:24 GMT
In-Reply-To: <4F2FBF46.2090604@de.ibm.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On 06/02/12 12:53, Carsten Otte wrote:
> Am 06.02.2012 10:59, schrieb Christian Borntraeger:
>> +static int __sigp_restart(struct kvm_vcpu *vcpu, u16 cpu_addr)
>> +{
>> +    int rc = 0;
>> +    struct kvm_s390_float_interrupt *fi =&vcpu->kvm->arch.float_int;
>> +
>> +    spin_lock(&fi->lock);
>> +    if (fi->local_int[cpu_addr]->action_bits&  ACTION_STOP_ON_STOP)
>> +        rc = 2; /* busy */
>> +    else
>> +        VCPU_EVENT(vcpu, 4, "sigp restart %x to handle userspace",
>> +            cpu_addr);
>> +    spin_unlock(&fi->lock);
>> +
>> +    return rc;
>> +}
>>
> local_int->action_bits is protected by the local int lock of subject CPU,
> as one can see in patch #2 of this series. This is racy.

Indeed the inner lock is missing (something like the addon-patch below)
Jens, can you update and test the patch accordingly?

--- a/arch/s390/kvm/sigp.c
+++ b/arch/s390/kvm/sigp.c
@@ -316,13 +316,26 @@ static int __sigp_restart(struct kvm_vcpu *vcpu, u16 cpu_addr)
 {
        int rc = 0;
        struct kvm_s390_float_interrupt *fi = &vcpu->kvm->arch.float_int;
+       struct kvm_s390_local_interrupt *li;
+
+       if (cpu_addr >= KVM_MAX_VCPUS)
+               return 3; /* not operational */
 
        spin_lock(&fi->lock);
-       if (fi->local_int[cpu_addr]->action_bits & ACTION_STOP_ON_STOP)
+       li = fi->local_int[cpu_addr];
+       if (li == NULL) {
+               rc = 3; /* not operational */
+               goto out;
+       }
+
+       spin_lock_bh(&li->lock);
+       if (li->action_bits & ACTION_STOP_ON_STOP)
                rc = 2; /* busy */
        else
                VCPU_EVENT(vcpu, 4, "sigp restart %x to handle userspace",
                        cpu_addr);
+       spin_unlock_bh(&li->lock);
+out:
        spin_unlock(&fi->lock);
 
        return rc;




Christian