Re: [XEN v3] xen/arm32: head: Replace load_paddr with adr_l when they are equivalent

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Ayan,

On 27/10/2023 20:07, 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). Consequently, there is no need to save/restore r10.
> 
> 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().
> 
> v2 :- 1. No need to save/restore r10 in lookup_processor_type().
> 2. Update the commit message title.
> 
>  xen/arch/arm/arm32/head.S | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 33b038e7e0..1fcc6f745e 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -171,7 +171,7 @@ past_zImage:
>  
>          /* Using the DTB in the .dtb section? */
>  .ifnes CONFIG_DTB_FILE,""
> -        load_paddr r8, _sdtb
> +        adr_l r8, _sdtb
>  .endif
>  
>          /* Initialize the UART if earlyprintk has been enabled. */
> @@ -213,7 +213,7 @@ GLOBAL(init_secondary)
>          mrc   CP32(r1, MPIDR)
>          bic   r7, r1, #(~MPIDR_HWID_MASK) /* Mask out flags to get CPU ID */
>  
> -        load_paddr r0, smp_up_cpu
> +        adr_l r0, smp_up_cpu
>          dsb
>  2:      ldr   r1, [r0]
>          cmp   r1, r7
> @@ -479,7 +479,7 @@ create_page_tables:
>           * create_table_entry_paddr() will clobber the register storing
>           * the physical address of the table to point to.
>           */
> -        load_paddr r5, boot_third
> +        adr_l r5, boot_third
>          mov_w r4, XEN_VIRT_START
>  .rept XEN_NR_ENTRIES(2)
>          mov   r0, r5                        /* r0 := paddr(l3 table) */
> @@ -578,7 +578,7 @@ enable_mmu:
>          flush_xen_tlb_local r0
>  
>          /* Write Xen's PT's paddr into the HTTBR */
> -        load_paddr r0, boot_pgtable
> +        adr_l r0, boot_pgtable
>          mov   r1, #0                 /* r0:r1 is paddr (boot_pagetable) */
>          mcrr  CP64(r0, r1, HTTBR)
>          isb
> @@ -876,11 +876,10 @@ putn:   mov   pc, lr
>  
>  /* This provides a C-API version of __lookup_processor_type */
>  ENTRY(lookup_processor_type)
> -        stmfd sp!, {r4, r10, lr}
> -        mov   r10, #0                   /* r10 := offset between virt&phys */
> +        stmfd sp!, {r4, lr}
>          bl    __lookup_processor_type
>          mov r0, r1
> -        ldmfd sp!, {r4, r10, pc}
> +        ldmfd sp!, {r4, pc}
>  
>  /*
>   *  Read processor ID register (CP#15, CR0), and Look up in the linker-built
> @@ -888,8 +887,6 @@ ENTRY(lookup_processor_type)
>   * the __proc_info lists since we aren't running with the MMU on (and 
> therefore,
>   * we are not in correct address space). We have to calculate the offset.
In v2, I mentioned that this comment needs to be tweaked as well. We no longer 
use load_paddr
thus we don't care about the offset. I would remove the comment starting from 
"Note that...".
to avoid confusion or add a proper explanation if you want to keep it.
With that addressed:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal