[RFC PATCH v1 0/2] xen/arm: Annotate code symbols

[RFC PATCH v1 0/2] xen/arm: Annotate code symbols

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>

On the way towards Xen safety certification we're evaluating the use
of tools to collect code-coverage/profiling information from execution
traces. Some tools rely on ELF symbols for code being declared with
type FUNC and having a symbol size.

We currently annotate some symbols but not all. Also, there seems to be
different ways to do the annotation so I'm sending out this RFC to first
figure out how we want to do things before I go ahead and edit more
of the ARM port.

In this first try I've followed the style from commit:
b3a9037550 x86: annotate entry points with type and size
IIUC, prefering to use macros from the generic framework in
xen/linkage.h in favor of ENTRY/ENDPROC.

But perhaps we would like to keep using ENTRY() for entry points
into the hypervisor?
Another way could be to add .type name, %function to the ENTRY macro
and use END from xen/linkage.h.

Or we can keep using ENTRY/GLOBAL/ENDPROC.

Any thoughts or better ideas?

Best regards,
Edgar

Edgar E. Iglesias (2):
  xen/arm64: entry: Use xen/linkage.h to annotate symbols
  xen/arm64: entry: Add missing code symbol annotations

 xen/arch/arm/arm64/entry.S | 72 +++++++++++++++++++++++++-------------
 1 file changed, 48 insertions(+), 24 deletions(-)

-- 
2.40.1

[RFC PATCH v1 1/2] xen/arm64: entry: Use xen/linkage.h to annotate symbols

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>

Use the generic xen/linkage.h macros when annotating symbols.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
---
 xen/arch/arm/arm64/entry.S | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index f963c923bb..6188dd2416 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -480,9 +480,9 @@ guest_fiq_invalid_compat:
 guest_error_compat:
         guest_vector compat=1, iflags=IFLAGS__AI_, trap=guest_serror
 
-ENTRY(return_to_new_vcpu32)
+FUNC(return_to_new_vcpu32)
         exit    hyp=0, compat=1
-ENTRY(return_to_new_vcpu64)
+FUNC(return_to_new_vcpu64)
         exit    hyp=0, compat=0
 
 return_from_trap:
@@ -536,7 +536,7 @@ return_from_trap:
  * it. So the function will unmask SError exception for a small window and
  * then mask it again.
  */
-check_pending_guest_serror:
+FUNC_LOCAL(check_pending_guest_serror)
         /*
          * Save elr_el2 to check whether the pending SError exception takes
          * place while we are doing this sync exception.
@@ -586,7 +586,7 @@ abort_guest_exit_end:
         cset    x19, ne
 
         ret
-ENDPROC(check_pending_guest_serror)
+END(check_pending_guest_serror)
 
 /*
  * Exception vectors.
@@ -597,7 +597,7 @@ ENDPROC(check_pending_guest_serror)
         .endm
 
         .align  11
-ENTRY(hyp_traps_vector)
+FUNC(hyp_traps_vector)
         ventry  hyp_sync_invalid            /* Synchronous EL2t */
         ventry  hyp_irq_invalid             /* IRQ EL2t */
         ventry  hyp_fiq_invalid             /* FIQ EL2t */
@@ -626,7 +626,7 @@ ENTRY(hyp_traps_vector)
  *
  * Returns prev in x0
  */
-ENTRY(__context_switch)
+FUNC(__context_switch)
         add     x8, x0, #VCPU_arch_saved_context
         mov     x9, sp
         stp     x19, x20, [x8], #16         /* store callee-saved registers */
-- 
2.40.1

[RFC PATCH v1 2/2] xen/arm64: entry: Add missing code symbol annotations

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>

Add missing code symbol annotations.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
---
 xen/arch/arm/arm64/entry.S | 60 ++++++++++++++++++++++++++------------
 1 file changed, 42 insertions(+), 18 deletions(-)

diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index 6188dd2416..af9a592cae 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -289,21 +289,25 @@
         b       do_bad_mode
         .endm
 
-hyp_sync_invalid:
+FUNC_LOCAL(hyp_sync_invalid)
         entry   hyp=1
         invalid BAD_SYNC
+END(hyp_sync_invalid)
 
-hyp_irq_invalid:
+FUNC_LOCAL(hyp_irq_invalid)
         entry   hyp=1
         invalid BAD_IRQ
+END(hyp_irq_invalid)
 
-hyp_fiq_invalid:
+FUNC_LOCAL(hyp_fiq_invalid)
         entry   hyp=1
         invalid BAD_FIQ
+END(hyp_fiq_invalid)
 
-hyp_error_invalid:
+FUNC_LOCAL(hyp_error_invalid)
         entry   hyp=1
         invalid BAD_ERROR
+END(hyp_error_invalid)
 
 /*
  * SError received while running in the hypervisor mode.
@@ -313,11 +317,12 @@ hyp_error_invalid:
  * simplicity, as SError should be rare and potentially fatal,
  * all interrupts are kept masked.
  */
-hyp_error:
+FUNC_LOCAL(hyp_error)
         entry   hyp=1
         mov     x0, sp
         bl      do_trap_hyp_serror
         exit    hyp=1
+END(hyp_error)
 
 /*
  * Synchronous exception received while running in the hypervisor mode.
@@ -327,7 +332,7 @@ hyp_error:
  * some of them. So we want to inherit the state from the interrupted
  * context.
  */
-hyp_sync:
+FUNC_LOCAL(hyp_sync)
         entry   hyp=1
 
         /* Inherit interrupts */
@@ -338,6 +343,7 @@ hyp_sync:
         mov     x0, sp
         bl      do_trap_hyp_sync
         exit    hyp=1
+END(hyp_sync)
 
 /*
  * IRQ received while running in the hypervisor mode.
@@ -352,7 +358,7 @@ hyp_sync:
  * would require some rework in some paths (e.g. panic, livepatch) to
  * ensure the ordering is enforced everywhere.
  */
-hyp_irq:
+FUNC_LOCAL(hyp_irq)
         entry   hyp=1
 
         /* Inherit D, A, F interrupts and keep I masked */
@@ -365,8 +371,9 @@ hyp_irq:
         mov     x0, sp
         bl      do_trap_irq
         exit    hyp=1
+END(hyp_irq)
 
-guest_sync:
+FUNC_LOCAL(guest_sync)
         /*
          * Save x0, x1 in advance
          */
@@ -413,8 +420,9 @@ fastpath_out_workaround:
         mov     x1, xzr
         eret
         sb
+END(guest_sync)
 
-wa2_ssbd:
+FUNC_LOCAL(wa2_ssbd)
 #ifdef CONFIG_ARM_SSBD
 alternative_cb arm_enable_wa2_handling
         b       wa2_end
@@ -450,42 +458,55 @@ wa2_end:
         mov     x0, xzr
         eret
         sb
-guest_sync_slowpath:
+END(wa2_ssbd)
+
+FUNC_LOCAL(guest_sync_slowpath)
         /*
          * x0/x1 may have been scratch by the fast path above, so avoid
          * to save them.
          */
         guest_vector compat=0, iflags=IFLAGS__AI_, trap=guest_sync, save_x0_x1=0
+END(guest_sync_slowpath)
 
-guest_irq:
+FUNC_LOCAL(guest_irq)
         guest_vector compat=0, iflags=IFLAGS__A__, trap=irq
+END(guest_irq)
 
-guest_fiq_invalid:
+FUNC_LOCAL(guest_fiq_invalid)
         entry   hyp=0, compat=0
         invalid BAD_FIQ
+END(guest_fiq_invalid)
 
-guest_error:
+FUNC_LOCAL(guest_error)
         guest_vector compat=0, iflags=IFLAGS__AI_, trap=guest_serror
+END(guest_error)
 
-guest_sync_compat:
+FUNC_LOCAL(guest_sync_compat)
         guest_vector compat=1, iflags=IFLAGS__AI_, trap=guest_sync
+END(guest_sync_compat)
 
-guest_irq_compat:
+FUNC_LOCAL(guest_irq_compat)
         guest_vector compat=1, iflags=IFLAGS__A__, trap=irq
+END(guest_irq_compat)
 
-guest_fiq_invalid_compat:
+FUNC_LOCAL(guest_fiq_invalid_compat)
         entry   hyp=0, compat=1
         invalid BAD_FIQ
+END(guest_fiq_invalid_compat)
 
-guest_error_compat:
+FUNC_LOCAL(guest_error_compat)
         guest_vector compat=1, iflags=IFLAGS__AI_, trap=guest_serror
+END(guest_error_compat)
 
 FUNC(return_to_new_vcpu32)
         exit    hyp=0, compat=1
+END(return_to_new_vcpu32)
+
 FUNC(return_to_new_vcpu64)
         exit    hyp=0, compat=0
+END(return_to_new_vcpu64)
 
-return_from_trap:
+FUNC_LOCAL(return_from_trap)
         msr     daifset, #IFLAGS___I_ /* Mask interrupts */
 
         ldr     x21, [sp, #UREGS_PC]            /* load ELR */
@@ -524,6 +545,7 @@ return_from_trap:
 
         eret
         sb
+END(return_from_trap)
 
 /*
  * Consume pending SError generated by the guest if any.
@@ -617,6 +639,7 @@ FUNC(hyp_traps_vector)
         ventry  guest_irq_compat            /* IRQ 32-bit EL0/EL1 */
         ventry  guest_fiq_invalid_compat    /* FIQ 32-bit EL0/EL1 */
         ventry  guest_error_compat          /* Error 32-bit EL0/EL1 */
+END(hyp_traps_vector)
 
 /*
  * struct vcpu *__context_switch(struct vcpu *prev, struct vcpu *next)
@@ -647,6 +670,7 @@ FUNC(__context_switch)
         ldr     lr, [x8]
         mov     sp, x9
         ret
+END(__context_switch)
 
 /*
  * Local variables:
-- 
2.40.1

Re: [RFC PATCH v1 1/2] xen/arm64: entry: Use xen/linkage.h to annotate symbols

[Andrew Cooper]

On 10/04/2024 10:19 am, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
>
> Use the generic xen/linkage.h macros when annotating symbols.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
> ---
>  xen/arch/arm/arm64/entry.S | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> index f963c923bb..6188dd2416 100644
> --- a/xen/arch/arm/arm64/entry.S
> +++ b/xen/arch/arm/arm64/entry.S
> @@ -480,9 +480,9 @@ guest_fiq_invalid_compat:
>  guest_error_compat:
>          guest_vector compat=1, iflags=IFLAGS__AI_, trap=guest_serror
>  
> -ENTRY(return_to_new_vcpu32)
> +FUNC(return_to_new_vcpu32)
>          exit    hyp=0, compat=1

In the new world, you want an END() too, which sets the size of the symbol.

A good cross-check of this annotation stuff is:

readelf -Wa xen-syms | grep return_to_new_vcpu32

which in this case will tell you that the symbol called
return_to_new_vcpu32 still has a size of 0.

~Andrew

Re: [RFC PATCH v1 1/2] xen/arm64: entry: Use xen/linkage.h to annotate symbols

[Edgar E. Iglesias]

[-- Attachment #1: Type: text/plain, Size: 1329 bytes --]

On Wed, Apr 10, 2024 at 12:21 PM Andrew Cooper <andrew.cooper3@citrix.com>
wrote:

> On 10/04/2024 10:19 am, Edgar E. Iglesias wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
> >
> > Use the generic xen/linkage.h macros when annotating symbols.
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
> > ---
> >  xen/arch/arm/arm64/entry.S | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> > index f963c923bb..6188dd2416 100644
> > --- a/xen/arch/arm/arm64/entry.S
> > +++ b/xen/arch/arm/arm64/entry.S
> > @@ -480,9 +480,9 @@ guest_fiq_invalid_compat:
> >  guest_error_compat:
> >          guest_vector compat=1, iflags=IFLAGS__AI_, trap=guest_serror
> >
> > -ENTRY(return_to_new_vcpu32)
> > +FUNC(return_to_new_vcpu32)
> >          exit    hyp=0, compat=1
>
> In the new world, you want an END() too, which sets the size of the symbol.
>
> A good cross-check of this annotation stuff is:
>
> readelf -Wa xen-syms | grep return_to_new_vcpu32
>
> which in this case will tell you that the symbol called
> return_to_new_vcpu32 still has a size of 0.
>

Thanks Andrew,

Patch 2/2 adds the END, I should probably have squashed them into one...

Best regards,
Edgar

[-- Attachment #2: Type: text/html, Size: 2007 bytes --]

Re: [RFC PATCH v1 1/2] xen/arm64: entry: Use xen/linkage.h to annotate symbols

[Jan Beulich]

On 10.04.2024 12:24, Edgar E. Iglesias wrote:
> On Wed, Apr 10, 2024 at 12:21 PM Andrew Cooper <andrew.cooper3@citrix.com>
> wrote:
> 
>> On 10/04/2024 10:19 am, Edgar E. Iglesias wrote:
>>> From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
>>>
>>> Use the generic xen/linkage.h macros when annotating symbols.
>>>
>>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
>>> ---
>>>  xen/arch/arm/arm64/entry.S | 12 ++++++------
>>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
>>> index f963c923bb..6188dd2416 100644
>>> --- a/xen/arch/arm/arm64/entry.S
>>> +++ b/xen/arch/arm/arm64/entry.S
>>> @@ -480,9 +480,9 @@ guest_fiq_invalid_compat:
>>>  guest_error_compat:
>>>          guest_vector compat=1, iflags=IFLAGS__AI_, trap=guest_serror
>>>
>>> -ENTRY(return_to_new_vcpu32)
>>> +FUNC(return_to_new_vcpu32)
>>>          exit    hyp=0, compat=1
>>
>> In the new world, you want an END() too, which sets the size of the symbol.
>>
>> A good cross-check of this annotation stuff is:
>>
>> readelf -Wa xen-syms | grep return_to_new_vcpu32
>>
>> which in this case will tell you that the symbol called
>> return_to_new_vcpu32 still has a size of 0.
> 
> Patch 2/2 adds the END, I should probably have squashed them into one...

Only partly afaics: return_to_new_vcpu{32,64} are still left without. And
yes, preferably the adjustments to the start annotation for a symbol
would come with an END() addition right away.

Jan

Re: [RFC PATCH v1 1/2] xen/arm64: entry: Use xen/linkage.h to annotate symbols

[Edgar E. Iglesias]

On Thu, Apr 18, 2024 at 8:10 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 10.04.2024 12:24, Edgar E. Iglesias wrote:
> > On Wed, Apr 10, 2024 at 12:21 PM Andrew Cooper <andrew.cooper3@citrix.com>
> > wrote:
> >
> >> On 10/04/2024 10:19 am, Edgar E. Iglesias wrote:
> >>> From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
> >>>
> >>> Use the generic xen/linkage.h macros when annotating symbols.
> >>>
> >>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
> >>> ---
> >>>  xen/arch/arm/arm64/entry.S | 12 ++++++------
> >>>  1 file changed, 6 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> >>> index f963c923bb..6188dd2416 100644
> >>> --- a/xen/arch/arm/arm64/entry.S
> >>> +++ b/xen/arch/arm/arm64/entry.S
> >>> @@ -480,9 +480,9 @@ guest_fiq_invalid_compat:
> >>>  guest_error_compat:
> >>>          guest_vector compat=1, iflags=IFLAGS__AI_, trap=guest_serror
> >>>
> >>> -ENTRY(return_to_new_vcpu32)
> >>> +FUNC(return_to_new_vcpu32)
> >>>          exit    hyp=0, compat=1
> >>
> >> In the new world, you want an END() too, which sets the size of the symbol.
> >>
> >> A good cross-check of this annotation stuff is:
> >>
> >> readelf -Wa xen-syms | grep return_to_new_vcpu32
> >>
> >> which in this case will tell you that the symbol called
> >> return_to_new_vcpu32 still has a size of 0.
> >
> > Patch 2/2 adds the END, I should probably have squashed them into one...
>
> Only partly afaics: return_to_new_vcpu{32,64} are still left without. And
> yes, preferably the adjustments to the start annotation for a symbol
> would come with an END() addition right away.
>

Thanks Jan,

Yes, in v2 I've squashed the patches into one to avoid confusion:
https://patchew.org/Xen/20240415231541.4140052-1-edgar.iglesias@gmail.com/

Here's the hunk in patch 2/2 of the first v1 RFC submission that added
the END's to return_to_new_vcpuXX:
https://lists.xenproject.org/archives/html/xen-devel/2024-04/msg00505.html

FUNC(return_to_new_vcpu32)
         exit    hyp=0, compat=1
+END(return_to_new_vcpu32)
+
 FUNC(return_to_new_vcpu64)
         exit    hyp=0, compat=0
+END(return_to_new_vcpu64)

Cheers,
Edgar