1. Patchew
  2. linux
  3. [PATCH 0/2] SBI PMU event related fixes
  4. View patch

[PATCH 2/2] drivers/perf: riscv: Do not allow invalid raw event config

Atish Patra posted 2 patches 2 weeks, 3 days ago
There is a newer version of this series
[PATCH 2/2] drivers/perf: riscv: Do not allow invalid raw event config
Posted by Atish Patra 2 weeks, 3 days ago 
The SBI specification allows only lower 48bits of hpmeventX to be
configured via SBI PMU. Currently, the driver masks of the higher
bits but doesn't return an error. This will lead to an additional
SBI call for config matching which should return for an invalid
event error in most of the cases.

However, if a platform(i.e Rocket and sifive cores) implements a
bitmap of all bits in the event encoding this will lead to an
incorrect event being programmed leading to user confusion.

Report the error to the user if higher bits are set during the
event mapping itself to avoid the confusion and save an additional
SBI call.

Suggested-by: Samuel Holland <samuel.holland@sifive.com>
Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 drivers/perf/riscv_pmu_sbi.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
index 3473ba02abf3..fb6eda90f771 100644
--- a/drivers/perf/riscv_pmu_sbi.c
+++ b/drivers/perf/riscv_pmu_sbi.c
@@ -507,7 +507,7 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
 {
 	u32 type = event->attr.type;
 	u64 config = event->attr.config;
-	int ret;
+	int ret = -ENOENT;
 
 	/*
 	 * Ensure we are finished checking standard hardware events for
@@ -536,8 +536,11 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
 
 		switch (config >> 62) {
 		case 0:
-			ret = RISCV_PMU_RAW_EVENT_IDX;
-			*econfig = config & RISCV_PMU_RAW_EVENT_MASK;
+			/* Return error any bits [48-63] is set  as it is not allowed by the spec */
+			if (!(config & ~RISCV_PMU_RAW_EVENT_MASK)) {
+				*econfig = config & RISCV_PMU_RAW_EVENT_MASK;
+				ret = RISCV_PMU_RAW_EVENT_IDX;
+			}
 			break;
 		case 2:
 			ret = (config & 0xFFFF) | (SBI_PMU_EVENT_TYPE_FW << 16);
@@ -554,7 +557,6 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
 		}
 		break;
 	default:
-		ret = -ENOENT;
 		break;
 	}
 

-- 
2.34.1
Re: [PATCH 2/2] drivers/perf: riscv: Do not allow invalid raw event config
Posted by Palmer Dabbelt 2 weeks, 1 day ago 
On Mon, 09 Dec 2024 16:04:46 PST (-0800), Atish Patra wrote:
> The SBI specification allows only lower 48bits of hpmeventX to be
> configured via SBI PMU. Currently, the driver masks of the higher
> bits but doesn't return an error. This will lead to an additional
> SBI call for config matching which should return for an invalid
> event error in most of the cases.
>
> However, if a platform(i.e Rocket and sifive cores) implements a
> bitmap of all bits in the event encoding this will lead to an
> incorrect event being programmed leading to user confusion.
>
> Report the error to the user if higher bits are set during the
> event mapping itself to avoid the confusion and save an additional
> SBI call.
>
> Suggested-by: Samuel Holland <samuel.holland@sifive.com>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>  drivers/perf/riscv_pmu_sbi.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 3473ba02abf3..fb6eda90f771 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -507,7 +507,7 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
>  {
>  	u32 type = event->attr.type;
>  	u64 config = event->attr.config;
> -	int ret;
> +	int ret = -ENOENT;
>
>  	/*
>  	 * Ensure we are finished checking standard hardware events for
> @@ -536,8 +536,11 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
>
>  		switch (config >> 62) {
>  		case 0:
> -			ret = RISCV_PMU_RAW_EVENT_IDX;
> -			*econfig = config & RISCV_PMU_RAW_EVENT_MASK;
> +			/* Return error any bits [48-63] is set  as it is not allowed by the spec */
> +			if (!(config & ~RISCV_PMU_RAW_EVENT_MASK)) {
> +				*econfig = config & RISCV_PMU_RAW_EVENT_MASK;
> +				ret = RISCV_PMU_RAW_EVENT_IDX;
> +			}
>  			break;
>  		case 2:
>  			ret = (config & 0xFFFF) | (SBI_PMU_EVENT_TYPE_FW << 16);
> @@ -554,7 +557,6 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
>  		}
>  		break;
>  	default:
> -		ret = -ENOENT;
>  		break;
>  	}

This doesn't have a Fixes, is it 

    Fixes: f0c9363db2dd ("perf/riscv-sbi: Add platform specific firmware event handling")

?
Re: [PATCH 2/2] drivers/perf: riscv: Do not allow invalid raw event config
Posted by Atish Kumar Patra 2 weeks, 1 day ago 
On Wed, Dec 11, 2024 at 11:46 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Mon, 09 Dec 2024 16:04:46 PST (-0800), Atish Patra wrote:
> > The SBI specification allows only lower 48bits of hpmeventX to be
> > configured via SBI PMU. Currently, the driver masks of the higher
> > bits but doesn't return an error. This will lead to an additional
> > SBI call for config matching which should return for an invalid
> > event error in most of the cases.
> >
> > However, if a platform(i.e Rocket and sifive cores) implements a
> > bitmap of all bits in the event encoding this will lead to an
> > incorrect event being programmed leading to user confusion.
> >
> > Report the error to the user if higher bits are set during the
> > event mapping itself to avoid the confusion and save an additional
> > SBI call.
> >
> > Suggested-by: Samuel Holland <samuel.holland@sifive.com>
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > ---
> >  drivers/perf/riscv_pmu_sbi.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> > index 3473ba02abf3..fb6eda90f771 100644
> > --- a/drivers/perf/riscv_pmu_sbi.c
> > +++ b/drivers/perf/riscv_pmu_sbi.c
> > @@ -507,7 +507,7 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
> >  {
> >       u32 type = event->attr.type;
> >       u64 config = event->attr.config;
> > -     int ret;
> > +     int ret = -ENOENT;
> >
> >       /*
> >        * Ensure we are finished checking standard hardware events for
> > @@ -536,8 +536,11 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
> >
> >               switch (config >> 62) {
> >               case 0:
> > -                     ret = RISCV_PMU_RAW_EVENT_IDX;
> > -                     *econfig = config & RISCV_PMU_RAW_EVENT_MASK;
> > +                     /* Return error any bits [48-63] is set  as it is not allowed by the spec */
> > +                     if (!(config & ~RISCV_PMU_RAW_EVENT_MASK)) {
> > +                             *econfig = config & RISCV_PMU_RAW_EVENT_MASK;
> > +                             ret = RISCV_PMU_RAW_EVENT_IDX;
> > +                     }
> >                       break;
> >               case 2:
> >                       ret = (config & 0xFFFF) | (SBI_PMU_EVENT_TYPE_FW << 16);
> > @@ -554,7 +557,6 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
> >               }
> >               break;
> >       default:
> > -             ret = -ENOENT;
> >               break;
> >       }
>
> This doesn't have a Fixes, is it
>
>     Fixes: f0c9363db2dd ("perf/riscv-sbi: Add platform specific firmware event handling")
>

I was not sure if a Fixes tag was worth it as the current
behavior(masking off the higher bits) is there from the beginning of
the driver.
perf tool throws a warning as well if a user tries to set any of the
upper 16 bits as event attributes is set to 0-47.

If it should be backported, this is the correct fixes tag.
Fixes: e9991434596f ("RISC-V: Add perf platform driver based on SBI
PMU extension")


> ?
Re: [PATCH 2/2] drivers/perf: riscv: Do not allow invalid raw event config
Posted by Samuel Holland 2 weeks, 1 day ago 
Hi Atish,

On 2024-12-11 3:10 PM, Atish Kumar Patra wrote:
> On Wed, Dec 11, 2024 at 11:46 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>
>> On Mon, 09 Dec 2024 16:04:46 PST (-0800), Atish Patra wrote:
>>> The SBI specification allows only lower 48bits of hpmeventX to be
>>> configured via SBI PMU. Currently, the driver masks of the higher
>>> bits but doesn't return an error. This will lead to an additional
>>> SBI call for config matching which should return for an invalid
>>> event error in most of the cases.
>>>
>>> However, if a platform(i.e Rocket and sifive cores) implements a
>>> bitmap of all bits in the event encoding this will lead to an
>>> incorrect event being programmed leading to user confusion.
>>>
>>> Report the error to the user if higher bits are set during the
>>> event mapping itself to avoid the confusion and save an additional
>>> SBI call.
>>>
>>> Suggested-by: Samuel Holland <samuel.holland@sifive.com>
>>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
>>> ---
>>>  drivers/perf/riscv_pmu_sbi.c | 10 ++++++----
>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
>>> index 3473ba02abf3..fb6eda90f771 100644
>>> --- a/drivers/perf/riscv_pmu_sbi.c
>>> +++ b/drivers/perf/riscv_pmu_sbi.c
>>> @@ -507,7 +507,7 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
>>>  {
>>>       u32 type = event->attr.type;
>>>       u64 config = event->attr.config;
>>> -     int ret;
>>> +     int ret = -ENOENT;
>>>
>>>       /*
>>>        * Ensure we are finished checking standard hardware events for
>>> @@ -536,8 +536,11 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
>>>
>>>               switch (config >> 62) {
>>>               case 0:
>>> -                     ret = RISCV_PMU_RAW_EVENT_IDX;
>>> -                     *econfig = config & RISCV_PMU_RAW_EVENT_MASK;
>>> +                     /* Return error any bits [48-63] is set  as it is not allowed by the spec */
>>> +                     if (!(config & ~RISCV_PMU_RAW_EVENT_MASK)) {
>>> +                             *econfig = config & RISCV_PMU_RAW_EVENT_MASK;
>>> +                             ret = RISCV_PMU_RAW_EVENT_IDX;
>>> +                     }
>>>                       break;
>>>               case 2:
>>>                       ret = (config & 0xFFFF) | (SBI_PMU_EVENT_TYPE_FW << 16);
>>> @@ -554,7 +557,6 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
>>>               }
>>>               break;
>>>       default:
>>> -             ret = -ENOENT;
>>>               break;
>>>       }
>>
>> This doesn't have a Fixes, is it
>>
>>     Fixes: f0c9363db2dd ("perf/riscv-sbi: Add platform specific firmware event handling")
>>
> 
> I was not sure if a Fixes tag was worth it as the current
> behavior(masking off the higher bits) is there from the beginning of
> the driver.
> perf tool throws a warning as well if a user tries to set any of the
> upper 16 bits as event attributes is set to 0-47.
> 
> If it should be backported, this is the correct fixes tag.
> Fixes: e9991434596f ("RISC-V: Add perf platform driver based on SBI
> PMU extension")

I would agree the masking isn't a real issue before changing the event attribute
from 48 to 56 bits wide. However, this patch also does a drive-by fix of a
different bug introduced by f0c9363db2dd: there is no "case 1" or default case,
so the function would return an uninitialized value if the high bits are 01.
Maybe that should be a separate patch, with a Fixes tag?

Regards,
Samuel
Re: [PATCH 2/2] drivers/perf: riscv: Do not allow invalid raw event config
Posted by Atish Kumar Patra 2 weeks ago 
On Wed, Dec 11, 2024 at 8:06 PM Samuel Holland
<samuel.holland@sifive.com> wrote:
>
> Hi Atish,
>
> On 2024-12-11 3:10 PM, Atish Kumar Patra wrote:
> > On Wed, Dec 11, 2024 at 11:46 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:
> >>
> >> On Mon, 09 Dec 2024 16:04:46 PST (-0800), Atish Patra wrote:
> >>> The SBI specification allows only lower 48bits of hpmeventX to be
> >>> configured via SBI PMU. Currently, the driver masks of the higher
> >>> bits but doesn't return an error. This will lead to an additional
> >>> SBI call for config matching which should return for an invalid
> >>> event error in most of the cases.
> >>>
> >>> However, if a platform(i.e Rocket and sifive cores) implements a
> >>> bitmap of all bits in the event encoding this will lead to an
> >>> incorrect event being programmed leading to user confusion.
> >>>
> >>> Report the error to the user if higher bits are set during the
> >>> event mapping itself to avoid the confusion and save an additional
> >>> SBI call.
> >>>
> >>> Suggested-by: Samuel Holland <samuel.holland@sifive.com>
> >>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> >>> ---
> >>>  drivers/perf/riscv_pmu_sbi.c | 10 ++++++----
> >>>  1 file changed, 6 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> >>> index 3473ba02abf3..fb6eda90f771 100644
> >>> --- a/drivers/perf/riscv_pmu_sbi.c
> >>> +++ b/drivers/perf/riscv_pmu_sbi.c
> >>> @@ -507,7 +507,7 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
> >>>  {
> >>>       u32 type = event->attr.type;
> >>>       u64 config = event->attr.config;
> >>> -     int ret;
> >>> +     int ret = -ENOENT;
> >>>
> >>>       /*
> >>>        * Ensure we are finished checking standard hardware events for
> >>> @@ -536,8 +536,11 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
> >>>
> >>>               switch (config >> 62) {
> >>>               case 0:
> >>> -                     ret = RISCV_PMU_RAW_EVENT_IDX;
> >>> -                     *econfig = config & RISCV_PMU_RAW_EVENT_MASK;
> >>> +                     /* Return error any bits [48-63] is set  as it is not allowed by the spec */
> >>> +                     if (!(config & ~RISCV_PMU_RAW_EVENT_MASK)) {
> >>> +                             *econfig = config & RISCV_PMU_RAW_EVENT_MASK;
> >>> +                             ret = RISCV_PMU_RAW_EVENT_IDX;
> >>> +                     }
> >>>                       break;
> >>>               case 2:
> >>>                       ret = (config & 0xFFFF) | (SBI_PMU_EVENT_TYPE_FW << 16);
> >>> @@ -554,7 +557,6 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
> >>>               }
> >>>               break;
> >>>       default:
> >>> -             ret = -ENOENT;
> >>>               break;
> >>>       }
> >>
> >> This doesn't have a Fixes, is it
> >>
> >>     Fixes: f0c9363db2dd ("perf/riscv-sbi: Add platform specific firmware event handling")
> >>
> >
> > I was not sure if a Fixes tag was worth it as the current
> > behavior(masking off the higher bits) is there from the beginning of
> > the driver.
> > perf tool throws a warning as well if a user tries to set any of the
> > upper 16 bits as event attributes is set to 0-47.
> >
> > If it should be backported, this is the correct fixes tag.
> > Fixes: e9991434596f ("RISC-V: Add perf platform driver based on SBI
> > PMU extension")
>
> I would agree the masking isn't a real issue before changing the event attribute
> from 48 to 56 bits wide. However, this patch also does a drive-by fix of a
> different bug introduced by f0c9363db2dd: there is no "case 1" or default case,
> so the function would return an uninitialized value if the high bits are 01.
> Maybe that should be a separate patch, with a Fixes tag?
>

Good eye ;). I will split this one into two with a fixes tag for the
switch case.

> Regards,
> Samuel
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.