[PATCH RFC] fix sun6i crash on setup_pagetables

[PATCH RFC] fix sun6i crash on setup_pagetables

[Bamvor Jian Zhang]

this patch move set_processor_id in front of setup_pagetables
in order to fix crash when calling this_cpu in that functions.
the HPTIDR is unknown value according to ARM ARM 7AR(DDI0406C),
B4-1598. we should use it before using it.

but i do not know why other board like arndale board. should
bootloader write this register?

Signed-off-by: Bamvor Jian Zhang <bjzhang@suse.com>
---
 xen/arch/arm/setup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 2f9fdc8..3bf1b40 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -437,6 +437,7 @@ void __init start_xen(unsigned long boot_phys_offset,
     cpus = smp_get_max_cpus();
     cmdline_parse(device_tree_bootargs(device_tree_flattened));
 
+    set_processor_id(0); /* needed early, for this_cpu() */
     setup_pagetables(boot_phys_offset, get_xen_paddr());
     setup_mm(fdt_paddr, fdt_size);
 
@@ -459,7 +460,6 @@ void __init start_xen(unsigned long boot_phys_offset,
     make_cpus_ready(cpus, boot_phys_offset);
 
     percpu_init_areas();
-    set_processor_id(0); /* needed early, for smp_processor_id() */
     set_current((struct vcpu *)0xfffff000); /* debug sanity */
     idle_vcpu[0] = current;
 
-- 
1.8.1.4

Re: [PATCH RFC] fix sun6i crash on setup_pagetables

[Julien Grall]

On 07/24/2013 01:14 PM, Bamvor Jian Zhang wrote:
> this patch move set_processor_id in front of setup_pagetables
> in order to fix crash when calling this_cpu in that functions.
> the HPTIDR is unknown value according to ARM ARM 7AR(DDI0406C),
> B4-1598. we should use it before using it.
> 
> but i do not know why other board like arndale board. should
> bootloader write this register?

On the Arndale, by default the value of HPTIDR is equal to 0. So it
works without any issue.

During the afternoon, I have noticed this error and send a similar
patch few minutes ago. I also moved percpu_init_areas earlier.

For the next patches, could you add the appropriate people? It will
avoid to loose your patch with the other mail of the mailing list :).

> 
> Signed-off-by: Bamvor Jian Zhang <bjzhang@suse.com>
> ---
>  xen/arch/arm/setup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 2f9fdc8..3bf1b40 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -437,6 +437,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>      cpus = smp_get_max_cpus();
>      cmdline_parse(device_tree_bootargs(device_tree_flattened));
>  
> +    set_processor_id(0); /* needed early, for this_cpu() */
>      setup_pagetables(boot_phys_offset, get_xen_paddr());
>      setup_mm(fdt_paddr, fdt_size);
>  
> @@ -459,7 +460,6 @@ void __init start_xen(unsigned long boot_phys_offset,
>      make_cpus_ready(cpus, boot_phys_offset);
>  
>      percpu_init_areas();

I think you can also move percpu_init_areas earlier.

> -    set_processor_id(0); /* needed early, for smp_processor_id() */
>      set_current((struct vcpu *)0xfffff000); /* debug sanity */
>      idle_vcpu[0] = current;
>  
> 

Re: [PATCH RFC] fix sun6i crash on setup_pagetables

[Ian Campbell]

On Wed, 2013-07-24 at 19:33 +0100, Julien Grall wrote:
> On 07/24/2013 01:14 PM, Bamvor Jian Zhang wrote:
> > this patch move set_processor_id in front of setup_pagetables
> > in order to fix crash when calling this_cpu in that functions.
> > the HPTIDR is unknown value according to ARM ARM 7AR(DDI0406C),
> > B4-1598. we should use it before using it.
> > 
> > but i do not know why other board like arndale board. should
> > bootloader write this register?
> 
> On the Arndale, by default the value of HPTIDR is equal to 0. So it
> works without any issue.

Right, but strictly the register is undefined reset on reset.

The bootloader cannot initialise it because the meaning is entirely OS
specific.

> During the afternoon, I have noticed this error and send a similar
> patch few minutes ago. I also moved percpu_init_areas earlier.

Right, I think this makes sense.

I suppose I should just take your (Julien's) version of this patch?

> For the next patches, could you add the appropriate people? It will
> avoid to loose your patch with the other mail of the mailing list :).

Yes please!

> 
> > 
> > Signed-off-by: Bamvor Jian Zhang <bjzhang@suse.com>
> > ---
> >  xen/arch/arm/setup.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index 2f9fdc8..3bf1b40 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -437,6 +437,7 @@ void __init start_xen(unsigned long boot_phys_offset,
> >      cpus = smp_get_max_cpus();
> >      cmdline_parse(device_tree_bootargs(device_tree_flattened));
> >  
> > +    set_processor_id(0); /* needed early, for this_cpu() */
> >      setup_pagetables(boot_phys_offset, get_xen_paddr());
> >      setup_mm(fdt_paddr, fdt_size);
> >  
> > @@ -459,7 +460,6 @@ void __init start_xen(unsigned long boot_phys_offset,
> >      make_cpus_ready(cpus, boot_phys_offset);
> >  
> >      percpu_init_areas();
> 
> I think you can also move percpu_init_areas earlier.
> 
> > -    set_processor_id(0); /* needed early, for smp_processor_id() */
> >      set_current((struct vcpu *)0xfffff000); /* debug sanity */
> >      idle_vcpu[0] = current;
> >  
> > 
> 

Re: [PATCH RFC] fix sun6i crash on setup_pagetables

[Bamvor Jian Zhang]

 >>>Written "Ian Campbell <ian.campbell@citrix.com>"> On Wed, 2013-07-24 at 19:33 +0100, Julien Grall wrote:
> > On 07/24/2013 01:14 PM, Bamvor Jian Zhang wrote: 
> > > this patch move set_processor_id in front of setup_pagetables 
> > > in order to fix crash when calling this_cpu in that functions. 
> > > the HPTIDR is unknown value according to ARM ARM 7AR(DDI0406C), 
> > > B4-1598. we should use it before using it. 
> > >  
> > > but i do not know why other board like arndale board. should 
> > > bootloader write this register? 
> >  
> > On the Arndale, by default the value of HPTIDR is equal to 0. So it 
> > works without any issue. 
>  
> Right, but strictly the register is undefined reset on reset. 
>  
> The bootloader cannot initialise it because the meaning is entirely OS 
> specific. 
>  
> > During the afternoon, I have noticed this error and send a similar 
> > patch few minutes ago. I also moved percpu_init_areas earlier. 
>  
> Right, I think this makes sense. 
>  
> I suppose I should just take your (Julien's) version of this patch? 
agree.
> > For the next patches, could you add the appropriate people? It will 
> > avoid to loose your patch with the other mail of the mailing list :). 
>  
> Yes please! 
i will, thanks. 
> >  
> > >  
> > > Signed-off-by: Bamvor Jian Zhang <bjzhang@suse.com> 
> > > --- 
> > >  xen/arch/arm/setup.c | 2 +- 
> > >  1 file changed, 1 insertion(+), 1 deletion(-) 
> > >  
> > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c 
> > > index 2f9fdc8..3bf1b40 100644 
> > > --- a/xen/arch/arm/setup.c 
> > > +++ b/xen/arch/arm/setup.c 
> > > @@ -437,6 +437,7 @@ void __init start_xen(unsigned long boot_phys_offset, 
> > >      cpus = smp_get_max_cpus(); 
> > >      cmdline_parse(device_tree_bootargs(device_tree_flattened)); 
> > >   
> > > +    set_processor_id(0); /* needed early, for this_cpu() */ 
> > >      setup_pagetables(boot_phys_offset, get_xen_paddr()); 
> > >      setup_mm(fdt_paddr, fdt_size); 
> > >   
> > > @@ -459,7 +460,6 @@ void __init start_xen(unsigned long boot_phys_offset, 
> > >      make_cpus_ready(cpus, boot_phys_offset); 
> > >   
> > >      percpu_init_areas(); 
> >  
> > I think you can also move percpu_init_areas earlier. 
> >  
> > > -    set_processor_id(0); /* needed early, for smp_processor_id() */ 
> > >      set_current((struct vcpu *)0xfffff000); /* debug sanity */ 
> > >      idle_vcpu[0] = current; 
> > >   
> > >  
> >  
>  
>  
>  
>  

Re: [PATCH RFC] fix sun6i crash on setup_pagetables

[Julien Grall]

On 07/25/2013 04:29 AM, Ian Campbell wrote:
> On Wed, 2013-07-24 at 19:33 +0100, Julien Grall wrote:
>> On 07/24/2013 01:14 PM, Bamvor Jian Zhang wrote:
>>> this patch move set_processor_id in front of setup_pagetables
>>> in order to fix crash when calling this_cpu in that functions.
>>> the HPTIDR is unknown value according to ARM ARM 7AR(DDI0406C),
>>> B4-1598. we should use it before using it.
>>>
>>> but i do not know why other board like arndale board. should
>>> bootloader write this register?
>>
>> On the Arndale, by default the value of HPTIDR is equal to 0. So it
>> works without any issue.
> 
> Right, but strictly the register is undefined reset on reset.
> 
> The bootloader cannot initialise it because the meaning is entirely OS
> specific.
> 
>> During the afternoon, I have noticed this error and send a similar
>> patch few minutes ago. I also moved percpu_init_areas earlier.
> 
> Right, I think this makes sense.
> 
> I suppose I should just take your (Julien's) version of this patch?

If you take my patch, could you slighty modify the commit message to
specify that's in an issue on sun6i?

>> For the next patches, could you add the appropriate people? It will
>> avoid to loose your patch with the other mail of the mailing list :).
> 
> Yes please!
> 
>>
>>>
>>> Signed-off-by: Bamvor Jian Zhang <bjzhang@suse.com>
>>> ---
>>>  xen/arch/arm/setup.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>>> index 2f9fdc8..3bf1b40 100644
>>> --- a/xen/arch/arm/setup.c
>>> +++ b/xen/arch/arm/setup.c
>>> @@ -437,6 +437,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>>>      cpus = smp_get_max_cpus();
>>>      cmdline_parse(device_tree_bootargs(device_tree_flattened));
>>>  
>>> +    set_processor_id(0); /* needed early, for this_cpu() */
>>>      setup_pagetables(boot_phys_offset, get_xen_paddr());
>>>      setup_mm(fdt_paddr, fdt_size);
>>>  
>>> @@ -459,7 +460,6 @@ void __init start_xen(unsigned long boot_phys_offset,
>>>      make_cpus_ready(cpus, boot_phys_offset);
>>>  
>>>      percpu_init_areas();
>>
>> I think you can also move percpu_init_areas earlier.
>>
>>> -    set_processor_id(0); /* needed early, for smp_processor_id() */
>>>      set_current((struct vcpu *)0xfffff000); /* debug sanity */
>>>      idle_vcpu[0] = current;
>>>  
>>>
>>
> 
>