Re: [PATCH] iommu/arm-smmu-v3: Enable PCI ATS in passthrough mode as well
From: Ganapatrao Kulkarni
Date: Tue Feb 07 2023 - 04:51:19 EST
On 07-02-2023 01:15 am, Jean-Philippe Brucker wrote:
On Mon, Feb 06, 2023 at 10:50:00PM +0530, Ganapatrao Kulkarni wrote:
On 03-02-2023 05:30 pm, Jean-Philippe Brucker wrote:
On Thu, Feb 02, 2023 at 04:40:53AM -0800, Ganapatrao Kulkarni wrote:
The current smmu-v3 driver does not enable PCI ATS for physical functions
of ATS capable End Points when booted in smmu bypass mode
(iommu.passthrough=1). This will not allow virtual functions to enable
ATS(even though EP supports it) while they are attached to a VM using
VFIO driver.
This patch adds changes to enable ATS support for physical functions
in passthrough/bypass mode as well.
[...]
@@ -2453,8 +2458,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
master->domain = smmu_domain;
- if (smmu_domain->stage != ARM_SMMU_DOMAIN_BYPASS)
- master->ats_enabled = arm_smmu_ats_supported(master);
+ master->ats_enabled = arm_smmu_ats_supported(master);
I should have added a comment for this. Only found the reason in an old
cover letter [1]:
"When no translation stages are enabled (0b100), ATS Translation Requests
(and Translated traffic, if SMMU_CR0.ATSCHK == 1) are denied as though
EATS == 0b00; the actual value of the EATS field is IGNORED. Such a
Translation Request causes F_BAD_ATS_TREQ and Translated traffic causes
F_TRANSL_FORBIDDEN."
(See 3.9.1.1 "Responses to ATS Translation Requests and Translated
transactions" and 5.2 "Stream Table Entry")
So I don't think we can enable ATS for bypass domains :/ The PF needs to
be in translated mode in that case.
Are you intending to say smmu-v3 driver/spec will not support ATS to a VF,
if it's PF is in bypass?
What I meant was that if, in order to enable the VF ATS capability, the
PCIe device requires to first enable the PF ATS capability, then given
that the SMMU does not allow enabling ATS for streams in bypass, we need
to enable translation on both PF and VF SMMU configurations.
But reading the PCIe spec, it looks like the capabilities are not
necessarily tied together:
"The ATS Capabilities in VFs and their associated PFs are permitted to be
enabled independently." 10.5.1 ATS Extended Capability
That wording doesn't indicate that all implementations must allow enabling
the caps independently, only that they are allowed to do so. If a device
provides independent capabilities, then I don't think you need to enable
ATS in the PF.
Yes makes sense, thanks.
Then the ATS Control Register seems to contradict this with the STU field:
"For VFs, this field must be hardwired to Zero. The associated PF's value
applies." 10.5.1.3 ATS Control Register (Offset 06h)
So pci_enable_ats() requires that pdev->ats_stu is set in order to enable
ATS on the VF:
/*
* Note that enabling ATS on a VF fails unless it's already enabled
* with the same STU on the PF.
*/
This comment was misleading!
...
if (dev->is_virtfn) {
pdev = pci_physfn(dev);
if (pdev->ats_stu != ps)
return -EINVAL;
But I suspect it's done this way only because it's easier to implement.
The spec doesn't require the PF ATS capability to be enabled, it just says
that the PF's STU value counts as the VF's one. It looks like we're
allowed to enable ATS on the VF without enabling it on the PF, right?
OK thanks, as per spec ATS enable on VF and PF are independent. only STU
and Capability are hardwired to be same.
If you rework the PCI driver to only write the PF's STU without enabling
the capability, then you could enable it in the VF.
Makes sense, It should be feasible to write to STU of a PF without
enabling it.
Below diff is more appropriate fix for this case.
diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index c967ad6e2626..f064c2be8593 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -74,6 +74,15 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
ctrl = PCI_ATS_CTRL_ENABLE;
if (dev->is_virtfn) {
pdev = pci_physfn(dev);
+
+ if (!pdev->ats_enabled) {
+ u16 ctrl2;
+
+ pdev->ats_stu = ps;
+ ctrl2 = PCI_ATS_CTRL_STU(pdev->ats_stu -
PCI_ATS_MIN_STU);
+ pci_write_config_word(pdev, pdev->ats_cap +
PCI_ATS_CTRL, ctrl2);
+ }
+
if (pdev->ats_stu != ps)
return -EINVAL;
} else {
Thanks,
Ganapat
Thanks,
Jean
I can send a patch adding the comment next cycle.
I am more keen to know, how we enable ATS to a VF of ATS capable EP when
it's PF is in bypass?
or it is mandatory to have a PF also translated? then that should be
captured somewhere in documentation.
Thanks,
Jean
[1] https://lore.kernel.org/linux-iommu/20190409165245.26500-1-jean-philippe.brucker@xxxxxxx/
Thanks,
Ganapat