qemu-ppc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Qemu-ppc] [PATCH v2 0/2] spapr: add "compat" machine option


From: Alexey Kardashevskiy
Subject: [Qemu-ppc] [PATCH v2 0/2] spapr: add "compat" machine option
Date: Fri, 8 Nov 2013 19:22:41 +1100

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

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]