| Message ID | 1383815511-21996-1-git-send-email-aik@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
On Thu, 7 Nov 2013 20:11:51 +1100 Alexey Kardashevskiy <aik@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 <pbonzini@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. 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) > 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 CPUClass = get_cpu_class_by_name(classname) class->cpu_translate_features_compat(classname, cpu_model_str) > break; > case QEMU_OPTION_hda: > {
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 <pbonzini@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. > > 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. 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). Regards, Andreas
On 11/08/2013 12:36 AM, Igor Mammedov wrote: > On Thu, 7 Nov 2013 20:11:51 +1100 > Alexey Kardashevskiy <aik@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 <pbonzini@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: >> { >
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 <pbonzini@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(-)
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 <aik@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 <pbonzini@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 :) 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: >>> {
Am 08.11.2013 09:22, schrieb Alexey Kardashevskiy: > On 11/08/2013 01:01 AM, Andreas Färber wrote: >> 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 :) Your "compat" and "compat1" properties in the previous patch were of type int. :) The question I raised was how to best implement Alex' suggestion of using literal "power6" rather than 206 as value. Andreas
On 11/09/2013 12:20 AM, Andreas Färber wrote: > 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. Does it mean I better stop wasting your time and relax and you guys do this -cpu subptions thing yourselves? Thanks.
Am 08.11.2013 15:57, schrieb Alexey Kardashevskiy: > On 11/09/2013 12:20 AM, Andreas Färber wrote: > >> 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. > > Does it mean I better stop wasting your time and relax and you guys do this > -cpu subptions thing yourselves? Thanks. No, I meant don't try to mess with translating -cpu to -global and looking up the class name that Igor mentioned. (Depending on when that is being done it may introduce subtle bugs like real -global being overridden.) If you wait for us then you'll wait for some time... Andreas
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_reset: * @cpu: The CPU whose state is to be reset. */ diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h index b27a1df..9007e44 100644 --- a/include/sysemu/sysemu.h +++ b/include/sysemu/sysemu.h @@ -192,6 +192,7 @@ DeviceState *get_boot_device_ex(uint32_t position, char **suffix); DeviceState *get_boot_device(uint32_t position); QemuOpts *qemu_get_machine_opts(void); +QemuOpts *qemu_get_cpu_opts(void); bool usb_enabled(bool default_usb); 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); +} + static void cpu_common_reset(CPUState *cpu) { CPUClass *cc = CPU_GET_CLASS(cpu); diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c index 93f7cba..d357b1f 100644 --- a/target-ppc/translate_init.c +++ b/target-ppc/translate_init.c @@ -28,6 +28,8 @@ #include "mmu-hash32.h" #include "mmu-hash64.h" #include "qemu/error-report.h" +#include "hw/qdev-properties.h" +#include "qapi/visitor.h" //#define PPC_DUMP_CPU //#define PPC_DEBUG_SPR @@ -4733,6 +4735,11 @@ POWERPC_FAMILY(e5500)(ObjectClass *oc, void *data) /* Non-embedded PowerPC */ +static Property powerpc_properties[] = { + DEFINE_PROP_INT32("compat1", PowerPCCPU, env.compat, -1), + DEFINE_PROP_END_OF_LIST(), +}; + /* POWER : same as 601, without mfmsr, mfsr */ POWERPC_FAMILY(POWER)(ObjectClass *oc, void *data) { @@ -4743,6 +4750,7 @@ POWERPC_FAMILY(POWER)(ObjectClass *oc, void *data) /* pcc->insns_flags = XXX_TODO; */ /* POWER RSC (from RAD6000) */ pcc->msr_mask = 0x00000000FEF0ULL; + dc->props = powerpc_properties; } #define POWERPC_MSRR_601 (0x0000000000001040ULL) @@ -7262,6 +7270,7 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data) POWERPC_FLAG_BUS_CLK | POWERPC_FLAG_CFAR; pcc->l1_dcache_size = 0x8000; pcc->l1_icache_size = 0x8000; + dc->props = powerpc_properties; } POWERPC_FAMILY(POWER7P)(ObjectClass *oc, void *data) @@ -8390,6 +8399,10 @@ PowerPCCPU *cpu_ppc_init(const char *cpu_model) cpu = POWERPC_CPU(object_new(object_class_get_name(oc))); + if (cpu_common_set_properties(OBJECT(cpu))) { + return NULL; + } + object_property_set_bool(OBJECT(cpu), true, "realized", &err); if (err != NULL) { error_report("%s", error_get_pretty(err)); @@ -8611,6 +8624,30 @@ static void ppc_cpu_reset(CPUState *s) tlb_flush(env, 1); } +static void powerpc_get_compat(Object *obj, Visitor *v, + void *opaque, const char *name, Error **errp) +{ + PowerPCCPU *cpu = POWERPC_CPU(obj); + int64_t value = cpu->env.compat; + + visit_type_int(v, &value, name, errp); +} + +static void powerpc_set_compat(Object *obj, Visitor *v, + void *opaque, const char *name, Error **errp) +{ + PowerPCCPU *cpu = POWERPC_CPU(obj); + Error *error = NULL; + int64_t value; + + visit_type_int(v, &value, name, &error); + if (error) { + error_propagate(errp, error); + return; + } + cpu->env.compat = value; +} + static void ppc_cpu_initfn(Object *obj) { CPUState *cs = CPU(obj); @@ -8618,6 +8655,9 @@ static void ppc_cpu_initfn(Object *obj) PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); CPUPPCState *env = &cpu->env; + object_property_add(obj, "compat", "int", + powerpc_get_compat, powerpc_set_compat, + NULL, NULL, NULL); cs->env_ptr = env; cpu_exec_init(env); 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 = { }, }; +static QemuOptsList qemu_cpu_opts = { + .name = "cpu", + .implied_opt_name = "type", + .merge_lists = true, + .head = QTAILQ_HEAD_INITIALIZER(qemu_cpu_opts.head), + .desc = { + { /* End of list */ } + }, +}; + static QemuOptsList qemu_boot_opts = { .name = "boot-opts", .implied_opt_name = "order", @@ -552,6 +562,20 @@ QemuOpts *qemu_get_machine_opts(void) return opts; } +QemuOpts *qemu_get_cpu_opts(void) +{ + QemuOptsList *list; + QemuOpts *opts; + + list = qemu_find_opts("cpu"); + assert(list); + opts = qemu_opts_find(list, NULL); + if (!opts) { + opts = qemu_opts_create_nofail(list); + } + return opts; +} + const char *qemu_get_vm_name(void) { return qemu_name; @@ -2942,6 +2966,7 @@ int main(int argc, char **argv, char **envp) qemu_add_opts(&qemu_trace_opts); qemu_add_opts(&qemu_option_rom_opts); qemu_add_opts(&qemu_machine_opts); + qemu_add_opts(&qemu_cpu_opts); qemu_add_opts(&qemu_smp_opts); qemu_add_opts(&qemu_boot_opts); qemu_add_opts(&qemu_sandbox_opts); @@ -3037,7 +3062,15 @@ int main(int argc, char **argv, char **envp) } case QEMU_OPTION_cpu: /* hw initialization will check this */ - cpu_model = optarg; + olist = qemu_find_opts("cpu"); + opts = qemu_opts_parse(olist, optarg, 1); + if (!opts) { + exit(1); + } + optarg = qemu_opt_get(opts, "type"); + if (optarg) { + cpu_model = optarg; + } break; case QEMU_OPTION_hda: {