Re: [V11 1/3] x86/xsaves: enable xsaves/xrstors/xsavec in xen

[Shuai Ruan <shuai.ruan@linux.intel.com>]

On Mon, Nov 23, 2015 at 03:06:01AM -0700, Jan Beulich wrote:
> >> numbered to named operands.
> > I prefer to use this option.
> > 
> > The original code is below(without xsaves patch) 
> >         asm volatile ( "1: .byte 0x48,0x0f,0xae,0x2f\n"
> >                        ".section .fixup,\"ax\"      \n"
> >                        "2: mov %5,%%ecx             \n"
> >                        "   xor %1,%1                \n"
> >                        "   rep stosb                \n"
> >                        "   lea %2,%0                \n"
> >                        "   mov %3,%1                \n"
> >                        "   jmp 1b                   \n"
> >                        ".previous                   \n"
> >                        _ASM_EXTABLE(1b, 2b)
> >                       : "+&D" (ptr), "+&a" (lmask)
> >                       : "m" (*ptr), "g" (lmask), "d" (hmask),
> >                       "m" (xsave_cntxt_size)
> >                       : "ecx" );
> > 
> > Then My code to using named operands is below(without xaves patch).
> > 
> >         asm volatile ( "1: .byte 0x48,0x0f,0xae,0x2f	\n"
> >                        ".section .fixup,\"ax\"          \n"
> >                        "2: mov %[SIZE],%%ecx            \n"
> >                        "   xor %[LMASK_A],%[LMASK_A]    \n"
> >                        "   rep stosb                    \n"
> >                        "   lea %[PTR_M],%[PTR]          \n"
> >                        "   mov %[LMASK_G],%[LMASK_A]    \n"
> >                        "   jmp 1b                   	\n"
> >                        ".previous                   	\n"
> >                        _ASM_EXTABLE(1b, 2b)
> >                       : [PTR] "+&D" (ptr),  [LMASK_A] "+&a" (lmask)
> >                       : [PTR_M] "m" (*ptr), [LMASK_G] "g" (lmask), [HMASK_D] "d" (hmask),
> >                         [SIZE] "m" (xsave_cntxt_size)
> >                       : "ecx" );
> > 
> > Is that ok to you ?
> 
> Well, kind of. The variable naming is quite strange: Why upper case?
lower case will be used.
> And why those strange _A, _G, etc suffixes (I think you instead mean
> _out and _in or some such respectively; but see also below - the fewer
> dependencies between macro and code surrounding the use sites, the
> less important the selection of asm() operand names)?
I intend to use _A _G to represent the register it use to make it more
readable. But actually it make the code more strange.
I intend to add prefix before operand (like below).

         asm volatile ( "1: .byte 0x48,0x0f,0xae,0x2f	   \n"
                        ".section .fixup,\"ax\"            \n"
                        "2: mov %[size_in],%%ecx           \n"
                        "   xor %[lmask_out],%[lmask_out]  \n"
                        "   rep stosb                      \n"
                        "   lea %[ptr_in],%[ptr_out]       \n"
                        "   mov %[lmask_in],%[lmask_out]   \n"
                        "   jmp 1b                   	   \n"
                        ".previous                   	   \n"
                        _ASM_EXTABLE(1b, 2b)
                        : [ptr_out] "+&D" (ptr),  [lmask_out] "+&a" (lmask)
                        : [ptr_in] "m" (*ptr), [lmask_in] "g" (lmask), [hmask_in] "d" (hmask),
                        [size_in] "m" (xsave_cntxt_size)
                       : "ecx" );

This seem make the XSAVE_FIXUP macro below depend more on code
surrounding the use sites.
But the XSAVE_FIXUP macro actually only used in xrstor. So I think it
may be proper used like this. 
Is naming operand in this way ok to you ?

> > Also , You ask to splitting things out into a macro ? I do not quite
> > unstandand your meaning ? Does it mean define Macro to handle fixup
> > code like below ?
> > #define XRSTOR_FIXUP
> >         ".section .fixup,\"ax\"          \n" \
> >         "2: mov %[SIZE],%%ecx            \n" \
> >         "   xor %[LMASK_A],%[LMASK_A]    \n" \
> >         "   rep stosb                    \n" \
> >         "   lea %[PTR_M],%[PTR]          \n" \
> >         "   mov %[LMASK_G],%[LMASK_A]    \n" \
> >         "   jmp 1b                   	\n"  \
> >         ".previous                   	\n"  \
> >         _ASM_EXTABLE(1b, 2b)
> > 
> > Then xrstor side can be:
> >         asm volatile ( "1: .byte 0x48,0x0f,0xae,0x2f	\n"
> > 		       XRSTOR_FIXUP
> >                       : [PTR] "+&D" (ptr), [LMASK_A] "+&a" (lmask)
> >                       : [PTR_M] "m" (*ptr), [LMASK_G] "g" (lmask), [HMASK_D] "d" (hmask),
> >                         [SIZE] "m" (xsave_cntxt_size)
> >                       : "ecx" );
> 
> Goes in the right direction, but I think all operands related to the fixup
> should also get moved to the macro. Of course you'll have to use your
> judgment with your actual patches in mind. (To be more precise, the
> macro should have as much inside it as possible, so that as little as
> possible dependencies exist between it and the code surrounding the
> macro invocation. This likely requires adding a parameter or two to
> macro.)
All the operands realted to fixup moved to macro is ok here. 
But as I will use alternative asm in restor side(in xsaves patch), alternative will 
seperate the fixup code and out/in operand definitions like above , and also I intend 
to reuse XRSTOR_FIXUP in restor side. So in my opintion, seperate like this is ok.
Perhaps, there is some point I am not of aware. Please let me if you
have much better way to do this ?

Thanks for your time.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>