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 <afaer...@suse.de>
>> Signed-off-by: Alexey Kardashevskiy <a...@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
