[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v6 10/12] libs/{light,guest}: implement xc_cpuid_apply_policy in libxl
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.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |