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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.02.18 at 12:35, <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxx>

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

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

Jan


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