Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option

[Andreas Färber]

Am 07.11.2013 10:11, schrieb Alexey Kardashevskiy:
> On 11/06/2013 12:53 AM, Andreas Färber wrote:> Am 05.11.2013 10:52, schrieb 
> Paolo Bonzini:
>>> Il 05/11/2013 10:16, Alexander Graf ha scritto:
>>>>
>>>> On 05.11.2013, at 10:06, Paolo Bonzini <address@hidden> wrote:
>>>>
>>>>> Il 30/09/2013 14:57, Alexey Kardashevskiy ha scritto:
>>>>>>>> Why is the option under -machine instead of -cpu?
>>>>>> Because it is still the same CPU and the guest will still read the real
>>>>>> PVR from the hardware (which it may not support but this is why we need
>>>>>> compatibility mode).
>>>>>
>>>>> How do you support migration from a newer to an older CPU then?  I think
>>>>> the guest should never see anything about the hardware CPU model.
>>>>
>>>> POWER can't model that. It always leaks the host CPU information into the 
>>>> guest. It's the guest kernel's responsibility to not expose that change to 
>>>> user space.
>>>>
>>>> Yes, it's broken :). I'm not even sure there is any sensible way to do 
>>>> live migration between different CPU types.
>>>
>>> Still in my opinion it should be "-cpu", not "-machine".  Even if it's
>>> just a "virtual" CPU model.
>>
>> PowerPC currently does not have -cpu option parsing. If you need to
>> implement it, I would ask for a generic hook in CPUClass set by
>> TYPE_POWERPC_CPU, so that the logic does not get hardcoded in cpu_init,
>> and for the p=v parsing logic to be so generic as to just set property p
>> to value v on the CPU instance. I.e. please make the compatibility
>> settings a static property or dynamic property of the CPU.
>>
>> Maybe the parsing code could even live in generic qom/cpu.c, overridden
>> by x86/sparc and reused for arm? Somewhere down my to-do list but
>> patches appreciated...
> 
> 
> I spent some time today trying to digest what you said, still having problems
> with understanding of what you meant and what Igor meant about global 
> variables
> (I just do not see the point in them).
> 
> Below is the result of my excercise. At the moment I would just like to know
> if I am going in the right direction or not.

The overall direction is good ... see below.

> 
> And few question while we are here:
> 1. the proposed common code handles both static and dynamic properties.
> What is the current QEMU trend about choosing static vs. dynamic? Can do
> both in POWERPC, both have benifits.

Static properties have mostly served to set a field to a value before
the object is realized. You can set a default value there. The setters
are usually no-op (error out) for realized objects.

Dynamic properties allow you (more easily) to implement any logic for
storing/retrieving the value and can serve to inspect or set a value at
runtime.

We were told on a KVM call that discovery of properties should not be a
decision factor towards static properties - management tools need to
inspect an object instance via QMP (and handle a property getting
dropped or renamed).

> 2. The static powerpc_properties array only works if defined with POWER7 
> family
> but not POWER family. Both families are abstract so I did not expect any
> difference but it is there. Any clue before I continue debugging? :)

There is no hierarchy among families. So POWER7 is not a POWER, it's a
powerpc at the bottom of the file. If you want power_properties rather
than powerpc_properties then you need to assign them individually for
POWER, ..., POWER5, ..., POWER7, POWER8 - or tweak the type hierarchy.

> 
> Thanks!
> 
> ---
> 
> This adds suboptions support for -cpu and demonstrates the use ot this
> by adding a static "compat1" and a dynamic "compat" options to POWERPC
> CPUs.

Unfortunately that approach won't work. Both x86 and sparc, as I
mentioned, need special handling, so you can't generalize it. Either we
need #ifdef'fery to rule out the exceptions, or better, what I suggested
was something along the lines of

struct CPUClass {
...
void (*parse_options)(CPUState *cpu, const char *str);
}

with cpu_common_parse_options() as the default implementation assigned
via cc->parse_options = cpu_common_parse_options; rather than called out
of common code.

You could have a trivial (inline?) function to obtain cc and call
cc->parse_options though, for use in cpu_ppc_init().

I also think you can use the object_property_* API rather than
qdev_prop_* for parsing and setting the value, compare Igor's code in
target-i386/cpu.c.

Please do separate these global preparations from the actual new ppc
property. Elsewhere it was discussed whether to use a readable string
value, which might hint at a dynamic property of type string or maybe
towards an enum (/me no experience with those yet and whether that works
better with dynamic or static).

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg