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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Feb 12, 2018 at 11:23:03AM +0000, Andrew Cooper wrote:
>  * On the C side, switch to using local lables rather than hardcoded numbers.
                                          ^ labels
>  * Rename parameters and lables to be consistent with alt_instr names, and
                           ^ labels
>    consistent between the the C and asm versions.
>  * On the asm side, factor some expressions out into macros to aid clarity.
>  * Consistently declare section attributes.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Just one nit...

> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---
>  xen/include/asm-x86/alternative-asm.h | 57 +++++++++++++++++--------------
>  xen/include/asm-x86/alternative.h     | 64 
> +++++++++++++++++++----------------
>  2 files changed, 67 insertions(+), 54 deletions(-)
> 
> diff --git a/xen/include/asm-x86/alternative-asm.h 
> b/xen/include/asm-x86/alternative-asm.h
> index 6640e85..150bd1a 100644
> --- 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:

I would also introduce a decl_orig(insn), seeing that ".L\@_orig_s" is
already used in two different places (ALTERNATIVE and ALTERNATIVE_2).

Thanks, Roger.

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