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

[Paul Mackerras <paulus@samba.org>]

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.