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 19:06:24 +0800 Message-ID: <40029.4508821846$1448276984@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> <20151123052255.GA12378@shuai.ruan@linux.intel.com> <5652F31902000078000B7B4D@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: <5652F31902000078000B7B4D@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 Mon, Nov 23, 2015 at 03:06:01AM -0700, Jan Beulich wrote: > >> 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? lower case will be used. > 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)? I intend to use _A _G to represent the register it use to make it more readable. But actually it make the code more strange. I intend to add prefix before operand (like below). asm volatile ( "1: .byte 0x48,0x0f,0xae,0x2f \n" ".section .fixup,\"ax\" \n" "2: mov %[size_in],%%ecx \n" " xor %[lmask_out],%[lmask_out] \n" " rep stosb \n" " lea %[ptr_in],%[ptr_out] \n" " mov %[lmask_in],%[lmask_out] \n" " jmp 1b \n" ".previous \n" _ASM_EXTABLE(1b, 2b) : [ptr_out] "+&D" (ptr), [lmask_out] "+&a" (lmask) : [ptr_in] "m" (*ptr), [lmask_in] "g" (lmask), [hmask_in] "d" (hmask), [size_in] "m" (xsave_cntxt_size) : "ecx" ); This seem make the XSAVE_FIXUP macro below depend more on code surrounding the use sites. But the XSAVE_FIXUP macro actually only used in xrstor. So I think it may be proper used like this. Is naming operand in this way 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" ); > > 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.) All the operands realted to fixup moved to macro is ok here. But as I will use alternative asm in restor side(in xsaves patch), alternative will seperate the fixup code and out/in operand definitions like above , and also I intend to reuse XRSTOR_FIXUP in restor side. So in my opintion, seperate like this is ok. Perhaps, there is some point I am not of aware. Please let me if you have much better way to do this ? Thanks for your time. > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel >