On 08/27/2013 12:32 AM, Andreas Färber wrote:
> Am 26.08.2013 15:04, schrieb Alexander Graf:
>>
>> On 19.08.2013, at 06:06, Alexey Kardashevskiy wrote:
>>
>>> IBM POWERPC processors encode PVR as a CPU family in higher 16 bits and
>>> a CPU version in lower 16 bits. Since there is no significant change
>>> in behavior between versions, there is no point to add every single CPU
>>> version in QEMU's CPU list. Also, new CPU versions of already supported
>>> CPU won't break the existing code.
>>>
>>> This does the following:
>>> 1. add a PVR mask support for a CPU family (in this patch for POWER7 only);
>>> 2. make CPU family not abstract;
>>> 3. add a @pvr_default (probably bad name) - the idea when QEMU instantiates
>>> POWERPC CPU from a CPU family class, this value will be written to PVR
>>> before the guest starts; KVM uses this to pass the actual PVR value to
>>> the guest if QEMU was started with -cpu host.
>>>
>>> As CPU family class name for POWER7 is "POWER7-family", there is no need
>>> to touch aliases.
>>>
>>> Cc: Andreas Färber <afaer...@suse.de>
>>> Signed-off-by: Alexey Kardashevskiy <a...@ozlabs.ru>
>>>
>>> ---
>>>
>>> This does not pretend to be the final valid patch which can go to any tree
>>> (at least because it does need a POWER7+ family difinition and some bits
>>> for POWER8 family), it is me again asking stupid question in order to
>>> reduce my ignorance and get some understanding if anyone going to care of
>>> the PVR masks problem. Thank you for comments.
>>>
>>> ps. :)
>>> ---
>>> Changes:
>>> v4:
>>> * removed bogus layer from hierarchy
>>>
>>> v3:
>>> * renamed macros to describe the functionality better
>>> * added default PVR value for the powerpc cpu family (what alias used to do)
>>>
>>> v2:
>>> * aliases are replaced with another level in class hierarchy
>>> ---
>>> target-ppc/cpu-models.c | 2 ++
>>> target-ppc/cpu-models.h | 7 +++++++
>>> target-ppc/cpu-qom.h | 2 ++
>>> target-ppc/kvm.c | 2 ++
>>> target-ppc/translate_init.c | 9 +++++++--
>>> 5 files changed, 20 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c
>>> index 8dea560..5ae88a5 100644
>>> --- a/target-ppc/cpu-models.c
>>> +++ b/target-ppc/cpu-models.c
>>> @@ -44,6 +44,8 @@
>>> PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
>>> \
>>>
>>> \
>>> pcc->pvr = _pvr;
>>> \
>>> + pcc->pvr_default = 0;
>>> \
>>
>> There is no need for pvr_default if you limit family instantiation to -cpu
>> host. Just make sure to filter out from cpu ? (and the qmp equivalent) and
>> we should be good.
>
> Matches what I was going to reply.
>
>>> + pcc->pvr_mask = CPU_POWERPC_DEFAULT_MASK;
>>> \
>>> pcc->svr = _svr;
>>> \
>>> dc->desc = _desc;
>>> \
>>> }
>>> \
>>> diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
>>> index d9145d1..2233053 100644
>>> --- a/target-ppc/cpu-models.h
>>> +++ b/target-ppc/cpu-models.h
>>> @@ -39,6 +39,7 @@ extern PowerPCCPUAlias ppc_cpu_aliases[];
>>> /*****************************************************************************/
>>> /* PVR definitions for most known PowerPC
>>> */
>>> enum {
>>> + CPU_POWERPC_DEFAULT_MASK = 0xFFFFFFFF,
>>> /* PowerPC 401 family */
>>> /* Generic PowerPC 401 */
>>> #define CPU_POWERPC_401 CPU_POWERPC_401G2
>>> @@ -557,6 +558,12 @@ enum {
>>> CPU_POWERPC_POWER7_v23 = 0x003F0203,
>>> CPU_POWERPC_POWER7P_v21 = 0x004A0201,
>>> CPU_POWERPC_POWER8_v10 = 0x004B0100,
>>> + CPU_POWERPC_POWER7 = 0x003F0000,
>>> + CPU_POWERPC_POWER7_MASK = 0xFFFF0000,
>>> + CPU_POWERPC_POWER7P = 0x004A0000,
>>> + CPU_POWERPC_POWER7P_MASK = 0xFFFF0000,
>>> + CPU_POWERPC_POWER8 = 0x004B0000,
>>> + CPU_POWERPC_POWER8_MASK = 0xFFFF0000,
>>
>> Please add support for all the other CPUs supported by PR and HV KVM at
>> least on Book3S, but preferably everything Linux supports once this patch is
>> out of RFC.
>
> Personally I didn't see that as a hard requirement - better to start
> somewhere where we are sure than touching everything and finding no one
> to ack. ;)
>
> What I would prefer here is to move the mask before the concrete PVRs
> and possibly to come up with a more telling suffix for the base PVR so
> that it doesn't get mixed up with a "real" PVR.
>
> E.g.,
> CPU_POWERPC_POWER7_BASE = 0x12340000,
> CPU_POWERPC_POWER7_MASK = 0xffff0000,
> CPU_POWERPC_POWER7_V21 = 0x12340201,
> CPU_POWERPC_POWER8_...
>
>>
>>> CPU_POWERPC_970 = 0x00390202,
>>> CPU_POWERPC_970FX_v10 = 0x00391100,
>>> CPU_POWERPC_970FX_v20 = 0x003C0200,
>>> diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
>>> index f3c710a..a1a712c 100644
>>> --- a/target-ppc/cpu-qom.h
>>> +++ b/target-ppc/cpu-qom.h
>>> @@ -54,6 +54,8 @@ typedef struct PowerPCCPUClass {
>>> void (*parent_reset)(CPUState *cpu);
>>>
>>> uint32_t pvr;
>>> + uint32_t pvr_default;
>>> + uint32_t pvr_mask;
>>> uint32_t svr;
>>> uint64_t insns_flags;
>>> uint64_t insns_flags2;
>>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>>> index 30a870e..315e499 100644
>>> --- a/target-ppc/kvm.c
>>> +++ b/target-ppc/kvm.c
>>> @@ -1731,6 +1731,8 @@ static void kvmppc_host_cpu_class_init(ObjectClass
>>> *oc, void *data)
>>> uint32_t dcache_size = kvmppc_read_int_cpu_dt("d-cache-size");
>>> uint32_t icache_size = kvmppc_read_int_cpu_dt("i-cache-size");
>>>
>>> + pcc->pvr_default = mfpvr();
>>> +
>>> /* Now fix up the class with information we can query from the host */
>>>
>>> if (vmx != -1) {
>
> This should be moved under the "fix up ... from host" heading. :)
>
>>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>>> index 13b290c..dfe398d 100644
>>> --- a/target-ppc/translate_init.c
>>> +++ b/target-ppc/translate_init.c
>>> @@ -3104,7 +3104,6 @@ static int check_pow_hid0_74xx (CPUPPCState *env)
>>> glue(glue(ppc_, _name), _cpu_family_type_info) = {
>>> \
>>> .name = stringify(_name) "-family-" TYPE_POWERPC_CPU,
>>> \
>>> .parent = TYPE_POWERPC_CPU,
>>> \
>>> - .abstract = true,
>>> \
>>> .class_init = glue(glue(ppc_, _name), _cpu_family_class_init),
>>> \
>>> };
>>> \
>>>
>>> \
>
> This should no longer be necessary (once the fuzzy search is implemented
> in KVM host type registration code path) and trivially solves the -cpu ?
> / QMP aspect Alex mentioned above.
I do not really understand this part. Will we still need/want
.abstract==true? I'll post yet another version of my patch in next 3
minutes, there I skip PPC CPU classes which parent is TYPE_POWERPC_CPU,
pretty trivial already :)
--
Alexey
