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 <afaer...@suse.de>
>>>> Signed-off-by: Alexey Kardashevskiy <a...@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)
>>>> \
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. 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.
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.
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
