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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.11.15 at 12:06, <shuai.ruan@xxxxxxxxxxxxxxx> wrote:
> 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 ?

Mostly. Please at least omit the suffixes where not needed to
disambiguate two operands. Also I don't think ptr_in is properly
reflecting the arguments purposes (which is a block of memory,
not a pointer to such).

>> > 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 ?

I can't judge on this without seeing the result. I can only again defer
to your judgment. As said, the restriction I'm placing on the set of
choices you have is that there should be no disconnect between
names chosen inside the macro and names chosen by the invoking
code. I.e. anything the scope of which is the macro alone can be
established by the macro, while anything the scope of which is
wider than the macro alone should be passed as argument by the
invoking code.

Jan

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