Re: [PATCH] x86/build: Use new .nops directive when available

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

On 17/08/18 13:45, Jan Beulich wrote:
>>>> On 15.08.18 at 19:57, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/Rules.mk
>> +++ b/xen/arch/x86/Rules.mk
>> @@ -29,6 +29,10 @@ $(call as-option-add,CFLAGS,CC,"invpcid 
>> (%rax)$$(comma)%rax",-DHAVE_AS_INVPCID)
>>  $(call as-option-add,CFLAGS,CC,\
>>      ".if ((1 > 0) < 0); .error \"\";.endif",,-DHAVE_AS_NEGATIVE_TRUE)
>>  
>> +# Check to see whether the assmbler supports the .nop directive.
>> +$(call as-option-add,CFLAGS,CC,\
>> +    ".L1: .L2: .nops (.L2 - .L1)$$(comma)9",-DHAVE_AS_NOP_DIRECTIVE)
> Can this please be HAVE_AS_NOPS_DIRECTIVE?

Ok.

>
>> --- a/xen/arch/x86/alternative.c
>> +++ b/xen/arch/x86/alternative.c
>> @@ -84,6 +84,19 @@ static const unsigned char * const p6_nops[ASM_NOP_MAX+1] 
>> init_or_livepatch_cons
>>  
>>  static const unsigned char * const *ideal_nops init_or_livepatch_data = p6_nops;
>>  
>> +#ifdef HAVE_AS_NOP_DIRECTIVE
>> +
>> +/* Nops in .init.rodata to compare against the runtime ideal nops. */
>> +asm ( ".pushsection .init.rodata, \"a\", @progbits\n\t"
>> +      "toolchain_nops: .nops " __stringify(ASM_NOP_MAX) "\n\t"
>> +      ".popsection\n\t");
> Any particular reason not to put them in .init.text?

Because its data, not executable code.

>
>> @@ -27,6 +26,14 @@ extern void add_nops(void *insns, unsigned int len);
>>  extern void apply_alternatives(struct alt_instr *start, struct alt_instr *end);
>>  extern void alternative_instructions(void);
>>  
>> +asm ( ".macro mknops nr_bytes\n\t"
>> +#ifdef HAVE_AS_NOP_DIRECTIVE
>> +      ".nops \\nr_bytes, " __stringify(ASM_NOP_MAX) "\n\t"
>> +#else
>> +      ".skip \\nr_bytes, 0x90\n\t"
>> +#endif
>> +      ".endm\n\t" );
> Did you take a look at "x86/alternatives: allow using assembler macros
> in favor of C ones" yet? Perhaps this redundant macro definition could
> be avoided that way? I'm not going to exclude though that some more
> work might be needed to get there.

No - not looked at that yet.  That does look like we can get a large net
cleanup, and can avoid using always_inline to work around the same problem.

>
> Anyway, I'm in principle fine with the change, irrespective of the other
> discussion we've had, so with the name change and possibly the other
> two remarks taken care of
> Acked-by: Jan Beulich <jbeulich@suse.com>

Thanks,

~Andrew

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