From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54973) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VDxq9-0007qa-Ru for qemu-devel@nongnu.org; Mon, 26 Aug 2013 10:32:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VDxq4-0007vZ-Bo for qemu-devel@nongnu.org; Mon, 26 Aug 2013 10:32:21 -0400 Message-ID: <521B66ED.1010905@suse.de> Date: Mon, 26 Aug 2013 16:32:13 +0200 From: =?ISO-8859-1?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <520CEA00.2020404@suse.de> <1376885192-31651-1-git-send-email-aik@ozlabs.ru> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC PATCH v4] powerpc: add PVR mask support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf , Alexey Kardashevskiy Cc: qemu-ppc@nongnu.org, Anthony Liguori , Paul Mackerras , qemu-devel@nongnu.org Am 26.08.2013 15:04, schrieb Alexander Graf: >=20 > On 19.08.2013, at 06:06, Alexey Kardashevskiy wrote: >=20 >> IBM POWERPC processors encode PVR as a CPU family in higher 16 bits an= d >> 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 CP= U >> version in QEMU's CPU list. Also, new CPU versions of already supporte= d >> 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 o= nly); >> 2. make CPU family not abstract; >> 3. add a @pvr_default (probably bad name) - the idea when QEMU instant= iates >> 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 ne= ed >> to touch aliases. >> >> Cc: Andreas F=E4rber >> Signed-off-by: Alexey Kardashevskiy >> >> --- >> >> 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 bi= ts >> 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 =3D POWERPC_CPU_CLASS(oc); = \ >> = \ >> pcc->pvr =3D _pvr; = \ >> + pcc->pvr_default =3D 0; = \ >=20 > 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 =3D CPU_POWERPC_DEFAULT_MASK; = \ >> pcc->svr =3D _svr; = \ >> dc->desc =3D _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 =3D 0xFFFFFFFF, >> /* PowerPC 401 family */ >> /* Generic PowerPC 401 */ >> #define CPU_POWERPC_401 CPU_POWERPC_401G2 >> @@ -557,6 +558,12 @@ enum { >> CPU_POWERPC_POWER7_v23 =3D 0x003F0203, >> CPU_POWERPC_POWER7P_v21 =3D 0x004A0201, >> CPU_POWERPC_POWER8_v10 =3D 0x004B0100, >> + CPU_POWERPC_POWER7 =3D 0x003F0000, >> + CPU_POWERPC_POWER7_MASK =3D 0xFFFF0000, >> + CPU_POWERPC_POWER7P =3D 0x004A0000, >> + CPU_POWERPC_POWER7P_MASK =3D 0xFFFF0000, >> + CPU_POWERPC_POWER8 =3D 0x004B0000, >> + CPU_POWERPC_POWER8_MASK =3D 0xFFFF0000, >=20 > 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 patc= h 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 =3D 0x12340000, CPU_POWERPC_POWER7_MASK =3D 0xffff0000, CPU_POWERPC_POWER7_V21 =3D 0x12340201, CPU_POWERPC_POWER8_... >=20 >> CPU_POWERPC_970 =3D 0x00390202, >> CPU_POWERPC_970FX_v10 =3D 0x00391100, >> CPU_POWERPC_970FX_v20 =3D 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(ObjectCla= ss *oc, void *data) >> uint32_t dcache_size =3D kvmppc_read_int_cpu_dt("d-cache-size"); >> uint32_t icache_size =3D kvmppc_read_int_cpu_dt("i-cache-size"); >> >> + pcc->pvr_default =3D mfpvr(); >> + >> /* Now fix up the class with information we can query from the hos= t */ >> >> if (vmx !=3D -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) =3D { = \ >> .name =3D stringify(_name) "-family-" TYPE_POWERPC_CPU, = \ >> .parent =3D TYPE_POWERPC_CPU, = \ >> - .abstract =3D true, = \ >> .class_init =3D 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 *da= ta) >> PowerPCCPUClass *pcc =3D POWERPC_CPU_CLASS(oc); >> >> dc->desc =3D "POWER7"; >> + pcc->pvr =3D CPU_POWERPC_POWER7; >> + pcc->pvr_mask =3D CPU_POWERPC_POWER7_MASK; >> + pcc->pvr_default =3D CPU_POWERPC_POWER7_v23; >> pcc->init_proc =3D init_proc_POWER7; >> pcc->check_pow =3D check_pow_nocheck; >> pcc->insns_flags =3D 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); >=20 > The automatically generated host class should just take on the host PVR= value, so there's no need for jumping through hoops here. >=20 > 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 >=20 >=20 > Alex >=20 >> /* Register SVR if it's defined to anything else than POWERPC_SVR_= NONE */ >> if (pcc->svr !=3D 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 =3D DEVICE_CLASS(oc); >> >> pcc->parent_realize =3D dc->realize; >> + pcc->pvr =3D CPU_POWERPC_DEFAULT_MASK; >> + pcc->pvr_mask =3D 0; >> + pcc->pvr_default =3D 0; >> dc->realize =3D ppc_cpu_realizefn; >> dc->unrealize =3D ppc_cpu_unrealizefn; >> >> --=20 >> 1.8.3.2 >> >=20 --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg