| Message ID | 146741290890.948.9125710372347515263.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
On Sat, Jul 02, 2016 at 12:41:48AM +0200, Greg Kurz wrote: > If we want to generate cpu_dt_id in the machine code, this must occur > before the cpu gets realized. We must open code the cpu creation to be > able to do this. > > This patch just does that. It borrows some lines from previous work > from Bharata to handle the feature parsing. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/ppc/ppc.c | 39 ++++++++++++++++++++++++++++++++++++++- > 1 file changed, 38 insertions(+), 1 deletion(-) > > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c > index dc3d214009c5..57f4ddd073d0 100644 > --- a/hw/ppc/ppc.c > +++ b/hw/ppc/ppc.c > @@ -32,6 +32,7 @@ > #include "sysemu/cpus.h" > #include "hw/timer/m48t59.h" > #include "qemu/log.h" > +#include "qapi/error.h" > #include "qemu/error-report.h" > #include "hw/loader.h" > #include "sysemu/kvm.h" > @@ -1353,5 +1354,41 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id) > > PowerPCCPU *ppc_cpu_init(const char *cpu_model) > { > - return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, cpu_model)); > + PowerPCCPU *cpu; > + CPUClass *cc; > + ObjectClass *oc; > + gchar **model_pieces; > + Error *err = NULL; > + > + model_pieces = g_strsplit(cpu_model, ",", 2); > + if (!model_pieces[0]) { > + error_report("Invalid/empty CPU model name"); > + return NULL; > + } > + > + oc = cpu_class_by_name(TYPE_POWERPC_CPU, model_pieces[0]); > + if (oc == NULL) { > + error_report("Unable to find CPU definition: %s", model_pieces[0]); > + return NULL; > + } > + > + cpu = POWERPC_CPU(object_new(object_class_get_name(oc))); > + > + cc = CPU_CLASS(oc); > + cc->parse_features(CPU(cpu), model_pieces[1], &err); Igor is working on a patchset to convert -cpu features into global properties. IIUC, after that patchset, it is not recommended to parse the -cpu features for every CPU but do it only once. That is what I attempted here in the context of supporting compat cpu type for pseries-2.7: https://www.mail-archive.com/qemu-devel@nongnu.org/msg381660.html Regards, Bharata.
On Sat, 2 Jul 2016 13:36:22 +0530 Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > On Sat, Jul 02, 2016 at 12:41:48AM +0200, Greg Kurz wrote: > > If we want to generate cpu_dt_id in the machine code, this must occur > > before the cpu gets realized. We must open code the cpu creation to be > > able to do this. > > > > This patch just does that. It borrows some lines from previous work > > from Bharata to handle the feature parsing. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > hw/ppc/ppc.c | 39 ++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 38 insertions(+), 1 deletion(-) > > > > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c > > index dc3d214009c5..57f4ddd073d0 100644 > > --- a/hw/ppc/ppc.c > > +++ b/hw/ppc/ppc.c > > @@ -32,6 +32,7 @@ > > #include "sysemu/cpus.h" > > #include "hw/timer/m48t59.h" > > #include "qemu/log.h" > > +#include "qapi/error.h" > > #include "qemu/error-report.h" > > #include "hw/loader.h" > > #include "sysemu/kvm.h" > > @@ -1353,5 +1354,41 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id) > > > > PowerPCCPU *ppc_cpu_init(const char *cpu_model) > > { > > - return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, cpu_model)); > > + PowerPCCPU *cpu; > > + CPUClass *cc; > > + ObjectClass *oc; > > + gchar **model_pieces; > > + Error *err = NULL; > > + > > + model_pieces = g_strsplit(cpu_model, ",", 2); > > + if (!model_pieces[0]) { > > + error_report("Invalid/empty CPU model name"); > > + return NULL; > > + } > > + > > + oc = cpu_class_by_name(TYPE_POWERPC_CPU, model_pieces[0]); > > + if (oc == NULL) { > > + error_report("Unable to find CPU definition: %s", model_pieces[0]); > > + return NULL; > > + } > > + > > + cpu = POWERPC_CPU(object_new(object_class_get_name(oc))); > > + > > + cc = CPU_CLASS(oc); > > + cc->parse_features(CPU(cpu), model_pieces[1], &err); > > Igor is working on a patchset to convert -cpu features into global properties. > IIUC, after that patchset, it is not recommended to parse the -cpu features > for every CPU but do it only once. > cpu_generic_init() in the current code also does the parsing, and as the title says, this patch is just about open coding the creation. I don't want to change behavior yet. But yes, I agree that we should only parse features once and I'll be more than happy to fix this in a followup patch, based on Igor's work. In the meantime, maybe I can add a comment stating that the parsing should go away ? > That is what I attempted here in the context of supporting compat cpu type > for pseries-2.7: > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg381660.html > Yeah and this is where I borrowed some lines. :) > Regards, > Bharata. > Cheers. -- Greg
On Sat, Jul 02, 2016 at 10:33:33AM +0200, Greg Kurz wrote: > On Sat, 2 Jul 2016 13:36:22 +0530 > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > > > On Sat, Jul 02, 2016 at 12:41:48AM +0200, Greg Kurz wrote: > > > If we want to generate cpu_dt_id in the machine code, this must occur > > > before the cpu gets realized. We must open code the cpu creation to be > > > able to do this. > > > > > > This patch just does that. It borrows some lines from previous work > > > from Bharata to handle the feature parsing. > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > --- > > > hw/ppc/ppc.c | 39 ++++++++++++++++++++++++++++++++++++++- > > > 1 file changed, 38 insertions(+), 1 deletion(-) > > > > > > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c > > > index dc3d214009c5..57f4ddd073d0 100644 > > > --- a/hw/ppc/ppc.c > > > +++ b/hw/ppc/ppc.c > > > @@ -32,6 +32,7 @@ > > > #include "sysemu/cpus.h" > > > #include "hw/timer/m48t59.h" > > > #include "qemu/log.h" > > > +#include "qapi/error.h" > > > #include "qemu/error-report.h" > > > #include "hw/loader.h" > > > #include "sysemu/kvm.h" > > > @@ -1353,5 +1354,41 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id) > > > > > > PowerPCCPU *ppc_cpu_init(const char *cpu_model) > > > { > > > - return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, cpu_model)); > > > + PowerPCCPU *cpu; > > > + CPUClass *cc; > > > + ObjectClass *oc; > > > + gchar **model_pieces; > > > + Error *err = NULL; > > > + > > > + model_pieces = g_strsplit(cpu_model, ",", 2); > > > + if (!model_pieces[0]) { > > > + error_report("Invalid/empty CPU model name"); > > > + return NULL; > > > + } > > > + > > > + oc = cpu_class_by_name(TYPE_POWERPC_CPU, model_pieces[0]); > > > + if (oc == NULL) { > > > + error_report("Unable to find CPU definition: %s", model_pieces[0]); > > > + return NULL; > > > + } > > > + > > > + cpu = POWERPC_CPU(object_new(object_class_get_name(oc))); > > > + > > > + cc = CPU_CLASS(oc); > > > + cc->parse_features(CPU(cpu), model_pieces[1], &err); > > > > Igor is working on a patchset to convert -cpu features into global properties. > > IIUC, after that patchset, it is not recommended to parse the -cpu features > > for every CPU but do it only once. > > > > cpu_generic_init() in the current code also does the parsing, and as the title > says, this patch is just about open coding the creation. I don't want to > change behavior yet. > > But yes, I agree that we should only parse features once and I'll be more than > happy to fix this in a followup patch, based on Igor's work. > > In the meantime, maybe I can add a comment stating that the parsing should go > away ? Right. But the thing is by open coding here, you're making two copies that need to be fixed instead of one, which increases the chances of error. It seems like it would be safer to change the generic code so there's a new generic function which doesn't do the realize which we can use on ppc (and other platforms when/if they need it). Doing the change just on ppc by making our own copy of cpu_generic_init() seems more like to lead to future mistakes. > > That is what I attempted here in the context of supporting compat cpu type > > for pseries-2.7: > > > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg381660.html > > > > Yeah and this is where I borrowed some lines. :) > > > Regards, > > Bharata. > > > > Cheers. >
On Mon, 4 Jul 2016 13:54:55 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Sat, Jul 02, 2016 at 10:33:33AM +0200, Greg Kurz wrote: > > On Sat, 2 Jul 2016 13:36:22 +0530 > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > > > > > On Sat, Jul 02, 2016 at 12:41:48AM +0200, Greg Kurz wrote: > > > > If we want to generate cpu_dt_id in the machine code, this must occur > > > > before the cpu gets realized. We must open code the cpu creation to be > > > > able to do this. > > > > > > > > This patch just does that. It borrows some lines from previous work > > > > from Bharata to handle the feature parsing. > > > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > --- > > > > hw/ppc/ppc.c | 39 ++++++++++++++++++++++++++++++++++++++- > > > > 1 file changed, 38 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c > > > > index dc3d214009c5..57f4ddd073d0 100644 > > > > --- a/hw/ppc/ppc.c > > > > +++ b/hw/ppc/ppc.c > > > > @@ -32,6 +32,7 @@ > > > > #include "sysemu/cpus.h" > > > > #include "hw/timer/m48t59.h" > > > > #include "qemu/log.h" > > > > +#include "qapi/error.h" > > > > #include "qemu/error-report.h" > > > > #include "hw/loader.h" > > > > #include "sysemu/kvm.h" > > > > @@ -1353,5 +1354,41 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id) > > > > > > > > PowerPCCPU *ppc_cpu_init(const char *cpu_model) > > > > { > > > > - return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, cpu_model)); > > > > + PowerPCCPU *cpu; > > > > + CPUClass *cc; > > > > + ObjectClass *oc; > > > > + gchar **model_pieces; > > > > + Error *err = NULL; > > > > + > > > > + model_pieces = g_strsplit(cpu_model, ",", 2); > > > > + if (!model_pieces[0]) { > > > > + error_report("Invalid/empty CPU model name"); > > > > + return NULL; > > > > + } > > > > + > > > > + oc = cpu_class_by_name(TYPE_POWERPC_CPU, model_pieces[0]); > > > > + if (oc == NULL) { > > > > + error_report("Unable to find CPU definition: %s", model_pieces[0]); > > > > + return NULL; > > > > + } > > > > + > > > > + cpu = POWERPC_CPU(object_new(object_class_get_name(oc))); > > > > + > > > > + cc = CPU_CLASS(oc); > > > > + cc->parse_features(CPU(cpu), model_pieces[1], &err); > > > > > > Igor is working on a patchset to convert -cpu features into global properties. > > > IIUC, after that patchset, it is not recommended to parse the -cpu features > > > for every CPU but do it only once. > > > > > > > cpu_generic_init() in the current code also does the parsing, and as the title > > says, this patch is just about open coding the creation. I don't want to > > change behavior yet. > > > > But yes, I agree that we should only parse features once and I'll be more than > > happy to fix this in a followup patch, based on Igor's work. > > > > In the meantime, maybe I can add a comment stating that the parsing should go > > away ? > > Right. But the thing is by open coding here, you're making two copies > that need to be fixed instead of one, which increases the chances of > error. > > It seems like it would be safer to change the generic code so there's > a new generic function which doesn't do the realize which we can use > on ppc (and other platforms when/if they need it). > > Doing the change just on ppc by making our own copy of > cpu_generic_init() seems more like to lead to future mistakes. > I had this in v1: http://patchwork.ozlabs.org/patch/642216/ I'll repost it in v3. > > > That is what I attempted here in the context of supporting compat cpu type > > > for pseries-2.7: > > > > > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg381660.html > > > > > > > Yeah and this is where I borrowed some lines. :) > > > > > Regards, > > > Bharata. > > > > > > > Cheers. > > >
On Mon, 4 Jul 2016 13:54:55 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Sat, Jul 02, 2016 at 10:33:33AM +0200, Greg Kurz wrote: > > On Sat, 2 Jul 2016 13:36:22 +0530 > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > > > > > On Sat, Jul 02, 2016 at 12:41:48AM +0200, Greg Kurz wrote: > > > > If we want to generate cpu_dt_id in the machine code, this must > > > > occur before the cpu gets realized. We must open code the cpu > > > > creation to be able to do this. > > > > > > > > This patch just does that. It borrows some lines from previous > > > > work from Bharata to handle the feature parsing. > > > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > --- > > > > hw/ppc/ppc.c | 39 ++++++++++++++++++++++++++++++++++++++- > > > > 1 file changed, 38 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c > > > > index dc3d214009c5..57f4ddd073d0 100644 > > > > --- a/hw/ppc/ppc.c > > > > +++ b/hw/ppc/ppc.c > > > > @@ -32,6 +32,7 @@ > > > > #include "sysemu/cpus.h" > > > > #include "hw/timer/m48t59.h" > > > > #include "qemu/log.h" > > > > +#include "qapi/error.h" > > > > #include "qemu/error-report.h" > > > > #include "hw/loader.h" > > > > #include "sysemu/kvm.h" > > > > @@ -1353,5 +1354,41 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int > > > > cpu_dt_id) > > > > > > > > PowerPCCPU *ppc_cpu_init(const char *cpu_model) > > > > { > > > > - return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, > > > > cpu_model)); > > > > + PowerPCCPU *cpu; > > > > + CPUClass *cc; > > > > + ObjectClass *oc; > > > > + gchar **model_pieces; > > > > + Error *err = NULL; > > > > + > > > > + model_pieces = g_strsplit(cpu_model, ",", 2); > > > > + if (!model_pieces[0]) { > > > > + error_report("Invalid/empty CPU model name"); > > > > + return NULL; > > > > + } > > > > + > > > > + oc = cpu_class_by_name(TYPE_POWERPC_CPU, model_pieces[0]); > > > > + if (oc == NULL) { > > > > + error_report("Unable to find CPU definition: %s", > > > > model_pieces[0]); > > > > + return NULL; > > > > + } > > > > + > > > > + cpu = POWERPC_CPU(object_new(object_class_get_name(oc))); > > > > + > > > > + cc = CPU_CLASS(oc); > > > > + cc->parse_features(CPU(cpu), model_pieces[1], &err); > > > > > > Igor is working on a patchset to convert -cpu features into > > > global properties. IIUC, after that patchset, it is not > > > recommended to parse the -cpu features for every CPU but do it > > > only once. > > > > > > > cpu_generic_init() in the current code also does the parsing, and > > as the title says, this patch is just about open coding the > > creation. I don't want to change behavior yet. > > > > But yes, I agree that we should only parse features once and I'll > > be more than happy to fix this in a followup patch, based on Igor's > > work. > > > > In the meantime, maybe I can add a comment stating that the parsing > > should go away ? > > Right. But the thing is by open coding here, you're making two copies > that need to be fixed instead of one, which increases the chances of > error. this patch matches what has been done for x86 target as a pert of decoupling *-user mode from machine emulation. > It seems like it would be safer to change the generic code so there's > a new generic function which doesn't do the realize which we can use > on ppc (and other platforms when/if they need it). We've had it in x86 until recently but I've replaced it cpu_generic_init(), please don't go that route. There is not much to generalize here so far it's basically following code cpu = object_new(cpu-type) parse-features(cpu) set_properties(cpu) /* optional machine specific */ cpu->realize() once parse-features refactoring is merged, there won't be anything cpu specific left as parse-features(cpu) could be moved to generic code and only very machine specific set_properties would be left which are not generalizable. > > Doing the change just on ppc by making our own copy of > cpu_generic_init() seems more like to lead to future mistakes. I wouldn't touch cpu_init()/cpu_generic_init() and leave it only for *-user users (as it's been done for x86). > > > > That is what I attempted here in the context of supporting compat > > > cpu type for pseries-2.7: > > > > > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg381660.html > > > > > > > Yeah and this is where I borrowed some lines. :) > > > > > Regards, > > > Bharata. > > > > > > > Cheers. > > >
On Mon, 4 Jul 2016 08:32:04 +0200 Greg Kurz <groug@kaod.org> wrote: > On Mon, 4 Jul 2016 13:54:55 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Sat, Jul 02, 2016 at 10:33:33AM +0200, Greg Kurz wrote: > > > On Sat, 2 Jul 2016 13:36:22 +0530 > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > > > > > > > On Sat, Jul 02, 2016 at 12:41:48AM +0200, Greg Kurz wrote: > > > > > If we want to generate cpu_dt_id in the machine code, this must occur > > > > > before the cpu gets realized. We must open code the cpu creation to be > > > > > able to do this. > > > > > > > > > > This patch just does that. It borrows some lines from previous work > > > > > from Bharata to handle the feature parsing. > > > > > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > > --- > > > > > hw/ppc/ppc.c | 39 ++++++++++++++++++++++++++++++++++++++- > > > > > 1 file changed, 38 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c > > > > > index dc3d214009c5..57f4ddd073d0 100644 > > > > > --- a/hw/ppc/ppc.c > > > > > +++ b/hw/ppc/ppc.c > > > > > @@ -32,6 +32,7 @@ > > > > > #include "sysemu/cpus.h" > > > > > #include "hw/timer/m48t59.h" > > > > > #include "qemu/log.h" > > > > > +#include "qapi/error.h" > > > > > #include "qemu/error-report.h" > > > > > #include "hw/loader.h" > > > > > #include "sysemu/kvm.h" > > > > > @@ -1353,5 +1354,41 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id) > > > > > > > > > > PowerPCCPU *ppc_cpu_init(const char *cpu_model) > > > > > { > > > > > - return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, cpu_model)); > > > > > + PowerPCCPU *cpu; > > > > > + CPUClass *cc; > > > > > + ObjectClass *oc; > > > > > + gchar **model_pieces; > > > > > + Error *err = NULL; > > > > > + > > > > > + model_pieces = g_strsplit(cpu_model, ",", 2); > > > > > + if (!model_pieces[0]) { > > > > > + error_report("Invalid/empty CPU model name"); > > > > > + return NULL; > > > > > + } > > > > > + > > > > > + oc = cpu_class_by_name(TYPE_POWERPC_CPU, model_pieces[0]); > > > > > + if (oc == NULL) { > > > > > + error_report("Unable to find CPU definition: %s", model_pieces[0]); > > > > > + return NULL; > > > > > + } > > > > > + > > > > > + cpu = POWERPC_CPU(object_new(object_class_get_name(oc))); > > > > > + > > > > > + cc = CPU_CLASS(oc); > > > > > + cc->parse_features(CPU(cpu), model_pieces[1], &err); > > > > > > > > Igor is working on a patchset to convert -cpu features into global properties. > > > > IIUC, after that patchset, it is not recommended to parse the -cpu features > > > > for every CPU but do it only once. > > > > > > > > > > cpu_generic_init() in the current code also does the parsing, and as the title > > > says, this patch is just about open coding the creation. I don't want to > > > change behavior yet. > > > > > > But yes, I agree that we should only parse features once and I'll be more than > > > happy to fix this in a followup patch, based on Igor's work. > > > > > > In the meantime, maybe I can add a comment stating that the parsing should go > > > away ? > > > > Right. But the thing is by open coding here, you're making two copies > > that need to be fixed instead of one, which increases the chances of > > error. > > > > It seems like it would be safer to change the generic code so there's > > a new generic function which doesn't do the realize which we can use > > on ppc (and other platforms when/if they need it). > > > > Doing the change just on ppc by making our own copy of > > cpu_generic_init() seems more like to lead to future mistakes. > > > > I had this in v1: > > http://patchwork.ozlabs.org/patch/642216/ > > I'll repost it in v3. > I tend to agree with Igor's latest feeback: https://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg00381.html I'll keep the open coded version in v3. > > > > That is what I attempted here in the context of supporting compat cpu type > > > > for pseries-2.7: > > > > > > > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg381660.html > > > > > > > > > > Yeah and this is where I borrowed some lines. :) > > > > > > > Regards, > > > > Bharata. > > > > > > > > > > Cheers. > > > > > >
On Mon, Jul 04, 2016 at 09:37:47AM +0200, Igor Mammedov wrote: > On Mon, 4 Jul 2016 13:54:55 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Sat, Jul 02, 2016 at 10:33:33AM +0200, Greg Kurz wrote: > > > On Sat, 2 Jul 2016 13:36:22 +0530 > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > > > > > > > On Sat, Jul 02, 2016 at 12:41:48AM +0200, Greg Kurz wrote: > > > > > If we want to generate cpu_dt_id in the machine code, this must > > > > > occur before the cpu gets realized. We must open code the cpu > > > > > creation to be able to do this. > > > > > > > > > > This patch just does that. It borrows some lines from previous > > > > > work from Bharata to handle the feature parsing. > > > > > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > > --- > > > > > hw/ppc/ppc.c | 39 ++++++++++++++++++++++++++++++++++++++- > > > > > 1 file changed, 38 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c > > > > > index dc3d214009c5..57f4ddd073d0 100644 > > > > > --- a/hw/ppc/ppc.c > > > > > +++ b/hw/ppc/ppc.c > > > > > @@ -32,6 +32,7 @@ > > > > > #include "sysemu/cpus.h" > > > > > #include "hw/timer/m48t59.h" > > > > > #include "qemu/log.h" > > > > > +#include "qapi/error.h" > > > > > #include "qemu/error-report.h" > > > > > #include "hw/loader.h" > > > > > #include "sysemu/kvm.h" > > > > > @@ -1353,5 +1354,41 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int > > > > > cpu_dt_id) > > > > > > > > > > PowerPCCPU *ppc_cpu_init(const char *cpu_model) > > > > > { > > > > > - return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, > > > > > cpu_model)); > > > > > + PowerPCCPU *cpu; > > > > > + CPUClass *cc; > > > > > + ObjectClass *oc; > > > > > + gchar **model_pieces; > > > > > + Error *err = NULL; > > > > > + > > > > > + model_pieces = g_strsplit(cpu_model, ",", 2); > > > > > + if (!model_pieces[0]) { > > > > > + error_report("Invalid/empty CPU model name"); > > > > > + return NULL; > > > > > + } > > > > > + > > > > > + oc = cpu_class_by_name(TYPE_POWERPC_CPU, model_pieces[0]); > > > > > + if (oc == NULL) { > > > > > + error_report("Unable to find CPU definition: %s", > > > > > model_pieces[0]); > > > > > + return NULL; > > > > > + } > > > > > + > > > > > + cpu = POWERPC_CPU(object_new(object_class_get_name(oc))); > > > > > + > > > > > + cc = CPU_CLASS(oc); > > > > > + cc->parse_features(CPU(cpu), model_pieces[1], &err); > > > > > > > > Igor is working on a patchset to convert -cpu features into > > > > global properties. IIUC, after that patchset, it is not > > > > recommended to parse the -cpu features for every CPU but do it > > > > only once. > > > > > > > > > > cpu_generic_init() in the current code also does the parsing, and > > > as the title says, this patch is just about open coding the > > > creation. I don't want to change behavior yet. > > > > > > But yes, I agree that we should only parse features once and I'll > > > be more than happy to fix this in a followup patch, based on Igor's > > > work. > > > > > > In the meantime, maybe I can add a comment stating that the parsing > > > should go away ? > > > > Right. But the thing is by open coding here, you're making two copies > > that need to be fixed instead of one, which increases the chances of > > error. > this patch matches what has been done for x86 target as a pert of > decoupling *-user mode from machine emulation. > > > It seems like it would be safer to change the generic code so there's > > a new generic function which doesn't do the realize which we can use > > on ppc (and other platforms when/if they need it). > We've had it in x86 until recently but I've replaced it > cpu_generic_init(), please don't go that route. > > There is not much to generalize here so far it's basically following > code > cpu = object_new(cpu-type) > parse-features(cpu) > > set_properties(cpu) /* optional machine specific */ > > cpu->realize() > > once parse-features refactoring is merged, there won't be anything cpu > specific left as parse-features(cpu) could be moved to generic code > and only very machine specific set_properties would be left which are > not generalizable. Ok. Greg, belay that suggestion :).
diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c index dc3d214009c5..57f4ddd073d0 100644 --- a/hw/ppc/ppc.c +++ b/hw/ppc/ppc.c @@ -32,6 +32,7 @@ #include "sysemu/cpus.h" #include "hw/timer/m48t59.h" #include "qemu/log.h" +#include "qapi/error.h" #include "qemu/error-report.h" #include "hw/loader.h" #include "sysemu/kvm.h" @@ -1353,5 +1354,41 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id) PowerPCCPU *ppc_cpu_init(const char *cpu_model) { - return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, cpu_model)); + PowerPCCPU *cpu; + CPUClass *cc; + ObjectClass *oc; + gchar **model_pieces; + Error *err = NULL; + + model_pieces = g_strsplit(cpu_model, ",", 2); + if (!model_pieces[0]) { + error_report("Invalid/empty CPU model name"); + return NULL; + } + + oc = cpu_class_by_name(TYPE_POWERPC_CPU, model_pieces[0]); + if (oc == NULL) { + error_report("Unable to find CPU definition: %s", model_pieces[0]); + return NULL; + } + + cpu = POWERPC_CPU(object_new(object_class_get_name(oc))); + + cc = CPU_CLASS(oc); + cc->parse_features(CPU(cpu), model_pieces[1], &err); + g_strfreev(model_pieces); + if (err != NULL) { + goto out; + } + + object_property_set_bool(OBJECT(cpu), true, "realized", &err); + +out: + if (err != NULL) { + error_report_err(err); + object_unref(OBJECT(cpu)); + return NULL; + } + + return cpu; }
If we want to generate cpu_dt_id in the machine code, this must occur before the cpu gets realized. We must open code the cpu creation to be able to do this. This patch just does that. It borrows some lines from previous work from Bharata to handle the feature parsing. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/ppc/ppc.c | 39 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-)