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 <npig...@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
