From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
Use the generic xen/linkage.h macros to annotate code symbols
and add missing annotations.
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
---
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)
.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
--
2.40.1On Wed, 1 May 2024, Edgar E. Iglesias wrote: > From: "Edgar E. Iglesias" <edgar.iglesias@amd.com> > > Use the generic xen/linkage.h macros to annotate code symbols > and add missing annotations. > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com> > --- > 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 >
On Sat, May 4, 2024 at 2:14 AM Stefano Stabellini <sstabellini@kernel.org> wrote: > > On Wed, 1 May 2024, Edgar E. Iglesias wrote: > > From: "Edgar E. Iglesias" <edgar.iglesias@amd.com> > > > > Use the generic xen/linkage.h macros to annotate code symbols > > and add missing annotations. > > > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com> > > --- > > 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 > >
Hi, On 06/05/2024 13:54, Edgar E. Iglesias wrote: > On Sat, May 4, 2024 at 2:14 AM Stefano Stabellini > <sstabellini@kernel.org> wrote: >> >> On Wed, 1 May 2024, Edgar E. Iglesias wrote: >>> From: "Edgar E. Iglesias" <edgar.iglesias@amd.com> >>> >>> Use the generic xen/linkage.h macros to annotate code symbols >>> and add missing annotations. >>> >>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com> >>> --- >>> 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, -- Julien Grall
On Tue, May 7, 2024 at 11:57 AM Julien Grall <julien@xen.org> wrote: > > Hi, > > On 06/05/2024 13:54, Edgar E. Iglesias wrote: > > On Sat, May 4, 2024 at 2:14 AM Stefano Stabellini > > <sstabellini@kernel.org> wrote: > >> > >> On Wed, 1 May 2024, Edgar E. Iglesias wrote: > >>> From: "Edgar E. Iglesias" <edgar.iglesias@amd.com> > >>> > >>> Use the generic xen/linkage.h macros to annotate code symbols > >>> and add missing annotations. > >>> > >>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com> > >>> --- > >>> 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
On 07/05/2024 17:55, Edgar E. Iglesias wrote: > On Tue, May 7, 2024 at 11:57 AM Julien Grall <julien@xen.org> 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, -- Julien Grall
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 <julien@xen.org> 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
On Tue, May 7, 2024 at 7:37 PM Julien Grall <julien@xen.org> wrote: > > > > On 07/05/2024 17:55, Edgar E. Iglesias wrote: > > On Tue, May 7, 2024 at 11:57 AM Julien Grall <julien@xen.org> 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
© 2016 - 2026 Red Hat, Inc.