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

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

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