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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

Jan