[08/17] xen/arm64: head: Rework and document zero_bss()
diff mbox series

Message ID 20190610193215.23704-9-julien.grall@arm.com
State New
Headers show
Series
  • xen/arm64: Rework head.S to make it more compliant with the Arm Arm
Related show

Commit Message

Julien Grall June 10, 2019, 7:32 p.m. UTC
On secondary CPUs, zero_bss() will be a NOP because BSS only need to be
zeroed once at boot. So the call in the secondary CPUs path can be
removed. It also means that x26 does not need to set and is now only
used by the boot CPU.

Lastly, document the behavior and the main registers usage within the
function.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/arm64/head.S | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Comments

Stefano Stabellini June 26, 2019, 1:01 a.m. UTC | #1
On Mon, 10 Jun 2019, Julien Grall wrote:
> On secondary CPUs, zero_bss() will be a NOP because BSS only need to be
> zeroed once at boot. So the call in the secondary CPUs path can be
> removed. It also means that x26 does not need to set and is now only
> used by the boot CPU.
> 
> Lastly, document the behavior and the main registers usage within the
> function.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/arm64/head.S | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 87fcd3be6c..6aa3148192 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -71,7 +71,7 @@
>   *  x23 - UART address
>   *  x24 -
>   *  x25 - identity map in place
> - *  x26 - skip_zero_bss
> + *  x26 - skip_zero_bss (boot cpu only)

you could remove this, see below...


>   *  x27 -
>   *  x28 -
>   *  x29 -
> @@ -313,8 +313,6 @@ GLOBAL(init_secondary)
>          sub   x20, x19, x0           /* x20 := phys-offset */
>  
>          mov   x22, #1                /* x22 := is_secondary_cpu */
> -        /* Boot CPU already zero BSS so skip it on secondary CPUs. */
> -        mov   x26, #1                /* X26 := skip_zero_bss */
>  
>          mrs   x0, mpidr_el1
>          ldr   x13, =(~MPIDR_HWID_MASK)
> @@ -337,7 +335,6 @@ GLOBAL(init_secondary)
>          PRINT(" booting -\r\n")
>  #endif
>          bl    check_cpu_mode
> -        bl    zero_bss
>          bl    cpu_init
>          bl    create_page_tables
>          bl    enable_mmu
> @@ -375,6 +372,14 @@ check_cpu_mode:
>          b fail
>  ENDPROC(check_cpu_mode)
>  
> +/*
> + * Zero BSS
> + *
> + * Inputs:
> + *   x26: Do we need to zero BSS?
> + *
> + * Clobbers x0 - x3
> + */
>  zero_bss:
>          /* Zero BSS only when requested */
>          cbnz  x26, skip_bss

In the commit message you wrote: "It also means that x26 does not need
to set and is now only used by the boot CPU." I think this statement is
correct, so you could also remove this "cbnz  x26, skip_bss" and also
the skip_bss label below.
Julien Grall June 26, 2019, 9:16 a.m. UTC | #2
Hi Stefano,

On 26/06/2019 02:01, Stefano Stabellini wrote:
> On Mon, 10 Jun 2019, Julien Grall wrote:
>> On secondary CPUs, zero_bss() will be a NOP because BSS only need to be
>> zeroed once at boot. So the call in the secondary CPUs path can be
>> removed. It also means that x26 does not need to set and is now only
>> used by the boot CPU.
>>
>> Lastly, document the behavior and the main registers usage within the
>> function.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/arm64/head.S | 13 +++++++++----
>>   1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index 87fcd3be6c..6aa3148192 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -71,7 +71,7 @@
>>    *  x23 - UART address
>>    *  x24 -
>>    *  x25 - identity map in place
>> - *  x26 - skip_zero_bss
>> + *  x26 - skip_zero_bss (boot cpu only)
> 
> you could remove this, see below...
> 
> 
>>    *  x27 -
>>    *  x28 -
>>    *  x29 -
>> @@ -313,8 +313,6 @@ GLOBAL(init_secondary)
>>           sub   x20, x19, x0           /* x20 := phys-offset */
>>   
>>           mov   x22, #1                /* x22 := is_secondary_cpu */
>> -        /* Boot CPU already zero BSS so skip it on secondary CPUs. */
>> -        mov   x26, #1                /* X26 := skip_zero_bss */
>>   
>>           mrs   x0, mpidr_el1
>>           ldr   x13, =(~MPIDR_HWID_MASK)
>> @@ -337,7 +335,6 @@ GLOBAL(init_secondary)
>>           PRINT(" booting -\r\n")
>>   #endif
>>           bl    check_cpu_mode
>> -        bl    zero_bss
>>           bl    cpu_init
>>           bl    create_page_tables
>>           bl    enable_mmu
>> @@ -375,6 +372,14 @@ check_cpu_mode:
>>           b fail
>>   ENDPROC(check_cpu_mode)
>>   
>> +/*
>> + * Zero BSS
>> + *
>> + * Inputs:
>> + *   x26: Do we need to zero BSS?
>> + *
>> + * Clobbers x0 - x3
>> + */
>>   zero_bss:
>>           /* Zero BSS only when requested */
>>           cbnz  x26, skip_bss
> 
> In the commit message you wrote: "It also means that x26 does not need
> to set and is now only used by the boot CPU." I think this statement is
> correct, so you could also remove this "cbnz  x26, skip_bss" and also
> the skip_bss label below.

I meant x26 does not need to be set on the secondary CPUs. However, we still 
need to keep it for boot CPU as we don't want to zero BSS when booting via EFI.

This is because the EFI stub will store information in BSS. Note that BSS was 
zeroed by EFI loader before hand.

I will reword the commit message.

Cheers,

Patch
diff mbox series

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 87fcd3be6c..6aa3148192 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -71,7 +71,7 @@ 
  *  x23 - UART address
  *  x24 -
  *  x25 - identity map in place
- *  x26 - skip_zero_bss
+ *  x26 - skip_zero_bss (boot cpu only)
  *  x27 -
  *  x28 -
  *  x29 -
@@ -313,8 +313,6 @@  GLOBAL(init_secondary)
         sub   x20, x19, x0           /* x20 := phys-offset */
 
         mov   x22, #1                /* x22 := is_secondary_cpu */
-        /* Boot CPU already zero BSS so skip it on secondary CPUs. */
-        mov   x26, #1                /* X26 := skip_zero_bss */
 
         mrs   x0, mpidr_el1
         ldr   x13, =(~MPIDR_HWID_MASK)
@@ -337,7 +335,6 @@  GLOBAL(init_secondary)
         PRINT(" booting -\r\n")
 #endif
         bl    check_cpu_mode
-        bl    zero_bss
         bl    cpu_init
         bl    create_page_tables
         bl    enable_mmu
@@ -375,6 +372,14 @@  check_cpu_mode:
         b fail
 ENDPROC(check_cpu_mode)
 
+/*
+ * Zero BSS
+ *
+ * Inputs:
+ *   x26: Do we need to zero BSS?
+ *
+ * Clobbers x0 - x3
+ */
 zero_bss:
         /* Zero BSS only when requested */
         cbnz  x26, skip_bss