linux-kernel - Re: [PATCH v2 5/5] perf: arm_cspmu: ampere

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <93c3f3a6-a3bc-f3ca-9077-8985865b8abf@arm.com>
Date:   Fri, 2 Jun 2023 12:51:34 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Ilkka Koskinen <ilkka@...amperecomputing.com>
Cc:     Jonathan Corbet <corbet@....net>, Will Deacon <will@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Besar Wicaksono <bwicaksono@...dia.com>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 5/5] perf: arm_cspmu: ampere_cspmu: Add support for
 Ampere SoC PMU

On 2023-06-02 08:13, Ilkka Koskinen wrote:
> 
> Hi Robin,
> 
> On Thu, 1 Jun 2023, Robin Murphy wrote:
>> On 2023-06-01 04:01, Ilkka Koskinen wrote:
>> [...]
>>> +static bool ampere_cspmu_validate_event(struct arm_cspmu *cspmu,
>>> +                    struct perf_event *new)
>>> +{
>>> +    struct perf_event *curr;
>>> +    unsigned int idx;
>>> +    u32 threshold = 0, rank = 0, bank = 0;
>>> +
>>> +    /* We compare the global filter settings to existing events */
>>> +    idx = find_first_bit(cspmu->hw_events.used_ctrs,
>>> +                 cspmu->cycle_counter_logical_idx);
>>> +
>>> +    /* This is the first event */
>>> +    if (idx == cspmu->cycle_counter_logical_idx)
>>> +        return true;
>>> +
>>> +    curr = cspmu->hw_events.events[idx];
>>> +
>>> +    if (get_filter_enable(new)) {
>>> +        threshold    = get_threshold(new);
>>> +        rank        = get_rank(new);
>>> +        bank        = get_bank(new);
>>> +    }
>>> +
>>> +    if (get_filter_enable(new) != get_filter_enable(curr) ||
>>
>> Is there any useful purpose in allowing the user to specify nonzero 
>> rank, bank or threshold values with filter_enable=0? Assuming not, 
>> then between this and ampere_cspmu_set_ev_filter() it appears that you 
>> don't need filter_enable at all.
> 
> Not really. I dropped filter_enable at one point but restored it later 
> to match the smmuv3 pmu driver. I totally agree, it's unnecessary and 
> it's better to remove it completely.

Ah, I see - the SMMU PMCG driver needs that to differentiate between 
"filter values defaulted to 0 because user didn't ask for filtering" and 
"user asked to filter an exact match on StreamID 0", since it's 
impractical to expect userspace tools to understand and manually set the 
all-ones mask value to indicate that filtering wasn't requested. In your 
case, though, since values of 0 appear to mean "no filter", it should 
just work as expected without needing any additional complexity. Ideally 
your interface should reflect the functionality and expected usage model 
of your PMU hardware in the way that's most intuitive and helpful for 
the user - it doesn't need to be influenced by other PMUs that work 
differently.

Thanks,
Robin.