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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.11.15 at 02:34, <shuai.ruan@xxxxxxxxxxxxxxx> wrote:
> On Wed, Nov 04, 2015 at 10:04:33AM -0700, Jan Beulich wrote:
>> >>> On 03.11.15 at 07:27, <shuai.ruan@xxxxxxxxxxxxxxx> wrote:
>> > @@ -158,6 +334,20 @@ void xsave(struct vcpu *v, uint64_t mask)
>> >          ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] = word_size;
>> >  }
>> > +#define XSTATE_FIXUP ".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"
>> > +
>> >  void xrstor(struct vcpu *v, uint64_t mask)
>> >  {
>> >      uint32_t hmask = mask >> 32;
>> > @@ -187,39 +377,22 @@ 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_input("1: "".byte 0x48,0x0f,0xae,0x2f",
>> > +                          ".byte 0x48,0x0f,0xc7,0x1f",
>> > +                          X86_FEATURE_XSAVES,
>> > +                          "D" (ptr), "m" (*ptr), "a" (lmask), "d" 
> (hmask));
>> > +        asm volatile (XSTATE_FIXUP);
>> >          break;
>> >      case 4: case 2:
>> > -        asm volatile ( "1: .byte 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_input("1: "".byte 0x0f,0xae,0x2f",
>> > +                          ".byte 0x0f,0xc7,0x1f",
>> > +                          X86_FEATURE_XSAVES,
>> > +                          "D" (ptr), "m" (*ptr), "a" (lmask), "d" 
> (hmask));
>> > +        asm volatile (XSTATE_FIXUP);
>> >          break;
>> >      }
>> >  }
>> > +#undef XSTATE_FIXUP
>> 
>> Repeating my comment on v8: "I wonder whether at least for the
>> restore side alternative asm wouldn't result in better readable code
>> and at the same time in a smaller patch." Did you at least look into
>> that option?
>> 
> I may misunderstand your meaning. I have adressed the comment by changing 
> the restor side using alternative_input. Does "alternative_input" not what 
> you want ? 
> if it is not what you want, please give me some suggestions how to
> address this ?  

Oh, I'm sorry, I should have looked more closely. The fact that
XSTATE_FIXUP survived made me draw wrong conclusions without
looking more closely. Now the bad news is - you can't split things
like this, as the compiler doesn't make any guarantees as to
register values between two asm()-s. The whole construct needs
to and up as a single asm(), which is why XSTATE_FIXUP and is
unlikely to be of much use here (at least in the context of this
patch; a separate cleanup patch might eliminate the redundancy).

Jan

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