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

[z <alfred.z.song@xxxxxxxxx>]

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@xxxxxxxx> wrote:

>>> On 05.09.14 at 17:45, <alfred.z.song@xxxxxxxxx> 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

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