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

[Julien Grall <julien@xxxxxxx>]

On 30/10/2023 08:28, Michal Orzel wrote:
> 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>
I have committed with the following diff:

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 1fcc6f745e31..bbbdf7daf89e 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -882,10 +882,8 @@ ENTRY(lookup_processor_type)
         ldmfd sp!, {r4, pc}

 /*
- *  Read processor ID register (CP#15, CR0), and Look up in the 
linker-built
- * supported processor list. Note that we can't use the absolute 
addresses for
- * 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.
+ * Read processor ID register (CP#15, CR0), and Look up in the linker-built
+ * supported processor list.
  *
  * Returns:
  * r0: CPUID

Note that I took the opportunity to remove the extra space on the first 
line of the comment.
> ~Michal
--
Julien Grall