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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.04.14 at 11:43, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 22/04/14 09:07, Jan Beulich wrote:
>>>>> 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.
> 
> copy_{to,from}_guest() are already long paths (particularly for HVM) so
> a single extra conditional is not going to be too bad (and as after boot
> it will remain constant, the branch predictor will have a reliable time
> with it).  It would certainly be fine for a v1 to get SMAP support working.

But that's not the paths I'm concerned about. There's going to be
a CLAC at exception, interrupt, and hypercall entry points. Those
are the ones I'm concerned about.

Jan


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