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

[Michal Orzel <michal.orzel@xxxxxxx>]

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@xxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxx>
>>> ---
>>>   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,
>