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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

2) Fix alternative_{io,input}() to no longer have the - afaict -
pointless dummy input. The comment says it's for API compatibility,
which we (other than Linux from where it was taken) don't care
about. The only current user of alternative_io() should be unaffected,
as it uses named operands already. (This would again be in a prereq
patch, and the main patch would leave indentation unaltered.)

3) Stay with what you have, but leave the original indentation and
add a comment explaining the apparently odd numbering.

Albeit if 1 or 2 was chosen, 3 would seem to be a good idea anyway.

Jan


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