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

["Andreas Färber" <afaerber@suse.de>]

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.

>> @@ -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