[PATCH 0/6] Further CPUID improvements

[PATCH 0/6] Further CPUID improvements

[Andrew Cooper]

The main purpose of this series is move all all xstate handling to the
non-legacy path.

Andrew Cooper (6):
  x86/xstate: Fix array overrun on hardware with LWP
  x86/cpuid: Introduce recalculate_xstate()
  x86/cpuid: Move all xstate leaf handling into guest_cpuid()
  tools/libxc: Remove xsave calculations from libxc
  x86/cpuid: Don't offer HVM hypervisor leaves to PV guests
  x86/cpuid: Offer ITSC to domains which are automatically non-migrateable

 tools/libxc/xc_cpuid_x86.c      | 143 +++-----------------
 xen/arch/x86/cpuid.c            | 283 +++++++++++++++++++---------------------
 xen/arch/x86/domctl.c           |   4 +-
 xen/arch/x86/traps.c            |  10 +-
 xen/arch/x86/xstate.c           |   2 +-
 xen/include/asm-x86/cpuid.h     |  17 ++-
 xen/include/asm-x86/processor.h |  10 ++
 xen/include/asm-x86/xstate.h    |   2 +-
 8 files changed, 184 insertions(+), 287 deletions(-)

-- 
2.1.4


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

[PATCH 1/6] x86/xstate: Fix array overrun on hardware with LWP

[Andrew Cooper]

c/s da62246e4c "x86/xsaves: enable xsaves/xrstors/xsavec in xen" introduced
setup_xstate_features() to allocate and fill xstate_offsets[] and
xstate_sizes[].

However, fls() casts xfeature_mask to 32bits which truncates LWP out of the
calculation.  As a result, the arrays are allocated too short, and the cpuid
infrastructure reads off the end of them when calculating xstate_size for the
guest.

On one test system, this results in 0x3fec83c0 being returned as the maximum
size of an xsave area, which surprisingly appears not to bother Windows or
Linux too much.  I suspect they both use current size based on xcr0, which Xen
forwards from real hardware.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>

This needs backporting to 4.8 and 4.7
---
 xen/arch/x86/xstate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 6073e1d..b8ae656 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -92,7 +92,7 @@ static int setup_xstate_features(bool_t bsp)
 
     if ( bsp )
     {
-        xstate_features = fls(xfeature_mask);
+        xstate_features = flsl(xfeature_mask);
         xstate_offsets = xzalloc_array(unsigned int, xstate_features);
         if ( !xstate_offsets )
             return -ENOMEM;
-- 
2.1.4


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

[PATCH 2/6] x86/cpuid: Introduce recalculate_xstate()

[Andrew Cooper]

All data in the xstate union, other than the Da1 feature word, is derived from
other state; either feature bits from other words, or layout information which
has already been collected by Xen's xstate driver.

Recalculate the xstate information for each policy object when the feature
bits may have changed.

The XSTATE_XSAVES_ONLY define needs extending to a 64bit value to avoid
problems when taking its converse for masking purposes.

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

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 95040f9..303568d 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -80,6 +80,103 @@ static void sanitise_featureset(uint32_t *fs)
                           (fs[FEATURESET_e1d] & ~CPUID_COMMON_1D_FEATURES));
 }
 
+static void recalculate_xstate(struct cpuid_policy *p)
+{
+    uint64_t xstates = XSTATE_FP_SSE;
+    uint32_t xstate_size = XSTATE_AREA_MIN_SIZE;
+    unsigned int i, Da1 = p->xstate.Da1;
+
+    /*
+     * The Da1 leaf is the only piece if information preserved.  Everything
+     * else is derived from other feature state.
+     */
+    memset(&p->xstate, 0, sizeof(p->xstate));
+
+    if ( !p->basic.xsave )
+        return;
+
+    if ( p->basic.avx )
+    {
+        xstates |= XSTATE_YMM;
+        xstate_size = max(xstate_size,
+                          xstate_offsets[_XSTATE_YMM] +
+                          xstate_sizes[_XSTATE_YMM]);
+    }
+
+    if ( p->feat.mpx )
+    {
+        xstates |= XSTATE_BNDREGS | XSTATE_BNDCSR;
+        xstate_size = max(xstate_size,
+                          xstate_offsets[_XSTATE_BNDCSR] +
+                          xstate_sizes[_XSTATE_BNDCSR]);
+    }
+
+    if ( p->feat.avx512f )
+    {
+        xstates |= XSTATE_OPMASK | XSTATE_ZMM | XSTATE_HI_ZMM;
+        xstate_size = max(xstate_size,
+                          xstate_offsets[_XSTATE_HI_ZMM] +
+                          xstate_sizes[_XSTATE_HI_ZMM]);
+    }
+
+    if ( p->feat.pku )
+    {
+        xstates |= XSTATE_PKRU;
+        xstate_size = max(xstate_size,
+                          xstate_offsets[_XSTATE_PKRU] +
+                          xstate_sizes[_XSTATE_PKRU]);
+    }
+
+    if ( p->extd.lwp )
+    {
+        xstates |= XSTATE_LWP;
+        xstate_size = max(xstate_size,
+                          xstate_offsets[_XSTATE_LWP] +
+                          xstate_sizes[_XSTATE_LWP]);
+    }
+
+    /* Sanity check we aren't advertising unknown states. */
+    ASSERT((xstates & ~XCNTXT_MASK) == 0);
+
+    p->xstate.max_size  =  xstate_size;
+    p->xstate.xcr0_low  =  xstates & ~XSTATE_XSAVES_ONLY;
+    p->xstate.xcr0_high = (xstates & ~XSTATE_XSAVES_ONLY) >> 32;
+
+    p->xstate.Da1 = Da1;
+    if ( p->xstate.xsaves )
+    {
+        p->xstate.xss_low   =  xstates & XSTATE_XSAVES_ONLY;
+        p->xstate.xss_high  = (xstates & XSTATE_XSAVES_ONLY) >> 32;
+    }
+    else
+        xstates &= ~XSTATE_XSAVES_ONLY;
+
+    for ( i = 2; i < min(63ul, ARRAY_SIZE(p->xstate.comp)); ++i )
+    {
+        uint64_t curr_xstate = 1ul << i;
+
+        if ( !(xstates & curr_xstate) )
+            continue;
+
+        p->xstate.comp[i].size   = xstate_sizes[i];
+        p->xstate.comp[i].offset = xstate_offsets[i];
+        p->xstate.comp[i].xss    = curr_xstate & XSTATE_XSAVES_ONLY;
+        p->xstate.comp[i].align  = curr_xstate & xstate_align;
+
+        /*
+         * Sanity checks:
+         * - All valid components should have non-zero size.
+         * - All xcr0 components should have non-zero offset.
+         * - All xss components should report 0 offset.
+         */
+        ASSERT(xstate_sizes[i]);
+        if ( curr_xstate & XSTATE_XSAVES_ONLY )
+            ASSERT(xstate_offsets[i] == 0);
+        else
+            ASSERT(xstate_offsets[i]);
+    }
+}
+
 static void __init calculate_raw_policy(void)
 {
     struct cpuid_policy *p = &raw_policy;
@@ -147,6 +244,7 @@ static void __init calculate_host_policy(void)
               0x80000000u + ARRAY_SIZE(p->extd.raw) - 1);
 
     cpuid_featureset_to_policy(boot_cpu_data.x86_capability, p);
+    recalculate_xstate(p);
 }
 
 static void __init calculate_pv_max_policy(void)
@@ -166,6 +264,7 @@ static void __init calculate_pv_max_policy(void)
 
     sanitise_featureset(pv_featureset);
     cpuid_featureset_to_policy(pv_featureset, p);
+    recalculate_xstate(p);
 }
 
 static void __init calculate_hvm_max_policy(void)
@@ -219,6 +318,7 @@ static void __init calculate_hvm_max_policy(void)
 
     sanitise_featureset(hvm_featureset);
     cpuid_featureset_to_policy(hvm_featureset, p);
+    recalculate_xstate(p);
 }
 
 void __init init_guest_cpuid(void)
@@ -326,6 +426,7 @@ void recalculate_cpuid_policy(struct domain *d)
                            special_features[FEATURESET_7b0]);
 
     cpuid_featureset_to_policy(fs, p);
+    recalculate_xstate(p);
 }
 
 int init_domain_cpuid_policy(struct domain *d)
diff --git a/xen/include/asm-x86/cpuid.h b/xen/include/asm-x86/cpuid.h
index b359b38..3f92955 100644
--- a/xen/include/asm-x86/cpuid.h
+++ b/xen/include/asm-x86/cpuid.h
@@ -77,18 +77,15 @@ struct cpuid_policy
      *
      * Global *_policy objects:
      *
-     * - Host accurate:
-     *   - {xcr0,xss}_{high,low}
-     *
      * - Guest accurate:
-     *   - All of the feat union
+     *   - All of the feat and xstate unions
      *   - max_{,sub}leaf
      *   - All FEATURESET_* words
      *
      * Per-domain objects:
      *
      * - Guest accurate:
-     *   - All of the feat union
+     *   - All of the feat and xstate unions
      *   - max_{,sub}leaf
      *   - All FEATURESET_* words
      *
@@ -143,9 +140,10 @@ struct cpuid_policy
     /* Xstate feature leaf: 0x0000000D[xx] */
     union {
         struct cpuid_leaf raw[CPUID_GUEST_NR_XSTATE];
+
         struct {
             /* Subleaf 0. */
-            uint32_t xcr0_low, /* b */:32, /* c */:32, xcr0_high;
+            uint32_t xcr0_low, /* b */:32, max_size, xcr0_high;
 
             /* Subleaf 1. */
             union {
@@ -154,6 +152,13 @@ struct cpuid_policy
             };
             uint32_t /* b */:32, xss_low, xss_high;
         };
+
+        /* Per-component common state.  Valid for i >= 2. */
+        struct {
+            uint32_t size, offset;
+            bool xss:1, align:1;
+            uint32_t /* c */:30, /* d */:32;
+        } comp[CPUID_GUEST_NR_XSTATE];
     } xstate;
 
     /* Extended leaves: 0x800000xx */
diff --git a/xen/include/asm-x86/xstate.h b/xen/include/asm-x86/xstate.h
index a3d37b8..c27735a 100644
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -55,7 +55,7 @@
 #define XSTATE_NONLAZY (XSTATE_LWP | XSTATE_BNDREGS | XSTATE_BNDCSR | \
                         XSTATE_PKRU)
 #define XSTATE_LAZY    (XSTATE_ALL & ~XSTATE_NONLAZY)
-#define XSTATE_XSAVES_ONLY         0
+#define XSTATE_XSAVES_ONLY         0ULL
 #define XSTATE_COMPACTION_ENABLED  (1ULL << 63)
 
 #define XSTATE_ALIGN64 (1U << 1)
-- 
2.1.4


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

[PATCH 3/6] x86/cpuid: Move all xstate leaf handling into guest_cpuid()

[Andrew Cooper]

The xstate union now contains sanitised values, so it can be handled fully in
the non-legacy path.

c/s 1c0bc709d "x86/cpuid: Perform max_leaf calculations in guest_cpuid()"
accidentally introduced a boundary error for the subleaf check, although it
was masked by the correct logic in the legacy path.

Two dynamic adjustments need making, but a TODO and BUILD_BUG_ON() are left to
cover a latent bug which will present itself when Xen starts supporting XSS
states for guests.

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

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 303568d..9f16502 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -484,8 +484,6 @@ static void pv_cpuid(uint32_t leaf, uint32_t subleaf, struct cpuid_leaf *res)
 
     switch ( leaf )
     {
-        uint32_t tmp;
-
     case 0x00000001:
         res->c = p->basic._1c;
         res->d = p->basic._1d;
@@ -637,57 +635,6 @@ static void pv_cpuid(uint32_t leaf, uint32_t subleaf, struct cpuid_leaf *res)
             res->a = (res->a & ~0xff) | 3;
         break;
 
-    case XSTATE_CPUID:
-        if ( !p->basic.xsave || subleaf >= 63 )
-            goto unsupported;
-        switch ( subleaf )
-        {
-        case 0:
-        {
-            uint64_t xfeature_mask = XSTATE_FP_SSE;
-            uint32_t xstate_size = XSTATE_AREA_MIN_SIZE;
-
-            if ( p->basic.avx )
-            {
-                xfeature_mask |= XSTATE_YMM;
-                xstate_size = (xstate_offsets[_XSTATE_YMM] +
-                               xstate_sizes[_XSTATE_YMM]);
-            }
-
-            if ( p->feat.avx512f )
-            {
-                xfeature_mask |= XSTATE_OPMASK | XSTATE_ZMM | XSTATE_HI_ZMM;
-                xstate_size = max(xstate_size,
-                                  xstate_offsets[_XSTATE_OPMASK] +
-                                  xstate_sizes[_XSTATE_OPMASK]);
-                xstate_size = max(xstate_size,
-                                  xstate_offsets[_XSTATE_ZMM] +
-                                  xstate_sizes[_XSTATE_ZMM]);
-                xstate_size = max(xstate_size,
-                                  xstate_offsets[_XSTATE_HI_ZMM] +
-                                  xstate_sizes[_XSTATE_HI_ZMM]);
-            }
-
-            res->a = xfeature_mask;
-            res->d = xfeature_mask >> 32;
-            res->c = xstate_size;
-
-            /*
-             * Always read CPUID.0xD[ECX=0].EBX from hardware, rather than
-             * domain policy.  It varies with enabled xstate, and the correct
-             * xcr0 is in context.
-             */
-            cpuid_count(leaf, subleaf, &tmp, &res->b, &tmp, &tmp);
-            break;
-        }
-
-        case 1:
-            res->a = p->xstate.Da1;
-            res->b = res->c = res->d = 0;
-            break;
-        }
-        break;
-
     case 0x80000001:
         res->c = p->extd.e1c;
         res->d = p->extd.e1d;
@@ -730,6 +677,7 @@ static void pv_cpuid(uint32_t leaf, uint32_t subleaf, struct cpuid_leaf *res)
         break;
 
     case 0x7:
+    case XSTATE_CPUID:
         ASSERT_UNREACHABLE();
         /* Now handled in guest_cpuid(). */
     }
@@ -797,98 +745,6 @@ static void hvm_cpuid(uint32_t leaf, uint32_t subleaf, struct cpuid_leaf *res)
         res->d = v->vcpu_id * 2;
         break;
 
-    case XSTATE_CPUID:
-        if ( !p->basic.xsave || subleaf >= 63 )
-        {
-            *res = EMPTY_LEAF;
-            break;
-        }
-        switch ( subleaf )
-        {
-        case 0:
-        {
-            uint64_t xfeature_mask = XSTATE_FP_SSE;
-            uint32_t xstate_size = XSTATE_AREA_MIN_SIZE;
-
-            if ( p->basic.avx )
-            {
-                xfeature_mask |= XSTATE_YMM;
-                xstate_size = max(xstate_size,
-                                  xstate_offsets[_XSTATE_YMM] +
-                                  xstate_sizes[_XSTATE_YMM]);
-            }
-
-            if ( p->feat.mpx )
-            {
-                xfeature_mask |= XSTATE_BNDREGS | XSTATE_BNDCSR;
-                xstate_size = max(xstate_size,
-                                  xstate_offsets[_XSTATE_BNDCSR] +
-                                  xstate_sizes[_XSTATE_BNDCSR]);
-            }
-
-            if ( p->feat.avx512f )
-            {
-                xfeature_mask |= XSTATE_OPMASK | XSTATE_ZMM | XSTATE_HI_ZMM;
-                xstate_size = max(xstate_size,
-                                  xstate_offsets[_XSTATE_OPMASK] +
-                                  xstate_sizes[_XSTATE_OPMASK]);
-                xstate_size = max(xstate_size,
-                                  xstate_offsets[_XSTATE_ZMM] +
-                                  xstate_sizes[_XSTATE_ZMM]);
-                xstate_size = max(xstate_size,
-                                  xstate_offsets[_XSTATE_HI_ZMM] +
-                                  xstate_sizes[_XSTATE_HI_ZMM]);
-            }
-
-            if ( p->feat.pku )
-            {
-                xfeature_mask |= XSTATE_PKRU;
-                xstate_size = max(xstate_size,
-                                  xstate_offsets[_XSTATE_PKRU] +
-                                  xstate_sizes[_XSTATE_PKRU]);
-            }
-
-            if ( p->extd.lwp )
-            {
-                xfeature_mask |= XSTATE_LWP;
-                xstate_size = max(xstate_size,
-                                  xstate_offsets[_XSTATE_LWP] +
-                                  xstate_sizes[_XSTATE_LWP]);
-            }
-
-            res->a = xfeature_mask;
-            res->d = xfeature_mask >> 32;
-            res->c = xstate_size;
-
-            /*
-             * Always read CPUID[0xD,0].EBX from hardware, rather than domain
-             * policy.  It varies with enabled xstate, and the correct xcr0 is
-             * in context.
-             */
-            cpuid_count(leaf, subleaf, &tmp, &res->b, &tmp, &tmp);
-            break;
-        }
-
-        case 1:
-            res->a = p->xstate.Da1;
-
-            if ( p->xstate.xsaves )
-            {
-                /*
-                 * Always read CPUID[0xD,1].EBX from hardware, rather than
-                 * domain policy.  It varies with enabled xstate, and the
-                 * correct xcr0/xss are in context.
-                 */
-                cpuid_count(leaf, subleaf, &tmp, &res->b, &tmp, &tmp);
-            }
-            else
-                res->b = 0;
-
-            res->c = res->d = 0;
-            break;
-        }
-        break;
-
     case 0x0000000a: /* Architectural Performance Monitor Features (Intel) */
         if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL || !vpmu_enabled(v) )
         {
@@ -970,6 +826,7 @@ static void hvm_cpuid(uint32_t leaf, uint32_t subleaf, struct cpuid_leaf *res)
         break;
 
     case 0x7:
+    case XSTATE_CPUID:
         ASSERT_UNREACHABLE();
         /* Now handled in guest_cpuid(). */
     }
@@ -1007,10 +864,13 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
             break;
 
         case XSTATE_CPUID:
-            if ( subleaf > ARRAY_SIZE(p->xstate.raw) )
+            if ( !p->basic.xsave || subleaf >= ARRAY_SIZE(p->xstate.raw) )
                 return;
 
-            /* Fallthrough. */
+            BUG_ON(subleaf >= ARRAY_SIZE(p->xstate.raw));
+            *res = p->xstate.raw[subleaf];
+            break;
+
         default:
             goto legacy;
         }
@@ -1067,6 +927,31 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
             break;
         }
         break;
+
+    case XSTATE_CPUID:
+        switch ( subleaf )
+        {
+        case 1:
+            if ( p->xstate.xsaves )
+            {
+                /*
+                 * TODO: Figure out what to do for XSS state.  VT-x manages
+                 * host vs guest MSR_XSS automatically, so as soon as we start
+                 * supporting any XSS states, the wrong XSS will be in
+                 * context.
+                 */
+                BUILD_BUG_ON(XSTATE_XSAVES_ONLY != 0);
+
+                /*
+                 * Read CPUID[0xD,0/1].EBX from hardware.  They vary with
+                 * enabled XSTATE, and appropraite XCR0|XSS are in context.
+                 */
+        case 0:
+                res->b = cpuid_count_ebx(leaf, subleaf);
+            }
+            break;
+        }
+        break;
     }
 
     /* Done. */
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 772c5d2..8e8437d 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -105,8 +105,10 @@ static int update_domain_cpuid_info(struct domain *d,
         if ( ctl->input[0] == 7 &&
              ctl->input[1] >= ARRAY_SIZE(p->feat.raw) )
             return 0;
+
+        BUILD_BUG_ON(ARRAY_SIZE(p->xstate.raw) < 2);
         if ( ctl->input[0] == XSTATE_CPUID &&
-             ctl->input[1] >= ARRAY_SIZE(p->xstate.raw) )
+             ctl->input[1] != 1 ) /* Everything else automatically calculated. */
             return 0;
         break;
 
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index b130f47..54d0a17 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -319,6 +319,16 @@ static always_inline unsigned int cpuid_edx(unsigned int op)
     return edx;
 }
 
+static always_inline unsigned int cpuid_count_ebx(
+    unsigned int leaf, unsigned int subleaf)
+{
+    unsigned int ebx, tmp;
+
+    cpuid_count(leaf, subleaf, &tmp, &ebx, &tmp, &tmp);
+
+    return ebx;
+}
+
 static inline unsigned long read_cr0(void)
 {
     unsigned long cr0;
-- 
2.1.4


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

[PATCH 4/6] tools/libxc: Remove xsave calculations from libxc

[Andrew Cooper]

libxc performs a lot of calculations for the xstate leaf when generating a
guests cpuid policy.  To correctly construct a policy for an HVM guest, this
logic depends on native cpuid leaking through from real hardware.

In particular, the logic is potentially wrong for an HVM-based toolstack
domain (e.g. PVH dom0), and definitely wrong if cpuid faulting is applied to a
PV domain.

Xen now performs all the necessary calculations, using native values.  The
only piece of information the toolstack need worry about is single xstate
feature leaf.

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 | 143 +++++----------------------------------------
 1 file changed, 15 insertions(+), 128 deletions(-)

diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index b32001b3..96d6025 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -400,125 +400,6 @@ static void intel_xc_cpuid_policy(xc_interface *xch,
     }
 }
 
-/* XSTATE bits in XCR0. */
-#define X86_XCR0_X87    (1ULL <<  0)
-#define X86_XCR0_SSE    (1ULL <<  1)
-#define X86_XCR0_AVX    (1ULL <<  2)
-#define X86_XCR0_BNDREG (1ULL <<  3)
-#define X86_XCR0_BNDCSR (1ULL <<  4)
-#define X86_XCR0_OPMASK (1ULL <<  5)
-#define X86_XCR0_ZMM    (1ULL <<  6)
-#define X86_XCR0_HI_ZMM (1ULL <<  7)
-#define X86_XCR0_PKRU   (1ULL <<  9)
-#define X86_XCR0_LWP    (1ULL << 62)
-
-#define X86_XSS_MASK    (0) /* No XSS states supported yet. */
-
-/* Per-component subleaf flags. */
-#define XSTATE_XSS      (1ULL <<  0)
-#define XSTATE_ALIGN64  (1ULL <<  1)
-
-/* Configure extended state enumeration leaves (0x0000000D for xsave) */
-static void xc_cpuid_config_xsave(xc_interface *xch,
-                                  const struct cpuid_domain_info *info,
-                                  const unsigned int *input, unsigned int *regs)
-{
-    uint64_t guest_xfeature_mask;
-
-    if ( info->xfeature_mask == 0 ||
-         !test_bit(X86_FEATURE_XSAVE, info->featureset) )
-    {
-        regs[0] = regs[1] = regs[2] = regs[3] = 0;
-        return;
-    }
-
-    guest_xfeature_mask = X86_XCR0_SSE | X86_XCR0_X87;
-
-    if ( test_bit(X86_FEATURE_AVX, info->featureset) )
-        guest_xfeature_mask |= X86_XCR0_AVX;
-
-    if ( test_bit(X86_FEATURE_MPX, info->featureset) )
-        guest_xfeature_mask |= X86_XCR0_BNDREG | X86_XCR0_BNDCSR;
-
-    if ( test_bit(X86_FEATURE_AVX512F, info->featureset) )
-        guest_xfeature_mask |= X86_XCR0_OPMASK | X86_XCR0_ZMM | X86_XCR0_HI_ZMM;
-
-    if ( test_bit(X86_FEATURE_PKU, info->featureset) )
-        guest_xfeature_mask |= X86_XCR0_PKRU;
-
-    if ( test_bit(X86_FEATURE_LWP, info->featureset) )
-        guest_xfeature_mask |= X86_XCR0_LWP;
-
-    /*
-     * In the common case, the toolstack will have queried Xen for the maximum
-     * available featureset, and guest_xfeature_mask should not able to be
-     * calculated as being greater than the host limit, info->xfeature_mask.
-     *
-     * Nothing currently prevents a toolstack (or an optimistic user) from
-     * purposefully trying to select a larger-than-available xstate set.
-     *
-     * To avoid the domain dying with an unexpected fault, clamp the
-     * calculated mask to the host limit.  Future development work will remove
-     * this possibility, when Xen fully audits the complete cpuid polcy set
-     * for a domain.
-     */
-    guest_xfeature_mask &= info->xfeature_mask;
-
-    switch ( input[1] )
-    {
-    case 0:
-        /* EAX: low 32bits of xfeature_enabled_mask */
-        regs[0] = (uint32_t)guest_xfeature_mask;
-        /* EDX: high 32bits of xfeature_enabled_mask */
-        regs[3] = guest_xfeature_mask >> 32;
-        /* ECX: max size required by all HW features */
-        {
-            unsigned int _input[2] = {0xd, 0x0}, _regs[4];
-            regs[2] = 0;
-            for ( _input[1] = 2; _input[1] <= 62; _input[1]++ )
-            {
-                cpuid(_input, _regs);
-                if ( (_regs[0] + _regs[1]) > regs[2] )
-                    regs[2] = _regs[0] + _regs[1];
-            }
-        }
-        /* EBX: max size required by enabled features.
-         * This register contains a dynamic value, which varies when a guest
-         * enables or disables XSTATE features (via xsetbv). The default size
-         * after reset is 576. */
-        regs[1] = 512 + 64; /* FP/SSE + XSAVE.HEADER */
-        break;
-
-    case 1: /* leaf 1 */
-        regs[0] = info->featureset[featureword_of(X86_FEATURE_XSAVEOPT)];
-        regs[1] = 0;
-
-        if ( test_bit(X86_FEATURE_XSAVES, info->featureset) )
-        {
-            regs[2] = (uint32_t)(guest_xfeature_mask & X86_XSS_MASK);
-            regs[3] = (guest_xfeature_mask & X86_XSS_MASK) >> 32;
-        }
-        else
-            regs[2] = regs[3] = 0;
-        break;
-
-    case 2 ... 62: /* per-component sub-leaves */
-        if ( !(guest_xfeature_mask & (1ULL << input[1])) )
-        {
-            regs[0] = regs[1] = regs[2] = regs[3] = 0;
-            break;
-        }
-        /* Don't touch EAX, EBX. Also cleanup ECX and EDX */
-        regs[2] &= XSTATE_XSS | XSTATE_ALIGN64;
-        regs[3] = 0;
-        break;
-
-    default:
-        regs[0] = regs[1] = regs[2] = regs[3] = 0;
-        break;
-    }
-}
-
 static void xc_cpuid_hvm_policy(xc_interface *xch,
                                 const struct cpuid_domain_info *info,
                                 const unsigned int *input, unsigned int *regs)
@@ -558,8 +439,12 @@ static void xc_cpuid_hvm_policy(xc_interface *xch,
         regs[0] = 0;
         break;
 
-    case 0x0000000d:
-        xc_cpuid_config_xsave(xch, info, input, regs);
+    case 0x0000000d: /* Xen automatically calculates almost everything. */
+        if ( input[1] == 1 )
+            regs[0] = info->featureset[featureword_of(X86_FEATURE_XSAVEOPT)];
+        else
+            regs[0] = 0;
+        regs[1] = regs[2] = regs[3] = 0;
         break;
 
     case 0x80000000:
@@ -656,8 +541,12 @@ static void xc_cpuid_pv_policy(xc_interface *xch,
         regs[0] = 0;
         break;
 
-    case 0x0000000d:
-        xc_cpuid_config_xsave(xch, info, input, regs);
+    case 0x0000000d: /* Xen automatically calculates almost everything. */
+        if ( input[1] == 1 )
+            regs[0] = info->featureset[featureword_of(X86_FEATURE_XSAVEOPT)];
+        else
+            regs[0] = 0;
+        regs[1] = regs[2] = regs[3] = 0;
         break;
 
     case 0x80000000:
@@ -891,17 +780,15 @@ int xc_cpuid_apply_policy(xc_interface *xch, domid_t domid,
                 continue;
         }
 
-        /* XSAVE information, subleaves 0-63. */
-        if ( (input[0] == 0xd) && (input[1]++ < 63) )
-            continue;
-
         input[0]++;
         if ( !(input[0] & 0x80000000u) && (input[0] > base_max ) )
             input[0] = 0x80000000u;
 
         input[1] = XEN_CPUID_INPUT_UNUSED;
-        if ( (input[0] == 4) || (input[0] == 7) || (input[0] == 0xd) )
+        if ( (input[0] == 4) || (input[0] == 7) )
             input[1] = 0;
+        else if ( input[0] == 0xd )
+            input[1] = 1; /* Xen automatically calculates almost everything. */
 
         if ( (input[0] & 0x80000000u) && (input[0] > ext_max) )
             break;
-- 
2.1.4


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

[PATCH 5/6] x86/cpuid: Don't offer HVM hypervisor leaves to PV guests

[Andrew Cooper]

Xen leaf 4 is HVM-only.  Report a lower max_leaf to PV guests, so they don't
go and investigate a leaf which will return all zeros.

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

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 4f29c3a..51bcd97 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -911,12 +911,16 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
     uint32_t idx  = leaf - base;
     unsigned int limit = is_viridian_domain(d) ? p->hv2_limit : p->hv_limit;
 
+    /* Current, HVM max leaf is 4 and PV max leaf is 3. */
+    unsigned int max_leaf = has_hvm_container_domain(d) ? 4 : 3;
+    BUILD_BUG_ON(XEN_CPUID_MAX_NUM_LEAVES != 4);
+
     if ( limit == 0 )
         /* Default number of leaves */
-        limit = XEN_CPUID_MAX_NUM_LEAVES;
+        limit = max_leaf;
     else
-        /* Clamp toolstack value between 2 and MAX_NUM_LEAVES. */
-        limit = min(max(limit, 2u), XEN_CPUID_MAX_NUM_LEAVES + 0u);
+        /* Clamp toolstack value between 2 and max_leaf. */
+        limit = min(max(limit, 2u), max_leaf);
 
     if ( idx > limit )
         return;
-- 
2.1.4


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

[PATCH 6/6] x86/cpuid: Offer ITSC to domains which are automatically non-migrateable

[Andrew Cooper]

Dom0 doesn't have a toolstack to explicitly decide that ITSC is safe to offer.
For domains which are constructed with disable_migrate set, offer ITSC
automatically.

This is important for HVM-based dom0, and for when cpuid faulting is imposed
on the control domain.

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

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 9f16502..bf1eed6 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -440,6 +440,9 @@ int init_domain_cpuid_policy(struct domain *d)
 
     *d->arch.cpuid = is_pv_domain(d) ? pv_max_policy : hvm_max_policy;
 
+    if ( d->disable_migrate )
+        d->arch.cpuid->extd.itsc = cpu_has_itsc;
+
     recalculate_cpuid_policy(d);
 
     for ( i = 0; i < MAX_CPUID_INPUT; i++ )
-- 
2.1.4


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

Re: [PATCH 4/6] tools/libxc: Remove xsave calculations from libxc

[Wei Liu]

On Mon, Jan 16, 2017 at 11:40:28AM +0000, Andrew Cooper wrote:
> libxc performs a lot of calculations for the xstate leaf when generating a
> guests cpuid policy.  To correctly construct a policy for an HVM guest, this
> logic depends on native cpuid leaking through from real hardware.
> 
> In particular, the logic is potentially wrong for an HVM-based toolstack
> domain (e.g. PVH dom0), and definitely wrong if cpuid faulting is applied to a
> PV domain.
> 
> Xen now performs all the necessary calculations, using native values.  The
> only piece of information the toolstack need worry about is single xstate
> feature leaf.
> 
> 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>

> ---
>  tools/libxc/xc_cpuid_x86.c | 143 +++++----------------------------------------
>  1 file changed, 15 insertions(+), 128 deletions(-)
> 

I like this diffstat.

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

Re: [PATCH 1/6] x86/xstate: Fix array overrun on hardware with LWP

[Jan Beulich]

>>> On 16.01.17 at 12:40, <andrew.cooper3@citrix.com> wrote:
> c/s da62246e4c "x86/xsaves: enable xsaves/xrstors/xsavec in xen" introduced
> setup_xstate_features() to allocate and fill xstate_offsets[] and
> xstate_sizes[].
> 
> However, fls() casts xfeature_mask to 32bits which truncates LWP out of the
> calculation.  As a result, the arrays are allocated too short, and the cpuid
> infrastructure reads off the end of them when calculating xstate_size for the
> guest.
> 
> On one test system, this results in 0x3fec83c0 being returned as the maximum
> size of an xsave area, which surprisingly appears not to bother Windows or
> Linux too much.  I suspect they both use current size based on xcr0, which Xen
> forwards from real hardware.
> 
> 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: Introduce recalculate_xstate()

[Jan Beulich]

>>> On 16.01.17 at 12:40, <andrew.cooper3@citrix.com> wrote:
> All data in the xstate union, other than the Da1 feature word, is derived from
> other state; either feature bits from other words, or layout information which
> has already been collected by Xen's xstate driver.
> 
> Recalculate the xstate information for each policy object when the feature
> bits may have changed.
> 
> The XSTATE_XSAVES_ONLY define needs extending to a 64bit value to avoid
> problems when taking its converse for masking purposes.

I don't understand this part - plain 0 is a signed quantity, so ~0 would
be sign extended to 64 bits as needed.

> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -80,6 +80,103 @@ static void sanitise_featureset(uint32_t *fs)
>                            (fs[FEATURESET_e1d] & ~CPUID_COMMON_1D_FEATURES));
>  }
>  
> +static void recalculate_xstate(struct cpuid_policy *p)
> +{
> +    uint64_t xstates = XSTATE_FP_SSE;
> +    uint32_t xstate_size = XSTATE_AREA_MIN_SIZE;
> +    unsigned int i, Da1 = p->xstate.Da1;
> +
> +    /*
> +     * The Da1 leaf is the only piece if information preserved.  Everything
> +     * else is derived from other feature state.
> +     */

Almost.

> +    memset(&p->xstate, 0, sizeof(p->xstate));
> +
> +    if ( !p->basic.xsave )
> +        return;

You're clobbering it here (but in all reality it should be zero in this
case).

> @@ -154,6 +152,13 @@ struct cpuid_policy
>              };
>              uint32_t /* b */:32, xss_low, xss_high;
>          };
> +
> +        /* Per-component common state.  Valid for i >= 2. */
> +        struct {
> +            uint32_t size, offset;
> +            bool xss:1, align:1;
> +            uint32_t /* c */:30, /* d */:32;
> +        } comp[CPUID_GUEST_NR_XSTATE];

Hmm, can we rely on this functioning on varying complier variants?
I think the standard doesn't exclude a uint32_t type bitfield to
start on a 4-byte boundary if not following another uint32_t one.
IOW I think we'd be better off giving the same type to all fields we
want to share a storage unit.

Jan


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

Re: [PATCH 3/6] x86/cpuid: Move all xstate leaf handling into guest_cpuid()

[Jan Beulich]

>>> On 16.01.17 at 12:40, <andrew.cooper3@citrix.com> wrote:
> @@ -1007,10 +864,13 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
>              break;
>  
>          case XSTATE_CPUID:
> -            if ( subleaf > ARRAY_SIZE(p->xstate.raw) )
> +            if ( !p->basic.xsave || subleaf >= ARRAY_SIZE(p->xstate.raw) )
>                  return;
>  
> -            /* Fallthrough. */
> +            BUG_ON(subleaf >= ARRAY_SIZE(p->xstate.raw));

Kind of pointless considering the if() right above? With this removed
(or the reason for it clarified)
Reviewed-by: Jan Beulich <jbeulich@suse.com>

> @@ -1067,6 +927,31 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
>              break;
>          }
>          break;
> +
> +    case XSTATE_CPUID:
> +        switch ( subleaf )
> +        {
> +        case 1:
> +            if ( p->xstate.xsaves )
> +            {
> +                /*
> +                 * TODO: Figure out what to do for XSS state.  VT-x manages
> +                 * host vs guest MSR_XSS automatically, so as soon as we start
> +                 * supporting any XSS states, the wrong XSS will be in
> +                 * context.
> +                 */
> +                BUILD_BUG_ON(XSTATE_XSAVES_ONLY != 0);

Yeah, I guess we won't have many options other than switching
XSS around for the CPUID invocation.

Jan


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

Re: [PATCH 5/6] x86/cpuid: Don't offer HVM hypervisor leaves to PV guests

[Jan Beulich]

>>> On 16.01.17 at 12:40, <andrew.cooper3@citrix.com> wrote:
> Xen leaf 4 is HVM-only.  Report a lower max_leaf to PV guests, so they don't
> go and investigate a leaf which will return all zeros.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Well, if you think it's worth it (we'll have to make this leaf visible
again anyway as soon as leaf 5 appears):
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: Introduce recalculate_xstate()

[Andrew Cooper]

On 16/01/17 16:45, Jan Beulich wrote:
>>>> On 16.01.17 at 12:40, <andrew.cooper3@citrix.com> wrote:
>> All data in the xstate union, other than the Da1 feature word, is derived from
>> other state; either feature bits from other words, or layout information which
>> has already been collected by Xen's xstate driver.
>>
>> Recalculate the xstate information for each policy object when the feature
>> bits may have changed.
>>
>> The XSTATE_XSAVES_ONLY define needs extending to a 64bit value to avoid
>> problems when taking its converse for masking purposes.
> I don't understand this part - plain 0 is a signed quantity, so ~0 would
> be sign extended to 64 bits as needed.

Hmm, now you point this out, I can't see how I came to this conclusion
to start with.  I will drop it.

>
>> --- a/xen/arch/x86/cpuid.c
>> +++ b/xen/arch/x86/cpuid.c
>> @@ -80,6 +80,103 @@ static void sanitise_featureset(uint32_t *fs)
>>                            (fs[FEATURESET_e1d] & ~CPUID_COMMON_1D_FEATURES));
>>  }
>>  
>> +static void recalculate_xstate(struct cpuid_policy *p)
>> +{
>> +    uint64_t xstates = XSTATE_FP_SSE;
>> +    uint32_t xstate_size = XSTATE_AREA_MIN_SIZE;
>> +    unsigned int i, Da1 = p->xstate.Da1;
>> +
>> +    /*
>> +     * The Da1 leaf is the only piece if information preserved.  Everything
>> +     * else is derived from other feature state.
>> +     */
> Almost.
>
>> +    memset(&p->xstate, 0, sizeof(p->xstate));
>> +
>> +    if ( !p->basic.xsave )
>> +        return;
> You're clobbering it here (but in all reality it should be zero in this
> case).

sanitise_featureset() should have made it all zero in the absence of xsave.

>
>> @@ -154,6 +152,13 @@ struct cpuid_policy
>>              };
>>              uint32_t /* b */:32, xss_low, xss_high;
>>          };
>> +
>> +        /* Per-component common state.  Valid for i >= 2. */
>> +        struct {
>> +            uint32_t size, offset;
>> +            bool xss:1, align:1;
>> +            uint32_t /* c */:30, /* d */:32;
>> +        } comp[CPUID_GUEST_NR_XSTATE];
> Hmm, can we rely on this functioning on varying complier variants?
> I think the standard doesn't exclude a uint32_t type bitfield to
> start on a 4-byte boundary if not following another uint32_t one.
> IOW I think we'd be better off giving the same type to all fields we
> want to share a storage unit.

Hmm.  In this case, something like:

bool xss:1, align:1;
uint32_t _res_d;

ought to work.

~Andrew

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

Re: [PATCH 6/6] x86/cpuid: Offer ITSC to domains which are automatically non-migrateable

[Jan Beulich]

>>> On 16.01.17 at 12:40, <andrew.cooper3@citrix.com> wrote:
> Dom0 doesn't have a toolstack to explicitly decide that ITSC is safe to offer.
> For domains which are constructed with disable_migrate set, offer ITSC
> automatically.

I'm afraid "constructed" is ambiguous here: To me, construction means
more than just domain_create(), i.e. includes all of the domctls the
tool stack issues until unpausing the domain the first time. Yet
XEN_DOMCTL_disable_migrate being among those isn't being covered
here. As to alternative wording, I would have wanted to suggest
"created", but I'm not convinced this is much less ambiguous.

> This is important for HVM-based dom0, and for when cpuid faulting is imposed
> on the control domain.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

The change itself is certainly fine:
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 3/6] x86/cpuid: Move all xstate leaf handling into guest_cpuid()

[Andrew Cooper]

On 16/01/17 16:58, Jan Beulich wrote:
>>>> On 16.01.17 at 12:40, <andrew.cooper3@citrix.com> wrote:
>> @@ -1007,10 +864,13 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
>>              break;
>>  
>>          case XSTATE_CPUID:
>> -            if ( subleaf > ARRAY_SIZE(p->xstate.raw) )
>> +            if ( !p->basic.xsave || subleaf >= ARRAY_SIZE(p->xstate.raw) )
>>                  return;
>>  
>> -            /* Fallthrough. */
>> +            BUG_ON(subleaf >= ARRAY_SIZE(p->xstate.raw));
> Kind of pointless considering the if() right above? With this removed
> (or the reason for it clarified)
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

It was for consistency with the other raw[] accesses, but given this
subleaf calculation, I can drop it.

>
>> @@ -1067,6 +927,31 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
>>              break;
>>          }
>>          break;
>> +
>> +    case XSTATE_CPUID:
>> +        switch ( subleaf )
>> +        {
>> +        case 1:
>> +            if ( p->xstate.xsaves )
>> +            {
>> +                /*
>> +                 * TODO: Figure out what to do for XSS state.  VT-x manages
>> +                 * host vs guest MSR_XSS automatically, so as soon as we start
>> +                 * supporting any XSS states, the wrong XSS will be in
>> +                 * context.
>> +                 */
>> +                BUILD_BUG_ON(XSTATE_XSAVES_ONLY != 0);
> Yeah, I guess we won't have many options other than switching
> XSS around for the CPUID invocation.

The other option is to have a function which takes xcr0|xss and performs
some calculations with xstate_{offsets[],sizes[],align}, which might
plausibly be faster than switching MSR_XSS.  Either way, this isn't a
problem I want to solve until I have a real piece of hardware using XSS
states to develop against.

~Andrew

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

Re: [PATCH 2/6] x86/cpuid: Introduce recalculate_xstate()

[Jan Beulich]

>>> On 16.01.17 at 18:02, <andrew.cooper3@citrix.com> wrote:
> On 16/01/17 16:45, Jan Beulich wrote:
>>>>> On 16.01.17 at 12:40, <andrew.cooper3@citrix.com> wrote:
>>> @@ -154,6 +152,13 @@ struct cpuid_policy
>>>              };
>>>              uint32_t /* b */:32, xss_low, xss_high;
>>>          };
>>> +
>>> +        /* Per-component common state.  Valid for i >= 2. */
>>> +        struct {
>>> +            uint32_t size, offset;
>>> +            bool xss:1, align:1;
>>> +            uint32_t /* c */:30, /* d */:32;
>>> +        } comp[CPUID_GUEST_NR_XSTATE];
>> Hmm, can we rely on this functioning on varying complier variants?
>> I think the standard doesn't exclude a uint32_t type bitfield to
>> start on a 4-byte boundary if not following another uint32_t one.
>> IOW I think we'd be better off giving the same type to all fields we
>> want to share a storage unit.
> 
> Hmm.  In this case, something like:
> 
> bool xss:1, align:1;
> uint32_t _res_d;
> 
> ought to work.

In a union you mean? Yes.

Jan


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

Re: [PATCH 6/6] x86/cpuid: Offer ITSC to domains which are automatically non-migrateable

[Andrew Cooper]

On 16/01/17 17:07, Jan Beulich wrote:
>>>> On 16.01.17 at 12:40, <andrew.cooper3@citrix.com> wrote:
>> Dom0 doesn't have a toolstack to explicitly decide that ITSC is safe to offer.
>> For domains which are constructed with disable_migrate set, offer ITSC
>> automatically.
> I'm afraid "constructed" is ambiguous here: To me, construction means
> more than just domain_create(), i.e. includes all of the domctls the
> tool stack issues until unpausing the domain the first time. Yet
> XEN_DOMCTL_disable_migrate being among those isn't being covered
> here. As to alternative wording, I would have wanted to suggest
> "created", but I'm not convinced this is much less ambiguous.

"For domains which are automatically built with disable_migrate set" ?

It is realistically only dom0 this is a problem for, despite this check
also applying to xenstore stubdomains.  A Xenstore stubdomain is
properly built by the toolstack, and should have ITSC set via the usual
policy mechanisms.

~Andrew

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

Re: [PATCH 6/6] x86/cpuid: Offer ITSC to domains which are automatically non-migrateable

[Jan Beulich]

>>> On 16.01.17 at 18:26, <andrew.cooper3@citrix.com> wrote:
> On 16/01/17 17:07, Jan Beulich wrote:
>>>>> On 16.01.17 at 12:40, <andrew.cooper3@citrix.com> wrote:
>>> Dom0 doesn't have a toolstack to explicitly decide that ITSC is safe to offer.
>>> For domains which are constructed with disable_migrate set, offer ITSC
>>> automatically.
>> I'm afraid "constructed" is ambiguous here: To me, construction means
>> more than just domain_create(), i.e. includes all of the domctls the
>> tool stack issues until unpausing the domain the first time. Yet
>> XEN_DOMCTL_disable_migrate being among those isn't being covered
>> here. As to alternative wording, I would have wanted to suggest
>> "created", but I'm not convinced this is much less ambiguous.
> 
> "For domains which are automatically built with disable_migrate set" ?

Well, yes, this should be clear enough.

Jan


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

Re: [PATCH 5/6] x86/cpuid: Don't offer HVM hypervisor leaves to PV guests

[Andrew Cooper]

On 16/01/17 17:02, Jan Beulich wrote:
>>>> On 16.01.17 at 12:40, <andrew.cooper3@citrix.com> wrote:
>> Xen leaf 4 is HVM-only.  Report a lower max_leaf to PV guests, so they don't
>> go and investigate a leaf which will return all zeros.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Well, if you think it's worth it (we'll have to make this leaf visible
> again anyway as soon as leaf 5 appears):
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>

Hmm - you make a good point.  I suppose there is no harm, and leaving
the code alone is more simple.  I will drop this change.

~Andrew

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

[PATCH v2 2/6] x86/cpuid: Introduce recalculate_xstate()

[Andrew Cooper]

All data in the xstate union, other than the Da1 feature word, is derived from
other state; either feature bits from other words, or layout information which
has already been collected by Xen's xstate driver.

Recalculate the xstate information for each policy object when the feature
bits may have changed.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>

v2:
 * Drop adjustment to XSTATE_XSAVES_ONLY.
 * Tweak wording concerning the preservation of Da1.
 * Alter the comp union to avoid mixing types of bitfields.
---
 xen/arch/x86/cpuid.c        | 101 ++++++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/cpuid.h |  17 +++++---
 2 files changed, 112 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index bcdac03..eae07cf 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -80,6 +80,103 @@ static void sanitise_featureset(uint32_t *fs)
                           (fs[FEATURESET_e1d] & ~CPUID_COMMON_1D_FEATURES));
 }
 
+static void recalculate_xstate(struct cpuid_policy *p)
+{
+    uint64_t xstates = XSTATE_FP_SSE;
+    uint32_t xstate_size = XSTATE_AREA_MIN_SIZE;
+    unsigned int i, Da1 = p->xstate.Da1;
+
+    /*
+     * The Da1 leaf is the only piece if information preserved in the common
+     * case.  Everything else is derived from other feature state.
+     */
+    memset(&p->xstate, 0, sizeof(p->xstate));
+
+    if ( !p->basic.xsave )
+        return;
+
+    if ( p->basic.avx )
+    {
+        xstates |= XSTATE_YMM;
+        xstate_size = max(xstate_size,
+                          xstate_offsets[_XSTATE_YMM] +
+                          xstate_sizes[_XSTATE_YMM]);
+    }
+
+    if ( p->feat.mpx )
+    {
+        xstates |= XSTATE_BNDREGS | XSTATE_BNDCSR;
+        xstate_size = max(xstate_size,
+                          xstate_offsets[_XSTATE_BNDCSR] +
+                          xstate_sizes[_XSTATE_BNDCSR]);
+    }
+
+    if ( p->feat.avx512f )
+    {
+        xstates |= XSTATE_OPMASK | XSTATE_ZMM | XSTATE_HI_ZMM;
+        xstate_size = max(xstate_size,
+                          xstate_offsets[_XSTATE_HI_ZMM] +
+                          xstate_sizes[_XSTATE_HI_ZMM]);
+    }
+
+    if ( p->feat.pku )
+    {
+        xstates |= XSTATE_PKRU;
+        xstate_size = max(xstate_size,
+                          xstate_offsets[_XSTATE_PKRU] +
+                          xstate_sizes[_XSTATE_PKRU]);
+    }
+
+    if ( p->extd.lwp )
+    {
+        xstates |= XSTATE_LWP;
+        xstate_size = max(xstate_size,
+                          xstate_offsets[_XSTATE_LWP] +
+                          xstate_sizes[_XSTATE_LWP]);
+    }
+
+    /* Sanity check we aren't advertising unknown states. */
+    ASSERT((xstates & ~XCNTXT_MASK) == 0);
+
+    p->xstate.max_size  =  xstate_size;
+    p->xstate.xcr0_low  =  xstates & ~XSTATE_XSAVES_ONLY;
+    p->xstate.xcr0_high = (xstates & ~XSTATE_XSAVES_ONLY) >> 32;
+
+    p->xstate.Da1 = Da1;
+    if ( p->xstate.xsaves )
+    {
+        p->xstate.xss_low   =  xstates & XSTATE_XSAVES_ONLY;
+        p->xstate.xss_high  = (xstates & XSTATE_XSAVES_ONLY) >> 32;
+    }
+    else
+        xstates &= ~XSTATE_XSAVES_ONLY;
+
+    for ( i = 2; i < min(63ul, ARRAY_SIZE(p->xstate.comp)); ++i )
+    {
+        uint64_t curr_xstate = 1ul << i;
+
+        if ( !(xstates & curr_xstate) )
+            continue;
+
+        p->xstate.comp[i].size   = xstate_sizes[i];
+        p->xstate.comp[i].offset = xstate_offsets[i];
+        p->xstate.comp[i].xss    = curr_xstate & XSTATE_XSAVES_ONLY;
+        p->xstate.comp[i].align  = curr_xstate & xstate_align;
+
+        /*
+         * Sanity checks:
+         * - All valid components should have non-zero size.
+         * - All xcr0 components should have non-zero offset.
+         * - All xss components should report 0 offset.
+         */
+        ASSERT(xstate_sizes[i]);
+        if ( curr_xstate & XSTATE_XSAVES_ONLY )
+            ASSERT(xstate_offsets[i] == 0);
+        else
+            ASSERT(xstate_offsets[i]);
+    }
+}
+
 static void __init calculate_raw_policy(void)
 {
     struct cpuid_policy *p = &raw_policy;
@@ -149,6 +246,7 @@ static void __init calculate_host_policy(void)
               0x80000000u + ARRAY_SIZE(p->extd.raw) - 1);
 
     cpuid_featureset_to_policy(boot_cpu_data.x86_capability, p);
+    recalculate_xstate(p);
 }
 
 static void __init calculate_pv_max_policy(void)
@@ -168,6 +266,7 @@ static void __init calculate_pv_max_policy(void)
 
     sanitise_featureset(pv_featureset);
     cpuid_featureset_to_policy(pv_featureset, p);
+    recalculate_xstate(p);
 }
 
 static void __init calculate_hvm_max_policy(void)
@@ -221,6 +320,7 @@ static void __init calculate_hvm_max_policy(void)
 
     sanitise_featureset(hvm_featureset);
     cpuid_featureset_to_policy(hvm_featureset, p);
+    recalculate_xstate(p);
 }
 
 void __init init_guest_cpuid(void)
@@ -328,6 +428,7 @@ void recalculate_cpuid_policy(struct domain *d)
                            special_features[FEATURESET_7b0]);
 
     cpuid_featureset_to_policy(fs, p);
+    recalculate_xstate(p);
 }
 
 int init_domain_cpuid_policy(struct domain *d)
diff --git a/xen/include/asm-x86/cpuid.h b/xen/include/asm-x86/cpuid.h
index 24ad3e0..e5140ca 100644
--- a/xen/include/asm-x86/cpuid.h
+++ b/xen/include/asm-x86/cpuid.h
@@ -77,18 +77,15 @@ struct cpuid_policy
      *
      * Global *_policy objects:
      *
-     * - Host accurate:
-     *   - {xcr0,xss}_{high,low}
-     *
      * - Guest accurate:
-     *   - All of the feat union
+     *   - All of the feat and xstate unions
      *   - max_{,sub}leaf
      *   - All FEATURESET_* words
      *
      * Per-domain objects:
      *
      * - Guest accurate:
-     *   - All of the feat union
+     *   - All of the feat and xstate unions
      *   - max_{,sub}leaf
      *   - All FEATURESET_* words
      *
@@ -143,9 +140,10 @@ struct cpuid_policy
     /* Xstate feature leaf: 0x0000000D[xx] */
     union {
         struct cpuid_leaf raw[CPUID_GUEST_NR_XSTATE];
+
         struct {
             /* Subleaf 0. */
-            uint32_t xcr0_low, /* b */:32, /* c */:32, xcr0_high;
+            uint32_t xcr0_low, /* b */:32, max_size, xcr0_high;
 
             /* Subleaf 1. */
             union {
@@ -154,6 +152,13 @@ struct cpuid_policy
             };
             uint32_t /* b */:32, xss_low, xss_high;
         };
+
+        /* Per-component common state.  Valid for i >= 2. */
+        struct {
+            uint32_t size, offset;
+            bool xss:1, align:1;
+            uint32_t _res_d;
+        } comp[CPUID_GUEST_NR_XSTATE];
     } xstate;
 
     /* Extended leaves: 0x800000xx */
-- 
2.1.4


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

Re: [PATCH v2 2/6] x86/cpuid: Introduce recalculate_xstate()

[Jan Beulich]

>>> On 17.01.17 at 12:27, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -80,6 +80,103 @@ static void sanitise_featureset(uint32_t *fs)
>                            (fs[FEATURESET_e1d] & ~CPUID_COMMON_1D_FEATURES));
>  }
>  
> +static void recalculate_xstate(struct cpuid_policy *p)
> +{
> +    uint64_t xstates = XSTATE_FP_SSE;
> +    uint32_t xstate_size = XSTATE_AREA_MIN_SIZE;
> +    unsigned int i, Da1 = p->xstate.Da1;
> +
> +    /*
> +     * The Da1 leaf is the only piece if information preserved in the common
> +     * case.  Everything else is derived from other feature state.
> +     */

"piece of" I think.

> +    memset(&p->xstate, 0, sizeof(p->xstate));
> +
> +    if ( !p->basic.xsave )
> +        return;
> +
> +    if ( p->basic.avx )
> +    {
> +        xstates |= XSTATE_YMM;
> +        xstate_size = max(xstate_size,
> +                          xstate_offsets[_XSTATE_YMM] +
> +                          xstate_sizes[_XSTATE_YMM]);
> +    }
> +
> +    if ( p->feat.mpx )
> +    {
> +        xstates |= XSTATE_BNDREGS | XSTATE_BNDCSR;
> +        xstate_size = max(xstate_size,
> +                          xstate_offsets[_XSTATE_BNDCSR] +
> +                          xstate_sizes[_XSTATE_BNDCSR]);
> +    }
> +
> +    if ( p->feat.avx512f )
> +    {
> +        xstates |= XSTATE_OPMASK | XSTATE_ZMM | XSTATE_HI_ZMM;
> +        xstate_size = max(xstate_size,
> +                          xstate_offsets[_XSTATE_HI_ZMM] +
> +                          xstate_sizes[_XSTATE_HI_ZMM]);
> +    }
> +
> +    if ( p->feat.pku )
> +    {
> +        xstates |= XSTATE_PKRU;
> +        xstate_size = max(xstate_size,
> +                          xstate_offsets[_XSTATE_PKRU] +
> +                          xstate_sizes[_XSTATE_PKRU]);
> +    }
> +
> +    if ( p->extd.lwp )
> +    {
> +        xstates |= XSTATE_LWP;
> +        xstate_size = max(xstate_size,
> +                          xstate_offsets[_XSTATE_LWP] +
> +                          xstate_sizes[_XSTATE_LWP]);
> +    }
> +
> +    /* Sanity check we aren't advertising unknown states. */
> +    ASSERT((xstates & ~XCNTXT_MASK) == 0);
> +
> +    p->xstate.max_size  =  xstate_size;
> +    p->xstate.xcr0_low  =  xstates & ~XSTATE_XSAVES_ONLY;
> +    p->xstate.xcr0_high = (xstates & ~XSTATE_XSAVES_ONLY) >> 32;
> +
> +    p->xstate.Da1 = Da1;
> +    if ( p->xstate.xsaves )
> +    {
> +        p->xstate.xss_low   =  xstates & XSTATE_XSAVES_ONLY;
> +        p->xstate.xss_high  = (xstates & XSTATE_XSAVES_ONLY) >> 32;
> +    }
> +    else
> +        xstates &= ~XSTATE_XSAVES_ONLY;
> +
> +    for ( i = 2; i < min(63ul, ARRAY_SIZE(p->xstate.comp)); ++i )
> +    {
> +        uint64_t curr_xstate = 1ul << i;
> +
> +        if ( !(xstates & curr_xstate) )
> +            continue;
> +
> +        p->xstate.comp[i].size   = xstate_sizes[i];
> +        p->xstate.comp[i].offset = xstate_offsets[i];
> +        p->xstate.comp[i].xss    = curr_xstate & XSTATE_XSAVES_ONLY;
> +        p->xstate.comp[i].align  = curr_xstate & xstate_align;
> +
> +        /*
> +         * Sanity checks:
> +         * - All valid components should have non-zero size.
> +         * - All xcr0 components should have non-zero offset.
> +         * - All xss components should report 0 offset.
> +         */
> +        ASSERT(xstate_sizes[i]);
> +        if ( curr_xstate & XSTATE_XSAVES_ONLY )
> +            ASSERT(xstate_offsets[i] == 0);
> +        else
> +            ASSERT(xstate_offsets[i]);
> +    }

Hmm, now that I look at this again - what business do these
assertions have here? They're checking host information, which
isn't going to change post boot. Such checking, if indeed wanted,
should be done once during system boot.

I also think such checks should be consistent in style - either both
explicitly comparing with zero, or using ! in the if() branch to match
the else one.

> @@ -154,6 +152,13 @@ struct cpuid_policy
>              };
>              uint32_t /* b */:32, xss_low, xss_high;
>          };
> +
> +        /* Per-component common state.  Valid for i >= 2. */
> +        struct {
> +            uint32_t size, offset;
> +            bool xss:1, align:1;
> +            uint32_t _res_d;

I see you've decided against an inner union. Should be fine of
course, at least until we have a need to access the full ECX value
by name.

Jan

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

Re: [PATCH v2 2/6] x86/cpuid: Introduce recalculate_xstate()

[Andrew Cooper]

On 17/01/17 12:52, Jan Beulich wrote:
>>>> On 17.01.17 at 12:27, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/cpuid.c
>> +++ b/xen/arch/x86/cpuid.c
>> @@ -80,6 +80,103 @@ static void sanitise_featureset(uint32_t *fs)
>>                            (fs[FEATURESET_e1d] & ~CPUID_COMMON_1D_FEATURES));
>>  }
>>  
>> +static void recalculate_xstate(struct cpuid_policy *p)
>> +{
>> +    uint64_t xstates = XSTATE_FP_SSE;
>> +    uint32_t xstate_size = XSTATE_AREA_MIN_SIZE;
>> +    unsigned int i, Da1 = p->xstate.Da1;
>> +
>> +    /*
>> +     * The Da1 leaf is the only piece if information preserved in the common
>> +     * case.  Everything else is derived from other feature state.
>> +     */
> "piece of" I think.

Ah yes - will fix.

>
>> +    memset(&p->xstate, 0, sizeof(p->xstate));
>> +
>> +    if ( !p->basic.xsave )
>> +        return;
>> +
>> +    if ( p->basic.avx )
>> +    {
>> +        xstates |= XSTATE_YMM;
>> +        xstate_size = max(xstate_size,
>> +                          xstate_offsets[_XSTATE_YMM] +
>> +                          xstate_sizes[_XSTATE_YMM]);
>> +    }
>> +
>> +    if ( p->feat.mpx )
>> +    {
>> +        xstates |= XSTATE_BNDREGS | XSTATE_BNDCSR;
>> +        xstate_size = max(xstate_size,
>> +                          xstate_offsets[_XSTATE_BNDCSR] +
>> +                          xstate_sizes[_XSTATE_BNDCSR]);
>> +    }
>> +
>> +    if ( p->feat.avx512f )
>> +    {
>> +        xstates |= XSTATE_OPMASK | XSTATE_ZMM | XSTATE_HI_ZMM;
>> +        xstate_size = max(xstate_size,
>> +                          xstate_offsets[_XSTATE_HI_ZMM] +
>> +                          xstate_sizes[_XSTATE_HI_ZMM]);
>> +    }
>> +
>> +    if ( p->feat.pku )
>> +    {
>> +        xstates |= XSTATE_PKRU;
>> +        xstate_size = max(xstate_size,
>> +                          xstate_offsets[_XSTATE_PKRU] +
>> +                          xstate_sizes[_XSTATE_PKRU]);
>> +    }
>> +
>> +    if ( p->extd.lwp )
>> +    {
>> +        xstates |= XSTATE_LWP;
>> +        xstate_size = max(xstate_size,
>> +                          xstate_offsets[_XSTATE_LWP] +
>> +                          xstate_sizes[_XSTATE_LWP]);
>> +    }
>> +
>> +    /* Sanity check we aren't advertising unknown states. */
>> +    ASSERT((xstates & ~XCNTXT_MASK) == 0);
>> +
>> +    p->xstate.max_size  =  xstate_size;
>> +    p->xstate.xcr0_low  =  xstates & ~XSTATE_XSAVES_ONLY;
>> +    p->xstate.xcr0_high = (xstates & ~XSTATE_XSAVES_ONLY) >> 32;
>> +
>> +    p->xstate.Da1 = Da1;
>> +    if ( p->xstate.xsaves )
>> +    {
>> +        p->xstate.xss_low   =  xstates & XSTATE_XSAVES_ONLY;
>> +        p->xstate.xss_high  = (xstates & XSTATE_XSAVES_ONLY) >> 32;
>> +    }
>> +    else
>> +        xstates &= ~XSTATE_XSAVES_ONLY;
>> +
>> +    for ( i = 2; i < min(63ul, ARRAY_SIZE(p->xstate.comp)); ++i )
>> +    {
>> +        uint64_t curr_xstate = 1ul << i;
>> +
>> +        if ( !(xstates & curr_xstate) )
>> +            continue;
>> +
>> +        p->xstate.comp[i].size   = xstate_sizes[i];
>> +        p->xstate.comp[i].offset = xstate_offsets[i];
>> +        p->xstate.comp[i].xss    = curr_xstate & XSTATE_XSAVES_ONLY;
>> +        p->xstate.comp[i].align  = curr_xstate & xstate_align;
>> +
>> +        /*
>> +         * Sanity checks:
>> +         * - All valid components should have non-zero size.
>> +         * - All xcr0 components should have non-zero offset.
>> +         * - All xss components should report 0 offset.
>> +         */
>> +        ASSERT(xstate_sizes[i]);
>> +        if ( curr_xstate & XSTATE_XSAVES_ONLY )
>> +            ASSERT(xstate_offsets[i] == 0);
>> +        else
>> +            ASSERT(xstate_offsets[i]);
>> +    }
> Hmm, now that I look at this again - what business do these
> assertions have here? They're checking host information, which
> isn't going to change post boot. Such checking, if indeed wanted,
> should be done once during system boot.

I put this in to try and ensure that we don't put junk into the policy,
but thinking about it more, by the time do have junk in these arrays, we
have bigger xstate problems.  I will drop these from here and focus on
beefing up the xstate driver itself.

>
> I also think such checks should be consistent in style - either both
> explicitly comparing with zero, or using ! in the if() branch to match
> the else one.
>
>> @@ -154,6 +152,13 @@ struct cpuid_policy
>>              };
>>              uint32_t /* b */:32, xss_low, xss_high;
>>          };
>> +
>> +        /* Per-component common state.  Valid for i >= 2. */
>> +        struct {
>> +            uint32_t size, offset;
>> +            bool xss:1, align:1;
>> +            uint32_t _res_d;
> I see you've decided against an inner union. Should be fine of
> course, at least until we have a need to access the full ECX value
> by name.

Oh - I misinterpreted what you meant then.

Did you mean

struct {
    uint32_t size, offset;
    union {
        struct {
            bool xss:1, align:1;
        };
        uint32_t c;
    };
    uint32_t /* d */:32;
};

Then?

~Andrew

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

Re: [PATCH v2 2/6] x86/cpuid: Introduce recalculate_xstate()

[Jan Beulich]

>>> On 17.01.17 at 16:15, <andrew.cooper3@citrix.com> wrote:
> On 17/01/17 12:52, Jan Beulich wrote:
>>>>> On 17.01.17 at 12:27, <andrew.cooper3@citrix.com> wrote:
>>> @@ -154,6 +152,13 @@ struct cpuid_policy
>>>              };
>>>              uint32_t /* b */:32, xss_low, xss_high;
>>>          };
>>> +
>>> +        /* Per-component common state.  Valid for i >= 2. */
>>> +        struct {
>>> +            uint32_t size, offset;
>>> +            bool xss:1, align:1;
>>> +            uint32_t _res_d;
>> I see you've decided against an inner union. Should be fine of
>> course, at least until we have a need to access the full ECX value
>> by name.
> 
> Oh - I misinterpreted what you meant then.
> 
> Did you mean
> 
> struct {
>     uint32_t size, offset;
>     union {
>         struct {
>             bool xss:1, align:1;
>         };
>         uint32_t c;
>     };
>     uint32_t /* d */:32;
> };
> 
> Then?

Yes. But in the end it's up to you which variant to use.

Jan


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

Re: [PATCH v2 2/6] x86/cpuid: Introduce recalculate_xstate()

[Andrew Cooper]

On 17/01/17 15:28, Jan Beulich wrote:
>>>> On 17.01.17 at 16:15, <andrew.cooper3@citrix.com> wrote:
>> On 17/01/17 12:52, Jan Beulich wrote:
>>>>>> On 17.01.17 at 12:27, <andrew.cooper3@citrix.com> wrote:
>>>> @@ -154,6 +152,13 @@ struct cpuid_policy
>>>>              };
>>>>              uint32_t /* b */:32, xss_low, xss_high;
>>>>          };
>>>> +
>>>> +        /* Per-component common state.  Valid for i >= 2. */
>>>> +        struct {
>>>> +            uint32_t size, offset;
>>>> +            bool xss:1, align:1;
>>>> +            uint32_t _res_d;
>>> I see you've decided against an inner union. Should be fine of
>>> course, at least until we have a need to access the full ECX value
>>> by name.
>> Oh - I misinterpreted what you meant then.
>>
>> Did you mean
>>
>> struct {
>>     uint32_t size, offset;
>>     union {
>>         struct {
>>             bool xss:1, align:1;
>>         };
>>         uint32_t c;
>>     };
>>     uint32_t /* d */:32;
>> };
>>
>> Then?
> Yes. But in the end it's up to you which variant to use.

We only write via the xss/align names, and read through raw[].

For now, lets go with mine which is a simpler structure.  We can always
change it if we need to.

~Andrew

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