All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] xc_cpuid_x86.c: Simplify masking conditions and remove redundant work
@ 2014-09-08 12:02 Zhuo Song
  2014-09-08 12:07 ` Andrew Cooper
  0 siblings, 1 reply; 4+ messages in thread
From: Zhuo Song @ 2014-09-08 12:02 UTC (permalink / raw)
  To: xen-devel
  Cc: ian.campbell, stefano.stabellini, jinsong.liu, ian.jackson,
	Zhuo Song, Zhuo Song, boyu.mt, JBeulich

* 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@alibaba-inc.com>
---
 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) |
                     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]);
-- 
1.8.5.2 (Apple Git-48)

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] xc_cpuid_x86.c: Simplify masking conditions and remove redundant work
  2014-09-08 12:02 [PATCH] xc_cpuid_x86.c: Simplify masking conditions and remove redundant work Zhuo Song
@ 2014-09-08 12:07 ` Andrew Cooper
  2014-09-08 12:17   ` z
  0 siblings, 1 reply; 4+ messages in thread
From: Andrew Cooper @ 2014-09-08 12:07 UTC (permalink / raw)
  To: Zhuo Song, xen-devel
  Cc: ian.campbell, stefano.stabellini, jinsong.liu, ian.jackson,
	Zhuo Song, boyu.mt, JBeulich

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@alibaba-inc.com>
> ---
>  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

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] xc_cpuid_x86.c: Simplify masking conditions and remove redundant work
  2014-09-08 12:07 ` Andrew Cooper
@ 2014-09-08 12:17   ` z
  2014-09-08 13:00     ` Andrew Cooper
  0 siblings, 1 reply; 4+ messages in thread
From: z @ 2014-09-08 12:17 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Ian Campbell, Stefano Stabellini, jinsong.liu, ian.jackson,
	Zhuo Song, 马涛(伯瑜),
	Jan Beulich, xen-devel


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

On Mon, Sep 8, 2014 at 8:07 PM, Andrew Cooper <andrew.cooper3@citrix.com>
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@alibaba-inc.com>
> > ---
> >  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]);
>
>

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

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

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] xc_cpuid_x86.c: Simplify masking conditions and remove redundant work
  2014-09-08 12:17   ` z
@ 2014-09-08 13:00     ` Andrew Cooper
  0 siblings, 0 replies; 4+ messages in thread
From: Andrew Cooper @ 2014-09-08 13:00 UTC (permalink / raw)
  To: z
  Cc: Ian Campbell, Stefano Stabellini, jinsong.liu, ian.jackson,
	Zhuo Song, "马涛(伯瑜)",
	Jan Beulich, xen-devel


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

On 08/09/14 13:17, z wrote:
>
>
> On Mon, Sep 8, 2014 at 8:07 PM, Andrew Cooper
> <andrew.cooper3@citrix.com <mailto:andrew.cooper3@citrix.com>> 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@alibaba-inc.com
>     <mailto:songzhuo.sz@alibaba-inc.com>>
>     > ---
>     >  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

Oh - its setting a bit for an AND mask where the maskee has already had
the bit cleared.

In which case, it looks like it isn't changing the behaviour.

~Andrew

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

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

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2014-09-08 13:00 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-08 12:02 [PATCH] xc_cpuid_x86.c: Simplify masking conditions and remove redundant work Zhuo Song
2014-09-08 12:07 ` Andrew Cooper
2014-09-08 12:17   ` z
2014-09-08 13:00     ` Andrew Cooper

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.