Re: [PATCH v2 1/7] x86: Add support for STAC/CLAC instructions

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 23/04/14 15:34, Feng Wu wrote:
> The STAC/CLAC instructions are only available when SMAP is enabled,
> but on the other hand they aren't needed if SMAP is not available,
> or before we start to run userspace, in that case, the functions and
> macros do nothing.
>
> Signed-off-by: Feng Wu <feng.wu@intel.com>
> ---
>  xen/arch/x86/x86_64/asm-offsets.c |  1 +
>  xen/include/asm-x86/asm_defns.h   | 43 +++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-x86/cpufeature.h  |  4 ++++
>  3 files changed, 48 insertions(+)
>
> diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c
> index b0098b3..fa4cbb6 100644
> --- a/xen/arch/x86/x86_64/asm-offsets.c
> +++ b/xen/arch/x86/x86_64/asm-offsets.c
> @@ -160,6 +160,7 @@ void __dummy__(void)
>      BLANK();
>  
>      OFFSET(CPUINFO86_ext_features, struct cpuinfo_x86, x86_capability[1]);
> +    OFFSET(CPUINFO86_leaf7_features, struct cpuinfo_x86, x86_capability[7]);
>      BLANK();
>  
>      OFFSET(MB_flags, multiboot_info_t, flags);
> diff --git a/xen/include/asm-x86/asm_defns.h b/xen/include/asm-x86/asm_defns.h
> index a4601ba..0f18728 100644
> --- a/xen/include/asm-x86/asm_defns.h
> +++ b/xen/include/asm-x86/asm_defns.h
> @@ -7,6 +7,8 @@
>  #include <asm/asm-offsets.h>
>  #endif
>  #include <asm/processor.h>
> +#include <xen/stringify.h>
> +#include <asm/cpufeature.h>
>  
>  #ifndef __ASSEMBLY__
>  void ret_from_intr(void);
> @@ -103,4 +105,45 @@ void ret_from_intr(void);
>  
>  #endif
>  
> +/* "Raw" instruction opcodes */
> +#define __ASM_CLAC      .byte 0x0f,0x01,0xca
> +#define __ASM_STAC      .byte 0x0f,0x01,0xcb
> +
> +#ifdef __ASSEMBLY__
> +#define ASM_AC(op)                                       \
> +        pushq %rax;                                      \
> +        leaq boot_cpu_data(%rip), %rax;                  \
> +        btl $X86_FEATURE_SMAP-7*32, CPUINFO86_leaf7_features(%rax);  \
> +        jnc 881f;                                        \
> +        op;                                              \
> +881:    popq %rax

While it is unlikely to change going forwards, $(X86_FEATURE_SMAP & 31)
looks more obviously correct.

Also, given that boot_cpu_data(%rip) is an assembler know constant
value, and CPUINFO86_leaf7_features is a constant offset from that, is
there any way of persuading the assembler to conjoin the two and forgo
the push, lea and pop.  (I have had a go, but lack sufficient caffeine
this early in the day)

~Andrew

> +
> +#define ASM_STAC ASM_AC(__ASM_STAC)
> +#define ASM_CLAC ASM_AC(__ASM_CLAC)
> +#else
> +#define ASM_AC(op, prefix)                                         \
> +        "\npushq " __stringify(prefix) "rax\n\t"                   \
> +        "leaq boot_cpu_data(" __stringify(prefix) "rip),"          \
> +        __stringify(prefix) "rax\n\t"                              \
> +        "btl $" __stringify(X86_FEATURE_SMAP) "-7*32,"             \
> +        __stringify(CPUINFO86_leaf7_features) "("                  \
> +        __stringify(prefix) "rax)\n\t"                             \
> +        "jnc 881f\n\t"                                             \
> +         __stringify(op) "\n\t"                                    \
> +"881:    popq " __stringify(prefix) "rax"
> +
> +#define ASM_STAC(prefix) ASM_AC(__ASM_STAC, prefix)
> +#define ASM_CLAC(prefix) ASM_AC(__ASM_CLAC, prefix)
> +
> +static inline void clac(void)
> +{
> +    asm volatile (ASM_CLAC(%));
> +}
> +
> +static inline void stac(void)
> +{
> +    asm volatile (ASM_STAC(%));
> +}
> +#endif
> +
>  #endif /* __X86_ASM_DEFNS_H__ */
> diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
> index 0c4d6c1..3dfb875 100644
> --- a/xen/include/asm-x86/cpufeature.h
> +++ b/xen/include/asm-x86/cpufeature.h
> @@ -7,7 +7,9 @@
>  #ifndef __ASM_I386_CPUFEATURE_H
>  #define __ASM_I386_CPUFEATURE_H
>  
> +#ifndef __ASSEMBLY__
>  #include <xen/bitops.h>
> +#endif
>  
>  #define NCAPINTS	8	/* N 32-bit words worth of info */
>  
> @@ -151,6 +153,7 @@
>  #define X86_FEATURE_ADX		(7*32+19) /* ADCX, ADOX instructions */
>  #define X86_FEATURE_SMAP	(7*32+20) /* Supervisor Mode Access Prevention */
>  
> +#ifndef __ASSEMBLY__
>  #define cpu_has(c, bit)		test_bit(bit, (c)->x86_capability)
>  #define boot_cpu_has(bit)	test_bit(bit, boot_cpu_data.x86_capability)
>  #define cpufeat_mask(idx)       (1u << ((idx) & 31))
> @@ -209,6 +212,7 @@
>  #define cpu_has_vmx		boot_cpu_has(X86_FEATURE_VMXE)
>  
>  #define cpu_has_cpuid_faulting	boot_cpu_has(X86_FEATURE_CPUID_FAULTING)
> +#endif
>  
>  #endif /* __ASM_I386_CPUFEATURE_H */
>