Re: [Qemu-devel] [PATCH v2 2/7] s390/kvm: Add support for machine checks.

[Cornelia Huck]

On Wed, 5 Sep 2012 09:22:32 +0200
Heiko Carstens <address@hidden> 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.