|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [V11 1/3] x86/xsaves: enable xsaves/xrstors/xsavec in xen
>>> On 23.11.15 at 06:22, <shuai.ruan@xxxxxxxxxxxxxxx> wrote:
> 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 ?
Well, kind of. The variable naming is quite strange: Why upper case?
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)?
> 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.)
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |