On 11/08/2013 01:01 AM, Andreas Färber wrote:> 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 <pbonz...@redhat.com> 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.
Good. Thanks for comments.
>
>>
>> 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.
I did all of this (I hope) so here is another try.
> 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).
Ufff... I did not get the part about a hint. Everything is string
in the command line :)
---
Alexey Kardashevskiy (2):
cpu: add suboptions support
target-ppc: add "compat" CPU option
hw/ppc/spapr.c | 12 +++++++++-
include/qom/cpu.h | 29 +++++++++++++++++++++++
include/sysemu/sysemu.h | 1 +
qom/cpu.c | 27 ++++++++++++++++++++++
target-ppc/cpu-models.h | 16 +++++++++++++
target-ppc/cpu.h | 4 ++++
target-ppc/translate_init.c | 56 +++++++++++++++++++++++++++++++++++++++++++++
vl.c | 42 ++++++++++++++++++++++++++++++++++
8 files changed, 186 insertions(+), 1 deletion(-)
--
1.8.4.rc4
