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

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

On Mon, Feb 26, 2018 at 01:08:05PM +0000, Andrew Cooper wrote:
> On 26/02/18 12:31, Roger Pau Monné wrote:
> > On Mon, Feb 26, 2018 at 11:35:04AM +0000, Andrew Cooper wrote:
> >> Newer versions of binutils are capable of emitting an exact number bytes 
> >> worth
> >> of optimised nops.  Use this in preference to .skip when available.
> >>
> >> 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>
> >>
> >> RFC until support is actually committed to binutils mainline.
> > Since RFC has been dropped from the subject, is this now committed?
> 
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=62a02d25b6e5d9f92c205260daa11355d0c62532

Thanks for the reference.

> >
> >> ---
> >>  xen/arch/x86/Rules.mk                 |  4 ++++
> >>  xen/include/asm-x86/alternative-asm.h | 14 ++++++++++++--
> >>  xen/include/asm-x86/alternative.h     | 13 ++++++++++---
> >>  3 files changed, 26 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
> >> index e169d67..bf5047f 100644
> >> --- a/xen/arch/x86/Rules.mk
> >> +++ b/xen/arch/x86/Rules.mk
> >> @@ -28,6 +28,10 @@ $(call as-option-add,CFLAGS,CC,".equ \"x\"$$(comma)1", \
> >>  $(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: .nop (.L2 - .L1)$$(comma)9",-DHAVE_AS_NOP_DIRECTIVE)
> >> +
> >>  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/include/asm-x86/alternative-asm.h 
> >> b/xen/include/asm-x86/alternative-asm.h
> >> index 25f79fe..9e46bed 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__
> >>  
> >>  /*
> >> @@ -18,6 +20,14 @@
> >>      .byte \pad_len
> >>  .endm
> >>  
> >> +.macro mknops nr_bytes
> >> +#ifdef HAVE_AS_NOP_DIRECTIVE
> >> +    .nop \nr_bytes, ASM_NOP_MAX
> > I'm not able to find any online document about the .nop directive, and
> > I cannot really figure out the purpose of the second parameter.
> 
> Its a bit woolly, named "control".  In practice, it is the maximum
> length of an individual nop.  Beyond 11 bytes (iirc), most pipelines
> take a decode stall.

Oh, I've looked at the commit above, and since control is an optional
parameter, why not leave the assembler chose the default value? AFAICT
in our case this should be 11 because it's 64bit code.

> >
> > Also, after this patch is applied it seems like the padding is not
> > going to be 0x90, because as will already emit optimized nops. Are
> > those nops more optimized than the ones added by the alternatives
> > framework?
> 
> They are the same nops (by and large), although arranged differently. 
> For example, GAS fills backwards rather than forwards, which is as
> recommended in the Intel ORM.

So that 'larger' nop instructions are going to be placed at the end of
the region instead of the beginning of it?

> > I would expect the nops added at runtime would be more optimized than
> > the ones added at build time, because the runtime ones could take into
> > account the specific CPU model Xen is running on.
> 
> There are only a very few 64-bit capable CPUs which prefer K8 nops over
> P6 nops, and they are all old.  The build-time nops are correct for the
> overwhelming majority of hardware.

OK, I have no idea about this, but if you think filling with .nop is
not going to introduce a performance penalty over the run-time filling
then that's fine for me.

Roger.

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