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

[Jan Beulich <jbeulich@xxxxxxxx>]

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