SPE_FEAT_FDS adds the ability to filter on the data source of packets.
Like the other existing filters, enable filtering with PMSFCR_EL1.FDS
when any of the filter bits are set.
Each bit maps to data sources 0-63 described by bits[0:5] in the data
source packet (although the full range of data source is 16 bits so
higher value data sources can't be filtered on). The filter is an OR of
all the bits, so for example setting bits 0 and 3 filters packets from
data sources 0 OR 3.
Signed-off-by: James Clark <james.clark@linaro.org>
---
drivers/perf/arm_spe_pmu.c | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
index 9309b846f642..d04318411f77 100644
--- a/drivers/perf/arm_spe_pmu.c
+++ b/drivers/perf/arm_spe_pmu.c
@@ -87,6 +87,7 @@ struct arm_spe_pmu {
#define SPE_PMU_FEAT_INV_FILT_EVT (1UL << 6)
#define SPE_PMU_FEAT_DISCARD (1UL << 7)
#define SPE_PMU_FEAT_EFT (1UL << 8)
+#define SPE_PMU_FEAT_FDS (1UL << 9)
#define SPE_PMU_FEAT_DEV_PROBED (1UL << 63)
u64 features;
@@ -232,6 +233,10 @@ static const struct attribute_group arm_spe_pmu_cap_group = {
#define ATTR_CFG_FLD_inv_event_filter_LO 0
#define ATTR_CFG_FLD_inv_event_filter_HI 63
+#define ATTR_CFG_FLD_data_src_filter_CFG config4 /* PMSDSFR_EL1 */
+#define ATTR_CFG_FLD_data_src_filter_LO 0
+#define ATTR_CFG_FLD_data_src_filter_HI 63
+
GEN_PMU_FORMAT_ATTR(ts_enable);
GEN_PMU_FORMAT_ATTR(pa_enable);
GEN_PMU_FORMAT_ATTR(pct_enable);
@@ -248,6 +253,7 @@ GEN_PMU_FORMAT_ATTR(float_filter);
GEN_PMU_FORMAT_ATTR(float_filter_mask);
GEN_PMU_FORMAT_ATTR(event_filter);
GEN_PMU_FORMAT_ATTR(inv_event_filter);
+GEN_PMU_FORMAT_ATTR(data_src_filter);
GEN_PMU_FORMAT_ATTR(min_latency);
GEN_PMU_FORMAT_ATTR(discard);
@@ -268,6 +274,7 @@ static struct attribute *arm_spe_pmu_formats_attr[] = {
&format_attr_float_filter_mask.attr,
&format_attr_event_filter.attr,
&format_attr_inv_event_filter.attr,
+ &format_attr_data_src_filter.attr,
&format_attr_min_latency.attr,
&format_attr_discard.attr,
NULL,
@@ -286,6 +293,9 @@ static umode_t arm_spe_pmu_format_attr_is_visible(struct kobject *kobj,
if (attr == &format_attr_inv_event_filter.attr && !(spe_pmu->features & SPE_PMU_FEAT_INV_FILT_EVT))
return 0;
+ if (attr == &format_attr_data_src_filter.attr && !(spe_pmu->features & SPE_PMU_FEAT_FDS))
+ return 0;
+
if ((attr == &format_attr_branch_filter_mask.attr ||
attr == &format_attr_load_filter_mask.attr ||
attr == &format_attr_store_filter_mask.attr ||
@@ -406,6 +416,9 @@ static u64 arm_spe_event_to_pmsfcr(struct perf_event *event)
if (ATTR_CFG_GET_FLD(attr, inv_event_filter))
reg |= PMSFCR_EL1_FnE;
+ if (ATTR_CFG_GET_FLD(attr, data_src_filter))
+ reg |= PMSFCR_EL1_FDS;
+
if (ATTR_CFG_GET_FLD(attr, min_latency))
reg |= PMSFCR_EL1_FL;
@@ -430,6 +443,12 @@ static u64 arm_spe_event_to_pmslatfr(struct perf_event *event)
return FIELD_PREP(PMSLATFR_EL1_MINLAT, ATTR_CFG_GET_FLD(attr, min_latency));
}
+static u64 arm_spe_event_to_pmsdsfr(struct perf_event *event)
+{
+ struct perf_event_attr *attr = &event->attr;
+ return ATTR_CFG_GET_FLD(attr, data_src_filter);
+}
+
static void arm_spe_pmu_pad_buf(struct perf_output_handle *handle, int len)
{
struct arm_spe_pmu_buf *buf = perf_get_aux(handle);
@@ -788,6 +807,10 @@ static int arm_spe_pmu_event_init(struct perf_event *event)
if (arm_spe_event_to_pmsnevfr(event) & arm_spe_pmsevfr_res0(spe_pmu->pmsver))
return -EOPNOTSUPP;
+ if (arm_spe_event_to_pmsdsfr(event) &&
+ !(spe_pmu->features & SPE_PMU_FEAT_FDS))
+ return -EOPNOTSUPP;
+
if (attr->exclude_idle)
return -EOPNOTSUPP;
@@ -857,6 +880,11 @@ static void arm_spe_pmu_start(struct perf_event *event, int flags)
write_sysreg_s(reg, SYS_PMSNEVFR_EL1);
}
+ if (spe_pmu->features & SPE_PMU_FEAT_FDS) {
+ reg = arm_spe_event_to_pmsdsfr(event);
+ write_sysreg_s(reg, SYS_PMSDSFR_EL1);
+ }
+
reg = arm_spe_event_to_pmslatfr(event);
write_sysreg_s(reg, SYS_PMSLATFR_EL1);
@@ -1116,6 +1144,9 @@ static void __arm_spe_pmu_dev_probe(void *info)
if (FIELD_GET(PMSIDR_EL1_EFT, reg))
spe_pmu->features |= SPE_PMU_FEAT_EFT;
+ if (FIELD_GET(PMSIDR_EL1_FDS, reg))
+ spe_pmu->features |= SPE_PMU_FEAT_FDS;
+
/* This field has a spaced out encoding, so just use a look-up */
fld = FIELD_GET(PMSIDR_EL1_INTERVAL, reg);
switch (fld) {
--
2.34.1On Tue, May 06, 2025 at 12:41:39PM +0100, James Clark wrote:
> SPE_FEAT_FDS adds the ability to filter on the data source of packets.
> Like the other existing filters, enable filtering with PMSFCR_EL1.FDS
> when any of the filter bits are set.
>
> Each bit maps to data sources 0-63 described by bits[0:5] in the data
> source packet (although the full range of data source is 16 bits so
> higher value data sources can't be filtered on). The filter is an OR of
> all the bits, so for example setting bits 0 and 3 filters packets from
> data sources 0 OR 3.
As Arm ARM says:
0b0 : If PMSFCR_EL1.FDS is 1, do not record load operations that have
bits [5:0] of the Data Source packet set to <m>.
0b1 : Load operations with Data Source <m> are unaffected by
PMSFCR_EL1.FDS.
We need extra handling for this configuration (0b0 means filtering,
0b1 means no affaction):
- By default, the driver should set all bits in the 'data_src_filter'
field.
- The perf tool needs an extra patch in userspace to initialize all
bits in config4 unless user specify other values.
Thanks,
Leo
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
> drivers/perf/arm_spe_pmu.c | 31 +++++++++++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
>
> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> index 9309b846f642..d04318411f77 100644
> --- a/drivers/perf/arm_spe_pmu.c
> +++ b/drivers/perf/arm_spe_pmu.c
> @@ -87,6 +87,7 @@ struct arm_spe_pmu {
> #define SPE_PMU_FEAT_INV_FILT_EVT (1UL << 6)
> #define SPE_PMU_FEAT_DISCARD (1UL << 7)
> #define SPE_PMU_FEAT_EFT (1UL << 8)
> +#define SPE_PMU_FEAT_FDS (1UL << 9)
> #define SPE_PMU_FEAT_DEV_PROBED (1UL << 63)
> u64 features;
>
> @@ -232,6 +233,10 @@ static const struct attribute_group arm_spe_pmu_cap_group = {
> #define ATTR_CFG_FLD_inv_event_filter_LO 0
> #define ATTR_CFG_FLD_inv_event_filter_HI 63
>
> +#define ATTR_CFG_FLD_data_src_filter_CFG config4 /* PMSDSFR_EL1 */
> +#define ATTR_CFG_FLD_data_src_filter_LO 0
> +#define ATTR_CFG_FLD_data_src_filter_HI 63
> +
> GEN_PMU_FORMAT_ATTR(ts_enable);
> GEN_PMU_FORMAT_ATTR(pa_enable);
> GEN_PMU_FORMAT_ATTR(pct_enable);
> @@ -248,6 +253,7 @@ GEN_PMU_FORMAT_ATTR(float_filter);
> GEN_PMU_FORMAT_ATTR(float_filter_mask);
> GEN_PMU_FORMAT_ATTR(event_filter);
> GEN_PMU_FORMAT_ATTR(inv_event_filter);
> +GEN_PMU_FORMAT_ATTR(data_src_filter);
> GEN_PMU_FORMAT_ATTR(min_latency);
> GEN_PMU_FORMAT_ATTR(discard);
>
> @@ -268,6 +274,7 @@ static struct attribute *arm_spe_pmu_formats_attr[] = {
> &format_attr_float_filter_mask.attr,
> &format_attr_event_filter.attr,
> &format_attr_inv_event_filter.attr,
> + &format_attr_data_src_filter.attr,
> &format_attr_min_latency.attr,
> &format_attr_discard.attr,
> NULL,
> @@ -286,6 +293,9 @@ static umode_t arm_spe_pmu_format_attr_is_visible(struct kobject *kobj,
> if (attr == &format_attr_inv_event_filter.attr && !(spe_pmu->features & SPE_PMU_FEAT_INV_FILT_EVT))
> return 0;
>
> + if (attr == &format_attr_data_src_filter.attr && !(spe_pmu->features & SPE_PMU_FEAT_FDS))
> + return 0;
> +
> if ((attr == &format_attr_branch_filter_mask.attr ||
> attr == &format_attr_load_filter_mask.attr ||
> attr == &format_attr_store_filter_mask.attr ||
> @@ -406,6 +416,9 @@ static u64 arm_spe_event_to_pmsfcr(struct perf_event *event)
> if (ATTR_CFG_GET_FLD(attr, inv_event_filter))
> reg |= PMSFCR_EL1_FnE;
>
> + if (ATTR_CFG_GET_FLD(attr, data_src_filter))
> + reg |= PMSFCR_EL1_FDS;
> +
> if (ATTR_CFG_GET_FLD(attr, min_latency))
> reg |= PMSFCR_EL1_FL;
>
> @@ -430,6 +443,12 @@ static u64 arm_spe_event_to_pmslatfr(struct perf_event *event)
> return FIELD_PREP(PMSLATFR_EL1_MINLAT, ATTR_CFG_GET_FLD(attr, min_latency));
> }
>
> +static u64 arm_spe_event_to_pmsdsfr(struct perf_event *event)
> +{
> + struct perf_event_attr *attr = &event->attr;
> + return ATTR_CFG_GET_FLD(attr, data_src_filter);
> +}
> +
> static void arm_spe_pmu_pad_buf(struct perf_output_handle *handle, int len)
> {
> struct arm_spe_pmu_buf *buf = perf_get_aux(handle);
> @@ -788,6 +807,10 @@ static int arm_spe_pmu_event_init(struct perf_event *event)
> if (arm_spe_event_to_pmsnevfr(event) & arm_spe_pmsevfr_res0(spe_pmu->pmsver))
> return -EOPNOTSUPP;
>
> + if (arm_spe_event_to_pmsdsfr(event) &&
> + !(spe_pmu->features & SPE_PMU_FEAT_FDS))
> + return -EOPNOTSUPP;
> +
> if (attr->exclude_idle)
> return -EOPNOTSUPP;
>
> @@ -857,6 +880,11 @@ static void arm_spe_pmu_start(struct perf_event *event, int flags)
> write_sysreg_s(reg, SYS_PMSNEVFR_EL1);
> }
>
> + if (spe_pmu->features & SPE_PMU_FEAT_FDS) {
> + reg = arm_spe_event_to_pmsdsfr(event);
> + write_sysreg_s(reg, SYS_PMSDSFR_EL1);
> + }
> +
> reg = arm_spe_event_to_pmslatfr(event);
> write_sysreg_s(reg, SYS_PMSLATFR_EL1);
>
> @@ -1116,6 +1144,9 @@ static void __arm_spe_pmu_dev_probe(void *info)
> if (FIELD_GET(PMSIDR_EL1_EFT, reg))
> spe_pmu->features |= SPE_PMU_FEAT_EFT;
>
> + if (FIELD_GET(PMSIDR_EL1_FDS, reg))
> + spe_pmu->features |= SPE_PMU_FEAT_FDS;
> +
> /* This field has a spaced out encoding, so just use a look-up */
> fld = FIELD_GET(PMSIDR_EL1_INTERVAL, reg);
> switch (fld) {
>
> --
> 2.34.1
>
On 20/05/2025 2:46 pm, Leo Yan wrote:
> On Tue, May 06, 2025 at 12:41:39PM +0100, James Clark wrote:
>> SPE_FEAT_FDS adds the ability to filter on the data source of packets.
>> Like the other existing filters, enable filtering with PMSFCR_EL1.FDS
>> when any of the filter bits are set.
>>
>> Each bit maps to data sources 0-63 described by bits[0:5] in the data
>> source packet (although the full range of data source is 16 bits so
>> higher value data sources can't be filtered on). The filter is an OR of
>> all the bits, so for example setting bits 0 and 3 filters packets from
>> data sources 0 OR 3.
>
> As Arm ARM says:
>
> 0b0 : If PMSFCR_EL1.FDS is 1, do not record load operations that have
> bits [5:0] of the Data Source packet set to <m>.
> 0b1 : Load operations with Data Source <m> are unaffected by
> PMSFCR_EL1.FDS.
>
> We need extra handling for this configuration (0b0 means filtering,
> 0b1 means no affaction):
>
> - By default, the driver should set all bits in the 'data_src_filter'
> field.
>
> - The perf tool needs an extra patch in userspace to initialize all
> bits in config4 unless user specify other values.
>
> Thanks,
> Leo
>
Did you take into account PMSFCR_EL1.FDS being set automatically? I
think the wording is slightly confusing but I tested it on the model and
it works.
If PMSFCR_EL1.FDS == 0 then PMSDSFR_EL1 does nothing, and if the data
source filter isn't set by the user then FDS isn't set so there's no
need to set all the bits in the filter to 1. Once the user asks for any
filter then we set FDS, at which point it's whatever filter they asked
for. They can set all the bits if they want, or just one.
This is same way PMSFCR_EL1.FT already works. If the user asks for any
filter then it's set automatically, but we don't allow the user to ask
for "no filters" but with FT set.
So the only thing we can't do is filter out samples with _any_ data
source. Which would be PMSFCR_EL1.FDS == 1 and PMSDSFR_EL1 == 0. But I
don't think that's useful, and there are other filters to get you all or
most of the way there.
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>> drivers/perf/arm_spe_pmu.c | 31 +++++++++++++++++++++++++++++++
>> 1 file changed, 31 insertions(+)
>>
>> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
>> index 9309b846f642..d04318411f77 100644
>> --- a/drivers/perf/arm_spe_pmu.c
>> +++ b/drivers/perf/arm_spe_pmu.c
>> @@ -87,6 +87,7 @@ struct arm_spe_pmu {
>> #define SPE_PMU_FEAT_INV_FILT_EVT (1UL << 6)
>> #define SPE_PMU_FEAT_DISCARD (1UL << 7)
>> #define SPE_PMU_FEAT_EFT (1UL << 8)
>> +#define SPE_PMU_FEAT_FDS (1UL << 9)
>> #define SPE_PMU_FEAT_DEV_PROBED (1UL << 63)
>> u64 features;
>>
>> @@ -232,6 +233,10 @@ static const struct attribute_group arm_spe_pmu_cap_group = {
>> #define ATTR_CFG_FLD_inv_event_filter_LO 0
>> #define ATTR_CFG_FLD_inv_event_filter_HI 63
>>
>> +#define ATTR_CFG_FLD_data_src_filter_CFG config4 /* PMSDSFR_EL1 */
>> +#define ATTR_CFG_FLD_data_src_filter_LO 0
>> +#define ATTR_CFG_FLD_data_src_filter_HI 63
>> +
>> GEN_PMU_FORMAT_ATTR(ts_enable);
>> GEN_PMU_FORMAT_ATTR(pa_enable);
>> GEN_PMU_FORMAT_ATTR(pct_enable);
>> @@ -248,6 +253,7 @@ GEN_PMU_FORMAT_ATTR(float_filter);
>> GEN_PMU_FORMAT_ATTR(float_filter_mask);
>> GEN_PMU_FORMAT_ATTR(event_filter);
>> GEN_PMU_FORMAT_ATTR(inv_event_filter);
>> +GEN_PMU_FORMAT_ATTR(data_src_filter);
>> GEN_PMU_FORMAT_ATTR(min_latency);
>> GEN_PMU_FORMAT_ATTR(discard);
>>
>> @@ -268,6 +274,7 @@ static struct attribute *arm_spe_pmu_formats_attr[] = {
>> &format_attr_float_filter_mask.attr,
>> &format_attr_event_filter.attr,
>> &format_attr_inv_event_filter.attr,
>> + &format_attr_data_src_filter.attr,
>> &format_attr_min_latency.attr,
>> &format_attr_discard.attr,
>> NULL,
>> @@ -286,6 +293,9 @@ static umode_t arm_spe_pmu_format_attr_is_visible(struct kobject *kobj,
>> if (attr == &format_attr_inv_event_filter.attr && !(spe_pmu->features & SPE_PMU_FEAT_INV_FILT_EVT))
>> return 0;
>>
>> + if (attr == &format_attr_data_src_filter.attr && !(spe_pmu->features & SPE_PMU_FEAT_FDS))
>> + return 0;
>> +
>> if ((attr == &format_attr_branch_filter_mask.attr ||
>> attr == &format_attr_load_filter_mask.attr ||
>> attr == &format_attr_store_filter_mask.attr ||
>> @@ -406,6 +416,9 @@ static u64 arm_spe_event_to_pmsfcr(struct perf_event *event)
>> if (ATTR_CFG_GET_FLD(attr, inv_event_filter))
>> reg |= PMSFCR_EL1_FnE;
>>
>> + if (ATTR_CFG_GET_FLD(attr, data_src_filter))
>> + reg |= PMSFCR_EL1_FDS;
>> +
>> if (ATTR_CFG_GET_FLD(attr, min_latency))
>> reg |= PMSFCR_EL1_FL;
>>
>> @@ -430,6 +443,12 @@ static u64 arm_spe_event_to_pmslatfr(struct perf_event *event)
>> return FIELD_PREP(PMSLATFR_EL1_MINLAT, ATTR_CFG_GET_FLD(attr, min_latency));
>> }
>>
>> +static u64 arm_spe_event_to_pmsdsfr(struct perf_event *event)
>> +{
>> + struct perf_event_attr *attr = &event->attr;
>> + return ATTR_CFG_GET_FLD(attr, data_src_filter);
>> +}
>> +
>> static void arm_spe_pmu_pad_buf(struct perf_output_handle *handle, int len)
>> {
>> struct arm_spe_pmu_buf *buf = perf_get_aux(handle);
>> @@ -788,6 +807,10 @@ static int arm_spe_pmu_event_init(struct perf_event *event)
>> if (arm_spe_event_to_pmsnevfr(event) & arm_spe_pmsevfr_res0(spe_pmu->pmsver))
>> return -EOPNOTSUPP;
>>
>> + if (arm_spe_event_to_pmsdsfr(event) &&
>> + !(spe_pmu->features & SPE_PMU_FEAT_FDS))
>> + return -EOPNOTSUPP;
>> +
>> if (attr->exclude_idle)
>> return -EOPNOTSUPP;
>>
>> @@ -857,6 +880,11 @@ static void arm_spe_pmu_start(struct perf_event *event, int flags)
>> write_sysreg_s(reg, SYS_PMSNEVFR_EL1);
>> }
>>
>> + if (spe_pmu->features & SPE_PMU_FEAT_FDS) {
>> + reg = arm_spe_event_to_pmsdsfr(event);
>> + write_sysreg_s(reg, SYS_PMSDSFR_EL1);
>> + }
>> +
>> reg = arm_spe_event_to_pmslatfr(event);
>> write_sysreg_s(reg, SYS_PMSLATFR_EL1);
>>
>> @@ -1116,6 +1144,9 @@ static void __arm_spe_pmu_dev_probe(void *info)
>> if (FIELD_GET(PMSIDR_EL1_EFT, reg))
>> spe_pmu->features |= SPE_PMU_FEAT_EFT;
>>
>> + if (FIELD_GET(PMSIDR_EL1_FDS, reg))
>> + spe_pmu->features |= SPE_PMU_FEAT_FDS;
>> +
>> /* This field has a spaced out encoding, so just use a look-up */
>> fld = FIELD_GET(PMSIDR_EL1_INTERVAL, reg);
>> switch (fld) {
>>
>> --
>> 2.34.1
>>
On Tue, May 20, 2025 at 04:00:59PM +0100, James Clark wrote: > On 20/05/2025 2:46 pm, Leo Yan wrote: > > On Tue, May 06, 2025 at 12:41:39PM +0100, James Clark wrote: > > > SPE_FEAT_FDS adds the ability to filter on the data source of packets. > > > Like the other existing filters, enable filtering with PMSFCR_EL1.FDS > > > when any of the filter bits are set. > > > > > > Each bit maps to data sources 0-63 described by bits[0:5] in the data > > > source packet (although the full range of data source is 16 bits so > > > higher value data sources can't be filtered on). The filter is an OR of > > > all the bits, so for example setting bits 0 and 3 filters packets from > > > data sources 0 OR 3. > > > > As Arm ARM says: > > > > 0b0 : If PMSFCR_EL1.FDS is 1, do not record load operations that have > > bits [5:0] of the Data Source packet set to <m>. > > 0b1 : Load operations with Data Source <m> are unaffected by > > PMSFCR_EL1.FDS. > > > > We need extra handling for this configuration (0b0 means filtering, > > 0b1 means no affaction): > > > > - By default, the driver should set all bits in the 'data_src_filter' > > field. > > > > - The perf tool needs an extra patch in userspace to initialize all > > bits in config4 unless user specify other values. > > > > Did you take into account PMSFCR_EL1.FDS being set automatically? Good point. TBH, I did not give it enough consideration until your remdinding, but let me elaborate on why I suggested the approach above. > I think the wording is slightly confusing but I tested it on the model and it works. > > If PMSFCR_EL1.FDS == 0 then PMSDSFR_EL1 does nothing, and if the data source > filter isn't set by the user then FDS isn't set so there's no need to set > all the bits in the filter to 1. Once the user asks for any filter then we > set FDS, at which point it's whatever filter they asked for. They can set > all the bits if they want, or just one. > > This is same way PMSFCR_EL1.FT already works. If the user asks for any > filter then it's set automatically, but we don't allow the user to ask for > "no filters" but with FT set. > > So the only thing we can't do is filter out samples with _any_ data source. > Which would be PMSFCR_EL1.FDS == 1 and PMSDSFR_EL1 == 0. But I don't think > that's useful, and there are other filters to get you all or most of the way > there. My suggestion is coming for handling the case you mentioned. Let us see the combinations: PMSFCR_EL1.FDS == 0 PMSDSFR_EL1 == 0xFFFF,FFFF,FFFF,FFFF No filtering on data source PMSFCR_EL1.FDS == 1 PMSDSFR_EL1 == 0xFFFF,FFFF,FFFF,FFFF No filtering on data source PMSFCR_EL1.FDS == 0 PMSDSFR_EL1 == 0x0 No filtering on data source PMSFCR_EL1.FDS == 1 PMSDSFR_EL1 == 0x0 Filtering on all data source If 'PMSFCR_EL1.FDS == 0 and PMSDSFR_EL1 == 0xFFFF,FFFF,FFFF,FFFF' is initialized state, when a user set all bits to '1' for the data source filter, then no matter we enable or disable FDS bit, it can work as expected for disabling filtering. If 'PMSFCR_EL1.FDS == 0 and PMSDSFR_EL1 == 0x0' is the init state, as you said, when user passed 0xFFFF,FFFF,FFFF,FFFF for data filter, we cannot distinguish it from the init state, as a result, we will fail to handle this case. How about you think? Thanks, Leo
On Tue, May 20, 2025 at 05:10:03PM +0100, Leo Yan wrote: [...] > If 'PMSFCR_EL1.FDS == 0 and PMSDSFR_EL1 == 0x0' is the init state, as > you said, when user passed 0xFFFF,FFFF,FFFF,FFFF for data filter, we > cannot distinguish it from the init state, as a result, we will fail > to handle this case. Correct a typo. The case above, it means "when a user passes 0x0 for data source filter ....". Sorry for spamming. Leo
On 20/05/2025 5:22 pm, Leo Yan wrote: > On Tue, May 20, 2025 at 05:10:03PM +0100, Leo Yan wrote: > > [...] > >> If 'PMSFCR_EL1.FDS == 0 and PMSDSFR_EL1 == 0x0' is the init state, as >> you said, when user passed 0xFFFF,FFFF,FFFF,FFFF for data filter, we >> cannot distinguish it from the init state, as a result, we will fail >> to handle this case. > > Correct a typo. The case above, it means "when a user passes 0x0 for > data source filter ....". > > Sorry for spamming. > > Leo I'm thinking I'd rather leave it consistent with PMSFCR_EL1.FT and automatically enable PMSFCR_EL1.FDS for any non zero data-source filter. This means we don't need a tool change to set some other flag when a filter is provided (even if it's zero) and it's much simpler. It also doesn't prevent the possibility of adding the enable flag in the future if someone comes out with a need for it, but I don't think it needs to be done now. TBH I can't imagine a case where someone would want to filter out any samples that have any data source. Surely you'd only be looking for a selected set of data sources, or no filtering at all.
On Wed, May 21, 2025 at 09:54:48AM +0100, James Clark wrote: > On 20/05/2025 5:22 pm, Leo Yan wrote: [...] > I'm thinking I'd rather leave it consistent with PMSFCR_EL1.FT and > automatically enable PMSFCR_EL1.FDS for any non zero data-source filter. This is fine for me. Just a minor thing, for the case PMSDSFR_EL1 = 0xFFFF,FFFF,FFFF,FFFF, we might consider to clear the PMSFCR_EL1.FDS bit. This would be a bit performance benefit for disabling data source filter rather than enabling the filter with unaffecting all data sources. > This means we don't need a tool change to set some other flag when a filter > is provided (even if it's zero) and it's much simpler. It also doesn't > prevent the possibility of adding the enable flag in the future if someone > comes out with a need for it, but I don't think it needs to be done now. The question comes down to the complexity in user-space tools. Perf initializes the attribute configs to zeros. If we want to set all bits in config4 as a default value, we would need additional change in the perf tool. Also initializing config4 to all ones is likely to cause confusion if other tools want to enable the feature. I agree that a cleaner way would be to use an enable flag + mask, we can defer to add flag if needed. > TBH I can't imagine a case where someone would want to filter out any samples > that have any data source. Surely you'd only be looking for a selected set > of data sources, or no filtering at all. Agreed this is a rare case. Thanks, Leo
On Tue, May 06, 2025 at 12:41:39PM +0100, James Clark wrote:
> SPE_FEAT_FDS adds the ability to filter on the data source of packets.
> Like the other existing filters, enable filtering with PMSFCR_EL1.FDS
> when any of the filter bits are set.
>
> Each bit maps to data sources 0-63 described by bits[0:5] in the data
> source packet (although the full range of data source is 16 bits so
> higher value data sources can't be filtered on). The filter is an OR of
> all the bits, so for example setting bits 0 and 3 filters packets from
> data sources 0 OR 3.
>
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
> drivers/perf/arm_spe_pmu.c | 31 +++++++++++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
>
> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> index 9309b846f642..d04318411f77 100644
> --- a/drivers/perf/arm_spe_pmu.c
> +++ b/drivers/perf/arm_spe_pmu.c
> @@ -87,6 +87,7 @@ struct arm_spe_pmu {
> #define SPE_PMU_FEAT_INV_FILT_EVT (1UL << 6)
> #define SPE_PMU_FEAT_DISCARD (1UL << 7)
> #define SPE_PMU_FEAT_EFT (1UL << 8)
> +#define SPE_PMU_FEAT_FDS (1UL << 9)
> #define SPE_PMU_FEAT_DEV_PROBED (1UL << 63)
> u64 features;
>
> @@ -232,6 +233,10 @@ static const struct attribute_group arm_spe_pmu_cap_group = {
> #define ATTR_CFG_FLD_inv_event_filter_LO 0
> #define ATTR_CFG_FLD_inv_event_filter_HI 63
>
> +#define ATTR_CFG_FLD_data_src_filter_CFG config4 /* PMSDSFR_EL1 */
> +#define ATTR_CFG_FLD_data_src_filter_LO 0
> +#define ATTR_CFG_FLD_data_src_filter_HI 63
> +
> GEN_PMU_FORMAT_ATTR(ts_enable);
> GEN_PMU_FORMAT_ATTR(pa_enable);
> GEN_PMU_FORMAT_ATTR(pct_enable);
> @@ -248,6 +253,7 @@ GEN_PMU_FORMAT_ATTR(float_filter);
> GEN_PMU_FORMAT_ATTR(float_filter_mask);
> GEN_PMU_FORMAT_ATTR(event_filter);
> GEN_PMU_FORMAT_ATTR(inv_event_filter);
> +GEN_PMU_FORMAT_ATTR(data_src_filter);
> GEN_PMU_FORMAT_ATTR(min_latency);
> GEN_PMU_FORMAT_ATTR(discard);
>
> @@ -268,6 +274,7 @@ static struct attribute *arm_spe_pmu_formats_attr[] = {
> &format_attr_float_filter_mask.attr,
> &format_attr_event_filter.attr,
> &format_attr_inv_event_filter.attr,
> + &format_attr_data_src_filter.attr,
> &format_attr_min_latency.attr,
> &format_attr_discard.attr,
> NULL,
> @@ -286,6 +293,9 @@ static umode_t arm_spe_pmu_format_attr_is_visible(struct kobject *kobj,
> if (attr == &format_attr_inv_event_filter.attr && !(spe_pmu->features & SPE_PMU_FEAT_INV_FILT_EVT))
> return 0;
>
> + if (attr == &format_attr_data_src_filter.attr && !(spe_pmu->features & SPE_PMU_FEAT_FDS))
> + return 0;
> +
> if ((attr == &format_attr_branch_filter_mask.attr ||
> attr == &format_attr_load_filter_mask.attr ||
> attr == &format_attr_store_filter_mask.attr ||
> @@ -406,6 +416,9 @@ static u64 arm_spe_event_to_pmsfcr(struct perf_event *event)
> if (ATTR_CFG_GET_FLD(attr, inv_event_filter))
> reg |= PMSFCR_EL1_FnE;
>
> + if (ATTR_CFG_GET_FLD(attr, data_src_filter))
> + reg |= PMSFCR_EL1_FDS;
> +
> if (ATTR_CFG_GET_FLD(attr, min_latency))
> reg |= PMSFCR_EL1_FL;
>
> @@ -430,6 +443,12 @@ static u64 arm_spe_event_to_pmslatfr(struct perf_event *event)
> return FIELD_PREP(PMSLATFR_EL1_MINLAT, ATTR_CFG_GET_FLD(attr, min_latency));
> }
>
> +static u64 arm_spe_event_to_pmsdsfr(struct perf_event *event)
> +{
> + struct perf_event_attr *attr = &event->attr;
> + return ATTR_CFG_GET_FLD(attr, data_src_filter);
> +}
Seems to me, arm_spe_event_to_pmsdsfr() is not needed as it does not do
any conversion from event config to register value. So simply read the
field value in opened code would be fine.
I am fine to keep it and would leave SPE driver maintainers to decide
which is preferring. Otherwise, LGTM:
Reviewed-by: Leo Yan <leo.yan@arm.com>
> +
> static void arm_spe_pmu_pad_buf(struct perf_output_handle *handle, int len)
> {
> struct arm_spe_pmu_buf *buf = perf_get_aux(handle);
> @@ -788,6 +807,10 @@ static int arm_spe_pmu_event_init(struct perf_event *event)
> if (arm_spe_event_to_pmsnevfr(event) & arm_spe_pmsevfr_res0(spe_pmu->pmsver))
> return -EOPNOTSUPP;
>
> + if (arm_spe_event_to_pmsdsfr(event) &&
> + !(spe_pmu->features & SPE_PMU_FEAT_FDS))
> + return -EOPNOTSUPP;
> +
> if (attr->exclude_idle)
> return -EOPNOTSUPP;
>
> @@ -857,6 +880,11 @@ static void arm_spe_pmu_start(struct perf_event *event, int flags)
> write_sysreg_s(reg, SYS_PMSNEVFR_EL1);
> }
>
> + if (spe_pmu->features & SPE_PMU_FEAT_FDS) {
> + reg = arm_spe_event_to_pmsdsfr(event);
> + write_sysreg_s(reg, SYS_PMSDSFR_EL1);
> + }
> +
> reg = arm_spe_event_to_pmslatfr(event);
> write_sysreg_s(reg, SYS_PMSLATFR_EL1);
>
> @@ -1116,6 +1144,9 @@ static void __arm_spe_pmu_dev_probe(void *info)
> if (FIELD_GET(PMSIDR_EL1_EFT, reg))
> spe_pmu->features |= SPE_PMU_FEAT_EFT;
>
> + if (FIELD_GET(PMSIDR_EL1_FDS, reg))
> + spe_pmu->features |= SPE_PMU_FEAT_FDS;
> +
> /* This field has a spaced out encoding, so just use a look-up */
> fld = FIELD_GET(PMSIDR_EL1_INTERVAL, reg);
> switch (fld) {
>
> --
> 2.34.1
>
On 20/05/2025 12:43 pm, Leo Yan wrote:
> On Tue, May 06, 2025 at 12:41:39PM +0100, James Clark wrote:
>> SPE_FEAT_FDS adds the ability to filter on the data source of packets.
>> Like the other existing filters, enable filtering with PMSFCR_EL1.FDS
>> when any of the filter bits are set.
>>
>> Each bit maps to data sources 0-63 described by bits[0:5] in the data
>> source packet (although the full range of data source is 16 bits so
>> higher value data sources can't be filtered on). The filter is an OR of
>> all the bits, so for example setting bits 0 and 3 filters packets from
>> data sources 0 OR 3.
>>
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>> drivers/perf/arm_spe_pmu.c | 31 +++++++++++++++++++++++++++++++
>> 1 file changed, 31 insertions(+)
>>
>> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
>> index 9309b846f642..d04318411f77 100644
>> --- a/drivers/perf/arm_spe_pmu.c
>> +++ b/drivers/perf/arm_spe_pmu.c
>> @@ -87,6 +87,7 @@ struct arm_spe_pmu {
>> #define SPE_PMU_FEAT_INV_FILT_EVT (1UL << 6)
>> #define SPE_PMU_FEAT_DISCARD (1UL << 7)
>> #define SPE_PMU_FEAT_EFT (1UL << 8)
>> +#define SPE_PMU_FEAT_FDS (1UL << 9)
>> #define SPE_PMU_FEAT_DEV_PROBED (1UL << 63)
>> u64 features;
>>
>> @@ -232,6 +233,10 @@ static const struct attribute_group arm_spe_pmu_cap_group = {
>> #define ATTR_CFG_FLD_inv_event_filter_LO 0
>> #define ATTR_CFG_FLD_inv_event_filter_HI 63
>>
>> +#define ATTR_CFG_FLD_data_src_filter_CFG config4 /* PMSDSFR_EL1 */
>> +#define ATTR_CFG_FLD_data_src_filter_LO 0
>> +#define ATTR_CFG_FLD_data_src_filter_HI 63
>> +
>> GEN_PMU_FORMAT_ATTR(ts_enable);
>> GEN_PMU_FORMAT_ATTR(pa_enable);
>> GEN_PMU_FORMAT_ATTR(pct_enable);
>> @@ -248,6 +253,7 @@ GEN_PMU_FORMAT_ATTR(float_filter);
>> GEN_PMU_FORMAT_ATTR(float_filter_mask);
>> GEN_PMU_FORMAT_ATTR(event_filter);
>> GEN_PMU_FORMAT_ATTR(inv_event_filter);
>> +GEN_PMU_FORMAT_ATTR(data_src_filter);
>> GEN_PMU_FORMAT_ATTR(min_latency);
>> GEN_PMU_FORMAT_ATTR(discard);
>>
>> @@ -268,6 +274,7 @@ static struct attribute *arm_spe_pmu_formats_attr[] = {
>> &format_attr_float_filter_mask.attr,
>> &format_attr_event_filter.attr,
>> &format_attr_inv_event_filter.attr,
>> + &format_attr_data_src_filter.attr,
>> &format_attr_min_latency.attr,
>> &format_attr_discard.attr,
>> NULL,
>> @@ -286,6 +293,9 @@ static umode_t arm_spe_pmu_format_attr_is_visible(struct kobject *kobj,
>> if (attr == &format_attr_inv_event_filter.attr && !(spe_pmu->features & SPE_PMU_FEAT_INV_FILT_EVT))
>> return 0;
>>
>> + if (attr == &format_attr_data_src_filter.attr && !(spe_pmu->features & SPE_PMU_FEAT_FDS))
>> + return 0;
>> +
>> if ((attr == &format_attr_branch_filter_mask.attr ||
>> attr == &format_attr_load_filter_mask.attr ||
>> attr == &format_attr_store_filter_mask.attr ||
>> @@ -406,6 +416,9 @@ static u64 arm_spe_event_to_pmsfcr(struct perf_event *event)
>> if (ATTR_CFG_GET_FLD(attr, inv_event_filter))
>> reg |= PMSFCR_EL1_FnE;
>>
>> + if (ATTR_CFG_GET_FLD(attr, data_src_filter))
>> + reg |= PMSFCR_EL1_FDS;
>> +
>> if (ATTR_CFG_GET_FLD(attr, min_latency))
>> reg |= PMSFCR_EL1_FL;
>>
>> @@ -430,6 +443,12 @@ static u64 arm_spe_event_to_pmslatfr(struct perf_event *event)
>> return FIELD_PREP(PMSLATFR_EL1_MINLAT, ATTR_CFG_GET_FLD(attr, min_latency));
>> }
>>
>> +static u64 arm_spe_event_to_pmsdsfr(struct perf_event *event)
>> +{
>> + struct perf_event_attr *attr = &event->attr;
>> + return ATTR_CFG_GET_FLD(attr, data_src_filter);
>> +}
>
> Seems to me, arm_spe_event_to_pmsdsfr() is not needed as it does not do
> any conversion from event config to register value. So simply read the
> field value in opened code would be fine.
>
> I am fine to keep it and would leave SPE driver maintainers to decide
> which is preferring. Otherwise, LGTM:
>
> Reviewed-by: Leo Yan <leo.yan@arm.com>
>
It's purely for consistency with the existing code. See
arm_spe_event_to_pmsevfr() etc.
>> +
>> static void arm_spe_pmu_pad_buf(struct perf_output_handle *handle, int len)
>> {
>> struct arm_spe_pmu_buf *buf = perf_get_aux(handle);
>> @@ -788,6 +807,10 @@ static int arm_spe_pmu_event_init(struct perf_event *event)
>> if (arm_spe_event_to_pmsnevfr(event) & arm_spe_pmsevfr_res0(spe_pmu->pmsver))
>> return -EOPNOTSUPP;
>>
>> + if (arm_spe_event_to_pmsdsfr(event) &&
>> + !(spe_pmu->features & SPE_PMU_FEAT_FDS))
>> + return -EOPNOTSUPP;
>> +
>> if (attr->exclude_idle)
>> return -EOPNOTSUPP;
>>
>> @@ -857,6 +880,11 @@ static void arm_spe_pmu_start(struct perf_event *event, int flags)
>> write_sysreg_s(reg, SYS_PMSNEVFR_EL1);
>> }
>>
>> + if (spe_pmu->features & SPE_PMU_FEAT_FDS) {
>> + reg = arm_spe_event_to_pmsdsfr(event);
>> + write_sysreg_s(reg, SYS_PMSDSFR_EL1);
>> + }
>> +
>> reg = arm_spe_event_to_pmslatfr(event);
>> write_sysreg_s(reg, SYS_PMSLATFR_EL1);
>>
>> @@ -1116,6 +1144,9 @@ static void __arm_spe_pmu_dev_probe(void *info)
>> if (FIELD_GET(PMSIDR_EL1_EFT, reg))
>> spe_pmu->features |= SPE_PMU_FEAT_EFT;
>>
>> + if (FIELD_GET(PMSIDR_EL1_FDS, reg))
>> + spe_pmu->features |= SPE_PMU_FEAT_FDS;
>> +
>> /* This field has a spaced out encoding, so just use a look-up */
>> fld = FIELD_GET(PMSIDR_EL1_INTERVAL, reg);
>> switch (fld) {
>>
>> --
>> 2.34.1
>>
>
© 2016 - 2025 Red Hat, Inc.