[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
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" );
--
Xenia
|
