Re: [PATCH v6 10/12] libs/{light,guest}: implement xc_cpuid_apply_policy in libxl

[Andrew Cooper <amc96@xxxxxxxx>]

On 17/01/2022 09:48, Roger Pau Monne wrote:
> diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
> index e1acf6648d..7dcdb35a4c 100644
> --- a/tools/libs/light/libxl_cpuid.c
> +++ b/tools/libs/light/libxl_cpuid.c
> @@ -454,6 +456,41 @@ 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) {
> +        LOGED(ERROR, domid, "Failed to init CPU policy");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    r = xc_cpu_policy_get_domain(ctx->xch, domid, policy);
> +    if (r) {
> +        LOGED(ERROR, domid, "Failed to fetch domain CPU policy");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    if (restore) {
> +        /*
> +         * Make sure the policy is compatible with pre Xen 4.13. Note that
> +         * newer Xen versions will pass policy data on the restore stream, so
> +         * any adjustments done here will be superseded.
> +         */
> +        r = xc_cpu_policy_make_compat_4_12(ctx->xch, policy, hvm);

One hidden host policy acquisition, and ...

> +        if (r) {
> +            LOGED(ERROR, domid, "Failed to setup compatible CPU policy");
> +            rc = ERROR_FAIL;
> +            goto out;
> +        }
> +    }
> +
> +    r = xc_cpu_policy_legacy_topology(ctx->xch, policy, hvm);

... one host featureset and ... (final comment)

> +    if (r) {
> +        LOGED(ERROR, domid, "Failed to setup CPU policy topology");
> +        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
> @@ -466,6 +503,13 @@ int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, 
> bool restore,
>       */
>      if (info->type == LIBXL_DOMAIN_TYPE_HVM)
>          pae = libxl_defbool_val(info->u.hvm.pae);
> +    rc = libxl_cpuid_parse_config(&info->cpuid, GCSPRINTF("pae=%d", pae));
> +    if (rc) {
> +        LOGD(ERROR, domid, "Failed to set PAE CPUID flag");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
>  
>      /*
>       * Advertising Invariant TSC to a guest means that the TSC frequency 
> won't
> @@ -481,14 +525,50 @@ 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) {
> +        LOGD(ERROR, domid, "Failed to set Invariant TSC CPUID flag");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
>  
> -    r = xc_cpuid_apply_policy(ctx->xch, domid, restore, NULL, 0,
> -                              pae, itsc, nested_virt, info->cpuid);
> -    if (r)
> -        LOGEVD(ERROR, -r, domid, "Failed to apply CPUID policy");
> +    /* Set Nested virt CPUID bits for HVM. */
> +    if (hvm) {
> +        rc = libxl_cpuid_parse_config(&info->cpuid, GCSPRINTF("vmx=%d",
> +                                                              nested_virt));
> +        if (rc) {
> +            LOGD(ERROR, domid, "Failed to set VMX CPUID flag");
> +            rc = ERROR_FAIL;
> +            goto out;
> +        }
> +
> +        rc = libxl_cpuid_parse_config(&info->cpuid, GCSPRINTF("svm=%d",
> +                                                              nested_virt));
> +        if (rc) {
> +            LOGD(ERROR, domid, "Failed to set SVM CPUID flag");
> +            rc = ERROR_FAIL;
> +            goto out;
> +        }
> +    }

libxl_cpuid_parse_config() is a large overhead, and cannot suffer errors
because libxl crashes rather than failing memory allocations.  The
invasiveness would be reduced substantially by having:

str = GCSPRINTF("pae=%d,invtsc=%d", pae, itsc);
if ( hvm )
    append GCSPRINTF("vmx=%d,svm=%d", nested_virt, nested_virt);

and a single call to libxl_cpuid_parse_config().


Depending on how much we care, these need inserting at the head of the
info->cpuid list so they get processed in the same order as before.

> +
> +    /* Apply the bits from info->cpuid if any. */

'if any' is stale now.

> +    r = xc_cpu_policy_apply_cpuid(ctx->xch, policy, info->cpuid, hvm);

... and both a host and default policy.

That is a lot of overhead added behind the scenes.  It would be rather
better to have this function obtain the host policy and pass it to all 3
helpers.  Default policy is harder to judge, but it would avoid needing
to pass an `hvm` boolean down into this helper.

~Andrew