Re: [Qemu-devel] [PATCH 13/17] ppc: Add POWER8 IAMR register

[Thomas Huth]

On 14.03.2016 17:56, Cédric Le Goater wrote:
> From: Benjamin Herrenschmidt 
> 
> With appropriate AMR-like masks. Not actually used by the translation
> logic at that point
> 
> Signed-off-by: Benjamin Herrenschmidt 
> [clg: add the use of spr_register_kvm_hv()]
> Signed-off-by: Cédric Le Goater 
> ---
>  target-ppc/cpu.h|  1 +
>  target-ppc/translate_init.c | 38 ++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 6952d789e518..81a3e6b5ed29 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -1364,6 +1364,7 @@ static inline int cpu_mmu_index (CPUPPCState *env, bool 
> ifetch)
>  #define SPR_BOOKE_CSRR0   (0x03A)
>  #define SPR_BOOKE_CSRR1   (0x03B)
>  #define SPR_BOOKE_DEAR(0x03D)
> +#define SPR_IAMR  (0x03D)
>  #define SPR_BOOKE_ESR (0x03E)
>  #define SPR_BOOKE_IVPR(0x03F)
>  #define SPR_MPC_EIE   (0x050)
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index f2eb5f041ecd..2fac6ea58698 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -1133,6 +1133,36 @@ static void spr_write_uamor(DisasContext *ctx, int 
> sprn, int gprn)
>  tcg_temp_free(t1);
>  tcg_temp_free(t2);
>  }
> +
> +static void spr_write_iamr(DisasContext *ctx, int sprn, int gprn)
> +{
> +TCGv t0 = tcg_temp_new();
> +TCGv t1 = tcg_temp_new();
> +TCGv t2 = tcg_temp_new();
> +
> +/* Note, the HV=1 case is handled earlier by simply using
> + * spr_write_generic for HV mode in the SPR table
> + */
> +
> +/* Build insertion mask into t1 based on context */
> +gen_load_spr(t1, SPR_AMOR);
> +
> +/* Mask new bits into t2 */
> +tcg_gen_and_tl(t2, t1, cpu_gpr[gprn]);
> +
> +/* Load AMR and clear new bits in t0 */
> +gen_load_spr(t0, SPR_IAMR);
> +tcg_gen_andc_tl(t0, t0, t1);
> +
> +/* Or'in new bits and write it out */
> +tcg_gen_or_tl(t0, t0, t2);
> +gen_store_spr(SPR_IAMR, t0);
> +spr_store_dump_spr(SPR_IAMR);
> +
> +tcg_temp_free(t0);
> +tcg_temp_free(t1);
> +tcg_temp_free(t2);
> +}
>  #endif /* CONFIG_USER_ONLY */
>  
>  static void gen_spr_amr(CPUPPCState *env, bool has_iamr)
> @@ -1161,6 +1191,14 @@ static void gen_spr_amr(CPUPPCState *env, bool 
> has_iamr)
>  SPR_NOACCESS, SPR_NOACCESS,
>  _read_generic, _write_generic,
>  0);
> +if (!has_iamr) {
> +return;
> +}
> +spr_register_kvm_hv(env, SPR_IAMR, "IAMR",
> +SPR_NOACCESS, SPR_NOACCESS,
> +_read_generic, _write_iamr,
> +_read_generic, _write_generic,
> +KVM_REG_PPC_IAMR, 0);

In case you rework this patch (e.g. by putting the has_iamr parameter
from the last patch in here), I think I'd also rather write this as:

if (has_iamr) {
spr_register_kvm_hv(...
}

That would be slightly easier to read.

Apart from that, the patch looks fine to me.

 Thomas

[Qemu-devel] [PATCH 13/17] ppc: Add POWER8 IAMR register

[Cédric Le Goater]

From: Benjamin Herrenschmidt 

With appropriate AMR-like masks. Not actually used by the translation
logic at that point

Signed-off-by: Benjamin Herrenschmidt 
[clg: add the use of spr_register_kvm_hv()]
Signed-off-by: Cédric Le Goater 
---
 target-ppc/cpu.h|  1 +
 target-ppc/translate_init.c | 38 ++
 2 files changed, 39 insertions(+)

diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 6952d789e518..81a3e6b5ed29 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -1364,6 +1364,7 @@ static inline int cpu_mmu_index (CPUPPCState *env, bool 
ifetch)
 #define SPR_BOOKE_CSRR0   (0x03A)
 #define SPR_BOOKE_CSRR1   (0x03B)
 #define SPR_BOOKE_DEAR(0x03D)
+#define SPR_IAMR  (0x03D)
 #define SPR_BOOKE_ESR (0x03E)
 #define SPR_BOOKE_IVPR(0x03F)
 #define SPR_MPC_EIE   (0x050)
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index f2eb5f041ecd..2fac6ea58698 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -1133,6 +1133,36 @@ static void spr_write_uamor(DisasContext *ctx, int sprn, 
int gprn)
 tcg_temp_free(t1);
 tcg_temp_free(t2);
 }
+
+static void spr_write_iamr(DisasContext *ctx, int sprn, int gprn)
+{
+TCGv t0 = tcg_temp_new();
+TCGv t1 = tcg_temp_new();
+TCGv t2 = tcg_temp_new();
+
+/* Note, the HV=1 case is handled earlier by simply using
+ * spr_write_generic for HV mode in the SPR table
+ */
+
+/* Build insertion mask into t1 based on context */
+gen_load_spr(t1, SPR_AMOR);
+
+/* Mask new bits into t2 */
+tcg_gen_and_tl(t2, t1, cpu_gpr[gprn]);
+
+/* Load AMR and clear new bits in t0 */
+gen_load_spr(t0, SPR_IAMR);
+tcg_gen_andc_tl(t0, t0, t1);
+
+/* Or'in new bits and write it out */
+tcg_gen_or_tl(t0, t0, t2);
+gen_store_spr(SPR_IAMR, t0);
+spr_store_dump_spr(SPR_IAMR);
+
+tcg_temp_free(t0);
+tcg_temp_free(t1);
+tcg_temp_free(t2);
+}
 #endif /* CONFIG_USER_ONLY */
 
 static void gen_spr_amr(CPUPPCState *env, bool has_iamr)
@@ -1161,6 +1191,14 @@ static void gen_spr_amr(CPUPPCState *env, bool has_iamr)
 SPR_NOACCESS, SPR_NOACCESS,
 _read_generic, _write_generic,
 0);
+if (!has_iamr) {
+return;
+}
+spr_register_kvm_hv(env, SPR_IAMR, "IAMR",
+SPR_NOACCESS, SPR_NOACCESS,
+_read_generic, _write_iamr,
+_read_generic, _write_generic,
+KVM_REG_PPC_IAMR, 0);
 #endif /* !CONFIG_USER_ONLY */
 }
 #endif /* TARGET_PPC64 */
-- 
2.1.4