Re: [Xen-devel] [PATCH] xc_cpuid_x86.c: Simplify masking conditions and remove redundant work

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

On Mon, Sep 8, 2014 at 8:07 PM, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:

On 08/09/14 13:02, Zhuo Song wrote:
> * Since there would not be 32-bit hypervisor, we do not need
>Â Âhypervisor_is_64bit() again.
>
> * Remove xen_64bit from xc_cpuid_pv_policy().
>
> * Because is_64bit only depends on is_pae, only use is_pae for both
>Â Âvendor specific functions.
>
> * Move conditions for LM/NX masking into architectural logic
>
> Signed-off-by: Zhuo Song <songzhuo.sz@xxxxxxxxxxxxxxx>
> ---
>Â tools/libxc/xc_cpuid_x86.c | 37 ++++++++++++++-----------------------
>Â 1 file changed, 14 insertions(+), 23 deletions(-)
>
> 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) |

You are changing the behaviour here, due to dropping is_pae.

This will break VM migrate.

~Andrew

I move it to architectural logic as I said. See:

In my opinion, for LM:

> +Â Â Â Â Â Â clear_bit(X86_FEATURE_LAHF_LM, regs[2]);
> +Â Â Â Â Â Â clear_bit(X86_FEATURE_LM, regs[3]);

for NX:

clear_bit(X86_FEATURE_NX, regs[3]);Â

should have done the work, so we do not need to do it again both in amd_xc_cpuid_policy or intel_xc_cpuid_policy

Zhuo

Â

>Â Â Â Â Â Â Â Â Â Â Â 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]);

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