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

[Julien Grall]

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.hal...@amd.com>
---
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.or...@amd.com>

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