Re: [Qemu-devel] [PATCH v4 27/33] target-i386: Register X86CPU feat-kvmclock feature
On Thu, Aug 14, 2014 at 08:59:17PM -0300, Eduardo Habkost wrote:
On Thu, Aug 14, 2014 at 11:08:30PM +0200, Michael S. Tsirkin wrote:
On Thu, Aug 14, 2014 at 04:25:56PM -0300, Eduardo Habkost wrote:
The kvmclock feature is special because it affects two bits in the KVM
CPUID leaf, so it has to be handled differently from the other feature
properties that will be added.
Signed-off-by: Eduardo Habkost ehabk...@redhat.com
---
target-i386/cpu.c | 61
+++
1 file changed, 61 insertions(+)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b005b0d..0eb401b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2774,6 +2774,61 @@ uint32_t x86_cpu_apic_id_from_index(unsigned int
cpu_index)
}
}
+typedef struct FeatureProperty {
+FeatureWord word;
+uint32_t mask;
+} FeatureProperty;
+
+
+static void x86_cpu_get_feature_prop(Object *obj,
+ struct Visitor *v,
+ void *opaque,
+ const char *name,
+ Error **errp)
+{
+X86CPU *cpu = X86_CPU(obj);
+CPUX86State *env = cpu-env;
+FeatureProperty *fp = opaque;
+bool value = (env-features[fp-word] fp-mask) == fp-mask;
+visit_type_bool(v, value, name, errp);
+}
+
+static void x86_cpu_set_feature_prop(Object *obj,
+ struct Visitor *v,
+ void *opaque,
+ const char *name,
+ Error **errp)
+{
+X86CPU *cpu = X86_CPU(obj);
+CPUX86State *env = cpu-env;
+FeatureProperty *fp = opaque;
+bool value;
+visit_type_bool(v, value, name, errp);
+if (value) {
+env-features[fp-word] |= fp-mask;
+} else {
+env-features[fp-word] = ~fp-mask;
+}
+}
+
+/* Register a boolean feature-bits property.
+ * If mask has multiple bits, all must be set for the property to return
true.
+ */
+static void x86_cpu_register_feature_prop(X86CPU *cpu,
+ const char *prop_name,
+ FeatureWord w,
+ uint32_t mask)
+{
+FeatureProperty *fp;
+fp = g_new0(FeatureProperty, 1);
+fp-word = w;
+fp-mask = mask;
+object_property_add(OBJECT(cpu), prop_name, bool,
+x86_cpu_set_feature_prop,
+x86_cpu_get_feature_prop,
+NULL, fp, error_abort);
+}
+
This looks similar to what what DEFINE_PROP_BIT does.
Can't this be reused in some way?
DEFINE_PROP_BIT is from the static property system, and I understand we
are preferring using object_property_add*() instead (and in the X86CPU
features case, registering the properties dynamically using the feature
name arrays saves us a lot of work).
I will take a look at the DEFINE_PROP_BIT code to see if anything from
that code can be reused, but I doubt so. It seems to be tightly coupled
to the static property system.
Main point is, can't we find a way to reduce code duplication?
It doesn't seem reasonable that we basically open-code
each property from scratch.
static void x86_cpu_initfn(Object *obj)
{
CPUState *cs = CPU(obj);
@@ -2819,6 +2874,12 @@ static void x86_cpu_initfn(Object *obj)
x86_cpu_get_feature_words,
NULL, NULL, (void *)cpu-filtered_features,
NULL);
+/* feat-kvmclock will affect both kvmclock feature bits */
+x86_cpu_register_feature_prop(cpu, feat-kvmclock, FEAT_KVM,
+ (1UL KVM_FEATURE_CLOCKSOURCE) |
+ (1UL KVM_FEATURE_CLOCKSOURCE2));
+
+
cpu-hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY;
env-cpuid_apic_id = x86_cpu_apic_id_from_index(cs-cpu_index);
--
1.9.3
--
Eduardo
Re: [Qemu-devel] [PATCH v4 27/33] target-i386: Register X86CPU feat-kvmclock feature
On Thu, Aug 14, 2014 at 04:25:56PM -0300, Eduardo Habkost wrote:
The kvmclock feature is special because it affects two bits in the KVM
CPUID leaf, so it has to be handled differently from the other feature
properties that will be added.
Signed-off-by: Eduardo Habkost ehabk...@redhat.com
---
target-i386/cpu.c | 61
+++
1 file changed, 61 insertions(+)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b005b0d..0eb401b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2774,6 +2774,61 @@ uint32_t x86_cpu_apic_id_from_index(unsigned int
cpu_index)
}
}
+typedef struct FeatureProperty {
+FeatureWord word;
+uint32_t mask;
+} FeatureProperty;
+
+
+static void x86_cpu_get_feature_prop(Object *obj,
+ struct Visitor *v,
+ void *opaque,
+ const char *name,
+ Error **errp)
+{
+X86CPU *cpu = X86_CPU(obj);
+CPUX86State *env = cpu-env;
+FeatureProperty *fp = opaque;
+bool value = (env-features[fp-word] fp-mask) == fp-mask;
+visit_type_bool(v, value, name, errp);
+}
+
+static void x86_cpu_set_feature_prop(Object *obj,
+ struct Visitor *v,
+ void *opaque,
+ const char *name,
+ Error **errp)
+{
+X86CPU *cpu = X86_CPU(obj);
+CPUX86State *env = cpu-env;
+FeatureProperty *fp = opaque;
+bool value;
+visit_type_bool(v, value, name, errp);
+if (value) {
+env-features[fp-word] |= fp-mask;
+} else {
+env-features[fp-word] = ~fp-mask;
+}
+}
+
+/* Register a boolean feature-bits property.
+ * If mask has multiple bits, all must be set for the property to return
true.
+ */
+static void x86_cpu_register_feature_prop(X86CPU *cpu,
+ const char *prop_name,
+ FeatureWord w,
+ uint32_t mask)
+{
+FeatureProperty *fp;
+fp = g_new0(FeatureProperty, 1);
+fp-word = w;
+fp-mask = mask;
+object_property_add(OBJECT(cpu), prop_name, bool,
+x86_cpu_set_feature_prop,
+x86_cpu_get_feature_prop,
+NULL, fp, error_abort);
+}
+
This looks similar to what what DEFINE_PROP_BIT does.
Can't this be reused in some way?
static void x86_cpu_initfn(Object *obj)
{
CPUState *cs = CPU(obj);
@@ -2819,6 +2874,12 @@ static void x86_cpu_initfn(Object *obj)
x86_cpu_get_feature_words,
NULL, NULL, (void *)cpu-filtered_features, NULL);
+/* feat-kvmclock will affect both kvmclock feature bits */
+x86_cpu_register_feature_prop(cpu, feat-kvmclock, FEAT_KVM,
+ (1UL KVM_FEATURE_CLOCKSOURCE) |
+ (1UL KVM_FEATURE_CLOCKSOURCE2));
+
+
cpu-hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY;
env-cpuid_apic_id = x86_cpu_apic_id_from_index(cs-cpu_index);
--
1.9.3
Re: [Qemu-devel] [PATCH v4 27/33] target-i386: Register X86CPU feat-kvmclock feature
On Thu, Aug 14, 2014 at 11:08:30PM +0200, Michael S. Tsirkin wrote:
On Thu, Aug 14, 2014 at 04:25:56PM -0300, Eduardo Habkost wrote:
The kvmclock feature is special because it affects two bits in the KVM
CPUID leaf, so it has to be handled differently from the other feature
properties that will be added.
Signed-off-by: Eduardo Habkost ehabk...@redhat.com
---
target-i386/cpu.c | 61
+++
1 file changed, 61 insertions(+)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b005b0d..0eb401b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2774,6 +2774,61 @@ uint32_t x86_cpu_apic_id_from_index(unsigned int
cpu_index)
}
}
+typedef struct FeatureProperty {
+FeatureWord word;
+uint32_t mask;
+} FeatureProperty;
+
+
+static void x86_cpu_get_feature_prop(Object *obj,
+ struct Visitor *v,
+ void *opaque,
+ const char *name,
+ Error **errp)
+{
+X86CPU *cpu = X86_CPU(obj);
+CPUX86State *env = cpu-env;
+FeatureProperty *fp = opaque;
+bool value = (env-features[fp-word] fp-mask) == fp-mask;
+visit_type_bool(v, value, name, errp);
+}
+
+static void x86_cpu_set_feature_prop(Object *obj,
+ struct Visitor *v,
+ void *opaque,
+ const char *name,
+ Error **errp)
+{
+X86CPU *cpu = X86_CPU(obj);
+CPUX86State *env = cpu-env;
+FeatureProperty *fp = opaque;
+bool value;
+visit_type_bool(v, value, name, errp);
+if (value) {
+env-features[fp-word] |= fp-mask;
+} else {
+env-features[fp-word] = ~fp-mask;
+}
+}
+
+/* Register a boolean feature-bits property.
+ * If mask has multiple bits, all must be set for the property to return
true.
+ */
+static void x86_cpu_register_feature_prop(X86CPU *cpu,
+ const char *prop_name,
+ FeatureWord w,
+ uint32_t mask)
+{
+FeatureProperty *fp;
+fp = g_new0(FeatureProperty, 1);
+fp-word = w;
+fp-mask = mask;
+object_property_add(OBJECT(cpu), prop_name, bool,
+x86_cpu_set_feature_prop,
+x86_cpu_get_feature_prop,
+NULL, fp, error_abort);
+}
+
This looks similar to what what DEFINE_PROP_BIT does.
Can't this be reused in some way?
DEFINE_PROP_BIT is from the static property system, and I understand we
are preferring using object_property_add*() instead (and in the X86CPU
features case, registering the properties dynamically using the feature
name arrays saves us a lot of work).
I will take a look at the DEFINE_PROP_BIT code to see if anything from
that code can be reused, but I doubt so. It seems to be tightly coupled
to the static property system.
static void x86_cpu_initfn(Object *obj)
{
CPUState *cs = CPU(obj);
@@ -2819,6 +2874,12 @@ static void x86_cpu_initfn(Object *obj)
x86_cpu_get_feature_words,
NULL, NULL, (void *)cpu-filtered_features, NULL);
+/* feat-kvmclock will affect both kvmclock feature bits */
+x86_cpu_register_feature_prop(cpu, feat-kvmclock, FEAT_KVM,
+ (1UL KVM_FEATURE_CLOCKSOURCE) |
+ (1UL KVM_FEATURE_CLOCKSOURCE2));
+
+
cpu-hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY;
env-cpuid_apic_id = x86_cpu_apic_id_from_index(cs-cpu_index);
--
1.9.3
--
Eduardo
