[PATCH 0/6] x86/cpuid: Handling of simple leaves in guest_cpuid()

[PATCH 0/6] x86/cpuid: Handling of simple leaves in guest_cpuid()

[Andrew Cooper]

The following series (with any luck) is sufficient to allow PVH-dom0's to
function without any CPUID policy hackary during construction.

Andrew Cooper (6):
  x86/cpuid: Hide VT-x/SVM from HVM-based control domains
  x86/cpuid: Remove BUG_ON() condition from guest_cpuid()
  x86/cpuid: Handle leaf 0 in guest_cpuid()
  x86/cpuid: Handle leaf 0x80000000 in guest_cpuid()
  x86/cpuid: Handle the long vendor string in guest_cpuid()
  x86/cpuid: Only recalculate the shared feature bits once

 tools/libxc/xc_cpuid_x86.c   |  19 -------
 xen/arch/x86/cpuid.c         | 119 ++++++++++++++++++++++++++++++-------------
 xen/arch/x86/domctl.c        |  10 +---
 xen/arch/x86/hvm/nestedhvm.c |   3 +-
 xen/include/asm-x86/cpuid.h  |   6 ++-
 xen/tools/gen-cpuid.py       |  29 ++++-------
 6 files changed, 101 insertions(+), 85 deletions(-)

-- 
2.1.4


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

[PATCH 1/6] x86/cpuid: Hide VT-x/SVM from HVM-based control domains

[Andrew Cooper]

The VT-x/SVM features are hidden from PV dom0 by the pv_featureset[] upper
mask, but nothing thusfar has prevented the features being visible in
HVM-based control domains (where there is no toolstack decision to hide the
features).

As a side effect of calling nestedhvm_enabled() earlier during domain
creation, it needs to cope with the params[] array array not having been
allocated.

Reported-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpuid.c         | 25 ++++++++++++++++++-------
 xen/arch/x86/hvm/nestedhvm.c |  3 ++-
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index eb829d7..7b9af1b 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -3,6 +3,7 @@
 #include <xen/sched.h>
 #include <asm/cpuid.h>
 #include <asm/hvm/hvm.h>
+#include <asm/hvm/nestedhvm.h>
 #include <asm/hvm/vmx/vmcs.h>
 #include <asm/processor.h>
 #include <asm/xstate.h>
@@ -361,14 +362,24 @@ void recalculate_cpuid_policy(struct domain *d)
     cpuid_policy_to_featureset(p, fs);
     cpuid_policy_to_featureset(max, max_fs);
 
-    /*
-     * HVM domains using Shadow paging have further restrictions on their
-     * available paging features.
-     */
-    if ( is_hvm_domain(d) && !hap_enabled(d) )
+    if ( is_hvm_domain(d) )
     {
-        for ( i = 0; i < ARRAY_SIZE(max_fs); i++ )
-            max_fs[i] &= hvm_shadow_featuremask[i];
+        /*
+         * HVM domains using Shadow paging have further restrictions on their
+         * available paging features.
+         */
+        if ( !hap_enabled(d) )
+        {
+            for ( i = 0; i < ARRAY_SIZE(max_fs); i++ )
+                max_fs[i] &= hvm_shadow_featuremask[i];
+        }
+
+        /* Hide nested-virt if it hasn't been explicitly configured. */
+        if ( !nestedhvm_enabled(d) )
+        {
+            __clear_bit(X86_FEATURE_VMX, max_fs);
+            __clear_bit(X86_FEATURE_SVM, max_fs);
+        }
     }
 
     /*
diff --git a/xen/arch/x86/hvm/nestedhvm.c b/xen/arch/x86/hvm/nestedhvm.c
index a400d55..f2f7469 100644
--- a/xen/arch/x86/hvm/nestedhvm.c
+++ b/xen/arch/x86/hvm/nestedhvm.c
@@ -29,7 +29,8 @@ static unsigned long *shadow_io_bitmap[3];
 /* Nested HVM on/off per domain */
 bool nestedhvm_enabled(const struct domain *d)
 {
-    return is_hvm_domain(d) && d->arch.hvm_domain.params[HVM_PARAM_NESTEDHVM];
+    return is_hvm_domain(d) && d->arch.hvm_domain.params &&
+        d->arch.hvm_domain.params[HVM_PARAM_NESTEDHVM];
 }
 
 /* Nested VCPU */
-- 
2.1.4


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

[PATCH 2/6] x86/cpuid: Remove BUG_ON() condition from guest_cpuid()

[Andrew Cooper]

Include a min() against the appropriate ARRAY_SIZE(), and ASSERT() that
max_subleaf is within ARRAY_SIZE().

This is more robust to unexpected problems in a release build of Xen.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/cpuid.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 7b9af1b..076fab3 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -856,10 +856,11 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
         switch ( leaf )
         {
         case 0x7:
-            if ( subleaf > p->feat.max_subleaf )
+            ASSERT(p->feat.max_subleaf < ARRAY_SIZE(p->feat.raw));
+            if ( subleaf > min_t(uint32_t, p->feat.max_subleaf,
+                                 ARRAY_SIZE(p->feat.raw) - 1) )
                 return;
 
-            BUG_ON(subleaf >= ARRAY_SIZE(p->feat.raw));
             *res = p->feat.raw[subleaf];
             break;
 
-- 
2.1.4


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

[PATCH 3/6] x86/cpuid: Handle leaf 0 in guest_cpuid()

[Andrew Cooper]

Calculate a domains x86_vendor early in recalculate_cpuid_policy(); subsequent
patches need to make other recalculation decisions based on it.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/cpuid.c        | 13 ++++++++++++-
 xen/arch/x86/domctl.c       | 10 ++--------
 xen/include/asm-x86/cpuid.h |  4 +++-
 3 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 076fab3..85c829d 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -355,6 +355,9 @@ void recalculate_cpuid_policy(struct domain *d)
     uint32_t fs[FSCAPINTS], max_fs[FSCAPINTS];
     unsigned int i;
 
+    p->x86_vendor = get_cpu_vendor(p->basic.vendor_ebx, p->basic.vendor_ecx,
+                                   p->basic.vendor_edx, gcv_guest);
+
     p->basic.max_leaf   = min(p->basic.max_leaf,   max->basic.max_leaf);
     p->feat.max_subleaf = min(p->feat.max_subleaf, max->feat.max_subleaf);
     p->extd.max_leaf    = min(p->extd.max_leaf,    max->extd.max_leaf);
@@ -677,6 +680,7 @@ static void pv_cpuid(uint32_t leaf, uint32_t subleaf, struct cpuid_leaf *res)
         *res = EMPTY_LEAF;
         break;
 
+    case 0x0:
     case 0x7:
     case XSTATE_CPUID:
         ASSERT_UNREACHABLE();
@@ -825,6 +829,7 @@ static void hvm_cpuid(uint32_t leaf, uint32_t subleaf, struct cpuid_leaf *res)
             res->a = 0;
         break;
 
+    case 0x0:
     case 0x7:
     case XSTATE_CPUID:
         ASSERT_UNREACHABLE();
@@ -850,7 +855,9 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
     switch ( leaf )
     {
     case 0 ... CPUID_GUEST_NR_BASIC - 1:
-        if ( leaf > p->basic.max_leaf )
+        ASSERT(p->basic.max_leaf < ARRAY_SIZE(p->basic.raw));
+        if ( leaf > min_t(uint32_t, p->basic.max_leaf,
+                          ARRAY_SIZE(p->basic.raw) - 1) )
             return;
 
         switch ( leaf )
@@ -873,6 +880,10 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
 
         default:
             goto legacy;
+
+        case 0x0:
+            *res = p->basic.raw[leaf];
+            break;
         }
         break;
 
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index a3f51f3..8e5259f 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -92,7 +92,7 @@ static int update_domain_cpuid_info(struct domain *d,
 {
     struct cpuid_policy *p = d->arch.cpuid;
     const struct cpuid_leaf leaf = { ctl->eax, ctl->ebx, ctl->ecx, ctl->edx };
-    int rc;
+    int rc, old_vendor = p->x86_vendor;
 
     /*
      * Skip update for leaves we don't care about.  This avoids the overhead
@@ -155,11 +155,7 @@ static int update_domain_cpuid_info(struct domain *d,
 
     switch ( ctl->input[0] )
     {
-    case 0: {
-        int old_vendor = p->x86_vendor;
-
-        p->x86_vendor = get_cpu_vendor(ctl->ebx, ctl->ecx, ctl->edx, gcv_guest);
-
+    case 0:
         if ( is_hvm_domain(d) && (p->x86_vendor != old_vendor) )
         {
             struct vcpu *v;
@@ -167,9 +163,7 @@ static int update_domain_cpuid_info(struct domain *d,
             for_each_vcpu( d, v )
                 hvm_update_guest_vendor(v);
         }
-
         break;
-    }
 
     case 1:
         if ( is_pv_domain(d) && ((levelling_caps & LCAP_1cd) == LCAP_1cd) )
diff --git a/xen/include/asm-x86/cpuid.h b/xen/include/asm-x86/cpuid.h
index e5140ca..299a026 100644
--- a/xen/include/asm-x86/cpuid.h
+++ b/xen/include/asm-x86/cpuid.h
@@ -81,6 +81,7 @@ struct cpuid_policy
      *   - All of the feat and xstate unions
      *   - max_{,sub}leaf
      *   - All FEATURESET_* words
+     *   - Low short vendor infomation
      *
      * Per-domain objects:
      *
@@ -88,6 +89,7 @@ struct cpuid_policy
      *   - All of the feat and xstate unions
      *   - max_{,sub}leaf
      *   - All FEATURESET_* words
+     *   - Low short vendor infomation
      *
      * Everything else should be considered inaccurate, and not necesserily 0.
      */
@@ -101,7 +103,7 @@ struct cpuid_policy
         struct cpuid_leaf raw[CPUID_GUEST_NR_BASIC];
         struct {
             /* Leaf 0x0 - Max and vendor. */
-            uint32_t max_leaf, /* b */:32, /* c */:32, /* d */:32;
+            uint32_t max_leaf, vendor_ebx, vendor_ecx, vendor_edx;
 
             /* Leaf 0x1 - Family/model/stepping and features. */
             uint32_t raw_fms, /* b */:32;
-- 
2.1.4


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

[PATCH 4/6] x86/cpuid: Handle leaf 0x80000000 in guest_cpuid()

[Andrew Cooper]

The calculations for p->extd.max_leaf are reworked to force a value of at
least 0x80000000, and to take the domains chosen vendor into account when
clamping maximum value.

The high short vendor information is clobbered or duplicated according to the
chosen vendor.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/cpuid.c        | 48 +++++++++++++++++++++++++++++++++++++++------
 xen/include/asm-x86/cpuid.h |  6 +++---
 2 files changed, 45 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 85c829d..dc76cf4 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -163,6 +163,24 @@ static void recalculate_xstate(struct cpuid_policy *p)
     }
 }
 
+static void recalculate_common(struct cpuid_policy *p)
+{
+    switch ( p->x86_vendor )
+    {
+    case X86_VENDOR_INTEL:
+        p->extd.vendor_ebx = 0;
+        p->extd.vendor_ecx = 0;
+        p->extd.vendor_edx = 0;
+        break;
+
+    case X86_VENDOR_AMD:
+        p->extd.vendor_ebx = p->basic.vendor_ebx;
+        p->extd.vendor_ecx = p->basic.vendor_ecx;
+        p->extd.vendor_edx = p->basic.vendor_edx;
+        break;
+    }
+}
+
 static void __init calculate_raw_policy(void)
 {
     struct cpuid_policy *p = &raw_policy;
@@ -227,12 +245,12 @@ static void __init calculate_host_policy(void)
         min_t(uint32_t, p->basic.max_leaf,   ARRAY_SIZE(p->basic.raw) - 1);
     p->feat.max_subleaf =
         min_t(uint32_t, p->feat.max_subleaf, ARRAY_SIZE(p->feat.raw) - 1);
-    p->extd.max_leaf =
-        min_t(uint32_t, p->extd.max_leaf,
-              0x80000000u + ARRAY_SIZE(p->extd.raw) - 1);
+    p->extd.max_leaf = 0x80000000 | min_t(uint32_t, p->extd.max_leaf & 0xffff,
+                                          ARRAY_SIZE(p->extd.raw) - 1);
 
     cpuid_featureset_to_policy(boot_cpu_data.x86_capability, p);
     recalculate_xstate(p);
+    recalculate_common(p);
 }
 
 static void __init calculate_pv_max_policy(void)
@@ -360,7 +378,10 @@ void recalculate_cpuid_policy(struct domain *d)
 
     p->basic.max_leaf   = min(p->basic.max_leaf,   max->basic.max_leaf);
     p->feat.max_subleaf = min(p->feat.max_subleaf, max->feat.max_subleaf);
-    p->extd.max_leaf    = min(p->extd.max_leaf,    max->extd.max_leaf);
+    p->extd.max_leaf    = 0x80000000 | min(p->extd.max_leaf & 0xffff,
+                                           (p->x86_vendor == X86_VENDOR_AMD
+                                            ? CPUID_GUEST_NR_EXTD_AMD
+                                            : CPUID_GUEST_NR_EXTD_INTEL) - 1);
 
     cpuid_policy_to_featureset(p, fs);
     cpuid_policy_to_featureset(max, max_fs);
@@ -428,6 +449,7 @@ void recalculate_cpuid_policy(struct domain *d)
 
     cpuid_featureset_to_policy(fs, p);
     recalculate_xstate(p);
+    recalculate_common(p);
 }
 
 int init_domain_cpuid_policy(struct domain *d)
@@ -683,6 +705,7 @@ static void pv_cpuid(uint32_t leaf, uint32_t subleaf, struct cpuid_leaf *res)
     case 0x0:
     case 0x7:
     case XSTATE_CPUID:
+    case 0x80000000:
         ASSERT_UNREACHABLE();
         /* Now handled in guest_cpuid(). */
     }
@@ -832,6 +855,7 @@ static void hvm_cpuid(uint32_t leaf, uint32_t subleaf, struct cpuid_leaf *res)
     case 0x0:
     case 0x7:
     case XSTATE_CPUID:
+    case 0x80000000:
         ASSERT_UNREACHABLE();
         /* Now handled in guest_cpuid(). */
     }
@@ -901,9 +925,21 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
         return cpuid_hypervisor_leaves(v, leaf, subleaf, res);
 
     case 0x80000000 ... 0x80000000 + CPUID_GUEST_NR_EXTD - 1:
-        if ( leaf > p->extd.max_leaf )
+        ASSERT((p->extd.max_leaf & 0xffff) < ARRAY_SIZE(p->extd.raw));
+        if ( (leaf & 0xffff) > min_t(uint32_t, p->extd.max_leaf & 0xffff,
+                                     ARRAY_SIZE(p->extd.raw) - 1) )
             return;
-        goto legacy;
+
+        switch ( leaf )
+        {
+        default:
+            goto legacy;
+
+        case 0x80000000:
+            *res = p->extd.raw[leaf & 0xffff];
+            break;
+        }
+        break;
 
     default:
         return;
diff --git a/xen/include/asm-x86/cpuid.h b/xen/include/asm-x86/cpuid.h
index 299a026..4712b73 100644
--- a/xen/include/asm-x86/cpuid.h
+++ b/xen/include/asm-x86/cpuid.h
@@ -81,7 +81,7 @@ struct cpuid_policy
      *   - All of the feat and xstate unions
      *   - max_{,sub}leaf
      *   - All FEATURESET_* words
-     *   - Low short vendor infomation
+     *   - Short vendor infomation
      *
      * Per-domain objects:
      *
@@ -89,7 +89,7 @@ struct cpuid_policy
      *   - All of the feat and xstate unions
      *   - max_{,sub}leaf
      *   - All FEATURESET_* words
-     *   - Low short vendor infomation
+     *   - Short vendor infomation
      *
      * Everything else should be considered inaccurate, and not necesserily 0.
      */
@@ -168,7 +168,7 @@ struct cpuid_policy
         struct cpuid_leaf raw[CPUID_GUEST_NR_EXTD];
         struct {
             /* Leaf 0x80000000 - Max and vendor. */
-            uint32_t max_leaf, /* b */:32, /* c */:32, /* d */:32;
+            uint32_t max_leaf, vendor_ebx, vendor_ecx, vendor_edx;
 
             /* Leaf 0x80000001 - Family/model/stepping and features. */
             uint32_t /* a */:32, /* b */:32;
-- 
2.1.4


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

[PATCH 5/6] x86/cpuid: Handle the long vendor string in guest_cpuid()

[Andrew Cooper]

Leaves 0x80000002 through 0x80000004 are plain ASCII text, and require no
specific recalculation.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/cpuid.c        | 3 +++
 xen/include/asm-x86/cpuid.h | 4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index dc76cf4..7926d0b 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -706,6 +706,7 @@ static void pv_cpuid(uint32_t leaf, uint32_t subleaf, struct cpuid_leaf *res)
     case 0x7:
     case XSTATE_CPUID:
     case 0x80000000:
+    case 0x80000002 ... 0x80000004:
         ASSERT_UNREACHABLE();
         /* Now handled in guest_cpuid(). */
     }
@@ -856,6 +857,7 @@ static void hvm_cpuid(uint32_t leaf, uint32_t subleaf, struct cpuid_leaf *res)
     case 0x7:
     case XSTATE_CPUID:
     case 0x80000000:
+    case 0x80000002 ... 0x80000004:
         ASSERT_UNREACHABLE();
         /* Now handled in guest_cpuid(). */
     }
@@ -936,6 +938,7 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
             goto legacy;
 
         case 0x80000000:
+        case 0x80000002 ... 0x80000004:
             *res = p->extd.raw[leaf & 0xffff];
             break;
         }
diff --git a/xen/include/asm-x86/cpuid.h b/xen/include/asm-x86/cpuid.h
index 4712b73..d181e9a 100644
--- a/xen/include/asm-x86/cpuid.h
+++ b/xen/include/asm-x86/cpuid.h
@@ -81,7 +81,7 @@ struct cpuid_policy
      *   - All of the feat and xstate unions
      *   - max_{,sub}leaf
      *   - All FEATURESET_* words
-     *   - Short vendor infomation
+     *   - Short and long vendor infomation
      *
      * Per-domain objects:
      *
@@ -89,7 +89,7 @@ struct cpuid_policy
      *   - All of the feat and xstate unions
      *   - max_{,sub}leaf
      *   - All FEATURESET_* words
-     *   - Short vendor infomation
+     *   - Short and long vendor infomation
      *
      * Everything else should be considered inaccurate, and not necesserily 0.
      */
-- 
2.1.4


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

[PATCH 6/6] x86/cpuid: Only recalculate the shared feature bits once

[Andrew Cooper]

With accurate vendor information available, the shared bits can be sorted out
during recalculation, rather than at query time in the legacy cpuid path.

This means that:
 * Duplication can be dropped from the automatically generated cpuid data.
 * The toolstack need not worry about setting them appropriately.
 * They can be dropped from the system maximum featuresets.

While editing gen-cpuid.py, reflow some comments which exceeded the expected
line length.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxc/xc_cpuid_x86.c | 19 -------------------
 xen/arch/x86/cpuid.c       | 25 +++++--------------------
 xen/tools/gen-cpuid.py     | 29 ++++++++++-------------------
 3 files changed, 15 insertions(+), 58 deletions(-)

diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 96d6025..918590f 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -41,7 +41,6 @@ enum {
 #define DEF_MAX_BASE 0x0000000du
 #define DEF_MAX_INTELEXT  0x80000008u
 #define DEF_MAX_AMDEXT    0x8000001cu
-#define COMMON_1D CPUID_COMMON_1D_FEATURES
 
 int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps)
 {
@@ -712,24 +711,6 @@ static void sanitise_featureset(struct cpuid_domain_info *info)
             disabled_features[i] &= ~dfs[i];
         }
     }
-
-    switch ( info->vendor )
-    {
-    case VENDOR_INTEL:
-        /* Intel clears the common bits in e1d. */
-        info->featureset[featureword_of(X86_FEATURE_SYSCALL)] &= ~COMMON_1D;
-        break;
-
-    case VENDOR_AMD:
-        /* AMD duplicates the common bits between 1d and e1d. */
-        info->featureset[featureword_of(X86_FEATURE_SYSCALL)] =
-            ((info->featureset[featureword_of(X86_FEATURE_FPU)] & COMMON_1D) |
-             (info->featureset[featureword_of(X86_FEATURE_SYSCALL)] & ~COMMON_1D));
-        break;
-
-    default:
-        break;
-    }
 }
 
 int xc_cpuid_apply_policy(xc_interface *xch, domid_t domid,
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 7926d0b..a27a8d6 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -67,18 +67,6 @@ static void sanitise_featureset(uint32_t *fs)
             disabled_features[j] &= ~dfs[j];
         }
     }
-
-    /*
-     * Sort out shared bits.  We are constructing a featureset which needs to
-     * be applicable to a cross-vendor case.  Intel strictly clears the common
-     * bits in e1d, while AMD strictly duplicates them.
-     *
-     * We duplicate them here to be compatible with AMD while on Intel, and
-     * rely on logic closer to the guest to make the featureset stricter if
-     * emulating Intel.
-     */
-    fs[FEATURESET_e1d] = ((fs[FEATURESET_1d]  &  CPUID_COMMON_1D_FEATURES) |
-                          (fs[FEATURESET_e1d] & ~CPUID_COMMON_1D_FEATURES));
 }
 
 static void recalculate_xstate(struct cpuid_policy *p)
@@ -165,6 +153,8 @@ static void recalculate_xstate(struct cpuid_policy *p)
 
 static void recalculate_common(struct cpuid_policy *p)
 {
+    p->extd.e1d &= ~CPUID_COMMON_1D_FEATURES;
+
     switch ( p->x86_vendor )
     {
     case X86_VENDOR_INTEL:
@@ -177,6 +167,8 @@ static void recalculate_common(struct cpuid_policy *p)
         p->extd.vendor_ebx = p->basic.vendor_ebx;
         p->extd.vendor_ecx = p->basic.vendor_ecx;
         p->extd.vendor_edx = p->basic.vendor_edx;
+
+        p->extd.e1d |= p->basic._1d & CPUID_COMMON_1D_FEATURES;
         break;
     }
 }
@@ -665,10 +657,6 @@ static void pv_cpuid(uint32_t leaf, uint32_t subleaf, struct cpuid_leaf *res)
         res->c = p->extd.e1c;
         res->d = p->extd.e1d;
 
-        /* If not emulating AMD, clear the duplicated features in e1d. */
-        if ( p->x86_vendor != X86_VENDOR_AMD )
-            res->d &= ~CPUID_COMMON_1D_FEATURES;
-
         /*
          * MTRR used to unconditionally leak into PV guests.  They cannot MTRR
          * infrastructure at all, and shouldn't be able to see the feature.
@@ -790,11 +778,8 @@ static void hvm_cpuid(uint32_t leaf, uint32_t subleaf, struct cpuid_leaf *res)
         res->c = p->extd.e1c;
         res->d = p->extd.e1d;
 
-        /* If not emulating AMD, clear the duplicated features in e1d. */
-        if ( p->x86_vendor != X86_VENDOR_AMD )
-            res->d &= ~CPUID_COMMON_1D_FEATURES;
         /* fast-forward MSR_APIC_BASE.EN if it hasn't already been clobbered. */
-        else if ( vlapic_hw_disabled(vcpu_vlapic(v)) )
+        if ( vlapic_hw_disabled(vcpu_vlapic(v)) )
             res->d &= ~cpufeat_bit(X86_FEATURE_APIC);
 
         /*
diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
index 6212e4f..5cab6db 100755
--- a/xen/tools/gen-cpuid.py
+++ b/xen/tools/gen-cpuid.py
@@ -129,16 +129,7 @@ def crunch_numbers(state):
     common_1d = (FPU, VME, DE, PSE, TSC, MSR, PAE, MCE, CX8, APIC,
                  MTRR, PGE, MCA, CMOV, PAT, PSE36, MMX, FXSR)
 
-    # All known features.  Duplicate the common features in e1d
-    e1d_base = SYSCALL & ~31
-    state.known = featureset_to_uint32s(
-        state.names.keys() + [ e1d_base + (x % 32) for x in common_1d ],
-        nr_entries)
-
-    # Fold common back into names
-    for f in common_1d:
-        state.names[e1d_base + (f % 32)] = "E1D_" + state.names[f]
-
+    state.known = featureset_to_uint32s(state.names.keys(), nr_entries)
     state.common_1d = featureset_to_uint32s(common_1d, 1)[0]
     state.special = featureset_to_uint32s(state.raw_special, nr_entries)
     state.pv = featureset_to_uint32s(state.raw_pv, nr_entries)
@@ -248,13 +239,14 @@ def crunch_numbers(state):
         # standard 3DNow in the earlier K6 processors.
         _3DNOW: [_3DNOWEXT],
 
-        # This is just the dependency between AVX512 and AVX2 of XSTATE feature flags.
-        # If want to use AVX512, AVX2 must be supported and enabled.
+        # This is just the dependency between AVX512 and AVX2 of XSTATE
+        # feature flags.  If want to use AVX512, AVX2 must be supported and
+        # enabled.
         AVX2: [AVX512F],
 
-        # AVX512F is taken to mean hardware support for EVEX encoded instructions,
-        # 512bit registers, and the instructions themselves. All further AVX512 features
-        # are built on top of AVX512F
+        # AVX512F is taken to mean hardware support for EVEX encoded
+        # instructions, 512bit registers, and the instructions themselves. All
+        # further AVX512 features are built on top of AVX512F
         AVX512F: [AVX512DQ, AVX512IFMA, AVX512PF, AVX512ER, AVX512CD,
                   AVX512BW, AVX512VL, AVX512VBMI, AVX512_4VNNIW,
                   AVX512_4FMAPS, AVX512_VPOPCNTDQ],
@@ -305,10 +297,9 @@ def crunch_numbers(state):
             if name and name[0] in "0123456789":
                 name = "_" + name
 
-            # Don't generate names for the duplicate features, or ones
-            # fast-forwarded from other state
-            if (name.startswith("E1D_") or
-                name in ("APIC", "OSXSAVE", "OSPKE")):
+            # Don't generate names for features fast-forwarded from other
+            # state
+            if name in ("APIC", "OSXSAVE", "OSPKE"):
                 name = ""
 
             names.append(name.lower())
-- 
2.1.4


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

Re: [PATCH 1/6] x86/cpuid: Hide VT-x/SVM from HVM-based control domains

[Doug Goldstein]

[-- Attachment #1.1.1: Type: text/plain, Size: 753 bytes --]

On 1/18/17 2:40 PM, Andrew Cooper wrote:
> The VT-x/SVM features are hidden from PV dom0 by the pv_featureset[] upper
> mask, but nothing thusfar has prevented the features being visible in

thus far? Could be the difference between British English and American
English.

> HVM-based control domains (where there is no toolstack decision to hide the
> features).
> 
> As a side effect of calling nestedhvm_enabled() earlier during domain
> creation, it needs to cope with the params[] array array not having been

array array?

> allocated.
> 
> Reported-by: Roger Pau Monné <roger.pau@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Doug Goldstein <cardoe@cardoe.com>

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

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

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

Re: [PATCH 2/6] x86/cpuid: Remove BUG_ON() condition from guest_cpuid()

[Doug Goldstein]

[-- Attachment #1.1.1: Type: text/plain, Size: 373 bytes --]

On 1/18/17 2:40 PM, Andrew Cooper wrote:
> Include a min() against the appropriate ARRAY_SIZE(), and ASSERT() that
> max_subleaf is within ARRAY_SIZE().
> 
> This is more robust to unexpected problems in a release build of Xen.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Doug Goldstein <cardoe@cardoe.com>

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

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

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

Re: [PATCH 3/6] x86/cpuid: Handle leaf 0 in guest_cpuid()

[Doug Goldstein]

[-- Attachment #1.1.1: Type: text/plain, Size: 331 bytes --]

On 1/18/17 2:40 PM, Andrew Cooper wrote:
> Calculate a domains x86_vendor early in recalculate_cpuid_policy(); subsequent
> patches need to make other recalculation decisions based on it.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Doug Goldstein <cardoe@cardoe.com>

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

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

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

Re: [PATCH 4/6] x86/cpuid: Handle leaf 0x80000000 in guest_cpuid()

[Doug Goldstein]

[-- Attachment #1.1.1: Type: text/plain, Size: 466 bytes --]

On 1/18/17 2:40 PM, Andrew Cooper wrote:
> The calculations for p->extd.max_leaf are reworked to force a value of at
> least 0x80000000, and to take the domains chosen vendor into account when
> clamping maximum value.
> 
> The high short vendor information is clobbered or duplicated according to the
> chosen vendor.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Doug Goldstein <cardoe@cardoe.com>

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

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

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

Re: [PATCH 5/6] x86/cpuid: Handle the long vendor string in guest_cpuid()

[Doug Goldstein]

[-- Attachment #1.1.1: Type: text/plain, Size: 286 bytes --]

On 1/18/17 2:40 PM, Andrew Cooper wrote:
> Leaves 0x80000002 through 0x80000004 are plain ASCII text, and require no
> specific recalculation.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Doug Goldstein <cardoe@cardoe.com>

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

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

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

Re: [PATCH 6/6] x86/cpuid: Only recalculate the shared feature bits once

[Doug Goldstein]

[-- Attachment #1.1.1: Type: text/plain, Size: 674 bytes --]

On 1/18/17 2:40 PM, Andrew Cooper wrote:
> With accurate vendor information available, the shared bits can be sorted out
> during recalculation, rather than at query time in the legacy cpuid path.
> 
> This means that:
>  * Duplication can be dropped from the automatically generated cpuid data.
>  * The toolstack need not worry about setting them appropriately.
>  * They can be dropped from the system maximum featuresets.
> 
> While editing gen-cpuid.py, reflow some comments which exceeded the expected
> line length.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Doug Goldstein <cardoe@cardoe.com>

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

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

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

Re: [PATCH 6/6] x86/cpuid: Only recalculate the shared feature bits once

[Wei Liu]

On Wed, Jan 18, 2017 at 07:40:58PM +0000, Andrew Cooper wrote:
> With accurate vendor information available, the shared bits can be sorted out
> during recalculation, rather than at query time in the legacy cpuid path.
> 
> This means that:
>  * Duplication can be dropped from the automatically generated cpuid data.
>  * The toolstack need not worry about setting them appropriately.
>  * They can be dropped from the system maximum featuresets.
> 
> While editing gen-cpuid.py, reflow some comments which exceeded the expected
> line length.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH 1/6] x86/cpuid: Hide VT-x/SVM from HVM-based control domains

[Andrew Cooper]

On 19/01/17 03:56, Doug Goldstein wrote:
> On 1/18/17 2:40 PM, Andrew Cooper wrote:
>> The VT-x/SVM features are hidden from PV dom0 by the pv_featureset[] upper
>> mask, but nothing thusfar has prevented the features being visible in
> thus far? Could be the difference between British English and American
> English.

Just a lack of a space on my behalf.

>
>> HVM-based control domains (where there is no toolstack decision to hide the
>> features).
>>
>> As a side effect of calling nestedhvm_enabled() earlier during domain
>> creation, it needs to cope with the params[] array array not having been
> array array?

Both fixed.

~Andrew

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

Re: [PATCH 1/6] x86/cpuid: Hide VT-x/SVM from HVM-based control domains

[Jan Beulich]

>>> On 18.01.17 at 20:40, <andrew.cooper3@citrix.com> wrote:
> The VT-x/SVM features are hidden from PV dom0 by the pv_featureset[] upper
> mask, but nothing thusfar has prevented the features being visible in
> HVM-based control domains (where there is no toolstack decision to hide the
> features).
> 
> As a side effect of calling nestedhvm_enabled() earlier during domain
> creation, it needs to cope with the params[] array array not having been
> allocated.
> 
> Reported-by: Roger Pau Monné <roger.pau@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


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

Re: [PATCH 2/6] x86/cpuid: Remove BUG_ON() condition from guest_cpuid()

[Jan Beulich]

>>> On 18.01.17 at 20:40, <andrew.cooper3@citrix.com> wrote:
> Include a min() against the appropriate ARRAY_SIZE(), and ASSERT() that
> max_subleaf is within ARRAY_SIZE().
> 
> This is more robust to unexpected problems in a release build of Xen.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



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

Re: [PATCH 3/6] x86/cpuid: Handle leaf 0 in guest_cpuid()

[Jan Beulich]

>>> On 18.01.17 at 20:40, <andrew.cooper3@citrix.com> wrote:
> Calculate a domains x86_vendor early in recalculate_cpuid_policy(); subsequent
> patches need to make other recalculation decisions based on it.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



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

Re: [PATCH 4/6] x86/cpuid: Handle leaf 0x80000000 in guest_cpuid()

[Jan Beulich]

>>> On 18.01.17 at 20:40, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -163,6 +163,24 @@ static void recalculate_xstate(struct cpuid_policy *p)
>      }
>  }
>  
> +static void recalculate_common(struct cpuid_policy *p)
> +{
> +    switch ( p->x86_vendor )
> +    {
> +    case X86_VENDOR_INTEL:
> +        p->extd.vendor_ebx = 0;
> +        p->extd.vendor_ecx = 0;
> +        p->extd.vendor_edx = 0;
> +        break;
> +
> +    case X86_VENDOR_AMD:
> +        p->extd.vendor_ebx = p->basic.vendor_ebx;
> +        p->extd.vendor_ecx = p->basic.vendor_ecx;
> +        p->extd.vendor_edx = p->basic.vendor_edx;
> +        break;
> +    }
> +}

I find the word "common" in the name here not very indicative
of what the function does, especially with ...

> @@ -227,12 +245,12 @@ static void __init calculate_host_policy(void)
>          min_t(uint32_t, p->basic.max_leaf,   ARRAY_SIZE(p->basic.raw) - 1);
>      p->feat.max_subleaf =
>          min_t(uint32_t, p->feat.max_subleaf, ARRAY_SIZE(p->feat.raw) - 1);
> -    p->extd.max_leaf =
> -        min_t(uint32_t, p->extd.max_leaf,
> -              0x80000000u + ARRAY_SIZE(p->extd.raw) - 1);
> +    p->extd.max_leaf = 0x80000000 | min_t(uint32_t, p->extd.max_leaf & 0xffff,
> +                                          ARRAY_SIZE(p->extd.raw) - 1);
>  
>      cpuid_featureset_to_policy(boot_cpu_data.x86_capability, p);
>      recalculate_xstate(p);
> +    recalculate_common(p);
>  }

... the neighboring call here (which is quite a bit more specific).
Of course possible alternatives depend on what further uses of
this function you do (or do not) plan, but by the name of the
other function here it could be recalculate_extd_vendor().

> @@ -901,9 +925,21 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
>          return cpuid_hypervisor_leaves(v, leaf, subleaf, res);
>  
>      case 0x80000000 ... 0x80000000 + CPUID_GUEST_NR_EXTD - 1:
> -        if ( leaf > p->extd.max_leaf )
> +        ASSERT((p->extd.max_leaf & 0xffff) < ARRAY_SIZE(p->extd.raw));
> +        if ( (leaf & 0xffff) > min_t(uint32_t, p->extd.max_leaf & 0xffff,
> +                                     ARRAY_SIZE(p->extd.raw) - 1) )
>              return;
> -        goto legacy;
> +
> +        switch ( leaf )
> +        {
> +        default:
> +            goto legacy;
> +
> +        case 0x80000000:
> +            *res = p->extd.raw[leaf & 0xffff];

I take it that the plan is to have further leaves and up here, or else
the array index could simply be literal zero.

Jan


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

Re: [PATCH 5/6] x86/cpuid: Handle the long vendor string in guest_cpuid()

[Jan Beulich]

>>> On 18.01.17 at 20:40, <andrew.cooper3@citrix.com> wrote:
> Leaves 0x80000002 through 0x80000004 are plain ASCII text, and require no
> specific recalculation.

Do they not? We don't currently get them in line with a perhaps
overridden vendor, but maybe we should?

> @@ -936,6 +938,7 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
>              goto legacy;
>  
>          case 0x80000000:
> +        case 0x80000002 ... 0x80000004:
>              *res = p->extd.raw[leaf & 0xffff];
>              break;
>          }

Ah yes, here we go.

In any event
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH 4/6] x86/cpuid: Handle leaf 0x80000000 in guest_cpuid()

[Andrew Cooper]

On 20/01/17 15:58, Jan Beulich wrote:
>>>> On 18.01.17 at 20:40, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/cpuid.c
>> +++ b/xen/arch/x86/cpuid.c
>> @@ -163,6 +163,24 @@ static void recalculate_xstate(struct cpuid_policy *p)
>>      }
>>  }
>>  
>> +static void recalculate_common(struct cpuid_policy *p)
>> +{
>> +    switch ( p->x86_vendor )
>> +    {
>> +    case X86_VENDOR_INTEL:
>> +        p->extd.vendor_ebx = 0;
>> +        p->extd.vendor_ecx = 0;
>> +        p->extd.vendor_edx = 0;
>> +        break;
>> +
>> +    case X86_VENDOR_AMD:
>> +        p->extd.vendor_ebx = p->basic.vendor_ebx;
>> +        p->extd.vendor_ecx = p->basic.vendor_ecx;
>> +        p->extd.vendor_edx = p->basic.vendor_edx;
>> +        break;
>> +    }
>> +}
> I find the word "common" in the name here not very indicative
> of what the function does, especially with ...
>
>> @@ -227,12 +245,12 @@ static void __init calculate_host_policy(void)
>>          min_t(uint32_t, p->basic.max_leaf,   ARRAY_SIZE(p->basic.raw) - 1);
>>      p->feat.max_subleaf =
>>          min_t(uint32_t, p->feat.max_subleaf, ARRAY_SIZE(p->feat.raw) - 1);
>> -    p->extd.max_leaf =
>> -        min_t(uint32_t, p->extd.max_leaf,
>> -              0x80000000u + ARRAY_SIZE(p->extd.raw) - 1);
>> +    p->extd.max_leaf = 0x80000000 | min_t(uint32_t, p->extd.max_leaf & 0xffff,
>> +                                          ARRAY_SIZE(p->extd.raw) - 1);
>>  
>>      cpuid_featureset_to_policy(boot_cpu_data.x86_capability, p);
>>      recalculate_xstate(p);
>> +    recalculate_common(p);
>>  }
> ... the neighboring call here (which is quite a bit more specific).
> Of course possible alternatives depend on what further uses of
> this function you do (or do not) plan, but by the name of the
> other function here it could be recalculate_extd_vendor().

It adjustments common to the global calculations, and the per-domain
recalculations.

It isn't however just per-vendor adjustments; there are global
adjustments (like clobbering the reserved leaves).

I am open to any naming suggestions.

>
>> @@ -901,9 +925,21 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
>>          return cpuid_hypervisor_leaves(v, leaf, subleaf, res);
>>  
>>      case 0x80000000 ... 0x80000000 + CPUID_GUEST_NR_EXTD - 1:
>> -        if ( leaf > p->extd.max_leaf )
>> +        ASSERT((p->extd.max_leaf & 0xffff) < ARRAY_SIZE(p->extd.raw));
>> +        if ( (leaf & 0xffff) > min_t(uint32_t, p->extd.max_leaf & 0xffff,
>> +                                     ARRAY_SIZE(p->extd.raw) - 1) )
>>              return;
>> -        goto legacy;
>> +
>> +        switch ( leaf )
>> +        {
>> +        default:
>> +            goto legacy;
>> +
>> +        case 0x80000000:
>> +            *res = p->extd.raw[leaf & 0xffff];
> I take it that the plan is to have further leaves and up here, or else
> the array index could simply be literal zero.

I believe you found your answer in the following patch?  Eventually
(when the legacy path disappears), this entire switch statement will go.

~Andrew

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

Re: [PATCH 5/6] x86/cpuid: Handle the long vendor string in guest_cpuid()

[Andrew Cooper]

On 20/01/17 16:00, Jan Beulich wrote:
>>>> On 18.01.17 at 20:40, <andrew.cooper3@citrix.com> wrote:
>> Leaves 0x80000002 through 0x80000004 are plain ASCII text, and require no
>> specific recalculation.
> Do they not? We don't currently get them in line with a perhaps
> overridden vendor, but maybe we should?

There is no existing logic doing this.  My point here is we don't need
to make any adjustments to the values provided by the toolstack.

~Andrew

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

Re: [PATCH 6/6] x86/cpuid: Only recalculate the shared feature bits once

[Jan Beulich]

>>> On 18.01.17 at 20:40, <andrew.cooper3@citrix.com> wrote:
> @@ -165,6 +153,8 @@ static void recalculate_xstate(struct cpuid_policy *p)
>  
>  static void recalculate_common(struct cpuid_policy *p)
>  {
> +    p->extd.e1d &= ~CPUID_COMMON_1D_FEATURES;
> +
>      switch ( p->x86_vendor )
>      {
>      case X86_VENDOR_INTEL:
> @@ -177,6 +167,8 @@ static void recalculate_common(struct cpuid_policy *p)
>          p->extd.vendor_ebx = p->basic.vendor_ebx;
>          p->extd.vendor_ecx = p->basic.vendor_ecx;
>          p->extd.vendor_edx = p->basic.vendor_edx;
> +
> +        p->extd.e1d |= p->basic._1d & CPUID_COMMON_1D_FEATURES;
>          break;
>      }
>  }

Coming back to the name of this function, with this new addition in
mind, perhaps recalculate_extd_common()?

Independent of this the patch here is
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH 1/6] x86/cpuid: Hide VT-x/SVM from HVM-based control domains

[Roger Pau Monné]

On Wed, Jan 18, 2017 at 07:40:53PM +0000, Andrew Cooper wrote:
> The VT-x/SVM features are hidden from PV dom0 by the pv_featureset[] upper
> mask, but nothing thusfar has prevented the features being visible in
> HVM-based control domains (where there is no toolstack decision to hide the
> features).
> 
> As a side effect of calling nestedhvm_enabled() earlier during domain
> creation, it needs to cope with the params[] array array not having been
> allocated.
> 
> Reported-by: Roger Pau Monné <roger.pau@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/cpuid.c         | 25 ++++++++++++++++++-------
>  xen/arch/x86/hvm/nestedhvm.c |  3 ++-
>  2 files changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
> index eb829d7..7b9af1b 100644
> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -3,6 +3,7 @@
>  #include <xen/sched.h>
>  #include <asm/cpuid.h>
>  #include <asm/hvm/hvm.h>
> +#include <asm/hvm/nestedhvm.h>
>  #include <asm/hvm/vmx/vmcs.h>
>  #include <asm/processor.h>
>  #include <asm/xstate.h>
> @@ -361,14 +362,24 @@ void recalculate_cpuid_policy(struct domain *d)
>      cpuid_policy_to_featureset(p, fs);
>      cpuid_policy_to_featureset(max, max_fs);
>  
> -    /*
> -     * HVM domains using Shadow paging have further restrictions on their
> -     * available paging features.
> -     */
> -    if ( is_hvm_domain(d) && !hap_enabled(d) )
> +    if ( is_hvm_domain(d) )

This should be has_hvm_container_domain or else classic PVH is broken, but I
don't know how much we care about classic PVH any longer.

Roger.

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

Re: [PATCH 1/6] x86/cpuid: Hide VT-x/SVM from HVM-based control domains

[Jan Beulich]

>>> On 24.01.17 at 15:38, <roger.pau@citrix.com> wrote:
> On Wed, Jan 18, 2017 at 07:40:53PM +0000, Andrew Cooper wrote:
>> The VT-x/SVM features are hidden from PV dom0 by the pv_featureset[] upper
>> mask, but nothing thusfar has prevented the features being visible in
>> HVM-based control domains (where there is no toolstack decision to hide the
>> features).
>> 
>> As a side effect of calling nestedhvm_enabled() earlier during domain
>> creation, it needs to cope with the params[] array array not having been
>> allocated.
>> 
>> Reported-by: Roger Pau Monné <roger.pau@citrix.com>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>>  xen/arch/x86/cpuid.c         | 25 ++++++++++++++++++-------
>>  xen/arch/x86/hvm/nestedhvm.c |  3 ++-
>>  2 files changed, 20 insertions(+), 8 deletions(-)
>> 
>> diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
>> index eb829d7..7b9af1b 100644
>> --- a/xen/arch/x86/cpuid.c
>> +++ b/xen/arch/x86/cpuid.c
>> @@ -3,6 +3,7 @@
>>  #include <xen/sched.h>
>>  #include <asm/cpuid.h>
>>  #include <asm/hvm/hvm.h>
>> +#include <asm/hvm/nestedhvm.h>
>>  #include <asm/hvm/vmx/vmcs.h>
>>  #include <asm/processor.h>
>>  #include <asm/xstate.h>
>> @@ -361,14 +362,24 @@ void recalculate_cpuid_policy(struct domain *d)
>>      cpuid_policy_to_featureset(p, fs);
>>      cpuid_policy_to_featureset(max, max_fs);
>>  
>> -    /*
>> -     * HVM domains using Shadow paging have further restrictions on their
>> -     * available paging features.
>> -     */
>> -    if ( is_hvm_domain(d) && !hap_enabled(d) )
>> +    if ( is_hvm_domain(d) )
> 
> This should be has_hvm_container_domain or else classic PVH is broken, but I
> don't know how much we care about classic PVH any longer.

The old check excluded PVHv1 (due to it depending on HAP), as
does the new check (in a more explicit way), so I don't see what's
wrong here.

Jan

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

Re: [PATCH 1/6] x86/cpuid: Hide VT-x/SVM from HVM-based control domains

[Roger Pau Monné]

On Tue, Jan 24, 2017 at 08:10:56AM -0700, Jan Beulich wrote:
> >>> On 24.01.17 at 15:38, <roger.pau@citrix.com> wrote:
> > On Wed, Jan 18, 2017 at 07:40:53PM +0000, Andrew Cooper wrote:
> >> The VT-x/SVM features are hidden from PV dom0 by the pv_featureset[] upper
> >> mask, but nothing thusfar has prevented the features being visible in
> >> HVM-based control domains (where there is no toolstack decision to hide the
> >> features).
> >> 
> >> As a side effect of calling nestedhvm_enabled() earlier during domain
> >> creation, it needs to cope with the params[] array array not having been
> >> allocated.
> >> 
> >> Reported-by: Roger Pau Monné <roger.pau@citrix.com>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> ---
> >> CC: Jan Beulich <JBeulich@suse.com>
> >> CC: Roger Pau Monné <roger.pau@citrix.com>
> >> ---
> >>  xen/arch/x86/cpuid.c         | 25 ++++++++++++++++++-------
> >>  xen/arch/x86/hvm/nestedhvm.c |  3 ++-
> >>  2 files changed, 20 insertions(+), 8 deletions(-)
> >> 
> >> diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
> >> index eb829d7..7b9af1b 100644
> >> --- a/xen/arch/x86/cpuid.c
> >> +++ b/xen/arch/x86/cpuid.c
> >> @@ -3,6 +3,7 @@
> >>  #include <xen/sched.h>
> >>  #include <asm/cpuid.h>
> >>  #include <asm/hvm/hvm.h>
> >> +#include <asm/hvm/nestedhvm.h>
> >>  #include <asm/hvm/vmx/vmcs.h>
> >>  #include <asm/processor.h>
> >>  #include <asm/xstate.h>
> >> @@ -361,14 +362,24 @@ void recalculate_cpuid_policy(struct domain *d)
> >>      cpuid_policy_to_featureset(p, fs);
> >>      cpuid_policy_to_featureset(max, max_fs);
> >>  
> >> -    /*
> >> -     * HVM domains using Shadow paging have further restrictions on their
> >> -     * available paging features.
> >> -     */
> >> -    if ( is_hvm_domain(d) && !hap_enabled(d) )
> >> +    if ( is_hvm_domain(d) )
> > 
> > This should be has_hvm_container_domain or else classic PVH is broken, but I
> > don't know how much we care about classic PVH any longer.
> 
> The old check excluded PVHv1 (due to it depending on HAP), as
> does the new check (in a more explicit way), so I don't see what's
> wrong here.

Right, I guess this is caused by e94ce5, which did:

     case EXIT_REASON_CPUID:
     {
-        int rc;
-
-        if ( is_pvh_vcpu(v) )
-        {
-            pv_cpuid(regs);
-            rc = 0;
-        }
-        else
-            rc = vmx_do_cpuid(regs);
+        int rc = vmx_do_cpuid(regs);

Which removed the special casing for the PVH CPUID, and I assume pv_cpuid used
to remove the VT-x extensions from the output of CPUID?

Roger.

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

Re: [PATCH 1/6] x86/cpuid: Hide VT-x/SVM from HVM-based control domains

[Andrew Cooper]

On 24/01/17 15:41, Roger Pau Monné wrote:
> On Tue, Jan 24, 2017 at 08:10:56AM -0700, Jan Beulich wrote:
>>>>> On 24.01.17 at 15:38, <roger.pau@citrix.com> wrote:
>>> On Wed, Jan 18, 2017 at 07:40:53PM +0000, Andrew Cooper wrote:
>>>> The VT-x/SVM features are hidden from PV dom0 by the pv_featureset[] upper
>>>> mask, but nothing thusfar has prevented the features being visible in
>>>> HVM-based control domains (where there is no toolstack decision to hide the
>>>> features).
>>>>
>>>> As a side effect of calling nestedhvm_enabled() earlier during domain
>>>> creation, it needs to cope with the params[] array array not having been
>>>> allocated.
>>>>
>>>> Reported-by: Roger Pau Monné <roger.pau@citrix.com>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> ---
>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>>> ---
>>>>  xen/arch/x86/cpuid.c         | 25 ++++++++++++++++++-------
>>>>  xen/arch/x86/hvm/nestedhvm.c |  3 ++-
>>>>  2 files changed, 20 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
>>>> index eb829d7..7b9af1b 100644
>>>> --- a/xen/arch/x86/cpuid.c
>>>> +++ b/xen/arch/x86/cpuid.c
>>>> @@ -3,6 +3,7 @@
>>>>  #include <xen/sched.h>
>>>>  #include <asm/cpuid.h>
>>>>  #include <asm/hvm/hvm.h>
>>>> +#include <asm/hvm/nestedhvm.h>
>>>>  #include <asm/hvm/vmx/vmcs.h>
>>>>  #include <asm/processor.h>
>>>>  #include <asm/xstate.h>
>>>> @@ -361,14 +362,24 @@ void recalculate_cpuid_policy(struct domain *d)
>>>>      cpuid_policy_to_featureset(p, fs);
>>>>      cpuid_policy_to_featureset(max, max_fs);
>>>>  
>>>> -    /*
>>>> -     * HVM domains using Shadow paging have further restrictions on their
>>>> -     * available paging features.
>>>> -     */
>>>> -    if ( is_hvm_domain(d) && !hap_enabled(d) )
>>>> +    if ( is_hvm_domain(d) )
>>> This should be has_hvm_container_domain or else classic PVH is broken, but I
>>> don't know how much we care about classic PVH any longer.
>> The old check excluded PVHv1 (due to it depending on HAP), as
>> does the new check (in a more explicit way), so I don't see what's
>> wrong here.
> Right, I guess this is caused by e94ce5, which did:
>
>      case EXIT_REASON_CPUID:
>      {
> -        int rc;
> -
> -        if ( is_pvh_vcpu(v) )
> -        {
> -            pv_cpuid(regs);
> -            rc = 0;
> -        }
> -        else
> -            rc = vmx_do_cpuid(regs);
> +        int rc = vmx_do_cpuid(regs);
>
> Which removed the special casing for the PVH CPUID, and I assume pv_cpuid used
> to remove the VT-x extensions from the output of CPUID?

PVH guests still enter pv_cpuid() via the legacy path in guest_cpuid().

However, PVH cpuid handling was quite broken to start with.  I am not
deliberately trying to make it worse, so your original suggestion should
probably be made (if anyone actually cares).

~Andrew

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