[PATCH v2 0/3] hw_breakpoint: Save privilege of access control via ptrace

[PATCH v2 0/3] hw_breakpoint: Save privilege of access control via ptrace

[Tiezhu Yang]

Hi all,

Thank you very much for your feedbacks in the v1 patch.
This series is based on 6.10-rc4, cross compile tested only.

As far as I can tell, these changes are not relevant with tools/perf,
on some archs such as ARM, ARM64 and LoongArch which have privilege
level of breakpoint, the initial aim is to make use of the value of
ctrl->privilege saved in decode_ctrl_reg() and then remove the check
whether bp virtual address is in kernel space to assign value for
hw->ctrl.privilege in arch_build_bp_info().

v2:
  -- Put the new member "bp_priv" at the end of the uapi
     struct perf_event_attr and add PERF_ATTR_SIZE_VER9.
  -- Update the commit message to make the goal clear.

Tiezhu Yang (3):
  perf: Add perf_event_attr::bp_priv
  arm: hw_breakpoint: Save privilege of access control via ptrace
  arm64: hw_breakpoint: Save privilege of access control via ptrace

 arch/arm/kernel/hw_breakpoint.c   |  4 +---
 arch/arm/kernel/ptrace.c          |  2 ++
 arch/arm64/kernel/hw_breakpoint.c | 11 ++---------
 arch/arm64/kernel/ptrace.c        |  2 ++
 include/uapi/linux/perf_event.h   |  3 +++
 kernel/events/hw_breakpoint.c     |  1 +
 6 files changed, 11 insertions(+), 12 deletions(-)

-- 
2.42.0

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

[Tiezhu Yang]

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 */
 };
 
 /*
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;
 }
 
-- 
2.42.0

[PATCH v2 2/3] arm: hw_breakpoint: Save privilege of access control via ptrace

[Tiezhu Yang]

Currently, decode_ctrl_reg() saves the privilege of access control
which is not used anymore, arch_build_bp_info() checks whether bp
virtual address is in kernel space to construct hw->ctrl.privilege,
the process seems not reasonable.

The value of ctrl->privilege saved in decode_ctrl_reg() can be used
in arch_build_bp_info(), there is no need to check bp virtual address
to assign value for hw->ctrl.privilege, just make use of "bp_priv" in
the struct perf_event_attr to save the privilege of access control via
ptrace for hardware breakpoint.

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 arch/arm/kernel/hw_breakpoint.c | 4 +---
 arch/arm/kernel/ptrace.c        | 2 ++
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index a12efd0f43e8..7720d39473d9 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -568,9 +568,7 @@ static int arch_build_bp_info(struct perf_event *bp,
 	hw->address = attr->bp_addr;
 
 	/* Privilege */
-	hw->ctrl.privilege = ARM_BREAKPOINT_USER;
-	if (arch_check_bp_in_kernelspace(hw))
-		hw->ctrl.privilege |= ARM_BREAKPOINT_PRIV;
+	hw->ctrl.privilege = attr->bp_priv;
 
 	/* Enabled? */
 	hw->ctrl.enabled = !attr->disabled;
diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index c421a899fc84..0d6d6b2a57a0 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -422,6 +422,7 @@ static struct perf_event *ptrace_hbp_create(struct task_struct *tsk, int type)
 	attr.bp_addr	= 0;
 	attr.bp_len	= HW_BREAKPOINT_LEN_4;
 	attr.bp_type	= type;
+	attr.bp_priv	= ARM_BREAKPOINT_USER;
 	attr.disabled	= 1;
 
 	return register_user_hw_breakpoint(&attr, ptrace_hbptriggered, NULL,
@@ -530,6 +531,7 @@ static int ptrace_sethbpregs(struct task_struct *tsk, long num,
 
 		attr.bp_len	= gen_len;
 		attr.bp_type	= gen_type;
+		attr.bp_priv	= ctrl.privilege;
 		attr.disabled	= !ctrl.enabled;
 	}
 
-- 
2.42.0

[PATCH v2 3/3] arm64: hw_breakpoint: Save privilege of access control via ptrace

[Tiezhu Yang]

Currently, decode_ctrl_reg() saves the privilege of access control
which is not used anymore, arch_build_bp_info() checks whether bp
virtual address is in kernel space to construct hw->ctrl.privilege,
the process seems not reasonable.

The value of ctrl->privilege saved in decode_ctrl_reg() can be used
in arch_build_bp_info(), there is no need to check bp virtual address
to assign value for hw->ctrl.privilege, just make use of "bp_priv" in
the struct perf_event_attr to save the privilege of access control via
ptrace for hardware breakpoint.

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 arch/arm64/kernel/hw_breakpoint.c | 11 ++---------
 arch/arm64/kernel/ptrace.c        |  2 ++
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
index 722ac45f9f7b..06e34bcdcf92 100644
--- a/arch/arm64/kernel/hw_breakpoint.c
+++ b/arch/arm64/kernel/hw_breakpoint.c
@@ -486,15 +486,8 @@ static int arch_build_bp_info(struct perf_event *bp,
 	/* Address */
 	hw->address = attr->bp_addr;
 
-	/*
-	 * Privilege
-	 * Note that we disallow combined EL0/EL1 breakpoints because
-	 * that would complicate the stepping code.
-	 */
-	if (arch_check_bp_in_kernelspace(hw))
-		hw->ctrl.privilege = AARCH64_BREAKPOINT_EL1;
-	else
-		hw->ctrl.privilege = AARCH64_BREAKPOINT_EL0;
+	/* Privilege */
+	hw->ctrl.privilege = attr->bp_priv;
 
 	/* Enabled? */
 	hw->ctrl.enabled = !attr->disabled;
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 0d022599eb61..3b37c4a2e0d4 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -309,6 +309,7 @@ static struct perf_event *ptrace_hbp_create(unsigned int note_type,
 	attr.bp_addr	= 0;
 	attr.bp_len	= HW_BREAKPOINT_LEN_4;
 	attr.bp_type	= type;
+	attr.bp_priv	= AARCH64_BREAKPOINT_EL0;
 	attr.disabled	= 1;
 
 	bp = register_user_hw_breakpoint(&attr, ptrace_hbptriggered, NULL, tsk);
@@ -352,6 +353,7 @@ static int ptrace_hbp_fill_attr_ctrl(unsigned int note_type,
 	attr->bp_len	= len;
 	attr->bp_type	= type;
 	attr->bp_addr	+= offset;
+	attr->bp_priv	= ctrl.privilege;
 
 	return 0;
 }
-- 
2.42.0

Re: [PATCH v2 0/3] hw_breakpoint: Save privilege of access control via ptrace

[Tiezhu Yang]

On 6/21/24 15:39, Tiezhu Yang wrote:
> Hi all,
> 
> Thank you very much for your feedbacks in the v1 patch.
> This series is based on 6.10-rc4, cross compile tested only.
> 
> As far as I can tell, these changes are not relevant with tools/perf,
> on some archs such as ARM, ARM64 and LoongArch which have privilege
> level of breakpoint, the initial aim is to make use of the value of
> ctrl->privilege saved in decode_ctrl_reg() and then remove the check
> whether bp virtual address is in kernel space to assign value for
> hw->ctrl.privilege in arch_build_bp_info().
> 
> v2:
>    -- Put the new member "bp_priv" at the end of the uapi
>       struct perf_event_attr and add PERF_ATTR_SIZE_VER9.
>    -- Update the commit message to make the goal clear.
> 
> Tiezhu Yang (3):
>    perf: Add perf_event_attr::bp_priv
>    arm: hw_breakpoint: Save privilege of access control via ptrace
>    arm64: hw_breakpoint: Save privilege of access control via ptrace
> 
>   arch/arm/kernel/hw_breakpoint.c   |  4 +---
>   arch/arm/kernel/ptrace.c          |  2 ++
>   arch/arm64/kernel/hw_breakpoint.c | 11 ++---------
>   arch/arm64/kernel/ptrace.c        |  2 ++
>   include/uapi/linux/perf_event.h   |  3 +++
>   kernel/events/hw_breakpoint.c     |  1 +
>   6 files changed, 11 insertions(+), 12 deletions(-)

Ping, any more comments? Is it possible to merge this series
for the coming merge window?

If this patch series has no value and is not acceptable,
or what should I do to update, please let me know.

Thanks,
Tiezhu

Re: [PATCH v2 0/3] hw_breakpoint: Save privilege of access control via ptrace

[Russell King (Oracle)]

On Thu, Jul 04, 2024 at 10:47:59PM +0800, Tiezhu Yang wrote:
> On 6/21/24 15:39, Tiezhu Yang wrote:
> > Hi all,
> > 
> > Thank you very much for your feedbacks in the v1 patch.
> > This series is based on 6.10-rc4, cross compile tested only.
> > 
> > As far as I can tell, these changes are not relevant with tools/perf,
> > on some archs such as ARM, ARM64 and LoongArch which have privilege
> > level of breakpoint, the initial aim is to make use of the value of
> > ctrl->privilege saved in decode_ctrl_reg() and then remove the check
> > whether bp virtual address is in kernel space to assign value for
> > hw->ctrl.privilege in arch_build_bp_info().
> > 
> > v2:
> >    -- Put the new member "bp_priv" at the end of the uapi
> >       struct perf_event_attr and add PERF_ATTR_SIZE_VER9.
> >    -- Update the commit message to make the goal clear.
> > 
> > Tiezhu Yang (3):
> >    perf: Add perf_event_attr::bp_priv
> >    arm: hw_breakpoint: Save privilege of access control via ptrace
> >    arm64: hw_breakpoint: Save privilege of access control via ptrace
> > 
> >   arch/arm/kernel/hw_breakpoint.c   |  4 +---
> >   arch/arm/kernel/ptrace.c          |  2 ++
> >   arch/arm64/kernel/hw_breakpoint.c | 11 ++---------
> >   arch/arm64/kernel/ptrace.c        |  2 ++
> >   include/uapi/linux/perf_event.h   |  3 +++
> >   kernel/events/hw_breakpoint.c     |  1 +
> >   6 files changed, 11 insertions(+), 12 deletions(-)
> 
> Ping, any more comments? Is it possible to merge this series
> for the coming merge window?

Maybe someone who knows the perf stuff can review it and give an
opinion?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Will Deacon]

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

Will

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

[Tiezhu Yang]

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

Thanks,
Tiezhu

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

[Peter Zijlstra]

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?

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

[Tiezhu Yang]

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@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 ?

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

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

[Will Deacon]

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

I disagree, as these are properties specific to hw_breakpoint. Privilege
is not.

> , the meaning of bp_priv field is
> explicit and different with exclude_{user,kernel,hv} fields.

How? You're changing the user ABI here, it needs to be properly justified.

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

Why? What problem is hw->ctrl.privilege causing?

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

You're just quoting bits of the Arm ARM. The architectural permission
checking is much more complicated and takes into account all of the PAC,
HMC, SSC and SSCE fields, but Linux doesn't need to care about most of
that because it's only managing user, kernel and possibly hypervisor.
These three can be expressed with the exclude_ options that we already
have.

So I really don't understand the rationale here.

Will

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

[Tiezhu Yang]

On 07/08/2024 07:15 PM, Will Deacon 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@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
>
> I disagree, as these are properties specific to hw_breakpoint. Privilege
> is not.
>
>> , the meaning of bp_priv field is
>> explicit and different with exclude_{user,kernel,hv} fields.
>
> How? You're changing the user ABI here, it needs to be properly justified.
>
>> 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.
>
> Why? What problem is hw->ctrl.privilege causing?
>
>> 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.
>
> You're just quoting bits of the Arm ARM. The architectural permission
> checking is much more complicated and takes into account all of the PAC,
> HMC, SSC and SSCE fields, but Linux doesn't need to care about most of
> that because it's only managing user, kernel and possibly hypervisor.
> These three can be expressed with the exclude_ options that we already
> have.
>
> So I really don't understand the rationale here.

Here is a reply to Peter Zijlstra, I hope that helps.

https://lore.kernel.org/lkml/da0c95e7-c676-e0c0-8b90-b1ea5fc7b72f@loongson.cn/

Thanks,
Tiezhu