Re: [RFC PATCH v7] ppc: Enable 2nd DAWR support on p10

[Shivaprasad G Bhat <sbhat@linux.ibm.com>]

Thanks for the review Nick!

On 1/23/24 17:36, Nicholas Piggin wrote:
> On Wed Nov 22, 2023 at 5:32 PM AEST, Shivaprasad G Bhat wrote:
>> Extend the existing watchpoint facility from TCG DAWR0 emulation
>> to DAWR1 on POWER10.
>>
>> As per the PAPR, bit 0 of byte 64 in pa-features property
>> indicates availability of 2nd DAWR registers. i.e. If this bit is set, 2nd
>> DAWR is present, otherwise not. Use KVM_CAP_PPC_DAWR1 capability to find
>> whether kvm supports 2nd DAWR or not. If it's supported, allow user to set
>> the pa-feature bit in guest DT using cap-dawr1 machine capability.
<snip>
> I don't really like the macros. I have nightmares from Linux going
> overboard with defining functions using spaghetti of generator macros.
>
> Could you just make most functions accept either SPR number or number
> (0, 1), or simply use if/else, to select between them?
>
> Splitting the change in 2 would be good, first add regs + TCG, then the
> spapr bits.
Sure.
> [snip]
>
>> diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
>> index a05bdf78c9..022b984e00 100644
>> --- a/target/ppc/misc_helper.c
>> +++ b/target/ppc/misc_helper.c
>> @@ -204,16 +204,24 @@ void helper_store_ciabr(CPUPPCState *env, target_ulong value)
>>       ppc_store_ciabr(env, value);
>>   }
>>
>> -void helper_store_dawr0(CPUPPCState *env, target_ulong value)
>> -{
>> -    ppc_store_dawr0(env, value);
>> +#define HELPER_STORE_DAWR(id)                                                 \
>> +void helper_store_dawr##id(CPUPPCState *env, target_ulong value)              \
>> +{                                                                             \
>> +    env->spr[SPR_DAWR##id] = value;                                           \
>>   }
>>
>> -void helper_store_dawrx0(CPUPPCState *env, target_ulong value)
>> -{
>> -    ppc_store_dawrx0(env, value);
>> +#define HELPER_STORE_DAWRX(id)                                                \
>> +void helper_store_dawrx##id(CPUPPCState *env, target_ulong value)             \
>> +{                                                                             \
>> +    env->spr[SPR_DAWRX##id] = value;                                          \
>>   }
> Did we lose the calls to ppc_store_dawr*? That will
> break direct register access (i.e., powernv) if so.

Yes. My test cases were more focussed on caps-dawr1 with pSeries

usecases, and missed this. I have taken care in the next version.

>> +HELPER_STORE_DAWR(0)
>> +HELPER_STORE_DAWRX(0)
>> +
>> +HELPER_STORE_DAWR(1)
>> +HELPER_STORE_DAWRX(1)
> I would say open-code all these too instead of generating. If we
> ever grew to >= 4 of them maybe, but as is this saves 2 lines,
> and makes 'helper_store_dawrx0' more difficult to grep for.

I open coded all of the functions with barely 12 lines more adding up

without macros.


The next version posted at

https://lore.kernel.org/qemu-devel/170679876639.188422.11634974895844092362.stgit@ltc-boston1.aus.stglabs.ibm.com/T/#t


Thanks,

Shivaprasad