* [Qemu-devel] [PATCH 01/16] target-i386: cleanup 'foo' feature handling'
2013-11-27 22:28 [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties Igor Mammedov
@ 2013-11-27 22:28 ` Igor Mammedov
2013-11-27 22:28 ` [Qemu-devel] [PATCH 02/16] target-i386: cleanup 'foo=val' feature handling Igor Mammedov
` (16 subsequent siblings)
17 siblings, 0 replies; 53+ messages in thread
From: Igor Mammedov @ 2013-11-27 22:28 UTC (permalink / raw)
To: qemu-devel; +Cc: afaerber
features check, enforce, hv_relaxed and hv_vapic are treated as boolean set to 'on'
when passed from command line, so it's not neccessary to handle each of them
separetly. Collapse them to one catch-all branch which will treat
any feature in format 'foo' as boolean set to 'on'.
PS:
Any unknown feature will be rejected by CPU property setter so there is no
need to check for unknown feature in cpu_x86_parse_featurestr(), therefore
it's replaced by above mentioned catch-all handler.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
v2:
* use feat2prop() to perform name convertion for hv_vapic and hv_relaxed
---
target-i386/cpu.c | 13 ++-----------
1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 7f74021..8cc4330 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1768,18 +1768,9 @@ static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp)
error_setg(errp, "unrecognized feature %s", featurestr);
goto out;
}
- } else if (!strcmp(featurestr, "check")) {
- object_property_parse(OBJECT(cpu), "on", featurestr, errp);
- } else if (!strcmp(featurestr, "enforce")) {
- object_property_parse(OBJECT(cpu), "on", featurestr, errp);
- } else if (!strcmp(featurestr, "hv_relaxed")) {
- object_property_parse(OBJECT(cpu), "on", "hv-relaxed", errp);
- } else if (!strcmp(featurestr, "hv_vapic")) {
- object_property_parse(OBJECT(cpu), "on", "hv-vapic", errp);
} else {
- error_setg(errp, "feature string `%s' not in format (+feature|"
- "-feature|feature=xyz)", featurestr);
- goto out;
+ feat2prop(featurestr);
+ object_property_parse(OBJECT(cpu), "on", featurestr, errp);
}
if (error_is_set(errp)) {
goto out;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 53+ messages in thread* [Qemu-devel] [PATCH 02/16] target-i386: cleanup 'foo=val' feature handling
2013-11-27 22:28 [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties Igor Mammedov
2013-11-27 22:28 ` [Qemu-devel] [PATCH 01/16] target-i386: cleanup 'foo' feature handling' Igor Mammedov
@ 2013-11-27 22:28 ` Igor Mammedov
2014-02-11 9:14 ` Eduardo Habkost
2013-11-27 22:28 ` [Qemu-devel] [PATCH 03/16] target-i386: cpu: convert 'level' to static property Igor Mammedov
` (15 subsequent siblings)
17 siblings, 1 reply; 53+ messages in thread
From: Igor Mammedov @ 2013-11-27 22:28 UTC (permalink / raw)
To: qemu-devel; +Cc: afaerber
features family, model, stepping, level, hv_spinlocks are treated similarly
when passed from command line, so it's not necessary to handle each of them
individually. Collapse them to one catch-all branch which will treat
any not explicitly handled feature in format 'foo=val'.
PS:
Any unknown feature will be rejected by property setter so there is no
need to check for unknown feature in cpu_x86_parse_featurestr(), therefore
it's replaced by above mentioned catch-all handler.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
- style fixes
---
target-i386/cpu.c | 17 ++---------------
1 file changed, 2 insertions(+), 15 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 8cc4330..34d3968 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1706,15 +1706,7 @@ static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp)
} else if ((val = strchr(featurestr, '='))) {
*val = 0; val++;
feat2prop(featurestr);
- if (!strcmp(featurestr, "family")) {
- object_property_parse(OBJECT(cpu), val, featurestr, errp);
- } else if (!strcmp(featurestr, "model")) {
- object_property_parse(OBJECT(cpu), val, featurestr, errp);
- } else if (!strcmp(featurestr, "stepping")) {
- object_property_parse(OBJECT(cpu), val, featurestr, errp);
- } else if (!strcmp(featurestr, "level")) {
- object_property_parse(OBJECT(cpu), val, featurestr, errp);
- } else if (!strcmp(featurestr, "xlevel")) {
+ if (!strcmp(featurestr, "xlevel")) {
char *err;
char num[32];
@@ -1730,10 +1722,6 @@ static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp)
}
snprintf(num, sizeof(num), "%" PRIu32, numvalue);
object_property_parse(OBJECT(cpu), num, featurestr, errp);
- } else if (!strcmp(featurestr, "vendor")) {
- object_property_parse(OBJECT(cpu), val, featurestr, errp);
- } else if (!strcmp(featurestr, "model-id")) {
- object_property_parse(OBJECT(cpu), val, featurestr, errp);
} else if (!strcmp(featurestr, "tsc-freq")) {
int64_t tsc_freq;
char *err;
@@ -1765,8 +1753,7 @@ static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp)
snprintf(num, sizeof(num), "%" PRId32, numvalue);
object_property_parse(OBJECT(cpu), num, featurestr, errp);
} else {
- error_setg(errp, "unrecognized feature %s", featurestr);
- goto out;
+ object_property_parse(OBJECT(cpu), val, featurestr, errp);
}
} else {
feat2prop(featurestr);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [Qemu-devel] [PATCH 02/16] target-i386: cleanup 'foo=val' feature handling
2013-11-27 22:28 ` [Qemu-devel] [PATCH 02/16] target-i386: cleanup 'foo=val' feature handling Igor Mammedov
@ 2014-02-11 9:14 ` Eduardo Habkost
2014-02-11 14:28 ` Andreas Färber
0 siblings, 1 reply; 53+ messages in thread
From: Eduardo Habkost @ 2014-02-11 9:14 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, afaerber
On Wed, Nov 27, 2013 at 11:28:42PM +0100, Igor Mammedov wrote:
> features family, model, stepping, level, hv_spinlocks are treated similarly
> when passed from command line, so it's not necessary to handle each of them
> individually. Collapse them to one catch-all branch which will treat
> any not explicitly handled feature in format 'foo=val'.
>
> PS:
> Any unknown feature will be rejected by property setter so there is no
> need to check for unknown feature in cpu_x86_parse_featurestr(), therefore
> it's replaced by above mentioned catch-all handler.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
--
Eduardo
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH 02/16] target-i386: cleanup 'foo=val' feature handling
2014-02-11 9:14 ` Eduardo Habkost
@ 2014-02-11 14:28 ` Andreas Färber
0 siblings, 0 replies; 53+ messages in thread
From: Andreas Färber @ 2014-02-11 14:28 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: Igor Mammedov, qemu-devel
Am 11.02.2014 10:14, schrieb Eduardo Habkost:
> On Wed, Nov 27, 2013 at 11:28:42PM +0100, Igor Mammedov wrote:
>> features family, model, stepping, level, hv_spinlocks are treated similarly
>> when passed from command line, so it's not necessary to handle each of them
>> individually. Collapse them to one catch-all branch which will treat
>> any not explicitly handled feature in format 'foo=val'.
>>
>> PS:
>> Any unknown feature will be rejected by property setter so there is no
>> need to check for unknown feature in cpu_x86_parse_featurestr(), therefore
>> it's replaced by above mentioned catch-all handler.
>>
>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Too late:
http://git.qemu.org/?p=qemu.git;a=commit;h=d024d209045b912eb6127861fab2af6c64880efd
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH 03/16] target-i386: cpu: convert 'level' to static property
2013-11-27 22:28 [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties Igor Mammedov
2013-11-27 22:28 ` [Qemu-devel] [PATCH 01/16] target-i386: cleanup 'foo' feature handling' Igor Mammedov
2013-11-27 22:28 ` [Qemu-devel] [PATCH 02/16] target-i386: cleanup 'foo=val' feature handling Igor Mammedov
@ 2013-11-27 22:28 ` Igor Mammedov
2014-02-11 9:14 ` Eduardo Habkost
2013-11-27 22:28 ` [Qemu-devel] [PATCH 04/16] target-i386: cpu: convert 'xlevel' " Igor Mammedov
` (14 subsequent siblings)
17 siblings, 1 reply; 53+ messages in thread
From: Igor Mammedov @ 2013-11-27 22:28 UTC (permalink / raw)
To: qemu-devel; +Cc: afaerber
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
- cpu_x86_properties changed to x86_cpu_properties,
upstream rebase on top of it.
---
target-i386/cpu.c | 20 +-------------------
1 file changed, 1 insertion(+), 19 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 34d3968..0f1066a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1384,22 +1384,6 @@ static void x86_cpuid_version_set_stepping(Object *obj, Visitor *v,
env->cpuid_version |= value & 0xf;
}
-static void x86_cpuid_get_level(Object *obj, Visitor *v, void *opaque,
- const char *name, Error **errp)
-{
- X86CPU *cpu = X86_CPU(obj);
-
- visit_type_uint32(v, &cpu->env.cpuid_level, name, errp);
-}
-
-static void x86_cpuid_set_level(Object *obj, Visitor *v, void *opaque,
- const char *name, Error **errp)
-{
- X86CPU *cpu = X86_CPU(obj);
-
- visit_type_uint32(v, &cpu->env.cpuid_level, name, errp);
-}
-
static void x86_cpuid_get_xlevel(Object *obj, Visitor *v, void *opaque,
const char *name, Error **errp)
{
@@ -2676,9 +2660,6 @@ static void x86_cpu_initfn(Object *obj)
object_property_add(obj, "stepping", "int",
x86_cpuid_version_get_stepping,
x86_cpuid_version_set_stepping, NULL, NULL, NULL);
- object_property_add(obj, "level", "int",
- x86_cpuid_get_level,
- x86_cpuid_set_level, NULL, NULL, NULL);
object_property_add(obj, "xlevel", "int",
x86_cpuid_get_xlevel,
x86_cpuid_set_xlevel, NULL, NULL, NULL);
@@ -2750,6 +2731,7 @@ static Property x86_cpu_properties[] = {
DEFINE_PROP_BOOL("hv-vapic", X86CPU, hyperv_vapic, false),
DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, false),
DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
+ DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
DEFINE_PROP_END_OF_LIST()
};
--
1.8.3.1
^ permalink raw reply related [flat|nested] 53+ messages in thread* [Qemu-devel] [PATCH 04/16] target-i386: cpu: convert 'xlevel' to static property
2013-11-27 22:28 [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties Igor Mammedov
` (2 preceding siblings ...)
2013-11-27 22:28 ` [Qemu-devel] [PATCH 03/16] target-i386: cpu: convert 'level' to static property Igor Mammedov
@ 2013-11-27 22:28 ` Igor Mammedov
2014-02-11 9:15 ` Eduardo Habkost
2013-11-27 22:28 ` [Qemu-devel] [PATCH 05/16] target-i386: cpu: convert 'family' " Igor Mammedov
` (13 subsequent siblings)
17 siblings, 1 reply; 53+ messages in thread
From: Igor Mammedov @ 2013-11-27 22:28 UTC (permalink / raw)
To: qemu-devel; +Cc: afaerber
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v22:
- cpu_x86_properties changed to x86_cpu_properties,
upstream rebase on top of it.
---
target-i386/cpu.c | 20 +-------------------
1 file changed, 1 insertion(+), 19 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 0f1066a..8a1f786 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1384,22 +1384,6 @@ static void x86_cpuid_version_set_stepping(Object *obj, Visitor *v,
env->cpuid_version |= value & 0xf;
}
-static void x86_cpuid_get_xlevel(Object *obj, Visitor *v, void *opaque,
- const char *name, Error **errp)
-{
- X86CPU *cpu = X86_CPU(obj);
-
- visit_type_uint32(v, &cpu->env.cpuid_xlevel, name, errp);
-}
-
-static void x86_cpuid_set_xlevel(Object *obj, Visitor *v, void *opaque,
- const char *name, Error **errp)
-{
- X86CPU *cpu = X86_CPU(obj);
-
- visit_type_uint32(v, &cpu->env.cpuid_xlevel, name, errp);
-}
-
static char *x86_cpuid_get_vendor(Object *obj, Error **errp)
{
X86CPU *cpu = X86_CPU(obj);
@@ -2660,9 +2644,6 @@ static void x86_cpu_initfn(Object *obj)
object_property_add(obj, "stepping", "int",
x86_cpuid_version_get_stepping,
x86_cpuid_version_set_stepping, NULL, NULL, NULL);
- object_property_add(obj, "xlevel", "int",
- x86_cpuid_get_xlevel,
- x86_cpuid_set_xlevel, NULL, NULL, NULL);
object_property_add_str(obj, "vendor",
x86_cpuid_get_vendor,
x86_cpuid_set_vendor, NULL);
@@ -2732,6 +2713,7 @@ static Property x86_cpu_properties[] = {
DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, false),
DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
+ DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
DEFINE_PROP_END_OF_LIST()
};
--
1.8.3.1
^ permalink raw reply related [flat|nested] 53+ messages in thread* [Qemu-devel] [PATCH 05/16] target-i386: cpu: convert 'family' to static property
2013-11-27 22:28 [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties Igor Mammedov
` (3 preceding siblings ...)
2013-11-27 22:28 ` [Qemu-devel] [PATCH 04/16] target-i386: cpu: convert 'xlevel' " Igor Mammedov
@ 2013-11-27 22:28 ` Igor Mammedov
2014-02-11 9:37 ` Eduardo Habkost
2013-11-27 22:28 ` [Qemu-devel] [PATCH 06/16] target-i386: cpu: convert 'model' " Igor Mammedov
` (12 subsequent siblings)
17 siblings, 1 reply; 53+ messages in thread
From: Igor Mammedov @ 2013-11-27 22:28 UTC (permalink / raw)
To: qemu-devel; +Cc: afaerber
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v3:
- cpu_x86_properties changed to x86_cpu_properties,
upstream rebase on top of it.
v2:
- afaerber: inline property definition inside of
property array.
---
target-i386/cpu.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 8a1f786..7483f28 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1313,6 +1313,12 @@ static void x86_cpuid_version_set_family(Object *obj, Visitor *v, void *opaque,
}
}
+static PropertyInfo qdev_prop_family = {
+ .name = "uint32",
+ .get = x86_cpuid_version_get_family,
+ .set = x86_cpuid_version_set_family,
+};
+
static void x86_cpuid_version_get_model(Object *obj, Visitor *v, void *opaque,
const char *name, Error **errp)
{
@@ -2635,9 +2641,6 @@ static void x86_cpu_initfn(Object *obj)
cs->env_ptr = env;
cpu_exec_init(env);
- object_property_add(obj, "family", "int",
- x86_cpuid_version_get_family,
- x86_cpuid_version_set_family, NULL, NULL, NULL);
object_property_add(obj, "model", "int",
x86_cpuid_version_get_model,
x86_cpuid_version_set_model, NULL, NULL, NULL);
@@ -2714,6 +2717,7 @@ static Property x86_cpu_properties[] = {
DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
+ { .name = "family", .info = &qdev_prop_family },
DEFINE_PROP_END_OF_LIST()
};
--
1.8.3.1
^ permalink raw reply related [flat|nested] 53+ messages in thread* [Qemu-devel] [PATCH 06/16] target-i386: cpu: convert 'model' to static property
2013-11-27 22:28 [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties Igor Mammedov
` (4 preceding siblings ...)
2013-11-27 22:28 ` [Qemu-devel] [PATCH 05/16] target-i386: cpu: convert 'family' " Igor Mammedov
@ 2013-11-27 22:28 ` Igor Mammedov
2014-02-11 9:40 ` Eduardo Habkost
2013-11-27 22:28 ` [Qemu-devel] [PATCH 07/16] target-i386: cpu: convert 'stepping' " Igor Mammedov
` (11 subsequent siblings)
17 siblings, 1 reply; 53+ messages in thread
From: Igor Mammedov @ 2013-11-27 22:28 UTC (permalink / raw)
To: qemu-devel; +Cc: afaerber
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v3:
- cpu_x86_properties changed to x86_cpu_properties,
upstream rebase on top of it.
v2:
- afaerber: inline property definition inside of
property array.
---
target-i386/cpu.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 7483f28..efbcaba 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1354,6 +1354,12 @@ static void x86_cpuid_version_set_model(Object *obj, Visitor *v, void *opaque,
env->cpuid_version |= ((value & 0xf) << 4) | ((value >> 4) << 16);
}
+static PropertyInfo qdev_prop_model = {
+ .name = "uint32",
+ .get = x86_cpuid_version_get_model,
+ .set = x86_cpuid_version_set_model,
+};
+
static void x86_cpuid_version_get_stepping(Object *obj, Visitor *v,
void *opaque, const char *name,
Error **errp)
@@ -2641,9 +2647,6 @@ static void x86_cpu_initfn(Object *obj)
cs->env_ptr = env;
cpu_exec_init(env);
- object_property_add(obj, "model", "int",
- x86_cpuid_version_get_model,
- x86_cpuid_version_set_model, NULL, NULL, NULL);
object_property_add(obj, "stepping", "int",
x86_cpuid_version_get_stepping,
x86_cpuid_version_set_stepping, NULL, NULL, NULL);
@@ -2718,6 +2721,7 @@ static Property x86_cpu_properties[] = {
DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
{ .name = "family", .info = &qdev_prop_family },
+ { .name = "model", .info = &qdev_prop_model },
DEFINE_PROP_END_OF_LIST()
};
--
1.8.3.1
^ permalink raw reply related [flat|nested] 53+ messages in thread* [Qemu-devel] [PATCH 07/16] target-i386: cpu: convert 'stepping' to static property
2013-11-27 22:28 [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties Igor Mammedov
` (5 preceding siblings ...)
2013-11-27 22:28 ` [Qemu-devel] [PATCH 06/16] target-i386: cpu: convert 'model' " Igor Mammedov
@ 2013-11-27 22:28 ` Igor Mammedov
2014-02-11 9:40 ` Eduardo Habkost
2013-11-27 22:28 ` [Qemu-devel] [PATCH 08/16] target-i386: cpu: convert 'vendor' " Igor Mammedov
` (10 subsequent siblings)
17 siblings, 1 reply; 53+ messages in thread
From: Igor Mammedov @ 2013-11-27 22:28 UTC (permalink / raw)
To: qemu-devel; +Cc: afaerber
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v3:
- cpu_x86_properties changed to x86_cpu_properties,
upstream rebase on top of it.
v2:
- afaerber: inline property definition inside of
property array.
---
target-i386/cpu.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index efbcaba..5fb6c68 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1396,6 +1396,12 @@ static void x86_cpuid_version_set_stepping(Object *obj, Visitor *v,
env->cpuid_version |= value & 0xf;
}
+static PropertyInfo qdev_prop_stepping = {
+ .name = "uint32",
+ .get = x86_cpuid_version_get_stepping,
+ .set = x86_cpuid_version_set_stepping,
+};
+
static char *x86_cpuid_get_vendor(Object *obj, Error **errp)
{
X86CPU *cpu = X86_CPU(obj);
@@ -2647,9 +2653,6 @@ static void x86_cpu_initfn(Object *obj)
cs->env_ptr = env;
cpu_exec_init(env);
- object_property_add(obj, "stepping", "int",
- x86_cpuid_version_get_stepping,
- x86_cpuid_version_set_stepping, NULL, NULL, NULL);
object_property_add_str(obj, "vendor",
x86_cpuid_get_vendor,
x86_cpuid_set_vendor, NULL);
@@ -2722,6 +2725,7 @@ static Property x86_cpu_properties[] = {
DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
{ .name = "family", .info = &qdev_prop_family },
{ .name = "model", .info = &qdev_prop_model },
+ { .name = "stepping", .info = &qdev_prop_stepping },
DEFINE_PROP_END_OF_LIST()
};
--
1.8.3.1
^ permalink raw reply related [flat|nested] 53+ messages in thread* [Qemu-devel] [PATCH 08/16] target-i386: cpu: convert 'vendor' to static property
2013-11-27 22:28 [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties Igor Mammedov
` (6 preceding siblings ...)
2013-11-27 22:28 ` [Qemu-devel] [PATCH 07/16] target-i386: cpu: convert 'stepping' " Igor Mammedov
@ 2013-11-27 22:28 ` Igor Mammedov
2014-02-11 11:31 ` Eduardo Habkost
2013-11-27 22:28 ` [Qemu-devel] [PATCH 09/16] target-i386: cpu: convert 'model-id' " Igor Mammedov
` (9 subsequent siblings)
17 siblings, 1 reply; 53+ messages in thread
From: Igor Mammedov @ 2013-11-27 22:28 UTC (permalink / raw)
To: qemu-devel; +Cc: afaerber
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v4:
- fix invalid use of error_is_set(errp) if errp == NULL
v3:
- cpu_x86_properties changed to x86_cpu_properties
upstream, rebase on top of it.
v2:
- afaerber: inline property definition inside of
property array.
---
target-i386/cpu.c | 29 ++++++++++++++++++++++-------
1 file changed, 22 insertions(+), 7 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 5fb6c68..1ad1087 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1402,7 +1402,8 @@ static PropertyInfo qdev_prop_stepping = {
.set = x86_cpuid_version_set_stepping,
};
-static char *x86_cpuid_get_vendor(Object *obj, Error **errp)
+static void x86_cpuid_get_vendor(Object *obj, Visitor *v, void *opaque,
+ const char *name, Error **errp)
{
X86CPU *cpu = X86_CPU(obj);
CPUX86State *env = &cpu->env;
@@ -1411,16 +1412,25 @@ static char *x86_cpuid_get_vendor(Object *obj, Error **errp)
value = (char *)g_malloc(CPUID_VENDOR_SZ + 1);
x86_cpu_vendor_words2str(value, env->cpuid_vendor1, env->cpuid_vendor2,
env->cpuid_vendor3);
- return value;
+ visit_type_str(v, &value, name, errp);
+ g_free(value);
}
-static void x86_cpuid_set_vendor(Object *obj, const char *value,
- Error **errp)
+static void x86_cpuid_set_vendor(Object *obj, Visitor *v, void *opaque,
+ const char *name, Error **errp)
{
X86CPU *cpu = X86_CPU(obj);
CPUX86State *env = &cpu->env;
+ Error *err = NULL;
+ char *value;
int i;
+ visit_type_str(v, &value, name, &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
+
if (strlen(value) != CPUID_VENDOR_SZ) {
error_set(errp, QERR_PROPERTY_VALUE_BAD, "",
"vendor", value);
@@ -1435,8 +1445,15 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
env->cpuid_vendor2 |= ((uint8_t)value[i + 4]) << (8 * i);
env->cpuid_vendor3 |= ((uint8_t)value[i + 8]) << (8 * i);
}
+ g_free(value);
}
+static PropertyInfo qdev_prop_vendor = {
+ .name = "string",
+ .get = x86_cpuid_get_vendor,
+ .set = x86_cpuid_set_vendor,
+};
+
static char *x86_cpuid_get_model_id(Object *obj, Error **errp)
{
X86CPU *cpu = X86_CPU(obj);
@@ -2653,9 +2670,6 @@ static void x86_cpu_initfn(Object *obj)
cs->env_ptr = env;
cpu_exec_init(env);
- object_property_add_str(obj, "vendor",
- x86_cpuid_get_vendor,
- x86_cpuid_set_vendor, NULL);
object_property_add_str(obj, "model-id",
x86_cpuid_get_model_id,
x86_cpuid_set_model_id, NULL);
@@ -2726,6 +2740,7 @@ static Property x86_cpu_properties[] = {
{ .name = "family", .info = &qdev_prop_family },
{ .name = "model", .info = &qdev_prop_model },
{ .name = "stepping", .info = &qdev_prop_stepping },
+ { .name = "vendor", .info = &qdev_prop_vendor },
DEFINE_PROP_END_OF_LIST()
};
--
1.8.3.1
^ permalink raw reply related [flat|nested] 53+ messages in thread* [Qemu-devel] [PATCH 09/16] target-i386: cpu: convert 'model-id' to static property
2013-11-27 22:28 [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties Igor Mammedov
` (7 preceding siblings ...)
2013-11-27 22:28 ` [Qemu-devel] [PATCH 08/16] target-i386: cpu: convert 'vendor' " Igor Mammedov
@ 2013-11-27 22:28 ` Igor Mammedov
2014-02-11 11:36 ` Eduardo Habkost
2013-11-27 22:28 ` [Qemu-devel] [PATCH 10/16] target-i386: cpu: convert 'tsc-frequency' " Igor Mammedov
` (8 subsequent siblings)
17 siblings, 1 reply; 53+ messages in thread
From: Igor Mammedov @ 2013-11-27 22:28 UTC (permalink / raw)
To: qemu-devel; +Cc: afaerber
* check "if (model_id == NULL)" looks unnecessary now, since all
builtin model-ids are not NULL and user shouldn't be able to set
it NULL (cpumodel string parsing code takes care of it, if feature
is specified as "model-id=" on command line, its parsing will
result in an empty string as value).
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v4:
- fix invalid use of error_is_set(errp) if errp == NULL
v3:
- cpu_x86_properties changed to x86_cpu_properties
upstream, rebase on top of it.
v2:
- afaerber: inline property definition inside of
property array.
---
target-i386/cpu.c | 29 ++++++++++++++++++++---------
1 file changed, 20 insertions(+), 9 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 1ad1087..4389ffa 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1454,7 +1454,8 @@ static PropertyInfo qdev_prop_vendor = {
.set = x86_cpuid_set_vendor,
};
-static char *x86_cpuid_get_model_id(Object *obj, Error **errp)
+static void x86_cpuid_get_model_id(Object *obj, Visitor *v, void *opaque,
+ const char *name, Error **errp)
{
X86CPU *cpu = X86_CPU(obj);
CPUX86State *env = &cpu->env;
@@ -1466,18 +1467,23 @@ static char *x86_cpuid_get_model_id(Object *obj, Error **errp)
value[i] = env->cpuid_model[i >> 2] >> (8 * (i & 3));
}
value[48] = '\0';
- return value;
+ visit_type_str(v, &value, name, errp);
+ g_free(value);
}
-static void x86_cpuid_set_model_id(Object *obj, const char *model_id,
- Error **errp)
+static void x86_cpuid_set_model_id(Object *obj, Visitor *v, void *opaque,
+ const char *name, Error **errp)
{
X86CPU *cpu = X86_CPU(obj);
CPUX86State *env = &cpu->env;
+ Error *err = NULL;
int c, len, i;
+ char *model_id;
- if (model_id == NULL) {
- model_id = "";
+ visit_type_str(v, &model_id, name, &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
}
len = strlen(model_id);
memset(env->cpuid_model, 0, 48);
@@ -1489,8 +1495,15 @@ static void x86_cpuid_set_model_id(Object *obj, const char *model_id,
}
env->cpuid_model[i >> 2] |= c << (8 * (i & 3));
}
+ g_free(model_id);
}
+static PropertyInfo qdev_prop_model_id = {
+ .name = "string",
+ .get = x86_cpuid_get_model_id,
+ .set = x86_cpuid_set_model_id,
+};
+
static void x86_cpuid_get_tsc_freq(Object *obj, Visitor *v, void *opaque,
const char *name, Error **errp)
{
@@ -2670,9 +2683,6 @@ static void x86_cpu_initfn(Object *obj)
cs->env_ptr = env;
cpu_exec_init(env);
- object_property_add_str(obj, "model-id",
- x86_cpuid_get_model_id,
- x86_cpuid_set_model_id, NULL);
object_property_add(obj, "tsc-frequency", "int",
x86_cpuid_get_tsc_freq,
x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
@@ -2741,6 +2751,7 @@ static Property x86_cpu_properties[] = {
{ .name = "model", .info = &qdev_prop_model },
{ .name = "stepping", .info = &qdev_prop_stepping },
{ .name = "vendor", .info = &qdev_prop_vendor },
+ { .name = "model-id", .info = &qdev_prop_model_id },
DEFINE_PROP_END_OF_LIST()
};
--
1.8.3.1
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [Qemu-devel] [PATCH 09/16] target-i386: cpu: convert 'model-id' to static property
2013-11-27 22:28 ` [Qemu-devel] [PATCH 09/16] target-i386: cpu: convert 'model-id' " Igor Mammedov
@ 2014-02-11 11:36 ` Eduardo Habkost
0 siblings, 0 replies; 53+ messages in thread
From: Eduardo Habkost @ 2014-02-11 11:36 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, afaerber
On Wed, Nov 27, 2013 at 11:28:49PM +0100, Igor Mammedov wrote:
> * check "if (model_id == NULL)" looks unnecessary now, since all
> builtin model-ids are not NULL and user shouldn't be able to set
> it NULL (cpumodel string parsing code takes care of it, if feature
> is specified as "model-id=" on command line, its parsing will
> result in an empty string as value).
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
--
Eduardo
^ permalink raw reply [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH 10/16] target-i386: cpu: convert 'tsc-frequency' to static property
2013-11-27 22:28 [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties Igor Mammedov
` (8 preceding siblings ...)
2013-11-27 22:28 ` [Qemu-devel] [PATCH 09/16] target-i386: cpu: convert 'model-id' " Igor Mammedov
@ 2013-11-27 22:28 ` Igor Mammedov
2014-02-11 11:36 ` Eduardo Habkost
2013-11-27 22:28 ` [Qemu-devel] [PATCH 11/16] target-i386: set [+-]feature using static properties Igor Mammedov
` (7 subsequent siblings)
17 siblings, 1 reply; 53+ messages in thread
From: Igor Mammedov @ 2013-11-27 22:28 UTC (permalink / raw)
To: qemu-devel; +Cc: afaerber
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v3:
- cpu_x86_properties changed to x86_cpu_properties
upstream, rebase on top of it.
v2:
- afaerber: inline property definition inside of
property array.
---
target-i386/cpu.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 4389ffa..b4f616e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1535,6 +1535,12 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
cpu->env.tsc_khz = value / 1000;
}
+static PropertyInfo qdev_prop_tsc_freq = {
+ .name = "int64",
+ .get = x86_cpuid_get_tsc_freq,
+ .set = x86_cpuid_set_tsc_freq,
+};
+
static void x86_cpuid_get_apic_id(Object *obj, Visitor *v, void *opaque,
const char *name, Error **errp)
{
@@ -2683,9 +2689,6 @@ static void x86_cpu_initfn(Object *obj)
cs->env_ptr = env;
cpu_exec_init(env);
- object_property_add(obj, "tsc-frequency", "int",
- x86_cpuid_get_tsc_freq,
- x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
object_property_add(obj, "apic-id", "int",
x86_cpuid_get_apic_id,
x86_cpuid_set_apic_id, NULL, NULL, NULL);
@@ -2752,6 +2755,7 @@ static Property x86_cpu_properties[] = {
{ .name = "stepping", .info = &qdev_prop_stepping },
{ .name = "vendor", .info = &qdev_prop_vendor },
{ .name = "model-id", .info = &qdev_prop_model_id },
+ { .name = "tsc-frequency", .info = &qdev_prop_tsc_freq },
DEFINE_PROP_END_OF_LIST()
};
--
1.8.3.1
^ permalink raw reply related [flat|nested] 53+ messages in thread* [Qemu-devel] [PATCH 11/16] target-i386: set [+-]feature using static properties
2013-11-27 22:28 [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties Igor Mammedov
` (9 preceding siblings ...)
2013-11-27 22:28 ` [Qemu-devel] [PATCH 10/16] target-i386: cpu: convert 'tsc-frequency' " Igor Mammedov
@ 2013-11-27 22:28 ` Igor Mammedov
2013-11-27 22:28 ` [Qemu-devel] [PATCH 12/16] qdev: introduce qdev_prop_find_bit() Igor Mammedov
` (6 subsequent siblings)
17 siblings, 0 replies; 53+ messages in thread
From: Igor Mammedov @ 2013-11-27 22:28 UTC (permalink / raw)
To: qemu-devel; +Cc: afaerber
* Define static properties for cpuid feature bits
* property names of CPUID feature bits are changed to have "feat-"
prefix, so that it would be easy to distinguish them from other
properties.
* Convert [+-]cpuid_features to a set(QDict) of key, value pairs, where
+foo => (feat-foo, on)
-foo => (feat-foo, off)
[+-] unknown foo => (feat-foo, on/off) - it's expected to be rejected
by property setter later
QDict is used as convinience sturcture to keep '-foo' semantic.
Once all +-foo are parsed, collected features are applied to CPU instance.
* When parsing 'kvmclock' feature, set additional feat-kvmclock2 feature
in cpu_x86_parse_featurestr() to mantain legacy kvmclock flag behavior
* Cleanup unused anymore:
add_flagname_to_bitmaps(), lookup_feature(), altcmp(), sstrcmp(),
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v8:
- add feat-kvm-pv-unhalt introduced by
f010bc643 "target-i386: add feature kvm_pv_unhalt"
v7:
- fix mispelling "feat-kvm-steal-tm" -> "feat-kvm-steal-time"
- cpu_x86_properties changed to x86_cpu_properties
upstream, rebase on top of it.
v6:
- substitute '_' with '-' for +-foo feature bits as well
- change "f-" feature prefix to "feat-" (req: afaerber)
- replace F_* macros with a single X86CPU_FEAT() macro
- simplify F_XXX macros to set default value to 0, defaults to be handled
in initfn()
- ammend F_XXX macros to use feature-words
- kvmclock fix endless loop on compat kvmclock2 append
v5:
- instead of introducing composit f-kvmclock property to maintain
legace behavior, set additional f-kvmclock2 in cpu_x86_parse_featurestr()
when kvmclock is parsed.
v4:
- major patch reordering, rebasing on current qom-cpu-next
- merged patches: "define static properties..." and "set [+-]feature..."
- merge in "make 'f-kvmclock' compatible..." to aviod behavior breakage
- use register name in feature macros, so that if we rename cpuid_* fields,
it wouldn't involve mass rename in features array.
- when converting feature names into property names, replace '_' with '-',
Requested-By: Don Slutz <Don@cloudswitch.com>,
mgs-id: <5085D4AA.1060208@CloudSwitch.Com>
v3:
- new static properties after rebase:
- add features "f-rdseed" & "f-adx"
- add features added by c8acc380
- add features f-kvm_steal_tm and f-kvmclock_stable
- ext4 features f-xstore, f-xstore-en, f-xcrypt, f-xcrypt-en,
f-ace2, f-ace2-en, f-phe, f-phe-en, f-pmm, f-pmm-en
- f-hypervisor set default to false as for the rest of features
- define helper FEAT for declaring CPUID feature properties to
make shorter lines (<80 characters)
v2:
- use X86CPU as a type to count of offset correctly, because env field
isn't starting at CPUstate beginning, but located after it.
---
target-i386/cpu.c | 303 +++++++++++++++++++++++++++++++++++-------------------
1 file changed, 197 insertions(+), 106 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b4f616e..1503e9a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -401,85 +401,6 @@ void host_cpuid(uint32_t function, uint32_t count,
#endif
}
-#define iswhite(c) ((c) && ((c) <= ' ' || '~' < (c)))
-
-/* general substring compare of *[s1..e1) and *[s2..e2). sx is start of
- * a substring. ex if !NULL points to the first char after a substring,
- * otherwise the string is assumed to sized by a terminating nul.
- * Return lexical ordering of *s1:*s2.
- */
-static int sstrcmp(const char *s1, const char *e1, const char *s2,
- const char *e2)
-{
- for (;;) {
- if (!*s1 || !*s2 || *s1 != *s2)
- return (*s1 - *s2);
- ++s1, ++s2;
- if (s1 == e1 && s2 == e2)
- return (0);
- else if (s1 == e1)
- return (*s2);
- else if (s2 == e2)
- return (*s1);
- }
-}
-
-/* compare *[s..e) to *altstr. *altstr may be a simple string or multiple
- * '|' delimited (possibly empty) strings in which case search for a match
- * within the alternatives proceeds left to right. Return 0 for success,
- * non-zero otherwise.
- */
-static int altcmp(const char *s, const char *e, const char *altstr)
-{
- const char *p, *q;
-
- for (q = p = altstr; ; ) {
- while (*p && *p != '|')
- ++p;
- if ((q == p && !*s) || (q != p && !sstrcmp(s, e, q, p)))
- return (0);
- if (!*p)
- return (1);
- else
- q = ++p;
- }
-}
-
-/* search featureset for flag *[s..e), if found set corresponding bit in
- * *pval and return true, otherwise return false
- */
-static bool lookup_feature(uint32_t *pval, const char *s, const char *e,
- const char **featureset)
-{
- uint32_t mask;
- const char **ppc;
- bool found = false;
-
- for (mask = 1, ppc = featureset; mask; mask <<= 1, ++ppc) {
- if (*ppc && !altcmp(s, e, *ppc)) {
- *pval |= mask;
- found = true;
- }
- }
- return found;
-}
-
-static void add_flagname_to_bitmaps(const char *flagname,
- FeatureWordArray words)
-{
- FeatureWord w;
- for (w = 0; w < FEATURE_WORDS; w++) {
- FeatureWordInfo *wi = &feature_word_info[w];
- if (wi->feat_names &&
- lookup_feature(&words[w], flagname, NULL, wi->feat_names)) {
- break;
- }
- }
- if (w == FEATURE_WORDS) {
- fprintf(stderr, "CPU feature %s not found\n", flagname);
- }
-}
-
typedef struct x86_def_t {
const char *name;
uint32_t level;
@@ -1710,24 +1631,48 @@ static inline void feat2prop(char *s)
static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp)
{
char *featurestr; /* Single 'key=value" string being parsed */
- /* Features to be added */
- FeatureWordArray plus_features = { 0 };
- /* Features to be removed */
- FeatureWordArray minus_features = { 0 };
uint32_t numvalue;
- CPUX86State *env = &cpu->env;
+ QDict *props = qdict_new();
+ const QDictEntry *ent;
featurestr = features ? strtok(features, ",") : NULL;
while (featurestr) {
char *val;
- if (featurestr[0] == '+') {
- add_flagname_to_bitmaps(featurestr + 1, plus_features);
- } else if (featurestr[0] == '-') {
- add_flagname_to_bitmaps(featurestr + 1, minus_features);
+ feat2prop(featurestr);
+ if (featurestr[0] == '+' || featurestr[0] == '-') {
+ const gchar *feat = featurestr + 1;
+ gchar *cpuid_fname = NULL;
+ bool set_kvmclock2 = false;
+
+ if (strncmp(feat, "feat-", 5)) {
+ cpuid_fname = g_strconcat("feat-", feat, NULL);
+ feat = cpuid_fname;
+ }
+
+ if (!strcmp(feat, "feat-kvmclock")) {
+ set_kvmclock2 = true;
+ }
+
+ rep_feat_set:
+ if (featurestr[0] == '+') {
+ /* preseve legacy behaviour, if feature was disabled once
+ * do not allow to enable it again */
+ if (!qdict_haskey(props, feat)) {
+ qdict_put(props, feat, qstring_from_str("on"));
+ }
+ } else {
+ qdict_put(props, feat, qstring_from_str("off"));
+ }
+
+ if (set_kvmclock2) {
+ feat = "feat-kvmclock2";
+ set_kvmclock2 = false;
+ goto rep_feat_set;
+ }
+ g_free(cpuid_fname);
} else if ((val = strchr(featurestr, '='))) {
*val = 0; val++;
- feat2prop(featurestr);
if (!strcmp(featurestr, "xlevel")) {
char *err;
char num[32];
@@ -1778,7 +1723,6 @@ static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp)
object_property_parse(OBJECT(cpu), val, featurestr, errp);
}
} else {
- feat2prop(featurestr);
object_property_parse(OBJECT(cpu), "on", featurestr, errp);
}
if (error_is_set(errp)) {
@@ -1786,24 +1730,20 @@ static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp)
}
featurestr = strtok(NULL, ",");
}
- env->features[FEAT_1_EDX] |= plus_features[FEAT_1_EDX];
- env->features[FEAT_1_ECX] |= plus_features[FEAT_1_ECX];
- env->features[FEAT_8000_0001_EDX] |= plus_features[FEAT_8000_0001_EDX];
- env->features[FEAT_8000_0001_ECX] |= plus_features[FEAT_8000_0001_ECX];
- env->features[FEAT_C000_0001_EDX] |= plus_features[FEAT_C000_0001_EDX];
- env->features[FEAT_KVM] |= plus_features[FEAT_KVM];
- env->features[FEAT_SVM] |= plus_features[FEAT_SVM];
- env->features[FEAT_7_0_EBX] |= plus_features[FEAT_7_0_EBX];
- env->features[FEAT_1_EDX] &= ~minus_features[FEAT_1_EDX];
- env->features[FEAT_1_ECX] &= ~minus_features[FEAT_1_ECX];
- env->features[FEAT_8000_0001_EDX] &= ~minus_features[FEAT_8000_0001_EDX];
- env->features[FEAT_8000_0001_ECX] &= ~minus_features[FEAT_8000_0001_ECX];
- env->features[FEAT_C000_0001_EDX] &= ~minus_features[FEAT_C000_0001_EDX];
- env->features[FEAT_KVM] &= ~minus_features[FEAT_KVM];
- env->features[FEAT_SVM] &= ~minus_features[FEAT_SVM];
- env->features[FEAT_7_0_EBX] &= ~minus_features[FEAT_7_0_EBX];
+
+ for (ent = qdict_first(props); ent; ent = qdict_next(props, ent)) {
+ const QString *qval = qobject_to_qstring(qdict_entry_value(ent));
+ /* TODO: switch to using global properties after subclasses are done */
+ object_property_parse(OBJECT(cpu), qstring_get_str(qval),
+ qdict_entry_key(ent), errp);
+ if (error_is_set(errp)) {
+ QDECREF(props);
+ return;
+ }
+ }
out:
+ QDECREF(props);
return;
}
@@ -2741,6 +2681,9 @@ static void x86_cpu_synchronize_from_tb(CPUState *cs, TranslationBlock *tb)
cpu->env.eip = tb->pc - tb->cs_base;
}
+#define X86CPU_FEAT(_name, _bit, _leaf) \
+ DEFINE_PROP_BIT(_name, X86CPU, env.features[_leaf], _bit, 0)
+
static Property x86_cpu_properties[] = {
DEFINE_PROP_BOOL("pmu", X86CPU, enable_pmu, false),
{ .name = "hv-spinlocks", .info = &qdev_prop_spinlocks },
@@ -2756,6 +2699,154 @@ static Property x86_cpu_properties[] = {
{ .name = "vendor", .info = &qdev_prop_vendor },
{ .name = "model-id", .info = &qdev_prop_model_id },
{ .name = "tsc-frequency", .info = &qdev_prop_tsc_freq },
+ X86CPU_FEAT("feat-fpu", 0, FEAT_1_EDX),
+ X86CPU_FEAT("feat-vme", 1, FEAT_1_EDX),
+ X86CPU_FEAT("feat-de", 2, FEAT_1_EDX),
+ X86CPU_FEAT("feat-pse", 3, FEAT_1_EDX),
+ X86CPU_FEAT("feat-tsc", 4, FEAT_1_EDX),
+ X86CPU_FEAT("feat-msr", 5, FEAT_1_EDX),
+ X86CPU_FEAT("feat-pae", 6, FEAT_1_EDX),
+ X86CPU_FEAT("feat-mce", 7, FEAT_1_EDX),
+ X86CPU_FEAT("feat-cx8", 8, FEAT_1_EDX),
+ X86CPU_FEAT("feat-apic", 9, FEAT_1_EDX),
+ X86CPU_FEAT("feat-sep", 11, FEAT_1_EDX),
+ X86CPU_FEAT("feat-mtrr", 12, FEAT_1_EDX),
+ X86CPU_FEAT("feat-pge", 13, FEAT_1_EDX),
+ X86CPU_FEAT("feat-mca", 14, FEAT_1_EDX),
+ X86CPU_FEAT("feat-cmov", 15, FEAT_1_EDX),
+ X86CPU_FEAT("feat-pat", 16, FEAT_1_EDX),
+ X86CPU_FEAT("feat-pse36", 17, FEAT_1_EDX),
+ /* Intel psn */
+ X86CPU_FEAT("feat-pn", 18, FEAT_1_EDX),
+ /* Intel clfsh */
+ X86CPU_FEAT("feat-clflush", 19, FEAT_1_EDX),
+ /* Intel dts */
+ X86CPU_FEAT("feat-ds", 21, FEAT_1_EDX),
+ X86CPU_FEAT("feat-acpi", 22, FEAT_1_EDX),
+ X86CPU_FEAT("feat-mmx", 23, FEAT_1_EDX),
+ X86CPU_FEAT("feat-fxsr", 24, FEAT_1_EDX),
+ X86CPU_FEAT("feat-sse", 25, FEAT_1_EDX),
+ X86CPU_FEAT("feat-sse2", 26, FEAT_1_EDX),
+ X86CPU_FEAT("feat-ss", 27, FEAT_1_EDX),
+ /* Intel htt */
+ X86CPU_FEAT("feat-ht", 28, FEAT_1_EDX),
+ X86CPU_FEAT("feat-tm", 29, FEAT_1_EDX),
+ X86CPU_FEAT("feat-ia64", 30, FEAT_1_EDX),
+ X86CPU_FEAT("feat-pbe", 31, FEAT_1_EDX),
+ /* Intel */
+ X86CPU_FEAT("feat-pni", 0, FEAT_1_ECX),
+ /* AMD sse3 */
+ X86CPU_FEAT("feat-sse3", 0, FEAT_1_ECX),
+ X86CPU_FEAT("feat-pclmulqdq", 1, FEAT_1_ECX),
+ X86CPU_FEAT("feat-pclmuldq", 1, FEAT_1_ECX),
+ X86CPU_FEAT("feat-dtes64", 2, FEAT_1_ECX),
+ X86CPU_FEAT("feat-monitor", 3, FEAT_1_ECX),
+ X86CPU_FEAT("feat-ds-cpl", 4, FEAT_1_ECX),
+ X86CPU_FEAT("feat-vmx", 5, FEAT_1_ECX),
+ X86CPU_FEAT("feat-smx", 6, FEAT_1_ECX),
+ X86CPU_FEAT("feat-est", 7, FEAT_1_ECX),
+ X86CPU_FEAT("feat-tm2", 8, FEAT_1_ECX),
+ X86CPU_FEAT("feat-ssse3", 9, FEAT_1_ECX),
+ X86CPU_FEAT("feat-cid", 10, FEAT_1_ECX),
+ X86CPU_FEAT("feat-fma", 12, FEAT_1_ECX),
+ X86CPU_FEAT("feat-cx16", 13, FEAT_1_ECX),
+ X86CPU_FEAT("feat-xtpr", 14, FEAT_1_ECX),
+ X86CPU_FEAT("feat-pdcm", 15, FEAT_1_ECX),
+ X86CPU_FEAT("feat-pcid", 17, FEAT_1_ECX),
+ X86CPU_FEAT("feat-dca", 18, FEAT_1_ECX),
+ X86CPU_FEAT("feat-sse4-1", 19, FEAT_1_ECX),
+ X86CPU_FEAT("feat-sse4.1", 19, FEAT_1_ECX),
+ X86CPU_FEAT("feat-sse4-2", 20, FEAT_1_ECX),
+ X86CPU_FEAT("feat-sse4.2", 20, FEAT_1_ECX),
+ X86CPU_FEAT("feat-x2apic", 21, FEAT_1_ECX),
+ X86CPU_FEAT("feat-movbe", 22, FEAT_1_ECX),
+ X86CPU_FEAT("feat-popcnt", 23, FEAT_1_ECX),
+ X86CPU_FEAT("feat-tsc-deadline", 24, FEAT_1_ECX),
+ X86CPU_FEAT("feat-aes", 25, FEAT_1_ECX),
+ X86CPU_FEAT("feat-xsave", 26, FEAT_1_ECX),
+ X86CPU_FEAT("feat-osxsave", 27, FEAT_1_ECX),
+ X86CPU_FEAT("feat-avx", 28, FEAT_1_ECX),
+ X86CPU_FEAT("feat-f16c", 29, FEAT_1_ECX),
+ X86CPU_FEAT("feat-rdrand", 30, FEAT_1_ECX),
+ X86CPU_FEAT("feat-hypervisor", 31, FEAT_1_ECX),
+ X86CPU_FEAT("feat-syscall", 11, FEAT_8000_0001_EDX),
+ X86CPU_FEAT("feat-nx", 20, FEAT_8000_0001_EDX),
+ X86CPU_FEAT("feat-xd", 20, FEAT_8000_0001_EDX),
+ X86CPU_FEAT("feat-mmxext", 22, FEAT_8000_0001_EDX),
+ X86CPU_FEAT("feat-fxsr-opt", 25, FEAT_8000_0001_EDX),
+ X86CPU_FEAT("feat-ffxsr", 25, FEAT_8000_0001_EDX),
+ /* AMD Page1GB */
+ X86CPU_FEAT("feat-pdpe1gb", 26, FEAT_8000_0001_EDX),
+ X86CPU_FEAT("feat-rdtscp", 27, FEAT_8000_0001_EDX),
+ X86CPU_FEAT("feat-lm", 29, FEAT_8000_0001_EDX),
+ X86CPU_FEAT("feat-i64", 29, FEAT_8000_0001_EDX),
+ X86CPU_FEAT("feat-3dnowext", 30, FEAT_8000_0001_EDX),
+ X86CPU_FEAT("feat-3dnow", 31, FEAT_8000_0001_EDX),
+ /* AMD LahfSahf */
+ X86CPU_FEAT("feat-lahf-lm", 0, FEAT_8000_0001_ECX),
+ X86CPU_FEAT("feat-cmp-legacy", 1, FEAT_8000_0001_ECX),
+ X86CPU_FEAT("feat-svm", 2, FEAT_8000_0001_ECX),
+ /* AMD ExtApicSpace */
+ X86CPU_FEAT("feat-extapic", 3, FEAT_8000_0001_ECX),
+ /* AMD AltMovCr8 */
+ X86CPU_FEAT("feat-cr8legacy", 4, FEAT_8000_0001_ECX),
+ X86CPU_FEAT("feat-abm", 5, FEAT_8000_0001_ECX),
+ X86CPU_FEAT("feat-sse4a", 6, FEAT_8000_0001_ECX),
+ X86CPU_FEAT("feat-misalignsse", 7, FEAT_8000_0001_ECX),
+ X86CPU_FEAT("feat-3dnowprefetch", 8, FEAT_8000_0001_ECX),
+ X86CPU_FEAT("feat-osvw", 9, FEAT_8000_0001_ECX),
+ X86CPU_FEAT("feat-ibs", 10, FEAT_8000_0001_ECX),
+ X86CPU_FEAT("feat-xop", 11, FEAT_8000_0001_ECX),
+ X86CPU_FEAT("feat-skinit", 12, FEAT_8000_0001_ECX),
+ X86CPU_FEAT("feat-wdt", 13, FEAT_8000_0001_ECX),
+ X86CPU_FEAT("feat-lwp", 15, FEAT_8000_0001_ECX),
+ X86CPU_FEAT("feat-fma4", 16, FEAT_8000_0001_ECX),
+ X86CPU_FEAT("feat-tce", 17, FEAT_8000_0001_ECX),
+ X86CPU_FEAT("feat-nodeid-msr", 19, FEAT_8000_0001_ECX),
+ X86CPU_FEAT("feat-tbm", 21, FEAT_8000_0001_ECX),
+ X86CPU_FEAT("feat-topoext", 22, FEAT_8000_0001_ECX),
+ X86CPU_FEAT("feat-perfctr-core", 23, FEAT_8000_0001_ECX),
+ X86CPU_FEAT("feat-perfctr-nb", 24, FEAT_8000_0001_ECX),
+ X86CPU_FEAT("feat-xstore", 2, FEAT_C000_0001_EDX),
+ X86CPU_FEAT("feat-xstore-en", 3, FEAT_C000_0001_EDX),
+ X86CPU_FEAT("feat-xcrypt", 6, FEAT_C000_0001_EDX),
+ X86CPU_FEAT("feat-xcrypt-en", 7, FEAT_C000_0001_EDX),
+ X86CPU_FEAT("feat-ace2", 8, FEAT_C000_0001_EDX),
+ X86CPU_FEAT("feat-ace2-en", 9, FEAT_C000_0001_EDX),
+ X86CPU_FEAT("feat-phe", 10, FEAT_C000_0001_EDX),
+ X86CPU_FEAT("feat-phe-en", 11, FEAT_C000_0001_EDX),
+ X86CPU_FEAT("feat-pmm", 12, FEAT_C000_0001_EDX),
+ X86CPU_FEAT("feat-pmm-en", 13, FEAT_C000_0001_EDX),
+ X86CPU_FEAT("feat-kvmclock", 0, FEAT_KVM),
+ X86CPU_FEAT("feat-kvm-nopiodelay", 1, FEAT_KVM),
+ X86CPU_FEAT("feat-kvm-mmu", 2, FEAT_KVM),
+ X86CPU_FEAT("feat-kvmclock2", 3, FEAT_KVM),
+ X86CPU_FEAT("feat-kvm-asyncpf", 4, FEAT_KVM),
+ X86CPU_FEAT("feat-kvm-steal-time", 5, FEAT_KVM),
+ X86CPU_FEAT("feat-kvm-pv-eoi", 6, FEAT_KVM),
+ X86CPU_FEAT("feat-kvm-pv-unhalt", 7, FEAT_KVM),
+ X86CPU_FEAT("feat-npt", 0, FEAT_SVM),
+ X86CPU_FEAT("feat-lbrv", 1, FEAT_SVM),
+ X86CPU_FEAT("feat-svm-lock", 2, FEAT_SVM),
+ X86CPU_FEAT("feat-nrip-save", 3, FEAT_SVM),
+ X86CPU_FEAT("feat-tsc-scale", 4, FEAT_SVM),
+ X86CPU_FEAT("feat-vmcb-clean", 5, FEAT_SVM),
+ X86CPU_FEAT("feat-flushbyasid", 6, FEAT_SVM),
+ X86CPU_FEAT("feat-decodeassists", 7, FEAT_SVM),
+ X86CPU_FEAT("feat-pause-filter", 10, FEAT_SVM),
+ X86CPU_FEAT("feat-pfthreshold", 12, FEAT_SVM),
+ X86CPU_FEAT("feat-fsgsbase", 0, FEAT_7_0_EBX),
+ X86CPU_FEAT("feat-bmi1", 3, FEAT_7_0_EBX),
+ X86CPU_FEAT("feat-hle", 4, FEAT_7_0_EBX),
+ X86CPU_FEAT("feat-avx2", 5, FEAT_7_0_EBX),
+ X86CPU_FEAT("feat-smep", 7, FEAT_7_0_EBX),
+ X86CPU_FEAT("feat-bmi2", 8, FEAT_7_0_EBX),
+ X86CPU_FEAT("feat-erms", 9, FEAT_7_0_EBX),
+ X86CPU_FEAT("feat-invpcid", 10, FEAT_7_0_EBX),
+ X86CPU_FEAT("feat-rtm", 11, FEAT_7_0_EBX),
+ X86CPU_FEAT("feat-rdseed", 18, FEAT_7_0_EBX),
+ X86CPU_FEAT("feat-adx", 19, FEAT_7_0_EBX),
+ X86CPU_FEAT("feat-smap", 20, FEAT_7_0_EBX),
DEFINE_PROP_END_OF_LIST()
};
--
1.8.3.1
^ permalink raw reply related [flat|nested] 53+ messages in thread* [Qemu-devel] [PATCH 12/16] qdev: introduce qdev_prop_find_bit()
2013-11-27 22:28 [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties Igor Mammedov
` (10 preceding siblings ...)
2013-11-27 22:28 ` [Qemu-devel] [PATCH 11/16] target-i386: set [+-]feature using static properties Igor Mammedov
@ 2013-11-27 22:28 ` Igor Mammedov
2013-11-27 22:28 ` [Qemu-devel] [PATCH 13/16] target-i386: use static properties in check_features_against_host() to print CPUID feature names Igor Mammedov
` (5 subsequent siblings)
17 siblings, 0 replies; 53+ messages in thread
From: Igor Mammedov @ 2013-11-27 22:28 UTC (permalink / raw)
To: qemu-devel; +Cc: afaerber
helper to find a static property corresponding to a specific bit
in a specified field.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
hw/core/qdev-properties.c | 15 +++++++++++++++
include/hw/qdev-properties.h | 13 +++++++++++++
2 files changed, 28 insertions(+)
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index dc8ae69..5b8b059 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -965,6 +965,21 @@ static Property *qdev_prop_find(DeviceState *dev, const char *name)
return NULL;
}
+const Property *qdev_prop_find_bit(const DeviceClass *dc, const int offset,
+ const uint8_t bitnr)
+{
+ const Property *prop;
+
+ QDEV_CLASS_FOREACH(dc, dc) {
+ QDEV_PROP_FOREACH(prop, dc) {
+ if (prop->offset == offset && prop->bitnr == bitnr) {
+ return prop;
+ }
+ }
+ }
+ return NULL;
+}
+
void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
Property *prop, const char *value)
{
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 692f82e..c559197 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -195,4 +195,17 @@ void qdev_property_add_static(DeviceState *dev, Property *prop, Error **errp);
*/
void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
Error **errp);
+
+#define QDEV_PROP_FOREACH(_var, _class) \
+ for ((_var) = DEVICE_CLASS((_class))->props; \
+ (_var) && (_var)->name; \
+ (_var)++)
+
+#define QDEV_CLASS_FOREACH(_var, _class) \
+ for ((_var) = (_class); \
+ (_var) != DEVICE_CLASS(object_class_by_name(TYPE_DEVICE)); \
+ (_var) = DEVICE_CLASS(object_class_get_parent(OBJECT_CLASS((_var)))))
+
+const Property *qdev_prop_find_bit(const DeviceClass *dc, const int offset,
+ const uint8_t bitnr);
#endif
--
1.8.3.1
^ permalink raw reply related [flat|nested] 53+ messages in thread* [Qemu-devel] [PATCH 13/16] target-i386: use static properties in check_features_against_host() to print CPUID feature names
2013-11-27 22:28 [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties Igor Mammedov
` (11 preceding siblings ...)
2013-11-27 22:28 ` [Qemu-devel] [PATCH 12/16] qdev: introduce qdev_prop_find_bit() Igor Mammedov
@ 2013-11-27 22:28 ` Igor Mammedov
2013-11-27 22:28 ` [Qemu-devel] [PATCH 14/16] target-i386: use static properties to list CPUID features Igor Mammedov
` (4 subsequent siblings)
17 siblings, 0 replies; 53+ messages in thread
From: Igor Mammedov @ 2013-11-27 22:28 UTC (permalink / raw)
To: qemu-devel; +Cc: afaerber
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
target-i386/cpu.c | 33 ++++++++++++++-------------------
1 file changed, 14 insertions(+), 19 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 1503e9a..5c3455f 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1119,24 +1119,6 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
#endif /* CONFIG_KVM */
}
-static int unavailable_host_feature(FeatureWordInfo *f, uint32_t mask)
-{
- int i;
-
- for (i = 0; i < 32; ++i)
- if (1 << i & mask) {
- const char *reg = get_register_name_32(f->cpuid_reg);
- assert(reg);
- fprintf(stderr, "warning: host doesn't support requested feature: "
- "CPUID.%02XH:%s%s%s [bit %d]\n",
- f->cpuid_eax, reg,
- f->feat_names[i] ? "." : "",
- f->feat_names[i] ? f->feat_names[i] : "", i);
- break;
- }
- return 0;
-}
-
/* Check if all requested cpu flags are making their way to the guest
*
* Returns 0 if all flags are supported by the host, non-zero otherwise.
@@ -1175,6 +1157,7 @@ static int kvm_check_features_against_host(X86CPU *cpu)
&host_def.features[FEAT_KVM],
FEAT_KVM },
};
+ const DeviceClass *dc = DEVICE_CLASS(object_get_class(OBJECT(cpu)));
assert(kvm_enabled());
@@ -1182,10 +1165,22 @@ static int kvm_check_features_against_host(X86CPU *cpu)
for (rv = 0, i = 0; i < ARRAY_SIZE(ft); ++i) {
FeatureWord w = ft[i].feat_word;
FeatureWordInfo *wi = &feature_word_info[w];
+ int offset = (char *)&((X86CPU *)0)->env.features[w] - (char *)0;
for (mask = 1; mask; mask <<= 1) {
if (*ft[i].guest_feat & mask &&
!(*ft[i].host_feat & mask)) {
- unavailable_host_feature(wi, mask);
+ int bitnr = ffsl(mask) - 1;
+ const Property *prop = qdev_prop_find_bit(dc, offset, bitnr);
+ const char *name = prop ? prop->name : NULL;
+ const char *reg = get_register_name_32(wi->cpuid_reg);
+
+ assert(reg);
+ fprintf(stderr, "warning: host doesn't support requested"
+ "feature: CPUID.%02XH:%s%s%s [bit %d]\n",
+ wi->cpuid_eax,
+ reg, name ? "." : "",
+ name ? name : "",
+ bitnr);
rv = 1;
}
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 53+ messages in thread* [Qemu-devel] [PATCH 14/16] target-i386: use static properties to list CPUID features
2013-11-27 22:28 [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties Igor Mammedov
` (12 preceding siblings ...)
2013-11-27 22:28 ` [Qemu-devel] [PATCH 13/16] target-i386: use static properties in check_features_against_host() to print CPUID feature names Igor Mammedov
@ 2013-11-27 22:28 ` Igor Mammedov
2013-11-27 22:28 ` [Qemu-devel] [PATCH 15/16] target-i386: remove unused *_feature_name arrays Igor Mammedov
` (3 subsequent siblings)
17 siblings, 0 replies; 53+ messages in thread
From: Igor Mammedov @ 2013-11-27 22:28 UTC (permalink / raw)
To: qemu-devel; +Cc: afaerber
- it breaks compatibility with previous output format by printing all features
in one string with "feat-" prefixes and all "_" replaced by "-"
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
target-i386/cpu.c | 44 ++++++++++----------------------------------
1 file changed, 10 insertions(+), 34 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 5c3455f..a9297cc 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1742,42 +1742,13 @@ out:
return;
}
-/* generate a composite string into buf of all cpuid names in featureset
- * selected by fbits. indicate truncation at bufsize in the event of overflow.
- * if flags, suppress names undefined in featureset.
- */
-static void listflags(char *buf, int bufsize, uint32_t fbits,
- const char **featureset, uint32_t flags)
-{
- const char **p = &featureset[31];
- char *q, *b, bit;
- int nc;
-
- b = 4 <= bufsize ? buf + (bufsize -= 3) - 1 : NULL;
- *buf = '\0';
- for (q = buf, bit = 31; fbits && bufsize; --p, fbits &= ~(1 << bit), --bit)
- if (fbits & 1 << bit && (*p || !flags)) {
- if (*p)
- nc = snprintf(q, bufsize, "%s%s", q == buf ? "" : " ", *p);
- else
- nc = snprintf(q, bufsize, "%s[%d]", q == buf ? "" : " ", bit);
- if (bufsize <= nc) {
- if (b) {
- memcpy(b, "...", sizeof("..."));
- }
- return;
- }
- q += nc;
- bufsize -= nc;
- }
-}
-
/* generate CPU information. */
void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
{
x86_def_t *def;
char buf[256];
int i;
+ const Property *prop;
for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
def = &builtin_x86_defs[i];
@@ -1791,12 +1762,17 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
#endif
(*cpu_fprintf)(f, "\nRecognized CPUID flags:\n");
- for (i = 0; i < ARRAY_SIZE(feature_word_info); i++) {
- FeatureWordInfo *fw = &feature_word_info[i];
- listflags(buf, sizeof(buf), (uint32_t)~0, fw->feat_names, 1);
- (*cpu_fprintf)(f, " %s\n", buf);
+ (*cpu_fprintf)(f, " ");
+ QDEV_PROP_FOREACH(prop, object_class_by_name(TYPE_X86_CPU)) {
+ const char *name = prop ? prop->name : "";
+
+ if (!g_str_has_prefix(name, "feat-")) {
+ continue;
+ }
+ (*cpu_fprintf)(f, " %s", name);
}
+ (*cpu_fprintf)(f, "\n");
}
CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 53+ messages in thread* [Qemu-devel] [PATCH 15/16] target-i386: remove unused *_feature_name arrays
2013-11-27 22:28 [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties Igor Mammedov
` (13 preceding siblings ...)
2013-11-27 22:28 ` [Qemu-devel] [PATCH 14/16] target-i386: use static properties to list CPUID features Igor Mammedov
@ 2013-11-27 22:28 ` Igor Mammedov
2013-11-27 22:28 ` [Qemu-devel] [PATCH 16/16] target-i386: cpu: fix invalid use of error_is_set(errp) if errp == NULL Igor Mammedov
` (2 subsequent siblings)
17 siblings, 0 replies; 53+ messages in thread
From: Igor Mammedov @ 2013-11-27 22:28 UTC (permalink / raw)
To: qemu-devel; +Cc: afaerber
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
fix conflict when removing kvm_feature_name[] with "kvm_pv_unhalt"
added by f010bc6 "target-i386: add feature kvm_pv_unhalt", earlier
patch "target-i386: set [+-]feature using static properties" were
ammended to include "feat-kvm-pv-unhalt" as static property
---
target-i386/cpu.c | 99 -------------------------------------------------------
1 file changed, 99 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index a9297cc..2220eae 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -172,98 +172,7 @@ static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
dst[CPUID_VENDOR_SZ] = '\0';
}
-/* feature flags taken from "Intel Processor Identification and the CPUID
- * Instruction" and AMD's "CPUID Specification". In cases of disagreement
- * between feature naming conventions, aliases may be added.
- */
-static const char *feature_name[] = {
- "fpu", "vme", "de", "pse",
- "tsc", "msr", "pae", "mce",
- "cx8", "apic", NULL, "sep",
- "mtrr", "pge", "mca", "cmov",
- "pat", "pse36", "pn" /* Intel psn */, "clflush" /* Intel clfsh */,
- NULL, "ds" /* Intel dts */, "acpi", "mmx",
- "fxsr", "sse", "sse2", "ss",
- "ht" /* Intel htt */, "tm", "ia64", "pbe",
-};
-static const char *ext_feature_name[] = {
- "pni|sse3" /* Intel,AMD sse3 */, "pclmulqdq|pclmuldq", "dtes64", "monitor",
- "ds_cpl", "vmx", "smx", "est",
- "tm2", "ssse3", "cid", NULL,
- "fma", "cx16", "xtpr", "pdcm",
- NULL, "pcid", "dca", "sse4.1|sse4_1",
- "sse4.2|sse4_2", "x2apic", "movbe", "popcnt",
- "tsc-deadline", "aes", "xsave", "osxsave",
- "avx", "f16c", "rdrand", "hypervisor",
-};
-/* Feature names that are already defined on feature_name[] but are set on
- * CPUID[8000_0001].EDX on AMD CPUs don't have their names on
- * ext2_feature_name[]. They are copied automatically to cpuid_ext2_features
- * if and only if CPU vendor is AMD.
- */
-static const char *ext2_feature_name[] = {
- NULL /* fpu */, NULL /* vme */, NULL /* de */, NULL /* pse */,
- NULL /* tsc */, NULL /* msr */, NULL /* pae */, NULL /* mce */,
- NULL /* cx8 */ /* AMD CMPXCHG8B */, NULL /* apic */, NULL, "syscall",
- NULL /* mtrr */, NULL /* pge */, NULL /* mca */, NULL /* cmov */,
- NULL /* pat */, NULL /* pse36 */, NULL, NULL /* Linux mp */,
- "nx|xd", NULL, "mmxext", NULL /* mmx */,
- NULL /* fxsr */, "fxsr_opt|ffxsr", "pdpe1gb" /* AMD Page1GB */, "rdtscp",
- NULL, "lm|i64", "3dnowext", "3dnow",
-};
-static const char *ext3_feature_name[] = {
- "lahf_lm" /* AMD LahfSahf */, "cmp_legacy", "svm", "extapic" /* AMD ExtApicSpace */,
- "cr8legacy" /* AMD AltMovCr8 */, "abm", "sse4a", "misalignsse",
- "3dnowprefetch", "osvw", "ibs", "xop",
- "skinit", "wdt", NULL, "lwp",
- "fma4", "tce", NULL, "nodeid_msr",
- NULL, "tbm", "topoext", "perfctr_core",
- "perfctr_nb", NULL, NULL, NULL,
- NULL, NULL, NULL, NULL,
-};
-
-static const char *ext4_feature_name[] = {
- NULL, NULL, "xstore", "xstore-en",
- NULL, NULL, "xcrypt", "xcrypt-en",
- "ace2", "ace2-en", "phe", "phe-en",
- "pmm", "pmm-en", NULL, NULL,
- NULL, NULL, NULL, NULL,
- NULL, NULL, NULL, NULL,
- NULL, NULL, NULL, NULL,
- NULL, NULL, NULL, NULL,
-};
-
-static const char *kvm_feature_name[] = {
- "kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvmclock",
- "kvm_asyncpf", "kvm_steal_time", "kvm_pv_eoi", "kvm_pv_unhalt",
- NULL, NULL, NULL, NULL,
- NULL, NULL, NULL, NULL,
- NULL, NULL, NULL, NULL,
- NULL, NULL, NULL, NULL,
- NULL, NULL, NULL, NULL,
- NULL, NULL, NULL, NULL,
-};
-
-static const char *svm_feature_name[] = {
- "npt", "lbrv", "svm_lock", "nrip_save",
- "tsc_scale", "vmcb_clean", "flushbyasid", "decodeassists",
- NULL, NULL, "pause_filter", NULL,
- "pfthreshold", NULL, NULL, NULL,
- NULL, NULL, NULL, NULL,
- NULL, NULL, NULL, NULL,
- NULL, NULL, NULL, NULL,
- NULL, NULL, NULL, NULL,
-};
-
-static const char *cpuid_7_0_ebx_feature_name[] = {
- "fsgsbase", NULL, NULL, "bmi1", "hle", "avx2", NULL, "smep",
- "bmi2", "erms", "invpcid", "rtm", NULL, NULL, NULL, NULL,
- NULL, NULL, "rdseed", "adx", "smap", NULL, NULL, NULL,
- NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
-};
-
typedef struct FeatureWordInfo {
- const char **feat_names;
uint32_t cpuid_eax; /* Input EAX for CPUID */
bool cpuid_needs_ecx; /* CPUID instruction uses ECX as input */
uint32_t cpuid_ecx; /* Input ECX value for CPUID */
@@ -272,35 +181,27 @@ typedef struct FeatureWordInfo {
static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
[FEAT_1_EDX] = {
- .feat_names = feature_name,
.cpuid_eax = 1, .cpuid_reg = R_EDX,
},
[FEAT_1_ECX] = {
- .feat_names = ext_feature_name,
.cpuid_eax = 1, .cpuid_reg = R_ECX,
},
[FEAT_8000_0001_EDX] = {
- .feat_names = ext2_feature_name,
.cpuid_eax = 0x80000001, .cpuid_reg = R_EDX,
},
[FEAT_8000_0001_ECX] = {
- .feat_names = ext3_feature_name,
.cpuid_eax = 0x80000001, .cpuid_reg = R_ECX,
},
[FEAT_C000_0001_EDX] = {
- .feat_names = ext4_feature_name,
.cpuid_eax = 0xC0000001, .cpuid_reg = R_EDX,
},
[FEAT_KVM] = {
- .feat_names = kvm_feature_name,
.cpuid_eax = KVM_CPUID_FEATURES, .cpuid_reg = R_EAX,
},
[FEAT_SVM] = {
- .feat_names = svm_feature_name,
.cpuid_eax = 0x8000000A, .cpuid_reg = R_EDX,
},
[FEAT_7_0_EBX] = {
- .feat_names = cpuid_7_0_ebx_feature_name,
.cpuid_eax = 7,
.cpuid_needs_ecx = true, .cpuid_ecx = 0,
.cpuid_reg = R_EBX,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 53+ messages in thread* [Qemu-devel] [PATCH 16/16] target-i386: cpu: fix invalid use of error_is_set(errp) if errp == NULL
2013-11-27 22:28 [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties Igor Mammedov
` (14 preceding siblings ...)
2013-11-27 22:28 ` [Qemu-devel] [PATCH 15/16] target-i386: remove unused *_feature_name arrays Igor Mammedov
@ 2013-11-27 22:28 ` Igor Mammedov
2013-12-15 22:50 ` [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties Andreas Färber
2014-02-11 17:17 ` [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU " Igor Mammedov
17 siblings, 0 replies; 53+ messages in thread
From: Igor Mammedov @ 2013-11-27 22:28 UTC (permalink / raw)
To: qemu-devel; +Cc: afaerber
in generic case errp may be NULL and if an Error gets raised in visitor
but not set to *errp for the lack of pointer, value might be uninitialized:
object_property_parse(obj, "invalid value", "foo", NULL);
and accessed futher in property setter leading to incorrect property
value of object instance.
So we cannot rely on error_is_set(errp) but must use a local variable
to detect error condition and return earlier.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
target-i386/cpu.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 2220eae..7064818 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1110,10 +1110,12 @@ static void x86_cpuid_version_set_family(Object *obj, Visitor *v, void *opaque,
CPUX86State *env = &cpu->env;
const int64_t min = 0;
const int64_t max = 0xff + 0xf;
+ Error *err = NULL;
int64_t value;
- visit_type_int(v, &value, name, errp);
- if (error_is_set(errp)) {
+ visit_type_int(v, &value, name, &err);
+ if (err) {
+ error_propagate(errp, err);
return;
}
if (value < min || value > max) {
@@ -1155,10 +1157,12 @@ static void x86_cpuid_version_set_model(Object *obj, Visitor *v, void *opaque,
CPUX86State *env = &cpu->env;
const int64_t min = 0;
const int64_t max = 0xff;
+ Error *err = NULL;
int64_t value;
- visit_type_int(v, &value, name, errp);
- if (error_is_set(errp)) {
+ visit_type_int(v, &value, name, &err);
+ if (err) {
+ error_propagate(errp, err);
return;
}
if (value < min || value > max) {
@@ -1197,10 +1201,12 @@ static void x86_cpuid_version_set_stepping(Object *obj, Visitor *v,
CPUX86State *env = &cpu->env;
const int64_t min = 0;
const int64_t max = 0xf;
+ Error *err = NULL;
int64_t value;
- visit_type_int(v, &value, name, errp);
- if (error_is_set(errp)) {
+ visit_type_int(v, &value, name, &err);
+ if (err) {
+ error_propagate(errp, err);
return;
}
if (value < min || value > max) {
@@ -1337,10 +1343,12 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
X86CPU *cpu = X86_CPU(obj);
const int64_t min = 0;
const int64_t max = INT64_MAX;
+ Error *err = NULL;
int64_t value;
- visit_type_int(v, &value, name, errp);
- if (error_is_set(errp)) {
+ visit_type_int(v, &value, name, &err);
+ if (err) {
+ error_propagate(errp, err);
return;
}
if (value < min || value > max) {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties
2013-11-27 22:28 [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties Igor Mammedov
` (15 preceding siblings ...)
2013-11-27 22:28 ` [Qemu-devel] [PATCH 16/16] target-i386: cpu: fix invalid use of error_is_set(errp) if errp == NULL Igor Mammedov
@ 2013-12-15 22:50 ` Andreas Färber
2013-12-16 15:01 ` Igor Mammedov
2014-02-05 14:40 ` Igor Mammedov
2014-02-11 17:17 ` [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU " Igor Mammedov
17 siblings, 2 replies; 53+ messages in thread
From: Andreas Färber @ 2013-12-15 22:50 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, Anthony Liguori, Eduardo Habkost
Am 27.11.2013 23:28, schrieb Igor Mammedov:
> Igor Mammedov (16):
> target-i386: cleanup 'foo' feature handling'
> target-i386: cleanup 'foo=val' feature handling
Thanks, I've queued these on qom-cpu-next:
https://github.com/afaerber/qemu-cpu/commits/qom-cpu-next
> target-i386: cpu: convert 'level' to static property
> target-i386: cpu: convert 'xlevel' to static property
> target-i386: cpu: convert 'family' to static property
> target-i386: cpu: convert 'model' to static property
> target-i386: cpu: convert 'stepping' to static property
> target-i386: cpu: convert 'vendor' to static property
> target-i386: cpu: convert 'model-id' to static property
> target-i386: cpu: convert 'tsc-frequency' to static property
But I still don't see the utility of this conversion after all the
discussions we've had... :( The below patches seem to only operate on
CPUID bits, which get added as properties in the following patch.
> target-i386: set [+-]feature using static properties
> qdev: introduce qdev_prop_find_bit()
> target-i386: use static properties in check_features_against_host() to
> print CPUID feature names
> target-i386: use static properties to list CPUID features
I am reading too many occurrences of "static properties" above that
should IMO just be "properties". You got permission to use a name-based
scheme to iterate over feat-* properties, so why are you still iterating
over static properties with a helper searching for offsets rather than
QOM properties with feat- prefix? Either we need that scheme for
automated processing as I understood you, then we should be consequent
in using it, or we don't. And I would prefer to keep these mappings in
x86 code rather than messing in generic device infrastructure and
iterating over *all* properties in your qdev_prop_find_bit() and making
generally available new QDEV_* macros QDEV_PROP_FOREACH() and
QDEV_CLASS_FOREACH().
The utility of the feat- prefix AIUI is to go from +foo to feat-foo=on;
going from bit position to name should work just as before and could
even be consolidated into a single array by using dynamic properties.
Am I the only one that finds the approach backwards? o.O
Regards,
Andreas
> target-i386: remove unused *_feature_name arrays
> target-i386: cpu: fix invalid use of error_is_set(errp) if errp ==
> NULL
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties
2013-12-15 22:50 ` [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties Andreas Färber
@ 2013-12-16 15:01 ` Igor Mammedov
2013-12-16 18:26 ` Eduardo Habkost
2014-02-05 14:40 ` Igor Mammedov
1 sibling, 1 reply; 53+ messages in thread
From: Igor Mammedov @ 2013-12-16 15:01 UTC (permalink / raw)
To: Andreas Färber; +Cc: qemu-devel, Anthony Liguori, Eduardo Habkost
On Sun, 15 Dec 2013 23:50:47 +0100
Andreas Färber <afaerber@suse.de> wrote:
> Am 27.11.2013 23:28, schrieb Igor Mammedov:
> > Igor Mammedov (16):
> > target-i386: cleanup 'foo' feature handling'
> > target-i386: cleanup 'foo=val' feature handling
>
> Thanks, I've queued these on qom-cpu-next:
> https://github.com/afaerber/qemu-cpu/commits/qom-cpu-next
>
> > target-i386: cpu: convert 'level' to static property
> > target-i386: cpu: convert 'xlevel' to static property
> > target-i386: cpu: convert 'family' to static property
> > target-i386: cpu: convert 'model' to static property
> > target-i386: cpu: convert 'stepping' to static property
> > target-i386: cpu: convert 'vendor' to static property
> > target-i386: cpu: convert 'model-id' to static property
> > target-i386: cpu: convert 'tsc-frequency' to static property
>
> But I still don't see the utility of this conversion after all the
> discussions we've had... :( The below patches seem to only operate on
> CPUID bits, which get added as properties in the following patch.
Above patches are there to simplify/unify current codebase. For example,
level & xlevel replace custom setters/getters with static property onliners.
The rest are making initfn more readable, not to mention that they
become visible in HMP along with the rest features wich is nice for
consistent behavior even is we do not care about HMP.
Otherwise there is not much difference between dynamic vs static anymore,
so this patches could be dropped, however with them ,I think,
code is a bit cleaner.
>
> > target-i386: set [+-]feature using static properties
> > qdev: introduce qdev_prop_find_bit()
> > target-i386: use static properties in check_features_against_host() to
> > print CPUID feature names
> > target-i386: use static properties to list CPUID features
>
> I am reading too many occurrences of "static properties" above that
> should IMO just be "properties". You got permission to use a name-based
in current code base static properties are almost the same as dynamic ones,
it's just a more abbreviated version of dynamic ones with static defaults,
range checking, bit handling ...
So I don't see why more verbose dynamic properties SHOULD be used,
where the same code could be written more compact with static properties.
> scheme to iterate over feat-* properties, so why are you still iterating
> over static properties with a helper searching for offsets rather than
> QOM properties with feat- prefix? Either we need that scheme for
> automated processing as I understood you, then we should be consequent
> in using it, or we don't. And I would prefer to keep these mappings in
> x86 code rather than messing in generic device infrastructure and
> iterating over *all* properties in your qdev_prop_find_bit() and making
> generally available new QDEV_* macros QDEV_PROP_FOREACH() and
> QDEV_CLASS_FOREACH().
this patch was, was on list for more than half a year without any
complaints/reviews. I don't have ICR for that time already, but if I recall
correctly something like this was suggested by Anthony to resolve problem
of mapping bit fields to names. If there is a more simple elegant way to do
it I'd like to hear more concrete suggestion(s) how it should be vs just
"messing" verdict with which I don't agree btw.
>
> The utility of the feat- prefix AIUI is to go from +foo to feat-foo=on;
> going from bit position to name should work just as before and could
> even be consolidated into a single array by using dynamic properties.
Could you elaborate more on what you are proposing please?
> Am I the only one that finds the approach backwards? o.O
>
> Regards,
> Andreas
>
> > target-i386: remove unused *_feature_name arrays
> > target-i386: cpu: fix invalid use of error_is_set(errp) if errp ==
> > NULL
>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties
2013-12-16 15:01 ` Igor Mammedov
@ 2013-12-16 18:26 ` Eduardo Habkost
2013-12-17 13:01 ` Igor Mammedov
2014-01-07 8:41 ` Igor Mammedov
0 siblings, 2 replies; 53+ messages in thread
From: Eduardo Habkost @ 2013-12-16 18:26 UTC (permalink / raw)
To: Igor Mammedov; +Cc: Andreas Färber, Anthony Liguori, qemu-devel
On Mon, Dec 16, 2013 at 04:01:05PM +0100, Igor Mammedov wrote:
> On Sun, 15 Dec 2013 23:50:47 +0100
> Andreas Färber <afaerber@suse.de> wrote:
>
> > Am 27.11.2013 23:28, schrieb Igor Mammedov:
> > > Igor Mammedov (16):
> > > target-i386: cleanup 'foo' feature handling'
> > > target-i386: cleanup 'foo=val' feature handling
> >
> > Thanks, I've queued these on qom-cpu-next:
> > https://github.com/afaerber/qemu-cpu/commits/qom-cpu-next
> >
> > > target-i386: cpu: convert 'level' to static property
> > > target-i386: cpu: convert 'xlevel' to static property
> > > target-i386: cpu: convert 'family' to static property
> > > target-i386: cpu: convert 'model' to static property
> > > target-i386: cpu: convert 'stepping' to static property
> > > target-i386: cpu: convert 'vendor' to static property
> > > target-i386: cpu: convert 'model-id' to static property
> > > target-i386: cpu: convert 'tsc-frequency' to static property
> >
> > But I still don't see the utility of this conversion after all the
> > discussions we've had... :( The below patches seem to only operate on
> > CPUID bits, which get added as properties in the following patch.
> Above patches are there to simplify/unify current codebase. For example,
> level & xlevel replace custom setters/getters with static property onliners.
> The rest are making initfn more readable, not to mention that they
> become visible in HMP along with the rest features wich is nice for
> consistent behavior even is we do not care about HMP.
> Otherwise there is not much difference between dynamic vs static anymore,
> so this patches could be dropped, however with them ,I think,
> code is a bit cleaner.
I agree with Igor, here. We don't strictly need to make those properties
"static" anymore, but it is still useful to do it, because it makes the
instrospection information visible to other code (inside and eventually
outside QEMU) before a CPU object gets created, and to me it really
looks simpler than registering the properties at instance_init().
>
> >
> > > target-i386: set [+-]feature using static properties
> > > qdev: introduce qdev_prop_find_bit()
> > > target-i386: use static properties in check_features_against_host() to
> > > print CPUID feature names
> > > target-i386: use static properties to list CPUID features
> >
> > I am reading too many occurrences of "static properties" above that
> > should IMO just be "properties". You got permission to use a name-based
> in current code base static properties are almost the same as dynamic ones,
> it's just a more abbreviated version of dynamic ones with static defaults,
> range checking, bit handling ...
> So I don't see why more verbose dynamic properties SHOULD be used,
> where the same code could be written more compact with static properties.
In the specific case of the feature-bit properties, I am more inclined
to agree with Andreas: is making the properties "static" worth the extra
code complexity?
We still don't have a QMP command to list all the properties supported
by a DeviceClass, do we? If we had it, having static properties would be
much more useful. Today they are not much better than simple "dynamic"
properties registered at instance_init().
>
> > scheme to iterate over feat-* properties, so why are you still iterating
> > over static properties with a helper searching for offsets rather than
> > QOM properties with feat- prefix? Either we need that scheme for
> > automated processing as I understood you, then we should be consequent
> > in using it, or we don't. And I would prefer to keep these mappings in
> > x86 code rather than messing in generic device infrastructure and
> > iterating over *all* properties in your qdev_prop_find_bit() and making
> > generally available new QDEV_* macros QDEV_PROP_FOREACH() and
> > QDEV_CLASS_FOREACH().
> this patch was, was on list for more than half a year without any
> complaints/reviews. I don't have ICR for that time already, but if I recall
> correctly something like this was suggested by Anthony to resolve problem
> of mapping bit fields to names. If there is a more simple elegant way to do
> it I'd like to hear more concrete suggestion(s) how it should be vs just
> "messing" verdict with which I don't agree btw.
The need to iterate over feature properties was something that I
considered "okay" only because we were forced to use static properties.
Now we moved global property handling to instance_post_init and we don't
really need it anymore.
One more simple way to do it is to simply keep the existing
feature_name[] arrays and use them to register/lookup feature property
names. I would prefer that to the convoluted way this new code do
feature bit->name lookup.
>
> >
> > The utility of the feat- prefix AIUI is to go from +foo to feat-foo=on;
> > going from bit position to name should work just as before and could
> > even be consolidated into a single array by using dynamic properties.
> Could you elaborate more on what you are proposing please?
Maybe Andreas is suggesting the same I suggest above: keep the
feature_names arrays on feature_word_info and do something this on
instance_init:
for (w = 0; w < FEATURE_WORDS; w++) {
FeatureWordInfo *wi = &feature_word_info[w];
char **feat_names = wi->feat_names;
if (!feat_names)
continue;
for (bit = 0; bit < 32; bit++) {
if (!feat_names[bit])
continue;
char *prop_name = g_strdup_printf("feat-%s", feat_names[bit]);
object_property_add_bit(OBJECT(cpu), prop_name, &cpu->features[w], bit);
}
}
This way feature name->bit lookup/set/unset code would use the QOM
property infrastructure, but feature bit->name lookup could still look
exactly the same.
>
> > Am I the only one that finds the approach backwards? o.O
You are not alone. As I said above, I found it acceptable because it was
the only way to make global properties work. But it is not really
necessary anymore.
> >
> > Regards,
> > Andreas
> >
> > > target-i386: remove unused *_feature_name arrays
> > > target-i386: cpu: fix invalid use of error_is_set(errp) if errp ==
> > > NULL
> >
>
--
Eduardo
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties
2013-12-16 18:26 ` Eduardo Habkost
@ 2013-12-17 13:01 ` Igor Mammedov
2014-01-07 8:41 ` Igor Mammedov
1 sibling, 0 replies; 53+ messages in thread
From: Igor Mammedov @ 2013-12-17 13:01 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: Andreas Färber, Anthony Liguori, qemu-devel
On Mon, 16 Dec 2013 16:26:55 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Mon, Dec 16, 2013 at 04:01:05PM +0100, Igor Mammedov wrote:
> > On Sun, 15 Dec 2013 23:50:47 +0100
> > Andreas Färber <afaerber@suse.de> wrote:
> >
> > > Am 27.11.2013 23:28, schrieb Igor Mammedov:
> > > > Igor Mammedov (16):
> > > > target-i386: cleanup 'foo' feature handling'
> > > > target-i386: cleanup 'foo=val' feature handling
> > >
> > > Thanks, I've queued these on qom-cpu-next:
> > > https://github.com/afaerber/qemu-cpu/commits/qom-cpu-next
> > >
> > > > target-i386: cpu: convert 'level' to static property
> > > > target-i386: cpu: convert 'xlevel' to static property
> > > > target-i386: cpu: convert 'family' to static property
> > > > target-i386: cpu: convert 'model' to static property
> > > > target-i386: cpu: convert 'stepping' to static property
> > > > target-i386: cpu: convert 'vendor' to static property
> > > > target-i386: cpu: convert 'model-id' to static property
> > > > target-i386: cpu: convert 'tsc-frequency' to static property
> > >
> > > But I still don't see the utility of this conversion after all the
> > > discussions we've had... :( The below patches seem to only operate on
> > > CPUID bits, which get added as properties in the following patch.
> > Above patches are there to simplify/unify current codebase. For example,
> > level & xlevel replace custom setters/getters with static property onliners.
> > The rest are making initfn more readable, not to mention that they
> > become visible in HMP along with the rest features wich is nice for
> > consistent behavior even is we do not care about HMP.
> > Otherwise there is not much difference between dynamic vs static anymore,
> > so this patches could be dropped, however with them ,I think,
> > code is a bit cleaner.
>
> I agree with Igor, here. We don't strictly need to make those properties
> "static" anymore, but it is still useful to do it, because it makes the
> instrospection information visible to other code (inside and eventually
> outside QEMU) before a CPU object gets created, and to me it really
> looks simpler than registering the properties at instance_init().
>
> >
> > >
> > > > target-i386: set [+-]feature using static properties
> > > > qdev: introduce qdev_prop_find_bit()
> > > > target-i386: use static properties in check_features_against_host() to
> > > > print CPUID feature names
> > > > target-i386: use static properties to list CPUID features
> > >
> > > I am reading too many occurrences of "static properties" above that
> > > should IMO just be "properties". You got permission to use a name-based
> > in current code base static properties are almost the same as dynamic ones,
> > it's just a more abbreviated version of dynamic ones with static defaults,
> > range checking, bit handling ...
> > So I don't see why more verbose dynamic properties SHOULD be used,
> > where the same code could be written more compact with static properties.
>
> In the specific case of the feature-bit properties, I am more inclined
> to agree with Andreas: is making the properties "static" worth the extra
> code complexity?
qdev_prop_find_bit() is the only complexity with static bit properties,
otherwise feature bits as static props are much more maintainable and
self descriptive then feature name arrays.
>
> We still don't have a QMP command to list all the properties supported
> by a DeviceClass, do we? If we had it, having static properties would be
> much more useful. Today they are not much better than simple "dynamic"
> properties registered at instance_init().
with static properties there won't be need to rewrite this twice, once
command becomes available.
>
> >
> > > scheme to iterate over feat-* properties, so why are you still iterating
> > > over static properties with a helper searching for offsets rather than
> > > QOM properties with feat- prefix? Either we need that scheme for
> > > automated processing as I understood you, then we should be consequent
> > > in using it, or we don't. And I would prefer to keep these mappings in
> > > x86 code rather than messing in generic device infrastructure and
> > > iterating over *all* properties in your qdev_prop_find_bit() and making
> > > generally available new QDEV_* macros QDEV_PROP_FOREACH() and
> > > QDEV_CLASS_FOREACH().
> > this patch was, was on list for more than half a year without any
> > complaints/reviews. I don't have ICR for that time already, but if I recall
> > correctly something like this was suggested by Anthony to resolve problem
> > of mapping bit fields to names. If there is a more simple elegant way to do
> > it I'd like to hear more concrete suggestion(s) how it should be vs just
> > "messing" verdict with which I don't agree btw.
>
> The need to iterate over feature properties was something that I
> considered "okay" only because we were forced to use static properties.
> Now we moved global property handling to instance_post_init and we don't
> really need it anymore.
>
> One more simple way to do it is to simply keep the existing
> feature_name[] arrays and use them to register/lookup feature property
> names. I would prefer that to the convoluted way this new code do
> feature bit->name lookup.
>
> >
> > >
> > > The utility of the feat- prefix AIUI is to go from +foo to feat-foo=on;
> > > going from bit position to name should work just as before and could
> > > even be consolidated into a single array by using dynamic properties.
> > Could you elaborate more on what you are proposing please?
>
> Maybe Andreas is suggesting the same I suggest above: keep the
> feature_names arrays on feature_word_info and do something this on
> instance_init:
>
> for (w = 0; w < FEATURE_WORDS; w++) {
> FeatureWordInfo *wi = &feature_word_info[w];
> char **feat_names = wi->feat_names;
> if (!feat_names)
> continue;
> for (bit = 0; bit < 32; bit++) {
> if (!feat_names[bit])
> continue;
> char *prop_name = g_strdup_printf("feat-%s", feat_names[bit]); is
> object_property_add_bit(OBJECT(cpu), prop_name, &cpu->features[w], bit);
> }
> }
>
> This way feature name->bit lookup/set/unset code would use the QOM
> property infrastructure, but feature bit->name lookup could still look
> exactly the same.
feature_name[] array is harder to maintain compared to proposed static props,
and bit->name lookup is not less convoluted than qdev_prop_find_bit(),
albeit the people are more used to it.
qdev_prop_find_bit() might be reused by other bit properties if bit->name
mapping would be needed while feature_name[] just hides problem.
>
>
> >
> > > Am I the only one that finds the approach backwards? o.O
>
> You are not alone. As I said above, I found it acceptable because it was
> the only way to make global properties work. But it is not really
> necessary anymore.
Anyway if majority are for using feature_name[] for bit->name lookup and
registering feature bits as dynamic properties, I'm not against it
considering we gave up on class introspection in favor of instance
introspection, there is not much difference currently between them,
except of maintainability of result.
>
> > >
> > > Regards,
> > > Andreas
> > >
> > > > target-i386: remove unused *_feature_name arrays
> > > > target-i386: cpu: fix invalid use of error_is_set(errp) if errp ==
> > > > NULL
> > >
> >
>
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties
2013-12-16 18:26 ` Eduardo Habkost
2013-12-17 13:01 ` Igor Mammedov
@ 2014-01-07 8:41 ` Igor Mammedov
1 sibling, 0 replies; 53+ messages in thread
From: Igor Mammedov @ 2014-01-07 8:41 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: Andreas Färber, Anthony Liguori, qemu-devel
On Mon, 16 Dec 2013 16:26:55 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Mon, Dec 16, 2013 at 04:01:05PM +0100, Igor Mammedov wrote:
> > On Sun, 15 Dec 2013 23:50:47 +0100
> > Andreas Färber <afaerber@suse.de> wrote:
> >
> > > Am 27.11.2013 23:28, schrieb Igor Mammedov:
> > > > Igor Mammedov (16):
> > > > target-i386: cleanup 'foo' feature handling'
> > > > target-i386: cleanup 'foo=val' feature handling
> > >
> > > Thanks, I've queued these on qom-cpu-next:
> > > https://github.com/afaerber/qemu-cpu/commits/qom-cpu-next
> > >
> > > > target-i386: cpu: convert 'level' to static property
> > > > target-i386: cpu: convert 'xlevel' to static property
> > > > target-i386: cpu: convert 'family' to static property
> > > > target-i386: cpu: convert 'model' to static property
> > > > target-i386: cpu: convert 'stepping' to static property
> > > > target-i386: cpu: convert 'vendor' to static property
> > > > target-i386: cpu: convert 'model-id' to static property
> > > > target-i386: cpu: convert 'tsc-frequency' to static property
> > >
> > > But I still don't see the utility of this conversion after all the
> > > discussions we've had... :( The below patches seem to only operate on
> > > CPUID bits, which get added as properties in the following patch.
> > Above patches are there to simplify/unify current codebase. For example,
> > level & xlevel replace custom setters/getters with static property onliners.
> > The rest are making initfn more readable, not to mention that they
> > become visible in HMP along with the rest features wich is nice for
> > consistent behavior even is we do not care about HMP.
> > Otherwise there is not much difference between dynamic vs static anymore,
> > so this patches could be dropped, however with them ,I think,
> > code is a bit cleaner.
>
> I agree with Igor, here. We don't strictly need to make those properties
> "static" anymore, but it is still useful to do it, because it makes the
> instrospection information visible to other code (inside and eventually
> outside QEMU) before a CPU object gets created, and to me it really
> looks simpler than registering the properties at instance_init().
>
> >
> > >
> > > > target-i386: set [+-]feature using static properties
> > > > qdev: introduce qdev_prop_find_bit()
> > > > target-i386: use static properties in check_features_against_host() to
> > > > print CPUID feature names
> > > > target-i386: use static properties to list CPUID features
> > >
> > > I am reading too many occurrences of "static properties" above that
> > > should IMO just be "properties". You got permission to use a name-based
> > in current code base static properties are almost the same as dynamic ones,
> > it's just a more abbreviated version of dynamic ones with static defaults,
> > range checking, bit handling ...
> > So I don't see why more verbose dynamic properties SHOULD be used,
> > where the same code could be written more compact with static properties.
>
> In the specific case of the feature-bit properties, I am more inclined
> to agree with Andreas: is making the properties "static" worth the extra
> code complexity?
On contrary, using static properties for feature-bits reduces overall code
complexity and provides much more better self documented properties compared
to current bit arrays and their handling.
So far we do not really need mapping bit2name for x86 CPU, there are 2
places where it's used now:
1. comparing current CPU model with host CPUID bits.
we could create temporary CPUhost model for it and compare properties
instead of bits, something like:
'compare(current_cpu, host_cpu, "feat-*")'
2. listing all feat-* properties for '-cpu ?'.
that task could be performed by something like:
object_property_foreach(object_new(x86_cpu), print_prop_if_name("feat-*"))
I guess it is what Andreas suggests instead of iterating over DEVICE->props
array.
[...]
>
> --
> Eduardo
--
Regards,
Igor
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties
2013-12-15 22:50 ` [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties Andreas Färber
2013-12-16 15:01 ` Igor Mammedov
@ 2014-02-05 14:40 ` Igor Mammedov
2014-02-05 16:14 ` Andreas Färber
1 sibling, 1 reply; 53+ messages in thread
From: Igor Mammedov @ 2014-02-05 14:40 UTC (permalink / raw)
To: Andreas Färber; +Cc: qemu-devel, Anthony Liguori, Eduardo Habkost
On Sun, 15 Dec 2013 23:50:47 +0100
Andreas Färber <afaerber@suse.de> wrote:
> Am 27.11.2013 23:28, schrieb Igor Mammedov:
> > Igor Mammedov (16):
> > target-i386: cleanup 'foo' feature handling'
> > target-i386: cleanup 'foo=val' feature handling
>
> Thanks, I've queued these on qom-cpu-next:
> https://github.com/afaerber/qemu-cpu/commits/qom-cpu-next
>
> > target-i386: cpu: convert 'level' to static property
> > target-i386: cpu: convert 'xlevel' to static property
> > target-i386: cpu: convert 'family' to static property
> > target-i386: cpu: convert 'model' to static property
> > target-i386: cpu: convert 'stepping' to static property
> > target-i386: cpu: convert 'vendor' to static property
> > target-i386: cpu: convert 'model-id' to static property
> > target-i386: cpu: convert 'tsc-frequency' to static property
>
> But I still don't see the utility of this conversion after all the
> discussions we've had... :(
It seems there is movement to make DEVICE self describing for purpose
of QAPI schema introspection, where static properties would be used
(dynamic ones are not suitable for this purpose)
> The below patches seem to only operate on
> CPUID bits, which get added as properties in the following patch.
>
> > target-i386: set [+-]feature using static properties
> > qdev: introduce qdev_prop_find_bit()
> > target-i386: use static properties in check_features_against_host() to
> > print CPUID feature names
> > target-i386: use static properties to list CPUID features
>
> I am reading too many occurrences of "static properties" above that
> should IMO just be "properties". You got permission to use a name-based
> scheme to iterate over feat-* properties, so why are you still iterating
> over static properties with a helper searching for offsets rather than
> QOM properties with feat- prefix? Either we need that scheme for
Ok, I'll use feat- prefix, there is not real need for iterating over array
when listing properties.
> automated processing as I understood you, then we should be consequent
> in using it, or we don't. And I would prefer to keep these mappings in
> x86 code rather than messing in generic device infrastructure and
> iterating over *all* properties in your qdev_prop_find_bit() and making
> generally available new QDEV_* macros QDEV_PROP_FOREACH() and
> QDEV_CLASS_FOREACH().
Unfortunatly we still need mapping from bit position to name for
kvm_check_features_against_host()
So there is 2 options:
1st: keep iterating over array local to x86
2nd: drop name reporting in kvm_check_features_against_host() and report
bit positions.
which one would you preffer?
>
> The utility of the feat- prefix AIUI is to go from +foo to feat-foo=on;
> going from bit position to name should work just as before and could
> even be consolidated into a single array by using dynamic properties.
I can't get you, Could you elaborate more on "consolidated into a single array
by using dynamic properties."
> Am I the only one that finds the approach backwards? o.O
>
> Regards,
> Andreas
>
> > target-i386: remove unused *_feature_name arrays
> > target-i386: cpu: fix invalid use of error_is_set(errp) if errp ==
> > NULL
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
--
Regards,
Igor
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties
2014-02-05 14:40 ` Igor Mammedov
@ 2014-02-05 16:14 ` Andreas Färber
2014-02-05 16:52 ` Igor Mammedov
0 siblings, 1 reply; 53+ messages in thread
From: Andreas Färber @ 2014-02-05 16:14 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, Anthony Liguori, Eduardo Habkost
Am 05.02.2014 15:40, schrieb Igor Mammedov:
> On Sun, 15 Dec 2013 23:50:47 +0100
> Andreas Färber <afaerber@suse.de> wrote:
>
>> Am 27.11.2013 23:28, schrieb Igor Mammedov:
>>> Igor Mammedov (16):
>>> target-i386: cleanup 'foo' feature handling'
>>> target-i386: cleanup 'foo=val' feature handling
>>
>> Thanks, I've queued these on qom-cpu-next:
>> https://github.com/afaerber/qemu-cpu/commits/qom-cpu-next
>>
>>> target-i386: cpu: convert 'level' to static property
>>> target-i386: cpu: convert 'xlevel' to static property
>>> target-i386: cpu: convert 'family' to static property
>>> target-i386: cpu: convert 'model' to static property
>>> target-i386: cpu: convert 'stepping' to static property
>>> target-i386: cpu: convert 'vendor' to static property
>>> target-i386: cpu: convert 'model-id' to static property
>>> target-i386: cpu: convert 'tsc-frequency' to static property
>>
>> But I still don't see the utility of this conversion after all the
>> discussions we've had... :(
> It seems there is movement to make DEVICE self describing for purpose
> of QAPI schema introspection, where static properties would be used
> (dynamic ones are not suitable for this purpose)
Do you have a pointer to such a discussion? Sounds like I was not
involved and Anthony probably not either...
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties
2014-02-05 16:14 ` Andreas Färber
@ 2014-02-05 16:52 ` Igor Mammedov
2014-02-06 15:19 ` Igor Mammedov
0 siblings, 1 reply; 53+ messages in thread
From: Igor Mammedov @ 2014-02-05 16:52 UTC (permalink / raw)
To: Andreas Färber
Cc: Eduardo Habkost, armbru, qemu-devel, Anthony Liguori, pbonzini,
akong
On Wed, 05 Feb 2014 17:14:27 +0100
Andreas Färber <afaerber@suse.de> wrote:
> Am 05.02.2014 15:40, schrieb Igor Mammedov:
> > On Sun, 15 Dec 2013 23:50:47 +0100
> > Andreas Färber <afaerber@suse.de> wrote:
> >
> >> Am 27.11.2013 23:28, schrieb Igor Mammedov:
> >>> Igor Mammedov (16):
> >>> target-i386: cleanup 'foo' feature handling'
> >>> target-i386: cleanup 'foo=val' feature handling
> >>
> >> Thanks, I've queued these on qom-cpu-next:
> >> https://github.com/afaerber/qemu-cpu/commits/qom-cpu-next
> >>
> >>> target-i386: cpu: convert 'level' to static property
> >>> target-i386: cpu: convert 'xlevel' to static property
> >>> target-i386: cpu: convert 'family' to static property
> >>> target-i386: cpu: convert 'model' to static property
> >>> target-i386: cpu: convert 'stepping' to static property
> >>> target-i386: cpu: convert 'vendor' to static property
> >>> target-i386: cpu: convert 'model-id' to static property
> >>> target-i386: cpu: convert 'tsc-frequency' to static property
> >>
> >> But I still don't see the utility of this conversion after all the
> >> discussions we've had... :(
> > It seems there is movement to make DEVICE self describing for purpose
> > of QAPI schema introspection, where static properties would be used
> > (dynamic ones are not suitable for this purpose)
>
> Do you have a pointer to such a discussion? Sounds like I was not
> involved and Anthony probably not either...
Not at the moment, CCing people who might know more about the topic.
But just thinking about creating QAPI schema for devices, it's not really
possible to generate one using dynamic properties (unless one resorts to
creating every supported device instance), while arrays of static properties
are there for every device and simplistically speaking just need conversion
to schema format.
>
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
--
Regards,
Igor
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties
2014-02-05 16:52 ` Igor Mammedov
@ 2014-02-06 15:19 ` Igor Mammedov
2014-02-06 15:51 ` Andreas Färber
0 siblings, 1 reply; 53+ messages in thread
From: Igor Mammedov @ 2014-02-06 15:19 UTC (permalink / raw)
To: Igor Mammedov
Cc: Eduardo Habkost, qemu-devel, armbru, Anthony Liguori, pbonzini,
akong, Andreas Färber
On Wed, 5 Feb 2014 17:52:16 +0100
Igor Mammedov <imammedo@redhat.com> wrote:
> On Wed, 05 Feb 2014 17:14:27 +0100
> Andreas Färber <afaerber@suse.de> wrote:
>
> > Am 05.02.2014 15:40, schrieb Igor Mammedov:
> > > On Sun, 15 Dec 2013 23:50:47 +0100
> > > Andreas Färber <afaerber@suse.de> wrote:
> > >
> > >> Am 27.11.2013 23:28, schrieb Igor Mammedov:
> > >>> Igor Mammedov (16):
> > >>> target-i386: cleanup 'foo' feature handling'
> > >>> target-i386: cleanup 'foo=val' feature handling
> > >>
> > >> Thanks, I've queued these on qom-cpu-next:
> > >> https://github.com/afaerber/qemu-cpu/commits/qom-cpu-next
> > >>
> > >>> target-i386: cpu: convert 'level' to static property
> > >>> target-i386: cpu: convert 'xlevel' to static property
> > >>> target-i386: cpu: convert 'family' to static property
> > >>> target-i386: cpu: convert 'model' to static property
> > >>> target-i386: cpu: convert 'stepping' to static property
> > >>> target-i386: cpu: convert 'vendor' to static property
> > >>> target-i386: cpu: convert 'model-id' to static property
> > >>> target-i386: cpu: convert 'tsc-frequency' to static property
> > >>
> > >> But I still don't see the utility of this conversion after all the
> > >> discussions we've had... :(
> > > It seems there is movement to make DEVICE self describing for purpose
> > > of QAPI schema introspection, where static properties would be used
> > > (dynamic ones are not suitable for this purpose)
> >
> > Do you have a pointer to such a discussion? Sounds like I was not
> > involved and Anthony probably not either...
> Not at the moment, CCing people who might know more about the topic.
>
> But just thinking about creating QAPI schema for devices, it's not really
> possible to generate one using dynamic properties (unless one resorts to
> creating every supported device instance), while arrays of static properties
> are there for every device and simplistically speaking just need conversion
> to schema format.
There is one more reason to use static properties for external user-settable
properties when it comes to device, i.e. to get list of properties user could
use command:
#qemu -device x86_64-cpu,?
x86_64-cpu.pmu=boolean
x86_64-cpu.hv-spinlocks=int
x86_64-cpu.hv-relaxed=boolean
x86_64-cpu.hv-vapic=boolean
x86_64-cpu.check=boolean
x86_64-cpu.enforce=boolean
which now yields only partial properties user is interested in, above
mentioned conversion patches make previously not available properties
visible to user via typical interface, perhaps eventually we could
drop list_cpu() interface in favor of -device foo-cpu,? command.
Taking in account Paolo's cleanup of legacy properties in static properties,
it might make them more suitable for moving concept to Object level.
(As far as I remember, Anthony objected to it due to existence of legacy
properties).
> >
> > Andreas
> >
> > --
> > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>
>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties
2014-02-06 15:19 ` Igor Mammedov
@ 2014-02-06 15:51 ` Andreas Färber
2014-02-06 16:16 ` [Qemu-devel] CPU models and feature probing (was Re: [PATCH qom-cpu 00/16 v10] target-i386: convert CPU) " Eduardo Habkost
0 siblings, 1 reply; 53+ messages in thread
From: Andreas Färber @ 2014-02-06 15:51 UTC (permalink / raw)
To: Igor Mammedov
Cc: Eduardo Habkost, qemu-devel, armbru, Anthony Liguori, pbonzini,
akong
Am 06.02.2014 16:19, schrieb Igor Mammedov:
> On Wed, 5 Feb 2014 17:52:16 +0100
> Igor Mammedov <imammedo@redhat.com> wrote:
>
>> On Wed, 05 Feb 2014 17:14:27 +0100
>> Andreas Färber <afaerber@suse.de> wrote:
>>
>>> Am 05.02.2014 15:40, schrieb Igor Mammedov:
>>>> On Sun, 15 Dec 2013 23:50:47 +0100
>>>> Andreas Färber <afaerber@suse.de> wrote:
>>>>
>>>>> Am 27.11.2013 23:28, schrieb Igor Mammedov:
>>>>>> Igor Mammedov (16):
>>>>>> target-i386: cleanup 'foo' feature handling'
>>>>>> target-i386: cleanup 'foo=val' feature handling
>>>>>
>>>>> Thanks, I've queued these on qom-cpu-next:
>>>>> https://github.com/afaerber/qemu-cpu/commits/qom-cpu-next
>>>>>
>>>>>> target-i386: cpu: convert 'level' to static property
>>>>>> target-i386: cpu: convert 'xlevel' to static property
>>>>>> target-i386: cpu: convert 'family' to static property
>>>>>> target-i386: cpu: convert 'model' to static property
>>>>>> target-i386: cpu: convert 'stepping' to static property
>>>>>> target-i386: cpu: convert 'vendor' to static property
>>>>>> target-i386: cpu: convert 'model-id' to static property
>>>>>> target-i386: cpu: convert 'tsc-frequency' to static property
>>>>>
>>>>> But I still don't see the utility of this conversion after all the
>>>>> discussions we've had... :(
>>>> It seems there is movement to make DEVICE self describing for purpose
>>>> of QAPI schema introspection, where static properties would be used
>>>> (dynamic ones are not suitable for this purpose)
>>>
>>> Do you have a pointer to such a discussion? Sounds like I was not
>>> involved and Anthony probably not either...
>> Not at the moment, CCing people who might know more about the topic.
>>
>> But just thinking about creating QAPI schema for devices, it's not really
>> possible to generate one using dynamic properties (unless one resorts to
>> creating every supported device instance), while arrays of static properties
>> are there for every device and simplistically speaking just need conversion
>> to schema format.
>
> There is one more reason to use static properties for external user-settable
> properties when it comes to device, i.e. to get list of properties user could
> use command:
> #qemu -device x86_64-cpu,?
> x86_64-cpu.pmu=boolean
> x86_64-cpu.hv-spinlocks=int
> x86_64-cpu.hv-relaxed=boolean
> x86_64-cpu.hv-vapic=boolean
> x86_64-cpu.check=boolean
> x86_64-cpu.enforce=boolean
>
> which now yields only partial properties user is interested in, above
> mentioned conversion patches make previously not available properties
> visible to user via typical interface, perhaps eventually we could
> drop list_cpu() interface in favor of -device foo-cpu,? command.
We already brought that up specifically for decision on a KVM call and
Anthony's clear statement was that the expected way for management tools
to discover properties was to instantiate the object and run qom-list on it.
It is a known issue, both for info qtree and -device, that they do not
list all properties. But I don't want to repeat this discussion over and
over again: Paolo's patches for static properties were rejected by
Anthony, therefore I could not queue them on qom-next back then and
therefore I had to code my properties for the X86CPU (which was not yet
a device back then) the new QOM way, and now you're trying to override
Anthony's decision in forcing me to accept patches that Anthony had
vetoed against!
If you or libvirt need all properties for -device, then send a patch. No
one did for two years, so apparently no one cared.
Static properties are considered a valid, convenient way to define
properties for a device but not the sole one for a device or object.
Using info qtree or -device as justification for implementation
decisions is backwards and wrong since those are considered legacy.
And specifically for libvirt Eduardo pushed into a release properties
that could be used to inspect CPUIDs. If that's not being used by
libvirt, as Eduardo seems to imply now, why did we put work in that in
the first place?
If there's no relation between a CPU model named, e.g., "Haswell" and
the one on an Intel Haswell chip any more, then we should give them
artificial names like "qemu64"; I strongly believe that Haswell
definition in code should match that of the specs/hardware and if TCG
can't emulate something that's one thing (subtractive: no AVX) and if
KVM wants to speed up things that's another (additive: kvmvapic,
in-kernel IRQ/PIT). What I am arguing against is watering the meaning of
our definitions from "this is this model" to "this is handy". In
particular, if we use the post_initialize hook like I suggested then
-global is still able to override it and Eduardo's properties should
correctly report them to libvirt.
> Taking in account Paolo's cleanup of legacy properties in static properties,
> it might make them more suitable for moving concept to Object level.
> (As far as I remember, Anthony objected to it due to existence of legacy
> properties).
That'll be for Anthony to answer, but static properties at Object level
would still not expose child<> and especially not link<> properties to
the user.
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 53+ messages in thread
* [Qemu-devel] CPU models and feature probing (was Re: [PATCH qom-cpu 00/16 v10] target-i386: convert CPU) features into properties
2014-02-06 15:51 ` Andreas Färber
@ 2014-02-06 16:16 ` Eduardo Habkost
2014-02-06 16:57 ` Andreas Färber
0 siblings, 1 reply; 53+ messages in thread
From: Eduardo Habkost @ 2014-02-06 16:16 UTC (permalink / raw)
To: Andreas Färber
Cc: libvir-list, armbru, qemu-devel, Anthony Liguori, pbonzini,
Igor Mammedov, Jiri Denemark, akong
(CCing libvir-list again, as this is continuing a discussion about a
subject that interests libvirt developers, from another thread.)
On Thu, Feb 06, 2014 at 04:51:17PM +0100, Andreas Färber wrote:
> Am 06.02.2014 16:19, schrieb Igor Mammedov:
> > On Wed, 5 Feb 2014 17:52:16 +0100
> > Igor Mammedov <imammedo@redhat.com> wrote:
> >
> >> On Wed, 05 Feb 2014 17:14:27 +0100
> >> Andreas Färber <afaerber@suse.de> wrote:
> >>
> >>> Am 05.02.2014 15:40, schrieb Igor Mammedov:
> >>>> On Sun, 15 Dec 2013 23:50:47 +0100
> >>>> Andreas Färber <afaerber@suse.de> wrote:
> >>>>
> >>>>> Am 27.11.2013 23:28, schrieb Igor Mammedov:
> >>>>>> Igor Mammedov (16):
> >>>>>> target-i386: cleanup 'foo' feature handling'
> >>>>>> target-i386: cleanup 'foo=val' feature handling
> >>>>>
> >>>>> Thanks, I've queued these on qom-cpu-next:
> >>>>> https://github.com/afaerber/qemu-cpu/commits/qom-cpu-next
> >>>>>
> >>>>>> target-i386: cpu: convert 'level' to static property
> >>>>>> target-i386: cpu: convert 'xlevel' to static property
> >>>>>> target-i386: cpu: convert 'family' to static property
> >>>>>> target-i386: cpu: convert 'model' to static property
> >>>>>> target-i386: cpu: convert 'stepping' to static property
> >>>>>> target-i386: cpu: convert 'vendor' to static property
> >>>>>> target-i386: cpu: convert 'model-id' to static property
> >>>>>> target-i386: cpu: convert 'tsc-frequency' to static property
> >>>>>
> >>>>> But I still don't see the utility of this conversion after all the
> >>>>> discussions we've had... :(
> >>>> It seems there is movement to make DEVICE self describing for purpose
> >>>> of QAPI schema introspection, where static properties would be used
> >>>> (dynamic ones are not suitable for this purpose)
> >>>
> >>> Do you have a pointer to such a discussion? Sounds like I was not
> >>> involved and Anthony probably not either...
> >> Not at the moment, CCing people who might know more about the topic.
> >>
> >> But just thinking about creating QAPI schema for devices, it's not really
> >> possible to generate one using dynamic properties (unless one resorts to
> >> creating every supported device instance), while arrays of static properties
> >> are there for every device and simplistically speaking just need conversion
> >> to schema format.
> >
> > There is one more reason to use static properties for external user-settable
> > properties when it comes to device, i.e. to get list of properties user could
> > use command:
> > #qemu -device x86_64-cpu,?
> > x86_64-cpu.pmu=boolean
> > x86_64-cpu.hv-spinlocks=int
> > x86_64-cpu.hv-relaxed=boolean
> > x86_64-cpu.hv-vapic=boolean
> > x86_64-cpu.check=boolean
> > x86_64-cpu.enforce=boolean
> >
> > which now yields only partial properties user is interested in, above
> > mentioned conversion patches make previously not available properties
> > visible to user via typical interface, perhaps eventually we could
> > drop list_cpu() interface in favor of -device foo-cpu,? command.
>
> We already brought that up specifically for decision on a KVM call and
> Anthony's clear statement was that the expected way for management tools
> to discover properties was to instantiate the object and run qom-list on it.
As far as I remember, this was decided as the recommended way to list
the feature _default values_ (e.g. discover which features are going to
be enabled on each CPU model). Is this really the recommended way to
discover which properties can be set on device_add, too? We are not
going to have any other instrospection mechanism that won't require
objects to be created?
>
> It is a known issue, both for info qtree and -device, that they do not
> list all properties. But I don't want to repeat this discussion over and
> over again: Paolo's patches for static properties were rejected by
> Anthony, therefore I could not queue them on qom-next back then and
> therefore I had to code my properties for the X86CPU (which was not yet
> a device back then) the new QOM way, and now you're trying to override
> Anthony's decision in forcing me to accept patches that Anthony had
> vetoed against!
>
> If you or libvirt need all properties for -device, then send a patch. No
> one did for two years, so apparently no one cared.
>
> Static properties are considered a valid, convenient way to define
> properties for a device but not the sole one for a device or object.
> Using info qtree or -device as justification for implementation
> decisions is backwards and wrong since those are considered legacy.
So, let me ask again, explicitly: we are not going to ever have a
QMP-based interface that will allow all available device_add options to
be queried without instantiating an object first? This really surprises
me.
(That's not a question just for Andreas. I would like to hear from
Paolo, Anthony, and others.)
>
> And specifically for libvirt Eduardo pushed into a release properties
> that could be used to inspect CPUIDs. If that's not being used by
> libvirt, as Eduardo seems to imply now, why did we put work in that in
> the first place?
It is not being used by libvirt because the current interface is
unusable without running QEMU once for each CPU model. With the CPU
model subclasses, we will be able to make it possible to create/destroy
CPUs of different models in a single QEMU instance, and then libvirt
will be able to use it.
>
> If there's no relation between a CPU model named, e.g., "Haswell" and
> the one on an Intel Haswell chip any more, then we should give them
> artificial names like "qemu64"; I strongly believe that Haswell
> definition in code should match that of the specs/hardware and if TCG
> can't emulate something that's one thing (subtractive: no AVX) and if
> KVM wants to speed up things that's another (additive: kvmvapic,
> in-kernel IRQ/PIT). What I am arguing against is watering the meaning of
> our definitions from "this is this model" to "this is handy". In
> particular, if we use the post_initialize hook like I suggested then
> -global is still able to override it and Eduardo's properties should
> correctly report them to libvirt.
"Haswell" is named this way not only because it looks like Haswell, but
also because it has useful features you are going to find only on
Haswell, and you (probably) are not going to be able to run it on hosts
older than Haswell. That's the main real-world application of CPU
models: making sure the VMs can run on a specific subset of hosts. So,
if you choose "Haswell", you are telling the management stack "I know
this is going to run only on Haswell and newer CPUs".
That's why x2apic is being proposed as an exception: it can be enabled
on any host, because it doesn't depend on host-side support. That's why
I propose we enable it on CPU models that don't have x2apic in the real
world.
(BTW, what is the relation between this subject and static properties? I
was expecting this to be discussed in the other existing thread about
x2apic)
>
> > Taking in account Paolo's cleanup of legacy properties in static properties,
> > it might make them more suitable for moving concept to Object level.
> > (As far as I remember, Anthony objected to it due to existence of legacy
> > properties).
>
> That'll be for Anthony to answer, but static properties at Object level
> would still not expose child<> and especially not link<> properties to
> the user.
>
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
--
Eduardo
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] CPU models and feature probing (was Re: [PATCH qom-cpu 00/16 v10] target-i386: convert CPU) features into properties
2014-02-06 16:16 ` [Qemu-devel] CPU models and feature probing (was Re: [PATCH qom-cpu 00/16 v10] target-i386: convert CPU) " Eduardo Habkost
@ 2014-02-06 16:57 ` Andreas Färber
2014-02-07 10:16 ` Eduardo Habkost
2014-02-07 10:37 ` Eduardo Habkost
0 siblings, 2 replies; 53+ messages in thread
From: Andreas Färber @ 2014-02-06 16:57 UTC (permalink / raw)
To: Eduardo Habkost
Cc: libvir-list, armbru, qemu-devel, Anthony Liguori, pbonzini,
Igor Mammedov, Jiri Denemark, akong
Am 06.02.2014 17:16, schrieb Eduardo Habkost:
> (CCing libvir-list again, as this is continuing a discussion about a
> subject that interests libvirt developers, from another thread.)
>
> On Thu, Feb 06, 2014 at 04:51:17PM +0100, Andreas Färber wrote:
>> Am 06.02.2014 16:19, schrieb Igor Mammedov:
>>> On Wed, 5 Feb 2014 17:52:16 +0100
>>> Igor Mammedov <imammedo@redhat.com> wrote:
>>>
>>>> On Wed, 05 Feb 2014 17:14:27 +0100
>>>> Andreas Färber <afaerber@suse.de> wrote:
>>>>
>>>>> Am 05.02.2014 15:40, schrieb Igor Mammedov:
>>>>>> On Sun, 15 Dec 2013 23:50:47 +0100
>>>>>> Andreas Färber <afaerber@suse.de> wrote:
>>>>>>
>>>>>>> Am 27.11.2013 23:28, schrieb Igor Mammedov:
>>>>>>>> Igor Mammedov (16):
>>>>>>>> target-i386: cleanup 'foo' feature handling'
>>>>>>>> target-i386: cleanup 'foo=val' feature handling
>>>>>>>
>>>>>>> Thanks, I've queued these on qom-cpu-next:
>>>>>>> https://github.com/afaerber/qemu-cpu/commits/qom-cpu-next
>>>>>>>
>>>>>>>> target-i386: cpu: convert 'level' to static property
>>>>>>>> target-i386: cpu: convert 'xlevel' to static property
>>>>>>>> target-i386: cpu: convert 'family' to static property
>>>>>>>> target-i386: cpu: convert 'model' to static property
>>>>>>>> target-i386: cpu: convert 'stepping' to static property
>>>>>>>> target-i386: cpu: convert 'vendor' to static property
>>>>>>>> target-i386: cpu: convert 'model-id' to static property
>>>>>>>> target-i386: cpu: convert 'tsc-frequency' to static property
>>>>>>>
>>>>>>> But I still don't see the utility of this conversion after all the
>>>>>>> discussions we've had... :(
>>>>>> It seems there is movement to make DEVICE self describing for purpose
>>>>>> of QAPI schema introspection, where static properties would be used
>>>>>> (dynamic ones are not suitable for this purpose)
>>>>>
>>>>> Do you have a pointer to such a discussion? Sounds like I was not
>>>>> involved and Anthony probably not either...
>>>> Not at the moment, CCing people who might know more about the topic.
>>>>
>>>> But just thinking about creating QAPI schema for devices, it's not really
>>>> possible to generate one using dynamic properties (unless one resorts to
>>>> creating every supported device instance), while arrays of static properties
>>>> are there for every device and simplistically speaking just need conversion
>>>> to schema format.
>>>
>>> There is one more reason to use static properties for external user-settable
>>> properties when it comes to device, i.e. to get list of properties user could
>>> use command:
>>> #qemu -device x86_64-cpu,?
>>> x86_64-cpu.pmu=boolean
>>> x86_64-cpu.hv-spinlocks=int
>>> x86_64-cpu.hv-relaxed=boolean
>>> x86_64-cpu.hv-vapic=boolean
>>> x86_64-cpu.check=boolean
>>> x86_64-cpu.enforce=boolean
>>>
>>> which now yields only partial properties user is interested in, above
>>> mentioned conversion patches make previously not available properties
>>> visible to user via typical interface, perhaps eventually we could
>>> drop list_cpu() interface in favor of -device foo-cpu,? command.
>>
>> We already brought that up specifically for decision on a KVM call and
>> Anthony's clear statement was that the expected way for management tools
>> to discover properties was to instantiate the object and run qom-list on it.
>
> As far as I remember, this was decided as the recommended way to list
> the feature _default values_ (e.g. discover which features are going to
> be enabled on each CPU model). Is this really the recommended way to
> discover which properties can be set on device_add, too? We are not
> going to have any other instrospection mechanism that won't require
> objects to be created?
It is, and no, no other introspection mechanism on the horizon.
I specifically suggested to have a copy of the ObjectProperty list in
ObjectClass (today it is just in Object) but Anthony rejected that idea
due to the dynamic nature of properties in QOM and pointed us to said
qom-list and object instantiation as the sole means for discovering
availability of properties.
And it's true that we could in fact just instantiate the object for
-device foo,? - it's just that nobody wrote code for that. I didn't do
the original QOM conversion so I don't feel guilty, I don't normally use
-device foo,? so not affected, I followed Anthony's instructions for how
to and how not to implement things. In a few cases Anthony has changed
his mind (method inheritence for instance) but for that you'll need to
discuss with him and not just argue with me about something that Anthony
has decided in the past. That's just pissing me off because it feels
like a waste of my time.
>> It is a known issue, both for info qtree and -device, that they do not
>> list all properties. But I don't want to repeat this discussion over and
>> over again: Paolo's patches for static properties were rejected by
>> Anthony, therefore I could not queue them on qom-next back then and
>> therefore I had to code my properties for the X86CPU (which was not yet
>> a device back then) the new QOM way, and now you're trying to override
>> Anthony's decision in forcing me to accept patches that Anthony had
>> vetoed against!
>>
>> If you or libvirt need all properties for -device, then send a patch. No
>> one did for two years, so apparently no one cared.
>>
>> Static properties are considered a valid, convenient way to define
>> properties for a device but not the sole one for a device or object.
>> Using info qtree or -device as justification for implementation
>> decisions is backwards and wrong since those are considered legacy.
>
> So, let me ask again, explicitly: we are not going to ever have a
> QMP-based interface that will allow all available device_add options to
> be queried without instantiating an object first? This really surprises
> me.
>
> (That's not a question just for Andreas. I would like to hear from
> Paolo, Anthony, and others.)
>
>
>>
>> And specifically for libvirt Eduardo pushed into a release properties
>> that could be used to inspect CPUIDs. If that's not being used by
>> libvirt, as Eduardo seems to imply now, why did we put work in that in
>> the first place?
>
> It is not being used by libvirt because the current interface is
> unusable without running QEMU once for each CPU model. With the CPU
> model subclasses, we will be able to make it possible to create/destroy
> CPUs of different models in a single QEMU instance, and then libvirt
> will be able to use it.
OK, that I understand now, so let's be sure to get (the remainder of)
that subclasses series through my review mills in time for 2.0. :)
>> If there's no relation between a CPU model named, e.g., "Haswell" and
>> the one on an Intel Haswell chip any more, then we should give them
>> artificial names like "qemu64"; I strongly believe that Haswell
>> definition in code should match that of the specs/hardware and if TCG
>> can't emulate something that's one thing (subtractive: no AVX) and if
>> KVM wants to speed up things that's another (additive: kvmvapic,
>> in-kernel IRQ/PIT). What I am arguing against is watering the meaning of
>> our definitions from "this is this model" to "this is handy". In
>> particular, if we use the post_initialize hook like I suggested then
>> -global is still able to override it and Eduardo's properties should
>> correctly report them to libvirt.
>
> "Haswell" is named this way not only because it looks like Haswell, but
> also because it has useful features you are going to find only on
> Haswell, and you (probably) are not going to be able to run it on hosts
> older than Haswell. That's the main real-world application of CPU
> models: making sure the VMs can run on a specific subset of hosts. So,
> if you choose "Haswell", you are telling the management stack "I know
> this is going to run only on Haswell and newer CPUs".
>
> That's why x2apic is being proposed as an exception: it can be enabled
> on any host, because it doesn't depend on host-side support. That's why
> I propose we enable it on CPU models that don't have x2apic in the real
> world.
>
> (BTW, what is the relation between this subject and static properties? I
> was expecting this to be discussed in the other existing thread about
> x2apic)
Sorry if I replied to two different series at once, that was for
"Enabling x2apic by default in KvM (was Re: [Qemu-devel] [PATCH]
target-i386: enable x2apic by default on more recent CPU models)".
Which is connected to CPU models/subclasses in what those types are
supposed to be good for if we define them.
Let's take an obvious example. Jan wants to emulate a legacy isapc
machine with -cpu 486. Then it feels counterproductive to enable the
latest and greatest features such a machine never had. If the user wants
to have the latest and greatest features, she can choose a newer -cpu
model which already has the flag today.
Regards,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] CPU models and feature probing (was Re: [PATCH qom-cpu 00/16 v10] target-i386: convert CPU) features into properties
2014-02-06 16:57 ` Andreas Färber
@ 2014-02-07 10:16 ` Eduardo Habkost
2014-02-07 10:55 ` Paolo Bonzini
2014-02-07 10:37 ` Eduardo Habkost
1 sibling, 1 reply; 53+ messages in thread
From: Eduardo Habkost @ 2014-02-07 10:16 UTC (permalink / raw)
To: Andreas Färber
Cc: libvir-list, armbru, qemu-devel, Anthony Liguori, pbonzini,
Igor Mammedov, Jiri Denemark, akong
On Thu, Feb 06, 2014 at 05:57:38PM +0100, Andreas Färber wrote:
[...]
> And it's true that we could in fact just instantiate the object for
> -device foo,? - it's just that nobody wrote code for that. I didn't do
> the original QOM conversion so I don't feel guilty, I don't normally use
> -device foo,? so not affected, I followed Anthony's instructions for how
> to and how not to implement things. In a few cases Anthony has changed
> his mind (method inheritence for instance) but for that you'll need to
> discuss with him and not just argue with me about something that Anthony
> has decided in the past. That's just pissing me off because it feels
> like a waste of my time.
You are not alone. I remember we spent lots of time trying to convince
Anthony to allow global properties and compat_props affect dynamic
properties not just static properties, and static properties were a big
deal due to reasons I didn't understand completely. Now I am hearing the
opposite message, and I don't understand the reasons for the change of
plans. I am confused.
--
Eduardo
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] CPU models and feature probing (was Re: [PATCH qom-cpu 00/16 v10] target-i386: convert CPU) features into properties
2014-02-07 10:16 ` Eduardo Habkost
@ 2014-02-07 10:55 ` Paolo Bonzini
2014-02-11 11:54 ` Eduardo Habkost
2014-02-11 14:31 ` Anthony Liguori
0 siblings, 2 replies; 53+ messages in thread
From: Paolo Bonzini @ 2014-02-07 10:55 UTC (permalink / raw)
To: Eduardo Habkost, Andreas Färber
Cc: libvir-list, armbru, qemu-devel, Anthony Liguori, Igor Mammedov,
Jiri Denemark, akong
Il 07/02/2014 11:16, Eduardo Habkost ha scritto:
> You are not alone. I remember we spent lots of time trying to convince
> Anthony to allow global properties and compat_props affect dynamic
> properties not just static properties, and static properties were a big
> deal due to reasons I didn't understand completely. Now I am hearing the
> opposite message, and I don't understand the reasons for the change of
> plans. I am confused.
Picture me confused as well, but at the same I think I understand the
reasons for the change of plans.
At some point you have to acknowledge that the grand vision of QEMU as a
small core and management only poking at QOM objects with properties has
never materialized, and probably never will.
After seeing no progress whatsoever on
http://wiki.qemu.org/Features/QOM#TODO after 2 years, we should perhaps
try to get fresh ideas into QOM based on how we've been using it and
what we'd like to do with it. "Design by committee" (more accurately:
"design by prophet") will not lead us anywhere.
QOM definitely needs dynamic properties for child<>, and for tricks such
as simulation of array properties. However, assume Igor or Eduardo or
Marcel can come up with a new QAPI-friendly static properties design,
that combines the best feature of QOM dynamic properties and qdev static
properties, why should it be rejected?
Code should not be frozen against some abstract design, it must evolve
to solve concrete problems until it can solve all of them easily. Or do
we want to become a project where good code is not anymore the best way
to have other developers change their mind?
Paolo
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] CPU models and feature probing (was Re: [PATCH qom-cpu 00/16 v10] target-i386: convert CPU) features into properties
2014-02-07 10:55 ` Paolo Bonzini
@ 2014-02-11 11:54 ` Eduardo Habkost
2014-02-11 14:31 ` Anthony Liguori
1 sibling, 0 replies; 53+ messages in thread
From: Eduardo Habkost @ 2014-02-11 11:54 UTC (permalink / raw)
To: Paolo Bonzini
Cc: libvir-list, qemu-devel, armbru, Anthony Liguori, Igor Mammedov,
Jiri Denemark, akong, Andreas Färber
On Fri, Feb 07, 2014 at 11:55:25AM +0100, Paolo Bonzini wrote:
> Il 07/02/2014 11:16, Eduardo Habkost ha scritto:
> >You are not alone. I remember we spent lots of time trying to convince
> >Anthony to allow global properties and compat_props affect dynamic
> >properties not just static properties, and static properties were a big
> >deal due to reasons I didn't understand completely. Now I am hearing the
> >opposite message, and I don't understand the reasons for the change of
> >plans. I am confused.
>
> Picture me confused as well, but at the same I think I understand
> the reasons for the change of plans.
>
> At some point you have to acknowledge that the grand vision of QEMU
> as a small core and management only poking at QOM objects with
> properties has never materialized, and probably never will.
>
> After seeing no progress whatsoever on
> http://wiki.qemu.org/Features/QOM#TODO after 2 years, we should
> perhaps try to get fresh ideas into QOM based on how we've been
> using it and what we'd like to do with it. "Design by committee"
> (more accurately: "design by prophet") will not lead us anywhere.
>
> QOM definitely needs dynamic properties for child<>, and for tricks
> such as simulation of array properties. However, assume Igor or
> Eduardo or Marcel can come up with a new QAPI-friendly static
> properties design, that combines the best feature of QOM dynamic
> properties and qdev static properties, why should it be rejected?
>
> Code should not be frozen against some abstract design, it must
> evolve to solve concrete problems until it can solve all of them
> easily. Or do we want to become a project where good code is not
> anymore the best way to have other developers change their mind?
Also, I don't see the point in deprecating and rejecting code that use
something which:
* Is used ~230 times, in ~180 different source files in QEMU;
* Is useful;
* Doesn't have a replacement yet.
Currently, static properties are useful, facilitate analyzing the code
before compile time, facilitate exposing class information through QMP
(with the existing "device-list-properties" command), and is better than
requiring objects to be created just to find out which properties are
available.
It may replaced by something better in the future, but that's what we
have today.
--
Eduardo
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] CPU models and feature probing (was Re: [PATCH qom-cpu 00/16 v10] target-i386: convert CPU) features into properties
2014-02-07 10:55 ` Paolo Bonzini
2014-02-11 11:54 ` Eduardo Habkost
@ 2014-02-11 14:31 ` Anthony Liguori
2014-02-11 15:25 ` Eduardo Habkost
1 sibling, 1 reply; 53+ messages in thread
From: Anthony Liguori @ 2014-02-11 14:31 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Eduardo Habkost, libvir-list, Markus Armbruster, qemu-devel,
Anthony Liguori, Igor Mammedov, Jiri Denemark, Amos Kong,
Andreas Färber
On Fri, Feb 7, 2014 at 2:55 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 07/02/2014 11:16, Eduardo Habkost ha scritto:
>
>> You are not alone. I remember we spent lots of time trying to convince
>> Anthony to allow global properties and compat_props affect dynamic
>> properties not just static properties, and static properties were a big
>> deal due to reasons I didn't understand completely. Now I am hearing the
>> opposite message, and I don't understand the reasons for the change of
>> plans. I am confused.
>
>
> Picture me confused as well, but at the same I think I understand the
> reasons for the change of plans.
There's no real convincing. It's just a question of code. There are
no defaults in classes for dynamic properties to modify. compat_props
are a nice mechanism, making them work for all properties is a
reasonable thing to do.
Regards,
Anthony Liguori
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] CPU models and feature probing (was Re: [PATCH qom-cpu 00/16 v10] target-i386: convert CPU) features into properties
2014-02-11 14:31 ` Anthony Liguori
@ 2014-02-11 15:25 ` Eduardo Habkost
2014-02-11 15:58 ` Anthony Liguori
0 siblings, 1 reply; 53+ messages in thread
From: Eduardo Habkost @ 2014-02-11 15:25 UTC (permalink / raw)
To: Anthony Liguori
Cc: libvir-list, qemu-devel, Markus Armbruster, Anthony Liguori,
Igor Mammedov, Paolo Bonzini, Jiri Denemark, Amos Kong,
Andreas Färber
On Tue, Feb 11, 2014 at 06:31:35AM -0800, Anthony Liguori wrote:
> On Fri, Feb 7, 2014 at 2:55 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > Il 07/02/2014 11:16, Eduardo Habkost ha scritto:
> >
> >> You are not alone. I remember we spent lots of time trying to convince
> >> Anthony to allow global properties and compat_props affect dynamic
> >> properties not just static properties, and static properties were a big
> >> deal due to reasons I didn't understand completely. Now I am hearing the
> >> opposite message, and I don't understand the reasons for the change of
> >> plans. I am confused.
> >
> >
> > Picture me confused as well, but at the same I think I understand the
> > reasons for the change of plans.
>
> There's no real convincing. It's just a question of code.
I am sure there's a lot of convincing involved, even after the code is
written (in this case, 15 months after the code was written).
> There are
> no defaults in classes for dynamic properties to modify. compat_props
> are a nice mechanism, making them work for all properties is a
> reasonable thing to do.
That's exactly the opposite of what you said before[1]. But that isn't
supposed to be a problem, I understand there may be change of plans (we
should be able to change our minds).
What I don't understand is the rejection of code that works, matches the
style used by 200+ other source files, adds more useful introspectable
information, done in the way that was suggested 16 months ago, because
we have some rough idea about how a new grand design will look like in
the far future.
[1] http://lists.gnu.org/archive/html/qemu-devel/2013-06/msg00990.html
--
Eduardo
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] CPU models and feature probing (was Re: [PATCH qom-cpu 00/16 v10] target-i386: convert CPU) features into properties
2014-02-11 15:25 ` Eduardo Habkost
@ 2014-02-11 15:58 ` Anthony Liguori
2014-02-11 16:43 ` Eduardo Habkost
2014-02-11 16:55 ` Andreas Färber
0 siblings, 2 replies; 53+ messages in thread
From: Anthony Liguori @ 2014-02-11 15:58 UTC (permalink / raw)
To: Eduardo Habkost
Cc: libvir-list, qemu-devel, Markus Armbruster, Anthony Liguori,
Igor Mammedov, Paolo Bonzini, Jiri Denemark, Amos Kong,
Andreas Färber
On Tue, Feb 11, 2014 at 7:25 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Tue, Feb 11, 2014 at 06:31:35AM -0800, Anthony Liguori wrote:
>> On Fri, Feb 7, 2014 at 2:55 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> > Il 07/02/2014 11:16, Eduardo Habkost ha scritto:
>> >
>> >> You are not alone. I remember we spent lots of time trying to convince
>> >> Anthony to allow global properties and compat_props affect dynamic
>> >> properties not just static properties, and static properties were a big
>> >> deal due to reasons I didn't understand completely. Now I am hearing the
>> >> opposite message, and I don't understand the reasons for the change of
>> >> plans. I am confused.
>> >
>> >
>> > Picture me confused as well, but at the same I think I understand the
>> > reasons for the change of plans.
>>
>> There's no real convincing. It's just a question of code.
>
> I am sure there's a lot of convincing involved, even after the code is
> written (in this case, 15 months after the code was written).
N.B. the code you refer to doesn't "make global propeties and
compat_props affect dynamic properties." It converts CPU properties
to static properties which I'm pretty sure I said many times is a
perfectly reasonable thing to do.
>> There are
>> no defaults in classes for dynamic properties to modify. compat_props
>> are a nice mechanism, making them work for all properties is a
>> reasonable thing to do.
>
> That's exactly the opposite of what you said before[1]. But that isn't
> supposed to be a problem, I understand there may be change of plans (we
> should be able to change our minds).
I think you're confusing a few things. You cannot make dynamic
properties work with globals today. Globals change class default
values and there are no class defaults for dynamic properties.[*]
There's a perfectly valid discussion to have about whether we should
even have dynamic properties. It's certainly been a long time since
they were introduced and they haven't made their way into all that
many devices so it's reasonable to say that perhaps we'd be better off
without them. I would not object to a patch series that moved
properties to classes entirely provided it removed existing uses of
dynamic properties and didn't just introduce yet another mechanism.
But compat properties as a concept could be made to work with dynamic
properties. They would have to be evaluated after instance init.
There's quite a few places they would end up touching I suspect.
Another point of confusion worth mention is legacy properties since
this usually comes up in the discussion. Legacy properties (the
properties that are set/get as strings) are something that we should
try to avoid. They end up as strings on the wire and make it harder
to write client code.
* I recognize that compat_props are implemented as globals. I'm
really talking about the current implementation of globals, not the
concept of -global which could be made with dynamic properties.
Regards,
Anthony Liguori
> What I don't understand is the rejection of code that works, matches the
> style used by 200+ other source files, adds more useful introspectable
> information, done in the way that was suggested 16 months ago, because
> we have some rough idea about how a new grand design will look like in
> the far future.
>
> [1] http://lists.gnu.org/archive/html/qemu-devel/2013-06/msg00990.html
>
> --
> Eduardo
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] CPU models and feature probing (was Re: [PATCH qom-cpu 00/16 v10] target-i386: convert CPU) features into properties
2014-02-11 15:58 ` Anthony Liguori
@ 2014-02-11 16:43 ` Eduardo Habkost
2014-02-11 16:45 ` Paolo Bonzini
2014-02-11 16:55 ` Andreas Färber
1 sibling, 1 reply; 53+ messages in thread
From: Eduardo Habkost @ 2014-02-11 16:43 UTC (permalink / raw)
To: Anthony Liguori
Cc: libvir-list, qemu-devel, Markus Armbruster, Anthony Liguori,
Igor Mammedov, Paolo Bonzini, Jiri Denemark, Amos Kong,
Andreas Färber
On Tue, Feb 11, 2014 at 07:58:30AM -0800, Anthony Liguori wrote:
> On Tue, Feb 11, 2014 at 7:25 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Tue, Feb 11, 2014 at 06:31:35AM -0800, Anthony Liguori wrote:
> >> On Fri, Feb 7, 2014 at 2:55 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> > Il 07/02/2014 11:16, Eduardo Habkost ha scritto:
> >> >
> >> >> You are not alone. I remember we spent lots of time trying to convince
> >> >> Anthony to allow global properties and compat_props affect dynamic
> >> >> properties not just static properties, and static properties were a big
> >> >> deal due to reasons I didn't understand completely. Now I am hearing the
> >> >> opposite message, and I don't understand the reasons for the change of
> >> >> plans. I am confused.
> >> >
> >> >
> >> > Picture me confused as well, but at the same I think I understand the
> >> > reasons for the change of plans.
> >>
> >> There's no real convincing. It's just a question of code.
> >
> > I am sure there's a lot of convincing involved, even after the code is
> > written (in this case, 15 months after the code was written).
>
> N.B. the code you refer to doesn't "make global propeties and
> compat_props affect dynamic properties." It converts CPU properties
> to static properties which I'm pretty sure I said many times is a
> perfectly reasonable thing to do.
Exactly. Have you read the rest of this thread?
>
> >> There are
> >> no defaults in classes for dynamic properties to modify. compat_props
> >> are a nice mechanism, making them work for all properties is a
> >> reasonable thing to do.
> >
> > That's exactly the opposite of what you said before[1]. But that isn't
> > supposed to be a problem, I understand there may be change of plans (we
> > should be able to change our minds).
>
> I think you're confusing a few things. You cannot make dynamic
> properties work with globals today. Globals change class default
> values and there are no class defaults for dynamic properties.[*]
They work today. Not that people _should_ use -global with them, but it
works. All we really needed (before this series) was to make
compat_props and -cpu be able to affect the dynamic properties.
>
> There's a perfectly valid discussion to have about whether we should
> even have dynamic properties. It's certainly been a long time since
> they were introduced and they haven't made their way into all that
> many devices so it's reasonable to say that perhaps we'd be better off
> without them. I would not object to a patch series that moved
> properties to classes entirely provided it removed existing uses of
> dynamic properties and didn't just introduce yet another mechanism.
That sounds like the opposite of what I have been reading in this
thread. Now I am even more confused.
>
> But compat properties as a concept could be made to work with dynamic
> properties. They would have to be evaluated after instance init.
> There's quite a few places they would end up touching I suspect.
They already work.
--
Eduardo
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] CPU models and feature probing (was Re: [PATCH qom-cpu 00/16 v10] target-i386: convert CPU) features into properties
2014-02-11 16:43 ` Eduardo Habkost
@ 2014-02-11 16:45 ` Paolo Bonzini
0 siblings, 0 replies; 53+ messages in thread
From: Paolo Bonzini @ 2014-02-11 16:45 UTC (permalink / raw)
To: Eduardo Habkost, Anthony Liguori
Cc: libvir-list, qemu-devel, Markus Armbruster, Anthony Liguori,
Igor Mammedov, Jiri Denemark, Amos Kong, Andreas Färber
Il 11/02/2014 17:43, Eduardo Habkost ha scritto:
>> > But compat properties as a concept could be made to work with dynamic
>> > properties. They would have to be evaluated after instance init.
>> > There's quite a few places they would end up touching I suspect.
> They already work.
But if they work by an ad hoc mechanism, this series actually improves
the situation.
Paolo
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] CPU models and feature probing (was Re: [PATCH qom-cpu 00/16 v10] target-i386: convert CPU) features into properties
2014-02-11 15:58 ` Anthony Liguori
2014-02-11 16:43 ` Eduardo Habkost
@ 2014-02-11 16:55 ` Andreas Färber
2014-02-11 18:57 ` Anthony Liguori
1 sibling, 1 reply; 53+ messages in thread
From: Andreas Färber @ 2014-02-11 16:55 UTC (permalink / raw)
To: Anthony Liguori, Eduardo Habkost
Cc: libvir-list, Markus Armbruster, qemu-devel, Anthony Liguori,
Igor Mammedov, Paolo Bonzini, Jiri Denemark, Amos Kong
Am 11.02.2014 16:58, schrieb Anthony Liguori:
> On Tue, Feb 11, 2014 at 7:25 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> On Tue, Feb 11, 2014 at 06:31:35AM -0800, Anthony Liguori wrote:
>>> On Fri, Feb 7, 2014 at 2:55 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>> Il 07/02/2014 11:16, Eduardo Habkost ha scritto:
>>>>
>>>>> You are not alone. I remember we spent lots of time trying to convince
>>>>> Anthony to allow global properties and compat_props affect dynamic
>>>>> properties not just static properties, and static properties were a big
>>>>> deal due to reasons I didn't understand completely. Now I am hearing the
>>>>> opposite message, and I don't understand the reasons for the change of
>>>>> plans. I am confused.
>>>>
>>>>
>>>> Picture me confused as well, but at the same I think I understand the
>>>> reasons for the change of plans.
>>>
>>> There's no real convincing. It's just a question of code.
>>
>> I am sure there's a lot of convincing involved, even after the code is
>> written (in this case, 15 months after the code was written).
>
> N.B. the code you refer to doesn't "make global propeties and
> compat_props affect dynamic properties." It converts CPU properties
> to static properties which I'm pretty sure I said many times is a
> perfectly reasonable thing to do.
>
>>> There are
>>> no defaults in classes for dynamic properties to modify. compat_props
>>> are a nice mechanism, making them work for all properties is a
>>> reasonable thing to do.
>>
>> That's exactly the opposite of what you said before[1]. But that isn't
>> supposed to be a problem, I understand there may be change of plans (we
>> should be able to change our minds).
>
> I think you're confusing a few things. You cannot make dynamic
> properties work with globals today. Globals change class default
> values and there are no class defaults for dynamic properties.[*]
>
> There's a perfectly valid discussion to have about whether we should
> even have dynamic properties. It's certainly been a long time since
> they were introduced and they haven't made their way into all that
> many devices so it's reasonable to say that perhaps we'd be better off
> without them. I would not object to a patch series that moved
> properties to classes entirely provided it removed existing uses of
> dynamic properties and didn't just introduce yet another mechanism.
>
> But compat properties as a concept could be made to work with dynamic
> properties. They would have to be evaluated after instance init.
> There's quite a few places they would end up touching I suspect.
Erm, sorry, that is already implemented in qemu.git!? instance_post_init
by Eduardo plus glue by me.
Andreas
>
> Another point of confusion worth mention is legacy properties since
> this usually comes up in the discussion. Legacy properties (the
> properties that are set/get as strings) are something that we should
> try to avoid. They end up as strings on the wire and make it harder
> to write client code.
>
> * I recognize that compat_props are implemented as globals. I'm
> really talking about the current implementation of globals, not the
> concept of -global which could be made with dynamic properties.
>
> Regards,
>
> Anthony Liguori
>
>> What I don't understand is the rejection of code that works, matches the
>> style used by 200+ other source files, adds more useful introspectable
>> information, done in the way that was suggested 16 months ago, because
>> we have some rough idea about how a new grand design will look like in
>> the far future.
>>
>> [1] http://lists.gnu.org/archive/html/qemu-devel/2013-06/msg00990.html
>>
>> --
>> Eduardo
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] CPU models and feature probing (was Re: [PATCH qom-cpu 00/16 v10] target-i386: convert CPU) features into properties
2014-02-11 16:55 ` Andreas Färber
@ 2014-02-11 18:57 ` Anthony Liguori
2014-02-11 21:38 ` Paolo Bonzini
0 siblings, 1 reply; 53+ messages in thread
From: Anthony Liguori @ 2014-02-11 18:57 UTC (permalink / raw)
To: Andreas Färber
Cc: Eduardo Habkost, libvir-list, qemu-devel, Markus Armbruster,
Anthony Liguori, Igor Mammedov, Paolo Bonzini, Jiri Denemark,
Amos Kong
On Tue, Feb 11, 2014 at 8:55 AM, Andreas Färber <afaerber@suse.de> wrote:
> Am 11.02.2014 16:58, schrieb Anthony Liguori:
>> On Tue, Feb 11, 2014 at 7:25 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:
>>> On Tue, Feb 11, 2014 at 06:31:35AM -0800, Anthony Liguori wrote:
>>>> On Fri, Feb 7, 2014 at 2:55 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>> Il 07/02/2014 11:16, Eduardo Habkost ha scritto:
>>>>>
>>>>>> You are not alone. I remember we spent lots of time trying to convince
>>>>>> Anthony to allow global properties and compat_props affect dynamic
>>>>>> properties not just static properties, and static properties were a big
>>>>>> deal due to reasons I didn't understand completely. Now I am hearing the
>>>>>> opposite message, and I don't understand the reasons for the change of
>>>>>> plans. I am confused.
>>>>>
>>>>>
>>>>> Picture me confused as well, but at the same I think I understand the
>>>>> reasons for the change of plans.
>>>>
>>>> There's no real convincing. It's just a question of code.
>>>
>>> I am sure there's a lot of convincing involved, even after the code is
>>> written (in this case, 15 months after the code was written).
>>
>> N.B. the code you refer to doesn't "make global propeties and
>> compat_props affect dynamic properties." It converts CPU properties
>> to static properties which I'm pretty sure I said many times is a
>> perfectly reasonable thing to do.
>>
>>>> There are
>>>> no defaults in classes for dynamic properties to modify. compat_props
>>>> are a nice mechanism, making them work for all properties is a
>>>> reasonable thing to do.
>>>
>>> That's exactly the opposite of what you said before[1]. But that isn't
>>> supposed to be a problem, I understand there may be change of plans (we
>>> should be able to change our minds).
>>
>> I think you're confusing a few things. You cannot make dynamic
>> properties work with globals today. Globals change class default
>> values and there are no class defaults for dynamic properties.[*]
>>
>> There's a perfectly valid discussion to have about whether we should
>> even have dynamic properties. It's certainly been a long time since
>> they were introduced and they haven't made their way into all that
>> many devices so it's reasonable to say that perhaps we'd be better off
>> without them. I would not object to a patch series that moved
>> properties to classes entirely provided it removed existing uses of
>> dynamic properties and didn't just introduce yet another mechanism.
>>
>> But compat properties as a concept could be made to work with dynamic
>> properties. They would have to be evaluated after instance init.
>> There's quite a few places they would end up touching I suspect.
>
> Erm, sorry, that is already implemented in qemu.git!? instance_post_init
> by Eduardo plus glue by me.
Ah, even better then :-)
Regards,
Anthony Liguori
> Andreas
>
>>
>> Another point of confusion worth mention is legacy properties since
>> this usually comes up in the discussion. Legacy properties (the
>> properties that are set/get as strings) are something that we should
>> try to avoid. They end up as strings on the wire and make it harder
>> to write client code.
>>
>> * I recognize that compat_props are implemented as globals. I'm
>> really talking about the current implementation of globals, not the
>> concept of -global which could be made with dynamic properties.
>>
>> Regards,
>>
>> Anthony Liguori
>>
>>> What I don't understand is the rejection of code that works, matches the
>>> style used by 200+ other source files, adds more useful introspectable
>>> information, done in the way that was suggested 16 months ago, because
>>> we have some rough idea about how a new grand design will look like in
>>> the far future.
>>>
>>> [1] http://lists.gnu.org/archive/html/qemu-devel/2013-06/msg00990.html
>>>
>>> --
>>> Eduardo
>
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] CPU models and feature probing (was Re: [PATCH qom-cpu 00/16 v10] target-i386: convert CPU) features into properties
2014-02-11 18:57 ` Anthony Liguori
@ 2014-02-11 21:38 ` Paolo Bonzini
0 siblings, 0 replies; 53+ messages in thread
From: Paolo Bonzini @ 2014-02-11 21:38 UTC (permalink / raw)
To: Anthony Liguori, Andreas Färber
Cc: Eduardo Habkost, libvir-list, Markus Armbruster, qemu-devel,
Anthony Liguori, Igor Mammedov, Jiri Denemark, Amos Kong
Il 11/02/2014 19:57, Anthony Liguori ha scritto:
>> > Erm, sorry, that is already implemented in qemu.git!? instance_post_init
>> > by Eduardo plus glue by me.
> Ah, even better then :-)
Still, the code is a bit ad hoc. Static properties would let us remove
that code and just read dc->props arrays along the hierarchy as usual.
Also, Igor reminded me offlist that right now we cannot dump the default
values of the properties. With this series, one could extend the
device-list-properties command with that information.
Paolo
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] CPU models and feature probing (was Re: [PATCH qom-cpu 00/16 v10] target-i386: convert CPU) features into properties
2014-02-06 16:57 ` Andreas Färber
2014-02-07 10:16 ` Eduardo Habkost
@ 2014-02-07 10:37 ` Eduardo Habkost
1 sibling, 0 replies; 53+ messages in thread
From: Eduardo Habkost @ 2014-02-07 10:37 UTC (permalink / raw)
To: Andreas Färber
Cc: libvir-list, armbru, qemu-devel, Anthony Liguori, pbonzini,
Igor Mammedov, Jiri Denemark, akong
On Thu, Feb 06, 2014 at 05:57:38PM +0100, Andreas Färber wrote:
[...]
> >> If there's no relation between a CPU model named, e.g., "Haswell" and
> >> the one on an Intel Haswell chip any more, then we should give them
> >> artificial names like "qemu64"; I strongly believe that Haswell
> >> definition in code should match that of the specs/hardware and if TCG
> >> can't emulate something that's one thing (subtractive: no AVX) and if
> >> KVM wants to speed up things that's another (additive: kvmvapic,
> >> in-kernel IRQ/PIT). What I am arguing against is watering the meaning of
> >> our definitions from "this is this model" to "this is handy". In
> >> particular, if we use the post_initialize hook like I suggested then
> >> -global is still able to override it and Eduardo's properties should
> >> correctly report them to libvirt.
> >
> > "Haswell" is named this way not only because it looks like Haswell, but
> > also because it has useful features you are going to find only on
> > Haswell, and you (probably) are not going to be able to run it on hosts
> > older than Haswell. That's the main real-world application of CPU
> > models: making sure the VMs can run on a specific subset of hosts. So,
> > if you choose "Haswell", you are telling the management stack "I know
> > this is going to run only on Haswell and newer CPUs".
> >
> > That's why x2apic is being proposed as an exception: it can be enabled
> > on any host, because it doesn't depend on host-side support. That's why
> > I propose we enable it on CPU models that don't have x2apic in the real
> > world.
> >
> > (BTW, what is the relation between this subject and static properties? I
> > was expecting this to be discussed in the other existing thread about
> > x2apic)
>
> Sorry if I replied to two different series at once, that was for
> "Enabling x2apic by default in KvM (was Re: [Qemu-devel] [PATCH]
> target-i386: enable x2apic by default on more recent CPU models)".
>
> Which is connected to CPU models/subclasses in what those types are
> supposed to be good for if we define them.
>
> Let's take an obvious example. Jan wants to emulate a legacy isapc
> machine with -cpu 486. Then it feels counterproductive to enable the
> latest and greatest features such a machine never had. If the user wants
> to have the latest and greatest features, she can choose a newer -cpu
> model which already has the flag today.
OK, trying to be pragmatic:
First, my main assumption: we don't want to make CPU models look
different in KVM and TCG mode, to keep the CPU model information APIs
simple and sane.
Based on that:
* I understand you don't want to force Jan to use "-cpu 486,-x2apic"
* I don't want to force libvirt to use "-cpu Opteron_G1,+x2apic"
I don't care about not having x2apic on 486 and other older CPU models
that are fully supported by TCG, and I believe people using TCG won't
care about having x2apic on Opteron_G1 and newer in the near future.
One day TCG may be able to run Opteron_G1 too. But I also believe that
the QEMU interfaces and the libvirt code that handles CPU features will
eventually get mature enough so libvirt will be less affected by QEMU's
decision about feature flags in CPU models. When that happen, we will be
able to disable x2apic on Opteron_G1 and let libvirt enable it
explicitly.
Considering that, isn't it a reasonable compromise to add x2apic to the
CPU models that today are useful only for KVM?
--
Eduardo
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties
2013-11-27 22:28 [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties Igor Mammedov
` (16 preceding siblings ...)
2013-12-15 22:50 ` [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties Andreas Färber
@ 2014-02-11 17:17 ` Igor Mammedov
2014-03-05 16:53 ` Igor Mammedov
17 siblings, 1 reply; 53+ messages in thread
From: Igor Mammedov @ 2014-02-11 17:17 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, afaerber
On Wed, 27 Nov 2013 23:28:40 +0100
Igor Mammedov <imammedo@redhat.com> wrote:
[...]
> target-i386: cpu: convert 'level' to static property
> target-i386: cpu: convert 'xlevel' to static property
> target-i386: cpu: convert 'family' to static property
> target-i386: cpu: convert 'model' to static property
> target-i386: cpu: convert 'stepping' to static property
> target-i386: cpu: convert 'vendor' to static property
> target-i386: cpu: convert 'model-id' to static property
> target-i386: cpu: convert 'tsc-frequency' to static property
[...]
Andreas,
Taking in account that we agreed that static properties are convenient
for using with Devices and that CPU is Device now,
Could you consider applying patches [3-10/16] to your qom-cpu branch, please.
As minimum they consolidate x86 CPU properties in one properties array
and are nice codebase cleanup. Patches 3-4, replace custom setters/getters
with generic ones, replacing them with DEFINE_PROP_UINT32() one-liners.
As you can see Anthony says it's reasonable thing to do:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg215491.html
As side effect of conversion it allows to leverage currently working
"legacy" commands -device/(HMP) info qtree/(QMP)device-list-properties
for x86 CPUs which provides immediate benefits (without waiting on
rewrite of everything in QOM way).
The rest of series, I'll respin utilizing current QOM infrastructure more
and make bit->name conversion local to x86 CPU code as you've suggested.
> target-i386: set [+-]feature using static properties
> qdev: introduce qdev_prop_find_bit()
> target-i386: use static properties in check_features_against_host() to
> print CPUID feature names
> target-i386: use static properties to list CPUID features
> target-i386: remove unused *_feature_name arrays
> target-i386: cpu: fix invalid use of error_is_set(errp) if errp ==
> NULL
>
> hw/core/qdev-properties.c | 15 +
> include/hw/qdev-properties.h | 13 +
> target-i386/cpu.c | 665 ++++++++++++++++++++-----------------------
> 3 files changed, 338 insertions(+), 355 deletions(-)
>
^ permalink raw reply [flat|nested] 53+ messages in thread* Re: [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties
2014-02-11 17:17 ` [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU " Igor Mammedov
@ 2014-03-05 16:53 ` Igor Mammedov
0 siblings, 0 replies; 53+ messages in thread
From: Igor Mammedov @ 2014-03-05 16:53 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, afaerber
On Tue, 11 Feb 2014 18:17:17 +0100
Igor Mammedov <imammedo@redhat.com> wrote:
> On Wed, 27 Nov 2013 23:28:40 +0100
> Igor Mammedov <imammedo@redhat.com> wrote:
>
> [...]
> > target-i386: cpu: convert 'level' to static property
> > target-i386: cpu: convert 'xlevel' to static property
> > target-i386: cpu: convert 'family' to static property
> > target-i386: cpu: convert 'model' to static property
> > target-i386: cpu: convert 'stepping' to static property
> > target-i386: cpu: convert 'vendor' to static property
> > target-i386: cpu: convert 'model-id' to static property
> > target-i386: cpu: convert 'tsc-frequency' to static property
> [...]
> Andreas,
>
> Taking in account that we agreed that static properties are convenient
> for using with Devices and that CPU is Device now,
>
> Could you consider applying patches [3-10/16] to your qom-cpu branch, please.
>
> As minimum they consolidate x86 CPU properties in one properties array
> and are nice codebase cleanup. Patches 3-4, replace custom setters/getters
> with generic ones, replacing them with DEFINE_PROP_UINT32() one-liners.
> As you can see Anthony says it's reasonable thing to do:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg215491.html
>
> As side effect of conversion it allows to leverage currently working
> "legacy" commands -device/(HMP) info qtree/(QMP)device-list-properties
> for x86 CPUs which provides immediate benefits (without waiting on
> rewrite of everything in QOM way).
ping
>
> The rest of series, I'll respin utilizing current QOM infrastructure more
> and make bit->name conversion local to x86 CPU code as you've suggested.
> > target-i386: set [+-]feature using static properties
> > qdev: introduce qdev_prop_find_bit()
> > target-i386: use static properties in check_features_against_host() to
> > print CPUID feature names
> > target-i386: use static properties to list CPUID features
> > target-i386: remove unused *_feature_name arrays
> > target-i386: cpu: fix invalid use of error_is_set(errp) if errp ==
> > NULL
> >
> > hw/core/qdev-properties.c | 15 +
> > include/hw/qdev-properties.h | 13 +
> > target-i386/cpu.c | 665 ++++++++++++++++++++-----------------------
> > 3 files changed, 338 insertions(+), 355 deletions(-)
> >
>
^ permalink raw reply [flat|nested] 53+ messages in thread