Re: [PATCH v2 2/2] tools/xg: Clean up xend-style overrides for CPU policies

["Roger Pau Monné" <roger.pau@citrix.com>]

On Fri, May 17, 2024 at 05:08:35PM +0100, Alejandro Vallejo wrote:
> Factor out policy getters/setters from both (CPUID and MSR) policy override
> functions. Additionally, use host policy rather than featureset when
> preparing the cur policy, saving one hypercall and several lines of
> boilerplate.
> 
> No functional change intended.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
> v2:
>   * Cosmetic change to comment (// into /**/).
>   * Added missing null pointer check to MSR override function.
> ---
>  tools/libs/guest/xg_cpuid_x86.c | 445 ++++++++++----------------------
>  1 file changed, 130 insertions(+), 315 deletions(-)
> 
> diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
> index 4f4b86b59470..74bca0e65b69 100644
> --- a/tools/libs/guest/xg_cpuid_x86.c
> +++ b/tools/libs/guest/xg_cpuid_x86.c
> @@ -36,6 +36,34 @@ enum {
>  #define bitmaskof(idx)      (1u << ((idx) & 31))
>  #define featureword_of(idx) ((idx) >> 5)
>  
> +static int deserialize_policy(xc_interface *xch, xc_cpu_policy_t *policy)
> +{
> +    uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
> +    int rc;
> +
> +    rc = x86_cpuid_copy_from_buffer(&policy->policy, policy->leaves,
> +                                    policy->nr_leaves, &err_leaf, &err_subleaf);
> +    if ( rc )
> +    {
> +        if ( err_leaf != -1 )
> +            ERROR("Failed to deserialise CPUID (err leaf %#x, subleaf %#x) (%d = %s)",
> +                  err_leaf, err_subleaf, -rc, strerror(-rc));
> +        return rc;
> +    }
> +
> +    rc = x86_msr_copy_from_buffer(&policy->policy, policy->msrs,
> +                                  policy->nr_msrs, &err_msr);
> +    if ( rc )
> +    {
> +        if ( err_msr != -1 )
> +            ERROR("Failed to deserialise MSR (err MSR %#x) (%d = %s)",
> +                  err_msr, -rc, strerror(-rc));
> +        return rc;
> +    }
> +
> +    return 0;
> +}
> +
>  int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps)
>  {
>      struct xen_sysctl sysctl = {};
> @@ -260,102 +288,34 @@ 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, ARRAY_SIZE(p->leaves),
> +                   sizeof(*p->leaves), compare_leaves);
>  }
>  
> -static int xc_cpuid_xend_policy(
> -    xc_interface *xch, uint32_t domid, const struct xc_xend_cpuid *xend)
> +static int xc_cpuid_xend_policy(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;
> -    }
> +    if ( !xend )
> +        return 0;
>  
> -    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++ )
> @@ -384,7 +344,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);
> @@ -394,25 +354,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)
> @@ -426,107 +368,37 @@ 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, ARRAY_SIZE(p->msrs),
> +                   sizeof(*p->msrs), 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,

host and def should likely be const?

> +                         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;
> -    }
> +    if ( !msrs )

Does this build?  Where is 'msrs' defined in this context?  The
function parameter is 'msr' AFAICT.

> +        return 0;

Should we also check for host, def, cur != NULL also?

>  
>      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);
> -        unsigned int i;
> +        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);
>  
>          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++ )
> +        for ( size_t i = 0; i < ARRAY_SIZE(msr->policy) - 1; i++ )
>          {
>              bool val;
>  
> @@ -542,8 +414,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 )
> @@ -553,25 +424,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,
> @@ -583,14 +436,16 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>      int rc;
>      bool hvm;
>      xc_domaininfo_t di;
> -    struct xc_cpu_policy *p = xc_cpu_policy_init();
> -    unsigned int i, nr_leaves = ARRAY_SIZE(p->leaves), nr_msrs = 0;
> -    uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
> -    uint32_t host_featureset[FEATURESET_NR_ENTRIES] = {};
> -    uint32_t len = ARRAY_SIZE(host_featureset);
>  
> -    if ( !p )
> -        return -ENOMEM;
> +    struct xc_cpu_policy *host = xc_cpu_policy_init();
> +    struct xc_cpu_policy *def = xc_cpu_policy_init();

I would be helpful to have some kind of mechanism to allocate + init a
policy at the same time, so that the resulting object could be made
const here.  (Not that you need to do it in this patch).

> +    struct xc_cpu_policy *cur = xc_cpu_policy_init();
> +
> +    if ( !host || !def || !cur )
> +    {
> +        rc = -ENOMEM;
> +        goto out;
> +    }
>  
>      if ( (rc = xc_domain_getinfo_single(xch, domid, &di)) < 0 )
>      {
> @@ -600,21 +455,19 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>      }
>      hvm = di.flags & XEN_DOMINF_hvm_guest;
>  
> -    /* Get the host policy. */
> -    rc = xc_get_cpu_featureset(xch, XEN_SYSCTL_cpu_featureset_host,
> -                               &len, host_featureset);
> -    /* Tolerate "buffer too small", as we've got the bits we need. */
> -    if ( rc && errno != ENOBUFS )
> +    /* Get the raw host policy */
> +    rc = xc_cpu_policy_get_system(xch, XEN_SYSCTL_cpu_policy_host, host);
> +    if ( rc )
>      {
> -        PERROR("Failed to obtain host featureset");
> +        PERROR("Failed to obtain host policy");
>          rc = -errno;
>          goto out;
>      }
>  
>      /* Get the domain's default policy. */
> -    rc = get_system_cpu_policy(xch, hvm ? XEN_SYSCTL_cpu_policy_hvm_default
> -                                        : XEN_SYSCTL_cpu_policy_pv_default,
> -                               &nr_leaves, p->leaves, &nr_msrs, NULL);
> +    rc = xc_cpu_policy_get_system(xch, hvm ? XEN_SYSCTL_cpu_policy_hvm_default
> +                                           : XEN_SYSCTL_cpu_policy_pv_default,
> +                                  def);
>      if ( rc )
>      {
>          PERROR("Failed to obtain %s default policy", hvm ? "hvm" : "pv");
> @@ -622,14 +475,8 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>          goto out;
>      }
>  
> -    rc = x86_cpuid_copy_from_buffer(&p->policy, 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;
> -    }
> +    /* Copy the deserialised default policy to modify it */
> +    memcpy(cur, def, sizeof(*cur));
>  
>      if ( restore )
>      {
> @@ -647,18 +494,18 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>           * - Re-enable features which have become (possibly) off by default.
>           */
>  
> -        p->policy.basic.rdrand = test_bit(X86_FEATURE_RDRAND, host_featureset);
> -        p->policy.feat.hle = test_bit(X86_FEATURE_HLE, host_featureset);
> -        p->policy.feat.rtm = test_bit(X86_FEATURE_RTM, host_featureset);
> +        cur->policy.basic.rdrand = host->policy.basic.rdrand;
> +        cur->policy.feat.hle = host->policy.feat.hle;
> +        cur->policy.feat.rtm = host->policy.feat.rtm;
>  
>          if ( hvm )
>          {
> -            p->policy.feat.mpx = test_bit(X86_FEATURE_MPX, host_featureset);
> +            cur->policy.feat.mpx = host->policy.feat.mpx;
>          }
>  
> -        p->policy.basic.max_leaf = min(p->policy.basic.max_leaf, 0xdu);
> -        p->policy.feat.max_subleaf = 0;
> -        p->policy.extd.max_leaf = min(p->policy.extd.max_leaf, 0x8000001c);
> +        cur->policy.basic.max_leaf = min(cur->policy.basic.max_leaf, 0xdu);
> +        cur->policy.feat.max_subleaf = 0;
> +        cur->policy.extd.max_leaf = min(cur->policy.extd.max_leaf, 0x8000001c);
>      }
>  
>      if ( featureset )
> @@ -666,7 +513,7 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>          uint32_t disabled_features[FEATURESET_NR_ENTRIES],
>              feat[FEATURESET_NR_ENTRIES] = {};
>          static const uint32_t deep_features[] = INIT_DEEP_FEATURES;
> -        unsigned int i, b;
> +        unsigned int b;
>  
>          /*
>           * The user supplied featureset may be shorter or longer than
> @@ -677,14 +524,14 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>  
>          /* Check for truncated set bits. */
>          rc = -EOPNOTSUPP;
> -        for ( i = user_len; i < nr_features; ++i )
> +        for ( size_t i = user_len; i < nr_features; ++i )
>              if ( featureset[i] != 0 )
>                  goto out;
>  
>          memcpy(feat, featureset, sizeof(*featureset) * user_len);
>  
>          /* Disable deep dependencies of disabled features. */
> -        for ( i = 0; i < ARRAY_SIZE(disabled_features); ++i )
> +        for ( size_t i = 0; i < ARRAY_SIZE(disabled_features); ++i )
>              disabled_features[i] = ~feat[i] & deep_features[i];
>  
>          for ( b = 0; b < sizeof(disabled_features) * CHAR_BIT; ++b )
> @@ -695,24 +542,24 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>                   !(dfs = x86_cpu_policy_lookup_deep_deps(b)) )
>                  continue;
>  
> -            for ( i = 0; i < ARRAY_SIZE(disabled_features); ++i )
> +            for ( size_t i = 0; i < ARRAY_SIZE(disabled_features); ++i )

All this loop index type changes could be done as a separate patch,
you are not even touching the surrounding lines.  It adds a lot of
churn to this patch for no reason IMO.

>              {
>                  feat[i] &= ~dfs[i];
>                  disabled_features[i] &= ~dfs[i];
>              }
>          }
>  
> -        x86_cpu_featureset_to_policy(feat, &p->policy);
> +        x86_cpu_featureset_to_policy(feat, &cur->policy);
>      }
>      else
>      {
> -        p->policy.extd.itsc = itsc;
> +        cur->policy.extd.itsc = itsc;
>  
>          if ( hvm )
>          {
> -            p->policy.basic.pae = pae;
> -            p->policy.basic.vmx = nested_virt;
> -            p->policy.extd.svm = nested_virt;
> +            cur->policy.basic.pae = pae;
> +            cur->policy.basic.vmx = nested_virt;
> +            cur->policy.extd.svm = nested_virt;
>          }
>      }
>  
> @@ -722,8 +569,8 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>           * On hardware without CPUID Faulting, PV guests see real topology.
>           * As a consequence, they also need to see the host htt/cmp fields.
>           */
> -        p->policy.basic.htt       = test_bit(X86_FEATURE_HTT, host_featureset);
> -        p->policy.extd.cmp_legacy = test_bit(X86_FEATURE_CMP_LEGACY, host_featureset);
> +        cur->policy.basic.htt       = host->policy.basic.htt;
> +        cur->policy.extd.cmp_legacy = host->policy.extd.cmp_legacy;
>      }
>      else
>      {
> @@ -731,28 +578,28 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>           * Topology for HVM guests is entirely controlled by Xen.  For now, we
>           * hardcode APIC_ID = vcpu_id * 2 to give the illusion of no SMT.
>           */
> -        p->policy.basic.htt = true;
> -        p->policy.extd.cmp_legacy = false;
> +        cur->policy.basic.htt = true;
> +        cur->policy.extd.cmp_legacy = false;
>  
>          /*
>           * Leaf 1 EBX[23:16] is Maximum Logical Processors Per Package.
>           * Update to reflect vLAPIC_ID = vCPU_ID * 2, but make sure to avoid
>           * overflow.
>           */
> -        if ( !p->policy.basic.lppp )
> -            p->policy.basic.lppp = 2;
> -        else if ( !(p->policy.basic.lppp & 0x80) )
> -            p->policy.basic.lppp *= 2;
> +        if ( !cur->policy.basic.lppp )
> +            cur->policy.basic.lppp = 2;
> +        else if ( !(cur->policy.basic.lppp & 0x80) )
> +            cur->policy.basic.lppp *= 2;
>  
> -        switch ( p->policy.x86_vendor )
> +        switch ( cur->policy.x86_vendor )
>          {
>          case X86_VENDOR_INTEL:
> -            for ( i = 0; (p->policy.cache.subleaf[i].type &&
> -                          i < ARRAY_SIZE(p->policy.cache.raw)); ++i )
> +            for ( size_t i = 0; (cur->policy.cache.subleaf[i].type &&
> +                                i < ARRAY_SIZE(cur->policy.cache.raw)); ++i )
>              {
> -                p->policy.cache.subleaf[i].cores_per_package =
> -                    (p->policy.cache.subleaf[i].cores_per_package << 1) | 1;
> -                p->policy.cache.subleaf[i].threads_per_cache = 0;
> +                cur->policy.cache.subleaf[i].cores_per_package =
> +                    (cur->policy.cache.subleaf[i].cores_per_package << 1) | 1;
> +                cur->policy.cache.subleaf[i].threads_per_cache = 0;
>              }
>              break;
>  
> @@ -772,49 +619,45 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>               * apic_id_size values greater than 7.  Limit the value to
>               * 7 for now.
>               */
> -            if ( p->policy.extd.nc < 0x7f )
> +            if ( cur->policy.extd.nc < 0x7f )
>              {
> -                if ( p->policy.extd.apic_id_size != 0 && p->policy.extd.apic_id_size < 0x7 )
> -                    p->policy.extd.apic_id_size++;
> +                if ( cur->policy.extd.apic_id_size != 0 && cur->policy.extd.apic_id_size < 0x7 )

I would split the line while there, it's overly long.

Thanks, Roger.