Re: [PATCH v2 2/7] s390/kvm: Add support for machine checks.
On Tue, Sep 04, 2012 at 05:13:25PM +0200, Cornelia Huck wrote:
Just some quick comments:
[...]
int kvm_s390_inject_program_int(struct kvm_vcpu *vcpu, u16 code)
{
struct kvm_s390_local_interrupt *li = vcpu-arch.local_int;
@@ -648,6 +747,12 @@ 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.mcic = s390int-parm64;
+ break;
The kvm_s390_interrupt struct seems to be inappropriate to pass machine check
data around.
E.g. if you want to inject an uncorrectable storage error, because the host
failed to swap in a page, you must also pass a failing storage address which
doesn't fit into this structure.
Just something you should consider. ;)
+static int handle_lpswe(struct kvm_vcpu *vcpu)
+{
+ int base2 = vcpu-arch.sie_block-ipb 28;
+ int disp2 = ((vcpu-arch.sie_block-ipb 0x0fff) 16);
Sooner or later we need helper functions which extract the significant parts
of an instruction.
Maybay something like insn_[type]_get_base2(...) or simply structures like
struct insn_[type], which allow to easily access parts of an instruction.
+ u64 addr;
+ u64 new_psw[2];
psw_t?
+
+ 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))) {
I assume that should be sizeof(new_psw). Did that ever work?!
+ if ((vcpu-arch.sie_block-gpsw.mask 0xb80800fe7fff) ||
+ (((vcpu-arch.sie_block-gpsw.mask 0x00011000) ==
+ 0x1000)
+ (vcpu-arch.sie_block-gpsw.addr 0x8000)) ||
+ (!(vcpu-arch.sie_block-gpsw.mask 0x00018000)
+ (vcpu-arch.sie_block-gpsw.addr 0xfff0)) ||
+ ((vcpu-arch.sie_block-gpsw.mask 0x00011000) ==
+ 0x0001)) {
This is not very readable...
Please make use of the PSW defines in ptrace.h and add new ones if needed.
Also please make use of (move) the PSW32 defines in compat.h.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/7] s390/kvm: Add support for machine checks.
On Wed, 5 Sep 2012 09:22:32 +0200
Heiko Carstens heiko.carst...@de.ibm.com wrote:
On Tue, Sep 04, 2012 at 05:13:25PM +0200, Cornelia Huck wrote:
Just some quick comments:
[...]
int kvm_s390_inject_program_int(struct kvm_vcpu *vcpu, u16 code)
{
struct kvm_s390_local_interrupt *li = vcpu-arch.local_int;
@@ -648,6 +747,12 @@ 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.mcic = s390int-parm64;
+ break;
The kvm_s390_interrupt struct seems to be inappropriate to pass machine check
data around.
E.g. if you want to inject an uncorrectable storage error, because the host
failed to swap in a page, you must also pass a failing storage address which
doesn't fit into this structure.
Just something you should consider. ;)
Sigh, it seems we might want a KVM_S390_INTERRUPT2 taking a larger and
more complex structure later on...
kvm_s390_interrupt should be fine for our current needs, though,
expecially as machine checks are currently only injected in-kernel.
+static int handle_lpswe(struct kvm_vcpu *vcpu)
+{
+ int base2 = vcpu-arch.sie_block-ipb 28;
+ int disp2 = ((vcpu-arch.sie_block-ipb 0x0fff) 16);
Sooner or later we need helper functions which extract the significant parts
of an instruction.
Maybay something like insn_[type]_get_base2(...) or simply structures like
struct insn_[type], which allow to easily access parts of an instruction.
Agree on helper functions, not sure about the use of structures for
that. Let's see how it looks coded up.
+ u64 addr;
+ u64 new_psw[2];
psw_t?
+
+ 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))) {
I assume that should be sizeof(new_psw). Did that ever work?!
No, because the code was never hit since the code for ICTL was
incorrect... The guest kernel just does a good job at keeping machine
checks open nearly all of the time :)
+ if ((vcpu-arch.sie_block-gpsw.mask 0xb80800fe7fff) ||
+ (((vcpu-arch.sie_block-gpsw.mask 0x00011000) ==
+ 0x1000)
+(vcpu-arch.sie_block-gpsw.addr 0x8000)) ||
+ (!(vcpu-arch.sie_block-gpsw.mask 0x00018000)
+(vcpu-arch.sie_block-gpsw.addr 0xfff0)) ||
+ ((vcpu-arch.sie_block-gpsw.mask 0x00011000) ==
+0x0001)) {
This is not very readable...
It's straight from the PoP's discussion of invalid bits.
Please make use of the PSW defines in ptrace.h and add new ones if needed.
Also please make use of (move) the PSW32 defines in compat.h.
I'll check these.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/7] s390/kvm: Add support for machine checks.
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.
Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
arch/s390/include/asm/kvm_host.h | 8 +++
arch/s390/kvm/intercept.c| 2 +
arch/s390/kvm/interrupt.c| 111
arch/s390/kvm/kvm-s390.h | 3 +
arch/s390/kvm/priv.c | 133 +++
arch/s390/kvm/trace-s390.h | 6 +-
include/linux/kvm.h | 1 +
7 files changed, 261 insertions(+), 3 deletions(-)
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index e47f697..556774d 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 {
__u8reserved40[4]; /* 0x0040 */
#define LCTL_CR0 0x8000
#define LCTL_CR6 0x0200
+#define LCTL_CR14 0x0002
__u16 lctl; /* 0x0044 */
__s16 icpua; /* 0x0046 */
+#define ICTL_LPSW 0x0200
__u32 ictl; /* 0x0048 */
__u32 eca;/* 0x004c */
__u8icptcode; /* 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 1dccfe7..edc065f 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 = 0x;
+ 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,
