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

2018-01-04 Thread Jan Beulich 
>>> On 04.01.18 at 01:15,  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 

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 
> +
> +.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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

2018-01-03 Thread Andrew Cooper 
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 
---
v4:
 * New
---
 xen/arch/x86/Makefile |  1 +
 xen/arch/x86/Rules.mk |  7 +++
 xen/arch/x86/indirect_thunk.S | 28 
 xen/arch/x86/xen.lds.S|  1 +
 4 files changed, 37 insertions(+)
 create mode 100644 xen/arch/x86/indirect_thunk.S

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index d5d58a2..433233c 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -36,6 +36,7 @@ obj-y += io_apic.o
 obj-$(CONFIG_LIVEPATCH) += alternative.o livepatch.o
 obj-y += msi.o
 obj-y += msr.o
+obj-$(CONFIG_INDIRECT_THUNK) += indirect_thunk.o
 obj-y += ioport_emulate.o
 obj-y += irq.o
 obj-$(CONFIG_KEXEC) += machine_kexec.o
diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
index 568657e..abcc4d4 100644
--- 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)
+CFLAGS += -mindirect-branch=thunk-extern -mindirect-branch-register
+CFLAGS += -DCONFIG_INDIRECT_THUNK
+export CONFIG_INDIRECT_THUNK=y
+endif
diff --git a/xen/arch/x86/indirect_thunk.S b/xen/arch/x86/indirect_thunk.S
new file mode 100644
index 000..4fef1c8
--- /dev/null
+++ b/xen/arch/x86/indirect_thunk.S
@@ -0,0 +1,28 @@
+#include 
+
+.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
+
+/* 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
+GEN_INDIRECT_THUNK name=\enc, reg=%\enc
+.endr
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index d5e8821..345946f 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -59,6 +59,7 @@ SECTIONS
   .text : {
 _stext = .;/* Text and read-only data */
*(.text)
+   *(.text.__x86.*)
*(.text.cold)
*(.text.unlikely)
*(.fixup)
-- 
2.1.4


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