Re: [Qemu-devel] [RFC 13/17] pseries: Move CPU compatibility property to machine

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6158 bytes --]

On Tue, Nov 08, 2016 at 04:56:10PM +1100, Alexey Kardashevskiy wrote:
> On 08/11/16 16:26, David Gibson wrote:
> > On Fri, Nov 04, 2016 at 06:43:52PM +1100, Alexey Kardashevskiy wrote:
> >> On 30/10/16 22:12, 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 new
> >>> removed cpu property.
> >>>
> >>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >>> ---
> >>>  hw/ppc/spapr.c              |  6 +++-
> >>>  hw/ppc/spapr_cpu_core.c     | 47 +++++++++++++++++++++++++++--
> >>>  hw/ppc/spapr_hcall.c        |  2 +-
> >>>  include/hw/ppc/spapr.h      | 10 +++++--
> >>>  target-ppc/compat.c         | 65 ++++++++++++++++++++++++++++++++++++++++
> >>>  target-ppc/cpu.h            |  6 ++--
> >>>  target-ppc/translate_init.c | 73 ---------------------------------------------
> >>>  7 files changed, 127 insertions(+), 82 deletions(-)
> >>>
> >>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >>> index 6c78889..b983faa 100644
> >>> --- a/hw/ppc/spapr.c
> >>> +++ b/hw/ppc/spapr.c
> >>> @@ -1849,7 +1849,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);
> >>>  
> >>> @@ -2191,6 +2191,10 @@ static void spapr_machine_initfn(Object *obj)
> >>>                                      " place of standard EPOW events when possible"
> >>>                                      " (required for memory hot-unplug support)",
> >>>                                      NULL);
> >>> +
> >>> +    object_property_add(obj, "max-cpu-compat", "str",
> >>> +                        ppc_compat_prop_get, ppc_compat_prop_set,
> >>> +                        NULL, &spapr->max_compat_pvr, &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 ee5cd14..0319516 100644
> >>> --- a/hw/ppc/spapr_cpu_core.c
> >>> +++ b/hw/ppc/spapr_cpu_core.c
> >>> @@ -18,6 +18,49 @@
> >>>  #include "target-ppc/mmu-hash64.h"
> >>>  #include "sysemu/numa.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, **outpieces;
> >>> +    int n, i, j;
> >>> +    gchar *compat_str = NULL;
> >>> +    gchar *filtered_model;
> >>> +
> >>> +    inpieces = g_strsplit(MACHINE(spapr)->cpu_model, ",", 0);
> >>> +    n = g_strv_length(inpieces);
> >>> +    outpieces = g_new0(gchar *, g_strv_length(inpieces));
> >>> +
> >>> +    /* inpieces[0] is the actual model string */
> >>> +    for (i = 0, j = 0; i < n; i++) {
> >>> +        if (g_str_has_prefix(inpieces[i], "compat=")) {
> >>> +            compat_str = inpieces[i];
> >>> +        } else {
> >>> +            outpieces[j++] = g_strdup(inpieces[i]);
> >>> +        }
> >>> +    }
> >>> +
> >>> +    if (compat_str) {
> >>> +        char *val = compat_str + strlen("compat=");
> >>> +        object_property_set_str(OBJECT(spapr), val, "max-cpu-compat",
> >>> +                                &error_fatal);
> >>
> >> This part is ok.
> >>
> >>> +    }
> >>> +
> >>> +    filtered_model = g_strjoinv(",", outpieces);
> >>> +    ppc_cpu_parse_features(filtered_model);
> >>
> >>
> >> Rather than reducing the CPU parameters string from the command line, I'd
> >> keep "dc->props = powerpc_servercpu_properties" and make them noop + warn
> >> to use the machine option instead. One day QEMU may start calling the CPU
> >> features parser itself and somebody will have to hack this thing
> >> again.
> > 
> > Hrm.  A deprecation message like that only works if a human is reading
> > it.  Usually qemu will be invoked by libvirt and the message will
> > probably disappear into some log file to scare someone unnecessarily.
> > 
> > Meanwhile, what will the actual behaviour be?  Pulling the CPU's
> > property value into the machine instead would be really ugly.
> > Ignoring it would break users with existing libvirt.
> 
> 
> I only suggested instead of removing "compat=" from the model string,
> - pass the model as is to ppc_cpu_parse_features() with no changes;
> - change powerpc_set_compat() to print a message and do nothing else, and
> add a comment there saying why it is so.

Ah, right, now I understand.  Yes, that's a good idea.  It will
greatly simplify the rather hideous string mangling, too.

> > The hack above is nasty, but I'm not really seeing a better option.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]