Re: [PATCH v2 4/9] xen: arm: correctly handle vtimer traps from userspace

[Ian Campbell <ian.campbell@citrix.com>]

On Tue, 2015-02-10 at 14:41 +0800, Julien Grall wrote:
> 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...

We should, I'll prepend such a patch to the series, since it should be
backported.

As it happens Linux doesn't allow user access to this register (due to
the settings in CNTKCTL), so it traps to EL1 in h/w and we never see it.
But if an OS were to expose the phys timer to userspace for some reason
then we would trap and inject undef, which is a bit mean of us.

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

Ian.