kvm-s390: revised version of kvm-s390 guest memory handling

PATCH In review [email protected] 3 participants 11 messages May 20, 2009 – May 26, 2009

Subsystems (3)

KVM s390 KVM s390

lore

Share & tools

lore

[email protected]

author

2009-05-20 13:35 UTC #0 lore raw

cover kvm-s390: revised version of kvm-s390 guest memory handling

From: Christian Ehrhardt <[email protected]>

This patch series results from our discussions about handling memslots and vcpu mmu reloads. It streamlines kvm-s390 a bit by using slots_lock, vcpu-request (KVM_REQ_MMU_RELOAD) and a kick mechanism to ensure vcpus come out of guest context to catch the update.

I tested the reworked code a while with multiple smp guests and some extra code that periodically injects kicks and/or mmu reload requests, but I'm happy about every additional review feedback.

Patches included:
Subject: [PATCH 1/3] kvm-s390: infrastructure to kick vcpus out of guest state
Subject: [PATCH 2/3] kvm-s390: fix signal handling
Subject: [PATCH 3/3] kvm-s390: streamline memslot handling

Overall-Diffstat:

 arch/s390/include/asm/kvm_host.h |    9 ++---
 arch/s390/kvm/gaccess.h          |   23 +++++++-------
 arch/s390/kvm/intercept.c        |   18 +++++++----
 arch/s390/kvm/kvm-s390.c         |   57 ++++++++++++++-----------------------
 arch/s390/kvm/kvm-s390.h         |   32 ++++++++++++++++++++
 arch/s390/kvm/sigp.c             |   60 +++++++++++++++++++++++----------------
 6 files changed, 117 insertions(+), 82 deletions(-)

[email protected]

author

2009-05-20 13:36 UTC #1 lore raw

PATCH 1/3 kvm-s390: infrastructure to kick vcpus out of guest state

From: Christian Ehrhardt <[email protected]>

To ensure vcpu's come out of guest context in certain cases this patch adds a s390 specific way to kick them out of guest context. Currently it kicks them out to rerun the vcpu_run path in the s390 code, but the mechanism itself is expandable and with a new flag we could also add e.g. kicks to userspace etc.

Signed-off-by Christian Ehrhardt <[email protected]>

---

[diffstat]

 include/asm/kvm_host.h |    5 ++--
 kvm/intercept.c        |   12 ++++++++---
 kvm/kvm-s390.c         |    5 ++++
 kvm/kvm-s390.h         |    3 ++
 kvm/sigp.c             |   53 +++++++++++++++++++++++++++++--------------------
 5 files changed, 52 insertions(+), 26 deletions(-)

[diff]

arch/s390/kvm/intercept.c +9 -3

Index: kvm/arch/s390/kvm/intercept.c
===================================================================
@@ -128,7 +128,7 @@ static int handle_noop(struct kvm_vcpu *
 
 static int handle_stop(struct kvm_vcpu *vcpu)
 {
-	int rc;
+	int rc = 0;
 
 	vcpu->stat.exit_stop_request++;
 	atomic_clear_mask(CPUSTAT_RUNNING, &vcpu->arch.sie_block->cpuflags);

@@ -141,12 +141,18 @@ static int handle_stop(struct kvm_vcpu *
 			rc = -ENOTSUPP;
 	}
 
+	if (vcpu->arch.local_int.action_bits & ACTION_RELOADVCPU_ON_STOP) {
+		vcpu->arch.local_int.action_bits &= ~ACTION_RELOADVCPU_ON_STOP;
+		rc = SIE_INTERCEPT_RERUNVCPU;
+		vcpu->run->exit_reason = KVM_EXIT_INTR;
+	}
+
 	if (vcpu->arch.local_int.action_bits & ACTION_STOP_ON_STOP) {
 		vcpu->arch.local_int.action_bits &= ~ACTION_STOP_ON_STOP;
 		VCPU_EVENT(vcpu, 3, "%s", "cpu stopped");
 		rc = -ENOTSUPP;
-	} else
-		rc = 0;
+	}
+
 	spin_unlock_bh(&vcpu->arch.local_int.lock);
 	return rc;
 }

arch/s390/kvm/kvm-s390.c +5 -0

Index: kvm/arch/s390/kvm/kvm-s390.c
===================================================================
@@ -487,6 +487,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_v
 
 	vcpu_load(vcpu);
 
+rerun_vcpu:
 	/* verify, that memory has been registered */
 	if (!vcpu->kvm->arch.guest_memsize) {
 		vcpu_put(vcpu);

@@ -506,6 +507,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_v
 		vcpu->arch.sie_block->gpsw.addr = kvm_run->s390_sieic.addr;
 		break;
 	case KVM_EXIT_UNKNOWN:
+	case KVM_EXIT_INTR:
 	case KVM_EXIT_S390_RESET:
 		break;
 	default:

@@ -519,6 +521,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_v
 		rc = kvm_handle_sie_intercept(vcpu);
 	} while (!signal_pending(current) && !rc);
 
+	if (rc == SIE_INTERCEPT_RERUNVCPU)
+		goto rerun_vcpu;
+
 	if (signal_pending(current) && !rc)
 		rc = -EINTR;

arch/s390/kvm/kvm-s390.h +3 -0

Index: kvm/arch/s390/kvm/kvm-s390.h
===================================================================
@@ -20,6 +20,8 @@
 
 typedef int (*intercept_handler_t)(struct kvm_vcpu *vcpu);
 
+/* negativ values are error codes, positive values for internal conditions */
+#define SIE_INTERCEPT_RERUNVCPU		(1<<0)
 int kvm_handle_sie_intercept(struct kvm_vcpu *vcpu);
 
 #define VM_EVENT(d_kvm, d_loglevel, d_string, d_args...)\

@@ -50,6 +52,7 @@ int kvm_s390_inject_vm(struct kvm *kvm,
 int kvm_s390_inject_vcpu(struct kvm_vcpu *vcpu,
 		struct kvm_s390_interrupt *s390int);
 int kvm_s390_inject_program_int(struct kvm_vcpu *vcpu, u16 code);
+int kvm_s390_inject_sigp_stop(struct kvm_vcpu *vcpu, int action);
 
 /* implemented in priv.c */
 int kvm_s390_handle_b2(struct kvm_vcpu *vcpu);

arch/s390/include/asm/kvm_host.h +3 -2

Index: kvm/arch/s390/include/asm/kvm_host.h
===================================================================
@@ -180,8 +180,9 @@ struct kvm_s390_interrupt_info {
 };
 
 /* for local_interrupt.action_flags */
-#define ACTION_STORE_ON_STOP 1
-#define ACTION_STOP_ON_STOP  2
+#define ACTION_STORE_ON_STOP		(1<<0)
+#define ACTION_STOP_ON_STOP		(1<<1)
+#define ACTION_RELOADVCPU_ON_STOP	(1<<2)
 
 struct kvm_s390_local_interrupt {
 	spinlock_t lock;

arch/s390/kvm/sigp.c +34 ~~-22~~

104 lines not on this page · raw message

[email protected]

author

2009-05-20 13:34 UTC #2 lore raw

PATCH 2/3 kvm-s390: fix signal handling

In reply to [email protected]

From: Christian Ehrhardt <[email protected]>

If signal pending is true we exit without updating kvm_run, userspace currently just does nothing and jumps to kvm_run again. Since we did not set an exit_reason we might end up with a random one (whatever was the last exit). Therefore it was possible to e.g. jump to the psw position the last real interruption set.
Setting the INTR exit reason ensures that no old psw data is swapped in on reentry.

Signed-off-by Christian Ehrhardt <[email protected]>

---

[diffstat]

 kvm-s390.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

[diff]

arch/s390/kvm/kvm-s390.c +3 -1

Index: kvm/arch/s390/kvm/kvm-s390.c
===================================================================
@@ -524,8 +524,10 @@ rerun_vcpu:
 	if (rc == SIE_INTERCEPT_RERUNVCPU)
 		goto rerun_vcpu;
 
-	if (signal_pending(current) && !rc)
+	if (signal_pending(current) && !rc) {
+		kvm_run->exit_reason = KVM_EXIT_INTR;
 		rc = -EINTR;
+	}
 
 	if (rc == -ENOTSUPP) {
 		/* intercept cannot be handled in-kernel, prepare kvm-run */

[email protected]

author

2009-05-20 13:34 UTC #3 lore raw

PATCH 3/3 kvm-s390: streamline memslot handling

In reply to [email protected]

From: Christian Ehrhardt <[email protected]>

This patch relocates the variables kvm-s390 uses to track guest mem addr/size. As discussed dropping the variables at struct kvm_arch level allows to use the common vcpu->request based mechanism to reload guest memory if e.g. changes via set_memory_region.
The kick mechanism introduced in this series is used to ensure running vcpus leave guest state to catch the update.

Signed-off-by Christian Ehrhardt <[email protected]>

---

[diffstat]

 include/asm/kvm_host.h |    4 +---
 kvm/gaccess.h          |   23 ++++++++++++-----------
 kvm/intercept.c        |    6 +++---
 kvm/kvm-s390.c         |   48 ++++++++++++++----------------------------------
 kvm/kvm-s390.h         |   29 ++++++++++++++++++++++++++++-
 kvm/sigp.c             |    4 ++--
 6 files changed, 60 insertions(+), 54 deletions(-)

[diff]

arch/s390/kvm/gaccess.h +12 ~~-11~~

Index: kvm/arch/s390/kvm/gaccess.h
===================================================================
@@ -1,7 +1,7 @@
 /*
  * gaccess.h -  access guest memory
  *
- * Copyright IBM Corp. 2008
+ * Copyright IBM Corp. 2008,2009
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License (version 2 only)

@@ -16,13 +16,14 @@
 #include <linux/compiler.h>
 #include <linux/kvm_host.h>
 #include <asm/uaccess.h>
+#include "kvm-s390.h"
 
 static inline void __user *__guestaddr_to_user(struct kvm_vcpu *vcpu,
 					       unsigned long guestaddr)
 {
 	unsigned long prefix  = vcpu->arch.sie_block->prefix;
-	unsigned long origin  = vcpu->kvm->arch.guest_origin;
-	unsigned long memsize = vcpu->kvm->arch.guest_memsize;
+	unsigned long origin  = vcpu->arch.sie_block->gmsor;
+	unsigned long memsize = kvm_s390_vcpu_get_memsize(vcpu);
 
 	if (guestaddr < 2 * PAGE_SIZE)
 		guestaddr += prefix;

@@ -158,8 +159,8 @@ static inline int copy_to_guest(struct k
 				const void *from, unsigned long n)
 {
 	unsigned long prefix  = vcpu->arch.sie_block->prefix;
-	unsigned long origin  = vcpu->kvm->arch.guest_origin;
-	unsigned long memsize = vcpu->kvm->arch.guest_memsize;
+	unsigned long origin  = vcpu->arch.sie_block->gmsor;
+	unsigned long memsize = kvm_s390_vcpu_get_memsize(vcpu);
 
 	if ((guestdest < 2 * PAGE_SIZE) && (guestdest + n > 2 * PAGE_SIZE))
 		goto slowpath;

@@ -209,8 +210,8 @@ static inline int copy_from_guest(struct
 				  unsigned long guestsrc, unsigned long n)
 {
 	unsigned long prefix  = vcpu->arch.sie_block->prefix;
-	unsigned long origin  = vcpu->kvm->arch.guest_origin;
-	unsigned long memsize = vcpu->kvm->arch.guest_memsize;
+	unsigned long origin  = vcpu->arch.sie_block->gmsor;
+	unsigned long memsize = kvm_s390_vcpu_get_memsize(vcpu);
 
 	if ((guestsrc < 2 * PAGE_SIZE) && (guestsrc + n > 2 * PAGE_SIZE))
 		goto slowpath;

@@ -244,8 +245,8 @@ static inline int copy_to_guest_absolute
 					 unsigned long guestdest,
 					 const void *from, unsigned long n)
 {
-	unsigned long origin  = vcpu->kvm->arch.guest_origin;
-	unsigned long memsize = vcpu->kvm->arch.guest_memsize;
+	unsigned long origin  = vcpu->arch.sie_block->gmsor;
+	unsigned long memsize = kvm_s390_vcpu_get_memsize(vcpu);
 
 	if (guestdest + n > memsize)
 		return -EFAULT;

@@ -262,8 +263,8 @@ static inline int copy_from_guest_absolu
 					   unsigned long guestsrc,
 					   unsigned long n)
 {
-	unsigned long origin  = vcpu->kvm->arch.guest_origin;
-	unsigned long memsize = vcpu->kvm->arch.guest_memsize;
+	unsigned long origin  = vcpu->arch.sie_block->gmsor;
+	unsigned long memsize = kvm_s390_vcpu_get_memsize(vcpu);
 
 	if (guestsrc + n > memsize)
 		return -EFAULT;

arch/s390/kvm/intercept.c +3 -3

Index: kvm/arch/s390/kvm/intercept.c
===================================================================
@@ -1,7 +1,7 @@
 /*
  * intercept.c - in-kernel handling for sie intercepts
  *
- * Copyright IBM Corp. 2008
+ * Copyright IBM Corp. 2008,2009
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License (version 2 only)

@@ -164,9 +164,9 @@ static int handle_validity(struct kvm_vc
 
 	vcpu->stat.exit_validity++;
 	if ((viwhy == 0x37) && (vcpu->arch.sie_block->prefix
-		<= vcpu->kvm->arch.guest_memsize - 2*PAGE_SIZE)){
+		<= kvm_s390_vcpu_get_memsize(vcpu) - 2*PAGE_SIZE)) {
 		rc = fault_in_pages_writeable((char __user *)
-			 vcpu->kvm->arch.guest_origin +
+			 vcpu->arch.sie_block->gmsor +
 			 vcpu->arch.sie_block->prefix,
 			 2*PAGE_SIZE);
 		if (rc)

arch/s390/kvm/kvm-s390.c +14 ~~-34~~

95 lines not on this page · raw message

arch/s390/kvm/kvm-s390.h +28 -1

Index: kvm/arch/s390/kvm/kvm-s390.h
===================================================================
@@ -1,7 +1,7 @@
 /*
  * kvm_s390.h -  definition for kvm on s390
  *
- * Copyright IBM Corp. 2008
+ * Copyright IBM Corp. 2008,2009
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License (version 2 only)

@@ -9,6 +9,7 @@
  *
  *    Author(s): Carsten Otte <[email protected]>
  *               Christian Borntraeger <[email protected]>
+ *               Christian Ehrhardt <[email protected]>
  */
 
 #ifndef ARCH_S390_KVM_S390_H

@@ -18,6 +19,9 @@
 #include <linux/kvm.h>
 #include <linux/kvm_host.h>
 
+/* The current code can have up to 256 pages for virtio */
+#define VIRTIODESCSPACE (256ul * 4096ul)
+
 typedef int (*intercept_handler_t)(struct kvm_vcpu *vcpu);
 
 /* negativ values are error codes, positive values for internal conditions */

@@ -54,6 +58,29 @@ int kvm_s390_inject_vcpu(struct kvm_vcpu
 int kvm_s390_inject_program_int(struct kvm_vcpu *vcpu, u16 code);
 int kvm_s390_inject_sigp_stop(struct kvm_vcpu *vcpu, int action);
 
+static inline int kvm_s390_vcpu_get_memsize(struct kvm_vcpu *vcpu)
+{
+	return vcpu->arch.sie_block->gmslm
+		- vcpu->arch.sie_block->gmsor
+		- VIRTIODESCSPACE + 1ul;
+}
+
+static inline void kvm_s390_vcpu_set_mem(struct kvm_vcpu *vcpu)
+{
+	struct kvm_memory_slot *mem;
+
+	down_read(&vcpu->kvm->slots_lock);
+	mem = &vcpu->kvm->memslots[0];
+
+	vcpu->arch.sie_block->gmsor = mem->userspace_addr;
+	vcpu->arch.sie_block->gmslm =
+		mem->userspace_addr +
+		(mem->npages << PAGE_SHIFT) +
+		VIRTIODESCSPACE - 1ul;
+
+	up_read(&vcpu->kvm->slots_lock);
+}
+
 /* implemented in priv.c */
 int kvm_s390_handle_b2(struct kvm_vcpu *vcpu);

arch/s390/include/asm/kvm_host.h +1 -3

Index: kvm/arch/s390/include/asm/kvm_host.h
===================================================================
@@ -1,7 +1,7 @@
 /*
  * asm-s390/kvm_host.h - definition for kernel virtual machines on s390
  *
- * Copyright IBM Corp. 2008
+ * Copyright IBM Corp. 2008,2009
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License (version 2 only)

@@ -226,8 +226,6 @@ struct kvm_vm_stat {
 };
 
 struct kvm_arch{
-	unsigned long guest_origin;
-	unsigned long guest_memsize;
 	struct sca_block *sca;
 	debug_info_t *dbf;
 	struct kvm_s390_float_interrupt float_int;

arch/s390/kvm/sigp.c +2 -2

Index: kvm/arch/s390/kvm/sigp.c
===================================================================
@@ -189,9 +189,9 @@ static int __sigp_set_prefix(struct kvm_
 	/* make sure that the new value is valid memory */
 	address = address & 0x7fffe000u;
 	if ((copy_from_guest(vcpu, &tmp,
-		(u64) (address + vcpu->kvm->arch.guest_origin) , 1)) ||
+		(u64) (address + vcpu->arch.sie_block->gmsor) , 1)) ||
 	   (copy_from_guest(vcpu, &tmp, (u64) (address +
-			vcpu->kvm->arch.guest_origin + PAGE_SIZE), 1))) {
+			vcpu->arch.sie_block->gmsor + PAGE_SIZE), 1))) {
 		*reg |= SIGP_STAT_INVALID_PARAMETER;
 		return 1; /* invalid parameter */
 	}

Avi Kivity

2009-05-24 14:41 UTC #4 lore raw

Re: [PATCH 3/3] kvm-s390: streamline memslot handling

Quoted 14 lines · [email protected] wrote:

From: Christian Ehrhardt <[email protected]>
This patch relocates the variables kvm-s390 uses to track guest mem addr/size. As discussed dropping the variables at struct kvm_arch level allows to use the common vcpu->request based mechanism to reload guest memory if e.g. changes via set_memory_region.
The kick mechanism introduced in this series is used to ensure running vcpus leave guest state to catch the update.
rerun_vcpu:
arch/s390/kvm/kvm-s390.c +1 -0
+	if (vcpu->requests)

arch/s390/kvm/kvm-s390.c int kvm_arch_vcpu_ioctl_run(struct kvm_v · line 495

Quoted 10 lines

arch/s390/kvm/kvm-s390.c +5 -1

+		if (test_and_clear_bit(KVM_REQ_MMU_RELOAD, &vcpu->requests))
+			kvm_s390_vcpu_set_mem(vcpu);
+
 	/* verify, that memory has been registered */
-	if (!vcpu->kvm->arch.guest_memsize) {
+	if (!vcpu->arch.sie_block->gmslm) {
 		vcpu_put(vcpu);
+		VCPU_EVENT(vcpu, 3, "%s", "no memory registered to run vcpu");
 		return -EINVAL;
 	}

x86 uses a double check: first we check vcpu->requests outside atomic context, then we enter the critical section and check again for signals and vcpu->requests.

This allows us (a) to do naughty things in vcpu->requests handlers, (b) keep the critical section short.

Does this apply here?

Quoted 7 lines

arch/s390/kvm/kvm-s390.c +1 -3

-	/* update sie control blocks, and unlock all vcpus */
+	/* request update of sie control block for all available vcpus */
 	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
 		if (kvm->vcpus[i]) {
-			kvm->vcpus[i]->arch.sie_block->gmsor =
-				kvm->arch.guest_origin;

arch/s390/kvm/kvm-s390.c int kvm_arch_set_memory_region(struct kv · line 713

Quoted 10 lines

arch/s390/kvm/kvm-s390.c +3 -5

-			kvm->vcpus[i]->arch.sie_block->gmslm =
-				kvm->arch.guest_memsize +
-				kvm->arch.guest_origin +
-				VIRTIODESCSPACE - 1ul;
-			mutex_unlock(&kvm->vcpus[i]->mutex);
+			set_bit(KVM_REQ_MMU_RELOAD, &kvm->vcpus[i]->requests);
+			kvm_s390_inject_sigp_stop(kvm->vcpus[i],
+						  ACTION_RELOADVCPU_ON_STOP);
 		}
 	}

There already exists a loop which does this, see make_all_cpus_request(). It uses an IPI (Marcelo, can't it use the reschedule interrupt?). It has a couple of optimizations -- if the request is already set, it skips the IPI, and it avoids the IPI for vcpus out of guest mode. Maybe it could fit s390 too.

Signature · 1 line

error compiling committee.c: too many arguments to function

Christian Ehrhardt

author

2009-05-25 08:34 UTC #5 lore raw

Re: [PATCH 3/3] kvm-s390: streamline memslot handling

Quoted 26 lines · Avi Kivity

Quoted 13 lines · [email protected] wrote:
From: Christian Ehrhardt <[email protected]>
This patch relocates the variables kvm-s390 uses to track guest mem addr/size. As discussed dropping the variables at struct kvm_arch level allows to use the common vcpu->request based mechanism to reload guest memory if e.g. changes via set_memory_region.
The kick mechanism introduced in this series is used to ensure running vcpus leave guest state to catch the update.
rerun_vcpu:
arch/s390/kvm/kvm-s390.c +1 -0
+    if (vcpu->requests)
arch/s390/kvm/kvm-s390.c int kvm_arch_vcpu_ioctl_run(struct kvm_v · line 495
Quoted 10 lines
arch/s390/kvm/kvm-s390.c +5 -1
+        if (test_and_clear_bit(KVM_REQ_MMU_RELOAD, &vcpu->requests))
+            kvm_s390_vcpu_set_mem(vcpu);
+
     /* verify, that memory has been registered */
-    if (!vcpu->kvm->arch.guest_memsize) {
+    if (!vcpu->arch.sie_block->gmslm) {
         vcpu_put(vcpu);
+        VCPU_EVENT(vcpu, 3, "%s", "no memory registered to run vcpu");
         return -EINVAL;
     }

Quoted 5 lines

x86 uses a double check: first we check vcpu->requests outside atomic context, then we enter the critical section and check again for signals and vcpu->requests.
This allows us (a) to do naughty things in vcpu->requests handlers, (b) keep the critical section short.
Does this apply here?

The patch already keeps the critical inner loop clear of extra code.
The check for vcpu->requests I added is only reached by either a heavyweight (userspace) exit/reentry or the explicit kickout of a vcpu to this label. Therefore weit fulfills a+b as you mentioned them above. Additionally the s390 reload is very rare as well as fast, therefore it would not even be an issue.

Quoted 18 lines

Quoted 7 lines

arch/s390/kvm/kvm-s390.c +1 -3

-    /* update sie control blocks, and unlock all vcpus */
+    /* request update of sie control block for all available vcpus */
     for (i = 0; i < KVM_MAX_VCPUS; ++i) {
         if (kvm->vcpus[i]) {
-            kvm->vcpus[i]->arch.sie_block->gmsor =
-                kvm->arch.guest_origin;

arch/s390/kvm/kvm-s390.c int kvm_arch_set_memory_region(struct kv · line 713

Quoted 10 lines

arch/s390/kvm/kvm-s390.c +3 -5

-            kvm->vcpus[i]->arch.sie_block->gmslm =
-                kvm->arch.guest_memsize +
-                kvm->arch.guest_origin +
-                VIRTIODESCSPACE - 1ul;
-            mutex_unlock(&kvm->vcpus[i]->mutex);
+            set_bit(KVM_REQ_MMU_RELOAD, &kvm->vcpus[i]->requests);
+            kvm_s390_inject_sigp_stop(kvm->vcpus[i],
+                          ACTION_RELOADVCPU_ON_STOP);
         }
     }

Quoted 1 line

There already exists a loop which does this, see make_all_cpus_request(). It uses an IPI (Marcelo, can't it use the reschedule interrupt?). It has a couple of optimizations -- if the request is already set, it skips the IPI, and it avoids the IPI for vcpus out of guest mode. Maybe it could fit s390 too.

I assume that the IPI on x86 is a implicit consequence of the smp_call_function_many function, but I think this doesn't work that way for us. The kick implied by that call would be recieved, but not reach the code the checke vcpu->request. I could add that behaviour, but that could make our normal interrupt handling much slower. Therefore I don't want to call that function, but on the other hand I like the "skip if the request is already set" functionality and think about adding that in my loop.

Signature · 2 lines

Grüsse / regards, Christian Ehrhardt
IBM Linux Technology Center, Open Virtualization

Christian Ehrhardt

author

2009-05-25 11:40 UTC #6 lore raw

Re: [PATCH 3/3] kvm-s390: streamline memslot handling

Quoted 23 lines · Christian Ehrhardt

Quoted 3 lines · Avi Kivity

Quoted 2 lines · [email protected] wrote:
From: Christian Ehrhardt <[email protected]>

[...]

Quoted 18 lines

Quoted 17 lines

arch/s390/kvm/kvm-s390.c +4 -8

-    /* update sie control blocks, and unlock all vcpus */
+    /* request update of sie control block for all available 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);
+            set_bit(KVM_REQ_MMU_RELOAD, &kvm->vcpus[i]->requests);
+            kvm_s390_inject_sigp_stop(kvm->vcpus[i],
+                          ACTION_RELOADVCPU_ON_STOP);
         }
     }

Quoted 3 lines

Quoted 1 line
There already exists a loop which does this, see make_all_cpus_request(). It uses an IPI (Marcelo, can't it use the reschedule interrupt?). It has a couple of optimizations -- if the request is already set, it skips the IPI, and it avoids the IPI for vcpus out of guest mode. Maybe it could fit s390 too.
I assume that the IPI on x86 is a implicit consequence of the smp_call_function_many function, but I think this doesn't work that way for us. The kick implied by that call would be recieved, but not reach the code the checke vcpu->request. I could add that behaviour, but that could make our normal interrupt handling much slower. Therefore I don't want to call that function, but on the other hand I like the "skip if the request is already set" functionality and think about adding that in my loop.

For now I added the optimization to skip kicking vcpus out of guest that had the request bit already set to the s390 specific loop (sent as v2 in a few minutes).

We might one day consider standardizing some generic kickout levels e.g. kick to "inner loop", "arch vcpu run", "generic vcpu run", "userspace", ... whatever levels fit *all* our use cases. And then let that kicks be implemented in an kvm_arch_* backend as it might be very different how they behave on different architectures. In case an architecture cannot achive reaching the specified kickout level it has to kick to the next available upper level which eventually will reach the desired step on the way to re-run the vcpu. Alltogether this should lead to a much more reliable and transparent interface that finally should be used all across the generic code.

Signature · 2 lines

Grüsse / regards, Christian Ehrhardt
IBM Linux Technology Center, Open Virtualization

Avi Kivity

2009-05-26 07:58 UTC #7 lore raw

Re: [PATCH 3/3] kvm-s390: streamline memslot handling

In reply to Christian Ehrhardt

Quoted 3 lines · Christian Ehrhardt

Quoted 2 lines
There already exists a loop which does this, see make_all_cpus_request(). It uses an IPI (Marcelo, can't it use the reschedule interrupt?). It has a couple of optimizations -- if the request is already set, it skips the IPI, and it avoids the IPI for vcpus out of guest mode. Maybe it could fit s390 too.
I assume that the IPI on x86 is a implicit consequence of the smp_call_function_many function,

Yes. It's only used to exit the guest, the IPI itself does nothing.

Quoted 1 line

but I think this doesn't work that way for us. The kick implied by that call would be recieved, but not reach the code the checke vcpu->request.

vcpu->requests is not checked by the IPI. Instead, it is checked just before entering guest mode, with interrupts disabled.

If the request is made before the check, no IPI is made, and the check finds the bit set.

If the request is made after the check, an IPI is made, and the guest exits immediately after entry.

Quoted 1 line

I could add that behaviour, but that could make our normal interrupt handling much slower. Therefore I don't want to call that function, but on the other hand I like the "skip if the request is already set" functionality and think about adding that in my loop.

I don't understand why it would affect your interrupt handling. We need someone that talks both x86 and s390 to break the language barrier...

I'll apply the patches, but please do look further into increasing commonality.

Signature · 1 line

error compiling committee.c: too many arguments to function

Christian Bornträger

2009-05-26 08:32 UTC #8 lore raw

Re: [PATCH 3/3] kvm-s390: streamline memslot handling

Am Dienstag 26 Mai 2009 09:57:58 schrieb Avi Kivity:

Quoted 6 lines

Quoted 4 lines
I could add that behaviour, but that could make our normal interrupt handling much slower. Therefore I don't want to call that function, but on the other hand I like the "skip if the request is already set" functionality and think about adding that in my loop.
I don't understand why it would affect your interrupt handling. We need

As far as I understand x86, every host interrupt causes a guest exit.

On s390 the SIE instruction is interruptible. On a host interrupt (like an IPI) the host interrupt handler runs and finally jumps back into the SIE instruction. The hardware will continue with guest execution. This has the advantage, that we dont have to load/save guest and host registers on host interrupts. (the low level interrupt handler saves the registers of the interrupted context)

In our low-level interrupt handler we do check for signal_pending, machine_check_pending and need_resched to leave the sie instruction. For anything else a the host sees a cpu bound guest always in the SIE instruction.

Christian

Avi Kivity

2009-05-26 09:27 UTC #9 lore raw

Re: [PATCH 3/3] kvm-s390: streamline memslot handling

Quoted 7 lines · Christian Bornträger

Am Dienstag 26 Mai 2009 09:57:58 schrieb Avi Kivity:
Quoted 5 lines
Quoted 5 lines
I could add that behaviour, but that could make our normal interrupt handling much slower. Therefore I don't want to call that function, but on the other hand I like the "skip if the request is already set" functionality and think about adding that in my loop.

Quoted 5 lines

Quoted 2 lines
I don't understand why it would affect your interrupt handling. We need
As far as I understand x86, every host interrupt causes a guest exit.

Yes.

Quoted 2 lines

On s390 the SIE instruction is interruptible. On a host interrupt (like an IPI) the host interrupt handler runs and finally jumps back into the SIE instruction. The hardware will continue with guest execution. This has the advantage, that we dont have to load/save guest and host registers on host interrupts. (the low level interrupt handler saves the registers of the interrupted context)

Neat stuff. Wish I had something like that.

Quoted 2 lines

In our low-level interrupt handler we do check for signal_pending, machine_check_pending and need_resched to leave the sie instruction. For anything else a the host sees a cpu bound guest always in the SIE instruction.

Okay, now I understand (and agree with) you multi-level kick thing. Maybe we could do it like so:

Interrupt handler (on s390 only) checks vcpu->requests, handles the ones it cans. If bits are still set, it exits to arch loop, which handles the bits it cans. If bits are still set, it exits to the generic code loop, which can finally exit to userspace.

Does this fit with s390 hardware?

Signature · 1 line

error compiling committee.c: too many arguments to function

Christian Ehrhardt

author

2009-05-26 10:31 UTC #10 lore raw

Re: [PATCH 3/3] kvm-s390: streamline memslot handling

Quoted 3 lines · Avi Kivity

Quoted 2 lines · Christian Bornträger
Am Dienstag 26 Mai 2009 09:57:58 schrieb Avi Kivity:

[...]

Quoted 8 lines

Quoted 1 line
In our low-level interrupt handler we do check for signal_pending, machine_check_pending and need_resched to leave the sie instruction. For anything else a the host sees a cpu bound guest always in the SIE instruction.
Okay, now I understand (and agree with) you multi-level kick thing. Maybe we could do it like so:
Interrupt handler (on s390 only) checks vcpu->requests, handles the ones it cans. If bits are still set, it exits to arch loop, which handles the bits it cans. If bits are still set, it exits to the generic code loop, which can finally exit to userspace.
Does this fit with s390 hardware?

I like this idea instead of explicitly kicking to an (upper) level to use the lowest kick and exit if not able to handle.
I think it should work (no guarantee) and I try to come up with something in the next few days - either a updated patch series or additional discussion input :-).

Signature · 2 lines

Grüsse / regards, Christian Ehrhardt
IBM Linux Technology Center, Open Virtualization