[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 5/7] target/ppc: Add privileged message send facilities
From: |
Cédric Le Goater |
Subject: |
Re: [PATCH 5/7] target/ppc: Add privileged message send facilities |
Date: |
Wed, 8 Jan 2020 16:32:19 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 |
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 <address@hidden>
>>
>> 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 <address@hidden>
>> [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 <address@hidden>
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
>
> 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
>> }
>>
>
- Re: [PATCH 5/7] target/ppc: Add privileged message send facilities,
Cédric Le Goater <=