Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Message-ID: <f24a78d3-0cf2-5be4-c4bd-2552e6b69ef1@arm.com>
Date:   Mon, 9 Oct 2023 17:30:11 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH v1 1/2] arm64: perf: Add support for event counting
 threshold
Content-Language: en-US
To:     Suzuki K Poulose <suzuki.poulose@arm.com>,
        linux-arm-kernel@lists.infradead.org,
        linux-perf-users@vger.kernel.org
Cc:     Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Jonathan Corbet <corbet@lwn.net>,
        Mark Rutland <mark.rutland@arm.com>,
        Marc Zyngier <maz@kernel.org>,
        Zaid Al-Bassam <zalbassam@google.com>,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230919095137.360963-1-james.clark@arm.com>
 <20230919095137.360963-2-james.clark@arm.com>
 <b72d321f-0cea-1049-3bbb-224d38a94749@arm.com>
From:   James Clark <james.clark@arm.com>
In-Reply-To: <b72d321f-0cea-1049-3bbb-224d38a94749@arm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 09/10/2023 13:50, Suzuki K Poulose wrote:
> Hi James
> 
> On 19/09/2023 10:51, James Clark wrote:
>> FEAT_PMUv3_TH (Armv8.8) permits a PMU counter to increment only on
>> events whose count meets a specified threshold condition. For example if
>> PMEVTYPERn.TC (Threshold Control) is set to 0b101 (Greater than or
>> equal, count), and the threshold is set to 2, then the PMU counter will
>> now only increment by 1 when an event would have previously incremented
>> the PMU counter by 2 or more on a single processor cycle.
>>
>> Two new Perf event config fields, 'threshold' and 'threshold_control'
>> have been added for controlling the feature:
>>
>>    $ perf stat -e stall_slot/threshold=2,threshold_control=5/
>>
>> A new capability for reading out the maximum supported threshold value
>> has also been added:
>>
>>    $ cat /sys/bus/event_source/devices/armv8_pmuv3/caps/threshold_max
>>
>>    0x000000ff
>>
>> If a threshold higher than threshold_max is provided, then no error is
>> generated but the threshold is clamped to the max value. If
>> FEAT_PMUv3_TH isn't implemented, then threshold_max reads zero, and
>> neither the 'threshold' nor 'threshold_control' parameters will be used.
>>
>> The threshold is per PMU counter, and there are potentially different
>> threshold_max values per PMU type on heterogeneous systems.
>>
>> Bits higher than 32 now need to be written into PMEVTYPER, so
>> armv8pmu_write_evtype() has to be updated to take a u64 value rather
>> than u32.
> 
> Is this supported in the Aarch32 state ? If so, do we need to change
> the arch specific register read/write "helpers" ?
> 

It's half supported. PMMIR.THWIDTH is readable and non-zero in aarch32,
but the threshold value can't be written because it's in the high part
of the event type register. Thresholding does affect aarch32 guests when
enabled from the host, but that doesn't really concern the code.

But yes you're right, it isn't building on aarch32 at the moment so I
need to do a V2. I'm going to make it so the max width is reported as 0
in sysfs for aarch32, and then don't ever try to write to the high part
of the event type (which was part of the compilation error anyway). No
change will be needed to the read/write helpers though, they already
handle 32/64 bits for the right platforms.

>>
>> Signed-off-by: James Clark <james.clark@arm.com>
>> ---
>>   drivers/perf/arm_pmuv3.c       | 59 +++++++++++++++++++++++++++++++++-
>>   include/linux/perf/arm_pmuv3.h |  7 +++-
>>   2 files changed, 64 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
>> index e5a2ac4155f6..ad6574b2bdab 100644
>> --- a/drivers/perf/arm_pmuv3.c
>> +++ b/drivers/perf/arm_pmuv3.c
>> @@ -294,9 +294,16 @@ static const struct attribute_group
>> armv8_pmuv3_events_attr_group = {
>>       .is_visible = armv8pmu_event_attr_is_visible,
>>   };
>>   +#define TH_LO    2
>> +#define TH_HI    13
>> +#define TH_CNTL_LO    14
>> +#define TH_CNTL_HI    16
>> +
>>   PMU_FORMAT_ATTR(event, "config:0-15");
>>   PMU_FORMAT_ATTR(long, "config1:0");
>>   PMU_FORMAT_ATTR(rdpmc, "config1:1");
>> +PMU_FORMAT_ATTR(threshold, "config1:" __stringify(TH_LO) "-"
>> __stringify(TH_HI));
>> +PMU_FORMAT_ATTR(threshold_control, "config1:" __stringify(TH_CNTL_LO)
>> "-" __stringify(TH_CNTL_HI));
> 
> The perf core doesn't yet support adding aliases for
> fields other than "events". May be we could add
> some support for that in the future and use it
> to provide meaningful aliases for the threshold
> control.
> 

I think it could be useful, but I'm wondering about the use case. To
make use of this feature you would probably need to have the docs or
reference manual open anyway, and that gives you the names of the
control values. Any tool could trivially hold the list of names too.

But I suppose you could say the same about event names, and we go
through some effort to report those.

> Not sure if we can extrapolate threshold_control[0]
> to be another config field => "counting_mode"
> We would need to check with the architecture folks
> to confirm that the meaning wouldn't change though.
> 

I think if was planned to be fixed in that way it would have already
been written that way? And I doubt they would agree to promise to not
re-use another value that didn't fit the pattern in the future. Then
we'd be in a big mess because we deviated from the reference manual and
it would be hard to re-map around whatever new value was introduced.

>>     static int sysctl_perf_user_access __read_mostly;
>>   @@ -310,10 +317,22 @@ static inline bool
>> armv8pmu_event_want_user_access(struct perf_event *event)
>>       return event->attr.config1 & 0x2;
>>   }
>>   +static inline u32 armv8pmu_event_threshold(struct perf_event_attr
>> *attr)
>> +{
>> +    return FIELD_GET(GENMASK(TH_HI, TH_LO), attr->config1);
>> +}
>> +
>> +static inline u8 armv8pmu_event_threshold_control(struct
>> perf_event_attr *attr)
>> +{
>> +    return FIELD_GET(GENMASK(TH_CNTL_HI, TH_CNTL_LO), attr->config1);
>> +}
>> +
>>   static struct attribute *armv8_pmuv3_format_attrs[] = {
>>       &format_attr_event.attr,
>>       &format_attr_long.attr,
>>       &format_attr_rdpmc.attr,
>> +    &format_attr_threshold.attr,
>> +    &format_attr_threshold_control.attr,
>>       NULL,
>>   };
>>   @@ -365,10 +384,31 @@ static ssize_t bus_width_show(struct device
>> *dev, struct device_attribute *attr,
>>     static DEVICE_ATTR_RO(bus_width);
>>   +static u32 threshold_max(struct arm_pmu *cpu_pmu)
>> +{
>> +    /*
>> +     * The largest value that can be written to PMEVTYPER<n>_EL0.TH is
>> +     * (2 ^ PMMIR.THWIDTH) - 1
>> +     */
>> +    return (1 << FIELD_GET(ARMV8_PMU_THWIDTH, cpu_pmu->reg_pmmir)) - 1;
>> +}
>> +
>> +static ssize_t threshold_max_show(struct device *dev,
>> +                  struct device_attribute *attr, char *page)
>> +{
>> +    struct pmu *pmu = dev_get_drvdata(dev);
>> +    struct arm_pmu *cpu_pmu = container_of(pmu, struct arm_pmu, pmu);
>> +
>> +    return sysfs_emit(page, "0x%08x\n", threshold_max(cpu_pmu));
>> +}
>> +
>> +static DEVICE_ATTR_RO(threshold_max);
>> +
>>   static struct attribute *armv8_pmuv3_caps_attrs[] = {
>>       &dev_attr_slots.attr,
>>       &dev_attr_bus_slots.attr,
>>       &dev_attr_bus_width.attr,
>> +    &dev_attr_threshold_max.attr,
>>       NULL,
>>   };
>>   @@ -552,7 +592,7 @@ static void armv8pmu_write_counter(struct
>> perf_event *event, u64 value)
>>           armv8pmu_write_hw_counter(event, value);
>>   }
>>   -static inline void armv8pmu_write_evtype(int idx, u32 val)
>> +static inline void armv8pmu_write_evtype(int idx, u64 val)>>   {
>>       u32 counter = ARMV8_IDX_TO_COUNTER(idx);
>>   @@ -912,6 +952,10 @@ static int armv8pmu_set_event_filter(struct
>> hw_perf_event *event,
>>                        struct perf_event_attr *attr)
>>   {
>>       unsigned long config_base = 0;
>> +    struct perf_event *perf_event = container_of(attr, struct
>> perf_event,
>> +                             attr);
>> +    struct arm_pmu *cpu_pmu = to_arm_pmu(perf_event->pmu);
>> +    u32 th, th_max;
>>         if (attr->exclude_idle)
>>           return -EPERM;
>> @@ -943,6 +987,19 @@ static int armv8pmu_set_event_filter(struct
>> hw_perf_event *event,
>>       if (attr->exclude_user)
>>           config_base |= ARMV8_PMU_EXCLUDE_EL0;
>>   +    /*
>> +     * Insert event counting threshold (FEAT_PMUv3_TH) values. If
>> +     * FEAT_PMUv3_TH isn't implemented, then THWIDTH (threshold_max)
>> will be
>> +     * 0 and no values will be written.
>> +     */
>> +    th_max = threshold_max(cpu_pmu);
>> +    if (th_max) {
>> +        th = min(armv8pmu_event_threshold(attr), th_max);
>> +        config_base |= FIELD_PREP(ARMV8_PMU_EVTYPE_TH, th);
>> +        config_base |= FIELD_PREP(ARMV8_PMU_EVTYPE_TC,
>> +                      armv8pmu_event_threshold_control(attr));
>> +    }
>> +
>>       /*
>>        * Install the filter into config_base as this is used to
>>        * construct the event type.
>> diff --git a/include/linux/perf/arm_pmuv3.h
>> b/include/linux/perf/arm_pmuv3.h
>> index e3899bd77f5c..0e2a3c927150 100644
>> --- a/include/linux/perf/arm_pmuv3.h
>> +++ b/include/linux/perf/arm_pmuv3.h
>> @@ -228,7 +228,11 @@
>>   /*
>>    * PMXEVTYPER: Event selection reg
>>    */
>> -#define ARMV8_PMU_EVTYPE_MASK    0xc800ffff    /* Mask for writable
>> bits */
>> +#define ARMV8_PMU_EVTYPE_TH    GENMASK(43, 32)
>> +#define ARMV8_PMU_EVTYPE_TC    GENMASK(63, 61)
>> +/* Mask for writable bits */
>> +#define ARMV8_PMU_EVTYPE_MASK    (0xc800ffff | ARMV8_PMU_EVTYPE_TH | \
>> +                ARMV8_PMU_EVTYPE_TC)
> 
> May need to be UL suffixed for safety ?
> 

I don't think it's strictly needed because GENMASK makes a UL so the
whole expression is auto promoted anyway. But I can add it for
completeness.

Also these will be split across the two asm/arm_pmuv3.h files in v2
because I need to keep the original mask for aarch32, so it will look a
little bit different.

Thanks
James

> 
> Suzuki
> 
>>   #define ARMV8_PMU_EVTYPE_EVENT    0xffff        /* Mask for EVENT
>> bits */
>>     /*
>> @@ -254,6 +258,7 @@
>>   #define ARMV8_PMU_BUS_SLOTS_MASK 0xff
>>   #define ARMV8_PMU_BUS_WIDTH_SHIFT 16
>>   #define ARMV8_PMU_BUS_WIDTH_MASK 0xf
>> +#define ARMV8_PMU_THWIDTH GENMASK(23, 20)
>>     /*
>>    * This code is really good
> 
>