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

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

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