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

[Cornelia Huck <cornelia.huck@de.ibm.com>]

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