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.

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>
---
 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);
 
     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)
+{
+    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) {
+        /* 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
 }
 
-- 
2.21.0

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

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

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

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

On Wed, Jan 08, 2020 at 04:32:19PM +0100, Cédric Le Goater wrote:
> 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 <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.
> 
> 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 <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?
> 
> 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 ?

Sorry, I don't understand the question.  The second argument to
book3s_dbell2irq() is a 'bool' type, so the parameter should be 'true'
rather than '1'.

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

Ok..

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

Ok.

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

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

>>>>  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 ?
> 
> Sorry, I don't understand the question.  The second argument to
> book3s_dbell2irq() is a 'bool' type, so the parameter should be 'true'
> rather than '1'.

Yes. I was thinking of adding defines for the 'true' and 'false' value
to clarify what the parameter is doing. But it does not seem really 
useful now.

Thanks,

C.

On Thu, Jan 09, 2020 at 08:13:38AM +0100, Cédric Le Goater wrote:
> >>>>  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 ?
> > 
> > Sorry, I don't understand the question.  The second argument to
> > book3s_dbell2irq() is a 'bool' type, so the parameter should be 'true'
> > rather than '1'.
> 
> Yes. I was thinking of adding defines for the 'true' and 'false' value
> to clarify what the parameter is doing. But it does not seem really 
> useful now.

You don't need your own defines, they already exist.

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