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(-)

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

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,

-- 
Julien Grall

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

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
>
>