[PATCH 0/5] KVM: s390: patches for 4.15

[PATCH 0/5] KVM: s390: patches for 4.15

[Christian Borntraeger]

My next pull request will be pretty small it seems as exitless
interrupt, crypto and large page support are all not ready yet.
Anyway here is what I have pending as of today and what will
be part of my next pull request. 

Christian Borntraeger (1):
  KVM: s390: document memory ordering for kvm_s390_vcpu_wakeup

David Hildenbrand (1):
  KVM: s390: vsie: use common code functions for pinning

Michael Mueller (2):
  KVM: s390: abstract conversion between isc and enum irq_types
  KVM: s390: clear_io_irq() requests are not expected for adapter
    interrupts

Tony Krowiak (1):
  KVM: s390: SIE considerations for AP Queue virtualization

 Documentation/virtual/kvm/devices/s390_flic.txt |  3 ++
 arch/s390/include/asm/kvm_host.h                | 25 +++++++++++--
 arch/s390/kvm/interrupt.c                       | 26 +++++++++++--
 arch/s390/kvm/vsie.c                            | 50 +++++++++----------------
 include/linux/kvm_host.h                        |  1 +
 virt/kvm/kvm_main.c                             |  4 +-
 6 files changed, 67 insertions(+), 42 deletions(-)

-- 
2.9.4

[PATCH 1/5] KVM: s390: document memory ordering for kvm_s390_vcpu_wakeup

[Christian Borntraeger]

swait_active does not enforce any ordering and it can therefore trigger
some subtle races when the CPU moves the read for the check before a
previous store and that store is then used on another CPU that is
preparing the swait.

On s390 there is a call to swait_active in kvm_s390_vcpu_wakeup. The
good thing is, on s390 all potential races cannot happen because all
callers of kvm_s390_vcpu_wakeup do not store (no race) or use an atomic
operation, which handles memory ordering. Since this is not guaranteed
by the Linux semantics (but by the implementation on s390) let's add
smp_mb_after_atomic to make this obvious and document the ordering.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Acked-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/kvm/interrupt.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index a832ad0..23d8fb2 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -1074,6 +1074,12 @@ void kvm_s390_vcpu_wakeup(struct kvm_vcpu *vcpu)
 	 * in kvm_vcpu_block without having the waitqueue set (polling)
 	 */
 	vcpu->valid_wakeup = true;
+	/*
+	 * This is mostly to document, that the read in swait_active could
+	 * be moved before other stores, leading to subtle races.
+	 * All current users do not store or use an atomic like update
+	 */
+	smp_mb__after_atomic();
 	if (swait_active(&vcpu->wq)) {
 		/*
 		 * The vcpu gave up the cpu voluntarily, mark it as a good
-- 
2.9.4

[PATCH 2/5] KVM: s390: SIE considerations for AP Queue virtualization

[Christian Borntraeger]

From: Tony Krowiak <akrowiak@linux.vnet.ibm.com>

The Crypto Control Block (CRYCB) is referenced by the SIE state
description and controls KVM guest access to the Adjunct
Processor (AP) adapters, usage domains and control domains.
This patch defines the AP control blocks to be used for
controlling guest access to the AP adapters and domains.

Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
Message-Id: <1507916344-3896-2-git-send-email-akrowiak@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/include/asm/kvm_host.h | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index fd006a2..646e1fe 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -685,11 +685,28 @@ struct kvm_s390_crypto {
 	__u8 dea_kw;
 };
 
+#define APCB0_MASK_SIZE 1
+struct kvm_s390_apcb0 {
+	__u64 apm[APCB0_MASK_SIZE];		/* 0x0000 */
+	__u64 aqm[APCB0_MASK_SIZE];		/* 0x0008 */
+	__u64 adm[APCB0_MASK_SIZE];		/* 0x0010 */
+	__u64 reserved18;			/* 0x0018 */
+};
+
+#define APCB1_MASK_SIZE 4
+struct kvm_s390_apcb1 {
+	__u64 apm[APCB1_MASK_SIZE];		/* 0x0000 */
+	__u64 aqm[APCB1_MASK_SIZE];		/* 0x0020 */
+	__u64 adm[APCB1_MASK_SIZE];		/* 0x0040 */
+	__u64 reserved60[4];			/* 0x0060 */
+};
+
 struct kvm_s390_crypto_cb {
-	__u8    reserved00[72];                 /* 0x0000 */
-	__u8    dea_wrapping_key_mask[24];      /* 0x0048 */
-	__u8    aes_wrapping_key_mask[32];      /* 0x0060 */
-	__u8    reserved80[128];                /* 0x0080 */
+	struct kvm_s390_apcb0 apcb0;		/* 0x0000 */
+	__u8    reserved20[40];			/* 0x0020 */
+	__u8    dea_wrapping_key_mask[24];	/* 0x0048 */
+	__u8    aes_wrapping_key_mask[32];	/* 0x0060 */
+	struct kvm_s390_apcb1 apcb1;		/* 0x0080 */
 };
 
 /*
-- 
2.9.4

[PATCH 3/5] KVM: s390: vsie: use common code functions for pinning

[Christian Borntraeger]

From: David Hildenbrand <david@redhat.com>

We will not see -ENOMEM (gfn_to_hva() will return KVM_ERR_PTR_BAD_PAGE
for all errors). So we can also get rid of special handling in the
callers of pin_guest_page() and always assume that it is a g2 error.

As also kvm_s390_inject_program_int() should never fail, we can
simplify pin_scb(), too.

Signed-off-by: David Hildenbrand <david@redhat.com>
Message-Id: <20170901151143.22714-1-david@redhat.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/kvm/vsie.c     | 50 +++++++++++++++++-------------------------------
 include/linux/kvm_host.h |  1 +
 virt/kvm/kvm_main.c      |  4 ++--
 3 files changed, 21 insertions(+), 34 deletions(-)

diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index b18b565..a311938 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -443,22 +443,14 @@ static int map_prefix(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
  *
  * Returns: - 0 on success
  *          - -EINVAL if the gpa is not valid guest storage
- *          - -ENOMEM if out of memory
  */
 static int pin_guest_page(struct kvm *kvm, gpa_t gpa, hpa_t *hpa)
 {
 	struct page *page;
-	hva_t hva;
-	int rc;
 
-	hva = gfn_to_hva(kvm, gpa_to_gfn(gpa));
-	if (kvm_is_error_hva(hva))
+	page = gfn_to_page(kvm, gpa_to_gfn(gpa));
+	if (is_error_page(page))
 		return -EINVAL;
-	rc = get_user_pages_fast(hva, 1, 1, &page);
-	if (rc < 0)
-		return rc;
-	else if (rc != 1)
-		return -ENOMEM;
 	*hpa = (hpa_t) page_to_virt(page) + (gpa & ~PAGE_MASK);
 	return 0;
 }
@@ -466,11 +458,7 @@ static int pin_guest_page(struct kvm *kvm, gpa_t gpa, hpa_t *hpa)
 /* Unpins a page previously pinned via pin_guest_page, marking it as dirty. */
 static void unpin_guest_page(struct kvm *kvm, gpa_t gpa, hpa_t hpa)
 {
-	struct page *page;
-
-	page = virt_to_page(hpa);
-	set_page_dirty_lock(page);
-	put_page(page);
+	kvm_release_pfn_dirty(hpa >> PAGE_SHIFT);
 	/* mark the page always as dirty for migration */
 	mark_page_dirty(kvm, gpa_to_gfn(gpa));
 }
@@ -557,7 +545,7 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 			rc = set_validity_icpt(scb_s, 0x003bU);
 		if (!rc) {
 			rc = pin_guest_page(vcpu->kvm, gpa, &hpa);
-			if (rc == -EINVAL)
+			if (rc)
 				rc = set_validity_icpt(scb_s, 0x0034U);
 		}
 		if (rc)
@@ -574,10 +562,10 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 		}
 		/* 256 bytes cannot cross page boundaries */
 		rc = pin_guest_page(vcpu->kvm, gpa, &hpa);
-		if (rc == -EINVAL)
+		if (rc) {
 			rc = set_validity_icpt(scb_s, 0x0080U);
-		if (rc)
 			goto unpin;
+		}
 		scb_s->itdba = hpa;
 	}
 
@@ -592,10 +580,10 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 		 * if this block gets bigger, we have to shadow it.
 		 */
 		rc = pin_guest_page(vcpu->kvm, gpa, &hpa);
-		if (rc == -EINVAL)
+		if (rc) {
 			rc = set_validity_icpt(scb_s, 0x1310U);
-		if (rc)
 			goto unpin;
+		}
 		scb_s->gvrd = hpa;
 	}
 
@@ -607,11 +595,11 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 		}
 		/* 64 bytes cannot cross page boundaries */
 		rc = pin_guest_page(vcpu->kvm, gpa, &hpa);
-		if (rc == -EINVAL)
+		if (rc) {
 			rc = set_validity_icpt(scb_s, 0x0043U);
-		/* Validity 0x0044 will be checked by SIE */
-		if (rc)
 			goto unpin;
+		}
+		/* Validity 0x0044 will be checked by SIE */
 		scb_s->riccbd = hpa;
 	}
 	if ((scb_s->ecb & ECB_GS) && !(scb_s->ecd & ECD_HOSTREGMGMT)) {
@@ -635,10 +623,10 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 		 * cross page boundaries
 		 */
 		rc = pin_guest_page(vcpu->kvm, gpa, &hpa);
-		if (rc == -EINVAL)
+		if (rc) {
 			rc = set_validity_icpt(scb_s, 0x10b0U);
-		if (rc)
 			goto unpin;
+		}
 		scb_s->sdnxo = hpa | sdnxc;
 	}
 	return 0;
@@ -663,7 +651,6 @@ static void unpin_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page,
  *
  * Returns: - 0 if the scb was pinned.
  *          - > 0 if control has to be given to guest 2
- *          - -ENOMEM if out of memory
  */
 static int pin_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page,
 		   gpa_t gpa)
@@ -672,14 +659,13 @@ static int pin_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page,
 	int rc;
 
 	rc = pin_guest_page(vcpu->kvm, gpa, &hpa);
-	if (rc == -EINVAL) {
+	if (rc) {
 		rc = kvm_s390_inject_program_int(vcpu, PGM_ADDRESSING);
-		if (!rc)
-			rc = 1;
+		WARN_ON_ONCE(rc);
+		return 1;
 	}
-	if (!rc)
-		vsie_page->scb_o = (struct kvm_s390_sie_block *) hpa;
-	return rc;
+	vsie_page->scb_o = (struct kvm_s390_sie_block *) hpa;
+	return 0;
 }
 
 /*
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 6882538..2e754b7 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -667,6 +667,7 @@ kvm_pfn_t __gfn_to_pfn_memslot(struct kvm_memory_slot *slot, gfn_t gfn,
 			       bool *writable);
 
 void kvm_release_pfn_clean(kvm_pfn_t pfn);
+void kvm_release_pfn_dirty(kvm_pfn_t pfn);
 void kvm_set_pfn_dirty(kvm_pfn_t pfn);
 void kvm_set_pfn_accessed(kvm_pfn_t pfn);
 void kvm_get_pfn(kvm_pfn_t pfn);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 9deb5a2..37731f6 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -122,7 +122,6 @@ static void hardware_disable_all(void);
 
 static void kvm_io_bus_destroy(struct kvm_io_bus *bus);
 
-static void kvm_release_pfn_dirty(kvm_pfn_t pfn);
 static void mark_page_dirty_in_slot(struct kvm_memory_slot *memslot, gfn_t gfn);
 
 __visible bool kvm_rebooting;
@@ -1679,11 +1678,12 @@ void kvm_release_page_dirty(struct page *page)
 }
 EXPORT_SYMBOL_GPL(kvm_release_page_dirty);
 
-static void kvm_release_pfn_dirty(kvm_pfn_t pfn)
+void kvm_release_pfn_dirty(kvm_pfn_t pfn)
 {
 	kvm_set_pfn_dirty(pfn);
 	kvm_release_pfn_clean(pfn);
 }
+EXPORT_SYMBOL_GPL(kvm_release_pfn_dirty);
 
 void kvm_set_pfn_dirty(kvm_pfn_t pfn)
 {
-- 
2.9.4

[PATCH 4/5] KVM: s390: abstract conversion between isc and enum irq_types

[Christian Borntraeger]

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

The abstraction of the conversion between an isc value and an irq_type
by means of functions isc_to_irq_type() and irq_type_to_isc() allows
to clarify the respective operations where used.

Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Reviewed-by: Pierre Morel <pmorel@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 | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 23d8fb2..a3da4f3 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -213,6 +213,16 @@ static inline unsigned long pending_irqs(struct kvm_vcpu *vcpu)
 	       vcpu->arch.local_int.pending_irqs;
 }
 
+static inline int isc_to_irq_type(unsigned long isc)
+{
+	return IRQ_PEND_IO_ISC_0 + isc;
+}
+
+static inline int irq_type_to_isc(unsigned long irq_type)
+{
+	return irq_type - IRQ_PEND_IO_ISC_0;
+}
+
 static unsigned long disable_iscs(struct kvm_vcpu *vcpu,
 				   unsigned long active_mask)
 {
@@ -220,7 +230,7 @@ static unsigned long disable_iscs(struct kvm_vcpu *vcpu,
 
 	for (i = 0; i <= MAX_ISC; i++)
 		if (!(vcpu->arch.sie_block->gcr[6] & isc_to_isc_bits(i)))
-			active_mask &= ~(1UL << (IRQ_PEND_IO_ISC_0 + i));
+			active_mask &= ~(1UL << (isc_to_irq_type(i)));
 
 	return active_mask;
 }
@@ -901,7 +911,7 @@ static int __must_check __deliver_io(struct kvm_vcpu *vcpu,
 	fi = &vcpu->kvm->arch.float_int;
 
 	spin_lock(&fi->lock);
-	isc_list = &fi->lists[irq_type - IRQ_PEND_IO_ISC_0];
+	isc_list = &fi->lists[irq_type_to_isc(irq_type)];
 	inti = list_first_entry_or_null(isc_list,
 					struct kvm_s390_interrupt_info,
 					list);
@@ -1401,7 +1411,7 @@ static struct kvm_s390_interrupt_info *get_io_int(struct kvm *kvm,
 		list_del_init(&iter->list);
 		fi->counters[FIRQ_CNTR_IO] -= 1;
 		if (list_empty(isc_list))
-			clear_bit(IRQ_PEND_IO_ISC_0 + isc, &fi->pending_irqs);
+			clear_bit(isc_to_irq_type(isc), &fi->pending_irqs);
 		spin_unlock(&fi->lock);
 		return iter;
 	}
@@ -1528,7 +1538,7 @@ static int __inject_io(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
 	isc = int_word_to_isc(inti->io.io_int_word);
 	list = &fi->lists[FIRQ_LIST_IO_ISC_0 + isc];
 	list_add_tail(&inti->list, list);
-	set_bit(IRQ_PEND_IO_ISC_0 + isc, &fi->pending_irqs);
+	set_bit(isc_to_irq_type(isc), &fi->pending_irqs);
 	spin_unlock(&fi->lock);
 	return 0;
 }
-- 
2.9.4

[PATCH 5/5] KVM: s390: clear_io_irq() requests are not expected for adapter interrupts

[Christian Borntraeger]

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

There is a chance to delete not yet delivered I/O interrupts if an
exploiter uses the subsystem identification word 0x0000 while
processing a KVM_DEV_FLIC_CLEAR_IO_IRQ ioctl. -EINVAL will be returned
now instead in that case.

Classic interrupts will always have bit 0x10000 set in the schid while
adapter interrupts have a zero schid. The clear_io_irq interface is
only useful for classic interrupts (as adapter interrupts belong to
many devices). Let's make this interface more strict and forbid a schid
of 0.

Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 Documentation/virtual/kvm/devices/s390_flic.txt | 3 +++
 arch/s390/kvm/interrupt.c                       | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/Documentation/virtual/kvm/devices/s390_flic.txt b/Documentation/virtual/kvm/devices/s390_flic.txt
index 2f1cbf1..27ad53c 100644
--- a/Documentation/virtual/kvm/devices/s390_flic.txt
+++ b/Documentation/virtual/kvm/devices/s390_flic.txt
@@ -156,3 +156,6 @@ FLIC with an unknown group or attribute gives the error code EINVAL (instead of
 ENXIO, as specified in the API documentation). It is not possible to conclude
 that a FLIC operation is unavailable based on the error code resulting from a
 usage attempt.
+
+Note: The KVM_DEV_FLIC_CLEAR_IO_IRQ ioctl will return EINVAL in case a zero
+schid is specified.
diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index a3da4f3..c8aacce 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -2191,6 +2191,8 @@ static int clear_io_irq(struct kvm *kvm, struct kvm_device_attr *attr)
 		return -EINVAL;
 	if (copy_from_user(&schid, (void __user *) attr->addr, sizeof(schid)))
 		return -EFAULT;
+	if (!schid)
+		return -EINVAL;
 	kfree(kvm_s390_get_io_int(kvm, isc_mask, schid));
 	/*
 	 * If userspace is conforming to the architecture, we can have at most
-- 
2.9.4

Re: [PATCH 1/5] KVM: s390: document memory ordering for kvm_s390_vcpu_wakeup

[Cornelia Huck]

On Wed,  8 Nov 2017 09:41:39 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> swait_active does not enforce any ordering and it can therefore trigger
> some subtle races when the CPU moves the read for the check before a
> previous store and that store is then used on another CPU that is
> preparing the swait.
> 
> On s390 there is a call to swait_active in kvm_s390_vcpu_wakeup. The
> good thing is, on s390 all potential races cannot happen because all
> callers of kvm_s390_vcpu_wakeup do not store (no race) or use an atomic
> operation, which handles memory ordering. Since this is not guaranteed
> by the Linux semantics (but by the implementation on s390) let's add
> smp_mb_after_atomic to make this obvious and document the ordering.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Acked-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/kvm/interrupt.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index a832ad0..23d8fb2 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -1074,6 +1074,12 @@ void kvm_s390_vcpu_wakeup(struct kvm_vcpu *vcpu)
>  	 * in kvm_vcpu_block without having the waitqueue set (polling)
>  	 */
>  	vcpu->valid_wakeup = true;
> +	/*
> +	 * This is mostly to document, that the read in swait_active could
> +	 * be moved before other stores, leading to subtle races.
> +	 * All current users do not store or use an atomic like update
> +	 */
> +	smp_mb__after_atomic();
>  	if (swait_active(&vcpu->wq)) {
>  		/*
>  		 * The vcpu gave up the cpu voluntarily, mark it as a good

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

Re: [PATCH 2/5] KVM: s390: SIE considerations for AP Queue virtualization

[Cornelia Huck]

On Wed,  8 Nov 2017 09:41:40 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> From: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
> 
> The Crypto Control Block (CRYCB) is referenced by the SIE state
> description and controls KVM guest access to the Adjunct
> Processor (AP) adapters, usage domains and control domains.
> This patch defines the AP control blocks to be used for
> controlling guest access to the AP adapters and domains.
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
> Message-Id: <1507916344-3896-2-git-send-email-akrowiak@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/include/asm/kvm_host.h | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)

I obviously can't verify (no doc), but I trust that you have done so,
so:

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

Re: [PATCH 3/5] KVM: s390: vsie: use common code functions for pinning

[Cornelia Huck]

On Wed,  8 Nov 2017 09:41:41 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> From: David Hildenbrand <david@redhat.com>
> 
> We will not see -ENOMEM (gfn_to_hva() will return KVM_ERR_PTR_BAD_PAGE
> for all errors). So we can also get rid of special handling in the
> callers of pin_guest_page() and always assume that it is a g2 error.
> 
> As also kvm_s390_inject_program_int() should never fail, we can
> simplify pin_scb(), too.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> Message-Id: <20170901151143.22714-1-david@redhat.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/kvm/vsie.c     | 50 +++++++++++++++++-------------------------------
>  include/linux/kvm_host.h |  1 +
>  virt/kvm/kvm_main.c      |  4 ++--
>  3 files changed, 21 insertions(+), 34 deletions(-)

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

Re: [PATCH 4/5] KVM: s390: abstract conversion between isc and enum irq_types

[Cornelia Huck]

On Wed,  8 Nov 2017 09:41:42 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> From: Michael Mueller <mimu@linux.vnet.ibm.com>
> 
> The abstraction of the conversion between an isc value and an irq_type
> by means of functions isc_to_irq_type() and irq_type_to_isc() allows
> to clarify the respective operations where used.
> 
> Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Reviewed-by: Pierre Morel <pmorel@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 | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)

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

Re: [PATCH 5/5] KVM: s390: clear_io_irq() requests are not expected for adapter interrupts

[Cornelia Huck]

On Wed,  8 Nov 2017 09:41:43 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> From: Michael Mueller <mimu@linux.vnet.ibm.com>
> 
> There is a chance to delete not yet delivered I/O interrupts if an
> exploiter uses the subsystem identification word 0x0000 while
> processing a KVM_DEV_FLIC_CLEAR_IO_IRQ ioctl. -EINVAL will be returned
> now instead in that case.
> 
> Classic interrupts will always have bit 0x10000 set in the schid while
> adapter interrupts have a zero schid. The clear_io_irq interface is
> only useful for classic interrupts (as adapter interrupts belong to
> many devices). Let's make this interface more strict and forbid a schid
> of 0.

I'm wondering: Is there any possible use case to clear adapter
interrupts? This interface was introduced to handle the case where a
CRW was made pending for a subchannel (which implies any pending
interrupt needs to be cleared.)

Alas, I cannot check the adapter interrupt question myself, as the
public doc is lacking :( But qdio as another adapter interrupt user
comes to mind (not that we support it in qemu, but still...)

> 
> Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  Documentation/virtual/kvm/devices/s390_flic.txt | 3 +++
>  arch/s390/kvm/interrupt.c                       | 2 ++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/Documentation/virtual/kvm/devices/s390_flic.txt b/Documentation/virtual/kvm/devices/s390_flic.txt
> index 2f1cbf1..27ad53c 100644
> --- a/Documentation/virtual/kvm/devices/s390_flic.txt
> +++ b/Documentation/virtual/kvm/devices/s390_flic.txt
> @@ -156,3 +156,6 @@ FLIC with an unknown group or attribute gives the error code EINVAL (instead of
>  ENXIO, as specified in the API documentation). It is not possible to conclude
>  that a FLIC operation is unavailable based on the error code resulting from a
>  usage attempt.
> +
> +Note: The KVM_DEV_FLIC_CLEAR_IO_IRQ ioctl will return EINVAL in case a zero
> +schid is specified.
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index a3da4f3..c8aacce 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -2191,6 +2191,8 @@ static int clear_io_irq(struct kvm *kvm, struct kvm_device_attr *attr)
>  		return -EINVAL;
>  	if (copy_from_user(&schid, (void __user *) attr->addr, sizeof(schid)))
>  		return -EFAULT;
> +	if (!schid)
> +		return -EINVAL;
>  	kfree(kvm_s390_get_io_int(kvm, isc_mask, schid));
>  	/*
>  	 * If userspace is conforming to the architecture, we can have at most

Re: [PATCH 2/5] KVM: s390: SIE considerations for AP Queue virtualization

[David Hildenbrand]

On 08.11.2017 09:41, Christian Borntraeger wrote:
> From: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
> 
> The Crypto Control Block (CRYCB) is referenced by the SIE state
> description and controls KVM guest access to the Adjunct
> Processor (AP) adapters, usage domains and control domains.
> This patch defines the AP control blocks to be used for
> controlling guest access to the AP adapters and domains.
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
> Message-Id: <1507916344-3896-2-git-send-email-akrowiak@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/include/asm/kvm_host.h | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index fd006a2..646e1fe 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -685,11 +685,28 @@ struct kvm_s390_crypto {
>  	__u8 dea_kw;
>  };
>  
> +#define APCB0_MASK_SIZE 1
> +struct kvm_s390_apcb0 {
> +	__u64 apm[APCB0_MASK_SIZE];		/* 0x0000 */
> +	__u64 aqm[APCB0_MASK_SIZE];		/* 0x0008 */
> +	__u64 adm[APCB0_MASK_SIZE];		/* 0x0010 */

Do these really have to be arrays? Also, size usually specifies bytes
and not double words (applies also to the one definition below).

> +	__u64 reserved18;			/* 0x0018 */
> +};
> +
> +#define APCB1_MASK_SIZE 4
> +struct kvm_s390_apcb1 {
> +	__u64 apm[APCB1_MASK_SIZE];		/* 0x0000 */
> +	__u64 aqm[APCB1_MASK_SIZE];		/* 0x0020 */
> +	__u64 adm[APCB1_MASK_SIZE];		/* 0x0040 */
> +	__u64 reserved60[4];			/* 0x0060 */
> +};
> +
>  struct kvm_s390_crypto_cb {
> -	__u8    reserved00[72];                 /* 0x0000 */
> -	__u8    dea_wrapping_key_mask[24];      /* 0x0048 */
> -	__u8    aes_wrapping_key_mask[32];      /* 0x0060 */
> -	__u8    reserved80[128];                /* 0x0080 */
> +	struct kvm_s390_apcb0 apcb0;		/* 0x0000 */
> +	__u8    reserved20[40];			/* 0x0020 */

Not sure if should turn this into [0x0048 - 0x0020], just like we do for
e.g. lowcore. But maybe the specification explicitly states "40 bytes
reserved". The 40 makes sense.

> +	__u8    dea_wrapping_key_mask[24];	/* 0x0048 */
> +	__u8    aes_wrapping_key_mask[32];	/* 0x0060 */
> +	struct kvm_s390_apcb1 apcb1;		/* 0x0080 */
>  };

While touching all lines, can we get rid of the strange indentation
after __u8?

Unfortunately I don't have access to any specification, so I can't
double check.

>  
>  /*
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH 1/5] KVM: s390: document memory ordering for kvm_s390_vcpu_wakeup

[David Hildenbrand]

On 08.11.2017 09:41, Christian Borntraeger wrote:
> swait_active does not enforce any ordering and it can therefore trigger
> some subtle races when the CPU moves the read for the check before a
> previous store and that store is then used on another CPU that is
> preparing the swait.
> 
> On s390 there is a call to swait_active in kvm_s390_vcpu_wakeup. The
> good thing is, on s390 all potential races cannot happen because all
> callers of kvm_s390_vcpu_wakeup do not store (no race) or use an atomic
> operation, which handles memory ordering. Since this is not guaranteed
> by the Linux semantics (but by the implementation on s390) let's add
> smp_mb_after_atomic to make this obvious and document the ordering.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Acked-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/kvm/interrupt.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index a832ad0..23d8fb2 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -1074,6 +1074,12 @@ void kvm_s390_vcpu_wakeup(struct kvm_vcpu *vcpu)
>  	 * in kvm_vcpu_block without having the waitqueue set (polling)
>  	 */
>  	vcpu->valid_wakeup = true;
> +	/*
> +	 * This is mostly to document, that the read in swait_active could
> +	 * be moved before other stores, leading to subtle races.
> +	 * All current users do not store or use an atomic like update

Wonder if it makes sense to document it in a way, that future code
changes (outside of kvm_s390_vcpu_wakeup) will outdate documentation.

/*
 * The read in swait_active could be moved before other stores, so avoid
 * any subtle races with potential callers.
 */

Whatever you go for:

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

> +	 */
> +	smp_mb__after_atomic();
>  	if (swait_active(&vcpu->wq)) {
>  		/*
>  		 * The vcpu gave up the cpu voluntarily, mark it as a good
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH 4/5] KVM: s390: abstract conversion between isc and enum irq_types

[David Hildenbrand]

On 08.11.2017 09:41, Christian Borntraeger wrote:
> From: Michael Mueller <mimu@linux.vnet.ibm.com>
> 
> The abstraction of the conversion between an isc value and an irq_type
> by means of functions isc_to_irq_type() and irq_type_to_isc() allows
> to clarify the respective operations where used.
> 
> Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Reviewed-by: Pierre Morel <pmorel@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 | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 23d8fb2..a3da4f3 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -213,6 +213,16 @@ static inline unsigned long pending_irqs(struct kvm_vcpu *vcpu)
>  	       vcpu->arch.local_int.pending_irqs;
>  }
>  
> +static inline int isc_to_irq_type(unsigned long isc)
> +{
> +	return IRQ_PEND_IO_ISC_0 + isc;
> +}
> +
> +static inline int irq_type_to_isc(unsigned long irq_type)
> +{
> +	return irq_type - IRQ_PEND_IO_ISC_0;
> +}
> +

(I would move it directly below is_ioirq()), so all IRQ_PEND_IO_ISC_0
users are at one place).

Let's make the r-b list longer.

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


-- 

Thanks,

David / dhildenb

Re: [PATCH 5/5] KVM: s390: clear_io_irq() requests are not expected for adapter interrupts

[Christian Borntraeger]

On 11/08/2017 10:19 AM, Cornelia Huck wrote:
> On Wed,  8 Nov 2017 09:41:43 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> From: Michael Mueller <mimu@linux.vnet.ibm.com>
>>
>> There is a chance to delete not yet delivered I/O interrupts if an
>> exploiter uses the subsystem identification word 0x0000 while
>> processing a KVM_DEV_FLIC_CLEAR_IO_IRQ ioctl. -EINVAL will be returned
>> now instead in that case.
>>
>> Classic interrupts will always have bit 0x10000 set in the schid while
>> adapter interrupts have a zero schid. The clear_io_irq interface is
>> only useful for classic interrupts (as adapter interrupts belong to
>> many devices). Let's make this interface more strict and forbid a schid
>> of 0.
> 
> I'm wondering: Is there any possible use case to clear adapter
> interrupts? This interface was introduced to handle the case where a
> CRW was made pending for a subchannel (which implies any pending
> interrupt needs to be cleared.)

The problem with clearing adapter interrupts is that is actually a summary
interrupt for every potential device. So I somewhat consider an adapter interrupt
pending when the summary indicator went from 0 to 1. So I dont think clearing
a single one makes not much sense. (And this interface would be wrong for
that I think)
The only use cases I can imagine for clearing adapter interrupts (e.g. reset)
is already covered by KVM_DEV_FLIC_CLEAR_IRQS

> 
> Alas, I cannot check the adapter interrupt question myself, as the
> public doc is lacking :( But qdio as another adapter interrupt user
> comes to mind (not that we support it in qemu, but still...)
> 
>>
>> Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
>> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  Documentation/virtual/kvm/devices/s390_flic.txt | 3 +++
>>  arch/s390/kvm/interrupt.c                       | 2 ++
>>  2 files changed, 5 insertions(+)
>>
>> diff --git a/Documentation/virtual/kvm/devices/s390_flic.txt b/Documentation/virtual/kvm/devices/s390_flic.txt
>> index 2f1cbf1..27ad53c 100644
>> --- a/Documentation/virtual/kvm/devices/s390_flic.txt
>> +++ b/Documentation/virtual/kvm/devices/s390_flic.txt
>> @@ -156,3 +156,6 @@ FLIC with an unknown group or attribute gives the error code EINVAL (instead of
>>  ENXIO, as specified in the API documentation). It is not possible to conclude
>>  that a FLIC operation is unavailable based on the error code resulting from a
>>  usage attempt.
>> +
>> +Note: The KVM_DEV_FLIC_CLEAR_IO_IRQ ioctl will return EINVAL in case a zero
>> +schid is specified.
>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>> index a3da4f3..c8aacce 100644
>> --- a/arch/s390/kvm/interrupt.c
>> +++ b/arch/s390/kvm/interrupt.c
>> @@ -2191,6 +2191,8 @@ static int clear_io_irq(struct kvm *kvm, struct kvm_device_attr *attr)
>>  		return -EINVAL;
>>  	if (copy_from_user(&schid, (void __user *) attr->addr, sizeof(schid)))
>>  		return -EFAULT;
>> +	if (!schid)
>> +		return -EINVAL;
>>  	kfree(kvm_s390_get_io_int(kvm, isc_mask, schid));
>>  	/*
>>  	 * If userspace is conforming to the architecture, we can have at most
> 

Re: [PATCH 5/5] KVM: s390: clear_io_irq() requests are not expected for adapter interrupts

[Cornelia Huck]

On Wed, 8 Nov 2017 12:04:22 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 11/08/2017 10:19 AM, Cornelia Huck wrote:
> > On Wed,  8 Nov 2017 09:41:43 +0100
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >   
> >> From: Michael Mueller <mimu@linux.vnet.ibm.com>
> >>
> >> There is a chance to delete not yet delivered I/O interrupts if an
> >> exploiter uses the subsystem identification word 0x0000 while
> >> processing a KVM_DEV_FLIC_CLEAR_IO_IRQ ioctl. -EINVAL will be returned
> >> now instead in that case.
> >>
> >> Classic interrupts will always have bit 0x10000 set in the schid while
> >> adapter interrupts have a zero schid. The clear_io_irq interface is
> >> only useful for classic interrupts (as adapter interrupts belong to
> >> many devices). Let's make this interface more strict and forbid a schid
> >> of 0.  
> > 
> > I'm wondering: Is there any possible use case to clear adapter
> > interrupts? This interface was introduced to handle the case where a
> > CRW was made pending for a subchannel (which implies any pending
> > interrupt needs to be cleared.)  
> 
> The problem with clearing adapter interrupts is that is actually a summary
> interrupt for every potential device. So I somewhat consider an adapter interrupt
> pending when the summary indicator went from 0 to 1. So I dont think clearing
> a single one makes not much sense. (And this interface would be wrong for
> that I think)

Yes, this interface would be problematic. I'm not sure what's supposed
to happen with crws vs. pending adapter interrupts, though.

> The only use cases I can imagine for clearing adapter interrupts (e.g. reset)
> is already covered by KVM_DEV_FLIC_CLEAR_IRQS
> 
> > 
> > Alas, I cannot check the adapter interrupt question myself, as the
> > public doc is lacking :( But qdio as another adapter interrupt user
> > comes to mind (not that we support it in qemu, but still...)
> >   
> >>
> >> Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
> >> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >> ---
> >>  Documentation/virtual/kvm/devices/s390_flic.txt | 3 +++
> >>  arch/s390/kvm/interrupt.c                       | 2 ++
> >>  2 files changed, 5 insertions(+)
> >>
> >> diff --git a/Documentation/virtual/kvm/devices/s390_flic.txt b/Documentation/virtual/kvm/devices/s390_flic.txt
> >> index 2f1cbf1..27ad53c 100644
> >> --- a/Documentation/virtual/kvm/devices/s390_flic.txt
> >> +++ b/Documentation/virtual/kvm/devices/s390_flic.txt
> >> @@ -156,3 +156,6 @@ FLIC with an unknown group or attribute gives the error code EINVAL (instead of
> >>  ENXIO, as specified in the API documentation). It is not possible to conclude
> >>  that a FLIC operation is unavailable based on the error code resulting from a
> >>  usage attempt.
> >> +
> >> +Note: The KVM_DEV_FLIC_CLEAR_IO_IRQ ioctl will return EINVAL in case a zero
> >> +schid is specified.
> >> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> >> index a3da4f3..c8aacce 100644
> >> --- a/arch/s390/kvm/interrupt.c
> >> +++ b/arch/s390/kvm/interrupt.c
> >> @@ -2191,6 +2191,8 @@ static int clear_io_irq(struct kvm *kvm, struct kvm_device_attr *attr)
> >>  		return -EINVAL;
> >>  	if (copy_from_user(&schid, (void __user *) attr->addr, sizeof(schid)))
> >>  		return -EFAULT;
> >> +	if (!schid)
> >> +		return -EINVAL;
> >>  	kfree(kvm_s390_get_io_int(kvm, isc_mask, schid));
> >>  	/*
> >>  	 * If userspace is conforming to the architecture, we can have at most  
> >   
> 

As this particular interface would not be a good match for whatever we
need to do for adapter interrupts, let's go with this patch.

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

Re: [PATCH 0/5] KVM: s390: patches for 4.15

[Cornelia Huck]

On Wed,  8 Nov 2017 09:41:38 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> My next pull request will be pretty small it seems as exitless
> interrupt, crypto and large page support are all not ready yet.

I'm looking forward to the exitless interrupt patches :)

> Anyway here is what I have pending as of today and what will
> be part of my next pull request. 

Yup, looks sane to me.

> 
> Christian Borntraeger (1):
>   KVM: s390: document memory ordering for kvm_s390_vcpu_wakeup
> 
> David Hildenbrand (1):
>   KVM: s390: vsie: use common code functions for pinning
> 
> Michael Mueller (2):
>   KVM: s390: abstract conversion between isc and enum irq_types
>   KVM: s390: clear_io_irq() requests are not expected for adapter
>     interrupts
> 
> Tony Krowiak (1):
>   KVM: s390: SIE considerations for AP Queue virtualization
> 
>  Documentation/virtual/kvm/devices/s390_flic.txt |  3 ++
>  arch/s390/include/asm/kvm_host.h                | 25 +++++++++++--
>  arch/s390/kvm/interrupt.c                       | 26 +++++++++++--
>  arch/s390/kvm/vsie.c                            | 50 +++++++++----------------
>  include/linux/kvm_host.h                        |  1 +
>  virt/kvm/kvm_main.c                             |  4 +-
>  6 files changed, 67 insertions(+), 42 deletions(-)
> 

Re: [PATCH 5/5] KVM: s390: clear_io_irq() requests are not expected for adapter interrupts

[Michael Mueller]

On 08.11.17 12:09, Cornelia Huck wrote:
> On Wed, 8 Nov 2017 12:04:22 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>
>> On 11/08/2017 10:19 AM, Cornelia Huck wrote:
>>> On Wed,  8 Nov 2017 09:41:43 +0100
>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>>    
>>>> From: Michael Mueller <mimu@linux.vnet.ibm.com>
>>>>
>>>> There is a chance to delete not yet delivered I/O interrupts if an
>>>> exploiter uses the subsystem identification word 0x0000 while
>>>> processing a KVM_DEV_FLIC_CLEAR_IO_IRQ ioctl. -EINVAL will be returned
>>>> now instead in that case.
>>>>
>>>> Classic interrupts will always have bit 0x10000 set in the schid while
>>>> adapter interrupts have a zero schid. The clear_io_irq interface is
>>>> only useful for classic interrupts (as adapter interrupts belong to
>>>> many devices). Let's make this interface more strict and forbid a schid
>>>> of 0.
>>> I'm wondering: Is there any possible use case to clear adapter
>>> interrupts? This interface was introduced to handle the case where a
>>> CRW was made pending for a subchannel (which implies any pending
>>> interrupt needs to be cleared.)
>> The problem with clearing adapter interrupts is that is actually a summary
>> interrupt for every potential device. So I somewhat consider an adapter interrupt
>> pending when the summary indicator went from 0 to 1. So I dont think clearing
>> a single one makes not much sense. (And this interface would be wrong for
>> that I think)
> Yes, this interface would be problematic. I'm not sure what's supposed
> to happen with crws vs. pending adapter interrupts, though.
They will be delivered based on ISC priority and the traditional first 
when both have the same class.
>
>> The only use cases I can imagine for clearing adapter interrupts (e.g. reset)
>> is already covered by KVM_DEV_FLIC_CLEAR_IRQS
>>
>>> Alas, I cannot check the adapter interrupt question myself, as the
>>> public doc is lacking :( But qdio as another adapter interrupt user
>>> comes to mind (not that we support it in qemu, but still...)
>>>    
>>>> Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
>>>> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> ---
>>>>   Documentation/virtual/kvm/devices/s390_flic.txt | 3 +++
>>>>   arch/s390/kvm/interrupt.c                       | 2 ++
>>>>   2 files changed, 5 insertions(+)
>>>>
>>>> diff --git a/Documentation/virtual/kvm/devices/s390_flic.txt b/Documentation/virtual/kvm/devices/s390_flic.txt
>>>> index 2f1cbf1..27ad53c 100644
>>>> --- a/Documentation/virtual/kvm/devices/s390_flic.txt
>>>> +++ b/Documentation/virtual/kvm/devices/s390_flic.txt
>>>> @@ -156,3 +156,6 @@ FLIC with an unknown group or attribute gives the error code EINVAL (instead of
>>>>   ENXIO, as specified in the API documentation). It is not possible to conclude
>>>>   that a FLIC operation is unavailable based on the error code resulting from a
>>>>   usage attempt.
>>>> +
>>>> +Note: The KVM_DEV_FLIC_CLEAR_IO_IRQ ioctl will return EINVAL in case a zero
>>>> +schid is specified.
>>>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>>>> index a3da4f3..c8aacce 100644
>>>> --- a/arch/s390/kvm/interrupt.c
>>>> +++ b/arch/s390/kvm/interrupt.c
>>>> @@ -2191,6 +2191,8 @@ static int clear_io_irq(struct kvm *kvm, struct kvm_device_attr *attr)
>>>>   		return -EINVAL;
>>>>   	if (copy_from_user(&schid, (void __user *) attr->addr, sizeof(schid)))
>>>>   		return -EFAULT;
>>>> +	if (!schid)
>>>> +		return -EINVAL;
>>>>   	kfree(kvm_s390_get_io_int(kvm, isc_mask, schid));
>>>>   	/*
>>>>   	 * If userspace is conforming to the architecture, we can have at most
>>>    
> As this particular interface would not be a good match for whatever we
> need to do for adapter interrupts, let's go with this patch.
>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>

Re: [PATCH 2/5] KVM: s390: SIE considerations for AP Queue virtualization

[Christian Borntraeger]

On 11/08/2017 10:29 AM, David Hildenbrand wrote:
> On 08.11.2017 09:41, Christian Borntraeger wrote:
>> From: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
>>
>> The Crypto Control Block (CRYCB) is referenced by the SIE state
>> description and controls KVM guest access to the Adjunct
>> Processor (AP) adapters, usage domains and control domains.
>> This patch defines the AP control blocks to be used for
>> controlling guest access to the AP adapters and domains.
>>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
>> Message-Id: <1507916344-3896-2-git-send-email-akrowiak@linux.vnet.ibm.com>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  arch/s390/include/asm/kvm_host.h | 25 +++++++++++++++++++++----
>>  1 file changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>> index fd006a2..646e1fe 100644
>> --- a/arch/s390/include/asm/kvm_host.h
>> +++ b/arch/s390/include/asm/kvm_host.h
>> @@ -685,11 +685,28 @@ struct kvm_s390_crypto {
>>  	__u8 dea_kw;
>>  };
>>  
>> +#define APCB0_MASK_SIZE 1
>> +struct kvm_s390_apcb0 {
>> +	__u64 apm[APCB0_MASK_SIZE];		/* 0x0000 */
>> +	__u64 aqm[APCB0_MASK_SIZE];		/* 0x0008 */
>> +	__u64 adm[APCB0_MASK_SIZE];		/* 0x0010 */
> 
> Do these really have to be arrays? Also, size usually specifies bytes
> and not double words (applies also to the one definition below).

This is later used with bit operations (e.g. see [RFC 16/19] KVM: s390: interface to configure KVM guest's AP matrix)
which operate on "long *", so I think its better than a char array.


> 
>> +	__u64 reserved18;			/* 0x0018 */
>> +};
>> +
>> +#define APCB1_MASK_SIZE 4
>> +struct kvm_s390_apcb1 {
>> +	__u64 apm[APCB1_MASK_SIZE];		/* 0x0000 */
>> +	__u64 aqm[APCB1_MASK_SIZE];		/* 0x0020 */
>> +	__u64 adm[APCB1_MASK_SIZE];		/* 0x0040 */
>> +	__u64 reserved60[4];			/* 0x0060 */
>> +};
>> +
>>  struct kvm_s390_crypto_cb {
>> -	__u8    reserved00[72];                 /* 0x0000 */
>> -	__u8    dea_wrapping_key_mask[24];      /* 0x0048 */
>> -	__u8    aes_wrapping_key_mask[32];      /* 0x0060 */
>> -	__u8    reserved80[128];                /* 0x0080 */
>> +	struct kvm_s390_apcb0 apcb0;		/* 0x0000 */
>> +	__u8    reserved20[40];			/* 0x0020 */
> 
> Not sure if should turn this into [0x0048 - 0x0020], just like we do for
> e.g. lowcore. But maybe the specification explicitly states "40 bytes
> reserved". The 40 makes sense.

Yes [0x0048 - 0x0020] looks better, will change.

> 
>> +	__u8    dea_wrapping_key_mask[24];	/* 0x0048 */
>> +	__u8    aes_wrapping_key_mask[32];	/* 0x0060 */
>> +	struct kvm_s390_apcb1 apcb1;		/* 0x0080 */
>>  };
> 
> While touching all lines, can we get rid of the strange indentation
> after __u8?

yes, will also fix.

Thanks

Re: [PATCH 5/5] KVM: s390: clear_io_irq() requests are not expected for adapter interrupts

[Cornelia Huck]

On Wed, 8 Nov 2017 13:14:01 +0100
Michael Mueller <mimu@linux.vnet.ibm.com> wrote:

> On 08.11.17 12:09, Cornelia Huck wrote:
> > On Wed, 8 Nov 2017 12:04:22 +0100
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >  
> >> On 11/08/2017 10:19 AM, Cornelia Huck wrote:  
> >>> On Wed,  8 Nov 2017 09:41:43 +0100
> >>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >>>      
> >>>> From: Michael Mueller <mimu@linux.vnet.ibm.com>
> >>>>
> >>>> There is a chance to delete not yet delivered I/O interrupts if an
> >>>> exploiter uses the subsystem identification word 0x0000 while
> >>>> processing a KVM_DEV_FLIC_CLEAR_IO_IRQ ioctl. -EINVAL will be returned
> >>>> now instead in that case.
> >>>>
> >>>> Classic interrupts will always have bit 0x10000 set in the schid while
> >>>> adapter interrupts have a zero schid. The clear_io_irq interface is
> >>>> only useful for classic interrupts (as adapter interrupts belong to
> >>>> many devices). Let's make this interface more strict and forbid a schid
> >>>> of 0.  
> >>> I'm wondering: Is there any possible use case to clear adapter
> >>> interrupts? This interface was introduced to handle the case where a
> >>> CRW was made pending for a subchannel (which implies any pending
> >>> interrupt needs to be cleared.)  
> >> The problem with clearing adapter interrupts is that is actually a summary
> >> interrupt for every potential device. So I somewhat consider an adapter interrupt
> >> pending when the summary indicator went from 0 to 1. So I dont think clearing
> >> a single one makes not much sense. (And this interface would be wrong for
> >> that I think)  
> > Yes, this interface would be problematic. I'm not sure what's supposed
> > to happen with crws vs. pending adapter interrupts, though.  
> They will be delivered based on ISC priority and the traditional first 
> when both have the same class.

I'm just wondering what is supposed to happen when all devices
associated with a summary indicator go away. The OS will hopefully
deregister the indicator if needed; the hypervisor might still set the
indicator and trigger an adapter interrupt before that happens (and
hopefully the OS can deal with that.)

Likely we don't have a problem, but I'm curious if there is anything
architected for the cases where real hardware exists (like qdio).

> >  
> >> The only use cases I can imagine for clearing adapter interrupts (e.g. reset)
> >> is already covered by KVM_DEV_FLIC_CLEAR_IRQS
> >>  
> >>> Alas, I cannot check the adapter interrupt question myself, as the
> >>> public doc is lacking :( But qdio as another adapter interrupt user
> >>> comes to mind (not that we support it in qemu, but still...)