Re: [Xen-devel] [PATCH v2 18/35] xen/arm64: head: Introduce a macro to get a PC-relative address of a symbol

[Julien Grall <julien.grall@arm.com>]

Hi,

On 7/30/19 7:24 PM, Stefano Stabellini wrote:
> On Mon, 22 Jul 2019, Julien Grall wrote:
>> Arm64 provides instructions to load a PC-relative address, but with some
>> limitations:
>>     - adr is enable to cope with +/-1MB
>>     - adrp is enale to cope with +/-4GB but relative to a 4KB page
>>       address
>>
>> Because of that, the code requires to use 2 instructions to load any Xen
>> symbol. To make the code more obvious, introducing a new macro adr_l is
>> introduced.
>>
>> The new macro is used to replace a couple of open-coded use in
>> efi_xen_start.
>>
>> The macro is copied from Linux 5.2-rc4.
> 
> I was going to ask why the strange name "adr_l", now I know why :-)

I think this stands for "load".

> 
> I'd suggest to name it more clearly to maybe "adr_relative"?

This is a bit weird to have one full word and the other one shorten. The 
current solution has the advantage to be short and therefore looks like 
an instruction (and so keep everything correctly aligned).

So I would prefer to keep the function as is.

> In any case:
> 
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>

Thank you.

> 
>> Signed-off-by: Julien Grall <julien.grall@arm.coM>
> 
> Typo in your address
> 
> 
>> ---
>>      Changes in v2:
>>          - Patch added
>> ---
>>   xen/arch/arm/arm64/head.S | 18 ++++++++++++++----
>>   1 file changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index 9afd89d447..2287f3ce48 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -111,6 +111,18 @@
>>   
>>   #endif /* !CONFIG_EARLY_PRINTK */
>>   
>> +/*
>> + * Pseudo-op for PC relative adr <reg>, <symbol> where <symbol> is
>> + * within the range +/- 4GB of the PC.
>> + *
>> + * @dst: destination register (64 bit wide)
>> + * @sym: name of the symbol
>> + */
>> +.macro  adr_l, dst, sym
>> +        adrp \dst, \sym
>> +        add  \dst, \dst, :lo12:\sym
>> +.endm
>> +
>>   /* Load the physical address of a symbol into xb */
>>   .macro load_paddr xb, sym
>>           ldr \xb, =\sym
>> @@ -886,11 +898,9 @@ ENTRY(efi_xen_start)
>>            * Flush dcache covering current runtime addresses
>>            * of xen text/data. Then flush all of icache.
>>            */
>> -        adrp  x1, _start
>> -        add   x1, x1, #:lo12:_start
>> +        adr_l x1, _start
>>           mov   x0, x1
>> -        adrp  x2, _end
>> -        add   x2, x2, #:lo12:_end
>> +        adr_l x2, _end
>>           sub   x1, x2, x1
>>   
>>           bl    __flush_dcache_area
>> -- 
>> 2.11.0
>>

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel