| Message ID | 20220705210218.483854-4-burzalodowa@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix MISRA C 2012 violations | expand |
Hi Xenia, On 05/07/2022 22:02, Xenia Ragiadakou wrote: > The function idle_loop() is referenced only in domain.c. > Change its linkage from external to internal by adding the storage-class > specifier static to its definitions. > > Since idle_loop() is referenced only in inline assembly, add the 'used' > attribute to suppress unused-function compiler warning. > > Also, this patch resolves indirectly a MISRA C 2012 Rule 8.4 violation warning. > > Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com> Acked-by: Julien Grall <jgrall@amazon.com> Cheers, > --- > xen/arch/arm/domain.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > index 2f8eaab7b5..2fa67266d0 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -63,7 +63,7 @@ static void do_idle(void) > rcu_idle_exit(cpu); > } > > -void idle_loop(void) > +static __used void idle_loop(void) > { > unsigned int cpu = smp_processor_id(); >
On 05.07.2022 23:02, Xenia Ragiadakou wrote: > The function idle_loop() is referenced only in domain.c. > Change its linkage from external to internal by adding the storage-class > specifier static to its definitions. > > Since idle_loop() is referenced only in inline assembly, add the 'used' > attribute to suppress unused-function compiler warning. While I see that Julien has already acked the patch, I'd like to point out that using __used here is somewhat bogus. Imo the better approach is to properly make visible to the compiler that the symbol is used by the asm(), by adding a fake(?) input. Jan
Hi Jan, On 7/6/22 10:10, Jan Beulich wrote: > On 05.07.2022 23:02, Xenia Ragiadakou wrote: >> The function idle_loop() is referenced only in domain.c. >> Change its linkage from external to internal by adding the storage-class >> specifier static to its definitions. >> >> Since idle_loop() is referenced only in inline assembly, add the 'used' >> attribute to suppress unused-function compiler warning. > > While I see that Julien has already acked the patch, I'd like to point > out that using __used here is somewhat bogus. Imo the better approach > is to properly make visible to the compiler that the symbol is used by > the asm(), by adding a fake(?) input. I 'm afraid I do not understand what do you mean by "adding a fake(?) input". Can you please elaborate a little on your suggestion?
On 06.07.2022 10:43, Xenia Ragiadakou wrote: > On 7/6/22 10:10, Jan Beulich wrote: >> On 05.07.2022 23:02, Xenia Ragiadakou wrote: >>> The function idle_loop() is referenced only in domain.c. >>> Change its linkage from external to internal by adding the storage-class >>> specifier static to its definitions. >>> >>> Since idle_loop() is referenced only in inline assembly, add the 'used' >>> attribute to suppress unused-function compiler warning. >> >> While I see that Julien has already acked the patch, I'd like to point >> out that using __used here is somewhat bogus. Imo the better approach >> is to properly make visible to the compiler that the symbol is used by >> the asm(), by adding a fake(?) input. > > I 'm afraid I do not understand what do you mean by "adding a fake(?) > input". Can you please elaborate a little on your suggestion? Once the asm() in question takes the function as an input, the compiler will know that the function has a user (unless, of course, it finds a way to elide the asm() itself). The "fake(?)" was because I'm not deeply enough into Arm inline assembly to know whether the input could then also be used as an instruction operand (which imo would be preferable) - if it can't (e.g. because there's no suitable constraint or operand modifier), it still can be an input just to inform the compiler. Jan
Hi Jan, On 7/6/22 11:51, Jan Beulich wrote: > On 06.07.2022 10:43, Xenia Ragiadakou wrote: >> On 7/6/22 10:10, Jan Beulich wrote: >>> On 05.07.2022 23:02, Xenia Ragiadakou wrote: >>>> The function idle_loop() is referenced only in domain.c. >>>> Change its linkage from external to internal by adding the storage-class >>>> specifier static to its definitions. >>>> >>>> Since idle_loop() is referenced only in inline assembly, add the 'used' >>>> attribute to suppress unused-function compiler warning. >>> >>> While I see that Julien has already acked the patch, I'd like to point >>> out that using __used here is somewhat bogus. Imo the better approach >>> is to properly make visible to the compiler that the symbol is used by >>> the asm(), by adding a fake(?) input. >> >> I 'm afraid I do not understand what do you mean by "adding a fake(?) >> input". Can you please elaborate a little on your suggestion? > > Once the asm() in question takes the function as an input, the compiler > will know that the function has a user (unless, of course, it finds a > way to elide the asm() itself). The "fake(?)" was because I'm not deeply > enough into Arm inline assembly to know whether the input could then > also be used as an instruction operand (which imo would be preferable) - > if it can't (e.g. because there's no suitable constraint or operand > modifier), it still can be an input just to inform the compiler. According to the following statement, your approach is the recommended one: "To prevent the compiler from removing global data or functions which are referenced from inline assembly statements, you can: -use __attribute__((used)) with the global data or functions. -pass the reference to global data or functions as operands to inline assembly statements. Arm recommends passing the reference to global data or functions as operands to inline assembly statements so that if the final image does not require the inline assembly statements and the referenced global data or function, then they can be removed." IIUC, you are suggesting to change asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" ) into asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" ); This gives, respectively: reset_stack_and_jump(idle_loop); 460: 9100001f mov sp, x0 464: 14000007 b 480 <idle_loop> reset_stack_and_jump(idle_loop); 460: 9100001f mov sp, x0 464: 14000000 b 600 <idle_loop> With this change, the functions return_to_new_vcpu32 and return_to_new_vcpu64, implemented in assembly and called in the same way as idle_loop(), need to be declared.
On 07.07.2022 09:27, Xenia Ragiadakou wrote: > On 7/6/22 11:51, Jan Beulich wrote: >> On 06.07.2022 10:43, Xenia Ragiadakou wrote: >>> On 7/6/22 10:10, Jan Beulich wrote: >>>> On 05.07.2022 23:02, Xenia Ragiadakou wrote: >>>>> The function idle_loop() is referenced only in domain.c. >>>>> Change its linkage from external to internal by adding the storage-class >>>>> specifier static to its definitions. >>>>> >>>>> Since idle_loop() is referenced only in inline assembly, add the 'used' >>>>> attribute to suppress unused-function compiler warning. >>>> >>>> While I see that Julien has already acked the patch, I'd like to point >>>> out that using __used here is somewhat bogus. Imo the better approach >>>> is to properly make visible to the compiler that the symbol is used by >>>> the asm(), by adding a fake(?) input. >>> >>> I 'm afraid I do not understand what do you mean by "adding a fake(?) >>> input". Can you please elaborate a little on your suggestion? >> >> Once the asm() in question takes the function as an input, the compiler >> will know that the function has a user (unless, of course, it finds a >> way to elide the asm() itself). The "fake(?)" was because I'm not deeply >> enough into Arm inline assembly to know whether the input could then >> also be used as an instruction operand (which imo would be preferable) - >> if it can't (e.g. because there's no suitable constraint or operand >> modifier), it still can be an input just to inform the compiler. > > According to the following statement, your approach is the recommended one: > "To prevent the compiler from removing global data or functions which > are referenced from inline assembly statements, you can: > -use __attribute__((used)) with the global data or functions. > -pass the reference to global data or functions as operands to inline > assembly statements. > Arm recommends passing the reference to global data or functions as > operands to inline assembly statements so that if the final image does > not require the inline assembly statements and the referenced global > data or function, then they can be removed." > > IIUC, you are suggesting to change > asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" ) > into > asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" ); Yes, except that I can't judge about the "S" constraint. > This gives, respectively: > reset_stack_and_jump(idle_loop); > > 460: 9100001f mov sp, x0 > > 464: 14000007 b 480 <idle_loop> > > > reset_stack_and_jump(idle_loop); > > 460: 9100001f mov sp, x0 > > 464: 14000000 b 600 <idle_loop> > > > With this change, the functions return_to_new_vcpu32 and > return_to_new_vcpu64, implemented in assembly and called in the same way > as idle_loop(), need to be declared. Which imo is a good thing - these probably shouldn't have lived entirely behind the back of the compiler. Jan
On 7/7/22 10:55, Jan Beulich wrote: > On 07.07.2022 09:27, Xenia Ragiadakou wrote: >> On 7/6/22 11:51, Jan Beulich wrote: >>> On 06.07.2022 10:43, Xenia Ragiadakou wrote: >>>> On 7/6/22 10:10, Jan Beulich wrote: >>>>> On 05.07.2022 23:02, Xenia Ragiadakou wrote: >>>>>> The function idle_loop() is referenced only in domain.c. >>>>>> Change its linkage from external to internal by adding the storage-class >>>>>> specifier static to its definitions. >>>>>> >>>>>> Since idle_loop() is referenced only in inline assembly, add the 'used' >>>>>> attribute to suppress unused-function compiler warning. >>>>> >>>>> While I see that Julien has already acked the patch, I'd like to point >>>>> out that using __used here is somewhat bogus. Imo the better approach >>>>> is to properly make visible to the compiler that the symbol is used by >>>>> the asm(), by adding a fake(?) input. >>>> >>>> I 'm afraid I do not understand what do you mean by "adding a fake(?) >>>> input". Can you please elaborate a little on your suggestion? >>> >>> Once the asm() in question takes the function as an input, the compiler >>> will know that the function has a user (unless, of course, it finds a >>> way to elide the asm() itself). The "fake(?)" was because I'm not deeply >>> enough into Arm inline assembly to know whether the input could then >>> also be used as an instruction operand (which imo would be preferable) - >>> if it can't (e.g. because there's no suitable constraint or operand >>> modifier), it still can be an input just to inform the compiler. >> >> According to the following statement, your approach is the recommended one: >> "To prevent the compiler from removing global data or functions which >> are referenced from inline assembly statements, you can: >> -use __attribute__((used)) with the global data or functions. >> -pass the reference to global data or functions as operands to inline >> assembly statements. >> Arm recommends passing the reference to global data or functions as >> operands to inline assembly statements so that if the final image does >> not require the inline assembly statements and the referenced global >> data or function, then they can be removed." >> >> IIUC, you are suggesting to change >> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" ) >> into >> asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" ); > > Yes, except that I can't judge about the "S" constraint. > This constraint does not work for arm32. Any other thoughts? Another way, as Jan suggested, is to pass the function as a 'fake' (unused) input. I 'm suspecting something like the following could work asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory") What do you think? >> This gives, respectively: >> reset_stack_and_jump(idle_loop); >> >> 460: 9100001f mov sp, x0 >> >> 464: 14000007 b 480 <idle_loop> >> >> >> reset_stack_and_jump(idle_loop); >> >> 460: 9100001f mov sp, x0 >> >> 464: 14000000 b 600 <idle_loop> >> >> >> With this change, the functions return_to_new_vcpu32 and >> return_to_new_vcpu64, implemented in assembly and called in the same way >> as idle_loop(), need to be declared. > > Which imo is a good thing - these probably shouldn't have lived entirely > behind the back of the compiler. > > Jan
On 24.07.2022 19:20, Xenia Ragiadakou wrote: > On 7/7/22 10:55, Jan Beulich wrote: >> On 07.07.2022 09:27, Xenia Ragiadakou wrote: >>> On 7/6/22 11:51, Jan Beulich wrote: >>>> On 06.07.2022 10:43, Xenia Ragiadakou wrote: >>>>> On 7/6/22 10:10, Jan Beulich wrote: >>>>>> On 05.07.2022 23:02, Xenia Ragiadakou wrote: >>>>>>> The function idle_loop() is referenced only in domain.c. >>>>>>> Change its linkage from external to internal by adding the storage-class >>>>>>> specifier static to its definitions. >>>>>>> >>>>>>> Since idle_loop() is referenced only in inline assembly, add the 'used' >>>>>>> attribute to suppress unused-function compiler warning. >>>>>> >>>>>> While I see that Julien has already acked the patch, I'd like to point >>>>>> out that using __used here is somewhat bogus. Imo the better approach >>>>>> is to properly make visible to the compiler that the symbol is used by >>>>>> the asm(), by adding a fake(?) input. >>>>> >>>>> I 'm afraid I do not understand what do you mean by "adding a fake(?) >>>>> input". Can you please elaborate a little on your suggestion? >>>> >>>> Once the asm() in question takes the function as an input, the compiler >>>> will know that the function has a user (unless, of course, it finds a >>>> way to elide the asm() itself). The "fake(?)" was because I'm not deeply >>>> enough into Arm inline assembly to know whether the input could then >>>> also be used as an instruction operand (which imo would be preferable) - >>>> if it can't (e.g. because there's no suitable constraint or operand >>>> modifier), it still can be an input just to inform the compiler. >>> >>> According to the following statement, your approach is the recommended one: >>> "To prevent the compiler from removing global data or functions which >>> are referenced from inline assembly statements, you can: >>> -use __attribute__((used)) with the global data or functions. >>> -pass the reference to global data or functions as operands to inline >>> assembly statements. >>> Arm recommends passing the reference to global data or functions as >>> operands to inline assembly statements so that if the final image does >>> not require the inline assembly statements and the referenced global >>> data or function, then they can be removed." >>> >>> IIUC, you are suggesting to change >>> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" ) >>> into >>> asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" ); >> >> Yes, except that I can't judge about the "S" constraint. >> > > This constraint does not work for arm32. Any other thoughts? > > Another way, as Jan suggested, is to pass the function as a 'fake' > (unused) input. > I 'm suspecting something like the following could work > asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory") > What do you think? Well, yes, X should always be a fallback option. But I said already when you suggested S that I can't judge about its use, so I guess I'm the wrong one to ask. Even more so that you only say "does not work", without any details ... Jan
On 7/25/22 09:32, Jan Beulich wrote: > On 24.07.2022 19:20, Xenia Ragiadakou wrote: >> On 7/7/22 10:55, Jan Beulich wrote: >>> On 07.07.2022 09:27, Xenia Ragiadakou wrote: >>>> On 7/6/22 11:51, Jan Beulich wrote: >>>>> On 06.07.2022 10:43, Xenia Ragiadakou wrote: >>>>>> On 7/6/22 10:10, Jan Beulich wrote: >>>>>>> On 05.07.2022 23:02, Xenia Ragiadakou wrote: >>>>>>>> The function idle_loop() is referenced only in domain.c. >>>>>>>> Change its linkage from external to internal by adding the storage-class >>>>>>>> specifier static to its definitions. >>>>>>>> >>>>>>>> Since idle_loop() is referenced only in inline assembly, add the 'used' >>>>>>>> attribute to suppress unused-function compiler warning. >>>>>>> >>>>>>> While I see that Julien has already acked the patch, I'd like to point >>>>>>> out that using __used here is somewhat bogus. Imo the better approach >>>>>>> is to properly make visible to the compiler that the symbol is used by >>>>>>> the asm(), by adding a fake(?) input. >>>>>> >>>>>> I 'm afraid I do not understand what do you mean by "adding a fake(?) >>>>>> input". Can you please elaborate a little on your suggestion? >>>>> >>>>> Once the asm() in question takes the function as an input, the compiler >>>>> will know that the function has a user (unless, of course, it finds a >>>>> way to elide the asm() itself). The "fake(?)" was because I'm not deeply >>>>> enough into Arm inline assembly to know whether the input could then >>>>> also be used as an instruction operand (which imo would be preferable) - >>>>> if it can't (e.g. because there's no suitable constraint or operand >>>>> modifier), it still can be an input just to inform the compiler. >>>> >>>> According to the following statement, your approach is the recommended one: >>>> "To prevent the compiler from removing global data or functions which >>>> are referenced from inline assembly statements, you can: >>>> -use __attribute__((used)) with the global data or functions. >>>> -pass the reference to global data or functions as operands to inline >>>> assembly statements. >>>> Arm recommends passing the reference to global data or functions as >>>> operands to inline assembly statements so that if the final image does >>>> not require the inline assembly statements and the referenced global >>>> data or function, then they can be removed." >>>> >>>> IIUC, you are suggesting to change >>>> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" ) >>>> into >>>> asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" ); >>> >>> Yes, except that I can't judge about the "S" constraint. >>> >> >> This constraint does not work for arm32. Any other thoughts? >> >> Another way, as Jan suggested, is to pass the function as a 'fake' >> (unused) input. >> I 'm suspecting something like the following could work >> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory") >> What do you think? > > Well, yes, X should always be a fallback option. But I said already when > you suggested S that I can't judge about its use, so I guess I'm the > wrong one to ask. Even more so that you only say "does not work", without > any details ... > The question is addressed to anyone familiar with arm inline assembly. I added the arm maintainers as primary recipients to this email to make this perfectly clear. When cross-compiling Xen on x86 for arm32 with asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" ); compilation fails with the error: impossible constraint in ‘asm'
On Mon, 25 Jul 2022, Xenia Ragiadakou wrote: > On 7/25/22 09:32, Jan Beulich wrote: > > On 24.07.2022 19:20, Xenia Ragiadakou wrote: > > > On 7/7/22 10:55, Jan Beulich wrote: > > > > On 07.07.2022 09:27, Xenia Ragiadakou wrote: > > > > > On 7/6/22 11:51, Jan Beulich wrote: > > > > > > On 06.07.2022 10:43, Xenia Ragiadakou wrote: > > > > > > > On 7/6/22 10:10, Jan Beulich wrote: > > > > > > > > On 05.07.2022 23:02, Xenia Ragiadakou wrote: > > > > > > > > > The function idle_loop() is referenced only in domain.c. > > > > > > > > > Change its linkage from external to internal by adding the > > > > > > > > > storage-class > > > > > > > > > specifier static to its definitions. > > > > > > > > > > > > > > > > > > Since idle_loop() is referenced only in inline assembly, add > > > > > > > > > the 'used' > > > > > > > > > attribute to suppress unused-function compiler warning. > > > > > > > > > > > > > > > > While I see that Julien has already acked the patch, I'd like to > > > > > > > > point > > > > > > > > out that using __used here is somewhat bogus. Imo the better > > > > > > > > approach > > > > > > > > is to properly make visible to the compiler that the symbol is > > > > > > > > used by > > > > > > > > the asm(), by adding a fake(?) input. > > > > > > > > > > > > > > I 'm afraid I do not understand what do you mean by "adding a > > > > > > > fake(?) > > > > > > > input". Can you please elaborate a little on your suggestion? > > > > > > > > > > > > Once the asm() in question takes the function as an input, the > > > > > > compiler > > > > > > will know that the function has a user (unless, of course, it finds > > > > > > a > > > > > > way to elide the asm() itself). The "fake(?)" was because I'm not > > > > > > deeply > > > > > > enough into Arm inline assembly to know whether the input could then > > > > > > also be used as an instruction operand (which imo would be > > > > > > preferable) - > > > > > > if it can't (e.g. because there's no suitable constraint or operand > > > > > > modifier), it still can be an input just to inform the compiler. > > > > > > > > > > According to the following statement, your approach is the recommended > > > > > one: > > > > > "To prevent the compiler from removing global data or functions which > > > > > are referenced from inline assembly statements, you can: > > > > > -use __attribute__((used)) with the global data or functions. > > > > > -pass the reference to global data or functions as operands to inline > > > > > assembly statements. > > > > > Arm recommends passing the reference to global data or functions as > > > > > operands to inline assembly statements so that if the final image does > > > > > not require the inline assembly statements and the referenced global > > > > > data or function, then they can be removed." > > > > > > > > > > IIUC, you are suggesting to change > > > > > asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" ) > > > > > into > > > > > asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" > > > > > ); > > > > > > > > Yes, except that I can't judge about the "S" constraint. > > > > > > > > > > This constraint does not work for arm32. Any other thoughts? > > > > > > Another way, as Jan suggested, is to pass the function as a 'fake' > > > (unused) input. > > > I 'm suspecting something like the following could work > > > asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : > > > "memory") > > > What do you think? > > > > Well, yes, X should always be a fallback option. But I said already when > > you suggested S that I can't judge about its use, so I guess I'm the > > wrong one to ask. Even more so that you only say "does not work", without > > any details ... > > > > The question is addressed to anyone familiar with arm inline assembly. > I added the arm maintainers as primary recipients to this email to make this > perfectly clear. > > When cross-compiling Xen on x86 for arm32 with > asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" ); > compilation fails with the error: impossible constraint in ‘asm' Unfortunately looking at the GCC manual pages [1], it doesn't seem to be possible. The problem is that the definition of "S" changes between aarch64 and arm (the 32-bit version). For aarch64: S An absolute symbolic address or a label reference This is what we want. For arm instead: S A symbol in the text segment of the current file This is not useful for what we need to do here. As far as I can tell, there is no other way in GCC assembly inline for arm to do this. So we have 2 choices: we use the __used keyword as Xenia did in this patch. Or we move the implementation of switch_stack_and_jump in assembly. I attempted a prototype of the latter to see how it would come out, see below. I don't like it very much. I prefer the version with __used that Xenia had in this patch. But I am OK either way. diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S index 38826142ad..4d28f7e9f7 100644 --- a/xen/arch/arm/arm32/entry.S +++ b/xen/arch/arm/arm32/entry.S @@ -442,6 +442,10 @@ ENTRY(__context_switch) add r4, r1, #VCPU_arch_saved_context ldmia r4, {r4 - sl, fp, sp, pc} /* Load registers and return */ +ENTRY(__switch_stack_and_jump) + mov sp, r0 + bx r1 + /* * Local variables: * mode: ASM diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S index 95f1a92684..5d5d713f80 100644 --- a/xen/arch/arm/arm64/entry.S +++ b/xen/arch/arm/arm64/entry.S @@ -618,6 +618,10 @@ ENTRY(__context_switch) mov sp, x9 ret +ENTRY(__switch_stack_and_jump) + mov sp, x0 + br x1 + /* * Local variables: * mode: ASM diff --git a/xen/arch/arm/include/asm/current.h b/xen/arch/arm/include/asm/current.h index 73e81458e5..7696440a57 100644 --- a/xen/arch/arm/include/asm/current.h +++ b/xen/arch/arm/include/asm/current.h @@ -44,8 +44,12 @@ static inline struct cpu_info *get_cpu_info(void) #define guest_cpu_user_regs() (&get_cpu_info()->guest_cpu_user_regs) +void return_to_new_vcpu32(void); +void return_to_new_vcpu64(void); +void __switch_stack_and_jump(void *p, void *f); + #define switch_stack_and_jump(stack, fn) do { \ - asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" ); \ + __switch_stack_and_jump(stack, fn); \ unreachable(); \ } while ( false ) [1] https://gcc.gnu.org/onlinedocs/gcc/Machine-Constraints.html
On 26.07.2022 02:33, Stefano Stabellini wrote: > On Mon, 25 Jul 2022, Xenia Ragiadakou wrote: >> On 7/25/22 09:32, Jan Beulich wrote: >>> On 24.07.2022 19:20, Xenia Ragiadakou wrote: >>>> On 7/7/22 10:55, Jan Beulich wrote: >>>>> On 07.07.2022 09:27, Xenia Ragiadakou wrote: >>>>>> On 7/6/22 11:51, Jan Beulich wrote: >>>>>>> On 06.07.2022 10:43, Xenia Ragiadakou wrote: >>>>>>>> On 7/6/22 10:10, Jan Beulich wrote: >>>>>>>>> On 05.07.2022 23:02, Xenia Ragiadakou wrote: >>>>>>>>>> The function idle_loop() is referenced only in domain.c. >>>>>>>>>> Change its linkage from external to internal by adding the >>>>>>>>>> storage-class >>>>>>>>>> specifier static to its definitions. >>>>>>>>>> >>>>>>>>>> Since idle_loop() is referenced only in inline assembly, add >>>>>>>>>> the 'used' >>>>>>>>>> attribute to suppress unused-function compiler warning. >>>>>>>>> >>>>>>>>> While I see that Julien has already acked the patch, I'd like to >>>>>>>>> point >>>>>>>>> out that using __used here is somewhat bogus. Imo the better >>>>>>>>> approach >>>>>>>>> is to properly make visible to the compiler that the symbol is >>>>>>>>> used by >>>>>>>>> the asm(), by adding a fake(?) input. >>>>>>>> >>>>>>>> I 'm afraid I do not understand what do you mean by "adding a >>>>>>>> fake(?) >>>>>>>> input". Can you please elaborate a little on your suggestion? >>>>>>> >>>>>>> Once the asm() in question takes the function as an input, the >>>>>>> compiler >>>>>>> will know that the function has a user (unless, of course, it finds >>>>>>> a >>>>>>> way to elide the asm() itself). The "fake(?)" was because I'm not >>>>>>> deeply >>>>>>> enough into Arm inline assembly to know whether the input could then >>>>>>> also be used as an instruction operand (which imo would be >>>>>>> preferable) - >>>>>>> if it can't (e.g. because there's no suitable constraint or operand >>>>>>> modifier), it still can be an input just to inform the compiler. >>>>>> >>>>>> According to the following statement, your approach is the recommended >>>>>> one: >>>>>> "To prevent the compiler from removing global data or functions which >>>>>> are referenced from inline assembly statements, you can: >>>>>> -use __attribute__((used)) with the global data or functions. >>>>>> -pass the reference to global data or functions as operands to inline >>>>>> assembly statements. >>>>>> Arm recommends passing the reference to global data or functions as >>>>>> operands to inline assembly statements so that if the final image does >>>>>> not require the inline assembly statements and the referenced global >>>>>> data or function, then they can be removed." >>>>>> >>>>>> IIUC, you are suggesting to change >>>>>> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" ) >>>>>> into >>>>>> asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" >>>>>> ); >>>>> >>>>> Yes, except that I can't judge about the "S" constraint. >>>>> >>>> >>>> This constraint does not work for arm32. Any other thoughts? >>>> >>>> Another way, as Jan suggested, is to pass the function as a 'fake' >>>> (unused) input. >>>> I 'm suspecting something like the following could work >>>> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : >>>> "memory") >>>> What do you think? >>> >>> Well, yes, X should always be a fallback option. But I said already when >>> you suggested S that I can't judge about its use, so I guess I'm the >>> wrong one to ask. Even more so that you only say "does not work", without >>> any details ... >>> >> >> The question is addressed to anyone familiar with arm inline assembly. >> I added the arm maintainers as primary recipients to this email to make this >> perfectly clear. >> >> When cross-compiling Xen on x86 for arm32 with >> asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" ); >> compilation fails with the error: impossible constraint in ‘asm' > > Unfortunately looking at the GCC manual pages [1], it doesn't seem to be > possible. The problem is that the definition of "S" changes between > aarch64 and arm (the 32-bit version). > > For aarch64: > > S An absolute symbolic address or a label reference > > This is what we want. For arm instead: > > S A symbol in the text segment of the current file > > This is not useful for what we need to do here. As far as I can tell, > there is no other way in GCC assembly inline for arm to do this. > > So we have 2 choices: we use the __used keyword as Xenia did in this > patch. Or we move the implementation of switch_stack_and_jump in > assembly. I attempted a prototype of the latter to see how it would come > out, see below. > > I don't like it very much. I prefer the version with __used that Xenia > had in this patch. But I am OK either way. You forgot the imo better intermediate option of using the "X" constraint. Jan
On Tue, 26 Jul 2022, Jan Beulich wrote: > On 26.07.2022 02:33, Stefano Stabellini wrote: > > On Mon, 25 Jul 2022, Xenia Ragiadakou wrote: > >> On 7/25/22 09:32, Jan Beulich wrote: > >>> On 24.07.2022 19:20, Xenia Ragiadakou wrote: > >>>> On 7/7/22 10:55, Jan Beulich wrote: > >>>>> On 07.07.2022 09:27, Xenia Ragiadakou wrote: > >>>>>> On 7/6/22 11:51, Jan Beulich wrote: > >>>>>>> On 06.07.2022 10:43, Xenia Ragiadakou wrote: > >>>>>>>> On 7/6/22 10:10, Jan Beulich wrote: > >>>>>>>>> On 05.07.2022 23:02, Xenia Ragiadakou wrote: > >>>>>>>>>> The function idle_loop() is referenced only in domain.c. > >>>>>>>>>> Change its linkage from external to internal by adding the > >>>>>>>>>> storage-class > >>>>>>>>>> specifier static to its definitions. > >>>>>>>>>> > >>>>>>>>>> Since idle_loop() is referenced only in inline assembly, add > >>>>>>>>>> the 'used' > >>>>>>>>>> attribute to suppress unused-function compiler warning. > >>>>>>>>> > >>>>>>>>> While I see that Julien has already acked the patch, I'd like to > >>>>>>>>> point > >>>>>>>>> out that using __used here is somewhat bogus. Imo the better > >>>>>>>>> approach > >>>>>>>>> is to properly make visible to the compiler that the symbol is > >>>>>>>>> used by > >>>>>>>>> the asm(), by adding a fake(?) input. > >>>>>>>> > >>>>>>>> I 'm afraid I do not understand what do you mean by "adding a > >>>>>>>> fake(?) > >>>>>>>> input". Can you please elaborate a little on your suggestion? > >>>>>>> > >>>>>>> Once the asm() in question takes the function as an input, the > >>>>>>> compiler > >>>>>>> will know that the function has a user (unless, of course, it finds > >>>>>>> a > >>>>>>> way to elide the asm() itself). The "fake(?)" was because I'm not > >>>>>>> deeply > >>>>>>> enough into Arm inline assembly to know whether the input could then > >>>>>>> also be used as an instruction operand (which imo would be > >>>>>>> preferable) - > >>>>>>> if it can't (e.g. because there's no suitable constraint or operand > >>>>>>> modifier), it still can be an input just to inform the compiler. > >>>>>> > >>>>>> According to the following statement, your approach is the recommended > >>>>>> one: > >>>>>> "To prevent the compiler from removing global data or functions which > >>>>>> are referenced from inline assembly statements, you can: > >>>>>> -use __attribute__((used)) with the global data or functions. > >>>>>> -pass the reference to global data or functions as operands to inline > >>>>>> assembly statements. > >>>>>> Arm recommends passing the reference to global data or functions as > >>>>>> operands to inline assembly statements so that if the final image does > >>>>>> not require the inline assembly statements and the referenced global > >>>>>> data or function, then they can be removed." > >>>>>> > >>>>>> IIUC, you are suggesting to change > >>>>>> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" ) > >>>>>> into > >>>>>> asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" > >>>>>> ); > >>>>> > >>>>> Yes, except that I can't judge about the "S" constraint. > >>>>> > >>>> > >>>> This constraint does not work for arm32. Any other thoughts? > >>>> > >>>> Another way, as Jan suggested, is to pass the function as a 'fake' > >>>> (unused) input. > >>>> I 'm suspecting something like the following could work > >>>> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : > >>>> "memory") > >>>> What do you think? > >>> > >>> Well, yes, X should always be a fallback option. But I said already when > >>> you suggested S that I can't judge about its use, so I guess I'm the > >>> wrong one to ask. Even more so that you only say "does not work", without > >>> any details ... > >>> > >> > >> The question is addressed to anyone familiar with arm inline assembly. > >> I added the arm maintainers as primary recipients to this email to make this > >> perfectly clear. > >> > >> When cross-compiling Xen on x86 for arm32 with > >> asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" ); > >> compilation fails with the error: impossible constraint in ‘asm' > > > > Unfortunately looking at the GCC manual pages [1], it doesn't seem to be > > possible. The problem is that the definition of "S" changes between > > aarch64 and arm (the 32-bit version). > > > > For aarch64: > > > > S An absolute symbolic address or a label reference > > > > This is what we want. For arm instead: > > > > S A symbol in the text segment of the current file > > > > This is not useful for what we need to do here. As far as I can tell, > > there is no other way in GCC assembly inline for arm to do this. > > > > So we have 2 choices: we use the __used keyword as Xenia did in this > > patch. Or we move the implementation of switch_stack_and_jump in > > assembly. I attempted a prototype of the latter to see how it would come > > out, see below. > > > > I don't like it very much. I prefer the version with __used that Xenia > > had in this patch. But I am OK either way. > > You forgot the imo better intermediate option of using the "X" constraint. I couldn't get "X" to compile in any way (not even for arm64). Do you have a concrete example that you think should work using "X" as constraint?
On 7/27/22 03:10, Stefano Stabellini wrote: > On Tue, 26 Jul 2022, Jan Beulich wrote: >> On 26.07.2022 02:33, Stefano Stabellini wrote: >>> On Mon, 25 Jul 2022, Xenia Ragiadakou wrote: >>>> On 7/25/22 09:32, Jan Beulich wrote: >>>>> On 24.07.2022 19:20, Xenia Ragiadakou wrote: >>>>>> On 7/7/22 10:55, Jan Beulich wrote: >>>>>>> On 07.07.2022 09:27, Xenia Ragiadakou wrote: >>>>>>>> On 7/6/22 11:51, Jan Beulich wrote: >>>>>>>>> On 06.07.2022 10:43, Xenia Ragiadakou wrote: >>>>>>>>>> On 7/6/22 10:10, Jan Beulich wrote: >>>>>>>>>>> On 05.07.2022 23:02, Xenia Ragiadakou wrote: >>>>>>>>>>>> The function idle_loop() is referenced only in domain.c. >>>>>>>>>>>> Change its linkage from external to internal by adding the >>>>>>>>>>>> storage-class >>>>>>>>>>>> specifier static to its definitions. >>>>>>>>>>>> >>>>>>>>>>>> Since idle_loop() is referenced only in inline assembly, add >>>>>>>>>>>> the 'used' >>>>>>>>>>>> attribute to suppress unused-function compiler warning. >>>>>>>>>>> >>>>>>>>>>> While I see that Julien has already acked the patch, I'd like to >>>>>>>>>>> point >>>>>>>>>>> out that using __used here is somewhat bogus. Imo the better >>>>>>>>>>> approach >>>>>>>>>>> is to properly make visible to the compiler that the symbol is >>>>>>>>>>> used by >>>>>>>>>>> the asm(), by adding a fake(?) input. >>>>>>>>>> >>>>>>>>>> I 'm afraid I do not understand what do you mean by "adding a >>>>>>>>>> fake(?) >>>>>>>>>> input". Can you please elaborate a little on your suggestion? >>>>>>>>> >>>>>>>>> Once the asm() in question takes the function as an input, the >>>>>>>>> compiler >>>>>>>>> will know that the function has a user (unless, of course, it finds >>>>>>>>> a >>>>>>>>> way to elide the asm() itself). The "fake(?)" was because I'm not >>>>>>>>> deeply >>>>>>>>> enough into Arm inline assembly to know whether the input could then >>>>>>>>> also be used as an instruction operand (which imo would be >>>>>>>>> preferable) - >>>>>>>>> if it can't (e.g. because there's no suitable constraint or operand >>>>>>>>> modifier), it still can be an input just to inform the compiler. >>>>>>>> >>>>>>>> According to the following statement, your approach is the recommended >>>>>>>> one: >>>>>>>> "To prevent the compiler from removing global data or functions which >>>>>>>> are referenced from inline assembly statements, you can: >>>>>>>> -use __attribute__((used)) with the global data or functions. >>>>>>>> -pass the reference to global data or functions as operands to inline >>>>>>>> assembly statements. >>>>>>>> Arm recommends passing the reference to global data or functions as >>>>>>>> operands to inline assembly statements so that if the final image does >>>>>>>> not require the inline assembly statements and the referenced global >>>>>>>> data or function, then they can be removed." >>>>>>>> >>>>>>>> IIUC, you are suggesting to change >>>>>>>> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" ) >>>>>>>> into >>>>>>>> asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" >>>>>>>> ); >>>>>>> >>>>>>> Yes, except that I can't judge about the "S" constraint. >>>>>>> >>>>>> >>>>>> This constraint does not work for arm32. Any other thoughts? >>>>>> >>>>>> Another way, as Jan suggested, is to pass the function as a 'fake' >>>>>> (unused) input. >>>>>> I 'm suspecting something like the following could work >>>>>> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : >>>>>> "memory") >>>>>> What do you think? >>>>> >>>>> Well, yes, X should always be a fallback option. But I said already when >>>>> you suggested S that I can't judge about its use, so I guess I'm the >>>>> wrong one to ask. Even more so that you only say "does not work", without >>>>> any details ... >>>>> >>>> >>>> The question is addressed to anyone familiar with arm inline assembly. >>>> I added the arm maintainers as primary recipients to this email to make this >>>> perfectly clear. >>>> >>>> When cross-compiling Xen on x86 for arm32 with >>>> asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" ); >>>> compilation fails with the error: impossible constraint in ‘asm' >>> >>> Unfortunately looking at the GCC manual pages [1], it doesn't seem to be >>> possible. The problem is that the definition of "S" changes between >>> aarch64 and arm (the 32-bit version). >>> >>> For aarch64: >>> >>> S An absolute symbolic address or a label reference >>> >>> This is what we want. For arm instead: >>> >>> S A symbol in the text segment of the current file >>> >>> This is not useful for what we need to do here. As far as I can tell, >>> there is no other way in GCC assembly inline for arm to do this. >>> >>> So we have 2 choices: we use the __used keyword as Xenia did in this >>> patch. Or we move the implementation of switch_stack_and_jump in >>> assembly. I attempted a prototype of the latter to see how it would come >>> out, see below. >>> >>> I don't like it very much. I prefer the version with __used that Xenia >>> had in this patch. But I am OK either way. >> >> You forgot the imo better intermediate option of using the "X" constraint. > > I couldn't get "X" to compile in any way (not even for arm64). Do you > have a concrete example that you think should work using "X" as > constraint? I proposed the X constraint for the case that the function is used as a fake (unused) input operand to the inline asm. For instance, in the example below, the function is listed in the input operands but there is not corresponding reference to it. asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory" );
On 27.07.2022 02:10, Stefano Stabellini wrote: > On Tue, 26 Jul 2022, Jan Beulich wrote: >> You forgot the imo better intermediate option of using the "X" constraint. > > I couldn't get "X" to compile in any way (not even for arm64). Do you > have a concrete example that you think should work using "X" as > constraint? Perhaps you tried to use the respective input then as an operand to the insn? That won't work afaik - as Xenia says, it can be used only as a "fake" operand (i.e. one that tells the compiler something, but having no direct meaning for the insn). Actually I thought we had uses of "X" already somewhere in Xen and/or XTF, but now that I looked I can't find any. (Anymore?) Jan
On Wed, 27 Jul 2022, Xenia Ragiadakou wrote: > On 7/27/22 03:10, Stefano Stabellini wrote: > > On Tue, 26 Jul 2022, Jan Beulich wrote: > > > On 26.07.2022 02:33, Stefano Stabellini wrote: > > > > On Mon, 25 Jul 2022, Xenia Ragiadakou wrote: > > > > > On 7/25/22 09:32, Jan Beulich wrote: > > > > > > On 24.07.2022 19:20, Xenia Ragiadakou wrote: > > > > > > > On 7/7/22 10:55, Jan Beulich wrote: > > > > > > > > On 07.07.2022 09:27, Xenia Ragiadakou wrote: > > > > > > > > > On 7/6/22 11:51, Jan Beulich wrote: > > > > > > > > > > On 06.07.2022 10:43, Xenia Ragiadakou wrote: > > > > > > > > > > > On 7/6/22 10:10, Jan Beulich wrote: > > > > > > > > > > > > On 05.07.2022 23:02, Xenia Ragiadakou wrote: > > > > > > > > > > > > > The function idle_loop() is referenced only in > > > > > > > > > > > > > domain.c. > > > > > > > > > > > > > Change its linkage from external to internal by adding > > > > > > > > > > > > > the > > > > > > > > > > > > > storage-class > > > > > > > > > > > > > specifier static to its definitions. > > > > > > > > > > > > > > > > > > > > > > > > > > Since idle_loop() is referenced only in inline > > > > > > > > > > > > > assembly, add > > > > > > > > > > > > > the 'used' > > > > > > > > > > > > > attribute to suppress unused-function compiler > > > > > > > > > > > > > warning. > > > > > > > > > > > > > > > > > > > > > > > > While I see that Julien has already acked the patch, I'd > > > > > > > > > > > > like to > > > > > > > > > > > > point > > > > > > > > > > > > out that using __used here is somewhat bogus. Imo the > > > > > > > > > > > > better > > > > > > > > > > > > approach > > > > > > > > > > > > is to properly make visible to the compiler that the > > > > > > > > > > > > symbol is > > > > > > > > > > > > used by > > > > > > > > > > > > the asm(), by adding a fake(?) input. > > > > > > > > > > > > > > > > > > > > > > I 'm afraid I do not understand what do you mean by > > > > > > > > > > > "adding a > > > > > > > > > > > fake(?) > > > > > > > > > > > input". Can you please elaborate a little on your > > > > > > > > > > > suggestion? > > > > > > > > > > > > > > > > > > > > Once the asm() in question takes the function as an input, > > > > > > > > > > the > > > > > > > > > > compiler > > > > > > > > > > will know that the function has a user (unless, of course, > > > > > > > > > > it finds > > > > > > > > > > a > > > > > > > > > > way to elide the asm() itself). The "fake(?)" was because > > > > > > > > > > I'm not > > > > > > > > > > deeply > > > > > > > > > > enough into Arm inline assembly to know whether the input > > > > > > > > > > could then > > > > > > > > > > also be used as an instruction operand (which imo would be > > > > > > > > > > preferable) - > > > > > > > > > > if it can't (e.g. because there's no suitable constraint or > > > > > > > > > > operand > > > > > > > > > > modifier), it still can be an input just to inform the > > > > > > > > > > compiler. > > > > > > > > > > > > > > > > > > According to the following statement, your approach is the > > > > > > > > > recommended > > > > > > > > > one: > > > > > > > > > "To prevent the compiler from removing global data or > > > > > > > > > functions which > > > > > > > > > are referenced from inline assembly statements, you can: > > > > > > > > > -use __attribute__((used)) with the global data or functions. > > > > > > > > > -pass the reference to global data or functions as operands to > > > > > > > > > inline > > > > > > > > > assembly statements. > > > > > > > > > Arm recommends passing the reference to global data or > > > > > > > > > functions as > > > > > > > > > operands to inline assembly statements so that if the final > > > > > > > > > image does > > > > > > > > > not require the inline assembly statements and the referenced > > > > > > > > > global > > > > > > > > > data or function, then they can be removed." > > > > > > > > > > > > > > > > > > IIUC, you are suggesting to change > > > > > > > > > asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : > > > > > > > > > "memory" ) > > > > > > > > > into > > > > > > > > > asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : > > > > > > > > > "memory" > > > > > > > > > ); > > > > > > > > > > > > > > > > Yes, except that I can't judge about the "S" constraint. > > > > > > > > > > > > > > > > > > > > > > This constraint does not work for arm32. Any other thoughts? > > > > > > > > > > > > > > Another way, as Jan suggested, is to pass the function as a 'fake' > > > > > > > (unused) input. > > > > > > > I 'm suspecting something like the following could work > > > > > > > asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : > > > > > > > "memory") > > > > > > > What do you think? > > > > > > > > > > > > Well, yes, X should always be a fallback option. But I said already > > > > > > when > > > > > > you suggested S that I can't judge about its use, so I guess I'm the > > > > > > wrong one to ask. Even more so that you only say "does not work", > > > > > > without > > > > > > any details ... > > > > > > > > > > > > > > > > The question is addressed to anyone familiar with arm inline assembly. > > > > > I added the arm maintainers as primary recipients to this email to > > > > > make this > > > > > perfectly clear. > > > > > > > > > > When cross-compiling Xen on x86 for arm32 with > > > > > asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" > > > > > ); > > > > > compilation fails with the error: impossible constraint in ‘asm' > > > > > > > > Unfortunately looking at the GCC manual pages [1], it doesn't seem to be > > > > possible. The problem is that the definition of "S" changes between > > > > aarch64 and arm (the 32-bit version). > > > > > > > > For aarch64: > > > > > > > > S An absolute symbolic address or a label reference > > > > > > > > This is what we want. For arm instead: > > > > > > > > S A symbol in the text segment of the current file > > > > > > > > This is not useful for what we need to do here. As far as I can tell, > > > > there is no other way in GCC assembly inline for arm to do this. > > > > > > > > So we have 2 choices: we use the __used keyword as Xenia did in this > > > > patch. Or we move the implementation of switch_stack_and_jump in > > > > assembly. I attempted a prototype of the latter to see how it would come > > > > out, see below. > > > > > > > > I don't like it very much. I prefer the version with __used that Xenia > > > > had in this patch. But I am OK either way. > > > > > > You forgot the imo better intermediate option of using the "X" constraint. > > > > I couldn't get "X" to compile in any way (not even for arm64). Do you > > have a concrete example that you think should work using "X" as > > constraint? > > I proposed the X constraint for the case that the function is used as a fake > (unused) input operand to the inline asm. > For instance, in the example below, the function is listed in the input > operands but there is not corresponding reference to it. > > asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory" ); Also replying to Jan, yes, this doesn't build for me, not even for arm64. The error is below. arch/arm/domain.c: In function ‘continue_new_vcpu’: arch/arm/domain.c:345:30: error: ‘return_to_new_vcpu32’ undeclared (first use in this function) 345 | reset_stack_and_jump(return_to_new_vcpu32); | ^~~~~~~~~~~~~~~~~~~~ ./arch/arm/include/asm/current.h:48:65: note: in definition of macro ‘switch_stack_and_jump’ 48 | asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory" ); \ | ^~ arch/arm/domain.c:345:9: note: in expansion of macro ‘reset_stack_and_jump’ 345 | reset_stack_and_jump(return_to_new_vcpu32); | ^~~~~~~~~~~~~~~~~~~~ arch/arm/domain.c:345:30: note: each undeclared identifier is reported only once for each function it appears in 345 | reset_stack_and_jump(return_to_new_vcpu32); | ^~~~~~~~~~~~~~~~~~~~ ./arch/arm/include/asm/current.h:48:65: note: in definition of macro ‘switch_stack_and_jump’ 48 | asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory" ); \ | ^~ arch/arm/domain.c:345:9: note: in expansion of macro ‘reset_stack_and_jump’ 345 | reset_stack_and_jump(return_to_new_vcpu32); | ^~~~~~~~~~~~~~~~~~~~ CC arch/arm/domain_build.o arch/arm/domain.c:348:30: error: ‘return_to_new_vcpu64’ undeclared (first use in this function) 348 | reset_stack_and_jump(return_to_new_vcpu64); | ^~~~~~~~~~~~~~~~~~~~ ./arch/arm/include/asm/current.h:48:65: note: in definition of macro ‘switch_stack_and_jump’ 48 | asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory" ); \ | ^~ arch/arm/domain.c:348:9: note: in expansion of macro ‘reset_stack_and_jump’ 348 | reset_stack_and_jump(return_to_new_vcpu64); | ^~~~~~~~~~~~~~~~~~~~ make[2]: *** [Rules.mk:246: arch/arm/domain.o] Error 1
Hi Stefano, On 7/27/22 23:26, Stefano Stabellini wrote: > On Wed, 27 Jul 2022, Xenia Ragiadakou wrote: >> On 7/27/22 03:10, Stefano Stabellini wrote: >>> On Tue, 26 Jul 2022, Jan Beulich wrote: >>>> On 26.07.2022 02:33, Stefano Stabellini wrote: >>>>> On Mon, 25 Jul 2022, Xenia Ragiadakou wrote: >>>>>> On 7/25/22 09:32, Jan Beulich wrote: >>>>>>> On 24.07.2022 19:20, Xenia Ragiadakou wrote: >>>>>>>> On 7/7/22 10:55, Jan Beulich wrote: >>>>>>>>> On 07.07.2022 09:27, Xenia Ragiadakou wrote: >>>>>>>>>> On 7/6/22 11:51, Jan Beulich wrote: >>>>>>>>>>> On 06.07.2022 10:43, Xenia Ragiadakou wrote: >>>>>>>>>>>> On 7/6/22 10:10, Jan Beulich wrote: >>>>>>>>>>>>> On 05.07.2022 23:02, Xenia Ragiadakou wrote: >>>>>>>>>>>>>> The function idle_loop() is referenced only in >>>>>>>>>>>>>> domain.c. >>>>>>>>>>>>>> Change its linkage from external to internal by adding >>>>>>>>>>>>>> the >>>>>>>>>>>>>> storage-class >>>>>>>>>>>>>> specifier static to its definitions. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Since idle_loop() is referenced only in inline >>>>>>>>>>>>>> assembly, add >>>>>>>>>>>>>> the 'used' >>>>>>>>>>>>>> attribute to suppress unused-function compiler >>>>>>>>>>>>>> warning. >>>>>>>>>>>>> >>>>>>>>>>>>> While I see that Julien has already acked the patch, I'd >>>>>>>>>>>>> like to >>>>>>>>>>>>> point >>>>>>>>>>>>> out that using __used here is somewhat bogus. Imo the >>>>>>>>>>>>> better >>>>>>>>>>>>> approach >>>>>>>>>>>>> is to properly make visible to the compiler that the >>>>>>>>>>>>> symbol is >>>>>>>>>>>>> used by >>>>>>>>>>>>> the asm(), by adding a fake(?) input. >>>>>>>>>>>> >>>>>>>>>>>> I 'm afraid I do not understand what do you mean by >>>>>>>>>>>> "adding a >>>>>>>>>>>> fake(?) >>>>>>>>>>>> input". Can you please elaborate a little on your >>>>>>>>>>>> suggestion? >>>>>>>>>>> >>>>>>>>>>> Once the asm() in question takes the function as an input, >>>>>>>>>>> the >>>>>>>>>>> compiler >>>>>>>>>>> will know that the function has a user (unless, of course, >>>>>>>>>>> it finds >>>>>>>>>>> a >>>>>>>>>>> way to elide the asm() itself). The "fake(?)" was because >>>>>>>>>>> I'm not >>>>>>>>>>> deeply >>>>>>>>>>> enough into Arm inline assembly to know whether the input >>>>>>>>>>> could then >>>>>>>>>>> also be used as an instruction operand (which imo would be >>>>>>>>>>> preferable) - >>>>>>>>>>> if it can't (e.g. because there's no suitable constraint or >>>>>>>>>>> operand >>>>>>>>>>> modifier), it still can be an input just to inform the >>>>>>>>>>> compiler. >>>>>>>>>> >>>>>>>>>> According to the following statement, your approach is the >>>>>>>>>> recommended >>>>>>>>>> one: >>>>>>>>>> "To prevent the compiler from removing global data or >>>>>>>>>> functions which >>>>>>>>>> are referenced from inline assembly statements, you can: >>>>>>>>>> -use __attribute__((used)) with the global data or functions. >>>>>>>>>> -pass the reference to global data or functions as operands to >>>>>>>>>> inline >>>>>>>>>> assembly statements. >>>>>>>>>> Arm recommends passing the reference to global data or >>>>>>>>>> functions as >>>>>>>>>> operands to inline assembly statements so that if the final >>>>>>>>>> image does >>>>>>>>>> not require the inline assembly statements and the referenced >>>>>>>>>> global >>>>>>>>>> data or function, then they can be removed." >>>>>>>>>> >>>>>>>>>> IIUC, you are suggesting to change >>>>>>>>>> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : >>>>>>>>>> "memory" ) >>>>>>>>>> into >>>>>>>>>> asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : >>>>>>>>>> "memory" >>>>>>>>>> ); >>>>>>>>> >>>>>>>>> Yes, except that I can't judge about the "S" constraint. >>>>>>>>> >>>>>>>> >>>>>>>> This constraint does not work for arm32. Any other thoughts? >>>>>>>> >>>>>>>> Another way, as Jan suggested, is to pass the function as a 'fake' >>>>>>>> (unused) input. >>>>>>>> I 'm suspecting something like the following could work >>>>>>>> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : >>>>>>>> "memory") >>>>>>>> What do you think? >>>>>>> >>>>>>> Well, yes, X should always be a fallback option. But I said already >>>>>>> when >>>>>>> you suggested S that I can't judge about its use, so I guess I'm the >>>>>>> wrong one to ask. Even more so that you only say "does not work", >>>>>>> without >>>>>>> any details ... >>>>>>> >>>>>> >>>>>> The question is addressed to anyone familiar with arm inline assembly. >>>>>> I added the arm maintainers as primary recipients to this email to >>>>>> make this >>>>>> perfectly clear. >>>>>> >>>>>> When cross-compiling Xen on x86 for arm32 with >>>>>> asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : "memory" >>>>>> ); >>>>>> compilation fails with the error: impossible constraint in ‘asm' >>>>> >>>>> Unfortunately looking at the GCC manual pages [1], it doesn't seem to be >>>>> possible. The problem is that the definition of "S" changes between >>>>> aarch64 and arm (the 32-bit version). >>>>> >>>>> For aarch64: >>>>> >>>>> S An absolute symbolic address or a label reference >>>>> >>>>> This is what we want. For arm instead: >>>>> >>>>> S A symbol in the text segment of the current file >>>>> >>>>> This is not useful for what we need to do here. As far as I can tell, >>>>> there is no other way in GCC assembly inline for arm to do this. >>>>> >>>>> So we have 2 choices: we use the __used keyword as Xenia did in this >>>>> patch. Or we move the implementation of switch_stack_and_jump in >>>>> assembly. I attempted a prototype of the latter to see how it would come >>>>> out, see below. >>>>> >>>>> I don't like it very much. I prefer the version with __used that Xenia >>>>> had in this patch. But I am OK either way. >>>> >>>> You forgot the imo better intermediate option of using the "X" constraint. >>> >>> I couldn't get "X" to compile in any way (not even for arm64). Do you >>> have a concrete example that you think should work using "X" as >>> constraint? >> >> I proposed the X constraint for the case that the function is used as a fake >> (unused) input operand to the inline asm. >> For instance, in the example below, the function is listed in the input >> operands but there is not corresponding reference to it. >> >> asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory" ); > > > Also replying to Jan, yes, this doesn't build for me, not even for > arm64. The error is below. > > > arch/arm/domain.c: In function ‘continue_new_vcpu’: > arch/arm/domain.c:345:30: error: ‘return_to_new_vcpu32’ undeclared (first use in this function) > 345 | reset_stack_and_jump(return_to_new_vcpu32); > | ^~~~~~~~~~~~~~~~~~~~ > ./arch/arm/include/asm/current.h:48:65: note: in definition of macro ‘switch_stack_and_jump’ > 48 | asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory" ); \ > | ^~ > arch/arm/domain.c:345:9: note: in expansion of macro ‘reset_stack_and_jump’ > 345 | reset_stack_and_jump(return_to_new_vcpu32); > | ^~~~~~~~~~~~~~~~~~~~ > arch/arm/domain.c:345:30: note: each undeclared identifier is reported only once for each function it appears in > 345 | reset_stack_and_jump(return_to_new_vcpu32); > | ^~~~~~~~~~~~~~~~~~~~ > ./arch/arm/include/asm/current.h:48:65: note: in definition of macro ‘switch_stack_and_jump’ > 48 | asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory" ); \ > | ^~ > arch/arm/domain.c:345:9: note: in expansion of macro ‘reset_stack_and_jump’ > 345 | reset_stack_and_jump(return_to_new_vcpu32); > | ^~~~~~~~~~~~~~~~~~~~ > CC arch/arm/domain_build.o > arch/arm/domain.c:348:30: error: ‘return_to_new_vcpu64’ undeclared (first use in this function) > 348 | reset_stack_and_jump(return_to_new_vcpu64); > | ^~~~~~~~~~~~~~~~~~~~ > ./arch/arm/include/asm/current.h:48:65: note: in definition of macro ‘switch_stack_and_jump’ > 48 | asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory" ); \ > | ^~ > arch/arm/domain.c:348:9: note: in expansion of macro ‘reset_stack_and_jump’ > 348 | reset_stack_and_jump(return_to_new_vcpu64); > | ^~~~~~~~~~~~~~~~~~~~ > make[2]: *** [Rules.mk:246: arch/arm/domain.o] Error 1 With this change, the compiler becomes aware that the functions idle_loop, return_to_new_vcpu32 and return_to_new_vcpu64 are used by the inline assembly. For idle loop, there is a previous declaration but for the other two there is not and when the compiler encounters their references complains. So, for this to compile, it is also required to declare return_to_new_vcpu32 and return_to_new_vcpu64.
On Thu, 28 Jul 2022, Xenia Ragiadakou wrote: > Hi Stefano, > > On 7/27/22 23:26, Stefano Stabellini wrote: > > On Wed, 27 Jul 2022, Xenia Ragiadakou wrote: > > > On 7/27/22 03:10, Stefano Stabellini wrote: > > > > On Tue, 26 Jul 2022, Jan Beulich wrote: > > > > > On 26.07.2022 02:33, Stefano Stabellini wrote: > > > > > > On Mon, 25 Jul 2022, Xenia Ragiadakou wrote: > > > > > > > On 7/25/22 09:32, Jan Beulich wrote: > > > > > > > > On 24.07.2022 19:20, Xenia Ragiadakou wrote: > > > > > > > > > On 7/7/22 10:55, Jan Beulich wrote: > > > > > > > > > > On 07.07.2022 09:27, Xenia Ragiadakou wrote: > > > > > > > > > > > On 7/6/22 11:51, Jan Beulich wrote: > > > > > > > > > > > > On 06.07.2022 10:43, Xenia Ragiadakou wrote: > > > > > > > > > > > > > On 7/6/22 10:10, Jan Beulich wrote: > > > > > > > > > > > > > > On 05.07.2022 23:02, Xenia Ragiadakou wrote: > > > > > > > > > > > > > > > The function idle_loop() is referenced only in > > > > > > > > > > > > > > > domain.c. > > > > > > > > > > > > > > > Change its linkage from external to internal by > > > > > > > > > > > > > > > adding > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > storage-class > > > > > > > > > > > > > > > specifier static to its definitions. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Since idle_loop() is referenced only in inline > > > > > > > > > > > > > > > assembly, add > > > > > > > > > > > > > > > the 'used' > > > > > > > > > > > > > > > attribute to suppress unused-function compiler > > > > > > > > > > > > > > > warning. > > > > > > > > > > > > > > > > > > > > > > > > > > > > While I see that Julien has already acked the patch, > > > > > > > > > > > > > > I'd > > > > > > > > > > > > > > like to > > > > > > > > > > > > > > point > > > > > > > > > > > > > > out that using __used here is somewhat bogus. Imo > > > > > > > > > > > > > > the > > > > > > > > > > > > > > better > > > > > > > > > > > > > > approach > > > > > > > > > > > > > > is to properly make visible to the compiler that the > > > > > > > > > > > > > > symbol is > > > > > > > > > > > > > > used by > > > > > > > > > > > > > > the asm(), by adding a fake(?) input. > > > > > > > > > > > > > > > > > > > > > > > > > > I 'm afraid I do not understand what do you mean by > > > > > > > > > > > > > "adding a > > > > > > > > > > > > > fake(?) > > > > > > > > > > > > > input". Can you please elaborate a little on your > > > > > > > > > > > > > suggestion? > > > > > > > > > > > > > > > > > > > > > > > > Once the asm() in question takes the function as an > > > > > > > > > > > > input, > > > > > > > > > > > > the > > > > > > > > > > > > compiler > > > > > > > > > > > > will know that the function has a user (unless, of > > > > > > > > > > > > course, > > > > > > > > > > > > it finds > > > > > > > > > > > > a > > > > > > > > > > > > way to elide the asm() itself). The "fake(?)" was > > > > > > > > > > > > because > > > > > > > > > > > > I'm not > > > > > > > > > > > > deeply > > > > > > > > > > > > enough into Arm inline assembly to know whether the > > > > > > > > > > > > input > > > > > > > > > > > > could then > > > > > > > > > > > > also be used as an instruction operand (which imo would > > > > > > > > > > > > be > > > > > > > > > > > > preferable) - > > > > > > > > > > > > if it can't (e.g. because there's no suitable constraint > > > > > > > > > > > > or > > > > > > > > > > > > operand > > > > > > > > > > > > modifier), it still can be an input just to inform the > > > > > > > > > > > > compiler. > > > > > > > > > > > > > > > > > > > > > > According to the following statement, your approach is the > > > > > > > > > > > recommended > > > > > > > > > > > one: > > > > > > > > > > > "To prevent the compiler from removing global data or > > > > > > > > > > > functions which > > > > > > > > > > > are referenced from inline assembly statements, you can: > > > > > > > > > > > -use __attribute__((used)) with the global data or > > > > > > > > > > > functions. > > > > > > > > > > > -pass the reference to global data or functions as > > > > > > > > > > > operands to > > > > > > > > > > > inline > > > > > > > > > > > assembly statements. > > > > > > > > > > > Arm recommends passing the reference to global data or > > > > > > > > > > > functions as > > > > > > > > > > > operands to inline assembly statements so that if the > > > > > > > > > > > final > > > > > > > > > > > image does > > > > > > > > > > > not require the inline assembly statements and the > > > > > > > > > > > referenced > > > > > > > > > > > global > > > > > > > > > > > data or function, then they can be removed." > > > > > > > > > > > > > > > > > > > > > > IIUC, you are suggesting to change > > > > > > > > > > > asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : > > > > > > > > > > > "memory" ) > > > > > > > > > > > into > > > > > > > > > > > asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) > > > > > > > > > > > : > > > > > > > > > > > "memory" > > > > > > > > > > > ); > > > > > > > > > > > > > > > > > > > > Yes, except that I can't judge about the "S" constraint. > > > > > > > > > > > > > > > > > > > > > > > > > > > > This constraint does not work for arm32. Any other thoughts? > > > > > > > > > > > > > > > > > > Another way, as Jan suggested, is to pass the function as a > > > > > > > > > 'fake' > > > > > > > > > (unused) input. > > > > > > > > > I 'm suspecting something like the following could work > > > > > > > > > asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" > > > > > > > > > (fn) : > > > > > > > > > "memory") > > > > > > > > > What do you think? > > > > > > > > > > > > > > > > Well, yes, X should always be a fallback option. But I said > > > > > > > > already > > > > > > > > when > > > > > > > > you suggested S that I can't judge about its use, so I guess I'm > > > > > > > > the > > > > > > > > wrong one to ask. Even more so that you only say "does not > > > > > > > > work", > > > > > > > > without > > > > > > > > any details ... > > > > > > > > > > > > > > > > > > > > > > The question is addressed to anyone familiar with arm inline > > > > > > > assembly. > > > > > > > I added the arm maintainers as primary recipients to this email to > > > > > > > make this > > > > > > > perfectly clear. > > > > > > > > > > > > > > When cross-compiling Xen on x86 for arm32 with > > > > > > > asm volatile ("mov sp,%0; b %1" : : "r" (stack), "S" (fn) : > > > > > > > "memory" > > > > > > > ); > > > > > > > compilation fails with the error: impossible constraint in ‘asm' > > > > > > > > > > > > Unfortunately looking at the GCC manual pages [1], it doesn't seem > > > > > > to be > > > > > > possible. The problem is that the definition of "S" changes between > > > > > > aarch64 and arm (the 32-bit version). > > > > > > > > > > > > For aarch64: > > > > > > > > > > > > S An absolute symbolic address or a label reference > > > > > > > > > > > > This is what we want. For arm instead: > > > > > > > > > > > > S A symbol in the text segment of the current file > > > > > > > > > > > > This is not useful for what we need to do here. As far as I can > > > > > > tell, > > > > > > there is no other way in GCC assembly inline for arm to do this. > > > > > > > > > > > > So we have 2 choices: we use the __used keyword as Xenia did in this > > > > > > patch. Or we move the implementation of switch_stack_and_jump in > > > > > > assembly. I attempted a prototype of the latter to see how it would > > > > > > come > > > > > > out, see below. > > > > > > > > > > > > I don't like it very much. I prefer the version with __used that > > > > > > Xenia > > > > > > had in this patch. But I am OK either way. > > > > > > > > > > You forgot the imo better intermediate option of using the "X" > > > > > constraint. > > > > > > > > I couldn't get "X" to compile in any way (not even for arm64). Do you > > > > have a concrete example that you think should work using "X" as > > > > constraint? > > > > > > I proposed the X constraint for the case that the function is used as a > > > fake > > > (unused) input operand to the inline asm. > > > For instance, in the example below, the function is listed in the input > > > operands but there is not corresponding reference to it. > > > > > > asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) : "memory" > > > ); > > > > > > Also replying to Jan, yes, this doesn't build for me, not even for > > arm64. The error is below. > > > > > > arch/arm/domain.c: In function ‘continue_new_vcpu’: > > arch/arm/domain.c:345:30: error: ‘return_to_new_vcpu32’ undeclared (first > > use in this function) > > 345 | reset_stack_and_jump(return_to_new_vcpu32); > > | ^~~~~~~~~~~~~~~~~~~~ > > ./arch/arm/include/asm/current.h:48:65: note: in definition of macro > > ‘switch_stack_and_jump’ > > 48 | asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) > > : "memory" ); \ > > | ^~ > > arch/arm/domain.c:345:9: note: in expansion of macro ‘reset_stack_and_jump’ > > 345 | reset_stack_and_jump(return_to_new_vcpu32); > > | ^~~~~~~~~~~~~~~~~~~~ > > arch/arm/domain.c:345:30: note: each undeclared identifier is reported only > > once for each function it appears in > > 345 | reset_stack_and_jump(return_to_new_vcpu32); > > | ^~~~~~~~~~~~~~~~~~~~ > > ./arch/arm/include/asm/current.h:48:65: note: in definition of macro > > ‘switch_stack_and_jump’ > > 48 | asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) > > : "memory" ); \ > > | ^~ > > arch/arm/domain.c:345:9: note: in expansion of macro ‘reset_stack_and_jump’ > > 345 | reset_stack_and_jump(return_to_new_vcpu32); > > | ^~~~~~~~~~~~~~~~~~~~ > > CC arch/arm/domain_build.o > > arch/arm/domain.c:348:30: error: ‘return_to_new_vcpu64’ undeclared (first > > use in this function) > > 348 | reset_stack_and_jump(return_to_new_vcpu64); > > | ^~~~~~~~~~~~~~~~~~~~ > > ./arch/arm/include/asm/current.h:48:65: note: in definition of macro > > ‘switch_stack_and_jump’ > > 48 | asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack), "X" (fn) > > : "memory" ); \ > > | ^~ > > arch/arm/domain.c:348:9: note: in expansion of macro ‘reset_stack_and_jump’ > > 348 | reset_stack_and_jump(return_to_new_vcpu64); > > | ^~~~~~~~~~~~~~~~~~~~ > > make[2]: *** [Rules.mk:246: arch/arm/domain.o] Error 1 > > With this change, the compiler becomes aware that the functions idle_loop, > return_to_new_vcpu32 and return_to_new_vcpu64 are used by the inline assembly. > For idle loop, there is a previous declaration but for the other two there is > not and when the compiler encounters their references complains. > So, for this to compile, it is also required to declare > return_to_new_vcpu32 and return_to_new_vcpu64. Ah, right! I didn't read close enough the error message. I can see now that it builds just fine on both arm32 and arm64. I am fine with this.
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 2f8eaab7b5..2fa67266d0 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -63,7 +63,7 @@ static void do_idle(void) rcu_idle_exit(cpu); } -void idle_loop(void) +static __used void idle_loop(void) { unsigned int cpu = smp_processor_id();
The function idle_loop() is referenced only in domain.c. Change its linkage from external to internal by adding the storage-class specifier static to its definitions. Since idle_loop() is referenced only in inline assembly, add the 'used' attribute to suppress unused-function compiler warning. Also, this patch resolves indirectly a MISRA C 2012 Rule 8.4 violation warning. Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com> --- xen/arch/arm/domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)