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 5:09 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:

>>> On 08.09.14 at 10:48, <alfred.z.song@xxxxxxxxx> wrote:

Things look fine from a general pov, but

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

... with this using hypervisor_is_64bit() and there not being a 32-bit
hypervisor anymore, there's clearly room for more cleanup (and in
particular no need to pass around an "is_64bit" variable that's always
going to be set to true).

Do your mean hypervisor_is_64bit() will always return true? Then it means that is_64bit will only depend on is_pae here, so we could simply use
is_pae instead of is_64bit in both vendor specific functions. If so, I think xen_64bit in xc_cpuid_pv_policy could also be dropped and function hypervisor_is_64bit() is also redundant now. Right?

Maybe something like this?

diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 6b81641..710fd61 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -34,13 +34,6 @@
Â#define DEF_MAX_INTELEXTÂ 0x80000008u
Â#define DEF_MAX_AMDEXTÂÂÂ 0x8000001cu
Â
-static int hypervisor_is_64bit(xc_interface *xch)
-{
-ÂÂÂ xen_capabilities_info_t xen_caps = "";
-ÂÂÂ return ((xc_version(xch, XENVER_capabilities, &xen_caps) == 0) &&
-ÂÂÂÂÂÂÂÂÂÂÂ (strstr(xen_caps, "x86_64") != NULL));
-}
-
Âstatic void cpuid(const unsigned int *input, unsigned int *regs)
Â{
ÂÂÂÂ unsigned int count = (input[1] == XEN_CPUID_INPUT_UNUSED) ? 0 : input[1];
@@ -95,13 +88,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) |
@@ -116,8 +107,8 @@ 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) |
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ (is_64bit ? bitmaskof(X86_FEATURE_LM) : 0) |
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ bitmaskof(X86_FEATURE_NX) |
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ bitmaskof(X86_FEATURE_LM) |
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ bitmaskof(X86_FEATURE_SYSCALL) |
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ bitmaskof(X86_FEATURE_MP) |
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ bitmaskof(X86_FEATURE_MMXEXT) |
@@ -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));
ÂÂÂÂÂÂÂÂ break;
ÂÂÂÂ }
Â
@@ -392,6 +381,8 @@ static void xc_cpuid_hvm_policy(
Â
ÂÂÂÂ case 0x80000001:
ÂÂÂÂÂÂÂÂ if ( !is_pae ) {
+ÂÂÂÂÂÂÂÂÂÂÂ 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]);
ÂÂÂÂÂÂÂÂ }
@@ -442,7 +433,7 @@ static void xc_cpuid_pv_policy(
Â{
ÂÂÂÂ DECLARE_DOMCTL;
ÂÂÂÂ unsigned int guest_width;
-ÂÂÂ int guest_64bit, xen_64bit = hypervisor_is_64bit(xch);
+ÂÂÂ int guest_64bit;
ÂÂÂÂ char brand[13];
ÂÂÂÂ uint64_t xfeature_mask;
Â
@@ -474,7 +465,7 @@ static void xc_cpuid_pv_policy(
ÂÂÂÂ switch ( input[0] )
ÂÂÂÂ {
ÂÂÂÂ case 0x00000001:
-ÂÂÂÂÂÂÂ if ( !xen_64bit || strstr(brand, "AMD") )
+ÂÂÂÂÂÂÂ if ( strstr(brand, "AMD") )
ÂÂÂÂÂÂÂÂÂÂÂÂ clear_bit(X86_FEATURE_SEP, regs[3]);
ÂÂÂÂÂÂÂÂ clear_bit(X86_FEATURE_DS, regs[3]);
ÂÂÂÂÂÂÂÂ clear_bit(X86_FEATURE_ACC, regs[3]);

Zhuo

> @@ -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 */
>Â Â Â Â Â }

The ordering of the if/else-if above seems wrong to me, but this
would become moot anyway if "is_64bit" got dropped.

Jan

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