Re: [Xen-devel] [PATCH v1 1/6] x86: Add support for STAC/CLAC instructions

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.04.14 at 15:01, <feng.wu@xxxxxxxxx> wrote:
> --- a/xen/include/asm-x86/x86_64/asm_defns.h
> +++ b/xen/include/asm-x86/x86_64/asm_defns.h

These changes should go into xen/include/asm-x86/asm_defns.h; the
x86-64 one should get merged into the previously common one sooner
or later, so let's not force that merging patch to be bigger than it needs
to be.

> @@ -228,4 +228,74 @@ __asm__(                                        \
>  # define _ASM_EX(p) #p "-."
>  #endif
>  
> +/* "Raw" instruction opcodes */
> +#define __ASM_CLAC      .byte 0x0f,0x01,0xca
> +#define __ASM_STAC      .byte 0x0f,0x01,0xcb
> +
> +/* Indirect stringification.  Doing two levels allows the parameter to be a
> + * macro itself.  For example, compile with -DFOO=bar, __stringify(FOO)
> + * converts to "bar".
> + */
> +#define __stringify_1(x...)     #x
> +#define __stringify(x...)       __stringify_1(x)

Please use xen/stringify.h rather than repeating its defintions.

> +
> +#ifdef __ASSEMBLY__
> +#define X86_FEATURE_SMAP    (7*32+20)

No - how would be spot this if we were to re-arrange the feature
array? Either use (or make usable) the definitions in cpufeature.h,
or propagate the necessary values through asm-offsets.h.

> +#define ASM_STAC                                         \
> +        pushq %rax;                                      \
> +        leaq boot_cpu_data(%rip),%rax;                   \
> +        btl $X86_FEATURE_SMAP-7*32, CPUINFO86_leaf7_features(%rax);  \
> +        jnc 881f;                                        \
> +        movq %cr4,%rax;                                  \
> +        testl $X86_CR4_SMAP,%eax;                        \
> +        jz 881f;                                         \
> +        __ASM_STAC;                                      \
> +881:    popq %rax
> +
> +#define ASM_CLAC                                         \
> +        pushq %rax;                                      \
> +        leaq boot_cpu_data(%rip),%rax;                   \
> +        btl $X86_FEATURE_SMAP-7*32, CPUINFO86_leaf7_features(%rax);  \
> +        jnc 881f;                                        \
> +        movq %cr4,%rax;                                  \
> +        testl $X86_CR4_SMAP,%eax;                        \
> +        jz 881f;                                         \
> +        __ASM_CLAC;                                      \
> +881:    popq %rax

The only difference between the two macros appears to be the final
instruction - please define one macro with an argument, and then
make the two definitions here simple wrappers around that macro.

That said, the macro contents itself is horrible too: A control register
access and two conditional branches in code intended to be used in
fast paths? Definitely not an option. Even the simplest possible
solution - adding a global flag to be checked here - would already be
questionable. Hence I think you should at least consider porting over
proper instruction patching abstraction from Linux.

> +#else
> +#define ASM_STAC                                         \
> +        "\npushq %%rax\n\t"                              \
> +        "leaq boot_cpu_data(%%rip),%%rax\n\t"            \
> +        "btl $" __stringify(X86_FEATURE_SMAP) "-7*32,"   \
> +        __stringify(CPUINFO86_leaf7_features) "(%%rax)\n\t"  \
> +        "jnc 881f\n\t"                                   \
> +        "movq %%cr4,%%rax\n\t"                           \
> +        "testl $" __stringify(X86_CR4_SMAP) ",%%eax\n\t" \
> +        "jz 881f\n\t"                                    \
> +         __stringify(__ASM_STAC) "\n\t"                  \
> +"881:    popq %%rax"
> +
> +#define ASM_CLAC                                         \
> +        "\npushq %%rax\n\t"                              \
> +        "leaq boot_cpu_data(%%rip),%%rax\n\t"            \
> +        "btl $" __stringify(X86_FEATURE_SMAP) "-7*32,"   \
> +        __stringify(CPUINFO86_leaf7_features) "(%%rax)\n\t"  \
> +        "jnc 881f\n\t"                                   \
> +        "movq %%cr4,%%rax\n\t"                           \
> +        "testl $" __stringify(X86_CR4_SMAP) ",%%eax\n\t" \
> +        "jz 881f\n\t"                                    \
> +         __stringify(__ASM_CLAC) "\n\t"                  \
> +"881:   popq %%rax"

All the same applies to these of course.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel