On Wed, 5 Sep 2012 09:22:32 +0200
Heiko Carstens <heiko.carstens@xxxxxxxxxx> 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 & 0x0fff0000) >> 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 & 0xb80800fe7fffffff) ||
> > + (((vcpu->arch.sie_block->gpsw.mask & 0x0000000110000000) ==
> > + 0x0000000010000000) &&
> > + (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 & 0x0000000110000000) ==
> > + 0x0000000100000000)) {
>
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
