From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60147) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V9vAe-0006cS-Rd for qemu-devel@nongnu.org; Thu, 15 Aug 2013 06:52:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V9vAU-0001PT-Bd for qemu-devel@nongnu.org; Thu, 15 Aug 2013 06:52:48 -0400 Message-ID: <520CB2EC.8060105@suse.de> Date: Thu, 15 Aug 2013 12:52:28 +0200 From: =?ISO-8859-1?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <7AAF2DA2-1FEF-4ECA-B220-366D94EC70C4@suse.de> <1376552735-2953-1-git-send-email-aik@ozlabs.ru> <520C8C11.6080905@ozlabs.ru> <98FAFA62-3B3A-40E9-8973-705C5F569CEE@suse.de> In-Reply-To: <98FAFA62-3B3A-40E9-8973-705C5F569CEE@suse.de> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC PATCH v3] powerpc: add PVR mask support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: Anthony Liguori , Alexey Kardashevskiy , qemu-devel@nongnu.org, qemu-ppc@nongnu.org, Paul Mackerras Am 15.08.2013 10:45, schrieb Alexander Graf: >=20 > On 15.08.2013, at 10:06, Alexey Kardashevskiy wrote: >=20 >> On 08/15/2013 05:55 PM, Alexander Graf wrote: >>> >>> On 15.08.2013, at 09:45, Alexey Kardashevskiy wrote: >>> >>>> IBM POWERPC processors encode PVR as a CPU family in higher 16 bits = and >>>> a CPU version in lower 16 bits. Since there is no significant change >>>> in behavior between versions, there is no point to add every single = CPU >>>> version in QEMU's CPU list. Also, new CPU versions of already suppor= ted >>>> CPU won't break the existing code. >>>> >>>> This adds a PVR mask support which means that aliases are replaced w= ith >>>> another layer in POWERPC CPU class hierarchy. The patch adds interme= diate >>>> POWER7, POWER7+ and POWER8 CPU classes and makes use of those in >>>> specific versioned POWERPC CPUs. >>>> >>>> Cc: Andreas F=E4rber >>>> Signed-off-by: Alexey Kardashevskiy >>>> >>>> --- >>>> Changes: >>>> v3: >>>> * renamed macros to describe the functionality better >>>> * added default PVR value for the powerpc cpu family (what alias use= d to do) >>>> >>>> v2: >>>> * aliases are replaced with another level in class hierarchy >>>> --- >>>> target-ppc/cpu-models.c | 54 ++++++++++++++++++++++++++++++++---= ---------- >>>> target-ppc/cpu-models.h | 7 ++++++ >>>> target-ppc/cpu-qom.h | 2 ++ >>>> target-ppc/translate_init.c | 4 ++-- >>>> 4 files changed, 50 insertions(+), 17 deletions(-) >>>> >>>> diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c >>>> index 8dea560..e48004b 100644 >>>> --- a/target-ppc/cpu-models.c >>>> +++ b/target-ppc/cpu-models.c >>>> @@ -35,7 +35,8 @@ >>>> /* PowerPC CPU definitions = */ >>>> #define POWERPC_DEF_PREFIX(pvr, svr, type) = \ >>>> glue(glue(glue(glue(pvr, _), svr), _), type) >>>> -#define POWERPC_DEF_SVR(_name, _desc, _pvr, _svr, _type) = \ >>>> +#define POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, _pvr_mask, _pvr_de= fault, \ >>>> + _svr, _type, _parent) = \ >>>> static void = \ >>>> glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_class_init) = \ >>>> (ObjectClass *oc, void *data) = \ Quite obviously this heavily conflicts with my patchset for Prerna's problem... >>>> @@ -44,6 +45,8 @@ >>>> PowerPCCPUClass *pcc =3D POWERPC_CPU_CLASS(oc); = \ >>>> = \ >>>> pcc->pvr =3D _pvr; = \ >>>> + pcc->pvr_default =3D _pvr_default; = \ >>>> + pcc->pvr_mask =3D _pvr_mask; = \ >>>> pcc->svr =3D _svr; = \ >>>> dc->desc =3D _desc; = \ >>>> } = \ >>>> @@ -51,7 +54,7 @@ >>>> static const TypeInfo = \ >>>> glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_type_info) =3D {= \ >>>> .name =3D _name "-" TYPE_POWERPC_CPU, = \ >>>> - .parent =3D stringify(_type) "-family-" TYPE_POWERPC_CP= U, \ >>>> + .parent =3D _parent, = \ >>>> .class_init =3D = \ >>>> glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_class_in= it), \ >>>> }; = \ >>>> @@ -66,9 +69,24 @@ >>>> type_init( = \ >>>> glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_register_typ= es)) >>>> >>>> +#define POWERPC_DEF_SVR(_name, _desc, _pvr, _svr, _type) = \ >>>> + POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, CPU_POWERPC_DEFAULT_MA= SK, 0, \ >>>> + _svr, _type, = \ >>>> + stringify(_type) "-family-" TYPE_POWERPC_C= PU) >>>> + >>>> #define POWERPC_DEF(_name, _pvr, _type, _desc) = \ >>>> POWERPC_DEF_SVR(_name, _desc, _pvr, POWERPC_SVR_NONE, _type) >>>> >>>> +#define POWERPC_DEF_FAMILY(_name, _pvr, _pvr_mask, _pvr_default, = \ >>>> + _type, _desc) = \ >>>> + POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, _pvr_mask, _pvr_defaul= t, \ >>>> + POWERPC_SVR_NONE, _type, = \ >>>> + stringify(_type) "-family-" TYPE_POWERPC_C= PU) >>>> + >>>> +#define POWERPC_DEF_FAMILY_MEMBER(_name, _pvr, _type, _desc, _paren= t) \ >>>> + POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, CPU_POWERPC_DEFAULT_MA= SK, 0, \ >>>> + POWERPC_SVR_NONE, _type, _parent) >>>> + >>>> /* Embedded PowerPC = */ >>>> /* PowerPC 401 family = */ >>>> POWERPC_DEF("401", CPU_POWERPC_401, = 401, >>>> @@ -1133,16 +1151,25 @@ >>>> POWERPC_DEF("POWER6A", CPU_POWERPC_POWER6A, = POWER6, >>>> "POWER6A") >>>> #endif >>>> - POWERPC_DEF("POWER7_v2.0", CPU_POWERPC_POWER7_v20, = POWER7, >>>> - "POWER7 v2.0") >>>> - POWERPC_DEF("POWER7_v2.1", CPU_POWERPC_POWER7_v21, = POWER7, >>>> - "POWER7 v2.1") >>>> - POWERPC_DEF("POWER7_v2.3", CPU_POWERPC_POWER7_v23, = POWER7, >>>> - "POWER7 v2.3") >>>> - POWERPC_DEF("POWER7+_v2.1", CPU_POWERPC_POWER7P_v21, = POWER7, >>>> - "POWER7+ v2.1") >>>> - POWERPC_DEF("POWER8_v1.0", CPU_POWERPC_POWER8_v10, = POWER8, >>>> - "POWER8 v1.0") >>>> + POWERPC_DEF_FAMILY("POWER7", CPU_POWERPC_POWER7, CPU_POWERPC_PO= WER7_MASK, >>>> + CPU_POWERPC_POWER7_v23, >>>> + POWER7, "POWER7") >>>> + POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.0", CPU_POWERPC_POWER7_v20= , POWER7, >>>> + "POWER7 v2.0", "POWER7-" TYPE_POWERPC_CPU) No. Alex wanted these macros to abstract all the QOM business, so make that "POWER7" if you need it here and assembly the type name inside the macro. I'll have a closer look later. We already have the POWER7 argument so I find this redundant. If we need more families we can simply add them, such as POWER7P. >>>> + POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.1", CPU_POWERPC_POWER7_v21= , POWER7, >>>> + "POWER7 v2.1", "POWER7-" TYPE_POWERPC_CPU) >>>> + POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.3", CPU_POWERPC_POWER7_v23= , POWER7, >>>> + "POWER7 v2.3", "POWER7-" TYPE_POWERPC_CPU) >>>> + POWERPC_DEF_FAMILY("POWER7+", CPU_POWERPC_POWER7P, CPU_POWERPC_= POWER7P_MASK, >>>> + CPU_POWERPC_POWER7P_v21, >>>> + POWER7, "POWER7") >>>> + POWERPC_DEF_FAMILY_MEMBER("POWER7+_v2.1", CPU_POWERPC_POWER7P_v= 21, POWER7, >>>> + "POWER7+ v2.1", "POWER7+-" TYPE_POWERPC_CPU) >>>> + POWERPC_DEF_FAMILY("POWER8", CPU_POWERPC_POWER8, CPU_POWERPC_PO= WER8_MASK, >>>> + CPU_POWERPC_POWER8_v10, >>>> + POWER8, "POWER8") >>>> + POWERPC_DEF_FAMILY_MEMBER("POWER8_v1.0", CPU_POWERPC_POWER8_v10= , POWER8, >>>> + "POWER8 v1.0", "POWER8-" TYPE_POWERPC= _CPU) >>>> POWERPC_DEF("970", CPU_POWERPC_970, = 970, >>>> "PowerPC 970") >>>> POWERPC_DEF("970fx_v1.0", CPU_POWERPC_970FX_v10, = 970FX, >>>> @@ -1389,9 +1416,6 @@ PowerPCCPUAlias ppc_cpu_aliases[] =3D { >>>> { "POWER3+", "631" }, >>>> { "POWER5gr", "POWER5" }, >>>> { "POWER5gs", "POWER5+" }, >>>> - { "POWER7", "POWER7_v2.3" }, >>>> - { "POWER7+", "POWER7+_v2.1" }, >>>> - { "POWER8", "POWER8_v1.0" }, >>>> { "970fx", "970fx_v3.1" }, >>>> { "970mp", "970mp_v1.1" }, >>>> { "Apache", "RS64" }, >>>> diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h >>>> index d9145d1..2233053 100644 >>>> --- a/target-ppc/cpu-models.h >>>> +++ b/target-ppc/cpu-models.h >>>> @@ -39,6 +39,7 @@ extern PowerPCCPUAlias ppc_cpu_aliases[]; >>>> /*******************************************************************= **********/ >>>> /* PVR definitions for most known PowerPC = */ >>>> enum { >>>> + CPU_POWERPC_DEFAULT_MASK =3D 0xFFFFFFFF, >>>> /* PowerPC 401 family */ >>>> /* Generic PowerPC 401 */ >>>> #define CPU_POWERPC_401 CPU_POWERPC_401G2 >>>> @@ -557,6 +558,12 @@ enum { >>>> CPU_POWERPC_POWER7_v23 =3D 0x003F0203, >>>> CPU_POWERPC_POWER7P_v21 =3D 0x004A0201, >>>> CPU_POWERPC_POWER8_v10 =3D 0x004B0100, >>>> + CPU_POWERPC_POWER7 =3D 0x003F0000, >>>> + CPU_POWERPC_POWER7_MASK =3D 0xFFFF0000, >>>> + CPU_POWERPC_POWER7P =3D 0x004A0000, >>>> + CPU_POWERPC_POWER7P_MASK =3D 0xFFFF0000, >>>> + CPU_POWERPC_POWER8 =3D 0x004B0000, >>>> + CPU_POWERPC_POWER8_MASK =3D 0xFFFF0000, >>>> CPU_POWERPC_970 =3D 0x00390202, >>>> CPU_POWERPC_970FX_v10 =3D 0x00391100, >>>> CPU_POWERPC_970FX_v20 =3D 0x003C0200, >>>> diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h >>>> index f3c710a..a1a712c 100644 >>>> --- a/target-ppc/cpu-qom.h >>>> +++ b/target-ppc/cpu-qom.h >>>> @@ -54,6 +54,8 @@ typedef struct PowerPCCPUClass { >>>> void (*parent_reset)(CPUState *cpu); >>>> >>>> uint32_t pvr; >>>> + uint32_t pvr_default; >>>> + uint32_t pvr_mask; >>>> uint32_t svr; >>>> uint64_t insns_flags; >>>> uint64_t insns_flags2; >>>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init= .c >>>> index 13b290c..e73792d 100644 >>>> --- a/target-ppc/translate_init.c >>>> +++ b/target-ppc/translate_init.c >>>> @@ -7309,7 +7309,7 @@ static void init_ppc_proc(PowerPCCPU *cpu) >>>> #endif >>>> SPR_NOACCESS, >>>> &spr_read_generic, SPR_NOACCESS, >>>> - pcc->pvr); >>>> + pcc->pvr_default ? pcc->pvr_default : pcc->pvr); >>> >> >> >>> This means that -cpu host on a POWER7_v20 system will still return >>> POWER7_v23 and thus expose a different CPU inside the guest than >>> expected with PR KVM, no? >> >> >> ./qemu-system-ppc64 \ >> -cpu \ >> POWER7_v2.0 \ >=20 > This is not -cpu host, is it? :) >=20 >> -S \ >> -m "1024" \ >> -machine "pseries,usb=3Doff" \ >> -nographic \ >> -vga "none" \ >> -enable-kvm \ >> -kernel "host.vmlinux" \ >> -initrd "1.cpio" >> >> QEMU 1.5.91 monitor - type 'help' for more information >> (qemu) info registers >> [...] >> FPSCR 0000000000000000 >> SRR0 0000000000000000 SRR1 0000000000000000 PVR 00000000003f0200 >> [...] >> (qemu) >> >> PVR is 003f0200 which is correct. >> >> >>> >>> Is there any case where major=3D0 minor=3D0 is valid? If not, we coul= d add a >>> check in the class constructor and then default to sane values when w= e >>> hit this case. We'll have to add logic there later anyways if we want= to >>> allow the user to manually specify major and minor numbers. >> >> As I was told and then noticed in the kernel (and sorry for my ignoran= ce if >> I am wrong), the lower 16 bits are major/minor numbers only for IBM PO= WERPC >> CPUs and other CPUs may use different number of bits for this purpose.= So I >> would not parse those numbers and carry them as major/minor version in= QEMU >> and rather use some fixed sane value for a whole family for the cases = when >> the user does not really care about the exact chip version. >=20 > Yes, I think it makes sense to keep the full PVR around when we want to= be specific. What I'm referring to is class specific logic that can asse= mble major/minor numbers from the command line. So >=20 > -cpu POWER7,major=3D2,minor=3D0 >=20 > would result in a PVR value that is identical to POWER7_v2.0. The assem= bly of this PVR value is class specific, because different classes of CPU= s have different semantics for their major and minor numbers. >=20 > That way in the future we won't have to add any new version specific CP= U types but instead the user can assemble those himself, making everyone'= s life a lot easier. >=20 > My point was that if we have that logic, we could at the same place jus= t say "if my major/minor is 0, default to something reasonable". >=20 > But let's ask Andreas whether he has a better idea here :). If you read the previous discussion on the initial POWER7+ patch, I believe I had proposed major-version / minor-version or so properties at family level, to be able to use different implementations or none at all where we don't see a scheme. However if we want to use that from -cpu as in your example above, we would have to implement custom parsing code for cpu_model, which I would rather avoid, given we want to replace it with -device in the future. But maybe I didn't fully catch the exact question. :) The custom parenting strikes me as a wrong consequence of us not having fully QOM'ified / cleaned up the family classes yet. We had discussed two ways: Either have, e.g., POWER7+ inherit from POWER7 (which looks like the only reason this is being done here) and/or have, e.g., POWER5+ copy and modify 970fx values via #defines. Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg