Re: [PATCH v1 1/1] PCI/ATS: Optimize pci_prg_resp_pasid_required() function

From: Kuppuswamy Sathyanarayanan
Date: Wed Oct 09 2019 - 18:53:40 EST

Next message: Bjorn Helgaas: "[PATCH 0/3] PCI/ATS: Clean up unnecessary stubs and exports"
Previous message: Paolo Bonzini: "Re: [PATCH AUTOSEL 5.3 28/68] KVM: x86: Expose XSAVEERPTR to the guest"
In reply to: Bjorn Helgaas: "Re: [PATCH v1 1/1] PCI/ATS: Optimize pci_prg_resp_pasid_required() function"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Bjorn,

Thanks for the review.

On 10/9/19 3:30 PM, Bjorn Helgaas wrote:

On Mon, Oct 07, 2019 at 04:32:42PM -0700, sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx wrote:

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>

Currently, pci_prg_resp_pasid_required() function reads the
PASID Required bit status from register every time we call
the function. Since PASID Required bit is a read-only value,
instead of reading it from register every time, read it once and
cache it in struct pci_dev.

Also, since we are caching PASID Required bit in pci_pri_init()
function, move the caching of PRI Capability check result to the same
function. This will group all PRI related caching at one place.

Since "pasid_required" structure member is protected by CONFIG_PRI,
its users should also be protected by same #ifdef. So correct the #ifdef
dependency of pci_prg_resp_pasid_required() function.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
Cc: Ashok Raj <ashok.raj@xxxxxxxxx>
Cc: Keith Busch <keith.busch@xxxxxxxxx>
---
drivers/pci/ats.c | 50 ++++++++++++++++++++++++---------------------
include/linux/pci.h | 1 +
2 files changed, 28 insertions(+), 23 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index cb4f62da7b8a..2b5df5ea208f 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -16,6 +16,24 @@
#include "pci.h"
+static void pci_pri_init(struct pci_dev *pdev)
+{
+#ifdef CONFIG_PCI_PRI
+ int pos;
+ u16 status;
+
+ pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
+ if (!pos)
+ return;
+
+ pdev->pri_cap = pos;
+
+ pci_read_config_word(pdev, pos + PCI_PRI_STATUS, &status);
+ if (status & PCI_PRI_STATUS_PASID)
+ pdev->pasid_required = 1;
+#endif
+}

I like this patch a lot but:

1) You started out with a pci_pri_init(), and I screwed up when I
suggested that you remove it. I think it makes a lot of sense to have
it and call it directly from pci_init_capabilities() just like we do
for other capabilities.

Yes, we could do that. But, I thought may be there is a history for grouping
all PRI/PASID related code in ats.c. That's why I did not move PRI/PASID
related init code outside ats.c. If its not an issue, having it in pci_init_capabiliies() is
more appropriate.

2) The PCI_PRI/PCI_PASID #ifdef thing is still a mess. Despite the
fact that its name contains "pasid", pci_prg_resp_pasid_required()
only looks at the PRI capability, so I think it should be under #ifdef
CONFIG_PCI_PRI along with the other PRI stuff.

3) I'm not sure why pci_prg_resp_pasid_required() was under
CONFIG_PCI_PASID, but it might be related to the fact that it's called
from code in intel-iommu.c where CONFIG_PCI_PASID is always defined,
but CONFIG_PCI_PRI may not be.

Main reason is, this function will only be used if PASID is enabled. Please check the following
code snippet from intel-iommu.c

1418ÂÂÂÂÂÂÂÂ if (info->pasid_supported && !pci_enable_pasid(pdev, info->pasid_supported & ~1))
1419ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ info->pasid_enabled = 1;
1420
1421ÂÂÂÂÂÂÂÂ if (info->pri_supported &&
1422ÂÂÂÂÂÂÂÂÂÂÂÂ (info->pasid_enabled ? pci_prg_resp_pasid_required(pdev) : 1)Â &&
1423ÂÂÂÂÂÂÂÂÂÂÂÂ !pci_reset_pri(pdev) && !pci_enable_pri(pdev, 32))
1424ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ info->pri_enabled = 1;

After looking at this code again, since there is no compile time dependency, we could just move
the definition of pri_prg_resp_pasid_required() function under CONFIG_PCI_PRI. it should not be a
problem.

I think this is an intel-iommu Kconfig
defect. I'll post patches to change the Kconfig and to move
pci_prg_resp_pasid_required() under CONFIG_PCI_PRI.

So I fiddled with all this and updated my pci/virtualization branch
with the result.

The interdiff from the v8 series is below. This includes the
intel-iommu Kconfig and pci_prg_resp_pasid_required() changes (which I
haven't posted yet), and the branch includes some unrelated follow-on
patches (which I also haven't posted yet). Let me know what you
think.

It looks good to me. Please go ahead.

Bjorn

void pci_ats_init(struct pci_dev *dev)
{
int pos;
@@ -28,6 +46,8 @@ void pci_ats_init(struct pci_dev *dev)
return;
dev->ats_cap = pos;
+
+ pci_pri_init(dev);
}
/**
@@ -185,12 +205,8 @@ int pci_enable_pri(struct pci_dev *pdev, u32 reqs)
if (WARN_ON(pdev->pri_enabled))
return -EBUSY;
- if (!pri) {
- pri = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
- if (!pri)
- return -EINVAL;
- pdev->pri_cap = pri;
- }
+ if (!pri)
+ return -EINVAL;
pci_read_config_word(pdev, pri + PCI_PRI_STATUS, &status);
if (!(status & PCI_PRI_STATUS_STOPPED))
@@ -425,6 +441,7 @@ int pci_pasid_features(struct pci_dev *pdev)
}
EXPORT_SYMBOL_GPL(pci_pasid_features);
+#ifdef CONFIG_PCI_PRI
/**
* pci_prg_resp_pasid_required - Return PRG Response PASID Required bit
* status.
@@ -432,31 +449,18 @@ EXPORT_SYMBOL_GPL(pci_pasid_features);
*
* Returns 1 if PASID is required in PRG Response Message, 0 otherwise.
*
- * Even though the PRG response PASID status is read from PRI Status
- * Register, since this API will mainly be used by PASID users, this
- * function is defined within #ifdef CONFIG_PCI_PASID instead of
- * CONFIG_PCI_PRI.
+ * Since this API has dependency on both PRI and PASID, protect it
+ * with both CONFIG_PCI_PRI and CONFIG_PCI_PASID.
*/
int pci_prg_resp_pasid_required(struct pci_dev *pdev)
{
- u16 status;
- int pri;
-
if (pdev->is_virtfn)
pdev = pci_physfn(pdev);
- pri = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
- if (!pri)
- return 0;
-
- pci_read_config_word(pdev, pri + PCI_PRI_STATUS, &status);
-
- if (status & PCI_PRI_STATUS_PASID)
- return 1;
-
- return 0;
+ return pdev->pasid_required;
}
EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required);
+#endif /* CONFIG_PCI_PRI */
#define PASID_NUMBER_SHIFT 8
#define PASID_NUMBER_MASK (0x1f << PASID_NUMBER_SHIFT)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 6542100bd2dd..f1131fee7fcd 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -456,6 +456,7 @@ struct pci_dev {
#ifdef CONFIG_PCI_PRI
u16 pri_cap; /* PRI Capability offset */
u32 pri_reqs_alloc; /* Number of PRI requests allocated */
+ unsigned int pasid_required:1; /* PRG Response PASID Required bit status */
#endif
#ifdef CONFIG_PCI_PASID
u16 pasid_cap; /* PASID Capability offset */
--
2.21.0

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index e3842eabcfdd..b183c9f916b0 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -207,6 +207,7 @@ config INTEL_IOMMU_SVM
bool "Support for Shared Virtual Memory with Intel IOMMU"
depends on INTEL_IOMMU && X86
select PCI_PASID
+ select PCI_PRI
select MMU_NOTIFIER
help
Shared Virtual Memory (SVM) provides a facility for devices
diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index cb4f62da7b8a..76ae518d55f4 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -159,6 +159,20 @@ int pci_ats_page_aligned(struct pci_dev *pdev)
EXPORT_SYMBOL_GPL(pci_ats_page_aligned);
#ifdef CONFIG_PCI_PRI
+void pci_pri_init(struct pci_dev *pdev)
+{
+ u16 status;
+
+ pdev->pri_cap = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
+
+ if (!pdev->pri_cap)
+ return;
+
+ pci_read_config_word(pdev, pdev->pri_cap + PCI_PRI_STATUS, &status);
+ if (status & PCI_PRI_STATUS_PASID)
+ pdev->pasid_required = 1;
+}
+
/**
* pci_enable_pri - Enable PRI capability
* @ pdev: PCI device structure
@@ -185,12 +199,8 @@ int pci_enable_pri(struct pci_dev *pdev, u32 reqs)
if (WARN_ON(pdev->pri_enabled))
return -EBUSY;
- if (!pri) {
- pri = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
- if (!pri)
- return -EINVAL;
- pdev->pri_cap = pri;
- }
+ if (!pri)
+ return -EINVAL;
pci_read_config_word(pdev, pri + PCI_PRI_STATUS, &status);
if (!(status & PCI_PRI_STATUS_STOPPED))
@@ -290,9 +300,30 @@ int pci_reset_pri(struct pci_dev *pdev)
return 0;
}
EXPORT_SYMBOL_GPL(pci_reset_pri);
+
+/**
+ * pci_prg_resp_pasid_required - Return PRG Response PASID Required bit
+ * status.
+ * @pdev: PCI device structure
+ *
+ * Returns 1 if PASID is required in PRG Response Message, 0 otherwise.
+ */
+int pci_prg_resp_pasid_required(struct pci_dev *pdev)
+{
+ if (pdev->is_virtfn)
+ pdev = pci_physfn(pdev);
+
+ return pdev->pasid_required;
+}
+EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required);
#endif /* CONFIG_PCI_PRI */
#ifdef CONFIG_PCI_PASID
+void pci_pasid_init(struct pci_dev *pdev)
+{
+ pdev->pasid_cap = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PASID);
+}
+
/**
* pci_enable_pasid - Enable the PASID capability
* @pdev: PCI device structure
@@ -323,12 +354,8 @@ int pci_enable_pasid(struct pci_dev *pdev, int features)
if (!pdev->eetlp_prefix_path)
return -EINVAL;
- if (!pasid) {
- pasid = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PASID);
- if (!pasid)
- return -EINVAL;
- pdev->pasid_cap = pasid;
- }
+ if (!pasid)
+ return -EINVAL;
pci_read_config_word(pdev, pasid + PCI_PASID_CAP, &supported);
supported &= PCI_PASID_CAP_EXEC | PCI_PASID_CAP_PRIV;
@@ -425,39 +452,6 @@ int pci_pasid_features(struct pci_dev *pdev)
}
EXPORT_SYMBOL_GPL(pci_pasid_features);
-/**
- * pci_prg_resp_pasid_required - Return PRG Response PASID Required bit
- * status.
- * @pdev: PCI device structure
- *
- * Returns 1 if PASID is required in PRG Response Message, 0 otherwise.
- *
- * Even though the PRG response PASID status is read from PRI Status
- * Register, since this API will mainly be used by PASID users, this
- * function is defined within #ifdef CONFIG_PCI_PASID instead of
- * CONFIG_PCI_PRI.
- */
-int pci_prg_resp_pasid_required(struct pci_dev *pdev)
-{
- u16 status;
- int pri;
-
- if (pdev->is_virtfn)
- pdev = pci_physfn(pdev);
-
- pri = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
- if (!pri)
- return 0;
-
- pci_read_config_word(pdev, pri + PCI_PRI_STATUS, &status);
-
- if (status & PCI_PRI_STATUS_PASID)
- return 1;
-
- return 0;
-}
-EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required);
-
#define PASID_NUMBER_SHIFT 8
#define PASID_NUMBER_MASK (0x1f << PASID_NUMBER_SHIFT)
/**
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 3f6947ee3324..ae84d28ba03a 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -456,6 +456,18 @@ static inline void pci_ats_init(struct pci_dev *d) { }
static inline void pci_restore_ats_state(struct pci_dev *dev) { }
#endif /* CONFIG_PCI_ATS */
+#ifdef CONFIG_PCI_PRI
+void pci_pri_init(struct pci_dev *dev);
+#else
+static inline void pci_pri_init(struct pci_dev *dev) { }
+#endif
+
+#ifdef CONFIG_PCI_PASID
+void pci_pasid_init(struct pci_dev *dev);
+#else
+static inline void pci_pasid_init(struct pci_dev *dev) { }
+#endif
+
#ifdef CONFIG_PCI_IOV
int pci_iov_init(struct pci_dev *dev);
void pci_iov_release(struct pci_dev *dev);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 3d5271a7a849..df2b77866f3b 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2324,6 +2324,12 @@ static void pci_init_capabilities(struct pci_dev *dev)
/* Address Translation Services */
pci_ats_init(dev);
+ /* Page Request Interface */
+ pci_pri_init(dev);
+
+ /* Process Address Space ID */
+ pci_pasid_init(dev);
+
/* Enable ACS P2P upstream forwarding */
pci_enable_acs(dev);
diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
index 1ebb88e7c184..a7a2b3d94fcc 100644
--- a/include/linux/pci-ats.h
+++ b/include/linux/pci-ats.h
@@ -10,6 +10,7 @@ int pci_enable_pri(struct pci_dev *pdev, u32 reqs);
void pci_disable_pri(struct pci_dev *pdev);
void pci_restore_pri_state(struct pci_dev *pdev);
int pci_reset_pri(struct pci_dev *pdev);
+int pci_prg_resp_pasid_required(struct pci_dev *pdev);
#else /* CONFIG_PCI_PRI */
@@ -31,6 +32,10 @@ static inline int pci_reset_pri(struct pci_dev *pdev)
return -ENODEV;
}
+static inline int pci_prg_resp_pasid_required(struct pci_dev *pdev)
+{
+ return 0;
+}
#endif /* CONFIG_PCI_PRI */
#ifdef CONFIG_PCI_PASID
@@ -40,7 +45,6 @@ void pci_disable_pasid(struct pci_dev *pdev);
void pci_restore_pasid_state(struct pci_dev *pdev);
int pci_pasid_features(struct pci_dev *pdev);
int pci_max_pasids(struct pci_dev *pdev);
-int pci_prg_resp_pasid_required(struct pci_dev *pdev);
#else /* CONFIG_PCI_PASID */
@@ -66,11 +70,6 @@ static inline int pci_max_pasids(struct pci_dev *pdev)
{
return -EINVAL;
}
-
-static inline int pci_prg_resp_pasid_required(struct pci_dev *pdev)
-{
- return 0;
-}
#endif /* CONFIG_PCI_PASID */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 6542100bd2dd..64d35e730fab 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -456,6 +456,7 @@ struct pci_dev {
#ifdef CONFIG_PCI_PRI
u16 pri_cap; /* PRI Capability offset */
u32 pri_reqs_alloc; /* Number of PRI requests allocated */
+ unsigned int pasid_required:1; /* PRG Response PASID Required */
#endif
#ifdef CONFIG_PCI_PASID
u16 pasid_cap; /* PASID Capability offset */

--
Sathyanarayanan Kuppuswamy
Linux kernel developer

Next message: Bjorn Helgaas: "[PATCH 0/3] PCI/ATS: Clean up unnecessary stubs and exports"
Previous message: Paolo Bonzini: "Re: [PATCH AUTOSEL 5.3 28/68] KVM: x86: Expose XSAVEERPTR to the guest"
In reply to: Bjorn Helgaas: "Re: [PATCH v1 1/1] PCI/ATS: Optimize pci_prg_resp_pasid_required() function"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]