Re: [XEN PATCH][for-4.19 v5 2/8] x86: add deviation for asm-only functions

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.10.2023 00:02, Stefano Stabellini wrote:
> On Mon, 30 Oct 2023, Jan Beulich wrote:
>> On 30.10.2023 10:11, Nicola Vetrini wrote:
>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> @@ -163,6 +163,15 @@ Therefore the absence of prior declarations is safe."
>>>  -config=MC3R1.R8.4,reports+={safe, "first_area(any_loc(file(gcov)))"}
>>>  -doc_end
>>>  
>>> +-doc_begin="Recognize the occurrence of current_stack_pointer as a 
>>> declaration."
>>> +-file_tag+={asm_defns, "^xen/arch/x86/include/asm/asm_defns\\.h$"}
>>> +-config=MC3R1.R8.4,declarations+={safe, 
>>> "loc(file(asm_defns))&&^current_stack_pointer$"}
>>> +-doc_end
>>> +
>>> +-doc_begin="asmlinkage is a marker to indicate that the function is only 
>>> used from asm modules."
>>> +-config=MC3R1.R8.4,declarations+={safe,"loc(text(^.*asmlinkage.*$, 
>>> -1..0))"}
>>> +-doc_end
>>
>> In the longer run asmlinkage will want using for functions used either way
>> between C and assembly (i.e. C->asm calls as well as asm->C ones). I'd like
>> to ask that the text please allow for that (e.g. s/from/to interface with/).
>>
>>> --- a/xen/arch/x86/hvm/svm/intr.c
>>> +++ b/xen/arch/x86/hvm/svm/intr.c
>>> @@ -123,7 +123,7 @@ static void svm_enable_intr_window(struct vcpu *v, 
>>> struct hvm_intack intack)
>>>          vmcb, general1_intercepts | GENERAL1_INTERCEPT_VINTR);
>>>  }
>>>  
>>> -void svm_intr_assist(void)
>>> +asmlinkage void svm_intr_assist(void)
>>
>> Nit (here and below): Attributes, unless impossible for some specific
>> reason, should always go between type and identifier. Not all our code
>> is conforming to that, but I think a majority is, and hence you should
>> be able to find ample examples (taking e.g. __init).
> 
> Hi Jan, in general I agree with this principle but I noticed that this
> is not the way Linux uses asmlinkage, a couple of examples:
> 
> asmlinkage void do_page_fault(struct pt_regs *regs);
> asmlinkage const sys_call_ptr_t sys_call_table[];
> 
> Should we go our way or follow Linux on this in terms of code style?

Linux isn't very consistent in attribute placement anyway, and doing it
"randomly" relies on the compiler guys never going to tighten what
attributes mean dependent on their placement (iirc gcc doc has text to
the effect of this possibly changing at any time). Aiui part of the
reason why parsing is more relaxed than it should be is that certain
attributes cannot be placed unambiguously at their nominally dedicated
place.

So my personal view on your question is that we should go our more
consistent way.

Jan