[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Xen-devel] [PATCH] x86/cpuid: Untangle Invariant TSC handling



ITSC being visible to the guest is currently implicit with the toolstack
unconditionally asking for it, and Xen clipping it based on the vTSC and/or
XEN_DOMCTL_disable_migrate settings.

This is problematic for several reasons.

First, the implicit vTSC behaviour manifests as a real bug on migration to a
host with a different frequency, with ITSC but without TSC scaling
capabilities, whereby the ITSC feature becomes advertised to the guest.  ITSC
will disappear again if the guest migrates to server with the same frequency
as the original, or to one with TSC scaling support.

Secondly, disallowing ITSC unless the guest doesn't migrate is conceptually
wrong.  It is common to have migration pools of identical hardware, at which
point the TSC frequency is the same, and more modern hardware has TSC scaling
support anyway.  In both cases, it is safe to advertise ITSC and migrate the
guest.

Remove all implicit logic logic in Xen, and make ITSC part of the max CPUID
policies for guests.  Plumb an itsc parameter into xc_cpuid_apply_policy() and
have libxl__cpuid_legacy() fill in the two cases where it can reasonably
expect ITSC to be safe for the guest to see.

This is a behaviour change for TSC_MODE_NATIVE, where the ITSC will now
reliably not appear, and for the case where the user explicitly requests ITSC,
in which case it will appear even if the guest isn't marked as nomigrate.

Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
---
CC: Jan Beulich <JBeulich@xxxxxxxx>
CC: Wei Liu <wl@xxxxxxx>
CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
CC: Ian Jackson <Ian.Jackson@xxxxxxxxxx>
CC: Anthony PERARD <anthony.perard@xxxxxxxxxx>
---
 tools/libxc/include/xenctrl.h               |  4 ++--
 tools/libxc/xc_cpuid_x86.c                  | 12 ++++++------
 tools/libxl/libxl_cpuid.c                   | 18 +++++++++++++++++-
 xen/arch/x86/cpuid.c                        |  8 --------
 xen/arch/x86/time.c                         |  2 --
 xen/include/public/arch-x86/cpufeatureset.h |  2 +-
 6 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 8d13a7e20b..80a42776e2 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1802,12 +1802,12 @@ int xc_cpuid_set(xc_interface *xch,
  * Make adjustments to the CPUID settings for a domain.
  *
  * Either pass a full new @featureset (and @nr_features), or adjust individual
- * features (@pae).
+ * features (@pae, @itsc).
  */
 int xc_cpuid_apply_policy(xc_interface *xch,
                           uint32_t domid,
                           const uint32_t *featureset,
-                          unsigned int nr_features, bool pae);
+                          unsigned int nr_features, bool pae, bool itsc);
 int xc_mca_op(xc_interface *xch, struct xen_mc *mc);
 int xc_mca_op_inject_v2(xc_interface *xch, unsigned int flags,
                         xc_cpumap_t cpumap, unsigned int nr_cpus);
diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index f045b03223..35fd36741b 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -438,7 +438,7 @@ int xc_cpuid_set(
 
 int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
                           const uint32_t *featureset, unsigned int nr_features,
-                          bool pae)
+                          bool pae, bool itsc)
 {
     int rc;
     xc_dominfo_t di;
@@ -534,6 +534,8 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
     }
     else
     {
+        p->extd.itsc = itsc;
+
         if ( di.hvm )
             p->basic.pae = pae;
     }
@@ -621,12 +623,10 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t 
domid,
         }
 
         /*
-         * These settings are necessary to cause earlier HVM_PARAM_NESTEDHVM /
-         * XEN_DOMCTL_disable_migrate settings to be reflected correctly in
-         * CPUID.  Xen will discard these bits if configuration hasn't been
-         * set for the domain.
+         * These settings are necessary to cause earlier HVM_PARAM_NESTEDHVM
+         * to be reflected correctly in CPUID.  Xen will discard these bits if
+         * configuration hasn't been set for the domain.
          */
-        p->extd.itsc = true;
         p->basic.vmx = true;
         p->extd.svm = true;
     }
diff --git a/tools/libxl/libxl_cpuid.c b/tools/libxl/libxl_cpuid.c
index b4f6fd590d..715d195a4c 100644
--- a/tools/libxl/libxl_cpuid.c
+++ b/tools/libxl/libxl_cpuid.c
@@ -418,6 +418,7 @@ void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid,
     int i;
     char *cpuid_res[4];
     bool pae = true;
+    bool itsc;
 
     /*
      * For PV guests, PAE is Xen-controlled (it is the 'p' that differentiates
@@ -432,7 +433,22 @@ void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid,
     if (info->type == LIBXL_DOMAIN_TYPE_HVM)
         pae = libxl_defbool_val(info->u.hvm.pae);
 
-    xc_cpuid_apply_policy(ctx->xch, domid, NULL, 0, pae);
+    /*
+     * Advertising Invariant TSC to a guest means that the TSC frequency won't
+     * change at any point in the future.
+     *
+     * We do not have enough information about potential migration
+     * destinations to know whether advertising ITSC is safe, but if the guest
+     * isn't going to migrate, then the current hardware is all that matters.
+     *
+     * Alternatively, an internal property of vTSC is that the values read are
+     * invariant.  Advertise ITSC when we know the domain will have emualted
+     * TSC everywhere it goes.
+     */
+    itsc = (libxl_defbool_val(info->disable_migrate) ||
+            info->tsc_mode == LIBXL_TSC_MODE_ALWAYS_EMULATE);
+
+    xc_cpuid_apply_policy(ctx->xch, domid, NULL, 0, pae, itsc);
 
     if (!cpuid)
         return;
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 6e01394fd2..1f9bab7bc1 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -594,14 +594,6 @@ void recalculate_cpuid_policy(struct domain *d)
     }
 
     /*
-     * ITSC is masked by default (so domains are safe to migrate), but a
-     * toolstack which has configured disable_migrate or vTSC for a domain may
-     * safely select it, and needs a way of doing so.
-     */
-    if ( cpu_has_itsc && (d->disable_migrate || d->arch.vtsc) )
-        __set_bit(X86_FEATURE_ITSC, max_fs);
-
-    /*
      * On hardware with MSR_TSX_CTRL, the admin may have elected to disable
      * TSX and hide the feature bits.  Migrating-in VMs may have been booted
      * pre-mitigation when the TSX features were visbile.
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index bb1b97787f..bdb7979d2a 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -2380,8 +2380,6 @@ int tsc_set_info(struct domain *d,
         }
     }
 
-    recalculate_cpuid_policy(d);
-
     return 0;
 }
 
diff --git a/xen/include/public/arch-x86/cpufeatureset.h 
b/xen/include/public/arch-x86/cpufeatureset.h
index e63a93119e..0e29ca763f 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -241,7 +241,7 @@ XEN_CPUFEATURE(RDPID,         6*32+22) /*A  RDPID 
instruction */
 XEN_CPUFEATURE(CLDEMOTE,      6*32+25) /*A  CLDEMOTE instruction */
 
 /* AMD-defined CPU features, CPUID level 0x80000007.edx, word 7 */
-XEN_CPUFEATURE(ITSC,          7*32+ 8) /*   Invariant TSC */
+XEN_CPUFEATURE(ITSC,          7*32+ 8) /*a  Invariant TSC */
 XEN_CPUFEATURE(EFRO,          7*32+10) /*   APERF/MPERF Read Only interface */
 
 /* AMD-defined CPU features, CPUID level 0x80000008.ebx, word 8 */
-- 
2.11.0


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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.