Re: [RFC PATCH 5/5] spapr: Allow up to 8 threads SMT configuration

[Nicholas Piggin]

On Thu Jun 1, 2023 at 5:20 PM AEST, Cédric Le Goater wrote:
> On 5/31/23 03:23, Nicholas Piggin wrote:
> > TCG now supports multi-threaded configuration at least enough for
> > pseries to be functional enough to boot Linux.
> > 
> > This requires PIR and TIR be set, because that's how sibling thread
> > matching is done.
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >   hw/ppc/spapr.c          | 4 ++--
> >   hw/ppc/spapr_cpu_core.c | 7 +++++--
> >   2 files changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index dcb7f1c70a..11074cefea 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2524,8 +2524,8 @@ static void spapr_set_vsmt_mode(SpaprMachineState 
> > *spapr, Error **errp)
> >       int ret;
> >       unsigned int smp_threads = ms->smp.threads;
> >   
> > -    if (!kvm_enabled() && (smp_threads > 1)) {
> > -        error_setg(errp, "TCG cannot support more than 1 thread/core "
> > +    if (!kvm_enabled() && (smp_threads > 8)) {
> > +        error_setg(errp, "TCG cannot support more than 8 threads/core "
> >                      "on a pseries machine");
>
> I think we should add test on the CPU also.

On the CPU type, POWER7 can have 1/2/4, POWER8 can have 1/2/4/8?
POWER9 could also switch PVR between big and small core depending
on whether you select SMT8 I suppose.

> >           return;
> >       }
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 9b88dd549a..f35ee600f1 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -255,7 +255,7 @@ static void spapr_cpu_core_unrealize(DeviceState *dev)
> >   }
> >   
> >   static bool spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
> > -                               SpaprCpuCore *sc, Error **errp)
> > +                               SpaprCpuCore *sc, int thread_nr, Error 
> > **errp)
>
> thread_index may be ?

Sure.

> >   {
> >       CPUPPCState *env = &cpu->env;
> >       CPUState *cs = CPU(cpu);
> > @@ -267,6 +267,9 @@ static bool spapr_realize_vcpu(PowerPCCPU *cpu, 
> > SpaprMachineState *spapr,
> >       cpu_ppc_set_vhyp(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
> >       kvmppc_set_papr(cpu);
>
> so, spapr_create_vcpu() set cs->cpu_index :
>      cs->cpu_index = cc->core_id + i;
>
> and spapr_realize_vcpu :
>     
> > +    env->spr_cb[SPR_PIR].default_value = cs->cpu_index;
> > +    env->spr_cb[SPR_TIR].default_value = thread_nr;
> > +
> it would be cleaner to do the SPR assignment in one place.

I'll try that, it sounds good.

Thanks,
Nick