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 <da...@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. > The hack above is nasty, but I'm not really seeing a better option. -- Alexey
signature.asc
Description: OpenPGP digital signature
