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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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?