Re: [Qemu-devel] [PATCH 12/17] ppc: Fix writing to AMR/UAMOR

[Cédric Le Goater]

On 03/14/2016 09:26 PM, Thomas Huth wrote:
>> > @@ -8093,7 +8137,7 @@ static void init_proc_book3s_64(CPUPPCState *env, 
>> > int version)
>> >  case BOOK3S_CPU_POWER7:
>> >  case BOOK3S_CPU_POWER8:
>> >  gen_spr_book3s_ids(env);
>> > -gen_spr_amr(env);
>> > +gen_spr_amr(env, version >= BOOK3S_CPU_POWER8);
>> >  gen_spr_book3s_purr(env);
>> >  env->ci_large_pages = true;
>> >  break;
> I think this last hunk (and thus the "has_iamr" parameter of that
> function) rather belong to the next patch, since it is not used here yet.

Yes. I fixed a compile break but I moved the hunk in the wrong patch.

Thanks,

C. 

Re: [Qemu-devel] [PATCH 12/17] ppc: Fix writing to AMR/UAMOR

[Thomas Huth]

On 14.03.2016 17:56, Cédric Le Goater wrote:
> From: Benjamin Herrenschmidt 
> 
> The masks weren't chosen nor applied properly. The architecture specifies
> that writes to AMR are masked by UAMOR for PR=1, otherwise AMOR for HV=0.
> 
> The writes to UAMOR are masked by AMOR for HV=0
> 
> Signed-off-by: Benjamin Herrenschmidt 
> [clg: fixed gen_spr_amr() call in init_proc_book3s_64()]
> Signed-off-by: Cédric Le Goater 
> ---
>  target-ppc/translate_init.c | 78 
> +++--
>  1 file changed, 61 insertions(+), 17 deletions(-)
> 
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index c921d9f53984..f2eb5f041ecd 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -1070,30 +1070,72 @@ static void gen_spr_7xx (CPUPPCState *env)
>  
>  #ifdef TARGET_PPC64
>  #ifndef CONFIG_USER_ONLY
> -static void spr_read_uamr (DisasContext *ctx, int gprn, int sprn)
> +static void spr_write_amr(DisasContext *ctx, int sprn, int gprn)
>  {
> -gen_load_spr(cpu_gpr[gprn], SPR_AMR);
> -spr_load_dump_spr(SPR_AMR);
> -}
> +TCGv t0 = tcg_temp_new();
> +TCGv t1 = tcg_temp_new();
> +TCGv t2 = tcg_temp_new();
>  
> -static void spr_write_uamr (DisasContext *ctx, int sprn, int gprn)
> -{
> -gen_store_spr(SPR_AMR, cpu_gpr[gprn]);
> +/* Note, the HV=1 PR=0 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 */
> +if (ctx->pr) {
> +gen_load_spr(t1, SPR_UAMOR);
> +} else {
> +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_AMR);
> +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_AMR, t0);
>  spr_store_dump_spr(SPR_AMR);
> +
> +tcg_temp_free(t0);
> +tcg_temp_free(t1);
> +tcg_temp_free(t2);
>  }
>  
> -static void spr_write_uamr_pr (DisasContext *ctx, int sprn, int gprn)
> +static void spr_write_uamor(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_UAMOR);
> -tcg_gen_and_tl(t0, t0, cpu_gpr[gprn]);
> -gen_store_spr(SPR_AMR, t0);
> -spr_store_dump_spr(SPR_AMR);
> +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_UAMOR, t0);
> +spr_store_dump_spr(SPR_UAMOR);
> +
> +tcg_temp_free(t0);
> +tcg_temp_free(t1);
> +tcg_temp_free(t2);
>  }
>  #endif /* CONFIG_USER_ONLY */
>  
> -static void gen_spr_amr (CPUPPCState *env)
> +static void gen_spr_amr(CPUPPCState *env, bool has_iamr)
>  {
>  #ifndef CONFIG_USER_ONLY
>  /* Virtual Page Class Key protection */
> @@ -1101,15 +1143,17 @@ static void gen_spr_amr (CPUPPCState *env)
>   * userspace accessible, 29 is privileged.  So we only need to set
>   * the kvm ONE_REG id on one of them, we use 29 */
>  spr_register(env, SPR_UAMR, "UAMR",
> - _read_uamr, _write_uamr_pr,
> - _read_uamr, _write_uamr,
> + _read_generic, _write_amr,
> + _read_generic, _write_amr,
>   0);
> -spr_register_kvm(env, SPR_AMR, "AMR",
> +spr_register_kvm_hv(env, SPR_AMR, "AMR",
>   SPR_NOACCESS, SPR_NOACCESS,
> + _read_generic, _write_amr,
>   _read_generic, _write_generic,
>   KVM_REG_PPC_AMR, 0);
> -spr_register_kvm(env, SPR_UAMOR, "UAMOR",
> +spr_register_kvm_hv(env, SPR_UAMOR, "UAMOR",
>   SPR_NOACCESS, SPR_NOACCESS,
> + _read_generic, _write_uamor,
>   _read_generic, _write_generic,
>   KVM_REG_PPC_UAMOR, 0);
>  spr_register_hv(env, SPR_AMOR, "AMOR",
> @@ -8093,7 +8137,7 @@ static void init_proc_book3s_64(CPUPPCState *env, int 
> version)
>  case BOOK3S_CPU_POWER7:
>  case BOOK3S_CPU_POWER8:
>  gen_spr_book3s_ids(env);
> -gen_spr_amr(env);
> +gen_spr_amr(env, version >= BOOK3S_CPU_POWER8);
>  gen_spr_book3s_purr(env);
>  env->ci_large_pages = true;
>  break;

I think this last hunk (and thus the "has_iamr" parameter of that

[Qemu-devel] [PATCH 12/17] ppc: Fix writing to AMR/UAMOR

[Cédric Le Goater]

From: Benjamin Herrenschmidt 

The masks weren't chosen nor applied properly. The architecture specifies
that writes to AMR are masked by UAMOR for PR=1, otherwise AMOR for HV=0.

The writes to UAMOR are masked by AMOR for HV=0

Signed-off-by: Benjamin Herrenschmidt 
[clg: fixed gen_spr_amr() call in init_proc_book3s_64()]
Signed-off-by: Cédric Le Goater 
---
 target-ppc/translate_init.c | 78 +++--
 1 file changed, 61 insertions(+), 17 deletions(-)

diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index c921d9f53984..f2eb5f041ecd 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -1070,30 +1070,72 @@ static void gen_spr_7xx (CPUPPCState *env)
 
 #ifdef TARGET_PPC64
 #ifndef CONFIG_USER_ONLY
-static void spr_read_uamr (DisasContext *ctx, int gprn, int sprn)
+static void spr_write_amr(DisasContext *ctx, int sprn, int gprn)
 {
-gen_load_spr(cpu_gpr[gprn], SPR_AMR);
-spr_load_dump_spr(SPR_AMR);
-}
+TCGv t0 = tcg_temp_new();
+TCGv t1 = tcg_temp_new();
+TCGv t2 = tcg_temp_new();
 
-static void spr_write_uamr (DisasContext *ctx, int sprn, int gprn)
-{
-gen_store_spr(SPR_AMR, cpu_gpr[gprn]);
+/* Note, the HV=1 PR=0 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 */
+if (ctx->pr) {
+gen_load_spr(t1, SPR_UAMOR);
+} else {
+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_AMR);
+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_AMR, t0);
 spr_store_dump_spr(SPR_AMR);
+
+tcg_temp_free(t0);
+tcg_temp_free(t1);
+tcg_temp_free(t2);
 }
 
-static void spr_write_uamr_pr (DisasContext *ctx, int sprn, int gprn)
+static void spr_write_uamor(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_UAMOR);
-tcg_gen_and_tl(t0, t0, cpu_gpr[gprn]);
-gen_store_spr(SPR_AMR, t0);
-spr_store_dump_spr(SPR_AMR);
+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_UAMOR, t0);
+spr_store_dump_spr(SPR_UAMOR);
+
+tcg_temp_free(t0);
+tcg_temp_free(t1);
+tcg_temp_free(t2);
 }
 #endif /* CONFIG_USER_ONLY */
 
-static void gen_spr_amr (CPUPPCState *env)
+static void gen_spr_amr(CPUPPCState *env, bool has_iamr)
 {
 #ifndef CONFIG_USER_ONLY
 /* Virtual Page Class Key protection */
@@ -1101,15 +1143,17 @@ static void gen_spr_amr (CPUPPCState *env)
  * userspace accessible, 29 is privileged.  So we only need to set
  * the kvm ONE_REG id on one of them, we use 29 */
 spr_register(env, SPR_UAMR, "UAMR",
- _read_uamr, _write_uamr_pr,
- _read_uamr, _write_uamr,
+ _read_generic, _write_amr,
+ _read_generic, _write_amr,
  0);
-spr_register_kvm(env, SPR_AMR, "AMR",
+spr_register_kvm_hv(env, SPR_AMR, "AMR",
  SPR_NOACCESS, SPR_NOACCESS,
+ _read_generic, _write_amr,
  _read_generic, _write_generic,
  KVM_REG_PPC_AMR, 0);
-spr_register_kvm(env, SPR_UAMOR, "UAMOR",
+spr_register_kvm_hv(env, SPR_UAMOR, "UAMOR",
  SPR_NOACCESS, SPR_NOACCESS,
+ _read_generic, _write_uamor,
  _read_generic, _write_generic,
  KVM_REG_PPC_UAMOR, 0);
 spr_register_hv(env, SPR_AMOR, "AMOR",
@@ -8093,7 +8137,7 @@ static void init_proc_book3s_64(CPUPPCState *env, int 
version)
 case BOOK3S_CPU_POWER7:
 case BOOK3S_CPU_POWER8:
 gen_spr_book3s_ids(env);
-gen_spr_amr(env);
+gen_spr_amr(env, version >= BOOK3S_CPU_POWER8);
 gen_spr_book3s_purr(env);
 env->ci_large_pages = true;
 break;
-- 
2.1.4