[Qemu-devel] [PATCH] target-ppc: Add POWER7+ CPU model

[Qemu-devel] [PATCH] target-ppc: Add POWER7+ CPU model

[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")
     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

[Andreas Färber]

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

Re: [Qemu-devel] [PATCH] target-ppc: Add POWER7+ CPU model

[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".

> 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

[Andreas Färber]

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

Re: [Qemu-devel] [PATCH] target-ppc: Add POWER7+ CPU model

[Benjamin Herrenschmidt]

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.

Re: [Qemu-devel] [PATCH] target-ppc: Add POWER7+ CPU model

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori