Re: [PATCH] xen/arm: Init traps very early

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

On 05/28/2014 03:04 PM, Andrew Cooper wrote:
> On 28/05/14 14:58, Julien Grall wrote:
>> Hi Andrew,
>>
>> On 05/28/2014 02:49 PM, Andrew Cooper wrote:
>>> On 28/05/14 14:33, Julien Grall wrote:
>>>> The function init_traps setups the handler taken when Xen hits a BUG_ON/ASSERT.
>>>>
>>>> If an error happen before init_traps is called, we loose the backtrace.
>>>>
>>>> As the function doesn't require any specific setup, we can call it just
>>>> after Xen has jumped in C code.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>>> ---
>>>>  xen/arch/arm/setup.c |    3 +--
>>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>>>> index b9ce7a9..5bf8686 100644
>>>> --- a/xen/arch/arm/setup.c
>>>> +++ b/xen/arch/arm/setup.c
>>>> @@ -666,6 +666,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>>>>      struct domain *dom0;
>>>>  
>>>>      setup_cache();
>>>> +    init_traps();
>>> Having recently redone this in x86, it must be after
>>> set_processor_id(0), set_current() for debug sanity, and after
>>> percpu_init_areas() for future proofing.
>> Even though it doesn't harm to call before (the stack will be
>> corrupted), I agree to call init_traps after set_processor_id(0), and
>> therefore percpu_init_areas.
>>
>> But we don't need to have set_current correctly set up. The trap entries
>> won't save anything about the guest if the exception are taken from HYP
>> mode.
>>
> 
> set_current((struct vcpu *)0xfffff000); /* debug sanity */
> 
> This is designed to force a pagefault if a trap handler tries to use
> current before current is a valid vcpu.  It is a debugging measure
> rather than a functional one, and does pick up issues.

At this stage if we hit a trap it's mostly for a data abort, alignment
issue, undefined instruction. *none* of them if using current.

I prefer to move init_traps earlier and get useful call stack 95% of the
time, rather than loosing it for the sack of debug sanity.

It's very painful to debug when the ASSERT is in the header and can be
called on multiple place.

Regards,

-- 
Julien Grall