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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.03.18 at 17:48, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 08/03/18 15:53, Jan Beulich wrote:
>>>>> On 07.03.18 at 16:51, <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel