[PATCH v2 00/12] KVM: s390: exitless interrupt support for KVM

[PATCH v2 00/12] KVM: s390: exitless interrupt support for KVM

[Christian Borntraeger]

Here is ithe 2nd version of the exitless interrupts for I/O adapter
interrupts. Lots of fixes and changes from Michael and myself:

- use BITS_PER_BYTE
- remove the aligned statement (since the structure is aligned naturally)
- put_guest_lc are on one line now
- comment about injecting the interrupt via SW if not yet done by hw
- rework return code handling
- unexport some gisa helpers by moving them before the users
- spaces before ":" for bitfields
- fix comments
- added acked and reviewed-bys

Jens Freimann (1):
  s390/bitops: add test_and_clear_bit_inv()

Michael Mueller (11):
  KVM: s390: reverse bit ordering of irqs in pending mask
  KVM: s390: define GISA format-0 data structure
  KVM: s390: implement GISA IPM related primitives
  s390/css: indicate the availability of the AIV facility
  KVM: s390: exploit GISA and AIV for emulated interrupts
  KVM: s390: abstract adapter interruption word generation from ISC
  KVM: s390: add GISA interrupts to FLIC ioctl interface
  KVM: s390: make kvm_s390_get_io_int() aware of GISA
  KVM: s390: activate GISA for emulated interrupts
  s390/sclp: expose the GISA format facility
  KVM: s390: introduce the format-1 GISA

 arch/s390/include/asm/bitops.h    |   5 +
 arch/s390/include/asm/css_chars.h |   4 +-
 arch/s390/include/asm/kvm_host.h  | 102 +++++++++++-----
 arch/s390/include/asm/sclp.h      |   1 +
 arch/s390/kvm/interrupt.c         | 237 ++++++++++++++++++++++++++++++++------
 arch/s390/kvm/kvm-s390.c          |  11 ++
 arch/s390/kvm/kvm-s390.h          |   3 +
 drivers/s390/char/sclp_early.c    |   3 +-
 8 files changed, 300 insertions(+), 66 deletions(-)

-- 
2.9.4

[PATCH v2 01/12] KVM: s390: reverse bit ordering of irqs in pending mask

[Christian Borntraeger]

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

This patch prepares a simplification of bit operations between the irq
pending mask for emulated interrupts and the Interruption Pending Mask
(IPM) which is part of the Guest Interruption State Area (GISA), a feature
that allows interrupt delivery to guests by means of the SIE instruction.

Without that change, a bit-wise *or* operation on parts of these two masks
would either require a look-up table of size 256 bytes to map the IPM
to the emulated irq pending mask bit orientation (all bits mirrored at half
byte) or a sequence of up to 8 condidional branches to perform tests of
single bit positions. Both options are to be rejected either by performance
or space utilization reasons.

Beyond that this change will be transparent.

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>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/include/asm/kvm_host.h | 54 ++++++++++++++++++++--------------------
 arch/s390/kvm/interrupt.c        | 12 ++++-----
 2 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index e16a9f2..9981721 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -409,35 +409,35 @@ struct kvm_vcpu_stat {
 #define PGM_PER				0x80
 #define PGM_CRYPTO_OPERATION		0x119
 
-/* irq types in order of priority */
+/* irq types in ascend order of priorities */
 enum irq_types {
-	IRQ_PEND_MCHK_EX = 0,
-	IRQ_PEND_SVC,
-	IRQ_PEND_PROG,
-	IRQ_PEND_MCHK_REP,
-	IRQ_PEND_EXT_IRQ_KEY,
-	IRQ_PEND_EXT_MALFUNC,
-	IRQ_PEND_EXT_EMERGENCY,
-	IRQ_PEND_EXT_EXTERNAL,
-	IRQ_PEND_EXT_CLOCK_COMP,
-	IRQ_PEND_EXT_CPU_TIMER,
-	IRQ_PEND_EXT_TIMING,
-	IRQ_PEND_EXT_SERVICE,
-	IRQ_PEND_EXT_HOST,
-	IRQ_PEND_PFAULT_INIT,
-	IRQ_PEND_PFAULT_DONE,
-	IRQ_PEND_VIRTIO,
-	IRQ_PEND_IO_ISC_0,
-	IRQ_PEND_IO_ISC_1,
-	IRQ_PEND_IO_ISC_2,
-	IRQ_PEND_IO_ISC_3,
-	IRQ_PEND_IO_ISC_4,
-	IRQ_PEND_IO_ISC_5,
-	IRQ_PEND_IO_ISC_6,
-	IRQ_PEND_IO_ISC_7,
-	IRQ_PEND_SIGP_STOP,
+	IRQ_PEND_SET_PREFIX = 0,
 	IRQ_PEND_RESTART,
-	IRQ_PEND_SET_PREFIX,
+	IRQ_PEND_SIGP_STOP,
+	IRQ_PEND_IO_ISC_7,
+	IRQ_PEND_IO_ISC_6,
+	IRQ_PEND_IO_ISC_5,
+	IRQ_PEND_IO_ISC_4,
+	IRQ_PEND_IO_ISC_3,
+	IRQ_PEND_IO_ISC_2,
+	IRQ_PEND_IO_ISC_1,
+	IRQ_PEND_IO_ISC_0,
+	IRQ_PEND_VIRTIO,
+	IRQ_PEND_PFAULT_DONE,
+	IRQ_PEND_PFAULT_INIT,
+	IRQ_PEND_EXT_HOST,
+	IRQ_PEND_EXT_SERVICE,
+	IRQ_PEND_EXT_TIMING,
+	IRQ_PEND_EXT_CPU_TIMER,
+	IRQ_PEND_EXT_CLOCK_COMP,
+	IRQ_PEND_EXT_EXTERNAL,
+	IRQ_PEND_EXT_EMERGENCY,
+	IRQ_PEND_EXT_MALFUNC,
+	IRQ_PEND_EXT_IRQ_KEY,
+	IRQ_PEND_MCHK_REP,
+	IRQ_PEND_PROG,
+	IRQ_PEND_SVC,
+	IRQ_PEND_MCHK_EX,
 	IRQ_PEND_COUNT
 };
 
diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index f8eb2cf..b7b2865 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -189,8 +189,8 @@ static int cpu_timer_irq_pending(struct kvm_vcpu *vcpu)
 
 static inline int is_ioirq(unsigned long irq_type)
 {
-	return ((irq_type >= IRQ_PEND_IO_ISC_0) &&
-		(irq_type <= IRQ_PEND_IO_ISC_7));
+	return ((irq_type >= IRQ_PEND_IO_ISC_7) &&
+		(irq_type <= IRQ_PEND_IO_ISC_0));
 }
 
 static uint64_t isc_to_isc_bits(int isc)
@@ -211,12 +211,12 @@ static inline unsigned long pending_irqs(struct kvm_vcpu *vcpu)
 
 static inline int isc_to_irq_type(unsigned long isc)
 {
-	return IRQ_PEND_IO_ISC_0 + 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;
+	return IRQ_PEND_IO_ISC_0 - irq_type;
 }
 
 static unsigned long disable_iscs(struct kvm_vcpu *vcpu,
@@ -1154,8 +1154,8 @@ int __must_check kvm_s390_deliver_pending_interrupts(struct kvm_vcpu *vcpu)
 		set_bit(IRQ_PEND_EXT_CPU_TIMER, &li->pending_irqs);
 
 	while ((irqs = deliverable_irqs(vcpu)) && !rc) {
-		/* bits are in the order of interrupt priority */
-		irq_type = find_first_bit(&irqs, IRQ_PEND_COUNT);
+		/* bits are in the reverse order of interrupt priority */
+		irq_type = find_last_bit(&irqs, IRQ_PEND_COUNT);
 		if (is_ioirq(irq_type)) {
 			rc = __deliver_io(vcpu, irq_type);
 		} else {
-- 
2.9.4

[PATCH v2 02/12] KVM: s390: define GISA format-0 data structure

[Christian Borntraeger]

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

In preperation to support pass-through adapter interrupts, the Guest
Interruption State Area (GISA) and the Adapter Interruption Virtualization
(AIV) features will be introduced here.

This patch introduces format-0 GISA (that is defines the struct describing
the GISA, allocates storage for it, and introduces fields for the
GISA address in kvm_s390_sie_block and kvm_s390_vsie).

As the GISA requires storage below 2GB, it is put in sie_page2, which is
already allocated in ZONE_DMA. In addition, The GISA requires alignment to
its integral boundary. This is already naturally aligned via the
padding in the sie_page2.

Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
Reviewed-by: Pierre Morel <pmorel@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>
---
 arch/s390/include/asm/kvm_host.h | 23 +++++++++++++++++++----
 arch/s390/kvm/kvm-s390.c         |  1 +
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 9981721..77acade 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -183,6 +183,7 @@ struct kvm_s390_sie_block {
 #define ECA_IB		0x40000000
 #define ECA_SIGPI	0x10000000
 #define ECA_MVPGI	0x01000000
+#define ECA_AIV		0x00200000
 #define ECA_VX		0x00020000
 #define ECA_PROTEXCI	0x00002000
 #define ECA_SII		0x00000001
@@ -227,7 +228,8 @@ struct kvm_s390_sie_block {
 	__u8    epdx;			/* 0x0069 */
 	__u8    reserved6a[2];		/* 0x006a */
 	__u32	todpr;			/* 0x006c */
-	__u8	reserved70[16];		/* 0x0070 */
+	__u32	gd;			/* 0x0070 */
+	__u8	reserved74[12];		/* 0x0074 */
 	__u64	mso;			/* 0x0080 */
 	__u64	msl;			/* 0x0088 */
 	psw_t	gpsw;			/* 0x0090 */
@@ -703,14 +705,27 @@ struct kvm_s390_crypto_cb {
 	struct kvm_s390_apcb1 apcb1;		/* 0x0080 */
 };
 
+struct kvm_s390_gisa {
+	u32 next_alert;
+	u8 ipm;
+	u8 reserved01;
+	u8 : 6;
+	u8 g : 1;
+	u8 c : 1;
+	u8 iam;
+	u8 reserved02[4];
+	u32 airq_count;
+};
+
 /*
- * sie_page2 has to be allocated as DMA because fac_list and crycb need
- * 31bit addresses in the sie control block.
+ * sie_page2 has to be allocated as DMA because fac_list, crycb and
+ * gisa need 31bit addresses in the sie control block.
  */
 struct sie_page2 {
 	__u64 fac_list[S390_ARCH_FAC_LIST_SIZE_U64];	/* 0x0000 */
 	struct kvm_s390_crypto_cb crycb;		/* 0x0800 */
-	u8 reserved900[0x1000 - 0x900];			/* 0x0900 */
+	struct kvm_s390_gisa gisa;			/* 0x0900 */
+	u8 reserved910[0x1000 - 0x910];			/* 0x0910 */
 };
 
 struct kvm_s390_vsie {
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index de16c22..48f0099 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -1915,6 +1915,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 	if (!kvm->arch.dbf)
 		goto out_err;
 
+	BUILD_BUG_ON(sizeof(struct sie_page2) != 4096);
 	kvm->arch.sie_page2 =
 	     (struct sie_page2 *) get_zeroed_page(GFP_KERNEL | GFP_DMA);
 	if (!kvm->arch.sie_page2)
-- 
2.9.4

[PATCH v2 03/12] s390/bitops: add test_and_clear_bit_inv()

[Christian Borntraeger]

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

This patch adds a MSB0 bit numbering version of test_and_clear_bit().

Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/include/asm/bitops.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/s390/include/asm/bitops.h b/arch/s390/include/asm/bitops.h
index 31e400c..86e5b2f 100644
--- a/arch/s390/include/asm/bitops.h
+++ b/arch/s390/include/asm/bitops.h
@@ -261,6 +261,11 @@ static inline void clear_bit_inv(unsigned long nr, volatile unsigned long *ptr)
 	return clear_bit(nr ^ (BITS_PER_LONG - 1), ptr);
 }
 
+static inline int test_and_clear_bit_inv(unsigned long nr, volatile unsigned long *ptr)
+{
+	return test_and_clear_bit(nr ^ (BITS_PER_LONG - 1), ptr);
+}
+
 static inline void __set_bit_inv(unsigned long nr, volatile unsigned long *ptr)
 {
 	return __set_bit(nr ^ (BITS_PER_LONG - 1), ptr);
-- 
2.9.4

[PATCH v2 04/12] KVM: s390: implement GISA IPM related primitives

[Christian Borntraeger]

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

The patch implements routines to access the GISA to test and modify
its Interruption Pending Mask (IPM) from the host side.

Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
Reviewed-by: Pierre Morel <pmorel@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>
---
 arch/s390/kvm/interrupt.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index b7b2865..d559776 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -203,6 +203,28 @@ static inline u8 int_word_to_isc(u32 int_word)
 	return (int_word & 0x38000000) >> 27;
 }
 
+#define IPM_BIT_OFFSET (offsetof(struct kvm_s390_gisa, ipm) * BITS_PER_BYTE)
+
+static inline void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
+{
+	set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
+}
+
+static inline u8 kvm_s390_gisa_get_ipm(struct kvm_s390_gisa *gisa)
+{
+	return (u8) READ_ONCE(gisa->ipm);
+}
+
+static inline void kvm_s390_gisa_clear_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
+{
+	clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
+}
+
+static inline int kvm_s390_gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
+{
+	return test_and_clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
+}
+
 static inline unsigned long pending_irqs(struct kvm_vcpu *vcpu)
 {
 	return vcpu->kvm->arch.float_int.pending_irqs |
@@ -2682,3 +2704,4 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu, __u8 __user *buf, int len)
 
 	return n;
 }
+
-- 
2.9.4

[PATCH v2 05/12] s390/css: indicate the availability of the AIV facility

[Christian Borntraeger]

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

The patch adds an indication for the presence Adapter Interruption
Virtualization facility (AIV) of the general channel subsystem
characteristics.

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>
Acked-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
[change wording]
---
 arch/s390/include/asm/css_chars.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/s390/include/asm/css_chars.h b/arch/s390/include/asm/css_chars.h
index a478eb6..fb56fa3 100644
--- a/arch/s390/include/asm/css_chars.h
+++ b/arch/s390/include/asm/css_chars.h
@@ -20,7 +20,9 @@ struct css_general_char {
 	u32 aif_tdd : 1; /* bit 56 */
 	u32 : 1;
 	u32 qebsm : 1;	 /* bit 58 */
-	u32 : 8;
+	u32 : 2;
+	u32 aiv : 1;     /* bit 61 */
+	u32 : 5;
 	u32 aif_osa : 1; /* bit 67 */
 	u32 : 12;
 	u32 eadm_rf : 1; /* bit 80 */
-- 
2.9.4

[PATCH v2 06/12] KVM: s390: exploit GISA and AIV for emulated interrupts

[Christian Borntraeger]

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

The adapter interruption virtualization (AIV) facility is an
optional facility that comes with functionality expected to increase
the performance of adapter interrupt handling for both emulated and
passed-through adapter interrupts. With AIV, adapter interrupts can be
delivered to the guest without exiting SIE.

This patch provides some preparations for using AIV for emulated adapter
interrupts (inclusive virtio) if it's available. When using AIV, the
interrupts are delivered at the so called GISA by setting the bit
corresponding to its Interruption Subclass (ISC) in the Interruption
Pending Mask (IPM) instead of inserting a node into the floating interrupt
list.

To keep the change reasonably small, the handling of this new state is
deferred in get_all_floating_irqs and handle_tpi. This patch concentrates
on the code handling enqueuement of emulated adapter interrupts, and their
delivery to the guest.

Note that care is still required for adapter interrupts using AIV,
because there is no guarantee that AIV is going to deliver the adapter
interrupts pending at the GISA (consider all vcpus idle). When delivering
GISA adapter interrupts by the host (usual mechanism) special attention
is required to honor interrupt priorities.

Empirical results show that the time window between making an interrupt
pending at the GISA and doing kvm_s390_deliver_pending_interrupts is
sufficient for a guest with at least moderate cpu activity to get adapter
interrupts delivered within the SIE, and potentially save some SIE exits
(if not other deliverable interrupts).

The code will be activated with a follow-up patch.

Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/include/asm/kvm_host.h |   1 +
 arch/s390/kvm/interrupt.c        | 106 +++++++++++++++++++++++++++++++--------
 arch/s390/kvm/kvm-s390.c         |   8 +++
 arch/s390/kvm/kvm-s390.h         |   3 ++
 4 files changed, 98 insertions(+), 20 deletions(-)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 77acade..6802d5d 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -772,6 +772,7 @@ struct kvm_arch{
 	struct kvm_s390_migration_state *migration_state;
 	/* subset of available cpu features enabled by user space */
 	DECLARE_BITMAP(cpu_feat, KVM_S390_VM_CPU_FEAT_NR_BITS);
+	struct kvm_s390_gisa *gisa;
 };
 
 #define KVM_HVA_ERR_BAD		(-1UL)
diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index d559776..263cd10 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -228,7 +228,8 @@ static inline int kvm_s390_gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gis
 static inline unsigned long pending_irqs(struct kvm_vcpu *vcpu)
 {
 	return vcpu->kvm->arch.float_int.pending_irqs |
-	       vcpu->arch.local_int.pending_irqs;
+		vcpu->arch.local_int.pending_irqs |
+		kvm_s390_gisa_get_ipm(vcpu->kvm->arch.gisa) << IRQ_PEND_IO_ISC_7;
 }
 
 static inline int isc_to_irq_type(unsigned long isc)
@@ -918,18 +919,38 @@ static int __must_check __deliver_virtio(struct kvm_vcpu *vcpu)
 	return rc ? -EFAULT : 0;
 }
 
+static int __do_deliver_io(struct kvm_vcpu *vcpu, struct kvm_s390_io_info *io)
+{
+	int rc;
+
+	rc  = put_guest_lc(vcpu, io->subchannel_id, (u16 *)__LC_SUBCHANNEL_ID);
+	rc |= put_guest_lc(vcpu, io->subchannel_nr, (u16 *)__LC_SUBCHANNEL_NR);
+	rc |= put_guest_lc(vcpu, io->io_int_parm, (u32 *)__LC_IO_INT_PARM);
+	rc |= put_guest_lc(vcpu, io->io_int_word, (u32 *)__LC_IO_INT_WORD);
+	rc |= write_guest_lc(vcpu, __LC_IO_OLD_PSW,
+			     &vcpu->arch.sie_block->gpsw,
+			     sizeof(psw_t));
+	rc |= read_guest_lc(vcpu, __LC_IO_NEW_PSW,
+			    &vcpu->arch.sie_block->gpsw,
+			    sizeof(psw_t));
+	return rc ? -EFAULT : 0;
+}
+
 static int __must_check __deliver_io(struct kvm_vcpu *vcpu,
 				     unsigned long irq_type)
 {
 	struct list_head *isc_list;
 	struct kvm_s390_float_interrupt *fi;
 	struct kvm_s390_interrupt_info *inti = NULL;
+	struct kvm_s390_io_info io;
+	u32 isc;
 	int rc = 0;
 
 	fi = &vcpu->kvm->arch.float_int;
 
 	spin_lock(&fi->lock);
-	isc_list = &fi->lists[irq_type_to_isc(irq_type)];
+	isc = irq_type_to_isc(irq_type);
+	isc_list = &fi->lists[isc];
 	inti = list_first_entry_or_null(isc_list,
 					struct kvm_s390_interrupt_info,
 					list);
@@ -957,24 +978,32 @@ static int __must_check __deliver_io(struct kvm_vcpu *vcpu,
 	spin_unlock(&fi->lock);
 
 	if (inti) {
-		rc  = put_guest_lc(vcpu, inti->io.subchannel_id,
-				(u16 *)__LC_SUBCHANNEL_ID);
-		rc |= put_guest_lc(vcpu, inti->io.subchannel_nr,
-				(u16 *)__LC_SUBCHANNEL_NR);
-		rc |= put_guest_lc(vcpu, inti->io.io_int_parm,
-				(u32 *)__LC_IO_INT_PARM);
-		rc |= put_guest_lc(vcpu, inti->io.io_int_word,
-				(u32 *)__LC_IO_INT_WORD);
-		rc |= write_guest_lc(vcpu, __LC_IO_OLD_PSW,
-				&vcpu->arch.sie_block->gpsw,
-				sizeof(psw_t));
-		rc |= read_guest_lc(vcpu, __LC_IO_NEW_PSW,
-				&vcpu->arch.sie_block->gpsw,
-				sizeof(psw_t));
+		rc = __do_deliver_io(vcpu, &(inti->io));
 		kfree(inti);
+		goto out;
 	}
 
-	return rc ? -EFAULT : 0;
+	if (vcpu->kvm->arch.gisa) {
+		if (kvm_s390_gisa_tac_ipm_gisc(vcpu->kvm->arch.gisa, isc)) {
+			/*
+			 * in case an adapter interrupt was not delivered
+			 * in SIE context KVM will handle the delivery
+			 */
+			VCPU_EVENT(vcpu, 4, "%s isc %u", "deliver: I/O (AI/gisa)", isc);
+			memset(&io, 0, sizeof(io));
+			io.io_int_word = (isc << 27) | 0x80000000;
+			vcpu->stat.deliver_io_int++;
+			trace_kvm_s390_deliver_interrupt(vcpu->vcpu_id,
+				KVM_S390_INT_IO(1, 0, 0, 0),
+				((__u32)io.subchannel_id << 16) |
+				io.subchannel_nr,
+				((__u64)io.io_int_parm << 32) |
+				io.io_int_word);
+			rc = __do_deliver_io(vcpu, &io);
+		}
+	}
+out:
+	return rc;
 }
 
 typedef int (*deliver_irq_t)(struct kvm_vcpu *vcpu);
@@ -1537,12 +1566,23 @@ static int __inject_io(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
 	struct kvm_s390_float_interrupt *fi;
 	struct list_head *list;
 	int isc;
+	int rc = 0;
+
+	isc = int_word_to_isc(inti->io.io_int_word);
+
+	if (kvm->arch.gisa && inti->type & KVM_S390_INT_IO_AI_MASK) {
+		VM_EVENT(kvm, 4, "%s isc %1u", "inject: I/O (AI/gisa)", isc);
+		kvm_s390_gisa_set_ipm_gisc(kvm->arch.gisa, isc);
+		kfree(inti);
+		goto out;
+	}
 
 	fi = &kvm->arch.float_int;
 	spin_lock(&fi->lock);
 	if (fi->counters[FIRQ_CNTR_IO] >= KVM_S390_MAX_FLOAT_IRQS) {
 		spin_unlock(&fi->lock);
-		return -EBUSY;
+		rc = -EBUSY;
+		goto out;
 	}
 	fi->counters[FIRQ_CNTR_IO] += 1;
 
@@ -1553,12 +1593,12 @@ static int __inject_io(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
 			inti->io.subchannel_id >> 8,
 			inti->io.subchannel_id >> 1 & 0x3,
 			inti->io.subchannel_nr);
-	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(isc_to_irq_type(isc), &fi->pending_irqs);
 	spin_unlock(&fi->lock);
-	return 0;
+out:
+	return rc;
 }
 
 /*
@@ -2705,3 +2745,29 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu, __u8 __user *buf, int len)
 	return n;
 }
 
+void kvm_s390_gisa_clear(struct kvm *kvm)
+{
+	if (kvm->arch.gisa) {
+		memset(kvm->arch.gisa, 0, sizeof(struct kvm_s390_gisa));
+		kvm->arch.gisa->next_alert = (u32)(u64)kvm->arch.gisa;
+		VM_EVENT(kvm, 3, "gisa 0x%pK cleared", kvm->arch.gisa);
+	}
+}
+
+void kvm_s390_gisa_init(struct kvm *kvm)
+{
+	if (1 || !css_general_characteristics.aiv)
+		kvm->arch.gisa = NULL;
+	else {
+		kvm->arch.gisa = &kvm->arch.sie_page2->gisa;
+		VM_EVENT(kvm, 3, "gisa 0x%pK initialized", kvm->arch.gisa);
+		kvm_s390_gisa_clear(kvm);
+	}
+}
+
+void kvm_s390_gisa_destroy(struct kvm *kvm)
+{
+	if (!kvm->arch.gisa)
+		return;
+	kvm->arch.gisa = NULL;
+}
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 48f0099..68d7eef 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -1986,6 +1986,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 
 	spin_lock_init(&kvm->arch.start_stop_lock);
 	kvm_s390_vsie_init(kvm);
+	kvm_s390_gisa_init(kvm);
 	KVM_EVENT(3, "vm 0x%pK created by pid %u", kvm, current->pid);
 
 	return 0;
@@ -2048,6 +2049,7 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
 	kvm_free_vcpus(kvm);
 	sca_dispose(kvm);
 	debug_unregister(kvm->arch.dbf);
+	kvm_s390_gisa_destroy(kvm);
 	free_page((unsigned long)kvm->arch.sie_page2);
 	if (!kvm_is_ucontrol(kvm))
 		gmap_remove(kvm->arch.gmap);
@@ -2458,6 +2460,11 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
 	if (test_kvm_facility(vcpu->kvm, 139))
 		vcpu->arch.sie_block->ecd |= ECD_MEF;
 
+	if (vcpu->arch.sie_block->gd) {
+		vcpu->arch.sie_block->eca |= ECA_AIV;
+		VCPU_EVENT(vcpu, 3, "AIV gisa format-%u enabled for cpu %03u",
+			   vcpu->arch.sie_block->gd & 0x3, vcpu->vcpu_id);
+	}
 	vcpu->arch.sie_block->sdnxo = ((unsigned long) &vcpu->run->s.regs.sdnx)
 					| SDNXC;
 	vcpu->arch.sie_block->riccbd = (unsigned long) &vcpu->run->s.regs.riccb;
@@ -2510,6 +2517,7 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm,
 
 	vcpu->arch.sie_block->icpua = id;
 	spin_lock_init(&vcpu->arch.local_int.lock);
+	vcpu->arch.sie_block->gd = (u32)(u64)kvm->arch.gisa;
 	seqcount_init(&vcpu->arch.cputm_seqcount);
 
 	rc = kvm_vcpu_init(vcpu, kvm, id);
diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
index 8877116..05269a9 100644
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -367,6 +367,9 @@ int kvm_s390_set_irq_state(struct kvm_vcpu *vcpu,
 			   void __user *buf, int len);
 int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu,
 			   __u8 __user *buf, int len);
+void kvm_s390_gisa_init(struct kvm *kvm);
+void kvm_s390_gisa_clear(struct kvm *kvm);
+void kvm_s390_gisa_destroy(struct kvm *kvm);
 
 /* implemented in guestdbg.c */
 void kvm_s390_backup_guest_per_regs(struct kvm_vcpu *vcpu);
-- 
2.9.4

[PATCH v2 07/12] KVM: s390: abstract adapter interruption word generation from ISC

[Christian Borntraeger]

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

The function isc_to_int_word() allows the generation of interruption
words for adapter interrupts.

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

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 263cd10..7731c0d 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -198,6 +198,11 @@ static uint64_t isc_to_isc_bits(int isc)
 	return (0x80 >> isc) << 24;
 }
 
+static inline u32 isc_to_int_word(u8 isc)
+{
+	return ((u32)isc << 27) | 0x80000000;
+}
+
 static inline u8 int_word_to_isc(u32 int_word)
 {
 	return (int_word & 0x38000000) >> 27;
@@ -991,7 +996,7 @@ static int __must_check __deliver_io(struct kvm_vcpu *vcpu,
 			 */
 			VCPU_EVENT(vcpu, 4, "%s isc %u", "deliver: I/O (AI/gisa)", isc);
 			memset(&io, 0, sizeof(io));
-			io.io_int_word = (isc << 27) | 0x80000000;
+			io.io_int_word = isc_to_int_word(isc);
 			vcpu->stat.deliver_io_int++;
 			trace_kvm_s390_deliver_interrupt(vcpu->vcpu_id,
 				KVM_S390_INT_IO(1, 0, 0, 0),
@@ -2302,7 +2307,7 @@ static int kvm_s390_inject_airq(struct kvm *kvm,
 	struct kvm_s390_interrupt s390int = {
 		.type = KVM_S390_INT_IO(1, 0, 0, 0),
 		.parm = 0,
-		.parm64 = (adapter->isc << 27) | 0x80000000,
+		.parm64 = isc_to_int_word(adapter->isc),
 	};
 	int ret = 0;
 
-- 
2.9.4

[PATCH v2 08/12] KVM: s390: add GISA interrupts to FLIC ioctl interface

[Christian Borntraeger]

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

Pending interrupts marked in the GISA IPM are required to
become part of the answer of ioctl KVM_DEV_FLIC_GET_ALL_IRQS.

The ioctl KVM_DEV_FLIC_ENQUEUE is already capable to enqueue
adapter interrupts when a GISA is present.

With ioctl KVM_DEV_FLIC_CLEAR_IRQS the GISA IPM wil be cleared
now as well.

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, 18 insertions(+)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 7731c0d..0edf97a 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -1882,6 +1882,7 @@ void kvm_s390_clear_float_irqs(struct kvm *kvm)
 	for (i = 0; i < FIRQ_MAX_COUNT; i++)
 		fi->counters[i] = 0;
 	spin_unlock(&fi->lock);
+	kvm_s390_gisa_clear(kvm);
 };
 
 static int get_all_floating_irqs(struct kvm *kvm, u8 __user *usrbuf, u64 len)
@@ -1909,6 +1910,22 @@ static int get_all_floating_irqs(struct kvm *kvm, u8 __user *usrbuf, u64 len)
 
 	max_irqs = len / sizeof(struct kvm_s390_irq);
 
+	if (kvm->arch.gisa &&
+	    kvm_s390_gisa_get_ipm(kvm->arch.gisa)) {
+		for (i = 0; i <= MAX_ISC; i++) {
+			if (n == max_irqs) {
+				/* signal userspace to try again */
+				ret = -ENOMEM;
+				goto out_nolock;
+			}
+			if (kvm_s390_gisa_tac_ipm_gisc(kvm->arch.gisa, i)) {
+				irq = (struct kvm_s390_irq *) &buf[n];
+				irq->type = KVM_S390_INT_IO(1, 0, 0, 0);
+				irq->u.io.io_int_word = isc_to_int_word(i);
+				n++;
+			}
+		}
+	}
 	fi = &kvm->arch.float_int;
 	spin_lock(&fi->lock);
 	for (i = 0; i < FIRQ_LIST_COUNT; i++) {
@@ -1947,6 +1964,7 @@ static int get_all_floating_irqs(struct kvm *kvm, u8 __user *usrbuf, u64 len)
 
 out:
 	spin_unlock(&fi->lock);
+out_nolock:
 	if (!ret && n > 0) {
 		if (copy_to_user(usrbuf, buf, sizeof(struct kvm_s390_irq) * n))
 			ret = -EFAULT;
-- 
2.9.4

[PATCH v2 09/12] KVM: s390: make kvm_s390_get_io_int() aware of GISA

[Christian Borntraeger]

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

The function returns a pending I/O interrupt with the highest
priority defined by its ISC.

Together with AIV activation, pending adapter interrupts are
managed by the GISA IPM. Thus kvm_s390_get_io_int() needs to
inspect the IPM as well when the interrupt with the highest
priority has to be identified.

In case classic and adapter interrupts with the same ISC are
pending, the classic interrupt will be returned first.

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>
---
 arch/s390/kvm/interrupt.c | 71 +++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 65 insertions(+), 6 deletions(-)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 0edf97a..5c48930 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -1471,12 +1471,8 @@ static struct kvm_s390_interrupt_info *get_io_int(struct kvm *kvm,
 	return NULL;
 }
 
-/*
- * Dequeue and return an I/O interrupt matching any of the interruption
- * subclasses as designated by the isc mask in cr6 and the schid (if != 0).
- */
-struct kvm_s390_interrupt_info *kvm_s390_get_io_int(struct kvm *kvm,
-						    u64 isc_mask, u32 schid)
+static struct kvm_s390_interrupt_info *get_top_io_int(struct kvm *kvm,
+						      u64 isc_mask, u32 schid)
 {
 	struct kvm_s390_interrupt_info *inti = NULL;
 	int isc;
@@ -1488,6 +1484,69 @@ struct kvm_s390_interrupt_info *kvm_s390_get_io_int(struct kvm *kvm,
 	return inti;
 }
 
+static int get_top_gisa_isc(struct kvm *kvm, u64 isc_mask, u32 schid)
+{
+	unsigned long active_mask;
+	int isc;
+
+	if (schid)
+		goto out;
+	if (!kvm->arch.gisa)
+		goto out;
+
+	active_mask = (isc_mask & kvm_s390_gisa_get_ipm(kvm->arch.gisa) << 24) << 32;
+	while (active_mask) {
+		isc = __flogr(active_mask);
+		if (kvm_s390_gisa_tac_ipm_gisc(kvm->arch.gisa, isc))
+			return isc;
+		clear_bit_inv(isc, &active_mask);
+	}
+out:
+	return -EINVAL;
+}
+
+/*
+ * Dequeue and return an I/O interrupt matching any of the interruption
+ * subclasses as designated by the isc mask in cr6 and the schid (if != 0).
+ * Take into account the interrupts pending in the interrupt list and in GISA.
+ */
+struct kvm_s390_interrupt_info *kvm_s390_get_io_int(struct kvm *kvm,
+						    u64 isc_mask, u32 schid)
+{
+	struct kvm_s390_interrupt_info *inti, *tmp_inti;
+	int isc;
+
+	inti = get_top_io_int(kvm, isc_mask, schid);
+
+	isc = get_top_gisa_isc(kvm, isc_mask, schid);
+	if (isc < 0)
+		/* no AI in GISA */
+		goto out;
+
+	if (!inti)
+		/* AI in GISA but no classical IO int */
+		goto gisa_out;
+
+	/* both types of interrupts present */
+	if (int_word_to_isc(inti->io.io_int_word) <= isc) {
+		/* classical IO int with higher priority */
+		kvm_s390_gisa_set_ipm_gisc(kvm->arch.gisa, isc);
+		goto out;
+	}
+gisa_out:
+	tmp_inti = kzalloc(sizeof(*inti), GFP_KERNEL);
+	if (tmp_inti) {
+		tmp_inti->type = KVM_S390_INT_IO(1, 0, 0, 0);
+		tmp_inti->io.io_int_word = isc_to_int_word(isc);
+		if (inti)
+			kvm_s390_reinject_io_int(kvm, inti);
+		inti = tmp_inti;
+	} else
+		kvm_s390_gisa_set_ipm_gisc(kvm->arch.gisa, isc);
+out:
+	return inti;
+}
+
 #define SCCB_MASK 0xFFFFFFF8
 #define SCCB_EVENT_PENDING 0x3
 
-- 
2.9.4

[PATCH v2 10/12] KVM: s390: activate GISA for emulated interrupts

[Christian Borntraeger]

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

If the AIV facility is available, a GISA will be used to manage emulated
adapter interrupts.

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>
---
 arch/s390/kvm/interrupt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 5c48930..008019f 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -2838,7 +2838,7 @@ void kvm_s390_gisa_clear(struct kvm *kvm)
 
 void kvm_s390_gisa_init(struct kvm *kvm)
 {
-	if (1 || !css_general_characteristics.aiv)
+	if (!css_general_characteristics.aiv)
 		kvm->arch.gisa = NULL;
 	else {
 		kvm->arch.gisa = &kvm->arch.sie_page2->gisa;
-- 
2.9.4

[PATCH v2 11/12] s390/sclp: expose the GISA format facility

[Christian Borntraeger]

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

The GISA format facility is required by the host to be able to process
a format-1 GISA. If not available, the used GISA format will be format-0.
All format-1 related extension will not be available in this case.

Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Acked-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/include/asm/sclp.h   | 1 +
 drivers/s390/char/sclp_early.c | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/s390/include/asm/sclp.h b/arch/s390/include/asm/sclp.h
index d3c1a8a..3cae916 100644
--- a/arch/s390/include/asm/sclp.h
+++ b/arch/s390/include/asm/sclp.h
@@ -77,6 +77,7 @@ struct sclp_info {
 	unsigned char has_ibs : 1;
 	unsigned char has_skey : 1;
 	unsigned char has_kss : 1;
+	unsigned char has_gisaf : 1;
 	unsigned int ibc;
 	unsigned int mtid;
 	unsigned int mtid_cp;
diff --git a/drivers/s390/char/sclp_early.c b/drivers/s390/char/sclp_early.c
index d06bc56..6b18915 100644
--- a/drivers/s390/char/sclp_early.c
+++ b/drivers/s390/char/sclp_early.c
@@ -49,7 +49,7 @@ struct read_info_sccb {
 	u8	_pad_112[116 - 112];	/* 112-115 */
 	u8	fac116;			/* 116 */
 	u8	fac117;			/* 117 */
-	u8	_pad_118;		/* 118 */
+	u8	fac118;			/* 118 */
 	u8	fac119;			/* 119 */
 	u16	hcpua;			/* 120-121 */
 	u8	_pad_122[124 - 122];	/* 122-123 */
@@ -100,6 +100,7 @@ static void __init sclp_early_facilities_detect(struct read_info_sccb *sccb)
 	sclp.has_esca = !!(sccb->fac116 & 0x08);
 	sclp.has_pfmfi = !!(sccb->fac117 & 0x40);
 	sclp.has_ibs = !!(sccb->fac117 & 0x20);
+	sclp.has_gisaf = !!(sccb->fac118 & 0x08);
 	sclp.has_hvs = !!(sccb->fac119 & 0x80);
 	sclp.has_kss = !!(sccb->fac98 & 0x01);
 	if (sccb->fac85 & 0x02)
-- 
2.9.4

[PATCH v2 12/12] KVM: s390: introduce the format-1 GISA

[Christian Borntraeger]

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

The patch modifies the previously defined GISA data structure to be
able to store two GISA formats, format-0 and format-1. Additionally,
it verifies the availability of the GISA format facility and enables
the use of a format-1 GISA in the SIE control block accordingly.

Signed-off-by: Michael Mueller <mimu@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/include/asm/kvm_host.h | 44 +++++++++++++++++++++++++++++++---------
 arch/s390/kvm/kvm-s390.c         |  2 ++
 2 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 6802d5d..287c99b 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -228,6 +228,7 @@ struct kvm_s390_sie_block {
 	__u8    epdx;			/* 0x0069 */
 	__u8    reserved6a[2];		/* 0x006a */
 	__u32	todpr;			/* 0x006c */
+#define GISA_FORMAT1 0x00000001
 	__u32	gd;			/* 0x0070 */
 	__u8	reserved74[12];		/* 0x0074 */
 	__u64	mso;			/* 0x0080 */
@@ -706,15 +707,38 @@ struct kvm_s390_crypto_cb {
 };
 
 struct kvm_s390_gisa {
-	u32 next_alert;
-	u8 ipm;
-	u8 reserved01;
-	u8 : 6;
-	u8 g : 1;
-	u8 c : 1;
-	u8 iam;
-	u8 reserved02[4];
-	u32 airq_count;
+	union {
+		struct { /* common to all formats */
+			u32 next_alert;
+			u8  ipm;
+			u8  reserved01[2];
+			u8  iam;
+		};
+		struct { /* format 0 */
+			u32 next_alert;
+			u8  ipm;
+			u8  reserved01;
+			u8  : 6;
+			u8  g : 1;
+			u8  c : 1;
+			u8  iam;
+			u8  reserved02[4];
+			u32 airq_count;
+		} g0;
+		struct { /* format 1 */
+			u32 next_alert;
+			u8  ipm;
+			u8  simm;
+			u8  nimm;
+			u8  iam;
+			u8  aism[8];
+			u8  : 6;
+			u8  g : 1;
+			u8  c : 1;
+			u8  reserved03[11];
+			u32 airq_count;
+		} g1;
+	};
 };
 
 /*
@@ -725,7 +749,7 @@ struct sie_page2 {
 	__u64 fac_list[S390_ARCH_FAC_LIST_SIZE_U64];	/* 0x0000 */
 	struct kvm_s390_crypto_cb crycb;		/* 0x0800 */
 	struct kvm_s390_gisa gisa;			/* 0x0900 */
-	u8 reserved910[0x1000 - 0x910];			/* 0x0910 */
+	u8 reserved920[0x1000 - 0x920];			/* 0x0920 */
 };
 
 struct kvm_s390_vsie {
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 68d7eef..efde264 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2518,6 +2518,8 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm,
 	vcpu->arch.sie_block->icpua = id;
 	spin_lock_init(&vcpu->arch.local_int.lock);
 	vcpu->arch.sie_block->gd = (u32)(u64)kvm->arch.gisa;
+	if (vcpu->arch.sie_block->gd && sclp.has_gisaf)
+		vcpu->arch.sie_block->gd |= GISA_FORMAT1;
 	seqcount_init(&vcpu->arch.cputm_seqcount);
 
 	rc = kvm_vcpu_init(vcpu, kvm, id);
-- 
2.9.4

Re: [PATCH v2 03/12] s390/bitops: add test_and_clear_bit_inv()

[Martin Schwidefsky]

On Thu, 25 Jan 2018 14:28:39 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> From: Jens Freimann <jfrei@linux.vnet.ibm.com>
> 
> This patch adds a MSB0 bit numbering version of test_and_clear_bit().
> 
> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
> Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/include/asm/bitops.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/s390/include/asm/bitops.h b/arch/s390/include/asm/bitops.h
> index 31e400c..86e5b2f 100644
> --- a/arch/s390/include/asm/bitops.h
> +++ b/arch/s390/include/asm/bitops.h
> @@ -261,6 +261,11 @@ static inline void clear_bit_inv(unsigned long nr, volatile unsigned long *ptr)
>  	return clear_bit(nr ^ (BITS_PER_LONG - 1), ptr);
>  }
> 
> +static inline int test_and_clear_bit_inv(unsigned long nr, volatile unsigned long *ptr)
> +{
> +	return test_and_clear_bit(nr ^ (BITS_PER_LONG - 1), ptr);
> +}
> +
>  static inline void __set_bit_inv(unsigned long nr, volatile unsigned long *ptr)
>  {
>  	return __set_bit(nr ^ (BITS_PER_LONG - 1), ptr);

Acked-by: Martin Schwidefsky <schwidefsky@de.ibm.com>

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [PATCH v2 05/12] s390/css: indicate the availability of the AIV facility

[Martin Schwidefsky]

On Thu, 25 Jan 2018 14:28:41 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> From: Michael Mueller <mimu@linux.vnet.ibm.com>
> 
> The patch adds an indication for the presence Adapter Interruption
> Virtualization facility (AIV) of the general channel subsystem
> characteristics.
> 
> 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>
> Acked-by: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> [change wording]
> ---
>  arch/s390/include/asm/css_chars.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390/include/asm/css_chars.h b/arch/s390/include/asm/css_chars.h
> index a478eb6..fb56fa3 100644
> --- a/arch/s390/include/asm/css_chars.h
> +++ b/arch/s390/include/asm/css_chars.h
> @@ -20,7 +20,9 @@ struct css_general_char {
>  	u32 aif_tdd : 1; /* bit 56 */
>  	u32 : 1;
>  	u32 qebsm : 1;	 /* bit 58 */
> -	u32 : 8;
> +	u32 : 2;
> +	u32 aiv : 1;     /* bit 61 */
> +	u32 : 5;
>  	u32 aif_osa : 1; /* bit 67 */
>  	u32 : 12;
>  	u32 eadm_rf : 1; /* bit 80 */

Acked-by: Martin Schwidefsky <schwidefsky@de.ibm.com>

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [PATCH v2 11/12] s390/sclp: expose the GISA format facility

[Martin Schwidefsky]

On Thu, 25 Jan 2018 14:28:47 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> From: Michael Mueller <mimu@linux.vnet.ibm.com>
> 
> The GISA format facility is required by the host to be able to process
> a format-1 GISA. If not available, the used GISA format will be format-0.
> All format-1 related extension will not be available in this case.
> 
> Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Acked-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/include/asm/sclp.h   | 1 +
>  drivers/s390/char/sclp_early.c | 3 ++-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390/include/asm/sclp.h b/arch/s390/include/asm/sclp.h
> index d3c1a8a..3cae916 100644
> --- a/arch/s390/include/asm/sclp.h
> +++ b/arch/s390/include/asm/sclp.h
> @@ -77,6 +77,7 @@ struct sclp_info {
>  	unsigned char has_ibs : 1;
>  	unsigned char has_skey : 1;
>  	unsigned char has_kss : 1;
> +	unsigned char has_gisaf : 1;
>  	unsigned int ibc;
>  	unsigned int mtid;
>  	unsigned int mtid_cp;
> diff --git a/drivers/s390/char/sclp_early.c b/drivers/s390/char/sclp_early.c
> index d06bc56..6b18915 100644
> --- a/drivers/s390/char/sclp_early.c
> +++ b/drivers/s390/char/sclp_early.c
> @@ -49,7 +49,7 @@ struct read_info_sccb {
>  	u8	_pad_112[116 - 112];	/* 112-115 */
>  	u8	fac116;			/* 116 */
>  	u8	fac117;			/* 117 */
> -	u8	_pad_118;		/* 118 */
> +	u8	fac118;			/* 118 */
>  	u8	fac119;			/* 119 */
>  	u16	hcpua;			/* 120-121 */
>  	u8	_pad_122[124 - 122];	/* 122-123 */
> @@ -100,6 +100,7 @@ static void __init sclp_early_facilities_detect(struct read_info_sccb *sccb)
>  	sclp.has_esca = !!(sccb->fac116 & 0x08);
>  	sclp.has_pfmfi = !!(sccb->fac117 & 0x40);
>  	sclp.has_ibs = !!(sccb->fac117 & 0x20);
> +	sclp.has_gisaf = !!(sccb->fac118 & 0x08);
>  	sclp.has_hvs = !!(sccb->fac119 & 0x80);
>  	sclp.has_kss = !!(sccb->fac98 & 0x01);
>  	if (sccb->fac85 & 0x02)

Acked-by: Martin Schwidefsky <schwidefsky@de.ibm.com>

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [PATCH v2 05/12] s390/css: indicate the availability of the AIV facility

[David Hildenbrand]

On 25.01.2018 14:28, Christian Borntraeger wrote:
> From: Michael Mueller <mimu@linux.vnet.ibm.com>
> 
> The patch adds an indication for the presence Adapter Interruption
> Virtualization facility (AIV) of the general channel subsystem
> characteristics.
> 
> 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>
> Acked-by: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> [change wording]
> ---
>  arch/s390/include/asm/css_chars.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390/include/asm/css_chars.h b/arch/s390/include/asm/css_chars.h
> index a478eb6..fb56fa3 100644
> --- a/arch/s390/include/asm/css_chars.h
> +++ b/arch/s390/include/asm/css_chars.h
> @@ -20,7 +20,9 @@ struct css_general_char {
>  	u32 aif_tdd : 1; /* bit 56 */
>  	u32 : 1;
>  	u32 qebsm : 1;	 /* bit 58 */
> -	u32 : 8;
> +	u32 : 2;
> +	u32 aiv : 1;     /* bit 61 */
> +	u32 : 5;
>  	u32 aif_osa : 1; /* bit 67 */
>  	u32 : 12;
>  	u32 eadm_rf : 1; /* bit 80 */
> 

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

-- 

Thanks,

David / dhildenb

Re: [PATCH v2 01/12] KVM: s390: reverse bit ordering of irqs in pending mask

[David Hildenbrand]

On 25.01.2018 14:28, Christian Borntraeger wrote:
> From: Michael Mueller <mimu@linux.vnet.ibm.com>
> 
> This patch prepares a simplification of bit operations between the irq
> pending mask for emulated interrupts and the Interruption Pending Mask
> (IPM) which is part of the Guest Interruption State Area (GISA), a feature
> that allows interrupt delivery to guests by means of the SIE instruction.
> 
> Without that change, a bit-wise *or* operation on parts of these two masks
> would either require a look-up table of size 256 bytes to map the IPM
> to the emulated irq pending mask bit orientation (all bits mirrored at half
> byte) or a sequence of up to 8 condidional branches to perform tests of
> single bit positions. Both options are to be rejected either by performance
> or space utilization reasons.
> 
> Beyond that this change will be transparent.
> 
> 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>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/include/asm/kvm_host.h | 54 ++++++++++++++++++++--------------------
>  arch/s390/kvm/interrupt.c        | 12 ++++-----
>  2 files changed, 33 insertions(+), 33 deletions(-)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index e16a9f2..9981721 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -409,35 +409,35 @@ struct kvm_vcpu_stat {
>  #define PGM_PER				0x80
>  #define PGM_CRYPTO_OPERATION		0x119
>  
> -/* irq types in order of priority */
> +/* irq types in ascend order of priorities */
>  enum irq_types {
> -	IRQ_PEND_MCHK_EX = 0,
> -	IRQ_PEND_SVC,
> -	IRQ_PEND_PROG,
> -	IRQ_PEND_MCHK_REP,
> -	IRQ_PEND_EXT_IRQ_KEY,
> -	IRQ_PEND_EXT_MALFUNC,
> -	IRQ_PEND_EXT_EMERGENCY,
> -	IRQ_PEND_EXT_EXTERNAL,
> -	IRQ_PEND_EXT_CLOCK_COMP,
> -	IRQ_PEND_EXT_CPU_TIMER,
> -	IRQ_PEND_EXT_TIMING,
> -	IRQ_PEND_EXT_SERVICE,
> -	IRQ_PEND_EXT_HOST,
> -	IRQ_PEND_PFAULT_INIT,
> -	IRQ_PEND_PFAULT_DONE,
> -	IRQ_PEND_VIRTIO,
> -	IRQ_PEND_IO_ISC_0,
> -	IRQ_PEND_IO_ISC_1,
> -	IRQ_PEND_IO_ISC_2,
> -	IRQ_PEND_IO_ISC_3,
> -	IRQ_PEND_IO_ISC_4,
> -	IRQ_PEND_IO_ISC_5,
> -	IRQ_PEND_IO_ISC_6,
> -	IRQ_PEND_IO_ISC_7,
> -	IRQ_PEND_SIGP_STOP,
> +	IRQ_PEND_SET_PREFIX = 0,
>  	IRQ_PEND_RESTART,
> -	IRQ_PEND_SET_PREFIX,
> +	IRQ_PEND_SIGP_STOP,
> +	IRQ_PEND_IO_ISC_7,
> +	IRQ_PEND_IO_ISC_6,
> +	IRQ_PEND_IO_ISC_5,
> +	IRQ_PEND_IO_ISC_4,
> +	IRQ_PEND_IO_ISC_3,
> +	IRQ_PEND_IO_ISC_2,
> +	IRQ_PEND_IO_ISC_1,
> +	IRQ_PEND_IO_ISC_0,
> +	IRQ_PEND_VIRTIO,
> +	IRQ_PEND_PFAULT_DONE,
> +	IRQ_PEND_PFAULT_INIT,
> +	IRQ_PEND_EXT_HOST,
> +	IRQ_PEND_EXT_SERVICE,
> +	IRQ_PEND_EXT_TIMING,
> +	IRQ_PEND_EXT_CPU_TIMER,
> +	IRQ_PEND_EXT_CLOCK_COMP,
> +	IRQ_PEND_EXT_EXTERNAL,
> +	IRQ_PEND_EXT_EMERGENCY,
> +	IRQ_PEND_EXT_MALFUNC,
> +	IRQ_PEND_EXT_IRQ_KEY,
> +	IRQ_PEND_MCHK_REP,
> +	IRQ_PEND_PROG,
> +	IRQ_PEND_SVC,
> +	IRQ_PEND_MCHK_EX,
>  	IRQ_PEND_COUNT
>  };
>  
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index f8eb2cf..b7b2865 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -189,8 +189,8 @@ static int cpu_timer_irq_pending(struct kvm_vcpu *vcpu)
>  
>  static inline int is_ioirq(unsigned long irq_type)
>  {
> -	return ((irq_type >= IRQ_PEND_IO_ISC_0) &&
> -		(irq_type <= IRQ_PEND_IO_ISC_7));
> +	return ((irq_type >= IRQ_PEND_IO_ISC_7) &&
> +		(irq_type <= IRQ_PEND_IO_ISC_0));
>  }
>  
>  static uint64_t isc_to_isc_bits(int isc)
> @@ -211,12 +211,12 @@ static inline unsigned long pending_irqs(struct kvm_vcpu *vcpu)
>  
>  static inline int isc_to_irq_type(unsigned long isc)
>  {
> -	return IRQ_PEND_IO_ISC_0 + 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;
> +	return IRQ_PEND_IO_ISC_0 - irq_type;
>  }
>  
>  static unsigned long disable_iscs(struct kvm_vcpu *vcpu,
> @@ -1154,8 +1154,8 @@ int __must_check kvm_s390_deliver_pending_interrupts(struct kvm_vcpu *vcpu)
>  		set_bit(IRQ_PEND_EXT_CPU_TIMER, &li->pending_irqs);
>  
>  	while ((irqs = deliverable_irqs(vcpu)) && !rc) {
> -		/* bits are in the order of interrupt priority */
> -		irq_type = find_first_bit(&irqs, IRQ_PEND_COUNT);
> +		/* bits are in the reverse order of interrupt priority */
> +		irq_type = find_last_bit(&irqs, IRQ_PEND_COUNT);
>  		if (is_ioirq(irq_type)) {
>  			rc = __deliver_io(vcpu, irq_type);
>  		} else {
> 

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

-- 

Thanks,

David / dhildenb

Re: [PATCH v2 02/12] KVM: s390: define GISA format-0 data structure

[David Hildenbrand]

On 25.01.2018 14:28, Christian Borntraeger wrote:
> From: Michael Mueller <mimu@linux.vnet.ibm.com>
> 
> In preperation to support pass-through adapter interrupts, the Guest
> Interruption State Area (GISA) and the Adapter Interruption Virtualization
> (AIV) features will be introduced here.
> 
> This patch introduces format-0 GISA (that is defines the struct describing
> the GISA, allocates storage for it, and introduces fields for the
> GISA address in kvm_s390_sie_block and kvm_s390_vsie).
> 
> As the GISA requires storage below 2GB, it is put in sie_page2, which is
> already allocated in ZONE_DMA. In addition, The GISA requires alignment to
> its integral boundary. This is already naturally aligned via the
> padding in the sie_page2.
> 
> Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
> Reviewed-by: Pierre Morel <pmorel@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>
> ---
>  arch/s390/include/asm/kvm_host.h | 23 +++++++++++++++++++----
>  arch/s390/kvm/kvm-s390.c         |  1 +
>  2 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index 9981721..77acade 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -183,6 +183,7 @@ struct kvm_s390_sie_block {
>  #define ECA_IB		0x40000000
>  #define ECA_SIGPI	0x10000000
>  #define ECA_MVPGI	0x01000000
> +#define ECA_AIV		0x00200000
>  #define ECA_VX		0x00020000
>  #define ECA_PROTEXCI	0x00002000
>  #define ECA_SII		0x00000001
> @@ -227,7 +228,8 @@ struct kvm_s390_sie_block {
>  	__u8    epdx;			/* 0x0069 */
>  	__u8    reserved6a[2];		/* 0x006a */
>  	__u32	todpr;			/* 0x006c */
> -	__u8	reserved70[16];		/* 0x0070 */
> +	__u32	gd;			/* 0x0070 */
> +	__u8	reserved74[12];		/* 0x0074 */
>  	__u64	mso;			/* 0x0080 */
>  	__u64	msl;			/* 0x0088 */
>  	psw_t	gpsw;			/* 0x0090 */
> @@ -703,14 +705,27 @@ struct kvm_s390_crypto_cb {
>  	struct kvm_s390_apcb1 apcb1;		/* 0x0080 */
>  };
>  
> +struct kvm_s390_gisa {
> +	u32 next_alert;
> +	u8 ipm;
> +	u8 reserved01;
> +	u8 : 6;
> +	u8 g : 1;
> +	u8 c : 1;
> +	u8 iam;
> +	u8 reserved02[4];
> +	u32 airq_count;
> +};
> +
>  /*
> - * sie_page2 has to be allocated as DMA because fac_list and crycb need
> - * 31bit addresses in the sie control block.
> + * sie_page2 has to be allocated as DMA because fac_list, crycb and
> + * gisa need 31bit addresses in the sie control block.
>   */
>  struct sie_page2 {
>  	__u64 fac_list[S390_ARCH_FAC_LIST_SIZE_U64];	/* 0x0000 */
>  	struct kvm_s390_crypto_cb crycb;		/* 0x0800 */
> -	u8 reserved900[0x1000 - 0x900];			/* 0x0900 */
> +	struct kvm_s390_gisa gisa;			/* 0x0900 */
> +	u8 reserved910[0x1000 - 0x910];			/* 0x0910 */
>  };
>  
>  struct kvm_s390_vsie {
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index de16c22..48f0099 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -1915,6 +1915,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>  	if (!kvm->arch.dbf)
>  		goto out_err;
>  
> +	BUILD_BUG_ON(sizeof(struct sie_page2) != 4096);
>  	kvm->arch.sie_page2 =
>  	     (struct sie_page2 *) get_zeroed_page(GFP_KERNEL | GFP_DMA);
>  	if (!kvm->arch.sie_page2)
> 

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

-- 

Thanks,

David / dhildenb

Re: [PATCH v2 04/12] KVM: s390: implement GISA IPM related primitives

[David Hildenbrand]

On 25.01.2018 14:28, Christian Borntraeger wrote:
> From: Michael Mueller <mimu@linux.vnet.ibm.com>
> 
> The patch implements routines to access the GISA to test and modify
> its Interruption Pending Mask (IPM) from the host side.
> 
> Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
> Reviewed-by: Pierre Morel <pmorel@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>
> ---
>  arch/s390/kvm/interrupt.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index b7b2865..d559776 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -203,6 +203,28 @@ static inline u8 int_word_to_isc(u32 int_word)
>  	return (int_word & 0x38000000) >> 27;
>  }
>  

Can we add a comment like

/*
 * To use atomic bitmap functions, we have to provide a bitmap address
 * that is u64 aligned. However, the ipm might be u32 aligned.
 * Therefore, we logically start the bitmap at the very beginning of the
 * struct and fixup the bit number.
 */

> +#define IPM_BIT_OFFSET (offsetof(struct kvm_s390_gisa, ipm) * BITS_PER_BYTE)
> +
> +static inline void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
> +{
> +	set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
> +}
> +
> +static inline u8 kvm_s390_gisa_get_ipm(struct kvm_s390_gisa *gisa)
> +{
> +	return (u8) READ_ONCE(gisa->ipm);

Is this cast necessary? (ipm is defined as u8)

> +}
> +
> +static inline void kvm_s390_gisa_clear_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)

wonder if an u8 for gisc would be better.

> +{
> +	clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
> +}
> +
> +static inline int kvm_s390_gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
> +{
> +	return test_and_clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
> +}
> +
>  static inline unsigned long pending_irqs(struct kvm_vcpu *vcpu)
>  {
>  	return vcpu->kvm->arch.float_int.pending_irqs |
> @@ -2682,3 +2704,4 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu, __u8 __user *buf, int len)
>  
>  	return n;
>  }
> +
> 

Unrelated whitespace change.

-- 

Thanks,

David / dhildenb

Re: [PATCH v2 06/12] KVM: s390: exploit GISA and AIV for emulated interrupts

[David Hildenbrand]

On 25.01.2018 14:28, Christian Borntraeger wrote:
> From: Michael Mueller <mimu@linux.vnet.ibm.com>
> 
> The adapter interruption virtualization (AIV) facility is an
> optional facility that comes with functionality expected to increase
> the performance of adapter interrupt handling for both emulated and
> passed-through adapter interrupts. With AIV, adapter interrupts can be
> delivered to the guest without exiting SIE.
> 
> This patch provides some preparations for using AIV for emulated adapter
> interrupts (inclusive virtio) if it's available. When using AIV, the
> interrupts are delivered at the so called GISA by setting the bit
> corresponding to its Interruption Subclass (ISC) in the Interruption
> Pending Mask (IPM) instead of inserting a node into the floating interrupt
> list.
> 
> To keep the change reasonably small, the handling of this new state is
> deferred in get_all_floating_irqs and handle_tpi. This patch concentrates
> on the code handling enqueuement of emulated adapter interrupts, and their
> delivery to the guest.
> 
> Note that care is still required for adapter interrupts using AIV,
> because there is no guarantee that AIV is going to deliver the adapter
> interrupts pending at the GISA (consider all vcpus idle). When delivering
> GISA adapter interrupts by the host (usual mechanism) special attention
> is required to honor interrupt priorities.
> 
> Empirical results show that the time window between making an interrupt
> pending at the GISA and doing kvm_s390_deliver_pending_interrupts is
> sufficient for a guest with at least moderate cpu activity to get adapter
> interrupts delivered within the SIE, and potentially save some SIE exits
> (if not other deliverable interrupts).
> 
> The code will be activated with a follow-up patch.
> 
> Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/include/asm/kvm_host.h |   1 +
>  arch/s390/kvm/interrupt.c        | 106 +++++++++++++++++++++++++++++++--------
>  arch/s390/kvm/kvm-s390.c         |   8 +++
>  arch/s390/kvm/kvm-s390.h         |   3 ++
>  4 files changed, 98 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index 77acade..6802d5d 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -772,6 +772,7 @@ struct kvm_arch{
>  	struct kvm_s390_migration_state *migration_state;
>  	/* subset of available cpu features enabled by user space */
>  	DECLARE_BITMAP(cpu_feat, KVM_S390_VM_CPU_FEAT_NR_BITS);
> +	struct kvm_s390_gisa *gisa;
>  };
>  
>  #define KVM_HVA_ERR_BAD		(-1UL)
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index d559776..263cd10 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -228,7 +228,8 @@ static inline int kvm_s390_gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gis
>  static inline unsigned long pending_irqs(struct kvm_vcpu *vcpu)
>  {
>  	return vcpu->kvm->arch.float_int.pending_irqs |
> -	       vcpu->arch.local_int.pending_irqs;
> +		vcpu->arch.local_int.pending_irqs |
> +		kvm_s390_gisa_get_ipm(vcpu->kvm->arch.gisa) << IRQ_PEND_IO_ISC_7;
>  }
>  
>  static inline int isc_to_irq_type(unsigned long isc)
> @@ -918,18 +919,38 @@ static int __must_check __deliver_virtio(struct kvm_vcpu *vcpu)
>  	return rc ? -EFAULT : 0;
>  }
>  
> +static int __do_deliver_io(struct kvm_vcpu *vcpu, struct kvm_s390_io_info *io)
> +{
> +	int rc;
> +
> +	rc  = put_guest_lc(vcpu, io->subchannel_id, (u16 *)__LC_SUBCHANNEL_ID);
> +	rc |= put_guest_lc(vcpu, io->subchannel_nr, (u16 *)__LC_SUBCHANNEL_NR);
> +	rc |= put_guest_lc(vcpu, io->io_int_parm, (u32 *)__LC_IO_INT_PARM);
> +	rc |= put_guest_lc(vcpu, io->io_int_word, (u32 *)__LC_IO_INT_WORD);
> +	rc |= write_guest_lc(vcpu, __LC_IO_OLD_PSW,
> +			     &vcpu->arch.sie_block->gpsw,
> +			     sizeof(psw_t));
> +	rc |= read_guest_lc(vcpu, __LC_IO_NEW_PSW,
> +			    &vcpu->arch.sie_block->gpsw,
> +			    sizeof(psw_t));

These should now it into less lines. Can you factor that change out into
a separate patch?

> +	return rc ? -EFAULT : 0;
> +}
> +
>  static int __must_check __deliver_io(struct kvm_vcpu *vcpu,
>  				     unsigned long irq_type)
>  {
>  	struct list_head *isc_list;
>  	struct kvm_s390_float_interrupt *fi;
>  	struct kvm_s390_interrupt_info *inti = NULL;
> +	struct kvm_s390_io_info io;
> +	u32 isc;
>  	int rc = 0;
>  
>  	fi = &vcpu->kvm->arch.float_int;
>  
>  	spin_lock(&fi->lock);
> -	isc_list = &fi->lists[irq_type_to_isc(irq_type)];
> +	isc = irq_type_to_isc(irq_type);
> +	isc_list = &fi->lists[isc];
>  	inti = list_first_entry_or_null(isc_list,
>  					struct kvm_s390_interrupt_info,
>  					list);
> @@ -957,24 +978,32 @@ static int __must_check __deliver_io(struct kvm_vcpu *vcpu,
>  	spin_unlock(&fi->lock);
>  
>  	if (inti) {
> -		rc  = put_guest_lc(vcpu, inti->io.subchannel_id,
> -				(u16 *)__LC_SUBCHANNEL_ID);
> -		rc |= put_guest_lc(vcpu, inti->io.subchannel_nr,
> -				(u16 *)__LC_SUBCHANNEL_NR);
> -		rc |= put_guest_lc(vcpu, inti->io.io_int_parm,
> -				(u32 *)__LC_IO_INT_PARM);
> -		rc |= put_guest_lc(vcpu, inti->io.io_int_word,
> -				(u32 *)__LC_IO_INT_WORD);
> -		rc |= write_guest_lc(vcpu, __LC_IO_OLD_PSW,
> -				&vcpu->arch.sie_block->gpsw,
> -				sizeof(psw_t));
> -		rc |= read_guest_lc(vcpu, __LC_IO_NEW_PSW,
> -				&vcpu->arch.sie_block->gpsw,
> -				sizeof(psw_t));
> +		rc = __do_deliver_io(vcpu, &(inti->io));
>  		kfree(inti);
> +		goto out;
>  	}
>  
> -	return rc ? -EFAULT : 0;
> +	if (vcpu->kvm->arch.gisa) {
> +		if (kvm_s390_gisa_tac_ipm_gisc(vcpu->kvm->arch.gisa, isc)) {


if (vcpu->kvm->arch.gisa &&
    kvm_s390_gisa_tac_ipm_gisc(vcpu->kvm->arch.gisa, isc) {

avoids one nesting level

> +			/*
> +			 * in case an adapter interrupt was not delivered
> +			 * in SIE context KVM will handle the delivery
> +			 */
> +			VCPU_EVENT(vcpu, 4, "%s isc %u", "deliver: I/O (AI/gisa)", isc);
> +			memset(&io, 0, sizeof(io));
> +			io.io_int_word = (isc << 27) | 0x80000000;
> +			vcpu->stat.deliver_io_int++;
> +			trace_kvm_s390_deliver_interrupt(vcpu->vcpu_id,
> +				KVM_S390_INT_IO(1, 0, 0, 0),
> +				((__u32)io.subchannel_id << 16) |
> +				io.subchannel_nr,
> +				((__u64)io.io_int_parm << 32) |
> +				io.io_int_word);
> +			rc = __do_deliver_io(vcpu, &io);
> +		}
> +	}
> +out:
> +	return rc;
>  }
>  
>  typedef int (*deliver_irq_t)(struct kvm_vcpu *vcpu);
> @@ -1537,12 +1566,23 @@ static int __inject_io(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
>  	struct kvm_s390_float_interrupt *fi;
>  	struct list_head *list;
>  	int isc;
> +	int rc = 0;
> +
> +	isc = int_word_to_isc(inti->io.io_int_word);
> +
> +	if (kvm->arch.gisa && inti->type & KVM_S390_INT_IO_AI_MASK) {

&& (inti->type & KVM_S390_INT_IO_AI_MASK)) {

> +		VM_EVENT(kvm, 4, "%s isc %1u", "inject: I/O (AI/gisa)", isc);
> +		kvm_s390_gisa_set_ipm_gisc(kvm->arch.gisa, isc);

Can there only be one pending at a time? (e.g. what happens if the bit
is already set?)

> +		kfree(inti);
> +		goto out;

Why not simply return 0? ...

> +	}
>  
>  	fi = &kvm->arch.float_int;
>  	spin_lock(&fi->lock);
>  	if (fi->counters[FIRQ_CNTR_IO] >= KVM_S390_MAX_FLOAT_IRQS) {
>  		spin_unlock(&fi->lock);
> -		return -EBUSY;
> +		rc = -EBUSY;
> +		goto out;

... avoids this change ...

>  	}
>  	fi->counters[FIRQ_CNTR_IO] += 1;
>  
> @@ -1553,12 +1593,12 @@ static int __inject_io(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
>  			inti->io.subchannel_id >> 8,
>  			inti->io.subchannel_id >> 1 & 0x3,
>  			inti->io.subchannel_nr);
> -	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(isc_to_irq_type(isc), &fi->pending_irqs);
>  	spin_unlock(&fi->lock);
> -	return 0;
> +out:
> +	return rc;

... and this.

>  }
>  
>  /*
> @@ -2705,3 +2745,29 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu, __u8 __user *buf, int len)
>  	return n;
>  }
>  
> +void kvm_s390_gisa_clear(struct kvm *kvm)

can we instead call this "gisa_setup" or move all that directly into the
init function? (because it essentially inits the gisa)

> +{
> +	if (kvm->arch.gisa) {

This check is not needed: guaranteed to be set by the caller.

> +		memset(kvm->arch.gisa, 0, sizeof(struct kvm_s390_gisa));
> +		kvm->arch.gisa->next_alert = (u32)(u64)kvm->arch.gisa;
> +		VM_EVENT(kvm, 3, "gisa 0x%pK cleared", kvm->arch.gisa);
> +	}
> +}
> +
> +void kvm_s390_gisa_init(struct kvm *kvm)
> +{
> +	if (1 || !css_general_characteristics.aiv)
> +		kvm->arch.gisa = NULL;

1 || ... ? This will always trigger. -> gisa never active with this patch

> +	else {
> +		kvm->arch.gisa = &kvm->arch.sie_page2->gisa;
> +		VM_EVENT(kvm, 3, "gisa 0x%pK initialized", kvm->arch.gisa);
> +		kvm_s390_gisa_clear(kvm);
> +	}
> +}
> +
> +void kvm_s390_gisa_destroy(struct kvm *kvm)
> +{
> +	if (!kvm->arch.gisa)
> +		return;
> +	kvm->arch.gisa = NULL;

You can do this unconditionally. Nevertheless, this function is not
really useful: only called when we destroy the VM.

> +}
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 48f0099..68d7eef 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -1986,6 +1986,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>  
>  	spin_lock_init(&kvm->arch.start_stop_lock);
>  	kvm_s390_vsie_init(kvm);
> +	kvm_s390_gisa_init(kvm);
>  	KVM_EVENT(3, "vm 0x%pK created by pid %u", kvm, current->pid);
>  
>  	return 0;
> @@ -2048,6 +2049,7 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>  	kvm_free_vcpus(kvm);
>  	sca_dispose(kvm);
>  	debug_unregister(kvm->arch.dbf);
> +	kvm_s390_gisa_destroy(kvm);
>  	free_page((unsigned long)kvm->arch.sie_page2);
>  	if (!kvm_is_ucontrol(kvm))
>  		gmap_remove(kvm->arch.gmap);
> @@ -2458,6 +2460,11 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
>  	if (test_kvm_facility(vcpu->kvm, 139))
>  		vcpu->arch.sie_block->ecd |= ECD_MEF;
>  
> +	if (vcpu->arch.sie_block->gd) {
> +		vcpu->arch.sie_block->eca |= ECA_AIV;
> +		VCPU_EVENT(vcpu, 3, "AIV gisa format-%u enabled for cpu %03u",
> +			   vcpu->arch.sie_block->gd & 0x3, vcpu->vcpu_id);
> +	}
>  	vcpu->arch.sie_block->sdnxo = ((unsigned long) &vcpu->run->s.regs.sdnx)
>  					| SDNXC;
>  	vcpu->arch.sie_block->riccbd = (unsigned long) &vcpu->run->s.regs.riccb;
> @@ -2510,6 +2517,7 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm,
>  
>  	vcpu->arch.sie_block->icpua = id;
>  	spin_lock_init(&vcpu->arch.local_int.lock);
> +	vcpu->arch.sie_block->gd = (u32)(u64)kvm->arch.gisa;
>  	seqcount_init(&vcpu->arch.cputm_seqcount);
>  
>  	rc = kvm_vcpu_init(vcpu, kvm, id);
> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> index 8877116..05269a9 100644
> --- a/arch/s390/kvm/kvm-s390.h
> +++ b/arch/s390/kvm/kvm-s390.h
> @@ -367,6 +367,9 @@ int kvm_s390_set_irq_state(struct kvm_vcpu *vcpu,
>  			   void __user *buf, int len);
>  int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu,
>  			   __u8 __user *buf, int len);
> +void kvm_s390_gisa_init(struct kvm *kvm);
> +void kvm_s390_gisa_clear(struct kvm *kvm);
> +void kvm_s390_gisa_destroy(struct kvm *kvm);
>  
>  /* implemented in guestdbg.c */
>  void kvm_s390_backup_guest_per_regs(struct kvm_vcpu *vcpu);
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH v2 04/12] KVM: s390: implement GISA IPM related primitives

[Christian Borntraeger]

On 01/25/2018 03:06 PM, David Hildenbrand wrote:
> On 25.01.2018 14:28, Christian Borntraeger wrote:
>> From: Michael Mueller <mimu@linux.vnet.ibm.com>
>>
>> The patch implements routines to access the GISA to test and modify
>> its Interruption Pending Mask (IPM) from the host side.
>>
>> Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
>> Reviewed-by: Pierre Morel <pmorel@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>
>> ---
>>  arch/s390/kvm/interrupt.c | 23 +++++++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>>
>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>> index b7b2865..d559776 100644
>> --- a/arch/s390/kvm/interrupt.c
>> +++ b/arch/s390/kvm/interrupt.c
>> @@ -203,6 +203,28 @@ static inline u8 int_word_to_isc(u32 int_word)
>>  	return (int_word & 0x38000000) >> 27;
>>  }
>>  
> 
> Can we add a comment like
> 
> /*
>  * To use atomic bitmap functions, we have to provide a bitmap address
>  * that is u64 aligned. However, the ipm might be u32 aligned.
>  * Therefore, we logically start the bitmap at the very beginning of the
>  * struct and fixup the bit number.
>  */

Yes, makes sense, will add.

> 
>> +#define IPM_BIT_OFFSET (offsetof(struct kvm_s390_gisa, ipm) * BITS_PER_BYTE)
>> +
>> +static inline void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>> +{
>> +	set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
>> +}
>> +
>> +static inline u8 kvm_s390_gisa_get_ipm(struct kvm_s390_gisa *gisa)
>> +{
>> +	return (u8) READ_ONCE(gisa->ipm);
> 
> Is this cast necessary? (ipm is defined as u8)

I think it can go. will fix.

> 
>> +}
>> +
>> +static inline void kvm_s390_gisa_clear_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
> 
> wonder if an u8 for gisc would be better.

isc is a 32bit value throughout that code, so lets keep it here as well.

> 
>> +{
>> +	clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
>> +}
>> +
>> +static inline int kvm_s390_gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>> +{
>> +	return test_and_clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
>> +}
>> +
>>  static inline unsigned long pending_irqs(struct kvm_vcpu *vcpu)
>>  {
>>  	return vcpu->kvm->arch.float_int.pending_irqs |
>> @@ -2682,3 +2704,4 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu, __u8 __user *buf, int len)
>>  
>>  	return n;
>>  }
>> +
>>
> 
> Unrelated whitespace change.

Will fixup. 

Re: [PATCH v2 06/12] KVM: s390: exploit GISA and AIV for emulated interrupts

[Christian Borntraeger]

On 01/25/2018 03:20 PM, David Hildenbrand wrote:

>> +void kvm_s390_gisa_init(struct kvm *kvm)
>> +{
>> +	if (1 || !css_general_characteristics.aiv)
>> +		kvm->arch.gisa = NULL;
> 
> 1 || ... ? This will always trigger. -> gisa never active with this patch

See patch 10. 

Re: [PATCH v2 06/12] KVM: s390: exploit GISA and AIV for emulated interrupts

[David Hildenbrand]

On 25.01.2018 15:32, Christian Borntraeger wrote:
> 
> 
> On 01/25/2018 03:20 PM, David Hildenbrand wrote:
> 
>>> +void kvm_s390_gisa_init(struct kvm *kvm)
>>> +{
>>> +	if (1 || !css_general_characteristics.aiv)
>>> +		kvm->arch.gisa = NULL;
>>
>> 1 || ... ? This will always trigger. -> gisa never active with this patch
> 
> See patch 10. 
> 

Well than this is just ugly this way.

-- 

Thanks,

David / dhildenb

Re: [PATCH v2 06/12] KVM: s390: exploit GISA and AIV for emulated interrupts

[Christian Borntraeger]

On 01/25/2018 03:42 PM, David Hildenbrand wrote:
> On 25.01.2018 15:32, Christian Borntraeger wrote:
>>
>>
>> On 01/25/2018 03:20 PM, David Hildenbrand wrote:
>>
>>>> +void kvm_s390_gisa_init(struct kvm *kvm)
>>>> +{
>>>> +	if (1 || !css_general_characteristics.aiv)
>>>> +		kvm->arch.gisa = NULL;
>>>
>>> 1 || ... ? This will always trigger. -> gisa never active with this patch
>>
>> See patch 10. 
>>
> 
> Well than this is just ugly this way.

Reshuffling this is hard due to code dependencies. I can try to minimize this.
The alternative is to merge patches 6,7,8,9 and 10, but this split makes it
easier to review the parts. 

Re: [PATCH v2 06/12] KVM: s390: exploit GISA and AIV for emulated interrupts

[Cornelia Huck]

On Thu, 25 Jan 2018 15:45:14 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 01/25/2018 03:42 PM, David Hildenbrand wrote:
> > On 25.01.2018 15:32, Christian Borntraeger wrote:  
> >>
> >>
> >> On 01/25/2018 03:20 PM, David Hildenbrand wrote:
> >>  
> >>>> +void kvm_s390_gisa_init(struct kvm *kvm)
> >>>> +{
> >>>> +	if (1 || !css_general_characteristics.aiv)
> >>>> +		kvm->arch.gisa = NULL;  
> >>>
> >>> 1 || ... ? This will always trigger. -> gisa never active with this patch  
> >>
> >> See patch 10. 
> >>  
> > 
> > Well than this is just ugly this way.  
> 
> Reshuffling this is hard due to code dependencies. I can try to minimize this.
> The alternative is to merge patches 6,7,8,9 and 10, but this split makes it
> easier to review the parts. 
> 

if (1 /* disabled for now */ || !css_general_characteristics.aiv)

?

I dunno, I'm not really bothered by this.

Re: [PATCH v2 06/12] KVM: s390: exploit GISA and AIV for emulated interrupts

[David Hildenbrand]

On 25.01.2018 16:05, Cornelia Huck wrote:
> On Thu, 25 Jan 2018 15:45:14 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 01/25/2018 03:42 PM, David Hildenbrand wrote:
>>> On 25.01.2018 15:32, Christian Borntraeger wrote:  
>>>>
>>>>
>>>> On 01/25/2018 03:20 PM, David Hildenbrand wrote:
>>>>  
>>>>>> +void kvm_s390_gisa_init(struct kvm *kvm)
>>>>>> +{
>>>>>> +	if (1 || !css_general_characteristics.aiv)
>>>>>> +		kvm->arch.gisa = NULL;  
>>>>>
>>>>> 1 || ... ? This will always trigger. -> gisa never active with this patch  
>>>>
>>>> See patch 10. 
>>>>  
>>>
>>> Well than this is just ugly this way.  
>>
>> Reshuffling this is hard due to code dependencies. I can try to minimize this.
>> The alternative is to merge patches 6,7,8,9 and 10, but this split makes it
>> easier to review the parts. 
>>
> 
> if (1 /* disabled for now */ || !css_general_characteristics.aiv)
> 
> ?
> 
> I dunno, I'm not really bothered by this.
> 

Or add the enabling code when you can enable it?

-- 

Thanks,

David / dhildenb

Re: [PATCH v2 06/12] KVM: s390: exploit GISA and AIV for emulated interrupts

[Christian Borntraeger]

On 01/25/2018 04:27 PM, David Hildenbrand wrote:
> On 25.01.2018 16:05, Cornelia Huck wrote:
>> On Thu, 25 Jan 2018 15:45:14 +0100
>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>
>>> On 01/25/2018 03:42 PM, David Hildenbrand wrote:
>>>> On 25.01.2018 15:32, Christian Borntraeger wrote:  
>>>>>
>>>>>
>>>>> On 01/25/2018 03:20 PM, David Hildenbrand wrote:
>>>>>  
>>>>>>> +void kvm_s390_gisa_init(struct kvm *kvm)
>>>>>>> +{
>>>>>>> +	if (1 || !css_general_characteristics.aiv)
>>>>>>> +		kvm->arch.gisa = NULL;  
>>>>>>
>>>>>> 1 || ... ? This will always trigger. -> gisa never active with this patch  
>>>>>
>>>>> See patch 10. 
>>>>>  
>>>>
>>>> Well than this is just ugly this way.  
>>>
>>> Reshuffling this is hard due to code dependencies. I can try to minimize this.
>>> The alternative is to merge patches 6,7,8,9 and 10, but this split makes it
>>> easier to review the parts. 
>>>
>>
>> if (1 /* disabled for now */ || !css_general_characteristics.aiv)
>>
>> ?
>>
>> I dunno, I'm not really bothered by this.
>>
> 
> Or add the enabling code when you can enable it?

I will do that.

Re: [PATCH v2 12/12] KVM: s390: introduce the format-1 GISA

[David Hildenbrand]

On 25.01.2018 14:28, Christian Borntraeger wrote:
> From: Michael Mueller <mimu@linux.vnet.ibm.com>
> 
> The patch modifies the previously defined GISA data structure to be
> able to store two GISA formats, format-0 and format-1. Additionally,
> it verifies the availability of the GISA format facility and enables
> the use of a format-1 GISA in the SIE control block accordingly.
> 
> Signed-off-by: Michael Mueller <mimu@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/include/asm/kvm_host.h | 44 +++++++++++++++++++++++++++++++---------
>  arch/s390/kvm/kvm-s390.c         |  2 ++
>  2 files changed, 36 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index 6802d5d..287c99b 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -228,6 +228,7 @@ struct kvm_s390_sie_block {
>  	__u8    epdx;			/* 0x0069 */
>  	__u8    reserved6a[2];		/* 0x006a */
>  	__u32	todpr;			/* 0x006c */
> +#define GISA_FORMAT1 0x00000001
>  	__u32	gd;			/* 0x0070 */
>  	__u8	reserved74[12];		/* 0x0074 */
>  	__u64	mso;			/* 0x0080 */
> @@ -706,15 +707,38 @@ struct kvm_s390_crypto_cb {
>  };
>  
>  struct kvm_s390_gisa {
> -	u32 next_alert;
> -	u8 ipm;
> -	u8 reserved01;
> -	u8 : 6;
> -	u8 g : 1;
> -	u8 c : 1;
> -	u8 iam;
> -	u8 reserved02[4];
> -	u32 airq_count;
> +	union {
> +		struct { /* common to all formats */
> +			u32 next_alert;
> +			u8  ipm;
> +			u8  reserved01[2];
> +			u8  iam;
> +		};
> +		struct { /* format 0 */
> +			u32 next_alert;
> +			u8  ipm;
> +			u8  reserved01;
> +			u8  : 6;
> +			u8  g : 1;
> +			u8  c : 1;
> +			u8  iam;
> +			u8  reserved02[4];
> +			u32 airq_count;
> +		} g0;
> +		struct { /* format 1 */
> +			u32 next_alert;
> +			u8  ipm;
> +			u8  simm;
> +			u8  nimm;
> +			u8  iam;
> +			u8  aism[8];
> +			u8  : 6;
> +			u8  g : 1;
> +			u8  c : 1;
> +			u8  reserved03[11];
> +			u32 airq_count;
> +		} g1;
> +	};
>  };
>  
>  /*
> @@ -725,7 +749,7 @@ struct sie_page2 {
>  	__u64 fac_list[S390_ARCH_FAC_LIST_SIZE_U64];	/* 0x0000 */
>  	struct kvm_s390_crypto_cb crycb;		/* 0x0800 */
>  	struct kvm_s390_gisa gisa;			/* 0x0900 */
> -	u8 reserved910[0x1000 - 0x910];			/* 0x0910 */
> +	u8 reserved920[0x1000 - 0x920];			/* 0x0920 */
>  };
>  
>  struct kvm_s390_vsie {
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 68d7eef..efde264 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2518,6 +2518,8 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm,
>  	vcpu->arch.sie_block->icpua = id;
>  	spin_lock_init(&vcpu->arch.local_int.lock);
>  	vcpu->arch.sie_block->gd = (u32)(u64)kvm->arch.gisa;
> +	if (vcpu->arch.sie_block->gd && sclp.has_gisaf)
> +		vcpu->arch.sie_block->gd |= GISA_FORMAT1;
>  	seqcount_init(&vcpu->arch.cputm_seqcount);
>  
>  	rc = kvm_vcpu_init(vcpu, kvm, id);
> 

So, what does this bring us? We don't seem to be using any new GISA-1
features.

-- 

Thanks,

David / dhildenb

Re: [PATCH v2 12/12] KVM: s390: introduce the format-1 GISA

[Christian Borntraeger]

On 01/25/2018 04:31 PM, David Hildenbrand wrote:
[...]
>>  struct kvm_s390_vsie {
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index 68d7eef..efde264 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -2518,6 +2518,8 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm,
>>  	vcpu->arch.sie_block->icpua = id;
>>  	spin_lock_init(&vcpu->arch.local_int.lock);
>>  	vcpu->arch.sie_block->gd = (u32)(u64)kvm->arch.gisa;
>> +	if (vcpu->arch.sie_block->gd && sclp.has_gisaf)
>> +		vcpu->arch.sie_block->gd |= GISA_FORMAT1;
>>  	seqcount_init(&vcpu->arch.cputm_seqcount);
>>  
>>  	rc = kvm_vcpu_init(vcpu, kvm, id);
>>
> 
> So, what does this bring us? We don't seem to be using any new GISA-1
> features.

Preparation for device pass-through interrupt forwarding.

Re: [PATCH v2 12/12] KVM: s390: introduce the format-1 GISA

[Cornelia Huck]

On Thu, 25 Jan 2018 16:43:27 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 01/25/2018 04:31 PM, David Hildenbrand wrote:
> [...]
> >>  struct kvm_s390_vsie {
> >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> >> index 68d7eef..efde264 100644
> >> --- a/arch/s390/kvm/kvm-s390.c
> >> +++ b/arch/s390/kvm/kvm-s390.c
> >> @@ -2518,6 +2518,8 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm,
> >>  	vcpu->arch.sie_block->icpua = id;
> >>  	spin_lock_init(&vcpu->arch.local_int.lock);
> >>  	vcpu->arch.sie_block->gd = (u32)(u64)kvm->arch.gisa;
> >> +	if (vcpu->arch.sie_block->gd && sclp.has_gisaf)
> >> +		vcpu->arch.sie_block->gd |= GISA_FORMAT1;
> >>  	seqcount_init(&vcpu->arch.cputm_seqcount);
> >>  
> >>  	rc = kvm_vcpu_init(vcpu, kvm, id);
> >>  
> > 
> > So, what does this bring us? We don't seem to be using any new GISA-1
> > features.  
> 
> Preparation for device pass-through interrupt forwarding.
> 

Should we start out with a dual format-0/format-1 gisa block, then?
IIUC, you'll switch to gisa-1 if the facility is there and gisa-1 can
do anything that gisa-0 can do?

Re: [PATCH v2 12/12] KVM: s390: introduce the format-1 GISA

[Christian Borntraeger]

On 01/25/2018 04:47 PM, Cornelia Huck wrote:
> On Thu, 25 Jan 2018 16:43:27 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 01/25/2018 04:31 PM, David Hildenbrand wrote:
>> [...]
>>>>  struct kvm_s390_vsie {
>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>>> index 68d7eef..efde264 100644
>>>> --- a/arch/s390/kvm/kvm-s390.c
>>>> +++ b/arch/s390/kvm/kvm-s390.c
>>>> @@ -2518,6 +2518,8 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm,
>>>>  	vcpu->arch.sie_block->icpua = id;
>>>>  	spin_lock_init(&vcpu->arch.local_int.lock);
>>>>  	vcpu->arch.sie_block->gd = (u32)(u64)kvm->arch.gisa;
>>>> +	if (vcpu->arch.sie_block->gd && sclp.has_gisaf)
>>>> +		vcpu->arch.sie_block->gd |= GISA_FORMAT1;
>>>>  	seqcount_init(&vcpu->arch.cputm_seqcount);
>>>>  
>>>>  	rc = kvm_vcpu_init(vcpu, kvm, id);
>>>>  
>>>
>>> So, what does this bring us? We don't seem to be using any new GISA-1
>>> features.  
>>
>> Preparation for device pass-through interrupt forwarding.
>>
> 
> Should we start out with a dual format-0/format-1 gisa block, then?
> IIUC, you'll switch to gisa-1 if the facility is there and gisa-1 can
> do anything that gisa-0 can do?

There might be systems that only have gisa-0, so I think having both makes
sense.

Re: [PATCH v2 12/12] KVM: s390: introduce the format-1 GISA

[David Hildenbrand]

On 25.01.2018 16:43, Christian Borntraeger wrote:
> 
> 
> On 01/25/2018 04:31 PM, David Hildenbrand wrote:
> [...]
>>>  struct kvm_s390_vsie {
>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>> index 68d7eef..efde264 100644
>>> --- a/arch/s390/kvm/kvm-s390.c
>>> +++ b/arch/s390/kvm/kvm-s390.c
>>> @@ -2518,6 +2518,8 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm,
>>>  	vcpu->arch.sie_block->icpua = id;
>>>  	spin_lock_init(&vcpu->arch.local_int.lock);
>>>  	vcpu->arch.sie_block->gd = (u32)(u64)kvm->arch.gisa;
>>> +	if (vcpu->arch.sie_block->gd && sclp.has_gisaf)
>>> +		vcpu->arch.sie_block->gd |= GISA_FORMAT1;

wonder if the would be nicer via

if (kvm->arch.gisa) {
	vcpu->arch.sie_block->gd = (u32)(u64)kvm->arch.gisa;
	if (sclp.has_gisaf)
		cpu->arch.sie_block->gd |= GISA_FORMAT1;
}

>>>  	seqcount_init(&vcpu->arch.cputm_seqcount);
>>>  
>>>  	rc = kvm_vcpu_init(vcpu, kvm, id);
>>>
>>
>> So, what does this bring us? We don't seem to be using any new GISA-1
>> features.
> 
> Preparation for device pass-through interrupt forwarding.
> 

Can you add something like that to the patch description?

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

-- 

Thanks,

David / dhildenb

Re: [PATCH v2 12/12] KVM: s390: introduce the format-1 GISA

[Cornelia Huck]

On Thu, 25 Jan 2018 17:12:59 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 01/25/2018 04:47 PM, Cornelia Huck wrote:
> > On Thu, 25 Jan 2018 16:43:27 +0100
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >   
> >> On 01/25/2018 04:31 PM, David Hildenbrand wrote:
> >> [...]  
> >>>>  struct kvm_s390_vsie {
> >>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> >>>> index 68d7eef..efde264 100644
> >>>> --- a/arch/s390/kvm/kvm-s390.c
> >>>> +++ b/arch/s390/kvm/kvm-s390.c
> >>>> @@ -2518,6 +2518,8 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm,
> >>>>  	vcpu->arch.sie_block->icpua = id;
> >>>>  	spin_lock_init(&vcpu->arch.local_int.lock);
> >>>>  	vcpu->arch.sie_block->gd = (u32)(u64)kvm->arch.gisa;
> >>>> +	if (vcpu->arch.sie_block->gd && sclp.has_gisaf)
> >>>> +		vcpu->arch.sie_block->gd |= GISA_FORMAT1;
> >>>>  	seqcount_init(&vcpu->arch.cputm_seqcount);
> >>>>  
> >>>>  	rc = kvm_vcpu_init(vcpu, kvm, id);
> >>>>    
> >>>
> >>> So, what does this bring us? We don't seem to be using any new GISA-1
> >>> features.    
> >>
> >> Preparation for device pass-through interrupt forwarding.
> >>  
> > 
> > Should we start out with a dual format-0/format-1 gisa block, then?
> > IIUC, you'll switch to gisa-1 if the facility is there and gisa-1 can
> > do anything that gisa-0 can do?  
> 
> There might be systems that only have gisa-0, so I think having both makes
> sense.
> 

Yes, that's what I meant. Just do it earlier in the series - this patch
feels like an afterthought with no real user.

Re: [PATCH v2 06/12] KVM: s390: exploit GISA and AIV for emulated interrupts

[Christian Borntraeger]

On 01/25/2018 03:20 PM, David Hildenbrand wrote:
[...]
>> @@ -918,18 +919,38 @@ static int __must_check __deliver_virtio(struct kvm_vcpu *vcpu)
>>  	return rc ? -EFAULT : 0;
>>  }
>>  
>> +static int __do_deliver_io(struct kvm_vcpu *vcpu, struct kvm_s390_io_info *io)
>> +{
>> +	int rc;
>> +
>> +	rc  = put_guest_lc(vcpu, io->subchannel_id, (u16 *)__LC_SUBCHANNEL_ID);
>> +	rc |= put_guest_lc(vcpu, io->subchannel_nr, (u16 *)__LC_SUBCHANNEL_NR);
>> +	rc |= put_guest_lc(vcpu, io->io_int_parm, (u32 *)__LC_IO_INT_PARM);
>> +	rc |= put_guest_lc(vcpu, io->io_int_word, (u32 *)__LC_IO_INT_WORD);
>> +	rc |= write_guest_lc(vcpu, __LC_IO_OLD_PSW,
>> +			     &vcpu->arch.sie_block->gpsw,
>> +			     sizeof(psw_t));
>> +	rc |= read_guest_lc(vcpu, __LC_IO_NEW_PSW,
>> +			    &vcpu->arch.sie_block->gpsw,
>> +			    sizeof(psw_t));
> 
> These should now it into less lines.

The last two lines are way beyond 80. 

Can you factor that change out into
> a separate patch?


Unless Conny agrees that this is absolutely mandatory I would like to avoid that.

> 
>> +	return rc ? -EFAULT : 0;
>> +}
>> +
>>  static int __must_check __deliver_io(struct kvm_vcpu *vcpu,
>>  				     unsigned long irq_type)
>>  {
>>  	struct list_head *isc_list;
>>  	struct kvm_s390_float_interrupt *fi;
>>  	struct kvm_s390_interrupt_info *inti = NULL;
>> +	struct kvm_s390_io_info io;
>> +	u32 isc;
>>  	int rc = 0;
>>  
>>  	fi = &vcpu->kvm->arch.float_int;
>>  
>>  	spin_lock(&fi->lock);
>> -	isc_list = &fi->lists[irq_type_to_isc(irq_type)];
>> +	isc = irq_type_to_isc(irq_type);
>> +	isc_list = &fi->lists[isc];
>>  	inti = list_first_entry_or_null(isc_list,
>>  					struct kvm_s390_interrupt_info,
>>  					list);
>> @@ -957,24 +978,32 @@ static int __must_check __deliver_io(struct kvm_vcpu *vcpu,
>>  	spin_unlock(&fi->lock);
>>  
>>  	if (inti) {
>> -		rc  = put_guest_lc(vcpu, inti->io.subchannel_id,
>> -				(u16 *)__LC_SUBCHANNEL_ID);
>> -		rc |= put_guest_lc(vcpu, inti->io.subchannel_nr,
>> -				(u16 *)__LC_SUBCHANNEL_NR);
>> -		rc |= put_guest_lc(vcpu, inti->io.io_int_parm,
>> -				(u32 *)__LC_IO_INT_PARM);
>> -		rc |= put_guest_lc(vcpu, inti->io.io_int_word,
>> -				(u32 *)__LC_IO_INT_WORD);
>> -		rc |= write_guest_lc(vcpu, __LC_IO_OLD_PSW,
>> -				&vcpu->arch.sie_block->gpsw,
>> -				sizeof(psw_t));
>> -		rc |= read_guest_lc(vcpu, __LC_IO_NEW_PSW,
>> -				&vcpu->arch.sie_block->gpsw,
>> -				sizeof(psw_t));
>> +		rc = __do_deliver_io(vcpu, &(inti->io));
>>  		kfree(inti);
>> +		goto out;
>>  	}
>>  
>> -	return rc ? -EFAULT : 0;
>> +	if (vcpu->kvm->arch.gisa) {
>> +		if (kvm_s390_gisa_tac_ipm_gisc(vcpu->kvm->arch.gisa, isc)) {
> 
> 
> if (vcpu->kvm->arch.gisa &&
>     kvm_s390_gisa_tac_ipm_gisc(vcpu->kvm->arch.gisa, isc) {
> 
> avoids one nesting level

agreed.


> 
>> +			/*
>> +			 * in case an adapter interrupt was not delivered
>> +			 * in SIE context KVM will handle the delivery
>> +			 */
>> +			VCPU_EVENT(vcpu, 4, "%s isc %u", "deliver: I/O (AI/gisa)", isc);
>> +			memset(&io, 0, sizeof(io));
>> +			io.io_int_word = (isc << 27) | 0x80000000;
>> +			vcpu->stat.deliver_io_int++;
>> +			trace_kvm_s390_deliver_interrupt(vcpu->vcpu_id,
>> +				KVM_S390_INT_IO(1, 0, 0, 0),
>> +				((__u32)io.subchannel_id << 16) |
>> +				io.subchannel_nr,
>> +				((__u64)io.io_int_parm << 32) |
>> +				io.io_int_word);
>> +			rc = __do_deliver_io(vcpu, &io);
>> +		}
>> +	}
>> +out:
>> +	return rc;
>>  }
>>  
>>  typedef int (*deliver_irq_t)(struct kvm_vcpu *vcpu);
>> @@ -1537,12 +1566,23 @@ static int __inject_io(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
>>  	struct kvm_s390_float_interrupt *fi;
>>  	struct list_head *list;
>>  	int isc;
>> +	int rc = 0;
>> +
>> +	isc = int_word_to_isc(inti->io.io_int_word);
>> +
>> +	if (kvm->arch.gisa && inti->type & KVM_S390_INT_IO_AI_MASK) {
> 
> && (inti->type & KVM_S390_INT_IO_AI_MASK)) {

not necessary, & binds stronger than &&. So why?



> 
>> +		VM_EVENT(kvm, 4, "%s isc %1u", "inject: I/O (AI/gisa)", isc);
>> +		kvm_s390_gisa_set_ipm_gisc(kvm->arch.gisa, isc);
> 
> Can there only be one pending at a time? (e.g. what happens if the bit
> is already set?)

Yes there can be only one per ISC and the multiplexing is done on device
specific summary and queue indicator bits. (e.g. look at the virtio-ccw 
stuff)


> 
>> +		kfree(inti);
>> +		goto out;
> 
> Why not simply return 0? ...
> 
>> +	}
>>  
>>  	fi = &kvm->arch.float_int;
>>  	spin_lock(&fi->lock);
>>  	if (fi->counters[FIRQ_CNTR_IO] >= KVM_S390_MAX_FLOAT_IRQS) {
>>  		spin_unlock(&fi->lock);
>> -		return -EBUSY;
>> +		rc = -EBUSY;
>> +		goto out;
> 
> ... avoids this change ...
> 
>>  	}
>>  	fi->counters[FIRQ_CNTR_IO] += 1;
>>  
>> @@ -1553,12 +1593,12 @@ static int __inject_io(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
>>  			inti->io.subchannel_id >> 8,
>>  			inti->io.subchannel_id >> 1 & 0x3,
>>  			inti->io.subchannel_nr);
>> -	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(isc_to_irq_type(isc), &fi->pending_irqs);
>>  	spin_unlock(&fi->lock);
>> -	return 0;
>> +out:
>> +	return rc;
> 
> ... and this.
> 
>>  }

Will double check but at the moment I agree to 3 comments.





>>  
>>  /*
>> @@ -2705,3 +2745,29 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu, __u8 __user *buf, int len)
>>  	return n;
>>  }
>>  
>> +void kvm_s390_gisa_clear(struct kvm *kvm)
> 
> can we instead call this "gisa_setup" or move all that directly into the
> init function? (because it essentially inits the gisa)

It is also called in other places (kvm_s390_clear_float_irqs)

> 
>> +{
>> +	if (kvm->arch.gisa) {
> 
> This check is not needed: guaranteed to be set by the caller.

Huh? kvm->arch.gisa can be NULL, also called by kvm_s390_clear_float_irqs
> 
>> +		memset(kvm->arch.gisa, 0, sizeof(struct kvm_s390_gisa));



>> +
>> +void kvm_s390_gisa_destroy(struct kvm *kvm)
>> +{
>> +	if (!kvm->arch.gisa)
>> +		return;
>> +	kvm->arch.gisa = NULL;
> 
> You can do this unconditionally. Nevertheless, this function is not
> really useful: only called when we destroy the VM.

It can be useful for later support of passthrough, so I would like to
keep it as counter part of gisa_init

Re: [PATCH v2 06/12] KVM: s390: exploit GISA and AIV for emulated interrupts

[Cornelia Huck]

On Thu, 25 Jan 2018 17:32:29 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 01/25/2018 03:20 PM, David Hildenbrand wrote:
> [...]
> >> @@ -918,18 +919,38 @@ static int __must_check __deliver_virtio(struct kvm_vcpu *vcpu)
> >>  	return rc ? -EFAULT : 0;
> >>  }
> >>  
> >> +static int __do_deliver_io(struct kvm_vcpu *vcpu, struct kvm_s390_io_info *io)
> >> +{
> >> +	int rc;
> >> +
> >> +	rc  = put_guest_lc(vcpu, io->subchannel_id, (u16 *)__LC_SUBCHANNEL_ID);
> >> +	rc |= put_guest_lc(vcpu, io->subchannel_nr, (u16 *)__LC_SUBCHANNEL_NR);
> >> +	rc |= put_guest_lc(vcpu, io->io_int_parm, (u32 *)__LC_IO_INT_PARM);
> >> +	rc |= put_guest_lc(vcpu, io->io_int_word, (u32 *)__LC_IO_INT_WORD);
> >> +	rc |= write_guest_lc(vcpu, __LC_IO_OLD_PSW,
> >> +			     &vcpu->arch.sie_block->gpsw,
> >> +			     sizeof(psw_t));
> >> +	rc |= read_guest_lc(vcpu, __LC_IO_NEW_PSW,
> >> +			    &vcpu->arch.sie_block->gpsw,
> >> +			    sizeof(psw_t));  
> > 
> > These should now it into less lines.  
> 
> The last two lines are way beyond 80. 
> 
> Can you factor that change out into
> > a separate patch?  
> 
> 
> Unless Conny agrees that this is absolutely mandatory I would like to avoid that.

I don't think factoring this out would be very useful.

Re: [PATCH v2 12/12] KVM: s390: introduce the format-1 GISA

[Christian Borntraeger]

On 01/25/2018 05:16 PM, David Hildenbrand wrote:
> On 25.01.2018 16:43, Christian Borntraeger wrote:
>>
>>
>> On 01/25/2018 04:31 PM, David Hildenbrand wrote:
>> [...]
>>>>  struct kvm_s390_vsie {
>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>>> index 68d7eef..efde264 100644
>>>> --- a/arch/s390/kvm/kvm-s390.c
>>>> +++ b/arch/s390/kvm/kvm-s390.c
>>>> @@ -2518,6 +2518,8 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm,
>>>>  	vcpu->arch.sie_block->icpua = id;
>>>>  	spin_lock_init(&vcpu->arch.local_int.lock);
>>>>  	vcpu->arch.sie_block->gd = (u32)(u64)kvm->arch.gisa;
>>>> +	if (vcpu->arch.sie_block->gd && sclp.has_gisaf)
>>>> +		vcpu->arch.sie_block->gd |= GISA_FORMAT1;
> 
> wonder if the would be nicer via
> 
> if (kvm->arch.gisa) {
> 	vcpu->arch.sie_block->gd = (u32)(u64)kvm->arch.gisa;
> 	if (sclp.has_gisaf)
> 		cpu->arch.sie_block->gd |= GISA_FORMAT1;
> }

Dont know. I leave it as is.

> 
>>>>  	seqcount_init(&vcpu->arch.cputm_seqcount);
>>>>  
>>>>  	rc = kvm_vcpu_init(vcpu, kvm, id);
>>>>
>>>
>>> So, what does this bring us? We don't seem to be using any new GISA-1
>>> features.
>>
>> Preparation for device pass-through interrupt forwarding.
>>
> 
> Can you add something like that to the patch description?

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

Re: [PATCH v2 06/12] KVM: s390: exploit GISA and AIV for emulated interrupts

[David Hildenbrand]

On 25.01.2018 17:32, Christian Borntraeger wrote:
> 
> 
> On 01/25/2018 03:20 PM, David Hildenbrand wrote:
> [...]
>>> @@ -918,18 +919,38 @@ static int __must_check __deliver_virtio(struct kvm_vcpu *vcpu)
>>>  	return rc ? -EFAULT : 0;
>>>  }
>>>  
>>> +static int __do_deliver_io(struct kvm_vcpu *vcpu, struct kvm_s390_io_info *io)
>>> +{
>>> +	int rc;
>>> +
>>> +	rc  = put_guest_lc(vcpu, io->subchannel_id, (u16 *)__LC_SUBCHANNEL_ID);
>>> +	rc |= put_guest_lc(vcpu, io->subchannel_nr, (u16 *)__LC_SUBCHANNEL_NR);
>>> +	rc |= put_guest_lc(vcpu, io->io_int_parm, (u32 *)__LC_IO_INT_PARM);
>>> +	rc |= put_guest_lc(vcpu, io->io_int_word, (u32 *)__LC_IO_INT_WORD);
>>> +	rc |= write_guest_lc(vcpu, __LC_IO_OLD_PSW,
>>> +			     &vcpu->arch.sie_block->gpsw,
>>> +			     sizeof(psw_t));
>>> +	rc |= read_guest_lc(vcpu, __LC_IO_NEW_PSW,
>>> +			    &vcpu->arch.sie_block->gpsw,
>>> +			    sizeof(psw_t));
>>
>> These should now it into less lines.
> 
> The last two lines are way beyond 80. 
> 

Huh? At least in my world, I can reduce 3 to 2 lines (for both).

> Can you factor that change out into
>> a separate patch?
> 
> 
[...]

>>
>>> +			/*
>>> +			 * in case an adapter interrupt was not delivered
>>> +			 * in SIE context KVM will handle the delivery
>>> +			 */
>>> +			VCPU_EVENT(vcpu, 4, "%s isc %u", "deliver: I/O (AI/gisa)", isc);
>>> +			memset(&io, 0, sizeof(io));
>>> +			io.io_int_word = (isc << 27) | 0x80000000;
>>> +			vcpu->stat.deliver_io_int++;
>>> +			trace_kvm_s390_deliver_interrupt(vcpu->vcpu_id,
>>> +				KVM_S390_INT_IO(1, 0, 0, 0),
>>> +				((__u32)io.subchannel_id << 16) |
>>> +				io.subchannel_nr,
>>> +				((__u64)io.io_int_parm << 32) |
>>> +				io.io_int_word);
>>> +			rc = __do_deliver_io(vcpu, &io);
>>> +		}
>>> +	}
>>> +out:
>>> +	return rc;
>>>  }
>>>  
>>>  typedef int (*deliver_irq_t)(struct kvm_vcpu *vcpu);
>>> @@ -1537,12 +1566,23 @@ static int __inject_io(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
>>>  	struct kvm_s390_float_interrupt *fi;
>>>  	struct list_head *list;
>>>  	int isc;
>>> +	int rc = 0;
>>> +
>>> +	isc = int_word_to_isc(inti->io.io_int_word);
>>> +
>>> +	if (kvm->arch.gisa && inti->type & KVM_S390_INT_IO_AI_MASK) {
>>
>> && (inti->type & KVM_S390_INT_IO_AI_MASK)) {
> 
> not necessary, & binds stronger than &&. So why?

You're right, I thought we usually do that because of "suggest
parentheses around comparison in operand of ..." warnings from GCC. But
it only seems to apply when using e.g. == / !=.

> 
> 
> 
>>
>>> +		VM_EVENT(kvm, 4, "%s isc %1u", "inject: I/O (AI/gisa)", isc);
>>> +		kvm_s390_gisa_set_ipm_gisc(kvm->arch.gisa, isc);
>>
>> Can there only be one pending at a time? (e.g. what happens if the bit
>> is already set?)
> 
> Yes there can be only one per ISC and the multiplexing is done on device
> specific summary and queue indicator bits. (e.g. look at the virtio-ccw 
> stuff)
> 
[...]

> 
>>>  
>>>  /*
>>> @@ -2705,3 +2745,29 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu, __u8 __user *buf, int len)
>>>  	return n;
>>>  }
>>>  
>>> +void kvm_s390_gisa_clear(struct kvm *kvm)
>>
>> can we instead call this "gisa_setup" or move all that directly into the
>> init function? (because it essentially inits the gisa)
> 
> It is also called in other places (kvm_s390_clear_float_irqs)
> 
>>
>>> +{
>>> +	if (kvm->arch.gisa) {
>>
>> This check is not needed: guaranteed to be set by the caller.
> 
> Huh? kvm->arch.gisa can be NULL, also called by kvm_s390_clear_float_irqs

Okay, not clear when reviewing this patch on its own. But maybe just I
find the order in which things are added challenging to review :)

>>
>>> +		memset(kvm->arch.gisa, 0, sizeof(struct kvm_s390_gisa));
> 
> 
> 
>>> +
>>> +void kvm_s390_gisa_destroy(struct kvm *kvm)
>>> +{
>>> +	if (!kvm->arch.gisa)
>>> +		return;
>>> +	kvm->arch.gisa = NULL;
>>
>> You can do this unconditionally. Nevertheless, this function is not
>> really useful: only called when we destroy the VM.
> 
> It can be useful for later support of passthrough, so I would like to
> keep it as counter part of gisa_init

Makes sense.


-- 

Thanks,

David / dhildenb

Re: [PATCH v2 06/12] KVM: s390: exploit GISA and AIV for emulated interrupts

[Christian Borntraeger]

On 01/25/2018 05:47 PM, David Hildenbrand wrote:
> On 25.01.2018 17:32, Christian Borntraeger wrote:
>>
>>
>> On 01/25/2018 03:20 PM, David Hildenbrand wrote:
>> [...]
>>>> @@ -918,18 +919,38 @@ static int __must_check __deliver_virtio(struct kvm_vcpu *vcpu)
>>>>  	return rc ? -EFAULT : 0;
>>>>  }
>>>>  
>>>> +static int __do_deliver_io(struct kvm_vcpu *vcpu, struct kvm_s390_io_info *io)
>>>> +{
>>>> +	int rc;
>>>> +
>>>> +	rc  = put_guest_lc(vcpu, io->subchannel_id, (u16 *)__LC_SUBCHANNEL_ID);
>>>> +	rc |= put_guest_lc(vcpu, io->subchannel_nr, (u16 *)__LC_SUBCHANNEL_NR);
>>>> +	rc |= put_guest_lc(vcpu, io->io_int_parm, (u32 *)__LC_IO_INT_PARM);
>>>> +	rc |= put_guest_lc(vcpu, io->io_int_word, (u32 *)__LC_IO_INT_WORD);
>>>> +	rc |= write_guest_lc(vcpu, __LC_IO_OLD_PSW,
>>>> +			     &vcpu->arch.sie_block->gpsw,
>>>> +			     sizeof(psw_t));
>>>> +	rc |= read_guest_lc(vcpu, __LC_IO_NEW_PSW,
>>>> +			    &vcpu->arch.sie_block->gpsw,
>>>> +			    sizeof(psw_t));
>>>
>>> These should now it into less lines.
>>
>> The last two lines are way beyond 80. 
>>
> 
> Huh? At least in my world, I can reduce 3 to 2 lines (for both).

Yes, but if I have to break the line, then I prefer to have one parameter per line. (all or
nothing)

Re: [PATCH v2 12/12] KVM: s390: introduce the format-1 GISA

[Christian Borntraeger]

On 01/25/2018 05:17 PM, Cornelia Huck wrote:
> On Thu, 25 Jan 2018 17:12:59 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 01/25/2018 04:47 PM, Cornelia Huck wrote:
>>> On Thu, 25 Jan 2018 16:43:27 +0100
>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>>   
>>>> On 01/25/2018 04:31 PM, David Hildenbrand wrote:
>>>> [...]  
>>>>>>  struct kvm_s390_vsie {
>>>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>>>>> index 68d7eef..efde264 100644
>>>>>> --- a/arch/s390/kvm/kvm-s390.c
>>>>>> +++ b/arch/s390/kvm/kvm-s390.c
>>>>>> @@ -2518,6 +2518,8 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm,
>>>>>>  	vcpu->arch.sie_block->icpua = id;
>>>>>>  	spin_lock_init(&vcpu->arch.local_int.lock);
>>>>>>  	vcpu->arch.sie_block->gd = (u32)(u64)kvm->arch.gisa;
>>>>>> +	if (vcpu->arch.sie_block->gd && sclp.has_gisaf)
>>>>>> +		vcpu->arch.sie_block->gd |= GISA_FORMAT1;
>>>>>>  	seqcount_init(&vcpu->arch.cputm_seqcount);
>>>>>>  
>>>>>>  	rc = kvm_vcpu_init(vcpu, kvm, id);
>>>>>>    
>>>>>
>>>>> So, what does this bring us? We don't seem to be using any new GISA-1
>>>>> features.    
>>>>
>>>> Preparation for device pass-through interrupt forwarding.
>>>>  
>>>
>>> Should we start out with a dual format-0/format-1 gisa block, then?
>>> IIUC, you'll switch to gisa-1 if the facility is there and gisa-1 can
>>> do anything that gisa-0 can do?  
>>
>> There might be systems that only have gisa-0, so I think having both makes
>> sense.
>>
> 
> Yes, that's what I meant. Just do it earlier in the series - this patch
> feels like an afterthought with no real user.

I added 

A format-1 can do everything that format-0 can and we will need it
for real HW passthrough. As there are systems with only format-0
we keep both variants.

to the patch description. Maybe its now a bit less odd?

Re: [PATCH v2 12/12] KVM: s390: introduce the format-1 GISA

[Cornelia Huck]

On Thu, 25 Jan 2018 17:51:24 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 01/25/2018 05:17 PM, Cornelia Huck wrote:
> > On Thu, 25 Jan 2018 17:12:59 +0100
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >   
> >> On 01/25/2018 04:47 PM, Cornelia Huck wrote:  
> >>> On Thu, 25 Jan 2018 16:43:27 +0100
> >>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >>>     
> >>>> On 01/25/2018 04:31 PM, David Hildenbrand wrote:
> >>>> [...]    
> >>>>>>  struct kvm_s390_vsie {
> >>>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> >>>>>> index 68d7eef..efde264 100644
> >>>>>> --- a/arch/s390/kvm/kvm-s390.c
> >>>>>> +++ b/arch/s390/kvm/kvm-s390.c
> >>>>>> @@ -2518,6 +2518,8 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm,
> >>>>>>  	vcpu->arch.sie_block->icpua = id;
> >>>>>>  	spin_lock_init(&vcpu->arch.local_int.lock);
> >>>>>>  	vcpu->arch.sie_block->gd = (u32)(u64)kvm->arch.gisa;
> >>>>>> +	if (vcpu->arch.sie_block->gd && sclp.has_gisaf)
> >>>>>> +		vcpu->arch.sie_block->gd |= GISA_FORMAT1;
> >>>>>>  	seqcount_init(&vcpu->arch.cputm_seqcount);
> >>>>>>  
> >>>>>>  	rc = kvm_vcpu_init(vcpu, kvm, id);
> >>>>>>      
> >>>>>
> >>>>> So, what does this bring us? We don't seem to be using any new GISA-1
> >>>>> features.      
> >>>>
> >>>> Preparation for device pass-through interrupt forwarding.
> >>>>    
> >>>
> >>> Should we start out with a dual format-0/format-1 gisa block, then?
> >>> IIUC, you'll switch to gisa-1 if the facility is there and gisa-1 can
> >>> do anything that gisa-0 can do?    
> >>
> >> There might be systems that only have gisa-0, so I think having both makes
> >> sense.
> >>  
> > 
> > Yes, that's what I meant. Just do it earlier in the series - this patch
> > feels like an afterthought with no real user.  
> 
> I added 
> 
> A format-1 can do everything that format-0 can and we will need it
> for real HW passthrough. As there are systems with only format-0
> we keep both variants.
> 
> to the patch description. Maybe its now a bit less odd?

Also fine with me.

(I'll do a proper review round through the patches later.)

Re: [PATCH v2 02/12] KVM: s390: define GISA format-0 data structure

[Cornelia Huck]

On Thu, 25 Jan 2018 14:28:38 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> From: Michael Mueller <mimu@linux.vnet.ibm.com>
> 
> In preperation to support pass-through adapter interrupts, the Guest
> Interruption State Area (GISA) and the Adapter Interruption Virtualization
> (AIV) features will be introduced here.
> 
> This patch introduces format-0 GISA (that is defines the struct describing
> the GISA, allocates storage for it, and introduces fields for the
> GISA address in kvm_s390_sie_block and kvm_s390_vsie).
> 
> As the GISA requires storage below 2GB, it is put in sie_page2, which is
> already allocated in ZONE_DMA. In addition, The GISA requires alignment to
> its integral boundary. This is already naturally aligned via the
> padding in the sie_page2.
> 
> Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
> Reviewed-by: Pierre Morel <pmorel@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>
> ---
>  arch/s390/include/asm/kvm_host.h | 23 +++++++++++++++++++----
>  arch/s390/kvm/kvm-s390.c         |  1 +
>  2 files changed, 20 insertions(+), 4 deletions(-)

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

Re: [PATCH v2 08/12] KVM: s390: add GISA interrupts to FLIC ioctl interface

[Cornelia Huck]

On Thu, 25 Jan 2018 14:28:44 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> From: Michael Mueller <mimu@linux.vnet.ibm.com>
> 
> Pending interrupts marked in the GISA IPM are required to
> become part of the answer of ioctl KVM_DEV_FLIC_GET_ALL_IRQS.
> 
> The ioctl KVM_DEV_FLIC_ENQUEUE is already capable to enqueue
> adapter interrupts when a GISA is present.
> 
> With ioctl KVM_DEV_FLIC_CLEAR_IRQS the GISA IPM wil be cleared
> now as well.
> 
> 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, 18 insertions(+)

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

Re: [PATCH v2 09/12] KVM: s390: make kvm_s390_get_io_int() aware of GISA

[Cornelia Huck]

On Thu, 25 Jan 2018 14:28:45 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> From: Michael Mueller <mimu@linux.vnet.ibm.com>
> 
> The function returns a pending I/O interrupt with the highest
> priority defined by its ISC.
> 
> Together with AIV activation, pending adapter interrupts are
> managed by the GISA IPM. Thus kvm_s390_get_io_int() needs to
> inspect the IPM as well when the interrupt with the highest
> priority has to be identified.
> 
> In case classic and adapter interrupts with the same ISC are
> pending, the classic interrupt will be returned first.

Can this lead to starving? Consider a guest that never enables itself
for I/O interrupts, but collects pending interrupts via tpi. It will
always get the intis for an isc, but not the ai, wouldn't it?

> 
> 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>
> ---
>  arch/s390/kvm/interrupt.c | 71 +++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 65 insertions(+), 6 deletions(-)

Re: [PATCH v2 11/12] s390/sclp: expose the GISA format facility

[Cornelia Huck]

On Thu, 25 Jan 2018 14:28:47 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> From: Michael Mueller <mimu@linux.vnet.ibm.com>
> 
> The GISA format facility is required by the host to be able to process
> a format-1 GISA. If not available, the used GISA format will be format-0.
> All format-1 related extension will not be available in this case.
> 
> Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Acked-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/include/asm/sclp.h   | 1 +
>  drivers/s390/char/sclp_early.c | 3 ++-
>  2 files changed, 3 insertions(+), 1 deletion(-)

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

Re: [PATCH v2 12/12] KVM: s390: introduce the format-1 GISA

[Cornelia Huck]

On Thu, 25 Jan 2018 14:28:48 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> From: Michael Mueller <mimu@linux.vnet.ibm.com>
> 
> The patch modifies the previously defined GISA data structure to be
> able to store two GISA formats, format-0 and format-1. Additionally,
> it verifies the availability of the GISA format facility and enables
> the use of a format-1 GISA in the SIE control block accordingly.
> 
> Signed-off-by: Michael Mueller <mimu@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/include/asm/kvm_host.h | 44 +++++++++++++++++++++++++++++++---------
>  arch/s390/kvm/kvm-s390.c         |  2 ++
>  2 files changed, 36 insertions(+), 10 deletions(-)

With the description changed as discussed previously,

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

Re: [PATCH v2 09/12] KVM: s390: make kvm_s390_get_io_int() aware of GISA

[Christian Borntraeger]

On 01/26/2018 10:41 AM, Cornelia Huck wrote:
> On Thu, 25 Jan 2018 14:28:45 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> From: Michael Mueller <mimu@linux.vnet.ibm.com>
>>
>> The function returns a pending I/O interrupt with the highest
>> priority defined by its ISC.
>>
>> Together with AIV activation, pending adapter interrupts are
>> managed by the GISA IPM. Thus kvm_s390_get_io_int() needs to
>> inspect the IPM as well when the interrupt with the highest
>> priority has to be identified.
>>
>> In case classic and adapter interrupts with the same ISC are
>> pending, the classic interrupt will be returned first.
> 
> Can this lead to starving? Consider a guest that never enables itself
> for I/O interrupts, but collects pending interrupts via tpi. It will
> always get the intis for an isc, but not the ai, wouldn't it?

Only if it handles the interrupts slower than new ones arrive, in that case
you have a problem anyway. When looking at sane configuration, this priority
makes sense as the classic interrupts are used for configuration type ccw,
while adapter interrupts are for data. You want to get the control changes
quickly. In a sane environment nobody would probably put devices with adapter
interrupts on the same isc as different devices with only classic interrupts.

But looking at your theoretical "tpi only" case. If your statement is correct
then you would also starve interrupts with lets say isc 4 when also interrupts
with isc3 are pending, since isc3 will always be preferred. And it did not
seem to be an issue in the real world. Or did I miss your point?

> 
>>
>> 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>
>> ---
>>  arch/s390/kvm/interrupt.c | 71 +++++++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 65 insertions(+), 6 deletions(-)
> 

Re: [PATCH v2 09/12] KVM: s390: make kvm_s390_get_io_int() aware of GISA

[Cornelia Huck]

On Fri, 26 Jan 2018 10:57:32 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 01/26/2018 10:41 AM, Cornelia Huck wrote:
> > On Thu, 25 Jan 2018 14:28:45 +0100
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >   
> >> From: Michael Mueller <mimu@linux.vnet.ibm.com>
> >>
> >> The function returns a pending I/O interrupt with the highest
> >> priority defined by its ISC.
> >>
> >> Together with AIV activation, pending adapter interrupts are
> >> managed by the GISA IPM. Thus kvm_s390_get_io_int() needs to
> >> inspect the IPM as well when the interrupt with the highest
> >> priority has to be identified.
> >>
> >> In case classic and adapter interrupts with the same ISC are
> >> pending, the classic interrupt will be returned first.  
> > 
> > Can this lead to starving? Consider a guest that never enables itself
> > for I/O interrupts, but collects pending interrupts via tpi. It will
> > always get the intis for an isc, but not the ai, wouldn't it?  
> 
> Only if it handles the interrupts slower than new ones arrive, in that case
> you have a problem anyway. When looking at sane configuration, this priority
> makes sense as the classic interrupts are used for configuration type ccw,
> while adapter interrupts are for data. You want to get the control changes
> quickly. In a sane environment nobody would probably put devices with adapter
> interrupts on the same isc as different devices with only classic interrupts.

But if you have a lot of devices, all using the same isc, you might
have a lot of classic interrupts (for example, due to firing a volley
of channel programs at all subchannels) and they could starve out the
device(s) that are waiting for adapter interrupts.

It's probably not a problem with today's guests (due to the control vs.
data semantics you pointed out above), especially as the only guest I
know that does not enable interrupts is the s390-ccw bios. But maybe
add a comment?

> 
> But looking at your theoretical "tpi only" case. If your statement is correct
> then you would also starve interrupts with lets say isc 4 when also interrupts
> with isc3 are pending, since isc3 will always be preferred. And it did not
> seem to be an issue in the real world. Or did I miss your point?

That's how it supposed to work with different iscs. If you have a very
chatty device on isc 3 and enable iscs 3 and 4 in cr6, it may well
drone out a device on isc 4. But that's an issue with the setup done by
the guest; it needs to put the devices on sensible iscs and manipulate
cr6, if needed.

That said, the hypothetical tpi-only guest might work around the issue
by assigning different iscs for classic and adapter interrupts.

Re: [PATCH v2 09/12] KVM: s390: make kvm_s390_get_io_int() aware of GISA

[Christian Borntraeger]

On 01/26/2018 12:21 PM, Cornelia Huck wrote:
> On Fri, 26 Jan 2018 10:57:32 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 01/26/2018 10:41 AM, Cornelia Huck wrote:
>>> On Thu, 25 Jan 2018 14:28:45 +0100
>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>>   
>>>> From: Michael Mueller <mimu@linux.vnet.ibm.com>
>>>>
>>>> The function returns a pending I/O interrupt with the highest
>>>> priority defined by its ISC.
>>>>
>>>> Together with AIV activation, pending adapter interrupts are
>>>> managed by the GISA IPM. Thus kvm_s390_get_io_int() needs to
>>>> inspect the IPM as well when the interrupt with the highest
>>>> priority has to be identified.
>>>>
>>>> In case classic and adapter interrupts with the same ISC are
>>>> pending, the classic interrupt will be returned first.  
>>>
>>> Can this lead to starving? Consider a guest that never enables itself
>>> for I/O interrupts, but collects pending interrupts via tpi. It will
>>> always get the intis for an isc, but not the ai, wouldn't it?  
>>
>> Only if it handles the interrupts slower than new ones arrive, in that case
>> you have a problem anyway. When looking at sane configuration, this priority
>> makes sense as the classic interrupts are used for configuration type ccw,
>> while adapter interrupts are for data. You want to get the control changes
>> quickly. In a sane environment nobody would probably put devices with adapter
>> interrupts on the same isc as different devices with only classic interrupts.
> 
> But if you have a lot of devices, all using the same isc, you might
> have a lot of classic interrupts (for example, due to firing a volley
> of channel programs at all subchannels) and they could starve out the
> device(s) that are waiting for adapter interrupts.
> 
> It's probably not a problem with today's guests (due to the control vs.
> data semantics you pointed out above), especially as the only guest I
> know that does not enable interrupts is the s390-ccw bios. But maybe
> add a comment?

Do you have some proposal for a comment? Then I can certainly add that.

Re: [PATCH v2 09/12] KVM: s390: make kvm_s390_get_io_int() aware of GISA

[Cornelia Huck]

On Fri, 26 Jan 2018 12:25:03 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 01/26/2018 12:21 PM, Cornelia Huck wrote:
> > On Fri, 26 Jan 2018 10:57:32 +0100
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >   
> >> On 01/26/2018 10:41 AM, Cornelia Huck wrote:  
> >>> On Thu, 25 Jan 2018 14:28:45 +0100
> >>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >>>     
> >>>> From: Michael Mueller <mimu@linux.vnet.ibm.com>
> >>>>
> >>>> The function returns a pending I/O interrupt with the highest
> >>>> priority defined by its ISC.
> >>>>
> >>>> Together with AIV activation, pending adapter interrupts are
> >>>> managed by the GISA IPM. Thus kvm_s390_get_io_int() needs to
> >>>> inspect the IPM as well when the interrupt with the highest
> >>>> priority has to be identified.
> >>>>
> >>>> In case classic and adapter interrupts with the same ISC are
> >>>> pending, the classic interrupt will be returned first.    
> >>>
> >>> Can this lead to starving? Consider a guest that never enables itself
> >>> for I/O interrupts, but collects pending interrupts via tpi. It will
> >>> always get the intis for an isc, but not the ai, wouldn't it?    
> >>
> >> Only if it handles the interrupts slower than new ones arrive, in that case
> >> you have a problem anyway. When looking at sane configuration, this priority
> >> makes sense as the classic interrupts are used for configuration type ccw,
> >> while adapter interrupts are for data. You want to get the control changes
> >> quickly. In a sane environment nobody would probably put devices with adapter
> >> interrupts on the same isc as different devices with only classic interrupts.  
> > 
> > But if you have a lot of devices, all using the same isc, you might
> > have a lot of classic interrupts (for example, due to firing a volley
> > of channel programs at all subchannels) and they could starve out the
> > device(s) that are waiting for adapter interrupts.
> > 
> > It's probably not a problem with today's guests (due to the control vs.
> > data semantics you pointed out above), especially as the only guest I
> > know that does not enable interrupts is the s390-ccw bios. But maybe
> > add a comment?  
> 
> Do you have some proposal for a comment? Then I can certainly add that.
> 

/*
 * Note that for a guest that does not enable I/O interrupts
 * but relies on TPI, a flood of classic interrupts may starve
 * out adapter interrupts on the same isc. Linux does not do
 * that, and it is possible to work around the issue by configuring
 * different iscs for classic and adapter interrupts in the guest,
 * but we may want to revisit this in the future.
 */

I think we don't really need to spend more time on that right now (and
probably complicate the code), so with the comment

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

Re: [PATCH v2 09/12] KVM: s390: make kvm_s390_get_io_int() aware of GISA

[Christian Borntraeger]

On 01/25/2018 02:28 PM, Christian Borntraeger wrote:
> From: Michael Mueller <mimu@linux.vnet.ibm.com>
> 
> The function returns a pending I/O interrupt with the highest
> priority defined by its ISC.
> 
> Together with AIV activation, pending adapter interrupts are
> managed by the GISA IPM. Thus kvm_s390_get_io_int() needs to
> inspect the IPM as well when the interrupt with the highest
> priority has to be identified.
> 
> In case classic and adapter interrupts with the same ISC are
> pending, the classic interrupt will be returned first.
> 
> 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>
> ---
>  arch/s390/kvm/interrupt.c | 71 +++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 65 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 0edf97a..5c48930 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -1471,12 +1471,8 @@ static struct kvm_s390_interrupt_info *get_io_int(struct kvm *kvm,
>  	return NULL;
>  }
> 
> -/*
> - * Dequeue and return an I/O interrupt matching any of the interruption
> - * subclasses as designated by the isc mask in cr6 and the schid (if != 0).
> - */
> -struct kvm_s390_interrupt_info *kvm_s390_get_io_int(struct kvm *kvm,
> -						    u64 isc_mask, u32 schid)
> +static struct kvm_s390_interrupt_info *get_top_io_int(struct kvm *kvm,
> +						      u64 isc_mask, u32 schid)
>  {
>  	struct kvm_s390_interrupt_info *inti = NULL;
>  	int isc;
> @@ -1488,6 +1484,69 @@ struct kvm_s390_interrupt_info *kvm_s390_get_io_int(struct kvm *kvm,
>  	return inti;
>  }
> 
> +static int get_top_gisa_isc(struct kvm *kvm, u64 isc_mask, u32 schid)
> +{
> +	unsigned long active_mask;
> +	int isc;
> +
> +	if (schid)
> +		goto out;
> +	if (!kvm->arch.gisa)
> +		goto out;
> +
> +	active_mask = (isc_mask & kvm_s390_gisa_get_ipm(kvm->arch.gisa) << 24) << 32;
> +	while (active_mask) {
> +		isc = __flogr(active_mask);

Sebastian Ott noticed that newer binutils will complain about that when
compiling for z990 or older.

I will use the equivalent

-               isc = __flogr(active_mask);
+               isc = __fls(active_mask) ^ (BITS_PER_LONG - 1);