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 <sjitindarsi...@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 <da...@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
signature.asc
Description: PGP signature
