From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45701) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vg8Ez-0002a5-8R for qemu-devel@nongnu.org; Tue, 12 Nov 2013 02:18:34 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vg8Eq-0004w6-Ns for qemu-devel@nongnu.org; Tue, 12 Nov 2013 02:18:25 -0500 Received: from mail-pb0-f50.google.com ([209.85.160.50]:45784) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vg8Eq-0004u4-CB for qemu-devel@nongnu.org; Tue, 12 Nov 2013 02:18:16 -0500 Received: by mail-pb0-f50.google.com with SMTP id xb12so2444001pbc.9 for ; Mon, 11 Nov 2013 23:18:15 -0800 (PST) Message-ID: <5281D630.8080302@ozlabs.ru> Date: Tue, 12 Nov 2013 18:18:08 +1100 From: Alexey Kardashevskiy MIME-Version: 1.0 References: <1383878250-28432-1-git-send-email-aik@ozlabs.ru> <527CEACF.4020504@suse.de> <527CFB20.5000003@ozlabs.ru> <527D185E.5050909@suse.de> <527D7FCA.9040201@ozlabs.ru> In-Reply-To: <527D7FCA.9040201@ozlabs.ru> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v2] target-ppc: move POWER7+ to a separate family List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= , qemu-devel@nongnu.org Cc: Paul Mackerras , Jacques Mony , qemu-ppc@nongnu.org, Alexander Graf On 11/09/2013 11:20 AM, Alexey Kardashevskiy wrote: > On 11/09/2013 03:59 AM, Andreas Färber wrote: >> Am 08.11.2013 15:54, schrieb Alexey Kardashevskiy: >>> On 11/09/2013 12:44 AM, Andreas Färber wrote: >>>> Am 08.11.2013 03:37, schrieb Alexey Kardashevskiy: >>>>> So far POWER7+ was a part of POWER7 family. However it has a different >>>>> PVR base value so in order to support PVR masks, it needs a separate >>>>> family class. >>>>> >>>> >>>> Alexey, >>>> >>>>> Another reason to make a POWER7+ family is that its name in the device >>>>> tree (/proc/device-tree/cpus/cpu*) should be "Power7+" but not "Power7" >>>>> and this cannot be easily fixed without a new family class. >>>>> >>>>> This adds a new family class, PVR base and mask values and moves >>>>> Power7+ v2.1 CPU to a new family. The class init function is copied >>>>> from the POWER7 family. >>>>> >>>>> Signed-off-by: Alexey Kardashevskiy >>>>> --- >>>>> Changes: >>>>> v2: >>>>> * added VSX enable bit >>>>> --- >>>>> target-ppc/cpu-models.c | 2 +- >>>>> target-ppc/cpu-models.h | 2 ++ >>>>> target-ppc/translate_init.c | 38 ++++++++++++++++++++++++++++++++++++++ >>>>> 3 files changed, 41 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c >>>>> index 04d88c5..7c9466f 100644 >>>>> --- a/target-ppc/cpu-models.c >>>>> +++ b/target-ppc/cpu-models.c >>>>> @@ -1140,7 +1140,7 @@ >>>>> "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, >>>>> + POWERPC_DEF("POWER7+_v2.1", CPU_POWERPC_POWER7P_v21, POWER7P, >>>>> "POWER7+ v2.1") >>>>> POWERPC_DEF("POWER8_v1.0", CPU_POWERPC_POWER8_v10, POWER8, >>>>> "POWER8 v1.0") >>>>> diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h >>>>> index 731ec4a..49ba4a4 100644 >>>>> --- a/target-ppc/cpu-models.h >>>>> +++ b/target-ppc/cpu-models.h >>>>> @@ -558,6 +558,8 @@ enum { >>>>> CPU_POWERPC_POWER7_v20 = 0x003F0200, >>>>> CPU_POWERPC_POWER7_v21 = 0x003F0201, >>>>> CPU_POWERPC_POWER7_v23 = 0x003F0203, >>>>> + CPU_POWERPC_POWER7P_BASE = 0x004A0000, >>>>> + CPU_POWERPC_POWER7P_MASK = 0xFFFF0000, >>>>> CPU_POWERPC_POWER7P_v21 = 0x004A0201, >>>>> CPU_POWERPC_POWER8_BASE = 0x004B0000, >>>>> CPU_POWERPC_POWER8_MASK = 0xFFFF0000, >>>>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c >>>>> index 35d1389..c030a20 100644 >>>>> --- a/target-ppc/translate_init.c >>>>> +++ b/target-ppc/translate_init.c >>>>> @@ -7253,6 +7253,44 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data) >>>>> pcc->l1_icache_size = 0x8000; >>>>> } >>>>> >>>>> +POWERPC_FAMILY(POWER7P)(ObjectClass *oc, void *data) >>>>> +{ >>>>> + DeviceClass *dc = DEVICE_CLASS(oc); >>>>> + PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc); >>>>> + >>>>> + dc->fw_name = "PowerPC,POWER7+"; >>>> >>>> Apart from the commit message differing from the code... >>> >>> >>> In what part? >> >> The spelling of POWER7. You write it should be "Power7+" but implement >> it as upper-case "POWER7+" (ignoring the "PowerPC," prefix, that is). > > > Ah. Sorry. > > >>>> We've had this discussion before: Jacques reported that on his POWER7+ >>>> box only "POWER7" is shown, not "POWER7+", equivalent to my POWER5+ box >>>> showing only "PowerPC,POWER5". Compare my commit, which documents this: >>>> >>>> http://git.qemu.org/?p=qemu.git;a=commit;h=793826cd460828975591f289de78672af4a47ef9 >>>> >>>> So, adding a POWER7P family seems correct to me, just the fw_name seems >>>> wrong - or you'll need to investigate further why there are conflicting >>>> reports of how it is shown. Possibly based on revision or pHyp vs. SLOF? >>> >>> >>> Yes we have had this discussion. Paul said it should "POWER7+". The only >>> P7+ machine I have handy shows "+": >>> >>> [aik@vpl4 ~]$ ls -d /proc/device-tree/cpus/PowerPC* >>> /proc/device-tree/cpus/PowerPC,POWER7+@0 >>> /proc/device-tree/cpus/PowerPC,POWER7+@2c >>> /proc/device-tree/cpus/PowerPC,POWER7+@10 >>> /proc/device-tree/cpus/PowerPC,POWER7+@30 >>> /proc/device-tree/cpus/PowerPC,POWER7+@14 >>> /proc/device-tree/cpus/PowerPC,POWER7+@34 >>> /proc/device-tree/cpus/PowerPC,POWER7+@18 >>> /proc/device-tree/cpus/PowerPC,POWER7+@38 >>> /proc/device-tree/cpus/PowerPC,POWER7+@1c >>> /proc/device-tree/cpus/PowerPC,POWER7+@3c >>> /proc/device-tree/cpus/PowerPC,POWER7+@20 >>> /proc/device-tree/cpus/PowerPC,POWER7+@4 >>> /proc/device-tree/cpus/PowerPC,POWER7+@24 >>> /proc/device-tree/cpus/PowerPC,POWER7+@8 >>> /proc/device-tree/cpus/PowerPC,POWER7+@28 >>> /proc/device-tree/cpus/PowerPC,POWER7+@c >>> >>> And this is a host, not a guest. I do not see any good reason to make dt >>> names different. >>> >>> And this does not really matter if there is "+" or not for anybody as far >>> as we concerned, ppc64_cpu works either way. >> >> Right, it may not matter, but I expect you to reference the above commit >> id and explain why it should be POWER7+ after all. You failed to come up >> with that answer before that patch got applied, so we need to correct >> me/it now. >> >> I have checked with Dinar that under Linux using the Sapphire firmware >> "PowerPC,POWER7+@0" does indeed show up in /proc/device-tree/cpus. So >> that matches what this patch changes and what you report above. >> What could be different in Jacques' setup that he reported it different >> from us? He was checking from AIX, is that possibly using a different >> firmware, pHyp as for my POWER5+? > > It must be pHyp, I do not see any other options. > >> In any case let's please document this properly in the commit message >> and not just make contradictory statements about what things should be. > > I have no idea how to document this. No specification tells what the naming > should be so anything I write there is just my assumption. > > "This defines the cpu node name as PowerPC,POWER7+ to stay in sync with the > Sapphire host-side firmware"? > > >> Also, in qemu.git POWER7 does not have the VSX flag, only the >> instruction set VSX flag. The addition of this VSX flag for POWER7+ is >> not mentioned in the commit message. Does it depend on any of the >> lengthy VSX Stage X series on the list or something in ppc-next changing >> it for POWER7? > > The PPC-related patches I post are always made against Alex Graf "ppc-next" > tree and his tree contains VSX fixes. Since my patch simply copies POWER7 > family, I do not see much sense in mentioning all the CPU features it > enables for the new family. > > >> Either way, if you or Alex improve on the commit message then you can >> add my Reviewed-by, I verified that the VSX flag, desc and fw_name are >> the only differences. Would this commit message be ok? === target-ppc: move POWER7+ to a separate family So far POWER7+ was a part of POWER7 family. However it has a different PVR base value so in order to support PVR masks, it needs a separate family class. This adds a new family class, PVR base and mask values and moves Power7+ v2.1 CPU to a new family. The class init function is copied from the POWER7 family. This defines a firmware name for the new family as "PowerPC,POWER7+" instead of previously used "PowerPC,POWER7" from the POWER7 family. The reason for that is that the Sapphire firmware (a host firmware) uses "PowerPC,POWER7+" already and since no specification defines exactly the CPU nodes naming in the device tree, we better stay in sync with the host firmware. === -- Alexey