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

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

[Christian Borntraeger]

Here is a patch set that implements exitless interrupts for I/O adapter
interrupts. It was running for a while now internally so from a
"quality perspective" this seems to be good. Review is always welcome
though.

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: expose 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         | 233 +++++++++++++++++++++++++++++++++-----
 arch/s390/kvm/kvm-s390.c          |  11 ++
 arch/s390/kvm/kvm-s390.h          |   7 ++
 drivers/s390/char/sclp_early.c    |   3 +-
 8 files changed, 302 insertions(+), 64 deletions(-)

-- 
2.13.4

[PATCH 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 reject 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>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/include/asm/kvm_host.h | 54 ++++++++++++++++++++--------------------
 arch/s390/kvm/interrupt.c        | 10 ++++----
 2 files changed, 32 insertions(+), 32 deletions(-)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index e16a9f2a44ad..9981721f258f 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 f8eb2cfa763a..b94173560dcf 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,
@@ -1155,7 +1155,7 @@ int __must_check kvm_s390_deliver_pending_interrupts(struct kvm_vcpu *vcpu)
 
 	while ((irqs = deliverable_irqs(vcpu)) && !rc) {
 		/* bits are in the order of interrupt priority */
-		irq_type = find_first_bit(&irqs, IRQ_PEND_COUNT);
+		irq_type = find_last_bit(&irqs, IRQ_PEND_COUNT);
 		if (is_ioirq(irq_type)) {
 			rc = __deliver_io(vcpu, irq_type);
 		} else {
-- 
2.13.4

[PATCH 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.

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 | 24 ++++++++++++++++++++----
 arch/s390/kvm/kvm-s390.c         |  1 +
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 9981721f258f..11f15a51bfc7 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 __aligned(sizeof(struct kvm_s390_gisa)); /* 0x0900 */
+	u8 reserved910[0x1000 - 0x910];			/* 0x0910 */
 };
 
 struct kvm_s390_vsie {
@@ -757,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/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index de16c224319c..48f0099188df 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.13.4

[PATCH 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>
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 31e400c1a1f3..86e5b2fdee3c 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.13.4

[PATCH 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 +++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.h  |  4 ++++
 2 files changed, 27 insertions(+)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index b94173560dcf..dfdecff302d2 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -2682,3 +2682,26 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu, __u8 __user *buf, int len)
 
 	return n;
 }
+
+#define IPM_BIT_OFFSET (offsetof(struct kvm_s390_gisa, ipm) * 8)
+
+void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
+{
+	set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
+}
+
+u8 kvm_s390_gisa_get_ipm(struct kvm_s390_gisa *gisa)
+{
+	return (u8) READ_ONCE(gisa->ipm);
+}
+
+void kvm_s390_gisa_clear_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
+{
+	clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
+}
+
+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);
+}
+
diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
index 8877116f0159..b17e4dea7ea5 100644
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -367,6 +367,10 @@ 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_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc);
+u8 kvm_s390_gisa_get_ipm(struct kvm_s390_gisa *gisa);
+void kvm_s390_gisa_clear_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc);
+int kvm_s390_gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc);
 
 /* implemented in guestdbg.c */
 void kvm_s390_backup_guest_per_regs(struct kvm_vcpu *vcpu);
-- 
2.13.4

[PATCH 05/12] s390/css: expose the AIV facility

[Christian Borntraeger]

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

The patch exposes the 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>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 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 a478eb61aaf7..fb56fa3283a2 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.13.4

[PATCH 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/kvm/interrupt.c | 104 +++++++++++++++++++++++++++++++++++++---------
 arch/s390/kvm/kvm-s390.c  |   8 ++++
 arch/s390/kvm/kvm-s390.h  |   3 ++
 3 files changed, 96 insertions(+), 19 deletions(-)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index dfdecff302d2..6c34133f3842 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -206,7 +206,8 @@ static inline u8 int_word_to_isc(u32 int_word)
 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)
@@ -896,18 +897,42 @@ 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;
+}
+
 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);
@@ -935,23 +960,27 @@ 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;
 	}
 
+	if (vcpu->kvm->arch.gisa) {
+		if (kvm_s390_gisa_tac_ipm_gisc(vcpu->kvm->arch.gisa, isc)) {
+			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 ? -EFAULT : 0;
 }
 
@@ -1515,12 +1544,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;
 
@@ -1531,12 +1571,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_gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
 	return test_and_clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
 }
 
+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 48f0099188df..68d7eef44a98 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 b17e4dea7ea5..04a3e915cd67 100644
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -371,6 +371,9 @@ void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc);
 u8 kvm_s390_gisa_get_ipm(struct kvm_s390_gisa *gisa);
 void kvm_s390_gisa_clear_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc);
 int kvm_s390_gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc);
+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.13.4

[PATCH 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>
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 6c34133f3842..f4d81730d52d 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;
@@ -969,7 +974,7 @@ static int __must_check __deliver_io(struct kvm_vcpu *vcpu,
 		if (kvm_s390_gisa_tac_ipm_gisc(vcpu->kvm->arch.gisa, isc)) {
 			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),
@@ -2280,7 +2285,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.13.4

[PATCH 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 f4d81730d52d..6c7a024c2554 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -1860,6 +1860,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)
@@ -1887,6 +1888,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++) {
@@ -1925,6 +1942,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.13.4

[PATCH 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 6c7a024c2554..2f1a5603c366 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -1449,12 +1449,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;
@@ -1466,6 +1462,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.13.4

[PATCH 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 2f1a5603c366..5215fee30f8a 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.13.4

[PATCH 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>
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 d3c1a8a2e3ad..3cae9168f63c 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 d06bc5674e5f..6b1891539c84 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.13.4

[PATCH 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 11f15a51bfc7..e6687ab96686 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 __aligned(sizeof(struct kvm_s390_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 68d7eef44a98..efde2647c2ee 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.13.4

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

[David Hildenbrand]

On 16.01.2018 21:02, Christian Borntraeger 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>
> 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 31e400c1a1f3..86e5b2fdee3c 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);
> 

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

-- 

Thanks,

David / dhildenb

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

[David Hildenbrand]

On 16.01.2018 21:02, Christian Borntraeger 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>
> 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 d3c1a8a2e3ad..3cae9168f63c 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 d06bc5674e5f..6b1891539c84 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)
> 

No public documentation to validate.

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

-- 

Thanks,

David / dhildenb

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

[David Hildenbrand]

On 16.01.2018 21:02, 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 reject 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>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/include/asm/kvm_host.h | 54 ++++++++++++++++++++--------------------
>  arch/s390/kvm/interrupt.c        | 10 ++++----
>  2 files changed, 32 insertions(+), 32 deletions(-)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index e16a9f2a44ad..9981721f258f 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

We have to touch all because of irq priority, right?

>  };
>  
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index f8eb2cfa763a..b94173560dcf 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,
> @@ -1155,7 +1155,7 @@ int __must_check kvm_s390_deliver_pending_interrupts(struct kvm_vcpu *vcpu)
>  
>  	while ((irqs = deliverable_irqs(vcpu)) && !rc) {
>  		/* bits are in the order of interrupt priority */

this comment should now be "reversed order"

> -		irq_type = find_first_bit(&irqs, IRQ_PEND_COUNT);
> +		irq_type = find_last_bit(&irqs, IRQ_PEND_COUNT);
>  		if (is_ioirq(irq_type)) {
>  			rc = __deliver_io(vcpu, irq_type);
>  		} else {
> 

Looks sane to me on the first sight.

-- 

Thanks,

David / dhildenb

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

[David Hildenbrand]

> +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 __aligned(sizeof(struct kvm_s390_gisa)); /* 0x0900 */

can we instead specify sie_page2 as packed if really needed? (I can see
that we have a BUILD_BUG_ON below, which is nice)

(alignment within well defined data structures looks strange)

> +	u8 reserved910[0x1000 - 0x910];			/* 0x0910 */
>  };
>  
>  struct kvm_s390_vsie {
> @@ -757,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;

What's this little fellow doing in this patch? :)


-- 

Thanks,

David / dhildenb

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

[Heiko Carstens]

On Tue, Jan 16, 2018 at 09:25:12PM +0100, David Hildenbrand wrote:
> 
> > +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 __aligned(sizeof(struct kvm_s390_gisa)); /* 0x0900 */
> 
> can we instead specify sie_page2 as packed if really needed? (I can see
> that we have a BUILD_BUG_ON below, which is nice)
> 
> (alignment within well defined data structures looks strange)

The alignment, if needed, should go to the definition of struct
kvm_s390_gisa above. The structure needs at least an alignment of eight
bytes since bitops are used to modify ipm (in a later patch), by using a
(long *) cast to this structure.
But of course it is all naturally aligned anyway... ;)

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

[Heiko Carstens]

On Tue, Jan 16, 2018 at 09:02:11PM +0100, Christian Borntraeger wrote:
> +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);

They all fit into one line now ;)

> +	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;
> +}

return rc = -EFAULT : 0;

from below should be moved to this function, so it has a well defined
return value instead of random garbage. Well, it's -EFAULT or 0 currently
anyway, but nobody knows...

>  static int __must_check __deliver_io(struct kvm_vcpu *vcpu,
>  				     unsigned long irq_type)
>  {
...
> +out:
>  	return rc ? -EFAULT : 0;
>  }

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

[Christian Borntraeger]

On 01/16/2018 09:18 PM, David Hildenbrand wrote:
> On 16.01.2018 21:02, 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 reject 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>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  arch/s390/include/asm/kvm_host.h | 54 ++++++++++++++++++++--------------------
>>  arch/s390/kvm/interrupt.c        | 10 ++++----
>>  2 files changed, 32 insertions(+), 32 deletions(-)
>>
>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>> index e16a9f2a44ad..9981721f258f 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
> 
> We have to touch all because of irq priority, right?

Yes, we have to swap everything for priority.

> 
>>  };
>>  
>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>> index f8eb2cfa763a..b94173560dcf 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,
>> @@ -1155,7 +1155,7 @@ int __must_check kvm_s390_deliver_pending_interrupts(struct kvm_vcpu *vcpu)
>>  
>>  	while ((irqs = deliverable_irqs(vcpu)) && !rc) {
>>  		/* bits are in the order of interrupt priority */
> 
> this comment should now be "reversed order"

will fix.
> 
>> -		irq_type = find_first_bit(&irqs, IRQ_PEND_COUNT);
>> +		irq_type = find_last_bit(&irqs, IRQ_PEND_COUNT);
>>  		if (is_ioirq(irq_type)) {
>>  			rc = __deliver_io(vcpu, irq_type);
>>  		} else {
>>
> 
> Looks sane to me on the first sight.
> 

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

[David Hildenbrand]

On 16.01.2018 21:02, 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 +++++++++++++++++++++++
>  arch/s390/kvm/kvm-s390.h  |  4 ++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index b94173560dcf..dfdecff302d2 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -2682,3 +2682,26 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu, __u8 __user *buf, int len)
>  
>  	return n;
>  }
> +
> +#define IPM_BIT_OFFSET (offsetof(struct kvm_s390_gisa, ipm) * 8)

8 -> BITS_PER_BYTE, but ...

Am I wrong or can we only modify the 8 ipm bits this way? But we
want/have to do it in an atomic fashion?

Using an unsigned long seems wrong, because we "rewrite" more than we
should. Esp. everything beyond ipm. oi / ni and friends are not
available on older machines.

What about something as simple as the following


+void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u8 gisc)
+{
+       u8 value = (0x80 >> gisc);
+
+       __sync_fetch_and_or(&gisa->ipm, value);
+}
+


> +
> +void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
> +{
> +	set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
> +}
> +
> +u8 kvm_s390_gisa_get_ipm(struct kvm_s390_gisa *gisa)
> +{
> +	return (u8) READ_ONCE(gisa->ipm);
> +}
> +
> +void kvm_s390_gisa_clear_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
> +{
> +	clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
> +}
> +
> +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);
> +}
> +

shouldn't these be static inline instead?

> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> index 8877116f0159..b17e4dea7ea5 100644
> --- a/arch/s390/kvm/kvm-s390.h
> +++ b/arch/s390/kvm/kvm-s390.h
> @@ -367,6 +367,10 @@ 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_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc);
> +u8 kvm_s390_gisa_get_ipm(struct kvm_s390_gisa *gisa);
> +void kvm_s390_gisa_clear_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc);
> +int kvm_s390_gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc);
>  
>  /* implemented in guestdbg.c */
>  void kvm_s390_backup_guest_per_regs(struct kvm_vcpu *vcpu);
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH 05/12] s390/css: expose the AIV facility

[David Hildenbrand]

On 16.01.2018 21:02, Christian Borntraeger wrote:
> From: Michael Mueller <mimu@linux.vnet.ibm.com>
> 
> The patch exposes the 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>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  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 a478eb61aaf7..fb56fa3283a2 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 */
> 

"Expose" sounds like actually forwarding something / enabling a bit.
Wonder if this can be squashed with another patch?

Or rename to something like "define" ...

-- 

Thanks,

David / dhildenb

Re: [PATCH 05/12] s390/css: expose the AIV facility

[Michael Mueller]

On 17.01.18 16:19, David Hildenbrand wrote:
> On 16.01.2018 21:02, Christian Borntraeger wrote:
>> From: Michael Mueller <mimu@linux.vnet.ibm.com>
>>
>> The patch exposes the 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>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>   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 a478eb61aaf7..fb56fa3283a2 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 */
>>
> "Expose" sounds like actually forwarding something / enabling a bit.
Expose here basically means to make sth. visible what was already available.
> Wonder if this can be squashed with another patch?
>
> Or rename to something like "define" ...
I don't plan to change anything.
>
Thanks,
Michael

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

[Michael Mueller]

On 17.01.18 15:35, David Hildenbrand wrote:
> On 16.01.2018 21:02, 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 +++++++++++++++++++++++
>>   arch/s390/kvm/kvm-s390.h  |  4 ++++
>>   2 files changed, 27 insertions(+)
>>
>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>> index b94173560dcf..dfdecff302d2 100644
>> --- a/arch/s390/kvm/interrupt.c
>> +++ b/arch/s390/kvm/interrupt.c
>> @@ -2682,3 +2682,26 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu, __u8 __user *buf, int len)
>>   
>>   	return n;
>>   }
>> +
>> +#define IPM_BIT_OFFSET (offsetof(struct kvm_s390_gisa, ipm) * 8)
> 
> 8 -> BITS_PER_BYTE, but ...
> 
> Am I wrong or can we only modify the 8 ipm bits this way? But we
> want/have to do it in an atomic fashion?
> 
> Using an unsigned long seems wrong, because we "rewrite" more than we
> should. Esp. everything beyond ipm. oi / ni and friends are not
> available on older machines.
> 
> What about something as simple as the following
> 
> 
> +void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u8 gisc)
> +{
> +       u8 value = (0x80 >> gisc);
> +
> +       __sync_fetch_and_or(&gisa->ipm, value);
> +}
> +
> 

Nobody is using this compiler build-in in the kernel and a quick compile 
shows that it produces an ORK and CS instead of a LAOG beside a lot of 
supporting instructions. Unfortunately it's not saving what you promise 
... ;) Will not change.

> 
>> +
>> +void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>> +{
>> +	set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
>> +}
>> +
>> +u8 kvm_s390_gisa_get_ipm(struct kvm_s390_gisa *gisa)
>> +{
>> +	return (u8) READ_ONCE(gisa->ipm);
>> +}
>> +
>> +void kvm_s390_gisa_clear_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>> +{
>> +	clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
>> +}
>> +
>> +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);
>> +}
>> +
> 
> shouldn't these be static inline instead?

Well, I get requests in both directions for that... my opinion is also 
yes and I will change it a very last time!

> 
>> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
>> index 8877116f0159..b17e4dea7ea5 100644
>> --- a/arch/s390/kvm/kvm-s390.h
>> +++ b/arch/s390/kvm/kvm-s390.h
>> @@ -367,6 +367,10 @@ 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_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc);
>> +u8 kvm_s390_gisa_get_ipm(struct kvm_s390_gisa *gisa);
>> +void kvm_s390_gisa_clear_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc);
>> +int kvm_s390_gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc);
>>   
>>   /* implemented in guestdbg.c */
>>   void kvm_s390_backup_guest_per_regs(struct kvm_vcpu *vcpu);
>>
> 

Thanks a lot for your comments David.

Michael

> 

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

[David Hildenbrand]

On 18.01.2018 15:29, Michael Mueller wrote:
> 
> 
> On 17.01.18 15:35, David Hildenbrand wrote:
>> On 16.01.2018 21:02, 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 +++++++++++++++++++++++
>>>   arch/s390/kvm/kvm-s390.h  |  4 ++++
>>>   2 files changed, 27 insertions(+)
>>>
>>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>>> index b94173560dcf..dfdecff302d2 100644
>>> --- a/arch/s390/kvm/interrupt.c
>>> +++ b/arch/s390/kvm/interrupt.c
>>> @@ -2682,3 +2682,26 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu, __u8 __user *buf, int len)
>>>   
>>>   	return n;
>>>   }
>>> +
>>> +#define IPM_BIT_OFFSET (offsetof(struct kvm_s390_gisa, ipm) * 8)
>>
>> 8 -> BITS_PER_BYTE, but ...
>>
>> Am I wrong or can we only modify the 8 ipm bits this way? But we
>> want/have to do it in an atomic fashion?
>>
>> Using an unsigned long seems wrong, because we "rewrite" more than we
>> should. Esp. everything beyond ipm. oi / ni and friends are not
>> available on older machines.
>>
>> What about something as simple as the following
>>
>>
>> +void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u8 gisc)
>> +{
>> +       u8 value = (0x80 >> gisc);
>> +
>> +       __sync_fetch_and_or(&gisa->ipm, value);
>> +}
>> +
>>
> 
> Nobody is using this compiler build-in in the kernel and a quick compile 
> shows that it produces an ORK and CS instead of a LAOG beside a lot of 
> supporting instructions. Unfortunately it's not saving what you promise 
> ... ;) Will not change.
> 

Using unsigned long * bitmap operations to modify an u8 type atomically
just seems very very wrong.


-- 

Thanks,

David / dhildenb

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

[Michael Mueller]

On 17.01.18 08:57, Heiko Carstens wrote:
> On Tue, Jan 16, 2018 at 09:25:12PM +0100, David Hildenbrand wrote:
>>
>>> +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 __aligned(sizeof(struct kvm_s390_gisa)); /* 0x0900 */
>>
>> can we instead specify sie_page2 as packed if really needed? (I can see
>> that we have a BUILD_BUG_ON below, which is nice)
>>
>> (alignment within well defined data structures looks strange)

I'm doing that because the gisa requires "integral boundary" alignment 
that changes with a later patch when I introduce format-1.

> 
> The alignment, if needed, should go to the definition of struct
> kvm_s390_gisa above. The structure needs at least an alignment of eight
> bytes since bitops are used to modify ipm (in a later patch), by using a
> (long *) cast to this structure.

adding this to the definition does not allow to use sizeof() for the 
currently defined struct. Thus I would have to explicitly specify the
size... :(

> But of course it is all naturally aligned anyway... ;)
> 

Thanks
Michael

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

[Michael Mueller]

On 17.01.18 08:57, Heiko Carstens wrote:
> On Tue, Jan 16, 2018 at 09:25:12PM +0100, David Hildenbrand wrote:
>>
>>> +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 __aligned(sizeof(struct kvm_s390_gisa)); /* 0x0900 */
>>
>> can we instead specify sie_page2 as packed if really needed? (I can see
>> that we have a BUILD_BUG_ON below, which is nice)
>>
>> (alignment within well defined data structures looks strange)

I'm doing that because the gisa requires "integral boundary" alignment 
that changes with a later patch when I introduce format-1.

> 
> The alignment, if needed, should go to the definition of struct
> kvm_s390_gisa above. The structure needs at least an alignment of eight
> bytes since bitops are used to modify ipm (in a later patch), by using a
> (long *) cast to this structure.

adding this to the definition does not allow to use sizeof() for the 
currently defined struct. Thus I would have to explicitly specify the
size... :(

> But of course it is all naturally aligned anyway... ;)
> 

Thanks
Michael

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

[Michael Mueller]

On 18.01.18 15:33, David Hildenbrand wrote:
> On 18.01.2018 15:29, Michael Mueller wrote:
>>
>>
>> On 17.01.18 15:35, David Hildenbrand wrote:
>>> On 16.01.2018 21:02, 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 +++++++++++++++++++++++
>>>>    arch/s390/kvm/kvm-s390.h  |  4 ++++
>>>>    2 files changed, 27 insertions(+)
>>>>
>>>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>>>> index b94173560dcf..dfdecff302d2 100644
>>>> --- a/arch/s390/kvm/interrupt.c
>>>> +++ b/arch/s390/kvm/interrupt.c
>>>> @@ -2682,3 +2682,26 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu, __u8 __user *buf, int len)
>>>>    
>>>>    	return n;
>>>>    }
>>>> +
>>>> +#define IPM_BIT_OFFSET (offsetof(struct kvm_s390_gisa, ipm) * 8)
>>>
>>> 8 -> BITS_PER_BYTE, but ...
>>>
>>> Am I wrong or can we only modify the 8 ipm bits this way? But we
>>> want/have to do it in an atomic fashion?
>>>
>>> Using an unsigned long seems wrong, because we "rewrite" more than we
>>> should. Esp. everything beyond ipm. oi / ni and friends are not
>>> available on older machines.
>>>
>>> What about something as simple as the following
>>>
>>>
>>> +void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u8 gisc)
>>> +{
>>> +       u8 value = (0x80 >> gisc);
>>> +
>>> +       __sync_fetch_and_or(&gisa->ipm, value);
>>> +}
>>> +
>>>
>>
>> Nobody is using this compiler build-in in the kernel and a quick compile
>> shows that it produces an ORK and CS instead of a LAOG beside a lot of
>> supporting instructions. Unfortunately it's not saving what you promise
>> ... ;) Will not change.
>>
> 
> Using unsigned long * bitmap operations to modify an u8 type atomically
> just seems very very wrong >

I have to reconsider this...

> 

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

[Cornelia Huck]

On Tue, 16 Jan 2018 21:02:06 +0100
Christian Borntraeger <borntraeger@de.ibm.com> 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 reject either by performance or

s/to reject/to be rejected/

> 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>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/include/asm/kvm_host.h | 54 ++++++++++++++++++++--------------------
>  arch/s390/kvm/interrupt.c        | 10 ++++----
>  2 files changed, 32 insertions(+), 32 deletions(-)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index e16a9f2a44ad..9981721f258f 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 */

"ascending order of priority"?

Otherwise (and with the "reversed order" change),

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

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

[Cornelia Huck]

On Tue, 16 Jan 2018 21:02:08 +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>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/include/asm/bitops.h | 5 +++++
>  1 file changed, 5 insertions(+)

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

Re: [PATCH 05/12] s390/css: expose the AIV facility

[Cornelia Huck]

On Thu, 18 Jan 2018 13:02:45 +0100
Michael Mueller <mimu@linux.vnet.ibm.com> wrote:

> On 17.01.18 16:19, David Hildenbrand wrote:
> > On 16.01.2018 21:02, Christian Borntraeger wrote:  
> >> From: Michael Mueller <mimu@linux.vnet.ibm.com>
> >>
> >> The patch exposes the 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>
> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >> ---
> >>   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 a478eb61aaf7..fb56fa3283a2 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 */
> >>  
> > "Expose" sounds like actually forwarding something / enabling a bit.  
> Expose here basically means to make sth. visible what was already available.
> > Wonder if this can be squashed with another patch?
> >
> > Or rename to something like "define" ...  
> I don't plan to change anything.

I'd prefer to simply squash this with the next patch, where the bit is
actually checked.

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

[Cornelia Huck]

On Tue, 16 Jan 2018 21:02:11 +0100
Christian Borntraeger <borntraeger@de.ibm.com> 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/kvm/interrupt.c | 104 +++++++++++++++++++++++++++++++++++++---------
>  arch/s390/kvm/kvm-s390.c  |   8 ++++
>  arch/s390/kvm/kvm-s390.h  |   3 ++
>  3 files changed, 96 insertions(+), 19 deletions(-)
> 

> @@ -935,23 +960,27 @@ 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;
>  	}
>  
> +	if (vcpu->kvm->arch.gisa) {
> +		if (kvm_s390_gisa_tac_ipm_gisc(vcpu->kvm->arch.gisa, isc)) {

I think this could benefit from a comment here or there; e.g. that you
manually deliver interrupts here that have not yet been injected via
gisa. This is complex enough to confuse people having to look at this
some years from now :)

> +			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 ? -EFAULT : 0;
>  }
>  

The general approach seems sane from what I can see (unless I'm already
too tired...)

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

[Cornelia Huck]

On Tue, 16 Jan 2018 21:02:12 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> 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>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/kvm/interrupt.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

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

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

[David Hildenbrand]

On 18.01.2018 16:58, Michael Mueller wrote:
> 
> 
> On 18.01.18 15:33, David Hildenbrand wrote:
>> On 18.01.2018 15:29, Michael Mueller wrote:
>>>
>>>
>>> On 17.01.18 15:35, David Hildenbrand wrote:
>>>> On 16.01.2018 21:02, 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 +++++++++++++++++++++++
>>>>>    arch/s390/kvm/kvm-s390.h  |  4 ++++
>>>>>    2 files changed, 27 insertions(+)
>>>>>
>>>>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>>>>> index b94173560dcf..dfdecff302d2 100644
>>>>> --- a/arch/s390/kvm/interrupt.c
>>>>> +++ b/arch/s390/kvm/interrupt.c
>>>>> @@ -2682,3 +2682,26 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu, __u8 __user *buf, int len)
>>>>>    
>>>>>    	return n;
>>>>>    }
>>>>> +
>>>>> +#define IPM_BIT_OFFSET (offsetof(struct kvm_s390_gisa, ipm) * 8)
>>>>
>>>> 8 -> BITS_PER_BYTE, but ...
>>>>
>>>> Am I wrong or can we only modify the 8 ipm bits this way? But we
>>>> want/have to do it in an atomic fashion?
>>>>
>>>> Using an unsigned long seems wrong, because we "rewrite" more than we
>>>> should. Esp. everything beyond ipm. oi / ni and friends are not
>>>> available on older machines.
>>>>
>>>> What about something as simple as the following
>>>>
>>>>
>>>> +void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u8 gisc)
>>>> +{
>>>> +       u8 value = (0x80 >> gisc);
>>>> +
>>>> +       __sync_fetch_and_or(&gisa->ipm, value);
>>>> +}
>>>> +
>>>>
>>>
>>> Nobody is using this compiler build-in in the kernel and a quick compile
>>> shows that it produces an ORK and CS instead of a LAOG beside a lot of
>>> supporting instructions. Unfortunately it's not saving what you promise
>>> ... ;) Will not change.
>>>
>>
>> Using unsigned long * bitmap operations to modify an u8 type atomically
>> just seems very very wrong >
> 
> I have to reconsider this...
> 
>>
> 

Actually, the problem is there are no atomic byte-based operations on
s390x without Interlocked-Access-Facility 2.

Even __sync_fetch_and_or(&gisa->ipm, value) falls back to a
Compare-And-Swap loop. And Compare-And-Swap also operates at least on 32bit.

So I assume there isn't too much we can do about it. As storage
locations following the u8 are also written - but in an atomic matter,
it should in general not matter.

But can we avoid starting the bitmap at the beginning of the gisa?

What about something like this:

+void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u8 gisc)
+{
+       set_bit_inv(gisc, (unsigned long *) &gisa->ipm);
+}

-- 

Thanks,

David / dhildenb

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

[David Hildenbrand]

Two minor things

>  
> +struct kvm_s390_gisa {
> +	u32 next_alert;
> +	u8 ipm;
> +	u8 reserved01;
> +	u8:6;

Mind giving this also a reserved name

> +	u8 g:1;
> +	u8 c:1;

Can you put spaces before/after the colon?

> +	u8 iam;
> +	u8 reserved02[4];
> +	u32 airq_count;
> +};
> +
>  /*


-- 

Thanks,

David / dhildenb

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

[Heiko Carstens]

On Thu, Jan 18, 2018 at 09:45:03PM +0100, David Hildenbrand wrote:
> Actually, the problem is there are no atomic byte-based operations on
> s390x without Interlocked-Access-Facility 2.
> 
> Even __sync_fetch_and_or(&gisa->ipm, value) falls back to a
> Compare-And-Swap loop. And Compare-And-Swap also operates at least on 32bit.
> 
> So I assume there isn't too much we can do about it. As storage
> locations following the u8 are also written - but in an atomic matter,
> it should in general not matter.
> 
> But can we avoid starting the bitmap at the beginning of the gisa?
> 
> What about something like this:
> 
> +void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u8 gisc)
> +{
> +       set_bit_inv(gisc, (unsigned long *) &gisa->ipm);
> +}

set_bit_inv() may use a csg instruction which requires an 8 byte alignment
of the operand. What you propose would crash immediately.

The code written by Michael is fine as-is.

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

[Heiko Carstens]

On Thu, Jan 18, 2018 at 09:47:01PM +0100, David Hildenbrand wrote:
> Two minor things
> 
> >  
> > +struct kvm_s390_gisa {
> > +	u32 next_alert;
> > +	u8 ipm;
> > +	u8 reserved01;
> > +	u8:6;
> 
> Mind giving this also a reserved name

And then all reserved fields have to be renamed as soon as one bit gets
used? Please don't...

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

[David Hildenbrand]

On 19.01.2018 11:11, Heiko Carstens wrote:
> On Thu, Jan 18, 2018 at 09:45:03PM +0100, David Hildenbrand wrote:
>> Actually, the problem is there are no atomic byte-based operations on
>> s390x without Interlocked-Access-Facility 2.
>>
>> Even __sync_fetch_and_or(&gisa->ipm, value) falls back to a
>> Compare-And-Swap loop. And Compare-And-Swap also operates at least on 32bit.
>>
>> So I assume there isn't too much we can do about it. As storage
>> locations following the u8 are also written - but in an atomic matter,
>> it should in general not matter.
>>
>> But can we avoid starting the bitmap at the beginning of the gisa?
>>
>> What about something like this:
>>
>> +void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u8 gisc)
>> +{
>> +       set_bit_inv(gisc, (unsigned long *) &gisa->ipm);
>> +}
> 
> set_bit_inv() may use a csg instruction which requires an 8 byte alignment
> of the operand. What you propose would crash immediately.
> 
> The code written by Michael is fine as-is.
> 

That's unfortunate... and still looks hacky to me :) But if it works ...

-- 

Thanks,

David / dhildenb

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

[David Hildenbrand]

On 19.01.2018 11:12, Heiko Carstens wrote:
> On Thu, Jan 18, 2018 at 09:47:01PM +0100, David Hildenbrand wrote:
>> Two minor things
>>
>>>  
>>> +struct kvm_s390_gisa {
>>> +	u32 next_alert;
>>> +	u8 ipm;
>>> +	u8 reserved01;
>>> +	u8:6;
>>
>> Mind giving this also a reserved name
> 
> And then all reserved fields have to be renamed as soon as one bit gets
> used? Please don't...
> 

Only if one keeps the order of the reserved field numbers ...

-- 

Thanks,

David / dhildenb

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

[Christian Borntraeger]

On 01/19/2018 11:16 AM, David Hildenbrand wrote:
> On 19.01.2018 11:11, Heiko Carstens wrote:
>> On Thu, Jan 18, 2018 at 09:45:03PM +0100, David Hildenbrand wrote:
>>> Actually, the problem is there are no atomic byte-based operations on
>>> s390x without Interlocked-Access-Facility 2.
>>>
>>> Even __sync_fetch_and_or(&gisa->ipm, value) falls back to a
>>> Compare-And-Swap loop. And Compare-And-Swap also operates at least on 32bit.
>>>
>>> So I assume there isn't too much we can do about it. As storage
>>> locations following the u8 are also written - but in an atomic matter,
>>> it should in general not matter.
>>>
>>> But can we avoid starting the bitmap at the beginning of the gisa?
>>>
>>> What about something like this:
>>>
>>> +void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u8 gisc)
>>> +{
>>> +       set_bit_inv(gisc, (unsigned long *) &gisa->ipm);
>>> +}
>>
>> set_bit_inv() may use a csg instruction which requires an 8 byte alignment
>> of the operand. What you propose would crash immediately.
>>
>> The code written by Michael is fine as-is.
>>
> 
> That's unfortunate... and still looks hacky to me :) But if it works ...

I had looked at the same place, but the alignment thing makes this really
hard and I did not find a better solution.

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

[Heiko Carstens]

On Fri, Jan 19, 2018 at 11:17:10AM +0100, David Hildenbrand wrote:
> On 19.01.2018 11:12, Heiko Carstens wrote:
> > On Thu, Jan 18, 2018 at 09:47:01PM +0100, David Hildenbrand wrote:
> >> Two minor things
> >>
> >>>  
> >>> +struct kvm_s390_gisa {
> >>> +	u32 next_alert;
> >>> +	u8 ipm;
> >>> +	u8 reserved01;
> >>> +	u8:6;
> >>
> >> Mind giving this also a reserved name
> > 
> > And then all reserved fields have to be renamed as soon as one bit gets
> > used? Please don't...
> 
> Only if one keeps the order of the reserved field numbers ...

And that's what people usually do. Therefore having unnamed bitfields is
nice, since you don't have to care.

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

[Cornelia Huck]

On Fri, 19 Jan 2018 11:20:39 +0100
Heiko Carstens <heiko.carstens@de.ibm.com> wrote:

> On Fri, Jan 19, 2018 at 11:17:10AM +0100, David Hildenbrand wrote:
> > On 19.01.2018 11:12, Heiko Carstens wrote:  
> > > On Thu, Jan 18, 2018 at 09:47:01PM +0100, David Hildenbrand wrote:  
> > >> Two minor things
> > >>  
> > >>>  
> > >>> +struct kvm_s390_gisa {
> > >>> +	u32 next_alert;
> > >>> +	u8 ipm;
> > >>> +	u8 reserved01;
> > >>> +	u8:6;  
> > >>
> > >> Mind giving this also a reserved name  
> > > 
> > > And then all reserved fields have to be renamed as soon as one bit gets
> > > used? Please don't...  
> > 
> > Only if one keeps the order of the reserved field numbers ...  
> 
> And that's what people usually do. Therefore having unnamed bitfields is
> nice, since you don't have to care.
> 

+1

They make these kinds of definitions so much nicer...

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

[David Hildenbrand]

On 19.01.2018 11:29, Cornelia Huck wrote:
> On Fri, 19 Jan 2018 11:20:39 +0100
> Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
> 
>> On Fri, Jan 19, 2018 at 11:17:10AM +0100, David Hildenbrand wrote:
>>> On 19.01.2018 11:12, Heiko Carstens wrote:  
>>>> On Thu, Jan 18, 2018 at 09:47:01PM +0100, David Hildenbrand wrote:  
>>>>> Two minor things
>>>>>  
>>>>>>  
>>>>>> +struct kvm_s390_gisa {
>>>>>> +	u32 next_alert;
>>>>>> +	u8 ipm;
>>>>>> +	u8 reserved01;
>>>>>> +	u8:6;  
>>>>>
>>>>> Mind giving this also a reserved name  
>>>>
>>>> And then all reserved fields have to be renamed as soon as one bit gets
>>>> used? Please don't...  
>>>
>>> Only if one keeps the order of the reserved field numbers ...  
>>
>> And that's what people usually do. Therefore having unnamed bitfields is
>> nice, since you don't have to care.
>>
> 
> +1
> 
> They make these kinds of definitions so much nicer...
> 

I'm convinced :)

-- 

Thanks,

David / dhildenb

Re: [PATCH 05/12] s390/css: expose the AIV facility

[Christian Borntraeger]

On 01/18/2018 06:54 PM, Cornelia Huck wrote:
> On Thu, 18 Jan 2018 13:02:45 +0100
> Michael Mueller <mimu@linux.vnet.ibm.com> wrote:
> 
>> On 17.01.18 16:19, David Hildenbrand wrote:
>>> On 16.01.2018 21:02, Christian Borntraeger wrote:  
>>>> From: Michael Mueller <mimu@linux.vnet.ibm.com>
>>>>
>>>> The patch exposes the 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>
>>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> ---
>>>>   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 a478eb61aaf7..fb56fa3283a2 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 */
>>>>  
>>> "Expose" sounds like actually forwarding something / enabling a bit.  
>> Expose here basically means to make sth. visible what was already available.
>>> Wonder if this can be squashed with another patch?
>>>
>>> Or rename to something like "define" ...  
>> I don't plan to change anything.
> 
> I'd prefer to simply squash this with the next patch, where the bit is
> actually checked.

Its a separate code (not KVM) so I keep it separate.
What about


s390/css: indicate the availability of the AIV facility
  
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>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
[change wording]

Re: [PATCH 05/12] s390/css: expose the AIV facility

[Cornelia Huck]

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

> On 01/18/2018 06:54 PM, Cornelia Huck wrote:
> > On Thu, 18 Jan 2018 13:02:45 +0100
> > Michael Mueller <mimu@linux.vnet.ibm.com> wrote:
> >   
> >> On 17.01.18 16:19, David Hildenbrand wrote:  
> >>> On 16.01.2018 21:02, Christian Borntraeger wrote:    
> >>>> From: Michael Mueller <mimu@linux.vnet.ibm.com>
> >>>>
> >>>> The patch exposes the 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>
> >>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >>>> ---
> >>>>   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 a478eb61aaf7..fb56fa3283a2 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 */
> >>>>    
> >>> "Expose" sounds like actually forwarding something / enabling a bit.    
> >> Expose here basically means to make sth. visible what was already available.  
> >>> Wonder if this can be squashed with another patch?
> >>>
> >>> Or rename to something like "define" ...    
> >> I don't plan to change anything.  
> > 
> > I'd prefer to simply squash this with the next patch, where the bit is
> > actually checked.  
> 
> Its a separate code (not KVM) so I keep it separate.

I would just have collected an ack from the relevant maintainers, but
fair enough.

> What about
> 
> 
> s390/css: indicate the availability of the AIV facility
>   
> 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>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> [change wording]
> 

Also works for me.

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

Re: [PATCH 05/12] s390/css: expose the AIV facility

[Christian Borntraeger]

On 01/25/2018 01:00 PM, Cornelia Huck wrote:
> On Thu, 25 Jan 2018 12:42:28 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 01/18/2018 06:54 PM, Cornelia Huck wrote:
>>> On Thu, 18 Jan 2018 13:02:45 +0100
>>> Michael Mueller <mimu@linux.vnet.ibm.com> wrote:
>>>   
>>>> On 17.01.18 16:19, David Hildenbrand wrote:  
>>>>> On 16.01.2018 21:02, Christian Borntraeger wrote:    
>>>>>> From: Michael Mueller <mimu@linux.vnet.ibm.com>
>>>>>>
>>>>>> The patch exposes the 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>
>>>>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>>>> ---
>>>>>>   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 a478eb61aaf7..fb56fa3283a2 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 */
>>>>>>    
>>>>> "Expose" sounds like actually forwarding something / enabling a bit.    
>>>> Expose here basically means to make sth. visible what was already available.  
>>>>> Wonder if this can be squashed with another patch?
>>>>>
>>>>> Or rename to something like "define" ...    
>>>> I don't plan to change anything.  
>>>
>>> I'd prefer to simply squash this with the next patch, where the bit is
>>> actually checked.  
>>
>> Its a separate code (not KVM) so I keep it separate.
> 
> I would just have collected an ack from the relevant maintainers, but
> fair enough.

I need that anyway and its easier if I just have this hunk.

Heiko Martin, can any of you ack

this patch ([PATCH 05/12] s390/css: expose the AIV facility)
as well as
[PATCH 03/12] s390/bitops: add test_and_clear_bit_inv()
and
[PATCH 11/12] s390/sclp: expose the GISA format facility

for handling these patches via the KVM tree.


>>
>>
>> s390/css: indicate the availability of the AIV facility
>>   
>> 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>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> [change wording]
>>
> 
> Also works for me.
> 
> Acked-by: Cornelia Huck <cohuck@redhat.com>
Thanks.

Re: [PATCH 05/12] s390/css: expose the AIV facility

[Heiko Carstens]

On Thu, Jan 25, 2018 at 01:04:42PM +0100, Christian Borntraeger wrote:
> >> Its a separate code (not KVM) so I keep it separate.
> > 
> > I would just have collected an ack from the relevant maintainers, but
> > fair enough.
> 
> I need that anyway and its easier if I just have this hunk.
> 
> Heiko Martin, can any of you ack
> 
> this patch ([PATCH 05/12] s390/css: expose the AIV facility)
> as well as
> [PATCH 03/12] s390/bitops: add test_and_clear_bit_inv()
> and
> [PATCH 11/12] s390/sclp: expose the GISA format facility
> 
> for handling these patches via the KVM tree.

Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com>