diff mbox series

xen/arm64: head.S: Fix wrong enable_boot_cpu_mm() code movement

Message ID 20230916040649.1232558-1-Henry.Wang@arm.com (mailing list archive)
State New, archived
Headers show
Series xen/arm64: head.S: Fix wrong enable_boot_cpu_mm() code movement | expand

Commit Message

Henry Wang Sept. 16, 2023, 4:06 a.m. UTC
Some addressed comments on enable_boot_cpu_mm() were reintroduced
back during the code movement from arm64/head.S to arm64/mmu/head.S.
We should drop the unreachable code, move the 'mov lr, x5' closer to
'b remove_identity_mapping' so it is clearer that it will return,
and update the in-code comment accordingly.

Fixes: 6734327d76be ("xen/arm64: Split and move MMU-specific head.S to mmu/head.S")
Reported-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Henry Wang <Henry.Wang@arm.com>
---
 xen/arch/arm/arm64/mmu/head.S | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

Comments

Ayan Kumar Halder Sept. 21, 2023, 2 p.m. UTC | #1
On 16/09/2023 05:06, Henry Wang wrote:
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
>
>
> Some addressed comments on enable_boot_cpu_mm() were reintroduced
> back during the code movement from arm64/head.S to arm64/mmu/head.S.
> We should drop the unreachable code, move the 'mov lr, x5' closer to
> 'b remove_identity_mapping' so it is clearer that it will return,
> and update the in-code comment accordingly.
>
> Fixes: 6734327d76be ("xen/arm64: Split and move MMU-specific head.S to mmu/head.S")
> Reported-by: Julien Grall <jgrall@amazon.com>
> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
Reviewed-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
>   xen/arch/arm/arm64/mmu/head.S | 11 +++--------
>   1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/mmu/head.S b/xen/arch/arm/arm64/mmu/head.S
> index a5271e3880..88075ef083 100644
> --- a/xen/arch/arm/arm64/mmu/head.S
> +++ b/xen/arch/arm/arm64/mmu/head.S
> @@ -329,7 +329,6 @@ ENTRY(enable_boot_cpu_mm)
>           load_paddr x0, boot_pgtable
>
>           bl    enable_mmu
> -        mov   lr, x5
>
>           /*
>            * The MMU is turned on and we are in the 1:1 mapping. Switch
> @@ -338,19 +337,15 @@ ENTRY(enable_boot_cpu_mm)
>           ldr   x0, =1f
>           br    x0
>   1:
> +        mov   lr, x5
>           /*
>            * The 1:1 map may clash with other parts of the Xen virtual memory
>            * layout. As it is not used anymore, remove it completely to
>            * avoid having to worry about replacing existing mapping
> -         * afterwards. Function will return to primary_switched.
> +         * afterwards. Function will return to the virtual address requested
> +         * by the caller.
>            */
>           b     remove_identity_mapping
> -
> -        /*
> -         * Below is supposed to be unreachable code, as "ret" in
> -         * remove_identity_mapping will use the return address in LR in advance.
> -         */
> -        b     fail
>   ENDPROC(enable_boot_cpu_mm)
>
>   /*
> --
> 2.25.1
>
>
Julien Grall Sept. 21, 2023, 5:16 p.m. UTC | #2
Hi Henry,

On 16/09/2023 05:06, Henry Wang wrote:
> Some addressed comments on enable_boot_cpu_mm() were reintroduced
> back during the code movement from arm64/head.S to arm64/mmu/head.S.
> We should drop the unreachable code, move the 'mov lr, x5' closer to
> 'b remove_identity_mapping' so it is clearer that it will return,
> and update the in-code comment accordingly.
> 
> Fixes: 6734327d76be ("xen/arm64: Split and move MMU-specific head.S to mmu/head.S")
> Reported-by: Julien Grall <jgrall@amazon.com>
> Signed-off-by: Henry Wang <Henry.Wang@arm.com>

Reviewed-by: Julien Grall <jgrall@amazon.com>

I plan to commit this patch in staging so it is part of 4.18. Please let 
me know if there are any objection.

Cheers,
Henry Wang Sept. 22, 2023, 1:14 a.m. UTC | #3
Hi Julien,

> On Sep 22, 2023, at 01:16, Julien Grall <julien@xen.org> wrote:
> 
> Hi Henry,
> 
> On 16/09/2023 05:06, Henry Wang wrote:
>> Some addressed comments on enable_boot_cpu_mm() were reintroduced
>> back during the code movement from arm64/head.S to arm64/mmu/head.S.
>> We should drop the unreachable code, move the 'mov lr, x5' closer to
>> 'b remove_identity_mapping' so it is clearer that it will return,
>> and update the in-code comment accordingly.
>> Fixes: 6734327d76be ("xen/arm64: Split and move MMU-specific head.S to mmu/head.S")
>> Reported-by: Julien Grall <jgrall@amazon.com>
>> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
> 
> Reviewed-by: Julien Grall <jgrall@amazon.com>

Thanks very much!

> 
> I plan to commit this patch in staging so it is part of 4.18. Please let me know if there are any objection.

Yes, please commit this patch, as it should have been part of the committed patch
6734327d76be (again I am sorry for the mess).

Kind regards,
Henry

> 
> Cheers,
> 
> -- 
> Julien Grall
diff mbox series

Patch

diff --git a/xen/arch/arm/arm64/mmu/head.S b/xen/arch/arm/arm64/mmu/head.S
index a5271e3880..88075ef083 100644
--- a/xen/arch/arm/arm64/mmu/head.S
+++ b/xen/arch/arm/arm64/mmu/head.S
@@ -329,7 +329,6 @@  ENTRY(enable_boot_cpu_mm)
         load_paddr x0, boot_pgtable
 
         bl    enable_mmu
-        mov   lr, x5
 
         /*
          * The MMU is turned on and we are in the 1:1 mapping. Switch
@@ -338,19 +337,15 @@  ENTRY(enable_boot_cpu_mm)
         ldr   x0, =1f
         br    x0
 1:
+        mov   lr, x5
         /*
          * The 1:1 map may clash with other parts of the Xen virtual memory
          * layout. As it is not used anymore, remove it completely to
          * avoid having to worry about replacing existing mapping
-         * afterwards. Function will return to primary_switched.
+         * afterwards. Function will return to the virtual address requested
+         * by the caller.
          */
         b     remove_identity_mapping
-
-        /*
-         * Below is supposed to be unreachable code, as "ret" in
-         * remove_identity_mapping will use the return address in LR in advance.
-         */
-        b     fail
 ENDPROC(enable_boot_cpu_mm)
 
 /*