Re: [PATCH v2 1/3] perf: Add perf_event_attr::bp_priv

[Peter Zijlstra <peterz@infradead.org>]

On Sat, Jul 06, 2024 at 01:31:03PM +0800, Tiezhu Yang wrote:
> 
> 
> On 07/05/2024 06:34 PM, Will Deacon wrote:
> > On Fri, Jun 21, 2024 at 03:39:08PM +0800, Tiezhu Yang wrote:
> > > Add a member "bp_priv" at the end of the uapi struct perf_event_attr
> > > to make a bridge between ptrace and hardware breakpoint.
> > > 
> > > This is preparation for later patch on some archs such as ARM, ARM64
> > > and LoongArch which have privilege level of breakpoint.
> > > 
> > > Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> > > ---
> > >  include/uapi/linux/perf_event.h | 3 +++
> > >  kernel/events/hw_breakpoint.c   | 1 +
> > >  2 files changed, 4 insertions(+)
> > > 
> > > diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> > > index 3a64499b0f5d..f9f917e854e6 100644
> > > --- a/include/uapi/linux/perf_event.h
> > > +++ b/include/uapi/linux/perf_event.h
> > > @@ -379,6 +379,7 @@ enum perf_event_read_format {
> > >  #define PERF_ATTR_SIZE_VER6	120	/* add: aux_sample_size */
> > >  #define PERF_ATTR_SIZE_VER7	128	/* add: sig_data */
> > >  #define PERF_ATTR_SIZE_VER8	136	/* add: config3 */
> > > +#define PERF_ATTR_SIZE_VER9	144	/* add: bp_priv */
> > > 
> > >  /*
> > >   * Hardware event_id to monitor via a performance monitoring event:
> > > @@ -522,6 +523,8 @@ struct perf_event_attr {
> > >  	__u64	sig_data;
> > > 
> > >  	__u64	config3; /* extension of config2 */
> > > +
> > > +	__u8	bp_priv; /* privilege level of breakpoint */
> > >  };
> > 
> > Why are we extending the user ABI for this? Perf events already have the
> > privilege encoded (indirectly) by the exclude_{user,kernel,hv} fields in
> > 'struct perf_event_attr'.
> 
> IMO, add bp_priv is to keep consistent with the other fields
> bp_type, bp_addr and bp_len, the meaning of bp_priv field is
> explicit and different with exclude_{user,kernel,hv} fields.

In case it wasn't obvious, this structure has __u64 granularity. You
don't just add a __u8 to the end. Also, since you mention consistency,
you might have noticed those other bp_ fields are in a union on
config[12], so why can't this live in a union on config3 ?

> Additionally, there is only 1 bit for exclude_{user,kernel,hv},
> but bp_priv field has at least 2 bit according to the explanation
> of Arm Reference Manual. At last, the initial aim is to remove
> the check condition to assign the value of hw->ctrl.privilege.
> 
> https://developer.arm.com/documentation/ddi0487/latest/
> 
> 1. D23: AArch64 System Register Descriptions (Page 8562)
>    D23.3.11 DBGWCR<n>_EL1, Debug Watchpoint Control Registers, n = 0 - 63
>    PAC, bits [2:1]
>    Privilege of access control. Determines the Exception level or levels at
> which a Watchpoint debug
>    event for watchpoint n is generated.
> 
> 2. G8: AArch32 System Register Descriptions (Page 12334)
>    G8.3.26 DBGWCR<n>, Debug Watchpoint Control Registers, n = 0 - 15
>    PAC, bits [2:1]
>    Privilege of access control. Determines the Exception level or levels at
> which a Watchpoint debug
>    event for watchpoint n is generated.

That's all clear as mud for someone that don't speak arm. Can you please
provide a coherent reason for all this that does not rely on external
resources?