[PATCH 0/2] Add support to enable ATS on VFs independently.

[PATCH 0/2] Add support to enable ATS on VFs independently.

[Ganapatrao Kulkarni]

As discussed in [1], adding a helper function to configure the STU of an
ATS capability. Function pci_ats_stu_configure() can be called to program
the STU while enumerating the PF, to support scenarios like PF is not
enabled with ATS, whereas VFs can enable it.

In SMMU-V3 driver, calling pci_ats_stu_configure() to confgiure the STU
while enumerating a PF in passthrough mode.


[1] https://lore.kernel.org/linux-arm-kernel/20230221154624.GA3701506@bhelgaas/T/


Ganapatrao Kulkarni (2):
  PCI/ATS: Add a helper function to configure ATS STU of a PF.
  iommu/arm-smmu-v3: Configure STU of a PF if ATS is not enabled.

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 25 +++++++++++++++-
 drivers/pci/ats.c                           | 32 +++++++++++++++++++--
 include/linux/pci-ats.h                     |  1 +
 3 files changed, 55 insertions(+), 3 deletions(-)

-- 
2.38.1

[PATCH 0/2] Add support to enable ATS on VFs independently.

[Ganapatrao Kulkarni]

As discussed in [1], adding a helper function to configure the STU of an
ATS capability. Function pci_ats_stu_configure() can be called to program
the STU while enumerating the PF, to support scenarios like PF is not
enabled with ATS, whereas VFs can enable it.

In SMMU-V3 driver, calling pci_ats_stu_configure() to confgiure the STU
while enumerating a PF in passthrough mode.


[1] https://lore.kernel.org/linux-arm-kernel/20230221154624.GA3701506@bhelgaas/T/


Ganapatrao Kulkarni (2):
  PCI/ATS: Add a helper function to configure ATS STU of a PF.
  iommu/arm-smmu-v3: Configure STU of a PF if ATS is not enabled.

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 25 +++++++++++++++-
 drivers/pci/ats.c                           | 32 +++++++++++++++++++--
 include/linux/pci-ats.h                     |  1 +
 3 files changed, 55 insertions(+), 3 deletions(-)

-- 
2.38.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/2] PCI/ATS: Add a helper function to configure ATS STU of a PF.

[Ganapatrao Kulkarni]

As per PCI specification (PCI Express Base Specification Revision
6.0, Section 10.5) both PF and VFs of a PCI EP are permitted to be enabled
independently for ATS capability, however the STU(Smallest Translation
Unit) is shared between PF and VFs. For VFs, it is hardwired to Zero and
the associated PF's value applies to VFs.

In the current code, the STU is being configured while enabling the PF ATS.
Hence, it is not able to enable ATS for VFs, if it is not enabled on the
associated PF already.

Adding a function pci_ats_stu_configure(), which can be called to
configure the STU during PF enumeration.
Latter enumerations of VFs can successfully enable ATS independently.

Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
---
 drivers/pci/ats.c       | 32 ++++++++++++++++++++++++++++++--
 include/linux/pci-ats.h |  1 +
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index f9cc2e10b676..70e1982efdb4 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -46,6 +46,34 @@ bool pci_ats_supported(struct pci_dev *dev)
 }
 EXPORT_SYMBOL_GPL(pci_ats_supported);
 
+/**
+ * pci_ats_stu_configure - Configure STU of a PF.
+ * @dev: the PCI device
+ * @ps: the IOMMU page shift
+ *
+ * Returns 0 on success, or negative on failure.
+ */
+int pci_ats_stu_configure(struct pci_dev *dev, int ps)
+{
+	u16 ctrl;
+
+	if (dev->ats_enabled || dev->is_virtfn)
+		return 0;
+
+	if (!pci_ats_supported(dev))
+		return -EINVAL;
+
+	if (ps < PCI_ATS_MIN_STU)
+		return -EINVAL;
+
+	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_ats_stu_configure);
+
 /**
  * pci_enable_ats - enable the ATS capability
  * @dev: the PCI device
@@ -68,8 +96,8 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
 		return -EINVAL;
 
 	/*
-	 * Note that enabling ATS on a VF fails unless it's already enabled
-	 * with the same STU on the PF.
+	 * Note that enabling ATS on a VF fails unless it's already
+	 * configured with the same STU on the PF.
 	 */
 	ctrl = PCI_ATS_CTRL_ENABLE;
 	if (dev->is_virtfn) {
diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
index df54cd5b15db..9b40eb555124 100644
--- a/include/linux/pci-ats.h
+++ b/include/linux/pci-ats.h
@@ -8,6 +8,7 @@
 /* Address Translation Service */
 bool pci_ats_supported(struct pci_dev *dev);
 int pci_enable_ats(struct pci_dev *dev, int ps);
+int pci_ats_stu_configure(struct pci_dev *dev, int ps);
 void pci_disable_ats(struct pci_dev *dev);
 int pci_ats_queue_depth(struct pci_dev *dev);
 int pci_ats_page_aligned(struct pci_dev *dev);
-- 
2.38.1

[PATCH 1/2] PCI/ATS: Add a helper function to configure ATS STU of a PF.

[Ganapatrao Kulkarni]

As per PCI specification (PCI Express Base Specification Revision
6.0, Section 10.5) both PF and VFs of a PCI EP are permitted to be enabled
independently for ATS capability, however the STU(Smallest Translation
Unit) is shared between PF and VFs. For VFs, it is hardwired to Zero and
the associated PF's value applies to VFs.

In the current code, the STU is being configured while enabling the PF ATS.
Hence, it is not able to enable ATS for VFs, if it is not enabled on the
associated PF already.

Adding a function pci_ats_stu_configure(), which can be called to
configure the STU during PF enumeration.
Latter enumerations of VFs can successfully enable ATS independently.

Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
---
 drivers/pci/ats.c       | 32 ++++++++++++++++++++++++++++++--
 include/linux/pci-ats.h |  1 +
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index f9cc2e10b676..70e1982efdb4 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -46,6 +46,34 @@ bool pci_ats_supported(struct pci_dev *dev)
 }
 EXPORT_SYMBOL_GPL(pci_ats_supported);
 
+/**
+ * pci_ats_stu_configure - Configure STU of a PF.
+ * @dev: the PCI device
+ * @ps: the IOMMU page shift
+ *
+ * Returns 0 on success, or negative on failure.
+ */
+int pci_ats_stu_configure(struct pci_dev *dev, int ps)
+{
+	u16 ctrl;
+
+	if (dev->ats_enabled || dev->is_virtfn)
+		return 0;
+
+	if (!pci_ats_supported(dev))
+		return -EINVAL;
+
+	if (ps < PCI_ATS_MIN_STU)
+		return -EINVAL;
+
+	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_ats_stu_configure);
+
 /**
  * pci_enable_ats - enable the ATS capability
  * @dev: the PCI device
@@ -68,8 +96,8 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
 		return -EINVAL;
 
 	/*
-	 * Note that enabling ATS on a VF fails unless it's already enabled
-	 * with the same STU on the PF.
+	 * Note that enabling ATS on a VF fails unless it's already
+	 * configured with the same STU on the PF.
 	 */
 	ctrl = PCI_ATS_CTRL_ENABLE;
 	if (dev->is_virtfn) {
diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
index df54cd5b15db..9b40eb555124 100644
--- a/include/linux/pci-ats.h
+++ b/include/linux/pci-ats.h
@@ -8,6 +8,7 @@
 /* Address Translation Service */
 bool pci_ats_supported(struct pci_dev *dev);
 int pci_enable_ats(struct pci_dev *dev, int ps);
+int pci_ats_stu_configure(struct pci_dev *dev, int ps);
 void pci_disable_ats(struct pci_dev *dev);
 int pci_ats_queue_depth(struct pci_dev *dev);
 int pci_ats_page_aligned(struct pci_dev *dev);
-- 
2.38.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] iommu/arm-smmu-v3: Configure STU of a PF if ATS is not enabled.

[Ganapatrao Kulkarni]

When the host kernel is booted with iommu passthrough mode, PF and VFs
are enumerated with iommu/smmu domain set to bypass mode.
In bypass mode, ATS is not enabled on all VFs and associated PF.
When VFs are attached to a VM, the corresponding iommu domain is set to
SMMU translation mode and smmu-v3 driver try to enable the ATS and fails
due to invalid STU of a PF.

Adding a fix to configure STU of a PF while enumerating in passthrough
mode.

Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 25 ++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index f2425b0f0cd6..b218ef0bf001 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2292,6 +2292,23 @@ static bool arm_smmu_ats_supported(struct arm_smmu_master *master)
 	return dev_is_pci(dev) && pci_ats_supported(to_pci_dev(dev));
 }
 
+static void arm_smmu_ats_stu_init(struct arm_smmu_master *master)
+{
+	size_t stu;
+	struct pci_dev *pdev;
+	struct arm_smmu_device *smmu = master->smmu;
+
+	if (master->ats_enabled)
+		return;
+
+	/* Smallest Translation Unit: log2 of the smallest supported granule */
+	stu = __ffs(smmu->pgsize_bitmap);
+	pdev = to_pci_dev(master->dev);
+
+	if (pci_ats_stu_configure(pdev, stu))
+		dev_err(master->dev, "Failed to configure ATS STU (%zu)\n", stu);
+}
+
 static void arm_smmu_enable_ats(struct arm_smmu_master *master)
 {
 	size_t stu;
@@ -2404,6 +2421,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	struct arm_smmu_device *smmu;
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 	struct arm_smmu_master *master;
+	bool ats_supported;
 
 	if (!fwspec)
 		return -ENOENT;
@@ -2446,9 +2464,10 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	}
 
 	master->domain = smmu_domain;
+	ats_supported = arm_smmu_ats_supported(master);
 
 	if (smmu_domain->stage != ARM_SMMU_DOMAIN_BYPASS)
-		master->ats_enabled = arm_smmu_ats_supported(master);
+		master->ats_enabled = ats_supported;
 
 	arm_smmu_install_ste_for_dev(master);
 
@@ -2458,6 +2477,10 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 
 	arm_smmu_enable_ats(master);
 
+	/* Configure ATS STU of a PF in passthrough */
+	if (!master->ats_enabled && ats_supported)
+		arm_smmu_ats_stu_init(master);
+
 out_unlock:
 	mutex_unlock(&smmu_domain->init_mutex);
 	return ret;
-- 
2.38.1

[PATCH 2/2] iommu/arm-smmu-v3: Configure STU of a PF if ATS is not enabled.

[Ganapatrao Kulkarni]

When the host kernel is booted with iommu passthrough mode, PF and VFs
are enumerated with iommu/smmu domain set to bypass mode.
In bypass mode, ATS is not enabled on all VFs and associated PF.
When VFs are attached to a VM, the corresponding iommu domain is set to
SMMU translation mode and smmu-v3 driver try to enable the ATS and fails
due to invalid STU of a PF.

Adding a fix to configure STU of a PF while enumerating in passthrough
mode.

Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 25 ++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index f2425b0f0cd6..b218ef0bf001 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2292,6 +2292,23 @@ static bool arm_smmu_ats_supported(struct arm_smmu_master *master)
 	return dev_is_pci(dev) && pci_ats_supported(to_pci_dev(dev));
 }
 
+static void arm_smmu_ats_stu_init(struct arm_smmu_master *master)
+{
+	size_t stu;
+	struct pci_dev *pdev;
+	struct arm_smmu_device *smmu = master->smmu;
+
+	if (master->ats_enabled)
+		return;
+
+	/* Smallest Translation Unit: log2 of the smallest supported granule */
+	stu = __ffs(smmu->pgsize_bitmap);
+	pdev = to_pci_dev(master->dev);
+
+	if (pci_ats_stu_configure(pdev, stu))
+		dev_err(master->dev, "Failed to configure ATS STU (%zu)\n", stu);
+}
+
 static void arm_smmu_enable_ats(struct arm_smmu_master *master)
 {
 	size_t stu;
@@ -2404,6 +2421,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	struct arm_smmu_device *smmu;
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 	struct arm_smmu_master *master;
+	bool ats_supported;
 
 	if (!fwspec)
 		return -ENOENT;
@@ -2446,9 +2464,10 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	}
 
 	master->domain = smmu_domain;
+	ats_supported = arm_smmu_ats_supported(master);
 
 	if (smmu_domain->stage != ARM_SMMU_DOMAIN_BYPASS)
-		master->ats_enabled = arm_smmu_ats_supported(master);
+		master->ats_enabled = ats_supported;
 
 	arm_smmu_install_ste_for_dev(master);
 
@@ -2458,6 +2477,10 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 
 	arm_smmu_enable_ats(master);
 
+	/* Configure ATS STU of a PF in passthrough */
+	if (!master->ats_enabled && ats_supported)
+		arm_smmu_ats_stu_init(master);
+
 out_unlock:
 	mutex_unlock(&smmu_domain->init_mutex);
 	return ret;
-- 
2.38.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] PCI/ATS: Add a helper function to configure ATS STU of a PF.

[Sathyanarayanan Kuppuswamy]

Hi,

On 2/27/23 5:21 AM, Ganapatrao Kulkarni wrote:
> As per PCI specification (PCI Express Base Specification Revision
> 6.0, Section 10.5) both PF and VFs of a PCI EP are permitted to be enabled
> independently for ATS capability, however the STU(Smallest Translation
> Unit) is shared between PF and VFs. For VFs, it is hardwired to Zero and
> the associated PF's value applies to VFs.
> 
> In the current code, the STU is being configured while enabling the PF ATS.
> Hence, it is not able to enable ATS for VFs, if it is not enabled on the
> associated PF already.> 
> Adding a function pci_ats_stu_configure(), which can be called to
> configure the STU during PF enumeration.
> Latter enumerations of VFs can successfully enable ATS independently.

Why not enable ATS in PF before enabling it in VF? Just updating STU of
PF and not enabling it seem odd.

> 
> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
> ---
>  drivers/pci/ats.c       | 32 ++++++++++++++++++++++++++++++--
>  include/linux/pci-ats.h |  1 +
>  2 files changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index f9cc2e10b676..70e1982efdb4 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -46,6 +46,34 @@ bool pci_ats_supported(struct pci_dev *dev)
>  }
>  EXPORT_SYMBOL_GPL(pci_ats_supported);
>  
> +/**
> + * pci_ats_stu_configure - Configure STU of a PF.
> + * @dev: the PCI device
> + * @ps: the IOMMU page shift
> + *
> + * Returns 0 on success, or negative on failure.
> + */
> +int pci_ats_stu_configure(struct pci_dev *dev, int ps)
> +{
> +	u16 ctrl;
> +
> +	if (dev->ats_enabled || dev->is_virtfn)
> +		return 0;
> +
> +	if (!pci_ats_supported(dev))
> +		return -EINVAL;
> +
> +	if (ps < PCI_ATS_MIN_STU)
> +		return -EINVAL;
> +
> +	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);

If you just want to update the STU, don't overwrite other fields.

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(pci_ats_stu_configure);
> +
>  /**
>   * pci_enable_ats - enable the ATS capability
>   * @dev: the PCI device
> @@ -68,8 +96,8 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
>  		return -EINVAL;
>  
>  	/*
> -	 * Note that enabling ATS on a VF fails unless it's already enabled
> -	 * with the same STU on the PF.
> +	 * Note that enabling ATS on a VF fails unless it's already
> +	 * configured with the same STU on the PF.
>  	 */
>  	ctrl = PCI_ATS_CTRL_ENABLE;
>  	if (dev->is_virtfn) {
> diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
> index df54cd5b15db..9b40eb555124 100644
> --- a/include/linux/pci-ats.h
> +++ b/include/linux/pci-ats.h
> @@ -8,6 +8,7 @@
>  /* Address Translation Service */
>  bool pci_ats_supported(struct pci_dev *dev);
>  int pci_enable_ats(struct pci_dev *dev, int ps);
> +int pci_ats_stu_configure(struct pci_dev *dev, int ps);

What about dummy declaration for !CONFIG_PCI_ATS case?

>  void pci_disable_ats(struct pci_dev *dev);
>  int pci_ats_queue_depth(struct pci_dev *dev);
>  int pci_ats_page_aligned(struct pci_dev *dev);

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH 1/2] PCI/ATS: Add a helper function to configure ATS STU of a PF.

[Sathyanarayanan Kuppuswamy]

Hi,

On 2/27/23 5:21 AM, Ganapatrao Kulkarni wrote:
> As per PCI specification (PCI Express Base Specification Revision
> 6.0, Section 10.5) both PF and VFs of a PCI EP are permitted to be enabled
> independently for ATS capability, however the STU(Smallest Translation
> Unit) is shared between PF and VFs. For VFs, it is hardwired to Zero and
> the associated PF's value applies to VFs.
> 
> In the current code, the STU is being configured while enabling the PF ATS.
> Hence, it is not able to enable ATS for VFs, if it is not enabled on the
> associated PF already.> 
> Adding a function pci_ats_stu_configure(), which can be called to
> configure the STU during PF enumeration.
> Latter enumerations of VFs can successfully enable ATS independently.

Why not enable ATS in PF before enabling it in VF? Just updating STU of
PF and not enabling it seem odd.

> 
> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
> ---
>  drivers/pci/ats.c       | 32 ++++++++++++++++++++++++++++++--
>  include/linux/pci-ats.h |  1 +
>  2 files changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index f9cc2e10b676..70e1982efdb4 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -46,6 +46,34 @@ bool pci_ats_supported(struct pci_dev *dev)
>  }
>  EXPORT_SYMBOL_GPL(pci_ats_supported);
>  
> +/**
> + * pci_ats_stu_configure - Configure STU of a PF.
> + * @dev: the PCI device
> + * @ps: the IOMMU page shift
> + *
> + * Returns 0 on success, or negative on failure.
> + */
> +int pci_ats_stu_configure(struct pci_dev *dev, int ps)
> +{
> +	u16 ctrl;
> +
> +	if (dev->ats_enabled || dev->is_virtfn)
> +		return 0;
> +
> +	if (!pci_ats_supported(dev))
> +		return -EINVAL;
> +
> +	if (ps < PCI_ATS_MIN_STU)
> +		return -EINVAL;
> +
> +	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);

If you just want to update the STU, don't overwrite other fields.

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(pci_ats_stu_configure);
> +
>  /**
>   * pci_enable_ats - enable the ATS capability
>   * @dev: the PCI device
> @@ -68,8 +96,8 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
>  		return -EINVAL;
>  
>  	/*
> -	 * Note that enabling ATS on a VF fails unless it's already enabled
> -	 * with the same STU on the PF.
> +	 * Note that enabling ATS on a VF fails unless it's already
> +	 * configured with the same STU on the PF.
>  	 */
>  	ctrl = PCI_ATS_CTRL_ENABLE;
>  	if (dev->is_virtfn) {
> diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
> index df54cd5b15db..9b40eb555124 100644
> --- a/include/linux/pci-ats.h
> +++ b/include/linux/pci-ats.h
> @@ -8,6 +8,7 @@
>  /* Address Translation Service */
>  bool pci_ats_supported(struct pci_dev *dev);
>  int pci_enable_ats(struct pci_dev *dev, int ps);
> +int pci_ats_stu_configure(struct pci_dev *dev, int ps);

What about dummy declaration for !CONFIG_PCI_ATS case?

>  void pci_disable_ats(struct pci_dev *dev);
>  int pci_ats_queue_depth(struct pci_dev *dev);
>  int pci_ats_page_aligned(struct pci_dev *dev);

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] PCI/ATS: Add a helper function to configure ATS STU of a PF.

[Ganapatrao Kulkarni]

On 28-02-2023 12:59 am, Sathyanarayanan Kuppuswamy wrote:
> Hi,
> 
> On 2/27/23 5:21 AM, Ganapatrao Kulkarni wrote:
>> As per PCI specification (PCI Express Base Specification Revision
>> 6.0, Section 10.5) both PF and VFs of a PCI EP are permitted to be enabled
>> independently for ATS capability, however the STU(Smallest Translation
>> Unit) is shared between PF and VFs. For VFs, it is hardwired to Zero and
>> the associated PF's value applies to VFs.
>>
>> In the current code, the STU is being configured while enabling the PF ATS.
>> Hence, it is not able to enable ATS for VFs, if it is not enabled on the
>> associated PF already.>
>> Adding a function pci_ats_stu_configure(), which can be called to
>> configure the STU during PF enumeration.
>> Latter enumerations of VFs can successfully enable ATS independently.
> 
> Why not enable ATS in PF before enabling it in VF? Just updating STU of
> PF and not enabling it seem odd.

More details are in PATCH 0/2 and 2/2.

Also, This was discussed at
https://lore.kernel.org/linux-arm-kernel/20230221154624.GA3701506@bhelgaas/T/

> 
>>
>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
>> ---
>>   drivers/pci/ats.c       | 32 ++++++++++++++++++++++++++++++--
>>   include/linux/pci-ats.h |  1 +
>>   2 files changed, 31 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
>> index f9cc2e10b676..70e1982efdb4 100644
>> --- a/drivers/pci/ats.c
>> +++ b/drivers/pci/ats.c
>> @@ -46,6 +46,34 @@ bool pci_ats_supported(struct pci_dev *dev)
>>   }
>>   EXPORT_SYMBOL_GPL(pci_ats_supported);
>>   
>> +/**
>> + * pci_ats_stu_configure - Configure STU of a PF.
>> + * @dev: the PCI device
>> + * @ps: the IOMMU page shift
>> + *
>> + * Returns 0 on success, or negative on failure.
>> + */
>> +int pci_ats_stu_configure(struct pci_dev *dev, int ps)
>> +{
>> +	u16 ctrl;
>> +
>> +	if (dev->ats_enabled || dev->is_virtfn)
>> +		return 0;
>> +
>> +	if (!pci_ats_supported(dev))
>> +		return -EINVAL;
>> +
>> +	if (ps < PCI_ATS_MIN_STU)
>> +		return -EINVAL;
>> +
>> +	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);
> 
> If you just want to update the STU, don't overwrite other fields.

Can be read modify write, but felt not necessary, since all other fields 
are at default value zero.

> 
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(pci_ats_stu_configure);
>> +
>>   /**
>>    * pci_enable_ats - enable the ATS capability
>>    * @dev: the PCI device
>> @@ -68,8 +96,8 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
>>   		return -EINVAL;
>>   
>>   	/*
>> -	 * Note that enabling ATS on a VF fails unless it's already enabled
>> -	 * with the same STU on the PF.
>> +	 * Note that enabling ATS on a VF fails unless it's already
>> +	 * configured with the same STU on the PF.
>>   	 */
>>   	ctrl = PCI_ATS_CTRL_ENABLE;
>>   	if (dev->is_virtfn) {
>> diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
>> index df54cd5b15db..9b40eb555124 100644
>> --- a/include/linux/pci-ats.h
>> +++ b/include/linux/pci-ats.h
>> @@ -8,6 +8,7 @@
>>   /* Address Translation Service */
>>   bool pci_ats_supported(struct pci_dev *dev);
>>   int pci_enable_ats(struct pci_dev *dev, int ps);
>> +int pci_ats_stu_configure(struct pci_dev *dev, int ps);
> 
> What about dummy declaration for !CONFIG_PCI_ATS case?

Thanks, I overlooked else case.
> 
>>   void pci_disable_ats(struct pci_dev *dev);
>>   int pci_ats_queue_depth(struct pci_dev *dev);
>>   int pci_ats_page_aligned(struct pci_dev *dev);
> 


Thanks,
Ganapat

Re: [PATCH 1/2] PCI/ATS: Add a helper function to configure ATS STU of a PF.

[Ganapatrao Kulkarni]

On 28-02-2023 12:59 am, Sathyanarayanan Kuppuswamy wrote:
> Hi,
> 
> On 2/27/23 5:21 AM, Ganapatrao Kulkarni wrote:
>> As per PCI specification (PCI Express Base Specification Revision
>> 6.0, Section 10.5) both PF and VFs of a PCI EP are permitted to be enabled
>> independently for ATS capability, however the STU(Smallest Translation
>> Unit) is shared between PF and VFs. For VFs, it is hardwired to Zero and
>> the associated PF's value applies to VFs.
>>
>> In the current code, the STU is being configured while enabling the PF ATS.
>> Hence, it is not able to enable ATS for VFs, if it is not enabled on the
>> associated PF already.>
>> Adding a function pci_ats_stu_configure(), which can be called to
>> configure the STU during PF enumeration.
>> Latter enumerations of VFs can successfully enable ATS independently.
> 
> Why not enable ATS in PF before enabling it in VF? Just updating STU of
> PF and not enabling it seem odd.

More details are in PATCH 0/2 and 2/2.

Also, This was discussed at
https://lore.kernel.org/linux-arm-kernel/20230221154624.GA3701506@bhelgaas/T/

> 
>>
>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
>> ---
>>   drivers/pci/ats.c       | 32 ++++++++++++++++++++++++++++++--
>>   include/linux/pci-ats.h |  1 +
>>   2 files changed, 31 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
>> index f9cc2e10b676..70e1982efdb4 100644
>> --- a/drivers/pci/ats.c
>> +++ b/drivers/pci/ats.c
>> @@ -46,6 +46,34 @@ bool pci_ats_supported(struct pci_dev *dev)
>>   }
>>   EXPORT_SYMBOL_GPL(pci_ats_supported);
>>   
>> +/**
>> + * pci_ats_stu_configure - Configure STU of a PF.
>> + * @dev: the PCI device
>> + * @ps: the IOMMU page shift
>> + *
>> + * Returns 0 on success, or negative on failure.
>> + */
>> +int pci_ats_stu_configure(struct pci_dev *dev, int ps)
>> +{
>> +	u16 ctrl;
>> +
>> +	if (dev->ats_enabled || dev->is_virtfn)
>> +		return 0;
>> +
>> +	if (!pci_ats_supported(dev))
>> +		return -EINVAL;
>> +
>> +	if (ps < PCI_ATS_MIN_STU)
>> +		return -EINVAL;
>> +
>> +	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);
> 
> If you just want to update the STU, don't overwrite other fields.

Can be read modify write, but felt not necessary, since all other fields 
are at default value zero.

> 
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(pci_ats_stu_configure);
>> +
>>   /**
>>    * pci_enable_ats - enable the ATS capability
>>    * @dev: the PCI device
>> @@ -68,8 +96,8 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
>>   		return -EINVAL;
>>   
>>   	/*
>> -	 * Note that enabling ATS on a VF fails unless it's already enabled
>> -	 * with the same STU on the PF.
>> +	 * Note that enabling ATS on a VF fails unless it's already
>> +	 * configured with the same STU on the PF.
>>   	 */
>>   	ctrl = PCI_ATS_CTRL_ENABLE;
>>   	if (dev->is_virtfn) {
>> diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
>> index df54cd5b15db..9b40eb555124 100644
>> --- a/include/linux/pci-ats.h
>> +++ b/include/linux/pci-ats.h
>> @@ -8,6 +8,7 @@
>>   /* Address Translation Service */
>>   bool pci_ats_supported(struct pci_dev *dev);
>>   int pci_enable_ats(struct pci_dev *dev, int ps);
>> +int pci_ats_stu_configure(struct pci_dev *dev, int ps);
> 
> What about dummy declaration for !CONFIG_PCI_ATS case?

Thanks, I overlooked else case.
> 
>>   void pci_disable_ats(struct pci_dev *dev);
>>   int pci_ats_queue_depth(struct pci_dev *dev);
>>   int pci_ats_page_aligned(struct pci_dev *dev);
> 


Thanks,
Ganapat

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] PCI/ATS: Add a helper function to configure ATS STU of a PF.

[Sathyanarayanan Kuppuswamy]

On 2/27/23 6:53 PM, Ganapatrao Kulkarni wrote:
> 
> 
> On 28-02-2023 12:59 am, Sathyanarayanan Kuppuswamy wrote:
>> Hi,
>>
>> On 2/27/23 5:21 AM, Ganapatrao Kulkarni wrote:
>>> As per PCI specification (PCI Express Base Specification Revision
>>> 6.0, Section 10.5) both PF and VFs of a PCI EP are permitted to be enabled
>>> independently for ATS capability, however the STU(Smallest Translation
>>> Unit) is shared between PF and VFs. For VFs, it is hardwired to Zero and
>>> the associated PF's value applies to VFs.
>>>
>>> In the current code, the STU is being configured while enabling the PF ATS.
>>> Hence, it is not able to enable ATS for VFs, if it is not enabled on the
>>> associated PF already.>
>>> Adding a function pci_ats_stu_configure(), which can be called to
>>> configure the STU during PF enumeration.
>>> Latter enumerations of VFs can successfully enable ATS independently.
>>
>> Why not enable ATS in PF before enabling it in VF? Just updating STU of
>> PF and not enabling it seem odd.
> 
> More details are in PATCH 0/2 and 2/2.
> 
> Also, This was discussed at
> https://lore.kernel.org/linux-arm-kernel/20230221154624.GA3701506@bhelgaas/T/
> 

I agree with Bjorn's comments. It is incorrect to directly configure PF
registers from VF enable function. It is a buggy fix.

My question is, why not ensure PF ATS is configured and enabled before enabling
ATS for VF. Anyway, PF device will be enumerated before VF, right?

>>
>>>
>>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
>>> ---
>>>   drivers/pci/ats.c       | 32 ++++++++++++++++++++++++++++++--
>>>   include/linux/pci-ats.h |  1 +
>>>   2 files changed, 31 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
>>> index f9cc2e10b676..70e1982efdb4 100644
>>> --- a/drivers/pci/ats.c
>>> +++ b/drivers/pci/ats.c
>>> @@ -46,6 +46,34 @@ bool pci_ats_supported(struct pci_dev *dev)
>>>   }
>>>   EXPORT_SYMBOL_GPL(pci_ats_supported);
>>>   +/**
>>> + * pci_ats_stu_configure - Configure STU of a PF.
>>> + * @dev: the PCI device
>>> + * @ps: the IOMMU page shift
>>> + *
>>> + * Returns 0 on success, or negative on failure.
>>> + */
>>> +int pci_ats_stu_configure(struct pci_dev *dev, int ps)
>>> +{
>>> +    u16 ctrl;
>>> +
>>> +    if (dev->ats_enabled || dev->is_virtfn)
>>> +        return 0;
>>> +
>>> +    if (!pci_ats_supported(dev))
>>> +        return -EINVAL;
>>> +
>>> +    if (ps < PCI_ATS_MIN_STU)
>>> +        return -EINVAL;
>>> +
>>> +    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);
>>
>> If you just want to update the STU, don't overwrite other fields.
> 
> Can be read modify write, but felt not necessary, since all other fields are at default value zero.

It may not always be true. If there is a case where AMAD and AMAE attribute values are
configured independently, then your change can overwrite it.

IMO, since your intention is to update STU, I recommend just updating it.

> 
>>
>>> +
>>> +    return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(pci_ats_stu_configure);
>>> +
>>>   /**
>>>    * pci_enable_ats - enable the ATS capability
>>>    * @dev: the PCI device
>>> @@ -68,8 +96,8 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
>>>           return -EINVAL;
>>>         /*
>>> -     * Note that enabling ATS on a VF fails unless it's already enabled
>>> -     * with the same STU on the PF.
>>> +     * Note that enabling ATS on a VF fails unless it's already
>>> +     * configured with the same STU on the PF.
>>>        */
>>>       ctrl = PCI_ATS_CTRL_ENABLE;
>>>       if (dev->is_virtfn) {
>>> diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
>>> index df54cd5b15db..9b40eb555124 100644
>>> --- a/include/linux/pci-ats.h
>>> +++ b/include/linux/pci-ats.h
>>> @@ -8,6 +8,7 @@
>>>   /* Address Translation Service */
>>>   bool pci_ats_supported(struct pci_dev *dev);
>>>   int pci_enable_ats(struct pci_dev *dev, int ps);
>>> +int pci_ats_stu_configure(struct pci_dev *dev, int ps);
>>
>> What about dummy declaration for !CONFIG_PCI_ATS case?
> 
> Thanks, I overlooked else case.
>>
>>>   void pci_disable_ats(struct pci_dev *dev);
>>>   int pci_ats_queue_depth(struct pci_dev *dev);
>>>   int pci_ats_page_aligned(struct pci_dev *dev);
>>
> 
> 
> Thanks,
> Ganapat

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH 1/2] PCI/ATS: Add a helper function to configure ATS STU of a PF.

[Sathyanarayanan Kuppuswamy]

On 2/27/23 6:53 PM, Ganapatrao Kulkarni wrote:
> 
> 
> On 28-02-2023 12:59 am, Sathyanarayanan Kuppuswamy wrote:
>> Hi,
>>
>> On 2/27/23 5:21 AM, Ganapatrao Kulkarni wrote:
>>> As per PCI specification (PCI Express Base Specification Revision
>>> 6.0, Section 10.5) both PF and VFs of a PCI EP are permitted to be enabled
>>> independently for ATS capability, however the STU(Smallest Translation
>>> Unit) is shared between PF and VFs. For VFs, it is hardwired to Zero and
>>> the associated PF's value applies to VFs.
>>>
>>> In the current code, the STU is being configured while enabling the PF ATS.
>>> Hence, it is not able to enable ATS for VFs, if it is not enabled on the
>>> associated PF already.>
>>> Adding a function pci_ats_stu_configure(), which can be called to
>>> configure the STU during PF enumeration.
>>> Latter enumerations of VFs can successfully enable ATS independently.
>>
>> Why not enable ATS in PF before enabling it in VF? Just updating STU of
>> PF and not enabling it seem odd.
> 
> More details are in PATCH 0/2 and 2/2.
> 
> Also, This was discussed at
> https://lore.kernel.org/linux-arm-kernel/20230221154624.GA3701506@bhelgaas/T/
> 

I agree with Bjorn's comments. It is incorrect to directly configure PF
registers from VF enable function. It is a buggy fix.

My question is, why not ensure PF ATS is configured and enabled before enabling
ATS for VF. Anyway, PF device will be enumerated before VF, right?

>>
>>>
>>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
>>> ---
>>>   drivers/pci/ats.c       | 32 ++++++++++++++++++++++++++++++--
>>>   include/linux/pci-ats.h |  1 +
>>>   2 files changed, 31 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
>>> index f9cc2e10b676..70e1982efdb4 100644
>>> --- a/drivers/pci/ats.c
>>> +++ b/drivers/pci/ats.c
>>> @@ -46,6 +46,34 @@ bool pci_ats_supported(struct pci_dev *dev)
>>>   }
>>>   EXPORT_SYMBOL_GPL(pci_ats_supported);
>>>   +/**
>>> + * pci_ats_stu_configure - Configure STU of a PF.
>>> + * @dev: the PCI device
>>> + * @ps: the IOMMU page shift
>>> + *
>>> + * Returns 0 on success, or negative on failure.
>>> + */
>>> +int pci_ats_stu_configure(struct pci_dev *dev, int ps)
>>> +{
>>> +    u16 ctrl;
>>> +
>>> +    if (dev->ats_enabled || dev->is_virtfn)
>>> +        return 0;
>>> +
>>> +    if (!pci_ats_supported(dev))
>>> +        return -EINVAL;
>>> +
>>> +    if (ps < PCI_ATS_MIN_STU)
>>> +        return -EINVAL;
>>> +
>>> +    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);
>>
>> If you just want to update the STU, don't overwrite other fields.
> 
> Can be read modify write, but felt not necessary, since all other fields are at default value zero.

It may not always be true. If there is a case where AMAD and AMAE attribute values are
configured independently, then your change can overwrite it.

IMO, since your intention is to update STU, I recommend just updating it.

> 
>>
>>> +
>>> +    return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(pci_ats_stu_configure);
>>> +
>>>   /**
>>>    * pci_enable_ats - enable the ATS capability
>>>    * @dev: the PCI device
>>> @@ -68,8 +96,8 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
>>>           return -EINVAL;
>>>         /*
>>> -     * Note that enabling ATS on a VF fails unless it's already enabled
>>> -     * with the same STU on the PF.
>>> +     * Note that enabling ATS on a VF fails unless it's already
>>> +     * configured with the same STU on the PF.
>>>        */
>>>       ctrl = PCI_ATS_CTRL_ENABLE;
>>>       if (dev->is_virtfn) {
>>> diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
>>> index df54cd5b15db..9b40eb555124 100644
>>> --- a/include/linux/pci-ats.h
>>> +++ b/include/linux/pci-ats.h
>>> @@ -8,6 +8,7 @@
>>>   /* Address Translation Service */
>>>   bool pci_ats_supported(struct pci_dev *dev);
>>>   int pci_enable_ats(struct pci_dev *dev, int ps);
>>> +int pci_ats_stu_configure(struct pci_dev *dev, int ps);
>>
>> What about dummy declaration for !CONFIG_PCI_ATS case?
> 
> Thanks, I overlooked else case.
>>
>>>   void pci_disable_ats(struct pci_dev *dev);
>>>   int pci_ats_queue_depth(struct pci_dev *dev);
>>>   int pci_ats_page_aligned(struct pci_dev *dev);
>>
> 
> 
> Thanks,
> Ganapat

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] PCI/ATS: Add a helper function to configure ATS STU of a PF.

[Ganapatrao Kulkarni]

On 28-02-2023 08:38 am, Sathyanarayanan Kuppuswamy wrote:
> 
> 
> On 2/27/23 6:53 PM, Ganapatrao Kulkarni wrote:
>>
>>
>> On 28-02-2023 12:59 am, Sathyanarayanan Kuppuswamy wrote:
>>> Hi,
>>>
>>> On 2/27/23 5:21 AM, Ganapatrao Kulkarni wrote:
>>>> As per PCI specification (PCI Express Base Specification Revision
>>>> 6.0, Section 10.5) both PF and VFs of a PCI EP are permitted to be enabled
>>>> independently for ATS capability, however the STU(Smallest Translation
>>>> Unit) is shared between PF and VFs. For VFs, it is hardwired to Zero and
>>>> the associated PF's value applies to VFs.
>>>>
>>>> In the current code, the STU is being configured while enabling the PF ATS.
>>>> Hence, it is not able to enable ATS for VFs, if it is not enabled on the
>>>> associated PF already.>
>>>> Adding a function pci_ats_stu_configure(), which can be called to
>>>> configure the STU during PF enumeration.
>>>> Latter enumerations of VFs can successfully enable ATS independently.
>>>
>>> Why not enable ATS in PF before enabling it in VF? Just updating STU of
>>> PF and not enabling it seem odd.
>>
>> More details are in PATCH 0/2 and 2/2.
>>
>> Also, This was discussed at
>> https://lore.kernel.org/linux-arm-kernel/20230221154624.GA3701506@bhelgaas/T/
>>
> 
> I agree with Bjorn's comments. It is incorrect to directly configure PF
> registers from VF enable function. It is a buggy fix.
> 
> My question is, why not ensure PF ATS is configured and enabled before enabling
> ATS for VF. Anyway, PF device will be enumerated before VF, right?

Yes, however ATS is not enabled in passthrough.
When ATS is not enabled, this new helper can be called to init STU since 
it is shared with VFs.
> 
>>>
>>>>
>>>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
>>>> ---
>>>>    drivers/pci/ats.c       | 32 ++++++++++++++++++++++++++++++--
>>>>    include/linux/pci-ats.h |  1 +
>>>>    2 files changed, 31 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
>>>> index f9cc2e10b676..70e1982efdb4 100644
>>>> --- a/drivers/pci/ats.c
>>>> +++ b/drivers/pci/ats.c
>>>> @@ -46,6 +46,34 @@ bool pci_ats_supported(struct pci_dev *dev)
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(pci_ats_supported);
>>>>    +/**
>>>> + * pci_ats_stu_configure - Configure STU of a PF.
>>>> + * @dev: the PCI device
>>>> + * @ps: the IOMMU page shift
>>>> + *
>>>> + * Returns 0 on success, or negative on failure.
>>>> + */
>>>> +int pci_ats_stu_configure(struct pci_dev *dev, int ps)
>>>> +{
>>>> +    u16 ctrl;
>>>> +
>>>> +    if (dev->ats_enabled || dev->is_virtfn)
>>>> +        return 0;
>>>> +
>>>> +    if (!pci_ats_supported(dev))
>>>> +        return -EINVAL;
>>>> +
>>>> +    if (ps < PCI_ATS_MIN_STU)
>>>> +        return -EINVAL;
>>>> +
>>>> +    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);
>>>
>>> If you just want to update the STU, don't overwrite other fields.
>>
>> Can be read modify write, but felt not necessary, since all other fields are at default value zero.
> 
> It may not always be true. If there is a case where AMAD and AMAE attribute values are
> configured independently, then your change can overwrite it.
> 
> IMO, since your intention is to update STU, I recommend just updating it.

OK, I will change to it read-modify-write.
> 
>>
>>>
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(pci_ats_stu_configure);
>>>> +
>>>>    /**
>>>>     * pci_enable_ats - enable the ATS capability
>>>>     * @dev: the PCI device
>>>> @@ -68,8 +96,8 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
>>>>            return -EINVAL;
>>>>          /*
>>>> -     * Note that enabling ATS on a VF fails unless it's already enabled
>>>> -     * with the same STU on the PF.
>>>> +     * Note that enabling ATS on a VF fails unless it's already
>>>> +     * configured with the same STU on the PF.
>>>>         */
>>>>        ctrl = PCI_ATS_CTRL_ENABLE;
>>>>        if (dev->is_virtfn) {
>>>> diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
>>>> index df54cd5b15db..9b40eb555124 100644
>>>> --- a/include/linux/pci-ats.h
>>>> +++ b/include/linux/pci-ats.h
>>>> @@ -8,6 +8,7 @@
>>>>    /* Address Translation Service */
>>>>    bool pci_ats_supported(struct pci_dev *dev);
>>>>    int pci_enable_ats(struct pci_dev *dev, int ps);
>>>> +int pci_ats_stu_configure(struct pci_dev *dev, int ps);
>>>
>>> What about dummy declaration for !CONFIG_PCI_ATS case?
>>
>> Thanks, I overlooked else case.
>>>
>>>>    void pci_disable_ats(struct pci_dev *dev);
>>>>    int pci_ats_queue_depth(struct pci_dev *dev);
>>>>    int pci_ats_page_aligned(struct pci_dev *dev);
>>>
>>
>>
>> Thanks,
>> Ganapat
> 

Thanks,
Ganapat

Re: [PATCH 1/2] PCI/ATS: Add a helper function to configure ATS STU of a PF.

[Ganapatrao Kulkarni]

On 28-02-2023 08:38 am, Sathyanarayanan Kuppuswamy wrote:
> 
> 
> On 2/27/23 6:53 PM, Ganapatrao Kulkarni wrote:
>>
>>
>> On 28-02-2023 12:59 am, Sathyanarayanan Kuppuswamy wrote:
>>> Hi,
>>>
>>> On 2/27/23 5:21 AM, Ganapatrao Kulkarni wrote:
>>>> As per PCI specification (PCI Express Base Specification Revision
>>>> 6.0, Section 10.5) both PF and VFs of a PCI EP are permitted to be enabled
>>>> independently for ATS capability, however the STU(Smallest Translation
>>>> Unit) is shared between PF and VFs. For VFs, it is hardwired to Zero and
>>>> the associated PF's value applies to VFs.
>>>>
>>>> In the current code, the STU is being configured while enabling the PF ATS.
>>>> Hence, it is not able to enable ATS for VFs, if it is not enabled on the
>>>> associated PF already.>
>>>> Adding a function pci_ats_stu_configure(), which can be called to
>>>> configure the STU during PF enumeration.
>>>> Latter enumerations of VFs can successfully enable ATS independently.
>>>
>>> Why not enable ATS in PF before enabling it in VF? Just updating STU of
>>> PF and not enabling it seem odd.
>>
>> More details are in PATCH 0/2 and 2/2.
>>
>> Also, This was discussed at
>> https://lore.kernel.org/linux-arm-kernel/20230221154624.GA3701506@bhelgaas/T/
>>
> 
> I agree with Bjorn's comments. It is incorrect to directly configure PF
> registers from VF enable function. It is a buggy fix.
> 
> My question is, why not ensure PF ATS is configured and enabled before enabling
> ATS for VF. Anyway, PF device will be enumerated before VF, right?

Yes, however ATS is not enabled in passthrough.
When ATS is not enabled, this new helper can be called to init STU since 
it is shared with VFs.
> 
>>>
>>>>
>>>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
>>>> ---
>>>>    drivers/pci/ats.c       | 32 ++++++++++++++++++++++++++++++--
>>>>    include/linux/pci-ats.h |  1 +
>>>>    2 files changed, 31 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
>>>> index f9cc2e10b676..70e1982efdb4 100644
>>>> --- a/drivers/pci/ats.c
>>>> +++ b/drivers/pci/ats.c
>>>> @@ -46,6 +46,34 @@ bool pci_ats_supported(struct pci_dev *dev)
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(pci_ats_supported);
>>>>    +/**
>>>> + * pci_ats_stu_configure - Configure STU of a PF.
>>>> + * @dev: the PCI device
>>>> + * @ps: the IOMMU page shift
>>>> + *
>>>> + * Returns 0 on success, or negative on failure.
>>>> + */
>>>> +int pci_ats_stu_configure(struct pci_dev *dev, int ps)
>>>> +{
>>>> +    u16 ctrl;
>>>> +
>>>> +    if (dev->ats_enabled || dev->is_virtfn)
>>>> +        return 0;
>>>> +
>>>> +    if (!pci_ats_supported(dev))
>>>> +        return -EINVAL;
>>>> +
>>>> +    if (ps < PCI_ATS_MIN_STU)
>>>> +        return -EINVAL;
>>>> +
>>>> +    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);
>>>
>>> If you just want to update the STU, don't overwrite other fields.
>>
>> Can be read modify write, but felt not necessary, since all other fields are at default value zero.
> 
> It may not always be true. If there is a case where AMAD and AMAE attribute values are
> configured independently, then your change can overwrite it.
> 
> IMO, since your intention is to update STU, I recommend just updating it.

OK, I will change to it read-modify-write.
> 
>>
>>>
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(pci_ats_stu_configure);
>>>> +
>>>>    /**
>>>>     * pci_enable_ats - enable the ATS capability
>>>>     * @dev: the PCI device
>>>> @@ -68,8 +96,8 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
>>>>            return -EINVAL;
>>>>          /*
>>>> -     * Note that enabling ATS on a VF fails unless it's already enabled
>>>> -     * with the same STU on the PF.
>>>> +     * Note that enabling ATS on a VF fails unless it's already
>>>> +     * configured with the same STU on the PF.
>>>>         */
>>>>        ctrl = PCI_ATS_CTRL_ENABLE;
>>>>        if (dev->is_virtfn) {
>>>> diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
>>>> index df54cd5b15db..9b40eb555124 100644
>>>> --- a/include/linux/pci-ats.h
>>>> +++ b/include/linux/pci-ats.h
>>>> @@ -8,6 +8,7 @@
>>>>    /* Address Translation Service */
>>>>    bool pci_ats_supported(struct pci_dev *dev);
>>>>    int pci_enable_ats(struct pci_dev *dev, int ps);
>>>> +int pci_ats_stu_configure(struct pci_dev *dev, int ps);
>>>
>>> What about dummy declaration for !CONFIG_PCI_ATS case?
>>
>> Thanks, I overlooked else case.
>>>
>>>>    void pci_disable_ats(struct pci_dev *dev);
>>>>    int pci_ats_queue_depth(struct pci_dev *dev);
>>>>    int pci_ats_page_aligned(struct pci_dev *dev);
>>>
>>
>>
>> Thanks,
>> Ganapat
> 

Thanks,
Ganapat

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel