From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shuai Ruan Subject: Re: [V11 1/3] x86/xsaves: enable xsaves/xrstors/xsavec in xen Date: Mon, 23 Nov 2015 13:22:55 +0800 Message-ID: <21175.0391907573$1448256345@news.gmane.org> References: <1447982282-7437-1-git-send-email-shuai.ruan@linux.intel.com> <1447982282-7437-2-git-send-email-shuai.ruan@linux.intel.com> <564F137402000078000B714F@prv-mh.provo.novell.com> Reply-To: Shuai Ruan Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <564F137402000078000B714F@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: kevin.tian@intel.com, wei.liu2@citrix.com, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, jun.nakajima@intel.com, keir@xen.org List-Id: xen-devel@lists.xenproject.org On Fri, Nov 20, 2015 at 04:35:00AM -0700, Jan Beulich wrote: > >>> On 20.11.15 at 02:18, 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