On 20/09/2018 10:34, 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.
>
> 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 <r...@sysgo.com>
> ---
>
> 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)
>
Given that the previous (broken) version of this patch is already queued in
ppc-for-3.1, would it be better to send a v2 whereby these changes are squashed
into
the original?
ATB,
Mark.
