From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34246) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Velzh-00036Q-0R for qemu-devel@nongnu.org; Fri, 08 Nov 2013 08:21:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Velza-0003Hm-B0 for qemu-devel@nongnu.org; Fri, 08 Nov 2013 08:21:00 -0500 Message-ID: <527CE533.70504@suse.de> Date: Fri, 08 Nov 2013 14:20:51 +0100 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 References: <5278F84A.2010904@suse.de> <1383815511-21996-1-git-send-email-aik@ozlabs.ru> <20131107143630.11ca7b11@nial.usersys.redhat.com> <527C9F4A.7040001@ozlabs.ru> In-Reply-To: <527C9F4A.7040001@ozlabs.ru> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: Igor Mammedov , Paolo Bonzini , qemu-ppc@nongnu.org, qemu-devel@nongnu.org, Alexander Graf 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 wrote: >> >>> On 11/06/2013 12:53 AM, Andreas F=C3=A4rber 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 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 t= he 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 i= nto 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 t= o do live migration between different CPU types. >>>>> >>>>> Still in my opinion it should be "-cpu", not "-machine". Even if i= t'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_in= it, >>>> and for the p=3Dv parsing logic to be so generic as to just set prop= erty 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, overrid= den >>>> 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 ins= tead of >> setting properties on each CPU instance explicitly. It could done usin= g >> existing "global properties" mechanism. >> >> in current code -cpu type,foo1=3Dx,foo2=3Dy... are saved into cpu_mode= l 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=3Dx,foo2=3Dy..." vir= tually >> into a set of following options: >> "-global type.foo1=3Dx -global type.foo2=3Dy ..." >> these options when registered are transparently applied to each new CP= U >> instance (see device_post_init() for details). >> >> now why do we need translation hook, >> * not every target is able to handle "type,foo1=3Dx,foo2=3Dy" in term= s of >> properties yet >> * legacy feature string might be in non canonical format, like >> "+foo1,-foo2..." so for compatibility reasons with CLI we need targ= et >> specific code to convert to canonical format when it becomes availa= ble. >> * for targets that don't have feature string handling and implementin= g >> new features as properties we can implement generic default hook th= at >> 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. >=20 >=20 > 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 throu= gh > via qemu_opt_foreach() and set every property to a CPU, this will check= if > a property exists and assigns it =3D> 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=3D]va= lue. >> see comments below for pseudo code: >>> And few question while we are here: >>> 1. the proposed common code handles both static and dynamic propertie= s. >>> 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(CPU= State *cpu, vaddr addr) >>> #endif >>> =20 >>> /** >>> + * cpu_common_set_properties: >>> + * @cpu: The CPU whose properties to initialize from the command lin= e. >>> + */ >>> +int cpu_common_set_properties(Object *obj); >> >> cpu_translate_features_compat(classname, cpu_model_str) >=20 >=20 > 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" >>> =20 >>> bool cpu_exists(int64_t id) >>> { >>> @@ -186,6 +188,28 @@ void cpu_reset(CPUState *cpu) >>> } >>> } >>> =20 >>> +static int cpu_set_property(const char *name, const char *value, voi= d *opaque) >>> +{ >>> + DeviceState *dev =3D opaque; >>> + Error *err =3D NULL; >>> + >>> + if (strcmp(name, "type") =3D=3D 0) >>> + return 0; >>> + >>> + qdev_prop_parse(dev, name, value, &err); >>> + if (err !=3D 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, o= bj, 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=3Dval) >> } >> } >> >> 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 =3D { >>> }, >>> }; >>> =20 >> >>> case QEMU_OPTION_cpu: >>> /* hw initialization will check this */ >> >> cpu_model =3D optarg; >> classname =3D GET_CLASS_NAME_FROM_TYPE(cpu_model) <=3D not sure how >> implement it since naming is target specific, maybe Andreas has an ide= a >=20 > 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. >=20 >=20 > 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 =3D get_cpu_class_by_name(classname) >> class->cpu_translate_features_compat(classname, cpu_model_str) >> >>> break; >>> case QEMU_OPTION_hda: >>> { --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3=BC= rnberg