On 12/17/19 5:00 AM, David Gibson wrote: > On Thu, Nov 28, 2019 at 02:46:58PM +0100, Cédric Le Goater wrote: >> From: Suraj Jitindar Singh <sjitindarsi...@gmail.com> >> >> Privileged message send facilities exist on POWER8 processors and >> later and include a register and instructions which can be used to >> generate, observe/modify the state of and clear privileged doorbell >> exceptions as described below. > > So, it's probably worth noting somewhere here that the "privileged > doorbell" instructions, being supervisor privileged than the existing > doorbell instructions which are hypervisor privileged.
Sure. I will add a sentence on the topic. >> The Directed Privileged Doorbell Exception State (DPDES) register >> reflects the state of pending privileged doorbell exceptions and can >> also be used to modify that state. The register can be used to read >> and modify the state of privileged doorbell exceptions for all threads >> of a subprocessor and thus is a shared facility for that subprocessor. >> The register can be read/written by the hypervisor and read by the >> supervisor if enabled in the HFSCR, otherwise a hypervisor facility >> unavailable exception is generated. >> >> The privileged message send and clear instructions (msgsndp & msgclrp) >> are used to generate and clear the presence of a directed privileged >> doorbell exception, respectively. The msgsndp instruction can be used >> to target any thread of the current subprocessor, msgclrp acts on the >> thread issuing the instruction. These instructions are privileged, but >> will generate a hypervisor facility unavailable exception if not >> enabled in the HFSCR and executed in privileged non-hypervisor >> state. The HV facility unavailable exception will be addressed in >> other patch. >> >> Add and implement this register and instructions by reading or >> modifying the pending interrupt state of the cpu. >> >> Note that TCG only supports one thread per core and so we only need to >> worry about the cpu making the access. >> >> Signed-off-by: Suraj Jitindar Singh <sjitindarsi...@gmail.com> >> [clg: - introduced a book3s_msgsnd_common() helper to reduce code >> duplication >> - modified book3s_dbell2irq() >> - moved out the support for hypervisor facility unavailable >> exception >> - modified commit log to add a statement on the HV FU exception >> - checkpatch fixes >> - compile fixes for the ppc-softmmu and linux-user targets ] >> Signed-off-by: Cédric Le Goater <c...@kaod.org> >> >> Signed-off-by: Cédric Le Goater <c...@kaod.org> > > Double signoff? I don't know why git-am added an extra one :/ I hope this is gone. >> --- >> target/ppc/helper.h | 4 +++ >> target/ppc/excp_helper.c | 43 ++++++++++++++++++++++++++++----- >> target/ppc/misc_helper.c | 22 +++++++++++++++++ >> target/ppc/translate.c | 26 ++++++++++++++++++++ >> target/ppc/translate_init.inc.c | 20 ++++++++++++--- >> 5 files changed, 105 insertions(+), 10 deletions(-) >> >> diff --git a/target/ppc/helper.h b/target/ppc/helper.h >> index cd0dfe383a2a..76518a1df6f0 100644 >> --- a/target/ppc/helper.h >> +++ b/target/ppc/helper.h >> @@ -657,6 +657,10 @@ DEF_HELPER_FLAGS_1(load_601_rtcu, TCG_CALL_NO_RWG, tl, >> env) >> DEF_HELPER_FLAGS_1(load_purr, TCG_CALL_NO_RWG, tl, env) >> DEF_HELPER_FLAGS_2(store_purr, TCG_CALL_NO_RWG, void, env, tl) >> DEF_HELPER_2(store_ptcr, void, env, tl) >> +DEF_HELPER_FLAGS_1(load_dpdes, TCG_CALL_NO_RWG, tl, env) >> +DEF_HELPER_FLAGS_2(store_dpdes, TCG_CALL_NO_RWG, void, env, tl) >> +DEF_HELPER_1(book3s_msgsndp, void, tl) >> +DEF_HELPER_2(book3s_msgclrp, void, env, tl) >> #endif >> DEF_HELPER_2(store_sdr1, void, env, tl) >> DEF_HELPER_2(store_pidr, void, env, tl) >> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c >> index 50b004d00d1e..5a247945e97f 100644 >> --- a/target/ppc/excp_helper.c >> +++ b/target/ppc/excp_helper.c >> @@ -898,7 +898,11 @@ static void ppc_hw_interrupt(CPUPPCState *env) >> } >> if (env->pending_interrupts & (1 << PPC_INTERRUPT_DOORBELL)) { >> env->pending_interrupts &= ~(1 << PPC_INTERRUPT_DOORBELL); >> - powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_DOORI); >> + if (env->insns_flags & PPC_SEGMENT_64B) { >> + powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_SDOOR); >> + } else { >> + powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_DOORI); >> + } >> return; >> } >> if (env->pending_interrupts & (1 << PPC_INTERRUPT_HDOORBELL)) { >> @@ -1219,7 +1223,7 @@ void helper_msgsnd(target_ulong rb) >> } >> >> /* Server Processor Control */ >> -static int book3s_dbell2irq(target_ulong rb) >> +static int book3s_dbell2irq(target_ulong rb, bool hv_dbell) >> { >> int msg = rb & DBELL_TYPE_MASK; >> >> @@ -1228,12 +1232,16 @@ static int book3s_dbell2irq(target_ulong rb) >> * message type is 5. All other types are reserved and the >> * instruction is a no-op >> */ >> - return msg == DBELL_TYPE_DBELL_SERVER ? PPC_INTERRUPT_HDOORBELL : -1; >> + if (msg == DBELL_TYPE_DBELL_SERVER) { >> + return hv_dbell ? PPC_INTERRUPT_HDOORBELL : PPC_INTERRUPT_DOORBELL; >> + } >> + >> + return -1; >> } >> >> void helper_book3s_msgclr(CPUPPCState *env, target_ulong rb) >> { >> - int irq = book3s_dbell2irq(rb); >> + int irq = book3s_dbell2irq(rb, 1); > > true/false are preferred to 0/1 for bool types. yes or a define ? >> >> if (irq < 0) { >> return; >> @@ -1242,9 +1250,9 @@ void helper_book3s_msgclr(CPUPPCState *env, >> target_ulong rb) >> env->pending_interrupts &= ~(1 << irq); >> } >> >> -void helper_book3s_msgsnd(target_ulong rb) >> +static void book3s_msgsnd_common(target_ulong rb, bool hv_dbell) >> { >> - int irq = book3s_dbell2irq(rb); >> + int irq = book3s_dbell2irq(rb, hv_dbell); >> int pir = rb & DBELL_PROCIDTAG_MASK; btw, this mask is wrong for msgsndp(). >> CPUState *cs; >> >> @@ -1265,6 +1273,29 @@ void helper_book3s_msgsnd(target_ulong rb) >> } >> qemu_mutex_unlock_iothread(); >> } >> + >> +void helper_book3s_msgsnd(target_ulong rb) >> +{ > > As noted you only need to worry about a single thread here. But > shouldn't you validate or mask the target against that? The mask is applied in book3s_msgsnd_common() but I am going to change that, as the TAG bit fields have different sizes. > I don't know > what the hardware behaviour is if you give a bad target here: does it > ignore it, or trap? bad targets are no-ops. Just like for msgsnd(). >> + book3s_msgsnd_common(rb, 1); >> +} >> + >> +#if defined(TARGET_PPC64) >> +void helper_book3s_msgclrp(CPUPPCState *env, target_ulong rb) >> +{ >> + int irq = book3s_dbell2irq(rb, 0); >> + >> + if (irq < 0) { >> + return; >> + } >> + >> + env->pending_interrupts &= ~(1 << irq); >> +} >> + >> +void helper_book3s_msgsndp(target_ulong rb) >> +{ >> + book3s_msgsnd_common(rb, 0); >> +} >> +#endif >> #endif >> >> void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr, >> diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c >> index 2318f3ab45b2..a0e7bd9c32d3 100644 >> --- a/target/ppc/misc_helper.c >> +++ b/target/ppc/misc_helper.c >> @@ -105,6 +105,28 @@ void helper_store_pcr(CPUPPCState *env, target_ulong >> value) >> >> env->spr[SPR_PCR] = value & pcc->pcr_mask; >> } >> + >> +target_ulong helper_load_dpdes(CPUPPCState *env) >> +{ >> + if (env->pending_interrupts & (1 << PPC_INTERRUPT_DOORBELL)) { >> + return 1; >> + } >> + return 0; >> +} >> + >> +void helper_store_dpdes(CPUPPCState *env, target_ulong val) >> +{ >> + PowerPCCPU *cpu = env_archcpu(env); >> + CPUState *cs = CPU(cpu); >> + >> + if (val) { > > Likewise, shouldn't val be masked or validated, rather than treating > any non-zero value as setting the interrupt for this thread? This is a bit wrong indeed. each bit number corresponds to a HW thread of a core. DPDES being a shared register. The current implementation is fine for SMT=1 but we should add a warning for DPDES values modifying bits other than bit 63. Thanks, C. > >> + /* Only one cpu for now */ >> + env->pending_interrupts |= 1 << PPC_INTERRUPT_DOORBELL; >> + cpu_interrupt(cs, CPU_INTERRUPT_HARD); >> + } else { >> + env->pending_interrupts &= ~(1 << PPC_INTERRUPT_DOORBELL); >> + } >> +} >> #endif /* defined(TARGET_PPC64) */ >> >> void helper_store_pidr(CPUPPCState *env, target_ulong val) >> diff --git a/target/ppc/translate.c b/target/ppc/translate.c >> index f5fe5d06118a..ba759ab2bb0f 100644 >> --- a/target/ppc/translate.c >> +++ b/target/ppc/translate.c >> @@ -6645,6 +6645,28 @@ static void gen_msgsnd(DisasContext *ctx) >> #endif /* defined(CONFIG_USER_ONLY) */ >> } >> >> +#if defined(TARGET_PPC64) >> +static void gen_msgclrp(DisasContext *ctx) >> +{ >> +#if defined(CONFIG_USER_ONLY) >> + GEN_PRIV; >> +#else >> + CHK_SV; >> + gen_helper_book3s_msgclrp(cpu_env, cpu_gpr[rB(ctx->opcode)]); >> +#endif /* defined(CONFIG_USER_ONLY) */ >> +} >> + >> +static void gen_msgsndp(DisasContext *ctx) >> +{ >> +#if defined(CONFIG_USER_ONLY) >> + GEN_PRIV; >> +#else >> + CHK_SV; >> + gen_helper_book3s_msgsndp(cpu_gpr[rB(ctx->opcode)]); >> +#endif /* defined(CONFIG_USER_ONLY) */ >> +} >> +#endif >> + >> static void gen_msgsync(DisasContext *ctx) >> { >> #if defined(CONFIG_USER_ONLY) >> @@ -7187,6 +7209,10 @@ GEN_HANDLER(vmladduhm, 0x04, 0x11, 0xFF, 0x00000000, >> PPC_ALTIVEC), >> GEN_HANDLER_E(maddhd_maddhdu, 0x04, 0x18, 0xFF, 0x00000000, PPC_NONE, >> PPC2_ISA300), >> GEN_HANDLER_E(maddld, 0x04, 0x19, 0xFF, 0x00000000, PPC_NONE, PPC2_ISA300), >> +GEN_HANDLER2_E(msgsndp, "msgsndp", 0x1F, 0x0E, 0x04, 0x03ff0001, >> + PPC_NONE, PPC2_ISA207S), >> +GEN_HANDLER2_E(msgclrp, "msgclrp", 0x1F, 0x0E, 0x05, 0x03ff0001, >> + PPC_NONE, PPC2_ISA207S), >> #endif >> >> #undef GEN_INT_ARITH_ADD >> diff --git a/target/ppc/translate_init.inc.c >> b/target/ppc/translate_init.inc.c >> index 9815df6f77c8..7c74a763ba66 100644 >> --- a/target/ppc/translate_init.inc.c >> +++ b/target/ppc/translate_init.inc.c >> @@ -464,6 +464,17 @@ static void spr_write_pcr(DisasContext *ctx, int sprn, >> int gprn) >> { >> gen_helper_store_pcr(cpu_env, cpu_gpr[gprn]); >> } >> + >> +/* DPDES */ >> +static void spr_read_dpdes(DisasContext *ctx, int gprn, int sprn) >> +{ >> + gen_helper_load_dpdes(cpu_gpr[gprn], cpu_env); >> +} >> + >> +static void spr_write_dpdes(DisasContext *ctx, int sprn, int gprn) >> +{ >> + gen_helper_store_dpdes(cpu_env, cpu_gpr[gprn]); >> +} >> #endif >> #endif >> >> @@ -8233,10 +8244,11 @@ static void gen_spr_power8_dpdes(CPUPPCState *env) >> { >> #if !defined(CONFIG_USER_ONLY) >> /* Directed Privileged Door-bell Exception State, used for IPI */ >> - spr_register(env, SPR_DPDES, "DPDES", >> - SPR_NOACCESS, SPR_NOACCESS, >> - &spr_read_generic, SPR_NOACCESS, >> - 0x00000000); >> + spr_register_kvm_hv(env, SPR_DPDES, "DPDES", >> + SPR_NOACCESS, SPR_NOACCESS, >> + &spr_read_dpdes, SPR_NOACCESS, >> + &spr_read_dpdes, &spr_write_dpdes, >> + KVM_REG_PPC_DPDES, 0x00000000); >> #endif >> } >> >