[PATCH 0/3] s390/kvm fixes

[PATCH 0/3] s390/kvm fixes

[Christian Borntraeger]

Gleb, Marcelo,

here are 3 kvm fixes for kvm-next.

Christian Borntraeger (3):
  s390/kvm: Fix store status for ACRS/FPRS
  s390/virtio-ccw: Fix setup_vq error handling.
  s390/kvm: Fix instruction decoding

 arch/s390/kvm/kvm-s390.c      |  8 ++++++++
 arch/s390/kvm/kvm-s390.h      | 25 ++++++++++++++-----------
 drivers/s390/kvm/virtio_ccw.c | 20 +++++++++++---------
 3 files changed, 33 insertions(+), 20 deletions(-)

-- 
1.7.12.4

[PATCH 1/3] s390/kvm: Fix store status for ACRS/FPRS

[Christian Borntraeger]

On store status we need to copy the current state of registers
into a save area. Currently we might save stale versions:
The sie state descriptor doesnt have fields for guest ACRS,FPRS,
those registers are simply stored in the host registers. The host
program must copy these away if needed. We do that in vcpu_put/load.

If we now do a store status in KVM code between vcpu_put/load, the
saved values are not up-to-date. Lets collect the ACRS/FPRS before
saving them.

This also fixes some strange problems with hotplug and virtio-ccw,
since the low level machine check handler (on hotplug a machine check
will happen) will revalidate all registers with the content of the
save area.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
CC: stable@vger.kernel.org
---
 arch/s390/kvm/kvm-s390.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 5b01f09..4377d18 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -770,6 +770,14 @@ int kvm_s390_vcpu_store_status(struct kvm_vcpu *vcpu, unsigned long addr)
 	} else
 		prefix = 0;
 
+	/*
+	 * The guest FPRS and ACRS are in the host FPRS/ACRS due to the lazy
+	 * copying in vcpu load/put. Lets update our copies before we save
+	 * it into the save area
+	 */
+	save_fp_regs(&vcpu->arch.guest_fpregs);
+	save_access_regs(vcpu->run->s.regs.acrs);
+
 	if (__guestcopy(vcpu, addr + offsetof(struct save_area, fp_regs),
 			vcpu->arch.guest_fpregs.fprs, 128, prefix))
 		return -EFAULT;
-- 
1.7.12.4

[PATCH 2/3] s390/virtio-ccw: Fix setup_vq error handling.

[Christian Borntraeger]

virtio_ccw_setup_vq() failed to unwind correctly on errors. In
particular, it failed to delete the virtqueue on errors, leading to
list corruption when virtio_ccw_del_vqs() iterated over a virtqueue
that had not been added to the vcdev's list.

Fix this with redoing the error unwinding in virtio_ccw_setup_vq(),
using a single path for all errors.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 drivers/s390/kvm/virtio_ccw.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index 2edd94a..3217dfe 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -244,9 +244,9 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev,
 {
 	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
 	int err;
-	struct virtqueue *vq;
+	struct virtqueue *vq = NULL;
 	struct virtio_ccw_vq_info *info;
-	unsigned long size;
+	unsigned long size = 0; /* silence the compiler */
 	unsigned long flags;
 
 	/* Allocate queue. */
@@ -279,11 +279,8 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev,
 		/* For now, we fail if we can't get the requested size. */
 		dev_warn(&vcdev->cdev->dev, "no vq\n");
 		err = -ENOMEM;
-		free_pages_exact(info->queue, size);
 		goto out_err;
 	}
-	info->vq = vq;
-	vq->priv = info;
 
 	/* Register it with the host. */
 	info->info_block->queue = (__u64)info->queue;
@@ -297,12 +294,12 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev,
 	err = ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_SET_VQ | i);
 	if (err) {
 		dev_warn(&vcdev->cdev->dev, "SET_VQ failed\n");
-		free_pages_exact(info->queue, size);
-		info->vq = NULL;
-		vq->priv = NULL;
 		goto out_err;
 	}
 
+	info->vq = vq;
+	vq->priv = info;
+
 	/* Save it to our list. */
 	spin_lock_irqsave(&vcdev->lock, flags);
 	list_add(&info->node, &vcdev->virtqueues);
@@ -311,8 +308,13 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev,
 	return vq;
 
 out_err:
-	if (info)
+	if (vq)
+		vring_del_virtqueue(vq);
+	if (info) {
+		if (info->queue)
+			free_pages_exact(info->queue, size);
 		kfree(info->info_block);
+	}
 	kfree(info);
 	return ERR_PTR(err);
 }
-- 
1.7.12.4

Re: [PATCH 2/3] s390/virtio-ccw: Fix setup_vq error handling.

[Christian Borntraeger]

On 25/01/13 15:34, Christian Borntraeger wrote:
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Sorry, I messed up the From.
Should I resend or can you change the author to "Cornelia Huck <cornelia.huck@de.ibm.com>"

Christian

[PATCH 3/3] s390/kvm: Fix instruction decoding

[Christian Borntraeger]

Instructions with long displacement have a signed displacement.
Currently the sign bit is interpreted as 2^20: Lets fix it by doing the
sign extension from 20bit to 32bit and then use it as a signed variable
in the addition (see kvm_s390_get_base_disp_rsy).

Furthermore, there are lots of "int" in that code. This is problematic,
because shifting on a signed integer is undefined/implementation defined
if the bit value happens to be negative.
Fortunately the promotion rules will make the right hand side unsigned
anyway, so there is no real problem right now.
Let's convert them anyway to unsigned where appropriate to avoid
problems if the code is changed or copy/pasted later on.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 arch/s390/kvm/kvm-s390.h | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
index 3e05def..4d89d64 100644
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -67,8 +67,8 @@ static inline void kvm_s390_set_prefix(struct kvm_vcpu *vcpu, u32 prefix)
 
 static inline u64 kvm_s390_get_base_disp_s(struct kvm_vcpu *vcpu)
 {
-	int base2 = vcpu->arch.sie_block->ipb >> 28;
-	int disp2 = ((vcpu->arch.sie_block->ipb & 0x0fff0000) >> 16);
+	u32 base2 = vcpu->arch.sie_block->ipb >> 28;
+	u32 disp2 = ((vcpu->arch.sie_block->ipb & 0x0fff0000) >> 16);
 
 	return (base2 ? vcpu->run->s.regs.gprs[base2] : 0) + disp2;
 }
@@ -76,10 +76,10 @@ static inline u64 kvm_s390_get_base_disp_s(struct kvm_vcpu *vcpu)
 static inline void kvm_s390_get_base_disp_sse(struct kvm_vcpu *vcpu,
 					      u64 *address1, u64 *address2)
 {
-	int base1 = (vcpu->arch.sie_block->ipb & 0xf0000000) >> 28;
-	int disp1 = (vcpu->arch.sie_block->ipb & 0x0fff0000) >> 16;
-	int base2 = (vcpu->arch.sie_block->ipb & 0xf000) >> 12;
-	int disp2 = vcpu->arch.sie_block->ipb & 0x0fff;
+	u32 base1 = (vcpu->arch.sie_block->ipb & 0xf0000000) >> 28;
+	u32 disp1 = (vcpu->arch.sie_block->ipb & 0x0fff0000) >> 16;
+	u32 base2 = (vcpu->arch.sie_block->ipb & 0xf000) >> 12;
+	u32 disp2 = vcpu->arch.sie_block->ipb & 0x0fff;
 
 	*address1 = (base1 ? vcpu->run->s.regs.gprs[base1] : 0) + disp1;
 	*address2 = (base2 ? vcpu->run->s.regs.gprs[base2] : 0) + disp2;
@@ -87,17 +87,20 @@ static inline void kvm_s390_get_base_disp_sse(struct kvm_vcpu *vcpu,
 
 static inline u64 kvm_s390_get_base_disp_rsy(struct kvm_vcpu *vcpu)
 {
-	int base2 = vcpu->arch.sie_block->ipb >> 28;
-	int disp2 = ((vcpu->arch.sie_block->ipb & 0x0fff0000) >> 16) +
+	u32 base2 = vcpu->arch.sie_block->ipb >> 28;
+	u32 disp2 = ((vcpu->arch.sie_block->ipb & 0x0fff0000) >> 16) +
 			((vcpu->arch.sie_block->ipb & 0xff00) << 4);
+	/* The displacement is a 20bit _SIGNED_ value */
+	if (disp2 & 0x80000)
+		disp2+=0xfff00000;
 
-	return (base2 ? vcpu->run->s.regs.gprs[base2] : 0) + disp2;
+	return (base2 ? vcpu->run->s.regs.gprs[base2] : 0) + (long)(int)disp2;
 }
 
 static inline u64 kvm_s390_get_base_disp_rs(struct kvm_vcpu *vcpu)
 {
-	int base2 = vcpu->arch.sie_block->ipb >> 28;
-	int disp2 = ((vcpu->arch.sie_block->ipb & 0x0fff0000) >> 16);
+	u32 base2 = vcpu->arch.sie_block->ipb >> 28;
+	u32 disp2 = ((vcpu->arch.sie_block->ipb & 0x0fff0000) >> 16);
 
 	return (base2 ? vcpu->run->s.regs.gprs[base2] : 0) + disp2;
 }
-- 
1.7.12.4

Re: [PATCH 0/3] s390/kvm fixes

[Gleb Natapov]

On Fri, Jan 25, 2013 at 03:34:14PM +0100, Christian Borntraeger wrote:
> Gleb, Marcelo,
> 
Alex, can you review?

> here are 3 kvm fixes for kvm-next.
> 
> Christian Borntraeger (3):
>   s390/kvm: Fix store status for ACRS/FPRS
>   s390/virtio-ccw: Fix setup_vq error handling.
>   s390/kvm: Fix instruction decoding
> 
>  arch/s390/kvm/kvm-s390.c      |  8 ++++++++
>  arch/s390/kvm/kvm-s390.h      | 25 ++++++++++++++-----------
>  drivers/s390/kvm/virtio_ccw.c | 20 +++++++++++---------
>  3 files changed, 33 insertions(+), 20 deletions(-)
> 
> -- 
> 1.7.12.4

--
			Gleb.

Re: [PATCH 0/3] s390/kvm fixes

[Christian Borntraeger]

On 29/01/13 14:12, Gleb Natapov wrote:
> On Fri, Jan 25, 2013 at 03:34:14PM +0100, Christian Borntraeger wrote:
>> Gleb, Marcelo,
>>
> Alex, can you review?

Last time I checked, Cornelia and myself were s390/kvm/kernel maintainers ;-)
Alex is maintainer for s390/kvm/qemu. 
If the patch introduces or touched an kernel<->qemu interface, I usually try
to come to a conclusion with Alex before submission, but these patches are
only bugfixes and Alex is probably pretty busy working the last day before
qemus hard freeze for 1.4.

Christian

Re: [PATCH 0/3] s390/kvm fixes

[Gleb Natapov]

On Tue, Jan 29, 2013 at 09:41:21PM +0100, Christian Borntraeger wrote:
> On 29/01/13 14:12, Gleb Natapov wrote:
> > On Fri, Jan 25, 2013 at 03:34:14PM +0100, Christian Borntraeger wrote:
> >> Gleb, Marcelo,
> >>
> > Alex, can you review?
> 
> Last time I checked, Cornelia and myself were s390/kvm/kernel maintainers ;-)
Sorry, by bad :)

> Alex is maintainer for s390/kvm/qemu. 
> If the patch introduces or touched an kernel<->qemu interface, I usually try
> to come to a conclusion with Alex before submission, but these patches are
> only bugfixes and Alex is probably pretty busy working the last day before
> qemus hard freeze for 1.4.
> 
Got it.

The question about 1/1. It is CCed to stable, does this mean you want it
to go to 3.8? kvm-next is for 3.9.

--
			Gleb.

Re: [PATCH 0/3] s390/kvm fixes

[Gleb Natapov]

On Tue, Jan 29, 2013 at 10:46:00PM +0200, Gleb Natapov wrote:
> On Tue, Jan 29, 2013 at 09:41:21PM +0100, Christian Borntraeger wrote:
> > On 29/01/13 14:12, Gleb Natapov wrote:
> > > On Fri, Jan 25, 2013 at 03:34:14PM +0100, Christian Borntraeger wrote:
> > >> Gleb, Marcelo,
> > >>
> > > Alex, can you review?
> > 
> > Last time I checked, Cornelia and myself were s390/kvm/kernel maintainers ;-)
> Sorry, by bad :)
> 
> > Alex is maintainer for s390/kvm/qemu. 
> > If the patch introduces or touched an kernel<->qemu interface, I usually try
> > to come to a conclusion with Alex before submission, but these patches are
> > only bugfixes and Alex is probably pretty busy working the last day before
> > qemus hard freeze for 1.4.
> > 
> Got it.
> 
> The question about 1/1. It is CCed to stable, does this mean you want it
> to go to 3.8? kvm-next is for 3.9.
> 
On the second thought, if it is not a regression 3.9 is the right place.

--
			Gleb.

Re: [PATCH 0/3] s390/kvm fixes

[Christian Borntraeger]

On 29/01/13 22:03, Gleb Natapov wrote:

>> The question about 1/1. It is CCed to stable, does this mean you want it
>> to go to 3.8? kvm-next is for 3.9.
>>
> On the second thought, if it is not a regression 3.9 is the right place.

The store status part is broken, but it only has a severe impact in case of 
a machine check. (The machine check handler revalidates all registers with
the content of the save area).
Since machine checks are part of the virtio-ccw code, this can go into 3.9.
Feel free to remove the CC:stable. 

Christian

Re: [PATCH 0/3] s390/kvm fixes

[Gleb Natapov]

On Wed, Jan 30, 2013 at 09:51:24AM +0100, Christian Borntraeger wrote:
> On 29/01/13 22:03, Gleb Natapov wrote:
> 
> >> The question about 1/1. It is CCed to stable, does this mean you want it
> >> to go to 3.8? kvm-next is for 3.9.
> >>
> > On the second thought, if it is not a regression 3.9 is the right place.
> 
> The store status part is broken, but it only has a severe impact in case of 
> a machine check. (The machine check handler revalidates all registers with
> the content of the save area).
> Since machine checks are part of the virtio-ccw code, this can go into 3.9.
> Feel free to remove the CC:stable. 
> 
No reason to drop stable, but 3.8 will have to get the fix through
stable to after it is released.

--
			Gleb.

Re: [PATCH 0/3] s390/kvm fixes

[Gleb Natapov]

On Fri, Jan 25, 2013 at 03:34:14PM +0100, Christian Borntraeger wrote:
> Gleb, Marcelo,
> 
> here are 3 kvm fixes for kvm-next.
> 
> Christian Borntraeger (3):
>   s390/kvm: Fix store status for ACRS/FPRS
>   s390/virtio-ccw: Fix setup_vq error handling.
>   s390/kvm: Fix instruction decoding
> 
>  arch/s390/kvm/kvm-s390.c      |  8 ++++++++
>  arch/s390/kvm/kvm-s390.h      | 25 ++++++++++++++-----------
>  drivers/s390/kvm/virtio_ccw.c | 20 +++++++++++---------
>  3 files changed, 33 insertions(+), 20 deletions(-)
> 
Applied, thanks.

--
			Gleb.