The implementation of CPUClass::parse_features only supports CPU
features in "feature=value" format.  However, libvirt maybe send us a
CPU feature string in "+feature/-feature" format.  Hence, we need to
override CPUClass::parse_features to support CPU feature string in both
"feature=value" and "+feature/-feature" format.

The logic of AArch64CPUClass::parse_features is similar to that of
X86CPUClass::parse_features.  The diference is that feature name is
transformed to upper before it's added as a property because the name
of CPU features is upper for convenience.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Peng Liang <liangpeng10@huawei.com>
---
 target/arm/cpu64.c | 101 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 4b52505b6e..981011b3da 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -28,6 +28,7 @@
 #include "sysemu/kvm.h"
 #include "kvm_arm.h"
 #include "qapi/visitor.h"
+#include "hw/qdev-properties.h"
 
 #ifndef CONFIG_USER_ONLY
 static uint64_t a57_a53_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri)
@@ -778,6 +779,105 @@ static gchar *aarch64_gdb_arch_name(CPUState *cs)
     return g_strdup("aarch64");
 }
 
+static void
+cpu_add_feat_as_prop(const char *typename, const char *name, const char *val)
+{
+    GlobalProperty *prop = g_new0(typeof(*prop), 1);
+    prop->driver = typename;
+    prop->property = g_strdup(name);
+    prop->value = g_strdup(val);
+    qdev_prop_register_global(prop);
+}
+
+static gint compare_string(gconstpointer a, gconstpointer b)
+{
+    return g_strcmp0(a, b);
+}
+
+static GList *plus_features, *minus_features;
+
+static char *strtoupr(char *str)
+{
+    char *orign = str;
+
+    for (; *str != '\0'; str++) {
+        *str = toupper(*str);
+    }
+
+    return orign;
+}
+
+static void aarch64_cpu_parse_features(const char *typename, char *features,
+                                     Error **errp)
+{
+    GList *l;
+    char *featurestr; /* Single 'key=value" string being parsed */
+    static bool cpu_globals_initialized;
+
+    if (cpu_globals_initialized) {
+        return;
+    }
+    cpu_globals_initialized = true;
+
+    if (!features) {
+        return;
+    }
+    for (featurestr = strtok(features, ",");
+        featurestr;
+        featurestr = strtok(NULL, ",")) {
+        const char *name;
+        char *tmp;
+        const char *val = NULL;
+        char *eq = NULL;
+
+        /* Compatibility syntax: */
+        if (featurestr[0] == '+') {
+            plus_features = g_list_append(plus_features,
+                                          g_strdup(featurestr + 1));
+            continue;
+        } else if (featurestr[0] == '-') {
+            minus_features = g_list_append(minus_features,
+                                           g_strdup(featurestr + 1));
+            continue;
+        }
+
+        eq = strchr(featurestr, '=');
+        name = featurestr;
+        if (eq) {
+            *eq++ = 0;
+            val = eq;
+        } else {
+            error_setg(errp, "Unsupported property format: %s", name);
+            return;
+        }
+
+        if (g_list_find_custom(plus_features, name, compare_string)) {
+            warn_report("Ambiguous CPU model string. "
+                        "Don't mix both \"+%s\" and \"%s=%s\"",
+                        name, name, val);
+        }
+        if (g_list_find_custom(minus_features, name, compare_string)) {
+            warn_report("Ambiguous CPU model string. "
+                        "Don't mix both \"-%s\" and \"%s=%s\"",
+                        name, name, val);
+        }
+        tmp = g_strdup(name);
+        tmp = strtoupr(tmp);
+        cpu_add_feat_as_prop(typename, tmp, val);
+        g_free(tmp);
+    }
+
+    for (l = plus_features; l; l = l->next) {
+        const char *name = strtoupr(l->data); /* convert to upper string */
+        cpu_add_feat_as_prop(typename, name, "on");
+    }
+
+    for (l = minus_features; l; l = l->next) {
+        const char *name = strtoupr(l->data);
+        cpu_add_feat_as_prop(typename, name, "off");
+    }
+}
+
 static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
 {
     CPUClass *cc = CPU_CLASS(oc);
@@ -788,6 +888,7 @@ static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
     cc->gdb_num_core_regs = 34;
     cc->gdb_core_xml_file = "aarch64-core.xml";
     cc->gdb_arch_name = aarch64_gdb_arch_name;
+    cc->parse_features = aarch64_cpu_parse_features;
 }
 
 static void aarch64_cpu_instance_init(Object *obj)
-- 
2.18.4

On Thu, Aug 13, 2020 at 06:26:50PM +0800, Peng Liang wrote:
> The implementation of CPUClass::parse_features only supports CPU
> features in "feature=value" format.  However, libvirt maybe send us a
> CPU feature string in "+feature/-feature" format. 

Why would libvirt do that? If we have a boolean feature, then I'd think
libvirt should be capable of sending feature=on/off just as easily as a
+/-feature. Indeed libvirt seems to do the right thing with all the
CPU features we already have: pmu, sve*, ...

Thanks,
drew

On 8/13/2020 8:21 PM, Andrew Jones wrote:
> On Thu, Aug 13, 2020 at 06:26:50PM +0800, Peng Liang wrote:
>> The implementation of CPUClass::parse_features only supports CPU
>> features in "feature=value" format.  However, libvirt maybe send us a
>> CPU feature string in "+feature/-feature" format. 
> 
> Why would libvirt do that? If we have a boolean feature, then I'd think
> libvirt should be capable of sending feature=on/off just as easily as a
> +/-feature. Indeed libvirt seems to do the right thing with all the
> CPU features we already have: pmu, sve*, ...
> 
> Thanks,
> drew
> 
> .
> 

Libvirt before d47db7b16dd5422c7e487c8c8ee5b181a2f9cd66 ("qemu: command:
Support new cpu feature argument syntax") will send +/-feature.  Does QEMU
need to support it?  If not, I'll remote it.

Thanks,
Peng

On Sat, Aug 15, 2020 at 10:19:12AM +0800, Peng Liang wrote:
> On 8/13/2020 8:21 PM, Andrew Jones wrote:
> > On Thu, Aug 13, 2020 at 06:26:50PM +0800, Peng Liang wrote:
> >> The implementation of CPUClass::parse_features only supports CPU
> >> features in "feature=value" format.  However, libvirt maybe send us a
> >> CPU feature string in "+feature/-feature" format. 
> > 
> > Why would libvirt do that? If we have a boolean feature, then I'd think
> > libvirt should be capable of sending feature=on/off just as easily as a
> > +/-feature. Indeed libvirt seems to do the right thing with all the
> > CPU features we already have: pmu, sve*, ...
> > 
> > Thanks,
> > drew
> > 
> > .
> > 
> 
> Libvirt before d47db7b16dd5422c7e487c8c8ee5b181a2f9cd66 ("qemu: command:
> Support new cpu feature argument syntax") will send +/-feature.  Does QEMU
> need to support it?  If not, I'll remote it.

No, we don't need to support pre-2016 libvirt, at least not for ARM CPU
features, because libvirt didn't start querying QEMU for ARM CPU features
until just last year.

Thanks,
drew