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

[Andreas Färber]

Am 08.11.2013 09:22, schrieb Alexey Kardashevskiy:
> On 11/08/2013 12:36 AM, Igor Mammedov wrote:
>> On Thu,  7 Nov 2013 20:11:51 +1100
>> Alexey Kardashevskiy <address@hidden> wrote:
>>
>>> 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.
>>
>> what I've had in mind was a bit simpler and more implicit approach instead of
>> setting properties on each CPU instance explicitly. It could done using
>> existing "global properties" mechanism.
>>
>> in current code -cpu type,foo1=x,foo2=y... are saved into cpu_model string
>> which is parsed by target specific cpu_init() effectively parsing cpu_model
>> each time when creating a CPU.
>>
>> So to avoid fixing every target I suggest to leave cpu_model be as it is and
>>
>> add translation hook that will convert "type,foo1=x,foo2=y..." virtually
>> into a set of following options:
>>  "-global type.foo1=x -global type.foo2=y ..."
>> these options when registered are transparently applied to each new CPU
>> instance (see device_post_init() for details).
>>
>> now why do we need translation hook,
>>  * not every target is able to handle "type,foo1=x,foo2=y" in terms of
>>    properties yet
>>  * legacy feature string might be in non canonical format, like
>>    "+foo1,-foo2..." so for compatibility reasons with CLI we need target
>>    specific code to convert to canonical format when it becomes available.
>>  * for targets that don't have feature string handling and implementing
>>    new features as properties we can implement generic default hook that
>>    will convert canonical feature string into global properties.
>>
>> as result we eventually would be able drop cpu_model and use global
>> properties to store CPU features.
> 
> 
> What is wrong doing it in the way the "-machine" switch does it now?
> qemu_get_machine_opts() returns a global list which I can iterate through
> via qemu_opt_foreach() and set every property to a CPU, this will check if
> a property exists and assigns it => happy Aik :)

You are happy w/ ppc, but you would make x86 and sparc users unhappy! ;)
QemuOpts does not support the +feature,-feature notation, just [key=]value.

>> see comments below for pseudo code:
>>> 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.
>> I prefer static, since it's usually less boilerplate code.
>>
>>
>> [...]
>>
>>>
>>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>>> index 7739e00..a17cd73 100644
>>> --- a/include/qom/cpu.h
>>> +++ b/include/qom/cpu.h
>>> @@ -327,6 +327,12 @@ static inline hwaddr cpu_get_phys_page_debug(CPUState 
>>> *cpu, vaddr addr)
>>>  #endif
>>>  
>>>  /**
>>> + * cpu_common_set_properties:
>>> + * @cpu: The CPU whose properties to initialize from the command line.
>>> + */
>>> +int cpu_common_set_properties(Object *obj);
>>
>> cpu_translate_features_compat(classname, cpu_model_str)
> 
> 
> Here I lost you. I am looking to a generic way of adding any number of
> properties to "-cpu", not just "compat".

That's just naming and doesn't rule each other out, important is the
string argument that you pass in.

>>> diff --git a/qom/cpu.c b/qom/cpu.c
>>> index 818fb26..6516c63 100644
>>> --- a/qom/cpu.c
>>> +++ b/qom/cpu.c
>>> @@ -24,6 +24,8 @@
>>>  #include "qemu/notify.h"
>>>  #include "qemu/log.h"
>>>  #include "sysemu/sysemu.h"
>>> +#include "hw/qdev-properties.h"
>>> +#include "qapi/qmp/qerror.h"
>>>  
>>>  bool cpu_exists(int64_t id)
>>>  {
>>> @@ -186,6 +188,28 @@ void cpu_reset(CPUState *cpu)
>>>      }
>>>  }
>>>  
>>> +static int cpu_set_property(const char *name, const char *value, void 
>>> *opaque)
>>> +{
>>> +    DeviceState *dev = opaque;
>>> +    Error *err = NULL;
>>> +
>>> +    if (strcmp(name, "type") == 0)
>>> +        return 0;
>>> +
>>> +    qdev_prop_parse(dev, name, value, &err);
>>> +    if (err != NULL) {
>>> +        qerror_report_err(err);
>>> +        error_free(err);
>>> +        return -1;
>>> +    }
>>> +    return 0;
>>> +}
>>> +
>>> +int cpu_common_set_properties(Object *obj)
>>> +{
>>> +    return qemu_opt_foreach(qemu_get_cpu_opts(), cpu_set_property, obj, 1);
>>> +}
>> replace ^^^ with:
>>
>> void cpu_translate_features_compat(classname, cpu_model_str) {
>>     for_each_foo in cpu_model_str {
>>         qdev_prop_register_global(classname.foo=val)
>>     }
>> }
>>
>> and set default hook to NULL for every target that does custom
>> parsing (i.e. x86/sparc) so hook will be NOP there.
>>
>>
>>> diff --git a/vl.c b/vl.c
>>> index d5c5d8c..a5fbc38 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -437,6 +437,16 @@ static QemuOptsList qemu_machine_opts = {
>>>      },
>>>  };
>>>  
>>
>>>              case QEMU_OPTION_cpu:
>>>                  /* hw initialization will check this */
>>
>> cpu_model = optarg;
>> classname = GET_CLASS_NAME_FROM_TYPE(cpu_model) <= not sure how
>> implement it since naming is target specific, maybe Andreas has an idea
> 
> Heh. We cannot do this as on ppc we have to use ppc_cpu_class_by_name() and
> we definitely do not want to call it from vl.c.
> 
> 
> Thanks for comments but I'll try what Andreas suggested if I understood it
> all right and that suggestion is any different from yours :)

Actually it was more or less the same suggestion in parallel, modulo
whether to set properties on the instance (easier) or via global list.

In cpu_ppc_init() you have access to the instance and can use
object_get_typename(OBJECT(cpu)) to obtain the string class name to use.
Using a more specific type name than where the properties are defined
should work just okay, only the opposite wouldn't work. But it's gonna
be easier for you if you take one step a time. :)

When I am finally through with review of Igor's patches then he can
implement that for x86 and we/you can copy or adapt it for ppc. No need
to do big experiments for a concretely needed ppc feature.

Cheers,
Andreas

>> CPUClass = get_cpu_class_by_name(classname)
>> class->cpu_translate_features_compat(classname, cpu_model_str)
>>
>>>                  break;
>>>              case QEMU_OPTION_hda:
>>>                  {

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