From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@linaro.org>
Subject: Re: [PATCH v2 4/9] xen: arm: correctly handle vtimer
	traps from userspace
Date: Tue, 10 Feb 2015 14:41:10 +0800
Message-ID: <54D9A806.7030201@linaro.org>
References: <1423542956.5851.9.camel@citrix.com>
	<1423543523-8010-4-git-send-email-ian.campbell@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1423543523-8010-4-git-send-email-ian.campbell@citrix.com>
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: Ian Campbell <ian.campbell@citrix.com>, xen-devel@lists.xen.org
Cc: tim@xen.org, stefano.stabellini@eu.citrix.com
List-Id: xen-devel@lists.xenproject.org

Hi Ian,

On 10/02/2015 12:45, Ian Campbell wrote:
> Previously 32-bit userspace on 32-bit kernel and 64-bit userspace on 64-bit
> kernel could access these registers irrespective of whether the kernel had
> configured them to be allowed to. To fix this:
>
>   - Userspace access to CNTP_CTL_EL0 and CNTP_TVAL_EL0 should be gated on
>     CNTKCTL_EL1.EL0PTEN.

Should not we take care of CNTP_CVAL_EL0 too? It seems that we don't 
even trap it for now...

[..]

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

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.

[..]

> @@ -238,7 +250,7 @@ static int vtimer_emulate_cp64(struct cpu_user_regs *regs, union hsr hsr)
>       switch ( hsr.bits & HSR_CP64_REGS_MASK )
>       {
>       case HSR_CPREG64(CNTPCT):
> -        if (!vtimer_cntpct(regs, &x, cp64.read))
> +        if ( !vtimer_cntpct(regs, &x, cp64.read) )

I would mention the coding style change in the commit message.

Regards,

-- 
Julien Grall