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

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Tuesday, April 22, 2014 4:07 PM
> To: Wu, Feng
> Cc: Ian.Campbell@xxxxxxxxxx; Dong, Eddie; Nakajima, Jun;
> xen-devel@xxxxxxxxxxxxx
> Subject: RE: [PATCH v1 1/6] x86: Add support for STAC/CLAC instructions
> 
> >>> 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.
> 

Since the alternatives mechanism in Linux is something common and independent 
and needs
a bit more efforts to be ported to Xen, can we use the method I mentioned above
at the current stage. After that I will have a fully think about how to port the
alternatives mechanism Xen. 

What do you think about this?

> Jan

Thanks,
Feng

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