diff mbox series

[20/21] libs/{light,guest}: implement xc_cpuid_apply_policy in libxl

Message ID 20210323095849.37858-21-roger.pau@citrix.com (mailing list archive)
State Superseded
Headers show
Series libs/guest: new CPUID/MSR interface | expand

Commit Message

Roger Pau Monne March 23, 2021, 9:58 a.m. UTC
With the addition of the xc_cpu_policy_* now libxl can have better
control over the cpu policy, this allows removing the
xc_cpuid_apply_policy function and instead coding the required bits by
libxl in libxl__cpuid_legacy directly.

Remove xc_cpuid_apply_policy.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
I'm unsure why this is called libxl__cpuid_legacy, mostly because I
don't seem to be able to spot a libxl__cpuid (ie: non-legacy version).
Should this be renamed to libxl__cpuid?

1b3cec69bf300e012a mentions the process of switching to a new cpuid
interface, but I'm not sure we need to keep libxl__cpuid_legacy, since
that's an internal interface that's not exposed to libxl clients
anyway.
---
 tools/include/xenctrl.h         |  18 -----
 tools/libs/guest/xg_cpuid_x86.c | 123 --------------------------------
 tools/libs/light/libxl_cpuid.c  |  87 ++++++++++++++++++++--
 3 files changed, 83 insertions(+), 145 deletions(-)

Comments

Andrew Cooper April 1, 2021, 5:44 p.m. UTC | #1
On 23/03/2021 09:58, Roger Pau Monne wrote:
> With the addition of the xc_cpu_policy_* now libxl can have better
> control over the cpu policy, this allows removing the
> xc_cpuid_apply_policy function and instead coding the required bits by
> libxl in libxl__cpuid_legacy directly.
>
> Remove xc_cpuid_apply_policy.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> I'm unsure why this is called libxl__cpuid_legacy, mostly because I
> don't seem to be able to spot a libxl__cpuid (ie: non-legacy version).
> Should this be renamed to libxl__cpuid?
>
> 1b3cec69bf300e012a mentions the process of switching to a new cpuid
> interface, but I'm not sure we need to keep libxl__cpuid_legacy, since
> that's an internal interface that's not exposed to libxl clients
> anyway.

"legacy" was referring to the pre-4.14 migrate-in problem.  It was the
best I could come up with without having a firm plan of how this series
was going to look like.

That said, the resulting function is still very definitely doing thing's
I'd consider "legacy", so perhaps it wants to stay named like this until
we've got a better plan for how to enable non-trivial features.

~Andrew
Roger Pau Monne April 9, 2021, 2:57 p.m. UTC | #2
On Thu, Apr 01, 2021 at 06:44:02PM +0100, Andrew Cooper wrote:
> On 23/03/2021 09:58, Roger Pau Monne wrote:
> > With the addition of the xc_cpu_policy_* now libxl can have better
> > control over the cpu policy, this allows removing the
> > xc_cpuid_apply_policy function and instead coding the required bits by
> > libxl in libxl__cpuid_legacy directly.
> >
> > Remove xc_cpuid_apply_policy.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > I'm unsure why this is called libxl__cpuid_legacy, mostly because I
> > don't seem to be able to spot a libxl__cpuid (ie: non-legacy version).
> > Should this be renamed to libxl__cpuid?
> >
> > 1b3cec69bf300e012a mentions the process of switching to a new cpuid
> > interface, but I'm not sure we need to keep libxl__cpuid_legacy, since
> > that's an internal interface that's not exposed to libxl clients
> > anyway.
> 
> "legacy" was referring to the pre-4.14 migrate-in problem.  It was the
> best I could come up with without having a firm plan of how this series
> was going to look like.
> 
> That said, the resulting function is still very definitely doing thing's
> I'd consider "legacy", so perhaps it wants to stay named like this until
> we've got a better plan for how to enable non-trivial features.

I don't have a strong opinion either way, but as said this is an
internal libxl function, so naming it libxl__cpuid or some such and go
expanding it as required would be perfectly fine IMO, even if the
current parameters are going to change as part of expanding it.

In any case we would not have a libxl__cpuid_legacy and a libxl__cpuid
at the same time in libxl, as those are internal helpers so at some
point when we are happy with the logic in there the _legacy suffix
would just be dropped.

Thanks, Roger.
diff mbox series

Patch

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index a830fac1d12..5a576f72b4d 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -1890,24 +1890,6 @@  struct xc_xend_cpuid {
     char *policy[4];
 };
 
-/*
- * Make adjustments to the CPUID settings for a domain.
- *
- * This path is used in two cases.  First, for fresh boots of the domain, and
- * secondly for migrate-in/restore of pre-4.14 guests (where CPUID data was
- * missing from the stream).  The @restore parameter distinguishes these
- * cases, and the generated policy must be compatible with a 4.13.
- *
- * Either pass a full new @featureset (and @nr_features), or adjust individual
- * features (@pae, @itsc, @nested_virt).
- *
- * Then (optionally) apply legacy XEND overrides (@xend) to the result.
- */
-int xc_cpuid_apply_policy(xc_interface *xch,
-                          uint32_t domid, bool restore,
-                          const uint32_t *featureset,
-                          unsigned int nr_features, bool pae, bool itsc,
-                          bool nested_virt, const struct xc_xend_cpuid *xend);
 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/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 96b969342fa..dffb9923b33 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -383,129 +383,6 @@  int xc_cpu_policy_apply_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
     return rc;
 }
 
-int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
-                          const uint32_t *featureset, unsigned int nr_features,
-                          bool pae, bool itsc, bool nested_virt,
-                          const struct xc_xend_cpuid *cpuid)
-{
-    int rc;
-    xc_dominfo_t di;
-    unsigned int nr_leaves, nr_msrs;
-    xen_cpuid_leaf_t *leaves = NULL;
-    struct cpuid_policy *p = NULL;
-    struct cpu_policy policy = { };
-    uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
-
-    if ( xc_domain_getinfo(xch, domid, 1, &di) != 1 ||
-         di.domid != domid )
-    {
-        ERROR("Failed to obtain d%d info", domid);
-        rc = -ESRCH;
-        goto out;
-    }
-
-    rc = xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs);
-    if ( rc )
-    {
-        PERROR("Failed to obtain policy info size");
-        rc = -errno;
-        goto out;
-    }
-
-    rc = -ENOMEM;
-    if ( (leaves = calloc(nr_leaves, sizeof(*leaves))) == NULL ||
-         (p = calloc(1, sizeof(*p))) == NULL )
-        goto out;
-
-    /* Get the domain's default policy. */
-    nr_msrs = 0;
-    rc = get_system_cpu_policy(xch, di.hvm ? XEN_SYSCTL_cpu_policy_hvm_default
-                                           : XEN_SYSCTL_cpu_policy_pv_default,
-                               &nr_leaves, leaves, &nr_msrs, NULL);
-    if ( rc )
-    {
-        PERROR("Failed to obtain %s default policy", di.hvm ? "hvm" : "pv");
-        rc = -errno;
-        goto out;
-    }
-
-    rc = x86_cpuid_copy_from_buffer(p, leaves, nr_leaves,
-                                    &err_leaf, &err_subleaf);
-    if ( rc )
-    {
-        ERROR("Failed to deserialise CPUID (err leaf %#x, subleaf %#x) (%d = %s)",
-              err_leaf, err_subleaf, -rc, strerror(-rc));
-        goto out;
-    }
-
-    /*
-     * Account for feature which have been disabled by default since Xen 4.13,
-     * so migrated-in VM's don't risk seeing features disappearing.
-     */
-    if ( restore )
-    {
-        policy.cpuid = p;
-        xc_cpu_policy_make_compatible(xch, &policy, di.hvm);
-    }
-
-    if ( featureset )
-    {
-        policy.cpuid = p;
-        rc = xc_cpu_policy_apply_featureset(xch, &policy, featureset,
-                                            nr_features);
-        if ( rc )
-        {
-            ERROR("Failed to apply featureset to policy");
-            goto out;
-        }
-    }
-    else
-    {
-        p->extd.itsc = itsc;
-
-        if ( di.hvm )
-        {
-            p->basic.pae = pae;
-            p->basic.vmx = nested_virt;
-            p->extd.svm = nested_virt;
-        }
-    }
-
-    policy.cpuid = p;
-    rc = xc_cpu_policy_topology(xch, &policy, di.hvm);
-    if ( rc )
-        goto out;
-
-    rc = xc_cpu_policy_apply_cpuid(xch, &policy, cpuid, di.hvm);
-    if ( rc )
-        goto out;
-
-    rc = x86_cpuid_copy_to_buffer(p, leaves, &nr_leaves);
-    if ( rc )
-    {
-        ERROR("Failed to serialise CPUID (%d = %s)", -rc, strerror(-rc));
-        goto out;
-    }
-
-    rc = set_domain_cpu_policy(xch, domid, nr_leaves, leaves, 0, NULL,
-                               &err_leaf, &err_subleaf, &err_msr);
-    if ( rc )
-    {
-        PERROR("Failed to set d%d's policy (err leaf %#x, subleaf %#x, msr %#x)",
-               domid, err_leaf, err_subleaf, err_msr);
-        rc = -errno;
-        goto out;
-    }
-
-    rc = 0;
-
-out:
-    free(p);
-    free(leaves);
-
-    return rc;
-}
-
 xc_cpu_policy_t xc_cpu_policy_init(void)
 {
     xc_cpu_policy_t policy = calloc(1, sizeof(*policy));
diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
index a7b33bbcd06..f1418382b62 100644
--- a/tools/libs/light/libxl_cpuid.c
+++ b/tools/libs/light/libxl_cpuid.c
@@ -423,6 +423,8 @@  int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
                         libxl_domain_build_info *info)
 {
     GC_INIT(ctx);
+    xc_cpu_policy_t policy = NULL;
+    bool hvm = info->type == LIBXL_DOMAIN_TYPE_HVM;
     bool pae = true;
     bool itsc;
     int rc;
@@ -436,6 +438,42 @@  int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
      */
     bool nested_virt = info->nested_hvm.val > 0;
 
+    policy = xc_cpu_policy_init();
+    if (!policy) {
+        LOGE(ERROR, "Failed to init CPU policy");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    rc = xc_cpu_policy_get_domain(ctx->xch, domid, policy);
+    if (rc) {
+        LOGE(ERROR, "Failed to fetch domain %u CPU policy", domid);
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    /*
+     * Account for feature which have been disabled by default since Xen 4.13,
+     * so migrated-in VM's don't risk seeing features disappearing.
+     */
+    if (restore) {
+        rc = xc_cpu_policy_make_compatible(ctx->xch, policy, hvm);
+        if (rc) {
+            LOGE(ERROR, "Failed to setup compatible CPU policy for domain  %u",
+                 domid);
+            rc = ERROR_FAIL;
+            goto out;
+        }
+    }
+
+    rc = xc_cpu_policy_topology(ctx->xch, policy, hvm);
+    if (rc) {
+        LOGE(ERROR, "Failed to setup CPU policy topology for domain  %u",
+             domid);
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
     /*
      * For PV guests, PAE is Xen-controlled (it is the 'p' that differentiates
      * the xen-3.0-x86_32 and xen-3.0-x86_32p ABIs).  It is mandatory as Xen
@@ -446,8 +484,15 @@  int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
      *
      * HVM guests get a top-level choice of whether PAE is available.
      */
-    if (info->type == LIBXL_DOMAIN_TYPE_HVM)
+    if (hvm)
         pae = libxl_defbool_val(info->u.hvm.pae);
+    rc = libxl_cpuid_parse_config(&info->cpuid, GCSPRINTF("pae=%d", pae));
+    if (rc) {
+        LOG(ERROR, "Unable to set PAE CPUID flag: %d", rc);
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
 
     /*
      * Advertising Invariant TSC to a guest means that the TSC frequency won't
@@ -463,12 +508,46 @@  int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
      */
     itsc = (libxl_defbool_val(info->disable_migrate) ||
             info->tsc_mode == LIBXL_TSC_MODE_ALWAYS_EMULATE);
+    rc = libxl_cpuid_parse_config(&info->cpuid, GCSPRINTF("invtsc=%d", itsc));
+    if (rc) {
+        LOG(ERROR, "Unable to set Invariant TSC CPUID flag: %d", rc);
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    /* Set Nested virt CPUID bits for HVM. */
+    if (hvm) {
+        rc = libxl_cpuid_parse_config(&info->cpuid, GCSPRINTF("vmx=%d",
+                                                              nested_virt));
+        if (rc) {
+            LOG(ERROR, "Unable to set VMX CPUID flag: %d", rc);
+            rc = ERROR_FAIL;
+            goto out;
+        }
+
+        rc = libxl_cpuid_parse_config(&info->cpuid, GCSPRINTF("svm=%d",
+                                                              nested_virt));
+        if (rc) {
+            LOG(ERROR, "Unable to set SVM CPUID flag: %d", rc);
+            rc = ERROR_FAIL;
+            goto out;
+        }
+    }
+
+    /* Apply the bits from info->cpuid if any. */
+    rc = xc_cpu_policy_apply_cpuid(ctx->xch, policy, info->cpuid, hvm);
+    if (rc) {
+        LOGE(ERROR, "Failed to apply CPUID changes");
+        rc = ERROR_FAIL;
+        goto out;
+    }
 
-    rc = xc_cpuid_apply_policy(ctx->xch, domid, restore, NULL, 0,
-                               pae, itsc, nested_virt, info->cpuid);
+    rc = xc_cpu_policy_set_domain(ctx->xch, domid, policy);
     if (rc)
-        LOGE(ERROR, "Failed to apply CPUID policy");
+        LOGE(ERROR, "Failed to set domain %u CPUID policy", domid);
 
+ out:
+    xc_cpu_policy_destroy(policy);
     GC_FREE;
     return rc;
 }