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

[Shuai Ruan <shuai.ruan@xxxxxxxxxxxxxxx>]

On Fri, Nov 20, 2015 at 04:35:00AM -0700, Jan Beulich wrote:
> >>> On 20.11.15 at 02:18, <shuai.ruan@xxxxxxxxxxxxxxx> wrote:
> > @@ -187,36 +363,56 @@ void xrstor(struct vcpu *v, uint64_t mask)
> >      switch ( __builtin_expect(ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET], 8) )
> >      {
> >      default:
> > -        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" );
> > +        alternative_io( "1: "".byte 0x48,0x0f,0xae,0x2f \n"
> > +                        ".section .fixup,\"ax\"         \n"
> > +                        "2: mov %6,%%ecx                \n"
> > +                        "   xor %1,%1                   \n"
> > +                        "   rep stosb                   \n"
> > +                        "   lea %3,%0                   \n"
> > +                        "   mov %4,%1                   \n"
> > +                        "   jmp 1b                      \n"
> > +                        ".previous                      \n"
> > +                        _ASM_EXTABLE(1b, 2b),
> > +                        ".byte 0x48,0x0f,0xc7,0x1f      \n"
> > +                        ".section .fixup,\"ax\"         \n"
> > +                        "2: mov %6,%%ecx                \n"
> > +                        "   xor %1,%1                   \n"
> > +                        "   rep stosb                   \n"
> > +                        "   lea %3,%0                   \n"
> > +                        "   mov %4,%1                   \n"
> > +                        "   jmp 1b                      \n"
> > +                        ".previous                      \n"
> > +                        _ASM_EXTABLE(1b, 2b),
> > +                        X86_FEATURE_XSAVES,
> > +                        ASM_OUTPUT2("+&D" (ptr), "+&a" (lmask)),
> > +                        "m" (*ptr), "g" (lmask), "d" (hmask), "m" 
> > (xsave_cntxt_size)
> > +                        : "ecx" );
> 
> So I had specifically asked for _not_ altering the indentation (to help
> review), but you still modified the whole block. Which, if I hadn't
> looked closely, would have hidden the %5 -> %6 and similar other
> changes. I realize that's due to the dummy input alternative_io()
> inserts. So I see three options for you (in order of my preference):
> 
> 1) Do the conversion properly, splitting things out into a macro, in
> a separate, prereq patch. "Properly" here meaning to convert from
> 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 ?

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" );

Thanks
Shuai
        




 

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