| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20191009223056.GA139157@google.com>
Date: Wed, 9 Oct 2019 17:30:56 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: sathyanarayanan.kuppuswamy@...ux.intel.com
Cc: linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
ashok.raj@...el.com, keith.busch@...el.com
Subject: Re: [PATCH v1 1/1] PCI/ATS: Optimize pci_prg_resp_pasid_required()
function
On Mon, Oct 07, 2019 at 04:32:42PM -0700, sathyanarayanan.kuppuswamy@...ux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
>
> 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@...ux.intel.com>
> Cc: Ashok Raj <ashok.raj@...el.com>
> Cc: Keith Busch <keith.busch@...el.com>
> ---
> 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.
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. 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.
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 */
Powered by blists - more mailing lists