| Message ID | 20230703120301.45313-1-npiggin@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | Daniel Barboza |
| Headers | show |
| Series | [v3] target/ppc: Machine check on invalid real address access on POWER9/10 | expand |
On Mon Jul 3, 2023 at 10:03 PM AEST, Nicholas Piggin wrote: > ppc currently silently accepts invalid real address access. Catch > these and turn them into machine checks on POWER9/10 machines. Would there be any objections to merging this and the checkstop patch? We could disable this one before release if it turns out to cause breakage. I don't think it needs to rebase, and passes clang build and make check here. Just messed up the separator on the changelog of the checkstop patch. Thanks, Nick > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > Since v1: > - Only implement this for POWER9/10. Seems like previous IBM processors > may not catch this, trying to get info. > > Since v2: > - Split out from larger series since it is independent.
On 7/6/23 09:32, Nicholas Piggin wrote: > On Mon Jul 3, 2023 at 10:03 PM AEST, Nicholas Piggin wrote: >> ppc currently silently accepts invalid real address access. Catch >> these and turn them into machine checks on POWER9/10 machines. > > Would there be any objections to merging this and the checkstop patch? > We could disable this one before release if it turns out to cause > breakage. > > I don't think it needs to rebase, and passes clang build and make check > here. Just messed up the separator on the changelog of the checkstop > patch. I have been using the v2 series for a while : https://patchwork.ozlabs.org/project/qemu-ppc/list/?series=361456 without patch 4 and it looked good. Let's take v3 since this patch is unchanged. Thanks, C. > Thanks, > Nick > > >> >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >> --- >> Since v1: >> - Only implement this for POWER9/10. Seems like previous IBM processors >> may not catch this, trying to get info. >> >> Since v2: >> - Split out from larger series since it is independent.
On Thu, 6 Jul 2023, Cédric Le Goater wrote: > On 7/6/23 09:32, Nicholas Piggin wrote: >> On Mon Jul 3, 2023 at 10:03 PM AEST, Nicholas Piggin wrote: >>> ppc currently silently accepts invalid real address access. Catch >>> these and turn them into machine checks on POWER9/10 machines. >> >> Would there be any objections to merging this and the checkstop patch? >> We could disable this one before release if it turns out to cause >> breakage. >> >> I don't think it needs to rebase, and passes clang build and make check >> here. Just messed up the separator on the changelog of the checkstop >> patch. > > I have been using the v2 series for a while : > > https://patchwork.ozlabs.org/project/qemu-ppc/list/?series=361456 > > without patch 4 and it looked good. Let's take v3 since this patch is > unchanged. I think there was a newer version that dropped the test for the MSR bit from the checkstop function and left that at the call site to avoid adding a one line wrapper later. This version does not seem to be that so probably the next iteration is better but I was lost following these series.. Regards, BALATON Zoltan
On Thu, 6 Jul 2023, BALATON Zoltan wrote: > On Thu, 6 Jul 2023, Cédric Le Goater wrote: >> On 7/6/23 09:32, Nicholas Piggin wrote: >>> On Mon Jul 3, 2023 at 10:03 PM AEST, Nicholas Piggin wrote: >>>> ppc currently silently accepts invalid real address access. Catch >>>> these and turn them into machine checks on POWER9/10 machines. >>> >>> Would there be any objections to merging this and the checkstop patch? >>> We could disable this one before release if it turns out to cause >>> breakage. >>> >>> I don't think it needs to rebase, and passes clang build and make check >>> here. Just messed up the separator on the changelog of the checkstop >>> patch. >> >> I have been using the v2 series for a while : >> >> https://patchwork.ozlabs.org/project/qemu-ppc/list/?series=361456 >> >> without patch 4 and it looked good. Let's take v3 since this patch is >> unchanged. > > I think there was a newer version that dropped the test for the MSR bit from > the checkstop function and left that at the call site to avoid adding a one > line wrapper later. This version does not seem to be that so probably the > next iteration is better but I was lost following these series.. Or that was this one but then what's v3? I'm completely confused now about these so I'll just stop trying to follow. I've already said I'll only rebase my series changing excp_helper in next devel cycle so just go on with this without me, hopefully that's less confusing because at least one source of possible conflict is out of picture for now. Regards, BALATON Zoltan
On 7/6/23 13:43, BALATON Zoltan wrote: > On Thu, 6 Jul 2023, Cédric Le Goater wrote: >> On 7/6/23 09:32, Nicholas Piggin wrote: >>> On Mon Jul 3, 2023 at 10:03 PM AEST, Nicholas Piggin wrote: >>>> ppc currently silently accepts invalid real address access. Catch >>>> these and turn them into machine checks on POWER9/10 machines. >>> >>> Would there be any objections to merging this and the checkstop patch? >>> We could disable this one before release if it turns out to cause >>> breakage. >>> >>> I don't think it needs to rebase, and passes clang build and make check >>> here. Just messed up the separator on the changelog of the checkstop >>> patch. >> >> I have been using the v2 series for a while : >> >> https://patchwork.ozlabs.org/project/qemu-ppc/list/?series=361456 >> >> without patch 4 and it looked good. Let's take v3 since this patch is >> unchanged. > > I think there was a newer version that dropped the test for the MSR bit from the checkstop function and left that at the call site to avoid adding a one line wrapper later. This version does not seem to be that so probably the next iteration is better but I was lost following these series.. Yes. It was a bit confusing to track for me also. Things should cool down with soft freeze. After that, I am in favor of dealing with large series ! Thanks C.
On 7/6/23 04:32, Nicholas Piggin wrote: > On Mon Jul 3, 2023 at 10:03 PM AEST, Nicholas Piggin wrote: >> ppc currently silently accepts invalid real address access. Catch >> these and turn them into machine checks on POWER9/10 machines. > > Would there be any objections to merging this and the checkstop patch? > We could disable this one before release if it turns out to cause > breakage. I don't have objections but his bad boy has no acks. Cedric, if you vouch for this change send a R-b and I'll queue this up. Thanks, Daniel > > I don't think it needs to rebase, and passes clang build and make check > here. Just messed up the separator on the changelog of the checkstop > patch. > > Thanks, > Nick > > >> >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >> --- >> Since v1: >> - Only implement this for POWER9/10. Seems like previous IBM processors >> may not catch this, trying to get info. >> >> Since v2: >> - Split out from larger series since it is independent.
On 7/6/23 22:50, Daniel Henrique Barboza wrote: > > > On 7/6/23 04:32, Nicholas Piggin wrote: >> On Mon Jul 3, 2023 at 10:03 PM AEST, Nicholas Piggin wrote: >>> ppc currently silently accepts invalid real address access. Catch >>> these and turn them into machine checks on POWER9/10 machines. >> >> Would there be any objections to merging this and the checkstop patch? >> We could disable this one before release if it turns out to cause >> breakage. > > I don't have objections but his bad boy has no acks. > > Cedric, if you vouch for this change send a R-b and I'll queue this up. Reviewed-by: Cédric Le Goater <clg@kaod.org> Thanks, C. > > > Thanks, > > > Daniel > >> >> I don't think it needs to rebase, and passes clang build and make check >> here. Just messed up the separator on the changelog of the checkstop >> patch. >> >> Thanks, >> Nick >> >> >>> >>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >>> --- >>> Since v1: >>> - Only implement this for POWER9/10. Seems like previous IBM processors >>> may not catch this, trying to get info. >>> >>> Since v2: >>> - Split out from larger series since it is independent.
Queued in gitlab.com/danielhb/qemu/tree/ppc-next. Thanks, Daniel On 7/3/23 09:03, Nicholas Piggin wrote: > ppc currently silently accepts invalid real address access. Catch > these and turn them into machine checks on POWER9/10 machines. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > Since v1: > - Only implement this for POWER9/10. Seems like previous IBM processors > may not catch this, trying to get info. > > Since v2: > - Split out from larger series since it is independent. > > target/ppc/cpu_init.c | 1 + > target/ppc/excp_helper.c | 49 ++++++++++++++++++++++++++++++++++++++++ > target/ppc/internal.h | 5 ++++ > 3 files changed, 55 insertions(+) > > diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c > index 720aad9e05..6ac1765a8d 100644 > --- a/target/ppc/cpu_init.c > +++ b/target/ppc/cpu_init.c > @@ -7335,6 +7335,7 @@ static const struct TCGCPUOps ppc_tcg_ops = { > .cpu_exec_enter = ppc_cpu_exec_enter, > .cpu_exec_exit = ppc_cpu_exec_exit, > .do_unaligned_access = ppc_cpu_do_unaligned_access, > + .do_transaction_failed = ppc_cpu_do_transaction_failed, > #endif /* !CONFIG_USER_ONLY */ > }; > #endif /* CONFIG_TCG */ > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c > index 354392668e..e49e13a30d 100644 > --- a/target/ppc/excp_helper.c > +++ b/target/ppc/excp_helper.c > @@ -1428,7 +1428,9 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp) > /* machine check exceptions don't have ME set */ > new_msr &= ~((target_ulong)1 << MSR_ME); > > + msr |= env->error_code; > break; > + > case POWERPC_EXCP_DSI: /* Data storage exception */ > trace_ppc_excp_dsi(env->spr[SPR_DSISR], env->spr[SPR_DAR]); > break; > @@ -3184,5 +3186,52 @@ void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr, > env->error_code = insn & 0x03FF0000; > cpu_loop_exit(cs); > } > + > +void ppc_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr, > + vaddr vaddr, unsigned size, > + MMUAccessType access_type, > + int mmu_idx, MemTxAttrs attrs, > + MemTxResult response, uintptr_t retaddr) > +{ > + CPUPPCState *env = cs->env_ptr; > + > + switch (env->excp_model) { > +#if defined(TARGET_PPC64) > + case POWERPC_EXCP_POWER9: > + case POWERPC_EXCP_POWER10: > + /* > + * Machine check codes can be found in processor User Manual or > + * Linux or skiboot source. > + */ > + if (access_type == MMU_DATA_LOAD) { > + env->spr[SPR_DAR] = vaddr; > + env->spr[SPR_DSISR] = PPC_BIT(57); > + env->error_code = PPC_BIT(42); > + > + } else if (access_type == MMU_DATA_STORE) { > + /* > + * MCE for stores in POWER is asynchronous so hardware does > + * not set DAR, but QEMU can do better. > + */ > + env->spr[SPR_DAR] = vaddr; > + env->error_code = PPC_BIT(36) | PPC_BIT(43) | PPC_BIT(45); > + env->error_code |= PPC_BIT(42); > + > + } else { /* Fetch */ > + env->error_code = PPC_BIT(36) | PPC_BIT(44) | PPC_BIT(45); > + } > + break; > +#endif > + default: > + /* > + * TODO: Check behaviour for other CPUs, for now do nothing. > + * Could add a basic MCE even if real hardware ignores. > + */ > + return; > + } > + > + cs->exception_index = POWERPC_EXCP_MCHECK; > + cpu_loop_exit_restore(cs, retaddr); > +} > #endif /* CONFIG_TCG */ > #endif /* !CONFIG_USER_ONLY */ > diff --git a/target/ppc/internal.h b/target/ppc/internal.h > index 901bae6d39..57acb3212c 100644 > --- a/target/ppc/internal.h > +++ b/target/ppc/internal.h > @@ -296,6 +296,11 @@ bool ppc_cpu_tlb_fill(CPUState *cs, vaddr address, int size, > G_NORETURN void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr addr, > MMUAccessType access_type, int mmu_idx, > uintptr_t retaddr); > +void ppc_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr, > + vaddr addr, unsigned size, > + MMUAccessType access_type, > + int mmu_idx, MemTxAttrs attrs, > + MemTxResult response, uintptr_t retaddr); > #endif > > FIELD(GER_MSK, XMSK, 0, 4)
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c index 720aad9e05..6ac1765a8d 100644 --- a/target/ppc/cpu_init.c +++ b/target/ppc/cpu_init.c @@ -7335,6 +7335,7 @@ static const struct TCGCPUOps ppc_tcg_ops = { .cpu_exec_enter = ppc_cpu_exec_enter, .cpu_exec_exit = ppc_cpu_exec_exit, .do_unaligned_access = ppc_cpu_do_unaligned_access, + .do_transaction_failed = ppc_cpu_do_transaction_failed, #endif /* !CONFIG_USER_ONLY */ }; #endif /* CONFIG_TCG */ diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c index 354392668e..e49e13a30d 100644 --- a/target/ppc/excp_helper.c +++ b/target/ppc/excp_helper.c @@ -1428,7 +1428,9 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp) /* machine check exceptions don't have ME set */ new_msr &= ~((target_ulong)1 << MSR_ME); + msr |= env->error_code; break; + case POWERPC_EXCP_DSI: /* Data storage exception */ trace_ppc_excp_dsi(env->spr[SPR_DSISR], env->spr[SPR_DAR]); break; @@ -3184,5 +3186,52 @@ void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr, env->error_code = insn & 0x03FF0000; cpu_loop_exit(cs); } + +void ppc_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr, + vaddr vaddr, unsigned size, + MMUAccessType access_type, + int mmu_idx, MemTxAttrs attrs, + MemTxResult response, uintptr_t retaddr) +{ + CPUPPCState *env = cs->env_ptr; + + switch (env->excp_model) { +#if defined(TARGET_PPC64) + case POWERPC_EXCP_POWER9: + case POWERPC_EXCP_POWER10: + /* + * Machine check codes can be found in processor User Manual or + * Linux or skiboot source. + */ + if (access_type == MMU_DATA_LOAD) { + env->spr[SPR_DAR] = vaddr; + env->spr[SPR_DSISR] = PPC_BIT(57); + env->error_code = PPC_BIT(42); + + } else if (access_type == MMU_DATA_STORE) { + /* + * MCE for stores in POWER is asynchronous so hardware does + * not set DAR, but QEMU can do better. + */ + env->spr[SPR_DAR] = vaddr; + env->error_code = PPC_BIT(36) | PPC_BIT(43) | PPC_BIT(45); + env->error_code |= PPC_BIT(42); + + } else { /* Fetch */ + env->error_code = PPC_BIT(36) | PPC_BIT(44) | PPC_BIT(45); + } + break; +#endif + default: + /* + * TODO: Check behaviour for other CPUs, for now do nothing. + * Could add a basic MCE even if real hardware ignores. + */ + return; + } + + cs->exception_index = POWERPC_EXCP_MCHECK; + cpu_loop_exit_restore(cs, retaddr); +} #endif /* CONFIG_TCG */ #endif /* !CONFIG_USER_ONLY */ diff --git a/target/ppc/internal.h b/target/ppc/internal.h index 901bae6d39..57acb3212c 100644 --- a/target/ppc/internal.h +++ b/target/ppc/internal.h @@ -296,6 +296,11 @@ bool ppc_cpu_tlb_fill(CPUState *cs, vaddr address, int size, G_NORETURN void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr addr, MMUAccessType access_type, int mmu_idx, uintptr_t retaddr); +void ppc_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr, + vaddr addr, unsigned size, + MMUAccessType access_type, + int mmu_idx, MemTxAttrs attrs, + MemTxResult response, uintptr_t retaddr); #endif FIELD(GER_MSK, XMSK, 0, 4)
ppc currently silently accepts invalid real address access. Catch these and turn them into machine checks on POWER9/10 machines. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- Since v1: - Only implement this for POWER9/10. Seems like previous IBM processors may not catch this, trying to get info. Since v2: - Split out from larger series since it is independent. target/ppc/cpu_init.c | 1 + target/ppc/excp_helper.c | 49 ++++++++++++++++++++++++++++++++++++++++ target/ppc/internal.h | 5 ++++ 3 files changed, 55 insertions(+)