linux-kernel - Re: [PATCH 4/7] iommu/vt-d: Add IOMMU perfmon support

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <214ee67e-19f5-265f-63be-f8a6108d25d1@linux.intel.com>
Date:   Tue, 17 Jan 2023 16:12:16 +0800
From:   Baolu Lu <baolu.lu@...ux.intel.com>
To:     "Liang, Kan" <kan.liang@...ux.intel.com>, joro@...tes.org,
        will@...nel.org, dwmw2@...radead.org, robin.murphy@....com,
        robert.moore@...el.com, rafael.j.wysocki@...el.com,
        lenb@...nel.org, iommu@...ts.linux.dev,
        linux-kernel@...r.kernel.org
Cc:     baolu.lu@...ux.intel.com
Subject: Re: [PATCH 4/7] iommu/vt-d: Add IOMMU perfmon support

On 2023/1/16 23:12, Liang, Kan wrote:
>>> +static void iommu_pmu_start(struct perf_event *event, int flags)
>>> +{
>>> +    struct iommu_pmu *iommu_pmu = iommu_event_to_pmu(event);
>>> +    struct intel_iommu *iommu = iommu_pmu->iommu;
>>> +    struct hw_perf_event *hwc = &event->hw;
>>> +    u64 count;
>>> +
>>> +    if (WARN_ON_ONCE(!(hwc->state & PERF_HES_STOPPED)))
>>> +        return;
>>> +
>>> +    if (WARN_ON_ONCE(hwc->idx < 0 || hwc->idx >= IOMMU_PMU_IDX_MAX))
>>> +        return;
>>> +
>>> +    if (flags & PERF_EF_RELOAD)
>>> +        WARN_ON_ONCE(!(event->hw.state & PERF_HES_UPTODATE));
>>> +
>>> +    hwc->state = 0;
>>> +
>>> +    /* Always reprogram the period */
>>> +    count = dmar_readq(iommu_event_base(iommu_pmu, hwc->idx));
>>> +    local64_set((&hwc->prev_count), count);
>>> +
>>> +    ecmd_submit_sync(iommu, DMA_ECMD_ENABLE, hwc->idx, false, 0);
>> What happens when emcmd_submit_sync() returns an error? How should we
>> handle this case? The same queestion to other places in this patch.
>>
> The existing perf_event subsystem doesn't handle the error, because
> other PMUs never trigger such errors. Perf usually check all the PMU
> counters once at the beginning when registering/initializing them.
> 
> For IOMMU PMU, the error will be ignored. I think it should be OK. Because
> - It's a corner case, which is very unlikely to happen.
> - The worst case is that the user will get <not count> with perf
> command, which can the user some hints.
> 
> If it's not good enough, I think we can add a WARN_ON_ONCE() here and
> everywhere for ecmd to dump the error in the dmesg.
> 
> What do you think?

No need for a WARN() here. If the hardware is stuck, there should be
warnings everywhere.

It's fine to me if you add above comments around the code.

--
Best regards,
baolu