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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 12/02/18 17:26, Roger Pau Monné wrote:
> 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@citrix.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>
> Just one nit...
>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> ---
>>  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).

Actually, in combination with patch 5, that makes things less clear.  (I
already did as you suggested, then took it out.)

~Andrew

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