On Thu, Jun 01, 2017 at 09:29:08AM +0200, Greg Kurz wrote:
> On Thu, 01 Jun 2017 15:44:40 +1000
> Suraj Jitindar Singh <sjitindarsingh@gmail.com> 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 <david@gibson.dropbear.id.au>
> > > ---
> > >  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?
> > 
> > > +            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.
> > 
> 
> Yeah. This basically does:
> 
>     inpieces[i + 1] = inpieces[i];
> 
> and we end up overwriting the terminal NULL pointer with a non-NULL
> pointer.
> 
> What about simplifying the loop to:
> 
>     /* inpieces[0] is the actual model string */
>     i = 1;
>     while (inpieces[i]) {
>         if (g_str_has_prefix(inpieces[i], "compat=")) {
>             /* in case of multiple compat= optipons */
>             g_free(compat_str);
>             compat_str = inpieces[i];
>             /* Excise compat options from list */
>             inpieces[i] = inpieces[i + 1];
>         }
>         i++;
>     }

No.. that would duplicate the entry after the compat=, instead of
properly excising it.  I've already fixed this for my next draft.

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