Re: [PATCH v2 1/3] perf: Add perf_event_attr::bp_priv
From: Tiezhu Yang
Date: Mon Jul 08 2024 - 06:07:09 EST
On 07/08/2024 03:36 PM, Peter Zijlstra wrote:
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@xxxxxxxxxxx>
---
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 ?
Looks good, I will do it in v3, like this:
(no need to define PERF_ATTR_SIZE_VER9 144)
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 */
+ };
};
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?
In short, when developing hardware watchpoint on LoongArch, we want to
set the same privilege passed by the ptrace user data, but there is no
a middle bridge to save this value like bp_addr, bp_type and bp_len, I
think this is a common issue for the archs which have privilege level
of breakpoint.
Thanks,
Tiezhu