Re: [Xen-devel] [V10 1/3] x86/xsaves: enable xsaves/xrstors/xsavec in xen

[Shuai Ruan <shuai.ruan@xxxxxxxxxxxxxxx>]

On Wed, Nov 18, 2015 at 03:40:50AM -0700, Jan Beulich wrote:
> > For this save side. As cpu_has_xsaveopt should be handled indenpently.
> > If we just use alternative asm for xsaves or xsavec, the following code
> > handle xsaveopt/xsave should be like this:
> > if ( !cpu_has_xsavec && !cpu_has_xsaves && cpu_has_xsaveopt)
> > ......
> > ....
> > else if ( !cpu_has_xsavec && !cpu_has_xsaves )
> > .......
> > ......
> > 
> > if you think it is ok to do it like this. I will add this.
> > 
> > Also, for instruction without REX.W in save side, alternavitve asm can
> > be used, but a new alternative marco which must handle 4 instruction
> > base on 4 cpu_features will be added. So do this in this patchset or
> > another patchset ? which one is ok to you?
> 
> I'm confused - first you suggest using chains of if()s (which I dislike)
> and then you talk about a 4-variant alternative (which is what I'm
> after)? 
Let me make it clear(may show some code).
For xsave code. There is two cases: 

1:Use 64-bit operand size. 

The origin code will refine FPU selector handling code for XSAVEOPT. So if we
use alternative asm for 64-bit opreand size instruction(xsaves/xsavec/xsaveopt
/xsave) like below. 

alternative_io_3(".byte 0x48,0x0f,0xae,0x27",
                 ".byte 0x48,0x0f,0xae,0x37", X86_FEATURE_XSAVEOPT,
                 ".byte 0x48,0x0f,0xc7,0x27", X86_FEATURE_XSAVEC,
                 ".byte 0x48,0x0f,0xc7,0x2f", X86_FEATURE_XSAVES
                 : "=m" (*ptr),
                 : "a" (lmask), "d" (hmask), "D" (ptr) );

It may break the handling code for XSAVEOPT(code below). 

..................................
        else if ( cpu_has_xsaveopt )
        {
        /*
         * xsaveopt may not write the FPU portion even when the respective
         * mask bit is set. For the check further down to work we hence
         * need to put the save image back into the state that it was in
         * right after the previous xsaveopt.
         */
         if ( word_size > 0 &&
              (ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] == 4 ||
               ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] == 2) )
        {
              ptr->fpu_sse.fip.sel = 0;
              ptr->fpu_sse.fdp.sel = 0;
        }
................................

So what's your opintion about this or any suggestion on this ?
(hope I make it clear now )

2:Use 32-bit operand size.
For this part it is ok to use alternative asm .

alternative_io_3(".byte 0x0f,0xae,0x27",
                 ".byte 0x0f,0xae,0x37", X86_FEATURE_XSAVEOPT,
                 ".byte 0x0f,0xc7,0x27", X86_FEATURE_XSAVEC,
                 ".byte 0x0f,0xc7,0x2f", X86_FEATURE_XSAVES
                 : "=m" (*ptr),
                 : "a" (lmask), "d" (hmask), "D" (ptr) );

>Yes, this will need extending the base infrastructure
> (depending on how involved a change that would be, perhaps in a
> separate patch).
I will introduce two new macros in "alternative.h".
Change save part to alternative.

So do it in this patchset or do it totally in another separate 
patchset (leave save part as none-alternative in xsave patchset)? 
Which one is ok to you ? 

Thanks 
shuai


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

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