From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v2 1/7] x86: Add support for STAC/CLAC instructions Date: Fri, 25 Apr 2014 11:02:39 +0100 Message-ID: <535A32BF.7010206@citrix.com> References: <1398263648-31994-1-git-send-email-feng.wu@intel.com> <5357AF23020000780000B515@nat28.tlf.novell.com> <5357D216020000780000B766@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "Wu, Feng" Cc: "Tian, Kevin" , "ian.campbell@citrix.com" , "Dong, Eddie" , "xen-devel@lists.xen.org" , Jan Beulich , "Nakajima, Jun" List-Id: xen-devel@lists.xenproject.org On 25/04/14 09:51, Wu, Feng wrote: > >> -----Original Message----- >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: Wednesday, April 23, 2014 8:46 PM >> To: Wu, Feng >> Cc: andrew.cooper3@citrix.com; ian.campbell@citrix.com; Dong, Eddie; >> Nakajima, Jun; Tian, Kevin; xen-devel@lists.xen.org >> Subject: RE: [PATCH v2 1/7] x86: Add support for STAC/CLAC instructions >> >>>>> On 23.04.14 at 14:32, wrote: >>>> From: Jan Beulich [mailto:JBeulich@suse.com] >>>>>>> On 23.04.14 at 16:34, wrote: >>>>> +/* "Raw" instruction opcodes */ >>>>> +#define __ASM_CLAC .byte 0x0f,0x01,0xca >>>>> +#define __ASM_STAC .byte 0x0f,0x01,0xcb >>>>> + >>>>> +#ifdef __ASSEMBLY__ >>>>> +#define ASM_AC(op) \ >>>>> + pushq %rax; \ >>>>> + leaq boot_cpu_data(%rip), %rax; \ >>>>> + btl $X86_FEATURE_SMAP-7*32, >> CPUINFO86_leaf7_features(%rax); >>>> \ >>>>> + jnc 881f; \ >>>>> + op; \ >>>>> +881: popq %rax >>>> So why are you pushing/popping %rax here? There's no need for the >>>> lea. >>>> >>>> And the hard coded 7 here should be replaced too; I don't see a need >>>> for CPUINFO86_leaf7_features either - just calculate everything you >>>> need from X86_FEATURE_SMAP (these are all constants, so other than >>>> the expression getting a little long there's nothing keeping this from >>>> being a single btl). >>> In my understanding, CPUINFO86_leaf7_features is the offset for >>> x86_capability[i] in struct cpuinfo_x86{}, seems we cannot get the >>> right offset only from X86_FEATURE_SMAP? >> Of course you need to add in the offset of x86_capability[]. >> See my other reply just sent. >> >>>>> +#define ASM_STAC(prefix) ASM_AC(__ASM_STAC, prefix) >>>>> +#define ASM_CLAC(prefix) ASM_AC(__ASM_CLAC, prefix) >>>> What is "prefix" good for here, i.e. why can't you put the % right >>>> in the macro? >>> Because this macro will be used in the basic inline assembly (use "%" as the >>> register prefix) >>> and extended assembly (use "%%" as the register prefix). >> Perhaps worth avoiding the basic uses then, by converting them to >> extended? Passing these % or %% to the macro looks rather ugly, >> so if the suggestion isn't viable, some other trick can certainly be >> found to avoid this. > Need to add CLAC in the beginning of interrupt in the following macro, which uses > the basic inline assembly, seems it is hard to convert this one to extended format. I > have been thinking about this for some time and tried several method, but I am kind of > run out of ideas about it. Jan, do you have any suggestion about this? What do you mean about "basic" and "extended" format ? > > Thanks very much in advance! > > #define BUILD_COMMON_IRQ() \ > __asm__( \ > "\n" __ALIGN_STR"\n" \ > "common_interrupt:\n\t" \ > STR(SAVE_ALL) "\n\t" \ > "movq %rsp,%rdi\n\t" \ > "callq " STR(do_IRQ) "\n\t" \ > "jmp ret_from_intr\n"); Independently of the SMAP question, this code chunk would probably be better living in in entry.S than as a macro in a header file. ~Andrew