|
[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 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.
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |