* [Qemu-devel] [PATCH v2] target-ppc: move POWER7+ to a separate family
@ 2013-11-08 2:37 Alexey Kardashevskiy
2013-11-08 13:44 ` Andreas Färber
0 siblings, 1 reply; 8+ messages in thread
From: Alexey Kardashevskiy @ 2013-11-08 2:37 UTC (permalink / raw)
To: qemu-devel
Cc: Alexey Kardashevskiy, qemu-ppc, Alexander Graf,
Andreas Färber
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(-)
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);
--
1.8.4.rc4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] target-ppc: move POWER7+ to a separate family
2013-11-08 2:37 [Qemu-devel] [PATCH v2] target-ppc: move POWER7+ to a separate family Alexey Kardashevskiy
@ 2013-11-08 13:44 ` Andreas Färber
2013-11-08 14:54 ` Alexey Kardashevskiy
0 siblings, 1 reply; 8+ messages in thread
From: Andreas Färber @ 2013-11-08 13:44 UTC (permalink / raw)
To: Alexey Kardashevskiy, qemu-devel; +Cc: Jacques Mony, qemu-ppc, Alexander Graf
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);
>
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] target-ppc: move POWER7+ to a separate family
2013-11-08 13:44 ` Andreas Färber
@ 2013-11-08 14:54 ` Alexey Kardashevskiy
2013-11-08 16:59 ` Andreas Färber
0 siblings, 1 reply; 8+ messages in thread
From: Alexey Kardashevskiy @ 2013-11-08 14:54 UTC (permalink / raw)
To: Andreas Färber, qemu-devel; +Cc: Jacques Mony, qemu-ppc, Alexander Graf
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);
>>
>
>
--
Alexey
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] target-ppc: move POWER7+ to a separate family
2013-11-08 14:54 ` Alexey Kardashevskiy
@ 2013-11-08 16:59 ` Andreas Färber
2013-11-09 0:20 ` Alexey Kardashevskiy
0 siblings, 1 reply; 8+ messages in thread
From: Andreas Färber @ 2013-11-08 16:59 UTC (permalink / raw)
To: Alexey Kardashevskiy, qemu-devel
Cc: Paul Mackerras, Jacques Mony, qemu-ppc, Alexander Graf
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
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] target-ppc: move POWER7+ to a separate family
2013-11-08 16:59 ` Andreas Färber
@ 2013-11-09 0:20 ` Alexey Kardashevskiy
2013-11-12 7:18 ` Alexey Kardashevskiy
0 siblings, 1 reply; 8+ messages in thread
From: Alexey Kardashevskiy @ 2013-11-09 0:20 UTC (permalink / raw)
To: Andreas Färber, qemu-devel
Cc: Paul Mackerras, Jacques Mony, qemu-ppc, Alexander Graf
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
>
--
Alexey
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] target-ppc: move POWER7+ to a separate family
2013-11-09 0:20 ` Alexey Kardashevskiy
@ 2013-11-12 7:18 ` Alexey Kardashevskiy
2013-11-18 8:55 ` Alexey Kardashevskiy
0 siblings, 1 reply; 8+ messages in thread
From: Alexey Kardashevskiy @ 2013-11-12 7:18 UTC (permalink / raw)
To: Andreas Färber, qemu-devel
Cc: Paul Mackerras, Jacques Mony, qemu-ppc, 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 <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.
===
--
Alexey
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] target-ppc: move POWER7+ to a separate family
2013-11-12 7:18 ` Alexey Kardashevskiy
@ 2013-11-18 8:55 ` Alexey Kardashevskiy
2013-11-18 21:03 ` Alexander Graf
0 siblings, 1 reply; 8+ messages in thread
From: Alexey Kardashevskiy @ 2013-11-18 8:55 UTC (permalink / raw)
To: Andreas Färber, qemu-devel
Cc: Paul Mackerras, Jacques Mony, qemu-ppc, Alexander Graf
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?
--
Alexey
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] target-ppc: move POWER7+ to a separate family
2013-11-18 8:55 ` Alexey Kardashevskiy
@ 2013-11-18 21:03 ` Alexander Graf
0 siblings, 0 replies; 8+ messages in thread
From: Alexander Graf @ 2013-11-18 21:03 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Paul Mackerras, Jacques Mony, qemu-ppc, Andreas Färber,
QEMU Developers
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-11-18 21:03 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-08 2:37 [Qemu-devel] [PATCH v2] target-ppc: move POWER7+ to a separate family Alexey Kardashevskiy
2013-11-08 13:44 ` Andreas Färber
2013-11-08 14:54 ` Alexey Kardashevskiy
2013-11-08 16:59 ` Andreas Färber
2013-11-09 0:20 ` Alexey Kardashevskiy
2013-11-12 7:18 ` Alexey Kardashevskiy
2013-11-18 8:55 ` Alexey Kardashevskiy
2013-11-18 21:03 ` Alexander Graf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).