diff mbox series

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

Message ID f594928de550e151d3537fdd64099de34ffa30da.1570490792.git.sathyanarayanan.kuppuswamy@linux.intel.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show
Series [v1,1/1] PCI/ATS: Optimize pci_prg_resp_pasid_required() function | expand

Commit Message

Kuppuswamy Sathyanarayanan Oct. 7, 2019, 11:32 p.m. UTC 
From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.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@linux.intel.com>
Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/ats.c   | 50 ++++++++++++++++++++++++---------------------
 include/linux/pci.h |  1 +
 2 files changed, 28 insertions(+), 23 deletions(-)

Comments

Bjorn Helgaas Oct. 9, 2019, 10:30 p.m. UTC | #1 
On Mon, Oct 07, 2019 at 04:32:42PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.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@linux.intel.com>
> Cc: Ashok Raj <ashok.raj@intel.com>
> Cc: Keith Busch <keith.busch@intel.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 */
Kuppuswamy Sathyanarayanan Oct. 9, 2019, 10:51 p.m. UTC | #2 
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@linux.intel.com wrote:
>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.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@linux.intel.com>
>> Cc: Ashok Raj <ashok.raj@intel.com>
>> Cc: Keith Busch <keith.busch@intel.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.

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 */
>
diff mbox series

Patch

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
+}
+
 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 */