[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 3/4] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation
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.
--
Xenia
|
