[PATCH 0/2] SBI PMU event related fixes

[PATCH 0/2] SBI PMU event related fixes

[Atish Patra]

Here are two minor improvement/fixes in the PMU event path. The first patch
was part of the series[1]. The 2nd patch was suggested during the series
review. 

While the series can only be merged once SBI v3.0 is frozen, these two
patches can be independent of SBI v3.0 and can be merged sooner. Hence, these
two patches are sent as a separate series.
 
[1] https://lore.kernel.org/kvm/20241119-pmu_event_info-v1-7-a4f9691421f8@rivosinc.com/T/#u

To: Paul Walmsley <paul.walmsley@sifive.com>
To: Palmer Dabbelt <palmer@dabbelt.com>
To: Atish Patra <atishp@atishpatra.org>
To: Anup Patel <anup@brainfault.org>
To: Will Deacon <will@kernel.org>
To: Mark Rutland <mark.rutland@arm.com>
To: Mayuresh Chitale <mchitale@ventanamicro.com>
To: Samuel Holland <samuel.holland@sifive.com>
Cc: Palmer Dabbelt <palmer@rivosinc.com>
Cc: linux-riscv@lists.infradead.org
Cc: linux-kernel@vger.kernel.org

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
Atish Patra (2):
      drivers/perf: riscv: Fix Platform firmware event data
      drivers/perf: riscv: Do not allow invalid raw event config

 arch/riscv/include/asm/sbi.h |  1 +
 drivers/perf/riscv_pmu_sbi.c | 20 ++++++++++----------
 2 files changed, 11 insertions(+), 10 deletions(-)
---
base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4
change-id: 20241209-pmu_event_fixes-72cd448624a8
--
Regards,
Atish patra


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 1/2] drivers/perf: riscv: Fix Platform firmware event data

[Atish Patra]

Platform firmware event data field is allowed to be 62 bits for
Linux as uppper most two bits are reserved to indicate SBI fw or
platform specific firmware events.
However, the event data field is masked as per the hardware raw
event mask which is not correct.

Fix the platform firmware event data field with proper mask.

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

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 arch/riscv/include/asm/sbi.h |  1 +
 drivers/perf/riscv_pmu_sbi.c | 12 +++++-------
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
index 6c82318065cf..3d250824178b 100644
--- a/arch/riscv/include/asm/sbi.h
+++ b/arch/riscv/include/asm/sbi.h
@@ -159,6 +159,7 @@ struct riscv_pmu_snapshot_data {
 };
 
 #define RISCV_PMU_RAW_EVENT_MASK GENMASK_ULL(47, 0)
+#define RISCV_PMU_PLAT_FW_EVENT_MASK GENMASK_ULL(61, 0)
 #define RISCV_PMU_RAW_EVENT_IDX 0x20000
 #define RISCV_PLAT_FW_EVENT	0xFFFF
 
diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
index 1aa303f76cc7..3473ba02abf3 100644
--- a/drivers/perf/riscv_pmu_sbi.c
+++ b/drivers/perf/riscv_pmu_sbi.c
@@ -507,7 +507,6 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
 {
 	u32 type = event->attr.type;
 	u64 config = event->attr.config;
-	u64 raw_config_val;
 	int ret;
 
 	/*
@@ -528,21 +527,20 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
 	case PERF_TYPE_RAW:
 		/*
 		 * As per SBI specification, the upper 16 bits must be unused
-		 * for a raw event.
+		 * for a hardware raw event.
 		 * Bits 63:62 are used to distinguish between raw events
 		 * 00 - Hardware raw event
 		 * 10 - SBI firmware events
 		 * 11 - Risc-V platform specific firmware event
 		 */
-		raw_config_val = config & RISCV_PMU_RAW_EVENT_MASK;
+
 		switch (config >> 62) {
 		case 0:
 			ret = RISCV_PMU_RAW_EVENT_IDX;
-			*econfig = raw_config_val;
+			*econfig = config & RISCV_PMU_RAW_EVENT_MASK;
 			break;
 		case 2:
-			ret = (raw_config_val & 0xFFFF) |
-				(SBI_PMU_EVENT_TYPE_FW << 16);
+			ret = (config & 0xFFFF) | (SBI_PMU_EVENT_TYPE_FW << 16);
 			break;
 		case 3:
 			/*
@@ -551,7 +549,7 @@ static int pmu_sbi_event_map(struct perf_event *event, u64 *econfig)
 			 * Event data - raw event encoding
 			 */
 			ret = SBI_PMU_EVENT_TYPE_FW << 16 | RISCV_PLAT_FW_EVENT;
-			*econfig = raw_config_val;
+			*econfig = config & RISCV_PMU_PLAT_FW_EVENT_MASK;
 			break;
 		}
 		break;

-- 
2.34.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Atish Patra]

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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Palmer Dabbelt]

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")

?

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Atish Kumar Patra]

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")


> ?

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Samuel Holland]

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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Atish Kumar Patra]

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
>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv