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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 31 Oct 2023, Jan Beulich wrote:
> On 31.10.2023 09:22, Nicola Vetrini wrote:
> > On 2023-10-30 16:12, 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).
> >>
> >>> --- a/xen/include/xen/compiler.h
> >>> +++ b/xen/include/xen/compiler.h
> >>> @@ -159,6 +159,9 @@
> >>>  # define ASM_FLAG_OUT(yes, no) no
> >>>  #endif
> >>>
> >>> +/* Mark a function or variable as used only from asm */
> >>> +#define asmlinkage
> >>
> >> I appreciate this being an immediately "natural" place, but considering
> >> what we know from Linux I think we ought to allow for arch overrides 
> >> here
> >> right away. For that I'm afraid compiler.h isn't best; it may still be
> >> okay as long as at least an #ifndef is put around it. Imo, however, 
> >> this
> >> ought to go into xen/linkage.h, as is being introduced by "common:
> >> assembly entry point type/size annotations". It's somewhat a shame that
> >> this and the rest of that series has missed 4.18 ...
> > 
> > An #ifndef around what, exactly?
> 
> The #define. That way at least an arch's config.h can override it.

I think the #ifdef is the way to go for now to reduce dependencies
between series


> > Anyway, making (part of) this series 
> > wait for approval
> > until the other has been accepted into 4.19 (for which I have no 
> > specific timeframe)
> > does not seem that desirable to me.
> 
> It's not ideal, but you or anyone else are free to help that other work
> make progress.