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

[Andrew Cooper <andrew.cooper3@citrix.com>]

[-- 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