| Message ID | 20211209173323.2166642-1-farosas@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] Revert "target/ppc: Move SPR_DSISR setting to powerpc_excp" | expand |
On 12/9/21 9:33 AM, Fabiano Rosas wrote: > This reverts commit 336e91f85332dda0ede4c1d15b87a19a0fb898a2. > > It breaks the --disable-tcg build: > > ../target/ppc/excp_helper.c:463:29: error: implicit declaration of > function ‘cpu_ldl_code’ [-Werror=implicit-function-declaration] > > We should not have TCG code in powerpc_excp because some kvm-only > routines use it indirectly to dispatch interrupts. See > kvm_handle_debug, spapr_mce_req_event and > spapr_do_system_reset_on_cpu. > > We can re-introduce the change once we have split the interrupt > injection code between KVM and TCG. > > Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> > --- > target/ppc/excp_helper.c | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) This is fine. I had thought it would turn out to be helpful in conjunction with my user-only unaligned patch set, but in the end I went a different way and have a separate hook for user-only. It is correct to simply revert the patch first. I have ideas for further cleanup, if you have time: (1) The assignment to DSISR does not need to wait until powerpc_excp. I believe we can assign to it directly in do_unaligned_access, and avoid using env->error_code as an intermediary. (2) The note about opcode fields being set incorrectly could be fixed during translation. You'd use TARGET_INSN_START_EXTRA_WORDS to record the necessary information during translation, is provided to restore_state_to_opc during unwinding, and then moved into DSISR in do_unaligned_access. Similar to target/arm and how env->exception.syndrome is managed, especially disas_set_insn_syndrome. r~
On 12/9/21 18:33, Fabiano Rosas wrote: > This reverts commit 336e91f85332dda0ede4c1d15b87a19a0fb898a2. > > It breaks the --disable-tcg build: > > ../target/ppc/excp_helper.c:463:29: error: implicit declaration of > function ‘cpu_ldl_code’ [-Werror=implicit-function-declaration] > > We should not have TCG code in powerpc_excp because some kvm-only > routines use it indirectly to dispatch interrupts. See > kvm_handle_debug, spapr_mce_req_event and > spapr_do_system_reset_on_cpu. > > We can re-introduce the change once we have split the interrupt > injection code between KVM and TCG. > > Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> Applied to ppc-next. Thanks, C.
Richard Henderson <richard.henderson@linaro.org> writes: > On 12/9/21 9:33 AM, Fabiano Rosas wrote: >> This reverts commit 336e91f85332dda0ede4c1d15b87a19a0fb898a2. >> >> It breaks the --disable-tcg build: >> >> ../target/ppc/excp_helper.c:463:29: error: implicit declaration of >> function ‘cpu_ldl_code’ [-Werror=implicit-function-declaration] >> >> We should not have TCG code in powerpc_excp because some kvm-only >> routines use it indirectly to dispatch interrupts. See >> kvm_handle_debug, spapr_mce_req_event and >> spapr_do_system_reset_on_cpu. >> >> We can re-introduce the change once we have split the interrupt >> injection code between KVM and TCG. >> >> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> >> --- >> target/ppc/excp_helper.c | 21 ++++++++++++--------- >> 1 file changed, 12 insertions(+), 9 deletions(-) > > This is fine. I had thought it would turn out to be helpful in conjunction with my > user-only unaligned patch set, but in the end I went a different way and have a separate > hook for user-only. > > It is correct to simply revert the patch first. > > I have ideas for further cleanup, if you have time: > > (1) The assignment to DSISR does not need to wait until powerpc_excp. I believe we can > assign to it directly in do_unaligned_access, and avoid using env->error_code as an > intermediary. Makes sense. I see that not all processors use DSISR during the Alignment interrupt. I'll check the manuals and fix that as well. I'm writing some tests to check each individual Alignment case and DAR is not being set for ALIGN_LE. There might be others missing. I'll probably end up moving the DAR code from ppc_cpu_do_unaligned_access into powerpc_excp after all. > > (2) The note about opcode fields being set incorrectly could be fixed during translation. > You'd use TARGET_INSN_START_EXTRA_WORDS to record the necessary information during > translation, is provided to restore_state_to_opc during unwinding, and then moved into > DSISR in do_unaligned_access. Similar to target/arm and how env->exception.syndrome is > managed, especially disas_set_insn_syndrome. > Ok, I'll give it a shot. Thanks for the detailed pointers.
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c index 17607adbe4..1c8b373078 100644 --- a/target/ppc/excp_helper.c +++ b/target/ppc/excp_helper.c @@ -454,15 +454,13 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp) break; } case POWERPC_EXCP_ALIGN: /* Alignment exception */ + /* Get rS/rD and rA from faulting opcode */ /* - * Get rS/rD and rA from faulting opcode. - * Note: We will only invoke ALIGN for atomic operations, - * so all instructions are X-form. + * Note: the opcode fields will not be set properly for a + * direct store load/store, but nobody cares as nobody + * actually uses direct store segments. */ - { - uint32_t insn = cpu_ldl_code(env, env->nip); - env->spr[SPR_DSISR] |= (insn & 0x03FF0000) >> 16; - } + env->spr[SPR_DSISR] |= (env->error_code & 0x03FF0000) >> 16; break; case POWERPC_EXCP_PROGRAM: /* Program exception */ switch (env->error_code & ~0xF) { @@ -1461,6 +1459,11 @@ void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr, int mmu_idx, uintptr_t retaddr) { CPUPPCState *env = cs->env_ptr; + uint32_t insn; + + /* Restore state and reload the insn we executed, for filling in DSISR. */ + cpu_restore_state(cs, retaddr, true); + insn = cpu_ldl_code(env, env->nip); switch (env->mmu_model) { case POWERPC_MMU_SOFT_4xx: @@ -1477,8 +1480,8 @@ void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr, } cs->exception_index = POWERPC_EXCP_ALIGN; - env->error_code = 0; - cpu_loop_exit_restore(cs, retaddr); + env->error_code = insn & 0x03FF0000; + cpu_loop_exit(cs); } #endif /* CONFIG_TCG */ #endif /* !CONFIG_USER_ONLY */
This reverts commit 336e91f85332dda0ede4c1d15b87a19a0fb898a2. It breaks the --disable-tcg build: ../target/ppc/excp_helper.c:463:29: error: implicit declaration of function ‘cpu_ldl_code’ [-Werror=implicit-function-declaration] We should not have TCG code in powerpc_excp because some kvm-only routines use it indirectly to dispatch interrupts. See kvm_handle_debug, spapr_mce_req_event and spapr_do_system_reset_on_cpu. We can re-introduce the change once we have split the interrupt injection code between KVM and TCG. Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> --- target/ppc/excp_helper.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)