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

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

On 13/02/18 14:37, Jan Beulich wrote:
>  >>> On 12.02.18 at 12:23, <andrew.cooper3@citrix.com> 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?

How would that work?  \() is the token separator.

>
>> --- 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?

It is intended to be the same, for consistency.  As there are no asm (
".include alternatives-asm.h" ), there is no chance of both definitions
existing in the same translation unit.

As for potentially doing an asm level include, the resulting is
prohibitively awkward to use, so I don't think it is a worry.

>
> 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()?

Ok.

~Andrew

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