[Qemu-devel] [RFC] powernv: CPU compatibility modes don't make sense for powernv

[Qemu-devel] [RFC] powernv: CPU compatibility modes don't make sense for powernv

[David Gibson]

powernv has some code (derived from the spapr equivalent) used in device
tree generation which depends on the CPU's compatibility mode / logical
PVR.  However, compatibility modes don't make sense on powernv - at least
not as a property controlled by the host - because the guest in powernv
has full hypervisor level access to the virtual system, and so owns the
PCR (Processor Compatibility Register) which implements compatiblity modes.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/pnv.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

Hi Cédric, I'd appreciate it if you can double check my reasoning
here.  This patch gets powernv out of the way of a bunch of
compatibility mode cleanups I have in the works.

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 82276e0..6af3424 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -110,7 +110,7 @@ static void powernv_create_core_node(PnvChip *chip, PnvCore *pc, void *fdt)
     CPUState *cs = CPU(DEVICE(pc->threads));
     DeviceClass *dc = DEVICE_GET_CLASS(cs);
     PowerPCCPU *cpu = POWERPC_CPU(cs);
-    int smt_threads = ppc_get_compat_smt_threads(cpu);
+    int smt_threads = CPU_CORE(pc)->nr_threads;
     CPUPPCState *env = &cpu->env;
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
     uint32_t servers_prop[smt_threads];
@@ -206,10 +206,6 @@ static void powernv_create_core_node(PnvChip *chip, PnvCore *pc, void *fdt)
     _FDT((fdt_setprop(fdt, offset, "ibm,pa-features",
                        pa_features, sizeof(pa_features))));
 
-    if (cpu->cpu_version) {
-        _FDT((fdt_setprop_cell(fdt, offset, "cpu-version", cpu->cpu_version)));
-    }
-
     /* Build interrupt servers properties */
     for (i = 0; i < smt_threads; i++) {
         servers_prop[i] = cpu_to_be32(pc->pir + i);
-- 
2.7.4

Re: [Qemu-devel] [RFC] powernv: CPU compatibility modes don't make sense for powernv

[Greg Kurz]

On Fri, 28 Oct 2016 13:05:01 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> powernv has some code (derived from the spapr equivalent) used in device
> tree generation which depends on the CPU's compatibility mode / logical
> PVR.  However, compatibility modes don't make sense on powernv - at least
> not as a property controlled by the host - because the guest in powernv
> has full hypervisor level access to the virtual system, and so owns the
> PCR (Processor Compatibility Register) which implements compatiblity modes.
> 

This makes a lot of sense indeed.

> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/pnv.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> Hi Cédric, I'd appreciate it if you can double check my reasoning
> here.  This patch gets powernv out of the way of a bunch of
> compatibility mode cleanups I have in the works.
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 82276e0..6af3424 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -110,7 +110,7 @@ static void powernv_create_core_node(PnvChip *chip, PnvCore *pc, void *fdt)
>      CPUState *cs = CPU(DEVICE(pc->threads));
>      DeviceClass *dc = DEVICE_GET_CLASS(cs);
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
> -    int smt_threads = ppc_get_compat_smt_threads(cpu);
> +    int smt_threads = CPU_CORE(pc)->nr_threads;

Not speaking of the PCR, this has also another effect since 
ppc_get_compat_smt_threads() also take the KVM smt capability
into account:

    int ret = MIN(cs->nr_threads, kvmppc_smt_threads());

Shouldn't you do the same here ?

>      CPUPPCState *env = &cpu->env;
>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
>      uint32_t servers_prop[smt_threads];
> @@ -206,10 +206,6 @@ static void powernv_create_core_node(PnvChip *chip, PnvCore *pc, void *fdt)
>      _FDT((fdt_setprop(fdt, offset, "ibm,pa-features",
>                         pa_features, sizeof(pa_features))));
>  
> -    if (cpu->cpu_version) {
> -        _FDT((fdt_setprop_cell(fdt, offset, "cpu-version", cpu->cpu_version)));
> -    }
> -
>      /* Build interrupt servers properties */
>      for (i = 0; i < smt_threads; i++) {
>          servers_prop[i] = cpu_to_be32(pc->pir + i);

Re: [Qemu-devel] [RFC] powernv: CPU compatibility modes don't make sense for powernv

[David Gibson]

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

On Fri, Oct 28, 2016 at 11:32:43AM +0200, Greg Kurz wrote:
> On Fri, 28 Oct 2016 13:05:01 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > powernv has some code (derived from the spapr equivalent) used in device
> > tree generation which depends on the CPU's compatibility mode / logical
> > PVR.  However, compatibility modes don't make sense on powernv - at least
> > not as a property controlled by the host - because the guest in powernv
> > has full hypervisor level access to the virtual system, and so owns the
> > PCR (Processor Compatibility Register) which implements compatiblity modes.
> > 
> 
> This makes a lot of sense indeed.
> 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/pnv.c | 6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> > 
> > Hi Cédric, I'd appreciate it if you can double check my reasoning
> > here.  This patch gets powernv out of the way of a bunch of
> > compatibility mode cleanups I have in the works.
> > 
> > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > index 82276e0..6af3424 100644
> > --- a/hw/ppc/pnv.c
> > +++ b/hw/ppc/pnv.c
> > @@ -110,7 +110,7 @@ static void powernv_create_core_node(PnvChip *chip, PnvCore *pc, void *fdt)
> >      CPUState *cs = CPU(DEVICE(pc->threads));
> >      DeviceClass *dc = DEVICE_GET_CLASS(cs);
> >      PowerPCCPU *cpu = POWERPC_CPU(cs);
> > -    int smt_threads = ppc_get_compat_smt_threads(cpu);
> > +    int smt_threads = CPU_CORE(pc)->nr_threads;
> 
> Not speaking of the PCR, this has also another effect since 
> ppc_get_compat_smt_threads() also take the KVM smt capability
> into account:
> 
>     int ret = MIN(cs->nr_threads, kvmppc_smt_threads());
> 
> Shouldn't you do the same here ?

Actually, no, though possible that needs mention in the commit
message.  I'm pretty sure there are checks elsewhere that ensure
nr_threads <= kvmppc_smt_threads().  If not, having more virtual
threads than kvmppc allows certainly can't work, so we're no more
broken than before.

Plus, kvmppc_smt_threads() is really only relevant for KVM HV which
doesn't, and can't support the powernv machine type anyway.

-- 
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] [RFC] powernv: CPU compatibility modes don't make sense for powernv

[Greg Kurz]

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

On Fri, 28 Oct 2016 22:14:12 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Oct 28, 2016 at 11:32:43AM +0200, Greg Kurz wrote:
> > On Fri, 28 Oct 2016 13:05:01 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > powernv has some code (derived from the spapr equivalent) used in device
> > > tree generation which depends on the CPU's compatibility mode / logical
> > > PVR.  However, compatibility modes don't make sense on powernv - at least
> > > not as a property controlled by the host - because the guest in powernv
> > > has full hypervisor level access to the virtual system, and so owns the
> > > PCR (Processor Compatibility Register) which implements compatiblity modes.
> > >   
> > 
> > This makes a lot of sense indeed.
> >   
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  hw/ppc/pnv.c | 6 +-----
> > >  1 file changed, 1 insertion(+), 5 deletions(-)
> > > 
> > > Hi Cédric, I'd appreciate it if you can double check my reasoning
> > > here.  This patch gets powernv out of the way of a bunch of
> > > compatibility mode cleanups I have in the works.
> > > 
> > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > > index 82276e0..6af3424 100644
> > > --- a/hw/ppc/pnv.c
> > > +++ b/hw/ppc/pnv.c
> > > @@ -110,7 +110,7 @@ static void powernv_create_core_node(PnvChip *chip, PnvCore *pc, void *fdt)
> > >      CPUState *cs = CPU(DEVICE(pc->threads));
> > >      DeviceClass *dc = DEVICE_GET_CLASS(cs);
> > >      PowerPCCPU *cpu = POWERPC_CPU(cs);
> > > -    int smt_threads = ppc_get_compat_smt_threads(cpu);
> > > +    int smt_threads = CPU_CORE(pc)->nr_threads;  
> > 
> > Not speaking of the PCR, this has also another effect since 
> > ppc_get_compat_smt_threads() also take the KVM smt capability
> > into account:
> > 
> >     int ret = MIN(cs->nr_threads, kvmppc_smt_threads());
> > 
> > Shouldn't you do the same here ?  
> 
> Actually, no, though possible that needs mention in the commit
> message.  I'm pretty sure there are checks elsewhere that ensure
> nr_threads <= kvmppc_smt_threads().  If not, having more virtual
> threads than kvmppc allows certainly can't work, so we're no more
> broken than before.
> 

Indeed, there's only one in ppc_cpu_realizefn() and I'm not sure all
vCPUs need to perform this same check. But you're right, the check does
not belong here either, it should probably be done in the machine init
function.

> Plus, kvmppc_smt_threads() is really only relevant for KVM HV which
> doesn't, and can't support the powernv machine type anyway.
> 

kvmppc_smt_threads() is actually ill-named since it is used to know
the number of threads supported by the CPU accelerator, which, I agree,
is 1 for anything but KVM HV at the present time. Maybe it could change
the day we support the emulation of multiple threads.

So in the end, this patch looks the way to go.

Reviewed-by: Greg Kurz <groug@kaod.org>

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

Re: [Qemu-devel] [RFC] powernv: CPU compatibility modes don't make sense for powernv

[Cédric Le Goater]

On 10/28/2016 04:05 AM, David Gibson wrote:
> powernv has some code (derived from the spapr equivalent) used in device
> tree generation which depends on the CPU's compatibility mode / logical
> PVR.  However, compatibility modes don't make sense on powernv - at least
> not as a property controlled by the host - because the guest in powernv
> has full hypervisor level access to the virtual system, and so owns the
> PCR (Processor Compatibility Register) which implements compatiblity modes.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/pnv.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> Hi Cédric, I'd appreciate it if you can double check my reasoning
> here.  This patch gets powernv out of the way of a bunch of
> compatibility mode cleanups I have in the works.

It makes perfect sense. The "cpu-version" property is for PAPR, not for OPAL.
hostboot and skiboot put SPR_PVR in this property. 

I will be careful using 'CPU_CORE(pc)->nr_threads' in the ICP patches also. 

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.


> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 82276e0..6af3424 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -110,7 +110,7 @@ static void powernv_create_core_node(PnvChip *chip, PnvCore *pc, void *fdt)
>      CPUState *cs = CPU(DEVICE(pc->threads));
>      DeviceClass *dc = DEVICE_GET_CLASS(cs);
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
> -    int smt_threads = ppc_get_compat_smt_threads(cpu);
> +    int smt_threads = CPU_CORE(pc)->nr_threads;
>      CPUPPCState *env = &cpu->env;
>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
>      uint32_t servers_prop[smt_threads];
> @@ -206,10 +206,6 @@ static void powernv_create_core_node(PnvChip *chip, PnvCore *pc, void *fdt)
>      _FDT((fdt_setprop(fdt, offset, "ibm,pa-features",
>                         pa_features, sizeof(pa_features))));
>  
> -    if (cpu->cpu_version) {
> -        _FDT((fdt_setprop_cell(fdt, offset, "cpu-version", cpu->cpu_version)));
> -    }
> -
>      /* Build interrupt servers properties */
>      for (i = 0; i < smt_threads; i++) {
>          servers_prop[i] = cpu_to_be32(pc->pir + i);
> 

Re: [Qemu-devel] [RFC] powernv: CPU compatibility modes don't make sense for powernv

[Benjamin Herrenschmidt]

On Fri, 2016-10-28 at 18:40 +0200, Cédric Le Goater wrote:
> 
> It makes perfect sense. The "cpu-version" property is for PAPR, not
> for OPAL.
> hostboot and skiboot put SPR_PVR in this property. 
> 
> I will be careful using 'CPU_CORE(pc)->nr_threads' in the ICP patches
> also. 

No, the cpu-version is part of the old OF bindings afaik, at least it
exists under OPAL, so it should be there but yes, it should just
contain the PVR.

Cheers,
Ben.

> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> Thanks,
> 
> C.
> 
> 
> > 
> > 
> > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > index 82276e0..6af3424 100644
> > --- a/hw/ppc/pnv.c
> > +++ b/hw/ppc/pnv.c
> > @@ -110,7 +110,7 @@ static void powernv_create_core_node(PnvChip
> > *chip, PnvCore *pc, void *fdt)
> >      CPUState *cs = CPU(DEVICE(pc->threads));
> >      DeviceClass *dc = DEVICE_GET_CLASS(cs);
> >      PowerPCCPU *cpu = POWERPC_CPU(cs);
> > -    int smt_threads = ppc_get_compat_smt_threads(cpu);
> > +    int smt_threads = CPU_CORE(pc)->nr_threads;
> >      CPUPPCState *env = &cpu->env;
> >      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
> >      uint32_t servers_prop[smt_threads];
> > @@ -206,10 +206,6 @@ static void powernv_create_core_node(PnvChip
> > *chip, PnvCore *pc, void *fdt)
> >      _FDT((fdt_setprop(fdt, offset, "ibm,pa-features",
> >                         pa_features, sizeof(pa_features))));
> >  
> > -    if (cpu->cpu_version) {
> > -        _FDT((fdt_setprop_cell(fdt, offset, "cpu-version", cpu-
> > >cpu_version)));
> > -    }
> > -
> >      /* Build interrupt servers properties */
> >      for (i = 0; i < smt_threads; i++) {
> >          servers_prop[i] = cpu_to_be32(pc->pir + i);
> > 

Re: [Qemu-devel] [RFC] powernv: CPU compatibility modes don't make sense for powernv

[Cédric Le Goater]

On 10/28/2016 06:46 PM, Benjamin Herrenschmidt wrote:
> On Fri, 2016-10-28 at 18:40 +0200, Cédric Le Goater wrote:
>>  
>> It makes perfect sense. The "cpu-version" property is for PAPR, not
>> for OPAL.
>> hostboot and skiboot put SPR_PVR in this property. 
>>
>> I will be careful using 'CPU_CORE(pc)->nr_threads' in the ICP patches
>> also. 
> 
> No, the cpu-version is part of the old OF bindings afaik, at least it
> exists under OPAL, so it should be there but yes, it should just
> contain the PVR.

powernv_create_core_node() sets "cpu-version" twice :

    _FDT((fdt_setprop_cell(fdt, offset, "cpu-version", env->spr[SPR_PVR])));

and then : 

    if (cpu->cpu_version) {
        _FDT((fdt_setprop_cell(fdt, offset, "cpu-version", cpu->cpu_version)));
    }

but the patch only removes the second one but you don't really see the first ...
I should have said that.

Cheers,

C. 

> Cheers,
> Ben.
> 
>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>
>> Thanks,
>>
>> C.
>>
>>
>>>
>>>
>>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>>> index 82276e0..6af3424 100644
>>> --- a/hw/ppc/pnv.c
>>> +++ b/hw/ppc/pnv.c
>>> @@ -110,7 +110,7 @@ static void powernv_create_core_node(PnvChip
>>> *chip, PnvCore *pc, void *fdt)
>>>      CPUState *cs = CPU(DEVICE(pc->threads));
>>>      DeviceClass *dc = DEVICE_GET_CLASS(cs);
>>>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>>> -    int smt_threads = ppc_get_compat_smt_threads(cpu);
>>> +    int smt_threads = CPU_CORE(pc)->nr_threads;
>>>      CPUPPCState *env = &cpu->env;
>>>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
>>>      uint32_t servers_prop[smt_threads];
>>> @@ -206,10 +206,6 @@ static void powernv_create_core_node(PnvChip
>>> *chip, PnvCore *pc, void *fdt)
>>>      _FDT((fdt_setprop(fdt, offset, "ibm,pa-features",
>>>                         pa_features, sizeof(pa_features))));
>>>  
>>> -    if (cpu->cpu_version) {
>>> -        _FDT((fdt_setprop_cell(fdt, offset, "cpu-version", cpu-
>>>> cpu_version)));
>>> -    }
>>> -
>>>      /* Build interrupt servers properties */
>>>      for (i = 0; i < smt_threads; i++) {
>>>          servers_prop[i] = cpu_to_be32(pc->pir + i);
>>>