[PATCH 0/4] x86: some assembler macro rework

[PATCH 0/4] x86: some assembler macro rework

[Jan Beulich]

Parts of this were discussed in the context of Andrew's CET-SS work.
Other parts simply fit the underlying picture.

1: replace __ASM_{CL,ST}AC
2: reduce CET-SS related #ifdef-ary
3: drop ASM_{CL,ST}AC
4: fold indirect_thunk_asm.h into asm-defns.h

Jan

[PATCH 1/4] x86: replace __ASM_{CL,ST}AC

[Jan Beulich]

Introduce proper assembler macros instead, enabled only when the
assembler itself doesn't support the insns. To avoid duplicating the
macros for assembly and C files, have them processed into asm-macros.h.
This in turn requires adding a multiple inclusion guard when generating
that header.

No change to generated code.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -235,7 +235,10 @@ $(BASEDIR)/include/asm-x86/asm-macros.h:
 	echo '#if 0' >$@.new
 	echo '.if 0' >>$@.new
 	echo '#endif' >>$@.new
+	echo '#ifndef __ASM_MACROS_H__' >>$@.new
+	echo '#define __ASM_MACROS_H__' >>$@.new
 	echo 'asm ( ".include \"$@\"" );' >>$@.new
+	echo '#endif /* __ASM_MACROS_H__ */' >>$@.new
 	echo '#if 0' >>$@.new
 	echo '.endif' >>$@.new
 	cat $< >>$@.new
--- a/xen/arch/x86/arch.mk
+++ b/xen/arch/x86/arch.mk
@@ -20,6 +20,7 @@ $(call as-option-add,CFLAGS,CC,"rdrand %
 $(call as-option-add,CFLAGS,CC,"rdfsbase %rax",-DHAVE_AS_FSGSBASE)
 $(call as-option-add,CFLAGS,CC,"xsaveopt (%rax)",-DHAVE_AS_XSAVEOPT)
 $(call as-option-add,CFLAGS,CC,"rdseed %eax",-DHAVE_AS_RDSEED)
+$(call as-option-add,CFLAGS,CC,"clac",-DHAVE_AS_CLAC_STAC)
 $(call as-option-add,CFLAGS,CC,"clwb (%rax)",-DHAVE_AS_CLWB)
 $(call as-option-add,CFLAGS,CC,".equ \"x\"$$(comma)1",-DHAVE_AS_QUOTED_SYM)
 $(call as-option-add,CFLAGS,CC,"invpcid (%rax)$$(comma)%rax",-DHAVE_AS_INVPCID)
--- a/xen/arch/x86/asm-macros.c
+++ b/xen/arch/x86/asm-macros.c
@@ -1 +1,2 @@
+#include <asm/asm-defns.h>
 #include <asm/alternative-asm.h>
--- /dev/null
+++ b/xen/include/asm-x86/asm-defns.h
@@ -0,0 +1,9 @@
+#ifndef HAVE_AS_CLAC_STAC
+.macro clac
+    .byte 0x0f, 0x01, 0xca
+.endm
+
+.macro stac
+    .byte 0x0f, 0x01, 0xcb
+.endm
+#endif
--- a/xen/include/asm-x86/asm_defns.h
+++ b/xen/include/asm-x86/asm_defns.h
@@ -13,10 +13,12 @@
 #include <asm/alternative.h>
 
 #ifdef __ASSEMBLY__
+#include <asm/asm-defns.h>
 #ifndef CONFIG_INDIRECT_THUNK
 .equ CONFIG_INDIRECT_THUNK, 0
 #endif
 #else
+#include <asm/asm-macros.h>
 asm ( "\t.equ CONFIG_INDIRECT_THUNK, "
       __stringify(IS_ENABLED(CONFIG_INDIRECT_THUNK)) );
 #endif
@@ -200,34 +202,27 @@ register unsigned long current_stack_poi
 
 #endif
 
-/* "Raw" instruction opcodes */
-#define __ASM_CLAC      ".byte 0x0f,0x01,0xca"
-#define __ASM_STAC      ".byte 0x0f,0x01,0xcb"
-
 #ifdef __ASSEMBLY__
 .macro ASM_STAC
-    ALTERNATIVE "", __ASM_STAC, X86_FEATURE_XEN_SMAP
+    ALTERNATIVE "", stac, X86_FEATURE_XEN_SMAP
 .endm
 .macro ASM_CLAC
-    ALTERNATIVE "", __ASM_CLAC, X86_FEATURE_XEN_SMAP
+    ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
 .endm
 #else
 static always_inline void clac(void)
 {
     /* Note: a barrier is implicit in alternative() */
-    alternative("", __ASM_CLAC, X86_FEATURE_XEN_SMAP);
+    alternative("", "clac", X86_FEATURE_XEN_SMAP);
 }
 
 static always_inline void stac(void)
 {
     /* Note: a barrier is implicit in alternative() */
-    alternative("", __ASM_STAC, X86_FEATURE_XEN_SMAP);
+    alternative("", "stac", X86_FEATURE_XEN_SMAP);
 }
 #endif
 
-#undef __ASM_STAC
-#undef __ASM_CLAC
-
 #ifdef __ASSEMBLY__
 .macro SAVE_ALL op, compat=0
 .ifeqs "\op", "CLAC"

Re: [PATCH 1/4] x86: replace __ASM_{CL,ST}AC

[Roger Pau Monné]

On Wed, Jul 15, 2020 at 12:48:14PM +0200, Jan Beulich wrote:
> Introduce proper assembler macros instead, enabled only when the
> assembler itself doesn't support the insns. To avoid duplicating the
> macros for assembly and C files, have them processed into asm-macros.h.
> This in turn requires adding a multiple inclusion guard when generating
> that header.
> 
> No change to generated code.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -235,7 +235,10 @@ $(BASEDIR)/include/asm-x86/asm-macros.h:
>  	echo '#if 0' >$@.new
>  	echo '.if 0' >>$@.new
>  	echo '#endif' >>$@.new
> +	echo '#ifndef __ASM_MACROS_H__' >>$@.new
> +	echo '#define __ASM_MACROS_H__' >>$@.new
>  	echo 'asm ( ".include \"$@\"" );' >>$@.new
> +	echo '#endif /* __ASM_MACROS_H__ */' >>$@.new
>  	echo '#if 0' >>$@.new
>  	echo '.endif' >>$@.new
>  	cat $< >>$@.new
> --- a/xen/arch/x86/arch.mk
> +++ b/xen/arch/x86/arch.mk
> @@ -20,6 +20,7 @@ $(call as-option-add,CFLAGS,CC,"rdrand %
>  $(call as-option-add,CFLAGS,CC,"rdfsbase %rax",-DHAVE_AS_FSGSBASE)
>  $(call as-option-add,CFLAGS,CC,"xsaveopt (%rax)",-DHAVE_AS_XSAVEOPT)
>  $(call as-option-add,CFLAGS,CC,"rdseed %eax",-DHAVE_AS_RDSEED)
> +$(call as-option-add,CFLAGS,CC,"clac",-DHAVE_AS_CLAC_STAC)
>  $(call as-option-add,CFLAGS,CC,"clwb (%rax)",-DHAVE_AS_CLWB)
>  $(call as-option-add,CFLAGS,CC,".equ \"x\"$$(comma)1",-DHAVE_AS_QUOTED_SYM)
>  $(call as-option-add,CFLAGS,CC,"invpcid (%rax)$$(comma)%rax",-DHAVE_AS_INVPCID)
> --- a/xen/arch/x86/asm-macros.c
> +++ b/xen/arch/x86/asm-macros.c
> @@ -1 +1,2 @@
> +#include <asm/asm-defns.h>
>  #include <asm/alternative-asm.h>
> --- /dev/null
> +++ b/xen/include/asm-x86/asm-defns.h

Maybe this could be asm-insn.h or a different name? I find it
confusing to have asm-defns.h and an asm_defs.h.

Thanks, Roger.

Re: [PATCH 1/4] x86: replace __ASM_{CL,ST}AC

[Jan Beulich]

On 27.07.2020 16:55, Roger Pau Monné wrote:
> On Wed, Jul 15, 2020 at 12:48:14PM +0200, Jan Beulich wrote:
>> --- /dev/null
>> +++ b/xen/include/asm-x86/asm-defns.h
> 
> Maybe this could be asm-insn.h or a different name? I find it
> confusing to have asm-defns.h and an asm_defs.h.

While indeed I anticipated a reply to this effect, I don't consider
asm-insn.h or asm-macros.h suitable: We don't want to limit this
header to a more narrow purpose than "all sorts of definition", I
don't think. Hence I chose that name despite its similarity to the
C header's one.

Jan

Re: [PATCH 1/4] x86: replace __ASM_{CL,ST}AC

[Roger Pau Monné]

On Mon, Jul 27, 2020 at 09:47:52PM +0200, Jan Beulich wrote:
> On 27.07.2020 16:55, Roger Pau Monné wrote:
> > On Wed, Jul 15, 2020 at 12:48:14PM +0200, Jan Beulich wrote:
> > > --- /dev/null
> > > +++ b/xen/include/asm-x86/asm-defns.h
> > 
> > Maybe this could be asm-insn.h or a different name? I find it
> > confusing to have asm-defns.h and an asm_defs.h.
> 
> While indeed I anticipated a reply to this effect, I don't consider
> asm-insn.h or asm-macros.h suitable: We don't want to limit this
> header to a more narrow purpose than "all sorts of definition", I
> don't think. Hence I chose that name despite its similarity to the
> C header's one.

I think it's confusing, but I also think the whole magic we do with
asm includes is already confusing (me), so if you and Andrew agree
this is the best name I'm certainly fine with it. FWIW:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Please quote the clac/stac instructions in order to match the other
usages of ALTERNATIVE.

Thanks, Roger.

Re: [PATCH 1/4] x86: replace __ASM_{CL,ST}AC

[Jan Beulich]

On 28.07.2020 11:06, Roger Pau Monné wrote:
> On Mon, Jul 27, 2020 at 09:47:52PM +0200, Jan Beulich wrote:
>> On 27.07.2020 16:55, Roger Pau Monné wrote:
>>> On Wed, Jul 15, 2020 at 12:48:14PM +0200, Jan Beulich wrote:
>>>> --- /dev/null
>>>> +++ b/xen/include/asm-x86/asm-defns.h
>>>
>>> Maybe this could be asm-insn.h or a different name? I find it
>>> confusing to have asm-defns.h and an asm_defs.h.
>>
>> While indeed I anticipated a reply to this effect, I don't consider
>> asm-insn.h or asm-macros.h suitable: We don't want to limit this
>> header to a more narrow purpose than "all sorts of definition", I
>> don't think. Hence I chose that name despite its similarity to the
>> C header's one.
> 
> I think it's confusing, but I also think the whole magic we do with
> asm includes is already confusing (me), so if you and Andrew agree
> this is the best name I'm certainly fine with it. FWIW:
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Please quote the clac/stac instructions in order to match the other
> usages of ALTERNATIVE.

We're not consistently quoting when there's just a single word, see
in particular spec_ctrl_asm.h. And thinking about it again I also
don't see why we would want or need to enforce quotation when none
is needed. Therefore both here and in patch 2 I'll keep (or make,
when I touch a line anyway) things consistently unquoted where no
quotes are needed. Please let me know if your R-b holds without the
requested adjustment.

Jan

Re: [PATCH 1/4] x86: replace __ASM_{CL,ST}AC

[Roger Pau Monné]

On Fri, Jul 31, 2020 at 10:05:07AM +0200, Jan Beulich wrote:
> On 28.07.2020 11:06, Roger Pau Monné wrote:
> > On Mon, Jul 27, 2020 at 09:47:52PM +0200, Jan Beulich wrote:
> >> On 27.07.2020 16:55, Roger Pau Monné wrote:
> >>> On Wed, Jul 15, 2020 at 12:48:14PM +0200, Jan Beulich wrote:
> >>>> --- /dev/null
> >>>> +++ b/xen/include/asm-x86/asm-defns.h
> >>>
> >>> Maybe this could be asm-insn.h or a different name? I find it
> >>> confusing to have asm-defns.h and an asm_defs.h.
> >>
> >> While indeed I anticipated a reply to this effect, I don't consider
> >> asm-insn.h or asm-macros.h suitable: We don't want to limit this
> >> header to a more narrow purpose than "all sorts of definition", I
> >> don't think. Hence I chose that name despite its similarity to the
> >> C header's one.
> > 
> > I think it's confusing, but I also think the whole magic we do with
> > asm includes is already confusing (me), so if you and Andrew agree
> > this is the best name I'm certainly fine with it. FWIW:
> > 
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> > 
> > Please quote the clac/stac instructions in order to match the other
> > usages of ALTERNATIVE.
> 
> We're not consistently quoting when there's just a single word, see
> in particular spec_ctrl_asm.h. And thinking about it again I also
> don't see why we would want or need to enforce quotation when none
> is needed. Therefore both here and in patch 2 I'll keep (or make,
> when I touch a line anyway) things consistently unquoted where no
> quotes are needed. Please let me know if your R-b holds without the
> requested adjustment.

Yes, I'm fine as long as we are consistent with quoting of single word
instructions. Ideally I would like that we quote both single and multi
word for consistency, but you are the one doing the work so I'm not
going to oppose to not quoting single words.

Thanks, Roger.

Re: [PATCH 1/4] x86: replace __ASM_{CL,ST}AC

[Andrew Cooper]

On 27/07/2020 20:47, Jan Beulich wrote:
> On 27.07.2020 16:55, Roger Pau Monné wrote:
>> On Wed, Jul 15, 2020 at 12:48:14PM +0200, Jan Beulich wrote:
>>> --- /dev/null
>>> +++ b/xen/include/asm-x86/asm-defns.h
>>
>> Maybe this could be asm-insn.h or a different name? I find it
>> confusing to have asm-defns.h and an asm_defs.h.
>
> While indeed I anticipated a reply to this effect, I don't consider
> asm-insn.h or asm-macros.h suitable: We don't want to limit this
> header to a more narrow purpose than "all sorts of definition", I
> don't think. Hence I chose that name despite its similarity to the
> C header's one.

Roger is correct.  Having asm-defns.h and asm_defs.h is too confusing,
and there is already too much behind the scenes magic here.

What is the anticipated end result, file wise, because that might
highlight a better way forward.

~Andrew

Re: [PATCH 1/4] x86: replace __ASM_{CL,ST}AC

[Jan Beulich]

On 28.07.2020 15:59, Andrew Cooper wrote:
> On 27/07/2020 20:47, Jan Beulich wrote:
>> On 27.07.2020 16:55, Roger Pau Monné wrote:
>>> On Wed, Jul 15, 2020 at 12:48:14PM +0200, Jan Beulich wrote:
>>>> --- /dev/null
>>>> +++ b/xen/include/asm-x86/asm-defns.h
>>>
>>> Maybe this could be asm-insn.h or a different name? I find it
>>> confusing to have asm-defns.h and an asm_defs.h.
>>
>> While indeed I anticipated a reply to this effect, I don't consider
>> asm-insn.h or asm-macros.h suitable: We don't want to limit this
>> header to a more narrow purpose than "all sorts of definition", I
>> don't think. Hence I chose that name despite its similarity to the
>> C header's one.
> 
> Roger is correct.  Having asm-defns.h and asm_defs.h is too confusing,
> and there is already too much behind the scenes magic here.
> 
> What is the anticipated end result, file wise, because that might
> highlight a better way forward.

For one I'm afraid I don't understand "file wise" here. The one meaning
I could guess can't be it: The name of the file.

And then, "the anticipated end result" is at least ambiguous too: You
can surely see what the file contains by the end of this series, so
again this can't be meant. I have no immediate plans beyond this
series, so I can only state what I did say in reply to Roger's remark
already: "all sorts of asm definitions".

I'd also like to emphasize that asm-defns.h really is a companion of
asm_defns.h, supposed to be include only by the latter (as I think
can be seen from the patches). In this role I think its name being
as similar to its "parent" as suggested makes sufficient sense.

Jan

Re: [PATCH 1/4] x86: replace __ASM_{CL,ST}AC

[Andrew Cooper]

On 15/07/2020 11:48, Jan Beulich wrote:
> --- a/xen/arch/x86/arch.mk
> +++ b/xen/arch/x86/arch.mk
> @@ -20,6 +20,7 @@ $(call as-option-add,CFLAGS,CC,"rdrand %
>  $(call as-option-add,CFLAGS,CC,"rdfsbase %rax",-DHAVE_AS_FSGSBASE)
>  $(call as-option-add,CFLAGS,CC,"xsaveopt (%rax)",-DHAVE_AS_XSAVEOPT)
>  $(call as-option-add,CFLAGS,CC,"rdseed %eax",-DHAVE_AS_RDSEED)
> +$(call as-option-add,CFLAGS,CC,"clac",-DHAVE_AS_CLAC_STAC)

Kconfig please, rather than extending this legacy section.

That said, surely stac/clac support is old enough for us to start using
unconditionally?

Could we see about sorting a reasonable minimum toolchain version,
before we churn all the logic to deal with obsolete toolchains?

~Andrew

Re: [PATCH 1/4] x86: replace __ASM_{CL,ST}AC

[Jan Beulich]

On 28.07.2020 15:55, Andrew Cooper wrote:
> On 15/07/2020 11:48, Jan Beulich wrote:
>> --- a/xen/arch/x86/arch.mk
>> +++ b/xen/arch/x86/arch.mk
>> @@ -20,6 +20,7 @@ $(call as-option-add,CFLAGS,CC,"rdrand %
>>   $(call as-option-add,CFLAGS,CC,"rdfsbase %rax",-DHAVE_AS_FSGSBASE)
>>   $(call as-option-add,CFLAGS,CC,"xsaveopt (%rax)",-DHAVE_AS_XSAVEOPT)
>>   $(call as-option-add,CFLAGS,CC,"rdseed %eax",-DHAVE_AS_RDSEED)
>> +$(call as-option-add,CFLAGS,CC,"clac",-DHAVE_AS_CLAC_STAC)
> 
> Kconfig please, rather than extending this legacy section.

Did you forget for a moment that we're still to discuss this use of
Kconfig before we extend it to further instances? I'm pretty sure I
gave an ack to one of the respective changes of yours only on the
condition that we'd sort out whether this is indeed the way forward,
without a preset outcome (and without reasoning like "let's do it
because Linux does so").

> That said, surely stac/clac support is old enough for us to start using
> unconditionally?

Can't check right now, but I'm sure I wouldn't have introduced the
construct if we could rely on all supported tool chains to have
support for them.

> Could we see about sorting a reasonable minimum toolchain version,
> before we churn all the logic to deal with obsolete toolchains?

Who's "we" here? I see you keep proposing this every once in a
while, but I don't see who's going to do the work. The main reason
why, while I agree we should bump the base line, I don't see myself
do this is because I don't see any even just half way clear
criteria by which to decide what the new level is supposed to be.
Once again I don't think "let's follow what Linux does" is a
suitable approach.

Additionally I fear that with raising the tool chain base line,
people may start considering to raise other minimum versions.
While I'm personally quite fine building my own binutils and gcc
(and maybe a few other pieces), I don't fancy having to rebuild,
say, coreutils just to be able to build Xen.

Maybe a topic for the next community call, which isn't too far
out?

Jan

Re: [PATCH 1/4] x86: replace __ASM_{CL,ST}AC

[Jan Beulich]

On 28.07.2020 15:55, Andrew Cooper wrote:
> On 15/07/2020 11:48, Jan Beulich wrote:
>> --- a/xen/arch/x86/arch.mk
>> +++ b/xen/arch/x86/arch.mk
>> @@ -20,6 +20,7 @@ $(call as-option-add,CFLAGS,CC,"rdrand %
>>  $(call as-option-add,CFLAGS,CC,"rdfsbase %rax",-DHAVE_AS_FSGSBASE)
>>  $(call as-option-add,CFLAGS,CC,"xsaveopt (%rax)",-DHAVE_AS_XSAVEOPT)
>>  $(call as-option-add,CFLAGS,CC,"rdseed %eax",-DHAVE_AS_RDSEED)
>> +$(call as-option-add,CFLAGS,CC,"clac",-DHAVE_AS_CLAC_STAC)
> 
> Kconfig please, rather than extending this legacy section.
> 
> That said, surely stac/clac support is old enough for us to start using
> unconditionally?

It's available in binutils 2.24 and newer.

Jan

[PATCH 2/4] x86: reduce CET-SS related #ifdef-ary

[Jan Beulich]

Commit b586a81b7a90 ("x86/CET: Fix build following c/s 43b98e7190") had
to introduce a number of #ifdef-s to make the build work with older tool
chains. Introduce an assembler macro covering for tool chains not
knowing of CET-SS, allowing some conditionals where just SETSSBSY is the
problem to be dropped again.

No change to generated code.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Now that I've done this I'm not longer sure which direction is better to
follow: On one hand this introduces dead code (even if just NOPs) into
CET-SS-disabled builds. Otoh this is a step towards breaking the tool
chain version dependency of the feature.

I've also dropped conditionals around bigger chunks of code; while I
think that's preferable, I'm open to undo those parts.

--- a/xen/arch/x86/boot/x86_64.S
+++ b/xen/arch/x86/boot/x86_64.S
@@ -31,7 +31,6 @@ ENTRY(__high_start)
         jz      .L_bsp
 
         /* APs.  Set up shadow stacks before entering C. */
-#ifdef CONFIG_XEN_SHSTK
         testl   $cpufeat_mask(X86_FEATURE_XEN_SHSTK), \
                 CPUINFO_FEATURE_OFFSET(X86_FEATURE_XEN_SHSTK) + boot_cpu_data(%rip)
         je      .L_ap_shstk_done
@@ -55,7 +54,6 @@ ENTRY(__high_start)
         mov     $XEN_MINIMAL_CR4 | X86_CR4_CET, %ecx
         mov     %rcx, %cr4
         setssbsy
-#endif
 
 .L_ap_shstk_done:
         call    start_secondary
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -668,7 +668,7 @@ static void __init noreturn reinit_bsp_s
     stack_base[0] = stack;
     memguard_guard_stack(stack);
 
-    if ( IS_ENABLED(CONFIG_XEN_SHSTK) && cpu_has_xen_shstk )
+    if ( cpu_has_xen_shstk )
     {
         wrmsrl(MSR_PL0_SSP,
                (unsigned long)stack + (PRIMARY_SHSTK_SLOT + 1) * PAGE_SIZE - 8);
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -198,9 +198,7 @@ ENTRY(cr4_pv32_restore)
 
 /* See lstar_enter for entry register state. */
 ENTRY(cstar_enter)
-#ifdef CONFIG_XEN_SHSTK
         ALTERNATIVE "", "setssbsy", X86_FEATURE_XEN_SHSTK
-#endif
         /* sti could live here when we don't switch page tables below. */
         CR4_PV32_RESTORE
         movq  8(%rsp),%rax /* Restore %rax. */
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -237,9 +237,7 @@ iret_exit_to_guest:
  * %ss must be saved into the space left by the trampoline.
  */
 ENTRY(lstar_enter)
-#ifdef CONFIG_XEN_SHSTK
         ALTERNATIVE "", "setssbsy", X86_FEATURE_XEN_SHSTK
-#endif
         /* sti could live here when we don't switch page tables below. */
         movq  8(%rsp),%rax /* Restore %rax. */
         movq  $FLAT_KERNEL_SS,8(%rsp)
@@ -273,9 +271,7 @@ ENTRY(lstar_enter)
         jmp   test_all_events
 
 ENTRY(sysenter_entry)
-#ifdef CONFIG_XEN_SHSTK
         ALTERNATIVE "", "setssbsy", X86_FEATURE_XEN_SHSTK
-#endif
         /* sti could live here when we don't switch page tables below. */
         pushq $FLAT_USER_SS
         pushq $0
--- a/xen/include/asm-x86/asm-defns.h
+++ b/xen/include/asm-x86/asm-defns.h
@@ -7,3 +7,9 @@
     .byte 0x0f, 0x01, 0xcb
 .endm
 #endif
+
+#ifndef CONFIG_HAS_AS_CET_SS
+.macro setssbsy
+    .byte 0xf3, 0x0f, 0x01, 0xe8
+.endm
+#endif

Re: [PATCH 2/4] x86: reduce CET-SS related #ifdef-ary

[Roger Pau Monné]

On Wed, Jul 15, 2020 at 12:48:46PM +0200, Jan Beulich wrote:
> Commit b586a81b7a90 ("x86/CET: Fix build following c/s 43b98e7190") had
> to introduce a number of #ifdef-s to make the build work with older tool
> chains. Introduce an assembler macro covering for tool chains not
> knowing of CET-SS, allowing some conditionals where just SETSSBSY is the
> problem to be dropped again.
> 
> No change to generated code.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Looks like an improvement overall in code clarity:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Can you test on clang? Just to be on the safe side, otherwise I can
test it.

> ---
> Now that I've done this I'm not longer sure which direction is better to
> follow: On one hand this introduces dead code (even if just NOPs) into
> CET-SS-disabled builds. Otoh this is a step towards breaking the tool
> chain version dependency of the feature.
> 
> I've also dropped conditionals around bigger chunks of code; while I
> think that's preferable, I'm open to undo those parts.
> 
> --- a/xen/arch/x86/boot/x86_64.S
> +++ b/xen/arch/x86/boot/x86_64.S
> @@ -31,7 +31,6 @@ ENTRY(__high_start)
>          jz      .L_bsp
>  
>          /* APs.  Set up shadow stacks before entering C. */
> -#ifdef CONFIG_XEN_SHSTK
>          testl   $cpufeat_mask(X86_FEATURE_XEN_SHSTK), \
>                  CPUINFO_FEATURE_OFFSET(X86_FEATURE_XEN_SHSTK) + boot_cpu_data(%rip)
>          je      .L_ap_shstk_done
> @@ -55,7 +54,6 @@ ENTRY(__high_start)
>          mov     $XEN_MINIMAL_CR4 | X86_CR4_CET, %ecx
>          mov     %rcx, %cr4
>          setssbsy
> -#endif
>  
>  .L_ap_shstk_done:
>          call    start_secondary
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -668,7 +668,7 @@ static void __init noreturn reinit_bsp_s
>      stack_base[0] = stack;
>      memguard_guard_stack(stack);
>  
> -    if ( IS_ENABLED(CONFIG_XEN_SHSTK) && cpu_has_xen_shstk )
> +    if ( cpu_has_xen_shstk )
>      {
>          wrmsrl(MSR_PL0_SSP,
>                 (unsigned long)stack + (PRIMARY_SHSTK_SLOT + 1) * PAGE_SIZE - 8);
> --- a/xen/arch/x86/x86_64/compat/entry.S
> +++ b/xen/arch/x86/x86_64/compat/entry.S
> @@ -198,9 +198,7 @@ ENTRY(cr4_pv32_restore)
>  
>  /* See lstar_enter for entry register state. */
>  ENTRY(cstar_enter)
> -#ifdef CONFIG_XEN_SHSTK
>          ALTERNATIVE "", "setssbsy", X86_FEATURE_XEN_SHSTK
> -#endif
>          /* sti could live here when we don't switch page tables below. */
>          CR4_PV32_RESTORE
>          movq  8(%rsp),%rax /* Restore %rax. */
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -237,9 +237,7 @@ iret_exit_to_guest:
>   * %ss must be saved into the space left by the trampoline.
>   */
>  ENTRY(lstar_enter)
> -#ifdef CONFIG_XEN_SHSTK
>          ALTERNATIVE "", "setssbsy", X86_FEATURE_XEN_SHSTK

Should the setssbsy be quoted, or it doesn't matter? I'm asking
because the same construction used by CLAC/STAC doesn't quote the
instruction.

Roger.

Re: [PATCH 2/4] x86: reduce CET-SS related #ifdef-ary

[Jan Beulich]

On 27.07.2020 17:00, Roger Pau Monné wrote:
> On Wed, Jul 15, 2020 at 12:48:46PM +0200, Jan Beulich wrote:
>> Commit b586a81b7a90 ("x86/CET: Fix build following c/s 43b98e7190") had
>> to introduce a number of #ifdef-s to make the build work with older tool
>> chains. Introduce an assembler macro covering for tool chains not
>> knowing of CET-SS, allowing some conditionals where just SETSSBSY is the
>> problem to be dropped again.
>>
>> No change to generated code.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Looks like an improvement overall in code clarity:
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> Can you test on clang? Just to be on the safe side, otherwise I can
> test it.

Works with 5.<whatever> that I have on one of my boxes.

>> --- a/xen/arch/x86/x86_64/entry.S
>> +++ b/xen/arch/x86/x86_64/entry.S
>> @@ -237,9 +237,7 @@ iret_exit_to_guest:
>>    * %ss must be saved into the space left by the trampoline.
>>    */
>>   ENTRY(lstar_enter)
>> -#ifdef CONFIG_XEN_SHSTK
>>           ALTERNATIVE "", "setssbsy", X86_FEATURE_XEN_SHSTK
> 
> Should the setssbsy be quoted, or it doesn't matter? I'm asking
> because the same construction used by CLAC/STAC doesn't quote the
> instruction.

I actually thought we consistently quote these. It doesn't matter
as long as it's a single word. Quoting becomes necessary when
there are e.g. blanks involved, which happens for insns with
operands.

Jan

Re: [PATCH 2/4] x86: reduce CET-SS related #ifdef-ary

[Roger Pau Monné]

On Mon, Jul 27, 2020 at 09:50:23PM +0200, Jan Beulich wrote:
> On 27.07.2020 17:00, Roger Pau Monné wrote:
> > On Wed, Jul 15, 2020 at 12:48:46PM +0200, Jan Beulich wrote:
> > Should the setssbsy be quoted, or it doesn't matter? I'm asking
> > because the same construction used by CLAC/STAC doesn't quote the
> > instruction.
> 
> I actually thought we consistently quote these. It doesn't matter
> as long as it's a single word. Quoting becomes necessary when
> there are e.g. blanks involved, which happens for insns with
> operands.

Could you quote the usages in patch 1 please then in order to be
consistent?

Thanks, Roger.

Re: [PATCH 2/4] x86: reduce CET-SS related #ifdef-ary

[Andrew Cooper]

On 15/07/2020 11:48, Jan Beulich wrote:
> Commit b586a81b7a90 ("x86/CET: Fix build following c/s 43b98e7190") had
> to introduce a number of #ifdef-s to make the build work with older tool
> chains. Introduce an assembler macro covering for tool chains not
> knowing of CET-SS, allowing some conditionals where just SETSSBSY is the
> problem to be dropped again.
>
> No change to generated code.



>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Now that I've done this I'm not longer sure which direction is better to
> follow: On one hand this introduces dead code (even if just NOPs) into
> CET-SS-disabled builds. Otoh this is a step towards breaking the tool
> chain version dependency of the feature.

The toolchain dependency can't be broken, because of incssp and wrss in C.

There is 0 value and added complexity to trying to partially support
legacy toolchains.  Furthermore, this adds a pile of nops into builds
which have specifically opted out of CONFIG_XEN_SHSTK, which isn't ideal
for embedded usecases.

As a consequence, I think its better to keep things consistent with how
they are now.

One thing I already considered was to make cpu_has_xen_shstk return
false for !CONFIG_XEN_SHSTK, which subsumes at least one hunk in this
change.

> --- a/xen/arch/x86/x86_64/compat/entry.S
> +++ b/xen/arch/x86/x86_64/compat/entry.S
> @@ -198,9 +198,7 @@ ENTRY(cr4_pv32_restore)
>  
>  /* See lstar_enter for entry register state. */
>  ENTRY(cstar_enter)
> -#ifdef CONFIG_XEN_SHSTK
>          ALTERNATIVE "", "setssbsy", X86_FEATURE_XEN_SHSTK
> -#endif

I can't currently think of any option better than leaving these ifdef's
in place, other than perhaps

#ifdef CONFIG_XEN_SHSTK
# define MAYBE_SETSSBSY ALTERNATIVE "", "setssbsy", X86_FEATURE_XEN_SHSTK
#else
# define MAYBE_SETSSBSY
#endif

and I don't like it much.

The think is that everything present there is semantically relevant
information, and dropping it makes the code worse rather than better.

~Andrew

Re: [PATCH 2/4] x86: reduce CET-SS related #ifdef-ary

[Jan Beulich]

On 28.07.2020 16:29, Andrew Cooper wrote:
> On 15/07/2020 11:48, Jan Beulich wrote:
>> Now that I've done this I'm not longer sure which direction is better to
>> follow: On one hand this introduces dead code (even if just NOPs) into
>> CET-SS-disabled builds. Otoh this is a step towards breaking the tool
>> chain version dependency of the feature.
> 
> The toolchain dependency can't be broken, because of incssp and wrss in C.
> 
> There is 0 value and added complexity to trying to partially support
> legacy toolchains.

Complexity: Yes. Zero value - surely not. I'm having a hard time seeing
why you may think so. Would you mind explaining yourself?

>  Furthermore, this adds a pile of nops into builds
> which have specifically opted out of CONFIG_XEN_SHSTK, which isn't ideal
> for embedded usecases.
> 
> As a consequence, I think its better to keep things consistent with how
> they are now.
> 
> One thing I already considered was to make cpu_has_xen_shstk return
> false for !CONFIG_XEN_SHSTK, which subsumes at least one hunk in this
> change.

One is better than nothing, but still pretty little.

>> --- a/xen/arch/x86/x86_64/compat/entry.S
>> +++ b/xen/arch/x86/x86_64/compat/entry.S
>> @@ -198,9 +198,7 @@ ENTRY(cr4_pv32_restore)
>>   
>>   /* See lstar_enter for entry register state. */
>>   ENTRY(cstar_enter)
>> -#ifdef CONFIG_XEN_SHSTK
>>           ALTERNATIVE "", "setssbsy", X86_FEATURE_XEN_SHSTK
>> -#endif
> 
> I can't currently think of any option better than leaving these ifdef's
> in place, other than perhaps
> 
> #ifdef CONFIG_XEN_SHSTK
> # define MAYBE_SETSSBSY ALTERNATIVE "", "setssbsy", X86_FEATURE_XEN_SHSTK
> #else
> # define MAYBE_SETSSBSY
> #endif
> 
> and I don't like it much.

Neither do I. Then we'd also switch STAC/CLAC to MAYBE_STAC / MAYBE_CLAC.

> The think is that everything present there is semantically relevant
> information, and dropping it makes the code worse rather than better.

Everything? I don't see why the #ifdef-s are semantically relevant
(the needed infor is already conveyed by the ALTERNATIVE and its
arguments). I consider them primarily harming readability, and thus I
think we should strive to eliminate them if we can. Hence this patch
...

Jan

[PATCH 3/4] x86: drop ASM_{CL,ST}AC

[Jan Beulich]

Use ALTERNATIVE directly, such that at the use sites it is visible that
alternative code patching is in use. Similarly avoid hiding the fact in
SAVE_ALL.

No change to generated code.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -2165,9 +2165,9 @@ void activate_debugregs(const struct vcp
 void asm_domain_crash_synchronous(unsigned long addr)
 {
     /*
-     * We need clear AC bit here because in entry.S AC is set
-     * by ASM_STAC to temporarily allow accesses to user pages
-     * which is prevented by SMAP by default.
+     * We need to clear AC bit here because in entry.S it gets set to
+     * temporarily allow accesses to user pages which is prevented by
+     * SMAP by default.
      *
      * For some code paths, where this function is called, clac()
      * is not needed, but adding clac() here instead of each place
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -12,7 +12,7 @@
 #include <irq_vectors.h>
 
 ENTRY(entry_int82)
-        ASM_CLAC
+        ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
         pushq $0
         movl  $HYPERCALL_VECTOR, 4(%rsp)
         SAVE_ALL compat=1 /* DPL1 gate, restricted to 32bit PV guests only. */
@@ -285,7 +285,7 @@ ENTRY(compat_int80_direct_trap)
 compat_create_bounce_frame:
         ASSERT_INTERRUPTS_ENABLED
         mov   %fs,%edi
-        ASM_STAC
+        ALTERNATIVE "", stac, X86_FEATURE_XEN_SMAP
         testb $2,UREGS_cs+8(%rsp)
         jz    1f
         /* Push new frame at registered guest-OS stack base. */
@@ -332,7 +332,7 @@ compat_create_bounce_frame:
         movl  TRAPBOUNCE_error_code(%rdx),%eax
 .Lft8:  movl  %eax,%fs:(%rsi)           # ERROR CODE
 1:
-        ASM_CLAC
+        ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
         /* Rewrite our stack frame and return to guest-OS mode. */
         /* IA32 Ref. Vol. 3: TF, VM, RF and NT flags are cleared on trap. */
         andl  $~(X86_EFLAGS_VM|X86_EFLAGS_RF|\
@@ -372,7 +372,7 @@ compat_crash_page_fault_4:
         addl  $4,%esi
 compat_crash_page_fault:
 .Lft14: mov   %edi,%fs
-        ASM_CLAC
+        ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
         movl  %esi,%edi
         call  show_page_walk
         jmp   dom_crash_sync_extable
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -277,7 +277,7 @@ ENTRY(sysenter_entry)
         pushq $0
         pushfq
 GLOBAL(sysenter_eflags_saved)
-        ASM_CLAC
+        ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
         pushq $3 /* ring 3 null cs */
         pushq $0 /* null rip */
         pushq $0
@@ -331,7 +331,7 @@ UNLIKELY_END(sysenter_gpf)
         jmp   .Lbounce_exception
 
 ENTRY(int80_direct_trap)
-        ASM_CLAC
+        ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
         pushq $0
         movl  $0x80, 4(%rsp)
         SAVE_ALL
@@ -450,7 +450,7 @@ __UNLIKELY_END(create_bounce_frame_bad_s
 
         subq  $7*8,%rsi
         movq  UREGS_ss+8(%rsp),%rax
-        ASM_STAC
+        ALTERNATIVE "", stac, X86_FEATURE_XEN_SMAP
         movq  VCPU_domain(%rbx),%rdi
         STORE_GUEST_STACK(rax,6)        # SS
         movq  UREGS_rsp+8(%rsp),%rax
@@ -488,7 +488,7 @@ __UNLIKELY_END(create_bounce_frame_bad_s
         STORE_GUEST_STACK(rax,1)        # R11
         movq  UREGS_rcx+8(%rsp),%rax
         STORE_GUEST_STACK(rax,0)        # RCX
-        ASM_CLAC
+        ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
 
 #undef STORE_GUEST_STACK
 
@@ -525,11 +525,11 @@ domain_crash_page_fault_2x8:
 domain_crash_page_fault_1x8:
         addq  $8,%rsi
 domain_crash_page_fault_0x8:
-        ASM_CLAC
+        ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
         movq  %rsi,%rdi
         call  show_page_walk
 ENTRY(dom_crash_sync_extable)
-        ASM_CLAC
+        ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
         # Get out of the guest-save area of the stack.
         GET_STACK_END(ax)
         leaq  STACK_CPUINFO_FIELD(guest_cpu_user_regs)(%rax),%rsp
@@ -587,7 +587,8 @@ UNLIKELY_END(exit_cr3)
         iretq
 
 ENTRY(common_interrupt)
-        SAVE_ALL CLAC
+        ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
+        SAVE_ALL
 
         GET_STACK_END(14)
 
@@ -619,7 +620,8 @@ ENTRY(page_fault)
         movl  $TRAP_page_fault,4(%rsp)
 /* No special register assumptions. */
 GLOBAL(handle_exception)
-        SAVE_ALL CLAC
+        ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
+        SAVE_ALL
 
         GET_STACK_END(14)
 
@@ -824,7 +826,8 @@ ENTRY(entry_CP)
 ENTRY(double_fault)
         movl  $TRAP_double_fault,4(%rsp)
         /* Set AC to reduce chance of further SMAP faults */
-        SAVE_ALL STAC
+        ALTERNATIVE "", stac, X86_FEATURE_XEN_SMAP
+        SAVE_ALL
 
         GET_STACK_END(14)
 
@@ -857,7 +860,8 @@ ENTRY(nmi)
         pushq $0
         movl  $TRAP_nmi,4(%rsp)
 handle_ist_exception:
-        SAVE_ALL CLAC
+        ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
+        SAVE_ALL
 
         GET_STACK_END(14)
 
--- a/xen/include/asm-x86/asm_defns.h
+++ b/xen/include/asm-x86/asm_defns.h
@@ -200,16 +200,6 @@ register unsigned long current_stack_poi
         UNLIKELY_END_SECTION "\n"          \
         ".Llikely." #tag ".%=:"
 
-#endif
-
-#ifdef __ASSEMBLY__
-.macro ASM_STAC
-    ALTERNATIVE "", stac, X86_FEATURE_XEN_SMAP
-.endm
-.macro ASM_CLAC
-    ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
-.endm
-#else
 static always_inline void clac(void)
 {
     /* Note: a barrier is implicit in alternative() */
@@ -224,18 +214,7 @@ static always_inline void stac(void)
 #endif
 
 #ifdef __ASSEMBLY__
-.macro SAVE_ALL op, compat=0
-.ifeqs "\op", "CLAC"
-        ASM_CLAC
-.else
-.ifeqs "\op", "STAC"
-        ASM_STAC
-.else
-.ifnb \op
-        .err
-.endif
-.endif
-.endif
+.macro SAVE_ALL compat=0
         addq  $-(UREGS_error_code-UREGS_r15), %rsp
         cld
         movq  %rdi,UREGS_rdi(%rsp)

Re: [PATCH 3/4] x86: drop ASM_{CL,ST}AC

[Roger Pau Monné]

On Wed, Jul 15, 2020 at 12:49:04PM +0200, Jan Beulich wrote:
> Use ALTERNATIVE directly, such that at the use sites it is visible that
> alternative code patching is in use. Similarly avoid hiding the fact in
> SAVE_ALL.
> 
> No change to generated code.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I think the code is correct, I'm not sure however whether open coding
ASM_CLAC/STAC is better than what we currently do. I will leave for
Andrew to decide, since he is more knowledgeable of such piece of
code.

Thanks, Roger.

Re: [PATCH 3/4] x86: drop ASM_{CL,ST}AC

[Andrew Cooper]

On 15/07/2020 11:49, Jan Beulich wrote:
> Use ALTERNATIVE directly, such that at the use sites it is visible that
> alternative code patching is in use. Similarly avoid hiding the fact in
> SAVE_ALL.
>
> No change to generated code.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Definitely +1 to not hiding the STAC/CLAC in SAVE_ALL.  I've been
meaning to undo that mistake for ages.

OOI, what made you change your mind?  I'm pleased that you have.

>
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -2165,9 +2165,9 @@ void activate_debugregs(const struct vcp
>  void asm_domain_crash_synchronous(unsigned long addr)
>  {
>      /*
> -     * We need clear AC bit here because in entry.S AC is set
> -     * by ASM_STAC to temporarily allow accesses to user pages
> -     * which is prevented by SMAP by default.
> +     * We need to clear AC bit here because in entry.S it gets set to
> +     * temporarily allow accesses to user pages which is prevented by
> +     * SMAP by default.

As you're adjusting the text, It should read "We need to clear the AC
bit ..."

But I also think it would be clearer to say that exception fixup may
leave user access enabled, which we fix up here by unconditionally
disabling user access.

Preferably with this rewritten, Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

Re: [PATCH 3/4] x86: drop ASM_{CL,ST}AC

[Jan Beulich]

On 28.07.2020 16:51, Andrew Cooper wrote:
> On 15/07/2020 11:49, Jan Beulich wrote:
>> Use ALTERNATIVE directly, such that at the use sites it is visible that
>> alternative code patching is in use. Similarly avoid hiding the fact in
>> SAVE_ALL.
>>
>> No change to generated code.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Definitely +1 to not hiding the STAC/CLAC in SAVE_ALL.  I've been
> meaning to undo that mistake for ages.
> 
> OOI, what made you change your mind?  I'm pleased that you have.

This, to me, is a direct consequence of dropping ASM_STAC / ASM_CLAC:
If we don't want the fact of a use of ALTERNATIVE be hidden there, it
also shouldn't be hidden inside SAVE_ALL.

>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -2165,9 +2165,9 @@ void activate_debugregs(const struct vcp
>>   void asm_domain_crash_synchronous(unsigned long addr)
>>   {
>>       /*
>> -     * We need clear AC bit here because in entry.S AC is set
>> -     * by ASM_STAC to temporarily allow accesses to user pages
>> -     * which is prevented by SMAP by default.
>> +     * We need to clear AC bit here because in entry.S it gets set to
>> +     * temporarily allow accesses to user pages which is prevented by
>> +     * SMAP by default.
> 
> As you're adjusting the text, It should read "We need to clear the AC
> bit ..."
> 
> But I also think it would be clearer to say that exception fixup may
> leave user access enabled, which we fix up here by unconditionally
> disabling user access.

Can do.

> Preferably with this rewritten, Reviewed-by: Andrew Cooper
> <andrew.cooper3@citrix.com>

Thanks.

Jan

[PATCH 4/4] x86: fold indirect_thunk_asm.h into asm-defns.h

[Jan Beulich]

There's little point in having two separate headers both getting
included by asm_defns.h. This in particular reduces the number of
instances of guarding asm(".include ...") suitably in such dual use
headers.

No change to generated code.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/Makefile
+++ b/xen/Makefile
@@ -139,7 +139,7 @@ ifeq ($(TARGET_ARCH),x86)
 t1 = $(call as-insn,$(CC),".L0: .L1: .skip (.L1 - .L0)",,-no-integrated-as)
 
 # Check whether clang asm()-s support .include.
-t2 = $(call as-insn,$(CC) -I$(BASEDIR)/include,".include \"asm-x86/indirect_thunk_asm.h\"",,-no-integrated-as)
+t2 = $(call as-insn,$(CC) -I$(BASEDIR)/include,".include \"asm-x86/asm-defns.h\"",,-no-integrated-as)
 
 # Check whether clang keeps .macro-s between asm()-s:
 # https://bugs.llvm.org/show_bug.cgi?id=36110
--- a/xen/include/asm-x86/asm-defns.h
+++ b/xen/include/asm-x86/asm-defns.h
@@ -13,3 +13,40 @@
     .byte 0xf3, 0x0f, 0x01, 0xe8
 .endm
 #endif
+
+.macro INDIRECT_BRANCH insn:req arg:req
+/*
+ * Create an indirect branch.  insn is one of call/jmp, arg is a single
+ * register.
+ *
+ * With no compiler support, this degrades into a plain indirect call/jmp.
+ * With compiler support, dispatch to the correct __x86_indirect_thunk_*
+ */
+    .if CONFIG_INDIRECT_THUNK == 1
+
+        $done = 0
+        .irp reg, ax, cx, dx, bx, bp, si, di, 8, 9, 10, 11, 12, 13, 14, 15
+        .ifeqs "\arg", "%r\reg"
+            \insn __x86_indirect_thunk_r\reg
+            $done = 1
+           .exitm
+        .endif
+        .endr
+
+        .if $done != 1
+            .error "Bad register arg \arg"
+        .endif
+
+    .else
+        \insn *\arg
+    .endif
+.endm
+
+/* Convenience wrappers. */
+.macro INDIRECT_CALL arg:req
+    INDIRECT_BRANCH call \arg
+.endm
+
+.macro INDIRECT_JMP arg:req
+    INDIRECT_BRANCH jmp \arg
+.endm
--- a/xen/include/asm-x86/asm_defns.h
+++ b/xen/include/asm-x86/asm_defns.h
@@ -22,7 +22,6 @@
 asm ( "\t.equ CONFIG_INDIRECT_THUNK, "
       __stringify(IS_ENABLED(CONFIG_INDIRECT_THUNK)) );
 #endif
-#include <asm/indirect_thunk_asm.h>
 
 #ifndef __ASSEMBLY__
 void ret_from_intr(void);
--- a/xen/include/asm-x86/indirect_thunk_asm.h
+++ /dev/null
@@ -1,53 +0,0 @@
-/*
- * Trickery to allow this header to be included at the C level, to permit
- * proper dependency tracking in .*.o.d files, while still having it contain
- * assembler only macros.
- */
-#ifndef __ASSEMBLY__
-# if 0
-  .if 0
-# endif
-asm ( "\t.include \"asm/indirect_thunk_asm.h\"" );
-# if 0
-  .endif
-# endif
-#else
-
-.macro INDIRECT_BRANCH insn:req arg:req
-/*
- * Create an indirect branch.  insn is one of call/jmp, arg is a single
- * register.
- *
- * With no compiler support, this degrades into a plain indirect call/jmp.
- * With compiler support, dispatch to the correct __x86_indirect_thunk_*
- */
-    .if CONFIG_INDIRECT_THUNK == 1
-
-        $done = 0
-        .irp reg, ax, cx, dx, bx, bp, si, di, 8, 9, 10, 11, 12, 13, 14, 15
-        .ifeqs "\arg", "%r\reg"
-            \insn __x86_indirect_thunk_r\reg
-            $done = 1
-           .exitm
-        .endif
-        .endr
-
-        .if $done != 1
-            .error "Bad register arg \arg"
-        .endif
-
-    .else
-        \insn *\arg
-    .endif
-.endm
-
-/* Convenience wrappers. */
-.macro INDIRECT_CALL arg:req
-    INDIRECT_BRANCH call \arg
-.endm
-
-.macro INDIRECT_JMP arg:req
-    INDIRECT_BRANCH jmp \arg
-.endm
-
-#endif

Re: [PATCH 4/4] x86: fold indirect_thunk_asm.h into asm-defns.h

[Roger Pau Monné]

On Wed, Jul 15, 2020 at 12:49:40PM +0200, Jan Beulich wrote:
> There's little point in having two separate headers both getting
> included by asm_defns.h. This in particular reduces the number of
> instances of guarding asm(".include ...") suitably in such dual use
> headers.
> 
> No change to generated code.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

LGTM:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Some testing with clang might be required, as with the other patch.

Thanks, Roger.