[Xen-devel] [PATCH 00/10] x86: Default vs Max policies

[Xen-devel] [PATCH 00/10] x86: Default vs Max policies

[Andrew Cooper]

This series builds on several years worth of building blocks to finally create
a real distinction between default and max policies.

See the final patch for a concrete example.

Everything but the final patch is ready to go in now.  The final patch depends
on the still-in-review migration series, to provide suitable backwards
compatilbity for VMs coming from older versions of Xen.

Andrew Cooper (10):
  x86/sysctl: Don't return cpu policy data for compiled-out support (2)
  tools/libxc: Simplify xc_get_static_cpu_featuremask()
  x86/gen-cpuid: Rework internal logic to ease future changes
  x86/gen-cpuid: Create max and default variations of INIT_*_FEATURES
  x86/msr: Compile out unused logic/objects
  x86/msr: Introduce and use default MSR policies
  x86/cpuid: Compile out unused logic/objects
  x86/cpuid: Introduce and use default CPUID policies
  x86/gen-cpuid: Distinguish default vs max in feature annotations
  x86/hvm: Do not enable MPX by default

 tools/libxc/include/xenctrl.h               |  10 ++-
 tools/libxc/xc_cpuid_x86.c                  |  55 +++++--------
 tools/misc/xen-cpuid.c                      |  35 ++++++---
 xen/arch/x86/cpuid.c                        | 118 ++++++++++++++++++++++------
 xen/arch/x86/msr.c                          |  62 +++++++++++----
 xen/arch/x86/sysctl.c                       |  25 ++++--
 xen/include/asm-x86/cpuid.h                 |   3 +-
 xen/include/asm-x86/msr.h                   |   4 +-
 xen/include/public/arch-x86/cpufeatureset.h |   4 +-
 xen/include/public/sysctl.h                 |   2 +
 xen/tools/gen-cpuid.py                      | 100 ++++++++++++-----------
 11 files changed, 268 insertions(+), 150 deletions(-)

-- 
2.11.0


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

[Xen-devel] [PATCH 01/10] x86/sysctl: Don't return cpu policy data for compiled-out support (2)

[Andrew Cooper]

Just as with c/s 96dc77b4b1 for XEN_SYSCTL_get_cpu_policy,
XEN_SYSCTL_get_cpu_featureset needs to become conditional.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/misc/xen-cpuid.c      | 17 +++++++++++++----
 xen/arch/x86/sysctl.c       | 17 +++++++++++++----
 xen/include/public/sysctl.h |  2 ++
 3 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
index f55b67640a..36c17bf777 100644
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -251,7 +251,7 @@ static void decode_featureset(const uint32_t *features,
     }
 }
 
-static void get_featureset(xc_interface *xch, unsigned int idx)
+static int get_featureset(xc_interface *xch, unsigned int idx)
 {
     struct fsinfo *f = &featuresets[idx];
 
@@ -261,8 +261,7 @@ static void get_featureset(xc_interface *xch, unsigned int idx)
     if ( !f->fs )
         err(1, "calloc(, featureset)");
 
-    if ( xc_get_cpu_featureset(xch, idx, &f->len, f->fs) )
-        err(1, "xc_get_featureset()");
+    return xc_get_cpu_featureset(xch, idx, &f->len, f->fs);
 }
 
 static void dump_info(xc_interface *xch, bool detail)
@@ -294,7 +293,17 @@ static void dump_info(xc_interface *xch, bool detail)
     printf("\nDynamic sets:\n");
     for ( i = 0; i < ARRAY_SIZE(featuresets); ++i )
     {
-        get_featureset(xch, i);
+        if ( get_featureset(xch, i) )
+        {
+            if ( errno == EOPNOTSUPP )
+            {
+                printf("%s featureset not supported by Xen\n",
+                       featuresets[i].name);
+                continue;
+            }
+
+            err(1, "xc_get_featureset()");
+        }
 
         decode_featureset(featuresets[i].fs, featuresets[i].len,
                           featuresets[i].name, detail);
diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index 59a384023b..7ea8c38797 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -363,11 +363,15 @@ long arch_do_sysctl(
 
     case XEN_SYSCTL_get_cpu_featureset:
     {
-        static const struct cpuid_policy *const policy_table[] = {
+        static const struct cpuid_policy *const policy_table[4] = {
             [XEN_SYSCTL_cpu_featureset_raw]  = &raw_cpuid_policy,
             [XEN_SYSCTL_cpu_featureset_host] = &host_cpuid_policy,
+#ifdef CONFIG_PV
             [XEN_SYSCTL_cpu_featureset_pv]   = &pv_max_cpuid_policy,
+#endif
+#ifdef CONFIG_HVM
             [XEN_SYSCTL_cpu_featureset_hvm]  = &hvm_max_cpuid_policy,
+#endif
         };
         const struct cpuid_policy *p = NULL;
         uint32_t featureset[FSCAPINTS];
@@ -389,12 +393,17 @@ long arch_do_sysctl(
 
         /* Look up requested featureset. */
         if ( sysctl->u.cpu_featureset.index < ARRAY_SIZE(policy_table) )
+        {
             p = policy_table[sysctl->u.cpu_featureset.index];
 
-        /* Bad featureset index? */
-        if ( !p )
-            ret = -EINVAL;
+            if ( !p )
+                ret = -EOPNOTSUPP;
+        }
         else
+            /* Bad featureset index? */
+            ret = -EINVAL;
+
+        if ( !ret )
             cpuid_policy_to_featureset(p, featureset);
 
         /* Copy the requested featureset into place. */
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 4dfba39ed8..3d72fab49f 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -796,6 +796,8 @@ struct xen_sysctl_cpu_levelling_caps {
  *  - Host: The values Xen is using, (after command line overrides, etc).
  *  -   PV: Maximum set of features which can be given to a PV guest.
  *  -  HVM: Maximum set of features which can be given to a HVM guest.
+ * May fail with -EOPNOTSUPP if querying for PV or HVM data when support is
+ * compiled out of Xen.
  */
 struct xen_sysctl_cpu_featureset {
 #define XEN_SYSCTL_cpu_featureset_raw      0
-- 
2.11.0


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

[Xen-devel] [PATCH 02/10] tools/libxc: Simplify xc_get_static_cpu_featuremask()

[Andrew Cooper]

Drop XC_FEATUREMASK_DEEP_FEATURES.  It isn't used by any callers, and unlike
the other static masks, won't be of interest to anyone without other pieces of
cpuid-autogen.h

In xc_get_static_cpu_featuremask(), use a 2d array instead of individually
named variables, and drop the switch statement completely.

No practical change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libxc/include/xenctrl.h |  1 -
 tools/libxc/xc_cpuid_x86.c    | 46 ++++++++++++-------------------------------
 2 files changed, 13 insertions(+), 34 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 99552a5f73..dec3c5de2b 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2488,7 +2488,6 @@ enum xc_static_cpu_featuremask {
     XC_FEATUREMASK_PV,
     XC_FEATUREMASK_HVM_SHADOW,
     XC_FEATUREMASK_HVM_HAP,
-    XC_FEATUREMASK_DEEP_FEATURES,
 };
 const uint32_t *xc_get_static_cpu_featuremask(enum xc_static_cpu_featuremask);
 
diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 21b15b86ec..53cb72438a 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -90,43 +90,23 @@ uint32_t xc_get_cpu_featureset_size(void)
 const uint32_t *xc_get_static_cpu_featuremask(
     enum xc_static_cpu_featuremask mask)
 {
-    const static uint32_t known[FEATURESET_NR_ENTRIES] = INIT_KNOWN_FEATURES,
-        special[FEATURESET_NR_ENTRIES] = INIT_SPECIAL_FEATURES,
-        pv[FEATURESET_NR_ENTRIES] = INIT_PV_FEATURES,
-        hvm_shadow[FEATURESET_NR_ENTRIES] = INIT_HVM_SHADOW_FEATURES,
-        hvm_hap[FEATURESET_NR_ENTRIES] = INIT_HVM_HAP_FEATURES,
-        deep_features[FEATURESET_NR_ENTRIES] = INIT_DEEP_FEATURES;
-
-    BUILD_BUG_ON(ARRAY_SIZE(known) != FEATURESET_NR_ENTRIES);
-    BUILD_BUG_ON(ARRAY_SIZE(special) != FEATURESET_NR_ENTRIES);
-    BUILD_BUG_ON(ARRAY_SIZE(pv) != FEATURESET_NR_ENTRIES);
-    BUILD_BUG_ON(ARRAY_SIZE(hvm_shadow) != FEATURESET_NR_ENTRIES);
-    BUILD_BUG_ON(ARRAY_SIZE(hvm_hap) != FEATURESET_NR_ENTRIES);
-    BUILD_BUG_ON(ARRAY_SIZE(deep_features) != FEATURESET_NR_ENTRIES);
-
-    switch ( mask )
-    {
-    case XC_FEATUREMASK_KNOWN:
-        return known;
-
-    case XC_FEATUREMASK_SPECIAL:
-        return special;
-
-    case XC_FEATUREMASK_PV:
-        return pv;
+    const static uint32_t masks[][FEATURESET_NR_ENTRIES] = {
+#define MASK(x) [XC_FEATUREMASK_ ## x] = INIT_ ## x ## _FEATURES
 
-    case XC_FEATUREMASK_HVM_SHADOW:
-        return hvm_shadow;
+        MASK(KNOWN),
+        MASK(SPECIAL),
+        MASK(PV),
+        MASK(HVM_SHADOW),
+        MASK(HVM_HAP),
 
-    case XC_FEATUREMASK_HVM_HAP:
-        return hvm_hap;
+#undef MASK
+    };
+    BUILD_BUG_ON(ARRAY_SIZE(masks[0]) != FEATURESET_NR_ENTRIES);
 
-    case XC_FEATUREMASK_DEEP_FEATURES:
-        return deep_features;
-
-    default:
+    if ( (unsigned int)mask >= ARRAY_SIZE(masks) )
         return NULL;
-    }
+
+    return masks[mask];
 }
 
 int xc_get_cpu_policy_size(xc_interface *xch, uint32_t *nr_leaves,
-- 
2.11.0


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

[Xen-devel] [PATCH 03/10] x86/gen-cpuid: Rework internal logic to ease future changes

[Andrew Cooper]

Better split the logic between parse/calculate/write.  Collect the feature
comment by their comment character, and perform the accumulation operations in
crunch_numbers().

Avoid rendering the featuresets to C uint32_t's in crunch_numbers(), and
instead do this in write_results().  Update format_uint32s() to call
featureset_to_uint32s() internally.

No functional change - the generated cpuid-autogen.h is identical.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/tools/gen-cpuid.py | 77 +++++++++++++++++++-------------------------------
 1 file changed, 29 insertions(+), 48 deletions(-)

diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
index 71ea78f4eb..99b2e7aeee 100755
--- a/xen/tools/gen-cpuid.py
+++ b/xen/tools/gen-cpuid.py
@@ -20,20 +20,21 @@ def __init__(self, input, output):
         # State parsed from input
         self.names = {}  # Value => Name mapping
         self.values = {} # Name => Value mapping
-        self.raw_special = set()
-        self.raw_pv = set()
-        self.raw_hvm_shadow = set()
-        self.raw_hvm_hap = set()
+        self.raw = {
+            '!': set(),
+            'A': set(), 'S': set(), 'H': set(),
+        }
 
         # State calculated
         self.nr_entries = 0 # Number of words in a featureset
         self.common_1d = 0 # Common features between 1d and e1d
-        self.known = [] # All known features
-        self.special = [] # Features with special semantics
-        self.pv = []
-        self.hvm_shadow = []
-        self.hvm_hap = []
+        self.pv = set() # PV features
+        self.hvm_shadow = set() # HVM shadow features
+        self.hvm_hap = set() # HVM HAP features
         self.bitfields = [] # Text to declare named bitfields in C
+        self.deep_deps = {} # { feature num => dependant features }
+        self.nr_deep_deps = 0 # Number of entries in deep_deps
+        self.deep_features = set() # featureset of keys in deep_deps
 
 def parse_definitions(state):
     """
@@ -81,20 +82,9 @@ def parse_definitions(state):
         state.values[name.lower().replace("_", "-")] = val
 
         for a in attr:
-
-            if a == "!":
-                state.raw_special.add(val)
-            elif a in "ASH":
-                if a == "A":
-                    state.raw_pv.add(val)
-                    state.raw_hvm_shadow.add(val)
-                    state.raw_hvm_hap.add(val)
-                elif attr == "S":
-                    state.raw_hvm_shadow.add(val)
-                    state.raw_hvm_hap.add(val)
-                elif attr == "H":
-                    state.raw_hvm_hap.add(val)
-            else:
+            try:
+                state.raw[a].add(val)
+            except KeyError:
                 raise Fail("Unrecognised attribute '%s' for %s" % (a, name))
 
     if len(state.names) == 0:
@@ -117,10 +107,11 @@ def featureset_to_uint32s(fs, nr):
     if len(words) < nr:
         words.extend([0] * (nr - len(words)))
 
-    return [ "0x%08xU" % x for x in words ]
+    return ("0x%08xU" % x for x in words)
 
-def format_uint32s(words, indent):
+def format_uint32s(state, featureset, indent):
     """ Format a list of uint32_t's suitable for a macro definition """
+    words = featureset_to_uint32s(featureset, state.nr_entries)
     spaces = " " * indent
     return spaces + (", \\\n" + spaces).join(words) + ", \\"
 
@@ -133,13 +124,11 @@ def crunch_numbers(state):
     # Features common between 1d and e1d.
     common_1d = (FPU, VME, DE, PSE, TSC, MSR, PAE, MCE, CX8, APIC,
                  MTRR, PGE, MCA, CMOV, PAT, PSE36, MMX, FXSR)
+    state.common_1d = common_1d
 
-    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)
-    state.hvm_shadow = featureset_to_uint32s(state.raw_hvm_shadow, nr_entries)
-    state.hvm_hap = featureset_to_uint32s(state.raw_hvm_hap, nr_entries)
+    state.pv = state.raw['A']
+    state.hvm_shadow = state.pv | state.raw['S']
+    state.hvm_hap = state.hvm_shadow | state.raw['H']
 
     #
     # Feature dependency information.
@@ -317,17 +306,9 @@ def crunch_numbers(state):
 
         state.deep_deps[feat] = seen[1:]
 
-    state.deep_features = featureset_to_uint32s(deps.keys(), nr_entries)
+    state.deep_features = deps.keys()
     state.nr_deep_deps = len(state.deep_deps.keys())
 
-    try:
-        _tmp = state.deep_deps.iteritems()
-    except AttributeError:
-        _tmp = state.deep_deps.items()
-
-    for k, v in _tmp:
-        state.deep_deps[k] = featureset_to_uint32s(v, nr_entries)
-
     # Calculate the bitfield name declarations
     for word in range(nr_entries):
 
@@ -382,21 +363,21 @@ def write_results(state):
 
 #define INIT_DEEP_DEPS { \\
 """ % (state.nr_entries,
-       state.common_1d,
-       format_uint32s(state.known, 4),
-       format_uint32s(state.special, 4),
-       format_uint32s(state.pv, 4),
-       format_uint32s(state.hvm_shadow, 4),
-       format_uint32s(state.hvm_hap, 4),
+       next(featureset_to_uint32s(state.common_1d, 1)),
+       format_uint32s(state, state.names.keys(), 4),
+       format_uint32s(state, state.raw['!'], 4),
+       format_uint32s(state, state.pv, 4),
+       format_uint32s(state, state.hvm_shadow, 4),
+       format_uint32s(state, state.hvm_hap, 4),
        state.nr_deep_deps,
-       format_uint32s(state.deep_features, 4),
+       format_uint32s(state, state.deep_features, 4),
        ))
 
     for dep in sorted(state.deep_deps.keys()):
         state.output.write(
             "    { %#xU, /* %s */ { \\\n%s\n    }, }, \\\n"
             % (dep, state.names[dep],
-               format_uint32s(state.deep_deps[dep], 8)
+               format_uint32s(state, state.deep_deps[dep], 8)
            ))
 
     state.output.write(
-- 
2.11.0


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

[Xen-devel] [PATCH 04/10] x86/gen-cpuid: Create max and default variations of INIT_*_FEATURES

[Andrew Cooper]

For now, write the same content for both.  Update the users of the
initialisers to use the new name, and extend xen-cpuid to dump both default
and max featuresets.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libxc/include/xenctrl.h |  9 ++++++---
 tools/libxc/xc_cpuid_x86.c    |  9 ++++++---
 tools/misc/xen-cpuid.c        | 18 ++++++++++++------
 xen/arch/x86/cpuid.c          | 20 ++++++++++----------
 xen/tools/gen-cpuid.py        | 40 ++++++++++++++++++++++++++++------------
 5 files changed, 62 insertions(+), 34 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index dec3c5de2b..fc6e57a1a0 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2485,9 +2485,12 @@ uint32_t xc_get_cpu_featureset_size(void);
 enum xc_static_cpu_featuremask {
     XC_FEATUREMASK_KNOWN,
     XC_FEATUREMASK_SPECIAL,
-    XC_FEATUREMASK_PV,
-    XC_FEATUREMASK_HVM_SHADOW,
-    XC_FEATUREMASK_HVM_HAP,
+    XC_FEATUREMASK_PV_MAX,
+    XC_FEATUREMASK_PV_DEF,
+    XC_FEATUREMASK_HVM_SHADOW_MAX,
+    XC_FEATUREMASK_HVM_SHADOW_DEF,
+    XC_FEATUREMASK_HVM_HAP_MAX,
+    XC_FEATUREMASK_HVM_HAP_DEF,
 };
 const uint32_t *xc_get_static_cpu_featuremask(enum xc_static_cpu_featuremask);
 
diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 53cb72438a..c2ea0db25c 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -95,9 +95,12 @@ const uint32_t *xc_get_static_cpu_featuremask(
 
         MASK(KNOWN),
         MASK(SPECIAL),
-        MASK(PV),
-        MASK(HVM_SHADOW),
-        MASK(HVM_HAP),
+        MASK(PV_MAX),
+        MASK(PV_DEF),
+        MASK(HVM_SHADOW_MAX),
+        MASK(HVM_SHADOW_DEF),
+        MASK(HVM_HAP_MAX),
+        MASK(HVM_HAP_DEF),
 
 #undef MASK
     };
diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
index 36c17bf777..585b530b21 100644
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -283,12 +283,18 @@ static void dump_info(xc_interface *xch, bool detail)
                       nr_features, "Known", detail);
     decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_SPECIAL),
                       nr_features, "Special", detail);
-    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_PV),
-                      nr_features, "PV Mask", detail);
-    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_SHADOW),
-                      nr_features, "HVM Shadow Mask", detail);
-    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_HAP),
-                      nr_features, "HVM Hap Mask", detail);
+    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_PV_MAX),
+                      nr_features, "PV Max", detail);
+    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_PV_DEF),
+                      nr_features, "PV Default", detail);
+    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_SHADOW_MAX),
+                      nr_features, "HVM Shadow Max", detail);
+    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_SHADOW_DEF),
+                      nr_features, "HVM Shadow Default", detail);
+    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_HAP_MAX),
+                      nr_features, "HVM Hap Max", detail);
+    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_HAP_DEF),
+                      nr_features, "HVM Hap Default", detail);
 
     printf("\nDynamic sets:\n");
     for ( i = 0; i < ARRAY_SIZE(featuresets); ++i )
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index aee221dc44..546ae31bb9 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -15,9 +15,9 @@
 const uint32_t known_features[] = INIT_KNOWN_FEATURES;
 const uint32_t special_features[] = INIT_SPECIAL_FEATURES;
 
-static const uint32_t pv_featuremask[] = INIT_PV_FEATURES;
-static const uint32_t hvm_shadow_featuremask[] = INIT_HVM_SHADOW_FEATURES;
-static const uint32_t hvm_hap_featuremask[] = INIT_HVM_HAP_FEATURES;
+static const uint32_t pv_max_featuremask[] = INIT_PV_MAX_FEATURES;
+static const uint32_t hvm_shadow_max_featuremask[] = INIT_HVM_SHADOW_MAX_FEATURES;
+static const uint32_t hvm_hap_max_featuremask[] = INIT_HVM_HAP_MAX_FEATURES;
 static const uint32_t deep_features[] = INIT_DEEP_FEATURES;
 
 static int __init parse_xen_cpuid(const char *s)
@@ -359,7 +359,7 @@ static void __init calculate_pv_max_policy(void)
     cpuid_policy_to_featureset(p, pv_featureset);
 
     for ( i = 0; i < ARRAY_SIZE(pv_featureset); ++i )
-        pv_featureset[i] &= pv_featuremask[i];
+        pv_featureset[i] &= pv_max_featuremask[i];
 
     /*
      * If Xen isn't virtualising MSR_SPEC_CTRL for PV guests because of
@@ -391,7 +391,7 @@ static void __init calculate_hvm_max_policy(void)
     cpuid_policy_to_featureset(p, hvm_featureset);
 
     hvm_featuremask = hvm_hap_supported() ?
-        hvm_hap_featuremask : hvm_shadow_featuremask;
+        hvm_hap_max_featuremask : hvm_shadow_max_featuremask;
 
     for ( i = 0; i < ARRAY_SIZE(hvm_featureset); ++i )
         hvm_featureset[i] &= hvm_featuremask[i];
@@ -500,7 +500,7 @@ void recalculate_cpuid_policy(struct domain *d)
         if ( !hap_enabled(d) )
         {
             for ( i = 0; i < ARRAY_SIZE(max_fs); i++ )
-                max_fs[i] &= hvm_shadow_featuremask[i];
+                max_fs[i] &= hvm_shadow_max_featuremask[i];
         }
 
         /* Hide nested-virt if it hasn't been explicitly configured. */
@@ -964,7 +964,7 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
 
             /*
              * PSE36 is not supported in shadow mode.  This bit should be
-             * clear in hvm_shadow_featuremask[].
+             * clear in hvm_shadow_max_featuremask[].
              *
              * However, an unspecified version of Hyper-V from 2011 refuses to
              * start as the "cpu does not provide required hw features" if it
@@ -1003,9 +1003,9 @@ static void __init __maybe_unused build_assertions(void)
 {
     BUILD_BUG_ON(ARRAY_SIZE(known_features) != FSCAPINTS);
     BUILD_BUG_ON(ARRAY_SIZE(special_features) != FSCAPINTS);
-    BUILD_BUG_ON(ARRAY_SIZE(pv_featuremask) != FSCAPINTS);
-    BUILD_BUG_ON(ARRAY_SIZE(hvm_shadow_featuremask) != FSCAPINTS);
-    BUILD_BUG_ON(ARRAY_SIZE(hvm_hap_featuremask) != FSCAPINTS);
+    BUILD_BUG_ON(ARRAY_SIZE(pv_max_featuremask) != FSCAPINTS);
+    BUILD_BUG_ON(ARRAY_SIZE(hvm_shadow_max_featuremask) != FSCAPINTS);
+    BUILD_BUG_ON(ARRAY_SIZE(hvm_hap_max_featuremask) != FSCAPINTS);
     BUILD_BUG_ON(ARRAY_SIZE(deep_features) != FSCAPINTS);
 
     /* Find some more clever allocation scheme if this trips. */
diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
index 99b2e7aeee..af5610a5e6 100755
--- a/xen/tools/gen-cpuid.py
+++ b/xen/tools/gen-cpuid.py
@@ -28,9 +28,12 @@ def __init__(self, input, output):
         # State calculated
         self.nr_entries = 0 # Number of words in a featureset
         self.common_1d = 0 # Common features between 1d and e1d
-        self.pv = set() # PV features
-        self.hvm_shadow = set() # HVM shadow features
-        self.hvm_hap = set() # HVM HAP features
+        self.pv_def = set() # PV default features
+        self.hvm_shadow_def = set() # HVM shadow default features
+        self.hvm_hap_def = set() # HVM HAP default features
+        self.pv_max = set() # PV max features
+        self.hvm_shadow_max = set() # HVM shadow max features
+        self.hvm_hap_max = set() # HVM HAP max features
         self.bitfields = [] # Text to declare named bitfields in C
         self.deep_deps = {} # { feature num => dependant features }
         self.nr_deep_deps = 0 # Number of entries in deep_deps
@@ -126,9 +129,13 @@ def crunch_numbers(state):
                  MTRR, PGE, MCA, CMOV, PAT, PSE36, MMX, FXSR)
     state.common_1d = common_1d
 
-    state.pv = state.raw['A']
-    state.hvm_shadow = state.pv | state.raw['S']
-    state.hvm_hap = state.hvm_shadow | state.raw['H']
+    state.pv_def = state.raw['A']
+    state.hvm_shadow_def = state.pv_def | state.raw['S']
+    state.hvm_hap_def = state.hvm_shadow_def | state.raw['H']
+
+    state.pv_max = state.pv_def
+    state.hvm_shadow_max = state.hvm_shadow_def
+    state.hvm_hap_max = state.hvm_hap_def
 
     #
     # Feature dependency information.
@@ -351,11 +358,17 @@ def write_results(state):
 
 #define INIT_SPECIAL_FEATURES { \\\n%s\n}
 
-#define INIT_PV_FEATURES { \\\n%s\n}
+#define INIT_PV_DEF_FEATURES { \\\n%s\n}
+
+#define INIT_PV_MAX_FEATURES { \\\n%s\n}
+
+#define INIT_HVM_SHADOW_DEF_FEATURES { \\\n%s\n}
+
+#define INIT_HVM_SHADOW_MAX_FEATURES { \\\n%s\n}
 
-#define INIT_HVM_SHADOW_FEATURES { \\\n%s\n}
+#define INIT_HVM_HAP_DEF_FEATURES { \\\n%s\n}
 
-#define INIT_HVM_HAP_FEATURES { \\\n%s\n}
+#define INIT_HVM_HAP_MAX_FEATURES { \\\n%s\n}
 
 #define NR_DEEP_DEPS %sU
 
@@ -366,9 +379,12 @@ def write_results(state):
        next(featureset_to_uint32s(state.common_1d, 1)),
        format_uint32s(state, state.names.keys(), 4),
        format_uint32s(state, state.raw['!'], 4),
-       format_uint32s(state, state.pv, 4),
-       format_uint32s(state, state.hvm_shadow, 4),
-       format_uint32s(state, state.hvm_hap, 4),
+       format_uint32s(state, state.pv_def, 4),
+       format_uint32s(state, state.pv_max, 4),
+       format_uint32s(state, state.hvm_shadow_def, 4),
+       format_uint32s(state, state.hvm_shadow_max, 4),
+       format_uint32s(state, state.hvm_hap_def, 4),
+       format_uint32s(state, state.hvm_hap_max, 4),
        state.nr_deep_deps,
        format_uint32s(state, state.deep_features, 4),
        ))
-- 
2.11.0


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

[Xen-devel] [PATCH 05/10] x86/msr: Compile out unused logic/objects

[Andrew Cooper]

Arrange to compile out the PV or HVM logic and objects as applicable.  This
involves a bit of complexity in init_domain_msr_policy() as is_pv_domain()
can't be evaulated at compile time.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/msr.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index e39bb6dce4..738d7123f9 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -31,9 +31,13 @@
 DEFINE_PER_CPU(uint32_t, tsc_aux);
 
 struct msr_policy __read_mostly     raw_msr_policy,
-                  __read_mostly    host_msr_policy,
-                  __read_mostly hvm_max_msr_policy,
-                  __read_mostly  pv_max_msr_policy;
+                  __read_mostly    host_msr_policy;
+#ifdef CONFIG_PV
+struct msr_policy __read_mostly  pv_max_msr_policy;
+#endif
+#ifdef CONFIG_HVM
+struct msr_policy __read_mostly hvm_max_msr_policy;
+#endif
 
 static void __init calculate_raw_policy(void)
 {
@@ -56,9 +60,6 @@ static void __init calculate_hvm_max_policy(void)
 {
     struct msr_policy *mp = &hvm_max_msr_policy;
 
-    if ( !hvm_enabled )
-        return;
-
     *mp = host_msr_policy;
 
     /* It's always possible to emulate CPUID faulting for HVM guests */
@@ -76,16 +77,27 @@ void __init init_guest_msr_policy(void)
 {
     calculate_raw_policy();
     calculate_host_policy();
-    calculate_hvm_max_policy();
-    calculate_pv_max_policy();
+
+    if ( IS_ENABLED(CONFIG_PV) )
+        calculate_pv_max_policy();
+
+    if ( hvm_enabled )
+        calculate_hvm_max_policy();
 }
 
 int init_domain_msr_policy(struct domain *d)
 {
-    struct msr_policy *mp =
-        xmemdup(is_pv_domain(d) ?  &pv_max_msr_policy
-                                : &hvm_max_msr_policy);
+    struct msr_policy *mp = is_pv_domain(d)
+        ? (IS_ENABLED(CONFIG_PV)  ?  &pv_max_msr_policy : NULL)
+        : (IS_ENABLED(CONFIG_HVM) ? &hvm_max_msr_policy : NULL);
+
+    if ( !mp )
+    {
+        ASSERT_UNREACHABLE();
+        return -EOPNOTSUPP;
+    }
 
+    mp = xmemdup(mp);
     if ( !mp )
         return -ENOMEM;
 
-- 
2.11.0


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

[Xen-devel] [PATCH 06/10] x86/msr: Introduce and use default MSR policies

[Andrew Cooper]

For now, the default and max policies remain identical, but this will change
in the future.

Update XEN_SYSCTL_get_cpu_policy and init_domain_msr_policy() to use the
default policies.

Take the opportunity sort PV ahead of HVM, as is the prevailing style
elsewhere.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/msr.c        | 32 +++++++++++++++++++++++++++-----
 xen/arch/x86/sysctl.c     |  4 ++--
 xen/include/asm-x86/msr.h |  4 +++-
 3 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 738d7123f9..519222a2b8 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -34,9 +34,11 @@ struct msr_policy __read_mostly     raw_msr_policy,
                   __read_mostly    host_msr_policy;
 #ifdef CONFIG_PV
 struct msr_policy __read_mostly  pv_max_msr_policy;
+struct msr_policy __read_mostly  pv_def_msr_policy;
 #endif
 #ifdef CONFIG_HVM
 struct msr_policy __read_mostly hvm_max_msr_policy;
+struct msr_policy __read_mostly hvm_def_msr_policy;
 #endif
 
 static void __init calculate_raw_policy(void)
@@ -56,6 +58,20 @@ static void __init calculate_host_policy(void)
     mp->platform_info.cpuid_faulting = cpu_has_cpuid_faulting;
 }
 
+static void __init calculate_pv_max_policy(void)
+{
+    struct msr_policy *mp = &pv_max_msr_policy;
+
+    *mp = host_msr_policy;
+}
+
+static void __init calculate_pv_def_policy(void)
+{
+    struct msr_policy *mp = &pv_def_msr_policy;
+
+    *mp = pv_max_msr_policy;
+}
+
 static void __init calculate_hvm_max_policy(void)
 {
     struct msr_policy *mp = &hvm_max_msr_policy;
@@ -66,11 +82,11 @@ static void __init calculate_hvm_max_policy(void)
     mp->platform_info.cpuid_faulting = true;
 }
 
-static void __init calculate_pv_max_policy(void)
+static void __init calculate_hvm_def_policy(void)
 {
-    struct msr_policy *mp = &pv_max_msr_policy;
+    struct msr_policy *mp = &hvm_def_msr_policy;
 
-    *mp = host_msr_policy;
+    *mp = hvm_max_msr_policy;
 }
 
 void __init init_guest_msr_policy(void)
@@ -79,17 +95,23 @@ void __init init_guest_msr_policy(void)
     calculate_host_policy();
 
     if ( IS_ENABLED(CONFIG_PV) )
+    {
         calculate_pv_max_policy();
+        calculate_pv_def_policy();
+    }
 
     if ( hvm_enabled )
+    {
         calculate_hvm_max_policy();
+        calculate_hvm_def_policy();
+    }
 }
 
 int init_domain_msr_policy(struct domain *d)
 {
     struct msr_policy *mp = is_pv_domain(d)
-        ? (IS_ENABLED(CONFIG_PV)  ?  &pv_max_msr_policy : NULL)
-        : (IS_ENABLED(CONFIG_HVM) ? &hvm_max_msr_policy : NULL);
+        ? (IS_ENABLED(CONFIG_PV)  ?  &pv_def_msr_policy : NULL)
+        : (IS_ENABLED(CONFIG_HVM) ? &hvm_def_msr_policy : NULL);
 
     if ( !mp )
     {
diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index 7ea8c38797..cad7534373 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -49,7 +49,7 @@ const struct cpu_policy system_policies[6] = {
     },
     [ XEN_SYSCTL_cpu_policy_pv_default ] = {
         &pv_max_cpuid_policy,
-        &pv_max_msr_policy,
+        &pv_def_msr_policy,
     },
 #endif
 #ifdef CONFIG_HVM
@@ -59,7 +59,7 @@ const struct cpu_policy system_policies[6] = {
     },
     [ XEN_SYSCTL_cpu_policy_hvm_default ] = {
         &hvm_max_cpuid_policy,
-        &hvm_max_msr_policy,
+        &hvm_def_msr_policy,
     },
 #endif
 };
diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h
index bca41a3670..41397e19cf 100644
--- a/xen/include/asm-x86/msr.h
+++ b/xen/include/asm-x86/msr.h
@@ -269,8 +269,10 @@ static inline void wrmsr_tsc_aux(uint32_t val)
 
 extern struct msr_policy     raw_msr_policy,
                             host_msr_policy,
+                          pv_max_msr_policy,
+                          pv_def_msr_policy,
                          hvm_max_msr_policy,
-                          pv_max_msr_policy;
+                         hvm_def_msr_policy;
 
 /* Container object for per-vCPU MSRs */
 struct vcpu_msrs
-- 
2.11.0


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

[Xen-devel] [PATCH 07/10] x86/cpuid: Compile out unused logic/objects

[Andrew Cooper]

CPUID Policy objects are large (1860 bytes at the time of writing), so
compiling them out based on CONFIG_{PV,HVM} makes a lot of sense.

This involves a bit of complexity in init_domain_cpuid_policy() and
recalculate_cpuid_policy() as is_pv_domain() can't be evaulated at compile
time.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpuid.c | 47 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 546ae31bb9..cd9a02143c 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -95,10 +95,14 @@ static void zero_leaves(struct cpuid_leaf *l,
     memset(&l[first], 0, sizeof(*l) * (last - first + 1));
 }
 
-struct cpuid_policy __read_mostly raw_cpuid_policy,
-    __read_mostly host_cpuid_policy,
-    __read_mostly pv_max_cpuid_policy,
-    __read_mostly hvm_max_cpuid_policy;
+struct cpuid_policy __read_mostly     raw_cpuid_policy,
+                    __read_mostly    host_cpuid_policy;
+#ifdef CONFIG_PV
+struct cpuid_policy __read_mostly  pv_max_cpuid_policy;
+#endif
+#ifdef CONFIG_HVM
+struct cpuid_policy __read_mostly hvm_max_cpuid_policy;
+#endif
 
 static void sanitise_featureset(uint32_t *fs)
 {
@@ -384,9 +388,6 @@ static void __init calculate_hvm_max_policy(void)
     unsigned int i;
     const uint32_t *hvm_featuremask;
 
-    if ( !hvm_enabled )
-        return;
-
     *p = host_cpuid_policy;
     cpuid_policy_to_featureset(p, hvm_featureset);
 
@@ -443,8 +444,12 @@ void __init init_guest_cpuid(void)
 {
     calculate_raw_policy();
     calculate_host_policy();
-    calculate_pv_max_policy();
-    calculate_hvm_max_policy();
+
+    if ( IS_ENABLED(CONFIG_PV) )
+        calculate_pv_max_policy();
+
+    if ( hvm_enabled )
+        calculate_hvm_max_policy();
 }
 
 bool recheck_cpu_features(unsigned int cpu)
@@ -472,11 +477,18 @@ bool recheck_cpu_features(unsigned int cpu)
 void recalculate_cpuid_policy(struct domain *d)
 {
     struct cpuid_policy *p = d->arch.cpuid;
-    const struct cpuid_policy *max =
-        is_pv_domain(d) ? &pv_max_cpuid_policy : &hvm_max_cpuid_policy;
+    const struct cpuid_policy *max = is_pv_domain(d)
+        ? (IS_ENABLED(CONFIG_PV)  ?  &pv_max_cpuid_policy : NULL)
+        : (IS_ENABLED(CONFIG_HVM) ? &hvm_max_cpuid_policy : NULL);
     uint32_t fs[FSCAPINTS], max_fs[FSCAPINTS];
     unsigned int i;
 
+    if ( !max )
+    {
+        ASSERT_UNREACHABLE();
+        return;
+    }
+
     p->x86_vendor = x86_cpuid_lookup_vendor(
         p->basic.vendor_ebx, p->basic.vendor_ecx, p->basic.vendor_edx);
 
@@ -612,10 +624,17 @@ void recalculate_cpuid_policy(struct domain *d)
 
 int init_domain_cpuid_policy(struct domain *d)
 {
-    struct cpuid_policy *p =
-        xmemdup(is_pv_domain(d) ?  &pv_max_cpuid_policy
-                                : &hvm_max_cpuid_policy);
+    struct cpuid_policy *p = is_pv_domain(d)
+        ? (IS_ENABLED(CONFIG_PV)  ?  &pv_max_cpuid_policy : NULL)
+        : (IS_ENABLED(CONFIG_HVM) ? &hvm_max_cpuid_policy : NULL);
+
+    if ( !p )
+    {
+        ASSERT_UNREACHABLE();
+        return -EOPNOTSUPP;
+    }
 
+    p = xmemdup(p);
     if ( !p )
         return -ENOMEM;
 
-- 
2.11.0


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

[Xen-devel] [PATCH 08/10] x86/cpuid: Introduce and use default CPUID policies

[Andrew Cooper]

For now, the default and max policies remain identical, but this will change
in the future.  Write calculate_{pv,hvm}_def_policy() in a way which will cope
with simple feature differences for now.

Update XEN_SYSCTL_get_cpu_policy and init_domain_cpuid_policy() to use the
default policies.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpuid.c        | 55 +++++++++++++++++++++++++++++++++++++++++++--
 xen/arch/x86/sysctl.c       |  4 ++--
 xen/include/asm-x86/cpuid.h |  3 ++-
 3 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index cd9a02143c..6e01394fd2 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -18,6 +18,9 @@ const uint32_t special_features[] = INIT_SPECIAL_FEATURES;
 static const uint32_t pv_max_featuremask[] = INIT_PV_MAX_FEATURES;
 static const uint32_t hvm_shadow_max_featuremask[] = INIT_HVM_SHADOW_MAX_FEATURES;
 static const uint32_t hvm_hap_max_featuremask[] = INIT_HVM_HAP_MAX_FEATURES;
+static const uint32_t pv_def_featuremask[] = INIT_PV_DEF_FEATURES;
+static const uint32_t hvm_shadow_def_featuremask[] = INIT_HVM_SHADOW_DEF_FEATURES;
+static const uint32_t hvm_hap_def_featuremask[] = INIT_HVM_HAP_DEF_FEATURES;
 static const uint32_t deep_features[] = INIT_DEEP_FEATURES;
 
 static int __init parse_xen_cpuid(const char *s)
@@ -99,9 +102,11 @@ struct cpuid_policy __read_mostly     raw_cpuid_policy,
                     __read_mostly    host_cpuid_policy;
 #ifdef CONFIG_PV
 struct cpuid_policy __read_mostly  pv_max_cpuid_policy;
+struct cpuid_policy __read_mostly  pv_def_cpuid_policy;
 #endif
 #ifdef CONFIG_HVM
 struct cpuid_policy __read_mostly hvm_max_cpuid_policy;
+struct cpuid_policy __read_mostly hvm_def_cpuid_policy;
 #endif
 
 static void sanitise_featureset(uint32_t *fs)
@@ -381,6 +386,23 @@ static void __init calculate_pv_max_policy(void)
     p->extd.raw[0xa] = EMPTY_LEAF; /* No SVM for PV guests. */
 }
 
+static void __init calculate_pv_def_policy(void)
+{
+    struct cpuid_policy *p = &pv_def_cpuid_policy;
+    uint32_t pv_featureset[FSCAPINTS];
+    unsigned int i;
+
+    *p = pv_max_cpuid_policy;
+    cpuid_policy_to_featureset(p, pv_featureset);
+
+    for ( i = 0; i < ARRAY_SIZE(pv_featureset); ++i )
+        pv_featureset[i] &= pv_def_featuremask[i];
+
+    sanitise_featureset(pv_featureset);
+    cpuid_featureset_to_policy(pv_featureset, p);
+    recalculate_xstate(p);
+}
+
 static void __init calculate_hvm_max_policy(void)
 {
     struct cpuid_policy *p = &hvm_max_cpuid_policy;
@@ -440,16 +462,45 @@ static void __init calculate_hvm_max_policy(void)
     recalculate_xstate(p);
 }
 
+static void __init calculate_hvm_def_policy(void)
+{
+    struct cpuid_policy *p = &hvm_def_cpuid_policy;
+    uint32_t hvm_featureset[FSCAPINTS];
+    unsigned int i;
+    const uint32_t *hvm_featuremask;
+
+    *p = hvm_max_cpuid_policy;
+    cpuid_policy_to_featureset(p, hvm_featureset);
+
+    hvm_featuremask = hvm_hap_supported() ?
+        hvm_hap_def_featuremask : hvm_shadow_def_featuremask;
+
+    for ( i = 0; i < ARRAY_SIZE(hvm_featureset); ++i )
+        hvm_featureset[i] &= hvm_featuremask[i];
+
+    guest_common_feature_adjustments(hvm_featureset);
+
+    sanitise_featureset(hvm_featureset);
+    cpuid_featureset_to_policy(hvm_featureset, p);
+    recalculate_xstate(p);
+}
+
 void __init init_guest_cpuid(void)
 {
     calculate_raw_policy();
     calculate_host_policy();
 
     if ( IS_ENABLED(CONFIG_PV) )
+    {
         calculate_pv_max_policy();
+        calculate_pv_def_policy();
+    }
 
     if ( hvm_enabled )
+    {
         calculate_hvm_max_policy();
+        calculate_hvm_def_policy();
+    }
 }
 
 bool recheck_cpu_features(unsigned int cpu)
@@ -625,8 +676,8 @@ void recalculate_cpuid_policy(struct domain *d)
 int init_domain_cpuid_policy(struct domain *d)
 {
     struct cpuid_policy *p = is_pv_domain(d)
-        ? (IS_ENABLED(CONFIG_PV)  ?  &pv_max_cpuid_policy : NULL)
-        : (IS_ENABLED(CONFIG_HVM) ? &hvm_max_cpuid_policy : NULL);
+        ? (IS_ENABLED(CONFIG_PV)  ?  &pv_def_cpuid_policy : NULL)
+        : (IS_ENABLED(CONFIG_HVM) ? &hvm_def_cpuid_policy : NULL);
 
     if ( !p )
     {
diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index cad7534373..b7948f2663 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -48,7 +48,7 @@ const struct cpu_policy system_policies[6] = {
         &pv_max_msr_policy,
     },
     [ XEN_SYSCTL_cpu_policy_pv_default ] = {
-        &pv_max_cpuid_policy,
+        &pv_def_cpuid_policy,
         &pv_def_msr_policy,
     },
 #endif
@@ -58,7 +58,7 @@ const struct cpu_policy system_policies[6] = {
         &hvm_max_msr_policy,
     },
     [ XEN_SYSCTL_cpu_policy_hvm_default ] = {
-        &hvm_max_cpuid_policy,
+        &hvm_def_cpuid_policy,
         &hvm_def_msr_policy,
     },
 #endif
diff --git a/xen/include/asm-x86/cpuid.h b/xen/include/asm-x86/cpuid.h
index 1b00e832d6..7baf6c9628 100644
--- a/xen/include/asm-x86/cpuid.h
+++ b/xen/include/asm-x86/cpuid.h
@@ -49,7 +49,8 @@ DECLARE_PER_CPU(struct cpuidmasks, cpuidmasks);
 extern struct cpuidmasks cpuidmask_defaults;
 
 extern struct cpuid_policy raw_cpuid_policy, host_cpuid_policy,
-    pv_max_cpuid_policy, hvm_max_cpuid_policy;
+    pv_max_cpuid_policy, pv_def_cpuid_policy,
+    hvm_max_cpuid_policy, hvm_def_cpuid_policy;
 
 extern const struct cpu_policy system_policies[];
 
-- 
2.11.0


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

[Xen-devel] [PATCH 09/10] x86/gen-cpuid: Distinguish default vs max in feature annotations

[Andrew Cooper]

Allow lowercase a/s/h to be used to annotate a non-default feature.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/include/public/arch-x86/cpufeatureset.h | 2 ++
 xen/tools/gen-cpuid.py                      | 7 ++++---
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index 086736ac7b..d79a53befe 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -87,6 +87,8 @@ enum {
  *   'A' = All guests.
  *   'S' = All HVM guests (not PV guests).
  *   'H' = HVM HAP guests (not PV or HVM Shadow guests).
+ *   Upper case => Available by default
+ *   Lower case => Can be opted-in to, but not available by default.
  */
 
 /* Intel-defined CPU features, CPUID level 0x00000001.edx, word 0 */
diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
index af5610a5e6..c178e2470d 100755
--- a/xen/tools/gen-cpuid.py
+++ b/xen/tools/gen-cpuid.py
@@ -23,6 +23,7 @@ def __init__(self, input, output):
         self.raw = {
             '!': set(),
             'A': set(), 'S': set(), 'H': set(),
+            'a': set(), 's': set(), 'h': set(),
         }
 
         # State calculated
@@ -133,9 +134,9 @@ def crunch_numbers(state):
     state.hvm_shadow_def = state.pv_def | state.raw['S']
     state.hvm_hap_def = state.hvm_shadow_def | state.raw['H']
 
-    state.pv_max = state.pv_def
-    state.hvm_shadow_max = state.hvm_shadow_def
-    state.hvm_hap_max = state.hvm_hap_def
+    state.pv_max = state.raw['A'] | state.raw['a']
+    state.hvm_shadow_max = state.pv_max | state.raw['S'] | state.raw['s']
+    state.hvm_hap_max = state.hvm_shadow_max | state.raw['H'] | state.raw['h']
 
     #
     # Feature dependency information.
-- 
2.11.0


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

[Xen-devel] [PATCH 10/10] x86/hvm: Do not enable MPX by default

[Andrew Cooper]

Memory Protection eXtension support has been dropped from GCC and Linux, and
will be dropped from future Intel CPUs.

With all other default/max pieces in place, move MPX from default to max.
This means that VMs won't be offered it by default, but can explicitly opt
into using it via cpuid="host,mpx=1" in their vm.cfg file.

The difference as visible to the guest is:

  diff --git a/default b/mpx
  index 0e91765d6b..c8c33cd584 100644
  --- a/default
  +++ b/mpx
  @@ -13,15 +13,17 @@ Native cpuid:
     00000004:00000004 -> 00000000:00000000:00000000:00000000
     00000005:ffffffff -> 00000000:00000000:00000000:00000000
     00000006:ffffffff -> 00000000:00000000:00000000:00000000
  -  00000007:00000000 -> 00000000:009c2fbb:00000000:9c000400
  +  00000007:00000000 -> 00000000:009c6fbb:00000000:9c000400
     00000008:ffffffff -> 00000000:00000000:00000000:00000000
     00000009:ffffffff -> 00000000:00000000:00000000:00000000
     0000000a:ffffffff -> 00000000:00000000:00000000:00000000
     0000000b:ffffffff -> 00000000:00000000:00000000:00000000
     0000000c:ffffffff -> 00000000:00000000:00000000:00000000
  -  0000000d:00000000 -> 00000007:00000240:00000340:00000000
  +  0000000d:00000000 -> 0000001f:00000240:00000440:00000000
     0000000d:00000001 -> 0000000f:00000240:00000000:00000000
     0000000d:00000002 -> 00000100:00000240:00000000:00000000
  +  0000000d:00000003 -> 00000040:000003c0:00000000:00000000
  +  0000000d:00000004 -> 00000040:00000400:00000000:00000000
     40000000:ffffffff -> 40000005:566e6558:65584d4d:4d4d566e
     40000001:ffffffff -> 0004000e:00000000:00000000:00000000
     40000002:ffffffff -> 00000001:40000000:00000000:00000000

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>

XXX - One moving piece (the migration series) is still in review on xen-devel.
I won't commit this change until that is sorted, and I can double check the
backwards compatibility for VMs from previous versions of Xen.

The main purpose of posting this patch now is to illustrate the effects of the
previous patches in the series.
---
 xen/include/public/arch-x86/cpufeatureset.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index d79a53befe..81e4c2950f 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -207,7 +207,7 @@ XEN_CPUFEATURE(INVPCID,       5*32+10) /*H  Invalidate Process Context ID */
 XEN_CPUFEATURE(RTM,           5*32+11) /*A  Restricted Transactional Memory */
 XEN_CPUFEATURE(PQM,           5*32+12) /*   Platform QoS Monitoring */
 XEN_CPUFEATURE(NO_FPU_SEL,    5*32+13) /*!  FPU CS/DS stored as zero */
-XEN_CPUFEATURE(MPX,           5*32+14) /*S  Memory Protection Extensions */
+XEN_CPUFEATURE(MPX,           5*32+14) /*s  Memory Protection Extensions */
 XEN_CPUFEATURE(PQE,           5*32+15) /*   Platform QoS Enforcement */
 XEN_CPUFEATURE(AVX512F,       5*32+16) /*A  AVX-512 Foundation Instructions */
 XEN_CPUFEATURE(AVX512DQ,      5*32+17) /*A  AVX-512 Doubleword & Quadword Instrs */
-- 
2.11.0


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

Re: [Xen-devel] [PATCH 01/10] x86/sysctl: Don't return cpu policy data for compiled-out support (2)

[Jan Beulich]

On 26.02.2020 21:22, Andrew Cooper wrote:
> Just as with c/s 96dc77b4b1 for XEN_SYSCTL_get_cpu_policy,
> XEN_SYSCTL_get_cpu_featureset needs to become conditional.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Albeit I'd say "want", not "needs" in the description.

Jan

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

Re: [Xen-devel] [PATCH 02/10] tools/libxc: Simplify xc_get_static_cpu_featuremask()

[Jan Beulich]

On 26.02.2020 21:22, Andrew Cooper wrote:
> Drop XC_FEATUREMASK_DEEP_FEATURES.  It isn't used by any callers, and unlike
> the other static masks, won't be of interest to anyone without other pieces of
> cpuid-autogen.h
> 
> In xc_get_static_cpu_featuremask(), use a 2d array instead of individually
> named variables, and drop the switch statement completely.
> 
> No practical change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

> --- a/tools/libxc/xc_cpuid_x86.c
> +++ b/tools/libxc/xc_cpuid_x86.c
> @@ -90,43 +90,23 @@ uint32_t xc_get_cpu_featureset_size(void)
>  const uint32_t *xc_get_static_cpu_featuremask(
>      enum xc_static_cpu_featuremask mask)
>  {
> -    const static uint32_t known[FEATURESET_NR_ENTRIES] = INIT_KNOWN_FEATURES,
> -        special[FEATURESET_NR_ENTRIES] = INIT_SPECIAL_FEATURES,
> -        pv[FEATURESET_NR_ENTRIES] = INIT_PV_FEATURES,
> -        hvm_shadow[FEATURESET_NR_ENTRIES] = INIT_HVM_SHADOW_FEATURES,
> -        hvm_hap[FEATURESET_NR_ENTRIES] = INIT_HVM_HAP_FEATURES,
> -        deep_features[FEATURESET_NR_ENTRIES] = INIT_DEEP_FEATURES;
> -
> -    BUILD_BUG_ON(ARRAY_SIZE(known) != FEATURESET_NR_ENTRIES);
> -    BUILD_BUG_ON(ARRAY_SIZE(special) != FEATURESET_NR_ENTRIES);
> -    BUILD_BUG_ON(ARRAY_SIZE(pv) != FEATURESET_NR_ENTRIES);
> -    BUILD_BUG_ON(ARRAY_SIZE(hvm_shadow) != FEATURESET_NR_ENTRIES);
> -    BUILD_BUG_ON(ARRAY_SIZE(hvm_hap) != FEATURESET_NR_ENTRIES);
> -    BUILD_BUG_ON(ARRAY_SIZE(deep_features) != FEATURESET_NR_ENTRIES);
> -
> -    switch ( mask )
> -    {
> -    case XC_FEATUREMASK_KNOWN:
> -        return known;
> -
> -    case XC_FEATUREMASK_SPECIAL:
> -        return special;
> -
> -    case XC_FEATUREMASK_PV:
> -        return pv;
> +    const static uint32_t masks[][FEATURESET_NR_ENTRIES] = {

Would you mind switching to the more conventional "static const"?

> +#define MASK(x) [XC_FEATUREMASK_ ## x] = INIT_ ## x ## _FEATURES

I'm surprised to see you introduce such a construct, when more
than once I heard you say that you dislike macros using ## in
ways like it is done here.

> +    BUILD_BUG_ON(ARRAY_SIZE(masks[0]) != FEATURESET_NR_ENTRIES);

Isn't this quite pointless with the now changed definition of
the array?

Jan

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

Re: [Xen-devel] [PATCH 03/10] x86/gen-cpuid: Rework internal logic to ease future changes

[Jan Beulich]

On 26.02.2020 21:22, Andrew Cooper wrote:
> Better split the logic between parse/calculate/write.  Collect the feature
> comment by their comment character, and perform the accumulation operations in
> crunch_numbers().

Would you mind saying "character(s)" here, as there are items with
multiple of them?

> Avoid rendering the featuresets to C uint32_t's in crunch_numbers(), and
> instead do this in write_results().  Update format_uint32s() to call
> featureset_to_uint32s() internally.
> 
> No functional change - the generated cpuid-autogen.h is identical.

I notice the "special" field (or however such is called in Python)
goes away, in favor of using raw['!'] at the apparently sole
consuming site. I also notice the same isn't true for "pv", which
now could also be raw['A'] as it seems. If this is the case (i.e.
I'm not overlooking anything), could you say a word on the change
for "special" and/or the difference between "special" and "pv"?

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

With my limited Python skills merely
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

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

Re: [Xen-devel] [PATCH 04/10] x86/gen-cpuid: Create max and default variations of INIT_*_FEATURES

[Jan Beulich]

On 26.02.2020 21:22, Andrew Cooper wrote:
> For now, write the same content for both.  Update the users of the
> initialisers to use the new name, and extend xen-cpuid to dump both default
> and max featuresets.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Hypervisor and libxc parts
Reviewed-by: Jan Beulich <jbeulich@suse.com>

> --- a/tools/misc/xen-cpuid.c
> +++ b/tools/misc/xen-cpuid.c
> @@ -283,12 +283,18 @@ static void dump_info(xc_interface *xch, bool detail)
>                        nr_features, "Known", detail);
>      decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_SPECIAL),
>                        nr_features, "Special", detail);
> -    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_PV),
> -                      nr_features, "PV Mask", detail);
> -    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_SHADOW),
> -                      nr_features, "HVM Shadow Mask", detail);
> -    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_HAP),
> -                      nr_features, "HVM Hap Mask", detail);
> +    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_PV_MAX),
> +                      nr_features, "PV Max", detail);
> +    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_PV_DEF),
> +                      nr_features, "PV Default", detail);
> +    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_SHADOW_MAX),
> +                      nr_features, "HVM Shadow Max", detail);
> +    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_SHADOW_DEF),
> +                      nr_features, "HVM Shadow Default", detail);
> +    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_HAP_MAX),
> +                      nr_features, "HVM Hap Max", detail);
> +    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_HAP_DEF),
> +                      nr_features, "HVM Hap Default", detail);

Spotting differences between max and default this way is, I assume,
going to be quite difficult / error prone. Wouldn't it be better to
produce the default set in full, and then list just the extra items
in max? Aiui max is always going to be a superset of def.

Jan

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

Re: [Xen-devel] [PATCH 05/10] x86/msr: Compile out unused logic/objects

[Jan Beulich]

On 26.02.2020 21:22, Andrew Cooper wrote:
> @@ -76,16 +77,27 @@ void __init init_guest_msr_policy(void)
>  {
>      calculate_raw_policy();
>      calculate_host_policy();
> -    calculate_hvm_max_policy();
> -    calculate_pv_max_policy();
> +
> +    if ( IS_ENABLED(CONFIG_PV) )
> +        calculate_pv_max_policy();
> +
> +    if ( hvm_enabled )


Any chance of talking you into doing things more symmetrically,
by either also using IS_ENABLED(CONFIG_HVM) here or ...

> +        calculate_hvm_max_policy();
>  }
>  
>  int init_domain_msr_policy(struct domain *d)
>  {
> -    struct msr_policy *mp =
> -        xmemdup(is_pv_domain(d) ?  &pv_max_msr_policy
> -                                : &hvm_max_msr_policy);
> +    struct msr_policy *mp = is_pv_domain(d)
> +        ? (IS_ENABLED(CONFIG_PV)  ?  &pv_max_msr_policy : NULL)
> +        : (IS_ENABLED(CONFIG_HVM) ? &hvm_max_msr_policy : NULL);

... (imo preferably) hvm_enabled here? Either way
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

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

Re: [Xen-devel] [PATCH 06/10] x86/msr: Introduce and use default MSR policies

[Jan Beulich]

On 26.02.2020 21:22, Andrew Cooper wrote:
> For now, the default and max policies remain identical, but this will change
> in the future.
> 
> Update XEN_SYSCTL_get_cpu_policy and init_domain_msr_policy() to use the
> default policies.
> 
> Take the opportunity sort PV ahead of HVM, as is the prevailing style
> elsewhere.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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


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

Re: [Xen-devel] [PATCH 07/10] x86/cpuid: Compile out unused logic/objects

[Jan Beulich]

On 26.02.2020 21:22, Andrew Cooper wrote:
> CPUID Policy objects are large (1860 bytes at the time of writing), so
> compiling them out based on CONFIG_{PV,HVM} makes a lot of sense.
> 
> This involves a bit of complexity in init_domain_cpuid_policy() and
> recalculate_cpuid_policy() as is_pv_domain() can't be evaulated at compile
> time.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

With the same remark as for the MSR side
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

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

Re: [Xen-devel] [PATCH 08/10] x86/cpuid: Introduce and use default CPUID policies

[Jan Beulich]

On 26.02.2020 21:22, Andrew Cooper wrote:
> For now, the default and max policies remain identical, but this will change
> in the future.  Write calculate_{pv,hvm}_def_policy() in a way which will cope
> with simple feature differences for now.
> 
> Update XEN_SYSCTL_get_cpu_policy and init_domain_cpuid_policy() to use the
> default policies.

For the sysctl the statement looks to be broader than reality,
as (of course) you don't touch XEN_SYSCTL_cpu_policy_*_max.

> @@ -381,6 +386,23 @@ static void __init calculate_pv_max_policy(void)
>      p->extd.raw[0xa] = EMPTY_LEAF; /* No SVM for PV guests. */
>  }
>  
> +static void __init calculate_pv_def_policy(void)
> +{
> +    struct cpuid_policy *p = &pv_def_cpuid_policy;
> +    uint32_t pv_featureset[FSCAPINTS];
> +    unsigned int i;
> +
> +    *p = pv_max_cpuid_policy;
> +    cpuid_policy_to_featureset(p, pv_featureset);
> +
> +    for ( i = 0; i < ARRAY_SIZE(pv_featureset); ++i )
> +        pv_featureset[i] &= pv_def_featuremask[i];
> +
> +    sanitise_featureset(pv_featureset);
> +    cpuid_featureset_to_policy(pv_featureset, p);
> +    recalculate_xstate(p);
> +}

Is there a reason the call to guest_common_feature_adjustments()
is missing here? If so, I think you want to say a word on the why
in the description. If not, with it added
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

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

Re: [Xen-devel] [PATCH 09/10] x86/gen-cpuid: Distinguish default vs max in feature annotations

[Jan Beulich]

On 26.02.2020 21:22, Andrew Cooper wrote:
> Allow lowercase a/s/h to be used to annotate a non-default feature.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

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

Re: [Xen-devel] [PATCH 10/10] x86/hvm: Do not enable MPX by default

[Jan Beulich]

On 26.02.2020 21:22, Andrew Cooper wrote:
> Memory Protection eXtension support has been dropped from GCC and Linux, and
> will be dropped from future Intel CPUs.
> 
> With all other default/max pieces in place, move MPX from default to max.
> This means that VMs won't be offered it by default, but can explicitly opt
> into using it via cpuid="host,mpx=1" in their vm.cfg file.
> 
> The difference as visible to the guest is:
> 
>   diff --git a/default b/mpx
>   index 0e91765d6b..c8c33cd584 100644
>   --- a/default
>   +++ b/mpx
>   @@ -13,15 +13,17 @@ Native cpuid:
>      00000004:00000004 -> 00000000:00000000:00000000:00000000
>      00000005:ffffffff -> 00000000:00000000:00000000:00000000
>      00000006:ffffffff -> 00000000:00000000:00000000:00000000
>   -  00000007:00000000 -> 00000000:009c2fbb:00000000:9c000400
>   +  00000007:00000000 -> 00000000:009c6fbb:00000000:9c000400
>      00000008:ffffffff -> 00000000:00000000:00000000:00000000
>      00000009:ffffffff -> 00000000:00000000:00000000:00000000
>      0000000a:ffffffff -> 00000000:00000000:00000000:00000000
>      0000000b:ffffffff -> 00000000:00000000:00000000:00000000
>      0000000c:ffffffff -> 00000000:00000000:00000000:00000000
>   -  0000000d:00000000 -> 00000007:00000240:00000340:00000000
>   +  0000000d:00000000 -> 0000001f:00000240:00000440:00000000
>      0000000d:00000001 -> 0000000f:00000240:00000000:00000000
>      0000000d:00000002 -> 00000100:00000240:00000000:00000000
>   +  0000000d:00000003 -> 00000040:000003c0:00000000:00000000
>   +  0000000d:00000004 -> 00000040:00000400:00000000:00000000
>      40000000:ffffffff -> 40000005:566e6558:65584d4d:4d4d566e
>      40000001:ffffffff -> 0004000e:00000000:00000000:00000000
>      40000002:ffffffff -> 00000001:40000000:00000000:00000000
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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


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

Re: [Xen-devel] [PATCH 01/10] x86/sysctl: Don't return cpu policy data for compiled-out support (2)

[Andrew Cooper]

On 27/02/2020 07:38, Jan Beulich wrote:
> On 26.02.2020 21:22, Andrew Cooper wrote:
>> Just as with c/s 96dc77b4b1 for XEN_SYSCTL_get_cpu_policy,
>> XEN_SYSCTL_get_cpu_featureset needs to become conditional.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> Albeit I'd say "want", not "needs" in the description.

It occurs to me that XEN_SYSCTL_get_cpu_featureset is strictly a subset
of XEN_SYSCTL_get_cpu_policy, and that now I've adjusted the toolstack
onto get_cpu_policy, the sole remaining user is xen-cpuid.

get_cpu_policy already has separate default and max indices, whereas
get_cpu_featureset was written before the need for this has become obvious.

This leads to an asymmetry in xen-cpuid, where the -p (policy) option
provides two more sets of information than the featureset listing.

Instead, I think I'd like to drop XEN_SYSCTL_get_cpu_featureset and
update the sole user to the more complete interface.

Thoughts?

~Andrew

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

Re: [Xen-devel] [PATCH 01/10] x86/sysctl: Don't return cpu policy data for compiled-out support (2)

[Jan Beulich]

On 27.02.2020 10:33, Andrew Cooper wrote:
> On 27/02/2020 07:38, Jan Beulich wrote:
>> On 26.02.2020 21:22, Andrew Cooper wrote:
>>> Just as with c/s 96dc77b4b1 for XEN_SYSCTL_get_cpu_policy,
>>> XEN_SYSCTL_get_cpu_featureset needs to become conditional.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>
>> Albeit I'd say "want", not "needs" in the description.
> 
> It occurs to me that XEN_SYSCTL_get_cpu_featureset is strictly a subset
> of XEN_SYSCTL_get_cpu_policy, and that now I've adjusted the toolstack
> onto get_cpu_policy, the sole remaining user is xen-cpuid.
> 
> get_cpu_policy already has separate default and max indices, whereas
> get_cpu_featureset was written before the need for this has become obvious.
> 
> This leads to an asymmetry in xen-cpuid, where the -p (policy) option
> provides two more sets of information than the featureset listing.
> 
> Instead, I think I'd like to drop XEN_SYSCTL_get_cpu_featureset and
> update the sole user to the more complete interface.

Sounds like a good move to me.

Jan

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

Re: [Xen-devel] [PATCH 02/10] tools/libxc: Simplify xc_get_static_cpu_featuremask()

[Andrew Cooper]

On 27/02/2020 07:47, Jan Beulich wrote:
> On 26.02.2020 21:22, Andrew Cooper wrote:
>> Drop XC_FEATUREMASK_DEEP_FEATURES.  It isn't used by any callers, and unlike
>> the other static masks, won't be of interest to anyone without other pieces of
>> cpuid-autogen.h
>>
>> In xc_get_static_cpu_featuremask(), use a 2d array instead of individually
>> named variables, and drop the switch statement completely.
>>
>> No practical change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with three remarks:
>
>> --- a/tools/libxc/xc_cpuid_x86.c
>> +++ b/tools/libxc/xc_cpuid_x86.c
>> @@ -90,43 +90,23 @@ uint32_t xc_get_cpu_featureset_size(void)
>>  const uint32_t *xc_get_static_cpu_featuremask(
>>      enum xc_static_cpu_featuremask mask)
>>  {
>> -    const static uint32_t known[FEATURESET_NR_ENTRIES] = INIT_KNOWN_FEATURES,
>> -        special[FEATURESET_NR_ENTRIES] = INIT_SPECIAL_FEATURES,
>> -        pv[FEATURESET_NR_ENTRIES] = INIT_PV_FEATURES,
>> -        hvm_shadow[FEATURESET_NR_ENTRIES] = INIT_HVM_SHADOW_FEATURES,
>> -        hvm_hap[FEATURESET_NR_ENTRIES] = INIT_HVM_HAP_FEATURES,
>> -        deep_features[FEATURESET_NR_ENTRIES] = INIT_DEEP_FEATURES;
>> -
>> -    BUILD_BUG_ON(ARRAY_SIZE(known) != FEATURESET_NR_ENTRIES);
>> -    BUILD_BUG_ON(ARRAY_SIZE(special) != FEATURESET_NR_ENTRIES);
>> -    BUILD_BUG_ON(ARRAY_SIZE(pv) != FEATURESET_NR_ENTRIES);
>> -    BUILD_BUG_ON(ARRAY_SIZE(hvm_shadow) != FEATURESET_NR_ENTRIES);
>> -    BUILD_BUG_ON(ARRAY_SIZE(hvm_hap) != FEATURESET_NR_ENTRIES);
>> -    BUILD_BUG_ON(ARRAY_SIZE(deep_features) != FEATURESET_NR_ENTRIES);
>> -
>> -    switch ( mask )
>> -    {
>> -    case XC_FEATUREMASK_KNOWN:
>> -        return known;
>> -
>> -    case XC_FEATUREMASK_SPECIAL:
>> -        return special;
>> -
>> -    case XC_FEATUREMASK_PV:
>> -        return pv;
>> +    const static uint32_t masks[][FEATURESET_NR_ENTRIES] = {
> Would you mind switching to the more conventional "static const"?

Ok.

>
>> +#define MASK(x) [XC_FEATUREMASK_ ## x] = INIT_ ## x ## _FEATURES
> I'm surprised to see you introduce such a construct, when more
> than once I heard you say that you dislike macros using ## in
> ways like it is done here.

It is all about positioning.

Like this, the details are always visible (and clear) to anyone
inspecting the function, because the macro only exists adjacent to its
use.  It is also not an interesting function (logic wise), so the fact
it doesn't show up on trivial greps is a lesser evil, compared to the
opencoded version.

My issue with ## generally is that its use obfuscates logic, both in
terms of hiding the operation going on, and making it difficult to
locate related code patterns.

>
>> +    BUILD_BUG_ON(ARRAY_SIZE(masks[0]) != FEATURESET_NR_ENTRIES);
> Isn't this quite pointless with the now changed definition of
> the array?

I'd need to double check, but I suspect it is still necessary (in terms
of the condition which might cause it to trip - whether this condition
is an interesting thing to break the build over might be a different story).

~Andrew

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

Re: [Xen-devel] [PATCH 03/10] x86/gen-cpuid: Rework internal logic to ease future changes

[Andrew Cooper]

On 27/02/2020 07:57, Jan Beulich wrote:
> On 26.02.2020 21:22, Andrew Cooper wrote:
>> Better split the logic between parse/calculate/write.  Collect the feature
>> comment by their comment character, and perform the accumulation operations in
>> crunch_numbers().
> Would you mind saying "character(s)" here, as there are items with
> multiple of them?

Ok.

>
>> Avoid rendering the featuresets to C uint32_t's in crunch_numbers(), and
>> instead do this in write_results().  Update format_uint32s() to call
>> featureset_to_uint32s() internally.
>>
>> No functional change - the generated cpuid-autogen.h is identical.
> I notice the "special" field (or however such is called in Python)
> goes away, in favor of using raw['!'] at the apparently sole
> consuming site. I also notice the same isn't true for "pv", which
> now could also be raw['A'] as it seems. If this is the case (i.e.
> I'm not overlooking anything), could you say a word on the change
> for "special" and/or the difference between "special" and "pv"?

There is no point copying data just for the sake of copying data.

While we could drop state.pv (pv_def by the end of the series), that is
the only set it would be true for, and dropping it does interfere with
the derivation of raw_shadow (raw_shadow_def by the end of the series).

~Andrew

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

Re: [Xen-devel] [PATCH 04/10] x86/gen-cpuid: Create max and default variations of INIT_*_FEATURES

[Andrew Cooper]

On 27/02/2020 08:02, Jan Beulich wrote:
> On 26.02.2020 21:22, Andrew Cooper wrote:
>> For now, write the same content for both.  Update the users of the
>> initialisers to use the new name, and extend xen-cpuid to dump both default
>> and max featuresets.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Hypervisor and libxc parts
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Which other bit are you concerned with?  xen-cpuid.c is explicitly under
x86 maintainership.

>
>> --- a/tools/misc/xen-cpuid.c
>> +++ b/tools/misc/xen-cpuid.c
>> @@ -283,12 +283,18 @@ static void dump_info(xc_interface *xch, bool detail)
>>                        nr_features, "Known", detail);
>>      decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_SPECIAL),
>>                        nr_features, "Special", detail);
>> -    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_PV),
>> -                      nr_features, "PV Mask", detail);
>> -    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_SHADOW),
>> -                      nr_features, "HVM Shadow Mask", detail);
>> -    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_HAP),
>> -                      nr_features, "HVM Hap Mask", detail);
>> +    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_PV_MAX),
>> +                      nr_features, "PV Max", detail);
>> +    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_PV_DEF),
>> +                      nr_features, "PV Default", detail);
>> +    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_SHADOW_MAX),
>> +                      nr_features, "HVM Shadow Max", detail);
>> +    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_SHADOW_DEF),
>> +                      nr_features, "HVM Shadow Default", detail);
>> +    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_HAP_MAX),
>> +                      nr_features, "HVM Hap Max", detail);
>> +    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_HAP_DEF),
>> +                      nr_features, "HVM Hap Default", detail);
> Spotting differences between max and default this way is, I assume,
> going to be quite difficult / error prone.

Not any more or less than between other similar sets (most obviously,
hap and shadow, but raw and host also tend to fairly similar).

> Wouldn't it be better to
> produce the default set in full, and then list just the extra items
> in max?

I don't see how that would work.  The sets are either rendered as a hex
bitmap (so spotting a different is fairly easy), or tabulated with
feature names subdivided by word.

> Aiui max is always going to be a superset of def.

It is.  I did consider distinguishing using lower and upper case, which
is about the only way I can think of sensibly merging the two sets together.

However, this is a pain to do in C, and it would result in the set being
rendered differently depending on whether it was a static set, or a
user-provided one.  It would also result in the case being inverted
compared to the annotation character.

For now, I'm honestly not sure that it matters too much.  I'm probably
going to give xen-cpuid an overhaul anyway (perhaps into python) to be a
more useful calculator for policy settings.

~Andrew

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

Re: [Xen-devel] [PATCH 04/10] x86/gen-cpuid: Create max and default variations of INIT_*_FEATURES

[Jan Beulich]

On 27.02.2020 11:29, Andrew Cooper wrote:
> On 27/02/2020 08:02, Jan Beulich wrote:
>> On 26.02.2020 21:22, Andrew Cooper wrote:
>>> For now, write the same content for both.  Update the users of the
>>> initialisers to use the new name, and extend xen-cpuid to dump both default
>>> and max featuresets.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Hypervisor and libxc parts
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Which other bit are you concerned with?  xen-cpuid.c is explicitly under
> x86 maintainership.
> 
>>
>>> --- a/tools/misc/xen-cpuid.c
>>> +++ b/tools/misc/xen-cpuid.c
>>> @@ -283,12 +283,18 @@ static void dump_info(xc_interface *xch, bool detail)
>>>                        nr_features, "Known", detail);
>>>      decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_SPECIAL),
>>>                        nr_features, "Special", detail);
>>> -    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_PV),
>>> -                      nr_features, "PV Mask", detail);
>>> -    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_SHADOW),
>>> -                      nr_features, "HVM Shadow Mask", detail);
>>> -    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_HAP),
>>> -                      nr_features, "HVM Hap Mask", detail);
>>> +    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_PV_MAX),
>>> +                      nr_features, "PV Max", detail);
>>> +    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_PV_DEF),
>>> +                      nr_features, "PV Default", detail);
>>> +    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_SHADOW_MAX),
>>> +                      nr_features, "HVM Shadow Max", detail);
>>> +    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_SHADOW_DEF),
>>> +                      nr_features, "HVM Shadow Default", detail);
>>> +    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_HAP_MAX),
>>> +                      nr_features, "HVM Hap Max", detail);
>>> +    decode_featureset(xc_get_static_cpu_featuremask(XC_FEATUREMASK_HVM_HAP_DEF),
>>> +                      nr_features, "HVM Hap Default", detail);
>> Spotting differences between max and default this way is, I assume,
>> going to be quite difficult / error prone.
> 
> Not any more or less than between other similar sets (most obviously,
> hap and shadow, but raw and host also tend to fairly similar).

Well, yes, but it may matter more for the (def,max) pairs.

>> Wouldn't it be better to
>> produce the default set in full, and then list just the extra items
>> in max?
> 
> I don't see how that would work.  The sets are either rendered as a hex
> bitmap (so spotting a different is fairly easy), or tabulated with
> feature names subdivided by word.

For the hex printing I agree it's fine this way. For the tabulated
printing, why not simply mask out all "default" bits from "max"
before producing the output, adjusting the headline accordingly?

>> Aiui max is always going to be a superset of def.
> 
> It is.  I did consider distinguishing using lower and upper case, which
> is about the only way I can think of sensibly merging the two sets together.
> 
> However, this is a pain to do in C, and it would result in the set being
> rendered differently depending on whether it was a static set, or a
> user-provided one.  It would also result in the case being inverted
> compared to the annotation character.
> 
> For now, I'm honestly not sure that it matters too much.  I'm probably
> going to give xen-cpuid an overhaul anyway (perhaps into python) to be a
> more useful calculator for policy settings.

Oh, okay. In that case perhaps indeed not worth to spend too much
effort here anymore.

Jan

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

Re: [Xen-devel] [PATCH 05/10] x86/msr: Compile out unused logic/objects

[Andrew Cooper]

On 27/02/2020 08:07, Jan Beulich wrote:
> On 26.02.2020 21:22, Andrew Cooper wrote:
>> @@ -76,16 +77,27 @@ void __init init_guest_msr_policy(void)
>>  {
>>      calculate_raw_policy();
>>      calculate_host_policy();
>> -    calculate_hvm_max_policy();
>> -    calculate_pv_max_policy();
>> +
>> +    if ( IS_ENABLED(CONFIG_PV) )
>> +        calculate_pv_max_policy();
>> +
>> +    if ( hvm_enabled )
>
> Any chance of talking you into doing things more symmetrically,
> by either also using IS_ENABLED(CONFIG_HVM) here or ...
>
>> +        calculate_hvm_max_policy();
>>  }
>>  
>>  int init_domain_msr_policy(struct domain *d)
>>  {
>> -    struct msr_policy *mp =
>> -        xmemdup(is_pv_domain(d) ?  &pv_max_msr_policy
>> -                                : &hvm_max_msr_policy);
>> +    struct msr_policy *mp = is_pv_domain(d)
>> +        ? (IS_ENABLED(CONFIG_PV)  ?  &pv_max_msr_policy : NULL)
>> +        : (IS_ENABLED(CONFIG_HVM) ? &hvm_max_msr_policy : NULL);
> ... (imo preferably) hvm_enabled here? Either way
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

The asymmetry is deliberate.

In the former hunk, hvm_enabled is short-circuited to false for
!CONFIG_HVM, and if I don't use hvm_enabled, here, then I've got to
retain the logic at the top of calculate_hvm_max_policy().  That seems
silly.

In this later hunk, we are looking for the most efficient way to allow
the compiler to discard the reference to hvm_max_msr_policy.  Using
hvm_enabled would be logically equivalent, but compile to more code in
CONFIG_HVM case, as it is a real boolean needing checking.

~Andrew

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

Re: [Xen-devel] [PATCH 08/10] x86/cpuid: Introduce and use default CPUID policies

[Andrew Cooper]

On 27/02/2020 08:19, Jan Beulich wrote:
> On 26.02.2020 21:22, Andrew Cooper wrote:
>> For now, the default and max policies remain identical, but this will change
>> in the future.  Write calculate_{pv,hvm}_def_policy() in a way which will cope
>> with simple feature differences for now.
>>
>> Update XEN_SYSCTL_get_cpu_policy and init_domain_cpuid_policy() to use the
>> default policies.
> For the sysctl the statement looks to be broader than reality,
> as (of course) you don't touch XEN_SYSCTL_cpu_policy_*_max.

I'm afraid I don't understand what you mean here.  What would I need to
touch in XEN_SYSCTL_cpu_policy_*_max at all?

>> @@ -381,6 +386,23 @@ static void __init calculate_pv_max_policy(void)
>>      p->extd.raw[0xa] = EMPTY_LEAF; /* No SVM for PV guests. */
>>  }
>>  
>> +static void __init calculate_pv_def_policy(void)
>> +{
>> +    struct cpuid_policy *p = &pv_def_cpuid_policy;
>> +    uint32_t pv_featureset[FSCAPINTS];
>> +    unsigned int i;
>> +
>> +    *p = pv_max_cpuid_policy;
>> +    cpuid_policy_to_featureset(p, pv_featureset);
>> +
>> +    for ( i = 0; i < ARRAY_SIZE(pv_featureset); ++i )
>> +        pv_featureset[i] &= pv_def_featuremask[i];
>> +
>> +    sanitise_featureset(pv_featureset);
>> +    cpuid_featureset_to_policy(pv_featureset, p);
>> +    recalculate_xstate(p);
>> +}
> Is there a reason the call to guest_common_feature_adjustments()
> is missing here?

Yes, for the same reason that other logic is dropped.  Inheriting from
pv_max_cpuid_policy means that it has already been run on this object.

The host to *_max derivation is non-trivial.  Some features get added
in, others are conditional on external factors.  The *_max to *_def
derivation is much more simple in comparison.

Long term, I expect this logic to move into libx86 and further simplify
cpuid.c

However, I'm not sure why guest_common_feature_adjustments() is special
compared to the other removed logic, and why it should be called out.

~Andrew

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

Re: [Xen-devel] [PATCH 05/10] x86/msr: Compile out unused logic/objects

[Jan Beulich]

On 27.02.2020 11:37, Andrew Cooper wrote:
> On 27/02/2020 08:07, Jan Beulich wrote:
>> On 26.02.2020 21:22, Andrew Cooper wrote:
>>> @@ -76,16 +77,27 @@ void __init init_guest_msr_policy(void)
>>>  {
>>>      calculate_raw_policy();
>>>      calculate_host_policy();
>>> -    calculate_hvm_max_policy();
>>> -    calculate_pv_max_policy();
>>> +
>>> +    if ( IS_ENABLED(CONFIG_PV) )
>>> +        calculate_pv_max_policy();
>>> +
>>> +    if ( hvm_enabled )
>>
>> Any chance of talking you into doing things more symmetrically,
>> by either also using IS_ENABLED(CONFIG_HVM) here or ...
>>
>>> +        calculate_hvm_max_policy();
>>>  }
>>>  
>>>  int init_domain_msr_policy(struct domain *d)
>>>  {
>>> -    struct msr_policy *mp =
>>> -        xmemdup(is_pv_domain(d) ?  &pv_max_msr_policy
>>> -                                : &hvm_max_msr_policy);
>>> +    struct msr_policy *mp = is_pv_domain(d)
>>> +        ? (IS_ENABLED(CONFIG_PV)  ?  &pv_max_msr_policy : NULL)
>>> +        : (IS_ENABLED(CONFIG_HVM) ? &hvm_max_msr_policy : NULL);
>> ... (imo preferably) hvm_enabled here? Either way
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> The asymmetry is deliberate.
> 
> In the former hunk, hvm_enabled is short-circuited to false for
> !CONFIG_HVM, and if I don't use hvm_enabled, here, then I've got to
> retain the logic at the top of calculate_hvm_max_policy().  That seems
> silly.
> 
> In this later hunk, we are looking for the most efficient way to allow
> the compiler to discard the reference to hvm_max_msr_policy.  Using
> hvm_enabled would be logically equivalent, but compile to more code in
> CONFIG_HVM case, as it is a real boolean needing checking.

Fair enough, albeit I don't think the added evaluation of hvm_enabled
would be the end of the world.

Jan

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

Re: [Xen-devel] [PATCH 08/10] x86/cpuid: Introduce and use default CPUID policies

[Jan Beulich]

On 27.02.2020 11:55, Andrew Cooper wrote:
> On 27/02/2020 08:19, Jan Beulich wrote:
>> On 26.02.2020 21:22, Andrew Cooper wrote:
>>> For now, the default and max policies remain identical, but this will change
>>> in the future.  Write calculate_{pv,hvm}_def_policy() in a way which will cope
>>> with simple feature differences for now.
>>>
>>> Update XEN_SYSCTL_get_cpu_policy and init_domain_cpuid_policy() to use the
>>> default policies.
>> For the sysctl the statement looks to be broader than reality,
>> as (of course) you don't touch XEN_SYSCTL_cpu_policy_*_max.
> 
> I'm afraid I don't understand what you mean here.  What would I need to
> touch in XEN_SYSCTL_cpu_policy_*_max at all?

Nothing, and hence my "too broad" response. Only part of
XEN_SYSCTL_get_cpu_policy gets updated. But yes, thinking about it
again, I think I see now how you mean this. So never mind.

>>> @@ -381,6 +386,23 @@ static void __init calculate_pv_max_policy(void)
>>>      p->extd.raw[0xa] = EMPTY_LEAF; /* No SVM for PV guests. */
>>>  }
>>>  
>>> +static void __init calculate_pv_def_policy(void)
>>> +{
>>> +    struct cpuid_policy *p = &pv_def_cpuid_policy;
>>> +    uint32_t pv_featureset[FSCAPINTS];
>>> +    unsigned int i;
>>> +
>>> +    *p = pv_max_cpuid_policy;
>>> +    cpuid_policy_to_featureset(p, pv_featureset);
>>> +
>>> +    for ( i = 0; i < ARRAY_SIZE(pv_featureset); ++i )
>>> +        pv_featureset[i] &= pv_def_featuremask[i];
>>> +
>>> +    sanitise_featureset(pv_featureset);
>>> +    cpuid_featureset_to_policy(pv_featureset, p);
>>> +    recalculate_xstate(p);
>>> +}
>> Is there a reason the call to guest_common_feature_adjustments()
>> is missing here?
> 
> Yes, for the same reason that other logic is dropped.  Inheriting from
> pv_max_cpuid_policy means that it has already been run on this object.
> 
> The host to *_max derivation is non-trivial.  Some features get added
> in, others are conditional on external factors.  The *_max to *_def
> derivation is much more simple in comparison.
> 
> Long term, I expect this logic to move into libx86 and further simplify
> cpuid.c
> 
> However, I'm not sure why guest_common_feature_adjustments() is special
> compared to the other removed logic, and why it should be called out.

Well, the oddity isn't with removed logic (and in fact in this patch I
can't see much of a removal of anything), but with the call being there
in calculate_hvm_def_policy(). This difference, if intentional, is what
I think needs calling out.

Jan

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

Re: [Xen-devel] [PATCH 01/10] x86/sysctl: Don't return cpu policy data for compiled-out support (2)

[Andrew Cooper]

On 27/02/2020 09:40, Jan Beulich wrote:
> On 27.02.2020 10:33, Andrew Cooper wrote:
>> On 27/02/2020 07:38, Jan Beulich wrote:
>>> On 26.02.2020 21:22, Andrew Cooper wrote:
>>>> Just as with c/s 96dc77b4b1 for XEN_SYSCTL_get_cpu_policy,
>>>> XEN_SYSCTL_get_cpu_featureset needs to become conditional.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> Albeit I'd say "want", not "needs" in the description.
>> It occurs to me that XEN_SYSCTL_get_cpu_featureset is strictly a subset
>> of XEN_SYSCTL_get_cpu_policy, and that now I've adjusted the toolstack
>> onto get_cpu_policy, the sole remaining user is xen-cpuid.
>>
>> get_cpu_policy already has separate default and max indices, whereas
>> get_cpu_featureset was written before the need for this has become obvious.
>>
>> This leads to an asymmetry in xen-cpuid, where the -p (policy) option
>> provides two more sets of information than the featureset listing.
>>
>> Instead, I think I'd like to drop XEN_SYSCTL_get_cpu_featureset and
>> update the sole user to the more complete interface.
> Sounds like a good move to me.

Actually, after having spent almost a day trying to disentangle the
remains of this, I'm going to leave it for now.

It turns out it is still used by the legacy CPUID logic, and I could do
with getting a few other pieces of infrastructure before it is easy to
take out.

~Andrew

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

Re: [Xen-devel] [PATCH 02/10] tools/libxc: Simplify xc_get_static_cpu_featuremask()

[Andrew Cooper]

On 27/02/2020 09:55, Andrew Cooper wrote:
>>> +    BUILD_BUG_ON(ARRAY_SIZE(masks[0]) != FEATURESET_NR_ENTRIES);
>> Isn't this quite pointless with the now changed definition of
>> the array?
> I'd need to double check

Double check says it isn't necessary.  I'll drop.

~Andrew

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