| Message ID | 20170622112648.24815-1-lvivier@redhat.com |
|---|---|
| State | New |
| Headers | show |
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>
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.
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
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
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 >
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 > >
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
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?
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.
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
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.
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
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
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
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 > >
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 > > > > > >
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 > > > > > > > > > >
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
[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.
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.
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 >
>>> 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.
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
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
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,
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(-)