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

[Julien Grall <julien.grall@linaro.org>]

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.

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.

Regards,

-- 
Julien Grall