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

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 02/28/18 7:02 PM >>>
>On 28/02/18 16:16, Jan Beulich wrote:
>>>>> On 26.02.18 at 12:35, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>  .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.

Okay.

>>> @@ -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.

Well, fine with me then, and in that case it'll perhaps be indeed better to
avoid the use of max() here.

Jan



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