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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan