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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.02.18 at 15:03, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 13/02/18 14:37, Jan Beulich wrote:
>>  >>> 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?
> 
> How would that work?  \() is the token separator.

When there's nothing inside the parentheses, this construct
can be used as a token separator, but that's not its main
purpose. Instead \(<text>) means to take <text> literally,
without e.g. expanding macro arguments inside it.

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

Well, okay then.

Jan


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