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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

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)