From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Campbell <ian.campbell@citrix.com>
Subject: Re: [PATCH v2 4/9] xen: arm: correctly handle vtimer
 traps from userspace
Date: Thu, 19 Feb 2015 15:13:21 +0000
Message-ID: <1424358801.30924.85.camel@citrix.com>
References: <1423542956.5851.9.camel@citrix.com>
	<1423543523-8010-4-git-send-email-ian.campbell@citrix.com>
	<54D9A806.7030201@linaro.org> <1424347848.30924.53.camel@citrix.com>
	<54E5F658.4020904@linaro.org>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <54E5F658.4020904@linaro.org>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Julien Grall <julien.grall@linaro.org>
Cc: stefano.stabellini@eu.citrix.com, tim@xen.org, xen-devel@lists.xen.org
List-Id: xen-devel@lists.xenproject.org

On Thu, 2015-02-19 at 14:42 +0000, Julien Grall wrote:
> >>
> >> [..]
> >>
> >>> @@ -2062,8 +2053,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
> >>>           do_cp15_32(regs, hsr);
> >>>           break;
> >>>       case HSR_EC_CP15_64:
> >>> -        if ( !is_32bit_domain(current->domain) )
> >>> -            goto bad_trap;
> >>> +        BUG_ON(!psr_mode_is_32bit(regs->cpsr));
> >>
> >> You should mention the change from if ( .... ) goto bad_trap to BUG_ON( 
> >> ... ) in the commit message.
> > 
> > OK.
> > 
> >> Although, I think the debug message in bad_trap is useful to keep. It 
> >> may be handy to have the HSR and the guest stack trace printed if Xen 
> >> hit the condition.
> > 
> > Doesn't BUG_ON include all that? It should really.
> 
> Not really BUG_ON will jump into the exception mode and therefore print
> the HSR of the exception (breakpoint for ARM64 and undef for ARM32).

Hrm, good point.

Rather than reintroducing the goto idiom what about some form of
noreturn panic helper for checking for sane h/w state (since these
failures are really of the "buggy hardware" variety) e.g.
ASSERT_GUEST_STATE(is_32bit_domain(...)) which would dump the guest
state and then panic?

Since these scenarios really indicate some sort of h/w fault I'm
considering making them debug=y only, as indicated by the use of
ASSERT_FOO.

Ian.