Re: [PATCH v3 33/51] target/ppc: Simplify ppc_cpu_debug_check_watchpoint

On 7/11/26 03:54, Harsh Prateek Bora wrote:
> + Cedric, Shiva
> 
> On 11/07/26 2:23 am, Richard Henderson wrote:
>> We don't need to check either ISA bit, because that's already
>> done starting with SPR registration.  Replace target_ulong with
>> uint64_t, as we're under TARGET_PPC64.
>>
>> Signed-off-by: Richard Henderson <[email protected]>
>> ---
>>   target/ppc/tcg-excp_helper.c | 30 +++++++++---------------------
>>   1 file changed, 9 insertions(+), 21 deletions(-)
>>
>> diff --git a/target/ppc/tcg-excp_helper.c b/target/ppc/tcg-excp_helper.c
>> index 99ffe4a820..4065440949 100644
>> --- a/target/ppc/tcg-excp_helper.c
>> +++ b/target/ppc/tcg-excp_helper.c
>> @@ -353,35 +353,23 @@ bool ppc_cpu_debug_check_watchpoint(CPUState *cs, CPUBreakpoint *wp)
>>   {
>>   #if defined(TARGET_PPC64)
>>       CPUPPCState *env = cpu_env(cs);
>> -    bool wt, wti, hv, sv, pr;
>> -    uint32_t dawrx;
>> +    uint32_t dawrx = env->spr[wp->id ? SPR_DAWRX1 : SPR_DAWRX0];
>> +    bool wt = extract32(dawrx, PPC_BIT_NR(59), 1);
>> +    bool wti = extract32(dawrx, PPC_BIT_NR(60), 1);
>> +    bool hv = extract32(dawrx, PPC_BIT_NR(61), 1);
>> +    bool sv = extract32(dawrx, PPC_BIT_NR(62), 1);
>> +    bool pr = extract32(dawrx, PPC_BIT_NR(62), 1);
>> -    if ((env->insns_flags2 & PPC2_ISA207S) &&
>> -        (wp == env->dawr_watchpoint[0])) {
>> -        dawrx = env->spr[SPR_DAWRX0];
>> -    } else if ((env->insns_flags2 & PPC2_ISA310) &&
>> -               (wp == env->dawr_watchpoint[1])) {
>> -        dawrx = env->spr[SPR_DAWRX1];
>> -    } else {
> 
> Previously, there were different ISA version checks resulting in different behaviour, now 
> with this change we are allowing for both ISA versions. Not sure if this would be an 
> expected behaviour?

Per the commit comment, the ISA version checks are done when registering the system 
register.  One cannot write to DAWRX1 unless ISA310, and therefore dawr_watchpoint[1] will 
not be set nor wp->id be 1.


r~