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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 27 Jun 2022, at 11:09, Julien Grall <julien@xxxxxxx> 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@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
> 
> 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@xxxxxxx>

Cheers
Bertrand