Re: [PATCH v6 1/3] PCI: Add pci_ats_required() for CXL.cache capable devices

[Nicolin Chen <nicolinc@nvidia.com>]

On Thu, May 21, 2026 at 03:57:23PM -0500, Bjorn Helgaas wrote:
> On Thu, May 21, 2026 at 01:34:20PM -0700, Nicolin Chen wrote:
> > +bool pci_ats_required(struct pci_dev *pdev)
> > +{
> > +	if (!pci_ats_supported(pdev))
> > +		return false;
> > +
> > +	/* A VF inherits its PF's requirement for ATS function */
> > +	if (pdev->is_virtfn)
> > +		pdev = pci_physfn(pdev);
> > +
> > +	return pci_cxl_ats_required(pdev);
> 
> I acked this before I saw this sashiko feedback, which looks like a
> legit issue to me:
> 
>   Will this VF inheritance logic ever be reached?
> 
>   According to the PCIe SR-IOV specification (section 9.3.3.1), VFs do
>   not implement the ATS Extended Capability, which means pdev->ats_cap
>   is always 0 for VFs.
> 
>   Because of this, pci_ats_supported(pdev) will unconditionally return
>   false for any VF. This causes the function to return false before it
>   can ever reach the pdev->is_virtfn check.
> 
>   Could this prevent VFs from correctly enabling the ATS always on
>   feature and leave them unable to access host memory without
>   triggering IOMMU faults?
> 
> (From https://sashiko.dev/#/patchset/cover.1779304390.git.nicolinc%40nvidia.com)
>
> I withdraw my ack for now until we figure out if it's a real issue.

I did a bit of research here.

The existing pci_enable_ats() checks:

	if (!pci_ats_supported(dev))
		return -EINVAL;

at the top, prior to:

	if (dev->is_virtfn) {
		pdev = pci_physfn(dev);
		if (pdev->ats_stu != ps)
			return -EINVAL;

So, VF must *support* ATS (its ats_cap must be !0) so as to turn
on ATS, instead of relying on the PF's ats_cap.

IOW, Sashiko's comment "which means pdev->ats_cap is always 0 for
VFs" is wrong.

So, I kept the same order in this new pci_ats_required().

FWIW, I also did an inverted version and asked Sashiko to review:
====================================================================
> @@ -205,6 +205,52 @@ int pci_ats_page_aligned(struct pci_dev *pdev)
[ ... ]
> +bool pci_ats_required(struct pci_dev *pdev)
> +{
> +	/* A VF inherits its PF's requirement for ATS function */
> +	if (pdev->is_virtfn)
> +		pdev = pci_physfn(pdev);
> +
> +        if (!pci_ats_supported(pdev))
> +                return false;

This isn't a bug, but it looks like these two lines are indented with spaces
instead of tabs.
Also, since pdev is reassigned to pci_physfn(pdev) before checking ATS
support, does this incorrectly check the ATS capability of the PF rather than
the VF?
If a VF has its ATS support explicitly disabled (for example, via PCIe quirks
that set vf->ats_cap = 0), this check might bypass the VF's specific
properties and incorrectly return true based on the PF's capabilities.
Consequently, if a caller attempts to enable ATS on the VF by calling
pci_enable_ats(), it will fail because pci_enable_ats() correctly checks
pci_ats_supported() against the VF.
Would it be better to evaluate pci_ats_supported() on the original pdev
before reassigning it to the PF?
====================================================================

What would you like me to do, Bjorn?

Nicolin