From: Jason Gunthorpe <jgg@nvidia.com>
Sent: Friday, August 9, 2024 9:29 PM

On Fri, Aug 09, 2024 at 02:36:14AM +0000, Tian, Kevin wrote:

quoted

quoted

From: Jason Gunthorpe <jgg@nvidia.com>
Sent: Thursday, August 8, 2024 2:19 AM

PCI ATS has a global Smallest Translation Unit field that is located in
the PF but shared by all of the VFs.

The expectation is that the STU will be set to the root port's global STU
capability which is driven by the IO page table configuration of the iommu
HW. Today it becomes set when the iommu driver first enables ATS.

Thus, to enable ATS on the VF, the PF must have already had the correct
STU programmed, even if ATS is off on the PF.

Unfortunately the PF only programs the STU when the PF enables ATS.

The

quoted

quoted

iommu drivers tend to leave ATS disabled when IDENTITY translation is
being used.

Is there more context on this?

How do you mean?

quoted

Looking at intel-iommu driver ATS is disabled for IDENETITY when
the iommu is in legacy mode:

dmar_domain_attach_device()
{
	...
	if (sm_supported(info->iommu) || !domain_type_is_si(info-
domain))
		iommu_enable_pci_caps(info);
	...
}

But this follows what VT-d spec says (section 9.3):

TT: Translate Type
10b: Untranslated requests are processed as pass-through. The SSPTPTR
field is ignored by hardware. Translated and Translation Requests are
blocked.

Yes, HW like this is exactly the problem, it ends up not enabling ATS
on the PF and then we don't have the STU programmed so the VF is
effectively disabled too.

Ideally iommus would continue to work with translated requests when
ATS is enabled. Not supporting this configuration creates a nasty
problem for devices that are using PASID.

The PASID may require ATS to be enabled (ie SVA), but the RID may be
IDENTITY for performance. The poor device has no idea it is not
allowed to use ATS on the RID side :(

Okay, I see the problem now. 😊

quoted

quoted

+/**
+ * pci_prepare_ats - Setup the PS for ATS
+ * @dev: the PCI device
+ * @ps: the IOMMU page shift
+ *
+ * This must be done by the IOMMU driver on the PF before any VFs are
created to
+ * ensure that the VF can have ATS enabled.
+ *
+ * Returns 0 on success, or negative on failure.
+ */
+int pci_prepare_ats(struct pci_dev *dev, int ps)
+{
+	u16 ctrl;
+
+	if (!pci_ats_supported(dev))
+		return -EINVAL;
+
+	if (WARN_ON(dev->ats_enabled))
+		return -EBUSY;
+
+	if (ps < PCI_ATS_MIN_STU)
+		return -EINVAL;
+
+	if (dev->is_virtfn)
+		return 0;

missed a check that 'ps' matches pf's ats_stu.

Deliberate, that check is done when enabling ats:

quoted

quoted

+
+	dev->ats_stu = ps;
+	ctrl = PCI_ATS_CTRL_STU(dev->ats_stu - PCI_ATS_MIN_STU);
+	pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pci_prepare_ats);
+

Then there is no need to keep the 'ps' parameter in pci_enable_ats().

Which is why I left it here.

My initial thought was to save 'ps' to dev->ats_stu for both pf/vf
in pci_prepare_ats() then pci_enable_ats() can simply check
dev->ats_stu and dev->ats_enabled to avoid other two duplicated
checks and remove the 'ps' parameter.

But that comment is minor and if you still like the current way:

Reviewed-by: Kevin Tian <kevin.tian@intel.com>