| Message ID | f7228594-fa64-4fd8-b55b-506d004b73cb@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] Arm64: amend "xen/arm64: head: Add missing code symbol annotations" | expand |
Hi Jan, On 10/06/2024 14:37, Jan Beulich wrote: > While the change[1] didn't go in yet, there is the intention for the ELF > metadata annotations from xen/linkage.h to also effect honoring of > CONFIG_CC_SPLIT_SECTIONS. In code that's placement / ordering sensitive, > these annotations therefore need using with some care. Looking at the code, I think the ordering only really matter for 'start'. The rest can be ordered in any way within the assembly file. So... > > [1] https://lists.xen.org/archives/html/xen-devel/2024-02/msg00470.html > > Fixes: fba250ae604e ("xen/arm64: head: Add missing code symbol annotations") > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > An alternative would be to use LABEL{,_LOCAL}() instead of FUNC{,_LOCAL}(). > That would avoid the need for any override, but would also lose the type > information. ... I would suggest to only convert FUNC(start) to LABEL(start). > Question is whether the annotated ranges really are > "functions" in whichever wide or narrow sense. Everything but 'start' are functions. Cheers,
On 10.07.2024 23:29, Julien Grall wrote: > Hi Jan, > > On 10/06/2024 14:37, Jan Beulich wrote: >> While the change[1] didn't go in yet, there is the intention for the ELF >> metadata annotations from xen/linkage.h to also effect honoring of >> CONFIG_CC_SPLIT_SECTIONS. In code that's placement / ordering sensitive, >> these annotations therefore need using with some care. > > Looking at the code, I think the ordering only really matter for > 'start'. The rest can be ordered in any way within the assembly file. So... > >> >> [1] https://lists.xen.org/archives/html/xen-devel/2024-02/msg00470.html >> >> Fixes: fba250ae604e ("xen/arm64: head: Add missing code symbol annotations") >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> An alternative would be to use LABEL{,_LOCAL}() instead of FUNC{,_LOCAL}(). >> That would avoid the need for any override, but would also lose the type >> information. > > ... I would suggest to only convert FUNC(start) to LABEL(start). > >> Question is whether the annotated ranges really are >> "functions" in whichever wide or narrow sense. > > Everything but 'start' are functions. Well, maybe, but that's not the only criteria here. Expressions like .long _end - real_start /* SizeOfCode */ need to be possible for the assembler to resolve. Which requires both symbols to live in the same section, or the subtrahend to live in the current section. IOW I then also need to convert real_start to use LABEL_LOCAL(). Is that going to be okay? Jan
--- a/xen/arch/arm/arm64/head.S +++ b/xen/arch/arm/arm64/head.S @@ -28,6 +28,14 @@ #include <asm/arm64/efibind.h> #endif +/* + * Code here is, at least in part, ordering sensitive. Annotations + * from xen/linkage.h therefore may not switch sections (honoring + * CONFIG_CC_SPLIT_SECTIONS). Override the respective macro. + */ +#undef SYM_PUSH_SECTION +#define SYM_PUSH_SECTION(name, attr) + #define __HEAD_FLAG_PAGE_SIZE ((PAGE_SHIFT - 10) / 2) #define __HEAD_FLAG_PHYS_BASE 1
While the change[1] didn't go in yet, there is the intention for the ELF metadata annotations from xen/linkage.h to also effect honoring of CONFIG_CC_SPLIT_SECTIONS. In code that's placement / ordering sensitive, these annotations therefore need using with some care. [1] https://lists.xen.org/archives/html/xen-devel/2024-02/msg00470.html Fixes: fba250ae604e ("xen/arm64: head: Add missing code symbol annotations") Signed-off-by: Jan Beulich <jbeulich@suse.com> --- An alternative would be to use LABEL{,_LOCAL}() instead of FUNC{,_LOCAL}(). That would avoid the need for any override, but would also lose the type information. Question is whether the annotated ranges really are "functions" in whichever wide or narrow sense. The Fixes: tag is slightly questionable, seeing that the patch actually adding section switching didn't go in yet; the issue right now is a latent one only. Whichever form of an adjustment we'll settle on will likely want to become part of [1] above. Hence the RFC.