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

[Tiezhu Yang]

On 07/09/2024 06:03 PM, Will Deacon wrote:
> On Tue, Jul 09, 2024 at 05:55:04PM +0800, Tiezhu Yang wrote:
> > When developing hardware watchpoint on LoongArch, we want to set the
> > same privilege passed by the ptrace user data, but there is no bridge
> > to save this value like bp_addr, bp_type and bp_len. This is a common
> > issue for the archs which have privilege level of breakpoint.
> >
> > Add a member "bp_priv" which lives in a union on config3 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@xxxxxxxxxxx>
> > ---
> >  include/uapi/linux/perf_event.h | 5 ++++-
> >  kernel/events/hw_breakpoint.c   | 1 +
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> > index 3a64499b0f5d..abe8da7a1f60 100644
> > --- a/include/uapi/linux/perf_event.h
> > +++ b/include/uapi/linux/perf_event.h
> > @@ -521,7 +521,10 @@ struct perf_event_attr {
> >  	 */
> >  	__u64	sig_data;
> >
> > -	__u64	config3; /* extension of config2 */
> > +	union {
> > +		__u8	bp_priv; /* privilege level of breakpoint */
> > +		__u64	config3; /* extension of config2 */
> > +	};
> >  };
> >
> >  /*
> > diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> > index 6c2cb4e4f48d..3ad16b226e4f 100644
> > --- a/kernel/events/hw_breakpoint.c
> > +++ b/kernel/events/hw_breakpoint.c
> > @@ -754,6 +754,7 @@ static void hw_breakpoint_copy_attr(struct perf_event_attr *to,
> >  	to->bp_addr = from->bp_addr;
> >  	to->bp_type = from->bp_type;
> >  	to->bp_len  = from->bp_len;
> > +	to->bp_priv = from->bp_priv;
> >  	to->disabled = from->disabled;
> >  }
>
> Sorry, but I still don't see why we should change uapi for this. As I
> have said multiple times, this is unnecessary.

Thanks for your review, I see your point but let us wait
for more comments from perf people.

Thanks,
Tiezhu