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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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

> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---
>  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?

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

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.

>  }
>  
>  /* 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

#define DEFAULT_NOP P6_NOP1

And use it instead.

Thanks, Roger.

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