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

[Greg Kurz <groug@kaod.org>]

[-- 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 --]