Re: [Qemu-devel] [Qemu-ppc] [PATCHv4 3/5] pseries: Move CPU compatibility property to machine

[David Gibson]

On Thu, Jun 01, 2017 at 03:44:40PM +1000, Suraj Jitindar Singh wrote:
> On Fri, 2017-05-26 at 15:23 +1000, David Gibson wrote:
> > Server class POWER CPUs have a "compat" property, which is used to
> > set the
> > backwards compatibility mode for the processor.  However, this only
> > makes
> > sense for machine types which don't give the guest access to
> > hypervisor
> > privilege - otherwise the compatibility level is under the guest's
> > control.
> > 
> > To reflect this, this removes the CPU 'compat' property and instead
> > creates a 'max-cpu-compat' property on the pseries machine.  Strictly
> > speaking this breaks compatibility, but AFAIK the 'compat' option was
> > never (directly) used with -device or device_add.
> > 
> > The option was used with -cpu.  So, to maintain compatibility, this
> > patch adds a hack to the cpu option parsing to strip out any compat
> > options supplied with -cpu and set them on the machine property
> > instead of the now deprecated cpu property.
> 
> Generally looks good, a couple of comments below.
> 
> Suraj
> 
> > 
> > Signed-off-by: David Gibson <address@hidden>
> > ---
> >  hw/ppc/spapr.c              |   6 ++-
> >  hw/ppc/spapr_cpu_core.c     |  56 +++++++++++++++++++++++-
> >  hw/ppc/spapr_hcall.c        |   8 ++--
> >  include/hw/ppc/spapr.h      |  12 ++++--
> >  target/ppc/compat.c         | 102
> > ++++++++++++++++++++++++++++++++++++++++++++
> >  target/ppc/cpu.h            |   5 ++-
> >  target/ppc/translate_init.c |  86 +++++++++++-----------------------
> > ---
> >  7 files changed, 202 insertions(+), 73 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index ab3aab1..3c4e88f 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2134,7 +2134,7 @@ static void ppc_spapr_init(MachineState
> > *machine)
> >          machine->cpu_model = kvm_enabled() ? "host" : smc-
> > >tcg_default_cpu;
> >      }
> >  
> > -    ppc_cpu_parse_features(machine->cpu_model);
> > +    spapr_cpu_parse_features(spapr);
> >  
> >      spapr_init_cpus(spapr);
> >  
> > @@ -2497,6 +2497,10 @@ static void spapr_machine_initfn(Object *obj)
> >                                      " place of standard EPOW events
> > when possible"
> >                                      " (required for memory hot-
> > unplug support)",
> >                                      NULL);
> > +
> > +    ppc_compat_add_property(obj, "max-cpu-compat", &spapr-
> > >max_compat_pvr,
> > +                            "Maximum permitted CPU compatibility
> > mode",
> > +                            &error_fatal);
> >  }
> >  
> >  static void spapr_machine_finalizefn(Object *obj)
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index ff7058e..ab4102b 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -20,6 +20,58 @@
> >  #include "sysemu/numa.h"
> >  #include "qemu/error-report.h"
> >  
> > +void spapr_cpu_parse_features(sPAPRMachineState *spapr)
> > +{
> > +    /*
> > +     * Backwards compatibility hack:
> > +     *
> > +     *   CPUs had a "compat=" property which didn't make sense for
> > +     *   anything except pseries.  It was replaced by "max-cpu-
> > compat"
> > +     *   machine option.  This supports old command lines like
> > +     *       -cpu POWER8,compat=power7
> > +     *   By stripping the compat option and applying it to the
> > machine
> > +     *   before passing it on to the cpu level parser.
> > +     */
> > +    gchar **inpieces;
> > +    int i, j;
> > +    gchar *compat_str = NULL;
> > +
> > +    inpieces = g_strsplit(MACHINE(spapr)->cpu_model, ",", 0);
> > +
> > +    /* inpieces[0] is the actual model string */
> > +    i = 1;
> > +    j = 1;
> > +    while (inpieces[i]) {
> > +        if (g_str_has_prefix(inpieces[i], "compat=")) {
> > +            /* in case of multiple compat= optipons */
> 
> s/optipons/options?

Oops, fixed.

> > +            g_free(compat_str);
> > +            compat_str = inpieces[i];
> > +        } else {
> > +            j++;
> > +        }
> > +
> > +        /* Excise compat options from list */
> > +        inpieces[j] = inpieces[i];
> 
> it's worth noting that where previously when specifying an invalid
> option you got:
> 
> qemu-system-ppc64: Expected key=value format, found *blah*
> 
> You now get a segfault here.

Sod.  The joy of doing string manipulation in C.  Ok, I think I've
found and fixed that too.

-- 
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