cpu: x86: Add support for adding features to existing CPU models

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@redhat.com>
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

src/cpu/cpu_x86.c

@@ -148,6 +148,17 @@ struct _virCPUx86Model {
     virCPUx86Signatures *signatures;
     virCPUx86Data data;
     GStrv removedFeatures;
+
+    /* Features added to the CPU model after its original version was released.
+     * Such features are not really considered part of the model, 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. This should only be used for features which were always
+     * included in the CPU model by the hypervisor, but libvirt didn't support
+     * them when introducing the CPU model. In other words, they were enabled,
+     * but we ignored them.
+     */
+    GStrv addedFeatures;
 };
 
 typedef struct _virCPUx86Map virCPUx86Map;
@@ -1276,6 +1287,7 @@ x86ModelFree(virCPUx86Model *model)
     virCPUx86SignaturesFree(model->signatures);
     virCPUx86DataClear(&model->data);
     g_strfreev(model->removedFeatures);
+    g_strfreev(model->addedFeatures);
     g_free(model);
 }
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virCPUx86Model, x86ModelFree);
@@ -1291,6 +1303,7 @@ x86ModelCopy(virCPUx86Model *model)
     copy->signatures = virCPUx86SignaturesCopy(model->signatures);
     x86DataCopy(&copy->data, &model->data);
     copy->removedFeatures = g_strdupv(model->removedFeatures);
+    copy->addedFeatures = g_strdupv(model->addedFeatures);
     copy->vendor = model->vendor;
 
     return g_steal_pointer(&copy);
@@ -1596,17 +1609,20 @@ x86ModelParseFeatures(virCPUx86Model *model,
     g_autofree xmlNodePtr *nodes = NULL;
     size_t i;
     size_t nremoved = 0;
+    size_t nadded = 0;
     int n;
 
     if ((n = virXPathNodeSet("./feature", ctxt, &nodes)) <= 0)
         return n;
 
     model->removedFeatures = g_new0(char *, n + 1);
+    model->addedFeatures = g_new0(char *, n + 1);
 
     for (i = 0; i < n; i++) {
         g_autofree char *ftname = NULL;
         virCPUx86Feature *feature;
         virTristateBool rem;
+        virTristateBool added;
 
         if (!(ftname = virXMLPropString(nodes[i], "name"))) {
             virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -1632,10 +1648,21 @@ x86ModelParseFeatures(virCPUx86Model *model,
             continue;
         }
 
+        if (virXMLPropTristateBool(nodes[i], "added",
+                                   VIR_XML_PROP_NONE,
+                                   &added) < 0)
+            return -1;
+
+        if (added == VIR_TRISTATE_BOOL_YES) {
+            model->addedFeatures[nadded++] = g_strdup(ftname);
+            continue;
+        }
+
         x86DataAdd(&model->data, &feature->data);
     }
 
     model->removedFeatures = g_renew(char *, model->removedFeatures, nremoved + 1);
+    model->addedFeatures = g_renew(char *, model->addedFeatures, nadded + 1);
 
     return 0;
 }
@@ -3030,11 +3057,18 @@ virCPUx86UpdateLive(virCPUDef *cpu,
         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)
+
+            /* Extra features enabled by the hypervisor are ignored by
+             * check='full' in case they were added to the model later for
+             * backward compatibility with the older definition of the model.
+             */
+            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 +3533,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_INVALID_ARG,
+                       _("unknown CPU model %1$s"), modelName);
+        return -1;
+    }
+
+    *features = (const char **) model->addedFeatures;
+    return 0;
+}
+
+
 struct cpuArchDriver cpuDriverX86 = {
     .name = "x86",
     .arch = archs,

src/cpu/cpu_x86.h

@@ -48,3 +48,6 @@ bool virCPUx86FeatureFilterSelectMSR(const char *name,
 bool virCPUx86FeatureFilterDropMSR(const char *name,
                                    virCPUFeaturePolicy policy,
                                    void *opaque);
+
+int virCPUx86GetAddedFeatures(const char *modelName,
+                              const char * const **features);

src/libvirt_private.syms

@@ -1544,6 +1544,7 @@ virCPUx86DataSetSignature;
 virCPUx86DataSetVendor;
 virCPUx86FeatureFilterDropMSR;
 virCPUx86FeatureFilterSelectMSR;
+virCPUx86GetAddedFeatures;
 
 # datatypes.h
 virConnectClass;