[PATCH v3] target/ppc: Machine check on invalid real address access on POWER9/10

[PATCH v3] target/ppc: Machine check on invalid real address access on POWER9/10

[Nicholas Piggin]

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)
-- 
2.40.1

Re: [PATCH v3] target/ppc: Machine check on invalid real address access on POWER9/10

[Nicholas Piggin]

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.

Re: [PATCH v3] target/ppc: Machine check on invalid real address access on POWER9/10

[Cédric Le Goater]

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.

Re: [PATCH v3] target/ppc: Machine check on invalid real address access on POWER9/10

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 1137 bytes --]

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

Re: [PATCH v3] target/ppc: Machine check on invalid real address access on POWER9/10

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 1562 bytes --]

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

Re: [PATCH v3] target/ppc: Machine check on invalid real address access on POWER9/10

[Cédric Le Goater]

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.

Re: [PATCH v3] target/ppc: Machine check on invalid real address access on POWER9/10

[Daniel Henrique Barboza]

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.

Re: [PATCH v3] target/ppc: Machine check on invalid real address access on POWER9/10

[Cédric Le Goater]

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.

Re: [PATCH v3] target/ppc: Machine check on invalid real address access on POWER9/10

[Daniel Henrique Barboza]

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)