diff mbox series

[2/7] xen/arm32: head.S: Introduce a macro to load the physical address of a symbol

Message ID 20220624091146.35716-3-julien@xen.org (mailing list archive)
State Superseded
Headers show
Series xen/arm: mm: Bunch of clean-ups | expand

Commit Message

Julien Grall June 24, 2022, 9:11 a.m. UTC 
From: Julien Grall <jgrall@amazon.com>

A lot of places in the ARM32 assembly requires to load the physical address
of a symbol. Rather than open-coding the translation, introduce a new macro
that will load the phyiscal address of a symbol.

Lastly, use the new macro to replace all the current open-coded version.

Note that most of the comments associated to the code changed have been
removed because the code is now self-explanatory.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/arm32/head.S | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

Comments

Michal Orzel June 27, 2022, 6:31 a.m. UTC | #1 
Hi Julien,

On 24.06.2022 11:11, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> A lot of places in the ARM32 assembly requires to load the physical address
> of a symbol. Rather than open-coding the translation, introduce a new macro
> that will load the phyiscal address of a symbol.
> 
> Lastly, use the new macro to replace all the current open-coded version.
> 
> Note that most of the comments associated to the code changed have been
> removed because the code is now self-explanatory.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> ---
>  xen/arch/arm/arm32/head.S | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index c837d3054cf9..77f0a619ca51 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -65,6 +65,11 @@
>          .endif
>  .endm
>  
> +.macro load_paddr rb, sym
> +        ldr   \rb, =\sym
> +        add   \rb, \rb, r10
> +.endm
> +
All the macros in this file have a comment so it'd be useful to follow this convention.
Apart from that:
Reviewed-by: Michal Orzel <michal.orzel@arm.com>

Cheers,
Michal
Julien Grall June 27, 2022, 9:52 a.m. UTC | #2 
On 27/06/2022 07:31, Michal Orzel wrote:
> Hi Julien,

Hi Michal,

> On 24.06.2022 11:11, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> A lot of places in the ARM32 assembly requires to load the physical address
>> of a symbol. Rather than open-coding the translation, introduce a new macro
>> that will load the phyiscal address of a symbol.
>>
>> Lastly, use the new macro to replace all the current open-coded version.
>>
>> Note that most of the comments associated to the code changed have been
>> removed because the code is now self-explanatory.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>> ---
>>   xen/arch/arm/arm32/head.S | 23 +++++++++++------------
>>   1 file changed, 11 insertions(+), 12 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>> index c837d3054cf9..77f0a619ca51 100644
>> --- a/xen/arch/arm/arm32/head.S
>> +++ b/xen/arch/arm/arm32/head.S
>> @@ -65,6 +65,11 @@
>>           .endif
>>   .endm
>>   
>> +.macro load_paddr rb, sym
>> +        ldr   \rb, =\sym
>> +        add   \rb, \rb, r10
>> +.endm
>> +
> All the macros in this file have a comment so it'd be useful to follow this convention.
This is not really a convention. Most of the macros are non-trivial 
(e.g. they may clobber registers).

The comment I have in mind here would be:

"Load the physical address of \sym in \rb"

I am fairly confident that anyone can understand from the ".macro" 
line... So I don't feel the comment is necessary.

Cheers,
Michal Orzel June 27, 2022, 9:59 a.m. UTC | #3 
On 27.06.2022 11:52, Julien Grall wrote:
> 
> 
> On 27/06/2022 07:31, Michal Orzel wrote:
>> Hi Julien,
> 
> Hi Michal,
> 
>> On 24.06.2022 11:11, Julien Grall wrote:
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> A lot of places in the ARM32 assembly requires to load the physical address
>>> of a symbol. Rather than open-coding the translation, introduce a new macro
>>> that will load the phyiscal address of a symbol.
>>>
>>> Lastly, use the new macro to replace all the current open-coded version.
>>>
>>> Note that most of the comments associated to the code changed have been
>>> removed because the code is now self-explanatory.
>>>
>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>> ---
>>>   xen/arch/arm/arm32/head.S | 23 +++++++++++------------
>>>   1 file changed, 11 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>>> index c837d3054cf9..77f0a619ca51 100644
>>> --- a/xen/arch/arm/arm32/head.S
>>> +++ b/xen/arch/arm/arm32/head.S
>>> @@ -65,6 +65,11 @@
>>>           .endif
>>>   .endm
>>>   +.macro load_paddr rb, sym
>>> +        ldr   \rb, =\sym
>>> +        add   \rb, \rb, r10
>>> +.endm
>>> +
>> All the macros in this file have a comment so it'd be useful to follow this convention.
> This is not really a convention. Most of the macros are non-trivial (e.g. they may clobber registers).
> 
> The comment I have in mind here would be:
> 
> "Load the physical address of \sym in \rb"
> 
> I am fairly confident that anyone can understand from the ".macro" line... So I don't feel the comment is necessary.
> 
Fair enough although you did put a comment when introducing load_paddr for arm64 head.S.
Anyway, I'm ok not to add it.

> Cheers,
>
Julien Grall June 27, 2022, 10:09 a.m. UTC | #4 
Hi Michal,

On 27/06/2022 10:59, Michal Orzel wrote:
> 
> 
> On 27.06.2022 11:52, Julien Grall wrote:
>>
>>
>> On 27/06/2022 07:31, Michal Orzel wrote:
>>> Hi Julien,
>>
>> Hi Michal,
>>
>>> On 24.06.2022 11:11, Julien Grall wrote:
>>>> From: Julien Grall <jgrall@amazon.com>
>>>>
>>>> A lot of places in the ARM32 assembly requires to load the physical address
>>>> of a symbol. Rather than open-coding the translation, introduce a new macro
>>>> that will load the phyiscal address of a symbol.
>>>>
>>>> Lastly, use the new macro to replace all the current open-coded version.
>>>>
>>>> Note that most of the comments associated to the code changed have been
>>>> removed because the code is now self-explanatory.
>>>>
>>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>>> ---
>>>>    xen/arch/arm/arm32/head.S | 23 +++++++++++------------
>>>>    1 file changed, 11 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>>>> index c837d3054cf9..77f0a619ca51 100644
>>>> --- a/xen/arch/arm/arm32/head.S
>>>> +++ b/xen/arch/arm/arm32/head.S
>>>> @@ -65,6 +65,11 @@
>>>>            .endif
>>>>    .endm
>>>>    +.macro load_paddr rb, sym
>>>> +        ldr   \rb, =\sym
>>>> +        add   \rb, \rb, r10
>>>> +.endm
>>>> +
>>> All the macros in this file have a comment so it'd be useful to follow this convention.
>> This is not really a convention. Most of the macros are non-trivial (e.g. they may clobber registers).
>>
>> The comment I have in mind here would be:
>>
>> "Load the physical address of \sym in \rb"
>>
>> I am fairly confident that anyone can understand from the ".macro" line... So I don't feel the comment is necessary.
>>
> Fair enough although you did put a comment when introducing load_paddr for arm64 head.S

For the better (or the worse) my way to code has evolved in the past 5 
years. :) Commenting is something that changed. I learnt from other open 
source projects that it is better to comment when it is not clear what 
the function/code is doing.

Anyway, this is easy enough for me to add if either Bertrand or Stefano 
think that it is better to add a comment.

Cheers,
Bertrand Marquis June 27, 2022, 1:59 p.m. UTC | #5 
Hi Julien,

> On 27 Jun 2022, at 11:09, Julien Grall <julien@xen.org> wrote:
> 
> Hi Michal,
> 
> On 27/06/2022 10:59, Michal Orzel wrote:
>> On 27.06.2022 11:52, Julien Grall wrote:
>>> 
>>> 
>>> On 27/06/2022 07:31, Michal Orzel wrote:
>>>> Hi Julien,
>>> 
>>> Hi Michal,
>>> 
>>>> On 24.06.2022 11:11, Julien Grall wrote:
>>>>> From: Julien Grall <jgrall@amazon.com>
>>>>> 
>>>>> A lot of places in the ARM32 assembly requires to load the physical address
>>>>> of a symbol. Rather than open-coding the translation, introduce a new macro
>>>>> that will load the phyiscal address of a symbol.
>>>>> 
>>>>> Lastly, use the new macro to replace all the current open-coded version.
>>>>> 
>>>>> Note that most of the comments associated to the code changed have been
>>>>> removed because the code is now self-explanatory.
>>>>> 
>>>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>>>> ---
>>>>> xen/arch/arm/arm32/head.S | 23 +++++++++++------------
>>>>> 1 file changed, 11 insertions(+), 12 deletions(-)
>>>>> 
>>>>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>>>>> index c837d3054cf9..77f0a619ca51 100644
>>>>> --- a/xen/arch/arm/arm32/head.S
>>>>> +++ b/xen/arch/arm/arm32/head.S
>>>>> @@ -65,6 +65,11 @@
>>>>> .endif
>>>>> .endm
>>>>> +.macro load_paddr rb, sym
>>>>> +  ldr  \rb, =\sym
>>>>> +  add  \rb, \rb, r10
>>>>> +.endm
>>>>> +
>>>> All the macros in this file have a comment so it'd be useful to follow this convention.
>>> This is not really a convention. Most of the macros are non-trivial (e.g. they may clobber registers).
>>> 
>>> The comment I have in mind here would be:
>>> 
>>> "Load the physical address of \sym in \rb"
>>> 
>>> I am fairly confident that anyone can understand from the ".macro" line... So I don't feel the comment is necessary.
>>> 
>> Fair enough although you did put a comment when introducing load_paddr for arm64 head.S
> 
> For the better (or the worse) my way to code has evolved in the past 5 years. :) Commenting is something that changed. I learnt from other open source projects that it is better to comment when it is not clear what the function/code is doing.
> 
> Anyway, this is easy enough for me to add if either Bertrand or Stefano think that it is better to add a comment.

I do not think a comment to explain what is done in there is needed as it is quite obvious so:

Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand
diff mbox series

Patch

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index c837d3054cf9..77f0a619ca51 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -65,6 +65,11 @@ 
         .endif
 .endm
 
+.macro load_paddr rb, sym
+        ldr   \rb, =\sym
+        add   \rb, \rb, r10
+.endm
+
 /*
  * Common register usage in this file:
  *   r0  -
@@ -157,8 +162,7 @@  past_zImage:
 
         /* Using the DTB in the .dtb section? */
 .ifnes CONFIG_DTB_FILE,""
-        ldr   r8, =_sdtb
-        add   r8, r10                /* r8 := paddr(DTB) */
+        load_paddr r8, _stdb
 .endif
 
         /* Initialize the UART if earlyprintk has been enabled. */
@@ -208,8 +212,7 @@  GLOBAL(init_secondary)
         mrc   CP32(r1, MPIDR)
         bic   r7, r1, #(~MPIDR_HWID_MASK) /* Mask out flags to get CPU ID */
 
-        ldr   r0, =smp_up_cpu
-        add   r0, r0, r10            /* Apply physical offset */
+        load_paddr r0, smp_up_cpu
         dsb
 2:      ldr   r1, [r0]
         cmp   r1, r7
@@ -376,8 +379,7 @@  ENDPROC(cpu_init)
         and   r1, r1, r2             /* r1 := slot in \tlb */
         lsl   r1, r1, #3             /* r1 := slot offset in \tlb */
 
-        ldr   r4, =\tbl
-        add   r4, r4, r10            /* r4 := paddr(\tlb) */
+        load_paddr r4, \tbl
 
         movw  r2, #PT_PT             /* r2:r3 := right for linear PT */
         orr   r2, r2, r4             /*           + \tlb paddr */
@@ -536,8 +538,7 @@  enable_mmu:
         dsb   nsh
 
         /* Write Xen's PT's paddr into the HTTBR */
-        ldr   r0, =boot_pgtable
-        add   r0, r0, r10            /* r0 := paddr (boot_pagetable) */
+        load_paddr r0, boot_pgtable
         mov   r1, #0                 /* r0:r1 is paddr (boot_pagetable) */
         mcrr  CP64(r0, r1, HTTBR)
         isb
@@ -782,10 +783,8 @@  ENTRY(lookup_processor_type)
  */
 __lookup_processor_type:
         mrc   CP32(r0, MIDR)                /* r0 := our cpu id */
-        ldr   r1, = __proc_info_start
-        add   r1, r1, r10                   /* r1 := paddr of table (start) */
-        ldr   r2, = __proc_info_end
-        add   r2, r2, r10                   /* r2 := paddr of table (end) */
+        load_paddr r1, __proc_info_start
+        load_paddr r2, __proc_info_end
 1:      ldr   r3, [r1, #PROCINFO_cpu_mask]
         and   r4, r0, r3                    /* r4 := our cpu id with mask */
         ldr   r3, [r1, #PROCINFO_cpu_val]   /* r3 := cpu val in current proc info */