Re: [PATCH] xc_cpuid_x86.c: No need to mask NX twice

[z <alfred.z.song@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2467 bytes --]

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 <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 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
>
>

[-- Attachment #1.2: Type: text/html, Size: 3435 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel