diff mbox series

[6/7] tools/xg: Simplify xc_cpuid_xend_policy() and xc_msr_policy()

Message ID 20231107154921.54979-7-alejandro.vallejo@cloud.com (mailing list archive)
State New, archived
Headers show
Series Cleanup and code duplication removal in xenguest | expand

Commit Message

Alejandro Vallejo Nov. 7, 2023, 3:49 p.m. UTC
Remove all duplication in CPUID and MSR xend-style overrides. They had an
incredible amount of overhead for no good reason.

After this patch, CPU policy application takes a number of hypercalls to
recover the policy state and then those are passed to the xend-style
override code so it can avoid them.

Furthermore, the ultimate reapplication of the policy to the domain in Xen
is done only once after both CPUID and MSRs have been fixed up.

BUG!!! apply_policy is sending the policy after deserialise when it poked
at it in its serialised form.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 tools/libs/guest/xg_cpuid_x86.c | 261 +++++---------------------------
 1 file changed, 38 insertions(+), 223 deletions(-)

Comments

Alejandro Vallejo Nov. 22, 2023, 5:41 p.m. UTC | #1
Hi, while rebasing with something else I noticed that...

On Tue, Nov 07, 2023 at 03:49:20PM +0000, Alejandro Vallejo wrote:
> Remove all duplication in CPUID and MSR xend-style overrides. They had an
> incredible amount of overhead for no good reason.
> 
> After this patch, CPU policy application takes a number of hypercalls to
> recover the policy state and then those are passed to the xend-style
> override code so it can avoid them.
> 
> Furthermore, the ultimate reapplication of the policy to the domain in Xen
> is done only once after both CPUID and MSRs have been fixed up.
> 
> BUG!!! apply_policy is sending the policy after deserialise when it poked
> at it in its serialised form.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
>  tools/libs/guest/xg_cpuid_x86.c | 261 +++++---------------------------
>  1 file changed, 38 insertions(+), 223 deletions(-)


there's a blunder in the commit message. This was a note to myself to fix
something while in mid-dev, and I did, but then didn't remove the paragraph
from the commit message.

Obviously those last 2 lines before the S-by shouldn't be there.

Cheers,
Alejandro
diff mbox series

Patch

diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index acc94fb16b..e2a2659953 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -312,102 +312,32 @@  static int compare_leaves(const void *l, const void *r)
     return 0;
 }
 
-static xen_cpuid_leaf_t *find_leaf(
-    xen_cpuid_leaf_t *leaves, unsigned int nr_leaves,
-    const struct xc_xend_cpuid *xend)
+static xen_cpuid_leaf_t *find_leaf(xc_cpu_policy_t *p,
+                                   const struct xc_xend_cpuid *xend)
 {
     const xen_cpuid_leaf_t key = { xend->leaf, xend->subleaf };
 
-    return bsearch(&key, leaves, nr_leaves, sizeof(*leaves), compare_leaves);
+    return bsearch(&key, p->leaves.buf, p->leaves.len,
+                   sizeof(*p->leaves.buf), compare_leaves);
 }
 
 static int xc_cpuid_xend_policy(
-    xc_interface *xch, uint32_t domid, const struct xc_xend_cpuid *xend)
+    xc_interface *xch, uint32_t domid,
+    const struct xc_xend_cpuid *xend,
+    xc_cpu_policy_t *host,
+    xc_cpu_policy_t *def,
+    xc_cpu_policy_t *cur)
 {
-    int rc;
-    bool hvm;
-    xc_domaininfo_t di;
-    unsigned int nr_leaves, nr_msrs;
-    uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
-    /*
-     * Three full policies.  The host, default for the domain type,
-     * and domain current.
-     */
-    xen_cpuid_leaf_t *host = NULL, *def = NULL, *cur = NULL;
-    unsigned int nr_host, nr_def, nr_cur;
-
-    if ( (rc = xc_domain_getinfo_single(xch, domid, &di)) < 0 )
-    {
-        PERROR("Failed to obtain d%d info", domid);
-        rc = -errno;
-        goto fail;
-    }
-    hvm = di.flags & XEN_DOMINF_hvm_guest;
-
-    rc = xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs);
-    if ( rc )
-    {
-        PERROR("Failed to obtain policy info size");
-        rc = -errno;
-        goto fail;
-    }
-
-    rc = -ENOMEM;
-    if ( (host = calloc(nr_leaves, sizeof(*host))) == NULL ||
-         (def  = calloc(nr_leaves, sizeof(*def)))  == NULL ||
-         (cur  = calloc(nr_leaves, sizeof(*cur)))  == NULL )
-    {
-        ERROR("Unable to allocate memory for %u CPUID leaves", nr_leaves);
-        goto fail;
-    }
-
-    /* Get the domain's current policy. */
-    nr_msrs = 0;
-    nr_cur = nr_leaves;
-    rc = get_domain_cpu_policy(xch, domid, &nr_cur, cur, &nr_msrs, NULL);
-    if ( rc )
-    {
-        PERROR("Failed to obtain d%d current policy", domid);
-        rc = -errno;
-        goto fail;
-    }
-
-    /* Get the domain type's default policy. */
-    nr_msrs = 0;
-    nr_def = nr_leaves;
-    rc = get_system_cpu_policy(xch, hvm ? XEN_SYSCTL_cpu_policy_hvm_default
-                                        : XEN_SYSCTL_cpu_policy_pv_default,
-                               &nr_def, def, &nr_msrs, NULL);
-    if ( rc )
-    {
-        PERROR("Failed to obtain %s def policy", hvm ? "hvm" : "pv");
-        rc = -errno;
-        goto fail;
-    }
-
-    /* Get the host policy. */
-    nr_msrs = 0;
-    nr_host = nr_leaves;
-    rc = get_system_cpu_policy(xch, XEN_SYSCTL_cpu_policy_host,
-                               &nr_host, host, &nr_msrs, NULL);
-    if ( rc )
-    {
-        PERROR("Failed to obtain host policy");
-        rc = -errno;
-        goto fail;
-    }
-
-    rc = -EINVAL;
     for ( ; xend->leaf != XEN_CPUID_INPUT_UNUSED; ++xend )
     {
-        xen_cpuid_leaf_t *cur_leaf = find_leaf(cur, nr_cur, xend);
-        const xen_cpuid_leaf_t *def_leaf = find_leaf(def, nr_def, xend);
-        const xen_cpuid_leaf_t *host_leaf = find_leaf(host, nr_host, xend);
+        xen_cpuid_leaf_t *cur_leaf = find_leaf(cur, xend);
+        const xen_cpuid_leaf_t *def_leaf = find_leaf(def, xend);
+        const xen_cpuid_leaf_t *host_leaf = find_leaf(host, xend);
 
         if ( cur_leaf == NULL || def_leaf == NULL || host_leaf == NULL )
         {
             ERROR("Missing leaf %#x, subleaf %#x", xend->leaf, xend->subleaf);
-            goto fail;
+            return -EINVAL;
         }
 
         for ( unsigned int i = 0; i < ARRAY_SIZE(xend->policy); i++ )
@@ -436,7 +366,7 @@  static int xc_cpuid_xend_policy(
                 {
                     ERROR("Bad character '%c' in policy[%d] string '%s'",
                           xend->policy[i][j], i, xend->policy[i]);
-                    goto fail;
+                    return -EINVAL;
                 }
 
                 clear_bit(31 - j, cur_reg);
@@ -446,25 +376,7 @@  static int xc_cpuid_xend_policy(
         }
     }
 
-    /* Feed the transformed currrent policy back up to Xen. */
-    rc = xc_set_domain_cpu_policy(xch, domid, nr_cur, cur, 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 fail;
-    }
-
-    /* Success! */
-
- fail:
-    free(cur);
-    free(def);
-    free(host);
-
-    return rc;
+    return 0;
 }
 
 static int compare_msr(const void *l, const void *r)
@@ -478,104 +390,31 @@  static int compare_msr(const void *l, const void *r)
     return lhs->idx < rhs->idx ? -1 : 1;
 }
 
-static xen_msr_entry_t *find_msr(
-    xen_msr_entry_t *msrs, unsigned int nr_msrs,
-    uint32_t index)
+static xen_msr_entry_t *find_msr(xc_cpu_policy_t *p, uint32_t index)
 {
     const xen_msr_entry_t key = { .idx = index };
 
-    return bsearch(&key, msrs, nr_msrs, sizeof(*msrs), compare_msr);
+    return bsearch(&key, p->msrs.buf, p->msrs.len,
+                   sizeof(*p->msrs.buf), compare_msr);
 }
 
-
 static int xc_msr_policy(xc_interface *xch, domid_t domid,
-                         const struct xc_msr *msr)
+                         const struct xc_msr *msr,
+                         xc_cpu_policy_t *host,
+                         xc_cpu_policy_t *def,
+                         xc_cpu_policy_t *cur)
 {
-    int rc;
-    bool hvm;
-    xc_domaininfo_t di;
-    unsigned int nr_leaves, nr_msrs;
-    uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
-    /*
-     * Three full policies.  The host, default for the domain type,
-     * and domain current.
-     */
-    xen_msr_entry_t *host = NULL, *def = NULL, *cur = NULL;
-    unsigned int nr_host, nr_def, nr_cur;
-
-    if ( (rc = xc_domain_getinfo_single(xch, domid, &di)) < 0 )
-    {
-        PERROR("Failed to obtain d%d info", domid);
-        rc = -errno;
-        goto out;
-    }
-    hvm = di.flags & XEN_DOMINF_hvm_guest;
-
-    rc = xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs);
-    if ( rc )
-    {
-        PERROR("Failed to obtain policy info size");
-        rc = -errno;
-        goto out;
-    }
-
-    if ( (host = calloc(nr_msrs, sizeof(*host))) == NULL ||
-         (def  = calloc(nr_msrs, sizeof(*def)))  == NULL ||
-         (cur  = calloc(nr_msrs, sizeof(*cur)))  == NULL )
-    {
-        ERROR("Unable to allocate memory for %u CPUID leaves", nr_leaves);
-        rc = -ENOMEM;
-        goto out;
-    }
-
-    /* Get the domain's current policy. */
-    nr_leaves = 0;
-    nr_cur = nr_msrs;
-    rc = get_domain_cpu_policy(xch, domid, &nr_leaves, NULL, &nr_cur, cur);
-    if ( rc )
-    {
-        PERROR("Failed to obtain d%d current policy", domid);
-        rc = -errno;
-        goto out;
-    }
-
-    /* Get the domain type's default policy. */
-    nr_leaves = 0;
-    nr_def = nr_msrs;
-    rc = get_system_cpu_policy(xch, hvm ? XEN_SYSCTL_cpu_policy_hvm_default
-                                        : XEN_SYSCTL_cpu_policy_pv_default,
-                               &nr_leaves, NULL, &nr_def, def);
-    if ( rc )
-    {
-        PERROR("Failed to obtain %s def policy", hvm ? "hvm" : "pv");
-        rc = -errno;
-        goto out;
-    }
-
-    /* Get the host policy. */
-    nr_leaves = 0;
-    nr_host = nr_msrs;
-    rc = get_system_cpu_policy(xch, XEN_SYSCTL_cpu_policy_host,
-                               &nr_leaves, NULL, &nr_host, host);
-    if ( rc )
-    {
-        PERROR("Failed to obtain host policy");
-        rc = -errno;
-        goto out;
-    }
-
     for ( ; msr->index != XC_MSR_INPUT_UNUSED; ++msr )
     {
-        xen_msr_entry_t *cur_msr = find_msr(cur, nr_cur, msr->index);
-        const xen_msr_entry_t *def_msr = find_msr(def, nr_def, msr->index);
-        const xen_msr_entry_t *host_msr = find_msr(host, nr_host, msr->index);
+        xen_msr_entry_t *cur_msr = find_msr(cur, msr->index);
+        const xen_msr_entry_t *def_msr = find_msr(def, msr->index);
+        const xen_msr_entry_t *host_msr = find_msr(host, msr->index);
         unsigned int i;
 
         if ( cur_msr == NULL || def_msr == NULL || host_msr == NULL )
         {
             ERROR("Missing MSR %#x", msr->index);
-            rc = -ENOENT;
-            goto out;
+            return -ENOENT;
         }
 
         for ( i = 0; i < ARRAY_SIZE(msr->policy) - 1; i++ )
@@ -594,8 +433,7 @@  static int xc_msr_policy(xc_interface *xch, domid_t domid,
             {
                 ERROR("MSR index %#x: bad character '%c' in policy string '%s'",
                       msr->index, msr->policy[i], msr->policy);
-                rc = -EINVAL;
-                goto out;
+                return -EINVAL;
             }
 
             if ( val )
@@ -605,25 +443,7 @@  static int xc_msr_policy(xc_interface *xch, domid_t domid,
         }
     }
 
-    /* Feed the transformed policy back up to Xen. */
-    rc = xc_set_domain_cpu_policy(xch, domid, 0, NULL, nr_cur, cur,
-                                  &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;
-    }
-
-    /* Success! */
-
- out:
-    free(cur);
-    free(def);
-    free(host);
-
-    return rc;
+    return 0;
 }
 
 int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
@@ -822,25 +642,20 @@  int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
         }
     }
 
-    /* Re-apply the mutated policy onto the domain */
-    rc = xc_cpu_policy_set_domain(xch, domid, cur);
-    if ( rc )
-    {
-        rc = -errno;
+    /*
+     * Ensure changes we just made are reflected in the serialised form of
+     * the policy, as otherwise they they will be missed by the xend overrides
+     */
+    if ( (rc = cpu_policy_serialise_on_self(xch, cur)) )
         goto out;
-    }
 
-    if ( xend && (rc = xc_cpuid_xend_policy(xch, domid, xend)) )
+    /* Apply xend-style overrides */
+    if ( (xend && (rc = xc_cpuid_xend_policy(xch, domid, xend, host, def, cur))) ||
+         (msr && (rc = xc_msr_policy(xch, domid, msr, host, def, cur))) )
         goto out;
 
-    if ( msr )
-    {
-        rc = xc_msr_policy(xch, domid, msr);
-        if ( rc )
-            goto out;
-    }
-
-    rc = 0;
+    /* Re-apply the mutated policy onto the domain */
+    rc = xc_cpu_policy_set_domain_from_serialised(xch, domid, cur);
 
 out:
     xc_cpu_policy_destroy(cur);