| Message ID | 1477620301-24822-1-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
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);
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.
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>
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); >
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); > >
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); >>>
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);
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.