Re: [PATCH V4 2/7] iommu/vt-d: Retrieve IOMMU perfmon capability information

[Dmytro Maluka]

Sorry for the late reply, I've just stumbled upon this code and a few
things look confusing to me:

On Sat, Jan 28, 2023 at 12:04:23PM -0800, kan.liang@xxxxxxxxxxxxxxx wrote:
<...>
> +	/*
> +	 * Check per-counter capabilities. All counters should have the
> +	 * same capabilities on Interrupt on Overflow Support and Counter
> +	 * Width.
> +	 */
> +	for (i = 0; i < iommu_pmu->num_cntr; i++) {
> +		cap = dmar_readl(iommu_pmu->cfg_reg +
> +				 i * IOMMU_PMU_CFG_OFFSET +
> +				 IOMMU_PMU_CFG_CNTRCAP_OFFSET);
> +		if (!iommu_cntrcap_pcc(cap))
> +			continue;
> +
> +		/*
> +		 * It's possible that some counters have a different
> +		 * capability because of e.g., HW bug. Check the corner
> +		 * case here and simply drop those counters.
> +		 */
> +		if ((iommu_cntrcap_cw(cap) != iommu_pmu->cntr_width) ||
> +		    !iommu_cntrcap_ios(cap)) {
> +			iommu_pmu->num_cntr = i;
> +			pr_warn("PMU counter capability inconsistent, counter number reduced to %d\n",
> +				iommu_pmu->num_cntr);

1. AFAICS according to the VT-d spec section 11.4.13.2 (about the global
Counter Width in the PERFCAP register): "If per-counter capabilities are
supported, the counter width indicated in the PERFCNTRCAP registers
overrides this value."

which, I'd assume, means that a per-counter Counter Width is allowed
to be different from the global one (i.e. it is not necessarily a HW
bug)?

2. By truncating the number of counters here, we disregard all
subsequent counters, including possibly valid ones (which would pass
this check)?

3. If we disregard all subsequent counters anyway, why don't we break
from the loop here?

> +		}
> +
> +		/* Clear the pre-defined events group */
> +		for (j = 0; j < iommu_pmu->num_eg; j++)
> +			iommu_pmu->cntr_evcap[i][j] = 0;
> +
> +		/* Override with per-counter event capabilities */
> +		for (j = 0; j < iommu_cntrcap_egcnt(cap); j++) {
> +			cap = dmar_readl(iommu_pmu->cfg_reg + i * IOMMU_PMU_CFG_OFFSET +
> +					 IOMMU_PMU_CFG_CNTREVCAP_OFFSET +
> +					 (j * IOMMU_PMU_OFF_REGS_STEP));
> +			iommu_pmu->cntr_evcap[i][iommu_event_group(cap)] = iommu_event_select(cap);
> +			/*
> +			 * Some events may only be supported by a specific counter.
> +			 * Track them in the evcap as well.
> +			 */
> +			iommu_pmu->evcap[iommu_event_group(cap)] |= iommu_event_select(cap);
> +		}
> +	}
<...>