Re: [RFC PATCH 4/5] target/ppc: Add msgsnd/p and DPDES SMT support

[Nicholas Piggin]

On Thu Jun 1, 2023 at 5:13 PM AEST, Cédric Le Goater wrote:
> On 5/31/23 03:23, Nicholas Piggin wrote:
> > Doorbells in SMT need to coordinate msgsnd/msgclr and DPDES access from
> > multiple threads that affect the same state.
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >   hw/ppc/ppc.c                                  |  6 ++
> >   include/hw/ppc/ppc.h                          |  1 +
> >   target/ppc/cpu.h                              |  7 +-
> >   target/ppc/excp_helper.c                      | 86 +++++++++++++------
> >   target/ppc/gdbstub.c                          |  2 +-
> >   target/ppc/helper.h                           |  2 +-
> >   target/ppc/misc_helper.c                      | 60 +++++++++++--
> >   target/ppc/translate.c                        |  8 ++
> >   .../ppc/translate/processor-ctrl-impl.c.inc   |  2 +-
> >   9 files changed, 140 insertions(+), 34 deletions(-)
> > 
> > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> > index 80b4706db2..e30853413b 100644
> > --- a/hw/ppc/ppc.c
> > +++ b/hw/ppc/ppc.c
> > @@ -1434,6 +1434,12 @@ int ppc_cpu_pir(PowerPCCPU *cpu)
> >       return env->spr_cb[SPR_PIR].default_value;
> >   }
> >   
> > +int ppc_cpu_tir(PowerPCCPU *cpu)
> > +{
> > +    CPUPPCState *env = &cpu->env;
> > +    return env->spr_cb[SPR_PIR].default_value;
>
> PIR or TIR ?

Good catch, I think I "tidied" that up before sending it :\

> > @@ -3154,22 +3172,42 @@ void helper_book3s_msgclrp(CPUPPCState *env, 
> > target_ulong rb)
> >   }
> >   
> >   /*
> > - * sends a message to other threads that are on the same
> > + * sends a message to another thread  on the same
> >    * multi-threaded processor
> >    */
> >   void helper_book3s_msgsndp(CPUPPCState *env, target_ulong rb)
> >   {
> > -    int pir = env->spr_cb[SPR_PIR].default_value;
> > +    CPUState *cs = env_cpu(env);
> > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +    CPUState *ccs;
> > +    uint32_t nr_threads = cs->nr_threads;
> > +    int ttir = rb & PPC_BITMASK(57, 63);
> >   
> >       helper_hfscr_facility_check(env, HFSCR_MSGP, "msgsndp", 
> > HFSCR_IC_MSGP);
> >   
> > -    if (!dbell_type_server(rb)) {
> > +    if (!dbell_type_server(rb) || ttir >= nr_threads) {
>
> may be log bad ttir values ? even if the insn is a no-op in that case,
> telling the user would be good since it should be a guest os issue

Yeah. We don't seem to do that on a systematic basis in PPC but we
probably should. It's certainly helped me before when I've got
something wrong. Good idea.

> > @@ -192,14 +192,38 @@ void helper_store_pcr(CPUPPCState *env, target_ulong 
> > value)
> >    */
> >   target_ulong helper_load_dpdes(CPUPPCState *env)
> >   {
> > +    CPUState *cs = env_cpu(env);
> > +    CPUState *ccs;
> > +    uint32_t nr_threads = cs->nr_threads;
> > +    uint32_t core_id = env->spr[SPR_PIR] & ~(nr_threads - 1);
>
> you could add an helper for the above.

Yes.

Thanks,
Nick