[PATCH 0/6] Fixes for kvm on s390

[PATCH 0/6] Fixes for kvm on s390

[Christian Borntraeger]

Avi, Marcelo,

here are some fixes for kvm on s390.
Patch 1 (Sanitize fpc registers for KVM_SET_FPU) fixes a kernel bug that
can be triggered by a user, this should go into 3.3.
Patch 2 (do store status after handling STOP_ON_STOP bit) reorders some
stop actions but also fixes a scheduling while atomic problem. ->  3.3?
Patches 3-5 are bugs but can wait -> next
Patches 6 extends to sync register feature to control registers. -> next

Christian

Christian Borntraeger (2):
  kvm-s390: Sanitize fpc registers for KVM_SET_FPU
  kvm-s390: provide control registers via kvm_run

Jens Freimann (4):
  kvm-s390: do store status after handling STOP_ON_STOP bit
  kvm-s390: make sigp restart return busy when stop pending
  kvm-s390: ignore sigp stop overinitiative
  kvm-s390: add stop_on_stop flag when doing stop and store

 arch/s390/include/asm/kvm.h |    2 ++
 arch/s390/kvm/intercept.c   |   20 ++++++++++++--------
 arch/s390/kvm/kvm-s390.c    |   11 +++++++++--
 arch/s390/kvm/sigp.c        |   25 ++++++++++++++++++++++++-
 4 files changed, 47 insertions(+), 11 deletions(-)

-- 
1.7.9

[PATCH 1/6] kvm-s390: Sanitize fpc registers for KVM_SET_FPU

[Christian Borntraeger]

From: Christian Borntraeger <borntraeger@de.ibm.com>

commit 7eef87dc99e419b1cc051e4417c37e4744d7b661 (KVM: s390: fix
register setting) added a load of the floating point control register
to the KVM_SET_FPU path. Lets make sure that the fpc is valid.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/kvm/kvm-s390.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 0b91679..121316e 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -460,7 +460,7 @@ int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu,
 int kvm_arch_vcpu_ioctl_set_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu)
 {
 	memcpy(&vcpu->arch.guest_fpregs.fprs, &fpu->fprs, sizeof(fpu->fprs));
-	vcpu->arch.guest_fpregs.fpc = fpu->fpc;
+	vcpu->arch.guest_fpregs.fpc = fpu->fpc & FPC_VALID_MASK;
 	restore_fp_regs(&vcpu->arch.guest_fpregs);
 	return 0;
 }
-- 
1.7.9

[PATCH 2/6] kvm-s390: do store status after handling STOP_ON_STOP bit

[Christian Borntraeger]

From: Jens Freimann <jfrei@linux.vnet.ibm.com>

In handle_stop() handle the stop bit before doing the store status as
described for "Stop and Store Status" in the Principles of Operation.
We have to give up the local_int.lock before calling kvm store status
since it calls gmap_fault() which might sleep. Since local_int.lock
only protects local_int.* and not guest memory we can give up the lock.

Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/kvm/intercept.c |   20 ++++++++++++--------
 1 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
index 776ef83..3614565 100644
--- a/arch/s390/kvm/intercept.c
+++ b/arch/s390/kvm/intercept.c
@@ -133,13 +133,6 @@ static int handle_stop(struct kvm_vcpu *vcpu)
 
 	vcpu->stat.exit_stop_request++;
 	spin_lock_bh(&vcpu->arch.local_int.lock);
-	if (vcpu->arch.local_int.action_bits & ACTION_STORE_ON_STOP) {
-		vcpu->arch.local_int.action_bits &= ~ACTION_STORE_ON_STOP;
-		rc = kvm_s390_vcpu_store_status(vcpu,
-						  KVM_S390_STORE_STATUS_NOADDR);
-		if (rc >= 0)
-			rc = -EOPNOTSUPP;
-	}
 
 	if (vcpu->arch.local_int.action_bits & ACTION_RELOADVCPU_ON_STOP) {
 		vcpu->arch.local_int.action_bits &= ~ACTION_RELOADVCPU_ON_STOP;
@@ -155,7 +148,18 @@ static int handle_stop(struct kvm_vcpu *vcpu)
 		rc = -EOPNOTSUPP;
 	}
 
-	spin_unlock_bh(&vcpu->arch.local_int.lock);
+	if (vcpu->arch.local_int.action_bits & ACTION_STORE_ON_STOP) {
+		vcpu->arch.local_int.action_bits &= ~ACTION_STORE_ON_STOP;
+		/* store status must be called unlocked. Since local_int.lock
+		 * only protects local_int.* and not guest memory we can give
+		 * up the lock here */
+		spin_unlock_bh(&vcpu->arch.local_int.lock);
+		rc = kvm_s390_vcpu_store_status(vcpu,
+						KVM_S390_STORE_STATUS_NOADDR);
+		if (rc >= 0)
+			rc = -EOPNOTSUPP;
+	} else
+		spin_unlock_bh(&vcpu->arch.local_int.lock);
 	return rc;
 }
 
-- 
1.7.9

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

[Christian Borntraeger]

From: Jens Freimann <jfrei@linux.vnet.ibm.com>

On reboot the guest sends in smp_send_stop() a sigp stop to all CPUs
except for current CPU.  Then the guest switches to the IPL cpu by
sending a restart to the IPL CPU, followed by a sigp stop to the
current cpu. Since restart is handled by userspace it's possible that
the restart is delivered before the old stop.  This means that the IPL
CPU isn't restarted and we have no running CPUs. So let's make sure
that there is no stop action pending when we do the restart.

Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/kvm/sigp.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c
index 30eb0f7..336905c 100644
--- a/arch/s390/kvm/sigp.c
+++ b/arch/s390/kvm/sigp.c
@@ -309,6 +309,22 @@ static int __sigp_sense_running(struct kvm_vcpu *vcpu, u16 cpu_addr,
 	return rc;
 }
 
+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;
+}
+
 int kvm_s390_handle_sigp(struct kvm_vcpu *vcpu)
 {
 	int r1 = (vcpu->arch.sie_block->ipa & 0x00f0) >> 4;
@@ -372,6 +388,9 @@ int kvm_s390_handle_sigp(struct kvm_vcpu *vcpu)
 		break;
 	case SIGP_RESTART:
 		vcpu->stat.instruction_sigp_restart++;
+		rc = __sigp_restart(vcpu, cpu_addr);
+		if (rc == 2) /* busy */
+			break;
 		/* user space must know about restart */
 	default:
 		return -EOPNOTSUPP;
-- 
1.7.9

[PATCH 4/6] kvm-s390: ignore sigp stop overinitiative

[Christian Borntraeger]

From: Jens Freimann <jfrei@linux.vnet.ibm.com>

In __inject_sigp_stop() do nothing when the CPU is already in stopped
state.

Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/kvm/sigp.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c
index 336905c..aa7845a 100644
--- a/arch/s390/kvm/sigp.c
+++ b/arch/s390/kvm/sigp.c
@@ -160,12 +160,15 @@ static int __inject_sigp_stop(struct kvm_s390_local_interrupt *li, int action)
 	inti->type = KVM_S390_SIGP_STOP;
 
 	spin_lock_bh(&li->lock);
+	if ((atomic_read(li->cpuflags) & CPUSTAT_STOPPED))
+		goto out;
 	list_add_tail(&inti->list, &li->list);
 	atomic_set(&li->active, 1);
 	atomic_set_mask(CPUSTAT_STOP_INT, li->cpuflags);
 	li->action_bits |= action;
 	if (waitqueue_active(&li->wq))
 		wake_up_interruptible(&li->wq);
+out:
 	spin_unlock_bh(&li->lock);
 
 	return 0; /* order accepted */
-- 
1.7.9

[PATCH 5/6] kvm-s390: add stop_on_stop flag when doing stop and store

[Christian Borntraeger]

From: Jens Freimann <jfrei@linux.vnet.ibm.com>

When we do a stop and store status we need to pass ACTION_STOP_ON_STOP
flag to __sigp_stop().

Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/kvm/sigp.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c
index aa7845a..052c836 100644
--- a/arch/s390/kvm/sigp.c
+++ b/arch/s390/kvm/sigp.c
@@ -373,7 +373,8 @@ int kvm_s390_handle_sigp(struct kvm_vcpu *vcpu)
 		break;
 	case SIGP_STOP_STORE_STATUS:
 		vcpu->stat.instruction_sigp_stop++;
-		rc = __sigp_stop(vcpu, cpu_addr, ACTION_STORE_ON_STOP);
+		rc = __sigp_stop(vcpu, cpu_addr, ACTION_STORE_ON_STOP |
+						 ACTION_STOP_ON_STOP);
 		break;
 	case SIGP_SET_ARCH:
 		vcpu->stat.instruction_sigp_arch++;
-- 
1.7.9

[PATCH 6/6] kvm-s390: provide control registers via kvm_run

[Christian Borntraeger]

There are several cases were we need the control registers for
userspace. Lets also provide those in kvm_run.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/include/asm/kvm.h |    2 ++
 arch/s390/kvm/kvm-s390.c    |    9 ++++++++-
 2 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/arch/s390/include/asm/kvm.h b/arch/s390/include/asm/kvm.h
index 9acbde4..9607667 100644
--- a/arch/s390/include/asm/kvm.h
+++ b/arch/s390/include/asm/kvm.h
@@ -44,10 +44,12 @@ struct kvm_guest_debug_arch {
 #define KVM_SYNC_PREFIX (1UL << 0)
 #define KVM_SYNC_GPRS   (1UL << 1)
 #define KVM_SYNC_ACRS   (1UL << 2)
+#define KVM_SYNC_CRS    (1UL << 3)
 /* definition of registers in kvm_run */
 struct kvm_sync_regs {
 	__u64 prefix;	/* prefix register */
 	__u64 gprs[16];	/* general purpose registers */
 	__u32 acrs[16];	/* access registers */
+	__u64 crs[16];	/* control registers */
 };
 #endif
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 121316e..cf3c0a9 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -291,7 +291,8 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
 	vcpu->arch.gmap = vcpu->kvm->arch.gmap;
 	vcpu->run->kvm_valid_regs = KVM_SYNC_PREFIX |
 				    KVM_SYNC_GPRS |
-				    KVM_SYNC_ACRS;
+				    KVM_SYNC_ACRS |
+				    KVM_SYNC_CRS;
 	return 0;
 }
 
@@ -580,6 +581,11 @@ rerun_vcpu:
 		kvm_run->kvm_dirty_regs &= ~KVM_SYNC_PREFIX;
 		kvm_s390_set_prefix(vcpu, kvm_run->s.regs.prefix);
 	}
+	if (kvm_run->kvm_dirty_regs & KVM_SYNC_CRS) {
+		kvm_run->kvm_dirty_regs &= ~KVM_SYNC_CRS;
+		memcpy(&vcpu->arch.sie_block->gcr, &kvm_run->s.regs.crs, 128);
+		kvm_s390_set_prefix(vcpu, kvm_run->s.regs.prefix);
+	}
 
 	might_fault();
 
@@ -629,6 +635,7 @@ rerun_vcpu:
 	kvm_run->psw_mask     = vcpu->arch.sie_block->gpsw.mask;
 	kvm_run->psw_addr     = vcpu->arch.sie_block->gpsw.addr;
 	kvm_run->s.regs.prefix = vcpu->arch.sie_block->prefix;
+	memcpy(&kvm_run->s.regs.crs, &vcpu->arch.sie_block->gcr, 128);
 
 	if (vcpu->sigset_active)
 		sigprocmask(SIG_SETMASK, &sigsaved, NULL);
-- 
1.7.9

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

[Carsten Otte]

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.

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

[Christian Borntraeger]

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

Fixed restart patch

[Christian Borntraeger]

Avi, Marcelo,

here is an updated version of patch 3 taking care of Carstens finding.
It also handles invalid target cpus more gracefully. Tested and
lockdep-approved.

Jens Freimann (1):
  kvm-s390: make sigp restart return busy when stop pending

 arch/s390/kvm/sigp.c |   31 +++++++++++++++++++++++++++++++
 1 files changed, 31 insertions(+), 0 deletions(-)

-- 
1.7.9

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

[Christian Borntraeger]

From: Jens Freimann <jfrei@linux.vnet.ibm.com>

On reboot the guest sends in smp_send_stop() a sigp stop to all CPUs
except for current CPU.  Then the guest switches to the IPL cpu by
sending a restart to the IPL CPU, followed by a sigp stop to the
current cpu. Since restart is handled by userspace it's possible that
the restart is delivered before the old stop.  This means that the IPL
CPU isn't restarted and we have no running CPUs. So let's make sure
that there is no stop action pending when we do the restart.

Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/kvm/sigp.c |   31 +++++++++++++++++++++++++++++++
 1 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c
index 30eb0f7..c703b1c 100644
--- a/arch/s390/kvm/sigp.c
+++ b/arch/s390/kvm/sigp.c
@@ -309,6 +309,34 @@ static int __sigp_sense_running(struct kvm_vcpu *vcpu, u16 cpu_addr,
 	return rc;
 }
 
+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);
+	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;
+}
+
 int kvm_s390_handle_sigp(struct kvm_vcpu *vcpu)
 {
 	int r1 = (vcpu->arch.sie_block->ipa & 0x00f0) >> 4;
@@ -372,6 +400,9 @@ int kvm_s390_handle_sigp(struct kvm_vcpu *vcpu)
 		break;
 	case SIGP_RESTART:
 		vcpu->stat.instruction_sigp_restart++;
+		rc = __sigp_restart(vcpu, cpu_addr);
+		if (rc == 2) /* busy */
+			break;
 		/* user space must know about restart */
 	default:
 		return -EOPNOTSUPP;
-- 
1.7.9

Re: [PATCH 0/6] Fixes for kvm on s390

[Marcelo Tosatti]

On Mon, Feb 06, 2012 at 10:59:01AM +0100, Christian Borntraeger wrote:
> Avi, Marcelo,
> 
> here are some fixes for kvm on s390.
> Patch 1 (Sanitize fpc registers for KVM_SET_FPU) fixes a kernel bug that
> can be triggered by a user, this should go into 3.3.
> Patch 2 (do store status after handling STOP_ON_STOP bit) reorders some
> stop actions but also fixes a scheduling while atomic problem. ->  3.3?
> Patches 3-5 are bugs but can wait -> next
> Patches 6 extends to sync register feature to control registers. -> next
> 
> Christian
> 
> Christian Borntraeger (2):
>   kvm-s390: Sanitize fpc registers for KVM_SET_FPU
>   kvm-s390: provide control registers via kvm_run
> 
> Jens Freimann (4):
>   kvm-s390: do store status after handling STOP_ON_STOP bit
>   kvm-s390: make sigp restart return busy when stop pending
>   kvm-s390: ignore sigp stop overinitiative
>   kvm-s390: add stop_on_stop flag when doing stop and store

Applied, thanks.