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

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

On 16/08/18 10:55, Roger Pau Monné wrote:
> On Wed, Aug 15, 2018 at 06:57:38PM +0100, Andrew Cooper wrote:
>> Newer versions of binutils are capable of emitting an exact number bytes worth
>> of optimised nops, which are P6 nops.  Use this in preference to .skip when
>> available.
>>
>> Check at boot time whether the toolchain nops are the correct for the running
>> hardware, andskip optimising nops entirely when possible.
>                ^ missing space.
>
> TBH I'm not sure I see the benefit of using .nops over using .skip.

In this case, or in general?

In general, so we don't need to self/cross modify the alternatives
points which aren't patched.

In this case, because it is the .nops directive we're using to insert nops.

> Xen needs to do a memcmp in order to check whether the resulting nops
> are what Xen considers the more optimized instructions for the CPU
> currently running on. Xen can avoid the memcpy by using skip, because
> in that case Xen knows exactly the current instructions and there's no
> need to memcmp.

I'm afraid I don't understand what point you are attempting to make here.

> I guess the reason is that the memcmp will be done only once, and
> hopefully in most cases the assembler generated nops will be the most
> optimized version.

The memcmp() is once during init, and you've got to be on very ancient
hardware for the toolchain nops to not be the correct ones.  I'm going
to conservatively estimate that 98% of hardware running Xen will have P6
nops as ideal.

>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> 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/arch/x86/Rules.mk                 |  4 ++++
>>  xen/arch/x86/alternative.c            | 20 +++++++++++++++++++-
>>  xen/include/asm-x86/alternative-asm.h | 12 +++++++++++-
>>  xen/include/asm-x86/alternative.h     | 11 +++++++++--
>>  4 files changed, 43 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
>> index ac585a3..c84ed20 100644
>> --- 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)
> I think I remember commenting on an earlier version of this about the
> usage of the CONTROL parameter. I would expect the assembler to
> use the most optimized version by default, is that not the case?

Again, I don't understand what you're trying to say.

This expression is like this, because that's how we actually use it.

>
>> +
>>  CFLAGS += -mno-red-zone -fpic -fno-asynchronous-unwind-tables
>>  
>>  # Xen doesn't use SSE interally.  If the compiler supports it, also skip the
>> diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
>> index 0ef7a8b..2c844d6 100644
>> --- 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");
>> +extern char toolchain_nops[ASM_NOP_MAX];
>> +static bool __read_mostly toolchain_nops_are_ideal;
>> +
>> +#else
>> +# define toolchain_nops_are_ideal false
>> +#endif
>> +
>>  static void __init arch_init_ideal_nops(void)
>>  {
>>      switch ( boot_cpu_data.x86_vendor )
>> @@ -112,6 +125,11 @@ static void __init arch_init_ideal_nops(void)
>>              ideal_nops = k8_nops;
>>          break;
>>      }
>> +
>> +#ifdef HAVE_AS_NOP_DIRECTIVE
>> +    if ( memcmp(ideal_nops[ASM_NOP_MAX], toolchain_nops, ASM_NOP_MAX) == 0 )
>> +        toolchain_nops_are_ideal = true;
>> +#endif
> You are only comparing that the biggest nop instruction (9 bytes
> AFAICT) generated by the assembler is what Xen believes to be the more
> optimized version. What about shorter nops?

They are all variations on a theme.

For P6 nops, its the 0f 1f root which is important, which takes a modrm
byte.  Traditionally, its always encoded with eax and uses redundant
memory encodings for longer instructions.

I can't think of any way of detecting if the optimised nops if the
toolchain starts using alternative registers in the encoding, but I
expect this case won't happen in practice.

> I also see a chance that maybe newer assembler versions will at some
> point generate more optimized nops, but Xen will replace them with not
> so optimized versions if the Xen logic is not so up to date.

The nops which are the most optimised are a property of the pipeline. 
Its fixed for released hardware, and vendors would have to have a very
good reason to change it moving forwards, considering how much use the
current nops get in optimised code.

>
>>  }
>>  
>>  /* Use this to add nops to a buffer, then text_poke the whole buffer. */
>> @@ -209,7 +227,7 @@ void init_or_livepatch apply_alternatives(struct alt_instr *start,
>>              base->priv = 1;
>>  
>>              /* Nothing useful to do? */
>> -            if ( a->pad_len <= 1 )
>> +            if ( toolchain_nops_are_ideal || a->pad_len <= 1 )
>>                  continue;
>>  
>>              add_nops(buf, a->pad_len);
>> diff --git a/xen/include/asm-x86/alternative-asm.h b/xen/include/asm-x86/alternative-asm.h
>> index 0b61516..0d6fb4b 100644
>> --- a/xen/include/asm-x86/alternative-asm.h
>> +++ b/xen/include/asm-x86/alternative-asm.h
>> @@ -1,6 +1,8 @@
>>  #ifndef _ASM_X86_ALTERNATIVE_ASM_H_
>>  #define _ASM_X86_ALTERNATIVE_ASM_H_
>>  
>> +#include <asm/nops.h>
>> +
>>  #ifdef __ASSEMBLY__
>>  
>>  /*
>> @@ -19,6 +21,14 @@
>>      .byte 0 /* priv */
>>  .endm
>>  
>> +.macro mknops nr_bytes
>> +#ifdef HAVE_AS_NOP_DIRECTIVE
>> +    .nops \nr_bytes, ASM_NOP_MAX
>> +#else
>> +    .skip \nr_bytes, 0x90
> Use P6_NOP1 instead of open coding 0x90? Or have a

0x90 is the nop instruction, which IIRC has existed since the 8080.

~Andrew

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