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

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

On 2023-10-31 08:50, Jan Beulich wrote:
> 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/).
>>> 

Will do

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

Ok.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)