[PATCH 0/5] preview of first chunk of s390 patches for kvm/next

[PATCH 0/5] preview of first chunk of s390 patches for kvm/next

[Christian Borntraeger]

Just to get some reviewable patches out before the holidays.

----------------------------------------------------------------
KVM: s390: Fixes and features for 4.16

- add the virtio-ccw transport for kvmconfig
- more debug tracing for cpu model
- cleanups and fixes

----------------------------------------------------------------
Christian Borntraeger (3):
      KVM: s390: use created_vcpus in more places
      KVM: s390: add debug tracing for cpu features of CPU model
      kvm_config: add CONFIG_S390_GUEST

David Hildenbrand (1):
      s390x/mm: cleanup gmap_pte_op_walk()

Michael Mueller (1):
      KVM: s390: drop use of spin lock in __floating_irq_kick

 arch/s390/kvm/interrupt.c       |  2 --
 arch/s390/kvm/kvm-s390.c        | 26 +++++++++++++++++++-------
 arch/s390/mm/gmap.c             | 23 ++++++++---------------
 kernel/configs/kvm_guest.config |  1 +
 4 files changed, 28 insertions(+), 24 deletions(-)

[PATCH 1/5] s390x/mm: cleanup gmap_pte_op_walk()

[Christian Borntraeger]

From: David Hildenbrand <david@redhat.com>

gmap_mprotect_notify() refuses shadow gmaps. Turns out that
a) gmap_protect_range()
b) gmap_read_table()
c) gmap_pte_op_walk()

Are never called for gmap shadows. And never should be. This dates back
to gmap shadow prototypes where we allowed to call mprotect_notify() on
the gmap shadow (to get notified about the prefix pages getting removed).
This is avoided by always getting notified about any change on the gmap
shadow.

The only real function for walking page tables on shadow gmaps is
gmap_table_walk().

So, essentially, these functions should never get called and
gmap_pte_op_walk() can be cleaned up. Add some checks to callers of
gmap_pte_op_walk().

Signed-off-by: David Hildenbrand <david@redhat.com>
Message-Id: <20171110151805.7541-1-david@redhat.com>
Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/mm/gmap.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
index 05d459b638f5..54cfd51a5a27 100644
--- a/arch/s390/mm/gmap.c
+++ b/arch/s390/mm/gmap.c
@@ -815,27 +815,17 @@ static inline unsigned long *gmap_table_walk(struct gmap *gmap,
  * @ptl: pointer to the spinlock pointer
  *
  * Returns a pointer to the locked pte for a guest address, or NULL
- *
- * Note: Can also be called for shadow gmaps.
  */
 static pte_t *gmap_pte_op_walk(struct gmap *gmap, unsigned long gaddr,
 			       spinlock_t **ptl)
 {
 	unsigned long *table;
 
-	if (gmap_is_shadow(gmap))
-		spin_lock(&gmap->guest_table_lock);
+	BUG_ON(gmap_is_shadow(gmap));
 	/* Walk the gmap page table, lock and get pte pointer */
 	table = gmap_table_walk(gmap, gaddr, 1); /* get segment pointer */
-	if (!table || *table & _SEGMENT_ENTRY_INVALID) {
-		if (gmap_is_shadow(gmap))
-			spin_unlock(&gmap->guest_table_lock);
+	if (!table || *table & _SEGMENT_ENTRY_INVALID)
 		return NULL;
-	}
-	if (gmap_is_shadow(gmap)) {
-		*ptl = &gmap->guest_table_lock;
-		return pte_offset_map((pmd_t *) table, gaddr);
-	}
 	return pte_alloc_map_lock(gmap->mm, (pmd_t *) table, gaddr, ptl);
 }
 
@@ -889,8 +879,6 @@ static void gmap_pte_op_end(spinlock_t *ptl)
  * -EFAULT if gaddr is invalid (or mapping for shadows is missing).
  *
  * Called with sg->mm->mmap_sem in read.
- *
- * Note: Can also be called for shadow gmaps.
  */
 static int gmap_protect_range(struct gmap *gmap, unsigned long gaddr,
 			      unsigned long len, int prot, unsigned long bits)
@@ -900,6 +888,7 @@ static int gmap_protect_range(struct gmap *gmap, unsigned long gaddr,
 	pte_t *ptep;
 	int rc;
 
+	BUG_ON(gmap_is_shadow(gmap));
 	while (len) {
 		rc = -EAGAIN;
 		ptep = gmap_pte_op_walk(gmap, gaddr, &ptl);
@@ -960,7 +949,8 @@ EXPORT_SYMBOL_GPL(gmap_mprotect_notify);
  * @val: pointer to the unsigned long value to return
  *
  * Returns 0 if the value was read, -ENOMEM if out of memory and -EFAULT
- * if reading using the virtual address failed.
+ * if reading using the virtual address failed. -EINVAL if called on a gmap
+ * shadow.
  *
  * Called with gmap->mm->mmap_sem in read.
  */
@@ -971,6 +961,9 @@ int gmap_read_table(struct gmap *gmap, unsigned long gaddr, unsigned long *val)
 	pte_t *ptep, pte;
 	int rc;
 
+	if (gmap_is_shadow(gmap))
+		return -EINVAL;
+
 	while (1) {
 		rc = -EAGAIN;
 		ptep = gmap_pte_op_walk(gmap, gaddr, &ptl);
-- 
2.13.4

[PATCH 2/5] KVM: s390: use created_vcpus in more places

[Christian Borntraeger]

commit a03825bbd0c3 ("KVM: s390: use kvm->created_vcpus") introduced
kvm->created_vcpus to avoid races with the existing kvm->online_vcpus
scheme. One place was "forgotten" and one new place was "added".
Let's fix those.

Reported-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Fixes: 4e0b1ab72b8a ("KVM: s390: gs support for kvm guests")
Fixes: a03825bbd0c3 ("KVM: s390: use kvm->created_vcpus")
---
 arch/s390/kvm/kvm-s390.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 40f0ae5a883f..00ef6f47e466 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -573,7 +573,7 @@ static int kvm_vm_ioctl_enable_cap(struct kvm *kvm, struct kvm_enable_cap *cap)
 	case KVM_CAP_S390_GS:
 		r = -EINVAL;
 		mutex_lock(&kvm->lock);
-		if (atomic_read(&kvm->online_vcpus)) {
+		if (kvm->created_vcpus) {
 			r = -EBUSY;
 		} else if (test_facility(133)) {
 			set_kvm_facility(kvm->arch.model.fac_mask, 133);
@@ -1094,7 +1094,7 @@ static int kvm_s390_set_processor_feat(struct kvm *kvm,
 		return -EINVAL;
 
 	mutex_lock(&kvm->lock);
-	if (!atomic_read(&kvm->online_vcpus)) {
+	if (!kvm->created_vcpus) {
 		bitmap_copy(kvm->arch.cpu_feat, (unsigned long *) data.feat,
 			    KVM_S390_VM_CPU_FEAT_NR_BITS);
 		ret = 0;
-- 
2.13.4

[PATCH 3/5] KVM: s390: add debug tracing for cpu features of CPU model

[Christian Borntraeger]

The cpu model already traces the cpu facilities, the ibc and
guest CPU ids. We should do the same for the cpu features (on
success only).

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 arch/s390/kvm/kvm-s390.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 00ef6f47e466..c85d7c4e8a38 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -1084,7 +1084,6 @@ static int kvm_s390_set_processor_feat(struct kvm *kvm,
 				       struct kvm_device_attr *attr)
 {
 	struct kvm_s390_vm_cpu_feat data;
-	int ret = -EBUSY;
 
 	if (copy_from_user(&data, (void __user *)attr->addr, sizeof(data)))
 		return -EFAULT;
@@ -1094,13 +1093,18 @@ static int kvm_s390_set_processor_feat(struct kvm *kvm,
 		return -EINVAL;
 
 	mutex_lock(&kvm->lock);
-	if (!kvm->created_vcpus) {
-		bitmap_copy(kvm->arch.cpu_feat, (unsigned long *) data.feat,
-			    KVM_S390_VM_CPU_FEAT_NR_BITS);
-		ret = 0;
+	if (kvm->created_vcpus) {
+		mutex_unlock(&kvm->lock);
+		return -EBUSY;
 	}
+	bitmap_copy(kvm->arch.cpu_feat, (unsigned long *) data.feat,
+		    KVM_S390_VM_CPU_FEAT_NR_BITS);
 	mutex_unlock(&kvm->lock);
-	return ret;
+	VM_EVENT(kvm, 3, "SET: guest feat: 0x%16.16llx.0x%16.16llx.0x%16.16llx",
+			 data.feat[0],
+			 data.feat[1],
+			 data.feat[2]);
+	return 0;
 }
 
 static int kvm_s390_set_processor_subfunc(struct kvm *kvm,
@@ -1202,6 +1206,10 @@ static int kvm_s390_get_processor_feat(struct kvm *kvm,
 		    KVM_S390_VM_CPU_FEAT_NR_BITS);
 	if (copy_to_user((void __user *)attr->addr, &data, sizeof(data)))
 		return -EFAULT;
+	VM_EVENT(kvm, 3, "GET: guest feat: 0x%16.16llx.0x%16.16llx.0x%16.16llx",
+			 data.feat[0],
+			 data.feat[1],
+			 data.feat[2]);
 	return 0;
 }
 
@@ -1215,6 +1223,10 @@ static int kvm_s390_get_machine_feat(struct kvm *kvm,
 		    KVM_S390_VM_CPU_FEAT_NR_BITS);
 	if (copy_to_user((void __user *)attr->addr, &data, sizeof(data)))
 		return -EFAULT;
+	VM_EVENT(kvm, 3, "GET: host feat:  0x%16.16llx.0x%16.16llx.0x%16.16llx",
+			 data.feat[0],
+			 data.feat[1],
+			 data.feat[2]);
 	return 0;
 }
 
-- 
2.13.4

[PATCH 4/5] KVM: s390: drop use of spin lock in __floating_irq_kick

[Christian Borntraeger]

From: Michael Mueller <mimu@linux.vnet.ibm.com>

It is not required to take to a lock to protect access to the cpuflags
of the local interrupt structure of a vcpu as the performed operation
is an atomic_or.

Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/kvm/interrupt.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 024ad8bcc516..818aa4248b0f 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -1569,7 +1569,6 @@ static void __floating_irq_kick(struct kvm *kvm, u64 type)
 
 	/* make the VCPU drop out of the SIE, or wake it up if sleeping */
 	li = &dst_vcpu->arch.local_int;
-	spin_lock(&li->lock);
 	switch (type) {
 	case KVM_S390_MCHK:
 		atomic_or(CPUSTAT_STOP_INT, li->cpuflags);
@@ -1581,7 +1580,6 @@ static void __floating_irq_kick(struct kvm *kvm, u64 type)
 		atomic_or(CPUSTAT_EXT_INT, li->cpuflags);
 		break;
 	}
-	spin_unlock(&li->lock);
 	kvm_s390_vcpu_wakeup(dst_vcpu);
 }
 
-- 
2.13.4

[PATCH 5/5] kvm_config: add CONFIG_S390_GUEST

[Christian Borntraeger]

make kvmconfig currently does not select CONFIG_S390_GUEST. Since
the virtio-ccw transport depends on CONFIG_S390_GUEST, we want
to add CONFIG_S390_GUEST to kvmconfig.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 kernel/configs/kvm_guest.config | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/configs/kvm_guest.config b/kernel/configs/kvm_guest.config
index 8d9643767142..108fecc20fc1 100644
--- a/kernel/configs/kvm_guest.config
+++ b/kernel/configs/kvm_guest.config
@@ -18,6 +18,7 @@ CONFIG_VIRTUALIZATION=y
 CONFIG_HYPERVISOR_GUEST=y
 CONFIG_PARAVIRT=y
 CONFIG_KVM_GUEST=y
+CONFIG_S390_GUEST=y
 CONFIG_VIRTIO=y
 CONFIG_VIRTIO_PCI=y
 CONFIG_VIRTIO_BLK=y
-- 
2.13.4

Re: [PATCH 1/5] s390x/mm: cleanup gmap_pte_op_walk()

[Cornelia Huck]

On Wed, 20 Dec 2017 16:52:56 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> From: David Hildenbrand <david@redhat.com>
> 
> gmap_mprotect_notify() refuses shadow gmaps. Turns out that
> a) gmap_protect_range()
> b) gmap_read_table()
> c) gmap_pte_op_walk()
> 
> Are never called for gmap shadows. And never should be. This dates back
> to gmap shadow prototypes where we allowed to call mprotect_notify() on
> the gmap shadow (to get notified about the prefix pages getting removed).
> This is avoided by always getting notified about any change on the gmap
> shadow.
> 
> The only real function for walking page tables on shadow gmaps is
> gmap_table_walk().
> 
> So, essentially, these functions should never get called and
> gmap_pte_op_walk() can be cleaned up. Add some checks to callers of
> gmap_pte_op_walk().
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> Message-Id: <20171110151805.7541-1-david@redhat.com>
> Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/mm/gmap.c | 23 ++++++++---------------
>  1 file changed, 8 insertions(+), 15 deletions(-)

Acked-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH 2/5] KVM: s390: use created_vcpus in more places

[Cornelia Huck]

On Wed, 20 Dec 2017 16:52:57 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> commit a03825bbd0c3 ("KVM: s390: use kvm->created_vcpus") introduced
> kvm->created_vcpus to avoid races with the existing kvm->online_vcpus
> scheme. One place was "forgotten" and one new place was "added".
> Let's fix those.
> 
> Reported-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Fixes: 4e0b1ab72b8a ("KVM: s390: gs support for kvm guests")
> Fixes: a03825bbd0c3 ("KVM: s390: use kvm->created_vcpus")
> ---
>  arch/s390/kvm/kvm-s390.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH 3/5] KVM: s390: add debug tracing for cpu features of CPU model

[Cornelia Huck]

On Wed, 20 Dec 2017 16:52:58 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> The cpu model already traces the cpu facilities, the ibc and
> guest CPU ids. We should do the same for the cpu features (on
> success only).
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  arch/s390/kvm/kvm-s390.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH 4/5] KVM: s390: drop use of spin lock in __floating_irq_kick

[Cornelia Huck]

On Wed, 20 Dec 2017 16:52:59 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> From: Michael Mueller <mimu@linux.vnet.ibm.com>
> 
> It is not required to take to a lock to protect access to the cpuflags
> of the local interrupt structure of a vcpu as the performed operation
> is an atomic_or.
> 
> Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/kvm/interrupt.c | 2 --
>  1 file changed, 2 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH 5/5] kvm_config: add CONFIG_S390_GUEST

[Cornelia Huck]

On Wed, 20 Dec 2017 16:53:00 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> make kvmconfig currently does not select CONFIG_S390_GUEST. Since
> the virtio-ccw transport depends on CONFIG_S390_GUEST, we want
> to add CONFIG_S390_GUEST to kvmconfig.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  kernel/configs/kvm_guest.config | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/configs/kvm_guest.config b/kernel/configs/kvm_guest.config
> index 8d9643767142..108fecc20fc1 100644
> --- a/kernel/configs/kvm_guest.config
> +++ b/kernel/configs/kvm_guest.config
> @@ -18,6 +18,7 @@ CONFIG_VIRTUALIZATION=y
>  CONFIG_HYPERVISOR_GUEST=y
>  CONFIG_PARAVIRT=y
>  CONFIG_KVM_GUEST=y
> +CONFIG_S390_GUEST=y
>  CONFIG_VIRTIO=y
>  CONFIG_VIRTIO_PCI=y
>  CONFIG_VIRTIO_BLK=y

Acked-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH 2/5] KVM: s390: use created_vcpus in more places

[David Hildenbrand]

On 20.12.2017 16:52, Christian Borntraeger wrote:
> commit a03825bbd0c3 ("KVM: s390: use kvm->created_vcpus") introduced
> kvm->created_vcpus to avoid races with the existing kvm->online_vcpus
> scheme. One place was "forgotten" and one new place was "added".
> Let's fix those.
> 
> Reported-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Fixes: 4e0b1ab72b8a ("KVM: s390: gs support for kvm guests")
> Fixes: a03825bbd0c3 ("KVM: s390: use kvm->created_vcpus")
> ---
>  arch/s390/kvm/kvm-s390.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 40f0ae5a883f..00ef6f47e466 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -573,7 +573,7 @@ static int kvm_vm_ioctl_enable_cap(struct kvm *kvm, struct kvm_enable_cap *cap)
>  	case KVM_CAP_S390_GS:
>  		r = -EINVAL;
>  		mutex_lock(&kvm->lock);
> -		if (atomic_read(&kvm->online_vcpus)) {
> +		if (kvm->created_vcpus) {
>  			r = -EBUSY;
>  		} else if (test_facility(133)) {
>  			set_kvm_facility(kvm->arch.model.fac_mask, 133);
> @@ -1094,7 +1094,7 @@ static int kvm_s390_set_processor_feat(struct kvm *kvm,
>  		return -EINVAL;
>  
>  	mutex_lock(&kvm->lock);
> -	if (!atomic_read(&kvm->online_vcpus)) {
> +	if (!kvm->created_vcpus) {
>  		bitmap_copy(kvm->arch.cpu_feat, (unsigned long *) data.feat,
>  			    KVM_S390_VM_CPU_FEAT_NR_BITS);
>  		ret = 0;
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

Re: [PATCH 3/5] KVM: s390: add debug tracing for cpu features of CPU model

[David Hildenbrand]

On 20.12.2017 16:52, Christian Borntraeger wrote:

"We already trace ... related to the CPU model. We should ..."

> The cpu model already traces the cpu facilities, the ibc and
> guest CPU ids. We should do the same for the cpu features (on
> success only).

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

Re: [PATCH 4/5] KVM: s390: drop use of spin lock in __floating_irq_kick

[David Hildenbrand]

On 20.12.2017 16:52, Christian Borntraeger wrote:
> From: Michael Mueller <mimu@linux.vnet.ibm.com>
> 
> It is not required to take to a lock to protect access to the cpuflags
> of the local interrupt structure of a vcpu as the performed operation
> is an atomic_or.
> 
> Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/kvm/interrupt.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 024ad8bcc516..818aa4248b0f 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -1569,7 +1569,6 @@ static void __floating_irq_kick(struct kvm *kvm, u64 type)
>  
>  	/* make the VCPU drop out of the SIE, or wake it up if sleeping */
>  	li = &dst_vcpu->arch.local_int;
> -	spin_lock(&li->lock);
>  	switch (type) {
>  	case KVM_S390_MCHK:
>  		atomic_or(CPUSTAT_STOP_INT, li->cpuflags);
> @@ -1581,7 +1580,6 @@ static void __floating_irq_kick(struct kvm *kvm, u64 type)
>  		atomic_or(CPUSTAT_EXT_INT, li->cpuflags);
>  		break;
>  	}
> -	spin_unlock(&li->lock);
>  	kvm_s390_vcpu_wakeup(dst_vcpu);
>  }
>  
> 

I remember that being somehow used to avoid the target VCPU just
clearing the indicators via __reset_intercept_indicators().

But as far as I can see, this has no effect and
set_intercept_indicators() will take care of it. In the worst case, we
have too many flags set, simply resulting in a VCPU exit and the
superfluous flags getting cleared.

So this looks sane to me.

-- 

Thanks,

David / dhildenb