Re: [PATCH v2 5/9] xen: arm: Handle CP15 register traps from userspace

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

On Thu, 2015-02-19 at 14:53 +0000, Julien Grall wrote:
> On 19/02/15 12:15, Ian Campbell wrote:
> > On Tue, 2015-02-17 at 15:07 +0000, Julien Grall wrote:
> >> Hi Ian,
> >>
> >> On 10/02/15 04:45, Ian Campbell wrote:
> >>>      default:
> >>> @@ -2047,8 +2070,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
> >>>          advance_pc(regs, hsr);
> >>>          break;
> >>>      case HSR_EC_CP15_32:
> >>> -        if ( !is_32bit_domain(current->domain) )
> >>> -            goto bad_trap;
> >>> +        BUG_ON(!psr_mode_is_32bit(regs->cpsr));
> >>
> >> It's a bit strange that on the previous patch (#5) you fixed CP15_64 but
> >> not CP15_32. If I'm not mistaken you need both in-order to make the
> >> things correctly work.
> > 
> > The previous patch (I think you meant #4) made the CP15_64 handlers
> > correct (since as it happens they are all vtimer related), while it is
> > only at this point that the cp15_32 registers are all finally updated.
> 
> >> So, I would invert the 2 patches.
> > 
> > I'm not sure the ordering matters, none of this will be active until the
> > final patch removes the top-level check. IOW I think the patches make as
> > much sense in either order and this is the one it has ended up in.
> 
> It doesn't make sense to correct the both cp15_* on different patches.

Sure it does, it breaks things down into more logical chunks by
separating the vtimer stuff into its own patch, which is a completely
reasonable way to do things (even if not the only way).

> As it would never be possible to run 32 bit userspace on 64 bit guest.
> It doesn't harm to move the two in the same patch. It would be more logic.

If I was just abut to write these patches from scratch maybe I would do
that, but they exist now and are reasonable in the form they are in. I'm
afraid I'm not going to rework them for what amounts to bikeshedding
reasons.

Ian.