Re: [Xen-devel] [PATCH 3/7] x86/alt: Clean up the assembly used to generate alternatives

["Jan Beulich" <JBeulich@xxxxxxxx>]

 >>> On 12.02.18 at 12:23, <andrew.cooper3@xxxxxxxxxx> wrote:
> --- a/xen/include/asm-x86/alternative-asm.h
> +++ b/xen/include/asm-x86/alternative-asm.h
> @@ -9,60 +9,67 @@
>   * enough information for the alternatives patching code to patch an
>   * instruction. See apply_alternatives().
>   */
> -.macro altinstruction_entry orig alt feature orig_len alt_len
> +.macro altinstruction_entry orig repl feature orig_len repl_len
>      .long \orig - .
> -    .long \alt - .
> +    .long \repl - .
>      .word \feature
>      .byte \orig_len
> -    .byte \alt_len
> +    .byte \repl_len
>  .endm
>  
> +#define orig_len               (.L\@_orig_e       -     .L\@_orig_s)
> +#define repl_len(nr)           (.L\@_repl_e\()nr  -     .L\@_repl_s\()nr)
> +#define decl_repl(insn, nr)     .L\@_repl_s\()nr: insn; .L\@_repl_e\()nr:

Wouldn't it work equally well but look slightly less odd if you used
\(nr) instead of \()nr?

> --- a/xen/include/asm-x86/alternative.h
> +++ b/xen/include/asm-x86/alternative.h
> @@ -26,44 +26,50 @@ extern void apply_alternatives(const struct alt_instr 
> *start,
>                                 const struct alt_instr *end);
>  extern void alternative_instructions(void);
>  
> -#define OLDINSTR(oldinstr)      "661:\n\t" oldinstr "\n662:\n"
> +#define OLDINSTR(oldinstr)      ".L%=_orig_s:\n\t" oldinstr 
> "\n.L%=_orig_e:\n"

Isn't this too similar a naming scheme to what the assembler side
uses? I.e. is it entirely certain that no C file will ever (indirectly)
include alternative-asm.h, potentially resulting in a label name
clash then?

Here please also don't forget that you're competing with the
compiler for the .L name space, so some better disambiguation
may be advisable (e.g. starting the names with .LXEN).

> -#define b_replacement(number)   "663"#number
> -#define e_replacement(number)   "664"#number
> +#define repl_s(num)             ".L%=_repl_s"#num
> +#define repl_e(num)             ".L%=_repl_e"#num

Since you don't (and can't) #undef them, how about alt_repl_s()
and alt_repl_e()?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel