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

[Shivaprasad G Bhat]

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

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


Thanks,

Shivaprasad