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

2018-03-08 Thread Jan Beulich 
>>> On 08.03.18 at 17:48,  wrote:
> On 08/03/18 15:53, Jan Beulich wrote:
> On 07.03.18 at 16:51,  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)
>> The construct is right, but the comment still has the old directive name.
>>
>>> --- a/xen/arch/x86/alternative.c
>>> +++ b/xen/arch/x86/alternative.c
>>> @@ -207,7 +207,8 @@ void init_or_livepatch apply_alternatives(struct 
>>> alt_instr *start,
>>>  base->priv = 1;
>>>  
>>>  /* Nothing useful to do? */
>>> -if ( a->pad_len <= 1 )
>>> +if ( (TOOLCHAIN_P6_NOPS && ideal_nops == p6_nops) ||
>>> + a->pad_len <= 1 )
>>>  continue;
>> I'm sorry, but no - we can't assume all gas versions going forward
>> will continue to produce the NOPs we want. They may change at
>> any time, and we may change our mind at any time. If anything
>> you'd need to actively check that what their .nops produces
>> matches what our table has.
> 
> Hmm perhaps, but the chances of either of these actually happening are
> extremely low.
> 
>> Additionally such skipping on the vast majority of hardware is
>> prone to leave bugs undiscovered for quite some time.
> 
> Such as?  This statement isn't exactly helpful, and

Well, the code past the "continue" will effectively only get
compile tested for extended periods of time. Whatever bug it
is that might creep in there. (But then again your sentence
looks unfinished.)

>> Anyway - I continue to fail to see the value of this patch with the
>> immediately preceding one already doing all we need.
> 
> The purpose, as explained before, is to avoid patching whenever possible.
> 
> "Whenever possible" is every time we fail a feature check, and the
> toolchain puts out the correct nops (which, for a capable toolchain, is
> overwhelmingly likely given our 64bit-ness), and has a direct effect on
> our boot time safety.

If our patching has a bug, we'll have to fix it. Whereas (as said
before) the view on what is "correct" may vary over time.

>> An alternative might be to have a Kconfig option to suppress the
>> NOP optimization altogether, and rely on what the tool chain
>> produces.
> 
> How and why would a user wish to change this option?  I don't think
> anyone would find it useful.

If you find such an option useless, then why is this patch useful? I,
for one, would leave the option off, making sure runtime NOP
replacement happens at all times.

Jan


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

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

2018-03-08 Thread Andrew Cooper 
On 08/03/18 15:53, Jan Beulich wrote:
 On 07.03.18 at 16:51,  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)
> The construct is right, but the comment still has the old directive name.
>
>> --- a/xen/arch/x86/alternative.c
>> +++ b/xen/arch/x86/alternative.c
>> @@ -207,7 +207,8 @@ void init_or_livepatch apply_alternatives(struct 
>> alt_instr *start,
>>  base->priv = 1;
>>  
>>  /* Nothing useful to do? */
>> -if ( a->pad_len <= 1 )
>> +if ( (TOOLCHAIN_P6_NOPS && ideal_nops == p6_nops) ||
>> + a->pad_len <= 1 )
>>  continue;
> I'm sorry, but no - we can't assume all gas versions going forward
> will continue to produce the NOPs we want. They may change at
> any time, and we may change our mind at any time. If anything
> you'd need to actively check that what their .nops produces
> matches what our table has.

Hmm perhaps, but the chances of either of these actually happening are
extremely low.

> Additionally such skipping on the vast majority of hardware is
> prone to leave bugs undiscovered for quite some time.

Such as?  This statement isn't exactly helpful, and

> Anyway - I continue to fail to see the value of this patch with the
> immediately preceding one already doing all we need.

The purpose, as explained before, is to avoid patching whenever possible.

"Whenever possible" is every time we fail a feature check, and the
toolchain puts out the correct nops (which, for a capable toolchain, is
overwhelmingly likely given our 64bit-ness), and has a direct effect on
our boot time safety.

> An alternative might be to have a Kconfig option to suppress the
> NOP optimization altogether, and rely on what the tool chain
> produces.

How and why would a user wish to change this option?  I don't think
anyone would find it useful.

~Andrew

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

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

2018-03-08 Thread Jan Beulich 
>>> On 07.03.18 at 16:51,  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)

The construct is right, but the comment still has the old directive name.

> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -207,7 +207,8 @@ void init_or_livepatch apply_alternatives(struct 
> alt_instr *start,
>  base->priv = 1;
>  
>  /* Nothing useful to do? */
> -if ( a->pad_len <= 1 )
> +if ( (TOOLCHAIN_P6_NOPS && ideal_nops == p6_nops) ||
> + a->pad_len <= 1 )
>  continue;

I'm sorry, but no - we can't assume all gas versions going forward
will continue to produce the NOPs we want. They may change at
any time, and we may change our mind at any time. If anything
you'd need to actively check that what their .nops produces
matches what our table has.

Additionally such skipping on the vast majority of hardware is
prone to leave bugs undiscovered for quite some time.

Anyway - I continue to fail to see the value of this patch with the
immediately preceding one already doing all we need.

An alternative might be to have a Kconfig option to suppress the
NOP optimization altogether, and rely on what the tool chain
produces.

Jan


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

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

2018-03-07 Thread Andrew Cooper 
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, and skip optimising nops entirely when they correct for the running
hardware.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Konrad Rzeszutek Wilk 
CC: Roger Pau Monné 
CC: Wei Liu 

v3:
 * Rewrite toolstack P6 nops when K8 nops are the correct ones for the system.
---
 xen/arch/x86/Rules.mk |  4 
 xen/arch/x86/alternative.c|  3 ++-
 xen/include/asm-x86/alternative-asm.h | 12 +++-
 xen/include/asm-x86/alternative.h | 13 +++--
 4 files changed, 28 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)
+
 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 93adf56..dc3ef24 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -207,7 +207,8 @@ void init_or_livepatch apply_alternatives(struct alt_instr 
*start,
 base->priv = 1;
 
 /* Nothing useful to do? */
-if ( a->pad_len <= 1 )
+if ( (TOOLCHAIN_P6_NOPS && ideal_nops == p6_nops) ||
+ 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 
+
 #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
+#endif
+.endm
+
 /* GAS's idea of true is -1, while Clang's idea is 1. */
 #ifdef HAVE_AS_NEGATIVE_TRUE
 # define as_true(x) (-(x))
@@ -29,7 +39,7 @@
 #define decl_orig(insn, padding)  \
  .L\@_orig_s: insn; .L\@_orig_e:  \
  .L\@_diff = padding; \
- .skip as_true(.L\@_diff > 0) * .L\@_diff, 0x90;  \
+ mknops (as_true(.L\@_diff > 0) * .L\@_diff); \
  .L\@_orig_p:
 
 #define orig_len   (.L\@_orig_e   - .L\@_orig_s)
diff --git a/xen/include/asm-x86/alternative.h 
b/xen/include/asm-x86/alternative.h
index 4803368..7a88c43 100644
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -2,7 +2,6 @@
 #define __X86_ALTERNATIVE_H__
 
 #include 
-#include 
 
 #ifndef __ASSEMBLY__
 #include 
@@ -27,6 +26,16 @@ 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
+# define TOOLCHAIN_P6_NOPS 1
+  ".nops \\nr_bytes, " __stringify(ASM_NOP_MAX) "\n\t"
+#else
+# define TOOLCHAIN_P6_NOPS 0
+  ".skip \\nr_bytes, 0x90\n\t"
+#endif
+  ".endm\n\t" );
+
 #define alt_orig_len   "(.LXEN%=_orig_e - .LXEN%=_orig_s)"
 #define alt_pad_len"(.LXEN%=_orig_p - .LXEN%=_orig_e)"
 #define alt_total_len  "(.LXEN%=_orig_p - .LXEN%=_orig_s)"
@@ -46,7 +55,7 @@ extern void alternative_instructions(void);
 #define OLDINSTR(oldinstr, padding)  \
 ".LXEN%=_orig_s:\n\t" oldinstr "\n .LXEN%=_orig_e:\n\t"  \
 ".LXEN%=_diff = " padding "\n\t" \
-".skip "AS_TRUE"(.LXEN%=_diff > 0) * .LXEN%=_diff, 0x90\n\t" \
+"mknops ("AS_TRUE"(.LXEN%=_diff > 0) * .LXEN%=_diff)\n\t"\
 ".LXEN%=_orig_p:\n\t"
 
 #define OLDINSTR_1(oldinstr, n1) \
-- 
2.1.4


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