Re: [XEN v2] xen/arm: arm32: Use adr_l instead of load_paddr for getting address of symbols

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Ayan,

On 26/10/2023 13:19, Julien Grall wrote:
> 
> 
> Hi,
> 
> Title: This reads as you replace all adr_l with load_paddr. So how about:
> 
> xen/arm32: head: Replace load_paddr with adr_l when they are equivalent
> 
> On 26/10/2023 12:12, Ayan Kumar Halder wrote:
>> Before the MMU is turned on, PC uses physical address. Thus, one can use 
>> adr_l
>> instead of load_paddr to obtain the physical address of a symbol.
>>
>> The only exception (for this replacement) is create_table_entry() which is
>> called before and after MMU is turned on.
>>
>> Also, in lookup_processor_type() "r10" is no longer used. The reason being
>> __lookup_processor_type uses adr_l (thus r10 is no longer used to obtain the
>> physical address offset).
>>
>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
>> ---
>> Refer https://lists.archive.carbon60.com/xen/devel/682900 for details.
>>
>> Changes from :-
>>
>> v1 :- 1. No need to modify create_table_entry().
>> 2. Remove "mov   r10, #0 " in lookup_processor_type().
>>
>>   xen/arch/arm/arm32/head.S | 13 ++++++-------
>>   1 file changed, 6 insertions(+), 7 deletions(-)
>>

[...]
>>   /* This provides a C-API version of __lookup_processor_type */
>>   ENTRY(lookup_processor_type)
>>           stmfd sp!, {r4, r10, lr}
> 
> Do we still need to save/restore r10?
Apart from this remark...

> 
>> -        mov   r10, #0                   /* r10 := offset between virt&phys 
>> */
>>           bl    __lookup_processor_type
>>           mov r0, r1
>>           ldmfd sp!, {r4, r10, pc}
>> @@ -897,8 +896,8 @@ ENTRY(lookup_processor_type)
>>    */
there is also a comment here listing r10 as the register in use and explaining
the need to calculate the offset. This should be tweaked as well (r10 is no 
longer in use
and we don't care if we are running before or after MMU is on, since adr_l will 
always get us
the address in the current address space which is what we expect (before MMU - 
physical, after MMU - virtual).

>>   __lookup_processor_type:
>>           mrc   CP32(r0, MIDR)                /* r0 := our cpu id */
>> -        load_paddr r1, __proc_info_start
>> -        load_paddr r2, __proc_info_end
>> +        adr_l r1, __proc_info_start
>> +        adr_l 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 */
> 
> Cheers,
> 
> --
> Julien Grall

~Michal