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

[Baolu Lu]

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