| Message ID | 20191219163033.2608177-14-jean-philippe@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,01/13] iommu/arm-smmu-v3: Drop __GFP_ZERO flag from DMA allocation | expand |
Hi Jean, On 12/19/19 5:30 PM, Jean-Philippe Brucker wrote: > Enable PASID for PCI devices that support it. Since the SSID tables are > allocated by arm_smmu_attach_dev(), PASID has to be enabled early enough. > arm_smmu_dev_feature_enable() would be too late, since by that time the > main DMA domain has already been attached. Do it in add_device() instead. > > Tested-by: Zhangfei Gao <zhangfei.gao@linaro.org> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> Reviewed-by: Eric Auger <eric.auger@redhat.com> Thanks Eric > --- > drivers/iommu/arm-smmu-v3.c | 55 ++++++++++++++++++++++++++++++++++++- > 1 file changed, 54 insertions(+), 1 deletion(-) > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c > index e62ca80f2f76..8e95ecad4c9a 100644 > --- a/drivers/iommu/arm-smmu-v3.c > +++ b/drivers/iommu/arm-smmu-v3.c > @@ -2644,6 +2644,53 @@ static void arm_smmu_disable_ats(struct arm_smmu_master *master) > atomic_dec(&smmu_domain->nr_ats_masters); > } > > +static int arm_smmu_enable_pasid(struct arm_smmu_master *master) > +{ > + int ret; > + int features; > + int num_pasids; > + struct pci_dev *pdev; > + > + if (!dev_is_pci(master->dev)) > + return -ENODEV; > + > + pdev = to_pci_dev(master->dev); > + > + features = pci_pasid_features(pdev); > + if (features < 0) > + return features; > + > + num_pasids = pci_max_pasids(pdev); > + if (num_pasids <= 0) > + return num_pasids; > + > + ret = pci_enable_pasid(pdev, features); > + if (ret) { > + dev_err(&pdev->dev, "Failed to enable PASID\n"); > + return ret; > + } > + > + master->ssid_bits = min_t(u8, ilog2(num_pasids), > + master->smmu->ssid_bits); > + return 0; > +} > + > +static void arm_smmu_disable_pasid(struct arm_smmu_master *master) > +{ > + struct pci_dev *pdev; > + > + if (!dev_is_pci(master->dev)) > + return; > + > + pdev = to_pci_dev(master->dev); > + > + if (!pdev->pasid_enabled) > + return; > + > + master->ssid_bits = 0; > + pci_disable_pasid(pdev); > +} > + > static void arm_smmu_detach_dev(struct arm_smmu_master *master) > { > unsigned long flags; > @@ -2852,13 +2899,16 @@ static int arm_smmu_add_device(struct device *dev) > > master->ssid_bits = min(smmu->ssid_bits, fwspec->num_pasid_bits); > > + /* Note that PASID must be enabled before, and disabled after ATS */ > + arm_smmu_enable_pasid(master); > + > if (!(smmu->features & ARM_SMMU_FEAT_2_LVL_CDTAB)) > master->ssid_bits = min_t(u8, master->ssid_bits, > CTXDESC_LINEAR_CDMAX); > > ret = iommu_device_link(&smmu->iommu, dev); > if (ret) > - goto err_free_master; > + goto err_disable_pasid; > > group = iommu_group_get_for_dev(dev); > if (IS_ERR(group)) { > @@ -2871,6 +2921,8 @@ static int arm_smmu_add_device(struct device *dev) > > err_unlink: > iommu_device_unlink(&smmu->iommu, dev); > +err_disable_pasid: > + arm_smmu_disable_pasid(master); > err_free_master: > kfree(master); > fwspec->iommu_priv = NULL; > @@ -2891,6 +2943,7 @@ static void arm_smmu_remove_device(struct device *dev) > arm_smmu_detach_dev(master); > iommu_group_remove_device(dev); > iommu_device_unlink(&smmu->iommu, dev); > + arm_smmu_disable_pasid(master); > kfree(master); > iommu_fwspec_free(dev); > } >
On Thu, Dec 19, 2019 at 05:30:33PM +0100, Jean-Philippe Brucker wrote: > Enable PASID for PCI devices that support it. Since the SSID tables are > allocated by arm_smmu_attach_dev(), PASID has to be enabled early enough. > arm_smmu_dev_feature_enable() would be too late, since by that time the What is arm_smmu_dev_feature_enable()? > main DMA domain has already been attached. Do it in add_device() instead. > > Tested-by: Zhangfei Gao <zhangfei.gao@linaro.org> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> > --- > drivers/iommu/arm-smmu-v3.c | 55 ++++++++++++++++++++++++++++++++++++- > 1 file changed, 54 insertions(+), 1 deletion(-) > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c > index e62ca80f2f76..8e95ecad4c9a 100644 > --- a/drivers/iommu/arm-smmu-v3.c > +++ b/drivers/iommu/arm-smmu-v3.c > @@ -2644,6 +2644,53 @@ static void arm_smmu_disable_ats(struct arm_smmu_master *master) > atomic_dec(&smmu_domain->nr_ats_masters); > } > > +static int arm_smmu_enable_pasid(struct arm_smmu_master *master) > +{ > + int ret; > + int features; > + int num_pasids; > + struct pci_dev *pdev; > + > + if (!dev_is_pci(master->dev)) > + return -ENODEV; > + > + pdev = to_pci_dev(master->dev); > + > + features = pci_pasid_features(pdev); > + if (features < 0) > + return features; > + > + num_pasids = pci_max_pasids(pdev); > + if (num_pasids <= 0) > + return num_pasids; > + > + ret = pci_enable_pasid(pdev, features); > + if (ret) { > + dev_err(&pdev->dev, "Failed to enable PASID\n"); > + return ret; > + } > + > + master->ssid_bits = min_t(u8, ilog2(num_pasids), > + master->smmu->ssid_bits); > + return 0; > +} > + > +static void arm_smmu_disable_pasid(struct arm_smmu_master *master) > +{ > + struct pci_dev *pdev; > + > + if (!dev_is_pci(master->dev)) > + return; > + > + pdev = to_pci_dev(master->dev); > + > + if (!pdev->pasid_enabled) > + return; > + > + master->ssid_bits = 0; > + pci_disable_pasid(pdev); > +} > + > static void arm_smmu_detach_dev(struct arm_smmu_master *master) > { > unsigned long flags; > @@ -2852,13 +2899,16 @@ static int arm_smmu_add_device(struct device *dev) > > master->ssid_bits = min(smmu->ssid_bits, fwspec->num_pasid_bits); > > + /* Note that PASID must be enabled before, and disabled after ATS */ > + arm_smmu_enable_pasid(master); Is that part of the PCIe specs? If so, please can you add a citation to the comment? Are there any other ordering requirements, i.e. with respect to enabling substreams at the SMMU? For example, can a speculative ATS request provide a PASID? Will
On Tue, Jan 14, 2020 at 12:45:42PM +0000, Will Deacon wrote: > On Thu, Dec 19, 2019 at 05:30:33PM +0100, Jean-Philippe Brucker wrote: > > Enable PASID for PCI devices that support it. Since the SSID tables are > > allocated by arm_smmu_attach_dev(), PASID has to be enabled early enough. > > arm_smmu_dev_feature_enable() would be too late, since by that time the > > What is arm_smmu_dev_feature_enable()? It's the implementation of the IOMMU op .dev_enable_feat(), which I'll add later (called by a device driver to enable the SVA feature). I'll reword this comment, since the only real requirement is enabling PASID before ATS. > > static void arm_smmu_detach_dev(struct arm_smmu_master *master) > > { > > unsigned long flags; > > @@ -2852,13 +2899,16 @@ static int arm_smmu_add_device(struct device *dev) > > > > master->ssid_bits = min(smmu->ssid_bits, fwspec->num_pasid_bits); > > > > + /* Note that PASID must be enabled before, and disabled after ATS */ > > + arm_smmu_enable_pasid(master); > > Is that part of the PCIe specs? If so, please can you add a citation to the > comment? Yes (PCIe 4.0r1.0 10.5.1.3 ATS Control register). > Are there any other ordering requirements, i.e. with respect to enabling > substreams at the SMMU? For example, can a speculative ATS request provide > a PASID? You recent fix bfff88ec1afe ("iommu/arm-smmu-v3: Rework enabling/disabling of ATS for PCI masters") should prevent from speculative ATS requests. More generally both ATS and SSID are enabled and disabled at the same time in the SMMU, when toggling STE.V, so any request arriving before STE enablement will be aborted regardless of SSID. Thanks, Jean
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c index e62ca80f2f76..8e95ecad4c9a 100644 --- a/drivers/iommu/arm-smmu-v3.c +++ b/drivers/iommu/arm-smmu-v3.c @@ -2644,6 +2644,53 @@ static void arm_smmu_disable_ats(struct arm_smmu_master *master) atomic_dec(&smmu_domain->nr_ats_masters); } +static int arm_smmu_enable_pasid(struct arm_smmu_master *master) +{ + int ret; + int features; + int num_pasids; + struct pci_dev *pdev; + + if (!dev_is_pci(master->dev)) + return -ENODEV; + + pdev = to_pci_dev(master->dev); + + features = pci_pasid_features(pdev); + if (features < 0) + return features; + + num_pasids = pci_max_pasids(pdev); + if (num_pasids <= 0) + return num_pasids; + + ret = pci_enable_pasid(pdev, features); + if (ret) { + dev_err(&pdev->dev, "Failed to enable PASID\n"); + return ret; + } + + master->ssid_bits = min_t(u8, ilog2(num_pasids), + master->smmu->ssid_bits); + return 0; +} + +static void arm_smmu_disable_pasid(struct arm_smmu_master *master) +{ + struct pci_dev *pdev; + + if (!dev_is_pci(master->dev)) + return; + + pdev = to_pci_dev(master->dev); + + if (!pdev->pasid_enabled) + return; + + master->ssid_bits = 0; + pci_disable_pasid(pdev); +} + static void arm_smmu_detach_dev(struct arm_smmu_master *master) { unsigned long flags; @@ -2852,13 +2899,16 @@ static int arm_smmu_add_device(struct device *dev) master->ssid_bits = min(smmu->ssid_bits, fwspec->num_pasid_bits); + /* Note that PASID must be enabled before, and disabled after ATS */ + arm_smmu_enable_pasid(master); + if (!(smmu->features & ARM_SMMU_FEAT_2_LVL_CDTAB)) master->ssid_bits = min_t(u8, master->ssid_bits, CTXDESC_LINEAR_CDMAX); ret = iommu_device_link(&smmu->iommu, dev); if (ret) - goto err_free_master; + goto err_disable_pasid; group = iommu_group_get_for_dev(dev); if (IS_ERR(group)) { @@ -2871,6 +2921,8 @@ static int arm_smmu_add_device(struct device *dev) err_unlink: iommu_device_unlink(&smmu->iommu, dev); +err_disable_pasid: + arm_smmu_disable_pasid(master); err_free_master: kfree(master); fwspec->iommu_priv = NULL; @@ -2891,6 +2943,7 @@ static void arm_smmu_remove_device(struct device *dev) arm_smmu_detach_dev(master); iommu_group_remove_device(dev); iommu_device_unlink(&smmu->iommu, dev); + arm_smmu_disable_pasid(master); kfree(master); iommu_fwspec_free(dev); }