Re: [Qemu-devel] [PATCH] target/ppc: fix dcbz, dcbzep, dcbtst and insn type

[Philippe Mathieu-Daudé]

Hi Roman,

On 9/20/18 11:34 AM, Roman Kapl wrote:
> dcbz was broken with the refactoring introduced in the External PID patch. The
> GETPC() call is moved directly to the helper in this patch, to report correct 
> PC
> address. The issue did not always manifest: if the compiler decided to inline
> the call, the PC wound up being correct. So in order to reproduce, use a debug
> build.

Can you split this patch in 4 please? (Easier to bisect or cherry-pick).

> A problem in dcbzep, which did not perform external PID access in its slow 
> path
> was also fixed.
> 
> dcbtst opcode is now fixed, it was swapped with dcbtstep.
> 
> I also misunderstood the instruction registration mechanism and the 
> instructions
> were not truly limited to BookE 2.06. The PPC_CACHE/PPC_INTEGER type mask was
> changed to PPC_NONE.
> 
> Fixes: ea8073c10d ("target/ppc: add external PID support")
> Signed-off-by: Roman Kapl <address@hidden>
> ---
> 
> This fixes the sandalfoot image boot. And thanks to PMM for spotting the GETPC
> issue.
> 
>  target/ppc/mem_helper.c | 15 ++++++++++-----
>  target/ppc/translate.c  | 20 +++++++++-----------
>  2 files changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/target/ppc/mem_helper.c b/target/ppc/mem_helper.c
> index 0c39141ab7..9d140dc4a3 100644
> --- a/target/ppc/mem_helper.c
> +++ b/target/ppc/mem_helper.c
> @@ -141,12 +141,13 @@ void helper_stsw(CPUPPCState *env, target_ulong addr, 
> uint32_t nb,
>      }
>  }
>  
> -static void helper_dcbz_common(CPUPPCState *env, target_ulong addr,
> -                               uint32_t opcode, int mmu_idx)
> +static void dcbz_common(CPUPPCState *env, target_ulong addr,
> +                        uint32_t opcode, bool epid, uintptr_t pc)
>  {
>      target_ulong mask, dcbz_size = env->dcache_line_size;
>      uint32_t i;
>      void *haddr;
> +    int mmu_idx = epid ? PPC_TLB_EPID_STORE : env->dmmu_idx;
>  
>  #if defined(TARGET_PPC64)
>      /* Check for dcbz vs dcbzl on 970 */
> @@ -172,19 +173,23 @@ static void helper_dcbz_common(CPUPPCState *env, 
> target_ulong addr,
>      } else {
>          /* Slow path */
>          for (i = 0; i < dcbz_size; i += 8) {
> -            cpu_stq_data_ra(env, addr + i, 0, GETPC());
> +            if (epid) {
> +                cpu_stq_eps_ra(env, addr + i, 0, pc);
> +            } else {
> +                cpu_stq_data_ra(env, addr + i, 0, pc);
> +            }
>          }
>      }
>  }
>  
>  void helper_dcbz(CPUPPCState *env, target_ulong addr, uint32_t opcode)
>  {
> -    helper_dcbz_common(env, addr, opcode, env->dmmu_idx);
> +    dcbz_common(env, addr, opcode, false, GETPC());
>  }
>  
>  void helper_dcbzep(CPUPPCState *env, target_ulong addr, uint32_t opcode)
>  {
> -    helper_dcbz_common(env, addr, opcode, PPC_TLB_EPID_STORE);
> +    dcbz_common(env, addr, opcode, true, GETPC());
>  }
>  
>  void helper_icbi(CPUPPCState *env, target_ulong addr)
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 41322fb9c4..3e279bcbc9 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -6881,24 +6881,22 @@ GEN_HANDLER_E(mcrxrx, 0x1F, 0x00, 0x12, 0x007FF801, 
> PPC_NONE, PPC2_ISA300),
>  GEN_HANDLER(mtmsr, 0x1F, 0x12, 0x04, 0x001EF801, PPC_MISC),
>  GEN_HANDLER(mtspr, 0x1F, 0x13, 0x0E, 0x00000000, PPC_MISC),
>  GEN_HANDLER(dcbf, 0x1F, 0x16, 0x02, 0x03C00001, PPC_CACHE),
> -GEN_HANDLER_E(dcbfep, 0x1F, 0x1F, 0x03, 0x03C00001, PPC_CACHE, 
> PPC2_BOOKE206),
> +GEN_HANDLER_E(dcbfep, 0x1F, 0x1F, 0x03, 0x03C00001, PPC_NONE, PPC2_BOOKE206),
>  GEN_HANDLER(dcbi, 0x1F, 0x16, 0x0E, 0x03E00001, PPC_CACHE),
>  GEN_HANDLER(dcbst, 0x1F, 0x16, 0x01, 0x03E00001, PPC_CACHE),
> -GEN_HANDLER_E(dcbstep, 0x1F, 0x1F, 0x01, 0x03E00001, PPC_CACHE, 
> PPC2_BOOKE206),
> +GEN_HANDLER_E(dcbstep, 0x1F, 0x1F, 0x01, 0x03E00001, PPC_NONE, 
> PPC2_BOOKE206),
>  GEN_HANDLER(dcbt, 0x1F, 0x16, 0x08, 0x00000001, PPC_CACHE),
> -GEN_HANDLER_E(dcbtep, 0x1F, 0x1F, 0x09, 0x00000001, PPC_CACHE, 
> PPC2_BOOKE206),
> -GEN_HANDLER(dcbtst, 0x1F, 0x1F, 0x07, 0x00000001, PPC_CACHE),
> -GEN_HANDLER_E(dcbtstep, 0x1F, 0x16, 0x07, 0x00000001, PPC_CACHE, 
> PPC2_BOOKE206),
> +GEN_HANDLER_E(dcbtep, 0x1F, 0x1F, 0x09, 0x00000001, PPC_NONE, PPC2_BOOKE206),
> +GEN_HANDLER(dcbtst, 0x1F, 0x16, 0x07, 0x00000001, PPC_CACHE),
> +GEN_HANDLER_E(dcbtstep, 0x1F, 0x1F, 0x07, 0x00000001, PPC_NONE, 
> PPC2_BOOKE206),
>  GEN_HANDLER_E(dcbtls, 0x1F, 0x06, 0x05, 0x02000001, PPC_BOOKE, 
> PPC2_BOOKE206),
>  GEN_HANDLER(dcbz, 0x1F, 0x16, 0x1F, 0x03C00001, PPC_CACHE_DCBZ),
> -GEN_HANDLER_E(dcbzep, 0x1F, 0x1F, 0x1F, 0x03C00001,
> -              PPC_CACHE_DCBZ, PPC2_BOOKE206),
> +GEN_HANDLER_E(dcbzep, 0x1F, 0x1F, 0x1F, 0x03C00001, PPC_NONE, PPC2_BOOKE206),
>  GEN_HANDLER(dst, 0x1F, 0x16, 0x0A, 0x01800001, PPC_ALTIVEC),
>  GEN_HANDLER(dstst, 0x1F, 0x16, 0x0B, 0x01800001, PPC_ALTIVEC),
>  GEN_HANDLER(dss, 0x1F, 0x16, 0x19, 0x019FF801, PPC_ALTIVEC),
>  GEN_HANDLER(icbi, 0x1F, 0x16, 0x1E, 0x03E00001, PPC_CACHE_ICBI),
> -GEN_HANDLER_E(icbiep, 0x1F, 0x1F, 0x1E, 0x03E00001,
> -              PPC_CACHE_ICBI, PPC2_BOOKE206),
> +GEN_HANDLER_E(icbiep, 0x1F, 0x1F, 0x1E, 0x03E00001, PPC_NONE, PPC2_BOOKE206),
>  GEN_HANDLER(dcba, 0x1F, 0x16, 0x17, 0x03E00001, PPC_CACHE_DCBA),
>  GEN_HANDLER(mfsr, 0x1F, 0x13, 0x12, 0x0010F801, PPC_SEGMENT),
>  GEN_HANDLER(mfsrin, 0x1F, 0x13, 0x14, 0x001F0001, PPC_SEGMENT),
> @@ -7205,7 +7203,7 @@ GEN_LDX(lwbr, ld32ur, 0x16, 0x10, PPC_INTEGER)
>  #undef GEN_LDEPX
>  #define GEN_LDEPX(name, ldop, opc2, opc3)                                    
>  \
>  GEN_HANDLER_E(name##epx, 0x1F, opc2, opc3,                                   
>  \
> -              0x00000001, PPC_INTEGER, PPC2_BOOKE206),
> +              0x00000001, PPC_NONE, PPC2_BOOKE206),
>  
>  GEN_LDEPX(lb, DEF_MEMOP(MO_UB), 0x1F, 0x02)
>  GEN_LDEPX(lh, DEF_MEMOP(MO_UW), 0x1F, 0x08)
> @@ -7251,7 +7249,7 @@ GEN_STX(stwbr, st32r, 0x16, 0x14, PPC_INTEGER)
>  #undef GEN_STEPX
>  #define GEN_STEPX(name, ldop, opc2, opc3)                                    
>  \
>  GEN_HANDLER_E(name##epx, 0x1F, opc2, opc3,                                   
>  \
> -              0x00000001, PPC_INTEGER, PPC2_BOOKE206),
> +              0x00000001, PPC_NONE, PPC2_BOOKE206),
>  
>  GEN_STEPX(stb, DEF_MEMOP(MO_UB), 0x1F, 0x06)
>  GEN_STEPX(sth, DEF_MEMOP(MO_UW), 0x1F, 0x0C)
>