Re: [Qemu-ppc] [PATCH v2 4/7] ppc: open code cpu creation for machine types

[David Gibson]

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 <address@hidden> 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 <address@hidden>
> > > ---
> > >  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/address@hidden/msg381660.html
> > 
> 
> Yeah and this is where I borrowed some lines. :)
> 
> > Regards,
> > Bharata.
> > 
> 
> Cheers.
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature