[PATCH 1/3] kvm-s390: fix registering memory regions while vcpus do exist

[PATCH 1/3] kvm-s390: fix registering memory regions while vcpus do exist

[Carsten Otte]

This patch fixes an incorrectness in the kvm backend for s390.
In case virtual cpus are being created before the corresponding
memory slot is being registered, we need to update the sie
control blocks for the virtual cpus. In order to do that, we
use the vcpu->mutex to lock out kvm_run and friends. This way
we can ensure a consistent update of the memory for the entire
smp configuration.

Signed-off-by: Carsten Otte <cotte@de.ibm.com>
---
 arch/s390/kvm/kvm-s390.c |   25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

Index: kvm/arch/s390/kvm/kvm-s390.c
===================================================================
--- kvm.orig/arch/s390/kvm/kvm-s390.c
+++ kvm/arch/s390/kvm/kvm-s390.c
@@ -660,6 +660,8 @@ int kvm_arch_set_memory_region(struct kv
 				struct kvm_memory_slot old,
 				int user_alloc)
 {
+	int i;
+
 	/* A few sanity checks. We can have exactly one memory slot which has
 	   to start at guest virtual zero and which has to be located at a
 	   page boundary in userland and which has to end at a page boundary.
@@ -679,13 +681,28 @@ int kvm_arch_set_memory_region(struct kv
 	if (mem->memory_size & (PAGE_SIZE - 1))
 		return -EINVAL;
 
+	/* lock all vcpus */
+	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
+		if (kvm->vcpus[i]) {
+			mutex_lock(&kvm->vcpus[i]->mutex);
+		}
+	}
+
 	kvm->arch.guest_origin = mem->userspace_addr;
 	kvm->arch.guest_memsize = mem->memory_size;
 
-	/* FIXME: we do want to interrupt running CPUs and update their memory
-	   configuration now to avoid race conditions. But hey, changing the
-	   memory layout while virtual CPUs are running is usually bad
-	   programming practice. */
+	/* update sie control blocks, and unlock all vcpus */
+	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
+		if (kvm->vcpus[i]) {
+			kvm->vcpus[i]->arch.sie_block->gmsor =
+				kvm->arch.guest_origin;
+			kvm->vcpus[i]->arch.sie_block->gmslm =
+				kvm->arch.guest_memsize +
+				kvm->arch.guest_origin +
+				VIRTIODESCSPACE - 1ul;
+			mutex_unlock(&kvm->vcpus[i]->mutex);
+		}
+	}
 
 	return 0;
 }

[PATCH 2/3] kvm-s390: verify that memory slot is present for vm in kvm_run

[Carsten Otte]

This check verifies that the guest we're trying to run in KVM_RUN
has some memory assigned to it. It enters an endless exception
loop if this is not the case.

Reported-by: Mijo Safradin <mijo@linux.vnet.ibm.com>
Signed-off-by: Carsten Otte <cotte@de.ibm.com>
---
 arch/s390/kvm/kvm-s390.c |    6 ++++++
 1 file changed, 6 insertions(+)

Index: kvm/arch/s390/kvm/kvm-s390.c
===================================================================
--- kvm.orig/arch/s390/kvm/kvm-s390.c
+++ kvm/arch/s390/kvm/kvm-s390.c
@@ -481,6 +481,12 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_v
 
 	vcpu_load(vcpu);
 
+	/* verify, that memory has been registered */
+	if (!vcpu->kvm->arch.guest_memsize) {
+		vcpu_put(vcpu);
+		return -EINVAL;
+	}
+
 	if (vcpu->sigset_active)
 		sigprocmask(SIG_SETMASK, &vcpu->sigset, &sigsaved);
 

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

[Carsten Otte]

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);

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

[Carsten Otte]

Am Thu, 5 Feb 2009 17:05:01 +0100
schrieb Carsten Otte <cotte@de.ibm.com>:
> 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.
This patch is broken, will refresh it. Please do _NOT_ apply (!)

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

[Carsten Otte]

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[n])
+		return -EEXIST;
+
 	vcpu = kvm_arch_vcpu_create(kvm, n);
 	if (IS_ERR(vcpu))
 		return PTR_ERR(vcpu);

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

[Marcelo Tosatti]

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.

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?

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

[Avi Kivity]

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

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

[Carsten Otte]

Am Sun, 8 Feb 2009 04:26:16 -0200
schrieb Marcelo Tosatti <mtosatti@redhat.com>:
> 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.
> 
> 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?
Okay, let me see what I can do to come up with a better fix.
On s390, we have to link our sie control blocks (representing
vcpu for the hardware) with our sca (representing virtual
machine for the hardware). These are bidirectional pointers.