From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v2 1/2] xc_cpuid_x86.c: Simplify masking conditions and remove redundant work Date: Tue, 9 Sep 2014 13:29:19 +0100 Message-ID: <540EF29F.9000501@citrix.com> References: <1410237112-21177-1-git-send-email-alfred.z.song@gmail.com> <540EF66C0200007800032929@mail.emea.novell.com> <1410265317.8217.151.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1410265317.8217.151.camel@kazak.uk.xensource.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: Ian Campbell , Jan Beulich Cc: stefano.stabellini@eu.citrix.com, jinsong.liu@alibaba-inc.com, ian.jackson@eu.citrix.com, Zhuo Song , Zhuo Song , boyu.mt@alibaba-inc.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 09/09/14 13:21, Ian Campbell wrote: > On Tue, 2014-09-09 at 11:45 +0100, Jan Beulich wrote: >>>>> On 09.09.14 at 06:31, wrote: >>> @@ -195,16 +186,14 @@ static void intel_xc_cpuid_policy( >>> break; >>> >>> case 0x80000001: { >>> - int is_64bit = hypervisor_is_64bit(xch) && is_pae; >>> - >>> /* Only a few features are advertised in Intel's 0x80000001. */ >>> - regs[2] &= (is_64bit ? bitmaskof(X86_FEATURE_LAHF_LM) : 0) | >>> - bitmaskof(X86_FEATURE_3DNOWPREFETCH) | >>> - bitmaskof(X86_FEATURE_ABM); >>> - regs[3] &= ((is_pae ? bitmaskof(X86_FEATURE_NX) : 0) | >>> - (is_64bit ? bitmaskof(X86_FEATURE_LM) : 0) | >>> - (is_64bit ? bitmaskof(X86_FEATURE_SYSCALL) : 0) | >>> - (is_64bit ? bitmaskof(X86_FEATURE_RDTSCP) : 0)); >>> + regs[2] &= (bitmaskof(X86_FEATURE_LAHF_LM) | >>> + bitmaskof(X86_FEATURE_3DNOWPREFETCH) | >>> + bitmaskof(X86_FEATURE_ABM); >>> + regs[3] &= (bitmaskof(X86_FEATURE_NX) | >>> + bitmaskof(X86_FEATURE_LM) | >>> + (is_pae ? bitmaskof(X86_FEATURE_SYSCALL) : 0) | >>> + (is_pae ? bitmaskof(X86_FEATURE_RDTSCP) : 0)); >> As said before, tying these two features to is_pae seems a >> little strange, but if the tools maintainers can live with that, I >> guess I can too (short of having a better suggestion other >> than to drop the conditionals altogether). > Patch #2 here seems to remove it from the RDTSCP, surely that should be > folded in. > > I also don't understand the link between PAE and the presence of > SYSCALL. On Intel, syscall is strictly only available in long mode, being an AMD instruction mandated in the 64bit spec. is_64bit is disappearing as Xen is unconditionally 64bit these days, but preventing the guest using PAE will preclude it being able to enter long mode. I would agree that it is not necessarily obvious, and based on this consideration, I think it would be better to keep the variable "is_64bit" as it is more informative than "is_pae" in the contexts used. ~Andrew