Re: [PATCH 3/4] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation

[Xenia Ragiadakou <burzalodowa@xxxxxxxxx>]

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
--
Xenia