[Qemu-devel] [PATCH] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[Qemu-devel] [PATCH] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[Laurent Vivier]

CPU_POWERPC_POWER9_DD1 is 0x004E0100, so this is the POWER9 v1.0.

When we run qemu on a POWER9 DD1 host, we must use either
"-cpu host" or "-cpu POWER9", but in the latter case it fails with

    Unable to find sPAPR CPU Core definition

because POWER9 DD1 doesn't appear in the list of known CPUs.

This patch fixes this by defining POWER9_v1.0 with POWER9 DD1
PVR instead of CPU_POWERPC_POWER9_BASE.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 target/ppc/cpu-models.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/ppc/cpu-models.c b/target/ppc/cpu-models.c
index 4d3e635..a22363c 100644
--- a/target/ppc/cpu-models.c
+++ b/target/ppc/cpu-models.c
@@ -1144,7 +1144,7 @@
     POWERPC_DEF("970_v2.2",      CPU_POWERPC_970_v22,                970,
                 "PowerPC 970 v2.2")
 
-    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_BASE,            POWER9,
+    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_DD1,             POWER9,
                 "POWER9 v1.0")
 
     POWERPC_DEF("970fx_v1.0",    CPU_POWERPC_970FX_v10,              970,
-- 
2.9.4

Re: [Qemu-devel] [PATCH] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[Thomas Huth]

On 22.06.2017 13:26, Laurent Vivier wrote:
> CPU_POWERPC_POWER9_DD1 is 0x004E0100, so this is the POWER9 v1.0.
> 
> When we run qemu on a POWER9 DD1 host, we must use either
> "-cpu host" or "-cpu POWER9", but in the latter case it fails with
> 
>     Unable to find sPAPR CPU Core definition
> 
> because POWER9 DD1 doesn't appear in the list of known CPUs.
> 
> This patch fixes this by defining POWER9_v1.0 with POWER9 DD1
> PVR instead of CPU_POWERPC_POWER9_BASE.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  target/ppc/cpu-models.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/ppc/cpu-models.c b/target/ppc/cpu-models.c
> index 4d3e635..a22363c 100644
> --- a/target/ppc/cpu-models.c
> +++ b/target/ppc/cpu-models.c
> @@ -1144,7 +1144,7 @@
>      POWERPC_DEF("970_v2.2",      CPU_POWERPC_970_v22,                970,
>                  "PowerPC 970 v2.2")
>  
> -    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_BASE,            POWER9,
> +    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_DD1,             POWER9,
>                  "POWER9 v1.0")
>  
>      POWERPC_DEF("970fx_v1.0",    CPU_POWERPC_970FX_v10,              970,
> 

I think this also makes sense for running in TCG mode to get a valid
real PVR there.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1870 bytes --]

On Thu, Jun 22, 2017 at 01:31:24PM +0200, Thomas Huth wrote:
> On 22.06.2017 13:26, Laurent Vivier wrote:
> > CPU_POWERPC_POWER9_DD1 is 0x004E0100, so this is the POWER9 v1.0.
> > 
> > When we run qemu on a POWER9 DD1 host, we must use either
> > "-cpu host" or "-cpu POWER9", but in the latter case it fails with
> > 
> >     Unable to find sPAPR CPU Core definition
> > 
> > because POWER9 DD1 doesn't appear in the list of known CPUs.
> > 
> > This patch fixes this by defining POWER9_v1.0 with POWER9 DD1
> > PVR instead of CPU_POWERPC_POWER9_BASE.
> > 
> > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > ---
> >  target/ppc/cpu-models.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/target/ppc/cpu-models.c b/target/ppc/cpu-models.c
> > index 4d3e635..a22363c 100644
> > --- a/target/ppc/cpu-models.c
> > +++ b/target/ppc/cpu-models.c
> > @@ -1144,7 +1144,7 @@
> >      POWERPC_DEF("970_v2.2",      CPU_POWERPC_970_v22,                970,
> >                  "PowerPC 970 v2.2")
> >  
> > -    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_BASE,            POWER9,
> > +    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_DD1,             POWER9,
> >                  "POWER9 v1.0")
> >  
> >      POWERPC_DEF("970fx_v1.0",    CPU_POWERPC_970FX_v10,              970,
> > 
> 
> I think this also makes sense for running in TCG mode to get a valid
> real PVR there.

I'm not so convinced.

IIUC, this will make TCG default (for now) to a DD1 POWER9.  That's a)
probably not what anyone wants - who'd select a buggy prototype and b)
not accurate - TCG does not implement DD1's bugs.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[Laurent Vivier]

On 23/06/2017 11:21, David Gibson wrote:
> On Thu, Jun 22, 2017 at 01:31:24PM +0200, Thomas Huth wrote:
>> On 22.06.2017 13:26, Laurent Vivier wrote:
>>> CPU_POWERPC_POWER9_DD1 is 0x004E0100, so this is the POWER9 v1.0.
>>>
>>> When we run qemu on a POWER9 DD1 host, we must use either
>>> "-cpu host" or "-cpu POWER9", but in the latter case it fails with
>>>
>>>     Unable to find sPAPR CPU Core definition
>>>
>>> because POWER9 DD1 doesn't appear in the list of known CPUs.
>>>
>>> This patch fixes this by defining POWER9_v1.0 with POWER9 DD1
>>> PVR instead of CPU_POWERPC_POWER9_BASE.
>>>
>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>> ---
>>>  target/ppc/cpu-models.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/target/ppc/cpu-models.c b/target/ppc/cpu-models.c
>>> index 4d3e635..a22363c 100644
>>> --- a/target/ppc/cpu-models.c
>>> +++ b/target/ppc/cpu-models.c
>>> @@ -1144,7 +1144,7 @@
>>>      POWERPC_DEF("970_v2.2",      CPU_POWERPC_970_v22,                970,
>>>                  "PowerPC 970 v2.2")
>>>  
>>> -    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_BASE,            POWER9,
>>> +    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_DD1,             POWER9,
>>>                  "POWER9 v1.0")
>>>  
>>>      POWERPC_DEF("970fx_v1.0",    CPU_POWERPC_970FX_v10,              970,
>>>
>>
>> I think this also makes sense for running in TCG mode to get a valid
>> real PVR there.
> 
> I'm not so convinced.
> 
> IIUC, this will make TCG default (for now) to a DD1 POWER9.  That's a)
> probably not what anyone wants - who'd select a buggy prototype and b)
> not accurate - TCG does not implement DD1's bugs.

According to the POWER8 user manual (I didn't fine the POWER9 one):

"3.6.3.1 Processor Version Register (PVR)

The processor revision level (PVR[16:31]) starts at x‘0100’, indicating
revision ‘1.0’. As revisions are made, bits [29:31] will indicate minor
revisions. Similarly, bits [20:23] indicate major changes."

POWER9 DD1 PVR is 0x004E0100, so this is really version 1.0 of the POWER9.

Perhaps we can define POWER9_v1.0 as CPU_POWERPC_POWER9_DD1, and
introduce a POWER9_v0.0 set to CPU_POWERPC_POWER9_BASE and define it as
the default one?

Laurent

Re: [Qemu-devel] [PATCH] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[Thomas Huth]

[-- Attachment #1: Type: text/plain, Size: 2079 bytes --]

On 23.06.2017 11:21, David Gibson wrote:
> On Thu, Jun 22, 2017 at 01:31:24PM +0200, Thomas Huth wrote:
>> On 22.06.2017 13:26, Laurent Vivier wrote:
>>> CPU_POWERPC_POWER9_DD1 is 0x004E0100, so this is the POWER9 v1.0.
>>>
>>> When we run qemu on a POWER9 DD1 host, we must use either
>>> "-cpu host" or "-cpu POWER9", but in the latter case it fails with
>>>
>>>     Unable to find sPAPR CPU Core definition
>>>
>>> because POWER9 DD1 doesn't appear in the list of known CPUs.
>>>
>>> This patch fixes this by defining POWER9_v1.0 with POWER9 DD1
>>> PVR instead of CPU_POWERPC_POWER9_BASE.
>>>
>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>> ---
>>>  target/ppc/cpu-models.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/target/ppc/cpu-models.c b/target/ppc/cpu-models.c
>>> index 4d3e635..a22363c 100644
>>> --- a/target/ppc/cpu-models.c
>>> +++ b/target/ppc/cpu-models.c
>>> @@ -1144,7 +1144,7 @@
>>>      POWERPC_DEF("970_v2.2",      CPU_POWERPC_970_v22,                970,
>>>                  "PowerPC 970 v2.2")
>>>  
>>> -    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_BASE,            POWER9,
>>> +    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_DD1,             POWER9,
>>>                  "POWER9 v1.0")
>>>  
>>>      POWERPC_DEF("970fx_v1.0",    CPU_POWERPC_970FX_v10,              970,
>>>
>>
>> I think this also makes sense for running in TCG mode to get a valid
>> real PVR there.
> 
> I'm not so convinced.
> 
> IIUC, this will make TCG default (for now) to a DD1 POWER9.  That's a)
> probably not what anyone wants - who'd select a buggy prototype and b)
> not accurate - TCG does not implement DD1's bugs.

But DD1 = v1.0. There is no real chip which is using
CPU_POWERPC_POWER9_BASE as PVR ... so using that is also not accurate,
and also likely not what users expect when the select "POWER9_v1.0", and
it just does not work as soon as KVM is enabled. So I think Laurent's
patch is the right way to go. Or do you have a better suggestion?

 Thomas


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[Suraj Jitindar Singh]

On Fri, 2017-06-23 at 18:05 +0200, Thomas Huth wrote:
> On 23.06.2017 11:21, David Gibson wrote:
> > On Thu, Jun 22, 2017 at 01:31:24PM +0200, Thomas Huth wrote:
> > > On 22.06.2017 13:26, Laurent Vivier wrote:
> > > > CPU_POWERPC_POWER9_DD1 is 0x004E0100, so this is the POWER9
> > > > v1.0.
> > > > 
> > > > When we run qemu on a POWER9 DD1 host, we must use either
> > > > "-cpu host" or "-cpu POWER9", but in the latter case it fails
> > > > with
> > > > 
> > > >     Unable to find sPAPR CPU Core definition
> > > > 
> > > > because POWER9 DD1 doesn't appear in the list of known CPUs.
> > > > 
> > > > This patch fixes this by defining POWER9_v1.0 with POWER9 DD1
> > > > PVR instead of CPU_POWERPC_POWER9_BASE.
> > > > 
> > > > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > > > ---
> > > >  target/ppc/cpu-models.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/target/ppc/cpu-models.c b/target/ppc/cpu-models.c
> > > > index 4d3e635..a22363c 100644
> > > > --- a/target/ppc/cpu-models.c
> > > > +++ b/target/ppc/cpu-models.c
> > > > @@ -1144,7 +1144,7 @@
> > > >      POWERPC_DEF("970_v2.2",      CPU_POWERPC_970_v22,         
> > > >        970,
> > > >                  "PowerPC 970 v2.2")
> > > >  
> > > > -    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_BASE,     
> > > >        POWER9,
> > > > +    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_DD1,      
> > > >        POWER9,
> > > >                  "POWER9 v1.0")
> > > >  
> > > >      POWERPC_DEF("970fx_v1.0",    CPU_POWERPC_970FX_v10,       
> > > >        970,
> > > > 
> > > 
> > > I think this also makes sense for running in TCG mode to get a
> > > valid
> > > real PVR there.
> > 
> > I'm not so convinced.
> > 
> > IIUC, this will make TCG default (for now) to a DD1 POWER9.  That's
> > a)
> > probably not what anyone wants - who'd select a buggy prototype and
> > b)
> > not accurate - TCG does not implement DD1's bugs.
> 
> But DD1 = v1.0. There is no real chip which is using
> CPU_POWERPC_POWER9_BASE as PVR ... so using that is also not
> accurate,
> and also likely not what users expect when the select "POWER9_v1.0",
> and
> it just does not work as soon as KVM is enabled. So I think Laurent's
> patch is the right way to go. Or do you have a better suggestion?
> 

I guess we have do decide what we want to be the primary behaviour when
someone puts -cpu POWER9 on the command line. Does that mean that they
want an ISAv3.0 compliant cpu or do they want a POWER9 cpu...

FWIW: Currently POWER8 is aliased to POWER8_v2.0 which afaik is the
last POWER8 revision. Should POWER9 just alias to the most recent
POWER9 cpu (currently DD1)? Then how would you specify an ISAv3.0
compliant processor?

TCG only implements a ISAv3 compliant POWER9 cpu so it doesn't make
sense, and should even refuse to boot with POWER9_DD1 since I think it
will fail to boot if we let it.

My personal preference would be to have POWER9 alias to the base and
the user have to explicitly select if they want a DD* version. But that
might frustrate/confuse people. Maybe we should have some table in
KVM_HV of allowable processor combinations so that if the user is on
ISA compliant hardware then they can specify any such cpu on the
command line and KVM_HV still start successfully.

>  Thomas
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[joserz]

On Fri, Jun 23, 2017 at 04:10:55PM +0200, Laurent Vivier wrote:
> On 23/06/2017 11:21, David Gibson wrote:
> > On Thu, Jun 22, 2017 at 01:31:24PM +0200, Thomas Huth wrote:
> >> On 22.06.2017 13:26, Laurent Vivier wrote:
> >>> CPU_POWERPC_POWER9_DD1 is 0x004E0100, so this is the POWER9 v1.0.
> >>>
> >>> When we run qemu on a POWER9 DD1 host, we must use either
> >>> "-cpu host" or "-cpu POWER9", but in the latter case it fails with
> >>>
> >>>     Unable to find sPAPR CPU Core definition
> >>>
> >>> because POWER9 DD1 doesn't appear in the list of known CPUs.
> >>>
> >>> This patch fixes this by defining POWER9_v1.0 with POWER9 DD1
> >>> PVR instead of CPU_POWERPC_POWER9_BASE.
> >>>
> >>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> >>> ---
> >>>  target/ppc/cpu-models.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/target/ppc/cpu-models.c b/target/ppc/cpu-models.c
> >>> index 4d3e635..a22363c 100644
> >>> --- a/target/ppc/cpu-models.c
> >>> +++ b/target/ppc/cpu-models.c
> >>> @@ -1144,7 +1144,7 @@
> >>>      POWERPC_DEF("970_v2.2",      CPU_POWERPC_970_v22,                970,
> >>>                  "PowerPC 970 v2.2")
> >>>  
> >>> -    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_BASE,            POWER9,
> >>> +    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_DD1,             POWER9,
> >>>                  "POWER9 v1.0")
> >>>  
> >>>      POWERPC_DEF("970fx_v1.0",    CPU_POWERPC_970FX_v10,              970,
> >>>
> >>
> >> I think this also makes sense for running in TCG mode to get a valid
> >> real PVR there.
> > 
> > I'm not so convinced.
> > 
> > IIUC, this will make TCG default (for now) to a DD1 POWER9.  That's a)
> > probably not what anyone wants - who'd select a buggy prototype and b)
> > not accurate - TCG does not implement DD1's bugs.
> 
> According to the POWER8 user manual (I didn't fine the POWER9 one):
> 
> "3.6.3.1 Processor Version Register (PVR)
> 
> The processor revision level (PVR[16:31]) starts at x‘0100’, indicating
> revision ‘1.0’. As revisions are made, bits [29:31] will indicate minor
> revisions. Similarly, bits [20:23] indicate major changes."
> 
> POWER9 DD1 PVR is 0x004E0100, so this is really version 1.0 of the POWER9.
> 
> Perhaps we can define POWER9_v1.0 as CPU_POWERPC_POWER9_DD1, and
> introduce a POWER9_v0.0 set to CPU_POWERPC_POWER9_BASE and define it as
> the default one?

I like the suggestion to set a v0.0 to CPU_POWERPC_POWER9_BASE. But, I
think we could have only that option, removing the
CPU_POWERPC_POWER9_DD1 entry. Then, we add the v2.0 (when ready) as the CPU
emulated by TCG.

To TCG, DD1 wouldn't be listed because it cannot be emulated today.

What do you think?

Ziviani

> 
> Laurent
> 
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[Thomas Huth]

On 28.06.2017 03:42, joserz@linux.vnet.ibm.com wrote:
> On Fri, Jun 23, 2017 at 04:10:55PM +0200, Laurent Vivier wrote:
>> On 23/06/2017 11:21, David Gibson wrote:
>>> On Thu, Jun 22, 2017 at 01:31:24PM +0200, Thomas Huth wrote:
>>>> On 22.06.2017 13:26, Laurent Vivier wrote:
>>>>> CPU_POWERPC_POWER9_DD1 is 0x004E0100, so this is the POWER9 v1.0.
>>>>>
>>>>> When we run qemu on a POWER9 DD1 host, we must use either
>>>>> "-cpu host" or "-cpu POWER9", but in the latter case it fails with
>>>>>
>>>>>     Unable to find sPAPR CPU Core definition
>>>>>
>>>>> because POWER9 DD1 doesn't appear in the list of known CPUs.
>>>>>
>>>>> This patch fixes this by defining POWER9_v1.0 with POWER9 DD1
>>>>> PVR instead of CPU_POWERPC_POWER9_BASE.
>>>>>
>>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>>>> ---
>>>>>  target/ppc/cpu-models.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/target/ppc/cpu-models.c b/target/ppc/cpu-models.c
>>>>> index 4d3e635..a22363c 100644
>>>>> --- a/target/ppc/cpu-models.c
>>>>> +++ b/target/ppc/cpu-models.c
>>>>> @@ -1144,7 +1144,7 @@
>>>>>      POWERPC_DEF("970_v2.2",      CPU_POWERPC_970_v22,                970,
>>>>>                  "PowerPC 970 v2.2")
>>>>>  
>>>>> -    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_BASE,            POWER9,
>>>>> +    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_DD1,             POWER9,
>>>>>                  "POWER9 v1.0")
>>>>>  
>>>>>      POWERPC_DEF("970fx_v1.0",    CPU_POWERPC_970FX_v10,              970,
>>>>>
>>>>
>>>> I think this also makes sense for running in TCG mode to get a valid
>>>> real PVR there.
>>>
>>> I'm not so convinced.
>>>
>>> IIUC, this will make TCG default (for now) to a DD1 POWER9.  That's a)
>>> probably not what anyone wants - who'd select a buggy prototype and b)
>>> not accurate - TCG does not implement DD1's bugs.
>>
>> According to the POWER8 user manual (I didn't fine the POWER9 one):
>>
>> "3.6.3.1 Processor Version Register (PVR)
>>
>> The processor revision level (PVR[16:31]) starts at x‘0100’, indicating
>> revision ‘1.0’. As revisions are made, bits [29:31] will indicate minor
>> revisions. Similarly, bits [20:23] indicate major changes."
>>
>> POWER9 DD1 PVR is 0x004E0100, so this is really version 1.0 of the POWER9.
>>
>> Perhaps we can define POWER9_v1.0 as CPU_POWERPC_POWER9_DD1, and
>> introduce a POWER9_v0.0 set to CPU_POWERPC_POWER9_BASE and define it as
>> the default one?
> 
> I like the suggestion to set a v0.0 to CPU_POWERPC_POWER9_BASE. But, I
> think we could have only that option, removing the
> CPU_POWERPC_POWER9_DD1 entry.
I really dislike the idea of having a CPU called "v0.0" ... we do not
have this for any other CPU generation, and it sounds like it could be
very confusing for the users (you'd need to document somewhere what the
v0.0 exactly means). If we really want to go this way, I think we should
name it "POWER9-generic" or "PowerISA-3.0" or something similar instead.

Or does somebody already know the exact PVR for DD2? If so, we could
simply add a POWER9_v2.0 CPU already and let the POWER9 alias point to
that version instead.

 Thomas

Re: [Qemu-devel] [Qemu-ppc] [PATCH] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3870 bytes --]

On Wed, Jun 28, 2017 at 09:09:24AM +0200, Thomas Huth wrote:
> On 28.06.2017 03:42, joserz@linux.vnet.ibm.com wrote:
> > On Fri, Jun 23, 2017 at 04:10:55PM +0200, Laurent Vivier wrote:
> >> On 23/06/2017 11:21, David Gibson wrote:
> >>> On Thu, Jun 22, 2017 at 01:31:24PM +0200, Thomas Huth wrote:
> >>>> On 22.06.2017 13:26, Laurent Vivier wrote:
> >>>>> CPU_POWERPC_POWER9_DD1 is 0x004E0100, so this is the POWER9 v1.0.
> >>>>>
> >>>>> When we run qemu on a POWER9 DD1 host, we must use either
> >>>>> "-cpu host" or "-cpu POWER9", but in the latter case it fails with
> >>>>>
> >>>>>     Unable to find sPAPR CPU Core definition
> >>>>>
> >>>>> because POWER9 DD1 doesn't appear in the list of known CPUs.
> >>>>>
> >>>>> This patch fixes this by defining POWER9_v1.0 with POWER9 DD1
> >>>>> PVR instead of CPU_POWERPC_POWER9_BASE.
> >>>>>
> >>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> >>>>> ---
> >>>>>  target/ppc/cpu-models.c | 2 +-
> >>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/target/ppc/cpu-models.c b/target/ppc/cpu-models.c
> >>>>> index 4d3e635..a22363c 100644
> >>>>> --- a/target/ppc/cpu-models.c
> >>>>> +++ b/target/ppc/cpu-models.c
> >>>>> @@ -1144,7 +1144,7 @@
> >>>>>      POWERPC_DEF("970_v2.2",      CPU_POWERPC_970_v22,                970,
> >>>>>                  "PowerPC 970 v2.2")
> >>>>>  
> >>>>> -    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_BASE,            POWER9,
> >>>>> +    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_DD1,             POWER9,
> >>>>>                  "POWER9 v1.0")
> >>>>>  
> >>>>>      POWERPC_DEF("970fx_v1.0",    CPU_POWERPC_970FX_v10,              970,
> >>>>>
> >>>>
> >>>> I think this also makes sense for running in TCG mode to get a valid
> >>>> real PVR there.
> >>>
> >>> I'm not so convinced.
> >>>
> >>> IIUC, this will make TCG default (for now) to a DD1 POWER9.  That's a)
> >>> probably not what anyone wants - who'd select a buggy prototype and b)
> >>> not accurate - TCG does not implement DD1's bugs.
> >>
> >> According to the POWER8 user manual (I didn't fine the POWER9 one):
> >>
> >> "3.6.3.1 Processor Version Register (PVR)
> >>
> >> The processor revision level (PVR[16:31]) starts at x‘0100’, indicating
> >> revision ‘1.0’. As revisions are made, bits [29:31] will indicate minor
> >> revisions. Similarly, bits [20:23] indicate major changes."
> >>
> >> POWER9 DD1 PVR is 0x004E0100, so this is really version 1.0 of the POWER9.
> >>
> >> Perhaps we can define POWER9_v1.0 as CPU_POWERPC_POWER9_DD1, and
> >> introduce a POWER9_v0.0 set to CPU_POWERPC_POWER9_BASE and define it as
> >> the default one?
> > 
> > I like the suggestion to set a v0.0 to CPU_POWERPC_POWER9_BASE. But, I
> > think we could have only that option, removing the
> > CPU_POWERPC_POWER9_DD1 entry.
> I really dislike the idea of having a CPU called "v0.0" ... we do not
> have this for any other CPU generation, and it sounds like it could be
> very confusing for the users (you'd need to document somewhere what the
> v0.0 exactly means). If we really want to go this way, I think we should
> name it "POWER9-generic" or "PowerISA-3.0" or something similar instead.
> 
> Or does somebody already know the exact PVR for DD2? If so, we could
> simply add a POWER9_v2.0 CPU already and let the POWER9 alias point to
> that version instead.

Yes, I think that's a better idea.  I don't know the DD2 PVR, but I'm
pretty sure we should be able to find out from someone at IBM.

I've CCed Sam & Suraj - can you ask Mikey or someone what the PVR
value for DD2.0 will be?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[Cédric Le Goater]

On 06/28/2017 10:18 AM, David Gibson wrote:
> On Wed, Jun 28, 2017 at 09:09:24AM +0200, Thomas Huth wrote:
>> On 28.06.2017 03:42, joserz@linux.vnet.ibm.com wrote:
>>> On Fri, Jun 23, 2017 at 04:10:55PM +0200, Laurent Vivier wrote:
>>>> On 23/06/2017 11:21, David Gibson wrote:
>>>>> On Thu, Jun 22, 2017 at 01:31:24PM +0200, Thomas Huth wrote:
>>>>>> On 22.06.2017 13:26, Laurent Vivier wrote:
>>>>>>> CPU_POWERPC_POWER9_DD1 is 0x004E0100, so this is the POWER9 v1.0.
>>>>>>>
>>>>>>> When we run qemu on a POWER9 DD1 host, we must use either
>>>>>>> "-cpu host" or "-cpu POWER9", but in the latter case it fails with
>>>>>>>
>>>>>>>     Unable to find sPAPR CPU Core definition
>>>>>>>
>>>>>>> because POWER9 DD1 doesn't appear in the list of known CPUs.
>>>>>>>
>>>>>>> This patch fixes this by defining POWER9_v1.0 with POWER9 DD1
>>>>>>> PVR instead of CPU_POWERPC_POWER9_BASE.
>>>>>>>
>>>>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>>>>>> ---
>>>>>>>  target/ppc/cpu-models.c | 2 +-
>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/target/ppc/cpu-models.c b/target/ppc/cpu-models.c
>>>>>>> index 4d3e635..a22363c 100644
>>>>>>> --- a/target/ppc/cpu-models.c
>>>>>>> +++ b/target/ppc/cpu-models.c
>>>>>>> @@ -1144,7 +1144,7 @@
>>>>>>>      POWERPC_DEF("970_v2.2",      CPU_POWERPC_970_v22,                970,
>>>>>>>                  "PowerPC 970 v2.2")
>>>>>>>  
>>>>>>> -    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_BASE,            POWER9,
>>>>>>> +    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_DD1,             POWER9,
>>>>>>>                  "POWER9 v1.0")
>>>>>>>  
>>>>>>>      POWERPC_DEF("970fx_v1.0",    CPU_POWERPC_970FX_v10,              970,
>>>>>>>
>>>>>>
>>>>>> I think this also makes sense for running in TCG mode to get a valid
>>>>>> real PVR there.
>>>>>
>>>>> I'm not so convinced.
>>>>>
>>>>> IIUC, this will make TCG default (for now) to a DD1 POWER9.  That's a)
>>>>> probably not what anyone wants - who'd select a buggy prototype and b)
>>>>> not accurate - TCG does not implement DD1's bugs.
>>>>
>>>> According to the POWER8 user manual (I didn't fine the POWER9 one):
>>>>
>>>> "3.6.3.1 Processor Version Register (PVR)
>>>>
>>>> The processor revision level (PVR[16:31]) starts at x‘0100’, indicating
>>>> revision ‘1.0’. As revisions are made, bits [29:31] will indicate minor
>>>> revisions. Similarly, bits [20:23] indicate major changes."
>>>>
>>>> POWER9 DD1 PVR is 0x004E0100, so this is really version 1.0 of the POWER9.
>>>>
>>>> Perhaps we can define POWER9_v1.0 as CPU_POWERPC_POWER9_DD1, and
>>>> introduce a POWER9_v0.0 set to CPU_POWERPC_POWER9_BASE and define it as
>>>> the default one?
>>>
>>> I like the suggestion to set a v0.0 to CPU_POWERPC_POWER9_BASE. But, I
>>> think we could have only that option, removing the
>>> CPU_POWERPC_POWER9_DD1 entry.
>> I really dislike the idea of having a CPU called "v0.0" ... we do not
>> have this for any other CPU generation, and it sounds like it could be
>> very confusing for the users (you'd need to document somewhere what the
>> v0.0 exactly means). If we really want to go this way, I think we should
>> name it "POWER9-generic" or "PowerISA-3.0" or something similar instead.
>>
>> Or does somebody already know the exact PVR for DD2? If so, we could
>> simply add a POWER9_v2.0 CPU already and let the POWER9 alias point to
>> that version instead.
> 
> Yes, I think that's a better idea.  I don't know the DD2 PVR, but I'm
> pretty sure we should be able to find out from someone at IBM.
> 
> I've CCed Sam & Suraj - can you ask Mikey or someone what the PVR
> value for DD2.0 will be?

I would expect something like :

 0x200D104980000000UL; /* P9 Nimbus DD2.0 */

C.

Re: [Qemu-devel] [Qemu-ppc] [PATCH] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[Laurent Vivier]

On 28/06/2017 11:11, Cédric Le Goater wrote:
> On 06/28/2017 10:18 AM, David Gibson wrote:
>> On Wed, Jun 28, 2017 at 09:09:24AM +0200, Thomas Huth wrote:
>>> On 28.06.2017 03:42, joserz@linux.vnet.ibm.com wrote:
>>>> On Fri, Jun 23, 2017 at 04:10:55PM +0200, Laurent Vivier wrote:
>>>>> On 23/06/2017 11:21, David Gibson wrote:
>>>>>> On Thu, Jun 22, 2017 at 01:31:24PM +0200, Thomas Huth wrote:
>>>>>>> On 22.06.2017 13:26, Laurent Vivier wrote:
>>>>>>>> CPU_POWERPC_POWER9_DD1 is 0x004E0100, so this is the POWER9 v1.0.
>>>>>>>>
>>>>>>>> When we run qemu on a POWER9 DD1 host, we must use either
>>>>>>>> "-cpu host" or "-cpu POWER9", but in the latter case it fails with
>>>>>>>>
>>>>>>>>     Unable to find sPAPR CPU Core definition
>>>>>>>>
>>>>>>>> because POWER9 DD1 doesn't appear in the list of known CPUs.
>>>>>>>>
>>>>>>>> This patch fixes this by defining POWER9_v1.0 with POWER9 DD1
>>>>>>>> PVR instead of CPU_POWERPC_POWER9_BASE.
>>>>>>>>
>>>>>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>>>>>>> ---
>>>>>>>>  target/ppc/cpu-models.c | 2 +-
>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/target/ppc/cpu-models.c b/target/ppc/cpu-models.c
>>>>>>>> index 4d3e635..a22363c 100644
>>>>>>>> --- a/target/ppc/cpu-models.c
>>>>>>>> +++ b/target/ppc/cpu-models.c
>>>>>>>> @@ -1144,7 +1144,7 @@
>>>>>>>>      POWERPC_DEF("970_v2.2",      CPU_POWERPC_970_v22,                970,
>>>>>>>>                  "PowerPC 970 v2.2")
>>>>>>>>  
>>>>>>>> -    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_BASE,            POWER9,
>>>>>>>> +    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_DD1,             POWER9,
>>>>>>>>                  "POWER9 v1.0")
>>>>>>>>  
>>>>>>>>      POWERPC_DEF("970fx_v1.0",    CPU_POWERPC_970FX_v10,              970,
>>>>>>>>
>>>>>>>
>>>>>>> I think this also makes sense for running in TCG mode to get a valid
>>>>>>> real PVR there.
>>>>>>
>>>>>> I'm not so convinced.
>>>>>>
>>>>>> IIUC, this will make TCG default (for now) to a DD1 POWER9.  That's a)
>>>>>> probably not what anyone wants - who'd select a buggy prototype and b)
>>>>>> not accurate - TCG does not implement DD1's bugs.
>>>>>
>>>>> According to the POWER8 user manual (I didn't fine the POWER9 one):
>>>>>
>>>>> "3.6.3.1 Processor Version Register (PVR)
>>>>>
>>>>> The processor revision level (PVR[16:31]) starts at x‘0100’, indicating
>>>>> revision ‘1.0’. As revisions are made, bits [29:31] will indicate minor
>>>>> revisions. Similarly, bits [20:23] indicate major changes."
>>>>>
>>>>> POWER9 DD1 PVR is 0x004E0100, so this is really version 1.0 of the POWER9.
>>>>>
>>>>> Perhaps we can define POWER9_v1.0 as CPU_POWERPC_POWER9_DD1, and
>>>>> introduce a POWER9_v0.0 set to CPU_POWERPC_POWER9_BASE and define it as
>>>>> the default one?
>>>>
>>>> I like the suggestion to set a v0.0 to CPU_POWERPC_POWER9_BASE. But, I
>>>> think we could have only that option, removing the
>>>> CPU_POWERPC_POWER9_DD1 entry.
>>> I really dislike the idea of having a CPU called "v0.0" ... we do not
>>> have this for any other CPU generation, and it sounds like it could be
>>> very confusing for the users (you'd need to document somewhere what the
>>> v0.0 exactly means). If we really want to go this way, I think we should
>>> name it "POWER9-generic" or "PowerISA-3.0" or something similar instead.
>>>
>>> Or does somebody already know the exact PVR for DD2? If so, we could
>>> simply add a POWER9_v2.0 CPU already and let the POWER9 alias point to
>>> that version instead.
>>
>> Yes, I think that's a better idea.  I don't know the DD2 PVR, but I'm
>> pretty sure we should be able to find out from someone at IBM.
>>
>> I've CCed Sam & Suraj - can you ask Mikey or someone what the PVR
>> value for DD2.0 will be?
> 
> I would expect something like :
> 
>  0x200D104980000000UL; /* P9 Nimbus DD2.0 */


I would expect something like 0x004Exxxx.

Laurent

Re: [Qemu-devel] [Qemu-ppc] [PATCH] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[Cédric Le Goater]

On 06/28/2017 11:18 AM, Laurent Vivier wrote:
> On 28/06/2017 11:11, Cédric Le Goater wrote:
>> On 06/28/2017 10:18 AM, David Gibson wrote:
>>> On Wed, Jun 28, 2017 at 09:09:24AM +0200, Thomas Huth wrote:
>>>> On 28.06.2017 03:42, joserz@linux.vnet.ibm.com wrote:
>>>>> On Fri, Jun 23, 2017 at 04:10:55PM +0200, Laurent Vivier wrote:
>>>>>> On 23/06/2017 11:21, David Gibson wrote:
>>>>>>> On Thu, Jun 22, 2017 at 01:31:24PM +0200, Thomas Huth wrote:
>>>>>>>> On 22.06.2017 13:26, Laurent Vivier wrote:
>>>>>>>>> CPU_POWERPC_POWER9_DD1 is 0x004E0100, so this is the POWER9 v1.0.
>>>>>>>>>
>>>>>>>>> When we run qemu on a POWER9 DD1 host, we must use either
>>>>>>>>> "-cpu host" or "-cpu POWER9", but in the latter case it fails with
>>>>>>>>>
>>>>>>>>>     Unable to find sPAPR CPU Core definition
>>>>>>>>>
>>>>>>>>> because POWER9 DD1 doesn't appear in the list of known CPUs.
>>>>>>>>>
>>>>>>>>> This patch fixes this by defining POWER9_v1.0 with POWER9 DD1
>>>>>>>>> PVR instead of CPU_POWERPC_POWER9_BASE.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>>>>>>>> ---
>>>>>>>>>  target/ppc/cpu-models.c | 2 +-
>>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/target/ppc/cpu-models.c b/target/ppc/cpu-models.c
>>>>>>>>> index 4d3e635..a22363c 100644
>>>>>>>>> --- a/target/ppc/cpu-models.c
>>>>>>>>> +++ b/target/ppc/cpu-models.c
>>>>>>>>> @@ -1144,7 +1144,7 @@
>>>>>>>>>      POWERPC_DEF("970_v2.2",      CPU_POWERPC_970_v22,                970,
>>>>>>>>>                  "PowerPC 970 v2.2")
>>>>>>>>>  
>>>>>>>>> -    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_BASE,            POWER9,
>>>>>>>>> +    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_DD1,             POWER9,
>>>>>>>>>                  "POWER9 v1.0")
>>>>>>>>>  
>>>>>>>>>      POWERPC_DEF("970fx_v1.0",    CPU_POWERPC_970FX_v10,              970,
>>>>>>>>>
>>>>>>>>
>>>>>>>> I think this also makes sense for running in TCG mode to get a valid
>>>>>>>> real PVR there.
>>>>>>>
>>>>>>> I'm not so convinced.
>>>>>>>
>>>>>>> IIUC, this will make TCG default (for now) to a DD1 POWER9.  That's a)
>>>>>>> probably not what anyone wants - who'd select a buggy prototype and b)
>>>>>>> not accurate - TCG does not implement DD1's bugs.
>>>>>>
>>>>>> According to the POWER8 user manual (I didn't fine the POWER9 one):
>>>>>>
>>>>>> "3.6.3.1 Processor Version Register (PVR)
>>>>>>
>>>>>> The processor revision level (PVR[16:31]) starts at x‘0100’, indicating
>>>>>> revision ‘1.0’. As revisions are made, bits [29:31] will indicate minor
>>>>>> revisions. Similarly, bits [20:23] indicate major changes."
>>>>>>
>>>>>> POWER9 DD1 PVR is 0x004E0100, so this is really version 1.0 of the POWER9.
>>>>>>
>>>>>> Perhaps we can define POWER9_v1.0 as CPU_POWERPC_POWER9_DD1, and
>>>>>> introduce a POWER9_v0.0 set to CPU_POWERPC_POWER9_BASE and define it as
>>>>>> the default one?
>>>>>
>>>>> I like the suggestion to set a v0.0 to CPU_POWERPC_POWER9_BASE. But, I
>>>>> think we could have only that option, removing the
>>>>> CPU_POWERPC_POWER9_DD1 entry.
>>>> I really dislike the idea of having a CPU called "v0.0" ... we do not
>>>> have this for any other CPU generation, and it sounds like it could be
>>>> very confusing for the users (you'd need to document somewhere what the
>>>> v0.0 exactly means). If we really want to go this way, I think we should
>>>> name it "POWER9-generic" or "PowerISA-3.0" or something similar instead.
>>>>
>>>> Or does somebody already know the exact PVR for DD2? If so, we could
>>>> simply add a POWER9_v2.0 CPU already and let the POWER9 alias point to
>>>> that version instead.
>>>
>>> Yes, I think that's a better idea.  I don't know the DD2 PVR, but I'm
>>> pretty sure we should be able to find out from someone at IBM.
>>>
>>> I've CCed Sam & Suraj - can you ask Mikey or someone what the PVR
>>> value for DD2.0 will be?
>>
>> I would expect something like :
>>
>>  0x200D104980000000UL; /* P9 Nimbus DD2.0 */
> 
> 
> I would expect something like 0x004Exxxx.

ah yes, I am mistaking the PVR and the CFAM ID. 

C. 
 

Re: [Qemu-devel] [Qemu-ppc] [PATCH] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[Laurent Vivier]

On 28/06/2017 03:42, joserz@linux.vnet.ibm.com wrote:
> On Fri, Jun 23, 2017 at 04:10:55PM +0200, Laurent Vivier wrote:
>> On 23/06/2017 11:21, David Gibson wrote:
>>> On Thu, Jun 22, 2017 at 01:31:24PM +0200, Thomas Huth wrote:
>>>> On 22.06.2017 13:26, Laurent Vivier wrote:
>>>>> CPU_POWERPC_POWER9_DD1 is 0x004E0100, so this is the POWER9 v1.0.
>>>>>
>>>>> When we run qemu on a POWER9 DD1 host, we must use either
>>>>> "-cpu host" or "-cpu POWER9", but in the latter case it fails with
>>>>>
>>>>>     Unable to find sPAPR CPU Core definition
>>>>>
>>>>> because POWER9 DD1 doesn't appear in the list of known CPUs.
>>>>>
>>>>> This patch fixes this by defining POWER9_v1.0 with POWER9 DD1
>>>>> PVR instead of CPU_POWERPC_POWER9_BASE.
>>>>>
>>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>>>> ---
>>>>>  target/ppc/cpu-models.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/target/ppc/cpu-models.c b/target/ppc/cpu-models.c
>>>>> index 4d3e635..a22363c 100644
>>>>> --- a/target/ppc/cpu-models.c
>>>>> +++ b/target/ppc/cpu-models.c
>>>>> @@ -1144,7 +1144,7 @@
>>>>>      POWERPC_DEF("970_v2.2",      CPU_POWERPC_970_v22,                970,
>>>>>                  "PowerPC 970 v2.2")
>>>>>  
>>>>> -    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_BASE,            POWER9,
>>>>> +    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_DD1,             POWER9,
>>>>>                  "POWER9 v1.0")
>>>>>  
>>>>>      POWERPC_DEF("970fx_v1.0",    CPU_POWERPC_970FX_v10,              970,
>>>>>
>>>>
>>>> I think this also makes sense for running in TCG mode to get a valid
>>>> real PVR there.
>>>
>>> I'm not so convinced.
>>>
>>> IIUC, this will make TCG default (for now) to a DD1 POWER9.  That's a)
>>> probably not what anyone wants - who'd select a buggy prototype and b)
>>> not accurate - TCG does not implement DD1's bugs.
>>
>> According to the POWER8 user manual (I didn't fine the POWER9 one):
>>
>> "3.6.3.1 Processor Version Register (PVR)
>>
>> The processor revision level (PVR[16:31]) starts at x‘0100’, indicating
>> revision ‘1.0’. As revisions are made, bits [29:31] will indicate minor
>> revisions. Similarly, bits [20:23] indicate major changes."
>>
>> POWER9 DD1 PVR is 0x004E0100, so this is really version 1.0 of the POWER9.
>>
>> Perhaps we can define POWER9_v1.0 as CPU_POWERPC_POWER9_DD1, and
>> introduce a POWER9_v0.0 set to CPU_POWERPC_POWER9_BASE and define it as
>> the default one?
> 
> I like the suggestion to set a v0.0 to CPU_POWERPC_POWER9_BASE. But, I
> think we could have only that option, removing the
> CPU_POWERPC_POWER9_DD1 entry. Then, we add the v2.0 (when ready) as the CPU
> emulated by TCG.
> 
> To TCG, DD1 wouldn't be listed because it cannot be emulated today.
> 
> What do you think?

We need the POWER9 DD1 in the list to be able to start QEMU on DD1 host
with "-cpu POWER9" (it works with "-cpu host"). The real question is to
what we should set the default CPU when it is not provided on the CLI.

I agree that using POWER9 DD1 to boot a kernel with TCG will confuse the
kernel.

we could set POWER9 DD1 PVR to define the "POWER9_v1.0", and add a
"POWER9_tcg" (?) set to CPU_POWERPC_POWER9_BASE to use with tcg until we
have the PVR of a public release of POWER9?

Thanks,
Laurent

Re: [Qemu-devel] [Qemu-ppc] [PATCH] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 4636 bytes --]

On Wed, 28 Jun 2017 12:23:06 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> On 06/28/2017 11:18 AM, Laurent Vivier wrote:
> > On 28/06/2017 11:11, Cédric Le Goater wrote:  
> >> On 06/28/2017 10:18 AM, David Gibson wrote:  
> >>> On Wed, Jun 28, 2017 at 09:09:24AM +0200, Thomas Huth wrote:  
> >>>> On 28.06.2017 03:42, joserz@linux.vnet.ibm.com wrote:  
> >>>>> On Fri, Jun 23, 2017 at 04:10:55PM +0200, Laurent Vivier wrote:  
> >>>>>> On 23/06/2017 11:21, David Gibson wrote:  
> >>>>>>> On Thu, Jun 22, 2017 at 01:31:24PM +0200, Thomas Huth wrote:  
> >>>>>>>> On 22.06.2017 13:26, Laurent Vivier wrote:  
> >>>>>>>>> CPU_POWERPC_POWER9_DD1 is 0x004E0100, so this is the POWER9 v1.0.
> >>>>>>>>>
> >>>>>>>>> When we run qemu on a POWER9 DD1 host, we must use either
> >>>>>>>>> "-cpu host" or "-cpu POWER9", but in the latter case it fails with
> >>>>>>>>>
> >>>>>>>>>     Unable to find sPAPR CPU Core definition
> >>>>>>>>>
> >>>>>>>>> because POWER9 DD1 doesn't appear in the list of known CPUs.
> >>>>>>>>>
> >>>>>>>>> This patch fixes this by defining POWER9_v1.0 with POWER9 DD1
> >>>>>>>>> PVR instead of CPU_POWERPC_POWER9_BASE.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> >>>>>>>>> ---
> >>>>>>>>>  target/ppc/cpu-models.c | 2 +-
> >>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/target/ppc/cpu-models.c b/target/ppc/cpu-models.c
> >>>>>>>>> index 4d3e635..a22363c 100644
> >>>>>>>>> --- a/target/ppc/cpu-models.c
> >>>>>>>>> +++ b/target/ppc/cpu-models.c
> >>>>>>>>> @@ -1144,7 +1144,7 @@
> >>>>>>>>>      POWERPC_DEF("970_v2.2",      CPU_POWERPC_970_v22,                970,
> >>>>>>>>>                  "PowerPC 970 v2.2")
> >>>>>>>>>  
> >>>>>>>>> -    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_BASE,            POWER9,
> >>>>>>>>> +    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_DD1,             POWER9,
> >>>>>>>>>                  "POWER9 v1.0")
> >>>>>>>>>  
> >>>>>>>>>      POWERPC_DEF("970fx_v1.0",    CPU_POWERPC_970FX_v10,              970,
> >>>>>>>>>  
> >>>>>>>>
> >>>>>>>> I think this also makes sense for running in TCG mode to get a valid
> >>>>>>>> real PVR there.  
> >>>>>>>
> >>>>>>> I'm not so convinced.
> >>>>>>>
> >>>>>>> IIUC, this will make TCG default (for now) to a DD1 POWER9.  That's a)
> >>>>>>> probably not what anyone wants - who'd select a buggy prototype and b)
> >>>>>>> not accurate - TCG does not implement DD1's bugs.  
> >>>>>>
> >>>>>> According to the POWER8 user manual (I didn't fine the POWER9 one):
> >>>>>>
> >>>>>> "3.6.3.1 Processor Version Register (PVR)
> >>>>>>
> >>>>>> The processor revision level (PVR[16:31]) starts at x‘0100’, indicating
> >>>>>> revision ‘1.0’. As revisions are made, bits [29:31] will indicate minor
> >>>>>> revisions. Similarly, bits [20:23] indicate major changes."
> >>>>>>
> >>>>>> POWER9 DD1 PVR is 0x004E0100, so this is really version 1.0 of the POWER9.
> >>>>>>
> >>>>>> Perhaps we can define POWER9_v1.0 as CPU_POWERPC_POWER9_DD1, and
> >>>>>> introduce a POWER9_v0.0 set to CPU_POWERPC_POWER9_BASE and define it as
> >>>>>> the default one?  
> >>>>>
> >>>>> I like the suggestion to set a v0.0 to CPU_POWERPC_POWER9_BASE. But, I
> >>>>> think we could have only that option, removing the
> >>>>> CPU_POWERPC_POWER9_DD1 entry.  
> >>>> I really dislike the idea of having a CPU called "v0.0" ... we do not
> >>>> have this for any other CPU generation, and it sounds like it could be
> >>>> very confusing for the users (you'd need to document somewhere what the
> >>>> v0.0 exactly means). If we really want to go this way, I think we should
> >>>> name it "POWER9-generic" or "PowerISA-3.0" or something similar instead.
> >>>>
> >>>> Or does somebody already know the exact PVR for DD2? If so, we could
> >>>> simply add a POWER9_v2.0 CPU already and let the POWER9 alias point to
> >>>> that version instead.  
> >>>
> >>> Yes, I think that's a better idea.  I don't know the DD2 PVR, but I'm
> >>> pretty sure we should be able to find out from someone at IBM.
> >>>
> >>> I've CCed Sam & Suraj - can you ask Mikey or someone what the PVR
> >>> value for DD2.0 will be?  
> >>
> >> I would expect something like :
> >>
> >>  0x200D104980000000UL; /* P9 Nimbus DD2.0 */  
> > 
> > 
> > I would expect something like 0x004Exxxx.  
> 
> ah yes, I am mistaking the PVR and the CFAM ID. 
> 
> C. 
>  

According to https://patchwork.ozlabs.org/patch/776052/

POWER9 DD2's PVR is expected to be 0x004e1200

Cheers,

--
Greg

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[Laurent Vivier]

On 28/06/2017 13:59, Greg Kurz wrote:
> On Wed, 28 Jun 2017 12:23:06 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> On 06/28/2017 11:18 AM, Laurent Vivier wrote:
>>> On 28/06/2017 11:11, Cédric Le Goater wrote:  
>>>> On 06/28/2017 10:18 AM, David Gibson wrote:  
>>>>> On Wed, Jun 28, 2017 at 09:09:24AM +0200, Thomas Huth wrote:  
>>>>>> On 28.06.2017 03:42, joserz@linux.vnet.ibm.com wrote:  
>>>>>>> On Fri, Jun 23, 2017 at 04:10:55PM +0200, Laurent Vivier wrote:  
>>>>>>>> On 23/06/2017 11:21, David Gibson wrote:  
>>>>>>>>> On Thu, Jun 22, 2017 at 01:31:24PM +0200, Thomas Huth wrote:  
>>>>>>>>>> On 22.06.2017 13:26, Laurent Vivier wrote:  
>>>>>>>>>>> CPU_POWERPC_POWER9_DD1 is 0x004E0100, so this is the POWER9 v1.0.
>>>>>>>>>>>
>>>>>>>>>>> When we run qemu on a POWER9 DD1 host, we must use either
>>>>>>>>>>> "-cpu host" or "-cpu POWER9", but in the latter case it fails with
>>>>>>>>>>>
>>>>>>>>>>>     Unable to find sPAPR CPU Core definition
>>>>>>>>>>>
>>>>>>>>>>> because POWER9 DD1 doesn't appear in the list of known CPUs.
>>>>>>>>>>>
>>>>>>>>>>> This patch fixes this by defining POWER9_v1.0 with POWER9 DD1
>>>>>>>>>>> PVR instead of CPU_POWERPC_POWER9_BASE.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>>>>>>>>>> ---
>>>>>>>>>>>  target/ppc/cpu-models.c | 2 +-
>>>>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/target/ppc/cpu-models.c b/target/ppc/cpu-models.c
>>>>>>>>>>> index 4d3e635..a22363c 100644
>>>>>>>>>>> --- a/target/ppc/cpu-models.c
>>>>>>>>>>> +++ b/target/ppc/cpu-models.c
>>>>>>>>>>> @@ -1144,7 +1144,7 @@
>>>>>>>>>>>      POWERPC_DEF("970_v2.2",      CPU_POWERPC_970_v22,                970,
>>>>>>>>>>>                  "PowerPC 970 v2.2")
>>>>>>>>>>>  
>>>>>>>>>>> -    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_BASE,            POWER9,
>>>>>>>>>>> +    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_DD1,             POWER9,
>>>>>>>>>>>                  "POWER9 v1.0")
>>>>>>>>>>>  
>>>>>>>>>>>      POWERPC_DEF("970fx_v1.0",    CPU_POWERPC_970FX_v10,              970,
>>>>>>>>>>>  
>>>>>>>>>>
>>>>>>>>>> I think this also makes sense for running in TCG mode to get a valid
>>>>>>>>>> real PVR there.  
>>>>>>>>>
>>>>>>>>> I'm not so convinced.
>>>>>>>>>
>>>>>>>>> IIUC, this will make TCG default (for now) to a DD1 POWER9.  That's a)
>>>>>>>>> probably not what anyone wants - who'd select a buggy prototype and b)
>>>>>>>>> not accurate - TCG does not implement DD1's bugs.  
>>>>>>>>
>>>>>>>> According to the POWER8 user manual (I didn't fine the POWER9 one):
>>>>>>>>
>>>>>>>> "3.6.3.1 Processor Version Register (PVR)
>>>>>>>>
>>>>>>>> The processor revision level (PVR[16:31]) starts at x‘0100’, indicating
>>>>>>>> revision ‘1.0’. As revisions are made, bits [29:31] will indicate minor
>>>>>>>> revisions. Similarly, bits [20:23] indicate major changes."
>>>>>>>>
>>>>>>>> POWER9 DD1 PVR is 0x004E0100, so this is really version 1.0 of the POWER9.
>>>>>>>>
>>>>>>>> Perhaps we can define POWER9_v1.0 as CPU_POWERPC_POWER9_DD1, and
>>>>>>>> introduce a POWER9_v0.0 set to CPU_POWERPC_POWER9_BASE and define it as
>>>>>>>> the default one?  
>>>>>>>
>>>>>>> I like the suggestion to set a v0.0 to CPU_POWERPC_POWER9_BASE. But, I
>>>>>>> think we could have only that option, removing the
>>>>>>> CPU_POWERPC_POWER9_DD1 entry.  
>>>>>> I really dislike the idea of having a CPU called "v0.0" ... we do not
>>>>>> have this for any other CPU generation, and it sounds like it could be
>>>>>> very confusing for the users (you'd need to document somewhere what the
>>>>>> v0.0 exactly means). If we really want to go this way, I think we should
>>>>>> name it "POWER9-generic" or "PowerISA-3.0" or something similar instead.
>>>>>>
>>>>>> Or does somebody already know the exact PVR for DD2? If so, we could
>>>>>> simply add a POWER9_v2.0 CPU already and let the POWER9 alias point to
>>>>>> that version instead.  
>>>>>
>>>>> Yes, I think that's a better idea.  I don't know the DD2 PVR, but I'm
>>>>> pretty sure we should be able to find out from someone at IBM.
>>>>>
>>>>> I've CCed Sam & Suraj - can you ask Mikey or someone what the PVR
>>>>> value for DD2.0 will be?  
>>>>
>>>> I would expect something like :
>>>>
>>>>  0x200D104980000000UL; /* P9 Nimbus DD2.0 */  
>>>
>>>
>>> I would expect something like 0x004Exxxx.  
>>
>> ah yes, I am mistaking the PVR and the CFAM ID. 
>>
>> C. 
>>  
> 
> According to https://patchwork.ozlabs.org/patch/776052/
> 
> POWER9 DD2's PVR is expected to be 0x004e1200
>

So, perhaps I can send a v2 of the patch with POWER9_v1.0 set to DD1
PVR, and POWER9_v2.0 set to DD2 PVR?

Thanks,
Laurent

Re: [Qemu-devel] [Qemu-ppc] [PATCH] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 5237 bytes --]

On Wed, 28 Jun 2017 18:18:06 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> On 28/06/2017 13:59, Greg Kurz wrote:
> > On Wed, 28 Jun 2017 12:23:06 +0200
> > Cédric Le Goater <clg@kaod.org> wrote:
> >   
> >> On 06/28/2017 11:18 AM, Laurent Vivier wrote:  
> >>> On 28/06/2017 11:11, Cédric Le Goater wrote:    
> >>>> On 06/28/2017 10:18 AM, David Gibson wrote:    
> >>>>> On Wed, Jun 28, 2017 at 09:09:24AM +0200, Thomas Huth wrote:    
> >>>>>> On 28.06.2017 03:42, joserz@linux.vnet.ibm.com wrote:    
> >>>>>>> On Fri, Jun 23, 2017 at 04:10:55PM +0200, Laurent Vivier wrote:    
> >>>>>>>> On 23/06/2017 11:21, David Gibson wrote:    
> >>>>>>>>> On Thu, Jun 22, 2017 at 01:31:24PM +0200, Thomas Huth wrote:    
> >>>>>>>>>> On 22.06.2017 13:26, Laurent Vivier wrote:    
> >>>>>>>>>>> CPU_POWERPC_POWER9_DD1 is 0x004E0100, so this is the POWER9 v1.0.
> >>>>>>>>>>>
> >>>>>>>>>>> When we run qemu on a POWER9 DD1 host, we must use either
> >>>>>>>>>>> "-cpu host" or "-cpu POWER9", but in the latter case it fails with
> >>>>>>>>>>>
> >>>>>>>>>>>     Unable to find sPAPR CPU Core definition
> >>>>>>>>>>>
> >>>>>>>>>>> because POWER9 DD1 doesn't appear in the list of known CPUs.
> >>>>>>>>>>>
> >>>>>>>>>>> This patch fixes this by defining POWER9_v1.0 with POWER9 DD1
> >>>>>>>>>>> PVR instead of CPU_POWERPC_POWER9_BASE.
> >>>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> >>>>>>>>>>> ---
> >>>>>>>>>>>  target/ppc/cpu-models.c | 2 +-
> >>>>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>>>>>>
> >>>>>>>>>>> diff --git a/target/ppc/cpu-models.c b/target/ppc/cpu-models.c
> >>>>>>>>>>> index 4d3e635..a22363c 100644
> >>>>>>>>>>> --- a/target/ppc/cpu-models.c
> >>>>>>>>>>> +++ b/target/ppc/cpu-models.c
> >>>>>>>>>>> @@ -1144,7 +1144,7 @@
> >>>>>>>>>>>      POWERPC_DEF("970_v2.2",      CPU_POWERPC_970_v22,                970,
> >>>>>>>>>>>                  "PowerPC 970 v2.2")
> >>>>>>>>>>>  
> >>>>>>>>>>> -    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_BASE,            POWER9,
> >>>>>>>>>>> +    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_DD1,             POWER9,
> >>>>>>>>>>>                  "POWER9 v1.0")
> >>>>>>>>>>>  
> >>>>>>>>>>>      POWERPC_DEF("970fx_v1.0",    CPU_POWERPC_970FX_v10,              970,
> >>>>>>>>>>>    
> >>>>>>>>>>
> >>>>>>>>>> I think this also makes sense for running in TCG mode to get a valid
> >>>>>>>>>> real PVR there.    
> >>>>>>>>>
> >>>>>>>>> I'm not so convinced.
> >>>>>>>>>
> >>>>>>>>> IIUC, this will make TCG default (for now) to a DD1 POWER9.  That's a)
> >>>>>>>>> probably not what anyone wants - who'd select a buggy prototype and b)
> >>>>>>>>> not accurate - TCG does not implement DD1's bugs.    
> >>>>>>>>
> >>>>>>>> According to the POWER8 user manual (I didn't fine the POWER9 one):
> >>>>>>>>
> >>>>>>>> "3.6.3.1 Processor Version Register (PVR)
> >>>>>>>>
> >>>>>>>> The processor revision level (PVR[16:31]) starts at x‘0100’, indicating
> >>>>>>>> revision ‘1.0’. As revisions are made, bits [29:31] will indicate minor
> >>>>>>>> revisions. Similarly, bits [20:23] indicate major changes."
> >>>>>>>>
> >>>>>>>> POWER9 DD1 PVR is 0x004E0100, so this is really version 1.0 of the POWER9.
> >>>>>>>>
> >>>>>>>> Perhaps we can define POWER9_v1.0 as CPU_POWERPC_POWER9_DD1, and
> >>>>>>>> introduce a POWER9_v0.0 set to CPU_POWERPC_POWER9_BASE and define it as
> >>>>>>>> the default one?    
> >>>>>>>
> >>>>>>> I like the suggestion to set a v0.0 to CPU_POWERPC_POWER9_BASE. But, I
> >>>>>>> think we could have only that option, removing the
> >>>>>>> CPU_POWERPC_POWER9_DD1 entry.    
> >>>>>> I really dislike the idea of having a CPU called "v0.0" ... we do not
> >>>>>> have this for any other CPU generation, and it sounds like it could be
> >>>>>> very confusing for the users (you'd need to document somewhere what the
> >>>>>> v0.0 exactly means). If we really want to go this way, I think we should
> >>>>>> name it "POWER9-generic" or "PowerISA-3.0" or something similar instead.
> >>>>>>
> >>>>>> Or does somebody already know the exact PVR for DD2? If so, we could
> >>>>>> simply add a POWER9_v2.0 CPU already and let the POWER9 alias point to
> >>>>>> that version instead.    
> >>>>>
> >>>>> Yes, I think that's a better idea.  I don't know the DD2 PVR, but I'm
> >>>>> pretty sure we should be able to find out from someone at IBM.
> >>>>>
> >>>>> I've CCed Sam & Suraj - can you ask Mikey or someone what the PVR
> >>>>> value for DD2.0 will be?    
> >>>>
> >>>> I would expect something like :
> >>>>
> >>>>  0x200D104980000000UL; /* P9 Nimbus DD2.0 */    
> >>>
> >>>
> >>> I would expect something like 0x004Exxxx.    
> >>
> >> ah yes, I am mistaking the PVR and the CFAM ID. 
> >>
> >> C. 
> >>    
> > 
> > According to https://patchwork.ozlabs.org/patch/776052/
> > 
> > POWER9 DD2's PVR is expected to be 0x004e1200
> >  
> 
> So, perhaps I can send a v2 of the patch with POWER9_v1.0 set to DD1
> PVR, and POWER9_v2.0 set to DD2 PVR?
> 

FWIW Thomas suggested to do just that and David agreed it was "a better idea".

> Thanks,
> Laurent
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[Suraj Jitindar Singh]

On Wed, 2017-06-28 at 18:41 +0200, Greg Kurz wrote:
> On Wed, 28 Jun 2017 18:18:06 +0200
> Laurent Vivier <lvivier@redhat.com> wrote:
> 
> > On 28/06/2017 13:59, Greg Kurz wrote:
> > > On Wed, 28 Jun 2017 12:23:06 +0200
> > > Cédric Le Goater <clg@kaod.org> wrote:
> > >   
> > > > On 06/28/2017 11:18 AM, Laurent Vivier wrote:  
> > > > > On 28/06/2017 11:11, Cédric Le Goater wrote:    
> > > > > > On 06/28/2017 10:18 AM, David Gibson wrote:    
> > > > > > > On Wed, Jun 28, 2017 at 09:09:24AM +0200, Thomas Huth
> > > > > > > wrote:    
> > > > > > > > On 28.06.2017 03:42, joserz@linux.vnet.ibm.com
> > > > > > > > wrote:    
> > > > > > > > > On Fri, Jun 23, 2017 at 04:10:55PM +0200, Laurent
> > > > > > > > > Vivier wrote:    
> > > > > > > > > > On 23/06/2017 11:21, David Gibson wrote:    
> > > > > > > > > > > On Thu, Jun 22, 2017 at 01:31:24PM +0200, Thomas
> > > > > > > > > > > Huth wrote:    
> > > > > > > > > > > > On 22.06.2017 13:26, Laurent Vivier wrote:    
> > > > > > > > > > > > > CPU_POWERPC_POWER9_DD1 is 0x004E0100, so this
> > > > > > > > > > > > > is the POWER9 v1.0.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > When we run qemu on a POWER9 DD1 host, we
> > > > > > > > > > > > > must use either
> > > > > > > > > > > > > "-cpu host" or "-cpu POWER9", but in the
> > > > > > > > > > > > > latter case it fails with
> > > > > > > > > > > > > 
> > > > > > > > > > > > >     Unable to find sPAPR CPU Core definition
> > > > > > > > > > > > > 
> > > > > > > > > > > > > because POWER9 DD1 doesn't appear in the list
> > > > > > > > > > > > > of known CPUs.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > This patch fixes this by defining POWER9_v1.0
> > > > > > > > > > > > > with POWER9 DD1
> > > > > > > > > > > > > PVR instead of CPU_POWERPC_POWER9_BASE.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Signed-off-by: Laurent Vivier <lvivier@redhat
> > > > > > > > > > > > > .com>
> > > > > > > > > > > > > ---
> > > > > > > > > > > > >  target/ppc/cpu-models.c | 2 +-
> > > > > > > > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-
> > > > > > > > > > > > > )
> > > > > > > > > > > > > 
> > > > > > > > > > > > > diff --git a/target/ppc/cpu-models.c
> > > > > > > > > > > > > b/target/ppc/cpu-models.c
> > > > > > > > > > > > > index 4d3e635..a22363c 100644
> > > > > > > > > > > > > --- a/target/ppc/cpu-models.c
> > > > > > > > > > > > > +++ b/target/ppc/cpu-models.c
> > > > > > > > > > > > > @@ -1144,7 +1144,7 @@
> > > > > > > > > > > > >      POWERPC_DEF("970_v2.2",      CPU_POWERPC
> > > > > > > > > > > > > _970_v22,                970,
> > > > > > > > > > > > >                  "PowerPC 970 v2.2")
> > > > > > > > > > > > >  
> > > > > > > > > > > > > -    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC
> > > > > > > > > > > > > _POWER9_BASE,            POWER9,
> > > > > > > > > > > > > +    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC
> > > > > > > > > > > > > _POWER9_DD1,             POWER9,
> > > > > > > > > > > > >                  "POWER9 v1.0")
> > > > > > > > > > > > >  
> > > > > > > > > > > > >      POWERPC_DEF("970fx_v1.0",    CPU_POWERPC
> > > > > > > > > > > > > _970FX_v10,              970,
> > > > > > > > > > > > >    
> > > > > > > > > > > > 
> > > > > > > > > > > > I think this also makes sense for running in
> > > > > > > > > > > > TCG mode to get a valid
> > > > > > > > > > > > real PVR there.    
> > > > > > > > > > > 
> > > > > > > > > > > I'm not so convinced.
> > > > > > > > > > > 
> > > > > > > > > > > IIUC, this will make TCG default (for now) to a
> > > > > > > > > > > DD1 POWER9.  That's a)
> > > > > > > > > > > probably not what anyone wants - who'd select a
> > > > > > > > > > > buggy prototype and b)
> > > > > > > > > > > not accurate - TCG does not implement DD1's
> > > > > > > > > > > bugs.    
> > > > > > > > > > 
> > > > > > > > > > According to the POWER8 user manual (I didn't fine
> > > > > > > > > > the POWER9 one):
> > > > > > > > > > 
> > > > > > > > > > "3.6.3.1 Processor Version Register (PVR)
> > > > > > > > > > 
> > > > > > > > > > The processor revision level (PVR[16:31]) starts at
> > > > > > > > > > x‘0100’, indicating
> > > > > > > > > > revision ‘1.0’. As revisions are made, bits [29:31]
> > > > > > > > > > will indicate minor
> > > > > > > > > > revisions. Similarly, bits [20:23] indicate major
> > > > > > > > > > changes."
> > > > > > > > > > 
> > > > > > > > > > POWER9 DD1 PVR is 0x004E0100, so this is really
> > > > > > > > > > version 1.0 of the POWER9.
> > > > > > > > > > 
> > > > > > > > > > Perhaps we can define POWER9_v1.0 as
> > > > > > > > > > CPU_POWERPC_POWER9_DD1, and
> > > > > > > > > > introduce a POWER9_v0.0 set to
> > > > > > > > > > CPU_POWERPC_POWER9_BASE and define it as
> > > > > > > > > > the default one?    
> > > > > > > > > 
> > > > > > > > > I like the suggestion to set a v0.0 to
> > > > > > > > > CPU_POWERPC_POWER9_BASE. But, I
> > > > > > > > > think we could have only that option, removing the
> > > > > > > > > CPU_POWERPC_POWER9_DD1 entry.    
> > > > > > > > 
> > > > > > > > I really dislike the idea of having a CPU called "v0.0"
> > > > > > > > ... we do not
> > > > > > > > have this for any other CPU generation, and it sounds
> > > > > > > > like it could be
> > > > > > > > very confusing for the users (you'd need to document
> > > > > > > > somewhere what the
> > > > > > > > v0.0 exactly means). If we really want to go this way,
> > > > > > > > I think we should
> > > > > > > > name it "POWER9-generic" or "PowerISA-3.0" or something
> > > > > > > > similar instead.
> > > > > > > > 
> > > > > > > > Or does somebody already know the exact PVR for DD2? If
> > > > > > > > so, we could
> > > > > > > > simply add a POWER9_v2.0 CPU already and let the POWER9
> > > > > > > > alias point to
> > > > > > > > that version instead.    
> > > > > > > 
> > > > > > > Yes, I think that's a better idea.  I don't know the DD2
> > > > > > > PVR, but I'm
> > > > > > > pretty sure we should be able to find out from someone at
> > > > > > > IBM.
> > > > > > > 
> > > > > > > I've CCed Sam & Suraj - can you ask Mikey or someone what
> > > > > > > the PVR
> > > > > > > value for DD2.0 will be?    
> > > > > > 
> > > > > > I would expect something like :
> > > > > > 
> > > > > >  0x200D104980000000UL; /* P9 Nimbus DD2.0 */    
> > > > > 
> > > > > 
> > > > > I would expect something like 0x004Exxxx.    
> > > > 
> > > > ah yes, I am mistaking the PVR and the CFAM ID. 
> > > > 
> > > > C. 
> > > >    
> > > 
> > > According to https://patchwork.ozlabs.org/patch/776052/
> > > 
> > > POWER9 DD2's PVR is expected to be 0x004e1200
> > >  
> > 
> > So, perhaps I can send a v2 of the patch with POWER9_v1.0 set to
> > DD1
> > PVR, and POWER9_v2.0 set to DD2 PVR?
> > 
> 
> FWIW Thomas suggested to do just that and David agreed it was "a
> better idea".

I assume we would have just -cpu POWER9 alias to DD2?

We probably need to have a nice abort if someone tries to run TCG with
DD1, I'm not sure where it will break but I'm fairly sure it won't
boot.

That makes the assumption that DD2 doesn't require any work arounds
which TCG can't handle.

I think the absence of -cpu on the CLI should give -cpu host for KVM-
HV. FWIW currently TCG defaults to POWER8, so I guess we have to decide
if/when we want to bump that to POWER9.

> 
> > Thanks,
> > Laurent
> > 
> > 
> 
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[Suraj Jitindar Singh]

On Thu, 2017-06-29 at 15:37 +1000, Suraj Jitindar Singh wrote:
> On Wed, 2017-06-28 at 18:41 +0200, Greg Kurz wrote:
> > On Wed, 28 Jun 2017 18:18:06 +0200
> > Laurent Vivier <lvivier@redhat.com> wrote:
> > 
> > > On 28/06/2017 13:59, Greg Kurz wrote:
> > > > On Wed, 28 Jun 2017 12:23:06 +0200
> > > > Cédric Le Goater <clg@kaod.org> wrote:
> > > >   
> > > > > On 06/28/2017 11:18 AM, Laurent Vivier wrote:  
> > > > > > On 28/06/2017 11:11, Cédric Le Goater wrote:    
> > > > > > > On 06/28/2017 10:18 AM, David Gibson wrote:    
> > > > > > > > On Wed, Jun 28, 2017 at 09:09:24AM +0200, Thomas Huth
> > > > > > > > wrote:    
> > > > > > > > > On 28.06.2017 03:42, joserz@linux.vnet.ibm.com
> > > > > > > > > wrote:    
> > > > > > > > > > On Fri, Jun 23, 2017 at 04:10:55PM +0200, Laurent
> > > > > > > > > > Vivier wrote:    
> > > > > > > > > > > On 23/06/2017 11:21, David Gibson wrote:    
> > > > > > > > > > > > On Thu, Jun 22, 2017 at 01:31:24PM +0200,
> > > > > > > > > > > > Thomas
> > > > > > > > > > > > Huth wrote:    
> > > > > > > > > > > > > On 22.06.2017 13:26, Laurent Vivier
> > > > > > > > > > > > > wrote:    
> > > > > > > > > > > > > > CPU_POWERPC_POWER9_DD1 is 0x004E0100, so
> > > > > > > > > > > > > > this
> > > > > > > > > > > > > > is the POWER9 v1.0.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > When we run qemu on a POWER9 DD1 host, we
> > > > > > > > > > > > > > must use either
> > > > > > > > > > > > > > "-cpu host" or "-cpu POWER9", but in the
> > > > > > > > > > > > > > latter case it fails with
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > >     Unable to find sPAPR CPU Core
> > > > > > > > > > > > > > definition
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > because POWER9 DD1 doesn't appear in the
> > > > > > > > > > > > > > list
> > > > > > > > > > > > > > of known CPUs.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > This patch fixes this by defining
> > > > > > > > > > > > > > POWER9_v1.0
> > > > > > > > > > > > > > with POWER9 DD1
> > > > > > > > > > > > > > PVR instead of CPU_POWERPC_POWER9_BASE.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Signed-off-by: Laurent Vivier <lvivier@redh
> > > > > > > > > > > > > > at
> > > > > > > > > > > > > > .com>
> > > > > > > > > > > > > > ---
> > > > > > > > > > > > > >  target/ppc/cpu-models.c | 2 +-
> > > > > > > > > > > > > >  1 file changed, 1 insertion(+), 1
> > > > > > > > > > > > > > deletion(-
> > > > > > > > > > > > > > )
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > diff --git a/target/ppc/cpu-models.c
> > > > > > > > > > > > > > b/target/ppc/cpu-models.c
> > > > > > > > > > > > > > index 4d3e635..a22363c 100644
> > > > > > > > > > > > > > --- a/target/ppc/cpu-models.c
> > > > > > > > > > > > > > +++ b/target/ppc/cpu-models.c
> > > > > > > > > > > > > > @@ -1144,7 +1144,7 @@
> > > > > > > > > > > > > >      POWERPC_DEF("970_v2.2",      CPU_POWER
> > > > > > > > > > > > > > PC
> > > > > > > > > > > > > > _970_v22,                970,
> > > > > > > > > > > > > >                  "PowerPC 970 v2.2")
> > > > > > > > > > > > > >  
> > > > > > > > > > > > > > -    POWERPC_DEF("POWER9_v1.0",   CPU_POWER
> > > > > > > > > > > > > > PC
> > > > > > > > > > > > > > _POWER9_BASE,            POWER9,
> > > > > > > > > > > > > > +    POWERPC_DEF("POWER9_v1.0",   CPU_POWER
> > > > > > > > > > > > > > PC
> > > > > > > > > > > > > > _POWER9_DD1,             POWER9,
> > > > > > > > > > > > > >                  "POWER9 v1.0")
> > > > > > > > > > > > > >  
> > > > > > > > > > > > > >      POWERPC_DEF("970fx_v1.0",    CPU_POWER
> > > > > > > > > > > > > > PC
> > > > > > > > > > > > > > _970FX_v10,              970,
> > > > > > > > > > > > > >    
> > > > > > > > > > > > > 
> > > > > > > > > > > > > I think this also makes sense for running in
> > > > > > > > > > > > > TCG mode to get a valid
> > > > > > > > > > > > > real PVR there.    
> > > > > > > > > > > > 
> > > > > > > > > > > > I'm not so convinced.
> > > > > > > > > > > > 
> > > > > > > > > > > > IIUC, this will make TCG default (for now) to a
> > > > > > > > > > > > DD1 POWER9.  That's a)
> > > > > > > > > > > > probably not what anyone wants - who'd select a
> > > > > > > > > > > > buggy prototype and b)
> > > > > > > > > > > > not accurate - TCG does not implement DD1's
> > > > > > > > > > > > bugs.    
> > > > > > > > > > > 
> > > > > > > > > > > According to the POWER8 user manual (I didn't
> > > > > > > > > > > fine
> > > > > > > > > > > the POWER9 one):
> > > > > > > > > > > 
> > > > > > > > > > > "3.6.3.1 Processor Version Register (PVR)
> > > > > > > > > > > 
> > > > > > > > > > > The processor revision level (PVR[16:31]) starts
> > > > > > > > > > > at
> > > > > > > > > > > x‘0100’, indicating
> > > > > > > > > > > revision ‘1.0’. As revisions are made, bits
> > > > > > > > > > > [29:31]
> > > > > > > > > > > will indicate minor
> > > > > > > > > > > revisions. Similarly, bits [20:23] indicate major
> > > > > > > > > > > changes."
> > > > > > > > > > > 
> > > > > > > > > > > POWER9 DD1 PVR is 0x004E0100, so this is really
> > > > > > > > > > > version 1.0 of the POWER9.
> > > > > > > > > > > 
> > > > > > > > > > > Perhaps we can define POWER9_v1.0 as
> > > > > > > > > > > CPU_POWERPC_POWER9_DD1, and
> > > > > > > > > > > introduce a POWER9_v0.0 set to
> > > > > > > > > > > CPU_POWERPC_POWER9_BASE and define it as
> > > > > > > > > > > the default one?    
> > > > > > > > > > 
> > > > > > > > > > I like the suggestion to set a v0.0 to
> > > > > > > > > > CPU_POWERPC_POWER9_BASE. But, I
> > > > > > > > > > think we could have only that option, removing the
> > > > > > > > > > CPU_POWERPC_POWER9_DD1 entry.    
> > > > > > > > > 
> > > > > > > > > I really dislike the idea of having a CPU called
> > > > > > > > > "v0.0"
> > > > > > > > > ... we do not
> > > > > > > > > have this for any other CPU generation, and it sounds
> > > > > > > > > like it could be
> > > > > > > > > very confusing for the users (you'd need to document
> > > > > > > > > somewhere what the
> > > > > > > > > v0.0 exactly means). If we really want to go this
> > > > > > > > > way,
> > > > > > > > > I think we should
> > > > > > > > > name it "POWER9-generic" or "PowerISA-3.0" or
> > > > > > > > > something
> > > > > > > > > similar instead.
> > > > > > > > > 
> > > > > > > > > Or does somebody already know the exact PVR for DD2?
> > > > > > > > > If
> > > > > > > > > so, we could
> > > > > > > > > simply add a POWER9_v2.0 CPU already and let the
> > > > > > > > > POWER9
> > > > > > > > > alias point to
> > > > > > > > > that version instead.    
> > > > > > > > 
> > > > > > > > Yes, I think that's a better idea.  I don't know the
> > > > > > > > DD2
> > > > > > > > PVR, but I'm
> > > > > > > > pretty sure we should be able to find out from someone
> > > > > > > > at
> > > > > > > > IBM.
> > > > > > > > 
> > > > > > > > I've CCed Sam & Suraj - can you ask Mikey or someone
> > > > > > > > what
> > > > > > > > the PVR
> > > > > > > > value for DD2.0 will be?    
> > > > > > > 
> > > > > > > I would expect something like :
> > > > > > > 
> > > > > > >  0x200D104980000000UL; /* P9 Nimbus DD2.0 */    
> > > > > > 
> > > > > > 
> > > > > > I would expect something like 0x004Exxxx.    
> > > > > 
> > > > > ah yes, I am mistaking the PVR and the CFAM ID. 
> > > > > 
> > > > > C. 
> > > > >    
> > > > 
> > > > According to https://patchwork.ozlabs.org/patch/776052/
> > > > 
> > > > POWER9 DD2's PVR is expected to be 0x004e1200
> > > >  
> > > 
> > > So, perhaps I can send a v2 of the patch with POWER9_v1.0 set to
> > > DD1
> > > PVR, and POWER9_v2.0 set to DD2 PVR?
> > > 
> > 
> > FWIW Thomas suggested to do just that and David agreed it was "a
> > better idea".
> 
> I assume we would have just -cpu POWER9 alias to DD2?
> 
> We probably need to have a nice abort if someone tries to run TCG
> with
> DD1, I'm not sure where it will break but I'm fairly sure it won't
> boot.
> 
> That makes the assumption that DD2 doesn't require any work arounds
> which TCG can't handle.

Actually TCG is really a non-issue since we'll just go into the POWER9
architected mode.

Can't we just have -cpu POWER9 alias to DD1 for now and add DD2 when we
know the pvr?

> 
> I think the absence of -cpu on the CLI should give -cpu host for KVM-
> HV. FWIW currently TCG defaults to POWER8, so I guess we have to
> decide
> if/when we want to bump that to POWER9.
> 
> > 
> > > Thanks,
> > > Laurent
> > > 
> > > 
> > 
> > 

Re: [Qemu-devel] [Qemu-ppc] [PATCH] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[Thomas Huth]

On 29.06.2017 07:37, Suraj Jitindar Singh wrote:
> On Wed, 2017-06-28 at 18:41 +0200, Greg Kurz wrote:
>> On Wed, 28 Jun 2017 18:18:06 +0200
>> Laurent Vivier <lvivier@redhat.com> wrote:
>>
>>> On 28/06/2017 13:59, Greg Kurz wrote:
>>>> On Wed, 28 Jun 2017 12:23:06 +0200
>>>> Cédric Le Goater <clg@kaod.org> wrote:
>>>>   
>>>>> On 06/28/2017 11:18 AM, Laurent Vivier wrote:  
>>>>>> On 28/06/2017 11:11, Cédric Le Goater wrote:    
>>>>>>> On 06/28/2017 10:18 AM, David Gibson wrote:    
>>>>>>>> On Wed, Jun 28, 2017 at 09:09:24AM +0200, Thomas Huth
>>>>>>>> wrote:    
>>>>>>>>> On 28.06.2017 03:42, joserz@linux.vnet.ibm.com
>>>>>>>>> wrote:    
>>>>>>>>>> On Fri, Jun 23, 2017 at 04:10:55PM +0200, Laurent
>>>>>>>>>> Vivier wrote:    
>>>>>>>>>>> On 23/06/2017 11:21, David Gibson wrote:    
>>>>>>>>>>>> On Thu, Jun 22, 2017 at 01:31:24PM +0200, Thomas
>>>>>>>>>>>> Huth wrote:    
>>>>>>>>>>>>> On 22.06.2017 13:26, Laurent Vivier wrote:    
>>>>>>>>>>>>>> CPU_POWERPC_POWER9_DD1 is 0x004E0100, so this
>>>>>>>>>>>>>> is the POWER9 v1.0.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> When we run qemu on a POWER9 DD1 host, we
>>>>>>>>>>>>>> must use either
>>>>>>>>>>>>>> "-cpu host" or "-cpu POWER9", but in the
>>>>>>>>>>>>>> latter case it fails with
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>     Unable to find sPAPR CPU Core definition
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> because POWER9 DD1 doesn't appear in the list
>>>>>>>>>>>>>> of known CPUs.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This patch fixes this by defining POWER9_v1.0
>>>>>>>>>>>>>> with POWER9 DD1
>>>>>>>>>>>>>> PVR instead of CPU_POWERPC_POWER9_BASE.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Signed-off-by: Laurent Vivier <lvivier@redhat
>>>>>>>>>>>>>> .com>
>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>  target/ppc/cpu-models.c | 2 +-
>>>>>>>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-
>>>>>>>>>>>>>> )
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> diff --git a/target/ppc/cpu-models.c
>>>>>>>>>>>>>> b/target/ppc/cpu-models.c
>>>>>>>>>>>>>> index 4d3e635..a22363c 100644
>>>>>>>>>>>>>> --- a/target/ppc/cpu-models.c
>>>>>>>>>>>>>> +++ b/target/ppc/cpu-models.c
>>>>>>>>>>>>>> @@ -1144,7 +1144,7 @@
>>>>>>>>>>>>>>      POWERPC_DEF("970_v2.2",      CPU_POWERPC
>>>>>>>>>>>>>> _970_v22,                970,
>>>>>>>>>>>>>>                  "PowerPC 970 v2.2")
>>>>>>>>>>>>>>  
>>>>>>>>>>>>>> -    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC
>>>>>>>>>>>>>> _POWER9_BASE,            POWER9,
>>>>>>>>>>>>>> +    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC
>>>>>>>>>>>>>> _POWER9_DD1,             POWER9,
>>>>>>>>>>>>>>                  "POWER9 v1.0")
>>>>>>>>>>>>>>  
>>>>>>>>>>>>>>      POWERPC_DEF("970fx_v1.0",    CPU_POWERPC
>>>>>>>>>>>>>> _970FX_v10,              970,
>>>>>>>>>>>>>>    
>>>>>>>>>>>>>
>>>>>>>>>>>>> I think this also makes sense for running in
>>>>>>>>>>>>> TCG mode to get a valid
>>>>>>>>>>>>> real PVR there.    
>>>>>>>>>>>>
>>>>>>>>>>>> I'm not so convinced.
>>>>>>>>>>>>
>>>>>>>>>>>> IIUC, this will make TCG default (for now) to a
>>>>>>>>>>>> DD1 POWER9.  That's a)
>>>>>>>>>>>> probably not what anyone wants - who'd select a
>>>>>>>>>>>> buggy prototype and b)
>>>>>>>>>>>> not accurate - TCG does not implement DD1's
>>>>>>>>>>>> bugs.    
>>>>>>>>>>>
>>>>>>>>>>> According to the POWER8 user manual (I didn't fine
>>>>>>>>>>> the POWER9 one):
>>>>>>>>>>>
>>>>>>>>>>> "3.6.3.1 Processor Version Register (PVR)
>>>>>>>>>>>
>>>>>>>>>>> The processor revision level (PVR[16:31]) starts at
>>>>>>>>>>> x‘0100’, indicating
>>>>>>>>>>> revision ‘1.0’. As revisions are made, bits [29:31]
>>>>>>>>>>> will indicate minor
>>>>>>>>>>> revisions. Similarly, bits [20:23] indicate major
>>>>>>>>>>> changes."
>>>>>>>>>>>
>>>>>>>>>>> POWER9 DD1 PVR is 0x004E0100, so this is really
>>>>>>>>>>> version 1.0 of the POWER9.
>>>>>>>>>>>
>>>>>>>>>>> Perhaps we can define POWER9_v1.0 as
>>>>>>>>>>> CPU_POWERPC_POWER9_DD1, and
>>>>>>>>>>> introduce a POWER9_v0.0 set to
>>>>>>>>>>> CPU_POWERPC_POWER9_BASE and define it as
>>>>>>>>>>> the default one?    
>>>>>>>>>>
>>>>>>>>>> I like the suggestion to set a v0.0 to
>>>>>>>>>> CPU_POWERPC_POWER9_BASE. But, I
>>>>>>>>>> think we could have only that option, removing the
>>>>>>>>>> CPU_POWERPC_POWER9_DD1 entry.    
>>>>>>>>>
>>>>>>>>> I really dislike the idea of having a CPU called "v0.0"
>>>>>>>>> ... we do not
>>>>>>>>> have this for any other CPU generation, and it sounds
>>>>>>>>> like it could be
>>>>>>>>> very confusing for the users (you'd need to document
>>>>>>>>> somewhere what the
>>>>>>>>> v0.0 exactly means). If we really want to go this way,
>>>>>>>>> I think we should
>>>>>>>>> name it "POWER9-generic" or "PowerISA-3.0" or something
>>>>>>>>> similar instead.
>>>>>>>>>
>>>>>>>>> Or does somebody already know the exact PVR for DD2? If
>>>>>>>>> so, we could
>>>>>>>>> simply add a POWER9_v2.0 CPU already and let the POWER9
>>>>>>>>> alias point to
>>>>>>>>> that version instead.    
>>>>>>>>
>>>>>>>> Yes, I think that's a better idea.  I don't know the DD2
>>>>>>>> PVR, but I'm
>>>>>>>> pretty sure we should be able to find out from someone at
>>>>>>>> IBM.
>>>>>>>>
>>>>>>>> I've CCed Sam & Suraj - can you ask Mikey or someone what
>>>>>>>> the PVR
>>>>>>>> value for DD2.0 will be?    
>>>>>>>
>>>>>>> I would expect something like :
>>>>>>>
>>>>>>>  0x200D104980000000UL; /* P9 Nimbus DD2.0 */    
>>>>>>
>>>>>>
>>>>>> I would expect something like 0x004Exxxx.    
>>>>>
>>>>> ah yes, I am mistaking the PVR and the CFAM ID. 
>>>>>
>>>>> C. 
>>>>>    
>>>>
>>>> According to https://patchwork.ozlabs.org/patch/776052/
>>>>
>>>> POWER9 DD2's PVR is expected to be 0x004e1200
>>>>  
>>>
>>> So, perhaps I can send a v2 of the patch with POWER9_v1.0 set to
>>> DD1
>>> PVR, and POWER9_v2.0 set to DD2 PVR?
>>>
>>
>> FWIW Thomas suggested to do just that and David agreed it was "a
>> better idea".
> 
> I assume we would have just -cpu POWER9 alias to DD2?

Yes.

> We probably need to have a nice abort if someone tries to run TCG with
> DD1, I'm not sure where it will break but I'm fairly sure it won't
> boot.

I assume that we will remove DD1 again once DD2 is widely available
(just like we did on POWER8), so I would not bother adding special
logic here.

> I think the absence of -cpu on the CLI should give -cpu host for KVM-
> HV.

Yes, we've got this in spapr.c:

    /* init CPUs */
    if (machine->cpu_model == NULL) {
        machine->cpu_model = kvm_enabled() ? "host" : smc->tcg_default_cpu;
    }

> FWIW currently TCG defaults to POWER8, so I guess we have to decide
> if/when we want to bump that to POWER9.

The main reason for bumping to POWER8 was the fact that some (little
endian) Linux distros started to compile with -mcpu=power8 and thus suddenly
did not work by default anymore with QEMU. As long as we do notsee something
similar with POWER9 (which I do not expect), I think there is no
urgent need right now to increase the default CPU to POWER9.

 Thomas

Re: [Qemu-devel] [Qemu-ppc] [PATCH] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 939 bytes --]

[meta-comment]

On 06/29/2017 01:44 AM, Thomas Huth wrote:
...
>>>>>>>>>>>>>> On 22.06.2017 13:26, Laurent Vivier wrote:    
>>>>>>>>>>>>>>> CPU_POWERPC_POWER9_DD1 is 0x004E0100, so this

As a reminder, 14 levels of > quoting is a bit much. It's not only
acceptable, but encouraged, to trim replies to high-volume lists, such
that you only keep context relative to your reply, rather than the full
thread.

>>> FWIW Thomas suggested to do just that and David agreed it was "a
>>> better idea".
>>
>> I assume we would have just -cpu POWER9 alias to DD2?
> 
> Yes.

That way, existing thread readers can quickly see what you're adding to
the thread without scrolling past screens of quotations they've already
seen, and new readers can refer to the archives for the rest of the thread.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 9531 bytes --]

On Thu, Jun 29, 2017 at 03:42:03PM +1000, Suraj Jitindar Singh wrote:
> On Thu, 2017-06-29 at 15:37 +1000, Suraj Jitindar Singh wrote:
> > On Wed, 2017-06-28 at 18:41 +0200, Greg Kurz wrote:
> > > On Wed, 28 Jun 2017 18:18:06 +0200
> > > Laurent Vivier <lvivier@redhat.com> wrote:
> > > 
> > > > On 28/06/2017 13:59, Greg Kurz wrote:
> > > > > On Wed, 28 Jun 2017 12:23:06 +0200
> > > > > Cédric Le Goater <clg@kaod.org> wrote:
> > > > >   
> > > > > > On 06/28/2017 11:18 AM, Laurent Vivier wrote:  
> > > > > > > On 28/06/2017 11:11, Cédric Le Goater wrote:    
> > > > > > > > On 06/28/2017 10:18 AM, David Gibson wrote:    
> > > > > > > > > On Wed, Jun 28, 2017 at 09:09:24AM +0200, Thomas Huth
> > > > > > > > > wrote:    
> > > > > > > > > > On 28.06.2017 03:42, joserz@linux.vnet.ibm.com
> > > > > > > > > > wrote:    
> > > > > > > > > > > On Fri, Jun 23, 2017 at 04:10:55PM +0200, Laurent
> > > > > > > > > > > Vivier wrote:    
> > > > > > > > > > > > On 23/06/2017 11:21, David Gibson wrote:    
> > > > > > > > > > > > > On Thu, Jun 22, 2017 at 01:31:24PM +0200,
> > > > > > > > > > > > > Thomas
> > > > > > > > > > > > > Huth wrote:    
> > > > > > > > > > > > > > On 22.06.2017 13:26, Laurent Vivier
> > > > > > > > > > > > > > wrote:    
> > > > > > > > > > > > > > > CPU_POWERPC_POWER9_DD1 is 0x004E0100, so
> > > > > > > > > > > > > > > this
> > > > > > > > > > > > > > > is the POWER9 v1.0.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > When we run qemu on a POWER9 DD1 host, we
> > > > > > > > > > > > > > > must use either
> > > > > > > > > > > > > > > "-cpu host" or "-cpu POWER9", but in the
> > > > > > > > > > > > > > > latter case it fails with
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > >     Unable to find sPAPR CPU Core
> > > > > > > > > > > > > > > definition
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > because POWER9 DD1 doesn't appear in the
> > > > > > > > > > > > > > > list
> > > > > > > > > > > > > > > of known CPUs.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > This patch fixes this by defining
> > > > > > > > > > > > > > > POWER9_v1.0
> > > > > > > > > > > > > > > with POWER9 DD1
> > > > > > > > > > > > > > > PVR instead of CPU_POWERPC_POWER9_BASE.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > Signed-off-by: Laurent Vivier <lvivier@redh
> > > > > > > > > > > > > > > at
> > > > > > > > > > > > > > > .com>
> > > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > >  target/ppc/cpu-models.c | 2 +-
> > > > > > > > > > > > > > >  1 file changed, 1 insertion(+), 1
> > > > > > > > > > > > > > > deletion(-
> > > > > > > > > > > > > > > )
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > diff --git a/target/ppc/cpu-models.c
> > > > > > > > > > > > > > > b/target/ppc/cpu-models.c
> > > > > > > > > > > > > > > index 4d3e635..a22363c 100644
> > > > > > > > > > > > > > > --- a/target/ppc/cpu-models.c
> > > > > > > > > > > > > > > +++ b/target/ppc/cpu-models.c
> > > > > > > > > > > > > > > @@ -1144,7 +1144,7 @@
> > > > > > > > > > > > > > >      POWERPC_DEF("970_v2.2",      CPU_POWER
> > > > > > > > > > > > > > > PC
> > > > > > > > > > > > > > > _970_v22,                970,
> > > > > > > > > > > > > > >                  "PowerPC 970 v2.2")
> > > > > > > > > > > > > > >  
> > > > > > > > > > > > > > > -    POWERPC_DEF("POWER9_v1.0",   CPU_POWER
> > > > > > > > > > > > > > > PC
> > > > > > > > > > > > > > > _POWER9_BASE,            POWER9,
> > > > > > > > > > > > > > > +    POWERPC_DEF("POWER9_v1.0",   CPU_POWER
> > > > > > > > > > > > > > > PC
> > > > > > > > > > > > > > > _POWER9_DD1,             POWER9,
> > > > > > > > > > > > > > >                  "POWER9 v1.0")
> > > > > > > > > > > > > > >  
> > > > > > > > > > > > > > >      POWERPC_DEF("970fx_v1.0",    CPU_POWER
> > > > > > > > > > > > > > > PC
> > > > > > > > > > > > > > > _970FX_v10,              970,
> > > > > > > > > > > > > > >    
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > I think this also makes sense for running in
> > > > > > > > > > > > > > TCG mode to get a valid
> > > > > > > > > > > > > > real PVR there.    
> > > > > > > > > > > > > 
> > > > > > > > > > > > > I'm not so convinced.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > IIUC, this will make TCG default (for now) to a
> > > > > > > > > > > > > DD1 POWER9.  That's a)
> > > > > > > > > > > > > probably not what anyone wants - who'd select a
> > > > > > > > > > > > > buggy prototype and b)
> > > > > > > > > > > > > not accurate - TCG does not implement DD1's
> > > > > > > > > > > > > bugs.    
> > > > > > > > > > > > 
> > > > > > > > > > > > According to the POWER8 user manual (I didn't
> > > > > > > > > > > > fine
> > > > > > > > > > > > the POWER9 one):
> > > > > > > > > > > > 
> > > > > > > > > > > > "3.6.3.1 Processor Version Register (PVR)
> > > > > > > > > > > > 
> > > > > > > > > > > > The processor revision level (PVR[16:31]) starts
> > > > > > > > > > > > at
> > > > > > > > > > > > x‘0100’, indicating
> > > > > > > > > > > > revision ‘1.0’. As revisions are made, bits
> > > > > > > > > > > > [29:31]
> > > > > > > > > > > > will indicate minor
> > > > > > > > > > > > revisions. Similarly, bits [20:23] indicate major
> > > > > > > > > > > > changes."
> > > > > > > > > > > > 
> > > > > > > > > > > > POWER9 DD1 PVR is 0x004E0100, so this is really
> > > > > > > > > > > > version 1.0 of the POWER9.
> > > > > > > > > > > > 
> > > > > > > > > > > > Perhaps we can define POWER9_v1.0 as
> > > > > > > > > > > > CPU_POWERPC_POWER9_DD1, and
> > > > > > > > > > > > introduce a POWER9_v0.0 set to
> > > > > > > > > > > > CPU_POWERPC_POWER9_BASE and define it as
> > > > > > > > > > > > the default one?    
> > > > > > > > > > > 
> > > > > > > > > > > I like the suggestion to set a v0.0 to
> > > > > > > > > > > CPU_POWERPC_POWER9_BASE. But, I
> > > > > > > > > > > think we could have only that option, removing the
> > > > > > > > > > > CPU_POWERPC_POWER9_DD1 entry.    
> > > > > > > > > > 
> > > > > > > > > > I really dislike the idea of having a CPU called
> > > > > > > > > > "v0.0"
> > > > > > > > > > ... we do not
> > > > > > > > > > have this for any other CPU generation, and it sounds
> > > > > > > > > > like it could be
> > > > > > > > > > very confusing for the users (you'd need to document
> > > > > > > > > > somewhere what the
> > > > > > > > > > v0.0 exactly means). If we really want to go this
> > > > > > > > > > way,
> > > > > > > > > > I think we should
> > > > > > > > > > name it "POWER9-generic" or "PowerISA-3.0" or
> > > > > > > > > > something
> > > > > > > > > > similar instead.
> > > > > > > > > > 
> > > > > > > > > > Or does somebody already know the exact PVR for DD2?
> > > > > > > > > > If
> > > > > > > > > > so, we could
> > > > > > > > > > simply add a POWER9_v2.0 CPU already and let the
> > > > > > > > > > POWER9
> > > > > > > > > > alias point to
> > > > > > > > > > that version instead.    
> > > > > > > > > 
> > > > > > > > > Yes, I think that's a better idea.  I don't know the
> > > > > > > > > DD2
> > > > > > > > > PVR, but I'm
> > > > > > > > > pretty sure we should be able to find out from someone
> > > > > > > > > at
> > > > > > > > > IBM.
> > > > > > > > > 
> > > > > > > > > I've CCed Sam & Suraj - can you ask Mikey or someone
> > > > > > > > > what
> > > > > > > > > the PVR
> > > > > > > > > value for DD2.0 will be?    
> > > > > > > > 
> > > > > > > > I would expect something like :
> > > > > > > > 
> > > > > > > >  0x200D104980000000UL; /* P9 Nimbus DD2.0 */    
> > > > > > > 
> > > > > > > 
> > > > > > > I would expect something like 0x004Exxxx.    
> > > > > > 
> > > > > > ah yes, I am mistaking the PVR and the CFAM ID. 
> > > > > > 
> > > > > > C. 
> > > > > >    
> > > > > 
> > > > > According to https://patchwork.ozlabs.org/patch/776052/
> > > > > 
> > > > > POWER9 DD2's PVR is expected to be 0x004e1200
> > > > >  
> > > > 
> > > > So, perhaps I can send a v2 of the patch with POWER9_v1.0 set to
> > > > DD1
> > > > PVR, and POWER9_v2.0 set to DD2 PVR?
> > > > 
> > > 
> > > FWIW Thomas suggested to do just that and David agreed it was "a
> > > better idea".
> > 
> > I assume we would have just -cpu POWER9 alias to DD2?
> > 
> > We probably need to have a nice abort if someone tries to run TCG
> > with
> > DD1, I'm not sure where it will break but I'm fairly sure it won't
> > boot.
> > 
> > That makes the assumption that DD2 doesn't require any work arounds
> > which TCG can't handle.
> 
> Actually TCG is really a non-issue since we'll just go into the POWER9
> architected mode.
> 
> Can't we just have -cpu POWER9 alias to DD1 for now and add DD2 when we
> know the pvr?

No, because calling what qemu does DD1 is simply not accurate, in
important and guest-visible ways.

What we should do is add in DD2.0 - we know the PVR, even if the
chip's not out yet.  Then alias POWER9 to that.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5464 bytes --]

On Wed, Jun 28, 2017 at 06:18:06PM +0200, Laurent Vivier wrote:
> On 28/06/2017 13:59, Greg Kurz wrote:
> > On Wed, 28 Jun 2017 12:23:06 +0200
> > Cédric Le Goater <clg@kaod.org> wrote:
> > 
> >> On 06/28/2017 11:18 AM, Laurent Vivier wrote:
> >>> On 28/06/2017 11:11, Cédric Le Goater wrote:  
> >>>> On 06/28/2017 10:18 AM, David Gibson wrote:  
> >>>>> On Wed, Jun 28, 2017 at 09:09:24AM +0200, Thomas Huth wrote:  
> >>>>>> On 28.06.2017 03:42, joserz@linux.vnet.ibm.com wrote:  
> >>>>>>> On Fri, Jun 23, 2017 at 04:10:55PM +0200, Laurent Vivier wrote:  
> >>>>>>>> On 23/06/2017 11:21, David Gibson wrote:  
> >>>>>>>>> On Thu, Jun 22, 2017 at 01:31:24PM +0200, Thomas Huth wrote:  
> >>>>>>>>>> On 22.06.2017 13:26, Laurent Vivier wrote:  
> >>>>>>>>>>> CPU_POWERPC_POWER9_DD1 is 0x004E0100, so this is the POWER9 v1.0.
> >>>>>>>>>>>
> >>>>>>>>>>> When we run qemu on a POWER9 DD1 host, we must use either
> >>>>>>>>>>> "-cpu host" or "-cpu POWER9", but in the latter case it fails with
> >>>>>>>>>>>
> >>>>>>>>>>>     Unable to find sPAPR CPU Core definition
> >>>>>>>>>>>
> >>>>>>>>>>> because POWER9 DD1 doesn't appear in the list of known CPUs.
> >>>>>>>>>>>
> >>>>>>>>>>> This patch fixes this by defining POWER9_v1.0 with POWER9 DD1
> >>>>>>>>>>> PVR instead of CPU_POWERPC_POWER9_BASE.
> >>>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> >>>>>>>>>>> ---
> >>>>>>>>>>>  target/ppc/cpu-models.c | 2 +-
> >>>>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>>>>>>
> >>>>>>>>>>> diff --git a/target/ppc/cpu-models.c b/target/ppc/cpu-models.c
> >>>>>>>>>>> index 4d3e635..a22363c 100644
> >>>>>>>>>>> --- a/target/ppc/cpu-models.c
> >>>>>>>>>>> +++ b/target/ppc/cpu-models.c
> >>>>>>>>>>> @@ -1144,7 +1144,7 @@
> >>>>>>>>>>>      POWERPC_DEF("970_v2.2",      CPU_POWERPC_970_v22,                970,
> >>>>>>>>>>>                  "PowerPC 970 v2.2")
> >>>>>>>>>>>  
> >>>>>>>>>>> -    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_BASE,            POWER9,
> >>>>>>>>>>> +    POWERPC_DEF("POWER9_v1.0",   CPU_POWERPC_POWER9_DD1,             POWER9,
> >>>>>>>>>>>                  "POWER9 v1.0")
> >>>>>>>>>>>  
> >>>>>>>>>>>      POWERPC_DEF("970fx_v1.0",    CPU_POWERPC_970FX_v10,              970,
> >>>>>>>>>>>  
> >>>>>>>>>>
> >>>>>>>>>> I think this also makes sense for running in TCG mode to get a valid
> >>>>>>>>>> real PVR there.  
> >>>>>>>>>
> >>>>>>>>> I'm not so convinced.
> >>>>>>>>>
> >>>>>>>>> IIUC, this will make TCG default (for now) to a DD1 POWER9.  That's a)
> >>>>>>>>> probably not what anyone wants - who'd select a buggy prototype and b)
> >>>>>>>>> not accurate - TCG does not implement DD1's bugs.  
> >>>>>>>>
> >>>>>>>> According to the POWER8 user manual (I didn't fine the POWER9 one):
> >>>>>>>>
> >>>>>>>> "3.6.3.1 Processor Version Register (PVR)
> >>>>>>>>
> >>>>>>>> The processor revision level (PVR[16:31]) starts at x‘0100’, indicating
> >>>>>>>> revision ‘1.0’. As revisions are made, bits [29:31] will indicate minor
> >>>>>>>> revisions. Similarly, bits [20:23] indicate major changes."
> >>>>>>>>
> >>>>>>>> POWER9 DD1 PVR is 0x004E0100, so this is really version 1.0 of the POWER9.
> >>>>>>>>
> >>>>>>>> Perhaps we can define POWER9_v1.0 as CPU_POWERPC_POWER9_DD1, and
> >>>>>>>> introduce a POWER9_v0.0 set to CPU_POWERPC_POWER9_BASE and define it as
> >>>>>>>> the default one?  
> >>>>>>>
> >>>>>>> I like the suggestion to set a v0.0 to CPU_POWERPC_POWER9_BASE. But, I
> >>>>>>> think we could have only that option, removing the
> >>>>>>> CPU_POWERPC_POWER9_DD1 entry.  
> >>>>>> I really dislike the idea of having a CPU called "v0.0" ... we do not
> >>>>>> have this for any other CPU generation, and it sounds like it could be
> >>>>>> very confusing for the users (you'd need to document somewhere what the
> >>>>>> v0.0 exactly means). If we really want to go this way, I think we should
> >>>>>> name it "POWER9-generic" or "PowerISA-3.0" or something similar instead.
> >>>>>>
> >>>>>> Or does somebody already know the exact PVR for DD2? If so, we could
> >>>>>> simply add a POWER9_v2.0 CPU already and let the POWER9 alias point to
> >>>>>> that version instead.  
> >>>>>
> >>>>> Yes, I think that's a better idea.  I don't know the DD2 PVR, but I'm
> >>>>> pretty sure we should be able to find out from someone at IBM.
> >>>>>
> >>>>> I've CCed Sam & Suraj - can you ask Mikey or someone what the PVR
> >>>>> value for DD2.0 will be?  
> >>>>
> >>>> I would expect something like :
> >>>>
> >>>>  0x200D104980000000UL; /* P9 Nimbus DD2.0 */  
> >>>
> >>>
> >>> I would expect something like 0x004Exxxx.  
> >>
> >> ah yes, I am mistaking the PVR and the CFAM ID. 
> >>
> >> C. 
> >>  
> > 
> > According to https://patchwork.ozlabs.org/patch/776052/
> > 
> > POWER9 DD2's PVR is expected to be 0x004e1200

Uh.. I spoke to Michael Ellerman, and he said he expected 0x004e0200.
Though he did mention there might be several variants.  Can we please
get a definitive answer on this from IBM.

> So, perhaps I can send a v2 of the patch with POWER9_v1.0 set to DD1
> PVR, and POWER9_v2.0 set to DD2 PVR?
> 
> Thanks,
> Laurent
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[Cédric Le Goater]

>>> According to https://patchwork.ozlabs.org/patch/776052/
>>>
>>> POWER9 DD2's PVR is expected to be 0x004e1200
> 
> Uh.. I spoke to Michael Ellerman, and he said he expected 0x004e0200.
> Though he did mention there might be several variants.  Can we please
> get a definitive answer on this from IBM.

Adding Michael Ellerman in cc: but I think Greg is correct. 

C.

Re: [Qemu-devel] [Qemu-ppc] [PATCH] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[Laurent Vivier]

On 30/06/2017 09:12, David Gibson wrote:
> On Thu, Jun 29, 2017 at 03:42:03PM +1000, Suraj Jitindar Singh wrote:
>> On Thu, 2017-06-29 at 15:37 +1000, Suraj Jitindar Singh wrote:
>>> On Wed, 2017-06-28 at 18:41 +0200, Greg Kurz wrote:
...
>>> That makes the assumption that DD2 doesn't require any work arounds
>>> which TCG can't handle.
>>
>> Actually TCG is really a non-issue since we'll just go into the POWER9
>> architected mode.
>>
>> Can't we just have -cpu POWER9 alias to DD1 for now and add DD2 when we
>> know the pvr?
> 
> No, because calling what qemu does DD1 is simply not accurate, in
> important and guest-visible ways.
> 
> What we should do is add in DD2.0 - we know the PVR, even if the
> chip's not out yet.  Then alias POWER9 to that.
> 

OK, I have patch that define v1.0 to DD1, v2 to DD2, and set the POWER9
alias to v2, but:
- on a DD1 host and KVM HV, "-cpu host" works well and "cpu POWER9"
(that select then the v1.0) boots the kernel and hangs at early boot of
the kernel (first display)
- in TCG mode, boot by default with v2, but some services does not start
correctly, and I have never the console login (perhaps because of some
time out: I test TCG on the POWER9 host, I should test on a x86 one).

I'm trying for the moment to understand why "-cpu POWER9" hangs while
"-cpu host" works.

Thanks,
Laurent

Re: [Qemu-devel] [Qemu-ppc] [PATCH] target/ppc/cpu-models: set POWER9_v1.0 as POWER9 DD1

[Michael Ellerman]

Cédric Le Goater <clg@kaod.org> writes:

>>>> According to https://patchwork.ozlabs.org/patch/776052/
>>>>
>>>> POWER9 DD2's PVR is expected to be 0x004e1200
>> 
>> Uh.. I spoke to Michael Ellerman, and he said he expected 0x004e0200.
>> Though he did mention there might be several variants.  Can we please
>> get a definitive answer on this from IBM.
>
> Adding Michael Ellerman in cc: but I think Greg is correct. 

I don't claim to be correct :)

AFAIK Mikey's patch (linked above) is the most definitive public
documentation we have on this.

Which says:
  The P9 PVR bits 12-15 don't indicate a revision but instead different
  chip configurations.  From BookIV we have:
     Bits      Configuration
      0 :    Scale out 12 cores
      1 :    Scale out 24 cores
      2 :    Scale up  12 cores
      3 :    Scale up  24 cores

His heading of "Bits" is somewhat confusing, I'm pretty sure he just
means "the decimal value of bits 12-15", not a bit number.

Which means there may be "POWER9 DD2" chips with PVRs of:
  - 0x004e02xx
  - 0x004e12xx
  - 0x004e22xx
  - 0x004e32xx
 
So the question is just which of the scale out values to use. Neither
is technically correct, because Qemu won't necessarily be emulating a 12
or 24 core system.

Mikey did say "Linux will mostly use the "Scale out 24 core"
configuration", but AFAIK that's really just about the system designs
that are currently in plan.

Hope that makes it clear as mud :)

cheers