| Message ID | 58615a18-7f81-c000-d499-1a49f4752879@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: some assembler macro rework | expand |
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.
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
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.
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
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
--- 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
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.