All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/5] s390: Host support for channel I/O.
@ 2012-12-07 12:30 Cornelia Huck
  2012-12-07 12:30 ` [PATCH 1/5] KVM: s390: Support for I/O interrupts Cornelia Huck
                   ` (4 more replies)
  0 siblings, 5 replies; 24+ messages in thread
From: Cornelia Huck @ 2012-12-07 12:30 UTC (permalink / raw)
  To: Marcelo Tosatti, Gleb Natapov, KVM, linux-s390
  Cc: Avi Kivity, Christian Borntraeger, Carsten Otte, Alexander Graf,
	Heiko Carstens, Martin Schwidefsky, Sebastian Ott

Hi,

here are the host support patches for virtual channel I/O,
rebased against kvm/next.

Changes from v3:
- Corrected a wrong bitmask in machine check handling.
- Added Marcelo's Reviewed-by:

Please consider these patches for kvm/next.

Cornelia Huck (5):
  KVM: s390: Support for I/O interrupts.
  KVM: s390: Add support for machine checks.
  KVM: s390: In-kernel handling of I/O instructions.
  KVM: s390: Base infrastructure for enabling capabilities.
  KVM: s390: Add support for channel I/O instructions.

 Documentation/virtual/kvm/api.txt |  40 +++++-
 arch/s390/include/asm/kvm_host.h  |  11 ++
 arch/s390/kvm/intercept.c         |  22 ++-
 arch/s390/kvm/interrupt.c         | 264 +++++++++++++++++++++++++++++++++++-
 arch/s390/kvm/kvm-s390.c          |  38 ++++++
 arch/s390/kvm/kvm-s390.h          |   6 +
 arch/s390/kvm/priv.c              | 275 +++++++++++++++++++++++++++++++++++---
 arch/s390/kvm/trace-s390.h        |  26 +++-
 include/trace/events/kvm.h        |   2 +-
 include/uapi/linux/kvm.h          |  18 +++
 10 files changed, 673 insertions(+), 29 deletions(-)

-- 
1.7.12.4

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 1/5] KVM: s390: Support for I/O interrupts.
  2012-12-07 12:30 [PATCH v4 0/5] s390: Host support for channel I/O Cornelia Huck
@ 2012-12-07 12:30 ` Cornelia Huck
  2012-12-10  7:33   ` Alexander Graf
  2012-12-07 12:30 ` [PATCH 2/5] KVM: s390: Add support for machine checks Cornelia Huck
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 24+ messages in thread
From: Cornelia Huck @ 2012-12-07 12:30 UTC (permalink / raw)
  To: Marcelo Tosatti, Gleb Natapov, KVM, linux-s390
  Cc: Avi Kivity, Christian Borntraeger, Carsten Otte, Alexander Graf,
	Heiko Carstens, Martin Schwidefsky, Sebastian Ott

Add support for handling I/O interrupts (standard, subchannel-related
ones and rudimentary adapter interrupts).

The subchannel-identifying parameters are encoded into the interrupt
type.

I/O interrupts are floating, so they can't be injected on a specific
vcpu.

Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 Documentation/virtual/kvm/api.txt |   4 ++
 arch/s390/include/asm/kvm_host.h  |   2 +
 arch/s390/kvm/interrupt.c         | 115 ++++++++++++++++++++++++++++++++++++--
 include/uapi/linux/kvm.h          |   6 ++
 4 files changed, 122 insertions(+), 5 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 6671fdc..e298a72 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -2068,6 +2068,10 @@ KVM_S390_INT_VIRTIO (vm) - virtio external interrupt; external interrupt
 KVM_S390_INT_SERVICE (vm) - sclp external interrupt; sclp parameter in parm
 KVM_S390_INT_EMERGENCY (vcpu) - sigp emergency; source cpu in parm
 KVM_S390_INT_EXTERNAL_CALL (vcpu) - sigp external call; source cpu in parm
+KVM_S390_INT_IO(ai,cssid,ssid,schid) (vm) - compound value to indicate an
+    I/O interrupt (ai - adapter interrupt; cssid,ssid,schid - subchannel);
+    I/O interruption parameters in parm (subchannel) and parm64 (intparm,
+    interruption subclass)
 
 Note that the vcpu ioctl is asynchronous to vcpu execution.
 
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index b784154..e47f697 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -76,6 +76,7 @@ struct kvm_s390_sie_block {
 	__u64	epoch;			/* 0x0038 */
 	__u8	reserved40[4];		/* 0x0040 */
 #define LCTL_CR0	0x8000
+#define LCTL_CR6	0x0200
 	__u16   lctl;			/* 0x0044 */
 	__s16	icpua;			/* 0x0046 */
 	__u32	ictl;			/* 0x0048 */
@@ -127,6 +128,7 @@ struct kvm_vcpu_stat {
 	u32 deliver_prefix_signal;
 	u32 deliver_restart_signal;
 	u32 deliver_program_int;
+	u32 deliver_io_int;
 	u32 exit_wait_state;
 	u32 instruction_stidp;
 	u32 instruction_spx;
diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index c30615e..070ba22 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -21,11 +21,26 @@
 #include "gaccess.h"
 #include "trace-s390.h"
 
+#define IOINT_SCHID_MASK 0x0000ffff
+#define IOINT_SSID_MASK 0x00030000
+#define IOINT_CSSID_MASK 0x03fc0000
+#define IOINT_AI_MASK 0x04000000
+
+static int is_ioint(u64 type)
+{
+	return ((type & 0xfffe0000u) != 0xfffe0000u);
+}
+
 static int psw_extint_disabled(struct kvm_vcpu *vcpu)
 {
 	return !(vcpu->arch.sie_block->gpsw.mask & PSW_MASK_EXT);
 }
 
+static int psw_ioint_disabled(struct kvm_vcpu *vcpu)
+{
+	return !(vcpu->arch.sie_block->gpsw.mask & PSW_MASK_IO);
+}
+
 static int psw_interrupts_disabled(struct kvm_vcpu *vcpu)
 {
 	if ((vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PER) ||
@@ -68,7 +83,18 @@ static int __interrupt_is_deliverable(struct kvm_vcpu *vcpu,
 	case KVM_S390_RESTART:
 		return 1;
 	default:
-		BUG();
+		if (is_ioint(inti->type)) {
+			if (psw_ioint_disabled(vcpu))
+				return 0;
+			if (vcpu->arch.sie_block->gcr[6] &
+			    inti->io.io_int_word)
+				return 1;
+			return 0;
+		} else {
+			printk(KERN_WARNING "illegal interrupt type %llx\n",
+			       inti->type);
+			BUG();
+		}
 	}
 	return 0;
 }
@@ -117,6 +143,13 @@ static void __set_intercept_indicator(struct kvm_vcpu *vcpu,
 		__set_cpuflag(vcpu, CPUSTAT_STOP_INT);
 		break;
 	default:
+		if (is_ioint(inti->type)) {
+			if (psw_ioint_disabled(vcpu))
+				__set_cpuflag(vcpu, CPUSTAT_IO_INT);
+			else
+				vcpu->arch.sie_block->lctl |= LCTL_CR6;
+			break;
+		}
 		BUG();
 	}
 }
@@ -298,7 +331,49 @@ static void __do_deliver_interrupt(struct kvm_vcpu *vcpu,
 		break;
 
 	default:
-		BUG();
+		if (is_ioint(inti->type)) {
+			__u32 param0 = ((__u32)inti->io.subchannel_id << 16) |
+				inti->io.subchannel_nr;
+			__u64 param1 = ((__u64)inti->io.io_int_parm << 32) |
+				inti->io.io_int_word;
+			VCPU_EVENT(vcpu, 4,
+				   "interrupt: I/O %llx", inti->type);
+			vcpu->stat.deliver_io_int++;
+			trace_kvm_s390_deliver_interrupt(vcpu->vcpu_id, inti->type,
+							 param0, param1);
+			rc = put_guest_u16(vcpu, __LC_SUBCHANNEL_ID,
+					   inti->io.subchannel_id);
+			if (rc == -EFAULT)
+				exception = 1;
+
+			rc = put_guest_u16(vcpu, __LC_SUBCHANNEL_NR,
+					   inti->io.subchannel_nr);
+			if (rc == -EFAULT)
+				exception = 1;
+
+			rc = put_guest_u32(vcpu, __LC_IO_INT_PARM,
+					   inti->io.io_int_parm);
+			if (rc == -EFAULT)
+				exception = 1;
+
+			rc = put_guest_u32(vcpu, __LC_IO_INT_WORD,
+					   inti->io.io_int_word);
+			if (rc == -EFAULT)
+				exception = 1;
+
+			rc = copy_to_guest(vcpu, __LC_IO_OLD_PSW,
+					   &vcpu->arch.sie_block->gpsw,
+					   sizeof(psw_t));
+			if (rc == -EFAULT)
+				exception = 1;
+
+			rc = copy_from_guest(vcpu, &vcpu->arch.sie_block->gpsw,
+					     __LC_IO_NEW_PSW, sizeof(psw_t));
+			if (rc == -EFAULT)
+				exception = 1;
+			break;
+		} else
+			BUG();
 	}
 	if (exception) {
 		printk("kvm: The guest lowcore is not mapped during interrupt "
@@ -545,7 +620,7 @@ int kvm_s390_inject_vm(struct kvm *kvm,
 {
 	struct kvm_s390_local_interrupt *li;
 	struct kvm_s390_float_interrupt *fi;
-	struct kvm_s390_interrupt_info *inti;
+	struct kvm_s390_interrupt_info *inti, *iter;
 	int sigcpu;
 
 	inti = kzalloc(sizeof(*inti), GFP_KERNEL);
@@ -569,9 +644,26 @@ int kvm_s390_inject_vm(struct kvm *kvm,
 	case KVM_S390_SIGP_STOP:
 	case KVM_S390_INT_EXTERNAL_CALL:
 	case KVM_S390_INT_EMERGENCY:
-	default:
 		kfree(inti);
 		return -EINVAL;
+	default:
+		if (!is_ioint(s390int->type)) {
+			kfree(inti);
+			return -EINVAL;
+		}
+		if (s390int->type & IOINT_AI_MASK)
+			VM_EVENT(kvm, 5, "%s", "inject: I/O (AI)");
+		else
+			VM_EVENT(kvm, 5, "inject: I/O css %x ss %x schid %04x",
+				 s390int->type & IOINT_CSSID_MASK,
+				 s390int->type & IOINT_SSID_MASK,
+				 s390int->type & IOINT_SCHID_MASK);
+		inti->type = s390int->type;
+		inti->io.subchannel_id = s390int->parm >> 16;
+		inti->io.subchannel_nr = s390int->parm & 0x0000ffffu;
+		inti->io.io_int_parm = s390int->parm64 >> 32;
+		inti->io.io_int_word = s390int->parm64 & 0x00000000ffffffffull;
+		break;
 	}
 	trace_kvm_s390_inject_vm(s390int->type, s390int->parm, s390int->parm64,
 				 2);
@@ -579,7 +671,19 @@ int kvm_s390_inject_vm(struct kvm *kvm,
 	mutex_lock(&kvm->lock);
 	fi = &kvm->arch.float_int;
 	spin_lock(&fi->lock);
-	list_add_tail(&inti->list, &fi->list);
+	if (!is_ioint(inti->type))
+		list_add_tail(&inti->list, &fi->list);
+	else {
+		/* Keep I/O interrupts sorted in isc order. */
+		list_for_each_entry(iter, &fi->list, list) {
+			if (!is_ioint(iter->type))
+				continue;
+			if (iter->io.io_int_word <= inti->io.io_int_word)
+				continue;
+			break;
+		}
+		list_add_tail(&inti->list, &iter->list);
+	}
 	atomic_set(&fi->active, 1);
 	sigcpu = find_first_bit(fi->idle_mask, KVM_MAX_VCPUS);
 	if (sigcpu == KVM_MAX_VCPUS) {
@@ -654,6 +758,7 @@ int kvm_s390_inject_vcpu(struct kvm_vcpu *vcpu,
 	case KVM_S390_INT_VIRTIO:
 	case KVM_S390_INT_SERVICE:
 	default:
+		/* also includes IOINT */
 		kfree(inti);
 		return -EINVAL;
 	}
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 494a84c..e676940 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -401,6 +401,12 @@ struct kvm_s390_psw {
 #define KVM_S390_INT_SERVICE		0xffff2401u
 #define KVM_S390_INT_EMERGENCY		0xffff1201u
 #define KVM_S390_INT_EXTERNAL_CALL	0xffff1202u
+#define KVM_S390_INT_IO(ai,cssid,ssid,schid)   \
+	(((schid)) |			       \
+	 ((ssid) << 16) |		       \
+	 ((cssid) << 18) |		       \
+	 ((ai) << 26))
+
 
 struct kvm_s390_interrupt {
 	__u32 type;
-- 
1.7.12.4

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 2/5] KVM: s390: Add support for machine checks.
  2012-12-07 12:30 [PATCH v4 0/5] s390: Host support for channel I/O Cornelia Huck
  2012-12-07 12:30 ` [PATCH 1/5] KVM: s390: Support for I/O interrupts Cornelia Huck
@ 2012-12-07 12:30 ` Cornelia Huck
  2012-12-10  7:51   ` Alexander Graf
  2012-12-19  9:44     ` Heiko Carstens
  2012-12-07 12:30 ` [PATCH 3/5] KVM: s390: In-kernel handling of I/O instructions Cornelia Huck
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 24+ messages in thread
From: Cornelia Huck @ 2012-12-07 12:30 UTC (permalink / raw)
  To: Marcelo Tosatti, Gleb Natapov, KVM, linux-s390
  Cc: Avi Kivity, Christian Borntraeger, Carsten Otte, Alexander Graf,
	Heiko Carstens, Martin Schwidefsky, Sebastian Ott

Add support for injecting machine checks (only repressible
conditions for now).

This is a bit more involved than I/O interrupts, for these reasons:

- Machine checks come in both floating and cpu varieties.
- We don't have a bit for machine checks enabling, but have to use
  a roundabout approach with trapping PSW changing instructions and
  watching for opened machine checks.

Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 Documentation/virtual/kvm/api.txt |   4 ++
 arch/s390/include/asm/kvm_host.h  |   8 +++
 arch/s390/kvm/intercept.c         |   2 +
 arch/s390/kvm/interrupt.c         | 112 ++++++++++++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.h          |   3 +
 arch/s390/kvm/priv.c              | 133 ++++++++++++++++++++++++++++++++++++++
 arch/s390/kvm/trace-s390.h        |   6 +-
 include/uapi/linux/kvm.h          |   1 +
 8 files changed, 266 insertions(+), 3 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index e298a72..8617339 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -2072,6 +2072,10 @@ KVM_S390_INT_IO(ai,cssid,ssid,schid) (vm) - compound value to indicate an
     I/O interrupt (ai - adapter interrupt; cssid,ssid,schid - subchannel);
     I/O interruption parameters in parm (subchannel) and parm64 (intparm,
     interruption subclass)
+KVM_S390_MCHK (vm, vcpu) - machine check interrupt; cr 14 bits in parm,
+                           machine check interrupt code in parm64 (note that
+                           machine checks needing further payload are not
+                           supported by this ioctl)
 
 Note that the vcpu ioctl is asynchronous to vcpu execution.
 
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index e47f697..773859e 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -77,8 +77,10 @@ struct kvm_s390_sie_block {
 	__u8	reserved40[4];		/* 0x0040 */
 #define LCTL_CR0	0x8000
 #define LCTL_CR6	0x0200
+#define LCTL_CR14	0x0002
 	__u16   lctl;			/* 0x0044 */
 	__s16	icpua;			/* 0x0046 */
+#define ICTL_LPSW 0x00400000
 	__u32	ictl;			/* 0x0048 */
 	__u32	eca;			/* 0x004c */
 	__u8	icptcode;		/* 0x0050 */
@@ -189,6 +191,11 @@ struct kvm_s390_emerg_info {
 	__u16 code;
 };
 
+struct kvm_s390_mchk_info {
+	__u64 cr14;
+	__u64 mcic;
+};
+
 struct kvm_s390_interrupt_info {
 	struct list_head list;
 	u64	type;
@@ -199,6 +206,7 @@ struct kvm_s390_interrupt_info {
 		struct kvm_s390_emerg_info emerg;
 		struct kvm_s390_extcall_info extcall;
 		struct kvm_s390_prefix_info prefix;
+		struct kvm_s390_mchk_info mchk;
 	};
 };
 
diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
index 22798ec..ec1177f 100644
--- a/arch/s390/kvm/intercept.c
+++ b/arch/s390/kvm/intercept.c
@@ -106,10 +106,12 @@ static int handle_lctl(struct kvm_vcpu *vcpu)
 
 static intercept_handler_t instruction_handlers[256] = {
 	[0x01] = kvm_s390_handle_01,
+	[0x82] = kvm_s390_handle_lpsw,
 	[0x83] = kvm_s390_handle_diag,
 	[0xae] = kvm_s390_handle_sigp,
 	[0xb2] = kvm_s390_handle_b2,
 	[0xb7] = handle_lctl,
+	[0xb9] = kvm_s390_handle_b9,
 	[0xe5] = kvm_s390_handle_e5,
 	[0xeb] = handle_lctlg,
 };
diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 070ba22..6b10267 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -41,6 +41,11 @@ static int psw_ioint_disabled(struct kvm_vcpu *vcpu)
 	return !(vcpu->arch.sie_block->gpsw.mask & PSW_MASK_IO);
 }
 
+static int psw_mchk_disabled(struct kvm_vcpu *vcpu)
+{
+	return !(vcpu->arch.sie_block->gpsw.mask & PSW_MASK_MCHECK);
+}
+
 static int psw_interrupts_disabled(struct kvm_vcpu *vcpu)
 {
 	if ((vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PER) ||
@@ -82,6 +87,12 @@ static int __interrupt_is_deliverable(struct kvm_vcpu *vcpu,
 	case KVM_S390_SIGP_SET_PREFIX:
 	case KVM_S390_RESTART:
 		return 1;
+	case KVM_S390_MCHK:
+		if (psw_mchk_disabled(vcpu))
+			return 0;
+		if (vcpu->arch.sie_block->gcr[14] & inti->mchk.cr14)
+			return 1;
+		return 0;
 	default:
 		if (is_ioint(inti->type)) {
 			if (psw_ioint_disabled(vcpu))
@@ -119,6 +130,7 @@ static void __reset_intercept_indicators(struct kvm_vcpu *vcpu)
 		CPUSTAT_IO_INT | CPUSTAT_EXT_INT | CPUSTAT_STOP_INT,
 		&vcpu->arch.sie_block->cpuflags);
 	vcpu->arch.sie_block->lctl = 0x0000;
+	vcpu->arch.sie_block->ictl &= ~ICTL_LPSW;
 }
 
 static void __set_cpuflag(struct kvm_vcpu *vcpu, u32 flag)
@@ -142,6 +154,12 @@ static void __set_intercept_indicator(struct kvm_vcpu *vcpu,
 	case KVM_S390_SIGP_STOP:
 		__set_cpuflag(vcpu, CPUSTAT_STOP_INT);
 		break;
+	case KVM_S390_MCHK:
+		if (psw_mchk_disabled(vcpu))
+			vcpu->arch.sie_block->ictl |= ICTL_LPSW;
+		else
+			vcpu->arch.sie_block->lctl |= LCTL_CR14;
+		break;
 	default:
 		if (is_ioint(inti->type)) {
 			if (psw_ioint_disabled(vcpu))
@@ -330,6 +348,32 @@ static void __do_deliver_interrupt(struct kvm_vcpu *vcpu,
 			exception = 1;
 		break;
 
+	case KVM_S390_MCHK:
+		VCPU_EVENT(vcpu, 4, "interrupt: machine check mcic=%llx",
+			   inti->mchk.mcic);
+		trace_kvm_s390_deliver_interrupt(vcpu->vcpu_id, inti->type,
+						 inti->mchk.cr14,
+						 inti->mchk.mcic);
+		rc = kvm_s390_vcpu_store_status(vcpu,
+						KVM_S390_STORE_STATUS_PREFIXED);
+		if (rc == -EFAULT)
+			exception = 1;
+
+		rc = put_guest_u64(vcpu, __LC_MCCK_CODE, inti->mchk.mcic);
+		if (rc == -EFAULT)
+			exception = 1;
+
+		rc = copy_to_guest(vcpu, __LC_MCK_OLD_PSW,
+				   &vcpu->arch.sie_block->gpsw, sizeof(psw_t));
+		if (rc == -EFAULT)
+			exception = 1;
+
+		rc = copy_from_guest(vcpu, &vcpu->arch.sie_block->gpsw,
+				     __LC_MCK_NEW_PSW, sizeof(psw_t));
+		if (rc == -EFAULT)
+			exception = 1;
+		break;
+
 	default:
 		if (is_ioint(inti->type)) {
 			__u32 param0 = ((__u32)inti->io.subchannel_id << 16) |
@@ -593,6 +637,61 @@ void kvm_s390_deliver_pending_interrupts(struct kvm_vcpu *vcpu)
 	}
 }
 
+void kvm_s390_deliver_pending_machine_checks(struct kvm_vcpu *vcpu)
+{
+	struct kvm_s390_local_interrupt *li = &vcpu->arch.local_int;
+	struct kvm_s390_float_interrupt *fi = vcpu->arch.local_int.float_int;
+	struct kvm_s390_interrupt_info  *n, *inti = NULL;
+	int deliver;
+
+	__reset_intercept_indicators(vcpu);
+	if (atomic_read(&li->active)) {
+		do {
+			deliver = 0;
+			spin_lock_bh(&li->lock);
+			list_for_each_entry_safe(inti, n, &li->list, list) {
+				if ((inti->type == KVM_S390_MCHK) &&
+				    __interrupt_is_deliverable(vcpu, inti)) {
+					list_del(&inti->list);
+					deliver = 1;
+					break;
+				}
+				__set_intercept_indicator(vcpu, inti);
+			}
+			if (list_empty(&li->list))
+				atomic_set(&li->active, 0);
+			spin_unlock_bh(&li->lock);
+			if (deliver) {
+				__do_deliver_interrupt(vcpu, inti);
+				kfree(inti);
+			}
+		} while (deliver);
+	}
+
+	if (atomic_read(&fi->active)) {
+		do {
+			deliver = 0;
+			spin_lock(&fi->lock);
+			list_for_each_entry_safe(inti, n, &fi->list, list) {
+				if ((inti->type == KVM_S390_MCHK) &&
+				    __interrupt_is_deliverable(vcpu, inti)) {
+					list_del(&inti->list);
+					deliver = 1;
+					break;
+				}
+				__set_intercept_indicator(vcpu, inti);
+			}
+			if (list_empty(&fi->list))
+				atomic_set(&fi->active, 0);
+			spin_unlock(&fi->lock);
+			if (deliver) {
+				__do_deliver_interrupt(vcpu, inti);
+				kfree(inti);
+			}
+		} while (deliver);
+	}
+}
+
 int kvm_s390_inject_program_int(struct kvm_vcpu *vcpu, u16 code)
 {
 	struct kvm_s390_local_interrupt *li = &vcpu->arch.local_int;
@@ -646,6 +745,13 @@ int kvm_s390_inject_vm(struct kvm *kvm,
 	case KVM_S390_INT_EMERGENCY:
 		kfree(inti);
 		return -EINVAL;
+	case KVM_S390_MCHK:
+		VM_EVENT(kvm, 5, "inject: machine check parm64:%llx",
+			 s390int->parm64);
+		inti->type = s390int->type;
+		inti->mchk.cr14 = s390int->parm; /* upper bits are not used */
+		inti->mchk.mcic = s390int->parm64;
+		break;
 	default:
 		if (!is_ioint(s390int->type)) {
 			kfree(inti);
@@ -755,6 +861,12 @@ int kvm_s390_inject_vcpu(struct kvm_vcpu *vcpu,
 		inti->type = s390int->type;
 		inti->emerg.code = s390int->parm;
 		break;
+	case KVM_S390_MCHK:
+		VCPU_EVENT(vcpu, 5, "inject: machine check parm64:%llx",
+			   s390int->parm64);
+		inti->type = s390int->type;
+		inti->mchk.mcic = s390int->parm64;
+		break;
 	case KVM_S390_INT_VIRTIO:
 	case KVM_S390_INT_SERVICE:
 	default:
diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
index d75bc5e..b1e1cb6 100644
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -69,6 +69,7 @@ int kvm_s390_handle_wait(struct kvm_vcpu *vcpu);
 enum hrtimer_restart kvm_s390_idle_wakeup(struct hrtimer *timer);
 void kvm_s390_tasklet(unsigned long parm);
 void kvm_s390_deliver_pending_interrupts(struct kvm_vcpu *vcpu);
+void kvm_s390_deliver_pending_machine_checks(struct kvm_vcpu *vcpu);
 int kvm_s390_inject_vm(struct kvm *kvm,
 		struct kvm_s390_interrupt *s390int);
 int kvm_s390_inject_vcpu(struct kvm_vcpu *vcpu,
@@ -80,6 +81,8 @@ int kvm_s390_inject_sigp_stop(struct kvm_vcpu *vcpu, int action);
 int kvm_s390_handle_b2(struct kvm_vcpu *vcpu);
 int kvm_s390_handle_e5(struct kvm_vcpu *vcpu);
 int kvm_s390_handle_01(struct kvm_vcpu *vcpu);
+int kvm_s390_handle_b9(struct kvm_vcpu *vcpu);
+int kvm_s390_handle_lpsw(struct kvm_vcpu *vcpu);
 
 /* implemented in sigp.c */
 int kvm_s390_handle_sigp(struct kvm_vcpu *vcpu);
diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
index d768906..66258b9 100644
--- a/arch/s390/kvm/priv.c
+++ b/arch/s390/kvm/priv.c
@@ -176,6 +176,101 @@ static int handle_stfl(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
+static void handle_new_psw(struct kvm_vcpu *vcpu)
+{
+	/* Check whether the new psw is enabled for machine checks. */
+	if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_MCHECK)
+		kvm_s390_deliver_pending_machine_checks(vcpu);
+}
+
+int kvm_s390_handle_lpsw(struct kvm_vcpu *vcpu)
+{
+	int base2 = vcpu->arch.sie_block->ipb >> 28;
+	int disp2 = ((vcpu->arch.sie_block->ipb & 0x0fff0000) >> 16);
+	u64 addr;
+	u64 uninitialized_var(new_psw);
+
+	if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
+		return kvm_s390_inject_program_int(vcpu,
+						   PGM_PRIVILEGED_OPERATION);
+
+	addr = disp2;
+	if (base2)
+		addr += vcpu->run->s.regs.gprs[base2];
+
+	if (addr & 7) {
+		kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
+		goto out;
+	}
+
+	if (get_guest_u64(vcpu, addr, &new_psw)) {
+		kvm_s390_inject_program_int(vcpu, PGM_ADDRESSING);
+		goto out;
+	}
+
+	if (!(new_psw & 0x0008000000000000)) {
+		kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
+		goto out;
+	}
+
+	vcpu->arch.sie_block->gpsw.mask = new_psw & 0xfff7ffff80000000;
+	vcpu->arch.sie_block->gpsw.addr = new_psw & 0x000000007fffffff;
+
+	if ((vcpu->arch.sie_block->gpsw.mask & 0xb80800fe00000000) ||
+	    (!(vcpu->arch.sie_block->gpsw.mask & 0x0000000180000000) &&
+	     (vcpu->arch.sie_block->gpsw.addr & 0x000000007ff00000)) ||
+	    ((vcpu->arch.sie_block->gpsw.mask & 0x0000000180000000) ==
+	     0x0000000100000000)) {
+		kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
+		goto out;
+	}
+
+	handle_new_psw(vcpu);
+out:
+	return 0;
+}
+
+static int handle_lpswe(struct kvm_vcpu *vcpu)
+{
+	int base2 = vcpu->arch.sie_block->ipb >> 28;
+	int disp2 = ((vcpu->arch.sie_block->ipb & 0x0fff0000) >> 16);
+	u64 addr;
+	psw_t new_psw;
+
+	addr = disp2;
+	if (base2)
+		addr += vcpu->run->s.regs.gprs[base2];
+
+	if (addr & 7) {
+		kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
+		goto out;
+	}
+
+	if (copy_from_guest(vcpu, &new_psw, addr, sizeof(new_psw))) {
+		kvm_s390_inject_program_int(vcpu, PGM_ADDRESSING);
+		goto out;
+	}
+
+	vcpu->arch.sie_block->gpsw.mask = new_psw.mask;
+	vcpu->arch.sie_block->gpsw.addr = new_psw.addr;
+
+	if ((vcpu->arch.sie_block->gpsw.mask & 0xb80800fe7fffffff) ||
+	    (((vcpu->arch.sie_block->gpsw.mask & 0x0000000180000000) ==
+	      0x0000000080000000) &&
+	     (vcpu->arch.sie_block->gpsw.addr & 0xffffffff80000000)) ||
+	    (!(vcpu->arch.sie_block->gpsw.mask & 0x0000000180000000) &&
+	     (vcpu->arch.sie_block->gpsw.addr & 0xfffffffffff00000)) ||
+	    ((vcpu->arch.sie_block->gpsw.mask & 0x0000000180000000) ==
+	     0x0000000100000000)) {
+		kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
+		goto out;
+	}
+
+	handle_new_psw(vcpu);
+out:
+	return 0;
+}
+
 static int handle_stidp(struct kvm_vcpu *vcpu)
 {
 	int base2 = vcpu->arch.sie_block->ipb >> 28;
@@ -309,6 +404,7 @@ static intercept_handler_t priv_handlers[256] = {
 	[0x5f] = handle_chsc,
 	[0x7d] = handle_stsi,
 	[0xb1] = handle_stfl,
+	[0xb2] = handle_lpswe,
 };
 
 int kvm_s390_handle_b2(struct kvm_vcpu *vcpu)
@@ -333,6 +429,43 @@ int kvm_s390_handle_b2(struct kvm_vcpu *vcpu)
 	return -EOPNOTSUPP;
 }
 
+static int handle_epsw(struct kvm_vcpu *vcpu)
+{
+	int reg1, reg2;
+
+	reg1 = (vcpu->arch.sie_block->ipb & 0x00f00000) >> 24;
+	reg2 = (vcpu->arch.sie_block->ipb & 0x000f0000) >> 16;
+	vcpu->run->s.regs.gprs[reg1] &= 0xffffffff00000000;
+	vcpu->run->s.regs.gprs[reg1] |= vcpu->arch.sie_block->gpsw.mask >> 32;
+	if (reg2) {
+		vcpu->run->s.regs.gprs[reg2] &= 0xffffffff00000000;
+		vcpu->run->s.regs.gprs[reg2] |=
+			vcpu->arch.sie_block->gpsw.mask & 0x00000000ffffffff;
+	}
+	return 0;
+}
+
+static intercept_handler_t b9_handlers[256] = {
+	[0x8d] = handle_epsw,
+};
+
+int kvm_s390_handle_b9(struct kvm_vcpu *vcpu)
+{
+	intercept_handler_t handler;
+
+	/* This is handled just as for the B2 instructions. */
+	handler = b9_handlers[vcpu->arch.sie_block->ipa & 0x00ff];
+	if (handler) {
+		if ((handler != handle_epsw) &&
+		    (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE))
+			return kvm_s390_inject_program_int(vcpu,
+						   PGM_PRIVILEGED_OPERATION);
+		else
+			return handler(vcpu);
+	}
+	return -EOPNOTSUPP;
+}
+
 static int handle_tprot(struct kvm_vcpu *vcpu)
 {
 	int base1 = (vcpu->arch.sie_block->ipb & 0xf0000000) >> 28;
diff --git a/arch/s390/kvm/trace-s390.h b/arch/s390/kvm/trace-s390.h
index 90fdf85..95fbc1a 100644
--- a/arch/s390/kvm/trace-s390.h
+++ b/arch/s390/kvm/trace-s390.h
@@ -141,13 +141,13 @@ TRACE_EVENT(kvm_s390_inject_vcpu,
  * Trace point for the actual delivery of interrupts.
  */
 TRACE_EVENT(kvm_s390_deliver_interrupt,
-	    TP_PROTO(unsigned int id, __u64 type, __u32 data0, __u64 data1),
+	    TP_PROTO(unsigned int id, __u64 type, __u64 data0, __u64 data1),
 	    TP_ARGS(id, type, data0, data1),
 
 	    TP_STRUCT__entry(
 		    __field(int, id)
 		    __field(__u32, inttype)
-		    __field(__u32, data0)
+		    __field(__u64, data0)
 		    __field(__u64, data1)
 		    ),
 
@@ -159,7 +159,7 @@ TRACE_EVENT(kvm_s390_deliver_interrupt,
 		    ),
 
 	    TP_printk("deliver interrupt (vcpu %d): type:%x (%s) "	\
-		      "data:%08x %016llx",
+		      "data:%08llx %016llx",
 		      __entry->id, __entry->inttype,
 		      __print_symbolic(__entry->inttype, kvm_s390_int_type),
 		      __entry->data0, __entry->data1)
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index e676940..22859dc 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -397,6 +397,7 @@ struct kvm_s390_psw {
 #define KVM_S390_PROGRAM_INT		0xfffe0001u
 #define KVM_S390_SIGP_SET_PREFIX	0xfffe0002u
 #define KVM_S390_RESTART		0xfffe0003u
+#define KVM_S390_MCHK			0xfffe1000u
 #define KVM_S390_INT_VIRTIO		0xffff2603u
 #define KVM_S390_INT_SERVICE		0xffff2401u
 #define KVM_S390_INT_EMERGENCY		0xffff1201u
-- 
1.7.12.4

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 3/5] KVM: s390: In-kernel handling of I/O instructions.
  2012-12-07 12:30 [PATCH v4 0/5] s390: Host support for channel I/O Cornelia Huck
  2012-12-07 12:30 ` [PATCH 1/5] KVM: s390: Support for I/O interrupts Cornelia Huck
  2012-12-07 12:30 ` [PATCH 2/5] KVM: s390: Add support for machine checks Cornelia Huck
@ 2012-12-07 12:30 ` Cornelia Huck
  2012-12-10  7:53   ` Alexander Graf
  2012-12-07 12:30 ` [PATCH 4/5] KVM: s390: Base infrastructure for enabling capabilities Cornelia Huck
  2012-12-07 12:30 ` [PATCH 5/5] KVM: s390: Add support for channel I/O instructions Cornelia Huck
  4 siblings, 1 reply; 24+ messages in thread
From: Cornelia Huck @ 2012-12-07 12:30 UTC (permalink / raw)
  To: Marcelo Tosatti, Gleb Natapov, KVM, linux-s390
  Cc: Avi Kivity, Christian Borntraeger, Carsten Otte, Alexander Graf,
	Heiko Carstens, Martin Schwidefsky, Sebastian Ott

Explicitely catch all channel I/O related instructions intercepts
in the kernel and set condition code 3 for them.

This paves the way for properly handling these instructions later
on.

Note: This is not architecture compliant (the previous code wasn't
either) since setting cc 3 is not the correct thing to do for some
of these instructions. For Linux guests, however, it still has the
intended effect of stopping css probing.

Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 arch/s390/kvm/intercept.c | 19 +++++++++++++---
 arch/s390/kvm/kvm-s390.h  |  1 +
 arch/s390/kvm/priv.c      | 56 +++++++++++++++++++++++++++++++++--------------
 3 files changed, 56 insertions(+), 20 deletions(-)

diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
index ec1177f..754dc9e 100644
--- a/arch/s390/kvm/intercept.c
+++ b/arch/s390/kvm/intercept.c
@@ -33,8 +33,6 @@ static int handle_lctlg(struct kvm_vcpu *vcpu)
 	int reg, rc;
 
 	vcpu->stat.instruction_lctlg++;
-	if ((vcpu->arch.sie_block->ipb & 0xff) != 0x2f)
-		return -EOPNOTSUPP;
 
 	useraddr = disp2;
 	if (base2)
@@ -104,6 +102,21 @@ static int handle_lctl(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
+static intercept_handler_t eb_handlers[256] = {
+	[0x2f] = handle_lctlg,
+	[0x8a] = kvm_s390_handle_priv_eb,
+};
+
+static int handle_eb(struct kvm_vcpu *vcpu)
+{
+	intercept_handler_t handler;
+
+	handler = eb_handlers[vcpu->arch.sie_block->ipb & 0xff];
+	if (handler)
+		return handler(vcpu);
+	return -EOPNOTSUPP;
+}
+
 static intercept_handler_t instruction_handlers[256] = {
 	[0x01] = kvm_s390_handle_01,
 	[0x82] = kvm_s390_handle_lpsw,
@@ -113,7 +126,7 @@ static intercept_handler_t instruction_handlers[256] = {
 	[0xb7] = handle_lctl,
 	[0xb9] = kvm_s390_handle_b9,
 	[0xe5] = kvm_s390_handle_e5,
-	[0xeb] = handle_lctlg,
+	[0xeb] = handle_eb,
 };
 
 static int handle_noop(struct kvm_vcpu *vcpu)
diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
index b1e1cb6..7f50229 100644
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -83,6 +83,7 @@ int kvm_s390_handle_e5(struct kvm_vcpu *vcpu);
 int kvm_s390_handle_01(struct kvm_vcpu *vcpu);
 int kvm_s390_handle_b9(struct kvm_vcpu *vcpu);
 int kvm_s390_handle_lpsw(struct kvm_vcpu *vcpu);
+int kvm_s390_handle_priv_eb(struct kvm_vcpu *vcpu);
 
 /* implemented in sigp.c */
 int kvm_s390_handle_sigp(struct kvm_vcpu *vcpu);
diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
index 66258b9..e85f461 100644
--- a/arch/s390/kvm/priv.c
+++ b/arch/s390/kvm/priv.c
@@ -135,20 +135,9 @@ static int handle_skey(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
-static int handle_stsch(struct kvm_vcpu *vcpu)
+static int handle_io_inst(struct kvm_vcpu *vcpu)
 {
-	vcpu->stat.instruction_stsch++;
-	VCPU_EVENT(vcpu, 4, "%s", "store subchannel - CC3");
-	/* condition code 3 */
-	vcpu->arch.sie_block->gpsw.mask &= ~(3ul << 44);
-	vcpu->arch.sie_block->gpsw.mask |= (3 & 3ul) << 44;
-	return 0;
-}
-
-static int handle_chsc(struct kvm_vcpu *vcpu)
-{
-	vcpu->stat.instruction_chsc++;
-	VCPU_EVENT(vcpu, 4, "%s", "channel subsystem call - CC3");
+	VCPU_EVENT(vcpu, 4, "%s", "I/O instruction");
 	/* condition code 3 */
 	vcpu->arch.sie_block->gpsw.mask &= ~(3ul << 44);
 	vcpu->arch.sie_block->gpsw.mask |= (3 & 3ul) << 44;
@@ -392,7 +381,7 @@ out_fail:
 	return 0;
 }
 
-static intercept_handler_t priv_handlers[256] = {
+static intercept_handler_t b2_handlers[256] = {
 	[0x02] = handle_stidp,
 	[0x10] = handle_set_prefix,
 	[0x11] = handle_store_prefix,
@@ -400,8 +389,22 @@ static intercept_handler_t priv_handlers[256] = {
 	[0x29] = handle_skey,
 	[0x2a] = handle_skey,
 	[0x2b] = handle_skey,
-	[0x34] = handle_stsch,
-	[0x5f] = handle_chsc,
+	[0x30] = handle_io_inst,
+	[0x31] = handle_io_inst,
+	[0x32] = handle_io_inst,
+	[0x33] = handle_io_inst,
+	[0x34] = handle_io_inst,
+	[0x35] = handle_io_inst,
+	[0x36] = handle_io_inst,
+	[0x37] = handle_io_inst,
+	[0x38] = handle_io_inst,
+	[0x39] = handle_io_inst,
+	[0x3a] = handle_io_inst,
+	[0x3b] = handle_io_inst,
+	[0x3c] = handle_io_inst,
+	[0x5f] = handle_io_inst,
+	[0x74] = handle_io_inst,
+	[0x76] = handle_io_inst,
 	[0x7d] = handle_stsi,
 	[0xb1] = handle_stfl,
 	[0xb2] = handle_lpswe,
@@ -418,7 +421,7 @@ int kvm_s390_handle_b2(struct kvm_vcpu *vcpu)
 	 * state bit and (a) handle the instruction or (b) send a code 2
 	 * program check.
 	 * Anything else goes to userspace.*/
-	handler = priv_handlers[vcpu->arch.sie_block->ipa & 0x00ff];
+	handler = b2_handlers[vcpu->arch.sie_block->ipa & 0x00ff];
 	if (handler) {
 		if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
 			return kvm_s390_inject_program_int(vcpu,
@@ -447,6 +450,7 @@ static int handle_epsw(struct kvm_vcpu *vcpu)
 
 static intercept_handler_t b9_handlers[256] = {
 	[0x8d] = handle_epsw,
+	[0x9c] = handle_io_inst,
 };
 
 int kvm_s390_handle_b9(struct kvm_vcpu *vcpu)
@@ -466,6 +470,24 @@ int kvm_s390_handle_b9(struct kvm_vcpu *vcpu)
 	return -EOPNOTSUPP;
 }
 
+static intercept_handler_t eb_handlers[256] = {
+	[0x8a] = handle_io_inst,
+};
+
+int kvm_s390_handle_priv_eb(struct kvm_vcpu *vcpu)
+{
+	intercept_handler_t handler;
+
+	/* All eb instructions that end up here are privileged. */
+	if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
+		return kvm_s390_inject_program_int(vcpu,
+						   PGM_PRIVILEGED_OPERATION);
+	handler = eb_handlers[vcpu->arch.sie_block->ipb & 0xff];
+	if (handler)
+		return handler(vcpu);
+	return -EOPNOTSUPP;
+}
+
 static int handle_tprot(struct kvm_vcpu *vcpu)
 {
 	int base1 = (vcpu->arch.sie_block->ipb & 0xf0000000) >> 28;
-- 
1.7.12.4

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 4/5] KVM: s390: Base infrastructure for enabling capabilities.
  2012-12-07 12:30 [PATCH v4 0/5] s390: Host support for channel I/O Cornelia Huck
                   ` (2 preceding siblings ...)
  2012-12-07 12:30 ` [PATCH 3/5] KVM: s390: In-kernel handling of I/O instructions Cornelia Huck
@ 2012-12-07 12:30 ` Cornelia Huck
  2012-12-10  7:54   ` Alexander Graf
  2012-12-07 12:30 ` [PATCH 5/5] KVM: s390: Add support for channel I/O instructions Cornelia Huck
  4 siblings, 1 reply; 24+ messages in thread
From: Cornelia Huck @ 2012-12-07 12:30 UTC (permalink / raw)
  To: Marcelo Tosatti, Gleb Natapov, KVM, linux-s390
  Cc: Avi Kivity, Christian Borntraeger, Carsten Otte, Alexander Graf,
	Heiko Carstens, Martin Schwidefsky, Sebastian Ott

Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 Documentation/virtual/kvm/api.txt |  2 +-
 arch/s390/kvm/kvm-s390.c          | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 8617339..bba630b 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -913,7 +913,7 @@ documentation when it pops into existence).
 4.37 KVM_ENABLE_CAP
 
 Capability: KVM_CAP_ENABLE_CAP
-Architectures: ppc
+Architectures: ppc, s390
 Type: vcpu ioctl
 Parameters: struct kvm_enable_cap (in)
 Returns: 0 on success; -1 on error
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 731ddee..c015f6f 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -140,6 +140,7 @@ int kvm_dev_ioctl_check_extension(long ext)
 #endif
 	case KVM_CAP_SYNC_REGS:
 	case KVM_CAP_ONE_REG:
+	case KVM_CAP_ENABLE_CAP:
 		r = 1;
 		break;
 	case KVM_CAP_NR_VCPUS:
@@ -812,6 +813,22 @@ int kvm_s390_vcpu_store_status(struct kvm_vcpu *vcpu, unsigned long addr)
 	return 0;
 }
 
+static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
+				     struct kvm_enable_cap *cap)
+{
+	int r;
+
+	if (cap->flags)
+		return -EINVAL;
+
+	switch (cap->cap) {
+	default:
+		r = -EINVAL;
+		break;
+	}
+	return r;
+}
+
 long kvm_arch_vcpu_ioctl(struct file *filp,
 			 unsigned int ioctl, unsigned long arg)
 {
@@ -898,6 +915,15 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 			r = 0;
 		break;
 	}
+	case KVM_ENABLE_CAP:
+	{
+		struct kvm_enable_cap cap;
+		r = -EFAULT;
+		if (copy_from_user(&cap, argp, sizeof(cap)))
+			break;
+		r = kvm_vcpu_ioctl_enable_cap(vcpu, &cap);
+		break;
+	}
 	default:
 		r = -ENOTTY;
 	}
-- 
1.7.12.4

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 5/5] KVM: s390: Add support for channel I/O instructions.
  2012-12-07 12:30 [PATCH v4 0/5] s390: Host support for channel I/O Cornelia Huck
                   ` (3 preceding siblings ...)
  2012-12-07 12:30 ` [PATCH 4/5] KVM: s390: Base infrastructure for enabling capabilities Cornelia Huck
@ 2012-12-07 12:30 ` Cornelia Huck
  2012-12-10  8:01   ` Alexander Graf
  4 siblings, 1 reply; 24+ messages in thread
From: Cornelia Huck @ 2012-12-07 12:30 UTC (permalink / raw)
  To: Marcelo Tosatti, Gleb Natapov, KVM, linux-s390
  Cc: Avi Kivity, Christian Borntraeger, Carsten Otte, Alexander Graf,
	Heiko Carstens, Martin Schwidefsky, Sebastian Ott

Add a new capability, KVM_CAP_S390_CSS_SUPPORT, which will pass
intercepts for channel I/O instructions to userspace. Only I/O
instructions interacting with I/O interrupts need to be handled
in-kernel:

- TEST PENDING INTERRUPTION (tpi) dequeues and stores pending
  interrupts entirely in-kernel.
- TEST SUBCHANNEL (tsch) dequeues pending interrupts in-kernel
  and exits via KVM_EXIT_S390_TSCH to userspace for subchannel-
  related processing.

Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 Documentation/virtual/kvm/api.txt | 30 ++++++++++++
 arch/s390/include/asm/kvm_host.h  |  1 +
 arch/s390/kvm/intercept.c         |  1 +
 arch/s390/kvm/interrupt.c         | 37 +++++++++++++++
 arch/s390/kvm/kvm-s390.c          | 12 +++++
 arch/s390/kvm/kvm-s390.h          |  2 +
 arch/s390/kvm/priv.c              | 96 +++++++++++++++++++++++++++++++++++++--
 arch/s390/kvm/trace-s390.h        | 20 ++++++++
 include/trace/events/kvm.h        |  2 +-
 include/uapi/linux/kvm.h          | 11 +++++
 10 files changed, 207 insertions(+), 5 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index bba630b..f7627c9 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -2295,6 +2295,22 @@ The possible hypercalls are defined in the Power Architecture Platform
 Requirements (PAPR) document available from www.power.org (free
 developer registration required to access it).
 
+		/* KVM_EXIT_S390_TSCH */
+		struct {
+			__u16 subchannel_id;
+			__u16 subchannel_nr;
+			__u32 io_int_parm;
+			__u32 io_int_word;
+			__u32 ipb;
+			__u8 dequeued;
+		} s390_tsch;
+
+s390 specific. This exit occurs when KVM_CAP_S390_CSS_SUPPORT has been enabled
+and TEST SUBCHANNEL was intercepted. If dequeued is set, a pending I/O
+interrupt for the target subchannel has been dequeued and subchannel_id,
+subchannel_nr, io_int_parm and io_int_word contain the parameters for that
+interrupt. ipb is needed for instruction parameter decoding.
+
 		/* Fix the size of the union. */
 		char padding[256];
 	};
@@ -2416,3 +2432,17 @@ For mmu types KVM_MMU_FSL_BOOKE_NOHV and KVM_MMU_FSL_BOOKE_HV:
    where "num_sets" is the tlb_sizes[] value divided by the tlb_ways[] value.
  - The tsize field of mas1 shall be set to 4K on TLB0, even though the
    hardware ignores this value for TLB0.
+
+6.4 KVM_CAP_S390_CSS_SUPPORT
+
+Architectures: s390
+Parameters: none
+Returns: 0 on success; -1 on error
+
+This capability enables support for handling of channel I/O instructions.
+
+TEST PENDING INTERRUPTION and the interrupt portion of TEST SUBCHANNEL are
+handled in-kernel, while the other I/O instructions are passed to userspace.
+
+When this capability is enabled, KVM_EXIT_S390_TSCH will occur on TEST
+SUBCHANNEL intercepts.
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 773859e..091c581 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -264,6 +264,7 @@ struct kvm_arch{
 	debug_info_t *dbf;
 	struct kvm_s390_float_interrupt float_int;
 	struct gmap *gmap;
+	int css_support;
 };
 
 extern int sie64a(struct kvm_s390_sie_block *, u64 *);
diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
index 754dc9e..9ab2efd 100644
--- a/arch/s390/kvm/intercept.c
+++ b/arch/s390/kvm/intercept.c
@@ -273,6 +273,7 @@ static const intercept_handler_t intercept_funcs[] = {
 	[0x0C >> 2] = handle_instruction_and_prog,
 	[0x10 >> 2] = handle_noop,
 	[0x14 >> 2] = handle_noop,
+	[0x18 >> 2] = handle_noop,
 	[0x1C >> 2] = kvm_s390_handle_wait,
 	[0x20 >> 2] = handle_validity,
 	[0x28 >> 2] = handle_stop,
diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 6b10267..495a411 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -714,6 +714,43 @@ int kvm_s390_inject_program_int(struct kvm_vcpu *vcpu, u16 code)
 	return 0;
 }
 
+struct kvm_s390_interrupt_info *kvm_s390_get_io_int(struct kvm *kvm,
+						    u64 cr6, u64 schid)
+{
+	struct kvm_s390_float_interrupt *fi;
+	struct kvm_s390_interrupt_info *inti, *iter;
+
+	if ((!schid && !cr6) || (schid && cr6))
+		return NULL;
+	mutex_lock(&kvm->lock);
+	fi = &kvm->arch.float_int;
+	spin_lock(&fi->lock);
+	inti = NULL;
+	list_for_each_entry(iter, &fi->list, list) {
+		if (!is_ioint(iter->type))
+			continue;
+		if (cr6 && ((cr6 & iter->io.io_int_word) == 0))
+			continue;
+		if (schid) {
+			if (((schid & 0x00000000ffff0000) >> 16) !=
+			    iter->io.subchannel_id)
+				continue;
+			if ((schid & 0x000000000000ffff) !=
+			    iter->io.subchannel_nr)
+				continue;
+		}
+		inti = iter;
+		break;
+	}
+	if (inti)
+		list_del_init(&inti->list);
+	if (list_empty(&fi->list))
+		atomic_set(&fi->active, 0);
+	spin_unlock(&fi->lock);
+	mutex_unlock(&kvm->lock);
+	return inti;
+}
+
 int kvm_s390_inject_vm(struct kvm *kvm,
 		       struct kvm_s390_interrupt *s390int)
 {
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index c015f6f..c30257b 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -141,6 +141,7 @@ int kvm_dev_ioctl_check_extension(long ext)
 	case KVM_CAP_SYNC_REGS:
 	case KVM_CAP_ONE_REG:
 	case KVM_CAP_ENABLE_CAP:
+	case KVM_CAP_S390_CSS_SUPPORT:
 		r = 1;
 		break;
 	case KVM_CAP_NR_VCPUS:
@@ -235,6 +236,9 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 		if (!kvm->arch.gmap)
 			goto out_nogmap;
 	}
+
+	kvm->arch.css_support = 0;
+
 	return 0;
 out_nogmap:
 	debug_unregister(kvm->arch.dbf);
@@ -662,6 +666,7 @@ rerun_vcpu:
 	case KVM_EXIT_INTR:
 	case KVM_EXIT_S390_RESET:
 	case KVM_EXIT_S390_UCONTROL:
+	case KVM_EXIT_S390_TSCH:
 		break;
 	default:
 		BUG();
@@ -822,6 +827,13 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
 		return -EINVAL;
 
 	switch (cap->cap) {
+	case KVM_CAP_S390_CSS_SUPPORT:
+		if (!vcpu->kvm->arch.css_support) {
+			vcpu->kvm->arch.css_support = 1;
+			trace_kvm_s390_enable_css(vcpu->kvm);
+		}
+		r = 0;
+		break;
 	default:
 		r = -EINVAL;
 		break;
diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
index 7f50229..14a417f 100644
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -76,6 +76,8 @@ int kvm_s390_inject_vcpu(struct kvm_vcpu *vcpu,
 		struct kvm_s390_interrupt *s390int);
 int kvm_s390_inject_program_int(struct kvm_vcpu *vcpu, u16 code);
 int kvm_s390_inject_sigp_stop(struct kvm_vcpu *vcpu, int action);
+struct kvm_s390_interrupt_info *kvm_s390_get_io_int(struct kvm *kvm,
+						    u64 cr6, u64 schid);
 
 /* implemented in priv.c */
 int kvm_s390_handle_b2(struct kvm_vcpu *vcpu);
diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
index e85f461..22eb993 100644
--- a/arch/s390/kvm/priv.c
+++ b/arch/s390/kvm/priv.c
@@ -135,15 +135,103 @@ static int handle_skey(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
-static int handle_io_inst(struct kvm_vcpu *vcpu)
+static int handle_tpi(struct kvm_vcpu *vcpu)
 {
-	VCPU_EVENT(vcpu, 4, "%s", "I/O instruction");
-	/* condition code 3 */
+	u32 ipb = vcpu->arch.sie_block->ipb;
+	u64 addr;
+	struct kvm_s390_interrupt_info *inti;
+	int cc;
+
+	addr = ipb >> 28;
+	if (addr > 0)
+		addr = vcpu->run->s.regs.gprs[addr];
+
+	addr += (ipb & 0xfff0000) >> 16;
+
+	inti = kvm_s390_get_io_int(vcpu->kvm, vcpu->run->s.regs.crs[6], 0);
+	if (inti) {
+		if (addr) {
+			/*
+			 * Store the two-word I/O interruption code into the
+			 * provided area.
+			 */
+			put_guest_u16(vcpu, addr, inti->io.subchannel_id);
+			put_guest_u16(vcpu, addr + 2, inti->io.subchannel_nr);
+			put_guest_u32(vcpu, addr + 4, inti->io.io_int_parm);
+		} else {
+			/*
+			 * Store the three-word I/O interruption code into
+			 * the appropriate lowcore area.
+			 */
+			put_guest_u16(vcpu, 184, inti->io.subchannel_id);
+			put_guest_u16(vcpu, 186, inti->io.subchannel_nr);
+			put_guest_u32(vcpu, 188, inti->io.io_int_parm);
+			put_guest_u32(vcpu, 192, inti->io.io_int_word);
+		}
+		cc = 1;
+	} else
+		cc = 0;
+	kfree(inti);
+	/* Set condition code and we're done. */
 	vcpu->arch.sie_block->gpsw.mask &= ~(3ul << 44);
-	vcpu->arch.sie_block->gpsw.mask |= (3 & 3ul) << 44;
+	vcpu->arch.sie_block->gpsw.mask |= (cc & 3ul) << 44;
 	return 0;
 }
 
+static int handle_tsch(struct kvm_vcpu *vcpu)
+{
+	struct kvm_s390_interrupt_info *inti;
+
+	inti = kvm_s390_get_io_int(vcpu->kvm, 0,
+				   vcpu->run->s.regs.gprs[1]);
+
+	/*
+	 * Prepare exit to userspace.
+	 * We indicate whether we dequeued a pending I/O interrupt
+	 * so that userspace can re-inject it if the instruction gets
+	 * a program check. While this may re-order the pending I/O
+	 * interrupts, this is no problem since the priority is kept
+	 * intact.
+	 */
+	vcpu->run->exit_reason = KVM_EXIT_S390_TSCH;
+	vcpu->run->s390_tsch.dequeued = !!inti;
+	if (inti) {
+		vcpu->run->s390_tsch.subchannel_id = inti->io.subchannel_id;
+		vcpu->run->s390_tsch.subchannel_nr = inti->io.subchannel_nr;
+		vcpu->run->s390_tsch.io_int_parm = inti->io.io_int_parm;
+		vcpu->run->s390_tsch.io_int_word = inti->io.io_int_word;
+	}
+	vcpu->run->s390_tsch.ipb = vcpu->arch.sie_block->ipb;
+	kfree(inti);
+	return -EREMOTE;
+}
+
+static int handle_io_inst(struct kvm_vcpu *vcpu)
+{
+	VCPU_EVENT(vcpu, 4, "%s", "I/O instruction");
+
+	if (vcpu->kvm->arch.css_support) {
+		/*
+		 * Most I/O instructions will be handled by userspace.
+		 * Exceptions are tpi and the interrupt portion of tsch.
+		 */
+		if (vcpu->arch.sie_block->ipa == 0xb236)
+			return handle_tpi(vcpu);
+		if (vcpu->arch.sie_block->ipa == 0xb235)
+			return handle_tsch(vcpu);
+		/* Handle in userspace. */
+		return -EOPNOTSUPP;
+	} else {
+		/*
+		 * Set condition code 3 to stop the guest from issueing channel
+		 * I/O instructions.
+		 */
+		vcpu->arch.sie_block->gpsw.mask &= ~(3ul << 44);
+		vcpu->arch.sie_block->gpsw.mask |= (3 & 3ul) << 44;
+		return 0;
+	}
+}
+
 static int handle_stfl(struct kvm_vcpu *vcpu)
 {
 	unsigned int facility_list;
diff --git a/arch/s390/kvm/trace-s390.h b/arch/s390/kvm/trace-s390.h
index 95fbc1a..13f30f5 100644
--- a/arch/s390/kvm/trace-s390.h
+++ b/arch/s390/kvm/trace-s390.h
@@ -204,6 +204,26 @@ TRACE_EVENT(kvm_s390_stop_request,
 	);
 
 
+/*
+ * Trace point for enabling channel I/O instruction support.
+ */
+TRACE_EVENT(kvm_s390_enable_css,
+	    TP_PROTO(void *kvm),
+	    TP_ARGS(kvm),
+
+	    TP_STRUCT__entry(
+		    __field(void *, kvm)
+		    ),
+
+	    TP_fast_assign(
+		    __entry->kvm = kvm;
+		    ),
+
+	    TP_printk("enabling channel I/O support (kvm @ %p)\n",
+		      __entry->kvm)
+	);
+
+
 #endif /* _TRACE_KVMS390_H */
 
 /* This part must be outside protection */
diff --git a/include/trace/events/kvm.h b/include/trace/events/kvm.h
index 7ef9e75..a23f47c 100644
--- a/include/trace/events/kvm.h
+++ b/include/trace/events/kvm.h
@@ -14,7 +14,7 @@
 	ERSN(SHUTDOWN), ERSN(FAIL_ENTRY), ERSN(INTR), ERSN(SET_TPR),	\
 	ERSN(TPR_ACCESS), ERSN(S390_SIEIC), ERSN(S390_RESET), ERSN(DCR),\
 	ERSN(NMI), ERSN(INTERNAL_ERROR), ERSN(OSI), ERSN(PAPR_HCALL),	\
-	ERSN(S390_UCONTROL)
+	ERSN(S390_UCONTROL), ERSN(S390_TSCH)
 
 TRACE_EVENT(kvm_userspace_exit,
 	    TP_PROTO(__u32 reason, int errno),
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 22859dc..72aa4a3 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -168,6 +168,7 @@ struct kvm_pit_config {
 #define KVM_EXIT_PAPR_HCALL	  19
 #define KVM_EXIT_S390_UCONTROL	  20
 #define KVM_EXIT_WATCHDOG         21
+#define KVM_EXIT_S390_TSCH        22
 
 /* For KVM_EXIT_INTERNAL_ERROR */
 /* Emulate instruction failed. */
@@ -285,6 +286,15 @@ struct kvm_run {
 			__u64 ret;
 			__u64 args[9];
 		} papr_hcall;
+		/* KVM_EXIT_S390_TSCH */
+		struct {
+			__u16 subchannel_id;
+			__u16 subchannel_nr;
+			__u32 io_int_parm;
+			__u32 io_int_word;
+			__u32 ipb;
+			__u8 dequeued;
+		} s390_tsch;
 		/* Fix the size of the union. */
 		char padding[256];
 	};
@@ -641,6 +651,7 @@ struct kvm_ppc_smmu_info {
 #endif
 #define KVM_CAP_IRQFD_RESAMPLE 82
 #define KVM_CAP_PPC_BOOKE_WATCHDOG 83
+#define KVM_CAP_S390_CSS_SUPPORT 84
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
1.7.12.4

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/5] KVM: s390: Support for I/O interrupts.
  2012-12-07 12:30 ` [PATCH 1/5] KVM: s390: Support for I/O interrupts Cornelia Huck
@ 2012-12-10  7:33   ` Alexander Graf
  2012-12-10 10:09     ` Cornelia Huck
  0 siblings, 1 reply; 24+ messages in thread
From: Alexander Graf @ 2012-12-10  7:33 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: Marcelo Tosatti, Gleb Natapov, KVM, linux-s390, Avi Kivity,
	Christian Borntraeger, Carsten Otte, Heiko Carstens,
	Martin Schwidefsky, Sebastian Ott


On 07.12.2012, at 13:30, Cornelia Huck wrote:

> Add support for handling I/O interrupts (standard, subchannel-related
> ones and rudimentary adapter interrupts).
> 
> The subchannel-identifying parameters are encoded into the interrupt
> type.
> 
> I/O interrupts are floating, so they can't be injected on a specific
> vcpu.
> 
> Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
> Documentation/virtual/kvm/api.txt |   4 ++
> arch/s390/include/asm/kvm_host.h  |   2 +
> arch/s390/kvm/interrupt.c         | 115 ++++++++++++++++++++++++++++++++++++--
> include/uapi/linux/kvm.h          |   6 ++
> 4 files changed, 122 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 6671fdc..e298a72 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2068,6 +2068,10 @@ KVM_S390_INT_VIRTIO (vm) - virtio external interrupt; external interrupt
> KVM_S390_INT_SERVICE (vm) - sclp external interrupt; sclp parameter in parm
> KVM_S390_INT_EMERGENCY (vcpu) - sigp emergency; source cpu in parm
> KVM_S390_INT_EXTERNAL_CALL (vcpu) - sigp external call; source cpu in parm
> +KVM_S390_INT_IO(ai,cssid,ssid,schid) (vm) - compound value to indicate an
> +    I/O interrupt (ai - adapter interrupt; cssid,ssid,schid - subchannel);
> +    I/O interruption parameters in parm (subchannel) and parm64 (intparm,
> +    interruption subclass)
> 
> Note that the vcpu ioctl is asynchronous to vcpu execution.
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index b784154..e47f697 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -76,6 +76,7 @@ struct kvm_s390_sie_block {
> 	__u64	epoch;			/* 0x0038 */
> 	__u8	reserved40[4];		/* 0x0040 */
> #define LCTL_CR0	0x8000
> +#define LCTL_CR6	0x0200
> 	__u16   lctl;			/* 0x0044 */
> 	__s16	icpua;			/* 0x0046 */
> 	__u32	ictl;			/* 0x0048 */
> @@ -127,6 +128,7 @@ struct kvm_vcpu_stat {
> 	u32 deliver_prefix_signal;
> 	u32 deliver_restart_signal;
> 	u32 deliver_program_int;
> +	u32 deliver_io_int;
> 	u32 exit_wait_state;
> 	u32 instruction_stidp;
> 	u32 instruction_spx;
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index c30615e..070ba22 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -21,11 +21,26 @@
> #include "gaccess.h"
> #include "trace-s390.h"
> 
> +#define IOINT_SCHID_MASK 0x0000ffff
> +#define IOINT_SSID_MASK 0x00030000
> +#define IOINT_CSSID_MASK 0x03fc0000
> +#define IOINT_AI_MASK 0x04000000
> +
> +static int is_ioint(u64 type)
> +{
> +	return ((type & 0xfffe0000u) != 0xfffe0000u);
> +}
> +
> static int psw_extint_disabled(struct kvm_vcpu *vcpu)
> {
> 	return !(vcpu->arch.sie_block->gpsw.mask & PSW_MASK_EXT);
> }
> 
> +static int psw_ioint_disabled(struct kvm_vcpu *vcpu)
> +{
> +	return !(vcpu->arch.sie_block->gpsw.mask & PSW_MASK_IO);
> +}
> +
> static int psw_interrupts_disabled(struct kvm_vcpu *vcpu)
> {
> 	if ((vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PER) ||
> @@ -68,7 +83,18 @@ static int __interrupt_is_deliverable(struct kvm_vcpu *vcpu,
> 	case KVM_S390_RESTART:
> 		return 1;
> 	default:
> -		BUG();
> +		if (is_ioint(inti->type)) {

Though I usually like if (...) { positive) } else { abort(); } coding style in general, it makes code quite hard to read when you are limited to 80 characters per line :)

I think it'd really help readability if you instead would write

if (!is_ioint(...)) {
    BUG();
}

and then continue without indent. That problem gets even more obvious further down the file.

> +			if (psw_ioint_disabled(vcpu))
> +				return 0;
> +			if (vcpu->arch.sie_block->gcr[6] &
> +			    inti->io.io_int_word)
> +				return 1;
> +			return 0;
> +		} else {
> +			printk(KERN_WARNING "illegal interrupt type %llx\n",
> +			       inti->type);
> +			BUG();
> +		}
> 	}
> 	return 0;
> }
> @@ -117,6 +143,13 @@ static void __set_intercept_indicator(struct kvm_vcpu *vcpu,
> 		__set_cpuflag(vcpu, CPUSTAT_STOP_INT);
> 		break;
> 	default:
> +		if (is_ioint(inti->type)) {
> +			if (psw_ioint_disabled(vcpu))
> +				__set_cpuflag(vcpu, CPUSTAT_IO_INT);
> +			else
> +				vcpu->arch.sie_block->lctl |= LCTL_CR6;
> +			break;
> +		}
> 		BUG();

Please reverse the logic here, just like you did above :).

> 	}
> }
> @@ -298,7 +331,49 @@ static void __do_deliver_interrupt(struct kvm_vcpu *vcpu,
> 		break;
> 
> 	default:
> -		BUG();
> +		if (is_ioint(inti->type)) {
> +			__u32 param0 = ((__u32)inti->io.subchannel_id << 16) |
> +				inti->io.subchannel_nr;
> +			__u64 param1 = ((__u64)inti->io.io_int_parm << 32) |
> +				inti->io.io_int_word;
> +			VCPU_EVENT(vcpu, 4,
> +				   "interrupt: I/O %llx", inti->type);
> +			vcpu->stat.deliver_io_int++;
> +			trace_kvm_s390_deliver_interrupt(vcpu->vcpu_id, inti->type,
> +							 param0, param1);
> +			rc = put_guest_u16(vcpu, __LC_SUBCHANNEL_ID,
> +					   inti->io.subchannel_id);
> +			if (rc == -EFAULT)
> +				exception = 1;
> +
> +			rc = put_guest_u16(vcpu, __LC_SUBCHANNEL_NR,
> +					   inti->io.subchannel_nr);
> +			if (rc == -EFAULT)
> +				exception = 1;
> +
> +			rc = put_guest_u32(vcpu, __LC_IO_INT_PARM,
> +					   inti->io.io_int_parm);
> +			if (rc == -EFAULT)
> +				exception = 1;
> +
> +			rc = put_guest_u32(vcpu, __LC_IO_INT_WORD,
> +					   inti->io.io_int_word);
> +			if (rc == -EFAULT)
> +				exception = 1;
> +
> +			rc = copy_to_guest(vcpu, __LC_IO_OLD_PSW,
> +					   &vcpu->arch.sie_block->gpsw,
> +					   sizeof(psw_t));
> +			if (rc == -EFAULT)
> +				exception = 1;
> +
> +			rc = copy_from_guest(vcpu, &vcpu->arch.sie_block->gpsw,
> +					     __LC_IO_NEW_PSW, sizeof(psw_t));
> +			if (rc == -EFAULT)
> +				exception = 1;
> +			break;
> +		} else
> +			BUG();
> 	}
> 	if (exception) {
> 		printk("kvm: The guest lowcore is not mapped during interrupt "
> @@ -545,7 +620,7 @@ int kvm_s390_inject_vm(struct kvm *kvm,
> {
> 	struct kvm_s390_local_interrupt *li;
> 	struct kvm_s390_float_interrupt *fi;
> -	struct kvm_s390_interrupt_info *inti;
> +	struct kvm_s390_interrupt_info *inti, *iter;
> 	int sigcpu;
> 
> 	inti = kzalloc(sizeof(*inti), GFP_KERNEL);
> @@ -569,9 +644,26 @@ int kvm_s390_inject_vm(struct kvm *kvm,
> 	case KVM_S390_SIGP_STOP:
> 	case KVM_S390_INT_EXTERNAL_CALL:
> 	case KVM_S390_INT_EMERGENCY:
> -	default:
> 		kfree(inti);
> 		return -EINVAL;
> +	default:
> +		if (!is_ioint(s390int->type)) {
> +			kfree(inti);
> +			return -EINVAL;
> +		}
> +		if (s390int->type & IOINT_AI_MASK)
> +			VM_EVENT(kvm, 5, "%s", "inject: I/O (AI)");
> +		else
> +			VM_EVENT(kvm, 5, "inject: I/O css %x ss %x schid %04x",
> +				 s390int->type & IOINT_CSSID_MASK,
> +				 s390int->type & IOINT_SSID_MASK,
> +				 s390int->type & IOINT_SCHID_MASK);
> +		inti->type = s390int->type;
> +		inti->io.subchannel_id = s390int->parm >> 16;
> +		inti->io.subchannel_nr = s390int->parm & 0x0000ffffu;
> +		inti->io.io_int_parm = s390int->parm64 >> 32;
> +		inti->io.io_int_word = s390int->parm64 & 0x00000000ffffffffull;
> +		break;
> 	}
> 	trace_kvm_s390_inject_vm(s390int->type, s390int->parm, s390int->parm64,
> 				 2);
> @@ -579,7 +671,19 @@ int kvm_s390_inject_vm(struct kvm *kvm,
> 	mutex_lock(&kvm->lock);
> 	fi = &kvm->arch.float_int;
> 	spin_lock(&fi->lock);
> -	list_add_tail(&inti->list, &fi->list);
> +	if (!is_ioint(inti->type))
> +		list_add_tail(&inti->list, &fi->list);
> +	else {
> +		/* Keep I/O interrupts sorted in isc order. */
> +		list_for_each_entry(iter, &fi->list, list) {
> +			if (!is_ioint(iter->type))
> +				continue;
> +			if (iter->io.io_int_word <= inti->io.io_int_word)
> +				continue;
> +			break;
> +		}
> +		list_add_tail(&inti->list, &iter->list);
> +	}
> 	atomic_set(&fi->active, 1);
> 	sigcpu = find_first_bit(fi->idle_mask, KVM_MAX_VCPUS);
> 	if (sigcpu == KVM_MAX_VCPUS) {
> @@ -654,6 +758,7 @@ int kvm_s390_inject_vcpu(struct kvm_vcpu *vcpu,
> 	case KVM_S390_INT_VIRTIO:
> 	case KVM_S390_INT_SERVICE:
> 	default:
> +		/* also includes IOINT */
> 		kfree(inti);
> 		return -EINVAL;
> 	}
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 494a84c..e676940 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -401,6 +401,12 @@ struct kvm_s390_psw {
> #define KVM_S390_INT_SERVICE		0xffff2401u
> #define KVM_S390_INT_EMERGENCY		0xffff1201u
> #define KVM_S390_INT_EXTERNAL_CALL	0xffff1202u
> +#define KVM_S390_INT_IO(ai,cssid,ssid,schid)   \

This one wants a comment saying that the namespace for everything not starting with 0xfffe/0xffff is taken by INT_IO.


Alex

> +	(((schid)) |			       \
> +	 ((ssid) << 16) |		       \
> +	 ((cssid) << 18) |		       \
> +	 ((ai) << 26))
> +
> 
> struct kvm_s390_interrupt {
> 	__u32 type;
> -- 
> 1.7.12.4
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/5] KVM: s390: Add support for machine checks.
  2012-12-07 12:30 ` [PATCH 2/5] KVM: s390: Add support for machine checks Cornelia Huck
@ 2012-12-10  7:51   ` Alexander Graf
  2012-12-10 10:12     ` Cornelia Huck
  2012-12-19  9:44     ` Heiko Carstens
  1 sibling, 1 reply; 24+ messages in thread
From: Alexander Graf @ 2012-12-10  7:51 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: Marcelo Tosatti, Gleb Natapov, KVM, linux-s390, Avi Kivity,
	Christian Borntraeger, Carsten Otte, Heiko Carstens,
	Martin Schwidefsky, Sebastian Ott


On 07.12.2012, at 13:30, Cornelia Huck wrote:

> Add support for injecting machine checks (only repressible
> conditions for now).
> 
> This is a bit more involved than I/O interrupts, for these reasons:
> 
> - Machine checks come in both floating and cpu varieties.
> - We don't have a bit for machine checks enabling, but have to use
>  a roundabout approach with trapping PSW changing instructions and
>  watching for opened machine checks.
> 
> Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
> Documentation/virtual/kvm/api.txt |   4 ++
> arch/s390/include/asm/kvm_host.h  |   8 +++
> arch/s390/kvm/intercept.c         |   2 +
> arch/s390/kvm/interrupt.c         | 112 ++++++++++++++++++++++++++++++++
> arch/s390/kvm/kvm-s390.h          |   3 +
> arch/s390/kvm/priv.c              | 133 ++++++++++++++++++++++++++++++++++++++
> arch/s390/kvm/trace-s390.h        |   6 +-
> include/uapi/linux/kvm.h          |   1 +
> 8 files changed, 266 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index e298a72..8617339 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2072,6 +2072,10 @@ KVM_S390_INT_IO(ai,cssid,ssid,schid) (vm) - compound value to indicate an
>     I/O interrupt (ai - adapter interrupt; cssid,ssid,schid - subchannel);
>     I/O interruption parameters in parm (subchannel) and parm64 (intparm,
>     interruption subclass)
> +KVM_S390_MCHK (vm, vcpu) - machine check interrupt; cr 14 bits in parm,
> +                           machine check interrupt code in parm64 (note that
> +                           machine checks needing further payload are not
> +                           supported by this ioctl)
> 
> Note that the vcpu ioctl is asynchronous to vcpu execution.
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index e47f697..773859e 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -77,8 +77,10 @@ struct kvm_s390_sie_block {
> 	__u8	reserved40[4];		/* 0x0040 */
> #define LCTL_CR0	0x8000
> #define LCTL_CR6	0x0200
> +#define LCTL_CR14	0x0002
> 	__u16   lctl;			/* 0x0044 */
> 	__s16	icpua;			/* 0x0046 */
> +#define ICTL_LPSW 0x00400000
> 	__u32	ictl;			/* 0x0048 */
> 	__u32	eca;			/* 0x004c */
> 	__u8	icptcode;		/* 0x0050 */
> @@ -189,6 +191,11 @@ struct kvm_s390_emerg_info {
> 	__u16 code;
> };
> 
> +struct kvm_s390_mchk_info {
> +	__u64 cr14;
> +	__u64 mcic;
> +};
> +
> struct kvm_s390_interrupt_info {
> 	struct list_head list;
> 	u64	type;
> @@ -199,6 +206,7 @@ struct kvm_s390_interrupt_info {
> 		struct kvm_s390_emerg_info emerg;
> 		struct kvm_s390_extcall_info extcall;
> 		struct kvm_s390_prefix_info prefix;
> +		struct kvm_s390_mchk_info mchk;
> 	};
> };
> 
> diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
> index 22798ec..ec1177f 100644
> --- a/arch/s390/kvm/intercept.c
> +++ b/arch/s390/kvm/intercept.c
> @@ -106,10 +106,12 @@ static int handle_lctl(struct kvm_vcpu *vcpu)
> 
> static intercept_handler_t instruction_handlers[256] = {
> 	[0x01] = kvm_s390_handle_01,
> +	[0x82] = kvm_s390_handle_lpsw,
> 	[0x83] = kvm_s390_handle_diag,
> 	[0xae] = kvm_s390_handle_sigp,
> 	[0xb2] = kvm_s390_handle_b2,
> 	[0xb7] = handle_lctl,
> +	[0xb9] = kvm_s390_handle_b9,
> 	[0xe5] = kvm_s390_handle_e5,
> 	[0xeb] = handle_lctlg,
> };
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 070ba22..6b10267 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -41,6 +41,11 @@ static int psw_ioint_disabled(struct kvm_vcpu *vcpu)
> 	return !(vcpu->arch.sie_block->gpsw.mask & PSW_MASK_IO);
> }
> 
> +static int psw_mchk_disabled(struct kvm_vcpu *vcpu)
> +{
> +	return !(vcpu->arch.sie_block->gpsw.mask & PSW_MASK_MCHECK);
> +}
> +
> static int psw_interrupts_disabled(struct kvm_vcpu *vcpu)
> {
> 	if ((vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PER) ||
> @@ -82,6 +87,12 @@ static int __interrupt_is_deliverable(struct kvm_vcpu *vcpu,
> 	case KVM_S390_SIGP_SET_PREFIX:
> 	case KVM_S390_RESTART:
> 		return 1;
> +	case KVM_S390_MCHK:
> +		if (psw_mchk_disabled(vcpu))
> +			return 0;
> +		if (vcpu->arch.sie_block->gcr[14] & inti->mchk.cr14)
> +			return 1;
> +		return 0;
> 	default:
> 		if (is_ioint(inti->type)) {
> 			if (psw_ioint_disabled(vcpu))
> @@ -119,6 +130,7 @@ static void __reset_intercept_indicators(struct kvm_vcpu *vcpu)
> 		CPUSTAT_IO_INT | CPUSTAT_EXT_INT | CPUSTAT_STOP_INT,
> 		&vcpu->arch.sie_block->cpuflags);
> 	vcpu->arch.sie_block->lctl = 0x0000;
> +	vcpu->arch.sie_block->ictl &= ~ICTL_LPSW;
> }
> 
> static void __set_cpuflag(struct kvm_vcpu *vcpu, u32 flag)
> @@ -142,6 +154,12 @@ static void __set_intercept_indicator(struct kvm_vcpu *vcpu,
> 	case KVM_S390_SIGP_STOP:
> 		__set_cpuflag(vcpu, CPUSTAT_STOP_INT);
> 		break;
> +	case KVM_S390_MCHK:
> +		if (psw_mchk_disabled(vcpu))
> +			vcpu->arch.sie_block->ictl |= ICTL_LPSW;
> +		else
> +			vcpu->arch.sie_block->lctl |= LCTL_CR14;
> +		break;
> 	default:
> 		if (is_ioint(inti->type)) {
> 			if (psw_ioint_disabled(vcpu))
> @@ -330,6 +348,32 @@ static void __do_deliver_interrupt(struct kvm_vcpu *vcpu,
> 			exception = 1;
> 		break;
> 
> +	case KVM_S390_MCHK:
> +		VCPU_EVENT(vcpu, 4, "interrupt: machine check mcic=%llx",
> +			   inti->mchk.mcic);
> +		trace_kvm_s390_deliver_interrupt(vcpu->vcpu_id, inti->type,
> +						 inti->mchk.cr14,
> +						 inti->mchk.mcic);
> +		rc = kvm_s390_vcpu_store_status(vcpu,
> +						KVM_S390_STORE_STATUS_PREFIXED);
> +		if (rc == -EFAULT)
> +			exception = 1;
> +
> +		rc = put_guest_u64(vcpu, __LC_MCCK_CODE, inti->mchk.mcic);
> +		if (rc == -EFAULT)
> +			exception = 1;
> +
> +		rc = copy_to_guest(vcpu, __LC_MCK_OLD_PSW,
> +				   &vcpu->arch.sie_block->gpsw, sizeof(psw_t));
> +		if (rc == -EFAULT)
> +			exception = 1;
> +
> +		rc = copy_from_guest(vcpu, &vcpu->arch.sie_block->gpsw,
> +				     __LC_MCK_NEW_PSW, sizeof(psw_t));
> +		if (rc == -EFAULT)
> +			exception = 1;
> +		break;
> +
> 	default:
> 		if (is_ioint(inti->type)) {
> 			__u32 param0 = ((__u32)inti->io.subchannel_id << 16) |
> @@ -593,6 +637,61 @@ void kvm_s390_deliver_pending_interrupts(struct kvm_vcpu *vcpu)
> 	}
> }
> 
> +void kvm_s390_deliver_pending_machine_checks(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_s390_local_interrupt *li = &vcpu->arch.local_int;
> +	struct kvm_s390_float_interrupt *fi = vcpu->arch.local_int.float_int;
> +	struct kvm_s390_interrupt_info  *n, *inti = NULL;
> +	int deliver;
> +
> +	__reset_intercept_indicators(vcpu);
> +	if (atomic_read(&li->active)) {
> +		do {
> +			deliver = 0;
> +			spin_lock_bh(&li->lock);
> +			list_for_each_entry_safe(inti, n, &li->list, list) {
> +				if ((inti->type == KVM_S390_MCHK) &&
> +				    __interrupt_is_deliverable(vcpu, inti)) {
> +					list_del(&inti->list);
> +					deliver = 1;
> +					break;
> +				}
> +				__set_intercept_indicator(vcpu, inti);
> +			}
> +			if (list_empty(&li->list))
> +				atomic_set(&li->active, 0);
> +			spin_unlock_bh(&li->lock);
> +			if (deliver) {
> +				__do_deliver_interrupt(vcpu, inti);
> +				kfree(inti);
> +			}
> +		} while (deliver);
> +	}
> +
> +	if (atomic_read(&fi->active)) {
> +		do {
> +			deliver = 0;
> +			spin_lock(&fi->lock);
> +			list_for_each_entry_safe(inti, n, &fi->list, list) {
> +				if ((inti->type == KVM_S390_MCHK) &&
> +				    __interrupt_is_deliverable(vcpu, inti)) {
> +					list_del(&inti->list);
> +					deliver = 1;
> +					break;
> +				}
> +				__set_intercept_indicator(vcpu, inti);
> +			}
> +			if (list_empty(&fi->list))
> +				atomic_set(&fi->active, 0);
> +			spin_unlock(&fi->lock);
> +			if (deliver) {
> +				__do_deliver_interrupt(vcpu, inti);
> +				kfree(inti);
> +			}
> +		} while (deliver);
> +	}
> +}
> +
> int kvm_s390_inject_program_int(struct kvm_vcpu *vcpu, u16 code)
> {
> 	struct kvm_s390_local_interrupt *li = &vcpu->arch.local_int;
> @@ -646,6 +745,13 @@ int kvm_s390_inject_vm(struct kvm *kvm,
> 	case KVM_S390_INT_EMERGENCY:
> 		kfree(inti);
> 		return -EINVAL;
> +	case KVM_S390_MCHK:
> +		VM_EVENT(kvm, 5, "inject: machine check parm64:%llx",
> +			 s390int->parm64);
> +		inti->type = s390int->type;
> +		inti->mchk.cr14 = s390int->parm; /* upper bits are not used */
> +		inti->mchk.mcic = s390int->parm64;
> +		break;
> 	default:
> 		if (!is_ioint(s390int->type)) {
> 			kfree(inti);
> @@ -755,6 +861,12 @@ int kvm_s390_inject_vcpu(struct kvm_vcpu *vcpu,
> 		inti->type = s390int->type;
> 		inti->emerg.code = s390int->parm;
> 		break;
> +	case KVM_S390_MCHK:
> +		VCPU_EVENT(vcpu, 5, "inject: machine check parm64:%llx",
> +			   s390int->parm64);
> +		inti->type = s390int->type;
> +		inti->mchk.mcic = s390int->parm64;
> +		break;
> 	case KVM_S390_INT_VIRTIO:
> 	case KVM_S390_INT_SERVICE:
> 	default:
> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> index d75bc5e..b1e1cb6 100644
> --- a/arch/s390/kvm/kvm-s390.h
> +++ b/arch/s390/kvm/kvm-s390.h
> @@ -69,6 +69,7 @@ int kvm_s390_handle_wait(struct kvm_vcpu *vcpu);
> enum hrtimer_restart kvm_s390_idle_wakeup(struct hrtimer *timer);
> void kvm_s390_tasklet(unsigned long parm);
> void kvm_s390_deliver_pending_interrupts(struct kvm_vcpu *vcpu);
> +void kvm_s390_deliver_pending_machine_checks(struct kvm_vcpu *vcpu);
> int kvm_s390_inject_vm(struct kvm *kvm,
> 		struct kvm_s390_interrupt *s390int);
> int kvm_s390_inject_vcpu(struct kvm_vcpu *vcpu,
> @@ -80,6 +81,8 @@ int kvm_s390_inject_sigp_stop(struct kvm_vcpu *vcpu, int action);
> int kvm_s390_handle_b2(struct kvm_vcpu *vcpu);
> int kvm_s390_handle_e5(struct kvm_vcpu *vcpu);
> int kvm_s390_handle_01(struct kvm_vcpu *vcpu);
> +int kvm_s390_handle_b9(struct kvm_vcpu *vcpu);
> +int kvm_s390_handle_lpsw(struct kvm_vcpu *vcpu);
> 
> /* implemented in sigp.c */
> int kvm_s390_handle_sigp(struct kvm_vcpu *vcpu);
> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
> index d768906..66258b9 100644
> --- a/arch/s390/kvm/priv.c
> +++ b/arch/s390/kvm/priv.c
> @@ -176,6 +176,101 @@ static int handle_stfl(struct kvm_vcpu *vcpu)
> 	return 0;
> }
> 
> +static void handle_new_psw(struct kvm_vcpu *vcpu)
> +{
> +	/* Check whether the new psw is enabled for machine checks. */
> +	if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_MCHECK)
> +		kvm_s390_deliver_pending_machine_checks(vcpu);
> +}
> +
> +int kvm_s390_handle_lpsw(struct kvm_vcpu *vcpu)
> +{
> +	int base2 = vcpu->arch.sie_block->ipb >> 28;
> +	int disp2 = ((vcpu->arch.sie_block->ipb & 0x0fff0000) >> 16);
> +	u64 addr;
> +	u64 uninitialized_var(new_psw);
> +
> +	if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
> +		return kvm_s390_inject_program_int(vcpu,
> +						   PGM_PRIVILEGED_OPERATION);
> +
> +	addr = disp2;
> +	if (base2)
> +		addr += vcpu->run->s.regs.gprs[base2];

This is a pretty common address offset scheme. Please extract it into a separate function (which the compiler will inline again).

> +
> +	if (addr & 7) {
> +		kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
> +		goto out;
> +	}
> +
> +	if (get_guest_u64(vcpu, addr, &new_psw)) {
> +		kvm_s390_inject_program_int(vcpu, PGM_ADDRESSING);
> +		goto out;
> +	}
> +
> +	if (!(new_psw & 0x0008000000000000)) {

Magic number? Doesn't this bit have a name?

> +		kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
> +		goto out;
> +	}
> +
> +	vcpu->arch.sie_block->gpsw.mask = new_psw & 0xfff7ffff80000000;
> +	vcpu->arch.sie_block->gpsw.addr = new_psw & 0x000000007fffffff;
> +
> +	if ((vcpu->arch.sie_block->gpsw.mask & 0xb80800fe00000000) ||
> +	    (!(vcpu->arch.sie_block->gpsw.mask & 0x0000000180000000) &&
> +	     (vcpu->arch.sie_block->gpsw.addr & 0x000000007ff00000)) ||
> +	    ((vcpu->arch.sie_block->gpsw.mask & 0x0000000180000000) ==
> +	     0x0000000100000000)) {
> +		kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);

Lots of magic numbers :). I'm sure these have names too ;). I really like to be able to read this cod without looking at the POP all the time.

> +		goto out;
> +	}
> +
> +	handle_new_psw(vcpu);
> +out:
> +	return 0;
> +}
> +
> +static int handle_lpswe(struct kvm_vcpu *vcpu)
> +{
> +	int base2 = vcpu->arch.sie_block->ipb >> 28;
> +	int disp2 = ((vcpu->arch.sie_block->ipb & 0x0fff0000) >> 16);
> +	u64 addr;
> +	psw_t new_psw;
> +
> +	addr = disp2;
> +	if (base2)
> +		addr += vcpu->run->s.regs.gprs[base2];
> +
> +	if (addr & 7) {
> +		kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
> +		goto out;
> +	}
> +
> +	if (copy_from_guest(vcpu, &new_psw, addr, sizeof(new_psw))) {
> +		kvm_s390_inject_program_int(vcpu, PGM_ADDRESSING);
> +		goto out;
> +	}
> +
> +	vcpu->arch.sie_block->gpsw.mask = new_psw.mask;
> +	vcpu->arch.sie_block->gpsw.addr = new_psw.addr;
> +
> +	if ((vcpu->arch.sie_block->gpsw.mask & 0xb80800fe7fffffff) ||
> +	    (((vcpu->arch.sie_block->gpsw.mask & 0x0000000180000000) ==
> +	      0x0000000080000000) &&
> +	     (vcpu->arch.sie_block->gpsw.addr & 0xffffffff80000000)) ||
> +	    (!(vcpu->arch.sie_block->gpsw.mask & 0x0000000180000000) &&
> +	     (vcpu->arch.sie_block->gpsw.addr & 0xfffffffffff00000)) ||
> +	    ((vcpu->arch.sie_block->gpsw.mask & 0x0000000180000000) ==
> +	     0x0000000100000000)) {
> +		kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
> +		goto out;
> +	}

That function looks quite similar to the one above. Any chance to move them together?

Alex

> +
> +	handle_new_psw(vcpu);
> +out:
> +	return 0;
> +}
> +
> static int handle_stidp(struct kvm_vcpu *vcpu)
> {
> 	int base2 = vcpu->arch.sie_block->ipb >> 28;
> @@ -309,6 +404,7 @@ static intercept_handler_t priv_handlers[256] = {
> 	[0x5f] = handle_chsc,
> 	[0x7d] = handle_stsi,
> 	[0xb1] = handle_stfl,
> +	[0xb2] = handle_lpswe,
> };
> 
> int kvm_s390_handle_b2(struct kvm_vcpu *vcpu)
> @@ -333,6 +429,43 @@ int kvm_s390_handle_b2(struct kvm_vcpu *vcpu)
> 	return -EOPNOTSUPP;
> }
> 
> +static int handle_epsw(struct kvm_vcpu *vcpu)
> +{
> +	int reg1, reg2;
> +
> +	reg1 = (vcpu->arch.sie_block->ipb & 0x00f00000) >> 24;
> +	reg2 = (vcpu->arch.sie_block->ipb & 0x000f0000) >> 16;
> +	vcpu->run->s.regs.gprs[reg1] &= 0xffffffff00000000;
> +	vcpu->run->s.regs.gprs[reg1] |= vcpu->arch.sie_block->gpsw.mask >> 32;
> +	if (reg2) {
> +		vcpu->run->s.regs.gprs[reg2] &= 0xffffffff00000000;
> +		vcpu->run->s.regs.gprs[reg2] |=
> +			vcpu->arch.sie_block->gpsw.mask & 0x00000000ffffffff;
> +	}
> +	return 0;
> +}
> +
> +static intercept_handler_t b9_handlers[256] = {
> +	[0x8d] = handle_epsw,
> +};
> +
> +int kvm_s390_handle_b9(struct kvm_vcpu *vcpu)
> +{
> +	intercept_handler_t handler;
> +
> +	/* This is handled just as for the B2 instructions. */
> +	handler = b9_handlers[vcpu->arch.sie_block->ipa & 0x00ff];
> +	if (handler) {
> +		if ((handler != handle_epsw) &&
> +		    (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE))
> +			return kvm_s390_inject_program_int(vcpu,
> +						   PGM_PRIVILEGED_OPERATION);
> +		else
> +			return handler(vcpu);
> +	}
> +	return -EOPNOTSUPP;
> +}
> +
> static int handle_tprot(struct kvm_vcpu *vcpu)
> {
> 	int base1 = (vcpu->arch.sie_block->ipb & 0xf0000000) >> 28;
> diff --git a/arch/s390/kvm/trace-s390.h b/arch/s390/kvm/trace-s390.h
> index 90fdf85..95fbc1a 100644
> --- a/arch/s390/kvm/trace-s390.h
> +++ b/arch/s390/kvm/trace-s390.h
> @@ -141,13 +141,13 @@ TRACE_EVENT(kvm_s390_inject_vcpu,
>  * Trace point for the actual delivery of interrupts.
>  */
> TRACE_EVENT(kvm_s390_deliver_interrupt,
> -	    TP_PROTO(unsigned int id, __u64 type, __u32 data0, __u64 data1),
> +	    TP_PROTO(unsigned int id, __u64 type, __u64 data0, __u64 data1),
> 	    TP_ARGS(id, type, data0, data1),
> 
> 	    TP_STRUCT__entry(
> 		    __field(int, id)
> 		    __field(__u32, inttype)
> -		    __field(__u32, data0)
> +		    __field(__u64, data0)
> 		    __field(__u64, data1)
> 		    ),
> 
> @@ -159,7 +159,7 @@ TRACE_EVENT(kvm_s390_deliver_interrupt,
> 		    ),
> 
> 	    TP_printk("deliver interrupt (vcpu %d): type:%x (%s) "	\
> -		      "data:%08x %016llx",
> +		      "data:%08llx %016llx",
> 		      __entry->id, __entry->inttype,
> 		      __print_symbolic(__entry->inttype, kvm_s390_int_type),
> 		      __entry->data0, __entry->data1)
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index e676940..22859dc 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -397,6 +397,7 @@ struct kvm_s390_psw {
> #define KVM_S390_PROGRAM_INT		0xfffe0001u
> #define KVM_S390_SIGP_SET_PREFIX	0xfffe0002u
> #define KVM_S390_RESTART		0xfffe0003u
> +#define KVM_S390_MCHK			0xfffe1000u
> #define KVM_S390_INT_VIRTIO		0xffff2603u
> #define KVM_S390_INT_SERVICE		0xffff2401u
> #define KVM_S390_INT_EMERGENCY		0xffff1201u
> -- 
> 1.7.12.4
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 3/5] KVM: s390: In-kernel handling of I/O instructions.
  2012-12-07 12:30 ` [PATCH 3/5] KVM: s390: In-kernel handling of I/O instructions Cornelia Huck
@ 2012-12-10  7:53   ` Alexander Graf
  2012-12-10 10:14     ` Cornelia Huck
  0 siblings, 1 reply; 24+ messages in thread
From: Alexander Graf @ 2012-12-10  7:53 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: Marcelo Tosatti, Gleb Natapov, KVM, linux-s390, Avi Kivity,
	Christian Borntraeger, Carsten Otte, Heiko Carstens,
	Martin Schwidefsky, Sebastian Ott


On 07.12.2012, at 13:30, Cornelia Huck wrote:

> Explicitely catch all channel I/O related instructions intercepts
> in the kernel and set condition code 3 for them.
> 
> This paves the way for properly handling these instructions later
> on.
> 
> Note: This is not architecture compliant (the previous code wasn't
> either) since setting cc 3 is not the correct thing to do for some
> of these instructions. For Linux guests, however, it still has the
> intended effect of stopping css probing.
> 
> Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
> arch/s390/kvm/intercept.c | 19 +++++++++++++---
> arch/s390/kvm/kvm-s390.h  |  1 +
> arch/s390/kvm/priv.c      | 56 +++++++++++++++++++++++++++++++++--------------
> 3 files changed, 56 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
> index ec1177f..754dc9e 100644
> --- a/arch/s390/kvm/intercept.c
> +++ b/arch/s390/kvm/intercept.c
> @@ -33,8 +33,6 @@ static int handle_lctlg(struct kvm_vcpu *vcpu)
> 	int reg, rc;
> 
> 	vcpu->stat.instruction_lctlg++;
> -	if ((vcpu->arch.sie_block->ipb & 0xff) != 0x2f)
> -		return -EOPNOTSUPP;
> 
> 	useraddr = disp2;
> 	if (base2)
> @@ -104,6 +102,21 @@ static int handle_lctl(struct kvm_vcpu *vcpu)
> 	return 0;
> }
> 
> +static intercept_handler_t eb_handlers[256] = {

const.

Reviewed-by: Alexander Graf <agraf@suse.de>


Alex

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/5] KVM: s390: Base infrastructure for enabling capabilities.
  2012-12-07 12:30 ` [PATCH 4/5] KVM: s390: Base infrastructure for enabling capabilities Cornelia Huck
@ 2012-12-10  7:54   ` Alexander Graf
  2012-12-10 10:16     ` Cornelia Huck
  0 siblings, 1 reply; 24+ messages in thread
From: Alexander Graf @ 2012-12-10  7:54 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: Marcelo Tosatti, Gleb Natapov, KVM, linux-s390, Avi Kivity,
	Christian Borntraeger, Carsten Otte, Heiko Carstens,
	Martin Schwidefsky, Sebastian Ott

Missing patch description. Otherwise:

Acked-by: Alexander Graf <agraf@suse.de>


Alex

On 07.12.2012, at 13:30, Cornelia Huck wrote:

> Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
> Documentation/virtual/kvm/api.txt |  2 +-
> arch/s390/kvm/kvm-s390.c          | 26 ++++++++++++++++++++++++++
> 2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 8617339..bba630b 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -913,7 +913,7 @@ documentation when it pops into existence).
> 4.37 KVM_ENABLE_CAP
> 
> Capability: KVM_CAP_ENABLE_CAP
> -Architectures: ppc
> +Architectures: ppc, s390
> Type: vcpu ioctl
> Parameters: struct kvm_enable_cap (in)
> Returns: 0 on success; -1 on error
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 731ddee..c015f6f 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -140,6 +140,7 @@ int kvm_dev_ioctl_check_extension(long ext)
> #endif
> 	case KVM_CAP_SYNC_REGS:
> 	case KVM_CAP_ONE_REG:
> +	case KVM_CAP_ENABLE_CAP:
> 		r = 1;
> 		break;
> 	case KVM_CAP_NR_VCPUS:
> @@ -812,6 +813,22 @@ int kvm_s390_vcpu_store_status(struct kvm_vcpu *vcpu, unsigned long addr)
> 	return 0;
> }
> 
> +static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
> +				     struct kvm_enable_cap *cap)
> +{
> +	int r;
> +
> +	if (cap->flags)
> +		return -EINVAL;
> +
> +	switch (cap->cap) {
> +	default:
> +		r = -EINVAL;
> +		break;
> +	}
> +	return r;
> +}
> +
> long kvm_arch_vcpu_ioctl(struct file *filp,
> 			 unsigned int ioctl, unsigned long arg)
> {
> @@ -898,6 +915,15 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> 			r = 0;
> 		break;
> 	}
> +	case KVM_ENABLE_CAP:
> +	{
> +		struct kvm_enable_cap cap;
> +		r = -EFAULT;
> +		if (copy_from_user(&cap, argp, sizeof(cap)))
> +			break;
> +		r = kvm_vcpu_ioctl_enable_cap(vcpu, &cap);
> +		break;
> +	}
> 	default:
> 		r = -ENOTTY;
> 	}
> -- 
> 1.7.12.4
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 5/5] KVM: s390: Add support for channel I/O instructions.
  2012-12-07 12:30 ` [PATCH 5/5] KVM: s390: Add support for channel I/O instructions Cornelia Huck
@ 2012-12-10  8:01   ` Alexander Graf
  0 siblings, 0 replies; 24+ messages in thread
From: Alexander Graf @ 2012-12-10  8:01 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: Marcelo Tosatti, Gleb Natapov, KVM, linux-s390, Avi Kivity,
	Christian Borntraeger, Carsten Otte, Heiko Carstens,
	Martin Schwidefsky, Sebastian Ott


On 07.12.2012, at 13:30, Cornelia Huck wrote:

> Add a new capability, KVM_CAP_S390_CSS_SUPPORT, which will pass
> intercepts for channel I/O instructions to userspace. Only I/O
> instructions interacting with I/O interrupts need to be handled
> in-kernel:
> 
> - TEST PENDING INTERRUPTION (tpi) dequeues and stores pending
>  interrupts entirely in-kernel.
> - TEST SUBCHANNEL (tsch) dequeues pending interrupts in-kernel
>  and exits via KVM_EXIT_S390_TSCH to userspace for subchannel-
>  related processing.
> 
> Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Reviewed-by: Alexander Graf <agraf@suse.de>


Alex

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/5] KVM: s390: Support for I/O interrupts.
  2012-12-10  7:33   ` Alexander Graf
@ 2012-12-10 10:09     ` Cornelia Huck
  2012-12-11 10:22       ` Alexander Graf
  0 siblings, 1 reply; 24+ messages in thread
From: Cornelia Huck @ 2012-12-10 10:09 UTC (permalink / raw)
  To: Alexander Graf
  Cc: Marcelo Tosatti, Gleb Natapov, KVM, linux-s390, Avi Kivity,
	Christian Borntraeger, Carsten Otte, Heiko Carstens,
	Martin Schwidefsky, Sebastian Ott

On Mon, 10 Dec 2012 08:33:10 +0100
Alexander Graf <agraf@suse.de> wrote:

> 
> On 07.12.2012, at 13:30, Cornelia Huck wrote:
> 
> > Add support for handling I/O interrupts (standard, subchannel-related
> > ones and rudimentary adapter interrupts).
> > 
> > The subchannel-identifying parameters are encoded into the interrupt
> > type.
> > 
> > I/O interrupts are floating, so they can't be injected on a specific
> > vcpu.
> > 
> > Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > ---
> > Documentation/virtual/kvm/api.txt |   4 ++
> > arch/s390/include/asm/kvm_host.h  |   2 +
> > arch/s390/kvm/interrupt.c         | 115 ++++++++++++++++++++++++++++++++++++--
> > include/uapi/linux/kvm.h          |   6 ++
> > 4 files changed, 122 insertions(+), 5 deletions(-)
> > 
> > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> > index 6671fdc..e298a72 100644
> > --- a/Documentation/virtual/kvm/api.txt
> > +++ b/Documentation/virtual/kvm/api.txt
> > @@ -2068,6 +2068,10 @@ KVM_S390_INT_VIRTIO (vm) - virtio external interrupt; external interrupt
> > KVM_S390_INT_SERVICE (vm) - sclp external interrupt; sclp parameter in parm
> > KVM_S390_INT_EMERGENCY (vcpu) - sigp emergency; source cpu in parm
> > KVM_S390_INT_EXTERNAL_CALL (vcpu) - sigp external call; source cpu in parm
> > +KVM_S390_INT_IO(ai,cssid,ssid,schid) (vm) - compound value to indicate an
> > +    I/O interrupt (ai - adapter interrupt; cssid,ssid,schid - subchannel);
> > +    I/O interruption parameters in parm (subchannel) and parm64 (intparm,
> > +    interruption subclass)
> > 
> > Note that the vcpu ioctl is asynchronous to vcpu execution.
> > 
> > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> > index b784154..e47f697 100644
> > --- a/arch/s390/include/asm/kvm_host.h
> > +++ b/arch/s390/include/asm/kvm_host.h
> > @@ -76,6 +76,7 @@ struct kvm_s390_sie_block {
> > 	__u64	epoch;			/* 0x0038 */
> > 	__u8	reserved40[4];		/* 0x0040 */
> > #define LCTL_CR0	0x8000
> > +#define LCTL_CR6	0x0200
> > 	__u16   lctl;			/* 0x0044 */
> > 	__s16	icpua;			/* 0x0046 */
> > 	__u32	ictl;			/* 0x0048 */
> > @@ -127,6 +128,7 @@ struct kvm_vcpu_stat {
> > 	u32 deliver_prefix_signal;
> > 	u32 deliver_restart_signal;
> > 	u32 deliver_program_int;
> > +	u32 deliver_io_int;
> > 	u32 exit_wait_state;
> > 	u32 instruction_stidp;
> > 	u32 instruction_spx;
> > diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> > index c30615e..070ba22 100644
> > --- a/arch/s390/kvm/interrupt.c
> > +++ b/arch/s390/kvm/interrupt.c
> > @@ -21,11 +21,26 @@
> > #include "gaccess.h"
> > #include "trace-s390.h"
> > 
> > +#define IOINT_SCHID_MASK 0x0000ffff
> > +#define IOINT_SSID_MASK 0x00030000
> > +#define IOINT_CSSID_MASK 0x03fc0000
> > +#define IOINT_AI_MASK 0x04000000
> > +
> > +static int is_ioint(u64 type)
> > +{
> > +	return ((type & 0xfffe0000u) != 0xfffe0000u);
> > +}
> > +
> > static int psw_extint_disabled(struct kvm_vcpu *vcpu)
> > {
> > 	return !(vcpu->arch.sie_block->gpsw.mask & PSW_MASK_EXT);
> > }
> > 
> > +static int psw_ioint_disabled(struct kvm_vcpu *vcpu)
> > +{
> > +	return !(vcpu->arch.sie_block->gpsw.mask & PSW_MASK_IO);
> > +}
> > +
> > static int psw_interrupts_disabled(struct kvm_vcpu *vcpu)
> > {
> > 	if ((vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PER) ||
> > @@ -68,7 +83,18 @@ static int __interrupt_is_deliverable(struct kvm_vcpu *vcpu,
> > 	case KVM_S390_RESTART:
> > 		return 1;
> > 	default:
> > -		BUG();
> > +		if (is_ioint(inti->type)) {
> 
> Though I usually like if (...) { positive) } else { abort(); } coding style in general, it makes code quite hard to read when you are limited to 80 characters per line :)
> 
> I think it'd really help readability if you instead would write
> 
> if (!is_ioint(...)) {
>     BUG();
> }
> 
> and then continue without indent. That problem gets even more obvious further down the file.

Hm, "bad state last" seems to parse, though.

> 
> > +			if (psw_ioint_disabled(vcpu))
> > +				return 0;
> > +			if (vcpu->arch.sie_block->gcr[6] &
> > +			    inti->io.io_int_word)
> > +				return 1;
> > +			return 0;
> > +		} else {
> > +			printk(KERN_WARNING "illegal interrupt type %llx\n",
> > +			       inti->type);
> > +			BUG();
> > +		}
> > 	}
> > 	return 0;
> > }
> > @@ -117,6 +143,13 @@ static void __set_intercept_indicator(struct kvm_vcpu *vcpu,
> > 		__set_cpuflag(vcpu, CPUSTAT_STOP_INT);
> > 		break;
> > 	default:
> > +		if (is_ioint(inti->type)) {
> > +			if (psw_ioint_disabled(vcpu))
> > +				__set_cpuflag(vcpu, CPUSTAT_IO_INT);
> > +			else
> > +				vcpu->arch.sie_block->lctl |= LCTL_CR6;
> > +			break;
> > +		}
> > 		BUG();
> 
> Please reverse the logic here, just like you did above :).

Hm, they're both the same?

> 
> > 	}
> > }
> > @@ -298,7 +331,49 @@ static void __do_deliver_interrupt(struct kvm_vcpu *vcpu,
> > 		break;
> > 
> > 	default:
> > -		BUG();
> > +		if (is_ioint(inti->type)) {
> > +			__u32 param0 = ((__u32)inti->io.subchannel_id << 16) |
> > +				inti->io.subchannel_nr;
> > +			__u64 param1 = ((__u64)inti->io.io_int_parm << 32) |
> > +				inti->io.io_int_word;
> > +			VCPU_EVENT(vcpu, 4,
> > +				   "interrupt: I/O %llx", inti->type);
> > +			vcpu->stat.deliver_io_int++;
> > +			trace_kvm_s390_deliver_interrupt(vcpu->vcpu_id, inti->type,
> > +							 param0, param1);
> > +			rc = put_guest_u16(vcpu, __LC_SUBCHANNEL_ID,
> > +					   inti->io.subchannel_id);
> > +			if (rc == -EFAULT)
> > +				exception = 1;
> > +
> > +			rc = put_guest_u16(vcpu, __LC_SUBCHANNEL_NR,
> > +					   inti->io.subchannel_nr);
> > +			if (rc == -EFAULT)
> > +				exception = 1;
> > +
> > +			rc = put_guest_u32(vcpu, __LC_IO_INT_PARM,
> > +					   inti->io.io_int_parm);
> > +			if (rc == -EFAULT)
> > +				exception = 1;
> > +
> > +			rc = put_guest_u32(vcpu, __LC_IO_INT_WORD,
> > +					   inti->io.io_int_word);
> > +			if (rc == -EFAULT)
> > +				exception = 1;
> > +
> > +			rc = copy_to_guest(vcpu, __LC_IO_OLD_PSW,
> > +					   &vcpu->arch.sie_block->gpsw,
> > +					   sizeof(psw_t));
> > +			if (rc == -EFAULT)
> > +				exception = 1;
> > +
> > +			rc = copy_from_guest(vcpu, &vcpu->arch.sie_block->gpsw,
> > +					     __LC_IO_NEW_PSW, sizeof(psw_t));
> > +			if (rc == -EFAULT)
> > +				exception = 1;
> > +			break;
> > +		} else
> > +			BUG();
> > 	}
> > 	if (exception) {
> > 		printk("kvm: The guest lowcore is not mapped during interrupt "
> > @@ -545,7 +620,7 @@ int kvm_s390_inject_vm(struct kvm *kvm,
> > {
> > 	struct kvm_s390_local_interrupt *li;
> > 	struct kvm_s390_float_interrupt *fi;
> > -	struct kvm_s390_interrupt_info *inti;
> > +	struct kvm_s390_interrupt_info *inti, *iter;
> > 	int sigcpu;
> > 
> > 	inti = kzalloc(sizeof(*inti), GFP_KERNEL);
> > @@ -569,9 +644,26 @@ int kvm_s390_inject_vm(struct kvm *kvm,
> > 	case KVM_S390_SIGP_STOP:
> > 	case KVM_S390_INT_EXTERNAL_CALL:
> > 	case KVM_S390_INT_EMERGENCY:
> > -	default:
> > 		kfree(inti);
> > 		return -EINVAL;
> > +	default:
> > +		if (!is_ioint(s390int->type)) {
> > +			kfree(inti);
> > +			return -EINVAL;
> > +		}
> > +		if (s390int->type & IOINT_AI_MASK)
> > +			VM_EVENT(kvm, 5, "%s", "inject: I/O (AI)");
> > +		else
> > +			VM_EVENT(kvm, 5, "inject: I/O css %x ss %x schid %04x",
> > +				 s390int->type & IOINT_CSSID_MASK,
> > +				 s390int->type & IOINT_SSID_MASK,
> > +				 s390int->type & IOINT_SCHID_MASK);
> > +		inti->type = s390int->type;
> > +		inti->io.subchannel_id = s390int->parm >> 16;
> > +		inti->io.subchannel_nr = s390int->parm & 0x0000ffffu;
> > +		inti->io.io_int_parm = s390int->parm64 >> 32;
> > +		inti->io.io_int_word = s390int->parm64 & 0x00000000ffffffffull;
> > +		break;
> > 	}
> > 	trace_kvm_s390_inject_vm(s390int->type, s390int->parm, s390int->parm64,
> > 				 2);
> > @@ -579,7 +671,19 @@ int kvm_s390_inject_vm(struct kvm *kvm,
> > 	mutex_lock(&kvm->lock);
> > 	fi = &kvm->arch.float_int;
> > 	spin_lock(&fi->lock);
> > -	list_add_tail(&inti->list, &fi->list);
> > +	if (!is_ioint(inti->type))
> > +		list_add_tail(&inti->list, &fi->list);
> > +	else {
> > +		/* Keep I/O interrupts sorted in isc order. */
> > +		list_for_each_entry(iter, &fi->list, list) {
> > +			if (!is_ioint(iter->type))
> > +				continue;
> > +			if (iter->io.io_int_word <= inti->io.io_int_word)
> > +				continue;
> > +			break;
> > +		}
> > +		list_add_tail(&inti->list, &iter->list);
> > +	}
> > 	atomic_set(&fi->active, 1);
> > 	sigcpu = find_first_bit(fi->idle_mask, KVM_MAX_VCPUS);
> > 	if (sigcpu == KVM_MAX_VCPUS) {
> > @@ -654,6 +758,7 @@ int kvm_s390_inject_vcpu(struct kvm_vcpu *vcpu,
> > 	case KVM_S390_INT_VIRTIO:
> > 	case KVM_S390_INT_SERVICE:
> > 	default:
> > +		/* also includes IOINT */
> > 		kfree(inti);
> > 		return -EINVAL;
> > 	}
> > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > index 494a84c..e676940 100644
> > --- a/include/uapi/linux/kvm.h
> > +++ b/include/uapi/linux/kvm.h
> > @@ -401,6 +401,12 @@ struct kvm_s390_psw {
> > #define KVM_S390_INT_SERVICE		0xffff2401u
> > #define KVM_S390_INT_EMERGENCY		0xffff1201u
> > #define KVM_S390_INT_EXTERNAL_CALL	0xffff1202u
> > +#define KVM_S390_INT_IO(ai,cssid,ssid,schid)   \
> 
> This one wants a comment saying that the namespace for everything not starting with 0xfffe/0xffff is taken by INT_IO.

Sounds reasonable.

> 
> 
> Alex
> 
> > +	(((schid)) |			       \
> > +	 ((ssid) << 16) |		       \
> > +	 ((cssid) << 18) |		       \
> > +	 ((ai) << 26))
> > +
> > 
> > struct kvm_s390_interrupt {
> > 	__u32 type;
> > -- 
> > 1.7.12.4
> > 
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/5] KVM: s390: Add support for machine checks.
  2012-12-10  7:51   ` Alexander Graf
@ 2012-12-10 10:12     ` Cornelia Huck
  0 siblings, 0 replies; 24+ messages in thread
From: Cornelia Huck @ 2012-12-10 10:12 UTC (permalink / raw)
  To: Alexander Graf
  Cc: Marcelo Tosatti, Gleb Natapov, KVM, linux-s390, Avi Kivity,
	Christian Borntraeger, Carsten Otte, Heiko Carstens,
	Martin Schwidefsky, Sebastian Ott

On Mon, 10 Dec 2012 08:51:11 +0100
Alexander Graf <agraf@suse.de> wrote:

> 
> On 07.12.2012, at 13:30, Cornelia Huck wrote:
> 
> > Add support for injecting machine checks (only repressible
> > conditions for now).
> > 
> > This is a bit more involved than I/O interrupts, for these reasons:
> > 
> > - Machine checks come in both floating and cpu varieties.
> > - We don't have a bit for machine checks enabling, but have to use
> >  a roundabout approach with trapping PSW changing instructions and
> >  watching for opened machine checks.
> > 
> > Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > ---
> > Documentation/virtual/kvm/api.txt |   4 ++
> > arch/s390/include/asm/kvm_host.h  |   8 +++
> > arch/s390/kvm/intercept.c         |   2 +
> > arch/s390/kvm/interrupt.c         | 112 ++++++++++++++++++++++++++++++++
> > arch/s390/kvm/kvm-s390.h          |   3 +
> > arch/s390/kvm/priv.c              | 133 ++++++++++++++++++++++++++++++++++++++
> > arch/s390/kvm/trace-s390.h        |   6 +-
> > include/uapi/linux/kvm.h          |   1 +
> > 8 files changed, 266 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> > index e298a72..8617339 100644
> > --- a/Documentation/virtual/kvm/api.txt
> > +++ b/Documentation/virtual/kvm/api.txt
> > @@ -2072,6 +2072,10 @@ KVM_S390_INT_IO(ai,cssid,ssid,schid) (vm) - compound value to indicate an
> >     I/O interrupt (ai - adapter interrupt; cssid,ssid,schid - subchannel);
> >     I/O interruption parameters in parm (subchannel) and parm64 (intparm,
> >     interruption subclass)
> > +KVM_S390_MCHK (vm, vcpu) - machine check interrupt; cr 14 bits in parm,
> > +                           machine check interrupt code in parm64 (note that
> > +                           machine checks needing further payload are not
> > +                           supported by this ioctl)
> > 
> > Note that the vcpu ioctl is asynchronous to vcpu execution.
> > 
> > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> > index e47f697..773859e 100644
> > --- a/arch/s390/include/asm/kvm_host.h
> > +++ b/arch/s390/include/asm/kvm_host.h
> > @@ -77,8 +77,10 @@ struct kvm_s390_sie_block {
> > 	__u8	reserved40[4];		/* 0x0040 */
> > #define LCTL_CR0	0x8000
> > #define LCTL_CR6	0x0200
> > +#define LCTL_CR14	0x0002
> > 	__u16   lctl;			/* 0x0044 */
> > 	__s16	icpua;			/* 0x0046 */
> > +#define ICTL_LPSW 0x00400000
> > 	__u32	ictl;			/* 0x0048 */
> > 	__u32	eca;			/* 0x004c */
> > 	__u8	icptcode;		/* 0x0050 */
> > @@ -189,6 +191,11 @@ struct kvm_s390_emerg_info {
> > 	__u16 code;
> > };
> > 
> > +struct kvm_s390_mchk_info {
> > +	__u64 cr14;
> > +	__u64 mcic;
> > +};
> > +
> > struct kvm_s390_interrupt_info {
> > 	struct list_head list;
> > 	u64	type;
> > @@ -199,6 +206,7 @@ struct kvm_s390_interrupt_info {
> > 		struct kvm_s390_emerg_info emerg;
> > 		struct kvm_s390_extcall_info extcall;
> > 		struct kvm_s390_prefix_info prefix;
> > +		struct kvm_s390_mchk_info mchk;
> > 	};
> > };
> > 
> > diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
> > index 22798ec..ec1177f 100644
> > --- a/arch/s390/kvm/intercept.c
> > +++ b/arch/s390/kvm/intercept.c
> > @@ -106,10 +106,12 @@ static int handle_lctl(struct kvm_vcpu *vcpu)
> > 
> > static intercept_handler_t instruction_handlers[256] = {
> > 	[0x01] = kvm_s390_handle_01,
> > +	[0x82] = kvm_s390_handle_lpsw,
> > 	[0x83] = kvm_s390_handle_diag,
> > 	[0xae] = kvm_s390_handle_sigp,
> > 	[0xb2] = kvm_s390_handle_b2,
> > 	[0xb7] = handle_lctl,
> > +	[0xb9] = kvm_s390_handle_b9,
> > 	[0xe5] = kvm_s390_handle_e5,
> > 	[0xeb] = handle_lctlg,
> > };
> > diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> > index 070ba22..6b10267 100644
> > --- a/arch/s390/kvm/interrupt.c
> > +++ b/arch/s390/kvm/interrupt.c
> > @@ -41,6 +41,11 @@ static int psw_ioint_disabled(struct kvm_vcpu *vcpu)
> > 	return !(vcpu->arch.sie_block->gpsw.mask & PSW_MASK_IO);
> > }
> > 
> > +static int psw_mchk_disabled(struct kvm_vcpu *vcpu)
> > +{
> > +	return !(vcpu->arch.sie_block->gpsw.mask & PSW_MASK_MCHECK);
> > +}
> > +
> > static int psw_interrupts_disabled(struct kvm_vcpu *vcpu)
> > {
> > 	if ((vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PER) ||
> > @@ -82,6 +87,12 @@ static int __interrupt_is_deliverable(struct kvm_vcpu *vcpu,
> > 	case KVM_S390_SIGP_SET_PREFIX:
> > 	case KVM_S390_RESTART:
> > 		return 1;
> > +	case KVM_S390_MCHK:
> > +		if (psw_mchk_disabled(vcpu))
> > +			return 0;
> > +		if (vcpu->arch.sie_block->gcr[14] & inti->mchk.cr14)
> > +			return 1;
> > +		return 0;
> > 	default:
> > 		if (is_ioint(inti->type)) {
> > 			if (psw_ioint_disabled(vcpu))
> > @@ -119,6 +130,7 @@ static void __reset_intercept_indicators(struct kvm_vcpu *vcpu)
> > 		CPUSTAT_IO_INT | CPUSTAT_EXT_INT | CPUSTAT_STOP_INT,
> > 		&vcpu->arch.sie_block->cpuflags);
> > 	vcpu->arch.sie_block->lctl = 0x0000;
> > +	vcpu->arch.sie_block->ictl &= ~ICTL_LPSW;
> > }
> > 
> > static void __set_cpuflag(struct kvm_vcpu *vcpu, u32 flag)
> > @@ -142,6 +154,12 @@ static void __set_intercept_indicator(struct kvm_vcpu *vcpu,
> > 	case KVM_S390_SIGP_STOP:
> > 		__set_cpuflag(vcpu, CPUSTAT_STOP_INT);
> > 		break;
> > +	case KVM_S390_MCHK:
> > +		if (psw_mchk_disabled(vcpu))
> > +			vcpu->arch.sie_block->ictl |= ICTL_LPSW;
> > +		else
> > +			vcpu->arch.sie_block->lctl |= LCTL_CR14;
> > +		break;
> > 	default:
> > 		if (is_ioint(inti->type)) {
> > 			if (psw_ioint_disabled(vcpu))
> > @@ -330,6 +348,32 @@ static void __do_deliver_interrupt(struct kvm_vcpu *vcpu,
> > 			exception = 1;
> > 		break;
> > 
> > +	case KVM_S390_MCHK:
> > +		VCPU_EVENT(vcpu, 4, "interrupt: machine check mcic=%llx",
> > +			   inti->mchk.mcic);
> > +		trace_kvm_s390_deliver_interrupt(vcpu->vcpu_id, inti->type,
> > +						 inti->mchk.cr14,
> > +						 inti->mchk.mcic);
> > +		rc = kvm_s390_vcpu_store_status(vcpu,
> > +						KVM_S390_STORE_STATUS_PREFIXED);
> > +		if (rc == -EFAULT)
> > +			exception = 1;
> > +
> > +		rc = put_guest_u64(vcpu, __LC_MCCK_CODE, inti->mchk.mcic);
> > +		if (rc == -EFAULT)
> > +			exception = 1;
> > +
> > +		rc = copy_to_guest(vcpu, __LC_MCK_OLD_PSW,
> > +				   &vcpu->arch.sie_block->gpsw, sizeof(psw_t));
> > +		if (rc == -EFAULT)
> > +			exception = 1;
> > +
> > +		rc = copy_from_guest(vcpu, &vcpu->arch.sie_block->gpsw,
> > +				     __LC_MCK_NEW_PSW, sizeof(psw_t));
> > +		if (rc == -EFAULT)
> > +			exception = 1;
> > +		break;
> > +
> > 	default:
> > 		if (is_ioint(inti->type)) {
> > 			__u32 param0 = ((__u32)inti->io.subchannel_id << 16) |
> > @@ -593,6 +637,61 @@ void kvm_s390_deliver_pending_interrupts(struct kvm_vcpu *vcpu)
> > 	}
> > }
> > 
> > +void kvm_s390_deliver_pending_machine_checks(struct kvm_vcpu *vcpu)
> > +{
> > +	struct kvm_s390_local_interrupt *li = &vcpu->arch.local_int;
> > +	struct kvm_s390_float_interrupt *fi = vcpu->arch.local_int.float_int;
> > +	struct kvm_s390_interrupt_info  *n, *inti = NULL;
> > +	int deliver;
> > +
> > +	__reset_intercept_indicators(vcpu);
> > +	if (atomic_read(&li->active)) {
> > +		do {
> > +			deliver = 0;
> > +			spin_lock_bh(&li->lock);
> > +			list_for_each_entry_safe(inti, n, &li->list, list) {
> > +				if ((inti->type == KVM_S390_MCHK) &&
> > +				    __interrupt_is_deliverable(vcpu, inti)) {
> > +					list_del(&inti->list);
> > +					deliver = 1;
> > +					break;
> > +				}
> > +				__set_intercept_indicator(vcpu, inti);
> > +			}
> > +			if (list_empty(&li->list))
> > +				atomic_set(&li->active, 0);
> > +			spin_unlock_bh(&li->lock);
> > +			if (deliver) {
> > +				__do_deliver_interrupt(vcpu, inti);
> > +				kfree(inti);
> > +			}
> > +		} while (deliver);
> > +	}
> > +
> > +	if (atomic_read(&fi->active)) {
> > +		do {
> > +			deliver = 0;
> > +			spin_lock(&fi->lock);
> > +			list_for_each_entry_safe(inti, n, &fi->list, list) {
> > +				if ((inti->type == KVM_S390_MCHK) &&
> > +				    __interrupt_is_deliverable(vcpu, inti)) {
> > +					list_del(&inti->list);
> > +					deliver = 1;
> > +					break;
> > +				}
> > +				__set_intercept_indicator(vcpu, inti);
> > +			}
> > +			if (list_empty(&fi->list))
> > +				atomic_set(&fi->active, 0);
> > +			spin_unlock(&fi->lock);
> > +			if (deliver) {
> > +				__do_deliver_interrupt(vcpu, inti);
> > +				kfree(inti);
> > +			}
> > +		} while (deliver);
> > +	}
> > +}
> > +
> > int kvm_s390_inject_program_int(struct kvm_vcpu *vcpu, u16 code)
> > {
> > 	struct kvm_s390_local_interrupt *li = &vcpu->arch.local_int;
> > @@ -646,6 +745,13 @@ int kvm_s390_inject_vm(struct kvm *kvm,
> > 	case KVM_S390_INT_EMERGENCY:
> > 		kfree(inti);
> > 		return -EINVAL;
> > +	case KVM_S390_MCHK:
> > +		VM_EVENT(kvm, 5, "inject: machine check parm64:%llx",
> > +			 s390int->parm64);
> > +		inti->type = s390int->type;
> > +		inti->mchk.cr14 = s390int->parm; /* upper bits are not used */
> > +		inti->mchk.mcic = s390int->parm64;
> > +		break;
> > 	default:
> > 		if (!is_ioint(s390int->type)) {
> > 			kfree(inti);
> > @@ -755,6 +861,12 @@ int kvm_s390_inject_vcpu(struct kvm_vcpu *vcpu,
> > 		inti->type = s390int->type;
> > 		inti->emerg.code = s390int->parm;
> > 		break;
> > +	case KVM_S390_MCHK:
> > +		VCPU_EVENT(vcpu, 5, "inject: machine check parm64:%llx",
> > +			   s390int->parm64);
> > +		inti->type = s390int->type;
> > +		inti->mchk.mcic = s390int->parm64;
> > +		break;
> > 	case KVM_S390_INT_VIRTIO:
> > 	case KVM_S390_INT_SERVICE:
> > 	default:
> > diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> > index d75bc5e..b1e1cb6 100644
> > --- a/arch/s390/kvm/kvm-s390.h
> > +++ b/arch/s390/kvm/kvm-s390.h
> > @@ -69,6 +69,7 @@ int kvm_s390_handle_wait(struct kvm_vcpu *vcpu);
> > enum hrtimer_restart kvm_s390_idle_wakeup(struct hrtimer *timer);
> > void kvm_s390_tasklet(unsigned long parm);
> > void kvm_s390_deliver_pending_interrupts(struct kvm_vcpu *vcpu);
> > +void kvm_s390_deliver_pending_machine_checks(struct kvm_vcpu *vcpu);
> > int kvm_s390_inject_vm(struct kvm *kvm,
> > 		struct kvm_s390_interrupt *s390int);
> > int kvm_s390_inject_vcpu(struct kvm_vcpu *vcpu,
> > @@ -80,6 +81,8 @@ int kvm_s390_inject_sigp_stop(struct kvm_vcpu *vcpu, int action);
> > int kvm_s390_handle_b2(struct kvm_vcpu *vcpu);
> > int kvm_s390_handle_e5(struct kvm_vcpu *vcpu);
> > int kvm_s390_handle_01(struct kvm_vcpu *vcpu);
> > +int kvm_s390_handle_b9(struct kvm_vcpu *vcpu);
> > +int kvm_s390_handle_lpsw(struct kvm_vcpu *vcpu);
> > 
> > /* implemented in sigp.c */
> > int kvm_s390_handle_sigp(struct kvm_vcpu *vcpu);
> > diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
> > index d768906..66258b9 100644
> > --- a/arch/s390/kvm/priv.c
> > +++ b/arch/s390/kvm/priv.c
> > @@ -176,6 +176,101 @@ static int handle_stfl(struct kvm_vcpu *vcpu)
> > 	return 0;
> > }
> > 
> > +static void handle_new_psw(struct kvm_vcpu *vcpu)
> > +{
> > +	/* Check whether the new psw is enabled for machine checks. */
> > +	if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_MCHECK)
> > +		kvm_s390_deliver_pending_machine_checks(vcpu);
> > +}
> > +
> > +int kvm_s390_handle_lpsw(struct kvm_vcpu *vcpu)
> > +{
> > +	int base2 = vcpu->arch.sie_block->ipb >> 28;
> > +	int disp2 = ((vcpu->arch.sie_block->ipb & 0x0fff0000) >> 16);
> > +	u64 addr;
> > +	u64 uninitialized_var(new_psw);
> > +
> > +	if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
> > +		return kvm_s390_inject_program_int(vcpu,
> > +						   PGM_PRIVILEGED_OPERATION);
> > +
> > +	addr = disp2;
> > +	if (base2)
> > +		addr += vcpu->run->s.regs.gprs[base2];
> 
> This is a pretty common address offset scheme. Please extract it into a separate function (which the compiler will inline again).
> 
> > +
> > +	if (addr & 7) {
> > +		kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
> > +		goto out;
> > +	}
> > +
> > +	if (get_guest_u64(vcpu, addr, &new_psw)) {
> > +		kvm_s390_inject_program_int(vcpu, PGM_ADDRESSING);
> > +		goto out;
> > +	}
> > +
> > +	if (!(new_psw & 0x0008000000000000)) {
> 
> Magic number? Doesn't this bit have a name?
> 
> > +		kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
> > +		goto out;
> > +	}
> > +
> > +	vcpu->arch.sie_block->gpsw.mask = new_psw & 0xfff7ffff80000000;
> > +	vcpu->arch.sie_block->gpsw.addr = new_psw & 0x000000007fffffff;
> > +
> > +	if ((vcpu->arch.sie_block->gpsw.mask & 0xb80800fe00000000) ||
> > +	    (!(vcpu->arch.sie_block->gpsw.mask & 0x0000000180000000) &&
> > +	     (vcpu->arch.sie_block->gpsw.addr & 0x000000007ff00000)) ||
> > +	    ((vcpu->arch.sie_block->gpsw.mask & 0x0000000180000000) ==
> > +	     0x0000000100000000)) {
> > +		kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
> 
> Lots of magic numbers :). I'm sure these have names too ;). I really like to be able to read this cod without looking at the POP all the time.

I'll see what I can come up with.

> 
> > +		goto out;
> > +	}
> > +
> > +	handle_new_psw(vcpu);
> > +out:
> > +	return 0;
> > +}
> > +
> > +static int handle_lpswe(struct kvm_vcpu *vcpu)
> > +{
> > +	int base2 = vcpu->arch.sie_block->ipb >> 28;
> > +	int disp2 = ((vcpu->arch.sie_block->ipb & 0x0fff0000) >> 16);
> > +	u64 addr;
> > +	psw_t new_psw;
> > +
> > +	addr = disp2;
> > +	if (base2)
> > +		addr += vcpu->run->s.regs.gprs[base2];
> > +
> > +	if (addr & 7) {
> > +		kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
> > +		goto out;
> > +	}
> > +
> > +	if (copy_from_guest(vcpu, &new_psw, addr, sizeof(new_psw))) {
> > +		kvm_s390_inject_program_int(vcpu, PGM_ADDRESSING);
> > +		goto out;
> > +	}
> > +
> > +	vcpu->arch.sie_block->gpsw.mask = new_psw.mask;
> > +	vcpu->arch.sie_block->gpsw.addr = new_psw.addr;
> > +
> > +	if ((vcpu->arch.sie_block->gpsw.mask & 0xb80800fe7fffffff) ||
> > +	    (((vcpu->arch.sie_block->gpsw.mask & 0x0000000180000000) ==
> > +	      0x0000000080000000) &&
> > +	     (vcpu->arch.sie_block->gpsw.addr & 0xffffffff80000000)) ||
> > +	    (!(vcpu->arch.sie_block->gpsw.mask & 0x0000000180000000) &&
> > +	     (vcpu->arch.sie_block->gpsw.addr & 0xfffffffffff00000)) ||
> > +	    ((vcpu->arch.sie_block->gpsw.mask & 0x0000000180000000) ==
> > +	     0x0000000100000000)) {
> > +		kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
> > +		goto out;
> > +	}
> 
> That function looks quite similar to the one above. Any chance to move them together?

The checks are not quite the same.

> 
> Alex
> 
> > +
> > +	handle_new_psw(vcpu);
> > +out:
> > +	return 0;
> > +}
> > +
> > static int handle_stidp(struct kvm_vcpu *vcpu)
> > {
> > 	int base2 = vcpu->arch.sie_block->ipb >> 28;
> > @@ -309,6 +404,7 @@ static intercept_handler_t priv_handlers[256] = {
> > 	[0x5f] = handle_chsc,
> > 	[0x7d] = handle_stsi,
> > 	[0xb1] = handle_stfl,
> > +	[0xb2] = handle_lpswe,
> > };
> > 
> > int kvm_s390_handle_b2(struct kvm_vcpu *vcpu)
> > @@ -333,6 +429,43 @@ int kvm_s390_handle_b2(struct kvm_vcpu *vcpu)
> > 	return -EOPNOTSUPP;
> > }
> > 
> > +static int handle_epsw(struct kvm_vcpu *vcpu)
> > +{
> > +	int reg1, reg2;
> > +
> > +	reg1 = (vcpu->arch.sie_block->ipb & 0x00f00000) >> 24;
> > +	reg2 = (vcpu->arch.sie_block->ipb & 0x000f0000) >> 16;
> > +	vcpu->run->s.regs.gprs[reg1] &= 0xffffffff00000000;
> > +	vcpu->run->s.regs.gprs[reg1] |= vcpu->arch.sie_block->gpsw.mask >> 32;
> > +	if (reg2) {
> > +		vcpu->run->s.regs.gprs[reg2] &= 0xffffffff00000000;
> > +		vcpu->run->s.regs.gprs[reg2] |=
> > +			vcpu->arch.sie_block->gpsw.mask & 0x00000000ffffffff;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static intercept_handler_t b9_handlers[256] = {
> > +	[0x8d] = handle_epsw,
> > +};
> > +
> > +int kvm_s390_handle_b9(struct kvm_vcpu *vcpu)
> > +{
> > +	intercept_handler_t handler;
> > +
> > +	/* This is handled just as for the B2 instructions. */
> > +	handler = b9_handlers[vcpu->arch.sie_block->ipa & 0x00ff];
> > +	if (handler) {
> > +		if ((handler != handle_epsw) &&
> > +		    (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE))
> > +			return kvm_s390_inject_program_int(vcpu,
> > +						   PGM_PRIVILEGED_OPERATION);
> > +		else
> > +			return handler(vcpu);
> > +	}
> > +	return -EOPNOTSUPP;
> > +}
> > +
> > static int handle_tprot(struct kvm_vcpu *vcpu)
> > {
> > 	int base1 = (vcpu->arch.sie_block->ipb & 0xf0000000) >> 28;
> > diff --git a/arch/s390/kvm/trace-s390.h b/arch/s390/kvm/trace-s390.h
> > index 90fdf85..95fbc1a 100644
> > --- a/arch/s390/kvm/trace-s390.h
> > +++ b/arch/s390/kvm/trace-s390.h
> > @@ -141,13 +141,13 @@ TRACE_EVENT(kvm_s390_inject_vcpu,
> >  * Trace point for the actual delivery of interrupts.
> >  */
> > TRACE_EVENT(kvm_s390_deliver_interrupt,
> > -	    TP_PROTO(unsigned int id, __u64 type, __u32 data0, __u64 data1),
> > +	    TP_PROTO(unsigned int id, __u64 type, __u64 data0, __u64 data1),
> > 	    TP_ARGS(id, type, data0, data1),
> > 
> > 	    TP_STRUCT__entry(
> > 		    __field(int, id)
> > 		    __field(__u32, inttype)
> > -		    __field(__u32, data0)
> > +		    __field(__u64, data0)
> > 		    __field(__u64, data1)
> > 		    ),
> > 
> > @@ -159,7 +159,7 @@ TRACE_EVENT(kvm_s390_deliver_interrupt,
> > 		    ),
> > 
> > 	    TP_printk("deliver interrupt (vcpu %d): type:%x (%s) "	\
> > -		      "data:%08x %016llx",
> > +		      "data:%08llx %016llx",
> > 		      __entry->id, __entry->inttype,
> > 		      __print_symbolic(__entry->inttype, kvm_s390_int_type),
> > 		      __entry->data0, __entry->data1)
> > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > index e676940..22859dc 100644
> > --- a/include/uapi/linux/kvm.h
> > +++ b/include/uapi/linux/kvm.h
> > @@ -397,6 +397,7 @@ struct kvm_s390_psw {
> > #define KVM_S390_PROGRAM_INT		0xfffe0001u
> > #define KVM_S390_SIGP_SET_PREFIX	0xfffe0002u
> > #define KVM_S390_RESTART		0xfffe0003u
> > +#define KVM_S390_MCHK			0xfffe1000u
> > #define KVM_S390_INT_VIRTIO		0xffff2603u
> > #define KVM_S390_INT_SERVICE		0xffff2401u
> > #define KVM_S390_INT_EMERGENCY		0xffff1201u
> > -- 
> > 1.7.12.4
> > 
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 3/5] KVM: s390: In-kernel handling of I/O instructions.
  2012-12-10  7:53   ` Alexander Graf
@ 2012-12-10 10:14     ` Cornelia Huck
  0 siblings, 0 replies; 24+ messages in thread
From: Cornelia Huck @ 2012-12-10 10:14 UTC (permalink / raw)
  To: Alexander Graf
  Cc: Marcelo Tosatti, Gleb Natapov, KVM, linux-s390, Avi Kivity,
	Christian Borntraeger, Carsten Otte, Heiko Carstens,
	Martin Schwidefsky, Sebastian Ott

On Mon, 10 Dec 2012 08:53:41 +0100
Alexander Graf <agraf@suse.de> wrote:

> 
> On 07.12.2012, at 13:30, Cornelia Huck wrote:
> 
> > Explicitely catch all channel I/O related instructions intercepts
> > in the kernel and set condition code 3 for them.
> > 
> > This paves the way for properly handling these instructions later
> > on.
> > 
> > Note: This is not architecture compliant (the previous code wasn't
> > either) since setting cc 3 is not the correct thing to do for some
> > of these instructions. For Linux guests, however, it still has the
> > intended effect of stopping css probing.
> > 
> > Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > ---
> > arch/s390/kvm/intercept.c | 19 +++++++++++++---
> > arch/s390/kvm/kvm-s390.h  |  1 +
> > arch/s390/kvm/priv.c      | 56 +++++++++++++++++++++++++++++++++--------------
> > 3 files changed, 56 insertions(+), 20 deletions(-)
> > 
> > diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
> > index ec1177f..754dc9e 100644
> > --- a/arch/s390/kvm/intercept.c
> > +++ b/arch/s390/kvm/intercept.c
> > @@ -33,8 +33,6 @@ static int handle_lctlg(struct kvm_vcpu *vcpu)
> > 	int reg, rc;
> > 
> > 	vcpu->stat.instruction_lctlg++;
> > -	if ((vcpu->arch.sie_block->ipb & 0xff) != 0x2f)
> > -		return -EOPNOTSUPP;
> > 
> > 	useraddr = disp2;
> > 	if (base2)
> > @@ -104,6 +102,21 @@ static int handle_lctl(struct kvm_vcpu *vcpu)
> > 	return 0;
> > }
> > 
> > +static intercept_handler_t eb_handlers[256] = {
> 
> const.
> 
> Reviewed-by: Alexander Graf <agraf@suse.de>

Thanks.

> 
> 
> Alex
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/5] KVM: s390: Base infrastructure for enabling capabilities.
  2012-12-10  7:54   ` Alexander Graf
@ 2012-12-10 10:16     ` Cornelia Huck
  2012-12-11 10:24       ` Alexander Graf
  0 siblings, 1 reply; 24+ messages in thread
From: Cornelia Huck @ 2012-12-10 10:16 UTC (permalink / raw)
  To: Alexander Graf
  Cc: Marcelo Tosatti, Gleb Natapov, KVM, linux-s390, Avi Kivity,
	Christian Borntraeger, Carsten Otte, Heiko Carstens,
	Martin Schwidefsky, Sebastian Ott

On Mon, 10 Dec 2012 08:54:45 +0100
Alexander Graf <agraf@suse.de> wrote:

> Missing patch description. Otherwise:

"Add base infrastructure for enabling capabilities" would be redundant,
no?

> 
> Acked-by: Alexander Graf <agraf@suse.de>
> 
> 
> Alex
> 
> On 07.12.2012, at 13:30, Cornelia Huck wrote:
> 
> > Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > ---
> > Documentation/virtual/kvm/api.txt |  2 +-
> > arch/s390/kvm/kvm-s390.c          | 26 ++++++++++++++++++++++++++
> > 2 files changed, 27 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> > index 8617339..bba630b 100644
> > --- a/Documentation/virtual/kvm/api.txt
> > +++ b/Documentation/virtual/kvm/api.txt
> > @@ -913,7 +913,7 @@ documentation when it pops into existence).
> > 4.37 KVM_ENABLE_CAP
> > 
> > Capability: KVM_CAP_ENABLE_CAP
> > -Architectures: ppc
> > +Architectures: ppc, s390
> > Type: vcpu ioctl
> > Parameters: struct kvm_enable_cap (in)
> > Returns: 0 on success; -1 on error
> > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> > index 731ddee..c015f6f 100644
> > --- a/arch/s390/kvm/kvm-s390.c
> > +++ b/arch/s390/kvm/kvm-s390.c
> > @@ -140,6 +140,7 @@ int kvm_dev_ioctl_check_extension(long ext)
> > #endif
> > 	case KVM_CAP_SYNC_REGS:
> > 	case KVM_CAP_ONE_REG:
> > +	case KVM_CAP_ENABLE_CAP:
> > 		r = 1;
> > 		break;
> > 	case KVM_CAP_NR_VCPUS:
> > @@ -812,6 +813,22 @@ int kvm_s390_vcpu_store_status(struct kvm_vcpu *vcpu, unsigned long addr)
> > 	return 0;
> > }
> > 
> > +static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
> > +				     struct kvm_enable_cap *cap)
> > +{
> > +	int r;
> > +
> > +	if (cap->flags)
> > +		return -EINVAL;
> > +
> > +	switch (cap->cap) {
> > +	default:
> > +		r = -EINVAL;
> > +		break;
> > +	}
> > +	return r;
> > +}
> > +
> > long kvm_arch_vcpu_ioctl(struct file *filp,
> > 			 unsigned int ioctl, unsigned long arg)
> > {
> > @@ -898,6 +915,15 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> > 			r = 0;
> > 		break;
> > 	}
> > +	case KVM_ENABLE_CAP:
> > +	{
> > +		struct kvm_enable_cap cap;
> > +		r = -EFAULT;
> > +		if (copy_from_user(&cap, argp, sizeof(cap)))
> > +			break;
> > +		r = kvm_vcpu_ioctl_enable_cap(vcpu, &cap);
> > +		break;
> > +	}
> > 	default:
> > 		r = -ENOTTY;
> > 	}
> > -- 
> > 1.7.12.4
> > 
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/5] KVM: s390: Support for I/O interrupts.
  2012-12-10 10:09     ` Cornelia Huck
@ 2012-12-11 10:22       ` Alexander Graf
  2012-12-11 12:46         ` Cornelia Huck
  0 siblings, 1 reply; 24+ messages in thread
From: Alexander Graf @ 2012-12-11 10:22 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: Marcelo Tosatti, Gleb Natapov, KVM, linux-s390, Avi Kivity,
	Christian Borntraeger, Carsten Otte, Heiko Carstens,
	Martin Schwidefsky, Sebastian Ott


On 10.12.2012, at 11:09, Cornelia Huck wrote:

> On Mon, 10 Dec 2012 08:33:10 +0100
> Alexander Graf <agraf@suse.de> wrote:
> 
>> 
>> On 07.12.2012, at 13:30, Cornelia Huck wrote:
>> 
>>> Add support for handling I/O interrupts (standard, subchannel-related
>>> ones and rudimentary adapter interrupts).
>>> 
>>> The subchannel-identifying parameters are encoded into the interrupt
>>> type.
>>> 
>>> I/O interrupts are floating, so they can't be injected on a specific
>>> vcpu.
>>> 
>>> Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
>>> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>>> ---
>>> Documentation/virtual/kvm/api.txt |   4 ++
>>> arch/s390/include/asm/kvm_host.h  |   2 +
>>> arch/s390/kvm/interrupt.c         | 115 ++++++++++++++++++++++++++++++++++++--
>>> include/uapi/linux/kvm.h          |   6 ++
>>> 4 files changed, 122 insertions(+), 5 deletions(-)
>>> 
>>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>>> index 6671fdc..e298a72 100644
>>> --- a/Documentation/virtual/kvm/api.txt
>>> +++ b/Documentation/virtual/kvm/api.txt
>>> @@ -2068,6 +2068,10 @@ KVM_S390_INT_VIRTIO (vm) - virtio external interrupt; external interrupt
>>> KVM_S390_INT_SERVICE (vm) - sclp external interrupt; sclp parameter in parm
>>> KVM_S390_INT_EMERGENCY (vcpu) - sigp emergency; source cpu in parm
>>> KVM_S390_INT_EXTERNAL_CALL (vcpu) - sigp external call; source cpu in parm
>>> +KVM_S390_INT_IO(ai,cssid,ssid,schid) (vm) - compound value to indicate an
>>> +    I/O interrupt (ai - adapter interrupt; cssid,ssid,schid - subchannel);
>>> +    I/O interruption parameters in parm (subchannel) and parm64 (intparm,
>>> +    interruption subclass)
>>> 
>>> Note that the vcpu ioctl is asynchronous to vcpu execution.
>>> 
>>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>>> index b784154..e47f697 100644
>>> --- a/arch/s390/include/asm/kvm_host.h
>>> +++ b/arch/s390/include/asm/kvm_host.h
>>> @@ -76,6 +76,7 @@ struct kvm_s390_sie_block {
>>> 	__u64	epoch;			/* 0x0038 */
>>> 	__u8	reserved40[4];		/* 0x0040 */
>>> #define LCTL_CR0	0x8000
>>> +#define LCTL_CR6	0x0200
>>> 	__u16   lctl;			/* 0x0044 */
>>> 	__s16	icpua;			/* 0x0046 */
>>> 	__u32	ictl;			/* 0x0048 */
>>> @@ -127,6 +128,7 @@ struct kvm_vcpu_stat {
>>> 	u32 deliver_prefix_signal;
>>> 	u32 deliver_restart_signal;
>>> 	u32 deliver_program_int;
>>> +	u32 deliver_io_int;
>>> 	u32 exit_wait_state;
>>> 	u32 instruction_stidp;
>>> 	u32 instruction_spx;
>>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>>> index c30615e..070ba22 100644
>>> --- a/arch/s390/kvm/interrupt.c
>>> +++ b/arch/s390/kvm/interrupt.c
>>> @@ -21,11 +21,26 @@
>>> #include "gaccess.h"
>>> #include "trace-s390.h"
>>> 
>>> +#define IOINT_SCHID_MASK 0x0000ffff
>>> +#define IOINT_SSID_MASK 0x00030000
>>> +#define IOINT_CSSID_MASK 0x03fc0000
>>> +#define IOINT_AI_MASK 0x04000000
>>> +
>>> +static int is_ioint(u64 type)
>>> +{
>>> +	return ((type & 0xfffe0000u) != 0xfffe0000u);
>>> +}
>>> +
>>> static int psw_extint_disabled(struct kvm_vcpu *vcpu)
>>> {
>>> 	return !(vcpu->arch.sie_block->gpsw.mask & PSW_MASK_EXT);
>>> }
>>> 
>>> +static int psw_ioint_disabled(struct kvm_vcpu *vcpu)
>>> +{
>>> +	return !(vcpu->arch.sie_block->gpsw.mask & PSW_MASK_IO);
>>> +}
>>> +
>>> static int psw_interrupts_disabled(struct kvm_vcpu *vcpu)
>>> {
>>> 	if ((vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PER) ||
>>> @@ -68,7 +83,18 @@ static int __interrupt_is_deliverable(struct kvm_vcpu *vcpu,
>>> 	case KVM_S390_RESTART:
>>> 		return 1;
>>> 	default:
>>> -		BUG();
>>> +		if (is_ioint(inti->type)) {
>> 
>> Though I usually like if (...) { positive) } else { abort(); } coding style in general, it makes code quite hard to read when you are limited to 80 characters per line :)
>> 
>> I think it'd really help readability if you instead would write
>> 
>> if (!is_ioint(...)) {
>>    BUG();
>> }
>> 
>> and then continue without indent. That problem gets even more obvious further down the file.
> 
> Hm, "bad state last" seems to parse, though.

Fine with me. Extract the io bits into a separate function then :). Or maybe better even just use gcc switch magic:

  case 0x0 ... 0xfffdffff:


Alex

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/5] KVM: s390: Base infrastructure for enabling capabilities.
  2012-12-10 10:16     ` Cornelia Huck
@ 2012-12-11 10:24       ` Alexander Graf
  0 siblings, 0 replies; 24+ messages in thread
From: Alexander Graf @ 2012-12-11 10:24 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: Marcelo Tosatti, Gleb Natapov, KVM, linux-s390, Avi Kivity,
	Christian Borntraeger, Carsten Otte, Heiko Carstens,
	Martin Schwidefsky, Sebastian Ott


On 10.12.2012, at 11:16, Cornelia Huck wrote:

> On Mon, 10 Dec 2012 08:54:45 +0100
> Alexander Graf <agraf@suse.de> wrote:
> 
>> Missing patch description. Otherwise:
> 
> "Add base infrastructure for enabling capabilities" would be redundant,
> no?

Well, whoever pulls this gets to say what style he likes. I simply heads-on reject every patch that doesn't contain a patch description, but only a subject line :). Others are a lot more relaxed about this.

And yes, it'd be redundant, but easier readable imho.


Alex

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/5] KVM: s390: Support for I/O interrupts.
  2012-12-11 10:22       ` Alexander Graf
@ 2012-12-11 12:46         ` Cornelia Huck
  2012-12-12  0:36           ` Alexander Graf
  0 siblings, 1 reply; 24+ messages in thread
From: Cornelia Huck @ 2012-12-11 12:46 UTC (permalink / raw)
  To: Alexander Graf
  Cc: Marcelo Tosatti, Gleb Natapov, KVM, linux-s390, Avi Kivity,
	Christian Borntraeger, Carsten Otte, Heiko Carstens,
	Martin Schwidefsky, Sebastian Ott

On Tue, 11 Dec 2012 11:22:04 +0100
Alexander Graf <agraf@suse.de> wrote:

> 
> On 10.12.2012, at 11:09, Cornelia Huck wrote:
> 
> > On Mon, 10 Dec 2012 08:33:10 +0100
> > Alexander Graf <agraf@suse.de> wrote:
> > 
> >> 
> >> On 07.12.2012, at 13:30, Cornelia Huck wrote:
> >> 
> >>> Add support for handling I/O interrupts (standard, subchannel-related
> >>> ones and rudimentary adapter interrupts).
> >>> 
> >>> The subchannel-identifying parameters are encoded into the interrupt
> >>> type.
> >>> 
> >>> I/O interrupts are floating, so they can't be injected on a specific
> >>> vcpu.
> >>> 
> >>> Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
> >>> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> >>> ---
> >>> Documentation/virtual/kvm/api.txt |   4 ++
> >>> arch/s390/include/asm/kvm_host.h  |   2 +
> >>> arch/s390/kvm/interrupt.c         | 115 ++++++++++++++++++++++++++++++++++++--
> >>> include/uapi/linux/kvm.h          |   6 ++
> >>> 4 files changed, 122 insertions(+), 5 deletions(-)
> >>> 
> >>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> >>> index 6671fdc..e298a72 100644
> >>> --- a/Documentation/virtual/kvm/api.txt
> >>> +++ b/Documentation/virtual/kvm/api.txt
> >>> @@ -2068,6 +2068,10 @@ KVM_S390_INT_VIRTIO (vm) - virtio external interrupt; external interrupt
> >>> KVM_S390_INT_SERVICE (vm) - sclp external interrupt; sclp parameter in parm
> >>> KVM_S390_INT_EMERGENCY (vcpu) - sigp emergency; source cpu in parm
> >>> KVM_S390_INT_EXTERNAL_CALL (vcpu) - sigp external call; source cpu in parm
> >>> +KVM_S390_INT_IO(ai,cssid,ssid,schid) (vm) - compound value to indicate an
> >>> +    I/O interrupt (ai - adapter interrupt; cssid,ssid,schid - subchannel);
> >>> +    I/O interruption parameters in parm (subchannel) and parm64 (intparm,
> >>> +    interruption subclass)
> >>> 
> >>> Note that the vcpu ioctl is asynchronous to vcpu execution.
> >>> 
> >>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> >>> index b784154..e47f697 100644
> >>> --- a/arch/s390/include/asm/kvm_host.h
> >>> +++ b/arch/s390/include/asm/kvm_host.h
> >>> @@ -76,6 +76,7 @@ struct kvm_s390_sie_block {
> >>> 	__u64	epoch;			/* 0x0038 */
> >>> 	__u8	reserved40[4];		/* 0x0040 */
> >>> #define LCTL_CR0	0x8000
> >>> +#define LCTL_CR6	0x0200
> >>> 	__u16   lctl;			/* 0x0044 */
> >>> 	__s16	icpua;			/* 0x0046 */
> >>> 	__u32	ictl;			/* 0x0048 */
> >>> @@ -127,6 +128,7 @@ struct kvm_vcpu_stat {
> >>> 	u32 deliver_prefix_signal;
> >>> 	u32 deliver_restart_signal;
> >>> 	u32 deliver_program_int;
> >>> +	u32 deliver_io_int;
> >>> 	u32 exit_wait_state;
> >>> 	u32 instruction_stidp;
> >>> 	u32 instruction_spx;
> >>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> >>> index c30615e..070ba22 100644
> >>> --- a/arch/s390/kvm/interrupt.c
> >>> +++ b/arch/s390/kvm/interrupt.c
> >>> @@ -21,11 +21,26 @@
> >>> #include "gaccess.h"
> >>> #include "trace-s390.h"
> >>> 
> >>> +#define IOINT_SCHID_MASK 0x0000ffff
> >>> +#define IOINT_SSID_MASK 0x00030000
> >>> +#define IOINT_CSSID_MASK 0x03fc0000
> >>> +#define IOINT_AI_MASK 0x04000000
> >>> +
> >>> +static int is_ioint(u64 type)
> >>> +{
> >>> +	return ((type & 0xfffe0000u) != 0xfffe0000u);
> >>> +}
> >>> +
> >>> static int psw_extint_disabled(struct kvm_vcpu *vcpu)
> >>> {
> >>> 	return !(vcpu->arch.sie_block->gpsw.mask & PSW_MASK_EXT);
> >>> }
> >>> 
> >>> +static int psw_ioint_disabled(struct kvm_vcpu *vcpu)
> >>> +{
> >>> +	return !(vcpu->arch.sie_block->gpsw.mask & PSW_MASK_IO);
> >>> +}
> >>> +
> >>> static int psw_interrupts_disabled(struct kvm_vcpu *vcpu)
> >>> {
> >>> 	if ((vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PER) ||
> >>> @@ -68,7 +83,18 @@ static int __interrupt_is_deliverable(struct kvm_vcpu *vcpu,
> >>> 	case KVM_S390_RESTART:
> >>> 		return 1;
> >>> 	default:
> >>> -		BUG();
> >>> +		if (is_ioint(inti->type)) {
> >> 
> >> Though I usually like if (...) { positive) } else { abort(); } coding style in general, it makes code quite hard to read when you are limited to 80 characters per line :)
> >> 
> >> I think it'd really help readability if you instead would write
> >> 
> >> if (!is_ioint(...)) {
> >>    BUG();
> >> }
> >> 
> >> and then continue without indent. That problem gets even more obvious further down the file.
> > 
> > Hm, "bad state last" seems to parse, though.
> 
> Fine with me. Extract the io bits into a separate function then :). Or maybe better even just use gcc switch magic:
> 
>   case 0x0 ... 0xfffdffff:

What, magic numbers? ;)

I'll check how this works out. Adding a #define for that range might
even define better what INT_IO is.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/5] KVM: s390: Support for I/O interrupts.
  2012-12-11 12:46         ` Cornelia Huck
@ 2012-12-12  0:36           ` Alexander Graf
  0 siblings, 0 replies; 24+ messages in thread
From: Alexander Graf @ 2012-12-12  0:36 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: Marcelo Tosatti, Gleb Natapov, KVM, linux-s390, Avi Kivity,
	Christian Borntraeger, Carsten Otte, Heiko Carstens,
	Martin Schwidefsky, Sebastian Ott


On 11.12.2012, at 13:46, Cornelia Huck wrote:

> On Tue, 11 Dec 2012 11:22:04 +0100
> Alexander Graf <agraf@suse.de> wrote:
> 
>> 
>> On 10.12.2012, at 11:09, Cornelia Huck wrote:
>> 
>>> On Mon, 10 Dec 2012 08:33:10 +0100
>>> Alexander Graf <agraf@suse.de> wrote:
>>> 
>>>> 
>>>> On 07.12.2012, at 13:30, Cornelia Huck wrote:
>>>> 
>>>>> Add support for handling I/O interrupts (standard, subchannel-related
>>>>> ones and rudimentary adapter interrupts).
>>>>> 
>>>>> The subchannel-identifying parameters are encoded into the interrupt
>>>>> type.
>>>>> 
>>>>> I/O interrupts are floating, so they can't be injected on a specific
>>>>> vcpu.
>>>>> 
>>>>> Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
>>>>> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>>>>> ---
>>>>> Documentation/virtual/kvm/api.txt |   4 ++
>>>>> arch/s390/include/asm/kvm_host.h  |   2 +
>>>>> arch/s390/kvm/interrupt.c         | 115 ++++++++++++++++++++++++++++++++++++--
>>>>> include/uapi/linux/kvm.h          |   6 ++
>>>>> 4 files changed, 122 insertions(+), 5 deletions(-)
>>>>> 
>>>>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>>>>> index 6671fdc..e298a72 100644
>>>>> --- a/Documentation/virtual/kvm/api.txt
>>>>> +++ b/Documentation/virtual/kvm/api.txt
>>>>> @@ -2068,6 +2068,10 @@ KVM_S390_INT_VIRTIO (vm) - virtio external interrupt; external interrupt
>>>>> KVM_S390_INT_SERVICE (vm) - sclp external interrupt; sclp parameter in parm
>>>>> KVM_S390_INT_EMERGENCY (vcpu) - sigp emergency; source cpu in parm
>>>>> KVM_S390_INT_EXTERNAL_CALL (vcpu) - sigp external call; source cpu in parm
>>>>> +KVM_S390_INT_IO(ai,cssid,ssid,schid) (vm) - compound value to indicate an
>>>>> +    I/O interrupt (ai - adapter interrupt; cssid,ssid,schid - subchannel);
>>>>> +    I/O interruption parameters in parm (subchannel) and parm64 (intparm,
>>>>> +    interruption subclass)
>>>>> 
>>>>> Note that the vcpu ioctl is asynchronous to vcpu execution.
>>>>> 
>>>>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>>>>> index b784154..e47f697 100644
>>>>> --- a/arch/s390/include/asm/kvm_host.h
>>>>> +++ b/arch/s390/include/asm/kvm_host.h
>>>>> @@ -76,6 +76,7 @@ struct kvm_s390_sie_block {
>>>>> 	__u64	epoch;			/* 0x0038 */
>>>>> 	__u8	reserved40[4];		/* 0x0040 */
>>>>> #define LCTL_CR0	0x8000
>>>>> +#define LCTL_CR6	0x0200
>>>>> 	__u16   lctl;			/* 0x0044 */
>>>>> 	__s16	icpua;			/* 0x0046 */
>>>>> 	__u32	ictl;			/* 0x0048 */
>>>>> @@ -127,6 +128,7 @@ struct kvm_vcpu_stat {
>>>>> 	u32 deliver_prefix_signal;
>>>>> 	u32 deliver_restart_signal;
>>>>> 	u32 deliver_program_int;
>>>>> +	u32 deliver_io_int;
>>>>> 	u32 exit_wait_state;
>>>>> 	u32 instruction_stidp;
>>>>> 	u32 instruction_spx;
>>>>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>>>>> index c30615e..070ba22 100644
>>>>> --- a/arch/s390/kvm/interrupt.c
>>>>> +++ b/arch/s390/kvm/interrupt.c
>>>>> @@ -21,11 +21,26 @@
>>>>> #include "gaccess.h"
>>>>> #include "trace-s390.h"
>>>>> 
>>>>> +#define IOINT_SCHID_MASK 0x0000ffff
>>>>> +#define IOINT_SSID_MASK 0x00030000
>>>>> +#define IOINT_CSSID_MASK 0x03fc0000
>>>>> +#define IOINT_AI_MASK 0x04000000
>>>>> +
>>>>> +static int is_ioint(u64 type)
>>>>> +{
>>>>> +	return ((type & 0xfffe0000u) != 0xfffe0000u);
>>>>> +}
>>>>> +
>>>>> static int psw_extint_disabled(struct kvm_vcpu *vcpu)
>>>>> {
>>>>> 	return !(vcpu->arch.sie_block->gpsw.mask & PSW_MASK_EXT);
>>>>> }
>>>>> 
>>>>> +static int psw_ioint_disabled(struct kvm_vcpu *vcpu)
>>>>> +{
>>>>> +	return !(vcpu->arch.sie_block->gpsw.mask & PSW_MASK_IO);
>>>>> +}
>>>>> +
>>>>> static int psw_interrupts_disabled(struct kvm_vcpu *vcpu)
>>>>> {
>>>>> 	if ((vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PER) ||
>>>>> @@ -68,7 +83,18 @@ static int __interrupt_is_deliverable(struct kvm_vcpu *vcpu,
>>>>> 	case KVM_S390_RESTART:
>>>>> 		return 1;
>>>>> 	default:
>>>>> -		BUG();
>>>>> +		if (is_ioint(inti->type)) {
>>>> 
>>>> Though I usually like if (...) { positive) } else { abort(); } coding style in general, it makes code quite hard to read when you are limited to 80 characters per line :)
>>>> 
>>>> I think it'd really help readability if you instead would write
>>>> 
>>>> if (!is_ioint(...)) {
>>>>   BUG();
>>>> }
>>>> 
>>>> and then continue without indent. That problem gets even more obvious further down the file.
>>> 
>>> Hm, "bad state last" seems to parse, though.
>> 
>> Fine with me. Extract the io bits into a separate function then :). Or maybe better even just use gcc switch magic:
>> 
>>  case 0x0 ... 0xfffdffff:
> 
> What, magic numbers? ;)

Well, in code it would be

   case KVM_S390_IOINT_MIN ... KVM_S390_IOINT_MAX:

I was just trying to make the point clear :).

> I'll check how this works out. Adding a #define for that range might
> even define better what INT_IO is.

Yup :)


Alex

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/5] KVM: s390: Add support for machine checks.
  2012-12-07 12:30 ` [PATCH 2/5] KVM: s390: Add support for machine checks Cornelia Huck
@ 2012-12-19  9:44     ` Heiko Carstens
  2012-12-19  9:44     ` Heiko Carstens
  1 sibling, 0 replies; 24+ messages in thread
From: Heiko Carstens @ 2012-12-19  9:44 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: Marcelo Tosatti, Gleb Natapov, KVM, linux-s390, Avi Kivity,
	Christian Borntraeger, Carsten Otte, Alexander Graf,
	Martin Schwidefsky, Sebastian Ott

On Fri, Dec 07, 2012 at 01:30:22PM +0100, Cornelia Huck wrote:
> +		rc = put_guest_u64(vcpu, __LC_MCCK_CODE, inti->mchk.mcic);
> +		if (rc == -EFAULT)
> +			exception = 1;
> +
> +		rc = copy_to_guest(vcpu, __LC_MCK_OLD_PSW,
> +				   &vcpu->arch.sie_block->gpsw, sizeof(psw_t));
> +		if (rc == -EFAULT)
> +			exception = 1;

Please don't add more explicit -EFAULT checks on guest access paths. Just
make this like normal user space accesses. That is return code != 0 means
an error occured:

	rc = put_guest_u64(vcpu, __LC_MCCK_CODE, inti->mchk.mcic);
	if (rc)
		exception = 1;

In fact, with the current kvm gaccess code it's even broken, since on error
the guest access functions may return also -ENOMEM instead of -EFAULT, which
would be ignored by your code.
I addressed that with a patch when trying to clean up the guest access
functions. Maybe the patch below should be merged anyway. Christian?


===================


From db05454b6f3f49a7a10f5a1e546917303cf94532 Mon Sep 17 00:00:00 2001
From: Heiko Carstens <heiko.carstens@de.ibm.com>
Date: Mon, 10 Sep 2012 16:36:23 +0200
Subject: s390/kvm,gaccess: fix guest access return code handling

Guest access functions like copy_to/from_guest() call __guestaddr_to_user()
which in turn call gmap_fault() in order to translate a guest address to a
user space address.
In error case __guest_addr_to_user() returns either -EFAULT or -ENOMEM.
The copy_to/from_guest functions just pass these return values down to the
callers.
The -ENOMEM case however is problematic since there are several places
which access guest memory like:

rc = copy_to_guest(...);
if (rc == -EFAULT)
	error_handling();

So in case of -ENOMEM the code assumes that the guest memory access
succeeded even though it failed.
This can cause guest data or state corruption.

If __guestaddr_to_user() returns -ENOMEM the meaning is that a valid user
space mapping exists, but there was not enough memory available when trying
to build the guest mapping. In other words an out-of-memory situation
occured.
For normal user space accesses an out-of-memory situation causes the page
fault handler to map -ENOMEM to -EFAULT (see fixup code in do_no_context()).
We need to do exactly the same for the kvm gaccess functions.

So __guestaddr_to_user() should just map all error codes to -EFAULT.

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 arch/s390/kvm/gaccess.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kvm/gaccess.h b/arch/s390/kvm/gaccess.h
index 4703f12..84d01dd 100644
--- a/arch/s390/kvm/gaccess.h
+++ b/arch/s390/kvm/gaccess.h
@@ -22,13 +22,16 @@ static inline void __user *__guestaddr_to_user(struct kvm_vcpu *vcpu,
 					       unsigned long guestaddr)
 {
 	unsigned long prefix  = vcpu->arch.sie_block->prefix;
+	unsigned long uaddress;
 
 	if (guestaddr < 2 * PAGE_SIZE)
 		guestaddr += prefix;
 	else if ((guestaddr >= prefix) && (guestaddr < prefix + 2 * PAGE_SIZE))
 		guestaddr -= prefix;
-
-	return (void __user *) gmap_fault(guestaddr, vcpu->arch.gmap);
+	uaddress = gmap_fault(guestaddr, vcpu->arch.gmap);
+	if (IS_ERR_VALUE(uaddress))
+		uaddress = -EFAULT;
+	return (void __user *)uaddress;
 }
 
 static inline int get_guest_u64(struct kvm_vcpu *vcpu, unsigned long guestaddr,
-- 
1.7.12.4

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/5] KVM: s390: Add support for machine checks.
@ 2012-12-19  9:44     ` Heiko Carstens
  0 siblings, 0 replies; 24+ messages in thread
From: Heiko Carstens @ 2012-12-19  9:44 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: Marcelo Tosatti, Gleb Natapov, KVM, linux-s390, Avi Kivity,
	Christian Borntraeger, Carsten Otte, Alexander Graf,
	Martin Schwidefsky, Sebastian Ott

On Fri, Dec 07, 2012 at 01:30:22PM +0100, Cornelia Huck wrote:
> +		rc = put_guest_u64(vcpu, __LC_MCCK_CODE, inti->mchk.mcic);
> +		if (rc == -EFAULT)
> +			exception = 1;
> +
> +		rc = copy_to_guest(vcpu, __LC_MCK_OLD_PSW,
> +				   &vcpu->arch.sie_block->gpsw, sizeof(psw_t));
> +		if (rc == -EFAULT)
> +			exception = 1;

Please don't add more explicit -EFAULT checks on guest access paths. Just
make this like normal user space accesses. That is return code != 0 means
an error occured:

	rc = put_guest_u64(vcpu, __LC_MCCK_CODE, inti->mchk.mcic);
	if (rc)
		exception = 1;

In fact, with the current kvm gaccess code it's even broken, since on error
the guest access functions may return also -ENOMEM instead of -EFAULT, which
would be ignored by your code.
I addressed that with a patch when trying to clean up the guest access
functions. Maybe the patch below should be merged anyway. Christian?


===================


>From db05454b6f3f49a7a10f5a1e546917303cf94532 Mon Sep 17 00:00:00 2001
From: Heiko Carstens <heiko.carstens@de.ibm.com>
Date: Mon, 10 Sep 2012 16:36:23 +0200
Subject: s390/kvm,gaccess: fix guest access return code handling

Guest access functions like copy_to/from_guest() call __guestaddr_to_user()
which in turn call gmap_fault() in order to translate a guest address to a
user space address.
In error case __guest_addr_to_user() returns either -EFAULT or -ENOMEM.
The copy_to/from_guest functions just pass these return values down to the
callers.
The -ENOMEM case however is problematic since there are several places
which access guest memory like:

rc = copy_to_guest(...);
if (rc == -EFAULT)
	error_handling();

So in case of -ENOMEM the code assumes that the guest memory access
succeeded even though it failed.
This can cause guest data or state corruption.

If __guestaddr_to_user() returns -ENOMEM the meaning is that a valid user
space mapping exists, but there was not enough memory available when trying
to build the guest mapping. In other words an out-of-memory situation
occured.
For normal user space accesses an out-of-memory situation causes the page
fault handler to map -ENOMEM to -EFAULT (see fixup code in do_no_context()).
We need to do exactly the same for the kvm gaccess functions.

So __guestaddr_to_user() should just map all error codes to -EFAULT.

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 arch/s390/kvm/gaccess.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kvm/gaccess.h b/arch/s390/kvm/gaccess.h
index 4703f12..84d01dd 100644
--- a/arch/s390/kvm/gaccess.h
+++ b/arch/s390/kvm/gaccess.h
@@ -22,13 +22,16 @@ static inline void __user *__guestaddr_to_user(struct kvm_vcpu *vcpu,
 					       unsigned long guestaddr)
 {
 	unsigned long prefix  = vcpu->arch.sie_block->prefix;
+	unsigned long uaddress;
 
 	if (guestaddr < 2 * PAGE_SIZE)
 		guestaddr += prefix;
 	else if ((guestaddr >= prefix) && (guestaddr < prefix + 2 * PAGE_SIZE))
 		guestaddr -= prefix;
-
-	return (void __user *) gmap_fault(guestaddr, vcpu->arch.gmap);
+	uaddress = gmap_fault(guestaddr, vcpu->arch.gmap);
+	if (IS_ERR_VALUE(uaddress))
+		uaddress = -EFAULT;
+	return (void __user *)uaddress;
 }
 
 static inline int get_guest_u64(struct kvm_vcpu *vcpu, unsigned long guestaddr,
-- 
1.7.12.4


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/5] KVM: s390: Add support for machine checks.
  2012-12-19  9:44     ` Heiko Carstens
  (?)
@ 2012-12-19 10:20     ` Christian Borntraeger
  2012-12-19 13:07       ` Heiko Carstens
  -1 siblings, 1 reply; 24+ messages in thread
From: Christian Borntraeger @ 2012-12-19 10:20 UTC (permalink / raw)
  To: Heiko Carstens
  Cc: Cornelia Huck, Marcelo Tosatti, Gleb Natapov, KVM, linux-s390,
	Avi Kivity, Carsten Otte, Alexander Graf, Martin Schwidefsky,
	Sebastian Ott

On 19/12/12 10:44, Heiko Carstens wrote:
> On Fri, Dec 07, 2012 at 01:30:22PM +0100, Cornelia Huck wrote:
>> +		rc = put_guest_u64(vcpu, __LC_MCCK_CODE, inti->mchk.mcic);
>> +		if (rc == -EFAULT)
>> +			exception = 1;
>> +
>> +		rc = copy_to_guest(vcpu, __LC_MCK_OLD_PSW,
>> +				   &vcpu->arch.sie_block->gpsw, sizeof(psw_t));
>> +		if (rc == -EFAULT)
>> +			exception = 1;
> 
> Please don't add more explicit -EFAULT checks on guest access paths. Just
> make this like normal user space accesses. That is return code != 0 means
> an error occured:
> 
> 	rc = put_guest_u64(vcpu, __LC_MCCK_CODE, inti->mchk.mcic);
> 	if (rc)
> 		exception = 1;
> 
> In fact, with the current kvm gaccess code it's even broken, since on error
> the guest access functions may return also -ENOMEM instead of -EFAULT, which
> would be ignored by your code.
> I addressed that with a patch when trying to clean up the guest access
> functions. Maybe the patch below should be merged anyway. Christian?

The whole guest memory access of KVM needs to be reworked to work properly
in those corner cases. I have this on my todo list as one of things for next
year with lots of open questions that I dont want to answer before xmas.
what about in kernel intercepts? (shall we then return EFAULT for the KVM_RUN
ioctl, shall we kill the guest?.....)

We actually need to test the address for validity via the memslots (and not
via return value of copy_from/to_user) all across the s390 code.

I really want to avoid mixing this effort with the virtio-ccw patches.
So my proposal is to apply your patch below and keep Conny's patch as is.
Ok?

Christian


> From db05454b6f3f49a7a10f5a1e546917303cf94532 Mon Sep 17 00:00:00 2001
> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> Date: Mon, 10 Sep 2012 16:36:23 +0200
> Subject: s390/kvm,gaccess: fix guest access return code handling
> 
> Guest access functions like copy_to/from_guest() call __guestaddr_to_user()
> which in turn call gmap_fault() in order to translate a guest address to a
> user space address.
> In error case __guest_addr_to_user() returns either -EFAULT or -ENOMEM.
> The copy_to/from_guest functions just pass these return values down to the
> callers.
> The -ENOMEM case however is problematic since there are several places
> which access guest memory like:
> 
> rc = copy_to_guest(...);
> if (rc == -EFAULT)
> 	error_handling();
> 
> So in case of -ENOMEM the code assumes that the guest memory access
> succeeded even though it failed.
> This can cause guest data or state corruption.
> 
> If __guestaddr_to_user() returns -ENOMEM the meaning is that a valid user
> space mapping exists, but there was not enough memory available when trying
> to build the guest mapping. In other words an out-of-memory situation
> occured.
> For normal user space accesses an out-of-memory situation causes the page
> fault handler to map -ENOMEM to -EFAULT (see fixup code in do_no_context()).
> We need to do exactly the same for the kvm gaccess functions.
> 
> So __guestaddr_to_user() should just map all error codes to -EFAULT.
> 
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>



> ---
>  arch/s390/kvm/gaccess.h | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/s390/kvm/gaccess.h b/arch/s390/kvm/gaccess.h
> index 4703f12..84d01dd 100644
> --- a/arch/s390/kvm/gaccess.h
> +++ b/arch/s390/kvm/gaccess.h
> @@ -22,13 +22,16 @@ static inline void __user *__guestaddr_to_user(struct kvm_vcpu *vcpu,
>  					       unsigned long guestaddr)
>  {
>  	unsigned long prefix  = vcpu->arch.sie_block->prefix;
> +	unsigned long uaddress;
> 
>  	if (guestaddr < 2 * PAGE_SIZE)
>  		guestaddr += prefix;
>  	else if ((guestaddr >= prefix) && (guestaddr < prefix + 2 * PAGE_SIZE))
>  		guestaddr -= prefix;
> -
> -	return (void __user *) gmap_fault(guestaddr, vcpu->arch.gmap);
> +	uaddress = gmap_fault(guestaddr, vcpu->arch.gmap);
> +	if (IS_ERR_VALUE(uaddress))
> +		uaddress = -EFAULT;
> +	return (void __user *)uaddress;
>  }
> 
>  static inline int get_guest_u64(struct kvm_vcpu *vcpu, unsigned long guestaddr,
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/5] KVM: s390: Add support for machine checks.
  2012-12-19 10:20     ` Christian Borntraeger
@ 2012-12-19 13:07       ` Heiko Carstens
  0 siblings, 0 replies; 24+ messages in thread
From: Heiko Carstens @ 2012-12-19 13:07 UTC (permalink / raw)
  To: Christian Borntraeger
  Cc: Cornelia Huck, Marcelo Tosatti, Gleb Natapov, KVM, linux-s390,
	Avi Kivity, Carsten Otte, Alexander Graf, Martin Schwidefsky,
	Sebastian Ott

On Wed, Dec 19, 2012 at 11:20:18AM +0100, Christian Borntraeger wrote:
> On 19/12/12 10:44, Heiko Carstens wrote:
> > On Fri, Dec 07, 2012 at 01:30:22PM +0100, Cornelia Huck wrote:
> >> +		rc = put_guest_u64(vcpu, __LC_MCCK_CODE, inti->mchk.mcic);
> >> +		if (rc == -EFAULT)
> >> +			exception = 1;
> >> +
> >> +		rc = copy_to_guest(vcpu, __LC_MCK_OLD_PSW,
> >> +				   &vcpu->arch.sie_block->gpsw, sizeof(psw_t));
> >> +		if (rc == -EFAULT)
> >> +			exception = 1;
> > 
> > Please don't add more explicit -EFAULT checks on guest access paths. Just
> > make this like normal user space accesses. That is return code != 0 means
> > an error occured:
> > 
> > 	rc = put_guest_u64(vcpu, __LC_MCCK_CODE, inti->mchk.mcic);
> > 	if (rc)
> > 		exception = 1;
> > 
> > In fact, with the current kvm gaccess code it's even broken, since on error
> > the guest access functions may return also -ENOMEM instead of -EFAULT, which
> > would be ignored by your code.
> > I addressed that with a patch when trying to clean up the guest access
> > functions. Maybe the patch below should be merged anyway. Christian?
> 
> The whole guest memory access of KVM needs to be reworked to work properly
> in those corner cases. I have this on my todo list as one of things for next
> year with lots of open questions that I dont want to answer before xmas.
> what about in kernel intercepts? (shall we then return EFAULT for the KVM_RUN
> ioctl, shall we kill the guest?.....)
> 
> We actually need to test the address for validity via the memslots (and not
> via return value of copy_from/to_user) all across the s390 code.
> 
> I really want to avoid mixing this effort with the virtio-ccw patches.
> So my proposal is to apply your patch below and keep Conny's patch as is.
> Ok?

Sure. :)

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 3/5] KVM: s390: In-kernel handling of I/O instructions.
  2012-10-31 16:24 [RFC PATCH v3 0/5] s390: Host support for channel I/O Cornelia Huck
@ 2012-10-31 16:24 ` Cornelia Huck
  0 siblings, 0 replies; 24+ messages in thread
From: Cornelia Huck @ 2012-10-31 16:24 UTC (permalink / raw)
  To: KVM, linux-s390, qemu-devel
  Cc: Avi Kivity, Marcelo Tosatti, Anthony Liguori,
	Christian Borntraeger, Carsten Otte, Alexander Graf,
	Heiko Carstens, Martin Schwidefsky, Sebastian Ott

Explicitely catch all channel I/O related instructions intercepts
in the kernel and set condition code 3 for them.

This paws the way for properly handling these instructions later
on.

Note: This is not architecture compliant (the previous code wasn't
either) since setting cc 3 is not the correct thing to do for some
of these instructions. For Linux guests, however, it still has the
intended effect of stopping css probing.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 arch/s390/kvm/intercept.c | 19 +++++++++++++---
 arch/s390/kvm/kvm-s390.h  |  1 +
 arch/s390/kvm/priv.c      | 56 +++++++++++++++++++++++++++++++++--------------
 3 files changed, 56 insertions(+), 20 deletions(-)

diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
index ec1177f..754dc9e 100644
--- a/arch/s390/kvm/intercept.c
+++ b/arch/s390/kvm/intercept.c
@@ -33,8 +33,6 @@ static int handle_lctlg(struct kvm_vcpu *vcpu)
 	int reg, rc;
 
 	vcpu->stat.instruction_lctlg++;
-	if ((vcpu->arch.sie_block->ipb & 0xff) != 0x2f)
-		return -EOPNOTSUPP;
 
 	useraddr = disp2;
 	if (base2)
@@ -104,6 +102,21 @@ static int handle_lctl(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
+static intercept_handler_t eb_handlers[256] = {
+	[0x2f] = handle_lctlg,
+	[0x8a] = kvm_s390_handle_priv_eb,
+};
+
+static int handle_eb(struct kvm_vcpu *vcpu)
+{
+	intercept_handler_t handler;
+
+	handler = eb_handlers[vcpu->arch.sie_block->ipb & 0xff];
+	if (handler)
+		return handler(vcpu);
+	return -EOPNOTSUPP;
+}
+
 static intercept_handler_t instruction_handlers[256] = {
 	[0x01] = kvm_s390_handle_01,
 	[0x82] = kvm_s390_handle_lpsw,
@@ -113,7 +126,7 @@ static intercept_handler_t instruction_handlers[256] = {
 	[0xb7] = handle_lctl,
 	[0xb9] = kvm_s390_handle_b9,
 	[0xe5] = kvm_s390_handle_e5,
-	[0xeb] = handle_lctlg,
+	[0xeb] = handle_eb,
 };
 
 static int handle_noop(struct kvm_vcpu *vcpu)
diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
index b1e1cb6..7f50229 100644
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -83,6 +83,7 @@ int kvm_s390_handle_e5(struct kvm_vcpu *vcpu);
 int kvm_s390_handle_01(struct kvm_vcpu *vcpu);
 int kvm_s390_handle_b9(struct kvm_vcpu *vcpu);
 int kvm_s390_handle_lpsw(struct kvm_vcpu *vcpu);
+int kvm_s390_handle_priv_eb(struct kvm_vcpu *vcpu);
 
 /* implemented in sigp.c */
 int kvm_s390_handle_sigp(struct kvm_vcpu *vcpu);
diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
index bf13ce9..2aba96b 100644
--- a/arch/s390/kvm/priv.c
+++ b/arch/s390/kvm/priv.c
@@ -135,20 +135,9 @@ static int handle_skey(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
-static int handle_stsch(struct kvm_vcpu *vcpu)
+static int handle_io_inst(struct kvm_vcpu *vcpu)
 {
-	vcpu->stat.instruction_stsch++;
-	VCPU_EVENT(vcpu, 4, "%s", "store subchannel - CC3");
-	/* condition code 3 */
-	vcpu->arch.sie_block->gpsw.mask &= ~(3ul << 44);
-	vcpu->arch.sie_block->gpsw.mask |= (3 & 3ul) << 44;
-	return 0;
-}
-
-static int handle_chsc(struct kvm_vcpu *vcpu)
-{
-	vcpu->stat.instruction_chsc++;
-	VCPU_EVENT(vcpu, 4, "%s", "channel subsystem call - CC3");
+	VCPU_EVENT(vcpu, 4, "%s", "I/O instruction");
 	/* condition code 3 */
 	vcpu->arch.sie_block->gpsw.mask &= ~(3ul << 44);
 	vcpu->arch.sie_block->gpsw.mask |= (3 & 3ul) << 44;
@@ -392,7 +381,7 @@ out_fail:
 	return 0;
 }
 
-static intercept_handler_t priv_handlers[256] = {
+static intercept_handler_t b2_handlers[256] = {
 	[0x02] = handle_stidp,
 	[0x10] = handle_set_prefix,
 	[0x11] = handle_store_prefix,
@@ -400,8 +389,22 @@ static intercept_handler_t priv_handlers[256] = {
 	[0x29] = handle_skey,
 	[0x2a] = handle_skey,
 	[0x2b] = handle_skey,
-	[0x34] = handle_stsch,
-	[0x5f] = handle_chsc,
+	[0x30] = handle_io_inst,
+	[0x31] = handle_io_inst,
+	[0x32] = handle_io_inst,
+	[0x33] = handle_io_inst,
+	[0x34] = handle_io_inst,
+	[0x35] = handle_io_inst,
+	[0x36] = handle_io_inst,
+	[0x37] = handle_io_inst,
+	[0x38] = handle_io_inst,
+	[0x39] = handle_io_inst,
+	[0x3a] = handle_io_inst,
+	[0x3b] = handle_io_inst,
+	[0x3c] = handle_io_inst,
+	[0x5f] = handle_io_inst,
+	[0x74] = handle_io_inst,
+	[0x76] = handle_io_inst,
 	[0x7d] = handle_stsi,
 	[0xb1] = handle_stfl,
 	[0xb2] = handle_lpswe,
@@ -418,7 +421,7 @@ int kvm_s390_handle_b2(struct kvm_vcpu *vcpu)
 	 * state bit and (a) handle the instruction or (b) send a code 2
 	 * program check.
 	 * Anything else goes to userspace.*/
-	handler = priv_handlers[vcpu->arch.sie_block->ipa & 0x00ff];
+	handler = b2_handlers[vcpu->arch.sie_block->ipa & 0x00ff];
 	if (handler) {
 		if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
 			return kvm_s390_inject_program_int(vcpu,
@@ -447,6 +450,7 @@ static int handle_epsw(struct kvm_vcpu *vcpu)
 
 static intercept_handler_t b9_handlers[256] = {
 	[0x8d] = handle_epsw,
+	[0x9c] = handle_io_inst,
 };
 
 int kvm_s390_handle_b9(struct kvm_vcpu *vcpu)
@@ -466,6 +470,24 @@ int kvm_s390_handle_b9(struct kvm_vcpu *vcpu)
 	return -EOPNOTSUPP;
 }
 
+static intercept_handler_t eb_handlers[256] = {
+	[0x8a] = handle_io_inst,
+};
+
+int kvm_s390_handle_priv_eb(struct kvm_vcpu *vcpu)
+{
+	intercept_handler_t handler;
+
+	/* All eb instructions that end up here are privileged. */
+	if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
+		return kvm_s390_inject_program_int(vcpu,
+						   PGM_PRIVILEGED_OPERATION);
+	handler = eb_handlers[vcpu->arch.sie_block->ipb & 0xff];
+	if (handler)
+		return handler(vcpu);
+	return -EOPNOTSUPP;
+}
+
 static int handle_tprot(struct kvm_vcpu *vcpu)
 {
 	int base1 = (vcpu->arch.sie_block->ipb & 0xf0000000) >> 28;
-- 
1.7.12.4

^ permalink raw reply related	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2012-12-19 13:07 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-12-07 12:30 [PATCH v4 0/5] s390: Host support for channel I/O Cornelia Huck
2012-12-07 12:30 ` [PATCH 1/5] KVM: s390: Support for I/O interrupts Cornelia Huck
2012-12-10  7:33   ` Alexander Graf
2012-12-10 10:09     ` Cornelia Huck
2012-12-11 10:22       ` Alexander Graf
2012-12-11 12:46         ` Cornelia Huck
2012-12-12  0:36           ` Alexander Graf
2012-12-07 12:30 ` [PATCH 2/5] KVM: s390: Add support for machine checks Cornelia Huck
2012-12-10  7:51   ` Alexander Graf
2012-12-10 10:12     ` Cornelia Huck
2012-12-19  9:44   ` Heiko Carstens
2012-12-19  9:44     ` Heiko Carstens
2012-12-19 10:20     ` Christian Borntraeger
2012-12-19 13:07       ` Heiko Carstens
2012-12-07 12:30 ` [PATCH 3/5] KVM: s390: In-kernel handling of I/O instructions Cornelia Huck
2012-12-10  7:53   ` Alexander Graf
2012-12-10 10:14     ` Cornelia Huck
2012-12-07 12:30 ` [PATCH 4/5] KVM: s390: Base infrastructure for enabling capabilities Cornelia Huck
2012-12-10  7:54   ` Alexander Graf
2012-12-10 10:16     ` Cornelia Huck
2012-12-11 10:24       ` Alexander Graf
2012-12-07 12:30 ` [PATCH 5/5] KVM: s390: Add support for channel I/O instructions Cornelia Huck
2012-12-10  8:01   ` Alexander Graf
  -- strict thread matches above, loose matches on Subject: below --
2012-10-31 16:24 [RFC PATCH v3 0/5] s390: Host support for channel I/O Cornelia Huck
2012-10-31 16:24 ` [PATCH 3/5] KVM: s390: In-kernel handling of I/O instructions Cornelia Huck

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.