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

[Andreas Färber]

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 <address@hidden>
>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>>
>> ---
>>
>> 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.

>> @@ -7213,6 +7212,9 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data)
>>     PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
>>
>>     dc->desc = "POWER7";
>> +    pcc->pvr = CPU_POWERPC_POWER7;
>> +    pcc->pvr_mask = CPU_POWERPC_POWER7_MASK;
>> +    pcc->pvr_default = CPU_POWERPC_POWER7_v23;
>>     pcc->init_proc = init_proc_POWER7;
>>     pcc->check_pow = check_pow_nocheck;
>>     pcc->insns_flags = PPC_INSNS_BASE | PPC_ISEL | PPC_STRING | PPC_MFTB |
>> @@ -7309,7 +7311,7 @@ static void init_ppc_proc(PowerPCCPU *cpu)
>> #endif
>>                  SPR_NOACCESS,
>>                  &spr_read_generic, SPR_NOACCESS,
>> -                 pcc->pvr);
>> +                 pcc->pvr_default ? pcc->pvr_default : pcc->pvr);
> 
> The automatically generated host class should just take on the host PVR 
> value, so there's no need for jumping through hoops here.
> 
> This patch is also missing the matching part :).

2x Yep.

But it's an RFC and I think we're steering into the right direction now. :)

Andreas

> 
> 
> Alex
> 
>>     /* Register SVR if it's defined to anything else than POWERPC_SVR_NONE */
>>     if (pcc->svr != POWERPC_SVR_NONE) {
>>         if (pcc->svr & POWERPC_SVR_E500) {
>> @@ -8553,6 +8555,9 @@ static void ppc_cpu_class_init(ObjectClass *oc, void 
>> *data)
>>     DeviceClass *dc = DEVICE_CLASS(oc);
>>
>>     pcc->parent_realize = dc->realize;
>> +    pcc->pvr = CPU_POWERPC_DEFAULT_MASK;
>> +    pcc->pvr_mask = 0;
>> +    pcc->pvr_default = 0;
>>     dc->realize = ppc_cpu_realizefn;
>>     dc->unrealize = ppc_cpu_unrealizefn;
>>
>> -- 
>> 1.8.3.2
>>
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg