Re: [Xen-devel] [PATCH v6.5 09/26] x86: Support compiling with indirect branch thunks

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.01.18 at 01:15, <andrew.cooper3@xxxxxxxxxx> wrote:
> Use -mindirect-branch=thunk-extern/-mindirect-branch-register when available.
> To begin with, use the retpoline thunk.  Later work will add alternative
> thunks which can be selected at boot time.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

For the sake of completeness I'm going to reproduce my v6
comments here which weren't already addressed one way or the
other.

> --- a/xen/arch/x86/Rules.mk
> +++ b/xen/arch/x86/Rules.mk
> @@ -30,3 +30,10 @@ CFLAGS += -fno-asynchronous-unwind-tables
>  ifneq ($(call cc-option,$(CC),-fvisibility=hidden,n),n)
>  CFLAGS += -DGCC_HAS_VISIBILITY_ATTRIBUTE
>  endif
> +
> +# Compile with thunk-extern, indirect-branch-register if avaiable.
> +ifneq ($(call cc-option,$(CC),-mindirect-branch-register,n),n)

Why you don't use cc-option-add in favor of cc-option inside the
ifneq ()?

> +CFLAGS += -mindirect-branch=thunk-extern -mindirect-branch-register
> +CFLAGS += -DCONFIG_INDIRECT_THUNK
> +export CONFIG_INDIRECT_THUNK=y

Perhaps it would be better (more consistent) to make this a normal
config option, with the help text saying that enabling it requires a
capable compiler.

> --- /dev/null
> +++ b/xen/arch/x86/indirect_thunk.S

From the purely cosmetic angle, personally I would have preferred
indirect-thunk.S as a file name.

> @@ -0,0 +1,28 @@
> +#include <asm/asm_defns.h>
> +
> +.macro IND_THUNK_RETPOLINE reg:req
> +        call 2f
> +1:
> +        lfence
> +        jmp 1b
> +2:
> +        mov \reg, (%rsp)
> +        ret
> +.endm
> +
> +/*
> + * Build the __x86.indirect_thunk.* symbols.  Execution lands on an
> + * alternative patch point which implements one of the above THUNK_*'s
> + */
> +.macro GEN_INDIRECT_THUNK name:req reg:req
> +        .section .text.__x86.indirect_thunk.\name, "ax", @progbits
> +
> +ENTRY(__x86.indirect_thunk.\name)
> +        IND_THUNK_RETPOLINE \reg
> +.endm

I don't see why this needs two parameters:

.macro GEN_INDIRECT_THUNK name:req
        .section .text.__x86.indirect_thunk.\name, "ax", @progbits

ENTRY(__x86.indirect_thunk.\name)
        IND_THUNK_RETPOLINE %\name
.endm

or even push the addition of the % down into IND_THUNK_RETPOLINE.

I also don't see the need for the double underscores in the section
name - I think just .text.\name would suffice.

> +/* Instantiate GEN_INDIRECT_THUNK for each register except %rsp. */
> +.irp enc, rax, rbx, rcx, rdx, rsi, rdi, rbp, \
> +          r8, r9, r10, r11, r12, r13, r14, r15

Again purely cosmetic: Preferably these would be sorted either
by encoding value (my preference) or alphabetically. Personally
I would also have dropped the recurring r-s from here, adding
them upon use of the parameter (which with the earlier comment
would be exactly one instance).

Jan


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