Re: [PATCH 3/6] kvm-s390: make sigp restart return busy when stop pending

[Christian Borntraeger <borntraeger@de.ibm.com>]

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