| 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
| ||
|
Date: Fri, 18 Mar 2022 13:48:58 +0530
From: Sandipan Das <sandipan.das@....com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
x86@...nel.org, bp@...en8.de, dave.hansen@...ux.intel.com,
acme@...nel.org, mark.rutland@....com,
alexander.shishkin@...ux.intel.com, namhyung@...nel.org,
jolsa@...nel.org, tglx@...utronix.de, mingo@...hat.com,
pbonzini@...hat.com, jmattson@...gle.com, like.xu.linux@...il.com,
eranian@...gle.com, ananth.narayan@....com, ravi.bangoria@....com,
santosh.shukla@....com
Subject: Re: [PATCH 6/7] perf/x86/amd/core: Add PerfMonV2 overflow handling
On 3/17/2022 5:31 PM, Peter Zijlstra wrote:
> On Thu, Mar 17, 2022 at 11:58:35AM +0530, Sandipan Das wrote:
>
>> +static inline u64 amd_pmu_get_global_overflow(void)
>> +{
>> + u64 status;
>> +
>> + /* PerfCntrGlobalStatus is read-only */
>> + rdmsrl(MSR_AMD64_PERF_CNTR_GLOBAL_STATUS, status);
>> +
>> + return status & amd_pmu_global_cntr_mask;
>> +}
>> +
>> +static inline void amd_pmu_ack_global_overflow(u64 status)
>> +{
>> + /*
>> + * PerfCntrGlobalStatus is read-only but an overflow acknowledgment
>> + * mechanism exists; writing 1 to a bit in PerfCntrGlobalStatusClr
>> + * clears the same bit in PerfCntrGlobalStatus
>> + */
>> +
>> + /* Only allow modifications to PerfCntrGlobalStatus.PerfCntrOvfl */
>> + status &= amd_pmu_global_cntr_mask;
>> + wrmsrl(MSR_AMD64_PERF_CNTR_GLOBAL_STATUS_CLR, status);
>> +}
>> +
>> +static bool amd_pmu_legacy_has_overflow(int idx)
>> +{
>> + u64 counter;
>> +
>> + rdmsrl(x86_pmu_event_addr(idx), counter);
>> +
>> + return !(counter & BIT_ULL(x86_pmu.cntval_bits - 1));
>> +}
>> +
>> +static bool amd_pmu_global_has_overflow(int idx)
>> +{
>> + return amd_pmu_get_global_overflow() & BIT_ULL(idx);
>> +}
>> +
>> +DEFINE_STATIC_CALL(amd_pmu_has_overflow, amd_pmu_legacy_has_overflow);
>> +
>> /*
>> * When a PMC counter overflows, an NMI is used to process the event and
>> * reset the counter. NMI latency can result in the counter being updated
>> @@ -613,7 +653,6 @@ static inline void amd_pmu_set_global_ctl(u64 ctl)
>> static void amd_pmu_wait_on_overflow(int idx)
>> {
>> unsigned int i;
>> - u64 counter;
>>
>> /*
>> * Wait for the counter to be reset if it has overflowed. This loop
>> @@ -621,8 +660,7 @@ static void amd_pmu_wait_on_overflow(int idx)
>> * forever...
>> */
>> for (i = 0; i < OVERFLOW_WAIT_COUNT; i++) {
>> - rdmsrl(x86_pmu_event_addr(idx), counter);
>> - if (counter & (1ULL << (x86_pmu.cntval_bits - 1)))
>> + if (!static_call(amd_pmu_has_overflow)(idx))
>> break;
>>
>> /* Might be in IRQ context, so can't sleep */
>
> This scares me... please tell me you fixed that mess.
>
>> @@ -718,6 +756,83 @@ static void amd_pmu_enable_event(struct perf_event *event)
>> static_call(amd_pmu_enable_event)(event);
>> }
>>
>> +static int amd_pmu_global_handle_irq(struct pt_regs *regs)
>> +{
>> + struct perf_sample_data data;
>> + struct cpu_hw_events *cpuc;
>> + struct hw_perf_event *hwc;
>> + struct perf_event *event;
>> + u64 val, status, mask;
>> + int handled = 0, idx;
>> +
>> + status = amd_pmu_get_global_overflow();
>> +
>> + /* Check if any overflows are pending */
>> + if (!status)
>> + return 0;
>> +
>> + /* Stop counting */
>> + amd_pmu_global_disable_all();
>
>
> This seems weird to me, I'd first disable it, then read status. MSR
> access is expensive, you want to shut down the PMU asap.
>
> Also, this is written like PMI would not be the primary NMI source,
> which seems somewhat unlikely.
>
Yes, PMI is the primary NMI source. Will fix this.
>> +
>> + cpuc = this_cpu_ptr(&cpu_hw_events);
>> +
>> + /*
>> + * Some chipsets need to unmask the LVTPC in a particular spot
>> + * inside the nmi handler. As a result, the unmasking was
>> + * pushed into all the nmi handlers.
>> + *
>> + * This generic handler doesn't seem to have any issues where
>> + * the unmasking occurs so it was left at the top.
>> + *
>> + * N.B. Taken from x86_pmu_handle_irq()
>> + */
>
> Please write an AMD specific comment here. Note how 'recent' Intel chips
> ended up pushing this to the end of the handler. Verify with your
> hardware team where they want this and write as much of the rationale as
> you're allowed to share in the comment.
>
Sure. I'll follow-up on this.
>> + apic_write(APIC_LVTPC, APIC_DM_NMI);
>> +
>> + for (idx = 0; idx < x86_pmu.num_counters; idx++) {
>> + if (!test_bit(idx, cpuc->active_mask))
>> + continue;
>> +
>> + event = cpuc->events[idx];
>> + hwc = &event->hw;
>> + val = x86_perf_event_update(event);
>> + mask = BIT_ULL(idx);
>> +
>> + if (!(status & mask))
>> + continue;
>> +
>> + /* Event overflow */
>> + handled++;
>> + perf_sample_data_init(&data, 0, hwc->last_period);
>> +
>> + if (!x86_perf_event_set_period(event))
>> + continue;
>> +
>> + if (perf_event_overflow(event, &data, regs))
>> + x86_pmu_stop(event, 0);
>> +
>> + status &= ~mask;
>> + }
>> +
>> + /*
>> + * It should never be the case that some overflows are not handled as
>> + * the corresponding PMCs are expected to be inactive according to the
>> + * active_mask
>> + */
>> + WARN_ON(status > 0);
>> +
>> + /* Clear overflow bits */
>> + amd_pmu_ack_global_overflow(~status);
>> +
>> + inc_irq_stat(apic_perf_irqs);
>> +
>> + /* Resume counting */
>> + amd_pmu_global_enable_all(0);
>
> I think this is broken vs perf_pmu_{dis,en}able(), note how
> intel_pmu_handle_irq() saves/restores the enable state.
>
Yes, it is. Will fix it.
>> +
>> + return handled;
>> +}
>> +
>> +DEFINE_STATIC_CALL(amd_pmu_handle_irq, x86_pmu_handle_irq);
>> +
>> /*
>> * Because of NMI latency, if multiple PMC counters are active or other sources
>> * of NMIs are received, the perf NMI handler can handle one or more overflowed
>> @@ -741,7 +856,7 @@ static int amd_pmu_handle_irq(struct pt_regs *regs)
>> int handled;
>>
>> /* Process any counter overflows */
>> - handled = x86_pmu_handle_irq(regs);
>> + handled = static_call(amd_pmu_handle_irq)(regs);
>>
>> /*
>> * If a counter was handled, record a timestamp such that un-handled
>> @@ -1041,6 +1156,8 @@ static int __init amd_core_pmu_init(void)
>> static_call_update(amd_pmu_enable_all, amd_pmu_global_enable_all);
>> static_call_update(amd_pmu_disable_all, amd_pmu_global_disable_all);
>> static_call_update(amd_pmu_enable_event, amd_pmu_global_enable_event);
>> + static_call_update(amd_pmu_has_overflow, amd_pmu_global_has_overflow);
>> + static_call_update(amd_pmu_handle_irq, amd_pmu_global_handle_irq);
>> }
>
> Same, all this static_call() stuff is misguided.
>
> Also, if you feel like it, you can create amd_pmu_v2.
Sure.
- Sandipan
Powered by blists - more mailing lists