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

From: Ganapatrao Kulkarni
Date: Tue Mar 14 2023 - 06:08:41 EST



On 14-03-2023 04:00 am, Sathyanarayanan Kuppuswamy wrote:
Hi Kulkarni,

On 3/13/23 2:12 PM, Bjorn Helgaas wrote:
On Mon, Feb 27, 2023 at 08:21:36PM -0800, 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.

s/STU(Smallest/STU (Smallest/ (add space before paren)
s/Adding a function pci_ats_stu_configure()/Add pci_ats_stu_configure()/
s/Latter/Subsequent/

Add blank line between paragraphs (it looks like "Latter enumerations"
is intended to start a new paragraph).

Signed-off-by: Ganapatrao Kulkarni <gankulkarni@xxxxxxxxxxxxxxxxxxxxxx>

Acked-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>

Given an ack for the IOMMU patch, I'd be happy to merge both (and I
can do the commit log tweaks); just let me know.

One comment/question below.

---
drivers/pci/ats.c | 33 +++++++++++++++++++++++++++++++--
include/linux/pci-ats.h | 3 +++
2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index f9cc2e10b676..1611bfa1d5da 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -46,6 +46,35 @@ 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;

I might return an error for the VF case on the assumption that it's
likely an error in the caller. I guess one could argue that it
simplifies the caller if it doesn't have to check for PF vs VF. But
the fact that STU is shared between PF and VFs is an important part of
understanding how ATS works, so the caller should be aware of the
distinction anyway.

I have already asked this question. But let me repeat it.

We don't have any checks for the PF case here. That means you can re-configure
the STU as many times as you want until ATS is enabled in PF. So, if there are
active VFs which uses this STU, can PF re-configure the STU at will?


IMO, Since STU is shared, programming it multiple times is not expected from callers code do it, however we can add below check to allow to program STU once from a PF.

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index 1611bfa1d5da..f7bb01068e18 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -60,6 +60,10 @@ int pci_ats_stu_configure(struct pci_dev *dev, int ps)
if (dev->ats_enabled || dev->is_virtfn)
return 0;

+ /* Configured already */
+ if (dev->ats_stu)
+ return 0;
+
if (!pci_ats_supported(dev))
return -EINVAL;

+
+ if (!pci_ats_supported(dev))
+ return -EINVAL;
+
+ if (ps < PCI_ATS_MIN_STU)
+ return -EINVAL;
+
+ dev->ats_stu = ps;
+ pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, &ctrl);
+ 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 +97,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..7d62a92aaf23 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);
@@ -16,6 +17,8 @@ static inline bool pci_ats_supported(struct pci_dev *d)
{ return false; }
static inline int pci_enable_ats(struct pci_dev *d, int ps)
{ return -ENODEV; }
+static inline int pci_ats_stu_configure(struct pci_dev *d, int ps)
+{ return -ENODEV; }
static inline void pci_disable_ats(struct pci_dev *d) { }
static inline int pci_ats_queue_depth(struct pci_dev *d)
{ return -ENODEV; }
--
2.38.1


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


Thanks,
Ganapat