Re: [PATCH v2] x86/events/amd/iommu: Use a per-IOMMU cpumask

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] x86/events/amd/iommu: unlocked cpumask read in
  _iommu_cpumask_show()
--

commit 90882d03fd52ecfe91f951e077a922dfbbba36d6
Author: Vasant Hegde <vasant.hegde@amd.com>

x86/events/amd/iommu: Use a per-IOMMU cpumask

This commit replaces the global CPU0-pinned cpumask in the AMD IOMMU PMU
driver with a per-IOMMU NUMA-local cpumask and adds CPU hotplug callbacks
to migrate the reader context when CPUs go offline or return online.

> diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
> index f332c7089bd59..3b35702aa702b 100644
> --- a/arch/x86/events/amd/iommu.c
> +++ b/arch/x86/events/amd/iommu.c
[ ... ]
> @@ -131,13 +134,15 @@ static struct amd_iommu_event_desc amd_iommu_v2_event_descs[] = {
>  /*---------------------------------------------
>   * sysfs cpumask attributes
>   *---------------------------------------------*/
> -static cpumask_t iommu_cpumask;
> -
>  static ssize_t _iommu_cpumask_show(struct device *dev,
>  				   struct device_attribute *attr,
>  				   char *buf)
>  {
> -	return sysfs_emit(buf, "%*pbl\n", cpumask_pr_args(&iommu_cpumask));
> +	struct pmu *pmu = dev_get_drvdata(dev);
> +	struct perf_amd_iommu *perf_iommu =
> +			container_of(pmu, struct perf_amd_iommu, pmu);
> +
> +	return sysfs_emit(buf, "%*pbl\n", cpumask_pr_args(&perf_iommu->cpumask));
>  }

[Severity: Low]
Should _iommu_cpumask_show() hold perf_iommu->lock while reading
perf_iommu->cpumask?

In iommu_pmu_cpu_offline() and iommu_pmu_cpu_online(), perf_iommu->cpumask
is cleared under perf_iommu->lock before the replacement CPU bit is set:

CPU0                              CPU1
iommu_pmu_cpu_offline()           _iommu_cpumask_show()
  raw_spin_lock_irqsave(&perf_iommu->lock, ...)
  cpumask_test_and_clear_cpu(cpu, &perf_iommu->cpumask)
                                    sysfs_emit(..., &perf_iommu->cpumask)
                                      <-- sees empty cpumask
  target = iommu_pmu_pick_cpu(...)
  cpumask_set_cpu(target, &perf_iommu->cpumask)
  raw_spin_unlock_irqrestore(&perf_iommu->lock, ...)

Unlike perf_iommu_event_init(), which takes perf_iommu->lock around
cpumask_first(), a concurrent sysfs read of cpumask can observe the
transiently empty mask and return an empty string to userspace.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113150502.685887-1-vasant.hegde@amd.com?part=1