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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 08/15/2013 06:45 PM, Alexander Graf wrote:
> 
> On 15.08.2013, at 10:06, Alexey Kardashevskiy wrote:
> 
>> On 08/15/2013 05:55 PM, Alexander Graf wrote:
>>>
>>> On 15.08.2013, at 09:45, 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 adds a PVR mask support which means that aliases are replaced with
>>>> another layer in POWERPC CPU class hierarchy. The patch adds intermediate
>>>> POWER7, POWER7+ and POWER8 CPU classes and makes use of those in
>>>> specific versioned POWERPC CPUs.
>>>>
>>>> Cc: Andreas Färber <afaerber@suse.de>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>
>>>> ---
>>>> Changes:
>>>> 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     | 54 ++++++++++++++++++++++++++++++++-------------
>>>> target-ppc/cpu-models.h     |  7 ++++++
>>>> target-ppc/cpu-qom.h        |  2 ++
>>>> target-ppc/translate_init.c |  4 ++--
>>>> 4 files changed, 50 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c
>>>> index 8dea560..e48004b 100644
>>>> --- a/target-ppc/cpu-models.c
>>>> +++ b/target-ppc/cpu-models.c
>>>> @@ -35,7 +35,8 @@
>>>> /* PowerPC CPU definitions                                                 */
>>>> #define POWERPC_DEF_PREFIX(pvr, svr, type)                                  \
>>>>    glue(glue(glue(glue(pvr, _), svr), _), type)
>>>> -#define POWERPC_DEF_SVR(_name, _desc, _pvr, _svr, _type)                    \
>>>> +#define POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, _pvr_mask, _pvr_default,   \
>>>> +                             _svr, _type, _parent)                          \
>>>>    static void                                                             \
>>>>    glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_class_init)            \
>>>>    (ObjectClass *oc, void *data)                                           \
>>>> @@ -44,6 +45,8 @@
>>>>        PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);                       \
>>>>                                                                            \
>>>>        pcc->pvr          = _pvr;                                           \
>>>> +        pcc->pvr_default  = _pvr_default;                                   \
>>>> +        pcc->pvr_mask     = _pvr_mask;                                      \
>>>>        pcc->svr          = _svr;                                           \
>>>>        dc->desc          = _desc;                                          \
>>>>    }                                                                       \
>>>> @@ -51,7 +54,7 @@
>>>>    static const TypeInfo                                                   \
>>>>    glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_type_info) = {         \
>>>>        .name       = _name "-" TYPE_POWERPC_CPU,                           \
>>>> -        .parent     = stringify(_type) "-family-" TYPE_POWERPC_CPU,         \
>>>> +        .parent     = _parent,                                              \
>>>>        .class_init =                                                       \
>>>>            glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_class_init),   \
>>>>    };                                                                      \
>>>> @@ -66,9 +69,24 @@
>>>>    type_init(                                                              \
>>>>        glue(POWERPC_DEF_PREFIX(_pvr, _svr, _type), _cpu_register_types))
>>>>
>>>> +#define POWERPC_DEF_SVR(_name, _desc, _pvr, _svr, _type)                    \
>>>> +    POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, CPU_POWERPC_DEFAULT_MASK, 0,   \
>>>> +                         _svr, _type,                                       \
>>>> +                         stringify(_type) "-family-" TYPE_POWERPC_CPU)
>>>> +
>>>> #define POWERPC_DEF(_name, _pvr, _type, _desc)                              \
>>>>    POWERPC_DEF_SVR(_name, _desc, _pvr, POWERPC_SVR_NONE, _type)
>>>>
>>>> +#define POWERPC_DEF_FAMILY(_name, _pvr, _pvr_mask, _pvr_default,            \
>>>> +                           _type, _desc)                                    \
>>>> +    POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, _pvr_mask, _pvr_default,       \
>>>> +                         POWERPC_SVR_NONE, _type,                           \
>>>> +                         stringify(_type) "-family-" TYPE_POWERPC_CPU)
>>>> +
>>>> +#define POWERPC_DEF_FAMILY_MEMBER(_name, _pvr, _type, _desc, _parent)       \
>>>> +    POWERPC_DEF_SVR_MASK(_name, _desc, _pvr, CPU_POWERPC_DEFAULT_MASK, 0,   \
>>>> +                         POWERPC_SVR_NONE, _type, _parent)
>>>> +
>>>>    /* Embedded PowerPC                                                      */
>>>>    /* PowerPC 401 family                                                    */
>>>>    POWERPC_DEF("401",           CPU_POWERPC_401,                    401,
>>>> @@ -1133,16 +1151,25 @@
>>>>    POWERPC_DEF("POWER6A",       CPU_POWERPC_POWER6A,                POWER6,
>>>>                "POWER6A")
>>>> #endif
>>>> -    POWERPC_DEF("POWER7_v2.0",   CPU_POWERPC_POWER7_v20,             POWER7,
>>>> -                "POWER7 v2.0")
>>>> -    POWERPC_DEF("POWER7_v2.1",   CPU_POWERPC_POWER7_v21,             POWER7,
>>>> -                "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,
>>>> -                "POWER7+ v2.1")
>>>> -    POWERPC_DEF("POWER8_v1.0",   CPU_POWERPC_POWER8_v10,             POWER8,
>>>> -                "POWER8 v1.0")
>>>> +    POWERPC_DEF_FAMILY("POWER7", CPU_POWERPC_POWER7, CPU_POWERPC_POWER7_MASK,
>>>> +                       CPU_POWERPC_POWER7_v23,
>>>> +                       POWER7, "POWER7")
>>>> +    POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.0", CPU_POWERPC_POWER7_v20, POWER7,
>>>> +                "POWER7 v2.0", "POWER7-" TYPE_POWERPC_CPU)
>>>> +    POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.1", CPU_POWERPC_POWER7_v21, POWER7,
>>>> +                "POWER7 v2.1", "POWER7-" TYPE_POWERPC_CPU)
>>>> +    POWERPC_DEF_FAMILY_MEMBER("POWER7_v2.3", CPU_POWERPC_POWER7_v23, POWER7,
>>>> +                "POWER7 v2.3", "POWER7-" TYPE_POWERPC_CPU)
>>>> +    POWERPC_DEF_FAMILY("POWER7+", CPU_POWERPC_POWER7P, CPU_POWERPC_POWER7P_MASK,
>>>> +                       CPU_POWERPC_POWER7P_v21,
>>>> +                       POWER7, "POWER7")
>>>> +    POWERPC_DEF_FAMILY_MEMBER("POWER7+_v2.1", CPU_POWERPC_POWER7P_v21, POWER7,
>>>> +                "POWER7+ v2.1", "POWER7+-" TYPE_POWERPC_CPU)
>>>> +    POWERPC_DEF_FAMILY("POWER8", CPU_POWERPC_POWER8, CPU_POWERPC_POWER8_MASK,
>>>> +                       CPU_POWERPC_POWER8_v10,
>>>> +                       POWER8, "POWER8")
>>>> +    POWERPC_DEF_FAMILY_MEMBER("POWER8_v1.0", CPU_POWERPC_POWER8_v10, POWER8,
>>>> +                              "POWER8 v1.0", "POWER8-" TYPE_POWERPC_CPU)
>>>>    POWERPC_DEF("970",           CPU_POWERPC_970,                    970,
>>>>                "PowerPC 970")
>>>>    POWERPC_DEF("970fx_v1.0",    CPU_POWERPC_970FX_v10,              970FX,
>>>> @@ -1389,9 +1416,6 @@ PowerPCCPUAlias ppc_cpu_aliases[] = {
>>>>    { "POWER3+", "631" },
>>>>    { "POWER5gr", "POWER5" },
>>>>    { "POWER5gs", "POWER5+" },
>>>> -    { "POWER7", "POWER7_v2.3" },
>>>> -    { "POWER7+", "POWER7+_v2.1" },
>>>> -    { "POWER8", "POWER8_v1.0" },
>>>>    { "970fx", "970fx_v3.1" },
>>>>    { "970mp", "970mp_v1.1" },
>>>>    { "Apache", "RS64" },
>>>> 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,
>>>>    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/translate_init.c b/target-ppc/translate_init.c
>>>> index 13b290c..e73792d 100644
>>>> --- a/target-ppc/translate_init.c
>>>> +++ b/target-ppc/translate_init.c
>>>> @@ -7309,7 +7309,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);
>>>
>>
>>
>>> This means that -cpu host on a POWER7_v20 system will still return
>>> POWER7_v23 and thus expose a different CPU inside the guest than
>>> expected with PR KVM, no?
>>
>>
>> ./qemu-system-ppc64 \
>> -cpu \
>> POWER7_v2.0 \
> 
> This is not -cpu host, is it? :)


Just at the start of a guest. Then the guest reads PVR and if it is HV-KVM
- it will be the actual one OR if it is emulated (PR KVM or TCG) - who
cares, the guest will get what QEMU tells it. Who is unhappy? Sorry for my
deep ignorance if I miss the point. Thank you.


-- 
Alexey