>>> On 23.11.15 at 06:22, <shuai.r...@linux.intel.com> wrote: > On Fri, Nov 20, 2015 at 04:35:00AM -0700, Jan Beulich wrote: >> >>> On 20.11.15 at 02:18, <shuai.r...@linux.intel.com> 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@lists.xen.org http://lists.xen.org/xen-devel