[v3,6/9] xen/arm64: bpi: Add missing code symbol annotations

Commit Message

Edgar E. Iglesias May 1, 2024, 3:54 a.m. UTC

From: "Edgar E. Iglesias" <[e-mail]>

Use the generic xen/linkage.h macros to annotate code symbols
and add missing annotations.

Signed-off-by: Edgar E. Iglesias <[e-mail]>
---
 xen/arch/arm/arm64/bpi.S | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

Comments

Stefano Stabellini May 4, 2024, 12:14 a.m. UTC | #1

On Wed, 1 May 2024, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <[e-mail]>
> 
> Use the generic xen/linkage.h macros to annotate code symbols
> and add missing annotations.
> 
> Signed-off-by: Edgar E. Iglesias <[e-mail]>
> ---
>  xen/arch/arm/arm64/bpi.S | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/bpi.S b/xen/arch/arm/arm64/bpi.S
> index 4e63825220..b16e4d1e29 100644
> --- a/xen/arch/arm/arm64/bpi.S
> +++ b/xen/arch/arm/arm64/bpi.S
> @@ -52,14 +52,15 @@
>   * micro-architectures in a system.
>   */
>      .align	11
> -ENTRY(__bp_harden_hyp_vecs_start)
> +FUNC(__bp_harden_hyp_vecs_start)
>      .rept 4
>      vectors hyp_traps_vector
>      .endr
> -ENTRY(__bp_harden_hyp_vecs_end)
> +GLOBAL(__bp_harden_hyp_vecs_end)
> +END(__bp_harden_hyp_vecs_start)

Shouldn't GLOBAL be changed to FUNC as well?


>  .macro mitigate_spectre_bhb_loop count
> -ENTRY(__mitigate_spectre_bhb_loop_start_\count)
> +FUNC(__mitigate_spectre_bhb_loop_start_\count)
>      stp     x0, x1, [sp, #-16]!
>      mov     x0, \count
>  .Lspectre_bhb_loop\@:
> @@ -68,11 +69,12 @@ ENTRY(__mitigate_spectre_bhb_loop_start_\count)
>      b.ne    .Lspectre_bhb_loop\@
>      sb
>      ldp     x0, x1, [sp], #16
> -ENTRY(__mitigate_spectre_bhb_loop_end_\count)
> +GLOBAL(__mitigate_spectre_bhb_loop_end_\count)

Also here?


> +END(__mitigate_spectre_bhb_loop_start_\count)
>  .endm
>  
>  .macro smccc_workaround num smcc_id
> -ENTRY(__smccc_workaround_smc_start_\num)
> +FUNC(__smccc_workaround_smc_start_\num)
>      sub     sp, sp, #(8 * 4)
>      stp     x0, x1, [sp, #(8 * 2)]
>      stp     x2, x3, [sp, #(8 * 0)]
> @@ -81,13 +83,15 @@ ENTRY(__smccc_workaround_smc_start_\num)
>      ldp     x2, x3, [sp, #(8 * 0)]
>      ldp     x0, x1, [sp, #(8 * 2)]
>      add     sp, sp, #(8 * 4)
> -ENTRY(__smccc_workaround_smc_end_\num)
> +GLOBAL(__smccc_workaround_smc_end_\num)

And here?


> +END(__smccc_workaround_smc_start_\num)
>  .endm
>  
> -ENTRY(__mitigate_spectre_bhb_clear_insn_start)
> +FUNC(__mitigate_spectre_bhb_clear_insn_start)
>      clearbhb
>      isb
> -ENTRY(__mitigate_spectre_bhb_clear_insn_end)
> +GLOBAL(__mitigate_spectre_bhb_clear_insn_end)

and here?


> +END(__mitigate_spectre_bhb_clear_insn_start)
>  
>  mitigate_spectre_bhb_loop 8
>  mitigate_spectre_bhb_loop 24
> -- 
> 2.40.1
>

Edgar E. Iglesias May 6, 2024, 12:54 p.m. UTC | #2

On Sat, May 4, 2024 at 2:14 AM Stefano Stabellini
<[e-mail]> wrote:
>
> On Wed, 1 May 2024, Edgar E. Iglesias wrote:
> > From: "Edgar E. Iglesias" <[e-mail]>
> >
> > Use the generic xen/linkage.h macros to annotate code symbols
> > and add missing annotations.
> >
> > Signed-off-by: Edgar E. Iglesias <[e-mail]>
> > ---
> >  xen/arch/arm/arm64/bpi.S | 20 ++++++++++++--------
> >  1 file changed, 12 insertions(+), 8 deletions(-)
> >
> > diff --git a/xen/arch/arm/arm64/bpi.S b/xen/arch/arm/arm64/bpi.S
> > index 4e63825220..b16e4d1e29 100644
> > --- a/xen/arch/arm/arm64/bpi.S
> > +++ b/xen/arch/arm/arm64/bpi.S
> > @@ -52,14 +52,15 @@
> >   * micro-architectures in a system.
> >   */
> >      .align   11
> > -ENTRY(__bp_harden_hyp_vecs_start)
> > +FUNC(__bp_harden_hyp_vecs_start)
> >      .rept 4
> >      vectors hyp_traps_vector
> >      .endr
> > -ENTRY(__bp_harden_hyp_vecs_end)
> > +GLOBAL(__bp_harden_hyp_vecs_end)
> > +END(__bp_harden_hyp_vecs_start)
>
> Shouldn't GLOBAL be changed to FUNC as well?
>

I was a bit unsure but went for GLOBAL since the _end labels point to
addresses after and outside of the code sequence.
But I don't have a strong opinion and am happy to change them to FUNC
if you feel that's better.

Cheers,
Edgar


>
> >  .macro mitigate_spectre_bhb_loop count
> > -ENTRY(__mitigate_spectre_bhb_loop_start_\count)
> > +FUNC(__mitigate_spectre_bhb_loop_start_\count)
> >      stp     x0, x1, [sp, #-16]!
> >      mov     x0, \count
> >  .Lspectre_bhb_loop\@:
> > @@ -68,11 +69,12 @@ ENTRY(__mitigate_spectre_bhb_loop_start_\count)
> >      b.ne    .Lspectre_bhb_loop\@
> >      sb
> >      ldp     x0, x1, [sp], #16
> > -ENTRY(__mitigate_spectre_bhb_loop_end_\count)
> > +GLOBAL(__mitigate_spectre_bhb_loop_end_\count)
>
> Also here?
>
>
> > +END(__mitigate_spectre_bhb_loop_start_\count)
> >  .endm
> >
> >  .macro smccc_workaround num smcc_id
> > -ENTRY(__smccc_workaround_smc_start_\num)
> > +FUNC(__smccc_workaround_smc_start_\num)
> >      sub     sp, sp, #(8 * 4)
> >      stp     x0, x1, [sp, #(8 * 2)]
> >      stp     x2, x3, [sp, #(8 * 0)]
> > @@ -81,13 +83,15 @@ ENTRY(__smccc_workaround_smc_start_\num)
> >      ldp     x2, x3, [sp, #(8 * 0)]
> >      ldp     x0, x1, [sp, #(8 * 2)]
> >      add     sp, sp, #(8 * 4)
> > -ENTRY(__smccc_workaround_smc_end_\num)
> > +GLOBAL(__smccc_workaround_smc_end_\num)
>
> And here?
>
>
> > +END(__smccc_workaround_smc_start_\num)
> >  .endm
> >
> > -ENTRY(__mitigate_spectre_bhb_clear_insn_start)
> > +FUNC(__mitigate_spectre_bhb_clear_insn_start)
> >      clearbhb
> >      isb
> > -ENTRY(__mitigate_spectre_bhb_clear_insn_end)
> > +GLOBAL(__mitigate_spectre_bhb_clear_insn_end)
>
> and here?
>
>
> > +END(__mitigate_spectre_bhb_clear_insn_start)
> >
> >  mitigate_spectre_bhb_loop 8
> >  mitigate_spectre_bhb_loop 24
> > --
> > 2.40.1
> >

Julien Grall May 7, 2024, 9:57 a.m. UTC | #3

Hi,

On 06/05/2024 13:54, Edgar E. Iglesias wrote:
> On Sat, May 4, 2024 at 2:14 AM Stefano Stabellini
> <[e-mail]> wrote:
>>
>> On Wed, 1 May 2024, Edgar E. Iglesias wrote:
>>> From: "Edgar E. Iglesias" <[e-mail]>
>>>
>>> Use the generic xen/linkage.h macros to annotate code symbols
>>> and add missing annotations.
>>>
>>> Signed-off-by: Edgar E. Iglesias <[e-mail]>
>>> ---
>>>   xen/arch/arm/arm64/bpi.S | 20 ++++++++++++--------
>>>   1 file changed, 12 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/arm64/bpi.S b/xen/arch/arm/arm64/bpi.S
>>> index 4e63825220..b16e4d1e29 100644
>>> --- a/xen/arch/arm/arm64/bpi.S
>>> +++ b/xen/arch/arm/arm64/bpi.S
>>> @@ -52,14 +52,15 @@
>>>    * micro-architectures in a system.
>>>    */
>>>       .align   11
>>> -ENTRY(__bp_harden_hyp_vecs_start)
>>> +FUNC(__bp_harden_hyp_vecs_start)
>>>       .rept 4
>>>       vectors hyp_traps_vector
>>>       .endr
>>> -ENTRY(__bp_harden_hyp_vecs_end)
>>> +GLOBAL(__bp_harden_hyp_vecs_end)
>>> +END(__bp_harden_hyp_vecs_start)
>>
>> Shouldn't GLOBAL be changed to FUNC as well?
>>
> 
> I was a bit unsure but went for GLOBAL since the _end labels point to
> addresses after and outside of the code sequence.
> But I don't have a strong opinion and am happy to change them to FUNC
> if you feel that's better.

I don't think it should be FUNC as this is not meant to be called 
directly. I am also under the impression, we were planning to get rid of 
GLOBAL() as well.

Furthermore, __bp_harden_hyp_vec_start is not a function per say. It is 
a pointer to the vector table.

 From the brief look, the same remarks would apply to the rest of bpi.S. 
So I think we want to switch all the ENTRY() to LABEL().

Cheers,

Edgar E. Iglesias May 7, 2024, 4:55 p.m. UTC | #4

On Tue, May 7, 2024 at 11:57 AM Julien Grall <[e-mail]> wrote:
>
> Hi,
>
> On 06/05/2024 13:54, Edgar E. Iglesias wrote:
> > On Sat, May 4, 2024 at 2:14 AM Stefano Stabellini
> > <[e-mail]> wrote:
> >>
> >> On Wed, 1 May 2024, Edgar E. Iglesias wrote:
> >>> From: "Edgar E. Iglesias" <[e-mail]>
> >>>
> >>> Use the generic xen/linkage.h macros to annotate code symbols
> >>> and add missing annotations.
> >>>
> >>> Signed-off-by: Edgar E. Iglesias <[e-mail]>
> >>> ---
> >>>   xen/arch/arm/arm64/bpi.S | 20 ++++++++++++--------
> >>>   1 file changed, 12 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/xen/arch/arm/arm64/bpi.S b/xen/arch/arm/arm64/bpi.S
> >>> index 4e63825220..b16e4d1e29 100644
> >>> --- a/xen/arch/arm/arm64/bpi.S
> >>> +++ b/xen/arch/arm/arm64/bpi.S
> >>> @@ -52,14 +52,15 @@
> >>>    * micro-architectures in a system.
> >>>    */
> >>>       .align   11
> >>> -ENTRY(__bp_harden_hyp_vecs_start)
> >>> +FUNC(__bp_harden_hyp_vecs_start)
> >>>       .rept 4
> >>>       vectors hyp_traps_vector
> >>>       .endr
> >>> -ENTRY(__bp_harden_hyp_vecs_end)
> >>> +GLOBAL(__bp_harden_hyp_vecs_end)
> >>> +END(__bp_harden_hyp_vecs_start)
> >>
> >> Shouldn't GLOBAL be changed to FUNC as well?
> >>
> >
> > I was a bit unsure but went for GLOBAL since the _end labels point to
> > addresses after and outside of the code sequence.
> > But I don't have a strong opinion and am happy to change them to FUNC
> > if you feel that's better.
>
> I don't think it should be FUNC as this is not meant to be called
> directly. I am also under the impression, we were planning to get rid of
> GLOBAL() as well.
>
> Furthermore, __bp_harden_hyp_vec_start is not a function per say. It is
> a pointer to the vector table.
>
>  From the brief look, the same remarks would apply to the rest of bpi.S.
> So I think we want to switch all the ENTRY() to LABEL().

Hi Julien,

The reason I choose FUNC for the start of the symbol is because these
symbols contain
executable code (not only a table of pointers to code somewhere else)
and the ELF spec
says that STT_FUNC means the symbol contains functions or other executable
code (not only callable functions IIUC):

"STT_FUNC The symbol is associated with a function or other executable code."
https://refspecs.linuxbase.org/elf/elf.pdf
(Symbol Table 1-20).

I think using LABEL instead of GLOBAL for the _end labels of these
code sequences makes sense.
I'm happy to change the _start labels to LABEL too if you guys feel
that's better.

Cheers,
Edgar

Julien Grall May 7, 2024, 5:37 p.m. UTC | #5

On 07/05/2024 17:55, Edgar E. Iglesias wrote:
> On Tue, May 7, 2024 at 11:57 AM Julien Grall <[e-mail]> wrote:
> Hi Julien,

Hi Edgar,

> 
> The reason I choose FUNC for the start of the symbol is because these
> symbols contain
> executable code (not only a table of pointers to code somewhere else)
> and the ELF spec
> says that STT_FUNC means the symbol contains functions or other executable
> code (not only callable functions IIUC):
> 
> "STT_FUNC The symbol is associated with a function or other executable code."
> https://refspecs.linuxbase.org/elf/elf.pdf
> (Symbol Table 1-20).

Thanks for the pointer. I originally did intend to suggest the change, 
but then I saw the use of LABEL in x86 (such as svm_stgi_label). There 
are a few others example with LABEL_LOCAL.

AFAICT, this is also executable code which the only difference that it 
is not meant to be called by someone else. Furthermore, LABEL is using 
DO_CODE_ALIGN(...) for the alignment which imply that it is intended to 
be used by executable code. So I thought the only difference was whether 
the label was intended to be used as a function.

> 
> I think using LABEL instead of GLOBAL for the _end labels of these
> code sequences makes sense.
> I'm happy to change the _start labels to LABEL too if you guys feel
> that's better.

I have to admit I am little confused with the difference between LABEL 
vs FUNC. I think I will need some guidance from Jan (he introduced 
linkage.h).

Cheers,

Edgar E. Iglesias May 10, 2024, 4:26 p.m. UTC | #6

On Tue, May 7, 2024 at 7:37 PM Julien Grall <[e-mail]> wrote:
>
>
>
> On 07/05/2024 17:55, Edgar E. Iglesias wrote:
> > On Tue, May 7, 2024 at 11:57 AM Julien Grall <[e-mail]> wrote:
> > Hi Julien,
>
> Hi Edgar,
>
> >
> > The reason I choose FUNC for the start of the symbol is because these
> > symbols contain
> > executable code (not only a table of pointers to code somewhere else)
> > and the ELF spec
> > says that STT_FUNC means the symbol contains functions or other executable
> > code (not only callable functions IIUC):
> >
> > "STT_FUNC The symbol is associated with a function or other executable code."
> > https://refspecs.linuxbase.org/elf/elf.pdf
> > (Symbol Table 1-20).
>
> Thanks for the pointer. I originally did intend to suggest the change,
> but then I saw the use of LABEL in x86 (such as svm_stgi_label). There
> are a few others example with LABEL_LOCAL.
>
> AFAICT, this is also executable code which the only difference that it
> is not meant to be called by someone else. Furthermore, LABEL is using
> DO_CODE_ALIGN(...) for the alignment which imply that it is intended to
> be used by executable code. So I thought the only difference was whether
> the label was intended to be used as a function.
>

Thanks Julien, yes, good points.

> >
> > I think using LABEL instead of GLOBAL for the _end labels of these
> > code sequences makes sense.
> > I'm happy to change the _start labels to LABEL too if you guys feel
> > that's better.
>
> I have to admit I am little confused with the difference between LABEL
> vs FUNC. I think I will need some guidance from Jan (he introduced
> linkage.h).
>

Jan, do you have any guidance on this specific case using FUNC , LABEL
(or something else)?

Cheers,
Edgar

Jan Beulich May 14, 2024, 7:17 a.m. UTC | #7

On 07.05.2024 19:37, Julien Grall wrote:
> 
> 
> On 07/05/2024 17:55, Edgar E. Iglesias wrote:
>> On Tue, May 7, 2024 at 11:57 AM Julien Grall <[e-mail]> wrote:
>> Hi Julien,
> 
> Hi Edgar,
> 
>>
>> The reason I choose FUNC for the start of the symbol is because these
>> symbols contain
>> executable code (not only a table of pointers to code somewhere else)
>> and the ELF spec
>> says that STT_FUNC means the symbol contains functions or other executable
>> code (not only callable functions IIUC):
>>
>> "STT_FUNC The symbol is associated with a function or other executable code."
>> https://refspecs.linuxbase.org/elf/elf.pdf
>> (Symbol Table 1-20).
> 
> Thanks for the pointer. I originally did intend to suggest the change, 
> but then I saw the use of LABEL in x86 (such as svm_stgi_label). There 
> are a few others example with LABEL_LOCAL.
> 
> AFAICT, this is also executable code which the only difference that it 
> is not meant to be called by someone else. Furthermore, LABEL is using 
> DO_CODE_ALIGN(...) for the alignment which imply that it is intended to 
> be used by executable code. So I thought the only difference was whether 
> the label was intended to be used as a function.

No. See below.

>> I think using LABEL instead of GLOBAL for the _end labels of these
>> code sequences makes sense.
>> I'm happy to change the _start labels to LABEL too if you guys feel
>> that's better.
> 
> I have to admit I am little confused with the difference between LABEL 
> vs FUNC. I think I will need some guidance from Jan (he introduced 
> linkage.h).

For annotations the question is what is a "unit" of code. That wants to
be enclosed in FUNC() / END(). Any "inner" entry points or markers would
use LABEL(). On x86 I think it's mainly markers (i.e. addresses pointing
into code which we need e.g. for comparison operations on what Arm would
call PC) where we use LABEL().

Jan

Patch

diff --git a/xen/arch/arm/arm64/bpi.S b/xen/arch/arm/arm64/bpi.S
index 4e63825220..b16e4d1e29 100644
--- a/xen/arch/arm/arm64/bpi.S
+++ b/xen/arch/arm/arm64/bpi.S
@@ -52,14 +52,15 @@ 
  * micro-architectures in a system.
  */
     .align	11
-ENTRY(__bp_harden_hyp_vecs_start)
+FUNC(__bp_harden_hyp_vecs_start)
     .rept 4
     vectors hyp_traps_vector
     .endr
-ENTRY(__bp_harden_hyp_vecs_end)
+GLOBAL(__bp_harden_hyp_vecs_end)
+END(__bp_harden_hyp_vecs_start)
 
 .macro mitigate_spectre_bhb_loop count
-ENTRY(__mitigate_spectre_bhb_loop_start_\count)
+FUNC(__mitigate_spectre_bhb_loop_start_\count)
     stp     x0, x1, [sp, #-16]!
     mov     x0, \count
 .Lspectre_bhb_loop\@:
@@ -68,11 +69,12 @@  ENTRY(__mitigate_spectre_bhb_loop_start_\count)
     b.ne    .Lspectre_bhb_loop\@
     sb
     ldp     x0, x1, [sp], #16
-ENTRY(__mitigate_spectre_bhb_loop_end_\count)
+GLOBAL(__mitigate_spectre_bhb_loop_end_\count)
+END(__mitigate_spectre_bhb_loop_start_\count)
 .endm
 
 .macro smccc_workaround num smcc_id
-ENTRY(__smccc_workaround_smc_start_\num)
+FUNC(__smccc_workaround_smc_start_\num)
     sub     sp, sp, #(8 * 4)
     stp     x0, x1, [sp, #(8 * 2)]
     stp     x2, x3, [sp, #(8 * 0)]
@@ -81,13 +83,15 @@  ENTRY(__smccc_workaround_smc_start_\num)
     ldp     x2, x3, [sp, #(8 * 0)]
     ldp     x0, x1, [sp, #(8 * 2)]
     add     sp, sp, #(8 * 4)
-ENTRY(__smccc_workaround_smc_end_\num)
+GLOBAL(__smccc_workaround_smc_end_\num)
+END(__smccc_workaround_smc_start_\num)
 .endm
 
-ENTRY(__mitigate_spectre_bhb_clear_insn_start)
+FUNC(__mitigate_spectre_bhb_clear_insn_start)
     clearbhb
     isb
-ENTRY(__mitigate_spectre_bhb_clear_insn_end)
+GLOBAL(__mitigate_spectre_bhb_clear_insn_end)
+END(__mitigate_spectre_bhb_clear_insn_start)
 
 mitigate_spectre_bhb_loop 8
 mitigate_spectre_bhb_loop 24