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

[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 :)



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


>> 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 :)



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




-- 
Alexey