* [Qemu-devel] [PATCH] target-ppc: Add POWER7+ CPU model @ 2013-08-02 2:59 Alexey Kardashevskiy 2013-08-02 16:14 ` Andreas Färber 2013-08-14 16:28 ` Anthony Liguori 0 siblings, 2 replies; 6+ messages in thread From: Alexey Kardashevskiy @ 2013-08-02 2:59 UTC (permalink / raw) To: qemu-devel Cc: Anthony Liguori, Alexey Kardashevskiy, Alexander Graf, qemu-ppc, Paul Mackerras This patch adds CPU PVR definition for POWER7+. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- target-ppc/cpu-models.c | 2 ++ target-ppc/cpu-models.h | 1 + 2 files changed, 3 insertions(+) diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c index 9578ed8..c97c183 100644 --- a/target-ppc/cpu-models.c +++ b/target-ppc/cpu-models.c @@ -1143,6 +1143,8 @@ "POWER7 v2.1") POWERPC_DEF("POWER7_v2.3", CPU_POWERPC_POWER7_v23, POWER7, "POWER7 v2.3") + POWERPC_DEF("POWER7P", CPU_POWERPC_POWER7P, POWER7, + "POWER7P") POWERPC_DEF("POWER8_v1.0", CPU_POWERPC_POWER8_v10, POWER8, "POWER8 v1.0") POWERPC_DEF("970", CPU_POWERPC_970, 970, diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h index 01e488f..c3c78d1 100644 --- a/target-ppc/cpu-models.h +++ b/target-ppc/cpu-models.h @@ -556,6 +556,7 @@ enum { CPU_POWERPC_POWER7_v20 = 0x003F0200, CPU_POWERPC_POWER7_v21 = 0x003F0201, CPU_POWERPC_POWER7_v23 = 0x003F0203, + CPU_POWERPC_POWER7P = 0x004A0201, CPU_POWERPC_POWER8_v10 = 0x004B0100, CPU_POWERPC_970 = 0x00390202, CPU_POWERPC_970FX_v10 = 0x00391100, -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] target-ppc: Add POWER7+ CPU model 2013-08-02 2:59 [Qemu-devel] [PATCH] target-ppc: Add POWER7+ CPU model Alexey Kardashevskiy @ 2013-08-02 16:14 ` Andreas Färber 2013-08-05 5:43 ` Paul Mackerras 2013-08-14 16:28 ` Anthony Liguori 1 sibling, 1 reply; 6+ messages in thread From: Andreas Färber @ 2013-08-02 16:14 UTC (permalink / raw) To: Alexey Kardashevskiy Cc: Paul Mackerras, Anthony Liguori, qemu-ppc, qemu-devel, Alexander Graf Am 02.08.2013 04:59, schrieb Alexey Kardashevskiy: > This patch adds CPU PVR definition for POWER7+. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > target-ppc/cpu-models.c | 2 ++ > target-ppc/cpu-models.h | 1 + > 2 files changed, 3 insertions(+) > > diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c > index 9578ed8..c97c183 100644 > --- a/target-ppc/cpu-models.c > +++ b/target-ppc/cpu-models.c > @@ -1143,6 +1143,8 @@ > "POWER7 v2.1") > POWERPC_DEF("POWER7_v2.3", CPU_POWERPC_POWER7_v23, POWER7, > "POWER7 v2.3") > + POWERPC_DEF("POWER7P", CPU_POWERPC_POWER7P, POWER7, > + "POWER7P") Subject says POWER7+ rather than POWER7P. Since this is a string there's nothing wrong with +. How should it show up in SLOF? See also below. > POWERPC_DEF("POWER8_v1.0", CPU_POWERPC_POWER8_v10, POWER8, > "POWER8 v1.0") > POWERPC_DEF("970", CPU_POWERPC_970, 970, > diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h > index 01e488f..c3c78d1 100644 > --- a/target-ppc/cpu-models.h > +++ b/target-ppc/cpu-models.h > @@ -556,6 +556,7 @@ enum { > CPU_POWERPC_POWER7_v20 = 0x003F0200, > CPU_POWERPC_POWER7_v21 = 0x003F0201, > CPU_POWERPC_POWER7_v23 = 0x003F0203, > + CPU_POWERPC_POWER7P = 0x004A0201, Shouldn't this be ..._POWER7P_v21 to align with the surrounding models? Ditto for the model name then, POWER7+ being an alias to the latest version only. The first field becomes part of the type name, so we should properly name it from the start. Andreas > CPU_POWERPC_POWER8_v10 = 0x004B0100, > CPU_POWERPC_970 = 0x00390202, > CPU_POWERPC_970FX_v10 = 0x00391100, -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] target-ppc: Add POWER7+ CPU model 2013-08-02 16:14 ` Andreas Färber @ 2013-08-05 5:43 ` Paul Mackerras 2013-08-05 8:24 ` Andreas Färber 0 siblings, 1 reply; 6+ messages in thread From: Paul Mackerras @ 2013-08-05 5:43 UTC (permalink / raw) To: Andreas Färber Cc: Alexey Kardashevskiy, Anthony Liguori, qemu-ppc, qemu-devel, Alexander Graf On Fri, Aug 02, 2013 at 06:14:46PM +0200, Andreas Färber wrote: > Am 02.08.2013 04:59, schrieb Alexey Kardashevskiy: > > This patch adds CPU PVR definition for POWER7+. > > > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > --- > > target-ppc/cpu-models.c | 2 ++ > > target-ppc/cpu-models.h | 1 + > > 2 files changed, 3 insertions(+) > > > > diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c > > index 9578ed8..c97c183 100644 > > --- a/target-ppc/cpu-models.c > > +++ b/target-ppc/cpu-models.c > > @@ -1143,6 +1143,8 @@ > > "POWER7 v2.1") > > POWERPC_DEF("POWER7_v2.3", CPU_POWERPC_POWER7_v23, POWER7, > > "POWER7 v2.3") > > + POWERPC_DEF("POWER7P", CPU_POWERPC_POWER7P, POWER7, > > + "POWER7P") > > Subject says POWER7+ rather than POWER7P. Since this is a string there's > nothing wrong with +. How should it show up in SLOF? Do you mean in the device tree? The children of the /cpus node should be named like "PowerPC,POWER7+@xxx". > See also below. > > > POWERPC_DEF("POWER8_v1.0", CPU_POWERPC_POWER8_v10, POWER8, > > "POWER8 v1.0") > > POWERPC_DEF("970", CPU_POWERPC_970, 970, > > diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h > > index 01e488f..c3c78d1 100644 > > --- a/target-ppc/cpu-models.h > > +++ b/target-ppc/cpu-models.h > > @@ -556,6 +556,7 @@ enum { > > CPU_POWERPC_POWER7_v20 = 0x003F0200, > > CPU_POWERPC_POWER7_v21 = 0x003F0201, > > CPU_POWERPC_POWER7_v23 = 0x003F0203, > > + CPU_POWERPC_POWER7P = 0x004A0201, > > Shouldn't this be ..._POWER7P_v21 to align with the surrounding models? > Ditto for the model name then, POWER7+ being an alias to the latest > version only. Does the fact that all these minor revisions are enumerated imply that QEMU is always matching all the bits of the PVR value? If so then that seems very fragile to me, given that QEMU looks at the host's PVR value when using KVM. All it takes is for IBM to release a new minor revision of a CPU to break existing compiled versions of qemu (e.g. from a distro). Wouldn't it be better to be able to match only the top 16 bits, at least for the situations where there is no architectural or significant behavioural change between the versions? Paul. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] target-ppc: Add POWER7+ CPU model 2013-08-05 5:43 ` Paul Mackerras @ 2013-08-05 8:24 ` Andreas Färber 2013-08-05 9:15 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 6+ messages in thread From: Andreas Färber @ 2013-08-05 8:24 UTC (permalink / raw) To: Paul Mackerras Cc: Anthony Liguori, Stuart Yoder, Alexey Kardashevskiy, qemu-devel, Alexander Graf, qemu-ppc, Scott Wood Am 05.08.2013 07:43, schrieb Paul Mackerras: > On Fri, Aug 02, 2013 at 06:14:46PM +0200, Andreas Färber wrote: >> Am 02.08.2013 04:59, schrieb Alexey Kardashevskiy: >>> This patch adds CPU PVR definition for POWER7+. >>> >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>> --- >>> target-ppc/cpu-models.c | 2 ++ >>> target-ppc/cpu-models.h | 1 + >>> 2 files changed, 3 insertions(+) >>> >>> diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c >>> index 9578ed8..c97c183 100644 >>> --- a/target-ppc/cpu-models.c >>> +++ b/target-ppc/cpu-models.c >>> @@ -1143,6 +1143,8 @@ >>> "POWER7 v2.1") >>> POWERPC_DEF("POWER7_v2.3", CPU_POWERPC_POWER7_v23, POWER7, >>> "POWER7 v2.3") >>> + POWERPC_DEF("POWER7P", CPU_POWERPC_POWER7P, POWER7, >>> + "POWER7P") >> >> Subject says POWER7+ rather than POWER7P. Since this is a string there's >> nothing wrong with +. How should it show up in SLOF? > > Do you mean in the device tree? The children of the /cpus node should > be named like "PowerPC,POWER7+@xxx". Thanks. >> See also below. >> >>> POWERPC_DEF("POWER8_v1.0", CPU_POWERPC_POWER8_v10, POWER8, >>> "POWER8 v1.0") >>> POWERPC_DEF("970", CPU_POWERPC_970, 970, >>> diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h >>> index 01e488f..c3c78d1 100644 >>> --- a/target-ppc/cpu-models.h >>> +++ b/target-ppc/cpu-models.h >>> @@ -556,6 +556,7 @@ enum { >>> CPU_POWERPC_POWER7_v20 = 0x003F0200, >>> CPU_POWERPC_POWER7_v21 = 0x003F0201, >>> CPU_POWERPC_POWER7_v23 = 0x003F0203, >>> + CPU_POWERPC_POWER7P = 0x004A0201, >> >> Shouldn't this be ..._POWER7P_v21 to align with the surrounding models? >> Ditto for the model name then, POWER7+ being an alias to the latest >> version only. > > Does the fact that all these minor revisions are enumerated imply that > QEMU is always matching all the bits of the PVR value? Yes, see target-ppc/kvm.c. > If so then > that seems very fragile to me, given that QEMU looks at the host's PVR > value when using KVM. All it takes is for IBM to release a new minor > revision of a CPU to break existing compiled versions of qemu > (e.g. from a distro). > Wouldn't it be better to be able to match only > the top 16 bits, at least for the situations where there is no > architectural or significant behavioural change between the versions? Not being the ppc maintainer, I can only point out that this is how QEMU works today and that, independently, it makes sense to name the PVR and model after the revision they encode for self-documentation. Ben has pointed out that POWER5++ PVR-wise was a POWER5+ v3.0+ or so and, according to you, POWER5++ is not just a revision with hardware-only changes and a new marketing name. So no, it's not always just the upper 16 bits. For e500v1/v2 the revision appears to be encoded in the two least significant nibbles rather than in the two least significant bytes. For e300 it's in the second most significant byte. For the 405/440 models it's not linear from a to b to c at all. Cf. target-ppc/cpu-models.h. If we wanted to make our matching less precise, we'd need to specify an explicit wildcard per model. And we can only match known models, i.e. up to v2.1 as long as we don't know whether v2.2 or v3.0 will have the same guest-exposed/emulation features (in which case we could've picked them instead), because otherwise we'd need a versioning mechanism like x86 has, to avoid guest-visible CPU features changing between releases once we do know what features new revisions have. Also, if we stopped modeling exact revisions, it may be a good idea to add properties to the PowerPCCPU to specify a particular major/minor revision within the wildcard. We may need to limit such properties to IBM's CPU models since apparently that scheme does not apply to all PVRs. CC'ing Scott and Stuart. Examples: -device POWER7+-powerpc64-cpu,major-revision=2,minor-revision=0 -device POWER7+-powerpc64-cpu,pvr=0x12345678 Regards, Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] target-ppc: Add POWER7+ CPU model 2013-08-05 8:24 ` Andreas Färber @ 2013-08-05 9:15 ` Benjamin Herrenschmidt 0 siblings, 0 replies; 6+ messages in thread From: Benjamin Herrenschmidt @ 2013-08-05 9:15 UTC (permalink / raw) To: Andreas Färber Cc: Anthony Liguori, Stuart Yoder, Alexey Kardashevskiy, qemu-devel, Alexander Graf, qemu-ppc, Scott Wood, Paul Mackerras On Mon, 2013-08-05 at 10:24 +0200, Andreas Färber wrote: > Ben has pointed out that POWER5++ PVR-wise was a POWER5+ v3.0+ or so > and, according to you, POWER5++ is not just a revision with > hardware-only changes and a new marketing name. So no, it's not always > just the upper 16 bits. Also 4xx uses a different bit split etc... that's why qemu should do like the kernel and use a mask/value pair to do the matching. Cheers, Ben. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] target-ppc: Add POWER7+ CPU model 2013-08-02 2:59 [Qemu-devel] [PATCH] target-ppc: Add POWER7+ CPU model Alexey Kardashevskiy 2013-08-02 16:14 ` Andreas Färber @ 2013-08-14 16:28 ` Anthony Liguori 1 sibling, 0 replies; 6+ messages in thread From: Anthony Liguori @ 2013-08-14 16:28 UTC (permalink / raw) To: Alexey Kardashevskiy, qemu-devel Cc: Anthony Liguori, Stuart Yoder, Alexander Graf, Scott Wood, qemu-ppc Applied. Thanks. Regards, Anthony Liguori ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-08-14 16:28 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-08-02 2:59 [Qemu-devel] [PATCH] target-ppc: Add POWER7+ CPU model Alexey Kardashevskiy 2013-08-02 16:14 ` Andreas Färber 2013-08-05 5:43 ` Paul Mackerras 2013-08-05 8:24 ` Andreas Färber 2013-08-05 9:15 ` Benjamin Herrenschmidt 2013-08-14 16:28 ` Anthony Liguori
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).