On Mon, Feb 18, 2013 at 09:36:28PM +0100, Andreas Färber wrote:
> Am 18.02.2013 21:30, schrieb Eduardo Habkost:
> > On Mon, Feb 11, 2013 at 05:35:03PM +0100, Igor Mammedov wrote:
> >> Following properties are converted:
> >> * vendor
> >> * xlevel
> >> * custom setter/getter replaced by qdev's DEFINE_PROP_UINT32
> >> * level
> >> * custom setter/getter replaced by qdev's DEFINE_PROP_UINT32
> >> * tsc-frequency
> >> * stepping
> >> * model
> >> * family
> >> * model-id
> >> * 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).
> >> * use g_malloc0() instead of g_malloc() in x86_cpuid_get_model_id()
> >>
> >> Common changes to all properties:
> >> * properties code are moved to the top of file, before properties array
> >> definition
> >> * s/error_set/error_setg/;s/QERR*/with similar message/
> >
> > Why?
>
> Been having a similar question (sorry for not replying yet):
>
> Can't we leave my new-style getters and setters in place and invoke them
> through some glue code from whatever old-style machinery qdev static
> properties still use?
I don't understand your question. What do you mean by "my new-style
getters and setters"?
Igor's patch keeps the existing getters/setters almost the same (I
manually run "diff" to check that, see the end of this message), except
for the error_set() calls, the small differences I pointed out in my
message, and extra visit_type_str() calls required for the string
getter/setters (that have a different signature from
object_property_add_str() getter/setters).
>
> BTW I'm not yet clear on how we should proceed with subclasses and KVM.
> Proposing we proceed with your properties refactoring after all, then
> maybe it becomes more clear where the problems are.
I believe Igor will send a new version soon. I completely agree with
having KVM-specific and TCG-specific classes, because they _are_
different CPU models. The same reasons we have[1] to make "-cpu
SandyBridge" and "-cpu Haswell" different classes, apply to making
"-disable-kvm -cpu SandyBridge" and "-enable-kvm -cpu SandyBridge"
different classes as well.
About the default for the "vendor" property: right now it is not an
issue because Igor's patches are not setting any externally-visible
default value for the "vendor" property (so this is just an
implementation detail), and now I am happy with either of the solutions
we have been discussing (as my questions about QOM design have been
answered). In either case, when we actually get to set a introspectable
default for the property, I hope to have heard additional feedback from
other people (especially from the libvirt folks).
[1] The reasons I remember are:
1) Allowing CPU model definition probing, by simply querying the default
values of properties for each class;
* Rationale for separating TCG/KVM: the resulting CPU features are
different when running KVM and when running TCG, so they are in
practice different CPU models. For example: differences between
"-enable-kvm -cpu SandyBridge" and "-disable-kvm -cpu SandyBridge"
are huge when compared to the small differences between
"-enable-kvm -cpu SandyBridge" and "-enable-kvm -cpu Haswell".
2) Allowing machine-type compatibility to be specified easily using
global properties on the machine-type compat_props field.
* Rationale for separating TCG/KVM: we may want to set a
compatibility property only for TCG or only for KVM. For example,
the n270 MOVBE fix we are going to include QEMU in 1.5 will need to
disable MOVBE on pc-1.4, but only on the TCG CPU model, because KVM
already had MOVBE working on pc-1.4.
--
Eduardo
Diff between the getters and setters before/after this patch:
--- /tmp/oldsetters 2013-02-18 17:12:53.781477861 -0300
+++ /tmp/newsetters 2013-02-18 17:12:59.069477982 -0300
@@ -26,8 +26,9 @@
return;
}
if (value < min || value > max) {
- error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "",
- name ? name : "null", value, min, max);
+ error_setg(errp, "Property %s.%s doesn't take value %" PRId64 " (min"
+ "imum: %" PRId64 ", maximum: %" PRId64,
+ object_get_typename(obj), name, value, min, max);
return;
}
@@ -39,6 +40,14 @@
}
}
+PropertyInfo qdev_prop_family = {
+ .name = "uint32",
+ .get = x86_cpuid_version_get_family,
+ .set = x86_cpuid_version_set_family,
+};
+#define DEFINE_PROP_FAMILY(_n)
\
+ DEFINE_PROP(_n, X86CPU, env.cpuid_version, qdev_prop_family, uint32_t)
+
static void x86_cpuid_version_get_model(Object *obj, Visitor *v, void *opaque,
const char *name, Error **errp)
{
@@ -65,8 +74,9 @@
return;
}
if (value < min || value > max) {
- error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "",
- name ? name : "null", value, min, max);
+ error_setg(errp, "Property %s.%s doesn't take value %" PRId64 " (min"
+ "imum: %" PRId64 ", maximum: %" PRId64,
+ object_get_typename(obj), name, value, min, max);
return;
}
@@ -74,6 +84,14 @@
env->cpuid_version |= ((value & 0xf) << 4) | ((value >> 4) << 16);
}
+PropertyInfo qdev_prop_model = {
+ .name = "uint32",
+ .get = x86_cpuid_version_get_model,
+ .set = x86_cpuid_version_set_model,
+};
+#define DEFINE_PROP_MODEL(_n)
\
+ DEFINE_PROP(_n, X86CPU, env.cpuid_version, qdev_prop_model, uint32_t)
+
static void x86_cpuid_version_get_stepping(Object *obj, Visitor *v,
void *opaque, const char *name,
Error **errp)
@@ -101,8 +119,9 @@
return;
}
if (value < min || value > max) {
- error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "",
- name ? name : "null", value, min, max);
+ error_setg(errp, "Property %s.%s doesn't take value %" PRId64 " (min"
+ "imum: %" PRId64 ", maximum: %" PRId64,
+ object_get_typename(obj), name, value, min, max);
return;
}
@@ -110,39 +129,16 @@
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);
-}
+PropertyInfo qdev_prop_stepping = {
+ .name = "uint32",
+ .get = x86_cpuid_version_get_stepping,
+ .set = x86_cpuid_version_set_stepping,
+};
+#define DEFINE_PROP_STEPPING(_n)
\
+ DEFINE_PROP(_n, X86CPU, env.cpuid_version, qdev_prop_stepping, uint32_t)
-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)
-{
- 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)
+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;
@@ -151,19 +147,27 @@
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;
+ char *value;
int i;
+ visit_type_str(v, &value, name, errp);
+ if (error_is_set(errp)) {
+ return;
+ }
+
if (strlen(value) != CPUID_VENDOR_SZ) {
- error_set(errp, QERR_PROPERTY_VALUE_BAD, "",
- "vendor", value);
+ error_setg(errp, "Property '%s.%s' doesn't take value '%s'",
+ object_get_typename(obj), name, value);
+ g_free(value);
return;
}
@@ -171,47 +175,73 @@
env->cpuid_vendor2 = 0;
env->cpuid_vendor3 = 0;
for (i = 0; i < 4; i++) {
- env->cpuid_vendor1 |= ((uint8_t)value[i ]) << (8 * i);
+ env->cpuid_vendor1 |= ((uint8_t)value[i]) << (8 * i);
env->cpuid_vendor2 |= ((uint8_t)value[i + 4]) << (8 * i);
env->cpuid_vendor3 |= ((uint8_t)value[i + 8]) << (8 * i);
}
+ g_free(value);
+}
+
+PropertyInfo qdev_prop_vendor = {
+ .name = "string",
+ .get = x86_cpuid_get_vendor,
+ .set = x86_cpuid_set_vendor,
+};
+#define DEFINE_PROP_VENDOR(_n) {
\
+ .name = _n,
\
+ .info = &qdev_prop_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;
char *value;
int i;
- value = g_malloc(48 + 1);
+ value = g_malloc0(48 + 1);
for (i = 0; i < 48; i++) {
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;
int c, len, i;
+ char *value;
- if (model_id == NULL) {
- model_id = "";
+ visit_type_str(v, &value, name, errp);
+ if (error_is_set(errp)) {
+ return;
}
- len = strlen(model_id);
+
+ len = strlen(value);
memset(env->cpuid_model, 0, 48);
for (i = 0; i < 48; i++) {
if (i >= len) {
c = '\0';
} else {
- c = (uint8_t)model_id[i];
+ c = (uint8_t)value[i];
}
env->cpuid_model[i >> 2] |= c << (8 * (i & 3));
}
+ g_free(value);
+}
+
+PropertyInfo qdev_prop_model_id = {
+ .name = "string",
+ .get = x86_cpuid_get_model_id,
+ .set = x86_cpuid_set_model_id,
+};
+#define DEFINE_PROP_MODEL_ID(_n) {
\
+ .name = _n,
\
+ .info = &qdev_prop_model_id
\
}
static void x86_cpuid_get_tsc_freq(Object *obj, Visitor *v, void *opaque,
@@ -237,11 +267,20 @@
return;
}
if (value < min || value > max) {
- error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "",
- name ? name : "null", value, min, max);
+ error_setg(errp, "Property %s.%s doesn't take value %" PRId64 " (min"
+ "imum: %" PRId64 ", maximum: %" PRId64,
+ object_get_typename(obj), name, value, min, max);
return;
}
cpu->env.tsc_khz = value / 1000;
}
+PropertyInfo qdev_prop_tsc_freq = {
+ .name = "int64",
+ .get = x86_cpuid_get_tsc_freq,
+ .set = x86_cpuid_set_tsc_freq,
+};
+#define DEFINE_PROP_TSC_FREQ(_n)
\
+ DEFINE_PROP(_n, X86CPU, env.tsc_khz, qdev_prop_tsc_freq, int32_t)
+
