From mboxrd@z Thu Jan  1 00:00:00 1970
From: Heiko Carstens <heiko.carstens@de.ibm.com>
Subject: Re: [PATCH v2 2/7] s390/kvm: Add support for machine checks.
Date: Wed, 5 Sep 2012 09:22:32 +0200
Message-ID: <20120905072232.GA5852@osiris.de.ibm.com>
References: <1346771610-52423-1-git-send-email-cornelia.huck@de.ibm.com>
 <1346771610-52423-3-git-send-email-cornelia.huck@de.ibm.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: KVM <kvm@vger.kernel.org>, linux-s390 <linux-s390@vger.kernel.org>,
	qemu-devel <qemu-devel@nongnu.org>, Avi Kivity <avi@redhat.com>,
	Marcelo Tosatti <mtosatti@redhat.com>,
	Anthony Liguori <aliguori@us.ibm.com>,
	Rusty Russell <rusty@rustcorp.com.au>,
	Christian Borntraeger <borntraeger@de.ibm.com>,
	Carsten Otte <cotte@de.ibm.com>,
	Alexander Graf <agraf@suse.de>,
	Martin Schwidefsky <schwidefsky@de.ibm.com>,
	Sebastian Ott <sebott@linux.vnet.ibm.com>
To: Cornelia Huck <cornelia.huck@de.ibm.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from e06smtp13.uk.ibm.com ([195.75.94.109]:59571 "EHLO
	e06smtp13.uk.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1750826Ab2IEHW2 (ORCPT <rfc822;kvm@vger.kernel.org>);
	Wed, 5 Sep 2012 03:22:28 -0400
Received: from /spool/local
	by e06smtp13.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted
	for <kvm@vger.kernel.org> from <heiko.carstens@de.ibm.com>;
	Wed, 5 Sep 2012 08:22:26 +0100
Content-Disposition: inline
In-Reply-To: <1346771610-52423-3-git-send-email-cornelia.huck@de.ibm.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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.