Re: [PATCH v4 05/11] perf/cxl: Split the MSI vector out of info->irq

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The CXL PMU interrupt is requested with IRQF_SHARED, but its CPU affinity is dynamically modified during CPU hotplug, which is unsafe for shared interrupts.
--

--- Patch [5]: [PATCH v4 05/11] perf/cxl: Split the MSI vector out of info->irq ---
commit 7ced4bedee1c856d5114f92c2eb91db914c1a974
Author: Dave Jiang <[email protected]>

perf/cxl: Split the MSI vector out of info->irq

This commit separates the MSI/MSI-X message number from the Linux virq in
info->irq. It introduces info->msi_vec for the message number while retaining
info->irq for the Linux virq to prevent mix-ups between the two distinct
values.

> diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
> index 2bebbbe67a0a2..481d32d0c1b02 100644
> --- a/drivers/perf/cxl_pmu.c
> +++ b/drivers/perf/cxl_pmu.c

[ ... ]

> @@ -873,10 +874,10 @@ static int cxl_pmu_probe(struct device *dev)
>  		.capabilities = PERF_PMU_CAP_NO_EXCLUDE,
>  	};
>  
> -	if (info->irq < 0)
> +	if (info->msi_vec < 0)
>  		return -EINVAL;
>  
> -	rc = pci_irq_vector(pdev, info->irq);
> +	rc = pci_irq_vector(pdev, info->msi_vec);
>  	if (rc < 0)
>  		return rc;
>  	irq = rc;

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the IRQ
handling here, I noticed a potential problem with how the PMU interrupt is
managed in drivers/perf/cxl_pmu.c.

In cxl_pmu_probe(), the interrupt is requested as shared and without
IRQF_NOBALANCING:

rc = devm_request_irq(dev, irq, cxl_pmu_irq, IRQF_SHARED | IRQF_NO_THREAD,
                      irq_name, info);

However, the CPU hotplug callbacks dynamically modify the CPU affinity of
this shared interrupt:

cxl_pmu_online_cpu() {
    ...
    WARN_ON(irq_set_affinity(info->irq, cpumask_of(cpu)));
}

cxl_pmu_offline_cpu() {
    ...
    WARN_ON(irq_set_affinity(info->irq, cpumask_of(target)));
}

Is it safe to change the affinity of a shared interrupt like this?

Modifying the affinity alters it for all co-owners of the IRQ.

Additionally, if irqbalance changes the affinity (since IRQF_NOBALANCING is
missing), could the PMU interrupt handler be forced to execute on the wrong
CPU and potentially corrupt state when locklessly accessing per-CPU data?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5