Re: [Qemu-devel] [PATCH qom-next 08/12] target-i386: introduce cpu-model property for x86_cpu
On 05/30/2012 05:22 PM, Andreas Färber wrote: Am 30.05.2012 00:10, schrieb Igor Mammedov: it's probably intermidiate step till cpu modeled as sub-classes. After then we probably could drop it. However it still could be used for overiding default cpu subclasses definition, and probably renamed to something like 'features'. v2: - remove accidential tcg_* init code move Signed-off-by: Igor Mammedovimamm...@redhat.com --- cpu-defs.h |2 +- hw/pc.c | 10 -- target-i386/cpu.c| 24 target-i386/helper.c | 17 - 4 files changed, 37 insertions(+), 16 deletions(-) For me this is still the big no-go in this series: * Moving the default cpu_model into cpu_x86_init() buys us nothing IMO, it duplicates the property setting code and differs from all other targets where the default CPU is the machine's decision. Agreed. * As you rightly point out, we are heading towards sub-classes and that contradicts this two-step initialization. I don't see how this is an intermediate step? It's not clear to me how sub-classes contradict with two-step initialization, , could you elaborate more on this? About cpu_model property being an intermediate step, I think it is an easy and safe way to hide/isolate cpu implementation details in cpu.c and use only QOM interface to create cpus. Later with an introduction of sub-classes it could be reduced to feature override functionality and when cpu features are converted to proper properties then cpu-model property could be dropped altogether. There are 2 problems I am solving with cpu-model property: 1 - APIC should be created before cpu becomes run-able i.e. before x86_cpu_realize(). Now it's not so but cpu_reset() that is called after APIC is created kind of 'fixes' issue. Moving APIC [11/12] creation into property forces us to create it at the proper time. 2 - I'm not sure yet how to deal with APIC creation with sub-classes introduction. Should it be created in an each sub-class initfn /doubts: code duplication/ or in parent class /doubts: lack of APICless 486 cpu model/? I could move APIC from cpu-model property in initfn right now and create it there and in cpu-model property destroy APIC if cpu_def doesn't have APIC feature /only 486cpu, may we ignore it?/ or if feature is asked to be disabled via feature flag in cpu-model string then just disable it as it's done for the rest of supported cpus in real hw. I admit, I am the one to blame for not redoing the x86 CPU subclasses patches yet - major issue being the built-in vs. -cpudef split: Now that Eduardo refactored the config file reading, I wonder if we can outsource the built-in CPUs to the config file? If so, then I would appreciate one of you x86 experts to do that please (may need some rebasing when the occasional features get added/tweaked). Then I can base a series on top that initializes CPU subclasses in a consistent way rather than duplicating data for the builtins just to make -cpudef work or creating different intermediate subclasses for both types. I'll ask Eduardo how I can help here. * To override CPU features you should think about how to set x86 CPU features in a QOM way (think QMP), then we can design the infrastructure for setting them through global properties (or whatever needed) around it. I honestly don't know what the requirements are in practice, so I can't make suggestions there without some more feedback. Regards, Andreas -- - Igor
Re: [Qemu-devel] [PATCH qom-next 08/12] target-i386: introduce cpu-model property for x86_cpu
Am 04.06.2012 16:56, schrieb Igor Mammedov: On 05/30/2012 05:22 PM, Andreas Färber wrote: Am 30.05.2012 00:10, schrieb Igor Mammedov: it's probably intermidiate step till cpu modeled as sub-classes. After then we probably could drop it. However it still could be used for overiding default cpu subclasses definition, and probably renamed to something like 'features'. * As you rightly point out, we are heading towards sub-classes and that contradicts this two-step initialization. I don't see how this is an intermediate step? It's not clear to me how sub-classes contradict with two-step initialization, , could you elaborate more on this? CPU subclasses mean to me that for -cpu qemu64 we would have a QOM type qemu64 (or so). initfn would then take care of initializing all default values, and from cpu_x86_init() we would parse the remaining cpu_model parameters and set QOM properties on the CPU instance. Original attempt: https://github.com/afaerber/qemu-cpu/commit/a27feda42712606ca2303faeb6c7e8478660a1c1 Now, the contradiction is that once we have done object_new(qemu64) we cannot change its type qemu64 to anything else. Therefore I dislike sticking cpu_model into a cpu-model property. What I was talking about wrt features was doing in pseudocode: object_new(qemu64) object_property_set_int(family, 42) object_property_set_string(vendor, Me, myself and I) object_property_set_bool(x2apic, true) ... I.e. decoupling the back part of the cpu_model string from the model. My patches in master that you and others have reviewed did this for the mostly numeric CPUID parts (-cpu foo,x=42), with a view to code sharing. What's missing is properties to set CPU features (-cpu foo,+x,-y). There the question is how granular do we want to go and which types do we want to use. The example above shows using a bool property for a specific feature (without having checked that for correctness). Other possibilities would be to have a feature string with all those space-separated acronyms or an int that is a bitfield. One doesn't rule out the other. Jan's requirement, I think, was to be able to set them from global properties for pc-1.x backwards compatibility. Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH qom-next 08/12] target-i386: introduce cpu-model property for x86_cpu
Am 30.05.2012 00:10, schrieb Igor Mammedov: it's probably intermidiate step till cpu modeled as sub-classes. After then we probably could drop it. However it still could be used for overiding default cpu subclasses definition, and probably renamed to something like 'features'. v2: - remove accidential tcg_* init code move Signed-off-by: Igor Mammedov imamm...@redhat.com --- cpu-defs.h |2 +- hw/pc.c | 10 -- target-i386/cpu.c| 24 target-i386/helper.c | 17 - 4 files changed, 37 insertions(+), 16 deletions(-) For me this is still the big no-go in this series: * Moving the default cpu_model into cpu_x86_init() buys us nothing IMO, it duplicates the property setting code and differs from all other targets where the default CPU is the machine's decision. * As you rightly point out, we are heading towards sub-classes and that contradicts this two-step initialization. I don't see how this is an intermediate step? I admit, I am the one to blame for not redoing the x86 CPU subclasses patches yet - major issue being the built-in vs. -cpudef split: Now that Eduardo refactored the config file reading, I wonder if we can outsource the built-in CPUs to the config file? If so, then I would appreciate one of you x86 experts to do that please (may need some rebasing when the occasional features get added/tweaked). Then I can base a series on top that initializes CPU subclasses in a consistent way rather than duplicating data for the builtins just to make -cpudef work or creating different intermediate subclasses for both types. * To override CPU features you should think about how to set x86 CPU features in a QOM way (think QMP), then we can design the infrastructure for setting them through global properties (or whatever needed) around it. I honestly don't know what the requirements are in practice, so I can't make suggestions there without some more feedback. 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
[Qemu-devel] [PATCH qom-next 08/12] target-i386: introduce cpu-model property for x86_cpu
it's probably intermidiate step till cpu modeled as
sub-classes. After then we probably could drop it.
However it still could be used for overiding default
cpu subclasses definition, and probably renamed to
something like 'features'.
v2:
- remove accidential tcg_* init code move
Signed-off-by: Igor Mammedov imamm...@redhat.com
---
cpu-defs.h |2 +-
hw/pc.c | 10 --
target-i386/cpu.c| 24
target-i386/helper.c | 17 -
4 files changed, 37 insertions(+), 16 deletions(-)
diff --git a/cpu-defs.h b/cpu-defs.h
index f49e950..8f4623c 100644
--- a/cpu-defs.h
+++ b/cpu-defs.h
@@ -221,7 +221,7 @@ typedef struct CPUWatchpoint {
struct QemuCond *halt_cond; \
int thread_kicked; \
struct qemu_work_item *queued_work_first, *queued_work_last;\
-const char *cpu_model_str; \
+char *cpu_model_str;\
struct KVMState *kvm_state; \
struct kvm_run *kvm_run;\
int kvm_fd; \
diff --git a/hw/pc.c b/hw/pc.c
index 2f681db..4a687d6 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -948,7 +948,6 @@ static X86CPU *pc_new_cpu(const char *cpu_model)
cpu = cpu_x86_init(cpu_model);
if (cpu == NULL) {
-fprintf(stderr, Unable to find x86 CPU definition\n);
exit(1);
}
env = cpu-env;
@@ -974,15 +973,6 @@ void pc_cpus_init(const char *cpu_model)
{
int i;
-/* init CPUs */
-if (cpu_model == NULL) {
-#ifdef TARGET_X86_64
-cpu_model = qemu64;
-#else
-cpu_model = qemu32;
-#endif
-}
-
for(i = 0; i smp_cpus; i++) {
pc_new_cpu(cpu_model);
}
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index f029f2a..2610d96 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1731,6 +1731,27 @@ static void mce_init(X86CPU *cpu)
}
}
+static char *x86_get_cpu_model(Object *obj, Error **errp)
+{
+X86CPU *cpu = X86_CPU(obj);
+CPUX86State *env = cpu-env;
+return g_strdup(env-cpu_model_str);
+}
+
+static void x86_set_cpu_model(Object *obj, const char *value, Error **errp)
+{
+X86CPU *cpu = X86_CPU(obj);
+CPUX86State *env = cpu-env;
+
+g_free((gpointer)env-cpu_model_str);
+env-cpu_model_str = g_strdup(value);
+
+if (cpu_x86_register(cpu, env-cpu_model_str) 0) {
+fprintf(stderr, Unable to find x86 CPU definition\n);
+error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
+}
+}
+
void x86_cpu_realize(Object *obj, Error **errp)
{
X86CPU *cpu = X86_CPU(obj);
@@ -1772,6 +1793,9 @@ static void x86_cpu_initfn(Object *obj)
x86_cpuid_get_tsc_freq,
x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
+object_property_add_str(obj, cpu-model,
+x86_get_cpu_model, x86_set_cpu_model, NULL);
+
env-cpuid_apic_id = env-cpu_index;
/* init various static tables used in TCG mode */
diff --git a/target-i386/helper.c b/target-i386/helper.c
index fd20fd4..748eee8 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1152,14 +1152,21 @@ int cpu_x86_get_descr_debug(CPUX86State *env, unsigned
int selector,
X86CPU *cpu_x86_init(const char *cpu_model)
{
X86CPU *cpu;
-CPUX86State *env;
+Error *errp = NULL;
cpu = X86_CPU(object_new(TYPE_X86_CPU));
-env = cpu-env;
-env-cpu_model_str = cpu_model;
-if (cpu_x86_register(cpu, cpu_model) 0) {
-object_delete(OBJECT(cpu));
+if (cpu_model) {
+object_property_set_str(OBJECT(cpu), cpu_model, cpu-model, errp);
+} else {
+#ifdef TARGET_X86_64
+object_property_set_str(OBJECT(cpu), qemu64, cpu-model, errp);
+#else
+object_property_set_str(OBJECT(cpu), qemu32, cpu-model, errp);
+#endif
+}
+if (errp) {
+ object_delete(OBJECT(cpu));
return NULL;
}
--
1.7.7.6
