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

[Julien Grall <julien@xxxxxxx>]

On 16/11/2023 14:07, Julien Grall wrote:
> 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.
Oh I didn't realize there was a v4 sent. Looking at it this was this 
only change. So I will not revert.
Sorry for that.

Cheers,

--
Julien Grall