From mboxrd@z Thu Jan 1 00:00:00 1970 From: z Subject: Re: [PATCH] xc_cpuid_x86.c: No need to mask NX twice Date: Sat, 6 Sep 2014 00:35:08 +0800 Message-ID: References: <1409911893-18037-1-git-send-email-alfred.z.song@gmail.com> <5409AD6E0200007800031618@mail.emea.novell.com> <5409E07D020000780003182B@mail.emea.novell.com> <5409FC650200007800031931@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1611960006063323248==" Return-path: In-Reply-To: <5409FC650200007800031931@mail.emea.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: Ian Campbell , Stefano Stabellini , jinsong.liu@alibaba-inc.com, ian.jackson@eu.citrix.com, Zhuo Song , =?UTF-8?B?6ams5rabKOS8r+eRnCk=?= , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org --===============1611960006063323248== Content-Type: multipart/alternative; boundary=089e0158ad9c62e7b40502540d02 --089e0158ad9c62e7b40502540d02 Content-Type: text/plain; charset=UTF-8 Got it. Thanks, Jan. If so, I think we could remove the condition for masking NX in both vendor specific functions, since the architectural logic has help cover it and the judgement is unnecessary. For example: diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c index 61af3e6..6bd89b0 100644 --- a/tools/libxc/xc_cpuid_x86.c +++ b/tools/libxc/xc_cpuid_x86.c @@ -116,7 +116,7 @@ static void amd_xc_cpuid_policy( bitmaskof(X86_FEATURE_TBM) | bitmaskof(X86_FEATURE_DBEXT)); regs[3] &= (0x0183f3ff | /* features shared with 0x00000001:EDX */ - (is_pae ? bitmaskof(X86_FEATURE_NX) : 0) | + bitmaskof(X86_FEATURE_NX) | (is_64bit ? bitmaskof(X86_FEATURE_LM) : 0) | bitmaskof(X86_FEATURE_SYSCALL) | bitmaskof(X86_FEATURE_MP) | @@ -201,7 +201,7 @@ static void intel_xc_cpuid_policy( 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) | + regs[3] &= (bitmaskof(X86_FEATURE_NX) | (is_64bit ? bitmaskof(X86_FEATURE_LM) : 0) | (is_64bit ? bitmaskof(X86_FEATURE_SYSCALL) : 0) | (is_64bit ? bitmaskof(X86_FEATURE_RDTSCP) : 0)); Zhuo On Sat, Sep 6, 2014 at 12:09 AM, Jan Beulich wrote: > >>> On 05.09.14 at 17:45, wrote: > > Hi, Jan > > > > I am sorry for making you confused. > > > > What I mean is there seems to be some redundant work. For example, to > leaf > > 0x80000001, the generic work (masking NX and PSE36) has been overwritten > by > > the the vendor's functions (amd_xc_cpuid_policy and > intel_xc_cpuid_policy) > > , why couldn't we just drop them and leave the work to the vendor? Of > > cause, another way is just like you said, keeping the generic ones, and > > changing the logic in the vendor-based implementation. > > > > What I want to do is to simplify this if possible. :) > > Right, yet conceptually anything that is defined by the architecture > would better be done in the generic routine. Anything that is truly > vendor specific should be done in the vendor ones. And the > dependency of NX on PAE is (nowadays) an architectural one. > > Jan > > --089e0158ad9c62e7b40502540d02 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Got it. Thanks, Jan.
If so, I think we= could remove the condition for masking NX in both vendor specific function= s, since the architectural logic has help cover it and the judgement is unn= ecessary.=C2=A0=C2=A0=C2=A0 For example:

diff --git a/tools/libxc/xc= _cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 61af3e6..6bd89b0 100644<= br>--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
= @@ -116,7 +116,7 @@ static void amd_xc_cpuid_policy(
=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0 bitmaskof(X86_FEATURE_TBM) |
=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 bitmaskof(X86_FEATURE_DBEXT));
=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 regs[3] &=3D (0x0183f3ff | /* f= eatures shared with 0x00000001:EDX */
-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 (is_pae ? bitmaskof(X86_FEATURE_NX) : 0) |
+=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 bitmaskof(X86_FEATURE_NX) |
=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0 (is_64bit ? bitmaskof(X86_FEATURE_LM) : 0) |
=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 bitmaskof(X86_FEATURE_SYSCALL) |=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 bitmaskof(X86_FEATURE_M= P) |
@@ -201,7 +201,7 @@ static void intel_xc_cpuid_policy(
=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 regs[2] &=3D (is_64bit ? bitmas= kof(X86_FEATURE_LAHF_LM) : 0) |
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 bi= tmaskof(X86_FEATURE_3DNOWPREFETCH) |
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 bitmaskof(X86_FEATURE_ABM);
-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 regs[3] &=3D ((is_pae ? bitmaskof(X86_FEATURE_NX) : 0) |
+=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 regs[3] &=3D (bitmaskof(X86_FEATUR= E_NX) |
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (is_64bit ? bitma= skof(X86_FEATURE_LM) : 0) |
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 (is_64bit ? bitmaskof(X86_FEATURE_SYSCALL) : 0) |
=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0 (is_64bit ? bitmaskof(X86_FEATURE_RDTSCP) : 0))= ;


Zhuo

=

On Sat, Sep 6, 2014 at 12:09 AM, Jan Beu= lich <JBeulich@suse.com> wrote:
>>> On 05.09.14 at 17:45, <alfred.z.song@gmail.com> wrote:
> Hi, Jan
>
> I am sorry for making you confused.
>
> What I mean is there seems to be some redundant work. For example, to = leaf
> 0x80000001, the generic work (masking NX and PSE36) has been overwritt= en by
> the the vendor's functions (amd_xc_cpuid_policy and intel_xc_cpuid= _policy)
> , why couldn't we just drop them and leave the work to the vendor?= =C2=A0 Of
> cause, another way is just like you said, keeping the generic ones, an= d
> changing the logic in the vendor-based implementation.
>
> What I want to do is to simplify this if possible. :)

Right, yet conceptually anything that is defined by the architecture=
would better be done in the generic routine. Anything that is truly
vendor specific should be done in the vendor ones. And the
dependency of NX on PAE is (nowadays) an architectural one.

Jan


--089e0158ad9c62e7b40502540d02-- --===============1611960006063323248== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============1611960006063323248==--