On 11/08/2013 12:36 AM, Igor Mammedov wrote:
> On Thu, 7 Nov 2013 20:11:51 +1100
> Alexey Kardashevskiy <a...@ozlabs.ru> 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 <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.
>
> 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
