On Tue, Mar 12, 2024 at 18:06:41 +0100, Jiri Denemark wrote:
> This is not a good idea in general, but we can (and have to) do it in
> specific cases when a feature has always been part of a CPU model in
> hypervisor's definition, but we ignored it and did not include the
> feature in our definition.
>
> Blindly adding the features to the CPU map and not adding them to
> existing CPU models breaks migration between old and new libvirt in both
> directions. New libvirt would complain the features got unexpectedly
> enabled (as they were not mentioned in the incoming domain XML) even
> though they were also enabled on the source and the old libvirt just
> didn't know about them. On the other hand, old libvirt would refuse to
> accept incoming migration of a domain started by new libvirt because the
> domain XML would contain CPU features unknown to the old libvirt.
>
> This is exactly what happened when several vmx-* features were added a
> few releases back. Migration between libvirt releases before and after
> the addition is now broken.
>
> This patch adds support for added these features to existing CPU models
> by marking them with added='yes'. The features will not be considered
> part of the CPU model and will be described explicitly via additional
> <feature/> elements, but the compatibility check will not complain if
> they are enabled by the hypervisor even though they were not explicitly
> mentioned in the CPU definition and incoming migration from old libvirt
> will succeed.
>
> To fix outgoing migration to old libvirt, we also need to drop all those
> features from domain XML unless they were explicitly requested by the
> user. This will be handled by a later patch.
>
> Signed-off-by: Jiri Denemark <jdenemar@xxxxxxxxxx>
> ---
> src/cpu/cpu_x86.c | 69 ++++++++++++++++++++++++++++++++++++++--
> src/cpu/cpu_x86.h | 3 ++
> src/libvirt_private.syms | 1 +
> 3 files changed, 70 insertions(+), 3 deletions(-)
> @@ -3030,11 +3057,13 @@ virCPUx86UpdateLive(virCPUDef *cpu,
[...]
Even in the context of virCPUx86UpdateLive this conidtion is getting a
bit overcrowded with the extra check. Please add a comment at least
stating that this specifically ignores the features added later.
> if (expected == VIR_CPU_FEATURE_DISABLE &&
> x86DataIsSubset(&enabled, &feature->data)) {
> VIR_DEBUG("Feature '%s' enabled by the hypervisor", feature->name);
> - if (cpu->check == VIR_CPU_CHECK_FULL)
> + if (cpu->check == VIR_CPU_CHECK_FULL &&
> + !g_strv_contains((const char **) model->addedFeatures, feature->name)) {
> virBufferAsprintf(&bufAdded, "%s,", feature->name);
> - else if (virCPUDefUpdateFeature(cpu, feature->name,
> - VIR_CPU_FEATURE_REQUIRE) < 0)
> + } else if (virCPUDefUpdateFeature(cpu, feature->name,
> + VIR_CPU_FEATURE_REQUIRE) < 0) {
> return -1;
> + }
> }
>
> if (x86DataIsSubset(&disabled, &feature->data) ||
> @@ -3499,6 +3528,40 @@ virCPUx86FeatureFilterDropMSR(const char *name,
> }
>
>
> +/**
> + * virCPUx86GetAddedFeatures:
> + * @modelName: CPU model
> + * @features: where to store a pointer to the list of added features
> + *
> + * Gets a list of features added to a specified CPU model after its original
> + * version was already released. The @features will be set to NULL if the list
> + * is empty or it will point to internal structures and thus it must not be
> + * freed or modified by the caller. The pointer is valid for the whole lifetime
> + * of the process.
> + *
> + * Returns 0 on success, -1 otherwise.
> + */
> +int
> +virCPUx86GetAddedFeatures(const char *modelName,
> + const char * const **features)
> +{
> + virCPUx86Map *map;
> + virCPUx86Model *model;
> +
> + if (!(map = virCPUx86GetMap()))
> + return -1;
> +
> + if (!(model = x86ModelFind(map, modelName))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unknown CPU model %1$s"), modelName);
While I didn't yet check where this is used using INTERNAL_ERROR for a
CPU model which most likely came from the user is weird. Consider using
VIR_ERR_INVALID_ARG.
Reviewed-by: Peter Krempa <pkrempa@xxxxxxxxxx>
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
