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

[Heiko Carstens <heiko.carstens@de.ibm.com>]

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 & 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.

> +	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 & 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...

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.