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.st...@ltc-boston1.aus.stglabs.ibm.com/T/#t
Thanks,
Shivaprasad
