Re: [PATCH v2 5/7] x86/alt: Support for automatic padding calculations

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 28/02/18 16:16, Jan Beulich wrote:
>>>> On 26.02.18 at 12:35, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/Rules.mk
>> +++ b/xen/arch/x86/Rules.mk
>> @@ -24,6 +24,10 @@ $(call as-option-add,CFLAGS,CC,".equ \"x\"$$(comma)1", \
>>                       -U__OBJECT_LABEL__ -DHAVE_GAS_QUOTED_SYM \
>>                       '-D__OBJECT_LABEL__=$(subst $(BASEDIR)/,,$(CURDIR))/$$@')
>>  
>> +# GCC's idea of true is -1.  Clang's idea is 1
>> +$(call as-option-add,CFLAGS,CC,\
>> +    ".if ((1 > 0) < 0); .error \"\";.endif",,-DHAVE_AS_NEGATIVE_TRUE)
> With GCC replaced by gas, as indicated already be Roger
> (also in alternative*.h then)
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> However, I have a question to raise and a remark to make:
>
>>  .macro ALTERNATIVE oldinstr, newinstr, feature
>>  .L\@_orig_s:
>>      \oldinstr
>>  .L\@_orig_e:
>> +    /*
>> +     * Calculate the difference in size between the replacement and original
>> +     * instructions, to derive how much padding to introduce.
>> +     */
>> +    .L\@_diff = repl_len(1) - orig_len
>> +
>> +    .skip as_true(.L\@_diff > 0) * .L\@_diff, 0x90
> How certain are you that these forward references actually work on
> at least all half way recent gas versions? IOW I wonder whether it
> wouldn't be more safe to make these backward references (i.e.
> emitting the replacement code first).

This code is used unconditionally in Linux, so any binutils within their
minimum set work.

So overall, I'm going to err on the side of "yes" in answer to your
question.

>
>> @@ -45,18 +70,31 @@
>>  .L\@_orig_s:
>>      \oldinstr
>>  .L\@_orig_e:
>> +    /*
>> +     * Calculate the difference in size between the largest replacement and
>> +     * the original instructions, to derive how much padding to introduce.
>> +     */
>> +    .L\@_diff = as_max(repl_len(1), repl_len(2)) - orig_len
>> +
>> +     .skip as_true(.L\@_diff > 0) * .L\@_diff, 0x90
>> +.L\@_orig_p:
>>  
>>      .pushsection .altinstructions, "a", @progbits
>>  
>>      altinstruction_entry .L\@_orig_s, .L\@_repl_s1, \feature1, \
>> -        orig_len, repl_len(1)
>> +        orig_len, repl_len(1), pad_len
>>      altinstruction_entry .L\@_orig_s, .L\@_repl_s2, \feature2, \
>> -        orig_len, repl_len(2)
>> +        orig_len, repl_len(2), pad_len
>>  
>>      .section .discard, "a", @progbits
>> -    /* Assembler-time check that \newinstr{1,2} aren't longer than \oldinstr. */
>> -    .byte 0xff + repl_len(1) - orig_len
>> -    .byte 0xff + repl_len(2) - orig_len
>> +    /*
>> +     * Assembler-time checks:
>> +     *   - total_len <= 255
>> +     *   - \newinstr* <= total_len
>> +     */
>> +    .byte total_len
>> +    .byte 0xff + repl_len(1) - total_len
>> +    .byte 0xff + repl_len(2) - total_len
> Use as_max() here and emit only a single byte? I don't think the
> diagnostic will be any less helpful, as iirc it doesn't point out the
> line inside the macro anyway, so the developer will be left guessing
> anyway which of the two alternatives it was. Otoh with the
> padding size now being calculated, I don't see much point in these
> checks anymore anyway.

The bytes are discarded, so the quantity doesn't matter.  I can use max,
and you are correct that the line reference only ends up pointing to the
.macro itself.

However, I would prefer to keep these.  They've spotted two bugs in the
binutils development effort of .nops, and I'd prefer to be safe than sorry.

~Andrew

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