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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Xen-devel <xen-devel@lists.xen.org>,
	"Wei Liu" <wei.liu2@citrix.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [PATCH v3 7/7] x86/build: Use new .nops directive when available
Date: Thu, 8 Mar 2018 16:48:08 +0000	[thread overview]
Message-ID: <2c720709-c0a0-bfcf-2cfc-2b72976844e3@citrix.com> (raw)
In-Reply-To: <5AA16AA502000078001AFE4A@prv-mh.provo.novell.com>

On 08/03/18 15:53, Jan Beulich wrote:
>>>> On 07.03.18 at 16:51, <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)
> 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