| Message ID | 20241209-pmu_event_fixes-v1-2-d9525e90072c@rivosinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | SBI PMU event related fixes | expand |
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") ?
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") > ?
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
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 >
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; }
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(-)