[Qemu-devel] [PATCH] target-ppc: Add POWER7+ CPU model
This patch adds CPU PVR definition for POWER7+. Signed-off-by: Alexey Kardashevskiy --- 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
Re: [Qemu-devel] [PATCH] target-ppc: Add POWER7+ CPU model
Applied. Thanks. Regards, Anthony Liguori
Re: [Qemu-devel] [PATCH] target-ppc: Add POWER7+ CPU model
Am 02.08.2013 04:59, schrieb Alexey Kardashevskiy: > This patch adds CPU PVR definition for POWER7+. > > Signed-off-by: Alexey Kardashevskiy > --- > 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
Re: [Qemu-devel] [PATCH] target-ppc: Add POWER7+ CPU model
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 > > --- > > 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.
Re: [Qemu-devel] [PATCH] target-ppc: Add POWER7+ CPU model
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 >>> --- >>> 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
Re: [Qemu-devel] [PATCH] target-ppc: Add POWER7+ CPU model
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.