qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Alexander Graf
Subject: Re: [Qemu-devel] [RFC PATCH v3] powerpc: add PVR mask support
Date: Thu, 15 Aug 2013 13:03:01 +0200

On 15.08.2013, at 12:52, Andreas Färber wrote:

> Am 15.08.2013 10:45, schrieb Alexander Graf:
>> 
>> 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 <address@hidden>
>>>>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>>>>> 
>>>>> ---
>>>>> 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)                                           
>>>>> \
> 
> Quite obviously this heavily conflicts with my patchset for Prerna's
> problem...
> 
>>>>> @@ -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)
> 
> No. Alex wanted these macros to abstract all the QOM business, so make
> that "POWER7" if you need it here and assembly the type name inside the
> macro. I'll have a closer look later. We already have the POWER7
> argument so I find this redundant. If we need more families we can
> simply add them, such as POWER7P.
> 
>>>>> +    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? :)
>> 
>>> -S \
>>> -m "1024" \
>>> -machine "pseries,usb=off" \
>>> -nographic \
>>> -vga "none" \
>>> -enable-kvm \
>>> -kernel "host.vmlinux" \
>>> -initrd "1.cpio"
>>> 
>>> QEMU 1.5.91 monitor - type 'help' for more information
>>> (qemu) info registers
>>> [...]
>>> FPSCR 0000000000000000
>>> SRR0 0000000000000000  SRR1 0000000000000000    PVR 00000000003f0200
>>> [...]
>>> (qemu)
>>> 
>>> PVR is 003f0200 which is correct.
>>> 
>>> 
>>>> 
>>>> Is there any case where major=0 minor=0 is valid? If not, we could add a
>>>> check in the class constructor and then default to sane values when we
>>>> hit this case. We'll have to add logic there later anyways if we want to
>>>> allow the user to manually specify major and minor numbers.
>>> 
>>> As I was told and then noticed in the kernel (and sorry for my ignorance if
>>> I am wrong), the lower 16 bits are major/minor numbers only for IBM POWERPC
>>> CPUs and other CPUs may use different number of bits for this purpose. So I
>>> would not parse those numbers and carry them as major/minor version in QEMU
>>> and rather use some fixed sane value for a whole family for the cases when
>>> the user does not really care about the exact chip version.
>> 
>> Yes, I think it makes sense to keep the full PVR around when we want to be 
>> specific. What I'm referring to is class specific logic that can assemble 
>> major/minor numbers from the command line. So
>> 
>>  -cpu POWER7,major=2,minor=0
>> 
>> would result in a PVR value that is identical to POWER7_v2.0. The assembly 
>> of this PVR value is class specific, because different classes of CPUs have 
>> different semantics for their major and minor numbers.
>> 
>> That way in the future we won't have to add any new version specific CPU 
>> types but instead the user can assemble those himself, making everyone's 
>> life a lot easier.
>> 
>> My point was that if we have that logic, we could at the same place just say 
>> "if my major/minor is 0, default to something reasonable".
>> 
>> But let's ask Andreas whether he has a better idea here :).
> 
> If you read the previous discussion on the initial POWER7+ patch, I
> believe I had proposed major-version / minor-version or so properties at
> family level, to be able to use different implementations or none at all
> where we don't see a scheme.

Sounds like a good idea.

> However if we want to use that from -cpu as
> in your example above, we would have to implement custom parsing code
> for cpu_model, which I would rather avoid, given we want to replace it
> with -device in the future.

Can't we make this generic QOM property parsing code?

  -cpu POWER7,major-version=2,minor-version=0

would do

  cpu = new POWER7(major-version = 2, minor_version = 0);

and then the POWER7 class can decide what to do with this additional 
information?

> But maybe I didn't fully catch the exact question. :)
> 
> The custom parenting strikes me as a wrong consequence of us not having
> fully QOM'ified / cleaned up the family classes yet. We had discussed
> two ways: Either have, e.g., POWER7+ inherit from POWER7 (which looks
> like the only reason this is being done here) and/or have, e.g., POWER5+
> copy and modify 970fx values via #defines.

IIUC the family parenting is orthogonal to this. Here we're looking at having 
families as classes at all. Currently we don't - we only have explicit 
versioned implementations as classes. Whether we have

PowerPC
  `- POWER7
    `- POWER7+
      `- POWER7+ v1.0

or

PowerPC
  `- POWER7+
    `- POWER7+ v1.0

is a different question I think.


Alex




reply via email to

[Prev in Thread] Current Thread [Next in Thread]