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

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

[Andrew Cooper]

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 */
>  

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

[Jan Beulich]

>>> On 23.04.14 at 16:34, <feng.wu@intel.com> wrote:
> @@ -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

So why are you pushing/popping %rax here? There's no need for the
lea.

And the hard coded 7 here should be replaced too; I don't see a need
for CPUINFO86_leaf7_features either - just calculate everything you
need from X86_FEATURE_SMAP (these are all constants, so other than
the expression getting a little long there's nothing keeping this from
being a single btl).

> +#define ASM_STAC(prefix) ASM_AC(__ASM_STAC, prefix)
> +#define ASM_CLAC(prefix) ASM_AC(__ASM_CLAC, prefix)

What is "prefix" good for here, i.e. why can't you put the % right
in the macro?

Jan

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

[Wu, Feng]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Wednesday, April 23, 2014 6:06 PM
> To: Wu, Feng
> Cc: xen-devel@lists.xen.org; JBeulich@suse.com; Tian, Kevin; Dong, Eddie;
> Nakajima, Jun; ian.campbell@citrix.com
> Subject: Re: [PATCH v2 1/7] x86: Add support for STAC/CLAC instructions
> 
> 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.

Okay.
> 
> 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

I thought about this idea before posting this version, but I didn't get a good
solution. I will continue to find a better way to handle this.

> 
> > +
> > +#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 */
> >

Thanks,
Feng

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

[Wu, Feng]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, April 23, 2014 6:17 PM
> To: Wu, Feng
> Cc: andrew.cooper3@citrix.com; ian.campbell@citrix.com; Dong, Eddie;
> Nakajima, Jun; Tian, Kevin; xen-devel@lists.xen.org
> Subject: Re: [PATCH v2 1/7] x86: Add support for STAC/CLAC instructions
> 
> >>> On 23.04.14 at 16:34, <feng.wu@intel.com> wrote:
> > @@ -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
> 
> So why are you pushing/popping %rax here? There's no need for the
> lea.
> 
> And the hard coded 7 here should be replaced too; I don't see a need
> for CPUINFO86_leaf7_features either - just calculate everything you
> need from X86_FEATURE_SMAP (these are all constants, so other than
> the expression getting a little long there's nothing keeping this from
> being a single btl).

In my understanding, CPUINFO86_leaf7_features is the offset for
x86_capability[i] in struct cpuinfo_x86{}, seems we cannot get the
right offset only from X86_FEATURE_SMAP?

> 
> > +#define ASM_STAC(prefix) ASM_AC(__ASM_STAC, prefix)
> > +#define ASM_CLAC(prefix) ASM_AC(__ASM_CLAC, prefix)
> 
> What is "prefix" good for here, i.e. why can't you put the % right
> in the macro?

Because this macro will be used in the basic inline assembly (use "%" as the register prefix)
and extended assembly (use "%%" as the register prefix).

> 
> Jan

Thanks,
Feng

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

[Jan Beulich]

>>> On 23.04.14 at 14:11, <feng.wu@intel.com> wrote:
>> > +#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.
> 
> Okay.
>> 
>> 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
> 
> I thought about this idea before posting this version, but I didn't get a 
> good
> solution. I will continue to find a better way to handle this.

btl $X86_FEATURE_SMAP & 31, ((X86_FEATURE_SMAP >> 3) & ~3)+CPUINFO86_features+boot_cpu_data(%rip)

(at once replacing CPUINFO86_ext_features with CPUINFO86_features,
perhaps also changing the [inconsistent] name prefix; likely a suitable
macro could be created usable both here and wherever
CPUINFO86_ext_features is being used currently)

Jan

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

[Jan Beulich]

>>> On 23.04.14 at 14:32, <feng.wu@intel.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> >>> On 23.04.14 at 16:34, <feng.wu@intel.com> wrote:
>> > +/* "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
>> 
>> So why are you pushing/popping %rax here? There's no need for the
>> lea.
>> 
>> And the hard coded 7 here should be replaced too; I don't see a need
>> for CPUINFO86_leaf7_features either - just calculate everything you
>> need from X86_FEATURE_SMAP (these are all constants, so other than
>> the expression getting a little long there's nothing keeping this from
>> being a single btl).
> 
> In my understanding, CPUINFO86_leaf7_features is the offset for
> x86_capability[i] in struct cpuinfo_x86{}, seems we cannot get the
> right offset only from X86_FEATURE_SMAP?

Of course you need to add in the offset of x86_capability[].
See my other reply just sent.

>> > +#define ASM_STAC(prefix) ASM_AC(__ASM_STAC, prefix)
>> > +#define ASM_CLAC(prefix) ASM_AC(__ASM_CLAC, prefix)
>> 
>> What is "prefix" good for here, i.e. why can't you put the % right
>> in the macro?
> 
> Because this macro will be used in the basic inline assembly (use "%" as the 
> register prefix)
> and extended assembly (use "%%" as the register prefix).

Perhaps worth avoiding the basic uses then, by converting them to
extended? Passing these % or %% to the macro looks rather ugly,
so if the suggestion isn't viable, some other trick can certainly be
found to avoid this.

Jan

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

[Andrew Cooper]

On 23/04/14 13:42, Jan Beulich wrote:
>>>> On 23.04.14 at 14:11, <feng.wu@intel.com> wrote:
>>>> +#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.
>> Okay.
>>> 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
>> I thought about this idea before posting this version, but I didn't get a 
>> good
>> solution. I will continue to find a better way to handle this.
> btl $X86_FEATURE_SMAP & 31, ((X86_FEATURE_SMAP >> 3) & ~3)+CPUINFO86_features+boot_cpu_data(%rip)
>
> (at once replacing CPUINFO86_ext_features with CPUINFO86_features,
> perhaps also changing the [inconsistent] name prefix; likely a suitable
> macro could be created usable both here and wherever
> CPUINFO86_ext_features is being used currently)
>
> Jan
>

This needs careful synchronising with the "no-smap" command line
parameter which can shoot that bit out of boot features some time after
it being set.

It might be easier just to have  bool_t __read_mostly smap_in_use;

~Andrew

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

[Jan Beulich]

>>> On 23.04.14 at 14:50, <andrew.cooper3@citrix.com> wrote:
> On 23/04/14 13:42, Jan Beulich wrote:
>>>>> On 23.04.14 at 14:11, <feng.wu@intel.com> wrote:
>>>>> +#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.
>>> Okay.
>>>> 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
>>> I thought about this idea before posting this version, but I didn't get a 
>>> good
>>> solution. I will continue to find a better way to handle this.
>> btl $X86_FEATURE_SMAP & 31, ((X86_FEATURE_SMAP >> 3) & 
> ~3)+CPUINFO86_features+boot_cpu_data(%rip)
>>
>> (at once replacing CPUINFO86_ext_features with CPUINFO86_features,
>> perhaps also changing the [inconsistent] name prefix; likely a suitable
>> macro could be created usable both here and wherever
>> CPUINFO86_ext_features is being used currently)
> 
> This needs careful synchronising with the "no-smap" command line
> parameter which can shoot that bit out of boot features some time after
> it being set.
> 
> It might be easier just to have  bool_t __read_mostly smap_in_use;

For one I can't see how this relates to the coding suggestion - from
a functional pov the code above does nothing else than what Feng's
original code did.

And then, as long as the SMAP feature bit was ever set in the feature
mask (i.e. even if it gets cleared later), executing an extra stac or
clac will be harmless - we don't care whether we run with EFLAGS.AC
set or clear when CR4.SMAP is zero.

Jan

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

[Andrew Cooper]

On 23/04/14 13:56, Jan Beulich wrote:
>>>> On 23.04.14 at 14:50, <andrew.cooper3@citrix.com> wrote:
>> On 23/04/14 13:42, Jan Beulich wrote:
>>>>>> On 23.04.14 at 14:11, <feng.wu@intel.com> wrote:
>>>>>> +#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.
>>>> Okay.
>>>>> 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
>>>> I thought about this idea before posting this version, but I didn't get a 
>>>> good
>>>> solution. I will continue to find a better way to handle this.
>>> btl $X86_FEATURE_SMAP & 31, ((X86_FEATURE_SMAP >> 3) & 
>> ~3)+CPUINFO86_features+boot_cpu_data(%rip)
>>> (at once replacing CPUINFO86_ext_features with CPUINFO86_features,
>>> perhaps also changing the [inconsistent] name prefix; likely a suitable
>>> macro could be created usable both here and wherever
>>> CPUINFO86_ext_features is being used currently)
>> This needs careful synchronising with the "no-smap" command line
>> parameter which can shoot that bit out of boot features some time after
>> it being set.
>>
>> It might be easier just to have  bool_t __read_mostly smap_in_use;
> For one I can't see how this relates to the coding suggestion - from
> a functional pov the code above does nothing else than what Feng's
> original code did.

My appologies - it was not in reference to the coding suggestion persay,
but a thought that occurred to me while reading the thread.

>
> And then, as long as the SMAP feature bit was ever set in the feature
> mask (i.e. even if it gets cleared later), executing an extra stac or
> clac will be harmless - we don't care whether we run with EFLAGS.AC
> set or clear when CR4.SMAP is zero.
>
> Jan
>

However, this being true means that it should be fine as-was.

~Andrew

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

[Feng Wu]

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
+
+#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 */
 
-- 
1.8.3.1

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

[Wu, Feng]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, April 23, 2014 8:46 PM
> To: Wu, Feng
> Cc: andrew.cooper3@citrix.com; ian.campbell@citrix.com; Dong, Eddie;
> Nakajima, Jun; Tian, Kevin; xen-devel@lists.xen.org
> Subject: RE: [PATCH v2 1/7] x86: Add support for STAC/CLAC instructions
> 
> >>> On 23.04.14 at 14:32, <feng.wu@intel.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> >>> On 23.04.14 at 16:34, <feng.wu@intel.com> wrote:
> >> > +/* "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
> >>
> >> So why are you pushing/popping %rax here? There's no need for the
> >> lea.
> >>
> >> And the hard coded 7 here should be replaced too; I don't see a need
> >> for CPUINFO86_leaf7_features either - just calculate everything you
> >> need from X86_FEATURE_SMAP (these are all constants, so other than
> >> the expression getting a little long there's nothing keeping this from
> >> being a single btl).
> >
> > In my understanding, CPUINFO86_leaf7_features is the offset for
> > x86_capability[i] in struct cpuinfo_x86{}, seems we cannot get the
> > right offset only from X86_FEATURE_SMAP?
> 
> Of course you need to add in the offset of x86_capability[].
> See my other reply just sent.
> 
> >> > +#define ASM_STAC(prefix) ASM_AC(__ASM_STAC, prefix)
> >> > +#define ASM_CLAC(prefix) ASM_AC(__ASM_CLAC, prefix)
> >>
> >> What is "prefix" good for here, i.e. why can't you put the % right
> >> in the macro?
> >
> > Because this macro will be used in the basic inline assembly (use "%" as the
> > register prefix)
> > and extended assembly (use "%%" as the register prefix).
> 
> Perhaps worth avoiding the basic uses then, by converting them to
> extended? Passing these % or %% to the macro looks rather ugly,
> so if the suggestion isn't viable, some other trick can certainly be
> found to avoid this.

Need to add CLAC in the beginning of interrupt in the following macro, which uses
the basic inline assembly, seems it is hard to convert this one to extended format. I
have been thinking about this for some time and tried several method, but I am kind of
run out of ideas about it. Jan, do you have any suggestion about this?

Thanks very much in advance!

#define BUILD_COMMON_IRQ()                      \
__asm__(                                        \
    "\n" __ALIGN_STR"\n"                        \
    "common_interrupt:\n\t"                     \
    STR(SAVE_ALL) "\n\t"                        \
    "movq %rsp,%rdi\n\t"                        \
    "callq " STR(do_IRQ) "\n\t"                 \
    "jmp ret_from_intr\n");

> 
> Jan

Thanks,
Feng

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

[Andrew Cooper]

On 25/04/14 09:51, Wu, Feng wrote:
>
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Wednesday, April 23, 2014 8:46 PM
>> To: Wu, Feng
>> Cc: andrew.cooper3@citrix.com; ian.campbell@citrix.com; Dong, Eddie;
>> Nakajima, Jun; Tian, Kevin; xen-devel@lists.xen.org
>> Subject: RE: [PATCH v2 1/7] x86: Add support for STAC/CLAC instructions
>>
>>>>> On 23.04.14 at 14:32, <feng.wu@intel.com> wrote:
>>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>>>>>> On 23.04.14 at 16:34, <feng.wu@intel.com> wrote:
>>>>> +/* "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
>>>> So why are you pushing/popping %rax here? There's no need for the
>>>> lea.
>>>>
>>>> And the hard coded 7 here should be replaced too; I don't see a need
>>>> for CPUINFO86_leaf7_features either - just calculate everything you
>>>> need from X86_FEATURE_SMAP (these are all constants, so other than
>>>> the expression getting a little long there's nothing keeping this from
>>>> being a single btl).
>>> In my understanding, CPUINFO86_leaf7_features is the offset for
>>> x86_capability[i] in struct cpuinfo_x86{}, seems we cannot get the
>>> right offset only from X86_FEATURE_SMAP?
>> Of course you need to add in the offset of x86_capability[].
>> See my other reply just sent.
>>
>>>>> +#define ASM_STAC(prefix) ASM_AC(__ASM_STAC, prefix)
>>>>> +#define ASM_CLAC(prefix) ASM_AC(__ASM_CLAC, prefix)
>>>> What is "prefix" good for here, i.e. why can't you put the % right
>>>> in the macro?
>>> Because this macro will be used in the basic inline assembly (use "%" as the
>>> register prefix)
>>> and extended assembly (use "%%" as the register prefix).
>> Perhaps worth avoiding the basic uses then, by converting them to
>> extended? Passing these % or %% to the macro looks rather ugly,
>> so if the suggestion isn't viable, some other trick can certainly be
>> found to avoid this.
> Need to add CLAC in the beginning of interrupt in the following macro, which uses
> the basic inline assembly, seems it is hard to convert this one to extended format. I
> have been thinking about this for some time and tried several method, but I am kind of
> run out of ideas about it. Jan, do you have any suggestion about this?

What do you mean about "basic" and "extended" format ?

>
> Thanks very much in advance!
>
> #define BUILD_COMMON_IRQ()                      \
> __asm__(                                        \
>     "\n" __ALIGN_STR"\n"                        \
>     "common_interrupt:\n\t"                     \
>     STR(SAVE_ALL) "\n\t"                        \
>     "movq %rsp,%rdi\n\t"                        \
>     "callq " STR(do_IRQ) "\n\t"                 \
>     "jmp ret_from_intr\n");

Independently of the SMAP question, this code chunk would probably be
better living in in entry.S than as a macro in a header file.

~Andrew

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

[Jan Beulich]

>>> On 25.04.14 at 12:02, <andrew.cooper3@citrix.com> wrote:
> On 25/04/14 09:51, Wu, Feng wrote:
>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>>>>> On 23.04.14 at 14:32, <feng.wu@intel.com> wrote:
>>>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>>>>>>> On 23.04.14 at 16:34, <feng.wu@intel.com> wrote:
>>>>>> +#define ASM_STAC(prefix) ASM_AC(__ASM_STAC, prefix)
>>>>>> +#define ASM_CLAC(prefix) ASM_AC(__ASM_CLAC, prefix)
>>>>> What is "prefix" good for here, i.e. why can't you put the % right
>>>>> in the macro?
>>>> Because this macro will be used in the basic inline assembly (use "%" as the
>>>> register prefix)
>>>> and extended assembly (use "%%" as the register prefix).
>>> Perhaps worth avoiding the basic uses then, by converting them to
>>> extended? Passing these % or %% to the macro looks rather ugly,
>>> so if the suggestion isn't viable, some other trick can certainly be
>>> found to avoid this.
>> Need to add CLAC in the beginning of interrupt in the following macro, which 
> uses
>> the basic inline assembly, seems it is hard to convert this one to extended 
> format. I
>> have been thinking about this for some time and tried several method, but I 
> am kind of
>> run out of ideas about it. Jan, do you have any suggestion about this?
> 
> What do you mean about "basic" and "extended" format ?

The ones respectively without and with inputs/outputs/clobbers.

Jan

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

[Jan Beulich]

>>> On 25.04.14 at 10:51, <feng.wu@intel.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Perhaps worth avoiding the basic uses then, by converting them to
>> extended? Passing these % or %% to the macro looks rather ugly,
>> so if the suggestion isn't viable, some other trick can certainly be
>> found to avoid this.
> 
> Need to add CLAC in the beginning of interrupt in the following macro, which 
> uses
> the basic inline assembly, seems it is hard to convert this one to extended 
> format. I
> have been thinking about this for some time and tried several method, but I 
> am kind of
> run out of ideas about it. Jan, do you have any suggestion about this?
> 
> Thanks very much in advance!
> 
> #define BUILD_COMMON_IRQ()                      \
> __asm__(                                        \
>     "\n" __ALIGN_STR"\n"                        \
>     "common_interrupt:\n\t"                     \
>     STR(SAVE_ALL) "\n\t"                        \
>     "movq %rsp,%rdi\n\t"                        \
>     "callq " STR(do_IRQ) "\n\t"                 \
>     "jmp ret_from_intr\n");

I agree with Andrew - now that we don't have to care about
otherwise resulting code duplication (leaving aside that there wasn't
much of it here anyway, the bulk of it is from BUILD_IRQ()), this
should simply be moved into entry.S, at once making it better readable
(and a follow-up patch, unless you want to do it all in one go, would
be to also move the BUILD_IRQ() consumer into entry.S).

Jan

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

[Wu, Feng]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, April 25, 2014 6:39 PM
> To: Wu, Feng
> Cc: andrew.cooper3@citrix.com; ian.campbell@citrix.com; Dong, Eddie;
> Nakajima, Jun; Tian, Kevin; xen-devel@lists.xen.org
> Subject: RE: [PATCH v2 1/7] x86: Add support for STAC/CLAC instructions
> 
> >>> On 25.04.14 at 10:51, <feng.wu@intel.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Perhaps worth avoiding the basic uses then, by converting them to
> >> extended? Passing these % or %% to the macro looks rather ugly,
> >> so if the suggestion isn't viable, some other trick can certainly be
> >> found to avoid this.
> >
> > Need to add CLAC in the beginning of interrupt in the following macro, which
> > uses
> > the basic inline assembly, seems it is hard to convert this one to extended
> > format. I
> > have been thinking about this for some time and tried several method, but I
> > am kind of
> > run out of ideas about it. Jan, do you have any suggestion about this?
> >
> > Thanks very much in advance!
> >
> > #define BUILD_COMMON_IRQ()                      \
> > __asm__(                                        \
> >     "\n" __ALIGN_STR"\n"                        \
> >     "common_interrupt:\n\t"                     \
> >     STR(SAVE_ALL) "\n\t"                        \
> >     "movq %rsp,%rdi\n\t"                        \
> >     "callq " STR(do_IRQ) "\n\t"                 \
> >     "jmp ret_from_intr\n");
> 
> I agree with Andrew - now that we don't have to care about
> otherwise resulting code duplication (leaving aside that there wasn't
> much of it here anyway, the bulk of it is from BUILD_IRQ()), this
> should simply be moved into entry.S, at once making it better readable
> (and a follow-up patch, unless you want to do it all in one go, would
> be to also move the BUILD_IRQ() consumer into entry.S).
> 

Okay, I will move common_interrupt to entry.S first, and then will handle
BUILD_IRQ in a follow-up patch

> Jan

Thanks,
Feng