Re: [Qemu-devel] [RFC PATCH v4] powerpc: add PVR mask support

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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 <afaerber@suse.de>
>>> Signed-off-by: Alexey Kardashevskiy <aik@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