| Message ID | 1383878250-28432-1-git-send-email-aik@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
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 <aik@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... 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? Regards, Andreas > + dc->desc = "POWER7+"; > + pcc->pvr = CPU_POWERPC_POWER7P_BASE; > + pcc->pvr_mask = CPU_POWERPC_POWER7P_MASK; > + pcc->init_proc = init_proc_POWER7; > + pcc->check_pow = check_pow_nocheck; > + pcc->insns_flags = PPC_INSNS_BASE | PPC_ISEL | PPC_STRING | PPC_MFTB | > + PPC_FLOAT | PPC_FLOAT_FSEL | PPC_FLOAT_FRES | > + PPC_FLOAT_FSQRT | PPC_FLOAT_FRSQRTE | > + PPC_FLOAT_STFIWX | > + PPC_CACHE | PPC_CACHE_ICBI | PPC_CACHE_DCBZ | > + PPC_MEM_SYNC | PPC_MEM_EIEIO | > + PPC_MEM_TLBIE | PPC_MEM_TLBSYNC | > + PPC_64B | PPC_ALTIVEC | > + PPC_SEGMENT_64B | PPC_SLBI | > + PPC_POPCNTB | PPC_POPCNTWD; > + pcc->insns_flags2 = PPC2_VSX | PPC2_DFP | PPC2_DBRX | PPC2_ISA205; > + pcc->msr_mask = 0x800000000204FF37ULL; > + pcc->mmu_model = POWERPC_MMU_2_06; > +#if defined(CONFIG_SOFTMMU) > + pcc->handle_mmu_fault = ppc_hash64_handle_mmu_fault; > +#endif > + pcc->excp_model = POWERPC_EXCP_POWER7; > + pcc->bus_model = PPC_FLAGS_INPUT_POWER7; > + pcc->bfd_mach = bfd_mach_ppc64; > + pcc->flags = POWERPC_FLAG_VRE | POWERPC_FLAG_SE | > + POWERPC_FLAG_BE | POWERPC_FLAG_PMM | > + POWERPC_FLAG_BUS_CLK | POWERPC_FLAG_CFAR | > + POWERPC_FLAG_VSX; > + pcc->l1_dcache_size = 0x8000; > + pcc->l1_icache_size = 0x8000; > +} > + > POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data) > { > DeviceClass *dc = DEVICE_CLASS(oc); >
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 <aik@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? > 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. > > Regards, > Andreas > >> + dc->desc = "POWER7+"; >> + pcc->pvr = CPU_POWERPC_POWER7P_BASE; >> + pcc->pvr_mask = CPU_POWERPC_POWER7P_MASK; >> + pcc->init_proc = init_proc_POWER7; >> + pcc->check_pow = check_pow_nocheck; >> + pcc->insns_flags = PPC_INSNS_BASE | PPC_ISEL | PPC_STRING | PPC_MFTB | >> + PPC_FLOAT | PPC_FLOAT_FSEL | PPC_FLOAT_FRES | >> + PPC_FLOAT_FSQRT | PPC_FLOAT_FRSQRTE | >> + PPC_FLOAT_STFIWX | >> + PPC_CACHE | PPC_CACHE_ICBI | PPC_CACHE_DCBZ | >> + PPC_MEM_SYNC | PPC_MEM_EIEIO | >> + PPC_MEM_TLBIE | PPC_MEM_TLBSYNC | >> + PPC_64B | PPC_ALTIVEC | >> + PPC_SEGMENT_64B | PPC_SLBI | >> + PPC_POPCNTB | PPC_POPCNTWD; >> + pcc->insns_flags2 = PPC2_VSX | PPC2_DFP | PPC2_DBRX | PPC2_ISA205; >> + pcc->msr_mask = 0x800000000204FF37ULL; >> + pcc->mmu_model = POWERPC_MMU_2_06; >> +#if defined(CONFIG_SOFTMMU) >> + pcc->handle_mmu_fault = ppc_hash64_handle_mmu_fault; >> +#endif >> + pcc->excp_model = POWERPC_EXCP_POWER7; >> + pcc->bus_model = PPC_FLAGS_INPUT_POWER7; >> + pcc->bfd_mach = bfd_mach_ppc64; >> + pcc->flags = POWERPC_FLAG_VRE | POWERPC_FLAG_SE | >> + POWERPC_FLAG_BE | POWERPC_FLAG_PMM | >> + POWERPC_FLAG_BUS_CLK | POWERPC_FLAG_CFAR | >> + POWERPC_FLAG_VSX; >> + pcc->l1_dcache_size = 0x8000; >> + pcc->l1_icache_size = 0x8000; >> +} >> + >> POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data) >> { >> DeviceClass *dc = DEVICE_CLASS(oc); >> > >
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 <aik@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). >> 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+? In any case let's please document this properly in the commit message and not just make contradictory statements about what things should be. 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? 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. Thanks, Andreas
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 <aik@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. > > Thanks, > Andreas >
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 <aik@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. ===
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 <aik@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?
On 18.11.2013, at 03:55, Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > 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 <aik@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? Works for me. Please repost. Alex
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+"; + dc->desc = "POWER7+"; + pcc->pvr = CPU_POWERPC_POWER7P_BASE; + pcc->pvr_mask = CPU_POWERPC_POWER7P_MASK; + pcc->init_proc = init_proc_POWER7; + pcc->check_pow = check_pow_nocheck; + pcc->insns_flags = PPC_INSNS_BASE | PPC_ISEL | PPC_STRING | PPC_MFTB | + PPC_FLOAT | PPC_FLOAT_FSEL | PPC_FLOAT_FRES | + PPC_FLOAT_FSQRT | PPC_FLOAT_FRSQRTE | + PPC_FLOAT_STFIWX | + PPC_CACHE | PPC_CACHE_ICBI | PPC_CACHE_DCBZ | + PPC_MEM_SYNC | PPC_MEM_EIEIO | + PPC_MEM_TLBIE | PPC_MEM_TLBSYNC | + PPC_64B | PPC_ALTIVEC | + PPC_SEGMENT_64B | PPC_SLBI | + PPC_POPCNTB | PPC_POPCNTWD; + pcc->insns_flags2 = PPC2_VSX | PPC2_DFP | PPC2_DBRX | PPC2_ISA205; + pcc->msr_mask = 0x800000000204FF37ULL; + pcc->mmu_model = POWERPC_MMU_2_06; +#if defined(CONFIG_SOFTMMU) + pcc->handle_mmu_fault = ppc_hash64_handle_mmu_fault; +#endif + pcc->excp_model = POWERPC_EXCP_POWER7; + pcc->bus_model = PPC_FLAGS_INPUT_POWER7; + pcc->bfd_mach = bfd_mach_ppc64; + pcc->flags = POWERPC_FLAG_VRE | POWERPC_FLAG_SE | + POWERPC_FLAG_BE | POWERPC_FLAG_PMM | + POWERPC_FLAG_BUS_CLK | POWERPC_FLAG_CFAR | + POWERPC_FLAG_VSX; + pcc->l1_dcache_size = 0x8000; + pcc->l1_icache_size = 0x8000; +} + POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data) { DeviceClass *dc = DEVICE_CLASS(oc);
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. 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 <aik@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(-)