| Message ID | 20210323095849.37858-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libs/guest: new CPUID/MSR interface | expand |
On 23.03.2021 10:58, Roger Pau Monne wrote: > Also change libxl__cpuid_legacy to propagate the error from > xc_cpuid_apply_policy into callers. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> This looks to do what it means to do and also none of the present callers of the functions modified here ignore the return values, so Reviewed-by: Jan Beulich <jbeulich@suse.com> I wonder however how to ensure similar problems won't get re-introduced down the road. In the hypervisor I'd recommend adding __must_check everywhere, but I have no idea what the tool stack policy is in this regard. Jan
On 23/03/2021 09:58, Roger Pau Monne wrote: > Also change libxl__cpuid_legacy to propagate the error from > xc_cpuid_apply_policy into callers. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> This path has never previously had error checking. The set-cpu-policy hypercall, in principle, returns a triple of (leaf, subleaf, msr) to try and help the caller fractionally more than just getting EINVAL, but doesn't actually fail yet for interesting reasons. My plan was only to wire up the error handling with the new interface, which requires plumbing the extra failure information through into suitable locations, and ideally also looking up the offending values to render into error messages. ~Andrew
On 23/03/2021 09:58, Roger Pau Monne wrote: > @@ -462,8 +464,13 @@ void 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); > > - xc_cpuid_apply_policy(ctx->xch, domid, restore, NULL, 0, > - pae, itsc, nested_virt, info->cpuid); > + rc = xc_cpuid_apply_policy(ctx->xch, domid, restore, NULL, 0, > + pae, itsc, nested_virt, info->cpuid); > + if (rc) > + LOGE(ERROR, "Failed to apply CPUID policy"); If we are planning to take this patch, then you need to convert from xc errors (-errno) to libxl errors here, or the caller is going to receive gibberish. ~Andrew > + > + GC_FREE; > + return rc; > }
diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c index 289c59c7420..a7b33bbcd06 100644 --- a/tools/libs/light/libxl_cpuid.c +++ b/tools/libs/light/libxl_cpuid.c @@ -419,11 +419,13 @@ int libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list *cpuid, return 0; } -void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore, - libxl_domain_build_info *info) +int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore, + libxl_domain_build_info *info) { + GC_INIT(ctx); bool pae = true; bool itsc; + int rc; /* * Gross hack. Using libxl_defbool_val() here causes libvirt to crash in @@ -462,8 +464,13 @@ void 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); - xc_cpuid_apply_policy(ctx->xch, domid, restore, NULL, 0, - pae, itsc, nested_virt, info->cpuid); + rc = xc_cpuid_apply_policy(ctx->xch, domid, restore, NULL, 0, + pae, itsc, nested_virt, info->cpuid); + if (rc) + LOGE(ERROR, "Failed to apply CPUID policy"); + + GC_FREE; + return rc; } static const char *input_names[2] = { "leaf", "subleaf" }; diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c index 1131b2a733e..3b7474979de 100644 --- a/tools/libs/light/libxl_create.c +++ b/tools/libs/light/libxl_create.c @@ -1443,6 +1443,7 @@ int libxl__srm_callout_callback_static_data_done(unsigned int missing, libxl_domain_config *d_config = dcs->guest_config; libxl_domain_build_info *info = &d_config->b_info; + int rc = 0; /* * CPUID/MSR information is not present in pre Xen-4.14 streams. @@ -1452,9 +1453,9 @@ int libxl__srm_callout_callback_static_data_done(unsigned int missing, * stream doesn't contain any CPUID data. */ if (missing & XGR_SDD_MISSING_CPUID) - libxl__cpuid_legacy(ctx, dcs->guest_domid, true, info); + rc = libxl__cpuid_legacy(ctx, dcs->guest_domid, true, info); - return 0; + return rc; } void libxl__srm_callout_callback_restore_results(xen_pfn_t store_mfn, diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c index 842a51c86cb..e9f58ee4b2b 100644 --- a/tools/libs/light/libxl_dom.c +++ b/tools/libs/light/libxl_dom.c @@ -384,7 +384,7 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid, * being migrated-in/restored have CPUID handled during the * static_data_done() callback. */ if (!state->restore) - libxl__cpuid_legacy(ctx, domid, false, info); + rc = libxl__cpuid_legacy(ctx, domid, false, info); return rc; } diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h index 028bc013d9c..22b1775b752 100644 --- a/tools/libs/light/libxl_internal.h +++ b/tools/libs/light/libxl_internal.h @@ -2052,8 +2052,8 @@ typedef yajl_gen_status (*libxl__gen_json_callback)(yajl_gen hand, void *); _hidden char *libxl__object_to_json(libxl_ctx *ctx, const char *type, libxl__gen_json_callback gen, void *p); -_hidden void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool retore, - libxl_domain_build_info *info); +_hidden int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool retore, + libxl_domain_build_info *info); /* Calls poll() again - useful to check whether a signaled condition * is still true. Cannot fail. Returns currently-true revents. */ diff --git a/tools/libs/light/libxl_nocpuid.c b/tools/libs/light/libxl_nocpuid.c index f47336565b9..0630959e760 100644 --- a/tools/libs/light/libxl_nocpuid.c +++ b/tools/libs/light/libxl_nocpuid.c @@ -34,9 +34,10 @@ int libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list *cpuid, return 0; } -void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore, - libxl_domain_build_info *info) +int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore, + libxl_domain_build_info *info) { + return 0; } yajl_gen_status libxl_cpuid_policy_list_gen_json(yajl_gen hand,
Also change libxl__cpuid_legacy to propagate the error from xc_cpuid_apply_policy into callers. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- tools/libs/light/libxl_cpuid.c | 15 +++++++++++---- tools/libs/light/libxl_create.c | 5 +++-- tools/libs/light/libxl_dom.c | 2 +- tools/libs/light/libxl_internal.h | 4 ++-- tools/libs/light/libxl_nocpuid.c | 5 +++-- 5 files changed, 20 insertions(+), 11 deletions(-)