Re: [PATCH 5/7] target/ppc: Add privileged message send facilities

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 11030 bytes --]

On Thu, Nov 28, 2019 at 02:46:58PM +0100, Cédric Le Goater wrote:
> From: Suraj Jitindar Singh <sjitindarsingh@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.

> 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 <sjitindarsingh@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 <clg@kaod.org>
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Double signoff?

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

>  
>      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;
>      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?  I don't know
what the hardware behaviour is if you give a bad target here: does it
ignore it, or trap?

> +    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?

> +        /* 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
>  }
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]