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

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

On Mon, Sep 8, 2014 at 4:48 PM, z <alfred.z.song@xxxxxxxxx> wrote:

On Mon, Sep 8, 2014 at 2:59 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:

>>> On 05.09.14 at 18:35, <alfred.z.song@xxxxxxxxx> wrote:
> 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));

Right, and I think you could go further and move at least the LM
check into vendor-independent code too.

Â

Hi, Jan

Thanks. Yes, and I did some changes as below.
Please correct me if I did some improper ones.

diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 9add109..7d79dc2 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -80,7 +80,7 @@ static void xc_cpuid_brand_get(char *str)
Âstatic void amd_xc_cpuid_policy(
ÂÂÂÂ xc_interface *xch, domid_t domid,
ÂÂÂÂ const unsigned int *input, unsigned int *regs,
-ÂÂÂ int is_pae, int is_nestedhvm)
+ÂÂÂ int is_64bit, int is_pae, int is_nestedhvm)
Â{
ÂÂÂÂ switch ( input[0] )
ÂÂÂÂ {
@@ -95,13 +95,11 @@ static void amd_xc_cpuid_policy(
ÂÂÂÂÂÂÂÂ break;
Â
ÂÂÂÂ case 0x80000001: {
-ÂÂÂÂÂÂÂ int is_64bit = hypervisor_is_64bit(xch) && is_pae;
-
ÂÂÂÂÂÂÂÂ if ( !is_pae )
ÂÂÂÂÂÂÂÂÂÂÂÂ clear_bit(X86_FEATURE_PAE, regs[3]);
Â
ÂÂÂÂÂÂÂÂ /* Filter all other features according to a whitelist. */
-ÂÂÂÂÂÂÂ regs[2] &= ((is_64bit ? bitmaskof(X86_FEATURE_LAHF_LM) : 0) |
+ÂÂÂÂÂÂÂ regs[2] &= (bitmaskof(X86_FEATURE_LAHF_LM) |
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ bitmaskof(X86_FEATURE_CMP_LEGACY) |
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ (is_nestedhvm ? bitmaskof(X86_FEATURE_SVM) : 0) |
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ bitmaskof(X86_FEATURE_CR8_LEGACY) |
@@ -117,7 +115,7 @@ static void amd_xc_cpuid_policy(
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ bitmaskof(X86_FEATURE_DBEXT));
ÂÂÂÂÂÂÂÂ regs[3] &= (0x0183f3ff | /* features shared with 0x00000001:EDX */
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ bitmaskof(X86_FEATURE_NX) |
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ (is_64bit ? bitmaskof(X86_FEATURE_LM) : 0) |
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ bitmaskof(X86_FEATURE_LM) |
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ bitmaskof(X86_FEATURE_SYSCALL) |
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ bitmaskof(X86_FEATURE_MP) |
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ bitmaskof(X86_FEATURE_MMXEXT) |
@@ -169,7 +167,7 @@ static void amd_xc_cpuid_policy(
Âstatic void intel_xc_cpuid_policy(
ÂÂÂÂ xc_interface *xch, domid_t domid,
ÂÂÂÂ const unsigned int *input, unsigned int *regs,
-ÂÂÂ int is_pae, int is_nestedhvm)
+ÂÂÂ int is_64bit, int is_pae, int is_nestedhvm)
Â{
ÂÂÂÂ switch ( input[0] )
ÂÂÂÂ {
@@ -195,14 +193,12 @@ 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[2] &= (bitmaskof(X86_FEATURE_LAHF_LM) |
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ bitmaskof(X86_FEATURE_3DNOWPREFETCH) |
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ bitmaskof(X86_FEATURE_ABM);
ÂÂÂÂÂÂÂÂ regs[3] &= (bitmaskof(X86_FEATURE_NX) |
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ (is_64bit ? bitmaskof(X86_FEATURE_LM) : 0) |
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ bitmaskof(X86_FEATURE_LM) |
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ (is_64bit ? bitmaskof(X86_FEATURE_SYSCALL) : 0) |
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ (is_64bit ? bitmaskof(X86_FEATURE_RDTSCP) : 0));
ÂÂÂÂÂÂÂÂ break;
@@ -278,12 +274,14 @@ static void xc_cpuid_hvm_policy(
ÂÂÂÂ DECLARE_DOMCTL;
ÂÂÂÂ char brand[13];
ÂÂÂÂ uint64_t val;
-ÂÂÂ int is_pae, is_nestedhvm;
+ÂÂÂ int is_64bit, is_pae, is_nestedhvm;
ÂÂÂÂ uint64_t xfeature_mask;
Â
ÂÂÂÂ xc_hvm_param_get(xch, domid, HVM_PARAM_PAE_ENABLED, &val);
ÂÂÂÂ is_pae = !!val;
-
+ÂÂÂ
+ÂÂÂ is_64bit = hypervisor_is_64bit(xch) && is_pae;
+ÂÂÂ
ÂÂÂÂ /* Detecting Xen's atitude towards XSAVE */
ÂÂÂÂ memset(&domctl, 0, sizeof(domctl));
ÂÂÂÂ domctl.cmd = XEN_DOMCTL_getvcpuextstate;
@@ -391,10 +389,18 @@ static void xc_cpuid_hvm_policy(
ÂÂÂÂÂÂÂÂ break;
Â
ÂÂÂÂ case 0x80000001:
-ÂÂÂÂÂÂÂ if ( !is_pae ) {
+ÂÂÂÂÂÂÂ if ( !is_64bit ) {
+ÂÂÂÂÂÂÂÂÂÂÂ clear_bit(X86_FEATURE_LAHF_LM, regs[2]);
+ÂÂÂÂÂÂÂÂÂÂÂ clear_bit(X86_FEATURE_LM, regs[3]);
+ÂÂÂÂÂÂÂÂÂÂÂ clear_bit(X86_FEATURE_NX, regs[3]);
+ÂÂÂÂÂÂÂÂÂÂÂ clear_bit(X86_FEATURE_PSE36, regs[3]);
+ÂÂÂÂÂÂÂ } else if ( !is_pae ) {
ÂÂÂÂÂÂÂÂÂÂÂÂ clear_bit(X86_FEATURE_NX, regs[3]);
ÂÂÂÂÂÂÂÂÂÂÂÂ clear_bit(X86_FEATURE_PSE36, regs[3]);
+ÂÂÂÂÂÂÂ } else {
+ÂÂÂÂÂÂÂÂÂÂÂ /* Do nothing for 32-bit guest */
ÂÂÂÂÂÂÂÂ }
+
ÂÂÂÂÂÂÂÂ break;
Â
ÂÂÂÂ case 0x80000007:
@@ -430,10 +436,11 @@ static void xc_cpuid_hvm_policy(
Â
ÂÂÂÂ xc_cpuid_brand_get(brand);
ÂÂÂÂ if ( strstr(brand, "AMD") )
-ÂÂÂÂÂÂÂ amd_xc_cpuid_policy(xch, domid, input, regs, is_pae, is_nestedhvm);
+ÂÂÂÂÂÂÂ amd_xc_cpuid_policy(xch, domid, input, regs,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ is_64bit, is_pae, is_nestedhvm);
ÂÂÂÂ else
-ÂÂÂÂÂÂÂ intel_xc_cpuid_policy(xch, domid, input, regs, is_pae, is_nestedhvm);
-
+ÂÂÂÂÂÂÂ intel_xc_cpuid_policy(xch, domid, input, regs,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ is_64bit, is_pae, is_nestedhvm);

Â

Looking at the context above I also wonder whether tying RDTSCP
to 64-bit guests is really correct in (at least) the Intel case.

I am not so sure for now, but I will check it later as well:)

Hi, Jan

I just checked Intel SDM. It seems you are right. RDTSCP could be used for both 64-bit and 32-bit architectures. Maybe the limit for 64-bit should be removed too.

diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 710fd61..e7b50b1 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -193,7 +193,7 @@ static void intel_xc_cpuid_policy(
ÂÂÂÂÂÂÂÂ regs[3] &= (bitmaskof(X86_FEATURE_NX) |
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ bitmaskof(X86_FEATURE_LM) |
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ (is_pae ? bitmaskof(X86_FEATURE_SYSCALL) : 0) |
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ (is_pae ? bitmaskof(X86_FEATURE_RDTSCP) : 0));
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ (bitmaskof(X86_FEATURE_RDTSCP));
ÂÂÂÂÂÂÂÂ break;
ÂÂÂÂ }

Zhuo

Â

Â

Jan

Zhuo

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