Re: [PATCH 3/3] kvm common: verify that cpu slot is available when creating new vcpu

[Avi Kivity <avi@redhat.com>]

Marcelo Tosatti wrote:
> On Thu, Feb 05, 2009 at 05:05:01PM +0100, Carsten Otte wrote:
>   
>> KVM common code should'nt try to create the same virtual cpu twice.
>> In case of s390, it crashes badly in kvm_arch_vcpu_create.
>>
>> Reported-by: Mijo Safradin <mijo@linux.vnet.ibm.com>
>> Signed-off-by: Carsten Otte <cotte@de.ibm.com>
>> ---
>>  virt/kvm/kvm_main.c |    3 +++
>>  1 file changed, 3 insertions(+)
>>
>> Index: kvm/virt/kvm/kvm_main.c
>> ===================================================================
>> --- kvm.orig/virt/kvm/kvm_main.c
>> +++ kvm/virt/kvm/kvm_main.c
>> @@ -1605,6 +1605,9 @@ static int kvm_vm_ioctl_create_vcpu(stru
>>  	if (!valid_vcpu(n))
>>  		return -EINVAL;
>>  
>> +	if (kvm->vcpus[i])
>> +		return -EEXIST;
>> +
>>  	vcpu = kvm_arch_vcpu_create(kvm, n);
>>  	if (IS_ERR(vcpu))
>>  		return PTR_ERR(vcpu);
>>     
>
> Its confusing that there is the exact same check below, with kvm->lock
> held, and that both are needed since assignment happens under the lock.
>   

Right, also the proposed fix still leaves a race.

> Can you also make it straightforward while fixing the bug please.
>
> Probably just hold it all the way through kvm_vm_ioctl_create_vcpu? Or
> is that not possible?
>   

The original intent was that kvm_arch_vcpu_create() not "link in" the 
vcpu to any registers.  That allows most of the vcpu creation to happen 
outside a lock.

If it's not doable for s390 we can give this up, but I suggest checking 
if it's possible to keep things as is and modify s390's 
kvm_arch_vcpu_create() not to screw up instead.

-- 
error compiling committee.c: too many arguments to function