1. Patchew
  2. Xen
  3. View series

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

Ayan Kumar Halder posted 1 patch 2 years, 3 months ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20231027180715.3484439-1-ayan.kumar.halder@amd.com
xen/arch/arm/arm32/head.S | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
[XEN v3] xen/arm32: head: Replace load_paddr with adr_l when they are equivalent
Posted by Ayan Kumar Halder 2 years, 3 months ago 
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@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.
  *
- * r10: offset between virt&phys
- *
  * Returns:
  * r0: CPUID
  * r1: proc_info pointer
@@ -897,8 +894,8 @@ ENTRY(lookup_processor_type)
  */
 __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 */
-- 
2.25.1
Re: [XEN v3] xen/arm32: head: Replace load_paddr with adr_l when they are equivalent
Posted by Michal Orzel 2 years, 3 months ago 
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@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.orzel@amd.com>

~Michal
Re: [XEN v3] xen/arm32: head: Replace load_paddr with adr_l when they are equivalent
Posted by Julien Grall 2 years, 2 months ago 

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@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.orzel@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
Re: [XEN v3] xen/arm32: head: Replace load_paddr with adr_l when they are equivalent
Posted by Julien Grall 2 years, 2 months ago 
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@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.orzel@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.

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.