[PATCH] xen/arm: Init traps very early

[PATCH] xen/arm: Init traps very early

[Julien Grall]

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();
 
     percpu_init_areas();
     set_processor_id(0); /* needed early, for smp_processor_id() */
@@ -709,8 +710,6 @@ void __init start_xen(unsigned long boot_phys_offset,
     set_current((struct vcpu *)0xfffff000); /* debug sanity */
     idle_vcpu[0] = current;
 
-    init_traps();
-
     setup_virt_paging();
 
     p2m_vmid_allocator_init();
-- 
1.7.10.4

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

[Naresh Bhat]

[-- Attachment #1.1: Type: text/plain, Size: 1229 bytes --]

Tested/Acked by: Naresh Bhat <naresh.bhat@linaro.org>


On 28 May 2014 19:03, Julien Grall <julien.grall@linaro.org> 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();
>
>      percpu_init_areas();
>      set_processor_id(0); /* needed early, for smp_processor_id() */
> @@ -709,8 +710,6 @@ void __init start_xen(unsigned long boot_phys_offset,
>      set_current((struct vcpu *)0xfffff000); /* debug sanity */
>      idle_vcpu[0] = current;
>
> -    init_traps();
> -
>      setup_virt_paging();
>
>      p2m_vmid_allocator_init();
> --
> 1.7.10.4
>
>

[-- Attachment #1.2: Type: text/html, Size: 1843 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Andrew Cooper]

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.

~Andrew

>  
>      percpu_init_areas();
>      set_processor_id(0); /* needed early, for smp_processor_id() */
> @@ -709,8 +710,6 @@ void __init start_xen(unsigned long boot_phys_offset,
>      set_current((struct vcpu *)0xfffff000); /* debug sanity */
>      idle_vcpu[0] = current;
>  
> -    init_traps();
> -
>      setup_virt_paging();
>  
>      p2m_vmid_allocator_init();

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

[Julien Grall]

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.

-- 
Julien Grall

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

[Andrew Cooper]

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.

~Andrew

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

[Julien Grall]

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

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

[Andrew Cooper]

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