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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.04.14 at 09:41, <feng.wu@xxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> 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.
>> 
> 
> Jan, I did some investigation about how to handle this two instructions
> in Linux, basically, it uses the alternatives mechanism to handle these
> kind of cases. Let's take the following definition of ASM_STAC in Linux for 
> example:
> 
> #define ASM_CLAC                                                        \
>         661: ASM_NOP3 ;                                                 \
>         .pushsection .altinstr_replacement, "ax" ;                      \
>         662: __ASM_CLAC ;                                               \
>         .popsection ;                                                   \
>         .pushsection .altinstructions, "a" ;                            \
>         altinstruction_entry 661b, 662b, X86_FEATURE_SMAP, 3, 3 ;       \
>         .popsection
> 
> ASM_CLAC is defined as NOP by default, it puts the real CLAC instruction in 
> section "altinstr_replacement" and
> the needed information to " altinstructions " section, which is useful to 
> replace the default
> definition by the alternative one. Here is the routine call path: 
> start_kernel () --> check_bugs() --> alternative_instructions().
> 
> In function alternative_instructions(), it will check the related features 
> in CPU, if it exists, the alternative definition will
> overwrite the default one. So there is no conditional branches after this 
> replacement when the Macro is being used.
> 
> Do you think we need to port this whole mechanism to Xen to support 
> CLAC/STAC? I am not sure if it is a little overkilled.

Obviously we could use this machinery for other things. But whether it's
needed here depends on the alternatives.

> BTW, from the Linux implementation, I think we don't need to check the 'cr4' 
> for the macros, we just need
> to check whether the feature exists in the CPU. So is it acceptable to use 
> the original code by eliminating the cr4 check?

That _might_ be acceptable if you bring it down to just the three
really necessary instructions: BT, JNC, CLAC/STAC. But the "might"
has to stand - this, after all, remains an addition of a conditional
branch (and for the performance of STAC/CLAC I haven't seen any
documentation so far either) to several fast paths, and hence the
patching alternative can't be discarded as the potentially better one.

Jan


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