On 11/12/2013 06:18 PM, Alexey Kardashevskiy wrote: > 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 <a...@ozlabs.ru> >>>>>> --- >>>>>> 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. > ===
Anyone, ping? -- Alexey