Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Should the GLOBAL()s also be replaced?
---
v7: New.

--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -31,7 +31,7 @@
  *  r4: Set to a non-zero value if a pending Abort exception took place.
  *      Otherwise, it will be set to zero.
  */
-prepare_context_from_guest:
+FUNC_LOCAL(prepare_context_from_guest)
 #ifdef CONFIG_ARM32_HARDEN_BRANCH_PREDICTOR
         /*
          * Restore vectors table to the default as it may have been
@@ -140,7 +140,7 @@ abort_guest_exit_end:
 
 skip_check:
         b   enter_hypervisor_from_guest_preirq
-ENDPROC(prepare_context_from_guest)
+END(prepare_context_from_guest)
 
         /*
          * Macro to define a trap entry.
@@ -362,13 +362,13 @@ trap_irq:
 trap_fiq:
         vector fiq
 
-return_from_trap:
+LABEL_LOCAL(return_from_trap)
         /*
          * Restore the stack pointer from r11. It was saved on exception
          * entry (see __DEFINE_TRAP_ENTRY).
          */
         mov sp, r11
-ENTRY(return_to_new_vcpu32)
+LABEL(return_to_new_vcpu32)
         ldr r11, [sp, #UREGS_cpsr]
         and r11, #PSR_MODE_MASK
         cmp r11, #PSR_MODE_HYP
@@ -426,6 +426,7 @@ return_to_hypervisor:
         clrex
         eret
         sb
+END(return_from_trap)
 
 /*
  * struct vcpu *__context_switch(struct vcpu *prev, struct vcpu *next)
@@ -435,12 +436,13 @@ return_to_hypervisor:
  *
  * Returns prev in r0
  */
-ENTRY(__context_switch)
+FUNC(__context_switch)
         add     ip, r0, #VCPU_arch_saved_context
         stmia   ip!, {r4 - sl, fp, sp, lr}      /* Save register state */
 
         add     r4, r1, #VCPU_arch_saved_context
         ldmia   r4, {r4 - sl, fp, sp, pc}       /* Load registers and return */
+END(__context_switch)
 
 /*
  * Local variables:

Hi Jan,

On 01/10/2024 16:17, Jan Beulich wrote:
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Should the GLOBAL()s also be replaced?

What would you replace them with?

> ---
> v7: New.
> 
> --- a/xen/arch/arm/arm32/entry.S
> +++ b/xen/arch/arm/arm32/entry.S
> @@ -31,7 +31,7 @@
>    *  r4: Set to a non-zero value if a pending Abort exception took place.
>    *      Otherwise, it will be set to zero.
>    */
> -prepare_context_from_guest:
> +FUNC_LOCAL(prepare_context_from_guest)
>   #ifdef CONFIG_ARM32_HARDEN_BRANCH_PREDICTOR
>           /*
>            * Restore vectors table to the default as it may have been
> @@ -140,7 +140,7 @@ abort_guest_exit_end:
>   
>   skip_check:
>           b   enter_hypervisor_from_guest_preirq
> -ENDPROC(prepare_context_from_guest)
> +END(prepare_context_from_guest)
>   
>           /*
>            * Macro to define a trap entry.
> @@ -362,13 +362,13 @@ trap_irq:
>   trap_fiq:
>           vector fiq
>   
> -return_from_trap:
> +LABEL_LOCAL(return_from_trap)

OOI, why do we need to annotate return_from_trap?

>           /*
>            * Restore the stack pointer from r11. It was saved on exception
>            * entry (see __DEFINE_TRAP_ENTRY).
>            */
>           mov sp, r11
> -ENTRY(return_to_new_vcpu32)
> +LABEL(return_to_new_vcpu32)

I am a bit confused why this is a LABEL rather than a FUNC. But I wonder 
if either of them are corrrect? The code above is meant to fall into 
this one. So I think at least the alignment should be 0.

>           ldr r11, [sp, #UREGS_cpsr]
>           and r11, #PSR_MODE_MASK
>           cmp r11, #PSR_MODE_HYP
> @@ -426,6 +426,7 @@ return_to_hypervisor:
>           clrex
>           eret
>           sb
> +END(return_from_trap)
>   
>   /*
>    * struct vcpu *__context_switch(struct vcpu *prev, struct vcpu *next)
> @@ -435,12 +436,13 @@ return_to_hypervisor:
>    *
>    * Returns prev in r0
>    */
> -ENTRY(__context_switch)
> +FUNC(__context_switch)
>           add     ip, r0, #VCPU_arch_saved_context
>           stmia   ip!, {r4 - sl, fp, sp, lr}      /* Save register state */
>   
>           add     r4, r1, #VCPU_arch_saved_context
>           ldmia   r4, {r4 - sl, fp, sp, pc}       /* Load registers and return */
> +END(__context_switch)
>   
>   /*
>    * Local variables:
> 

Cheers,

-- 
Julien Grall

On 25.11.2024 21:25, Julien Grall wrote:
> On 01/10/2024 16:17, Jan Beulich wrote:
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Should the GLOBAL()s also be replaced?
> 
> What would you replace them with?

LABEL() or, perhaps better, DATA()/END() pairs.

>> @@ -362,13 +362,13 @@ trap_irq:
>>   trap_fiq:
>>           vector fiq
>>   
>> -return_from_trap:
>> +LABEL_LOCAL(return_from_trap)
> 
> OOI, why do we need to annotate return_from_trap?

We don't _need_ to; it's for consistency. Please advise whether you want me
to drop that change (it's the only use of LABEL_LOCAL() throughout the series).
But please be aware that then END(return_from_trap) also needs dropping (i.e.
leaving the range uncovered as far as plain ELF metadata goes).

>>           /*
>>            * Restore the stack pointer from r11. It was saved on exception
>>            * entry (see __DEFINE_TRAP_ENTRY).
>>            */
>>           mov sp, r11
>> -ENTRY(return_to_new_vcpu32)
>> +LABEL(return_to_new_vcpu32)
> 
> I am a bit confused why this is a LABEL rather than a FUNC. But I wonder 
> if either of them are corrrect? The code above is meant to fall into 
> this one.

My take is that function (or object) ranges shouldn't overlap. That's an
abstract reason. There's a practical reason, too: With "common: honor
CONFIG_CC_SPLIT_SECTIONS also for assembly functions" FUNC() will change
sections when CC_SPLIT_SECTIONS=y (which LABEL() won't). Section changes
clearly may not happen when fall-through is intended.

> So I think at least the alignment should be 0.

That would be a change compared to the prior use of ENTRY(). I can certainly
do so, but without having sufficient context my goal was to leave existing
alignment unaltered as far as possible (on the assumption that there's a
reason for it).

Jan