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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Jan 18, 2022 at 01:37:18PM +0000, Andrew Cooper wrote:
> 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.

I've fixed all the above, but haven't added a default policy parameter
to xc_cpu_policy_apply_cpuid. Let me know how strongly you feel about
that.

Thanks, Roger.