On 01/18/2018 05:15 AM, David Gibson wrote: > On Tue, Jan 16, 2018 at 08:41:57AM +0100, Cédric Le Goater wrote: >> The hypervisor doorbells are used by skiboot and Linux on POWER9 >> processors to wake up secondaries. >> >> This adds processor control support to the Server architecture by >> reusing the Embedded support. They are very similar, only the bits >> definition of the CPU identifier differ. >> >> Still to be done is message broadcast to all threads of the same >> processor. >> >> Signed-off-by: Cédric Le Goater <c...@kaod.org> >> --- >> target/ppc/cpu.h | 8 ++++++-- >> target/ppc/excp_helper.c | 39 ++++++++++++++++++++++++++++++++------- >> target/ppc/helper.h | 2 +- >> target/ppc/translate.c | 13 ++++++++++++- >> target/ppc/translate_init.c | 2 +- >> 5 files changed, 52 insertions(+), 12 deletions(-) >> >> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h >> index b8f4dfc1084a..603a38cae83f 100644 >> --- a/target/ppc/cpu.h >> +++ b/target/ppc/cpu.h >> @@ -930,7 +930,7 @@ enum { >> #define BOOKE206_MAX_TLBN 4 >> >> >> /*****************************************************************************/ >> -/* Embedded.Processor Control */ >> +/* Server and Embedded Processor Control */ >> >> #define DBELL_TYPE_SHIFT 27 >> #define DBELL_TYPE_MASK (0x1f << DBELL_TYPE_SHIFT) >> @@ -940,11 +940,15 @@ enum { >> #define DBELL_TYPE_G_DBELL_CRIT (0x03 << DBELL_TYPE_SHIFT) >> #define DBELL_TYPE_G_DBELL_MC (0x04 << DBELL_TYPE_SHIFT) >> >> -#define DBELL_BRDCAST (1 << 26) >> +#define DBELL_TYPE_DBELL_SERVER (0x05 << DBELL_TYPE_SHIFT) >> + >> +#define DBELL_BRDCAST PPC_BIT(37) >> #define DBELL_LPIDTAG_SHIFT 14 >> #define DBELL_LPIDTAG_MASK (0xfff << DBELL_LPIDTAG_SHIFT) >> #define DBELL_PIRTAG_MASK 0x3fff >> >> +#define DBELL_PROCIDTAG_MASK PPC_BITMASK(44, 63) >> + >> >> /*****************************************************************************/ >> /* Segment page size information, used by recent hash MMUs >> * The format of this structure mirrors kvm_ppc_smmu_info >> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c >> index 4e548a448747..0f32cab1ff57 100644 >> --- a/target/ppc/excp_helper.c >> +++ b/target/ppc/excp_helper.c >> @@ -417,6 +417,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int >> excp_model, int excp) >> case POWERPC_EXCP_HISI: /* Hypervisor instruction storage >> exception */ >> case POWERPC_EXCP_HDSEG: /* Hypervisor data segment exception >> */ >> case POWERPC_EXCP_HISEG: /* Hypervisor instruction segment >> exception */ >> + case POWERPC_EXCP_SDOOR_HV: /* Hypervisor Doorbell interrupt >> */ >> case POWERPC_EXCP_HV_EMU: >> srr0 = SPR_HSRR0; >> srr1 = SPR_HSRR1; >> @@ -846,6 +847,11 @@ static void ppc_hw_interrupt(CPUPPCState *env) >> powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_DOORI); >> return; >> } >> + if (env->pending_interrupts & (1 << PPC_INTERRUPT_HDOORBELL)) { >> + env->pending_interrupts &= ~(1 << PPC_INTERRUPT_HDOORBELL); >> + powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_SDOOR_HV); >> + return; >> + } >> if (env->pending_interrupts & (1 << PPC_INTERRUPT_PERFM)) { >> env->pending_interrupts &= ~(1 << PPC_INTERRUPT_PERFM); >> powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_PERFM); >> @@ -1088,8 +1094,8 @@ void helper_rfsvc(CPUPPCState *env) >> do_rfi(env, env->lr, env->ctr & 0x0000FFFF); >> } >> >> -/* Embedded.Processor Control */ >> -static int dbell2irq(target_ulong rb) >> +/* Server and Embedded Processor Control */ >> +static int dbell2irq(target_ulong rb, bool book3s) >> { >> int msg = rb & DBELL_TYPE_MASK; >> int irq = -1; >> @@ -1109,12 +1115,21 @@ static int dbell2irq(target_ulong rb) >> break; >> } >> >> + /* A Directed Hypervisor Doorbell message is sent only if the >> + * message type is 5. All other types are reserved and the >> + * instruction is a no-op */ > > I don't see that the logic here accomplishes that. Other types will > return the same value as for embedded - that doesn't seem like it will > result in a no-op.
yes ... I tried to reuse existing code. I will add a specific routine for book3s. >> + if (book3s && msg == DBELL_TYPE_DBELL_SERVER) { >> + irq = PPC_INTERRUPT_HDOORBELL; >> + } >> + >> return irq; >> } >> >> void helper_msgclr(CPUPPCState *env, target_ulong rb) >> { >> - int irq = dbell2irq(rb); >> + /* 64-bit server processors compliant with arch 2.x */ >> + bool book3s = (env->insns_flags & PPC_SEGMENT_64B); > > Keying off an otherwise unrelated instruction bit seems bogus to me. That is the option we took for rfi in commit 6ca038c292d9 "ppc: restrict the use of the rfi instruction" May be we could introduce an helper for it ? > >> + int irq = dbell2irq(rb, book3s); >> >> if (irq < 0) { >> return; >> @@ -1123,10 +1138,11 @@ void helper_msgclr(CPUPPCState *env, target_ulong rb) >> env->pending_interrupts &= ~(1 << irq); >> } >> >> -void helper_msgsnd(target_ulong rb) >> +void helper_msgsnd(CPUPPCState *env, target_ulong rb) >> { >> - int irq = dbell2irq(rb); >> - int pir = rb & DBELL_PIRTAG_MASK; >> + /* 64-bit server processors compliant with arch 2.x */ >> + bool book3s = (env->insns_flags & PPC_SEGMENT_64B); >> + int irq = dbell2irq(rb, book3s); >> CPUState *cs; >> >> if (irq < 0) { >> @@ -1137,8 +1153,17 @@ void helper_msgsnd(target_ulong rb) >> CPU_FOREACH(cs) { >> PowerPCCPU *cpu = POWERPC_CPU(cs); >> CPUPPCState *cenv = &cpu->env; >> + bool send; >> >> - if ((rb & DBELL_BRDCAST) || (cenv->spr[SPR_BOOKE_PIR] == pir)) { >> + /* TODO: broadcast message to all threads of the same processor */ >> + if (book3s) { >> + int pir = rb & DBELL_PROCIDTAG_MASK; >> + send = (cenv->spr_cb[SPR_PIR].default_value == pir); >> + } else { >> + int pir = rb & DBELL_PROCIDTAG_MASK; >> + send = (rb & DBELL_BRDCAST) || (cenv->spr[SPR_BOOKE_PIR] == >> pir); >> + } >> + if (send) { >> cenv->pending_interrupts |= 1 << irq; >> cpu_interrupt(cs, CPU_INTERRUPT_HARD); >> } > > TBH, these functions are small enough and the booke vs. books > differences large enough that I think you'd be better off just having > separate implementations for the booke and books variants. Those in > turn would have separate instruction bits. ok. I will and in that case we could introduce a specific dbell2irq() for each architecture possibly ? Thanks, C. > >> diff --git a/target/ppc/helper.h b/target/ppc/helper.h >> index bb6a94a8b390..3e98bd9eecb8 100644 >> --- a/target/ppc/helper.h >> +++ b/target/ppc/helper.h >> @@ -677,7 +677,7 @@ DEF_HELPER_FLAGS_2(load_sr, TCG_CALL_NO_RWG, tl, env, tl) >> DEF_HELPER_FLAGS_3(store_sr, TCG_CALL_NO_RWG, void, env, tl, tl) >> >> DEF_HELPER_FLAGS_1(602_mfrom, TCG_CALL_NO_RWG_SE, tl, tl) >> -DEF_HELPER_1(msgsnd, void, tl) >> +DEF_HELPER_2(msgsnd, void, env, tl) >> DEF_HELPER_2(msgclr, void, env, tl) >> #endif >> >> diff --git a/target/ppc/translate.c b/target/ppc/translate.c >> index bcd36d53537f..1ad7e0fd4efd 100644 >> --- a/target/ppc/translate.c >> +++ b/target/ppc/translate.c >> @@ -6180,10 +6180,19 @@ static void gen_msgsnd(DisasContext *ctx) >> GEN_PRIV; >> #else >> CHK_HV; >> - gen_helper_msgsnd(cpu_gpr[rB(ctx->opcode)]); >> + gen_helper_msgsnd(cpu_env, cpu_gpr[rB(ctx->opcode)]); >> #endif /* defined(CONFIG_USER_ONLY) */ >> } >> >> +static void gen_msgsync(DisasContext *ctx) >> +{ >> +#if defined(CONFIG_USER_ONLY) >> + GEN_PRIV; >> +#else >> + CHK_HV; >> +#endif /* defined(CONFIG_USER_ONLY) */ >> + /* interpreted as no-op */ >> +} >> >> #if defined(TARGET_PPC64) >> static void gen_maddld(DisasContext *ctx) >> @@ -6664,6 +6673,8 @@ GEN_HANDLER2_E(msgsnd, "msgsnd", 0x1F, 0x0E, 0x06, >> 0x03ff0001, >> PPC_NONE, PPC2_PRCNTL), >> GEN_HANDLER2_E(msgclr, "msgclr", 0x1F, 0x0E, 0x07, 0x03ff0001, >> PPC_NONE, PPC2_PRCNTL), >> +GEN_HANDLER2_E(msgsync, "msgsync", 0x1F, 0x16, 0x1B, 0x00000000, >> + PPC_NONE, PPC2_PRCNTL), >> GEN_HANDLER(wrtee, 0x1F, 0x03, 0x04, 0x000FFC01, PPC_WRTEE), >> GEN_HANDLER(wrteei, 0x1F, 0x03, 0x05, 0x000E7C01, PPC_WRTEE), >> GEN_HANDLER(dlmzb, 0x1F, 0x0E, 0x02, 0x00000000, PPC_440_SPEC), >> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c >> index 70ff15a51a6b..55c99c97e377 100644 >> --- a/target/ppc/translate_init.c >> +++ b/target/ppc/translate_init.c >> @@ -8866,7 +8866,7 @@ POWERPC_FAMILY(POWER9)(ObjectClass *oc, void *data) >> PPC2_FP_TST_ISA206 | PPC2_BCTAR_ISA207 | >> PPC2_LSQ_ISA207 | PPC2_ALTIVEC_207 | >> PPC2_ISA205 | PPC2_ISA207S | PPC2_FP_CVT_S64 | >> - PPC2_TM | PPC2_PM_ISA206 | PPC2_ISA300; >> + PPC2_TM | PPC2_PM_ISA206 | PPC2_ISA300 | >> PPC2_PRCNTL; >> pcc->msr_mask = (1ull << MSR_SF) | >> (1ull << MSR_TM) | >> (1ull << MSR_VR) | >