[Qemu-devel] [PATCH 0/2] i386: "unavailable-features" QOM property

[Qemu-devel] [PATCH 0/2] i386: "unavailable-features" QOM property

[Eduardo Habkost]

Currently, libvirt uses the "filtered-features" QOM property at
runtime to ensure no feature was accidentally disabled on VCPUs
because it's not available on the host.

However, the code for "feature-words" assumes that all missing
features have a corresponding CPUID bit, which is not true for
MSR-based features like the ones at FEAT_ARCH_CAPABILITIES.

We could extend X86CPUFeatureWordInfo to include information
about MSR features, but it's impossible to do that while keeping
compatibility with clients that (reasonably) expect all elements
of "filtered-features" to have the cpuid-* fields.

We have a field in "query-cpu-definitions" that already describes
all features that are missing on a CPU, including MSR features:
CpuDefinitionInfo.unavailable-features.  The existing code for
building the unavailable-features array even uses
X86CPU::filtered_features to build the feature list.

This series adds a "unavailable-features" QOM property to X86CPU
objects that have the same semantics of "unavailable-features" on
query-cpu-definitions.  The new property has the same goal of
"filtered-features", but is generic enough to let any kind of CPU
feature to be listed there without relying on low level details
like CPUID leaves or MSR numbers.

Eduardo Habkost (2):
  i386: x86_cpu_list_feature_names() function
  i386: "unavailable-features" QOM property

 target/i386/cpu.c | 55 ++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 42 insertions(+), 13 deletions(-)

-- 
2.18.0.rc1.1.g3f1ff2140

[Qemu-devel] [PATCH 0/2] i386: "unavailable-features" QOM property

[Eduardo Habkost]

Currently, libvirt uses the "filtered-features" QOM property at
runtime to ensure no feature was accidentally disabled on VCPUs
because it's not available on the host.

However, the code for "feature-words" assumes that all missing
features have a corresponding CPUID bit, which is not true for
MSR-based features like the ones at FEAT_ARCH_CAPABILITIES.

We could extend X86CPUFeatureWordInfo to include information
about MSR features, but it's impossible to do that while keeping
compatibility with clients that (reasonably) expect all elements
of "filtered-features" to have the cpuid-* fields.

We have a field in "query-cpu-definitions" that already describes
all features that are missing on a CPU, including MSR features:
CpuDefinitionInfo.unavailable-features.  The existing code for
building the unavailable-features array even uses
X86CPU::filtered_features to build the feature list.

This series adds a "unavailable-features" QOM property to X86CPU
objects that have the same semantics of "unavailable-features" on
query-cpu-definitions.  The new property has the same goal of
"filtered-features", but is generic enough to let any kind of CPU
feature to be listed there without relying on low level details
like CPUID leaves or MSR numbers.

Eduardo Habkost (2):
  i386: x86_cpu_list_feature_names() function
  i386: "unavailable-features" QOM property

 target/i386/cpu.c | 55 ++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 42 insertions(+), 13 deletions(-)

-- 
2.18.0.rc1.1.g3f1ff2140

[Qemu-devel] [PATCH 1/2] i386: x86_cpu_list_feature_names() function

[Eduardo Habkost]

Extract feature name listing code from
x86_cpu_class_check_missing_features().  It will be reused to
return information about CPU filtered features at runtime.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target/i386/cpu.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index d6bb57d210..23e55a9ef2 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3619,6 +3619,27 @@ static void x86_cpu_parse_featurestr(const char *typename, char *features,
 static void x86_cpu_expand_features(X86CPU *cpu, Error **errp);
 static int x86_cpu_filter_features(X86CPU *cpu);
 
+/* Build a list with the name of all features on a feature word array */
+static void x86_cpu_list_feature_names(FeatureWordArray features,
+                                       strList **feat_names)
+{
+    FeatureWord w;
+    strList **next = feat_names;
+
+    for (w = 0; w < FEATURE_WORDS; w++) {
+        uint32_t filtered = features[w];
+        int i;
+        for (i = 0; i < 32; i++) {
+            if (filtered & (1UL << i)) {
+                strList *new = g_new0(strList, 1);
+                new->value = g_strdup(x86_cpu_feature_name(w, i));
+                *next = new;
+                next = &new->next;
+            }
+        }
+    }
+}
+
 /* Check for missing features that may prevent the CPU class from
  * running using the current machine and accelerator.
  */
@@ -3626,7 +3647,6 @@ static void x86_cpu_class_check_missing_features(X86CPUClass *xcc,
                                                  strList **missing_feats)
 {
     X86CPU *xc;
-    FeatureWord w;
     Error *err = NULL;
     strList **next = missing_feats;
 
@@ -3653,18 +3673,7 @@ static void x86_cpu_class_check_missing_features(X86CPUClass *xcc,
 
     x86_cpu_filter_features(xc);
 
-    for (w = 0; w < FEATURE_WORDS; w++) {
-        uint32_t filtered = xc->filtered_features[w];
-        int i;
-        for (i = 0; i < 32; i++) {
-            if (filtered & (1UL << i)) {
-                strList *new = g_new0(strList, 1);
-                new->value = g_strdup(x86_cpu_feature_name(w, i));
-                *next = new;
-                next = &new->next;
-            }
-        }
-    }
+    x86_cpu_list_feature_names(xc->filtered_features, next);
 
     object_unref(OBJECT(xc));
 }
-- 
2.18.0.rc1.1.g3f1ff2140

[Qemu-devel] [PATCH 1/2] i386: x86_cpu_list_feature_names() function

[Eduardo Habkost]

Extract feature name listing code from
x86_cpu_class_check_missing_features().  It will be reused to
return information about CPU filtered features at runtime.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target/i386/cpu.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index d6bb57d210..23e55a9ef2 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3619,6 +3619,27 @@ static void x86_cpu_parse_featurestr(const char *typename, char *features,
 static void x86_cpu_expand_features(X86CPU *cpu, Error **errp);
 static int x86_cpu_filter_features(X86CPU *cpu);
 
+/* Build a list with the name of all features on a feature word array */
+static void x86_cpu_list_feature_names(FeatureWordArray features,
+                                       strList **feat_names)
+{
+    FeatureWord w;
+    strList **next = feat_names;
+
+    for (w = 0; w < FEATURE_WORDS; w++) {
+        uint32_t filtered = features[w];
+        int i;
+        for (i = 0; i < 32; i++) {
+            if (filtered & (1UL << i)) {
+                strList *new = g_new0(strList, 1);
+                new->value = g_strdup(x86_cpu_feature_name(w, i));
+                *next = new;
+                next = &new->next;
+            }
+        }
+    }
+}
+
 /* Check for missing features that may prevent the CPU class from
  * running using the current machine and accelerator.
  */
@@ -3626,7 +3647,6 @@ static void x86_cpu_class_check_missing_features(X86CPUClass *xcc,
                                                  strList **missing_feats)
 {
     X86CPU *xc;
-    FeatureWord w;
     Error *err = NULL;
     strList **next = missing_feats;
 
@@ -3653,18 +3673,7 @@ static void x86_cpu_class_check_missing_features(X86CPUClass *xcc,
 
     x86_cpu_filter_features(xc);
 
-    for (w = 0; w < FEATURE_WORDS; w++) {
-        uint32_t filtered = xc->filtered_features[w];
-        int i;
-        for (i = 0; i < 32; i++) {
-            if (filtered & (1UL << i)) {
-                strList *new = g_new0(strList, 1);
-                new->value = g_strdup(x86_cpu_feature_name(w, i));
-                *next = new;
-                next = &new->next;
-            }
-        }
-    }
+    x86_cpu_list_feature_names(xc->filtered_features, next);
 
     object_unref(OBJECT(xc));
 }
-- 
2.18.0.rc1.1.g3f1ff2140

[Qemu-devel] [PATCH 2/2] i386: "unavailable-features" QOM property

[Eduardo Habkost]

Add a "unavailable-features" QOM property to X86CPU objects that
have the same semantics of "unavailable-features" on
query-cpu-definitions.  The new property has the same goal of
"filtered-features", but is generic enough to let any kind of CPU
feature to be listed there without relying on low level details
like CPUID leaves or MSR numbers.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target/i386/cpu.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 23e55a9ef2..042d09a1bb 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3640,6 +3640,17 @@ static void x86_cpu_list_feature_names(FeatureWordArray features,
     }
 }
 
+static void x86_cpu_get_unavailable_features(Object *obj, Visitor *v,
+                                             const char *name, void *opaque,
+                                             Error **errp)
+{
+    X86CPU *xc = X86_CPU(obj);
+    strList *result = NULL;
+
+    x86_cpu_list_feature_names(xc->filtered_features, &result);
+    visit_type_strList(v, "unavailable-features", &result, errp);
+}
+
 /* Check for missing features that may prevent the CPU class from
  * running using the current machine and accelerator.
  */
@@ -5580,6 +5591,15 @@ static void x86_cpu_initfn(Object *obj)
     object_property_add(obj, "filtered-features", "X86CPUFeatureWordInfo",
                         x86_cpu_get_feature_words,
                         NULL, NULL, (void *)cpu->filtered_features, NULL);
+    /*
+     * The "unavailable-features" property has the same semantics as
+     * CpuDefinitionInfo.unavailable-features on the "query-cpu-definitions"
+     * QMP command: they list the features that would have prevented the
+     * CPU from running if the "enforce" flag was set.
+     */
+    object_property_add(obj, "unavailable-features", "strList",
+                        x86_cpu_get_unavailable_features,
+                        NULL, NULL, NULL, &error_abort);
 
     object_property_add(obj, "crash-information", "GuestPanicInformation",
                         x86_cpu_get_crash_info_qom, NULL, NULL, NULL, NULL);
-- 
2.18.0.rc1.1.g3f1ff2140

[Qemu-devel] [PATCH 2/2] i386: "unavailable-features" QOM property

[Eduardo Habkost]

Add a "unavailable-features" QOM property to X86CPU objects that
have the same semantics of "unavailable-features" on
query-cpu-definitions.  The new property has the same goal of
"filtered-features", but is generic enough to let any kind of CPU
feature to be listed there without relying on low level details
like CPUID leaves or MSR numbers.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target/i386/cpu.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 23e55a9ef2..042d09a1bb 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3640,6 +3640,17 @@ static void x86_cpu_list_feature_names(FeatureWordArray features,
     }
 }
 
+static void x86_cpu_get_unavailable_features(Object *obj, Visitor *v,
+                                             const char *name, void *opaque,
+                                             Error **errp)
+{
+    X86CPU *xc = X86_CPU(obj);
+    strList *result = NULL;
+
+    x86_cpu_list_feature_names(xc->filtered_features, &result);
+    visit_type_strList(v, "unavailable-features", &result, errp);
+}
+
 /* Check for missing features that may prevent the CPU class from
  * running using the current machine and accelerator.
  */
@@ -5580,6 +5591,15 @@ static void x86_cpu_initfn(Object *obj)
     object_property_add(obj, "filtered-features", "X86CPUFeatureWordInfo",
                         x86_cpu_get_feature_words,
                         NULL, NULL, (void *)cpu->filtered_features, NULL);
+    /*
+     * The "unavailable-features" property has the same semantics as
+     * CpuDefinitionInfo.unavailable-features on the "query-cpu-definitions"
+     * QMP command: they list the features that would have prevented the
+     * CPU from running if the "enforce" flag was set.
+     */
+    object_property_add(obj, "unavailable-features", "strList",
+                        x86_cpu_get_unavailable_features,
+                        NULL, NULL, NULL, &error_abort);
 
     object_property_add(obj, "crash-information", "GuestPanicInformation",
                         x86_cpu_get_crash_info_qom, NULL, NULL, NULL, NULL);
-- 
2.18.0.rc1.1.g3f1ff2140

Re: [Qemu-devel] [PATCH 0/2] i386: "unavailable-features" QOM property

[Jiri Denemark]

On Mon, Apr 22, 2019 at 20:47:40 -0300, Eduardo Habkost wrote:
> Currently, libvirt uses the "filtered-features" QOM property at
> runtime to ensure no feature was accidentally disabled on VCPUs
> because it's not available on the host.
> 
> However, the code for "feature-words" assumes that all missing
> features have a corresponding CPUID bit, which is not true for
> MSR-based features like the ones at FEAT_ARCH_CAPABILITIES.
> 
> We could extend X86CPUFeatureWordInfo to include information
> about MSR features, but it's impossible to do that while keeping
> compatibility with clients that (reasonably) expect all elements
> of "filtered-features" to have the cpuid-* fields.
> 
> We have a field in "query-cpu-definitions" that already describes
> all features that are missing on a CPU, including MSR features:
> CpuDefinitionInfo.unavailable-features.  The existing code for
> building the unavailable-features array even uses
> X86CPU::filtered_features to build the feature list.
> 
> This series adds a "unavailable-features" QOM property to X86CPU
> objects that have the same semantics of "unavailable-features" on
> query-cpu-definitions.  The new property has the same goal of
> "filtered-features", but is generic enough to let any kind of CPU
> feature to be listed there without relying on low level details
> like CPUID leaves or MSR numbers.

Thanks.

Would this unavailable-features property contain only canonical names of
the features or all possible aliases of all features? For example,
"tsc-adjust" can also be spelled as "tsc_adjust". When calling
query-cpu-model-expansion, we have a way to request all variants by
running full expansion on the result of a previous static expansion. Can
we get something like this for unavailable-features too?

As you mentioned, there are two interesting QOM properties:
filtered-features and feature-words and both are used by libvirt. We use
feature-words to get CPU features which were enabled in the guest CPU on
top of what we expected. This is the case of, e.g., a feature added to a
given CPU model for new machine types. I guess we could switch to
checking QOM properties for individual features as a replacement for
feature-words which covers both CPUID and MSR features.

Jirka

Re: [Qemu-devel] [PATCH 0/2] i386: "unavailable-features" QOM property

[Eduardo Habkost]

On Thu, May 09, 2019 at 03:35:37PM +0200, Jiri Denemark wrote:
> On Mon, Apr 22, 2019 at 20:47:40 -0300, Eduardo Habkost wrote:
> > Currently, libvirt uses the "filtered-features" QOM property at
> > runtime to ensure no feature was accidentally disabled on VCPUs
> > because it's not available on the host.
> > 
> > However, the code for "feature-words" assumes that all missing
> > features have a corresponding CPUID bit, which is not true for
> > MSR-based features like the ones at FEAT_ARCH_CAPABILITIES.
> > 
> > We could extend X86CPUFeatureWordInfo to include information
> > about MSR features, but it's impossible to do that while keeping
> > compatibility with clients that (reasonably) expect all elements
> > of "filtered-features" to have the cpuid-* fields.
> > 
> > We have a field in "query-cpu-definitions" that already describes
> > all features that are missing on a CPU, including MSR features:
> > CpuDefinitionInfo.unavailable-features.  The existing code for
> > building the unavailable-features array even uses
> > X86CPU::filtered_features to build the feature list.
> > 
> > This series adds a "unavailable-features" QOM property to X86CPU
> > objects that have the same semantics of "unavailable-features" on
> > query-cpu-definitions.  The new property has the same goal of
> > "filtered-features", but is generic enough to let any kind of CPU
> > feature to be listed there without relying on low level details
> > like CPUID leaves or MSR numbers.
> 
> Thanks.
> 
> Would this unavailable-features property contain only canonical names of
> the features or all possible aliases of all features? For example,
> "tsc-adjust" can also be spelled as "tsc_adjust". When calling
> query-cpu-model-expansion, we have a way to request all variants by
> running full expansion on the result of a previous static expansion. Can
> we get something like this for unavailable-features too?

I'd like to avoid that, and refer only to the canonical names.

Could you explain the use case you have in mind, so we can look
for alternatives?

> 
> As you mentioned, there are two interesting QOM properties:
> filtered-features and feature-words and both are used by libvirt. We use
> feature-words to get CPU features which were enabled in the guest CPU on
> top of what we expected. This is the case of, e.g., a feature added to a
> given CPU model for new machine types. I guess we could switch to
> checking QOM properties for individual features as a replacement for
> feature-words which covers both CPUID and MSR features.

I guess it depends on your goal:

If your just want to know if one specific feature is missing for
some reason, you can check the QOM properties directly.  That's
OK, and it's even better than checking the `feature-words`
property.

If you want to be 100% sure no property was missing when starting
the VM (e.g. emulate the behavior of the "enforce" option), I
suggest you check if `unavailable-features` is empty.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/2] i386: "unavailable-features" QOM property

[Jiri Denemark]

On Thu, May 09, 2019 at 10:56:17 -0300, Eduardo Habkost wrote:
> On Thu, May 09, 2019 at 03:35:37PM +0200, Jiri Denemark wrote:
> > Would this unavailable-features property contain only canonical names of
> > the features or all possible aliases of all features? For example,
> > "tsc-adjust" can also be spelled as "tsc_adjust". When calling
> > query-cpu-model-expansion, we have a way to request all variants by
> > running full expansion on the result of a previous static expansion. Can
> > we get something like this for unavailable-features too?
> 
> I'd like to avoid that, and refer only to the canonical names.
> 
> Could you explain the use case you have in mind, so we can look
> for alternatives?

Libvirt only knows about a single spelling for each CPU feature and
quite often it is not the canonical variant. Thus libvirt would only
recognize features for which the name known by libvirt is the canonical
name.

We could theoretically make the translation in libvirt, but it's not
going to be future proof. If a new spelling is introduced, it's because
the old one is not considered correct and the new one becomes the
canonical version. When libvirt doesn't have the translation (libvirt is
older or just didn't catch up yet) we have a big problem.

I guess a good alternative would be some way of querying all CPU feature
names and their aliases. If I'm not mistaken, we can either get a list
of canonical names or all names, but without any clue which names
actually refer to a single feature.

> > As you mentioned, there are two interesting QOM properties:
> > filtered-features and feature-words and both are used by libvirt. We use
> > feature-words to get CPU features which were enabled in the guest CPU on
> > top of what we expected. This is the case of, e.g., a feature added to a
> > given CPU model for new machine types. I guess we could switch to
> > checking QOM properties for individual features as a replacement for
> > feature-words which covers both CPUID and MSR features.
> 
> I guess it depends on your goal:
> 
> If your just want to know if one specific feature is missing for
> some reason, you can check the QOM properties directly.  That's
> OK, and it's even better than checking the `feature-words`
> property.
> 
> If you want to be 100% sure no property was missing when starting
> the VM (e.g. emulate the behavior of the "enforce" option), I
> suggest you check if `unavailable-features` is empty.

That's what filtered-features is used for.

As I tried to explain (and apparently failed :-)) we use feature-words
for a different thing. I guess it will be more clear using a specific
example. For example, when arat CPU feature was introduced, it was added
into several existing CPU models and thus libvirt's version of the CPU
model differs from the one in QEMU. (This is actually safer and better
then changing the libvirt's CPU model too since migration relies on both
hosts having the same definition of the CPU model used by a domain.) So,
for example, when a domain with Broadwell CPU model is started, libvirt
checks feature-words to see whether some CPU features not defined in
libvirt's definition of Broadwell CPU model were enabled. And it will
see arat. As a result the live domain definition will actually be
changed to Broadwell+arat to make sure arat is not lost after migration.

Is the usage clear now?

Anyway, I think we can just check all features we know about via CPU
properties to get the same result. It's not going to be as nice as using
feature-words, but it's doable.

Jirka

Re: [Qemu-devel] [PATCH 0/2] i386: "unavailable-features" QOM property

[Eduardo Habkost]

On Thu, May 09, 2019 at 05:26:16PM +0200, Jiri Denemark wrote:
> On Thu, May 09, 2019 at 10:56:17 -0300, Eduardo Habkost wrote:
> > On Thu, May 09, 2019 at 03:35:37PM +0200, Jiri Denemark wrote:
> > > Would this unavailable-features property contain only canonical names of
> > > the features or all possible aliases of all features? For example,
> > > "tsc-adjust" can also be spelled as "tsc_adjust". When calling
> > > query-cpu-model-expansion, we have a way to request all variants by
> > > running full expansion on the result of a previous static expansion. Can
> > > we get something like this for unavailable-features too?
> > 
> > I'd like to avoid that, and refer only to the canonical names.
> > 
> > Could you explain the use case you have in mind, so we can look
> > for alternatives?
> 
> Libvirt only knows about a single spelling for each CPU feature and
> quite often it is not the canonical variant. Thus libvirt would only
> recognize features for which the name known by libvirt is the canonical
> name.
> 
> We could theoretically make the translation in libvirt, but it's not
> going to be future proof. If a new spelling is introduced, it's because
> the old one is not considered correct and the new one becomes the
> canonical version. When libvirt doesn't have the translation (libvirt is
> older or just didn't catch up yet) we have a big problem.
> 
> I guess a good alternative would be some way of querying all CPU feature
> names and their aliases. If I'm not mistaken, we can either get a list
> of canonical names or all names, but without any clue which names
> actually refer to a single feature.

Right.  But (as described below) if you want to know the status a
specific feature you already know about (even if you are using
the old spelling), qom-get should work for you.

The goal of filtered-features and unavailable-features is to
catch all the rest: features you might not know about, but that
should prevent a CPU model from running.

> 
> > > As you mentioned, there are two interesting QOM properties:
> > > filtered-features and feature-words and both are used by libvirt. We use
> > > feature-words to get CPU features which were enabled in the guest CPU on
> > > top of what we expected. This is the case of, e.g., a feature added to a
> > > given CPU model for new machine types. I guess we could switch to
> > > checking QOM properties for individual features as a replacement for
> > > feature-words which covers both CPUID and MSR features.
> > 
> > I guess it depends on your goal:
> > 
> > If your just want to know if one specific feature is missing for
> > some reason, you can check the QOM properties directly.  That's
> > OK, and it's even better than checking the `feature-words`
> > property.
> > 
> > If you want to be 100% sure no property was missing when starting
> > the VM (e.g. emulate the behavior of the "enforce" option), I
> > suggest you check if `unavailable-features` is empty.
> 
> That's what filtered-features is used for.

Right.  My goal here is to replace filtered-features, not
feature-words.

> 
> As I tried to explain (and apparently failed :-)) we use feature-words
> for a different thing. I guess it will be more clear using a specific
> example. For example, when arat CPU feature was introduced, it was added
> into several existing CPU models and thus libvirt's version of the CPU
> model differs from the one in QEMU. (This is actually safer and better
> then changing the libvirt's CPU model too since migration relies on both
> hosts having the same definition of the CPU model used by a domain.) So,
> for example, when a domain with Broadwell CPU model is started, libvirt
> checks feature-words to see whether some CPU features not defined in
> libvirt's definition of Broadwell CPU model were enabled. And it will
> see arat. As a result the live domain definition will actually be
> changed to Broadwell+arat to make sure arat is not lost after migration.
> 
> Is the usage clear now?
> 
> Anyway, I think we can just check all features we know about via CPU
> properties to get the same result. It's not going to be as nice as using
> feature-words, but it's doable.

For the use case you describe here, qom-get sounds better than
using feature-words, yes.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/2] i386: "unavailable-features" QOM property

[Jiri Denemark]

On Thu, May 09, 2019 at 13:08:25 -0300, Eduardo Habkost wrote:
> On Thu, May 09, 2019 at 05:26:16PM +0200, Jiri Denemark wrote:
> > On Thu, May 09, 2019 at 10:56:17 -0300, Eduardo Habkost wrote:
> > > On Thu, May 09, 2019 at 03:35:37PM +0200, Jiri Denemark wrote:
> > > > Would this unavailable-features property contain only canonical names of
> > > > the features or all possible aliases of all features? For example,
> > > > "tsc-adjust" can also be spelled as "tsc_adjust". When calling
> > > > query-cpu-model-expansion, we have a way to request all variants by
> > > > running full expansion on the result of a previous static expansion. Can
> > > > we get something like this for unavailable-features too?
> > > 
> > > I'd like to avoid that, and refer only to the canonical names.
> > > 
> > > Could you explain the use case you have in mind, so we can look
> > > for alternatives?
> > 
> > Libvirt only knows about a single spelling for each CPU feature and
> > quite often it is not the canonical variant. Thus libvirt would only
> > recognize features for which the name known by libvirt is the canonical
> > name.
> > 
> > We could theoretically make the translation in libvirt, but it's not
> > going to be future proof. If a new spelling is introduced, it's because
> > the old one is not considered correct and the new one becomes the
> > canonical version. When libvirt doesn't have the translation (libvirt is
> > older or just didn't catch up yet) we have a big problem.
> > 
> > I guess a good alternative would be some way of querying all CPU feature
> > names and their aliases. If I'm not mistaken, we can either get a list
> > of canonical names or all names, but without any clue which names
> > actually refer to a single feature.
> 
> Right.  But (as described below) if you want to know the status a
> specific feature you already know about (even if you are using
> the old spelling), qom-get should work for you.

Yeah, since we'll be checking all features anyway, we can detect enabled
and disabled features at the same time. However, we don't know whether a
specific feature was disabled because we did not ask for it to be
enabled (remember, CPU model definition may differ between libvirt and
QEMU) or because it was filtered out.

Depending on the domain XML used to start a domain, libvirt may not (and
usually will not) refuse to start a domain for which QEMU filtered out
some CPU features. Of course, once the domain is running, the checking
becomes very strict and libvirt would refuse to start the domain on
another host during migration if any feature is filtered out.

Thus libvirt stores all features QEMU filtered out when a domain was
started in the non-strict way. So we need to match the features in the
unavailable-features list with our features. Just checking for the list
being empty is not sufficient.

Jirka

Re: [Qemu-devel] [PATCH 0/2] i386: "unavailable-features" QOM property

[Eduardo Habkost]

On Fri, May 10, 2019 at 01:33:03PM +0200, Jiri Denemark wrote:
> On Thu, May 09, 2019 at 13:08:25 -0300, Eduardo Habkost wrote:
> > On Thu, May 09, 2019 at 05:26:16PM +0200, Jiri Denemark wrote:
> > > On Thu, May 09, 2019 at 10:56:17 -0300, Eduardo Habkost wrote:
> > > > On Thu, May 09, 2019 at 03:35:37PM +0200, Jiri Denemark wrote:
> > > > > Would this unavailable-features property contain only canonical names of
> > > > > the features or all possible aliases of all features? For example,
> > > > > "tsc-adjust" can also be spelled as "tsc_adjust". When calling
> > > > > query-cpu-model-expansion, we have a way to request all variants by
> > > > > running full expansion on the result of a previous static expansion. Can
> > > > > we get something like this for unavailable-features too?
> > > > 
> > > > I'd like to avoid that, and refer only to the canonical names.
> > > > 
> > > > Could you explain the use case you have in mind, so we can look
> > > > for alternatives?
> > > 
> > > Libvirt only knows about a single spelling for each CPU feature and
> > > quite often it is not the canonical variant. Thus libvirt would only
> > > recognize features for which the name known by libvirt is the canonical
> > > name.
> > > 
> > > We could theoretically make the translation in libvirt, but it's not
> > > going to be future proof. If a new spelling is introduced, it's because
> > > the old one is not considered correct and the new one becomes the
> > > canonical version. When libvirt doesn't have the translation (libvirt is
> > > older or just didn't catch up yet) we have a big problem.
> > > 
> > > I guess a good alternative would be some way of querying all CPU feature
> > > names and their aliases. If I'm not mistaken, we can either get a list
> > > of canonical names or all names, but without any clue which names
> > > actually refer to a single feature.
> > 
> > Right.  But (as described below) if you want to know the status a
> > specific feature you already know about (even if you are using
> > the old spelling), qom-get should work for you.
> 
> Yeah, since we'll be checking all features anyway, we can detect enabled
> and disabled features at the same time. However, we don't know whether a
> specific feature was disabled because we did not ask for it to be
> enabled (remember, CPU model definition may differ between libvirt and
> QEMU) or because it was filtered out.
> 
> Depending on the domain XML used to start a domain, libvirt may not (and
> usually will not) refuse to start a domain for which QEMU filtered out
> some CPU features. Of course, once the domain is running, the checking
> becomes very strict and libvirt would refuse to start the domain on
> another host during migration if any feature is filtered out.
> 
> Thus libvirt stores all features QEMU filtered out when a domain was
> started in the non-strict way. So we need to match the features in the
> unavailable-features list with our features. Just checking for the list
> being empty is not sufficient.

OK, I understand you want to translate the canonical names on
unavailable-features back to the old names on some cases.

But I really prefer Igor's suggestion of deprecating aliases and
getting rid of them in the future, instead of increasing the
complexity of our QMP interfaces just to accommodate the existing
aliases.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/2] i386: "unavailable-features" QOM property

[Jiri Denemark]

On Fri, May 10, 2019 at 17:23:13 -0300, Eduardo Habkost wrote:
> On Fri, May 10, 2019 at 01:33:03PM +0200, Jiri Denemark wrote:
> > On Thu, May 09, 2019 at 13:08:25 -0300, Eduardo Habkost wrote:
> > > On Thu, May 09, 2019 at 05:26:16PM +0200, Jiri Denemark wrote:
> > > > On Thu, May 09, 2019 at 10:56:17 -0300, Eduardo Habkost wrote:
> > > > > On Thu, May 09, 2019 at 03:35:37PM +0200, Jiri Denemark wrote:
> > > > > > Would this unavailable-features property contain only canonical names of
> > > > > > the features or all possible aliases of all features? For example,
> > > > > > "tsc-adjust" can also be spelled as "tsc_adjust". When calling
> > > > > > query-cpu-model-expansion, we have a way to request all variants by
> > > > > > running full expansion on the result of a previous static expansion. Can
> > > > > > we get something like this for unavailable-features too?
> > > > > 
> > > > > I'd like to avoid that, and refer only to the canonical names.
> > > > > 
> > > > > Could you explain the use case you have in mind, so we can look
> > > > > for alternatives?
> > > > 
> > > > Libvirt only knows about a single spelling for each CPU feature and
> > > > quite often it is not the canonical variant. Thus libvirt would only
> > > > recognize features for which the name known by libvirt is the canonical
> > > > name.
> > > > 
> > > > We could theoretically make the translation in libvirt, but it's not
> > > > going to be future proof. If a new spelling is introduced, it's because
> > > > the old one is not considered correct and the new one becomes the
> > > > canonical version. When libvirt doesn't have the translation (libvirt is
> > > > older or just didn't catch up yet) we have a big problem.
> > > > 
> > > > I guess a good alternative would be some way of querying all CPU feature
> > > > names and their aliases. If I'm not mistaken, we can either get a list
> > > > of canonical names or all names, but without any clue which names
> > > > actually refer to a single feature.
> > > 
> > > Right.  But (as described below) if you want to know the status a
> > > specific feature you already know about (even if you are using
> > > the old spelling), qom-get should work for you.
> > 
> > Yeah, since we'll be checking all features anyway, we can detect enabled
> > and disabled features at the same time. However, we don't know whether a
> > specific feature was disabled because we did not ask for it to be
> > enabled (remember, CPU model definition may differ between libvirt and
> > QEMU) or because it was filtered out.
> > 
> > Depending on the domain XML used to start a domain, libvirt may not (and
> > usually will not) refuse to start a domain for which QEMU filtered out
> > some CPU features. Of course, once the domain is running, the checking
> > becomes very strict and libvirt would refuse to start the domain on
> > another host during migration if any feature is filtered out.
> > 
> > Thus libvirt stores all features QEMU filtered out when a domain was
> > started in the non-strict way. So we need to match the features in the
> > unavailable-features list with our features. Just checking for the list
> > being empty is not sufficient.
> 
> OK, I understand you want to translate the canonical names on
> unavailable-features back to the old names on some cases.
> 
> But I really prefer Igor's suggestion of deprecating aliases and
> getting rid of them in the future, instead of increasing the
> complexity of our QMP interfaces just to accommodate the existing
> aliases.

OK, I think you can go on and implement the unavailable-features
property the way you suggested and we'll deal with the translation
internally in libvirt.

Jirka

Re: [Qemu-devel] [PATCH 0/2] i386: "unavailable-features" QOM property

[Eduardo Habkost]

On Wed, May 22, 2019 at 10:42:56AM +0200, Jiri Denemark wrote:
> On Fri, May 10, 2019 at 17:23:13 -0300, Eduardo Habkost wrote:
> > On Fri, May 10, 2019 at 01:33:03PM +0200, Jiri Denemark wrote:
> > > On Thu, May 09, 2019 at 13:08:25 -0300, Eduardo Habkost wrote:
> > > > On Thu, May 09, 2019 at 05:26:16PM +0200, Jiri Denemark wrote:
> > > > > On Thu, May 09, 2019 at 10:56:17 -0300, Eduardo Habkost wrote:
> > > > > > On Thu, May 09, 2019 at 03:35:37PM +0200, Jiri Denemark wrote:
> > > > > > > Would this unavailable-features property contain only canonical names of
> > > > > > > the features or all possible aliases of all features? For example,
> > > > > > > "tsc-adjust" can also be spelled as "tsc_adjust". When calling
> > > > > > > query-cpu-model-expansion, we have a way to request all variants by
> > > > > > > running full expansion on the result of a previous static expansion. Can
> > > > > > > we get something like this for unavailable-features too?
> > > > > > 
> > > > > > I'd like to avoid that, and refer only to the canonical names.
> > > > > > 
> > > > > > Could you explain the use case you have in mind, so we can look
> > > > > > for alternatives?
> > > > > 
> > > > > Libvirt only knows about a single spelling for each CPU feature and
> > > > > quite often it is not the canonical variant. Thus libvirt would only
> > > > > recognize features for which the name known by libvirt is the canonical
> > > > > name.
> > > > > 
> > > > > We could theoretically make the translation in libvirt, but it's not
> > > > > going to be future proof. If a new spelling is introduced, it's because
> > > > > the old one is not considered correct and the new one becomes the
> > > > > canonical version. When libvirt doesn't have the translation (libvirt is
> > > > > older or just didn't catch up yet) we have a big problem.
> > > > > 
> > > > > I guess a good alternative would be some way of querying all CPU feature
> > > > > names and their aliases. If I'm not mistaken, we can either get a list
> > > > > of canonical names or all names, but without any clue which names
> > > > > actually refer to a single feature.
> > > > 
> > > > Right.  But (as described below) if you want to know the status a
> > > > specific feature you already know about (even if you are using
> > > > the old spelling), qom-get should work for you.
> > > 
> > > Yeah, since we'll be checking all features anyway, we can detect enabled
> > > and disabled features at the same time. However, we don't know whether a
> > > specific feature was disabled because we did not ask for it to be
> > > enabled (remember, CPU model definition may differ between libvirt and
> > > QEMU) or because it was filtered out.
> > > 
> > > Depending on the domain XML used to start a domain, libvirt may not (and
> > > usually will not) refuse to start a domain for which QEMU filtered out
> > > some CPU features. Of course, once the domain is running, the checking
> > > becomes very strict and libvirt would refuse to start the domain on
> > > another host during migration if any feature is filtered out.
> > > 
> > > Thus libvirt stores all features QEMU filtered out when a domain was
> > > started in the non-strict way. So we need to match the features in the
> > > unavailable-features list with our features. Just checking for the list
> > > being empty is not sufficient.
> > 
> > OK, I understand you want to translate the canonical names on
> > unavailable-features back to the old names on some cases.
> > 
> > But I really prefer Igor's suggestion of deprecating aliases and
> > getting rid of them in the future, instead of increasing the
> > complexity of our QMP interfaces just to accommodate the existing
> > aliases.
> 
> OK, I think you can go on and implement the unavailable-features
> property the way you suggested and we'll deal with the translation
> internally in libvirt.

OK, I'm queueing this series on x86-next.  Thanks for the
feedback!

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/2] i386: "unavailable-features" QOM property

[Igor Mammedov]

On Thu, 9 May 2019 10:56:17 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, May 09, 2019 at 03:35:37PM +0200, Jiri Denemark wrote:
> > On Mon, Apr 22, 2019 at 20:47:40 -0300, Eduardo Habkost wrote:
> > > Currently, libvirt uses the "filtered-features" QOM property at
> > > runtime to ensure no feature was accidentally disabled on VCPUs
> > > because it's not available on the host.
> > > 
> > > However, the code for "feature-words" assumes that all missing
> > > features have a corresponding CPUID bit, which is not true for
> > > MSR-based features like the ones at FEAT_ARCH_CAPABILITIES.
> > > 
> > > We could extend X86CPUFeatureWordInfo to include information
> > > about MSR features, but it's impossible to do that while keeping
> > > compatibility with clients that (reasonably) expect all elements
> > > of "filtered-features" to have the cpuid-* fields.
> > > 
> > > We have a field in "query-cpu-definitions" that already describes
> > > all features that are missing on a CPU, including MSR features:
> > > CpuDefinitionInfo.unavailable-features.  The existing code for
> > > building the unavailable-features array even uses
> > > X86CPU::filtered_features to build the feature list.
> > > 
> > > This series adds a "unavailable-features" QOM property to X86CPU
> > > objects that have the same semantics of "unavailable-features" on
> > > query-cpu-definitions.  The new property has the same goal of
> > > "filtered-features", but is generic enough to let any kind of CPU
> > > feature to be listed there without relying on low level details
> > > like CPUID leaves or MSR numbers.
> > 
> > Thanks.
> > 
> > Would this unavailable-features property contain only canonical names of
> > the features or all possible aliases of all features? For example,
> > "tsc-adjust" can also be spelled as "tsc_adjust". When calling
> > query-cpu-model-expansion, we have a way to request all variants by
> > running full expansion on the result of a previous static expansion. Can
> > we get something like this for unavailable-features too?
> 
> I'd like to avoid that, and refer only to the canonical names.

Can we deprecate aliases to avoid confusion in future?
(there aren't that many of them that used pre-QOM name format)

Re: [Qemu-devel] [PATCH 0/2] i386: "unavailable-features" QOM property

[Jiri Denemark]

On Thu, May 09, 2019 at 18:06:03 +0200, Igor Mammedov wrote:
> On Thu, 9 May 2019 10:56:17 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, May 09, 2019 at 03:35:37PM +0200, Jiri Denemark wrote:
> > > Would this unavailable-features property contain only canonical names of
> > > the features or all possible aliases of all features? For example,
> > > "tsc-adjust" can also be spelled as "tsc_adjust". When calling
> > > query-cpu-model-expansion, we have a way to request all variants by
> > > running full expansion on the result of a previous static expansion. Can
> > > we get something like this for unavailable-features too?
> > 
> > I'd like to avoid that, and refer only to the canonical names.
> 
> Can we deprecate aliases to avoid confusion in future?
> (there aren't that many of them that used pre-QOM name format)

If you come up with a way libvirt could use to detect which name it
should use when talking to QEMU...

Jirka

Re: [Qemu-devel] [PATCH 0/2] i386: "unavailable-features" QOM property

[Eduardo Habkost]

On Thu, May 09, 2019 at 06:36:18PM +0200, Jiri Denemark wrote:
> On Thu, May 09, 2019 at 18:06:03 +0200, Igor Mammedov wrote:
> > On Thu, 9 May 2019 10:56:17 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Thu, May 09, 2019 at 03:35:37PM +0200, Jiri Denemark wrote:
> > > > Would this unavailable-features property contain only canonical names of
> > > > the features or all possible aliases of all features? For example,
> > > > "tsc-adjust" can also be spelled as "tsc_adjust". When calling
> > > > query-cpu-model-expansion, we have a way to request all variants by
> > > > running full expansion on the result of a previous static expansion. Can
> > > > we get something like this for unavailable-features too?
> > > 
> > > I'd like to avoid that, and refer only to the canonical names.
> > 
> > Can we deprecate aliases to avoid confusion in future?
> > (there aren't that many of them that used pre-QOM name format)
> 
> If you come up with a way libvirt could use to detect which name it
> should use when talking to QEMU...

The property names are part of the API, and deprecation would
just be documented in the QEMU documentation.  Why would you need
to enumerate them dynamically at runtime?

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/2] i386: "unavailable-features" QOM property

[Jiri Denemark]

On Fri, May 10, 2019 at 17:32:03 -0300, Eduardo Habkost wrote:
> On Thu, May 09, 2019 at 06:36:18PM +0200, Jiri Denemark wrote:
> > On Thu, May 09, 2019 at 18:06:03 +0200, Igor Mammedov wrote:
> > > On Thu, 9 May 2019 10:56:17 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > 
> > > > On Thu, May 09, 2019 at 03:35:37PM +0200, Jiri Denemark wrote:
> > > > > Would this unavailable-features property contain only canonical names of
> > > > > the features or all possible aliases of all features? For example,
> > > > > "tsc-adjust" can also be spelled as "tsc_adjust". When calling
> > > > > query-cpu-model-expansion, we have a way to request all variants by
> > > > > running full expansion on the result of a previous static expansion. Can
> > > > > we get something like this for unavailable-features too?
> > > > 
> > > > I'd like to avoid that, and refer only to the canonical names.
> > > 
> > > Can we deprecate aliases to avoid confusion in future?
> > > (there aren't that many of them that used pre-QOM name format)
> > 
> > If you come up with a way libvirt could use to detect which name it
> > should use when talking to QEMU...
> 
> The property names are part of the API, and deprecation would
> just be documented in the QEMU documentation.  Why would you need
> to enumerate them dynamically at runtime?

The tricky part is to know which variant of a particular feature name we
should use when talking to a specific version of QEMU. But I guess we
can use the new "unavailable-features" property for this purpose. When
the property is present, we can translate all feature names to their
canonical names (via a static translation table in libvirt). We'd be
using the old untranslated names when talking to any QEMU which does not
support the "unavailable-features" property.

But I hope we won't get into a situation when some CPU feature needs to
be renamed again, that would make a big mess.

Jirka

Re: [Qemu-devel] [PATCH 0/2] i386: "unavailable-features" QOM property

[Eduardo Habkost]

On Wed, May 22, 2019 at 10:27:41AM +0200, Jiri Denemark wrote:
> On Fri, May 10, 2019 at 17:32:03 -0300, Eduardo Habkost wrote:
> > On Thu, May 09, 2019 at 06:36:18PM +0200, Jiri Denemark wrote:
> > > On Thu, May 09, 2019 at 18:06:03 +0200, Igor Mammedov wrote:
> > > > On Thu, 9 May 2019 10:56:17 -0300
> > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > 
> > > > > On Thu, May 09, 2019 at 03:35:37PM +0200, Jiri Denemark wrote:
> > > > > > Would this unavailable-features property contain only canonical names of
> > > > > > the features or all possible aliases of all features? For example,
> > > > > > "tsc-adjust" can also be spelled as "tsc_adjust". When calling
> > > > > > query-cpu-model-expansion, we have a way to request all variants by
> > > > > > running full expansion on the result of a previous static expansion. Can
> > > > > > we get something like this for unavailable-features too?
> > > > > 
> > > > > I'd like to avoid that, and refer only to the canonical names.
> > > > 
> > > > Can we deprecate aliases to avoid confusion in future?
> > > > (there aren't that many of them that used pre-QOM name format)
> > > 
> > > If you come up with a way libvirt could use to detect which name it
> > > should use when talking to QEMU...
> > 
> > The property names are part of the API, and deprecation would
> > just be documented in the QEMU documentation.  Why would you need
> > to enumerate them dynamically at runtime?
> 
> The tricky part is to know which variant of a particular feature name we
> should use when talking to a specific version of QEMU. But I guess we
> can use the new "unavailable-features" property for this purpose. [...]

You can run device-list-properties on the typenames returned by
query-cpu-definitions, to check which CPU properties exist in a
QEMU binary.

>                                                           [...]   When
> the property is present, we can translate all feature names to their
> canonical names (via a static translation table in libvirt). We'd be
> using the old untranslated names when talking to any QEMU which does not
> support the "unavailable-features" property.

That would help us simplify the interface between QEMU and libvirt.

> 
> But I hope we won't get into a situation when some CPU feature needs to
> be renamed again, that would make a big mess.

I promise we'll try to avoid doing that.  :)

-- 
Eduardo