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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 28/05/14 15:09, Julien Grall wrote:
> 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,
>

You are guarding against changes in the future, not the current behaviour.

Having correctly set up per_cpu areas, processor IDs and 'current' are
crucial before the trap handlers can be safely run in general.

In the end, set_processor_id(), set_current() and percpu_init_areas()
are trivial operations which will not fault.

~Andrew