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

[Nicholas Piggin]

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.

Sorry for the late review.

>
> Signed-off-by: Ravi Bangoria <ravi.bangoria at linux.ibm.com>
> Signed-off-by: Shivaprasad G Bhat <sbhat at linux.ibm.com>
> ---
> Changelog:
> v6: 
> https://lore.kernel.org/qemu-devel/168871963321.58984.15628382614621248470.stgit@ltcd89-lp2/
> v6->v7:
>   - Sorry about the delay in sending out this version, I have dropped the
>     Reviewed-bys as suggested and converted the patch to RFC back again.
>   - Added the TCG support. Basically, converted the existing DAWR0 support
>     routines into macros for reuse by the DAWR1. Let me know if the macro
>     conversions should be moved to a separate independent patch.

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.

[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.

>
> +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.

Thanks,
Nick